Use externalauth::register event vs hook_user_presave

@iStryker nice patch!
I tested #2 a little and everything appears to work as excpected, new ExternalAuthEvents::REGISTER is really great to have.
There is only one thing which makes me confused a little, in SAML Authentication admin form, I can leave following options empty:
- Synchronize email address on every login
- User name attribute
This should allow me to synchronize users without email addresses, yet, within your onUserRegister() method,
I can see that email attribute is validated, so when I leave the configuration empty, I am getting "Email address is not provided in SAML attribute." error.
Shouldn't that part follow the module configuration?

As I understand it: you always need a name AND an email to register, the admin options are only used on login synchronisation (managed by UserSyncEventSubscriber::onUserSync).

So far this patch works fine, it could be merged and closed.

@roderik

Hi,

This patch is running fine for around 7 month on production, I think it could be marked as RTBC ?

Is this issue is fixed on the 3.x branch ?

I hope this ends up in the 3.x beta release.

I am unfortunately not ready to answer this fully. But since there's a new release now and the issue queue is cleaned up, I'll give a sign of life.

This happens after the user is created. But is this a problem?

IMHO this is a potential problem, yes. If you first save the user, then let an event run and save the user again... that means any exception thrown in a custom event handler will leave you with a user account containing bogus information. This is the reason I have done this in such an ugly way: I want to prevent data corruption. This module should not be allowed to silently save corrupt user data in a way that site administrators may not even see for months/years. (IMHO this is even more relevant given the user base of this module, which includes large multi-site installations built by people who just install the samlauth module without really knowing what it does / without being responsible for any of the IdP/SP/SAML configuration.)

This is making it difficult to map new attributes/roles with a custom module that extends samlauth.

I was going to ask for more details on this. But as of November 2019, I am halfway a couple of interconnected issues:

• I acknowledge we should do something about the prefix and started to read code, to decide how to do that
• I remembered how I came to the current ugly construction of the code in 2016, and that I had the feeling there was something that I felt was 'off' about the events fired by the externalauth module. I didn't like what I needed to do, but it was the only way without introducing the potential for data corruption.
• I discovered something strange in the externalauth module. Not dangerous, but strange. And I was thinking about it, in the hope to get to a conclusion about what I think should be done with it. (I don't remember the specifics but I believe that there still is a piece of code / event somewhere in there, that supposedly is using the provider prefix. Even though the provider prefix is supposed to be removed from the code. This means... something weird is going on / should be fixed / we have dead code.)

I was basically at a point where I was trying to figure out if I have a reason to propose an externalauth 2.x branch with new events that other supported modules would clearly benefit from. That needs a good writeup.

...but that's still pending. After almost two weeks, the samlauth issue queue in a state of "all over the place" anymore, but... I'll need to switch to some other stuff first. Sorry.

Hmmmm I made a version which applies cleanly to the 3.x branch. I'm still a little unclear on if it's necessary or not now though?

Hmmm let's try this one instead... I must have lost my place.

Identical to https://www.drupal.org/project/samlauth/issues/2925171#comment-13756330 ... whitespace fail.

In followup to my previous comment: I've made some progress in deciphering the externalauth module. The "strange thing" I saw was already fixed (#3104478: Clean up linkExistingAccount method so it doesn't needless prefix username with provider name), by the way. But I've managed to wrap my head around what I'd like to do, and proposed work on the module in #3179148: Redo authmap_alter / register events..

I've worked out and manually tested the changes to the samlauth module that I want to do, and am uploading the patch here. (So this patch is not meant to be used with the current v1 externalauth module!) It removes the Samluth::USER_SYNC event because it now uses the native Externalauth::USER_SYNC event.

In doing so, I've decided that if samlauth doesn't decide to go in this direction, I'll be putting this forked ExternalAuth class into this module -- because it reduces the code complexity / ugliness of the other samlauth code, without giving up on "not wanting to save the user account until after the event has been dispatched".

----

@iStryker and others:

This issue seems to cover several different issues that can IMHO be separated. Unfortunately I did not see until now, that they are separate - because I was confused by the ugliness of the first group of issues, which is

• The hook_user_presave() is ugly and hard to create tests for
• Any mention of the provider prefix should disappear from this module

I want to solve this by doing the above.

Then, separaately, there's the following assertions:

During a the hook_user_presave(), not all of the information is available (...) This is making it difficult to map new attributes/roles with a custom module that extends samlauth.

