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.

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

Merged
merged 7 commits into from
Dec 7, 2023

Conversation

k-burt-uch
Copy link
Contributor

New Features

Breaking Changes

Bug Fixes

  • Fixes issue with usersync and MFA policy management in cases where a user has not been persisted by Fence.

Improvements

Dependency updates

Deployment changes

Fixes issue with usersync and MFA policy management in cases where a user has not been persisted by Fence.
paulineribeyre
paulineribeyre previously approved these changes Dec 4, 2023
Copy link
Contributor

@paulineribeyre paulineribeyre left a comment

Choose a reason for hiding this comment

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

looks good

Copy link
Contributor

@paulineribeyre paulineribeyre left a 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"]
Copy link
Contributor

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

Comment on lines +1045 to +1048
syncer._revoke_all_policies_preserve_mfa(user_that_doesnt_exist, "mock_idp")

# we only care that this doesn't error
assert True
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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")

@paulineribeyre
Copy link
Contributor

I tested this in the broken environment and it fixed the issue. I reran jenkins. @Avantol13 @k-burt-uch are you ok with merging?

@Avantol13
Copy link
Contributor

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

@paulineribeyre
Copy link
Contributor

Yes we can wait for jenkins, there seems to be a CI bottleneck but it will probably run by tomorrow

@paulineribeyre paulineribeyre merged commit e624421 into master Dec 7, 2023
@paulineribeyre paulineribeyre deleted the fix/user-undefined-revoke-mfa branch December 7, 2023 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants