Repository: greyli/apiflask
Pull request #235 (2022-04-05 04:45:26)

Refactor and improve the security handling
- Improve the security handling code
- Remove the app-level auth_required detection (meaningless and not documented).
- Add tests for automatic 401 response on blueprint-level auth_required.

Checklist:

- [ ] Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
- [ ] Add or update relevant docs, in the `docs` folder and in code docstring.
- [ ] Add an entry in `CHANGES.md` summarizing the change and linking to the issue and your username.
- [ ] Add `*Version changed*` or `*Version added*` note in any relevant docs and docstring.
- [x] Run `pytest` and `tox`, no tests failed.



Patch:
diff --git a/requirements/dev.txt b/requirements/dev.txt
index 3cefc6c..e695f05 100644
--- a/requirements/dev.txt
+++ b/requirements/dev.txt
@@ -68,14 +68,14 @@ mccabe==0.6.1
     # via flake8
 mergedeep==1.3.4
     # via mkdocs
-mkdocs==1.3.0
+mkdocs==1.2.3
     # via
     #   mkdocs-autorefs
     #   mkdocs-material
     #   mkdocstrings
 mkdocs-autorefs==0.3.1
     # via mkdocstrings
-mkdocs-material==8.2.8
+mkdocs-material==8.2.3
     # via -r requirements/docs.in
 mkdocs-material-extensions==1.0.3
     # via mkdocs-material
diff --git a/requirements/docs.txt b/requirements/docs.txt
index eaec1db..974deb9 100644
--- a/requirements/docs.txt
+++ b/requirements/docs.txt
@@ -28,14 +28,14 @@ markupsafe==2.0.1
     #   mkdocstrings
 mergedeep==1.3.4
     # via mkdocs
-mkdocs==1.3.0
+mkdocs==1.2.3
     # via
     #   mkdocs-autorefs
     #   mkdocs-material
     #   mkdocstrings
 mkdocs-autorefs==0.3.1
     # via mkdocstrings
-mkdocs-material==8.2.8
+mkdocs-material==8.2.3
     # via -r requirements/docs.in
 mkdocs-material-extensions==1.0.3
     # via mkdocs-material
diff --git a/src/apiflask/app.py b/src/apiflask/app.py
index d7ca5ee..9164703 100644
--- a/src/apiflask/app.py
+++ b/src/apiflask/app.py
@@ -318,6 +318,8 @@ class APIFlask(APIScaffold, Flask):
         self.schema_name_resolver = self._schema_name_resolver
 
         self._spec: t.Optional[t.Union[dict, str]] = None
+        self._auth_blueprints: t.Dict[str, t.Dict[str, t.Any]] = {}
+
         self._register_openapi_blueprint()
         self._register_error_handlers()
 
@@ -711,6 +713,49 @@ class APIFlask(APIScaffold, Flask):
                     tags.append(tag)  # type: ignore
         return tags  # type: ignore
 
