-
-
Notifications
You must be signed in to change notification settings - Fork 762
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
Rework required_scopes checking #1474
Rework required_scopes checking #1474
Conversation
8d7bdda
to
11679cc
Compare
5646a4f
to
003531b
Compare
Pull Request Test Coverage Report for Build 2006287019
💛 - Coveralls |
token_info = func(request) | ||
if token_info is not cls.no_value: | ||
break | ||
# TODO: Catch any error that might be raised instead of specific ones? |
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.
To check which errors should be caught here
ffd0212
to
7d52eb6
Compare
ac353b1
to
3cf5ffe
Compare
ac10089
to
fa54e55
Compare
4d108e5
to
ca64f85
Compare
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.
Thanks @Ruwann! LGTM.
Hi As a result of this patch, I am quite confused about the behaviour I am now seeing. We have an API with two possible security schemes,
We have two token validation functions defined, which we specify using environment variables:
Calling the GET /accounts endpoint with an OAUTH token causes
... but the behaviour is identical. |
Hi @SpudInNZ , I think there are two things here:
In theory, the order of the security schemes doesn't matter as it's an unordered mapping. However, in practice, connexion will first check the first security scheme, if that succeeds, we're done. If that fails, the second security scheme is checked. So it could have some impact depending on the performance if one function is more expensive than the other. |
@SpudInNZ Did you ever find a good solution to this? This broke my app, too. (It's a bit frustrating TBH: upgrading from an (admittedly old) 2.x.x release, I've bumped into two separate breaking changes not clearly marked as breaking (this one and #1265 which I've already worked around). Sorry for complaining about an otherwise great open source project that I got for free.) |
Hi @vashek , we try to limit the breaking changes as much as possible, but we didn't anticipate breaking behaviour from this patch. To go further on what you aim to achieve and how it is breaking your application: as discussed in the comment above, there is undefined behaviour when there are multiple security schemes for which scopes are defined. For example, security:
- oauth: [ read ]
- oauth: [ admin ]
- api_key: [] I don't see a clear answer to what should happen in this case. What would you like to happen here? |
Fixes #1423 .
Moves the required_scopes argument as current approach assumes there is one possible set of scopes per operation/security object.
I have changed the tested behaviour for this test.
If there is optional auth, if an incorrect API key is provided, the
verify_none
still passes and given that the security schemes are specified in OR fashion, the security check should pass.(For example, changing the security schemes around in the spec (link) also causes the test to fail, which shouldn't happen as the order shouldn't matter)