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 authregister()
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
Comment | File | Size | Author |
---|---|---|---|
#8 | 3072508-8.patch | 4.5 KB | claudiu.cristea |
cas_alter_drupal_username.test-only.patch | 1.02 KB | claudiu.cristea | |
Comments
Comment #2
claudiu.cristeaI 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.
Comment #3
claudiu.cristeaComment #4
claudiu.cristeaComment #5
bkosborneCan 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.
Comment #6
claudiu.cristea@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 aboutEVENT_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?Comment #7
bkosborneOk, yes, I see now they are distinct issues. This issue was a good catch!
I don't think I would deprecate anything in 1.10, but instead a 2.x version.
Let's take these two variable assignments out of the try block
Beyond that, this is good to go.
Comment #8
claudiu.cristeaThank you for review. Fixed #7.
Comment #10
bkosborneCommitted, thank you!