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,31 +75,29 @@ def security_decorator(self):
             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())
                 continue
 
             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':
                     if oauth:
                         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:
                         logger.warning("... x-tokenInfoFunc missing", extra=vars(self))
                         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 @@ def security_decorator(self):
                 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 @@ async def wrapper(request, token, required_scopes):
         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)
-                while asyncio.iscoroutine(token_info):
-                    token_info = await token_info
-                if token_info is not cls.no_value:
-                    break
+                try:
+                    token_info = func(request)
+                    while asyncio.iscoroutine(token_info):
+                        token_info = await token_info
+                    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.")
-                raise OAuthProblem(description='No authorization token provided')
+                if errors != []:
+                    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 @@ def get_auth_header_value(request):
             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 @@ def wrapper(request, required_scopes):
     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 @@ def wrapper(request, required_scopes):
             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 @@ def get_cookie_value(cookies, name):
     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 @@ def _immutable_pop(_dict, key):
             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 @@ def verify_bearer(self, token_info_func):
         """
         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 @@ def verify_multiple_schemes(self, schemes):
         :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 @@ def verify_none():
         :rtype: types.FunctionType
         """
 
-        def wrapper(request, required_scopes):
+        def wrapper(request):
             return {}
 
         return wrapper
@@ -362,18 +362,25 @@ def wrapper(request, token, required_scopes):
         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)
-                if token_info is not cls.no_value:
-                    break
+                try:
+                    token_info = func(request)
+                    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.")
-                raise OAuthProblem(description='No authorization token provided')
+                if errors != []:
+                    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 @@ def wrapper(request):
 
         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
-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.
+parameters: username, password and required_scopes.
 
 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(