Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

properly set is_system_auditor on initial LDAP login #4396

Merged
merged 1 commit into from
Jul 30, 2019
Merged

properly set is_system_auditor on initial LDAP login #4396

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 16 additions & 12 deletions awx/main/models/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,18 +122,22 @@ def user_is_system_auditor(user):

@user_is_system_auditor.setter
def user_is_system_auditor(user, tf):
if user.id:
if tf:
role = Role.singleton('system_auditor')
# must check if member to not duplicate activity stream
if user not in role.members.all():
role.members.add(user)
user._is_system_auditor = True
else:
role = Role.singleton('system_auditor')
if user in role.members.all():
role.members.remove(user)
user._is_system_auditor = False
if not user.id:
# 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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The getter would also probably need modification, user_is_system_auditor above this.

Copy link
Contributor Author

@ryanpetrello ryanpetrello Jul 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TFW you see someone has already responded more extensively than oneself after you hit 'comment' 🤦‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤷‍♂

if tf:
role = Role.singleton('system_auditor')
# must check if member to not duplicate activity stream
if user not in role.members.all():
role.members.add(user)
user._is_system_auditor = True
else:
role = Role.singleton('system_auditor')
if user in role.members.all():
role.members.remove(user)
user._is_system_auditor = False


User.add_to_class('is_system_auditor', user_is_system_auditor)
Expand Down