Rework required_scopes checking (#1474)

* WIP: rework required_scopes checking

* Update tests for security scopes

* Add test for oauth security scheme with multiple possible scopes

* Update security tests

* Change optional auth test to correct behaviour

* Update security documentation

* Remove TODOs

* Catch possible exceptions from failed checks in async security factory

* Add .venv/ to gitignore

* Try to raise most specific exception

* Add test for raising most specific error

* Update async security handler factory

* Fix security handler error catching

* Fix imports order

.gitignore

@@ -13,5 +13,6 @@ htmlcov/
 .idea/
 .vscode/
 venv/
+.venv/
 src/
 *.un~

connexion/operations/secure.py

@@ -75,7 +75,6 @@ class SecureOperation:
             return self._api.security_handler_factory.security_passthrough
 
         auth_funcs = []
-        required_scopes = None
         for security_req in self.security:
             if not security_req:
                 auth_funcs.append(self._api.security_handler_factory.verify_none())
@@ -83,7 +82,7 @@ class SecureOperation:
 
             sec_req_funcs = {}
             oauth = False
-            for scheme_name, scopes in security_req.items():
+            for scheme_name, required_scopes in security_req.items():
                 security_scheme = self.security_schemes[scheme_name]
 
                 if security_scheme['type'] == 'oauth2':
@@ -91,7 +90,6 @@ class SecureOperation:
                         logger.warning("... multiple OAuth2 security schemes in AND fashion not supported", extra=vars(self))
                         break
                     oauth = True
-                    required_scopes = scopes
                     token_info_func = self._api.security_handler_factory.get_tokeninfo_func(security_scheme)
                     scope_validate_func = self._api.security_handler_factory.get_scope_validate_func(security_scheme)
                     if not token_info_func:
@@ -99,7 +97,7 @@ class SecureOperation:
                         break
 
                     sec_req_funcs[scheme_name] = self._api.security_handler_factory.verify_oauth(
-                        token_info_func, scope_validate_func)
+                        token_info_func, scope_validate_func, required_scopes)
 
                 # Swagger 2.0
                 elif security_scheme['type'] == 'basic':
@@ -159,7 +157,7 @@ class SecureOperation:
                 else:
                     auth_funcs.append(self._api.security_handler_factory.verify_multiple_schemes(sec_req_funcs))
 
-        return functools.partial(self._api.security_handler_factory.verify_security, auth_funcs, required_scopes)
+        return functools.partial(self._api.security_handler_factory.verify_security, auth_funcs)
 
     def get_mimetype(self):
         return DEFAULT_MIMETYPE

connexion/security/async_security_handler_factory.py

@@ -62,20 +62,27 @@ class AbstractAsyncSecurityHandlerFactory(AbstractSecurityHandlerFactory):
         return wrapper
 
     @classmethod
-    def verify_security(cls, auth_funcs, required_scopes, function):
+    def verify_security(cls, auth_funcs, function):
         @functools.wraps(function)
         async def wrapper(request):
             token_info = cls.no_value
+            errors = []
             for func in auth_funcs:
-                token_info = func(request, required_scopes)
+                try:
-                while asyncio.iscoroutine(token_info):
+                    token_info = func(request)
-                    token_info = await token_info
+                    while asyncio.iscoroutine(token_info):
-                if token_info is not cls.no_value:
+                        token_info = await token_info
-                    break
+                    if token_info is not cls.no_value:
+                        break
+                except Exception as err:
+                    errors.append(err)
 
             if token_info is cls.no_value:
-                logger.info("... No auth provided. Aborting with 401.")
+                if errors != []:
-                raise OAuthProblem(description='No authorization token provided')
+                    cls._raise_most_specific(errors)
+                else:
+                    logger.info("... No auth provided. Aborting with 401.")
+                    raise OAuthProblem(description='No authorization token provided')
 
             # Fallback to 'uid' for backward compatibility
             request.context['user'] = token_info.get('sub', token_info.get('uid'))

connexion/security/security_handler_factory.py

@@ -173,10 +173,10 @@ class AbstractSecurityHandlerFactory(abc.ABC):
             raise OAuthProblem(description='Invalid authorization header')
         return auth_type.lower(), value
 
