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

Add try/catch blocks to methods for retrieving user info #8002

Merged
merged 9 commits into from
Aug 12, 2024

Conversation

mehansen
Copy link
Collaborator

@mehansen mehansen commented Aug 5, 2024

BACKEND PULL REQUEST

Related Issue

Related to a couple support requests where support admin folks aren't able to load a user using the Manager users page if they don't have access to any facilities.
With our Okta migration code, we are persisting auth data every time we load a user. If the user has an invalid state because of no facility access, we throw an exception and that user's page can't be loaded.

Changes Proposed

Catch the exception potentially created when migrating a user's roles and facilities to the DB only in methods where we are retrieving user info (not updating).

Additional Information

Folks will still get an error if trying to update the target user in any way if the target user is in an invalid state. This seems like a good idea because it'll force users to fix the facility access for that target user if they want to do any updates to them.

Testing

deployed to dev3
you can update a user in Okta to not have access to any groups and check that you can still view them in the org admin or support admin manage users pages

Copy link

sonarqubecloud bot commented Aug 6, 2024

@mehansen mehansen marked this pull request as ready for review August 7, 2024 18:00
Copy link
Contributor

@fzhao99 fzhao99 left a comment

Choose a reason for hiding this comment

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

I know that we're assuming in the test case that a user has a NO_ACCESS membership, but wondering if we should try accounting for that? In the event that someone's group membership gets purged, would imagine their NO_ACCESS permissions would probably also disappear, so depending on how complicated it is might want to handle that scenario

Otherwise LGTM!

Screen recording for reference
https://github.com/user-attachments/assets/99aa2824-2a79-4dde-889b-fd5987fa6b0f

Copy link
Collaborator

@emyl3 emyl3 left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for fixing this, Merethe 😄 🥹

Re: Bob's comment about searching a user with no NO_ACCESS Okta group --Maybe logging the user ID in a separate PR when we throw the OktaAccountUserException can be helpful so we know how frequent this state is and making it easier to debug.

For the purpose of the migration and bug fix, we wouldn't be migrating anything so leaving this as-is seems fine to me.

@mehansen mehansen added this pull request to the merge queue Aug 12, 2024
Merged via the queue into main with commit 680ba6e Aug 12, 2024
38 checks passed
@mehansen mehansen deleted the merethe/catch-facility-exception branch August 12, 2024 21:19
@mehansen
Copy link
Collaborator Author

I know that we're assuming in the test case that a user has a NO_ACCESS membership, but wondering if we should try accounting for that? In the event that someone's group membership gets purged, would imagine their NO_ACCESS permissions would probably also disappear, so depending on how complicated it is might want to handle that scenario

Otherwise LGTM!

Screen recording for reference https://github.com/user-attachments/assets/99aa2824-2a79-4dde-889b-fd5987fa6b0f

@fzhao99 I believe the behavior of user has no groups -> we show the misconfigured user toast was part of the design of the support admin Manage users page, but maybe that was a miss because it seems like it would be more useful to support admin if they could view a user even if they have no groups.
It might be worthwhile to ask support if they would like this capability but as I think about it more I feel like if a user's groups are getting purged maybe it's best support has to reach out to us because we should probably try to figure out how that would happen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants