Extract security to middleware #1514

RobbeSneyders · 2022-04-19T22:35:48Z

Part of #1489.
Fixes #1120.

This PR adds the SecurityMiddleware, which checks the security for incoming requests.

Changes proposed in this pull request:

Extract security from AbstractAPI and FlaskApi to SecurityApi
Replace werkzeug exceptions with starlette.HTTPException subclasses
Remove SecureOperation base class and extract security from all operations to separate SecurityOperation class only used in middleware
Collapse all security handler factory classes into a single async SecurityHandlerFactory class.
Split security and general operation tests.

It's quite a big one, but unfortunately it wasn't possible to split this into smaller PRs that would still pass the tests.

connexion/__init__.py

RobbeSneyders · 2022-04-19T22:40:28Z

connexion/operations/abstract.py

@@ -390,6 +385,17 @@ def function(self):

        return function

+    @property
+    def _request_response_decorator(self):


Lifted from the removed SecureOperation as it's not actually related to security.

RobbeSneyders · 2022-04-19T22:41:24Z

tests/api/test_secure_api.py

@@ -36,7 +36,6 @@ def test_security(oauth_requests, secure_endpoint_app):
    assert get_bye_no_auth.status_code == 401
    assert get_bye_no_auth.content_type == 'application/problem+json'
    get_bye_no_auth_reponse = json.loads(get_bye_no_auth.data.decode('utf-8', 'replace'))  # type: dict
-    assert get_bye_no_auth_reponse['title'] == 'Unauthorized'


starlette.HTTPException does not have a title attribute.

RobbeSneyders · 2022-04-19T22:41:54Z

tests/conftest.py

-
-    monkeypatch.setattr('connexion.security.flask_security_handler_factory.session.get', fake_get)
+
+    class FakeClient:


Refactored to work with httpx instead of requests.

RobbeSneyders · 2022-04-19T22:44:51Z

tests/test_operation2.py

                                  definitions=DEFINITIONS,
                                  parameter_definitions=PARAMETER_DEFINITIONS,
                                  resolver=Resolver())
-    assert isinstance(operation.function, types.FunctionType)


Removed intermediate security checks for tests not related to security.

RobbeSneyders · 2022-04-19T22:45:32Z

tests/test_operation2.py


-    assert operation.method == 'GET'


Use SecurityOperation and remove intermediate checks not related to security for tests related to security.

RobbeSneyders · 2022-04-19T22:50:34Z

connexion/middleware/security.py

+            **kwargs
+    ):
+        super().__init__(specification, *args, **kwargs)
+        self.security_handler_factory = SecurityHandlerFactory('context')


Hardcoded pass_context_arg_name here.
Would propose to get rid of the parameter and choose a fixed name, but to do so in a follow-up PR.

connexion/middleware/security.py

coveralls · 2022-04-20T07:45:33Z

Pull Request Test Coverage Report for Build 2222571101

189 of 220 (85.91%) changed or added relevant lines in 12 files are covered.
11 unchanged lines in 2 files lost coverage.
Overall coverage increased (+1.04%) to 94.332%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
connexion/middleware/exceptions.py	5	7	71.43%
connexion/security/security_handler_factory.py	26	30	86.67%
connexion/middleware/security.py	119	144	82.64%

Files with Coverage Reduction	New Missed Lines	%
connexion/exceptions.py	1	95.12%
connexion/security/security_handler_factory.py	10	79.24%

Totals
Change from base Build 2192132070:	1.04%
Covered Lines:	2713
Relevant Lines:	2876

💛 - Coveralls

Ruwann

First review - some initial comments/questions, but still need to go over it again and have a better look at test_operation2.py

connexion/security/security_handler_factory.py

setup.py

connexion/apis/flask_api.py

connexion/exceptions.py

connexion/middleware/exceptions.py

connexion/security/security_handler_factory.py

connexion/middleware/security.py

nielsbox

Reviewed half of the PR, left some questions and nitpicks. Will do the rest this weekend.

connexion/middleware/exceptions.py

connexion/middleware/security.py

nielsbox · 2022-04-22T14:10:38Z

connexion/middleware/security.py

+        for path, methods in paths.items():
+            for method, operation in methods.items():
+                if method not in METHODS:
+                    continue


