Change user_is_authorised for check_allowed for google

oauthenticator/google.py

@@ -116,56 +116,78 @@ def _cast_hosted_domain(self, proposal):
         help="""Google Apps hosted domain string, e.g. My College""",
     )
 
-    async def user_is_authorized(self, auth_model):
+    # async def user_is_authorized(self, auth_model):
+    #     """
+    #     Checks that the google user has a verified email and is part of
+    #     `hosted_domain` if set.
+
+    #     Authorizes users part of: `allowed_users`, `admin_users`,
+    #     `allowed_google_groups`, or `admin_google_groups`.
+
+    #     Note that this function also updates the auth_model with admin status
+    #     and the user's google groups if either `allowed_google_groups` or
+    #     `admin_google_groups` are configured.
+    #     """
+    async def check_allowed(self, username, auth_model):
         """
-        Checks that the google user has a verified email and is part of
-        `hosted_domain` if set.
+        Returns True for users allowed to be authorized.
 
-        Authorizes users part of: `allowed_users`, `admin_users`,
-        `allowed_google_groups`, or `admin_google_groups`.
-
-        Note that this function also updates the auth_model with admin status
-        and the user's google groups if either `allowed_google_groups` or
-        `admin_google_groups` are configured.
+        Overrides the OAuthenticator.check_allowed implementation to allow users
+        either part of `allowed_users` or `allowed_organizations`, and not just those
+        part of `allowed_users`.
         """
+        # allow admin users recognized via admin_users or update_auth_model
+        if auth_model["admin"]:
+            return True
+
         user_info = auth_model["auth_state"][self.user_auth_state_key]
         user_email = user_info["email"]
         user_domain = user_email.split("@")[1]
+        user_groups = set(self._google_groups_for_user(user_email, user_domain))
+        # FIXME: consider overriding the `is_admin` and add this logic there
+        admin_groups = self.admin_google_groups.get(user_domain, set())
+
+        if any(user_groups & admin_groups):
+            auth_model["admin"] = True
+            return True
 
         if not user_info["verified_email"]:
             self.log.warning(f"Google OAuth unverified email attempt: {user_email}")
             raise HTTPError(403, f"Google email {user_email} not verified")
 
-        if self.hosted_domain and user_domain not in self.hosted_domain:
-            self.log.warning(f"Google OAuth unauthorized domain attempt: {user_email}")
-            raise HTTPError(403, f"Google account domain @{user_domain} not authorized")
-
-        username = auth_model["name"]
-        if username in self.admin_users:
-            auth_model["admin"] = True
-
-        # always set google_groups if associated config is provided, and to a
-        # list rather than set, for backward compatibility
-        if self.allowed_google_groups or self.admin_google_groups:
-            # FIXME: _google_groups_for_user is a non-async function that blocks
-            #        JupyterHub, and it also doesn't have any cache. If this is
-            #        solved, we could also let this function not modify the
-            #        auth_model.
-            #
-            user_groups = self._google_groups_for_user(user_email, user_domain)
-            user_info["google_groups"] = list(user_groups)
-
-            allowed_groups = self.allowed_google_groups.get(user_domain, set())
-            admin_groups = self.admin_google_groups.get(user_domain, set())
-
-            # only set admin if not already set
-            if not auth_model["admin"]:
-                auth_model["admin"] = any(user_groups & admin_groups)
-
-            if any(user_groups & (allowed_groups | admin_groups)):
+        if self.hosted_domain:
+            if user_domain not in self.hosted_domain:
+                self.log.error(
+                    f"Google OAuth unauthorized domain attempt: {user_email}"
+                )
+                raise HTTPError(
+                    403, f"Google account domain @{user_domain} not authorized"
+                )
+
+        # if allowed_users or allowed_google_groups is configured, we deny users not part of either
+        if self.allowed_users or self.allowed_google_groups:
+            if username in self.allowed_users:
                 return True
 
