Problem/Motivation

Currently the synchronization of username and password happening on every login via hook_user_login. There is a @todo mentioned, which says that this should be configurable.

Proposed resolution

- Add new config + possibility to change this in admin form
- Add new method to the manager class which handles the synchronization.
- Force sync while external register

Remaining tasks

- Provide patch.

CommentFileSizeAuthor
#7 2604456-7.patch6.43 KBsvendecabooter
#5 simplesamlphp_auth-make_username_and_mail_sync_configurable-2604456-5.patch6.33 KBeyilmaz
#2 simplesamlphp_auth-make_username_and_mail_sync_configurable-2604456-1.patch6.34 KBeyilmaz
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

eyilmaz created an issue. See original summary.

eyilmaz’s picture

eyilmaz
Primary language German
Location Berlin, Germany
CreditAttribution: eyilmaz at FFW commented
Assigned: eyilmaz » Unassigned
Status: Active » Needs review
FileSize
simplesamlphp_auth-make_username_and_mail_sync_configurable-2604456-1.patch6.34 KB

Here is the patch.

eyilmaz’s picture

eyilmaz
Primary language German
Location Berlin, Germany
CreditAttribution: eyilmaz at FFW commented
Issue summary: View changes
snufkin’s picture

snufkin CreditAttribution: snufkin commented
Status: Needs review » Needs work

Some changes went into the D8 branch, could you reroll the patch please?

eyilmaz’s picture

eyilmaz
Primary language German
Location Berlin, Germany
CreditAttribution: eyilmaz at FFW commented
FileSize
simplesamlphp_auth-make_username_and_mail_sync_configurable-2604456-5.patch6.33 KB

rerolled patch.

eyilmaz’s picture

eyilmaz
Primary language German
Location Berlin, Germany
CreditAttribution: eyilmaz at FFW commented
Status: Needs work » Needs review
svendecabooter’s picture

svendecabooter
Primary language Dutch
Location Gent
CreditAttribution: svendecabooter for Acquia commented
Status: Needs review » Reviewed & tested by the community
FileSize
2604456-7.patch6.43 KB

Nice improvement! The code looks good to me, and works as advertised in my tests, except for a few minor things (see below).
I updated the patch by eyilmaz slightly in the attached patch:

  • Fixed wrong call to get config values in synchronizeUserAttributes().
  • Checkboxes no longer nested in Form API array, because they didn't show up in the admin screen for me
  • Updated description text which had some spelling errors.

The call to synchronizeUserAttributes to force saving of user attributes in externalRegister() might need some improvement in a followup patch, since now we might be saving the user account 3 times upon registration:

  • Saving upon registration in externalRegister()
  • Saving upon forced user attribute synchronisation after registration (if a valid DefaultName exists)
  • Saving upon logging in after registration, when user attributes are synced again (if checkboxes are checked in settings).

I'll give some thought on how to improve that, but that seems to be out of scope of this issue, so will do that in a followup issue.

  • snufkin committed f63b648 on authored by eyilmaz 
    Issue #2604456 by eyilmaz, svendecabooter: make username and e-mail sync...
snufkin’s picture

snufkin CreditAttribution: snufkin commented
Status: Reviewed & tested by the community » Fixed

Fixed, thanks to both of you. I've put eylmaz as the commit author, as he proposed the initial big patch.

Status: Fixed » Closed (fixed)

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

malik.kotob’s picture

malik.kotob CreditAttribution: malik.kotob commented

Question for you all. What's the rationale behind forcing the sync for external registrations? We currently have a use case where the username in Drupal matches the authname from the IdP, and the existing account is linked with that. The email address, however, does not match the email address from the IdP, and we were hoping to keep it that way. Instead, it's being synced despite our config settings (because it's forced).