Fetch group info for users from DB when oktaMigrationEnabled is true #8105

emyl3 · 2024-09-06T17:56:11Z

BACKEND PULL REQUEST

Related Issue

Part of Initiate rolling migration, phase 2 #7598

Changes Proposed

Regardless of oktaMigrationEnabled flag status, continue to update groups and roles in Okta
When the oktaMigrationEnabled flag is set to true, read groups and roles from DB
When the oktaMigrationEnabled flag is set to false, read groups and roles from Okta

Affected flows:

adding org to queue (checking if the org is a duplicate)
reprovisioning user
updating user roles and privileges
users graphql query (I don't think we are calling this on the frontend 🤔)
verifying a pending organization
getting org admin users
confirmation modal when deleting a facility via site admin "Manage facility" tool (it displays how many users and patients are associated with only that facility)

Additional Information

Separate tickets to address loading users with their status for the "Manage users" page

Testing

dev2 - oktaMigrationEnabled - true

dev2 metabase link

dev3 - oktaMigrationEnabled - false

dev3 metabase link to see roles and facilities at a glance

backend/src/main/java/gov/cdc/usds/simplereport/idp/repository/DemoOktaRepository.java

backend/src/test/java/gov/cdc/usds/simplereport/service/DbAuthorizationServiceTest.java

emyl3 · 2024-09-10T15:15:37Z

backend/src/main/java/gov/cdc/usds/simplereport/service/ApiUserService.java

  }

