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

Log error if Okta and DB role claims unequal #8182

Merged
merged 1 commit into from
Oct 23, 2024
Merged

Conversation

emyl3
Copy link
Collaborator

@emyl3 emyl3 commented Oct 9, 2024

BACKEND PULL REQUEST

Related Issue

Changes Proposed

  • Compare role claims in the DB and Okta and log an error if they are unequal for the following scenarios:
    • call getUser method
    • call getUserByLoginEmail method
    • when we set the organizationRolesContext

Additional Information

Testing

  • deployed on dev3 with the feature flag on
  • Removed the following group in Okta for this user [email protected] on dev3:
    • SR-DEV3-TENANT:AK-Bobans-org-5145a894-3be7-45ea-a5a1-79895df0352c:ALL_FACILITIES
    • searched the user in the "Manage users" support admin tool
    • logged here in Azure
Screenshot 2024-10-11 at 13 35 15
  • log in as a non-site admin user on the envs above and navigate around the app

@emyl3 emyl3 force-pushed the elisa/7598-add-logs branch from 8cd863e to f800b08 Compare October 11, 2024 16:28
Copy link

@@ -60,6 +60,8 @@ public List<OrganizationRoleClaims> findAllOrganizationRoles() {
String username = currentAuth.getName();
List<OrganizationRoleClaims> dbOrgRoleClaims =
_dbOrgRoleClaimsService.getOrganizationRoleClaims(username);
_dbOrgRoleClaimsService.checkOrgRoleClaimsEquality(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

⚠️ Anyone know how often we reset organizationRolesContext and have to fetch it again? 🤔

Based on digging around in Azure, it seems like on initial log in and page load fetchCurrentOrganizationRoles (which is the only method that calls findAllOrganizationRoles is called a bunch of times and then not again for 20-30 mins or so...

I'm asking because I'm wondering if we are doing the check too frequently if I set the check at this point. 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't know off the top of my head. And not sure if the better place to look would be spring docs or okta's 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

does this line mean that the context is only valid for a single request? if so it seems like it would need to be refetched any time we are handling a new request that needs the current org roles

@Scope(scopeName = WebApplicationContext.SCOPE_REQUEST, proxyMode = ScopedProxyMode.TARGET_CLASS)

Copy link
Collaborator

Choose a reason for hiding this comment

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

fwiw since we are condensing the logs into a single alert per day (?) it doesn't seem that harmful if we are logging frequently. though it may be a little bit of a pain to parse through the log lines later I don't see a super simple alternative 🤷

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you for digging into that @mehansen. Based on what you shared, I'm surprised I didn't see it log more frequently when testing. 🤷 After I merge, I'll keep on eye on prod and the logs...

@@ -159,7 +159,7 @@ void getUser_withAdminUser_withOktaMigrationDisabled_success() {

UserInfo userInfo = _service.getUser(apiUser.getInternalId());

verify(_dbOrgRoleClaimsService, times(0)).getOrganizationRoleClaims((ApiUser) any());
verify(_dbOrgRoleClaimsService, times(1)).getOrganizationRoleClaims((ApiUser) any());
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

⚠️ Had to increment by 1 in this test file because of how we are comparing the role claims -- we fetch from the DB regardless of whether the feature flag is on or off.

@emyl3 emyl3 marked this pull request as ready for review October 12, 2024 00:18
Copy link
Collaborator

@DanielSass DanielSass left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Collaborator

@mpbrown mpbrown left a comment

Choose a reason for hiding this comment

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

LGTM! Great work on this Elisa!

@emyl3 emyl3 added this pull request to the merge queue Oct 23, 2024
Merged via the queue into main with commit 7365421 Oct 23, 2024
38 checks passed
@emyl3 emyl3 deleted the elisa/7598-add-logs branch October 23, 2024 15:25
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.

Initiate rolling migration, phase 2
5 participants