Disallow login based on attributes

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.

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();

> 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?

(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.)

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".

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 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.)

> 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.

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.