When a user that's been authenticated through SSO tries to create other users, those users have the name & email address of the admin applied to them. This results in a DB constraint error on the user name.

CommentFileSizeAuthor
#13 user-data-leaks-when-editing-2365079-13.patch3.65 KBbkosborne
#13 interdiff.txt483 bytesbkosborne
#12 user-data-leaks-when-editing-2365079-12.patch3.62 KBbkosborne
#11 simplesamlphp_auth-admin-users-cannot-other-users-9291915-11-with-early-return.patch3.95 KBvmachado
#9 simplesamlphp_auth-admin-users-cannot-other-users-9291915-9-with-early-return.patch3.71 KBchgasparoto
#9 simplesamlphp_auth-admin-users-cannot-other-users-9291915-9.patch707 byteschgasparoto
#8 simplesamlphp_auth-admin-users-cannot-other-users-9291915-8.patch3.06 KBchrisgross
#2 simplesamlphp_auth-admin-users-cannot-other-users-9291915-1.patch3.06 KBrichburke
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

richburke’s picture

richburke CreditAttribution: richburke commented
Title: SSO authenticated admin users cannot create other users » SimpleSAML authenticated admin users cannot create other users
richburke’s picture

richburke CreditAttribution: richburke commented
FileSize
simplesamlphp_auth-admin-users-cannot-other-users-9291915-1.patch3.06 KB

The attached patch addresses the issue detailed above.

m.abdulqader’s picture

m.abdulqader CreditAttribution: m.abdulqader commented

I have the same bug I will try the patch and let you now

m.abdulqader’s picture

m.abdulqader CreditAttribution: m.abdulqader commented

OK the patch works for me. Thank you.

tbsiqueira’s picture

tbsiqueira
He/him
CreditAttribution: tbsiqueira at CI&T commented
Status: Active » Reviewed & tested by the community

I've tested this patch and it works very well, the code is precise and clean. Thank you "richburke" for your patch. This is ready to commited!

snufkin’s picture

snufkin CreditAttribution: snufkin commented

Hey, which version have you tested this against? 2.x?

chrisgross’s picture

chrisgross CreditAttribution: chrisgross commented

For some reason, git apply simplesamlphp_auth-admin-users-cannot-other-users-9291915-1.patch does not work for me, but patch -p1 < simplesamlphp_auth-admin-users-cannot-other-users-9291915-1.patch does. I am going to test this out, and if it works, I will try re-roll the patch in a way that works with git apply. Due to the other active branches that are newer, I assume this is not going to get committed to an alpha branch anytime soon. If I'm wrong, please commit it.

chrisgross’s picture

chrisgross CreditAttribution: chrisgross commented
FileSize
simplesamlphp_auth-admin-users-cannot-other-users-9291915-8.patch3.06 KB

Re-rolled the patch. This successfully applies against alpha2 and works perfectly.

chgasparoto’s picture

chgasparoto CreditAttribution: chgasparoto as a volunteer and at CI&T commented
FileSize
simplesamlphp_auth-admin-users-cannot-other-users-9291915-9.patch707 bytes
simplesamlphp_auth-admin-users-cannot-other-users-9291915-9-with-early-return.patch3.71 KB

I think the solution is simpler, because we need to compare on line 361 and not assign the $category variable.

Please review.

Thanks.

P.s.: I attached a version of the patch with early return approach. For me, is more readable.

chgasparoto’s picture

chgasparoto CreditAttribution: chgasparoto as a volunteer and at CI&T commented
Status: Reviewed & tested by the community » Needs review
vmachado’s picture

vmachado CreditAttribution: vmachado as a volunteer and at CI&T commented
FileSize
simplesamlphp_auth-admin-users-cannot-other-users-9291915-11-with-early-return.patch3.95 KB

The uploaded patch #8 has an issue when creating a new user during user first login. The user is created with success however it's email isn't set correctly. The same issue continues happening in patch #9, since it didn't fix this issue.

I'm uploading a new patch combining the correction from #8 and #9 and fixing all issues.

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
CreditAttribution: bkosborne at Princeton University commented
Version: 7.x-2.0-alpha2 » 7.x-2.x-dev
Priority: Normal » Critical
FileSize
user-data-leaks-when-editing-2365079-12.patch3.62 KB

Wow, what a big, awful, bug. The method taken here to set the username and email address on users during hook_user_insert makes no sense. It should be done during hook_user_login or somewhere else.

Anyway, patches should be made against the dev branch, which I what I did here. I also cleaned it up further.

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
CreditAttribution: bkosborne at Princeton University commented
FileSize
interdiff.txt483 bytes
user-data-leaks-when-editing-2365079-13.patch3.65 KB

Found a small bug in this, where the $_simplesamlphp_auth_as is not always defined in hook_user_insert. Fixed.

xs2bharat’s picture

xs2bharat CreditAttribution: xs2bharat commented

I am still having same issue that newly created user have email and name from signed in user after apply patch # 13

sguglielmo’s picture

sguglielmo CreditAttribution: sguglielmo commented

Tested #11 against 7.x-2.0-alpha2: Working well so far!

+1 RTBC

laryn’s picture

laryn
he/him
CreditAttribution: laryn at Aten Design Group, Backdrop CMS commented

I took the patch from #11 and manually worked it into a PR for the Backdrop version of this module and can confirm that the basic logic works well.