Problem/Motivation

In simplesamlphp_auth, there is a hook hook_simplesamlphp_auth_allow_login() to deny certain acounts, for example if they are missing certain data or roles.

I don't see a direct alternative for this in samlauth. I guess I could throw an exception during the user sync event, but if the attributes change for an existing user, it wouldn't prevent the login.

Maybe I'm missing something, if not then that could be considered a feature request.

Steps to reproduce

Proposed resolution

Issue fork samlauth-3509117

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

berdir created an issue. See original summary.

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
Status: Active » Postponed (maintainer needs more info)

I guess I could throw an exception during the user sync event, but if the attributes change for an existing user, it wouldn't prevent the login.

Not sure I'm interpreting this correctly.

  • The login is always prevented if you throw an exception.
  • What you can't do (well I guess you can, but not in a recommended way), is prevent the login but still update the Drupal user data with data from the IdP. I wouldn't expect anyone to want to do that... so... just asking for confirmation: is that really what you want to do? I never considered there would be a use case for that.
berdir’s picture

berdir
Primary language German
Location Switzerland
commented

My use case is that users have a certain attribute that is used to control what kind of access users have on the site (using something like group or og module or just roles). If they don't have that attribute, they must not be allowed to log in.

> The login is always prevented if you throw an exception.

I somehow assumed that the UserSync event would only run for new users, but it does always run of course. It does specifically for new users run quite late though, only when already saving the new user. That will also catch and rethrow the exception in the storage handler. This would also break the ability to display a specific message to the user. That really seems like a last resort thing.

simplesamlphp does it like this

        if (!$this->simplesaml->allowUserByAttribute()) {
          return [
            '#markup' => $this->t('You are not allowed to login via this service.'),
          ];
        }

  public function allowUserByAttribute() {
    $attributes = $this->getAttributes();
    $return = TRUE;
    $this->moduleHandler->invokeAllWith('simplesamlphp_auth_allow_login', function (callable $hook) use ($attributes, &$return) {
      // Once an implementation has returned false do not call any other
      // implementation.
      if ($return) {
        $return = $hook($attributes);
      }
    });
    return $return;
  }

In samlauth, this would then run after $this->processLoginResponse();

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented

> I somehow assumed that the UserSync event would only run for new users

OK, that's cleared up then.

> It does specifically for new users run quite late though, only when already saving the new user. That will also catch and rethrow the exception in the storage handler. This would also break the ability to display a specific message to the user.

Well yeah, interpreting the SAML message and saving the new user is basically the only thing it does. I never considered that "late". (I guess I never cared about the catch + rethrow you mention? What mattered to me only is that it gets caught again in the controller. Also: apologies for the code spaghetti.)

For displaying a message to the user when login (and/or creation / sync) fails, you can throw a Drupal\samlauth\UserVisibleException. Does that help?

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented

(Aaaaaaaactually re. "done during saving":

There are complications. The gist is: I would like to do separate the user sync out from the eventual user_save(), but that requires a rewrite of the externalauth module if you never want to run the risk of ending up with a saved user with half-incorrect data.

I think. It's been a while since I looked at the details.)

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

I read up on the issues around user_user_presave(), awkward, but I understand.

> For displaying a message to the user when login (and/or creation / sync) fails, you can throw a Drupal\samlauth\UserVisibleException. Does that help?

But because of how that works, this doesn't work in that case. See \Drupal\Core\Entity\Sql\SqlContentEntityStorage::save(), it catches all exceptions and wraps them. Arguably this should probably go. It was done a very long time ago and it still plays very badly with our general exception handling (logs then don't contain the actual place where the exception happened). But it's there.

This means that your module won't see a UserVisibleException, it will see an EntityStorageException.

> Well yeah, interpreting the SAML message and saving the new user is basically the only thing it does. I never considered that "late"

I think making a decision to create a new user, handing it of to external_auth, and then bailing out in the middle of the entity storage operation of saving that user is "late".

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
Status: Postponed (maintainer needs more info) » Active

Thanks for the re-explanation, I'm not always fast picking everything up :)

Regardless whether this solution would be suitable for your needs: the fact that a UserVisibleException thrown in a USER_SYNC event subscriber isn't recognized, is a bug that should be fixed. (And noone's reported it so far + I somehow failed to spot it + there are no tests for this.)

I "plan to soon" test and extend SamlController::handleExceptionInRenderContext() to also recognize the wrapped EntityStorageException, then. (and/or any exception that wraps a UserVisibleException...)

That's arguably a hack to counter a hacky situation (the fact that the event is dispatched during hook_user_presave). But it is a recognizable specific hack without further issues. And for the moment I'm still sticking to the hacky situation and living in hope I'll tweak externalauth someday.

(Because - better summary: otherwise I'd need to give up on either dispatching the same event for new and re-logging-in users, or having protection against saving half-constructed user entities with bogus data.)

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

> Regardless whether this solution would be suitable for your needs: the fact that a UserVisibleException thrown in a USER_SYNC event subscriber isn't recognized, is a bug that should be fixed. (And noone's reported it so far + I somehow failed to spot it + there are no tests for this.)

True, but I feel like that would be better resolved in a separate issue. Your call.

I do plan to propose a MR with an event for this, didn't get to that part yet.

