cph-cachet/carp-webservices-spring

Study details and status endpoints throw 403 error instead of 400 when querying a study that doesn't exist

Closed this issue · 4 comments

This is related to the recent change to use claims in the token to look up if a researcher has access to the study they are querying. It seems like the checks for whether the user has the required claim is run before checking if the study exists at all, so we get a 403 error. This is less useful for error handling - it now says "Permission denied" instead of something like "the study doesn't exist"

Yes, study existence check happens at service layer; Claim check happens at controller layer, which is before service layer. We should either find a way to check study existence without using a lot of resources and also enforce modulith, or just combine the error message to "study doesn't exist or permission denied"

@yuanchen233 will tell @davidscavnicky how to fix this.

Suggested solution:

1.Remove the @PreAuthorize annotation from the getParticipantGroupStatus method.
2.Check if the study exists before performing the authorization check.
3.Return a 400 error if the study does not exist, otherwise proceed with the authorization (check the identity of the user) using the canManageStudy function from authenticationService.
4.If the user does not have permission, return a 403 error.

it's a bad practice to inject CoreParticipantRepository with ProxiesMethodSecurityExpressionRoot(participantRepository, authenticationService)

ProxiesMethodSecurity is a part of SecurityConfig class, therefore I think its best to call a autherizationService and calling it from the service layer to check canManageStudy()

We decided not to change the endpoints but instead make a better textual description in the error message.