Remove Duplicated Auth from API Key Check #647
Conversation
joneubank
changed the title
| @@ -12,7 +12,6 @@ | |||
| @JsonView(Views.REST.class) | |||
| public class ApiKeyScopeResponse { | |||
| private UUID user_id; | |||
| private String client_id; | |||
There was a problem hiding this comment.
not sure if removing this will break song/score.
since you still have the authorization is there a way to keep this if needed ?
There was a problem hiding this comment.
Yes, its doable with a bit of moving around where the response object is built and pulling the client ID from the auth.
The client_id value is just the application client_id that made the request. No application should need to see this when they generated the original request anyways... so I removed it because it seems to have no value and complicated building the response.
I would prefer removing any dependency on this field vs adding it back in, unless you can see a reason to provide this.
There was a problem hiding this comment.
yeah I agree that it's better to remove it I'm just wondering if it will break anything, I checked that score is using spring class (RemoteTokensService) which maybe expecting this for some reason, because it seems to be here intentionally, I just don't want to break other services, if you are sure of it no problem from my end
There was a problem hiding this comment.
Its a valid concern. Hold off on this review until I can confirm what the spring RemoteTokensService expects in the endpoint.
There was a problem hiding this comment.
This is the custom class in Score used to process the check_api_token response:
https://github.com/overture-stack/score/blob/develop/score-server/src/main/java/bio/overture/score/server/security/AccessTokenConverterWithExpiry.java#L14
The fields which are needed are scope and exp. Pensato is also using user_id. I find no references to use of client_id in these two cases. Should I also check Song?
There was a problem hiding this comment.
no I think based on this it seems that Spring won't complain, it can be tested in overture QA for final confirmation by uploading a sample file
Removed authorization check from
TokenService.checkApiKey. Auth checking is done at the request level, it should not be repeated here. See ticket for full details.