Intentionally *deny* access if requested_scopes are not granted

oauthenticator/oauth2.py

@@ -18,7 +18,7 @@
 from tornado.httpclient import AsyncHTTPClient, HTTPClientError, HTTPRequest
 from tornado.httputil import url_concat
 from tornado.log import app_log
-from traitlets import Any, Bool, Dict, List, Unicode, default
+from traitlets import Any, Bool, Dict, List, Unicode, default, validate
 
 
 def guess_callback_uri(protocol, host, hub_server_url):
@@ -478,6 +478,16 @@ def _logout_redirect_url_default(self):
         """,
     )
 
+    @validate('required_scopes')
+    def _required_scopes_validation(self, proposal: list[str]):
+        # required scopes must be a subset of requested scopes
+        if set(proposal.value) - set(self.scope):
+            raise ValueError(f"Required Scopes must be a subset of Requested Scopes. {self.scope} is requested but {proposal.value} is required")
+        if self.allow_all:
+            # Can't set allow_all *and* require scopes
+            raise ValueError("Required Scopes is mutually exclusive with allow_all. Unset one of those configuration properties")
+        return proposal.value
+
     extra_authorize_params = Dict(
         config=True,
         help="""
@@ -1020,6 +1030,8 @@ async def check_allowed(self, username, auth_model):
         """
         Returns True for users allowed to be authorized
 
+        If a user must be *disallowed*, raises a 403 exception.
+
         Overrides Authenticator.check_allowed that is called from
         `Authenticator.get_authenticated_user` after
         `OAuthenticator.authenticate` has been called, and therefore also after
@@ -1037,6 +1049,19 @@ async def check_allowed(self, username, auth_model):
         if self.allow_all:
             return True
 
+        # If we specific scope grants are required, validate that they have been granted
+        # If not, we explicitly raise an exception here, since they should not be allowed
+        # *regardless* of any other config allowing them access.
+        if self.required_scopes:
+            granted_scopes = auth_model.get('auth_state', {}).get('scope', [])
+            missing_scopes = set(self.required_scopes) - set(granted_scopes)
+            if missing_scopes:
+                message = f"Denying access to user {username}. {self.scope} scopes were requested, {self.required_scopes} are required for authorization, but only {granted_scopes} were granted"
+                self.log.info(message)
+                raise web.HTTPError(403, message)
+            else:
+                return True
+
         # allow users with admin status set to True via admin_users config or
         # update_auth_model override
         if auth_model["admin"]:
@@ -1047,18 +1072,6 @@ async def check_allowed(self, username, auth_model):
         if username in self.allowed_users:
             return True
 
-        # If we specific scope grants are required, validate that they have been granted
-        if self.required_scopes:
-            granted_scopes = auth_model.get('auth_state', {}).get('scope', [])
-            missing_scopes = set(self.required_scopes) - set(granted_scopes)
-            if missing_scopes:
-                self.log.info(
-                    f"Denying access to user {username}. {self.scope} scopes were requested, {self.required_scopes} are required for authorization, but only {granted_scopes} were granted"
-                )
-                return False
-            else:
-                return True
-
         # users should be explicitly allowed via config, otherwise they aren't
         return False
 

oauthenticator/tests/test_generic.py

@@ -1,7 +1,9 @@
 import json
+import re
 from functools import partial
+from tornado import web
 
-from pytest import fixture, mark
+from pytest import fixture, mark, raises
 from traitlets.config import Config
 
 from ..generic import GenericOAuthenticator
@@ -215,8 +217,32 @@ async def test_required_scopes(
     handled_user_model = user_model("user1")
     handler = generic_client.handler_for_user(handled_user_model)
     auth_model = await authenticator.authenticate(handler)
-    assert allowed == await authenticator.check_allowed(auth_model["name"], auth_model)
+    if allowed:
+        assert await authenticator.check_allowed(auth_model["name"], auth_model)
+    else:
+        with raises(web.HTTPError):
+            await authenticator.check_allowed(auth_model["name"], auth_model)
 
+async def test_required_scopes_validation_scope_subset(
+    get_authenticator
+):
+    c = Config()
+    # Test that if we require more scopes than we request, validation fails
+    c.GenericOAuthenticator.required_scopes = ["a", "b"]
+    c.GenericOAuthenticator.scope = ["a"]
+    with raises(ValueError, match=re.escape("Required Scopes must be a subset of Requested Scopes. ['a'] is requested but ['a', 'b'] is required")):
+        get_authenticator(config=c)
+
+async def test_required_scopes_validation_no_allow_all(
+    get_authenticator
+):
+    c = Config()
+    # Test that we can't have allow all and required_scopes together
+    c.GenericOAuthenticator.required_scopes = ["a"]
+    c.GenericOAuthenticator.scope = ["a", "b"]
+    c.GenericOAuthenticator.allow_all = True
+    with raises(ValueError, match=re.escape("Required Scopes is mutually exclusive with allow_all. Unset one of those configuration properties")):
+        get_authenticator(config=c)
 
 async def test_generic_callable_username_key(get_authenticator, generic_client):
     c = Config()