From ea5c2c87b041e51a4c5c0e533080ac26d8b42a11 Mon Sep 17 00:00:00 2001 From: Omar Richardson Date: Thu, 25 Jul 2024 22:51:34 +0200 Subject: [PATCH 1/6] Add a boolean to indicate if domains should be stripped --- oauthenticator/google.py | 36 ++++++++++++++++++++++++------------ 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/oauthenticator/google.py b/oauthenticator/google.py index 5ed613d9..99a71bdb 100644 --- a/oauthenticator/google.py +++ b/oauthenticator/google.py @@ -7,7 +7,7 @@ from jupyterhub.auth import LocalAuthenticator from tornado.auth import GoogleOAuth2Mixin from tornado.web import HTTPError -from traitlets import Dict, List, Set, Unicode, default, validate +from traitlets import Bool, Dict, List, Set, Unicode, default, validate from .oauth2 import OAuthenticator @@ -105,6 +105,26 @@ def _userdata_url_default(self): """, ) + strip_domain = Bool( + False, + config=True, + help=""" + Strip the username to exclude the `@domain` part. + + .. warning:: + If multiple `hosted_domains` are specified, there is a chance of clashing usernames. + """, + ) + + @validate('strip_domain') + def _check_multiple_hosted_domain(self, strip_domain): + if len(self.hosted_domain) > 1 and strip_domain: + self.log.warning( + "User names are stripped of `@domain`, but multiple domains are specified." + " This can lead to clashing usernames" + ) + return strip_domain.value + hosted_domain = List( Unicode(), config=True, @@ -179,21 +199,13 @@ def user_info_to_username(self, user_info): """ username = super().user_info_to_username(user_info) user_email = user_info["email"] - user_domain = user_info["domain"] = user_email.split("@")[1].lower() + user_info["domain"] = user_email.split("@")[1].lower() # NOTE: This is not an authorization check, it just about username # derivation. Decoupling hosted_domain from this is considered in # https://github.com/jupyterhub/oauthenticator/issues/733. - # - # NOTE: This code is written with without knowing for sure if the user - # email's domain could be different from the domain in hd, so we - # assume it could be even though it seems like it can't be. If a - # Google organization/workspace manages users in a "primary - # domain" and a "secondary domain", users with respective email - # domain have their hd field set respectively. - # - if len(self.hosted_domain) == 1 and user_domain == self.hosted_domain[0]: - # strip the domain in this situation + + if self.strip_domain and user_info["domain"] in self.hosted_domain: username = username.split("@")[0] return username From 187f9f0a6f10b56811823ff9c8a0137d7e0a1ec7 Mon Sep 17 00:00:00 2001 From: Omar Richardson Date: Thu, 25 Jul 2024 22:51:40 +0200 Subject: [PATCH 2/6] Update test to new domain stripping behaviour --- oauthenticator/tests/test_google.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/oauthenticator/tests/test_google.py b/oauthenticator/tests/test_google.py index 3aeb652d..18a0ffd6 100644 --- a/oauthenticator/tests/test_google.py +++ b/oauthenticator/tests/test_google.py @@ -164,6 +164,7 @@ async def test_google( c = Config() c.GoogleOAuthenticator = Config(class_config) c.GoogleOAuthenticator.username_claim = "custom" + c.GoogleOAuthenticator.strip_domain = True authenticator = GoogleOAuthenticator(config=c) handled_user_model = user_model("user1@example.com", "user1") @@ -201,7 +202,7 @@ async def test_google( ("06", "user1@other.org", "ok-hd.org", "user1@other.org", True, None), ], ) -async def test_hosted_domain_single_entry( +async def test_hosted_domain_strip_domain( google_client, test_variation_id, user_email, @@ -217,6 +218,7 @@ async def test_hosted_domain_single_entry( """ c = Config() c.GoogleOAuthenticator.hosted_domain = ["ok-hd.org"] + c.GoogleOAuthenticator.strip_domain = True c.GoogleOAuthenticator.admin_users = {"user1"} c.GoogleOAuthenticator.allowed_users = {"user2", "blocked", "user1@other.org"} c.GoogleOAuthenticator.blocked_users = {"blocked"} From 662a3c17982a4ff48268c72a10f972c6c9efc4a7 Mon Sep 17 00:00:00 2001 From: Omar Richardson Date: Fri, 26 Jul 2024 12:30:44 +0200 Subject: [PATCH 3/6] Fix setup for doc-generation --- oauthenticator/google.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/oauthenticator/google.py b/oauthenticator/google.py index 99a71bdb..0bac0abb 100644 --- a/oauthenticator/google.py +++ b/oauthenticator/google.py @@ -112,7 +112,8 @@ def _userdata_url_default(self): Strip the username to exclude the `@domain` part. .. warning:: - If multiple `hosted_domains` are specified, there is a chance of clashing usernames. + + If multiple `hosted_domains` are specified, there is a chance of clashing usernames. """, ) From 27682648019a0eadf8a7725f11745f5e58743ebe Mon Sep 17 00:00:00 2001 From: Omar Richardson Date: Fri, 26 Jul 2024 12:42:57 +0200 Subject: [PATCH 4/6] Change strip domain default back to old behaviour --- oauthenticator/google.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/oauthenticator/google.py b/oauthenticator/google.py index 0bac0abb..64aa0e6b 100644 --- a/oauthenticator/google.py +++ b/oauthenticator/google.py @@ -106,7 +106,6 @@ def _userdata_url_default(self): ) strip_domain = Bool( - False, config=True, help=""" Strip the username to exclude the `@domain` part. @@ -117,6 +116,10 @@ def _userdata_url_default(self): """, ) + @default('strip_domain') + def _strip_if_single_domain(self): + return len(self.hosted_domain) > 1 + @validate('strip_domain') def _check_multiple_hosted_domain(self, strip_domain): if len(self.hosted_domain) > 1 and strip_domain: From 8be2bbe22353ffde7f9da3a67902600ef7f9746c Mon Sep 17 00:00:00 2001 From: Omar Richardson Date: Fri, 26 Jul 2024 12:47:12 +0200 Subject: [PATCH 5/6] Revert test changes --- oauthenticator/google.py | 2 +- oauthenticator/tests/test_google.py | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/oauthenticator/google.py b/oauthenticator/google.py index 64aa0e6b..dab55997 100644 --- a/oauthenticator/google.py +++ b/oauthenticator/google.py @@ -118,7 +118,7 @@ def _userdata_url_default(self): @default('strip_domain') def _strip_if_single_domain(self): - return len(self.hosted_domain) > 1 + return len(self.hosted_domain) <= 1 @validate('strip_domain') def _check_multiple_hosted_domain(self, strip_domain): diff --git a/oauthenticator/tests/test_google.py b/oauthenticator/tests/test_google.py index 18a0ffd6..3aeb652d 100644 --- a/oauthenticator/tests/test_google.py +++ b/oauthenticator/tests/test_google.py @@ -164,7 +164,6 @@ async def test_google( c = Config() c.GoogleOAuthenticator = Config(class_config) c.GoogleOAuthenticator.username_claim = "custom" - c.GoogleOAuthenticator.strip_domain = True authenticator = GoogleOAuthenticator(config=c) handled_user_model = user_model("user1@example.com", "user1") @@ -202,7 +201,7 @@ async def test_google( ("06", "user1@other.org", "ok-hd.org", "user1@other.org", True, None), ], ) -async def test_hosted_domain_strip_domain( +async def test_hosted_domain_single_entry( google_client, test_variation_id, user_email, @@ -218,7 +217,6 @@ async def test_hosted_domain_strip_domain( """ c = Config() c.GoogleOAuthenticator.hosted_domain = ["ok-hd.org"] - c.GoogleOAuthenticator.strip_domain = True c.GoogleOAuthenticator.admin_users = {"user1"} c.GoogleOAuthenticator.allowed_users = {"user2", "blocked", "user1@other.org"} c.GoogleOAuthenticator.blocked_users = {"blocked"} From 81fd6bec167811d08f028bd17c3f4f95528e0065 Mon Sep 17 00:00:00 2001 From: Omar Richardson Date: Fri, 26 Jul 2024 12:48:42 +0200 Subject: [PATCH 6/6] Update documentation --- oauthenticator/google.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/oauthenticator/google.py b/oauthenticator/google.py index dab55997..5d901cd9 100644 --- a/oauthenticator/google.py +++ b/oauthenticator/google.py @@ -108,11 +108,13 @@ def _userdata_url_default(self): strip_domain = Bool( config=True, help=""" - Strip the username to exclude the `@domain` part. + Strip the username to exclude the `@domain` part. + This happens by default when there is only one hosted domain specified .. warning:: - If multiple `hosted_domains` are specified, there is a chance of clashing usernames. + If domains are stripped from usernames and multiple `hosted_domains` are specified, + there is a chance of clashing usernames. """, )