+    def _collect_security_info(self) -> t.Tuple[t.List[str], t.List[HTTPAuthType]]:
+        """Detect `auth_required` on blueprint before_request functions and view functions."""
+        # security schemes
+        auth_names: t.List[str] = []
+        auth_schemes: t.List[HTTPAuthType] = []
+
+        def _update_auth_info(auth: HTTPAuthType) -> None:
+            # update auth_schemes and auth_names
+            auth_schemes.append(auth)
+            auth_name: str = get_auth_name(auth, auth_names)
+            auth_names.append(auth_name)
+
+        # collect auth info on blueprint before_request functions
+        for blueprint_name, funcs in self.before_request_funcs.items():
+            # skip app-level before_request functions (blueprint_name is None)
+            if blueprint_name is None or \
+               not self.blueprints[blueprint_name].enable_openapi:  # type: ignore
+                continue
+            for f in funcs:
+                if hasattr(f, '_spec'):  # pragma: no cover
+                    auth = f._spec.get('auth')  # type: ignore
+                    if auth is not None and auth not in auth_schemes:
+                        self._auth_blueprints[blueprint_name] = {
+                            'auth': auth,
+                            'roles': f._spec.get('roles')  # type: ignore
+                        }
+                        _update_auth_info(auth)
+        # collect auth info on view functions
+        for rule in self.url_map.iter_rules():
+            view_func: ViewFuncType = self.view_functions[rule.endpoint]  # type: ignore
+            if hasattr(view_func, '_spec'):
+                auth = view_func._spec.get('auth')
+                if auth is not None and auth not in auth_schemes:
+                    _update_auth_info(auth)
+            # method views
+            if hasattr(view_func, '_method_spec'):
+                for method_spec in view_func._method_spec.values():
+                    auth = method_spec.get('auth')
+                    if auth is not None and auth not in auth_schemes:
+                        _update_auth_info(auth)
+
+        return auth_names, auth_schemes
+
     def _generate_spec(self) -> APISpec:
         """Generate the spec, return an instance of `apispec.APISpec`.
 
@@ -754,45 +799,7 @@ class APIFlask(APIScaffold, Flask):
             ma_plugin.converter.field_mapping[sqla.HyperlinkRelated] = \
                 ('string', 'url')
 
-        # security schemes
-        auth_names: t.List[str] = []
-        auth_schemes: t.List[HTTPAuthType] = []
-        auth_blueprints: t.Dict[t.Optional[str], t.Dict[str, t.Any]] = {}
-
-        def _update_auth_info(auth: HTTPAuthType) -> None:
-            # update auth_schemes and auth_names
-            auth_schemes.append(auth)
-            auth_name: str = get_auth_name(auth, auth_names)
-            auth_names.append(auth_name)
-
-        # detect auth_required on before_request functions
-        for blueprint_name, funcs in self.before_request_funcs.items():
-            if blueprint_name is not None and \
-               not self.blueprints[blueprint_name].enable_openapi:  # type: ignore
-                continue
-            for f in funcs:
-                if hasattr(f, '_spec'):  # pragma: no cover
-                    auth = f._spec.get('auth')  # type: ignore
-                    if auth is not None and auth not in auth_schemes:
-                        auth_blueprints[blueprint_name] = {
-                            'auth': auth,
-                            'roles': f._spec.get('roles')  # type: ignore
-                        }
-                        _update_auth_info(auth)
-        # collect auth info
-        for rule in self.url_map.iter_rules():
-            view_func: ViewFuncType = self.view_functions[rule.endpoint]  # type: ignore
-            if hasattr(view_func, '_spec'):
-                auth = view_func._spec.get('auth')
-                if auth is not None and auth not in auth_schemes:
-                    _update_auth_info(auth)
-            # method views
-            if hasattr(view_func, '_method_spec'):
-                for method_spec in view_func._method_spec.values():
-                    auth = method_spec.get('auth')
-                    if auth is not None and auth not in auth_schemes:
-                        _update_auth_info(auth)
-
+        auth_names, auth_schemes = self._collect_security_info()
         security, security_schemes = get_security_and_security_schemes(
             auth_names, auth_schemes
         )
@@ -980,10 +987,11 @@ class APIFlask(APIScaffold, Flask):
                     )
 
                 # add authentication error response
+                has_bp_level_auth = blueprint_name is not None and \
+                    blueprint_name in self._auth_blueprints
+                view_func_auth = view_func._spec.get('auth')
                 if self.config['AUTO_AUTH_ERROR_RESPONSE'] and \
-                   (view_func._spec.get('auth') or (
-                       blueprint_name is not None and blueprint_name in auth_blueprints
-                   )):
+                   (has_bp_level_auth or view_func_auth):
                     status_code: str = str(  # type: ignore
                         self.config['AUTH_ERROR_STATUS_CODE']
                     )
@@ -1046,24 +1054,16 @@ class APIFlask(APIScaffold, Flask):
                             'application/json']['examples'] = examples
 
                 # security
-                # app-wide auth
-                if None in auth_blueprints:
-                    operation['security'] = [{
-                        security[auth_blueprints[None]['auth']]:
-                            auth_blueprints[None]['roles']
-                    }]
-
-                # blueprint-wide auth
-                if blueprint_name is not None and blueprint_name in auth_blueprints:
+                if has_bp_level_auth:
+                    bp_auth_info = self._auth_blueprints[blueprint_name]  # type: ignore
                     operation['security'] = [{
-                        security[auth_blueprints[blueprint_name]['auth']]:
-                            auth_blueprints[blueprint_name]['roles']
+                        security[bp_auth_info['auth']]: bp_auth_info['roles']
                     }]
 
                 # view-wide auth
-                if view_func._spec.get('auth'):
+                if view_func_auth:
                     operation['security'] = [{
-                        security[view_func._spec['auth']]: view_func._spec['roles']
+                        security[view_func_auth]: view_func._spec['roles']
                     }]
 
                 operations[method.lower()] = operation


Test patch:
diff --git a/tests/test_decorator_auth_required.py b/tests/test_decorator_auth_required.py
index 2fa5c7b..be7b915 100644
--- a/tests/test_decorator_auth_required.py
+++ b/tests/test_decorator_auth_required.py
@@ -162,12 +162,13 @@ def test_auth_required_with_methodview(app, client):
 
 
 def test_auth_required_at_blueprint_before_request(app, client):
-    bp = APIBlueprint('test', __name__)
+    bp = APIBlueprint('auth', __name__)
+    no_auth_bp = APIBlueprint('no-auth', __name__)
 
     auth = HTTPTokenAuth()
 
     @bp.before_request
-    @app.auth_required(auth)
+    @bp.auth_required(auth)
     def before():
         pass
 
@@ -187,7 +188,12 @@ def test_auth_required_at_blueprint_before_request(app, client):
         def post(self):
             pass
 
+    @no_auth_bp.get('/eggs')
+    def eggs():
+        return 'no auth'
+
     app.register_blueprint(bp)
+    app.register_blueprint(no_auth_bp)
 
     rv = client.get('/foo')
     assert rv.status_code == 401
@@ -195,12 +201,16 @@ def test_auth_required_at_blueprint_before_request(app, client):
     assert rv.status_code == 401
     rv = client.get('/baz')
     assert rv.status_code == 401
-    rv = client.post('/baz')
-    assert rv.status_code == 401
+    rv = client.get('/eggs')
+    assert rv.status_code == 200
 
     rv = client.get('/openapi.json')
     assert rv.status_code == 200
     validate_spec(rv.json)
+
+    assert 'auth' in app._auth_blueprints
+    assert 'no-auth' not in app._auth_blueprints
+
     assert 'BearerAuth' in rv.json['components']['securitySchemes']
     assert rv.json['components']['securitySchemes']['BearerAuth'] == {
         'scheme': 'Bearer',
@@ -211,67 +221,4 @@ def test_auth_required_at_blueprint_before_request(app, client):
     assert 'BearerAuth' in rv.json['paths']['/bar']['get']['security'][0]
     assert 'BearerAuth' in rv.json['paths']['/baz']['get']['security'][0]
     assert 'BearerAuth' in rv.json['paths']['/baz']['post']['security'][0]
-
-
-def test_auth_required_at_app_before_request(app, client):
-    auth = HTTPBasicAuth()
-
-    @auth.verify_password
-    def verify_password(username, password):
-        if username == 'foo' and password == 'bar':
-            return {'user': 'foo'}
-
-    @app.before_request
-    @app.auth_required(auth)
-    def before():
-        pass
-
-    @app.get('/foo')
-    def foo():
-        pass
-
-    @app.route('/bar')
-    class Bar(MethodView):
-        def get(self):
-            pass
-
-        def post(self):
-            pass
-
-    bp = APIBlueprint('test', __name__)
-
-    @bp.get('/baz')
-    def baz():
-        pass
-
-    @bp.get('/eggs')
-    def eggs():
-        pass
-
-    app.register_blueprint(bp)
-
-    rv = client.get('/foo')
-    assert rv.status_code == 401
-    rv = client.get('/bar')
-    assert rv.status_code == 401
-    rv = client.post('/bar')
-    assert rv.status_code == 401
-    rv = client.get('/baz')
-    assert rv.status_code == 401
-    rv = client.get('/eggs')
-    assert rv.status_code == 401
-
-    rv = client.get('/openapi.json', headers={'Authorization': 'Basic Zm9vOmJhcg=='})
-    assert rv.status_code == 200
-    validate_spec(rv.json)
-    assert 'BasicAuth' in rv.json['components']['securitySchemes']
-    assert rv.json['components']['securitySchemes']['BasicAuth'] == {
-        'scheme': 'Basic',
-        'type': 'http'
-    }
-
-    assert 'BasicAuth' in rv.json['paths']['/foo']['get']['security'][0]
-    assert 'BasicAuth' in rv.json['paths']['/bar']['get']['security'][0]
-    assert 'BasicAuth' in rv.json['paths']['/bar']['post']['security'][0]
-    assert 'BasicAuth' in rv.json['paths']['/baz']['get']['security'][0]
-    assert 'BasicAuth' in rv.json['paths']['/eggs']['get']['security'][0]
+    assert 'security' not in rv.json['paths']['/eggs']['get']
diff --git a/tests/test_settings_auto_behaviour.py b/tests/test_settings_auto_behaviour.py
index 3a4341a..1b9b391 100644
--- a/tests/test_settings_auto_behaviour.py
+++ b/tests/test_settings_auto_behaviour.py
@@ -267,6 +267,54 @@ def test_auto_auth_error_response(app, client, config_value):
                 'content']['application/json']['schema']['$ref']
 
 
+@pytest.mark.parametrize('config_value', [True, False])
+def test_blueprint_level_auto_auth_error_response(app, client, config_value):
+    app.config['AUTO_AUTH_ERROR_RESPONSE'] = config_value
+    bp = APIBlueprint('auth', __name__)
+    no_auth_bp = APIBlueprint('no-auth', __name__)
+
+    auth = HTTPBasicAuth()
+
+    @bp.before_request
+    @bp.auth_required(auth)
+    def before():
+        pass
+
+    @bp.post('/foo')
+    def foo():
+        pass
+
+    @bp.post('/bar')
+    def bar():
+        pass
+
+    @no_auth_bp.post('/baz')
+    def baz():
+        return 'no auth'
+
+    app.register_blueprint(bp)
+    app.register_blueprint(no_auth_bp)
+
+    rv = client.get('/openapi.json')
+    assert rv.status_code == 200
+    validate_spec(rv.json)
+
+    assert 'auth' in app._auth_blueprints
+    assert 'no-auth' not in app._auth_blueprints
+
+    assert bool('401' in rv.json['paths']['/foo']['post']['responses']) is config_value
+    assert bool('401' in rv.json['paths']['/bar']['post']['responses']) is config_value
+    assert '401' not in rv.json['paths']['/baz']['post']['responses']
+    if config_value:
+        assert 'HTTPError' in rv.json['components']['schemas']
+        assert '#/components/schemas/HTTPError' in \
+            rv.json['paths']['/foo']['post']['responses']['401'][
+                'content']['application/json']['schema']['$ref']
+        assert '#/components/schemas/HTTPError' in \
+            rv.json['paths']['/bar']['post']['responses']['401'][
+                'content']['application/json']['schema']['$ref']
+
+
 @pytest.mark.parametrize('config_value', [True, False])
 def test_auto_404_error(app, client, config_value):
     app.config['AUTO_404_RESPONSE'] = config_value