diff --git a/oauthenticator/google.py b/oauthenticator/google.py index 6346489a..86138de0 100644 --- a/oauthenticator/google.py +++ b/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): """ diff --git a/oauthenticator/tests/test_google.py b/oauthenticator/tests/test_google.py index 9a6d7711..1cb3ef61 100644 --- a/oauthenticator/tests/test_google.py +++ b/oauthenticator/tests/test_google.py @@ -35,13 +35,18 @@ def google_client(client): async def test_google(google_client): authenticator = GoogleOAuthenticator() handler = google_client.handler_for_user(user_model('fake@email.com')) - user_info = await authenticator.get_authenticated_user(handler, None) - assert sorted(user_info) == ['admin', 'auth_state', 'name'] - name = user_info['name'] - assert name == 'fake@email.com' - 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 == 'fake@email.com' + 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): @@ -49,41 +54,56 @@ async def test_google_username_claim(google_client): cfg.GoogleOAuthenticator.username_claim = "sub" authenticator = GoogleOAuthenticator(config=cfg) handler = google_client.handler_for_user(user_model('fake@email.com')) - 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('fake@email.com')) - user_info = await authenticator.get_authenticated_user(handler, None) - name = user_info['name'] - assert name == 'fake@email.com' + 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 == 'fake@email.com' - handler = google_client.handler_for_user(user_model('notallowed@notemail.com')) - 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('notallowed@notemail.com')) + 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('fake@email.com')) - user_info = await authenticator.get_authenticated_user(handler, None) - name = user_info['name'] - assert name == 'fake@email.com' + 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 == 'fake@email.com' - handler = google_client.handler_for_user(user_model('fake2@mycollege.edu')) - user_info = await authenticator.get_authenticated_user(handler, None) - name = user_info['name'] - assert name == 'fake2@mycollege.edu' + handler = google_client.handler_for_user(user_model('fake2@mycollege.edu')) + user_info = await authenticator.get_authenticated_user(handler, None) + name = user_info['name'] + assert name == 'fake2@mycollege.edu' - handler = google_client.handler_for_user(user_model('notallowed@notemail.com')) - 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('notallowed@notemail.com')) + 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('fakenonalloweduser@email.com')) 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,7 +174,7 @@ 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 @@ -164,7 +182,7 @@ async def test_allowed_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.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('fake@mycollege.edu')) 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"}