I do not see which information is unavailable, besides the account ID (UID). So I also don't see how it is currently difficult to map new attributes/roles, and I don't see why introducing a second event is necessary.

If you need the account ID (UID) for something, I can be convinced by an example. But I so far (in #2900968: Attribute mapping module that is open since August 2017, and its duplicate #2817001: Map roles based on attribute from October 2016) I have not had a need for it. It's unfortunate that these open attribute/role mapping issues were not solved earlier, because then you'd have had an example already.

I'm looking at the code example for role assignment in #3, and that can be simplified. You don't need to know whether the account is already saved, so you don't need two separate methods assignRolesNew() and assignRolesSync(), if you call $event->markAccountChanged() instead of $account->save(). (This also decreases the necessary save operations if there are multiple event subscribers.)

(This might be an answer to your question "I was wondering why we are not using ExternalAuth register event?" -- the answer is, I've explicitly created a solution where an event subscriber could use the same code path, regardless whether we're calling ExternalAuth::register() or ExternalAuth::login(). So you don't need to implement two different methods that do almost the same.)

The result would be (untested):

namespace Drupal\my_module\EventSubscriber;

use Drupal\samlauth\Event\SamlauthEvents;
use Drupal\samlauth\Event\SamlauthUserEvent;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Drupal\user\UserInterface;

class MyModuleSubscriber implements EventSubscriberInterface {

  /**
   * {@inheritdoc}
   */
  public static function getSubscribedEvents() {
    $events[SamlauthEvents::USER_SYNC][] = ['assignRolesSync'];
    return $events;
  }

  /**
   * Assign roles to users.
   *
   * @param \Drupal\samlauth\Event\SamlauthUserEvent $event
   *   The event.
   */
  public function assignRolesSync(SamlauthUserEvent $event) {
    $account = $event->getAccount();
    $attributes = $event->getAttributes();
    $save = $this->assignRoles($account, $attributes);
    if ($save) {
      $event->markAccountChanged();
    }
  }

  /**
   * Helper function to assign the roles to the user.
   *
   * @param \Drupal\user\UserInterface $account
   *   The user logging in.
   * @param array $attributes
   *   Attributes of the user from assertion.
   *
   * @return bool
   *    If the account was changed.
   */
  private function assignRoles(UserInterface $account, array $attributes) {
    $save = FALSE;
    // sample code of assigning a role could look like.
    $group_id = 'urn:oid:1.3.6.1.4.1.198.2.1.10';
    if (array_key_exists($group_id, $attributes)) {
      $groups = $attributes[$group_id];
      $in_group = "CN=X, OU=Y...."
      if (in_array($in_group, $groups) {
        $account->addRole('X');
        $save = TRUE;
      }
    }

    return $save;
  }

.
Note that the impending change to different 'base' code won't influence the above example much. If my plan for an externalauth module v2 goes through and I remove the SamlauthEvents::USER_SYNC as a result.. the only change necessary is to change SamlauthEvents::USER_SYNC to ExternalAuthEvents::USER_SYNC.

An FYI for the people who came here looking for attribute/role mapping (and assuming the assertion "[...] is making it difficult to map new attributes/roles" is true), rather than for the issue title / the rest of the issue description:

Submodules for attribute and role mapping are now committed, as per #2900968: Attribute mapping module / #3070028: Role assignment.

The code in this issue wasn't used. This issue is still open for the title / some of the description: making sure we get rid of hook_user_presave() which I only want to do on a new major version of externalauth and/or a forked ExternalAuth service.

We are loading SAML IDP settings conditionally which may result in an invalid IDP settings. The patch adds a flag to prevent unnecessary execution of the user_presave hook and can be used as intermediate solution.

Update on what is blocking this: my plan in #3179148: Redo authmap_alter / register events. to untangle the ExternalAuth service has evolved into a new plan in #3361544: Datahandler plugins, (optionally) mappable from login providers: to extend the ExternalAuth service with plugins that can do much more.

Considerations around one vs the other plan are bigger than this issue, so I'll likely keep them in #2882568: Plan for SAML Authentication 4.x.

Patch in #16 worked around the issue I was encountering: inability to create a user (without SAML) in a non-production environment that does not have the keys required by SAML.

#16 is not related to the rest of this issue, so I overlooked it and its reasoning.

A similar thing has been committed in #3414466: Flag for disabling locally.