Problem/Motivation

As stated in #2856713-49: Authentication plugins and HTTP authentication, to be able to request a server behind an HTTP Password protection, we need to provide a couple username/password (maybe also with optional support on Key module) on the remote config entity.

And as regarding the basic auth authentication method, in practice, it is not possible to provide a different couple username/password for HTTP Password and basic auth, we need to take this into account by implementing a #state for example and overriding submitted values programatically, with priority to the basic auth plugin.

Maybe the Shield module will help for manual and automated tests.

CommentFileSizeAuthor
#16 interdiff-3167422-10-16.txt3.29 KBGrimreaper
#16 entity_share-http_pass_support-3167422-16.patch8.12 KBGrimreaper
#10 interdiff_9-10.txt3 KByarik.lutsiuk
#10 3167422-http-pass-support-10.patch7.38 KByarik.lutsiuk
#9 3167422-http-pass-support-9.patch4.37 KByarik.lutsiuk
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Grimreaper created an issue. See original summary.

yarik.lutsiuk’s picture

yarik.lutsiuk CreditAttribution: yarik.lutsiuk at Smile commented
Assigned: Unassigned » yarik.lutsiuk
yarik.lutsiuk’s picture

yarik.lutsiuk CreditAttribution: yarik.lutsiuk at Smile commented
Assigned: yarik.lutsiuk » Unassigned
yarik.lutsiuk’s picture

yarik.lutsiuk CreditAttribution: yarik.lutsiuk at Smile commented
Assigned: Unassigned » yarik.lutsiuk

Grimreaper credited osman_seferov.

Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper at Smile commented
Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper at Smile commented

Hello,

@yarik.lutsiuk, here is some context of the notification emails you may have received :)

@osman_seferov made an internal demonstration last week about a monitoring platform (I have added you as guest so you can have access to the code).

During the demonstration, there is a part that made me think of this issue. And we may inspire ourselves from this code to go beyond this issue's original scope. Especially the possibility to set a header name and value.

In the monitoring tool code. there is a field "http_auth_type" with 3 possible values:

static::PROJECT_HTTP_AUTH_TYPE_FREE => 'Free access',
static::PROJECT_HTTP_AUTH_TYPE_BASIC => 'Basic auth',
static::PROJECT_HTTP_AUTH_TYPE_HEADER => 'Header',

And that made me think of a new plugin type besides the authorization plugins. With 3 plugins: free, basic auth (this issue scope) and header.

In the current implementation this is not a plugin system but hidden fields depending on the value you select. And we need to see how to integrate that properly with Entity Share, because there is JSON:API HTTP client (getJsonApiClient()) and a HTTP client (getClient()) to prepare.

We can have a call if you have questions and/or want to discuss the pertinence of this idea.

Also in the case we switch to a plugin system, we may forget the "#states" depending on the authorization plugin selected, and only put a description saying that it can interfere.

Cheers,

Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper at Smile commented
Issue summary: View changes

Adding a link to the shield module in the issue summary for convenience.

yarik.lutsiuk’s picture

yarik.lutsiuk CreditAttribution: yarik.lutsiuk at Smile commented
FileSize
3167422-http-pass-support-9.patch4.37 KB

Hello,

added patch with header plugin, will add http password. WIP.

Cheers,

yarik.lutsiuk’s picture

yarik.lutsiuk CreditAttribution: yarik.lutsiuk at Smile commented
Status: Active » Needs review
FileSize
3167422-http-pass-support-10.patch7.38 KB
interdiff_9-10.txt3 KB

Added HTTP password protection only to anonymous user plugin,
because other plugins uses Authorization header too

Cheers,

yarik.lutsiuk’s picture

yarik.lutsiuk CreditAttribution: yarik.lutsiuk at Smile commented

also, after its will be merged, need to update
https://www.drupal.org/docs/contributed-modules/entity-share/authorizati...
and add new for Header plugin.

Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper at Smile commented
Assigned: yarik.lutsiuk » Grimreaper
Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper at Smile commented
Assigned: Grimreaper » yarik.lutsiuk
Status: Needs review » Needs work

Hello,

Thanks @yarik.lutsiuk for the patch. Great work!

And nice mentioning the documentation page to not forget to edit it :)

Finally this is much simpler as authentication methods already use the authentication header.

This is my review, as it is small stuff, I can do it before merging, if I can get your opinion that is ok :):

  1. +++ b/modules/entity_share_client/src/Plugin/ClientAuthorization/Header.php
@@ -0,0 +1,87 @@
+    // Basic Auth is a core module which any server can enable.

    I think this comment, can just be removed. 

  2. +++ b/modules/entity_share_client/src/Plugin/ClientAuthorization/Header.php
@@ -0,0 +1,87 @@
+        'Content-type' => 'application/vnd.api+json',

    Is it a copy/past mistake? The Content-type header is only for JSONApiClient. Or I may be missing something?

In addition to the review, about the changes in the Anonymous plugin. Finally it is equivalent to the Basic Auth plugin (If I am missing something, please correct me). So I think in the Anonymous plugin, we should only add a message saying that if the website is behind HTTP Password protection, the Basic Auth module should be used instead, and maybe even with that it is not guaranteed to work.

Cheers,

yarik.lutsiuk’s picture

yarik.lutsiuk CreditAttribution: yarik.lutsiuk at Smile commented

Hello,

1 and 2, yes my bad, can be removed.

Finally it is equivalent to the Basic Auth plugin (If I am missing something, please correct me)

Not fully, because we authenticate as user via 'login' form,

$http_client->post('/user/login', [
      'form_params' => [
        'name' => $credentials['username'],
        'pass' => $credentials['password'],
        'form_id' => 'user_login_form',
      ],
    ]);

and i'd added comment for Anon plugin credentials,
'Leave empty if Server website is not protected via HTTP Password.'

Cheers,

Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper at Smile commented
Assigned: yarik.lutsiuk » Grimreaper
Status: Needs work » Needs review
Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper at Smile commented
FileSize
entity_share-http_pass_support-3167422-16.patch8.12 KB
interdiff-3167422-10-16.txt3.29 KB

Discussed offline with @yarik.lutsiuk, added a restriction on the key type to be like Basic Auth on Anonymous HTTP Password support.

  • yarik.lutsiuk authored 936ab5d on 8.x-3.x 
    Issue #3167422 by yarik.lutsiuk, Grimreaper, osman_seferov: HTTP...
Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper at Smile commented
Assigned: Grimreaper » Unassigned
Status: Needs review » Fixed
Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper at Smile commented
Assigned: Unassigned » Grimreaper
Status: Fixed » Needs work

And I forgot to update the documentation...

Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper at Smile commented
Assigned: Grimreaper » Unassigned
Status: Needs work » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.