-
-
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
Make security pluggable #1671
Make security pluggable #1671
Conversation
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.
The image should also be deleted
Pull Request Test Coverage Report for Build 4645803828
💛 - Coveralls |
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.
This is starting to look great @Ruwann!
I left some comments already.
|
||
return None | ||
def _need_to_add_context_or_scopes(self, func): |
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.
Wondering if we should keep providing this behaviour.
- Context is now available as a global variable
- We could expect the security functions for the relevant schemes to always accept required scopes
It would remove some complexity if we would leave this out.
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.
For the context, it would make sense to keep only one way of accessing it instead of 2 ways to do the same thing.
Not sure about the required scopes:
in the past, only oauth and oidc were allowed to use scopes:
For other security scheme types, the array MUST be empty.
but in OpenAPI 3.1, all security schemes can now use the scopes:
For other security scheme types, the array MAY contain a list of role names which are required for the execution, but are not otherwise defined or exchanged in-band.
So it would depend on the signature of the security function again whether scopes need to be passed.
connexion/security.py
Outdated
# TODO: Move logic from here to `get_fn`? | ||
# that has the entire security definition, so should determine which function? | ||
# All API key checking funcs have signature func(api_key, required_scopes) -> result | ||
def _get_verify_func(self, api_key_info_func, loc, name): | ||
check_api_key_func = self.check_api_key(api_key_info_func) | ||
|
||
def wrapper(request): |
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.
Can't add a comment on the specific line below, but the API key used to be removed from the query string before passing it to the view function. I think this is necessary to prevent an unexpected keyword argument error. If we want to achieve the same behaviour, we should ensure it is removed from the scope.
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.
Very good remark, didn't think of that. I suspect this mean that we don't have a test for this now then, otherwise this test should be failing, no?
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.
I assume a test is missing indeed. Something to check before merging the PR.
Currently the scope is not available in the SecurityHandler
s though, which complicates modifying the scope. We could solve this by making the SecurityHandler
s ASGI callables, which could have some additional nice benefits.
- It would fit with the "everything is an ASGI callable" design
- It might remove some of the functional complexity we currently have, as the
SecurityHandler
would no longer need to return a function, but can just be called itself
57ecd33
to
ad235fb
Compare
f9af4bc
to
3934b32
Compare
b278251
to
15e53cc
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.
Looks great! Seems like you have cracked the nut :)
I left some smaller comments in-line.
On a higher level,
- I think the code would still benefit from making the handlers callable, even if they are not ASGI callable. There's currently still a lot of functions being passed around, which makes it harder to follow.
- It would be good to add a test for the
security_map
keyword argument.
Take them into account during validation instead of removing the security query parameters during security.
251871c
to
f44e99c
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!
This looks good to me except for
- Extending the interface of the Apps and APIs
- Adding a test for the validator_map
- Possibly making the security handlers callable
For me it's fine to merge this PR and tackle those in separate PRs though, which will make it easier to review.
I added the Follow-up items:
|
Follow-up of #1671 Let me know if there is an easier way doing the test instead of the current "custom" basic auth.
Follow-up on #1671 Since the request context is available globally, we can remove this complexity from the security handlers ("There should be one-- and preferably only one --obvious way to do it.") In addition it seems like there weren't any tests for the context in security handler functions, and I don't think there's a lot of value in a test for checking the context in security functions specifically.
Make security pluggable
security_deny
,security_passthrough
,verify_none
oauth2
flow?