Wouldn't it make more sense to handle this more upstream when parsing the spec? If not, we should warn here that a method is being skipped.

This is validated when parsing the spec, but there are other valid keys at this level. For instance for common parameters for a path. We want to skip those.

connexion/middleware/security.py

This reverts commit c1004c7.

RobbeSneyders · 2022-04-25T20:04:08Z

Thanks @Ruwann @nielsbox, all comments should be addressed.

RobbeSneyders commented Apr 19, 2022

View reviewed changes

RobbeSneyders force-pushed the feature/security-middleware branch from 16d4faf to 3f34999 Compare April 19, 2022 23:02

RobbeSneyders commented Apr 19, 2022

View reviewed changes

connexion/middleware/security.py Outdated Show resolved Hide resolved

RobbeSneyders force-pushed the feature/security-middleware branch from 3f34999 to 2ef49d7 Compare April 20, 2022 07:15

Extract security to middleware

e5dfc52

RobbeSneyders force-pushed the feature/security-middleware branch from 2ef49d7 to e5dfc52 Compare April 20, 2022 07:37

RobbeSneyders requested review from Ruwann and nielsbox April 20, 2022 07:46

RobbeSneyders mentioned this pull request Apr 20, 2022

Move functionality into pluggable ASGI middleware stack #1489

Closed

6 tasks

Ruwann reviewed Apr 20, 2022

View reviewed changes

nielsbox reviewed Apr 22, 2022

View reviewed changes

nielsbox reviewed Apr 24, 2022

View reviewed changes

connexion/middleware/security.py Outdated Show resolved Hide resolved

RobbeSneyders added 11 commits April 25, 2022 21:37

Add MissingMiddleware exception

42444bb

Extract add_auth_on_not_found in security middleware

0d8db57

Add detail kwarg to exceptions

90fba11

Return JSONResponse in ExceptionMiddleware

c1004c7

Type dicts in SecurityMiddleware

8b9031f

Add trailing comma in setup.py

056e9f5

Rework connexion context

493c982

Improve error handling for missing security operation

785808d

Refactor default security operation

9cc0da0

Revert "Return JSONResponse in ExceptionMiddleware"

edfeb27

This reverts commit c1004c7.

Move routing context name into constant

d20ef93

RobbeSneyders force-pushed the feature/security-middleware branch from c1f076d to d20ef93 Compare April 25, 2022 20:04

nielsbox approved these changes Apr 25, 2022

View reviewed changes

RobbeSneyders merged commit 4603e06 into main Apr 27, 2022

RobbeSneyders deleted the feature/security-middleware branch April 27, 2022 20:20

RobbeSneyders mentioned this pull request May 10, 2022

Clean up operation classes #1535

Merged

RobbeSneyders mentioned this pull request Jun 11, 2022

Use resolver in security middleware #1553

Merged

RobbeSneyders mentioned this pull request Feb 18, 2023

Security decorator should log client IP address #410

Closed

renovate bot mentioned this pull request Sep 27, 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

Extract security to middleware #1514

Extract security to middleware #1514

RobbeSneyders commented Apr 19, 2022

RobbeSneyders Apr 19, 2022

RobbeSneyders Apr 19, 2022

RobbeSneyders Apr 19, 2022

RobbeSneyders Apr 19, 2022

RobbeSneyders Apr 19, 2022

RobbeSneyders Apr 19, 2022

coveralls commented Apr 20, 2022 •

edited

Loading

Ruwann left a comment

nielsbox left a comment •

edited

Loading

nielsbox Apr 22, 2022

RobbeSneyders Apr 25, 2022

RobbeSneyders commented Apr 25, 2022


		monkeypatch.setattr('connexion.security.flask_security_handler_factory.session.get', fake_get)

		class FakeClient:

Extract security to middleware #1514

Extract security to middleware #1514

Conversation

RobbeSneyders commented Apr 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 20, 2022 • edited Loading

Pull Request Test Coverage Report for Build 2222571101

💛 - Coveralls

Ruwann left a comment

Choose a reason for hiding this comment

nielsbox left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RobbeSneyders commented Apr 25, 2022

coveralls commented Apr 20, 2022 •

edited

Loading

nielsbox left a comment •

edited

Loading