Authentication plugins and HTTP authentication

Add UI sketches.

Before having this plugin system. A dependency on the basic_auth module should be added.

Hello,

Thanks for your help.

I have updated the issue summary with the TODO list.

So if you want you can provide a patch for this issue.

Hello,

@FatherShawn, I am ok with that if a fallback is provided to not have hard dependency.

Thanks for the suggestion, nice idea :)!

I think with core/lib/Drupal/Core/EventSubscriber/AuthenticationSubscriber.php::onKernelRequestFilterProvider(), it raises exception if the authentication method is not supported.

I think only with that it will be ok.

Ok, no problem for the confusion.

The plugin system will be in Entity share, for the client interface, yes the form you wrote about in comment 16.

This plugin system will have plugins like:

• basic http
• cookie
• oauth

Similar to the authentication providers but a little bit different, for Entity share usage. And they will be in entity_share_client to allow the RemoteManager to work with.

Hi FatherShawn!

Huge thanks for this patch!

I have not tested it yet, but it looks promising. Below is my quick review. Please do not rework your patch yet, I making a lot of changes on 8.x-3.x with #3009221: Add an entity share client settings form.

1. +++ b/modules/entity_share_client/config/schema/entity_share_client.schema.yml
   @@ -0,0 +1,38 @@
   +entity_share_client.remote.*:
   

   Why changing the file name?

   I prefer the old one.

   I use module_name.schema.yml for simple config.

2. +++ b/modules/entity_share_client/entity_share_client.services.yml
   @@ -60,3 +59,13 @@ services:
   +      - [setKeyRepository, ['@?key.repository']]
   

   Nice! To avoid the dependency on the key module.

3. +++ b/modules/entity_share_client/src/Entity/Remote.php
   @@ -97,4 +90,41 @@ class Remote extends ConfigEntityBase implements RemoteInterface {
   +      // DI not available in entities:
   

   Yes. That's why we can create a dedicated service for data manipulation. Or putting it in the remote manager service later.

4. +++ b/modules/entity_share_client/src/Entity/RemoteInterface.php
   @@ -5,11 +5,40 @@ declare(strict_types = 1);
   +   *   Is this client for JSON operations?
   

   Thanks this gave me the idea on how I refactored the Remote Manager service on #3009221-13: Add an entity share client settings form

5. +++ b/modules/entity_share_client/src/Form/RemoteForm.php
   @@ -79,6 +90,22 @@ class RemoteForm extends EntityForm {
   +    $selectedPlugin->validateConfigurationForm($form['auth']['data'], $subformState);
   

   Add a check if the plugin instanceof PluginFormInterface. As done on #3009221-13: Add an entity share client settings form.

Hi FatherShawn!

Now that #3009221-17: Add an entity share client settings form had been merged, if you have time, you can take a look at the new code organization.

I also have updated #3082611: Entity share 3.x to order the issues.

This issue can be worked on without needing the alpha blockers.

If you don't have time, I will take it when alpha blockers will be done and if I will still have the time.

Regards, :)

Hi @FatherShawn!

Glad to see your comment and the reaction time! :)

Patch from comment 24 does not apply on 8.x-3.x. I tried to update it, but the remoteManager changes bugged me. And I didn't understand any more all the plugins.

So I moved back to 8.x-2.x where it applied and tested it. It works very well. Thanks!

(Except if the authentication is wrong, you have a fatal error on the pull form instead of just no data)

I attached a screenshot. Because when seing the UI changes, it was now clear why there is the complexity of the credential_provider on top of the complexity of the new plugin type + optional integration with the Key module...

Wow... Such a complex patch. I feel like 2 or 3 issues are handled at the same time...

I see that in case the Key module is not available, now you store username + password in a state and no more in config.

I understand that config has drawbacks:

• password is displayed in clear in the config
• config is expected to be the same when deploying a project accross environments, and a login/password is not expected to be the same accross environments

But also config has some benefits:

• It is overridable in settings.php, with environment variables for example
• It is overridable using additional modules like config split

