Skip to content

Commit

Permalink
Add try/catch blocks to methods for retrieving user info (#8002)
Browse files Browse the repository at this point in the history
* add try/catch blocks to methods for getting user info

* add test coverage

* test coverage for whoami for invalid facilities user

* fix demo user tests

* self nit

* clean up test to use demo user

* code smell
  • Loading branch information
mehansen authored Aug 12, 2024
1 parent 7886567 commit 680ba6e
Show file tree
Hide file tree
Showing 6 changed files with 62 additions and 13 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -599,7 +599,13 @@ public UserInfo getCurrentUserInfoForWhoAmI() {
Optional<OrganizationRoles> currentOrgRoles = _orgService.getCurrentOrganizationRoles();
boolean isAdmin = _authService.isSiteAdmin();
if (!_featureFlagsConfig.isOktaMigrationEnabled() && currentOrgRoles.isPresent() && !isAdmin) {
setRolesAndFacilities(currentOrgRoles.get(), currentUser);
try {
setRolesAndFacilities(currentOrgRoles.get(), currentUser);
} catch (PrivilegeUpdateFacilityAccessException e) {
log.warn(
"Could not migrate roles and facilities for user with id={} because facilities were invalid",
currentUser.getInternalId());
}
}
return new UserInfo(currentUser, currentOrgRoles, isAdmin);
}
Expand Down Expand Up @@ -739,7 +745,13 @@ private UserInfo consolidateUser(
OrganizationRoles orgRoles =
new OrganizationRoles(org, accessibleFacilities, claims.getGrantedRoles());
if (!_featureFlagsConfig.isOktaMigrationEnabled() && !isSiteAdmin) {
setRolesAndFacilities(orgRoles, apiUser);
try {
setRolesAndFacilities(orgRoles, apiUser);
} catch (PrivilegeUpdateFacilityAccessException e) {
log.warn(
"Could not migrate roles and facilities for user with id={} because facilities were invalid",
apiUser.getInternalId());
}
}
return new UserInfo(apiUser, Optional.of(orgRoles), isSiteAdmin, userStatus);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,10 @@ protected void useBrokenUser() {
username = TestUserIdentities.BROKEN_USER;
}

protected void useInvalidFacilitiesUser() {
username = TestUserIdentities.INVALID_FACILITIES_USER;
}

@BeforeEach
public void baseAuthenticatedFullStackTestSetup() {
truncateDb();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,16 @@ void whoami_nobody_okResponses() {
assertUserHasNoOrg(who);
}

@Test
void whoami_invalidFacilityAccess_okRolesFacilities() {
useInvalidFacilitiesUser();

ObjectNode who = (ObjectNode) runQuery("current-user-query").get("whoami");
assertFalse(who.get("isAdmin").asBoolean());
assertEquals(who.get("role").asText(), Role.USER.name());
assertTrue(extractFacilitiesFromUser(who).isEmpty());
}

@ParameterizedTest
@ValueSource(strings = {"addUserOp", "addUserLegacy"})
void addUser_superUser_success(String operation) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,17 +74,19 @@ void cleanup() {
void getUsersInCurrentOrg_adminUser_success() {
initSampleData();
List<ApiUser> users = _service.getUsersInCurrentOrg();
assertEquals(5, users.size());
assertEquals(6, users.size());
assertEquals("[email protected]", users.get(0).getLoginEmail());
assertEquals("Andrews", users.get(0).getNameInfo().getLastName());
assertEquals("[email protected]", users.get(1).getLoginEmail());
assertEquals("Bobberoo", users.get(1).getNameInfo().getLastName());
assertEquals("[email protected]", users.get(2).getLoginEmail());
assertEquals("Nixon", users.get(2).getNameInfo().getLastName());
assertEquals("[email protected]", users.get(3).getLoginEmail());
assertEquals("Reynolds", users.get(3).getNameInfo().getLastName());
assertEquals("[email protected]", users.get(4).getLoginEmail());
assertEquals("Williams", users.get(4).getNameInfo().getLastName());
assertEquals("[email protected]", users.get(2).getLoginEmail());
assertEquals("Irwin", users.get(2).getNameInfo().getLastName());
assertEquals("[email protected]", users.get(3).getLoginEmail());
assertEquals("Nixon", users.get(3).getNameInfo().getLastName());
assertEquals("[email protected]", users.get(4).getLoginEmail());
assertEquals("Reynolds", users.get(4).getNameInfo().getLastName());
assertEquals("[email protected]", users.get(5).getLoginEmail());
assertEquals("Williams", users.get(5).getNameInfo().getLastName());
}

@Test
Expand All @@ -103,14 +105,15 @@ void getUsersInCurrentOrg_standardUser_error() {
void getUsersAndStatusInCurrentOrg_success() {
initSampleData();
List<ApiUserWithStatus> users = _service.getUsersAndStatusInCurrentOrg();
assertEquals(5, users.size());
assertEquals(6, users.size());

checkApiUserWithStatus(users.get(0), "[email protected]", "Andrews", UserStatus.ACTIVE);
checkApiUserWithStatus(users.get(1), "[email protected]", "Bobberoo", UserStatus.ACTIVE);
checkApiUserWithStatus(users.get(2), "[email protected]", "Nixon", UserStatus.ACTIVE);
checkApiUserWithStatus(users.get(3), "[email protected]", "Reynolds", UserStatus.ACTIVE);
checkApiUserWithStatus(users.get(2), "[email protected]", "Irwin", UserStatus.ACTIVE);
checkApiUserWithStatus(users.get(3), "[email protected]", "Nixon", UserStatus.ACTIVE);
checkApiUserWithStatus(users.get(4), "[email protected]", "Reynolds", UserStatus.ACTIVE);
checkApiUserWithStatus(
users.get(4), "[email protected]", "Williams", UserStatus.ACTIVE);
users.get(5), "[email protected]", "Williams", UserStatus.ACTIVE);
}

@Test
Expand Down Expand Up @@ -497,6 +500,18 @@ void getUserByLoginEmail_not_authorized() {
assertEquals("Access Denied", caught.getMessage());
}

@Test
@WithSimpleReportSiteAdminUser
void getUserByLoginEmail_invalidClaims_success() {
initSampleData();
String email = "[email protected]";

// we should be able to retrieve user info for a user with invalid claims (no facility access)
// without failing
UserInfo result = _service.getUserByLoginEmail(email);
assertThat(result.getFacilities()).isEmpty();
}

@Test
@WithSimpleReportSiteAdminUser
void updateUserPrivilegesAndGroupAccess_assignToAllFacilities_success() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ public class TestUserIdentities {
public static final String ENTRY_ONLY_USER = "[email protected]";
public static final String ORG_ADMIN_USER = "[email protected]";
public static final String ALL_FACILITIES_USER = "[email protected]";
public static final String INVALID_FACILITIES_USER = "[email protected]";

public static final String OTHER_ORG_USER = "[email protected]";
public static final String OTHER_ORG_ADMIN = "[email protected]";
Expand Down
7 changes: 7 additions & 0 deletions backend/src/test/resources/application-test.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,13 @@ simple-report:
authorization: # USER_ALL_FACILITIES_ORG_ROLES
organization-external-id: DIS_ORG
granted-roles: NO_ACCESS, ADMIN
- identity: #standard user with invalid no facility access
username: [email protected]
first-name: Igor
last-name: Irwin
authorization:
organization-external-id: DIS_ORG
granted-roles: NO_ACCESS, USER
- identity: # OUTSIDE_ORG_USER
username: [email protected]
first-name: Bootstrap
Expand Down

0 comments on commit 680ba6e

Please sign in to comment.