Problem/Motivation

When an event subscriber, listening to CasHelper::EVENT_PRE_REGISTER, is altering the Drupal username, the external authname is also altered and this is wrong because will lead to a wrong entry in the {authmap} table.

We hit this in #3070744: Exception thrown when registering an account with the same username when trying to avoid a name collision with user_accounts.auto_register set to TRUE. In the CasPreRegisterEvent documentation is stated:

Subscribers to this event can:
- ...
- Change the username that will be assigned to the Drupal account. By default it is the same as the CAS username.
- ...

We tried to add subscriber to CasHelper::EVENT_PRE_REGISTER that checks for an existing local account with that name. If is found, we create a unique name by adding a hash as suffix. But this leads a wrong entry in the {authmap} table.

The attached "test only" patch is proving the bug.

Proposed resolution

  • Add the local Drupal username as third parameter to CasUserManager::register().
  • Make it optional but trigger a deprecation error if is missed, to ensure backward compatibility for any 3rd party code that might call it directly.
  • Set the local Drupal username as 'name' in the $property_values array, passed to external auth register() method. In this way this value will take precedence when registering the new user.

Remaining tasks

None.

User interface changes

None.

API changes

New parameter for CasUserManager::register() method.

Data model changes

None.

Release notes snippet

N/A

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

claudiu.cristea created an issue. See original summary.

claudiu.cristea’s picture

Priority: Normal » Major
Issue summary: View changes
FileSize
3.38 KB
4.4 KB

I think this is a major issue as it breaks a module API feature: the possibility to alter the Drupal username before the user is registered. I attached the fix.

claudiu.cristea’s picture

Issue summary: View changes
claudiu.cristea’s picture

Assigned: claudiu.cristea » Unassigned
bkosborne’s picture

Can you review #3015997: The authmap username mapping should not be configurable and see if this if it's a duplicate of that? I think I discovered this problem a while ago when reviewing code. I'll take a closer look hopefully soon.

claudiu.cristea’s picture

@bkosborne, I reviewed #3015997: The authmap username mapping should not be configurable and I left a comment there. However, that is about altering the CAS username (authname, external username). This ticket is about altering the local Drupal username. They are not related. The other is about EVENT_PRE_USER_LOAD event, while this is about EVENT_PRE_REGISTER event. The test from this patch is self-explanatory.

But there's a loose relation with #2798323: Make provider prefix to username optional. If that is fixed we can cleanup more, by dropping ExternalAuthSubscriber. I came there with a solution that keeps the actual behaviour (i.e. add the provider as prefix) but allows modules as CAS to skip that without needing a subscriber. Could you review that one?

bkosborne’s picture

Status: Needs review » Needs work

Ok, yes, I see now they are distinct issues. This issue was a good catch!

  1. +++ b/src/Service/CasUserManager.php
    @@ -124,15 +124,22 @@ class CasUserManager {
    +  public function register($authname, array $property_values = [], $local_username = NULL) {
    +    if (!$local_username) {
    +      @trigger_error('Calling CasUserManager::register() without the $local_username argument is deprecated in cas:8.x-1.6 and the $local_username argument will be required in cas:8.x-1.10.', E_USER_DEPRECATED);
    +      $local_username = $authname;
    +    }
    

    I don't think I would deprecate anything in 1.10, but instead a 2.x version.

  2. +++ b/src/Service/CasUserManager.php
    @@ -124,15 +124,22 @@ class CasUserManager {
         try {
    +      $property_values['name'] = $local_username;
           $property_values['pass'] = $this->randomPassword();
           $user = $this->externalAuth->register($authname, $this->provider, $property_values);
         }
    

    Let's take these two variable assignments out of the try block

Beyond that, this is good to go.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
4.5 KB
1.17 KB

Thank you for review. Fixed #7.

bkosborne’s picture

Status: Needs review » Fixed

Committed, thank you!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.