-    def verify_oauth(self, token_info_func, scope_validate_func):
+    def verify_oauth(self, token_info_func, scope_validate_func, required_scopes):
         check_oauth_func = self.check_oauth_func(token_info_func, scope_validate_func)
 
-        def wrapper(request, required_scopes):
+        def wrapper(request):
             auth_type, token = self.get_auth_header_value(request)
             if auth_type != 'bearer':
                 return self.no_value
@@ -188,7 +188,7 @@ class AbstractSecurityHandlerFactory(abc.ABC):
     def verify_basic(self, basic_info_func):
         check_basic_info_func = self.check_basic_auth(basic_info_func)
 
-        def wrapper(request, required_scopes):
+        def wrapper(request):
             auth_type, user_pass = self.get_auth_header_value(request)
             if auth_type != 'basic':
                 return self.no_value
@@ -198,7 +198,7 @@ class AbstractSecurityHandlerFactory(abc.ABC):
             except Exception:
                 raise OAuthProblem(description='Invalid authorization header')
 
-            return check_basic_info_func(request, username, password, required_scopes=required_scopes)
+            return check_basic_info_func(request, username, password)
 
         return wrapper
 
@@ -221,7 +221,7 @@ class AbstractSecurityHandlerFactory(abc.ABC):
     def verify_api_key(self, api_key_info_func, loc, name):
         check_api_key_func = self.check_api_key(api_key_info_func)
 
-        def wrapper(request, required_scopes):
+        def wrapper(request):
 
             def _immutable_pop(_dict, key):
                 """
@@ -252,7 +252,7 @@ class AbstractSecurityHandlerFactory(abc.ABC):
             if api_key is None:
                 return self.no_value
 
-            return check_api_key_func(request, api_key, required_scopes=required_scopes)
+            return check_api_key_func(request, api_key)
 
         return wrapper
 
@@ -263,11 +263,11 @@ class AbstractSecurityHandlerFactory(abc.ABC):
         """
         check_bearer_func = self.check_bearer_token(token_info_func)
 
-        def wrapper(request, required_scopes):
+        def wrapper(request):
             auth_type, token = self.get_auth_header_value(request)
             if auth_type != 'bearer':
                 return self.no_value
-            return check_bearer_func(request, token, required_scopes=required_scopes)
+            return check_bearer_func(request, token)
 
         return wrapper
 
@@ -281,10 +281,10 @@ class AbstractSecurityHandlerFactory(abc.ABC):
         :rtype: types.FunctionType
         """
 
-        def wrapper(request, required_scopes):
+        def wrapper(request):
             token_info = {}
             for scheme_name, func in schemes.items():
-                result = func(request, required_scopes)
+                result = func(request)
                 if result is self.no_value:
                     return self.no_value
                 token_info[scheme_name] = result
@@ -299,7 +299,7 @@ class AbstractSecurityHandlerFactory(abc.ABC):
         :rtype: types.FunctionType
         """
 
-        def wrapper(request, required_scopes):
+        def wrapper(request):
             return {}
 
         return wrapper
@@ -362,18 +362,25 @@ class AbstractSecurityHandlerFactory(abc.ABC):
         return wrapper
 
     @classmethod
-    def verify_security(cls, auth_funcs, required_scopes, function):
+    def verify_security(cls, auth_funcs, function):
         @functools.wraps(function)
         def wrapper(request):
             token_info = cls.no_value
+            errors = []
             for func in auth_funcs:
-                token_info = func(request, required_scopes)
+                try:
-                if token_info is not cls.no_value:
+                    token_info = func(request)
-                    break
+                    if token_info is not cls.no_value:
+                        break
+                except Exception as err:
+                    errors.append(err)
 
             if token_info is cls.no_value:
-                logger.info("... No auth provided. Aborting with 401.")
+                if errors != []:
-                raise OAuthProblem(description='No authorization token provided')
+                    cls._raise_most_specific(errors)
+                else:
+                    logger.info("... No auth provided. Aborting with 401.")
+                    raise OAuthProblem(description='No authorization token provided')
 
             # Fallback to 'uid' for backward compatibility
             request.context['user'] = token_info.get('sub', token_info.get('uid'))