Migrating from simplesamlphp_auth (yes, as the simplesamlphp_auth maintainer. It's just too much of a struggle with constant symfony dependency conflicts) to this as part of integrating a new IdP with our client. Seems to be working pretty well so far. There is one issue related to the whole user save trickery that I did run into. I'm synchronizing group memberships based on attributes, but that's only possible on a saved user, so I need to postpone that part until the user is actually saved. simplesamlphp_auth always saved the user first and then called the hook.

berdir opened merge request !29

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review

Created a proposal for the event, with a test. Possibly the event could also add way to set a message.

I also add a constant for the event name, it's standard these days to just use the class name, but wanted to match the existing pattern.

  • roderik committed 36f70423 on 8.x-3.x 
    Issue #3509117 by roderik, berdir: Fix propagation of...
roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented

Sorry, but I still either don't understand something or don't agree.

Trying to summarize the current state of things:

#8
>> the fact that a UserVisibleException [...] should be fixed.
> True, but I feel like that would be better resolved in a separate issue. Your call.
I fixed it in this issue, given 1) it isn't hard and 2) I doubt that more is needed. (See commit, if you care.)

#8
> I'm synchronizing group memberships based on attributes, but that's only possible on a saved user, so I need to postpone that part until the user is actually saved.

ExternalAuthEvents::LOGIN can be used for that (assuming you can do it after login). Yeah, I didn't document that properly in this module...

When I initially quickly read the above comment, I didn't respond immediately, because I mistakenly thought your planned patch would be related to that. Now I guess that's a separate issue.

I should have ideally responded earlier, because we're not on the same page yet re. the initial reported issue / your patch... Rewinding back to #3, now hopefully without misunderstandings:

> (...) That [using current USER_SYNC event] really seems like a last resort thing.

Is it, though?

With the committed fix, throwing a UserVisibleException during the USER_SYNC event is functionally nearly equivalent to the new event you're implementing - except the "way to set a message" already exists. They're dispatched at nearly identical times. The only things that are done between the new and existing hook (for new Drupal users only) are

  1. Starting an SQL transaction (without actually sending any SQL commands yet)
  2. Calling User::presave(), which contains some really non-problematic code
  3. invoking hook_user_presave()

I agree that the fact that a UserVisibleException cancels an already-active SQL transaction feels icky and ugly. But it doesn't actually do anything dangerous. I don't (yet?) see enough reason to add a new event, just because it feels icky.

Now that I look at things again: I could reorder the user_presave hook implementation so that it runs almost-guaranteed first.

In my view, the eventual solution is for externalauth to invoke an event before the user is saved, and then deprecate samlauth's own event(s). By now, that's part of a larger plan for rewriting the module's logic (and moving some generic stuff from samlauth into externalauth). Although I admit that my desire to set aside volunteer time for this large-ish rewrite isn't super realistic...

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
Status: Needs review » Postponed (maintainer needs more info)

Status change for "please protest now (against not implementing the proposed new event) or ..."

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

I don't really understand your feedback or the status you set.

We now use the proposed MR in production, it works well for us and I think is a far cleaner solution than throwing an exception as part of saving the user and having to deal with edge cases around that. Exceptions IMHO shouldn't be used for code flow/logic that isn't actually an "exception".

So, imho, needs review is the status I'd use here?

On the sync, in my case, I guess the LOGIN event would work as the I have the information I need available on the account as fields and roles, but in general, that event doesn't have access to the SAML attributes.

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
Status: Postponed (maintainer needs more info) » Postponed

To get "the sync" out of the way (which this issue is not about, so I guess we can close this off):

> On the sync, in my case, I guess the LOGIN event would work [...] that event doesn't have access to the SAML attributes.

OK. I've been holding off adding such an extra event, until someone gives me a practical case where they need the attributes AND truly need to have the user saved already.

---

> I don't really understand your feedback or the status you set.

TL/DR: I don't want to implement your patch and add a new event, for the reasons outlined / because IMHO the functionality fully overlaps with the existing event.

And I didn't want to set "won't fix" yet until you had an opportunity to respond.

> having to deal with edge cases around that. Exceptions IMHO shouldn't be used for code flow/logic that isn't actually an "exception".

1. The implementer of an event listener doesn't need to deal with edge cases. They currently have a single event to do all the things, which is IMHO simpler/clearer for them. They don't need to e.g. first do validation of attributes in one (your new) event, and then actually sync these attributes in another event.

2. "Exceptions IMHO shouldn't be used for code flow" is a new point (in this thread, for me). And I appreciate the argument.

But I'm still not planning to add the event to address that point. For a combination of these overlapping reasons: I find clarity on where/how to implement things more important than the alleged "far cleaner"-ness of the new event, for the remainder of v3.x. The new event doesn't actually add functionality. It's also not full functionality: there's no way to pass on a message right now. So, (and also because of above point 1) people will end up implementing a mix of the new event and USER_SYNC.

You're of course free to keep using your own patch for the remainder of samlauth v3.

--

In general, about this point 2 (if you care):

To address this point 'cleanly' implies deprecating UserVisibleException, AFAICT.

The world apparently agrees with you - judging by the fact that I have never seen things like UserVisibleExceptions implemented elsewhere.

But for situations (of which I've encountered several) where

  • code that needs to stop execution because of an error
  • and code that needs to decide what to do with the error (e.g. log or set message? Continue with some 'parent' processing or not?)

...are in completely different places, implemented by different modules... I so far haven't seen a good solution/implementation, that doesn't use exceptions.

The v4 rewrite that is happening in my daydreams** will move this logic into the externalauth module, with a different structure. I plan to ask you and some other people what you think about the structure before merging anything, and will consider above point 2.

** if everything goes perfectly, maybe this summer, but definitely don't hold your breath.