Reevaluate roles every time the user logs in: FALSE
Register users: FALSE

When users are added, hook_user_insert unconditionally forces the name and e-mail address of the user currently signed in through SSO onto the newly created user. This is problematic when an admin is adding a new user through /admin/people/create: in this case, the SSO user != the user being created.

CommentFileSizeAuthor
#6 simplesamlphp_auth-fixnewdrupalusers-1824194-6.patch674 bytesjeffam
#5 simplesamlphp_auth-fixnewdrupalusers-1824194-5.patch3.28 KBkenmc

Comments

colan’s picture

colan
he/him
Primary language English
Location Toronto 🇨🇦
commented
Version: 7.x-1.3 » 7.x-2.x-dev
Priority: Normal » Critical
Status: Needs work » Active

It's worse than that. If you've already got a user with that name, you get an integrity contraint violation:

PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '' for key 'name': UPDATE {users} SET name=:db_update_placeholder_0 WHERE (uid = :db_condition_placeholder_0) ; Array ( [:db_update_placeholder_0] => [:db_condition_placeholder_0] => 1599 ) in simplesamlphp_auth_user_insert() (line 378 of /path/to/drupal/sites/all/modules/simplesamlphp_auth/simplesamlphp_auth.module).

colan’s picture

colan
he/him
Primary language English
Location Toronto 🇨🇦
commented
Version: 7.x-2.x-dev » 7.x-2.0-alpha1

Logging against an actual release, now that there is one.

colan’s picture

colan
he/him
Primary language English
Location Toronto 🇨🇦
commented
Status: Active » Closed (duplicate)

Sorry, I mischaracterized the issue here. I think what the reporter is talking about would be handled by #1280930: Create authmap entries for pre-existing Drupal users, so marking this one as a duplicate.

There error I found above is really #1912262: Empty string in username attribute causes DB integrity constraint violation.

kenmc’s picture

kenmc commented
Version: 7.x-2.0-alpha1 » 7.x-2.0-alpha2
Status: Closed (duplicate) » Needs work

No, I don't think #1280930: Create authmap entries for pre-existing Drupal users has anything to do with it. Allow me to describe what happens:

- I sign into my website with my SSO account, which is authmapped to an administrator account on Drupal.
- I go to create a new user through /admin/people/create, fill out the form for the new user information, and click the submit button.
- The new account that is created has MY name as the username and MY e-mail address, not the values I filled out on the form. All of the other values seem fine, though.

From what I can tell, this is a problem in simplesamlphp_auth_user_insert(): it seems to be considerate of the logic for the code that automatically creates Drupal accounts, but not of when I am signed in through SSO and creating a different user account.

kenmc’s picture

kenmc commented
Version: 7.x-2.0-alpha2 » 7.x-2.x-dev
Status: Needs work » Needs review
StatusFileSize
new simplesamlphp_auth-fixnewdrupalusers-1824194-5.patch3.28 KB

The following patch adds a new test to make sure the current SSO user account matches the Drupal account being created before it updates any of the user information.

jeffam’s picture

jeffam
Primary language English
commented
StatusFileSize
new simplesamlphp_auth-fixnewdrupalusers-1824194-6.patch674 bytes

Above patch didn't work for me, but sure pointed me in the right direction. Here's what worked in my tests. I found that checking $account->init worked better than $account->name since the module allows the username to be generated from any saml attribute.

grasmash’s picture

grasmash commented
Issue summary: View changes

I've created a 7.x-3.x branch of this module with significant refactoring. It includes patches from the following issues:

#2190487: User_login form should have a federated link
#2213283: When simplesamlphp_auth_activate is 0 watchdog messages flood the syslog/dblog
#2221679: Destruction of session causes fatal error
#2020009: Session cookie being set, breaking through Varnish cache.
#1824194: hook_user_insert issues when user adding a new user is logged in via sso
#2205901: Allow SimpleSAML authentication on a per-user basis

https://github.com/grasmash/simplesamlphp_auth/tree/7.x-3.x

Given that so much work has been done (and it will not cleanly merge with 7.x-2.x) I propose that a new 7.x-3.x-dev branch be added to the module.

colan’s picture

colan
he/him
Primary language English
Location Toronto 🇨🇦
commented

@grasmash: Once folks RTBC your work, I'll happily merge all of it in as 3.x here. Thanks for refactoring!

cthos’s picture

cthos commented
Status: Needs review » Reviewed & tested by the community

Looks pretty good to me. Moving this over to RTBC, since no one else has chimed in yet.

I especially like that the setup code is no longer in hook_init.

colan’s picture

colan
he/him
Primary language English
Location Toronto 🇨🇦
commented

Thanks for the review. I'll get that branch set up!

eiriksm’s picture

eiriksm
he/him
Primary language Norwegian Bokmål
Location Norway
commented

Patch in #6 looks great!

Used on a 1.2 version of the module. Of course, the patch does not apply, but it is pretty obvious where you are supposed to put the changed line :)

snufkin’s picture

snufkin commented
Version: 7.x-2.x-dev » 7.x-3.x-dev
Status: Reviewed & tested by the community » Fixed

Fixed in 3.x

colan’s picture

colan
he/him
Primary language English
Location Toronto 🇨🇦
commented

@snufkin: Thanks for bringing things up-to-speed. I haven't had a chance lately.

Status: Fixed » Closed (fixed)

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

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented

FYI this bug exists in 2.x and there's a patch that fixes it here: #2365079: SimpleSAML authenticated admin users cannot create other users