@@ -382,6 +389,37 @@ class AbstractSecurityHandlerFactory(abc.ABC):
 
         return wrapper
 
+    @staticmethod
+    def _raise_most_specific(exceptions: t.List[Exception]) -> None:
+        """Raises the most specific error from a list of exceptions by status code.
+
+        The status codes are expected to be either in the `code`
+        or in the `status` attribute of the exceptions.
+
+        The order is as follows:
+            - 403: valid credentials but not enough privileges
+            - 401: no or invalid credentials
+            - for other status codes, the smallest one is selected
+
+        :param errors: List of exceptions.
+        :type errors: t.List[Exception]
+        """
+        if not exceptions:
+            return
+        # We only use status code attributes from exceptions
+        # We use 600 as default because 599 is highest valid status code
+        status_to_exc = {
+            getattr(exc, 'code', getattr(exc, 'status', 600)): exc
+            for exc in exceptions
+        }
+        if 403 in status_to_exc:
+            raise status_to_exc[403]
+        elif 401 in status_to_exc:
+            raise status_to_exc[401]
+        else:
+            lowest_status_code = min(status_to_exc)
+            raise status_to_exc[lowest_status_code]
+
     @abc.abstractmethod
     def get_token_info_remote(self, token_info_url):
         """

docs/security.rst

@@ -57,11 +57,7 @@ Basic Authentication
 With Connexion, the API security definition **must** include a
 ``x-basicInfoFunc`` or set ``BASICINFO_FUNC`` env var. It uses the same
 semantics as for ``x-tokenInfoFunc``, but the function accepts three
-parameters: username, password and required_scopes. If the security declaration
+parameters: username, password and required_scopes.
-of the operation also has an oauth security requirement, required_scopes is
-taken from there, otherwise it's None. This allows authorizing individual
-operations with `oauth scope`_ while using basic authentication for
-authentication.
 
 You can find a `minimal Basic Auth example application`_ in Connexion's "examples" folder.
 

tests/api/test_secure_api.py

@@ -96,7 +96,8 @@ def test_security(oauth_requests, secure_endpoint_app):
     assert response.data == b'"Authenticated"\n'
     headers = {"X-AUTH": "wrong-key"}
     response = app_client.get('/v1.0/optional-auth', headers=headers)  # type: flask.Response
-    assert response.status_code == 401
+    assert response.data == b'"Unauthenticated"\n'
+    assert response.status_code == 200
 
 
 def test_checking_that_client_token_has_all_necessary_scopes(

tests/decorators/test_security.py

@@ -3,7 +3,8 @@ from unittest.mock import MagicMock
 
 import pytest
 import requests
-from connexion.exceptions import (OAuthProblem, OAuthResponseProblem,
+from connexion.exceptions import (BadRequestProblem, ConnexionException,
+                                  OAuthProblem, OAuthResponseProblem,
                                   OAuthScopeProblem)
 
 
@@ -34,12 +35,12 @@ def test_verify_oauth_missing_auth_header(security_handler_factory):
     def somefunc(token):
         return None
 
-    wrapped_func = security_handler_factory.verify_oauth(somefunc, security_handler_factory.validate_scope)
+    wrapped_func = security_handler_factory.verify_oauth(somefunc, security_handler_factory.validate_scope, ['admin'])
 
     request = MagicMock()
     request.headers = {}
 
-    assert wrapped_func(request, ['admin']) is security_handler_factory.no_value
+    assert wrapped_func(request) is security_handler_factory.no_value
 
 
 def test_verify_oauth_scopes_remote(monkeypatch, security_handler_factory):
@@ -52,7 +53,7 @@ def test_verify_oauth_scopes_remote(monkeypatch, security_handler_factory):
         return tokeninfo_response
 
     token_info_func = security_handler_factory.get_tokeninfo_func({'x-tokenInfoUrl': 'https://example.org/tokeninfo'})
-    wrapped_func = security_handler_factory.verify_oauth(token_info_func, security_handler_factory.validate_scope)
+    wrapped_func = security_handler_factory.verify_oauth(token_info_func, security_handler_factory.validate_scope, ['admin'])
 
     request = MagicMock()
     request.headers = {"Authorization": "Bearer 123"}
@@ -62,30 +63,30 @@ def test_verify_oauth_scopes_remote(monkeypatch, security_handler_factory):
     monkeypatch.setattr('connexion.security.flask_security_handler_factory.session', session)
 
     with pytest.raises(OAuthScopeProblem, match="Provided token doesn't have the required scope"):
-        wrapped_func(request, ['admin'])
+        wrapped_func(request)
 
     tokeninfo["scope"] += " admin"
-    assert wrapped_func(request, ['admin']) is not None
+    assert wrapped_func(request) is not None
 
     tokeninfo["scope"] = ["foo", "bar"]
     with pytest.raises(OAuthScopeProblem, match="Provided token doesn't have the required scope"):
-        wrapped_func(request, ['admin'])
+        wrapped_func(request)
 
     tokeninfo["scope"].append("admin")
-    assert wrapped_func(request, ['admin']) is not None
+    assert wrapped_func(request) is not None
 
 
 def test_verify_oauth_invalid_local_token_response_none(security_handler_factory):
     def somefunc(token):
         return None
 
-    wrapped_func = security_handler_factory.verify_oauth(somefunc, security_handler_factory.validate_scope)
+    wrapped_func = security_handler_factory.verify_oauth(somefunc, security_handler_factory.validate_scope, ['admin'])
 
     request = MagicMock()
     request.headers = {"Authorization": "Bearer 123"}
 
     with pytest.raises(OAuthResponseProblem):
-        wrapped_func(request, ['admin'])
+        wrapped_func(request)
 
 
 def test_verify_oauth_scopes_local(security_handler_factory):
@@ -94,23 +95,23 @@ def test_verify_oauth_scopes_local(security_handler_factory):
     def token_info(token):
         return tokeninfo
 
-    wrapped_func = security_handler_factory.verify_oauth(token_info, security_handler_factory.validate_scope)
+    wrapped_func = security_handler_factory.verify_oauth(token_info, security_handler_factory.validate_scope, ['admin'])
 
     request = MagicMock()
     request.headers = {"Authorization": "Bearer 123"}
 
     with pytest.raises(OAuthScopeProblem, match="Provided token doesn't have the required scope"):
-        wrapped_func(request, ['admin'])
+        wrapped_func(request)
 
     tokeninfo["scope"] += " admin"
-    assert wrapped_func(request, ['admin']) is not None
+    assert wrapped_func(request) is not None
 
     tokeninfo["scope"] = ["foo", "bar"]
     with pytest.raises(OAuthScopeProblem, match="Provided token doesn't have the required scope"):
-        wrapped_func(request, ['admin'])
+        wrapped_func(request)
 
     tokeninfo["scope"].append("admin")
-    assert wrapped_func(request, ['admin']) is not None
+    assert wrapped_func(request) is not None
 
 
 def test_verify_basic_missing_auth_header(security_handler_factory):
@@ -122,7 +123,7 @@ def test_verify_basic_missing_auth_header(security_handler_factory):
     request = MagicMock()
     request.headers = {"Authorization": "Bearer 123"}
 
-    assert wrapped_func(request, ['admin']) is security_handler_factory.no_value
+    assert wrapped_func(request) is security_handler_factory.no_value
 
 
 def test_verify_basic(security_handler_factory):
@@ -136,7 +137,7 @@ def test_verify_basic(security_handler_factory):
     request = MagicMock()
     request.headers = {"Authorization": 'Basic Zm9vOmJhcg=='}
 
-    assert wrapped_func(request, ['admin']) is not None
+    assert wrapped_func(request) is not None
 
 
 def test_verify_apikey_query(security_handler_factory):
@@ -150,7 +151,7 @@ def test_verify_apikey_query(security_handler_factory):
     request = MagicMock()
     request.query = {"auth": 'foobar'}
 
-    assert wrapped_func(request, ['admin']) is not None
+    assert wrapped_func(request) is not None
 
 
 def test_verify_apikey_header(security_handler_factory):
@@ -164,7 +165,7 @@ def test_verify_apikey_header(security_handler_factory):
     request = MagicMock()
     request.headers = {"X-Auth": 'foobar'}
 
-    assert wrapped_func(request, ['admin']) is not None
+    assert wrapped_func(request) is not None
 
 
 def test_multiple_schemes(security_handler_factory):
@@ -189,12 +190,12 @@ def test_multiple_schemes(security_handler_factory):
     request = MagicMock()
     request.headers = {"X-Auth-1": 'foobar'}
 
-    assert wrapped_func(request, ['admin']) is security_handler_factory.no_value
+    assert wrapped_func(request) is security_handler_factory.no_value
 
     request = MagicMock()
     request.headers = {"X-Auth-2": 'bar'}
 
-    assert wrapped_func(request, ['admin']) is security_handler_factory.no_value
+    assert wrapped_func(request) is security_handler_factory.no_value
 
     # Supplying both keys does succeed
     request = MagicMock()
@@ -207,16 +208,33 @@ def test_multiple_schemes(security_handler_factory):
         'key1': {'sub': 'foo'},
         'key2': {'sub': 'bar'},
     }
-    assert wrapped_func(request, ['admin']) == expected_token_info
+    assert wrapped_func(request) == expected_token_info
 
 
 def test_verify_security_oauthproblem(security_handler_factory):
     """Tests whether verify_security raises an OAuthProblem if there are no auth_funcs."""
     func_to_secure = MagicMock(return_value='func')
-    secured_func = security_handler_factory.verify_security([], [], func_to_secure)
+    secured_func = security_handler_factory.verify_security([], func_to_secure)
 
     request = MagicMock()
     with pytest.raises(OAuthProblem) as exc_info:
         secured_func(request)
 
     assert str(exc_info.value) == '401 Unauthorized: No authorization token provided'
+
+@pytest.mark.parametrize(
+    'errors, most_specific',
+    [
+        ([OAuthProblem()], OAuthProblem),
+        ([OAuthProblem(), OAuthScopeProblem([], [])], OAuthScopeProblem),
+        ([OAuthProblem(), OAuthScopeProblem([], []), BadRequestProblem], OAuthScopeProblem),
+        ([OAuthProblem(), OAuthScopeProblem([], []), BadRequestProblem, ConnexionException], OAuthScopeProblem),
+        ([BadRequestProblem(), ConnexionException()], BadRequestProblem),
+        ([ConnexionException()], ConnexionException),
+    ]
+)
+def test_raise_most_specific(errors, most_specific, security_handler_factory):
+    """Tests whether most specific exception is raised from a list."""
+
+    with pytest.raises(most_specific):
+        security_handler_factory._raise_most_specific(errors)

tests/test_operation2.py

@@ -215,6 +215,11 @@ OPERATION10 = {'description': 'operation secured with 2 oauth schemes combined u
                 'responses': {'200': {'description': 'OK'}},
                 'security': [{'oauth_1': ['uid'], 'oauth_2': ['uid']}]}
 
+OPERATION11 = {'description': 'operation secured with an oauth schemes with 2 possible scopes (in OR)',
+                'operationId': 'fakeapi.hello.post_greeting',
+                'responses': {'200': {'description': 'OK'}},
+                'security': [{'oauth': ['myscope']}, {'oauth': ['myscope2']}]}
+
 SECURITY_DEFINITIONS_REMOTE = {'oauth': {'type': 'oauth2',
                                          'flow': 'password',
                                          'x-tokenInfoUrl': 'https://oauth.example/token_info',
@@ -223,7 +228,8 @@ SECURITY_DEFINITIONS_REMOTE = {'oauth': {'type': 'oauth2',
 SECURITY_DEFINITIONS_LOCAL = {'oauth': {'type': 'oauth2',
                                         'flow': 'password',
                                         'x-tokenInfoFunc': 'math.ceil',
-                                        'scopes': {'myscope': 'can do stuff'}}}
+                                        'scopes': {'myscope': 'can do stuff',
+                                                   'myscope2': 'can do other stuff'}}}
 
 SECURITY_DEFINITIONS_BOTH = {'oauth': {'type': 'oauth2',
                                        'flow': 'password',
@@ -296,8 +302,7 @@ def test_operation(api, security_handler_factory):
     security_decorator = operation.security_decorator
     assert len(security_decorator.args[0]) == 1
     assert security_decorator.args[0][0] == 'verify_oauth_result'
-    assert security_decorator.args[1] == ['uid']
+    verify_oauth.assert_called_with('get_token_info_remote_result', security_handler_factory.validate_scope, ['uid'])
-    verify_oauth.assert_called_with('get_token_info_remote_result',security_handler_factory.validate_scope)
     security_handler_factory.get_token_info_remote.assert_called_with('https://oauth.example/token_info')
 
     assert operation.method == 'GET'
@@ -384,8 +389,7 @@ def test_operation_local_security_oauth2(api):
     security_decorator = operation.security_decorator
     assert len(security_decorator.args[0]) == 1
     assert security_decorator.args[0][0] == 'verify_oauth_result'
-    assert security_decorator.args[1] == ['uid']
+    verify_oauth.assert_called_with(math.ceil, api.security_handler_factory.validate_scope, ['uid'])
-    verify_oauth.assert_called_with(math.ceil, api.security_handler_factory.validate_scope)
 
     assert operation.method == 'GET'
     assert operation.produces == ['application/json']
@@ -418,8 +422,7 @@ def test_operation_local_security_duplicate_token_info(api):
     security_decorator = operation.security_decorator
     assert len(security_decorator.args[0]) == 1
     assert security_decorator.args[0][0] == 'verify_oauth_result'
-    assert security_decorator.args[1] == ['uid']
+    verify_oauth.call_args.assert_called_with(math.ceil, api.security_handler_factory.validate_scope, ['uid'])
-    verify_oauth.call_args.assert_called_with(math.ceil, api.security_handler_factory.validate_scope)
 
     assert operation.method == 'GET'
     assert operation.produces == ['application/json']
@@ -514,7 +517,6 @@ def test_multiple_security_schemes_and(api):
     security_decorator = operation.security_decorator
     assert len(security_decorator.args[0]) == 1
     assert security_decorator.args[0][0] == 'verify_multiple_result'
-    assert security_decorator.args[1] is None
 
     assert operation.method == 'GET'
     assert operation.produces == ['application/json']
@@ -548,7 +550,6 @@ def test_multiple_oauth_in_and(api, caplog):
     security_decorator = operation.security_decorator
     assert len(security_decorator.args[0]) == 0
     assert security_decorator.args[0] == []
-    assert security_decorator.args[1] == ['uid']
 
     assert '... multiple OAuth2 security schemes in AND fashion not supported' in caplog.text
 
@@ -617,3 +618,37 @@ def test_get_path_parameter_types(api):
     )
 
     assert {'int_path': 'int', 'string_path': 'string', 'path_path': 'path'} == operation.get_path_parameter_types()
+
+
+def test_oauth_scopes_in_or(api):
+    """Tests whether an OAuth security scheme with 2 different possible scopes is correctly handled."""
+    verify_oauth = mock.MagicMock(return_value='verify_oauth_result')
+    api.security_handler_factory.verify_oauth = verify_oauth
+
+    op_spec = make_operation(OPERATION11)
+    operation = Swagger2Operation(api=api,
+                                  method='GET',
+                                  path='endpoint',
+                                  path_parameters=[],
+                                  operation=op_spec,
+                                  app_produces=['application/json'],
+                                  app_consumes=['application/json'],
+                                  app_security=[],
+                                  security_definitions=SECURITY_DEFINITIONS_LOCAL,
+                                  definitions=DEFINITIONS,
+                                  parameter_definitions=PARAMETER_DEFINITIONS,
+                                  resolver=Resolver())
+    assert isinstance(operation.function, types.FunctionType)
+    security_decorator = operation.security_decorator
+    assert len(security_decorator.args[0]) == 2
+    assert security_decorator.args[0][0] == 'verify_oauth_result'
+    assert security_decorator.args[0][1] == 'verify_oauth_result'
+    verify_oauth.assert_has_calls([
+        mock.call(math.ceil, api.security_handler_factory.validate_scope, ['myscope']),
+        mock.call(math.ceil, api.security_handler_factory.validate_scope, ['myscope2']),
+    ])
+
+    assert operation.method == 'GET'
+    assert operation.produces == ['application/json']
+    assert operation.consumes == ['application/json']
+    assert operation.security == [{'oauth': ['myscope']}, {'oauth': ['myscope2']}]