-        return username in (self.allowed_users | self.admin_users)
+            # FIXME: Decide on the following:
+            #        always set google_groups if associated config is provided, and to a
+            #        list rather than set, for backward compatibility
+            if self.allowed_google_groups or self.admin_google_groups:
+                # FIXME: _google_groups_for_user is a non-async function that blocks
+                #        JupyterHub, and it also doesn't have any cache. If this is
+                #        solved, we could also let this function not modify the
+                #        auth_model.
+                #        It is called one time either way, why store it?
+                #
+                user_info["google_groups"] = list(user_groups)
+                allowed_groups = self.allowed_google_groups.get(user_domain, set())
+
+                if any(user_groups & allowed_groups):
+                    return True
+            return False
+
+        # otherwise, authorize all users
+        return True
 
     def _service_client_credentials(self, scopes, user_email_domain):
         """

oauthenticator/tests/test_google.py

@@ -35,55 +35,75 @@ def google_client(client):
 async def test_google(google_client):
     authenticator = GoogleOAuthenticator()
     handler = google_client.handler_for_user(user_model('[email protected]'))
-    user_info = await authenticator.get_authenticated_user(handler, None)
-    assert sorted(user_info) == ['admin', 'auth_state', 'name']
-    name = user_info['name']
-    assert name == '[email protected]'
-    auth_state = user_info['auth_state']
-    assert 'access_token' in auth_state
-    assert 'google_user' in auth_state
+    with mock.patch.object(
+        authenticator,
+        '_google_groups_for_user',
+        lambda *args: ['anotherone', 'fakeadmingroup'],
+    ):
+        user_info = await authenticator.get_authenticated_user(handler, None)
+        assert sorted(user_info) == ['admin', 'auth_state', 'name']
+        name = user_info['name']
+        assert name == '[email protected]'
+        auth_state = user_info['auth_state']
+        assert 'access_token' in auth_state
+        assert 'google_user' in auth_state
 
 
 async def test_google_username_claim(google_client):
     cfg = Config()
     cfg.GoogleOAuthenticator.username_claim = "sub"
     authenticator = GoogleOAuthenticator(config=cfg)
     handler = google_client.handler_for_user(user_model('[email protected]'))
-    user_info = await authenticator.get_authenticated_user(handler, None)
-    assert sorted(user_info) == ['admin', 'auth_state', 'name']
-    name = user_info['name']
-    assert name == '724f95667e2fbe903ee1b4cffcae3b25'
+    with mock.patch.object(
+        authenticator,
+        '_google_groups_for_user',
+        lambda *args: ['anotherone', 'fakeadmingroup'],
+    ):
+        user_info = await authenticator.get_authenticated_user(handler, None)
+        assert sorted(user_info) == ['admin', 'auth_state', 'name']
+        name = user_info['name']
+        assert name == '724f95667e2fbe903ee1b4cffcae3b25'
 
 
 async def test_hosted_domain(google_client):
     authenticator = GoogleOAuthenticator(hosted_domain=['email.com'])
     handler = google_client.handler_for_user(user_model('[email protected]'))
-    user_info = await authenticator.get_authenticated_user(handler, None)
-    name = user_info['name']
-    assert name == '[email protected]'
+    with mock.patch.object(
+        authenticator,
+        '_google_groups_for_user',
+        lambda *args: ['anotherone', 'fakeadmingroup'],
+    ):
+        user_info = await authenticator.get_authenticated_user(handler, None)
+        name = user_info['name']
+        assert name == '[email protected]'
 
-    handler = google_client.handler_for_user(user_model('[email protected]'))
-    with raises(HTTPError) as exc:
-        name = await authenticator.get_authenticated_user(handler, None)
-    assert exc.value.status_code == 403
+        handler = google_client.handler_for_user(user_model('[email protected]'))
+        with raises(HTTPError) as exc:
+            name = await authenticator.get_authenticated_user(handler, None)
+        assert exc.value.status_code == 403
 
 
 async def test_multiple_hosted_domain(google_client):
     authenticator = GoogleOAuthenticator(hosted_domain=['email.com', 'mycollege.edu'])
     handler = google_client.handler_for_user(user_model('[email protected]'))
-    user_info = await authenticator.get_authenticated_user(handler, None)
-    name = user_info['name']
-    assert name == '[email protected]'
+    with mock.patch.object(
+        authenticator,
+        '_google_groups_for_user',
+        lambda *args: ['anotherone', 'fakeadmingroup'],
+    ):
+        user_info = await authenticator.get_authenticated_user(handler, None)
+        name = user_info['name']
+        assert name == '[email protected]'
 
-    handler = google_client.handler_for_user(user_model('[email protected]'))
-    user_info = await authenticator.get_authenticated_user(handler, None)
-    name = user_info['name']
-    assert name == '[email protected]'
+        handler = google_client.handler_for_user(user_model('[email protected]'))
+        user_info = await authenticator.get_authenticated_user(handler, None)
+        name = user_info['name']
+        assert name == '[email protected]'
 
-    handler = google_client.handler_for_user(user_model('[email protected]'))
-    with raises(HTTPError) as exc:
-        name = await authenticator.get_authenticated_user(handler, None)
-    assert exc.value.status_code == 403
+        handler = google_client.handler_for_user(user_model('[email protected]'))
+        with raises(HTTPError) as exc:
+            name = await authenticator.get_authenticated_user(handler, None)
+        assert exc.value.status_code == 403
 
 
 async def test_admin_google_groups(google_client):
@@ -96,7 +116,7 @@ async def test_admin_google_groups(google_client):
     with mock.patch.object(
         authenticator,
         '_google_groups_for_user',
-        return_value=['anotherone', 'fakeadmingroup'],
+        lambda *args: ['anotherone', 'fakeadmingroup'],
     ):
         admin_user_info = await authenticator.get_authenticated_user(handler, None)
         # Make sure the user authenticated successfully
@@ -107,22 +127,20 @@ async def test_admin_google_groups(google_client):
     with mock.patch.object(
         authenticator,
         '_google_groups_for_user',
-        return_value=['anotherone', 'fakegroup'],
+        lambda *args: ['anotherone', 'fakegroup'],
     ):
-        allowed_user_info = await authenticator.authenticate(
-            handler,
-        )
+        allowed_user_info = await authenticator.get_authenticated_user(handler, None)
         allowed_user_groups = allowed_user_info['auth_state']['google_user'][
             'google_groups'
         ]
         admin_user = allowed_user_info['admin']
         assert 'fakegroup' in allowed_user_groups
-        assert admin_user == False
+        assert not admin_user
     handler = google_client.handler_for_user(user_model('[email protected]'))
     with mock.patch.object(
         authenticator,
         '_google_groups_for_user',
-        return_value=['anotherone', 'fakenonallowedgroup'],
+        lambda *args: ['anotherone', 'fakenonallowedgroup'],
     ):
         allowed_user_groups = await authenticator.get_authenticated_user(handler, None)
         assert allowed_user_groups is None
@@ -138,7 +156,7 @@ async def test_admin_user_but_no_admin_google_groups(google_client):
     with mock.patch.object(
         authenticator,
         '_google_groups_for_user',
-        return_value=['anotherone', 'fakegroup'],
+        lambda *args: ['anotherone', 'fakegroup'],
     ):
         admin_user_info = await authenticator.get_authenticated_user(handler, data=None)
         # Make sure the user authenticated successfully
@@ -156,15 +174,15 @@ async def test_allowed_google_groups(google_client):
     with mock.patch.object(
         authenticator,
         '_google_groups_for_user',
-        return_value=['anotherone', 'fakeadmingroup'],
+        lambda *args: ['anotherone', 'fakeadmingroup'],
     ):
         admin_user_info = await authenticator.get_authenticated_user(handler, None)
         assert admin_user_info is None
     handler = google_client.handler_for_user(user_model('[email protected]'))
     with mock.patch.object(
         authenticator,
         '_google_groups_for_user',
-        return_value=['anotherone', 'fakegroup'],
+        lambda *args: ['anotherone', 'fakegroup'],
     ):
         allowed_user_info = await authenticator.get_authenticated_user(handler, None)
         allowed_user_groups = allowed_user_info['auth_state']['google_user'][
@@ -177,13 +195,13 @@ async def test_allowed_google_groups(google_client):
     with mock.patch.object(
         authenticator,
         '_google_groups_for_user',
-        return_value=['anotherone', 'fakenonallowedgroup'],
+        lambda *args: ['anotherone', 'fakenonallowedgroup'],
     ):
         allowed_user_groups = await authenticator.get_authenticated_user(handler, None)
         assert allowed_user_groups is None
     handler = google_client.handler_for_user(user_model('[email protected]'))
     with mock.patch.object(
-        authenticator, '_google_groups_for_user', return_value=['fakegroup']
+        authenticator, '_google_groups_for_user', lambda *args: ['fakegroup']
     ):
         allowed_user_groups = await authenticator.get_authenticated_user(handler, None)
         assert allowed_user_groups is None
@@ -198,7 +216,7 @@ async def test_admin_only_google_groups(google_client):
     with mock.patch.object(
         authenticator,
         '_google_groups_for_user',
-        return_value=['anotherone', 'fakeadmingroup'],
+        lambda *args: ['anotherone', 'fakeadmingroup'],
     ):
         admin_user_info = await authenticator.get_authenticated_user(handler, None)
         admin_user = admin_user_info['admin']
@@ -219,5 +237,5 @@ def test_deprecated_config(caplog):
         'GoogleOAuthenticator.allowed_google_groups instead',
     ) in caplog.record_tuples
 
-    assert authenticator.allowed_google_groups == {'email.com': ['group']}
+    assert authenticator.allowed_google_groups == {'email.com': {'group'}}
     assert authenticator.allowed_users == {"user1"}