Correctly handle per-operation scopes when importing from OpenAPI 3 #4756
Conversation
There was a problem hiding this comment.
@diesieben07 thank you for contributing this 😊 🙏
I noticed only after writing my comment on #4755 that you had opened a PR with a potential fix.
I wasn't aware that by the RFC we could send scopes space-delimited, so it was good to clear that app.
I just have one small comment bellow, let me know if you're up for doing the improvement, if not one of the members of team can help out and finish this PR when possible.
| @@ -44,7 +44,7 @@ | |||
| "authorizationUrl": "https://api.server.test/v1/auth", | |||
| "grantType": "authorization_code", | |||
| "redirectUrl": "{{ oauth2RedirectUrl }}", | |||
| "scope": "read:something write:something", | |||
| "scope": "", | |||
There was a problem hiding this comment.
@diesieben07 can you add 2 other endpoints where
- we only define one of these two scopes
- we define an entirely different scope (not
read:somethingandwrite:something)?
There was a problem hiding this comment.
Thank you for the review.
I have a question about what you mean by "entirely different scope". Do you want a test that covers a Schema where an endpoint specifies a scope that is not listed in the security scheme? If so, that would technically not be a valid OpenAPI Schema anymore, if I understand the specification correctly. I can add a test for it, but the question is how such a scope should be handled:
- Should it be ignored, since technically it is invalid here?
- Should it be gracefully added to this endpoint anyways, even though it is not specified in the security scheme?
There was a problem hiding this comment.
I've now updated the test to test no scopes, only one scope as well as both scopes on the endpoint level.
Please let me know how to proceed regarding my question above.
There was a problem hiding this comment.
@diesieben07 you are right, the spec would no longer be valid, and in theory in this case the authorization server would have to reply back with an invalid_scope according to spec.
The question becomes, do we want to allow folks to purposely setup invalid scopes in their specs for testing purposes... 🤷
For now the safest bet is probably the first option you mention: it should be ignored.
tagging @Kong/team-insomnia in case someone else might have another take on this.
filfreire
requested review from
jackkav,
johnwchadwick,
marckong,
dimitropoulos,
DMarby and
gatzjames
This fixes #4755.
I have also corrected the involved tests, as they were actually testing this wrong behavior.
changelog(Fixes): Fixed an issue with how OAuth2 scopes were handled when importing from OpenAPI 3 specs