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

OCD-4734: Allow revoking user access and re-inviting disabled users #1759

Open
wants to merge 19 commits into
base: staging
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
c5f994b
feat!: Support revoking org access from acb/developer users
kekey1 Dec 12, 2024
30f1e69
fix: Put back environment filtering in AdminGetUser requests to Cognito
kekey1 Dec 12, 2024
bbfd77b
feat: Allow disabled users to be re-invited through create acct workflow
kekey1 Dec 12, 2024
4dd3975
Merge branch 'staging' into OCD-4734
kekey1 Dec 13, 2024
c715b91
fix: Prevent failures getting activity if user does not exist
kekey1 Dec 13, 2024
668e475
feat: Enable logging to debug issues logging in
kekey1 Dec 13, 2024
12bf22f
feat!: Add ability to re-enable a disable user via invitation
kekey1 Dec 13, 2024
79c4eed
fix: Attempt to make retreiving users not fail if deb/acb doesn't exist
kekey1 Dec 16, 2024
d3cafba
refactor: Add some logging to debug jwt issues
kekey1 Dec 16, 2024
eada617
Merge branch 'staging' into OCD-4734
kekey1 Dec 16, 2024
d65c081
feat: Change leeway value to 30 seconds to match CHPL jwt offset
kekey1 Dec 16, 2024
368c7a2
feat: Make less db calls when loading cognito users - speeding it up?
kekey1 Dec 16, 2024
1afb8dd
feat!: Remove "includeDisabled" request param from GET user endpoints
kekey1 Dec 16, 2024
7620526
refactor: Remove commented code
kekey1 Dec 16, 2024
2b519c7
Merge branch 'staging' into OCD-4734
kekey1 Dec 19, 2024
dc4549b
Merge branch 'staging' into OCD-4734
kekey1 Jan 6, 2025
b637a97
Merge branch 'staging' into OCD-4734
kekey1 Jan 7, 2025
3e1d1fe
Merge remote-tracking branch 'upstream/staging' into OCD-4734
kekey1 Jan 8, 2025
084d1db
Merge branch 'staging' into OCD-4734
kekey1 Jan 17, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,7 @@

import java.util.List;

