-
Notifications
You must be signed in to change notification settings - Fork 48
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.
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
Fix MFA policy management with unknown users #1125
Conversation
Fixes issue with usersync and MFA policy management in cases where a user has not been persisted by Fence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we might need a unit test covering the use case of a user that doesn't exist in the db
@@ -1886,7 +1890,7 @@ def _revoke_all_policies_preserve_mfa(self, user): | |||
|
|||
policies = [] | |||
try: | |||
policies = self.arborist_client.get_user()["policies"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure how this ever worked... get_user
is missing the required username
parameter
syncer._revoke_all_policies_preserve_mfa(user_that_doesnt_exist, "mock_idp") | ||
|
||
# we only care that this doesn't error | ||
assert True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
syncer._revoke_all_policies_preserve_mfa(user_that_doesnt_exist, "mock_idp") | |
# we only care that this doesn't error | |
assert True | |
# we only care that this doesn't error | |
syncer._revoke_all_policies_preserve_mfa(user_that_doesnt_exist, "mock_idp") |
I tested this in the broken environment and it fixed the issue. I reran jenkins. @Avantol13 @k-burt-uch are you ok with merging? |
So long as we make sure the single failing data upload test eventually passes. It makes me a bit nervous force merging but I know this has some production impact |
Yes we can wait for jenkins, there seems to be a CI bottleneck but it will probably run by tomorrow |
New Features
Breaking Changes
Bug Fixes
Improvements
Dependency updates
Deployment changes