Log in directly with external IdP

Hi there. I took a look at this. When I went to apply the patch, it would not apply when I attempted it to apply it to the latest in the 8.x-3.x branch. I went ahead and re-rolled the patch and manually applied the changes in the appropriate places.

After doing this and testing it on my setup, it worked as advertised. I applied this to a stock version of Drupal 8.3.2, with an already working instance of SimpleSAMLphp working as my SP and using a Shibboleth IDP. The /user/login page automatically redirected to my IDP.

I'm attaching my patch and interdiff. I'm still rather new to the patching and interdiff process so please let me know if anything is off. The patch name is different as per step #11 on https://www.drupal.org/node/707484.

As a new patch is involved, I'm assuming another community member needs to confirm the patch worked so leaving this as 'Needs Review.'

We tested the patch in #4 for 8.x-3.0-rc2 and it works very well.

It can be applied directly to 8.x-3.x-dev. Can you please merge it? It is important functionality for us.

Thank you in advance.

There is a bug in the patch that if you access a website directly at ${site}/saml_login there is a redirection loop. Firefox, for instance, shows "The page isn’t redirecting properly: Firefox has detected that the server is redirecting the request for this address in a way that will never complete."

I'm working on a version that implements the EventSubscriberInterface instead.

This is quite an interesting development, I look forward to testing the event subscriber implementation.

Does this completely take over the local user login? What about Drush ULI?

I have a proposed solution.

Implementation

The concept is to directly redirect anonymous users that try to access the Drupal login page to the external identity provider.

    // Check if an anonymous user tries to access the Drupal login page.
    if (\Drupal::currentUser()->isAnonymous() && \Drupal::routeMatch()->getRouteName() == 'user.login') {

      // Get the path (default: '/saml_login') from the 'simplesamlphp_auth.saml_login' route.
      $saml_login_path = \Drupal::url('simplesamlphp_auth.saml_login');

      // Redirect directly to the external IdP.
      $response = new RedirectResponse($saml_login_path, RedirectResponse::HTTP_FOUND);
      $event->setResponse($response);
      $event->stopPropagation();

The sample code is generalized as possible and considers modified user or SAML paths. For instance, someone could rename /user/login to anything using a module such as Rename Admin Paths. So matching the RouteName is more robust than matching the path. Similarly, the SAML path could be changed, although this shouldn't be as common.

Integration to simplesamlphp_auth

Initially, I implemented this redirection in a separate module. But we strongly believe that this functionality would be useful to more people and should be integrated to the simplesamlphp_auth module.

The previous proposed patches define a new configuration in order to enable or disable this functionality. During the evaluation of this integration, I realized that this functionality can be based on the 'allow.default_login' configuration. The label states: 'Allow authentication with local Drupal accounts' and the description states: 'Check this box if you want to let people log in with local Drupal accounts (without using simpleSAMLphp).'

We believe it makes sense to completely skip the Drupal login page and directly redirect anonymous users to the external identity provider when the 'allow.default_login' option is not selected. Because even if a user accesses the Drupal login page he/she will never be able to login with a Drupal account. By skipping this page we also do not allow creating users or reseting passwords. But this is not a problem since any (new) user will not be able to login with neither a new nor an old password.

Proposed patch

Initially, I was going to create a new EventSubscriber but I noticed that ./src/EventSubscriber/SimplesamlSubscriber.php has generic documentation (i.e. 'Event subscriber subscribing to KernelEvents::REQUEST'). Since this patch is another event of the same KernelEvent, we can append it to that source file. That eliminates the need to create a new service. Also, if we had to create a new service we would need to make changes to that one as well to document why they differ.

I have attached the proposed patch.

Description changes

I mentioned this redirection in the description. I also removed the text about entering the Drupal user IDs to keep it simple. This was incomplete anyway because someone could ask why you mention the user ID option and not the ROLES option.

Drush ULI

Drush ULI works only when 'allow.default_login' is enabled. This will remain the case with these changes (thanks @dakku for the question).

Future improvements

At admin/config/people/simplesamlphp_auth/local, the following options
* 'Allow SAML users to set Drupal passwords'
* 'Which ROLES should be allowed to login with local accounts?'
* 'Which users should be allowed to login with local accounts?'
depend on
* 'Allow authentication with local Drupal accounts'
So it makes sence to hide/freeze them if the that one is not selected.

Any comment on that (dakku?)? Does someone prefer a separate menu option for this functionality?

Hi Romanos,
Thank you for the patch and summary. I havent had a chance to test out the functionality yet. But it seems like a great idea. Hopefully I will get a chance to review over the Christmas holidays. Have a fantastic time yourself.

If there is anyway to allow `drush uli` to work with the solution in the latest patch, we'd love to use this. Otherwise, we cannot use the patch as provided.

Perhaps something that checks whether it's a one-time login link, and allows that?

@timwood the latest patch does not affect the functionality of `drush uli` at all.

What I mentioned in #9 is that actually Drush ULI works only if 'Allow authentication with local Drupal accounts' is selected. This is a module option and irrelevant to this patch.

Just to mention a possible limitation of the latest patch:
When you disable the 'Allow authentication with local Drupal accounts' option, users are automatically redirected to saml when trying to log-in (this is the patch's main goal). Now, when you enable it back a cache-rebuild is required for the Drupal log-in page to reappear. Does anyone know a way to improve this? Would it be considered as a blocker bug?

@rwmanos. I understand what was explained in #9, but we actually want to use the convenience and enforcement of all-SAML logins which this patch provides (by unchecking "Allow authentication with local Drupal accounts"), EXCEPT for when using `drush uli`. Limiting drupal user #1's login to only those with access to run `drush uli` against each environment, provides stronger security control and separation of duties. Plus there is no need to share or even ever record user #1's password.

Based on your patch, I tried to find something to allow the one-time login's to work, but I couldn't figure out what to check against. Perhaps something like:

if (\Drupal::currentUser()->isAnonymous() && \Drupal::routeMatch()->getRouteName() == 'user.login' && !one-time-login) {

@timwood thanks for elaborating! The request is more clear now.

Still though, my point is that drush uli currently does not work when "Allow authentication with local Drupal accounts" is not selected, is it? So if this changes in the future we don't even know if this patch will affect it. We need to test it first. I believe this should be handled by another issue and not try to change two different things with a single patch.

Am I missing something?

@rwmanos
I get it now. I agree. Separate issue makes most sense for my request. Thanks!

Also, I reviewed the patch from #9 and it seems to work as advertised.

@rwmanos & @timwood, thanks guys. I will also do a quick test and aim to merge it in for the next release.

Committed, thank you all.