import org.apache.commons.lang3.BooleanUtils;
import org.apache.commons.lang3.ObjectUtils;
import org.apache.commons.lang3.StringUtils;
import org.quartz.SchedulerException;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.http.MediaType;
Expand Down Expand Up @@ -35,8 +33,6 @@
import gov.healthit.chpl.util.SwaggerSecurityRequirement;
import gov.healthit.chpl.web.controller.results.CertificationBodyResults;
import io.swagger.v3.oas.annotations.Operation;
import io.swagger.v3.oas.annotations.Parameter;
import io.swagger.v3.oas.annotations.enums.ParameterIn;
import io.swagger.v3.oas.annotations.media.Content;
import io.swagger.v3.oas.annotations.media.Schema;
import io.swagger.v3.oas.annotations.responses.ApiResponse;
Expand Down Expand Up @@ -245,15 +241,8 @@ public String deleteUserFromAcb(@PathVariable final Long acbId, @PathVariable fi
})
@RequestMapping(value = "/{acbId}/users", method = RequestMethod.GET,
produces = "application/json; charset=utf-8")
public @ResponseBody UsersResponse getUsers(@PathVariable("acbId") Long acbId,
@Parameter(description = "Whether to include users whose accounts have been marked as disabled. "
+ "Any string that can be evaluated as a boolean may be passed in (ex: true, false, off, on, yes, no). "
+ "The parameter only affects the response when called by an authenticated ADMIN or ONC user.",
allowEmptyValue = true, in = ParameterIn.QUERY, name = "includeDisabled")
@RequestParam(value = "includeDisabled", required = false, defaultValue = "false") String includeDisabled)
throws InvalidArgumentsException, EntityRetrievalException {
List<User> users = acbManager.getUsers(acbId,
StringUtils.isEmpty(includeDisabled) ? false : BooleanUtils.toBoolean(includeDisabled));
public @ResponseBody UsersResponse getUsers(@PathVariable("acbId") Long acbId) throws InvalidArgumentsException, EntityRetrievalException {
List<User> users = acbManager.getUsers(acbId);

UsersResponse results = new UsersResponse();
results.setUsers(users);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,7 @@
import java.util.List;

import org.apache.commons.collections.CollectionUtils;
import org.apache.commons.lang3.BooleanUtils;
import org.apache.commons.lang3.NotImplementedException;
import org.apache.commons.lang3.StringUtils;
import org.ff4j.FF4j;
import org.quartz.SchedulerException;
import org.springframework.beans.factory.annotation.Autowired;
Expand All @@ -18,7 +16,6 @@
import org.springframework.web.bind.annotation.RequestBody;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RequestMethod;
import org.springframework.web.bind.annotation.RequestParam;
import org.springframework.web.bind.annotation.ResponseBody;
import org.springframework.web.bind.annotation.RestController;

Expand Down Expand Up @@ -60,8 +57,6 @@
import gov.healthit.chpl.web.controller.results.DeveloperAttestationSubmissionResults;
import gov.healthit.chpl.web.controller.results.DeveloperResults;
import io.swagger.v3.oas.annotations.Operation;
import io.swagger.v3.oas.annotations.Parameter;
import io.swagger.v3.oas.annotations.enums.ParameterIn;
import io.swagger.v3.oas.annotations.security.SecurityRequirement;
import io.swagger.v3.oas.annotations.tags.Tag;
import lombok.extern.log4j.Log4j2;
Expand Down Expand Up @@ -309,15 +304,9 @@ public PermissionDeletedResponse deleteUserFromDeveloper(@PathVariable Long deve
})
@RequestMapping(value = "/{developerId}/users", method = RequestMethod.GET,
produces = "application/json; charset=utf-8")
public @ResponseBody UsersResponse getUsers(@PathVariable("developerId") Long developerId,
@Parameter(description = "Whether to include users whose accounts have been marked as disabled. "
+ "Any string that can be evaluated as a boolean may be passed in (ex: true, false, off, on, yes, no). "
+ "The parameter only affects the response when called by an authenticated ADMIN or ONC user.",
allowEmptyValue = true, in = ParameterIn.QUERY, name = "includeDisabled")
@RequestParam(value = "includeDisabled", required = false, defaultValue = "false") String includeDisabled)
throws InvalidArgumentsException, EntityRetrievalException {
List<User> domainUsers = developerManager.getAllUsersOnDeveloper(developerId,
StringUtils.isEmpty(includeDisabled) ? false : BooleanUtils.toBoolean(includeDisabled));
public @ResponseBody UsersResponse getUsers(@PathVariable("developerId") Long developerId)
throws InvalidArgumentsException, EntityRetrievalException {
List<User> domainUsers = developerManager.getAllUsersOnDeveloper(developerId);
UsersResponse results = new UsersResponse();
results.setUsers(domainUsers);
return results;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
import java.util.Set;
import java.util.UUID;

import org.apache.commons.lang3.BooleanUtils;
import org.apache.commons.lang3.NotImplementedException;
import org.apache.commons.lang3.StringUtils;
import org.ff4j.FF4j;
Expand All @@ -22,7 +21,6 @@
import org.springframework.web.bind.annotation.RequestHeader;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RequestMethod;
import org.springframework.web.bind.annotation.RequestParam;
import org.springframework.web.bind.annotation.ResponseBody;
import org.springframework.web.bind.annotation.RestController;

Expand Down Expand Up @@ -64,8 +62,6 @@
import gov.healthit.chpl.web.controller.annotation.DeprecatedApi;
import gov.healthit.chpl.web.controller.annotation.DeprecatedApiResponseFields;
import io.swagger.v3.oas.annotations.Operation;
import io.swagger.v3.oas.annotations.Parameter;
import io.swagger.v3.oas.annotations.enums.ParameterIn;
import io.swagger.v3.oas.annotations.security.SecurityRequirement;
import io.swagger.v3.oas.annotations.tags.Tag;
import lombok.Getter;
Expand Down Expand Up @@ -246,16 +242,16 @@ public void addUser(@RequestBody CreateUserFromInvitationRequest userInfo) throw
method = RequestMethod.PUT,
consumes = MediaType.APPLICATION_JSON_VALUE,
produces = "application/json; charset=utf-8")
public User updateUserDetails(@RequestBody User userInfo, @PathVariable("cognitoUserId") UUID cognitoUserId)
public User updateUserDetails(@RequestBody User user, @PathVariable("cognitoUserId") UUID cognitoUserId)
throws ValidationException, UserRetrievalException, ActivityException {
if (!ff4j.check(FeatureList.SSO)) {
throw new NotImplementedException("This method has not been implemented");
}

if (!cognitoUserId.equals(userInfo.getCognitoId())) {
if (!cognitoUserId.equals(user.getCognitoId())) {
throw new ValidationException(msgUtil.getMessage("url.body.notMatch"));
}
return cognitoUserManager.updateUser(userInfo);
return cognitoUserManager.updateUser(user);
}


Expand Down Expand Up @@ -525,18 +521,12 @@ public DeletedUser deleteUser(@PathVariable("userId") Long userId) throws UserRe
})
@RequestMapping(value = "", method = RequestMethod.GET, produces = "application/json; charset=utf-8")
@PreAuthorize("isAuthenticated()")
public @ResponseBody UsersResponse getUsers(
@Parameter(description = "Whether to include users whose accounts have been marked as disabled. "
+ "Any string that can be evaluated as a boolean may be passed in (ex: true, false, off, on, yes, no). "
+ "The parameter only affects the response when called by an authenticated ADMIN or ONC user.",
allowEmptyValue = true, in = ParameterIn.QUERY, name = "includeDisabled")
@RequestParam(value = "includeDisabled", required = false, defaultValue = "false") String includeDisabledStr) {
boolean includeDisabled = StringUtils.isEmpty(includeDisabledStr) ? false : BooleanUtils.toBoolean(includeDisabledStr);
public @ResponseBody UsersResponse getUsers() {
List<User> users = null;
if (ff4j.check(FeatureList.SSO)) {
users = getAllCognitoUsers(includeDisabled);
users = getAllCognitoUsers();
} else {
users = getAllChplUsers(includeDisabled);
users = getAllChplUsers();
}

UsersResponse response = new UsersResponse();
Expand Down Expand Up @@ -564,8 +554,8 @@ public DeletedUser deleteUser(@PathVariable("userId") Long userId) throws UserRe
return userManager.getUserInfo(id);
}

private List<User> getAllChplUsers(Boolean includeDisabled) {
List<UserDTO> userList = userManager.getAll(includeDisabled);
private List<User> getAllChplUsers() {
List<UserDTO> userList = userManager.getAll();
List<User> users = new ArrayList<User>(userList.size());

for (UserDTO userDto : userList) {
Expand All @@ -575,8 +565,8 @@ private List<User> getAllChplUsers(Boolean includeDisabled) {
return users;
}

private List<User> getAllCognitoUsers(Boolean includeDisabled) {
return cognitoUserManager.getAll(includeDisabled);
private List<User> getAllCognitoUsers() {
return cognitoUserManager.getAll();
}

private class DeletedUser {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ public JWTAuthenticatedUser getAuthenticatedUser(String jwt) throws JWTValidatio
try {
validatedClaims = jwtConsumer.consume(jwt);
} catch (InvalidJwtException e) {
LOGGER.error("Invalid JWT Exception", e);
throw new JWTValidationException("Invalid authentication token.");
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,11 @@
throw new JWTValidationException("Invalid authentication token.");
}
} catch (JWTValidationException e) {
LOGGER.error("JWT Validation Exception", e);
throw e;

} catch (Exception e) {
LOGGER.error("Error decoding JWT token", e);
return null;
}
}
Expand All @@ -74,8 +76,8 @@
RSAKeyProvider keyProvider = new CognitoRsaKeyProvider(region, userPoolId, tokenizeRsaKeyUrl);
Algorithm algorithm = Algorithm.RSA256(keyProvider);
JWTVerifier jwtVerifier = JWT.require(algorithm)
//.withAudience(clientId)
.build();
.acceptLeeway(30000) //allows for the CHPL server clock and AWS server clock to be off by 30 seconds

Check warning on line 79 in chpl/chpl-service/src/main/java/gov/healthit/chpl/auth/authentication/CognitoJwtUserConverter.java

View workflow job for this annotation

GitHub Actions / Checkstyle job

[reviewdog] reported by reviewdog 🐶 '30000' is a magic number. Raw Output: /github/workspace/./chpl/chpl-service/src/main/java/gov/healthit/chpl/auth/authentication/CognitoJwtUserConverter.java:79:31: warning: '30000' is a magic number. (com.puppycrawl.tools.checkstyle.checks.coding.MagicNumberCheck)
.build();

DecodedJWT decodedJwt = jwtVerifier.verify(jwt);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -210,18 +210,13 @@ public CertificationBody getById(Long id) throws EntityRetrievalException {
@Transactional(readOnly = true)
@PreAuthorize("@permissions.hasAccess(T(gov.healthit.chpl.permissions.Permissions).CERTIFICATION_BODY, "
+ "T(gov.healthit.chpl.permissions.domains.CertificationBodyDomainPermissions).GET_USERS, #acbId)")
public List<User> getUsers(Long acbId, boolean includeDisabled)
throws InvalidArgumentsException, EntityRetrievalException {
public List<User> getUsers(Long acbId) throws InvalidArgumentsException, EntityRetrievalException {
CertificationBody acb = resourcePermissionsFactory.get().getAcbIfPermissionById(acbId);
if (acb == null) {
throw new InvalidArgumentsException("Could not find the ACB specified.");
}

if (!resourcePermissionsFactory.get().isUserRoleAdmin()
&& !resourcePermissionsFactory.get().isUserRoleOnc()) {
includeDisabled = false;
}
List<User> users = resourcePermissionsFactory.get().getAllUsersOnAcb(acb, includeDisabled);
List<User> users = resourcePermissionsFactory.get().getAllUsersOnAcb(acb);
return users;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -245,13 +245,9 @@ private SimpleListing convertToSimpleListing(CertifiedProductDetailsDTO listingD
@Transactional(readOnly = true)
@PreAuthorize("@permissions.hasAccess(T(gov.healthit.chpl.permissions.Permissions).DEVELOPER, "
+ "T(gov.healthit.chpl.permissions.domains.DeveloperDomainPermissions).GET_ALL_USERS, #devId)")
public List<User> getAllUsersOnDeveloper(Long devId, boolean includeDisabled) throws EntityRetrievalException {
public List<User> getAllUsersOnDeveloper(Long devId) throws EntityRetrievalException {
Developer dev = getById(devId);
if (!resourcePermissionsFactory.get().isUserRoleAdmin()
&& !resourcePermissionsFactory.get().isUserRoleOnc()) {
includeDisabled = false;
}
List<User> users = resourcePermissionsFactory.get().getAllUsersOnDeveloper(dev, includeDisabled);
List<User> users = resourcePermissionsFactory.get().getAllUsersOnDeveloper(dev);
return users;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,7 @@
import java.util.Optional;
import java.util.Set;
import java.util.UUID;
import java.util.stream.Collectors;

import org.apache.commons.lang3.BooleanUtils;
import org.apache.commons.lang3.ObjectUtils;
import org.apache.commons.lang3.StringUtils;
import org.springframework.beans.factory.annotation.Autowired;
Expand Down Expand Up @@ -170,18 +168,8 @@ public void delete(UserDTO user) throws UserRetrievalException, ActivityExceptio
@Transactional
@PostFilter("@permissions.hasAccess(T(gov.healthit.chpl.permissions.Permissions).SECURED_USER, "
+ "T(gov.healthit.chpl.permissions.domains.SecuredUserDomainPermissions).GET_ALL, filterObject)")
public List<UserDTO> getAll(Boolean includeDisabled) {
if (includeDisabled == null
|| (!resourcePermissionsFactory.get().isUserRoleAdmin()
&& !resourcePermissionsFactory.get().isUserRoleOnc())) {
includeDisabled = false;
}
public List<UserDTO> getAll() {
List<UserDTO> allUsers = userDAO.findAll();
if (includeDisabled == null || BooleanUtils.isFalse(includeDisabled)) {
return allUsers.stream()
.filter(user -> user.isAccountEnabled())
.collect(Collectors.toList());
}
return allUsers;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,34 +68,20 @@ public boolean isDeveloperNotBannedOrSuspended(Long developerId) {
@Override
@Transactional(readOnly = true)
public List<User> getAllUsersOnAcb(CertificationBody acb) {
return getAllUsersOnAcb(acb, false);
}

@Override
@Transactional(readOnly = true)
public List<User> getAllUsersOnAcb(CertificationBody acb, boolean includeDisabled) {
List<UserCertificationBodyMapDTO> dtos = userCertificationBodyMapDAO.getByAcbId(acb.getId());

return dtos.stream()
.map(dto -> dto.getUser().toDomain())
.filter(user -> includeDisabled ? true : user.getAccountEnabled())
.toList();
}

@Override
@Transactional(readOnly = true)
public List<User> getAllUsersOnDeveloper(Developer dev) {
return getAllUsersOnDeveloper(dev, false);
}

@Override
@Transactional(readOnly = true)
public List<User> getAllUsersOnDeveloper(Developer dev, boolean includeDisabled) {
List<UserDeveloperMapDTO> dtos = userDeveloperMapDAO.getByDeveloperId(dev.getId());

return dtos.stream()
.map(udm -> udm.getUser().toDomain())
.filter(user -> includeDisabled ? true : user.getAccountEnabled())
.toList();
}

Expand All @@ -104,30 +90,15 @@ public List<User> getAllUsersOnDeveloper(Developer dev, boolean includeDisabled)
@PreAuthorize("@permissions.hasAccess(T(gov.healthit.chpl.permissions.Permissions).DEVELOPER, "
+ "T(gov.healthit.chpl.permissions.domains.DeveloperDomainPermissions).GET_ALL_USERS)")
public List<User> getAllDeveloperUsers() {
return getAllDeveloperUsers(false);
}

@Override
@Transactional(readOnly = true)
@PreAuthorize("@permissions.hasAccess(T(gov.healthit.chpl.permissions.Permissions).DEVELOPER, "
+ "T(gov.healthit.chpl.permissions.domains.DeveloperDomainPermissions).GET_ALL_USERS)")
public List<User> getAllDeveloperUsers(boolean includeDisabled) {
List<UserDeveloperMapDTO> dtos = userDeveloperMapDAO.getAllDeveloperUsers();
return dtos.stream()
.map(udm -> udm.getUser().toDomain())
.filter(user -> includeDisabled ? true : user.getAccountEnabled())
.toList();
}

@Override
@Transactional(readOnly = true)
public List<User> getAllUsersForCurrentUser() {
return getAllUsersForCurrentUser(false);
}

@Override
@Transactional(readOnly = true)
public List<User> getAllUsersForCurrentUser(boolean includeDisabled) {
JWTAuthenticatedUser user = AuthUtil.getCurrentUser();
List<User> users = new ArrayList<User>();

Expand Down Expand Up @@ -155,9 +126,7 @@ public List<User> getAllUsersForCurrentUser(boolean includeDisabled) {
} catch (UserRetrievalException ex) { }
}
}
return users.stream()
.filter(currUser -> includeDisabled ? true : currUser.getAccountEnabled())
.collect(Collectors.toList());
return users;
}

@Override
Expand Down
Loading
Loading