Make security pluggable #1671

Ruwann · 2023-03-07T20:00:10Z

Make security pluggable

Solution for standard security handlers: security_deny, security_passthrough, verify_none
HTTP security handlers & overlap with basic from swagger 2
Do we need a separate handler for each oauth2 flow?

Ruwann

The image should also be deleted

connexion/security.py

coveralls · 2023-03-07T20:16:11Z

Pull Request Test Coverage Report for Build 4645803828

185 of 206 (89.81%) changed or added relevant lines in 5 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+0.5%) to 93.859%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
connexion/middleware/request_validation.py	13	14	92.86%
connexion/security.py	156	176	88.64%

Files with Coverage Reduction	New Missed Lines	%
connexion/security.py	1	85.04%

Totals
Change from base Build 4568582322:	0.5%
Covered Lines:	3332
Relevant Lines:	3550

💛 - Coveralls

RobbeSneyders

This is starting to look great @Ruwann!

I left some comments already.

connexion/security.py

RobbeSneyders · 2023-03-23T18:35:31Z

connexion/security.py


-        return None
+    def _need_to_add_context_or_scopes(self, func):


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.

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

RobbeSneyders · 2023-03-23T19:01:24Z

connexion/security.py

+    # 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):


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.

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?

I assume a test is missing indeed. Something to check before merging the PR.

Currently the scope is not available in the SecurityHandlers though, which complicates modifying the scope. We could solve this by making the SecurityHandlers 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

connexion/security.py

connexion/lifecycle.py

RobbeSneyders

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.

connexion/middleware/main.py

connexion/middleware/request_validation.py

connexion/middleware/security.py

security.excalidraw.png

connexion/validators/parameter.py

connexion/security.py

Take them into account during validation instead of removing the security query parameters during security.

RobbeSneyders

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.

Ruwann · 2023-04-08T16:18:17Z

I added the security_map argument for Apps and APIs in this PRs. The other ones can indeed be done in separate PRs, then we can merge this one (finally :))

Follow-up items:

Add a test for the security_map
Possibly make security handlers callable
Rework context and required_scopes arguments
Add OIDC security handler
Possibly rework inner workings of ApiKeyHandler class
Extend documentation

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.

Ruwann commented Mar 7, 2023

View reviewed changes

connexion/security.py Outdated Show resolved Hide resolved

connexion/security.py Outdated Show resolved Hide resolved

RobbeSneyders reviewed Mar 23, 2023

View reviewed changes

Ruwann force-pushed the feature/pluggable-security branch 2 times, most recently from 57ecd33 to ad235fb Compare March 25, 2023 15:06

Ruwann force-pushed the feature/pluggable-security branch from f9af4bc to 3934b32 Compare April 4, 2023 16:16

Ruwann commented Apr 4, 2023

View reviewed changes

connexion/lifecycle.py Outdated Show resolved Hide resolved

Ruwann force-pushed the feature/pluggable-security branch from b278251 to 15e53cc Compare April 4, 2023 20:51

Ruwann marked this pull request as ready for review April 5, 2023 09:16

RobbeSneyders reviewed Apr 5, 2023

View reviewed changes

Ruwann and others added 2 commits April 6, 2023 19:11

Make security pluggable

3b3832b

Account for security query params in validation

f44e99c

Take them into account during validation instead of removing the security query parameters during security.

Ruwann force-pushed the feature/pluggable-security branch 2 times, most recently from 251871c to f44e99c Compare April 6, 2023 18:53

Add security_map argument to interface of Apps and APIs

f65c59c

RobbeSneyders approved these changes Apr 7, 2023

View reviewed changes

Ruwann changed the title ~~WIP: Make security pluggable~~ Make security pluggable Apr 8, 2023

Ruwann merged commit 5b4beeb into spec-first:main Apr 8, 2023

Ruwann deleted the feature/pluggable-security branch April 9, 2023 09:59

This was referenced Apr 21, 2023

Remove context keyword from security handlers #1690

Merged

Add test for security_map argument #1691

Merged

RobbeSneyders pushed a commit that referenced this pull request Apr 22, 2023

Add test for security_map argument (#1691)

e0550e6

Follow-up of #1671 Let me know if there is an easier way doing the test instead of the current "custom" basic auth.

RobbeSneyders mentioned this pull request Nov 1, 2023

Make security pluggable by authentication type #1526

Closed

renovate bot mentioned this pull request Jun 4, 2024

chore(deps): update backend chanzuckerberg/single-cell-data-portal#7096

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make security pluggable #1671

Make security pluggable #1671

Ruwann commented Mar 7, 2023 •

edited

Loading

Ruwann left a comment

coveralls commented Mar 7, 2023 •

edited

Loading

RobbeSneyders left a comment

RobbeSneyders Mar 23, 2023

Ruwann Mar 23, 2023

RobbeSneyders Mar 23, 2023

Ruwann Mar 24, 2023

RobbeSneyders Mar 26, 2023

RobbeSneyders left a comment

RobbeSneyders left a comment

Ruwann commented Apr 8, 2023 •

edited

Loading

Make security pluggable #1671

Make security pluggable #1671

Conversation

Ruwann commented Mar 7, 2023 • edited Loading

Ruwann left a comment

Choose a reason for hiding this comment

coveralls commented Mar 7, 2023 • edited Loading

Pull Request Test Coverage Report for Build 4645803828

💛 - Coveralls

RobbeSneyders left a comment

Choose a reason for hiding this comment

RobbeSneyders Mar 23, 2023

Choose a reason for hiding this comment

Ruwann Mar 23, 2023

Choose a reason for hiding this comment

RobbeSneyders Mar 23, 2023

Choose a reason for hiding this comment

Ruwann Mar 24, 2023

Choose a reason for hiding this comment

RobbeSneyders Mar 26, 2023

Choose a reason for hiding this comment

RobbeSneyders left a comment

Choose a reason for hiding this comment

RobbeSneyders left a comment

Choose a reason for hiding this comment

Ruwann commented Apr 8, 2023 • edited Loading

Ruwann commented Mar 7, 2023 •

edited

Loading

coveralls commented Mar 7, 2023 •

edited

Loading

Ruwann commented Apr 8, 2023 •

edited

Loading