And also to have tested the separately the Key module, I saw that it has a feature of config override in the UI. But once done it is no more editable... And it is also very nice to see that the Key module supports environment variables.

So I am very confused because I can't merge patch from comment 24 (outside technical reason of it no more applying on 8.x-3.x). And I don't want to reject a such big amount of work.

I hesitate a lot, because, I would like to reduce the complexity of the patch, aka avoiding to have a double storage system (local or key) to implement for each authentication plugins, by forcing the usage of the Key module.

If people use the Key module using keys in config storage it will be the same level of security as storing login/password in the remote config. Only 1 or 2 additional steps of configuration. With the appropriate documentation I don't find it very difficult.

And at the same time, the decision to force people using the Key module is hard. But it would also be the best moment because there are still no tagged version for the 8.x-3.x branch.

The problem I also have is: Will every authentication plugin store sensitive info that will require the Key module?

As you can see, I have a lot of questions and hesitation in my mind right now.

Regards,

Hi,

Thanks a lot @FatherShawn for your reply.

so I'd like to keep it against 2.x

No problem. And even if not merged in the 8.x-2.x, I would like to give you credit like I did for another contributor in #3009258-18: Allow skip synchronization for already synced entities.

but I'm happy to refactor it for 3.x going forward.

That would be amazing! Thanks.

My perspective is that confidential information should never be committed to version control.

I share your opinion. The paradox is that I have never been involved on a project using Entity Share... Only preparing features and stuff for teams using it. So I may miss some practical problems.

That's why I am more and more in favor of having a hard dependency on the Key module and simplify the patch.

I will try or ask one of my colleagues to write a patch with a second authentication method like Oauth to have a better idea of what will be required. And a better diff/view of the Key module support implication.

Not sure when I will have time for that.

We will see.

Thanks again!

Hi @FatherShawn,

If you want/can post it. Sure no problem, even if it needs to be reworked or not. That will still help!

Hello @FatherShawn,

your team was on holiday

???

As you want, @yarik.lusiuk is currently trying to implement an authentication plugin on Oauth on 3.x.

If you can provide a patch for 3.x great. If only for 2.x because it is the version you are using, no problem about that.

Regards,

Hello,

@FatherShawn, I have not tested Oauth modules yet, and planned to do at least when testing patches for this issue.

If we can avoid a dependency on a module, I would prefer it.

About the Key module, I think we will enforce this dependency for security reasons, do not rewrite your patches to enforce it. We will see that later.

Thanks all for the efforts on this issue.

Regards,

Hello everyone!

As the patch is progressing, maybe it is too late.

But so you think that creating sub issues to merge easily and progressively will be usefull? Also this would require more work...

I am thinking of the following steps:

1. Create the authentication plugin with two plugins, basic auth and anonymous (#3096716: Allow Clients to connect to Server anonymously is merged now)
2. Create an oauth authentication plugin
3. Have configuration options, outside of the plugins (but somehow linked with the basic auth as it overlaps when making request), to be able to request a server behind an HTTP Password protection (maybe the shield module will be helpful for tests on that)
4. Adapt authentication plugins to use the Key module and force its usage

Can you all please share your opinions?

And thanks all again for your work. This will be a huge milestone.

Hello,

Also, do you think this will help that I ask pull request to be enabled on this project?

Thanks @FatherShawn for the feedback.

I asked to enable the feature in #3152637-100: Opt-in to the Drupal.org Issue Forks and Merge Requests beta

I need to dive deep again into the patch to see what is going on.

Maybe a call with @FatherShawn, @yarik.lutsiuk, @ivan.vujovic and myself will help to clarify the situation and to easily organize us?

Hello,

The fork and pull request feature is now enabled.

Hello,

"Playing"/testing the new fork/MR feature.

Now I am going to manually test the patch.

Sorry for the delay.

I saw that the Key module integration is still optional. I still hesitate to make it a hard requirement seing the amount of work already done in the issue...

I haven't tested the Oauth plugin yet, because of the setup I am not used to.

And still nice work :)

