-
Notifications
You must be signed in to change notification settings - Fork 54
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 mutation to clear user roles and facilities #7978
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -376,6 +376,21 @@ public UserInfo resendActivationEmail(UUID userId) { | |
return new UserInfo(apiUser, Optional.of(orgRoles), false); | ||
} | ||
|
||
/** | ||
* Clear a user's roles and facilities - intended to be used on site admin users only, | ||
* non-site-admin users will be in a misconfigured state without roles and facilities | ||
* | ||
* @param username | ||
* @return ApiUser | ||
*/ | ||
@AuthorizationConfiguration.RequireGlobalAdminUser | ||
public ApiUser markUserRolesAndFacilitiesAsDeleted(String username) { | ||
ApiUser foundUser = | ||
_apiUserRepo.findByLoginEmail(username).orElseThrow(NonexistentUserException::new); | ||
foundUser.clearRolesAndFacilities(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. worth considering for a followup PR: could we stick a check for whether the found user is a site admin? Think this would entail extending our site admin validation code since from what I can tell, it grabs that information from the currently-logged in user auth properties, but think that
Since all this is behind a feature flag / this mutation is needed to clean up a testing env, no need to address here, but to consider as a follow up There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
return foundUser; | ||
} | ||
|
||
private ApiUser getApiUser(UUID id) { | ||
return getApiUser(id, false); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,17 @@ | ||
package gov.cdc.usds.simplereport.api.apiuser; | ||
|
||
import static org.assertj.core.api.Assertions.assertThat; | ||
import static org.junit.jupiter.api.Assertions.assertThrows; | ||
import static org.mockito.Mockito.times; | ||
import static org.mockito.Mockito.verify; | ||
import static org.mockito.Mockito.when; | ||
|
||
import gov.cdc.usds.simplereport.api.model.Role; | ||
import gov.cdc.usds.simplereport.api.model.User; | ||
import gov.cdc.usds.simplereport.api.model.errors.NonexistentUserException; | ||
import gov.cdc.usds.simplereport.db.model.ApiUser; | ||
import gov.cdc.usds.simplereport.db.model.Organization; | ||
import gov.cdc.usds.simplereport.db.repository.ApiUserRepository; | ||
import gov.cdc.usds.simplereport.service.ApiUserService; | ||
import gov.cdc.usds.simplereport.service.BaseServiceTest; | ||
import gov.cdc.usds.simplereport.service.model.UserInfo; | ||
|
@@ -21,6 +26,7 @@ | |
|
||
@WithSimpleReportOrgAdminUser | ||
class UserMutationResolverTest extends BaseServiceTest<ApiUserService> { | ||
@Autowired ApiUserRepository apiUserRepository; | ||
@Mock ApiUserService mockedApiUserService; | ||
|
||
@Autowired private TestDataFactory _dataFactory; | ||
|
@@ -32,11 +38,11 @@ class UserMutationResolverTest extends BaseServiceTest<ApiUserService> { | |
@BeforeEach | ||
void setup() { | ||
Organization org = _dataFactory.saveValidOrganization(); | ||
orgUserInfo = _dataFactory.createValidApiUser("[email protected]", org); | ||
orgUserInfo = _dataFactory.createValidApiUser("[email protected]", org, Role.USER); | ||
} | ||
|
||
@Test | ||
void reactivateUserAndResetPassword_orgAdmin_success() { | ||
void reactivateUserAndResetPassword_success() { | ||
UUID userInfoInternalId = orgUserInfo.getInternalId(); | ||
|
||
// GIVEN | ||
|
@@ -50,4 +56,34 @@ void reactivateUserAndResetPassword_orgAdmin_success() { | |
assertThat(resetUser.getInternalId()).isEqualTo(userInfoInternalId); | ||
verify(mockedApiUserService, times(1)).reactivateUserAndResetPassword(userInfoInternalId); | ||
} | ||
|
||
@Test | ||
void markUserRolesAndFacilitiesAsDeleted_nonExistentUser_throwException() { | ||
String username = orgUserInfo.getEmail(); | ||
|
||
// GIVEN | ||
ApiUser foundUser = apiUserRepository.findByLoginEmail(username).get(); | ||
when(mockedApiUserService.markUserRolesAndFacilitiesAsDeleted(username)).thenReturn(foundUser); | ||
|
||
// WHEN | ||
ApiUser resetUser = userMutationResolver.markUserRolesAndFacilitiesAsDeleted(username); | ||
|
||
// THEN | ||
assertThat(resetUser.getLoginEmail()).isEqualTo(orgUserInfo.getEmail()); | ||
verify(mockedApiUserService, times(1)).markUserRolesAndFacilitiesAsDeleted(username); | ||
} | ||
|
||
@Test | ||
void markUserRolesAndFacilitiesAsDeleted_failure() { | ||
String username = "[email protected]"; | ||
|
||
when(mockedApiUserService.markUserRolesAndFacilitiesAsDeleted(username)) | ||
.thenThrow(new NonexistentUserException()); | ||
|
||
assertThrows( | ||
NonexistentUserException.class, | ||
() -> { | ||
userMutationResolver.markUserRolesAndFacilitiesAsDeleted(username); | ||
}); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,6 +35,7 @@ | |
import gov.cdc.usds.simplereport.test_util.SliceTestConfiguration.WithSimpleReportOrgAdminUser; | ||
import gov.cdc.usds.simplereport.test_util.SliceTestConfiguration.WithSimpleReportSiteAdminUser; | ||
import gov.cdc.usds.simplereport.test_util.TestDataFactory; | ||
import gov.cdc.usds.simplereport.test_util.TestUserIdentities; | ||
import java.util.Collections; | ||
import java.util.EnumSet; | ||
import java.util.List; | ||
|
@@ -47,7 +48,9 @@ | |
import org.springframework.beans.factory.annotation.Autowired; | ||
import org.springframework.boot.test.mock.mockito.SpyBean; | ||
import org.springframework.security.access.AccessDeniedException; | ||
import org.springframework.test.context.TestPropertySource; | ||
|
||
@TestPropertySource(properties = {"spring.jpa.properties.hibernate.enable_lazy_load_no_trans=true"}) | ||
class ApiUserServiceTest extends BaseServiceTest<ApiUserService> { | ||
|
||
@Autowired @SpyBean ApiUserRepository _apiUserRepo; | ||
|
@@ -564,6 +567,48 @@ void updateUserPrivilegesAndGroupAccess_facilityToMoveNotFoundInOrg_throwsExcept | |
assertEquals(expectedError, caught.getMessage()); | ||
} | ||
|
||
@Test | ||
@WithSimpleReportOrgAdminUser | ||
void orgAdminUser_markUserRolesAndFacilitiesAsDeleted_error() { | ||
String username = "[email protected]"; | ||
assertThrows( | ||
AccessDeniedException.class, () -> _service.markUserRolesAndFacilitiesAsDeleted(username)); | ||
} | ||
|
||
@Test | ||
@WithSimpleReportSiteAdminUser | ||
void siteAdminUser_markUserRolesAndFacilitiesAsDeleted_nonExistentUser_throws() { | ||
final String email = "[email protected]"; | ||
|
||
assertThrows( | ||
NonexistentUserException.class, | ||
() -> { | ||
_service.markUserRolesAndFacilitiesAsDeleted(email); | ||
}); | ||
} | ||
|
||
@Test | ||
@WithSimpleReportSiteAdminUser | ||
void siteAdminUser_markUserRolesAndFacilitiesAsDeleted_success() { | ||
initSampleData(); | ||
final String email = TestUserIdentities.STANDARD_USER; | ||
ApiUser foundUser = _apiUserRepo.findByLoginEmail(email).get(); | ||
|
||
// check initial facilities and roles | ||
int initialFacilitiesCount = foundUser.getFacilities().size(); | ||
Set<OrganizationRole> initialOrgRoles = foundUser.getRoles(); | ||
assertEquals(1, initialFacilitiesCount); | ||
assertEquals(1, initialOrgRoles.size()); | ||
assertEquals("USER", initialOrgRoles.stream().findFirst().get().getName()); | ||
|
||
ApiUser updatedUser = _service.markUserRolesAndFacilitiesAsDeleted(email); | ||
// check facilities and roles after deletion | ||
int updatedFacilitiesCount = updatedUser.getFacilities().size(); | ||
int updatedOrgRolesCount = updatedUser.getRoles().size(); | ||
assertEquals(0, updatedFacilitiesCount); | ||
assertEquals(0, updatedOrgRolesCount); | ||
} | ||
|
||
private void roleCheck(final UserInfo userInfo, final Set<OrganizationRole> expected) { | ||
EnumSet<OrganizationRole> actual = EnumSet.copyOf(userInfo.getRoles()); | ||
assertEquals(expected, actual); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only nit is can we name these
clearUserRolesandFacilities
or something like that? "mark as deleted" to me implies that there's an isDeleted flag we are flipping and that this is reversible, but we're just clearing them for goodThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do! thank you, merethe!