+  // To be addressed in #8108
  @AuthorizationConfiguration.RequirePermissionManageUsers
  public List<ApiUserWithStatus> getUsersAndStatusInCurrentOrg() {


⚠️ holding off on updating this until this is addressed #8103

Then, this will be updated in #8108

emyl3 · 2024-09-10T19:36:09Z

backend/src/main/java/gov/cdc/usds/simplereport/service/OrganizationService.java

+        String orgAdminEmail = orgAdmin.get().getLoginEmail();
+        return oktaRepository.activateUser(orgAdminEmail);
+      } else {
+        throw new IllegalStateException("Organization does not have any org admins.");


⚠️ This else block changes the behavior of this method when the feature flag is enabled.

Previously, even if the org had no admin users this method would succeed

However, with this implementation if an org does not have an admin it will throw an exception.

Let me know if you think this should continue to have the same behavior as before. 🤔

I think this makes sense. Btw do we have any additional context on the comment on this method? Is this an old reference to some account request workflow that hasn't been implemented?

/** * This method is for verifying an organization after the Experian identity verification process. * It should not be used for any other purpose and once we move to the updated account request * workflow this should be removed. */ @Transactional(readOnly = false) public String verifyOrganizationNoPermissions(String externalId) {

That's a good question... Let me check that commit history in see if I can uncover anything... 🕵️

This is the PR that introduced this change: #2096
I did a search in the repo and couldn't find any mention of a new or updated account request flow 🤔 so I'm not entirely sure what that comment is referencing....

emyl3 · 2024-09-10T19:39:48Z

backend/src/main/java/gov/cdc/usds/simplereport/service/DbAuthorizationService.java

+   * @return Integer - count of ApiUsers
+   */
+  public Integer getUserWithSingleFacilityAccessCount(Facility facility) {
+    List<ApiUser> users = _userRepo.findAllBySingleFacilityAccess(facility);


⚠️ Let me know if this makes sense to generate this count 🙏

I think this will fix this bug #7873 🤩

backend/src/main/java/gov/cdc/usds/simplereport/db/repository/ApiUserRepository.java

mpbrown

Looks really great overall! Awesome work on this Elisa!

...end/src/main/java/gov/cdc/usds/simplereport/api/accountrequest/AccountRequestController.java

mpbrown · 2024-09-13T16:34:33Z

backend/src/main/java/gov/cdc/usds/simplereport/service/OrganizationService.java

+        String orgAdminEmail = orgAdmin.get().getLoginEmail();
+        return oktaRepository.activateUser(orgAdminEmail);
+      } else {
+        throw new IllegalStateException("Organization does not have any org admins.");


I think this makes sense. Btw do we have any additional context on the comment on this method? Is this an old reference to some account request workflow that hasn't been implemented?

/** * This method is for verifying an organization after the Experian identity verification process. * It should not be used for any other purpose and once we move to the updated account request * workflow this should be removed. */ @Transactional(readOnly = false) public String verifyOrganizationNoPermissions(String externalId) {

mpbrown · 2024-09-13T16:36:44Z

...end/src/main/java/gov/cdc/usds/simplereport/api/accountrequest/AccountRequestController.java

-            .anyMatch(Predicate.isEqual(email))) {
+        List<String> adminUserEmails = getOrgAdminUserEmails(duplicateOrg.get());
+
+        if (adminUserEmails.stream().anyMatch(Predicate.isEqual(email))) {


Do we need to handle or otherwise flag if the org doesn't have any admins? (was thinking about no admin users after this comment)

I think it's OK in this context because we are trying to check if the to-be-created new org might be a duplicate of another org. If there are no org admins in that duplicate org, it would throw the exception on line 155. Let me know what you think!

backend/src/main/java/gov/cdc/usds/simplereport/service/OrganizationService.java

bobbywells52

Tested in dev2 and dev3 and is working as excepted -- LGTM! Thank you for you continued heavy effort on this big lift 🙌🏻

emyl3 · 2024-09-17T14:55:38Z

@mpbrown @mehansen @DanielSass Ready for re-review! Changes have been redeployed to dev2 and dev3 respectively

mpbrown

LGTM on dev2 and dev3! Great work on this Elisa!

mehansen

looks great! I have some non-blocking naming nits but they are more personal preference so still approving

backend/src/main/java/gov/cdc/usds/simplereport/db/repository/ApiUserRepository.java

mehansen · 2024-09-18T06:32:53Z

backend/src/main/java/gov/cdc/usds/simplereport/service/DbAuthorizationService.java

+   * @return Integer - count of ApiUsers
+   */
+  public Integer getUserWithSingleFacilityAccessCount(Facility facility) {
+    List<ApiUser> users = _userRepo.findAllBySingleFacilityAccess(facility);


I think this will fix this bug #7873 🤩

sonarqubecloud · 2024-09-18T15:35:56Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
98.1% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

DanielSass · 2024-09-18T20:03:31Z

backend/src/main/java/gov/cdc/usds/simplereport/service/ApiUserService.java

+  private String getOrgExternalId(ApiUser apiUser) {
+    String orgExternalId;
+    if (_featureFlagsConfig.isOktaMigrationEnabled()) {
+      Optional<Organization> org = apiUser.getOrganizations().stream().findFirst();


Do we have users with more than one organization? I don't think we do, but do we know that for certain / are there guardrails in place to ensure that can't happen?

DanielSass

Everything looks good, thanks for pairing with me on the review @emyl3.
I think the only issues we noted were specific corner cases we need to make sure get included in the test plan.

Thanks for all your work on this

github-advanced-security bot found potential problems Sep 6, 2024

View reviewed changes

backend/src/main/java/gov/cdc/usds/simplereport/idp/repository/DemoOktaRepository.java Fixed Show fixed Hide fixed

backend/src/test/java/gov/cdc/usds/simplereport/service/DbAuthorizationServiceTest.java Fixed Show fixed Hide fixed

emyl3 force-pushed the elisa/7598-refactor-groupapi-calls branch 6 times, most recently from acf4def to 754ae1f Compare September 9, 2024 20:09

Fetch group info for users from DB when oktaMigrationEnabled is true

b7956bc

emyl3 force-pushed the elisa/7598-refactor-groupapi-calls branch from 754ae1f to b7956bc Compare September 10, 2024 14:37

emyl3 temporarily deployed to dev3 September 10, 2024 15:10 — with GitHub Actions Inactive

emyl3 commented Sep 10, 2024

View reviewed changes

emyl3 temporarily deployed to dev3 September 10, 2024 15:20 — with GitHub Actions Inactive

emyl3 temporarily deployed to dev3 September 10, 2024 15:23 — with GitHub Actions Inactive

emyl3 commented Sep 10, 2024

View reviewed changes

emyl3 marked this pull request as ready for review September 11, 2024 15:52

emyl3 requested review from mehansen, DanielSass, mpbrown and bobbywells52 September 11, 2024 15:53

bobbywells52 reviewed Sep 11, 2024

View reviewed changes

backend/src/main/java/gov/cdc/usds/simplereport/db/repository/ApiUserRepository.java Outdated Show resolved Hide resolved

mpbrown reviewed Sep 13, 2024

View reviewed changes

emyl3 added 2 commits September 16, 2024 12:20

Address PR comments

fd7d4a6

Lint backend

60799de

emyl3 temporarily deployed to dev3 September 16, 2024 17:41 — with GitHub Actions Inactive

emyl3 temporarily deployed to dev3 September 16, 2024 17:47 — with GitHub Actions Inactive

emyl3 temporarily deployed to dev3 September 16, 2024 17:51 — with GitHub Actions Inactive

bobbywells52 self-requested a review September 17, 2024 14:34

bobbywells52 previously approved these changes Sep 17, 2024

View reviewed changes

emyl3 requested a review from mpbrown September 17, 2024 14:54

mpbrown previously approved these changes Sep 17, 2024

View reviewed changes

mehansen previously approved these changes Sep 18, 2024

View reviewed changes

Address PR comments

d9af933

emyl3 dismissed stale reviews from mehansen, mpbrown, and bobbywells52 via d9af933 September 18, 2024 15:24

emyl3 temporarily deployed to dev3 September 18, 2024 15:32 — with GitHub Actions Inactive

emyl3 temporarily deployed to dev3 September 18, 2024 15:40 — with GitHub Actions Inactive

emyl3 temporarily deployed to dev3 September 18, 2024 15:42 — with GitHub Actions Inactive

DanielSass reviewed Sep 18, 2024

View reviewed changes

DanielSass approved these changes Sep 19, 2024

View reviewed changes

mehansen approved these changes Sep 19, 2024

View reviewed changes

mpbrown approved these changes Sep 19, 2024

View reviewed changes

emyl3 added this pull request to the merge queue Sep 19, 2024

Merged via the queue into main with commit 8f61a00 Sep 19, 2024
38 checks passed

emyl3 deleted the elisa/7598-refactor-groupapi-calls branch September 19, 2024 20:32

emyl3 mentioned this pull request Sep 20, 2024

Initiate rolling migration, phase 2 #7598

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fetch group info for users from DB when oktaMigrationEnabled is true #8105

Fetch group info for users from DB when oktaMigrationEnabled is true #8105

emyl3 commented Sep 6, 2024 •

edited

Loading

emyl3 Sep 10, 2024

emyl3 Sep 10, 2024 •

edited

Loading

mpbrown Sep 13, 2024

emyl3 Sep 16, 2024

emyl3 Sep 16, 2024

emyl3 Sep 10, 2024

mehansen Sep 18, 2024

mpbrown left a comment

mpbrown Sep 13, 2024

mpbrown Sep 13, 2024

emyl3 Sep 16, 2024

bobbywells52 left a comment

emyl3 commented Sep 17, 2024

mpbrown left a comment

mehansen left a comment

mehansen Sep 18, 2024

sonarqubecloud bot commented Sep 18, 2024

DanielSass Sep 18, 2024

DanielSass left a comment

Fetch group info for users from DB when oktaMigrationEnabled is true #8105

Fetch group info for users from DB when oktaMigrationEnabled is true #8105

Conversation

emyl3 commented Sep 6, 2024 • edited Loading

BACKEND PULL REQUEST

Related Issue

Changes Proposed

Additional Information

Testing

Choose a reason for hiding this comment

emyl3 Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpbrown left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobbywells52 left a comment

Choose a reason for hiding this comment

emyl3 commented Sep 17, 2024

mpbrown left a comment

Choose a reason for hiding this comment

mehansen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Sep 18, 2024

Quality Gate passed

Choose a reason for hiding this comment

DanielSass left a comment

Choose a reason for hiding this comment

emyl3 commented Sep 6, 2024 •

edited

Loading

emyl3 Sep 10, 2024 •

edited

Loading