Hello,

Thanks @FatherShawn for the comments.

Gitlab comments are super cool. Easier to mention someone on it.

We will test with @yarik.lutsiuk, how updating the MR will be.

When it will be ready, I will do commit the old fashion way to be sure to control comment and credit attribution.

We will test merging on smaller issues.

Hello,

@FatherShawn, thanks for the proposition, but @yarik.lutsiuk is also working on it. Maybe we should avoid telescoping each others. I don't know exactly how drupal.org MR works, but if I can give you write access on the fork for this issue no problem.

@yarik.lutsiuk, thanks for the updated patch. I will review it.

Changing to needs review to trigger automated tests.

Here is my review. I have not tested it yet.

One part comes from comment on the interdiff. The other one on the patch itself.

1. +++ b/modules/entity_share_client/src/Annotation/ClientAuthorization.php
   @@ -1,5 +1,7 @@
   +declare(strict_types = 1);
   
   +++ b/modules/entity_share_client/src/Plugin/ClientAuthorization/Anonymous.php
   @@ -0,0 +1,63 @@
   +
   

   declare(strict_types = 1);

   I will make a check before merging. No more lose time with that.

2. +++ b/modules/entity_share_client/src/Plugin/ClientAuthorization/Anonymous.php
   @@ -0,0 +1,63 @@
   +  public function validateConfigurationForm(array &$form, FormStateInterface $form_state) {}
   

   We can remove it if it is empty.

3. +++ b/modules/entity_share_client/src/Plugin/ClientAuthorization/Oauth.php
   @@ -110,13 +110,13 @@ class Oauth extends ClientAuthorizationBase {
   +    return '';
   

   Need to update the PHPDoc because the method no more throws exception.

1. +++ b/modules/entity_share_client/config/schema/remote.schema.yml
   @@ -11,9 +11,25 @@ entity_share_client.remote.*:
   +  type: sequence
   

   This will need to be completed I think. And need one for anonymous and oauth.

2. +++ b/modules/entity_share_client/entity_share_client.install
   @@ -40,3 +41,34 @@ function entity_share_client_update_8302() {
   +    $plugin = $manager->createInstance('basic_auth');
   

   I think this needs to be updated. As now it can be an anonymous connection.

3. +++ b/modules/entity_share_client/src/Plugin/KeyType/EntityShareOauth.php
   @@ -0,0 +1,31 @@
   + *   id = "entity_share_oauth",
   

   Maybe this also needs an entry in .schema.yml file.

   I don't know.

4. +++ b/modules/entity_share_client/src/Service/KeyProvider.php
   @@ -0,0 +1,94 @@
   +   * @see maw_luminate.services.yml
   

   ?

The only important part is to update the hook_update_N for anonymous connections. The rest of the stuff I can do it or it can be done in sub-issues.

@yarik.lutsiuk, almost done!!! :)

PS: Ok @FatherShawn, thanks :)

Thanks for the confirmation @FatherShawn!

Yep I know Config inspector, it is a development module on all my projects :). So helpful :)

Thanks @yarik.lutsiuk for your commits/patches!

And for checking all the points.

2: in this case a comment should be added to ensure it is done awarely.
1bis: Ok. I opened some sub-issues in the meantime with one dedicated on checking that. I will take it when this issue will be merged. Maybe nothing has to be done. Just checking.
2: As #3096716: Allow Clients to connect to Server anonymously had been merged, I agree that 99% websites have basic auth connections, but some may have started using anonymous connection.

Hello,

Thanks @yarik.lutsiuk for the updated commit.

It is ok for me.

I have done a last review locally.

This is the last things I changed:

• Fixing some typos and coding standards
• Move Authorization plugins base/manager/interface into a dedicated folder to uniformize like the import processors
• Renamed the authorization plugin manager service machine name

Triggering the automated tests.

If its is green. Then it is merging time!!!

And this is merged now!!! \o/

Thanks everyone!!! Now the sub-issues!