properly set is_system_auditor on initial LDAP login
#4396
Conversation
django-auth-ldap recently changed its behavior at login to *delay* the user.save() call: django-auth-ldap/django-auth-ldap@b777321 our current process of discovering and setting up the system auditor role at LDAP login *relies* on the user having a primary key, so this code now manually calls .save() to enforce one
| # If the user doesn't have a primary key yet (i.e., this is the *first* | ||
| # time they've logged in, and we've just created the new User in this | ||
| # request), we need one to set up the system auditor role | ||
| user.save() |
There was a problem hiding this comment.
The getter would also probably need modification, user_is_system_auditor above this.
There was a problem hiding this comment.
I'm wary of adding a .save() to a property getter like this, and I don't think it's actually necessary.
I dug through this code and I don't see any cases where the login gets the attribute before setting it.
This line for __setattr__ sets ._is_system_auditor:
https://github.com/ansible/awx/pull/4396/files#diff-b00f0ac10475072c791a4f7d06782447L131
...and if we do __getattr__ later, we'll hit this line:
https://github.com/ansible/awx/pull/4396/files#diff-b00f0ac10475072c791a4f7d06782447R120
So the only way you should enter this block:
https://github.com/ansible/awx/pull/4396/files#diff-b00f0ac10475072c791a4f7d06782447R118
...is if you do .is_system_auditor on a User before it's saved. I can't find any login code paths where that's the case, and it seems like that's always been the behavior (given the comment on that line). Also, it doesn't really make sense given how we implemented roles - in order for a user to be a system auditor, they must have a role set, and to have a role set, we must have persisted them w/ a primary key first.
There was a problem hiding this comment.
It looks to me like it does not need to be modified, since it already has a defensive check for the case where the user object hasn't been saved yet.
There was a problem hiding this comment.
TFW you see someone has already responded more extensively than oneself after you hit 'comment' 🤦♂️
|
Build succeeded.
|
|
see: #4397 |
|
Build succeeded (gate pipeline).
|
…t_use_reports_3.7.1 disable reports option for foreman (release_3.7.1)
django-auth-ldaprecently changed its behavior at login to delay theuser.save()call:django-auth-ldap/django-auth-ldap@b777321
our current process of discovering and setting up the system auditor
role at LDAP login relies on the user having a primary key, so this
code now manually calls .save() to enforce one