From 6bde66e5c7cac5ff8a376d973a76da9e41348da7 Mon Sep 17 00:00:00 2001
From: Georgiana Dolocan <georgiana.dolocan@gmail.com>
Date: Mon, 10 Apr 2023 16:51:35 +0300
Subject: [PATCH 01/57] Fix generic admin_groups bug

---
 oauthenticator/generic.py            | 50 +++++++++++++++-------------
 oauthenticator/tests/test_generic.py | 21 ++++++++++++
 2 files changed, 48 insertions(+), 23 deletions(-)

diff --git a/oauthenticator/generic.py b/oauthenticator/generic.py
index 9f946f96..c5b800a7 100644
--- a/oauthenticator/generic.py
+++ b/oauthenticator/generic.py
@@ -99,6 +99,27 @@ def user_info_to_username(self, user_info):
 
         return username
 
+    def get_user_groups(self, user_info):
+        if callable(self.claim_groups_key):
+            groups = self.claim_groups_key(user_info)
+        else:
+            try:
+                groups = reduce(
+                    dict.get, self.claim_groups_key.split("."), user_info
+                )
+            except TypeError:
+                # This happens if a nested key does not exist (reduce trying to call None.get)
+                self.log.error(
+                    f"The key {self.claim_groups_key} does not exist in the user token, or it is set to null"
+                )
+                groups = None
+
+        if not groups:
+            self.log.error(
+                f"No claim groups found for user! Something wrong with the `claim_groups_key` {self.claim_groups_key}? {user_info}"
+            )
+        return groups
+
     async def user_is_authorized(self, auth_model):
         user_info = auth_model["auth_state"][self.user_auth_state_key]
         if self.allowed_groups:
@@ -106,23 +127,8 @@ async def user_is_authorized(self, auth_model):
                 f"Validating if user claim groups match any of {self.allowed_groups}"
             )
 
-            if callable(self.claim_groups_key):
-                groups = self.claim_groups_key(user_info)
-            else:
-                try:
-                    groups = reduce(
-                        dict.get, self.claim_groups_key.split("."), user_info
-                    )
-                except TypeError:
-                    # This happens if a nested key does not exist (reduce trying to call None.get)
-                    self.log.error(
-                        f"The key {self.claim_groups_key} does not exist in the user token, or it is set to null"
-                    )
-                    groups = None
+            groups = self.get_user_groups(user_info)
             if not groups:
-                self.log.error(
-                    f"No claim groups found for user! Something wrong with the `claim_groups_key` {self.claim_groups_key}? {user_info}"
-                )
                 return False
 
             if not self.check_user_in_groups(groups, self.allowed_groups):
@@ -132,17 +138,15 @@ async def user_is_authorized(self, auth_model):
 
     async def update_auth_model(self, auth_model):
         user_info = auth_model["auth_state"][self.user_auth_state_key]
-        if self.allowed_groups:
-            if callable(self.claim_groups_key):
-                groups = self.claim_groups_key(user_info)
-            else:
-                groups = user_info.get(self.claim_groups_key)
-
-            # User has been checked to be in allowed_groups too if we're here
+        admin_status = True if auth_model['name'] in self.admin_users else None
+        # Check if user has been marked as admin by membership in self.admin_groups
+        if not admin_status and self.admin_groups:
+            groups = self.get_user_groups(user_info)
             if groups:
                 auth_model['admin'] = self.check_user_in_groups(
                     groups, self.admin_groups
                 )
+
         return auth_model
 
 
diff --git a/oauthenticator/tests/test_generic.py b/oauthenticator/tests/test_generic.py
index df3d4699..18855bc2 100644
--- a/oauthenticator/tests/test_generic.py
+++ b/oauthenticator/tests/test_generic.py
@@ -190,6 +190,27 @@ async def test_generic_groups_claim_key_with_allowed_groups_and_admin_groups_not
     assert user_info['admin'] is False
 
 
+async def test_generic_groups_claim_key_with_allowed_groups_and_no_admin_groups_but_admin_users(
+    get_authenticator, generic_client
+):
+    authenticator = get_authenticator(
+        scope=['openid', 'profile', 'roles'],
+        claim_groups_key='groups',
+        allowed_groups=['user'],
+        admin_groups=[],
+        admin_users=['wash'],
+    )
+    handler = generic_client.handler_for_user(
+        user_model('wash', alternate_username='zoe', groups=['user'])
+    )
+
+    # Assert that the authenticated user is actually an admin due to being listed in `admin_users`
+    # Even though admin_groups is empty
+    user_info = await authenticator.get_authenticated_user(handler, data=None)
+    assert user_info['name'] == 'wash'
+    assert user_info['admin'] is True
+
+
 async def test_generic_callable_groups_claim_key_with_allowed_groups_and_admin_groups(
     get_authenticator, generic_client
 ):

From 3309dd5da20e4930b5b1a02059b9e71714d90fab Mon Sep 17 00:00:00 2001
From: Georgiana Dolocan <georgiana.dolocan@gmail.com>
Date: Mon, 10 Apr 2023 18:05:00 +0300
Subject: [PATCH 02/57] Update openshift also

---
 oauthenticator/google.py            | 79 +++++++++++------------------
 oauthenticator/openshift.py         |  4 +-
 oauthenticator/tests/test_google.py | 45 ++++++++--------
 3 files changed, 58 insertions(+), 70 deletions(-)

diff --git a/oauthenticator/google.py b/oauthenticator/google.py
index e149c5a3..8661c50c 100644
--- a/oauthenticator/google.py
+++ b/oauthenticator/google.py
@@ -140,19 +140,36 @@ async def user_is_authorized(self, auth_model):
                 raise HTTPError(
                     403, f"Google account domain @{user_email_domain} not authorized."
                 )
+
+        if self.allowed_google_groups:
+            google_groups = self._google_groups_for_user(user_email, user_email_domain)
+            if not google_groups:
+                return False
+
+            auth_model['auth_state']['google_user']['google_groups'] = google_groups
+
+            # Check if user is a member of any allowed groups.
+            if user_email_domain in self.allowed_google_groups:
+                return check_user_in_groups(
+                    google_groups, self.allowed_google_groups[user_email_domain]
+                )
+
         return True
 
+
     async def update_auth_model(self, auth_model, google_groups=None):
         username = auth_model["name"]
-        user_email = auth_model["auth_state"][self.user_auth_state_key]['email']
-
-        if len(self.hosted_domain) == 1 and user_email == username:
-            # unambiguous domain, use only base name
-            username = user_email.split('@')[0]
-            auth_model["name"] = username
+        admin_status = True if username in self.admin_users else None
+        if admin_status:
+            auth_model['admin'] = is_admin
+        elif self.admin_google_groups:
+            user_email_domain = auth_model['auth_state'][self.user_auth_state_key]['hd']
 
-        if self.admin_google_groups or self.allowed_google_groups:
-            auth_model = await self._add_google_groups_info(auth_model, google_groups)
+            # Check if user is a member of any admin groups.
+            is_admin = check_user_in_groups(
+                google_groups, self.admin_google_groups[user_email_domain]
+            )
+            auth_model['admin'] = is_admin
 
         return auth_model
 
@@ -204,10 +221,14 @@ def _service_client(self, service_name, service_version, credentials, http=None)
             http=http,
         )
 
-    async def _google_groups_for_user(self, user_email, credentials, http=None):
+    def _google_groups_for_user(self, user_email, user_email_domain, http=None):
         """
         Return google groups a given user is a member of
         """
+        credentials = self._service_client_credentials(
+            scopes=[f"{self.google_api_url}/auth/admin.directory.group.readonly"],
+            user_email_domain=user_email_domain,
+        )
         service = self._service_client(
             service_name='admin',
             service_version='directory_v1',
@@ -222,46 +243,6 @@ async def _google_groups_for_user(self, user_email, credentials, http=None):
         self.log.debug(f"user_email {user_email} is a member of {results}")
         return results
 
-    async def _add_google_groups_info(self, user_info, google_groups=None):
-        user_email_domain = user_info['auth_state']['google_user']['hd']
-        user_email = user_info['auth_state']['google_user']['email']
-        if google_groups is None:
-            credentials = self._service_client_credentials(
-                scopes=[f"{self.google_api_url}/auth/admin.directory.group.readonly"],
-                user_email_domain=user_email_domain,
-            )
-            google_groups = await self._google_groups_for_user(
-                user_email=user_email, credentials=credentials
-            )
-        user_info['auth_state']['google_user']['google_groups'] = google_groups
-
-        # Check if user is a member of any admin groups.
-        if self.admin_google_groups:
-            is_admin = check_user_in_groups(
-                google_groups, self.admin_google_groups[user_email_domain]
-            )
-
-        # Check if user is a member of any allowed groups.
-        allowed_groups = self.allowed_google_groups
-
-        if allowed_groups:
-            if user_email_domain in allowed_groups:
-                user_in_group = check_user_in_groups(
-                    google_groups, allowed_groups[user_email_domain]
-                )
-            else:
-                return None
-        else:
-            user_in_group = True
-
-        if self.admin_google_groups and (is_admin or user_in_group):
-            user_info['admin'] = is_admin
-            return user_info
-        elif user_in_group:
-            return user_info
-        else:
-            return None
-
 
 class LocalGoogleOAuthenticator(LocalAuthenticator, GoogleOAuthenticator):
     """A version that mixes in local system user creation"""
diff --git a/oauthenticator/openshift.py b/oauthenticator/openshift.py
index 3e256073..de64b078 100644
--- a/oauthenticator/openshift.py
+++ b/oauthenticator/openshift.py
@@ -99,8 +99,10 @@ async def update_auth_model(self, auth_model):
         is an admin and update the auth_model with this info.
         """
         user_groups = set(auth_model['auth_state']['openshift_user']['groups'])
+        admin_status = True if auth_model['name'] in self.admin_users else None
 
-        if self.admin_groups:
+        # Check if user has been marked as admin by membership in self.admin_groups
+        if not admin_status and self.admin_groups:
             auth_model['admin'] = self.user_in_groups(user_groups, self.admin_groups)
 
         return auth_model
diff --git a/oauthenticator/tests/test_google.py b/oauthenticator/tests/test_google.py
index 7ed1c697..fe23370a 100644
--- a/oauthenticator/tests/test_google.py
+++ b/oauthenticator/tests/test_google.py
@@ -1,6 +1,7 @@
 import hashlib
 import logging
 import re
+from unittest import mock
 
 from pytest import fixture, raises
 from tornado.web import HTTPError
@@ -59,7 +60,7 @@ async def test_hosted_domain(google_client):
     handler = google_client.handler_for_user(user_model('fake@email.com'))
     user_info = await authenticator.authenticate(handler)
     name = user_info['name']
-    assert name == 'fake'
+    assert name == 'fake@email.com'
 
     handler = google_client.handler_for_user(user_model('notallowed@notemail.com'))
     with raises(HTTPError) as exc:
@@ -92,26 +93,30 @@ async def test_admin_google_groups(google_client):
         allowed_google_groups={'email.com': ['fakegroup']},
     )
     handler = google_client.handler_for_user(user_model('fakeadmin@email.com'))
-    admin_user_info = await authenticator.authenticate(
-        handler, google_groups=['anotherone', 'fakeadmingroup']
-    )
-    admin_user = admin_user_info['admin']
-    assert admin_user == True
+    def get_groups(arg1, arg2):
+        return ['anotherone', 'fakeadmingroup']
+    with mock.patch.object(authenticator, '_google_groups_for_user', get_groups):
+        admin_user_info = await authenticator.authenticate(
+            handler
+        )
+        print(admin_user_info)
+        # admin_user = admin_user_info['admin']
+        # assert admin_user == True
     handler = google_client.handler_for_user(user_model('fakealloweduser@email.com'))
-    allowed_user_info = await authenticator.authenticate(
-        handler, google_groups=['anotherone', 'fakegroup']
-    )
-    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
-    handler = google_client.handler_for_user(user_model('fakenonalloweduser@email.com'))
-    allowed_user_groups = await authenticator.authenticate(
-        handler, google_groups=['anotherone', 'fakenonallowedgroup']
-    )
-    assert allowed_user_groups is None
+    # allowed_user_info = await authenticator.authenticate(
+    #     handler, google_groups=['anotherone', 'fakegroup']
+    # )
+    # 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
+    # handler = google_client.handler_for_user(user_model('fakenonalloweduser@email.com'))
+    # allowed_user_groups = await authenticator.authenticate(
+    #     handler, google_groups=['anotherone', 'fakenonallowedgroup']
+    # )
+    # assert allowed_user_groups is None
 
 
 async def test_allowed_google_groups(google_client):

From b680635ec4afeccbb26025dced30fa2700152d32 Mon Sep 17 00:00:00 2001
From: Georgiana Dolocan <georgiana.dolocan@gmail.com>
Date: Tue, 11 Apr 2023 11:10:39 +0300
Subject: [PATCH 03/57] Update google tests

---
 oauthenticator/tests/test_google.py | 139 ++++++++++++++++++----------
 1 file changed, 90 insertions(+), 49 deletions(-)

diff --git a/oauthenticator/tests/test_google.py b/oauthenticator/tests/test_google.py
index fe23370a..93e5dd79 100644
--- a/oauthenticator/tests/test_google.py
+++ b/oauthenticator/tests/test_google.py
@@ -93,30 +93,58 @@ async def test_admin_google_groups(google_client):
         allowed_google_groups={'email.com': ['fakegroup']},
     )
     handler = google_client.handler_for_user(user_model('fakeadmin@email.com'))
-    def get_groups(arg1, arg2):
-        return ['anotherone', 'fakeadmingroup']
-    with mock.patch.object(authenticator, '_google_groups_for_user', get_groups):
-        admin_user_info = await authenticator.authenticate(
-            handler
-        )
-        print(admin_user_info)
-        # admin_user = admin_user_info['admin']
-        # assert admin_user == True
+    with mock.patch.object(
+        authenticator,
+        '_google_groups_for_user',
+        return_value=['anotherone', 'fakeadmingroup'],
+    ):
+        admin_user_info = await authenticator.authenticate(handler)
+        # Make sure the user authenticated successfully
+        assert admin_user_info
+        # Assert that the user is an admin
+        assert admin_user_info.get('admin', None) == True
     handler = google_client.handler_for_user(user_model('fakealloweduser@email.com'))
-    # allowed_user_info = await authenticator.authenticate(
-    #     handler, google_groups=['anotherone', 'fakegroup']
-    # )
-    # 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
-    # handler = google_client.handler_for_user(user_model('fakenonalloweduser@email.com'))
-    # allowed_user_groups = await authenticator.authenticate(
-    #     handler, google_groups=['anotherone', 'fakenonallowedgroup']
-    # )
-    # assert allowed_user_groups is None
+    with mock.patch.object(
+        authenticator,
+        '_google_groups_for_user',
+        return_value=['anotherone', 'fakegroup'],
+    ):
+        allowed_user_info = await authenticator.authenticate(
+            handler,
+        )
+        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
+    handler = google_client.handler_for_user(user_model('fakenonalloweduser@email.com'))
+    with mock.patch.object(
+        authenticator,
+        '_google_groups_for_user',
+        return_value=['anotherone', 'fakenonallowedgroup'],
+    ):
+        allowed_user_groups = await authenticator.authenticate(handler)
+        assert allowed_user_groups is None
+
+
+async def test_admin_user_but_no_admin_google_groups(google_client):
+    authenticator = GoogleOAuthenticator(
+        hosted_domain=['email.com', 'mycollege.edu'],
+        allowed_google_groups={'email.com': ['fakegroup']},
+        admin_users=['fakeadmin@email.com'],
+    )
+    handler = google_client.handler_for_user(user_model('fakeadmin@email.com'))
+    with mock.patch.object(
+        authenticator,
+        '_google_groups_for_user',
+        return_value=['anotherone', 'fakegroup'],
+    ):
+        admin_user_info = await authenticator.get_authenticated_user(handler, data=None)
+        # Make sure the user authenticated successfully
+        assert admin_user_info
+        # Assert that the user is an admin
+        assert admin_user_info.get('admin', None) == True
 
 
 async def test_allowed_google_groups(google_client):
@@ -125,30 +153,40 @@ async def test_allowed_google_groups(google_client):
         allowed_google_groups={'email.com': ['fakegroup'], ',mycollege.edu': []},
     )
     handler = google_client.handler_for_user(user_model('fakeadmin@email.com'))
-    admin_user_info = await authenticator.authenticate(
-        handler, google_groups=['anotherone', 'fakeadmingroup']
-    )
-    assert admin_user_info is None
+    with mock.patch.object(
+        authenticator,
+        '_google_groups_for_user',
+        return_value=['anotherone', 'fakeadmingroup'],
+    ):
+        admin_user_info = await authenticator.authenticate(handler)
+        assert admin_user_info is None
     handler = google_client.handler_for_user(user_model('fakealloweduser@email.com'))
-    allowed_user_info = await authenticator.authenticate(
-        handler, google_groups=['anotherone', 'fakegroup']
-    )
-    allowed_user_groups = allowed_user_info['auth_state']['google_user'][
-        'google_groups'
-    ]
-    admin_field = allowed_user_info.get('admin')
-    assert 'fakegroup' in allowed_user_groups
-    assert admin_field is None
+    with mock.patch.object(
+        authenticator,
+        '_google_groups_for_user',
+        return_value=['anotherone', 'fakegroup'],
+    ):
+        allowed_user_info = await authenticator.authenticate(handler)
+        allowed_user_groups = allowed_user_info['auth_state']['google_user'][
+            'google_groups'
+        ]
+        admin_field = allowed_user_info.get('admin')
+        assert 'fakegroup' in allowed_user_groups
+        assert admin_field is None
     handler = google_client.handler_for_user(user_model('fakenonalloweduser@email.com'))
-    allowed_user_groups = await authenticator.authenticate(
-        handler, google_groups=['anotherone', 'fakenonallowedgroup']
-    )
-    assert allowed_user_groups is None
+    with mock.patch.object(
+        authenticator,
+        '_google_groups_for_user',
+        return_value=['anotherone', 'fakenonallowedgroup'],
+    ):
+        allowed_user_groups = await authenticator.authenticate(handler)
+        assert allowed_user_groups is None
     handler = google_client.handler_for_user(user_model('fake@mycollege.edu'))
-    allowed_user_groups = await authenticator.authenticate(
-        handler, google_groups=['fakegroup']
-    )
-    assert allowed_user_groups is None
+    with mock.patch.object(
+        authenticator, '_google_groups_for_user', return_value=['fakegroup']
+    ):
+        allowed_user_groups = await authenticator.authenticate(handler)
+        assert allowed_user_groups is None
 
 
 async def test_admin_only_google_groups(google_client):
@@ -157,11 +195,14 @@ async def test_admin_only_google_groups(google_client):
         admin_google_groups={'email.com': ['fakeadmingroup']},
     )
     handler = google_client.handler_for_user(user_model('fakeadmin@email.com'))
-    admin_user_info = await authenticator.authenticate(
-        handler, google_groups=['anotherone', 'fakeadmingroup']
-    )
-    admin_user = admin_user_info['admin']
-    assert admin_user is True
+    with mock.patch.object(
+        authenticator,
+        '_google_groups_for_user',
+        return_value=['anotherone', 'fakeadmingroup'],
+    ):
+        admin_user_info = await authenticator.authenticate(handler)
+        admin_user = admin_user_info['admin']
+        assert admin_user is True
 
 
 def test_deprecated_config(caplog):

From d5f9bdfd3e0781d65eec3c4f94b5d770eb320d65 Mon Sep 17 00:00:00 2001
From: Georgiana Dolocan <georgiana.dolocan@gmail.com>
Date: Tue, 11 Apr 2023 11:11:39 +0300
Subject: [PATCH 04/57] Fix google auth groups logic

---
 oauthenticator/google.py | 43 +++++++++++++++++++---------------------
 1 file changed, 20 insertions(+), 23 deletions(-)

diff --git a/oauthenticator/google.py b/oauthenticator/google.py
index 8661c50c..598c126b 100644
--- a/oauthenticator/google.py
+++ b/oauthenticator/google.py
@@ -13,14 +13,6 @@
 from .oauth2 import OAuthenticator
 
 
-def check_user_in_groups(member_groups, allowed_groups):
-    # Check if user is a member of any group in the allowed groups
-    if any(g in member_groups for g in allowed_groups):
-        return True  # user _is_ in group
-    else:
-        return False
-
-
 class GoogleOAuthenticator(OAuthenticator, GoogleOAuth2Mixin):
     _deprecated_oauth_aliases = {
         "google_group_whitelist": ("allowed_google_groups", "0.12.0"),
@@ -148,28 +140,33 @@ async def user_is_authorized(self, auth_model):
 
             auth_model['auth_state']['google_user']['google_groups'] = google_groups
 
-            # Check if user is a member of any allowed groups.
-            if user_email_domain in self.allowed_google_groups:
-                return check_user_in_groups(
-                    google_groups, self.allowed_google_groups[user_email_domain]
+            # Check if user is a member of any allowed or admin groups.
+            allowed_groups_per_domain = self.allowed_google_groups.get(user_email_domain, []) + self.admin_google_groups.get(user_email_domain, [])
+            print(allowed_groups_per_domain)
+            if not allowed_groups_per_domain:
+                return False
+            else:
+                return self.user_groups_in_allowed_groups(
+                    google_groups, allowed_groups_per_domain
                 )
 
         return True
 
 
-    async def update_auth_model(self, auth_model, google_groups=None):
+    async def update_auth_model(self, auth_model):
         username = auth_model["name"]
         admin_status = True if username in self.admin_users else None
-        if admin_status:
-            auth_model['admin'] = is_admin
-        elif self.admin_google_groups:
-            user_email_domain = auth_model['auth_state'][self.user_auth_state_key]['hd']
-
-            # Check if user is a member of any admin groups.
-            is_admin = check_user_in_groups(
-                google_groups, self.admin_google_groups[user_email_domain]
-            )
-            auth_model['admin'] = is_admin
+        if not admin_status and self.admin_google_groups:
+            user_email = auth_model["auth_state"][self.user_auth_state_key]['email']
+            user_email_domain = user_email.split('@')[1]
+
+            if user_email_domain in self.admin_google_groups.keys():
+                # Check if user is a member of any admin groups.
+                google_groups = self._google_groups_for_user(user_email, user_email_domain)
+                if google_groups:
+                    auth_model['admin'] = check_user_in_groups(
+                        google_groups, self.admin_google_groups[user_email_domain]
+                    )
 
         return auth_model
 

From 0c717c177ec398713aa269a9c6f8f0ff752ade24 Mon Sep 17 00:00:00 2001
From: Georgiana Dolocan <georgiana.dolocan@gmail.com>
Date: Tue, 11 Apr 2023 11:12:04 +0300
Subject: [PATCH 05/57] Move static method in base class

---
 oauthenticator/generic.py   | 6 +-----
 oauthenticator/oauth2.py    | 8 ++++++++
 oauthenticator/openshift.py | 6 +-----
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/oauthenticator/generic.py b/oauthenticator/generic.py
index c5b800a7..5c824809 100644
--- a/oauthenticator/generic.py
+++ b/oauthenticator/generic.py
@@ -83,10 +83,6 @@ def _default_http_client(self):
             force_instance=True, defaults=dict(validate_cert=self.tls_verify)
         )
 
-    @staticmethod
-    def check_user_in_groups(member_groups, allowed_groups):
-        return bool(set(member_groups) & set(allowed_groups))
-
     def user_info_to_username(self, user_info):
         if callable(self.username_claim):
             username = self.username_claim(user_info)
@@ -131,7 +127,7 @@ async def user_is_authorized(self, auth_model):
             if not groups:
                 return False
 
-            if not self.check_user_in_groups(groups, self.allowed_groups):
+            if not self.user_groups_in_allowed_groups(groups, self.allowed_groups):
                 return False
 
         return True
diff --git a/oauthenticator/oauth2.py b/oauthenticator/oauth2.py
index 3d892b6a..18e8ba18 100644
--- a/oauthenticator/oauth2.py
+++ b/oauthenticator/oauth2.py
@@ -762,6 +762,14 @@ async def user_is_authorized(self, auth_model):
         """
         return True
 
+    @staticmethod
+    def user_groups_in_allowed_groups(user_groups: set, allowed_groups: set):
+        """
+        Returns True if user is a member of any group in the allowed groups,
+        and False otherwise
+        """
+        return any(user_groups.intersection(allowed_groups))
+
     async def authenticate(self, handler, data=None, **kwargs):
         # build the parameters to be used in the request exchanging the oauth code for the access token
         access_token_params = self.build_access_tokens_request_params(handler, data)
diff --git a/oauthenticator/openshift.py b/oauthenticator/openshift.py
index de64b078..a2391373 100644
--- a/oauthenticator/openshift.py
+++ b/oauthenticator/openshift.py
@@ -86,10 +86,6 @@ def _token_url_default(self):
     def _userdata_url_default(self):
         return f"{self.openshift_rest_api_url}/apis/user.openshift.io/v1/users/~"
 
-    @staticmethod
-    def user_in_groups(user_groups: set, allowed_groups: set):
-        return any(user_groups.intersection(allowed_groups))
-
     def user_info_to_username(self, user_info):
         return user_info['metadata']['name']
 
@@ -103,7 +99,7 @@ async def update_auth_model(self, auth_model):
 
         # Check if user has been marked as admin by membership in self.admin_groups
         if not admin_status and self.admin_groups:
-            auth_model['admin'] = self.user_in_groups(user_groups, self.admin_groups)
+            auth_model['admin'] = self.user_groups_in_allowed_groups(user_groups, self.admin_groups)
 
         return auth_model
 

From a807e185854f8fd3f464c5131226c0a7949a416b Mon Sep 17 00:00:00 2001
From: Georgiana Dolocan <georgiana.dolocan@gmail.com>
Date: Tue, 11 Apr 2023 11:14:37 +0300
Subject: [PATCH 06/57] User should be authorized also if only in admin groups

---
 oauthenticator/generic.py   | 2 +-
 oauthenticator/google.py    | 1 -
 oauthenticator/openshift.py | 8 ++++----
 3 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/oauthenticator/generic.py b/oauthenticator/generic.py
index 5c824809..2d1a1a89 100644
--- a/oauthenticator/generic.py
+++ b/oauthenticator/generic.py
@@ -127,7 +127,7 @@ async def user_is_authorized(self, auth_model):
             if not groups:
                 return False
 
-            if not self.user_groups_in_allowed_groups(groups, self.allowed_groups):
+            if not self.user_groups_in_allowed_groups(groups, self.allowed_groups + self.admin_groups):
                 return False
 
         return True
diff --git a/oauthenticator/google.py b/oauthenticator/google.py
index 598c126b..c87b5022 100644
--- a/oauthenticator/google.py
+++ b/oauthenticator/google.py
@@ -142,7 +142,6 @@ async def user_is_authorized(self, auth_model):
 
             # Check if user is a member of any allowed or admin groups.
             allowed_groups_per_domain = self.allowed_google_groups.get(user_email_domain, []) + self.admin_google_groups.get(user_email_domain, [])
-            print(allowed_groups_per_domain)
             if not allowed_groups_per_domain:
                 return False
             else:
diff --git a/oauthenticator/openshift.py b/oauthenticator/openshift.py
index a2391373..0b6d98b7 100644
--- a/oauthenticator/openshift.py
+++ b/oauthenticator/openshift.py
@@ -113,10 +113,10 @@ async def user_is_authorized(self, auth_model):
 
         if self.allowed_groups or self.admin_groups:
             msg = f"username:{username} User not in any of the allowed/admin groups"
-            if not self.user_in_groups(user_groups, self.allowed_groups):
-                if not self.user_in_groups(user_groups, self.admin_groups):
-                    self.log.warning(msg)
-                    return False
+            # User is authorized if either in allowed_groups or in admin_groups
+            if not self.user_in_groups(user_groups, self.allowed_groups + self.admin_groups):
+                self.log.warning(msg)
+                return False
 
         return True
 

From 6290b7bfe9a40b5e36b8b798863e3153523d30eb Mon Sep 17 00:00:00 2001
From: Georgiana Dolocan <georgiana.dolocan@gmail.com>
Date: Tue, 11 Apr 2023 11:29:08 +0300
Subject: [PATCH 07/57] Update missing funcs

---
 oauthenticator/generic.py   | 2 +-
 oauthenticator/google.py    | 4 ++--
 oauthenticator/oauth2.py    | 8 ++++++--
 oauthenticator/openshift.py | 2 +-
 4 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/oauthenticator/generic.py b/oauthenticator/generic.py
index 2d1a1a89..b7166d62 100644
--- a/oauthenticator/generic.py
+++ b/oauthenticator/generic.py
@@ -139,7 +139,7 @@ async def update_auth_model(self, auth_model):
         if not admin_status and self.admin_groups:
             groups = self.get_user_groups(user_info)
             if groups:
-                auth_model['admin'] = self.check_user_in_groups(
+                auth_model['admin'] = self.user_groups_in_allowed_groups(
                     groups, self.admin_groups
                 )
 
diff --git a/oauthenticator/google.py b/oauthenticator/google.py
index c87b5022..187ef52e 100644
--- a/oauthenticator/google.py
+++ b/oauthenticator/google.py
@@ -163,7 +163,7 @@ async def update_auth_model(self, auth_model):
                 # Check if user is a member of any admin groups.
                 google_groups = self._google_groups_for_user(user_email, user_email_domain)
                 if google_groups:
-                    auth_model['admin'] = check_user_in_groups(
+                    auth_model['admin'] = self.user_groups_in_allowed_groups(
                         google_groups, self.admin_google_groups[user_email_domain]
                     )
 
@@ -219,7 +219,7 @@ def _service_client(self, service_name, service_version, credentials, http=None)
 
     def _google_groups_for_user(self, user_email, user_email_domain, http=None):
         """
-        Return google groups a given user is a member of
+        Return a list with the google groups a given user is a member of
         """
         credentials = self._service_client_credentials(
             scopes=[f"{self.google_api_url}/auth/admin.directory.group.readonly"],
diff --git a/oauthenticator/oauth2.py b/oauthenticator/oauth2.py
index 18e8ba18..40bc8074 100644
--- a/oauthenticator/oauth2.py
+++ b/oauthenticator/oauth2.py
@@ -763,12 +763,16 @@ async def user_is_authorized(self, auth_model):
         return True
 
     @staticmethod
-    def user_groups_in_allowed_groups(user_groups: set, allowed_groups: set):
+    def user_groups_in_allowed_groups(user_groups, allowed_groups):
         """
         Returns True if user is a member of any group in the allowed groups,
         and False otherwise
         """
-        return any(user_groups.intersection(allowed_groups))
+        if not isinstance(user_groups, set):
+            user_groups = set(user_groups)
+        if not isinstance(allowed_groups, set):
+            allowed_groups = set(allowed_groups)
+        return any((user_groups).intersection(allowed_groups))
 
     async def authenticate(self, handler, data=None, **kwargs):
         # build the parameters to be used in the request exchanging the oauth code for the access token
diff --git a/oauthenticator/openshift.py b/oauthenticator/openshift.py
index 0b6d98b7..0fa53217 100644
--- a/oauthenticator/openshift.py
+++ b/oauthenticator/openshift.py
@@ -114,7 +114,7 @@ async def user_is_authorized(self, auth_model):
         if self.allowed_groups or self.admin_groups:
             msg = f"username:{username} User not in any of the allowed/admin groups"
             # User is authorized if either in allowed_groups or in admin_groups
-            if not self.user_in_groups(user_groups, self.allowed_groups + self.admin_groups):
+            if not self.user_groups_in_allowed_groups(user_groups, self.allowed_groups.union(self.admin_groups)):
                 self.log.warning(msg)
                 return False
 

From e11edea51e87025db9209896ee44115448bd2dab Mon Sep 17 00:00:00 2001
From: Georgiana Dolocan <georgiana.dolocan@gmail.com>
Date: Tue, 11 Apr 2023 11:32:35 +0300
Subject: [PATCH 08/57] Add test for the bug in openshift

---
 oauthenticator/tests/test_openshift.py | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/oauthenticator/tests/test_openshift.py b/oauthenticator/tests/test_openshift.py
index c5d8e5e9..60658b42 100644
--- a/oauthenticator/tests/test_openshift.py
+++ b/oauthenticator/tests/test_openshift.py
@@ -92,3 +92,13 @@ async def test_openshift_in_allowed_groups_and_is_not_admin(openshift_client):
     user_info = await authenticator.authenticate(handler)
     assert sorted(user_info) == ['admin', 'auth_state', 'name']
     assert user_info['admin'] == False
+
+async def test_openshift_not_in_admin_users_but_not_in_admin_groups(openshift_client):
+    authenticator = OpenShiftOAuthenticator()
+    authenticator.allowed_groups = {'group1'}
+    authenticator.admin_users = ['wash']
+    authenticator.openshift_auth_api_url = "https://openshift.default.svc.cluster.local"
+    handler = openshift_client.handler_for_user(user_model('wash'))
+    user_info = await authenticator.get_authenticated_user(handler, data=None)
+    assert sorted(user_info) == ['admin', 'auth_state', 'name']
+    assert user_info['admin'] == True

From ba1406165e509fe51c33a0e438d925777408b1f4 Mon Sep 17 00:00:00 2001
From: Georgiana Dolocan <georgiana.dolocan@gmail.com>
Date: Tue, 11 Apr 2023 11:33:58 +0300
Subject: [PATCH 09/57] Update func signature as no longer needed

---
 oauthenticator/oauth2.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/oauthenticator/oauth2.py b/oauthenticator/oauth2.py
index 40bc8074..430aa862 100644
--- a/oauthenticator/oauth2.py
+++ b/oauthenticator/oauth2.py
@@ -733,7 +733,7 @@ def build_auth_state_dict(self, token_info, user_info):
             self.user_auth_state_key: user_info,
         }
 
-    async def update_auth_model(self, auth_model, **kwargs):
+    async def update_auth_model(self, auth_model):
         """
         Updates `auth_model` dict if any fields have changed or additional information is available
         or returns the unchanged `auth_model`.
@@ -806,7 +806,7 @@ async def authenticate(self, handler, data=None, **kwargs):
             return None
 
         # update the auth model with any info if available
-        return await self.update_auth_model(auth_model, **kwargs)
+        return await self.update_auth_model(auth_model)
 
     _deprecated_oauth_aliases = {}
 

From 6e52be290118c621bcaffc8547c0d5c08815600f Mon Sep 17 00:00:00 2001
From: "pre-commit-ci[bot]"
 <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Date: Tue, 11 Apr 2023 08:57:25 +0000
Subject: [PATCH 10/57] [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci
---
 oauthenticator/generic.py              | 8 ++++----
 oauthenticator/google.py               | 9 ++++++---
 oauthenticator/openshift.py            | 8 ++++++--
 oauthenticator/tests/test_openshift.py | 1 +
 4 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/oauthenticator/generic.py b/oauthenticator/generic.py
index b7166d62..9dd2cc91 100644
--- a/oauthenticator/generic.py
+++ b/oauthenticator/generic.py
@@ -100,9 +100,7 @@ def get_user_groups(self, user_info):
             groups = self.claim_groups_key(user_info)
         else:
             try:
-                groups = reduce(
-                    dict.get, self.claim_groups_key.split("."), user_info
-                )
+                groups = reduce(dict.get, self.claim_groups_key.split("."), user_info)
             except TypeError:
                 # This happens if a nested key does not exist (reduce trying to call None.get)
                 self.log.error(
@@ -127,7 +125,9 @@ async def user_is_authorized(self, auth_model):
             if not groups:
                 return False
 
-            if not self.user_groups_in_allowed_groups(groups, self.allowed_groups + self.admin_groups):
+            if not self.user_groups_in_allowed_groups(
+                groups, self.allowed_groups + self.admin_groups
+            ):
                 return False
 
         return True
diff --git a/oauthenticator/google.py b/oauthenticator/google.py
index 187ef52e..4fa78889 100644
--- a/oauthenticator/google.py
+++ b/oauthenticator/google.py
@@ -141,7 +141,9 @@ async def user_is_authorized(self, auth_model):
             auth_model['auth_state']['google_user']['google_groups'] = google_groups
 
             # Check if user is a member of any allowed or admin groups.
-            allowed_groups_per_domain = self.allowed_google_groups.get(user_email_domain, []) + self.admin_google_groups.get(user_email_domain, [])
+            allowed_groups_per_domain = self.allowed_google_groups.get(
+                user_email_domain, []
+            ) + self.admin_google_groups.get(user_email_domain, [])
             if not allowed_groups_per_domain:
                 return False
             else:
@@ -151,7 +153,6 @@ async def user_is_authorized(self, auth_model):
 
         return True
 
-
     async def update_auth_model(self, auth_model):
         username = auth_model["name"]
         admin_status = True if username in self.admin_users else None
@@ -161,7 +162,9 @@ async def update_auth_model(self, auth_model):
 
             if user_email_domain in self.admin_google_groups.keys():
                 # Check if user is a member of any admin groups.
-                google_groups = self._google_groups_for_user(user_email, user_email_domain)
+                google_groups = self._google_groups_for_user(
+                    user_email, user_email_domain
+                )
                 if google_groups:
                     auth_model['admin'] = self.user_groups_in_allowed_groups(
                         google_groups, self.admin_google_groups[user_email_domain]
diff --git a/oauthenticator/openshift.py b/oauthenticator/openshift.py
index 0fa53217..ef5397f1 100644
--- a/oauthenticator/openshift.py
+++ b/oauthenticator/openshift.py
@@ -99,7 +99,9 @@ async def update_auth_model(self, auth_model):
 
         # Check if user has been marked as admin by membership in self.admin_groups
         if not admin_status and self.admin_groups:
-            auth_model['admin'] = self.user_groups_in_allowed_groups(user_groups, self.admin_groups)
+            auth_model['admin'] = self.user_groups_in_allowed_groups(
+                user_groups, self.admin_groups
+            )
 
         return auth_model
 
@@ -114,7 +116,9 @@ async def user_is_authorized(self, auth_model):
         if self.allowed_groups or self.admin_groups:
             msg = f"username:{username} User not in any of the allowed/admin groups"
             # User is authorized if either in allowed_groups or in admin_groups
-            if not self.user_groups_in_allowed_groups(user_groups, self.allowed_groups.union(self.admin_groups)):
+            if not self.user_groups_in_allowed_groups(
+                user_groups, self.allowed_groups.union(self.admin_groups)
+            ):
                 self.log.warning(msg)
                 return False
 
diff --git a/oauthenticator/tests/test_openshift.py b/oauthenticator/tests/test_openshift.py
index 60658b42..c1951bc1 100644
--- a/oauthenticator/tests/test_openshift.py
+++ b/oauthenticator/tests/test_openshift.py
@@ -93,6 +93,7 @@ async def test_openshift_in_allowed_groups_and_is_not_admin(openshift_client):
     assert sorted(user_info) == ['admin', 'auth_state', 'name']
     assert user_info['admin'] == False
 
+
 async def test_openshift_not_in_admin_users_but_not_in_admin_groups(openshift_client):
     authenticator = OpenShiftOAuthenticator()
     authenticator.allowed_groups = {'group1'}

From ca2e4663ca4724f38a0242d45a01bb90981176b6 Mon Sep 17 00:00:00 2001
From: Georgiana Dolocan <georgiana.dolocan@gmail.com>
Date: Tue, 11 Apr 2023 11:46:26 +0300
Subject: [PATCH 11/57] Authorize based on group membership if allowed_users is
 not set

---
 oauthenticator/generic.py   | 2 +-
 oauthenticator/google.py    | 2 +-
 oauthenticator/openshift.py | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/oauthenticator/generic.py b/oauthenticator/generic.py
index 9dd2cc91..02d6606c 100644
--- a/oauthenticator/generic.py
+++ b/oauthenticator/generic.py
@@ -116,7 +116,7 @@ def get_user_groups(self, user_info):
 
     async def user_is_authorized(self, auth_model):
         user_info = auth_model["auth_state"][self.user_auth_state_key]
-        if self.allowed_groups:
+        if not self.allowed_users and (self.allowed_groups or self.admin_groups):
             self.log.info(
                 f"Validating if user claim groups match any of {self.allowed_groups}"
             )
diff --git a/oauthenticator/google.py b/oauthenticator/google.py
index 4fa78889..4e7317eb 100644
--- a/oauthenticator/google.py
+++ b/oauthenticator/google.py
@@ -133,7 +133,7 @@ async def user_is_authorized(self, auth_model):
                     403, f"Google account domain @{user_email_domain} not authorized."
                 )
 
-        if self.allowed_google_groups:
+        if not self.allowed_users and (self.allowed_google_groups or self.admin_google_groups):
             google_groups = self._google_groups_for_user(user_email, user_email_domain)
             if not google_groups:
                 return False
diff --git a/oauthenticator/openshift.py b/oauthenticator/openshift.py
index ef5397f1..31c153df 100644
--- a/oauthenticator/openshift.py
+++ b/oauthenticator/openshift.py
@@ -113,7 +113,7 @@ async def user_is_authorized(self, auth_model):
         user_groups = set(auth_model['auth_state']['openshift_user']['groups'])
         username = auth_model['name']
 
-        if self.allowed_groups or self.admin_groups:
+        if not self.allowed_users and (self.allowed_groups or self.admin_groups):
             msg = f"username:{username} User not in any of the allowed/admin groups"
             # User is authorized if either in allowed_groups or in admin_groups
             if not self.user_groups_in_allowed_groups(

From 4fda8222c8ce2ab64827e6b49e1270047147ae59 Mon Sep 17 00:00:00 2001
From: Georgiana Dolocan <georgiana.dolocan@gmail.com>
Date: Wed, 12 Apr 2023 16:01:12 +0300
Subject: [PATCH 12/57] Update authorization funcs

---
 oauthenticator/generic.py   | 7 +++++--
 oauthenticator/google.py    | 6 ++++--
 oauthenticator/openshift.py | 7 +++++--
 3 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/oauthenticator/generic.py b/oauthenticator/generic.py
index 02d6606c..a79c7831 100644
--- a/oauthenticator/generic.py
+++ b/oauthenticator/generic.py
@@ -116,7 +116,7 @@ def get_user_groups(self, user_info):
 
     async def user_is_authorized(self, auth_model):
         user_info = auth_model["auth_state"][self.user_auth_state_key]
-        if not self.allowed_users and (self.allowed_groups or self.admin_groups):
+        if self.allowed_groups:
             self.log.info(
                 f"Validating if user claim groups match any of {self.allowed_groups}"
             )
@@ -125,8 +125,11 @@ async def user_is_authorized(self, auth_model):
             if not groups:
                 return False
 
+            all_allowed_groups = self.allowed_groups
+            if self.admin_groups:
+                all_allowed_groups += self.admin_groups
             if not self.user_groups_in_allowed_groups(
-                groups, self.allowed_groups + self.admin_groups
+                groups, all_allowed_groups
             ):
                 return False
 
diff --git a/oauthenticator/google.py b/oauthenticator/google.py
index 4e7317eb..e1ada523 100644
--- a/oauthenticator/google.py
+++ b/oauthenticator/google.py
@@ -133,7 +133,7 @@ async def user_is_authorized(self, auth_model):
                     403, f"Google account domain @{user_email_domain} not authorized."
                 )
 
-        if not self.allowed_users and (self.allowed_google_groups or self.admin_google_groups):
+        if self.allowed_google_groups:
             google_groups = self._google_groups_for_user(user_email, user_email_domain)
             if not google_groups:
                 return False
@@ -143,7 +143,9 @@ async def user_is_authorized(self, auth_model):
             # Check if user is a member of any allowed or admin groups.
             allowed_groups_per_domain = self.allowed_google_groups.get(
                 user_email_domain, []
-            ) + self.admin_google_groups.get(user_email_domain, [])
+            )
+            if self.admin_google_groups:
+                allowed_groups_per_domain += self.admin_google_groups.get(user_email_domain, [])
             if not allowed_groups_per_domain:
                 return False
             else:
diff --git a/oauthenticator/openshift.py b/oauthenticator/openshift.py
index 31c153df..121afb19 100644
--- a/oauthenticator/openshift.py
+++ b/oauthenticator/openshift.py
@@ -113,11 +113,14 @@ async def user_is_authorized(self, auth_model):
         user_groups = set(auth_model['auth_state']['openshift_user']['groups'])
         username = auth_model['name']
 
-        if not self.allowed_users and (self.allowed_groups or self.admin_groups):
+        if self.allowed_groups:
             msg = f"username:{username} User not in any of the allowed/admin groups"
             # User is authorized if either in allowed_groups or in admin_groups
+            all_allowed_groups = self.allowed_groups
+            if self.admin_groups:
+                all_allowed_groups = all_allowed_groups.unions(self.admin_groups)
             if not self.user_groups_in_allowed_groups(
-                user_groups, self.allowed_groups.union(self.admin_groups)
+                user_groups, all_allowed_groups
             ):
                 self.log.warning(msg)
                 return False

From 1e5992a1b166acb104dbe3c771646c7e28481a4f Mon Sep 17 00:00:00 2001
From: "pre-commit-ci[bot]"
 <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Date: Wed, 12 Apr 2023 13:05:37 +0000
Subject: [PATCH 13/57] [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci
---
 oauthenticator/generic.py   | 4 +---
 oauthenticator/google.py    | 4 +++-
 oauthenticator/openshift.py | 4 +---
 3 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/oauthenticator/generic.py b/oauthenticator/generic.py
index a79c7831..f1955b6d 100644
--- a/oauthenticator/generic.py
+++ b/oauthenticator/generic.py
@@ -128,9 +128,7 @@ async def user_is_authorized(self, auth_model):
             all_allowed_groups = self.allowed_groups
             if self.admin_groups:
                 all_allowed_groups += self.admin_groups
-            if not self.user_groups_in_allowed_groups(
-                groups, all_allowed_groups
-            ):
+            if not self.user_groups_in_allowed_groups(groups, all_allowed_groups):
                 return False
 
         return True
diff --git a/oauthenticator/google.py b/oauthenticator/google.py
index e1ada523..6dacdc51 100644
--- a/oauthenticator/google.py
+++ b/oauthenticator/google.py
@@ -145,7 +145,9 @@ async def user_is_authorized(self, auth_model):
                 user_email_domain, []
             )
             if self.admin_google_groups:
-                allowed_groups_per_domain += self.admin_google_groups.get(user_email_domain, [])
+                allowed_groups_per_domain += self.admin_google_groups.get(
+                    user_email_domain, []
+                )
             if not allowed_groups_per_domain:
                 return False
             else:
diff --git a/oauthenticator/openshift.py b/oauthenticator/openshift.py
index 121afb19..48367690 100644
--- a/oauthenticator/openshift.py
+++ b/oauthenticator/openshift.py
@@ -119,9 +119,7 @@ async def user_is_authorized(self, auth_model):
             all_allowed_groups = self.allowed_groups
             if self.admin_groups:
                 all_allowed_groups = all_allowed_groups.unions(self.admin_groups)
-            if not self.user_groups_in_allowed_groups(
-                user_groups, all_allowed_groups
-            ):
+            if not self.user_groups_in_allowed_groups(user_groups, all_allowed_groups):
                 self.log.warning(msg)
                 return False
 

From e43a9c259f4bdf492ce11bd5d90e7bcc4179a447 Mon Sep 17 00:00:00 2001
From: Georgiana Dolocan <georgiana.dolocan@gmail.com>
Date: Wed, 12 Apr 2023 16:53:51 +0300
Subject: [PATCH 14/57] Fix typo

---
 oauthenticator/openshift.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/oauthenticator/openshift.py b/oauthenticator/openshift.py
index 48367690..121afb19 100644
--- a/oauthenticator/openshift.py
+++ b/oauthenticator/openshift.py
@@ -119,7 +119,9 @@ async def user_is_authorized(self, auth_model):
             all_allowed_groups = self.allowed_groups
             if self.admin_groups:
                 all_allowed_groups = all_allowed_groups.unions(self.admin_groups)
-            if not self.user_groups_in_allowed_groups(user_groups, all_allowed_groups):
+            if not self.user_groups_in_allowed_groups(
+                user_groups, all_allowed_groups
+            ):
                 self.log.warning(msg)
                 return False
 

From 554fd76e5bf477f8f1e565b37c86836ba602e41d Mon Sep 17 00:00:00 2001
From: Georgiana Dolocan <georgiana.dolocan@gmail.com>
Date: Wed, 12 Apr 2023 16:57:29 +0300
Subject: [PATCH 15/57] Check if user was allowed through  before checking
 groups

---
 oauthenticator/generic.py   | 4 +++-
 oauthenticator/google.py    | 3 ++-
 oauthenticator/openshift.py | 3 ++-
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/oauthenticator/generic.py b/oauthenticator/generic.py
index f1955b6d..dc3226dd 100644
--- a/oauthenticator/generic.py
+++ b/oauthenticator/generic.py
@@ -116,7 +116,9 @@ def get_user_groups(self, user_info):
 
     async def user_is_authorized(self, auth_model):
         user_info = auth_model["auth_state"][self.user_auth_state_key]
-        if self.allowed_groups:
+        allowed_status = True if auth_model['name'] in self.allowed_users else None
+
+        if not allowed_status and self.allowed_groups:
             self.log.info(
                 f"Validating if user claim groups match any of {self.allowed_groups}"
             )
diff --git a/oauthenticator/google.py b/oauthenticator/google.py
index 6dacdc51..7439b3ad 100644
--- a/oauthenticator/google.py
+++ b/oauthenticator/google.py
@@ -133,7 +133,8 @@ async def user_is_authorized(self, auth_model):
                     403, f"Google account domain @{user_email_domain} not authorized."
                 )
 
-        if self.allowed_google_groups:
+        allowed_status = True if auth_model['name'] in self.allowed_users else None
+        if not allowed_status and self.allowed_google_groups:
             google_groups = self._google_groups_for_user(user_email, user_email_domain)
             if not google_groups:
                 return False
diff --git a/oauthenticator/openshift.py b/oauthenticator/openshift.py
index 121afb19..6f1f5228 100644
--- a/oauthenticator/openshift.py
+++ b/oauthenticator/openshift.py
@@ -112,8 +112,9 @@ async def user_is_authorized(self, auth_model):
         """
         user_groups = set(auth_model['auth_state']['openshift_user']['groups'])
         username = auth_model['name']
+        allowed_status = True if username in self.allowed_users else None
 
-        if self.allowed_groups:
+        if not allowed_status and self.allowed_groups:
             msg = f"username:{username} User not in any of the allowed/admin groups"
             # User is authorized if either in allowed_groups or in admin_groups
             all_allowed_groups = self.allowed_groups

From 43f60ae76dd414cf54e8db3d30b9b6f5c6609d4d Mon Sep 17 00:00:00 2001
From: "pre-commit-ci[bot]"
 <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Date: Wed, 12 Apr 2023 13:59:37 +0000
Subject: [PATCH 16/57] [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci
---
 oauthenticator/openshift.py | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/oauthenticator/openshift.py b/oauthenticator/openshift.py
index 6f1f5228..5fb2c68e 100644
--- a/oauthenticator/openshift.py
+++ b/oauthenticator/openshift.py
@@ -120,9 +120,7 @@ async def user_is_authorized(self, auth_model):
             all_allowed_groups = self.allowed_groups
             if self.admin_groups:
                 all_allowed_groups = all_allowed_groups.unions(self.admin_groups)
-            if not self.user_groups_in_allowed_groups(
-                user_groups, all_allowed_groups
-            ):
+            if not self.user_groups_in_allowed_groups(user_groups, all_allowed_groups):
                 self.log.warning(msg)
                 return False
 

From 15d62f8cf031ad636181915ca3ad2831cb2bb524 Mon Sep 17 00:00:00 2001
From: Georgiana Dolocan <georgiana.dolocan@gmail.com>
Date: Wed, 12 Apr 2023 17:11:32 +0300
Subject: [PATCH 17/57] Fix typo

---
 oauthenticator/openshift.py | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/oauthenticator/openshift.py b/oauthenticator/openshift.py
index 5fb2c68e..cd4a2b13 100644
--- a/oauthenticator/openshift.py
+++ b/oauthenticator/openshift.py
@@ -119,8 +119,10 @@ async def user_is_authorized(self, auth_model):
             # User is authorized if either in allowed_groups or in admin_groups
             all_allowed_groups = self.allowed_groups
             if self.admin_groups:
-                all_allowed_groups = all_allowed_groups.unions(self.admin_groups)
-            if not self.user_groups_in_allowed_groups(user_groups, all_allowed_groups):
+                all_allowed_groups = all_allowed_groups.union(self.admin_groups)
+            if not self.user_groups_in_allowed_groups(
+                user_groups, all_allowed_groups
+            ):
                 self.log.warning(msg)
                 return False
 

From 3c25adb1675939d12363532b5021c6fed604217c Mon Sep 17 00:00:00 2001
From: "pre-commit-ci[bot]"
 <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Date: Wed, 12 Apr 2023 14:14:58 +0000
Subject: [PATCH 18/57] [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci
---
 oauthenticator/openshift.py | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/oauthenticator/openshift.py b/oauthenticator/openshift.py
index cd4a2b13..9ee17386 100644
--- a/oauthenticator/openshift.py
+++ b/oauthenticator/openshift.py
@@ -120,9 +120,7 @@ async def user_is_authorized(self, auth_model):
             all_allowed_groups = self.allowed_groups
             if self.admin_groups:
                 all_allowed_groups = all_allowed_groups.union(self.admin_groups)
-            if not self.user_groups_in_allowed_groups(
-                user_groups, all_allowed_groups
-            ):
+            if not self.user_groups_in_allowed_groups(user_groups, all_allowed_groups):
                 self.log.warning(msg)
                 return False
 

From c5b37629e6ba28ee5c9e152a889f6936176e5911 Mon Sep 17 00:00:00 2001
From: Erik Sundell <erik.i.sundell@gmail.com>
Date: Sun, 23 Apr 2023 12:50:20 +0200
Subject: [PATCH 19/57] Rework attempt before considering overriding
 Authenticator.check_allowed

---
 oauthenticator/bitbucket.py |  19 +++----
 oauthenticator/generic.py   |  85 ++++++++++++++--------------
 oauthenticator/github.py    |   2 +-
 oauthenticator/google.py    | 110 ++++++++++++++++--------------------
 oauthenticator/oauth2.py    |  42 +++++++-------
 oauthenticator/openshift.py |  54 +++++++++---------
 6 files changed, 151 insertions(+), 161 deletions(-)

diff --git a/oauthenticator/bitbucket.py b/oauthenticator/bitbucket.py
index 53e53fb6..3c2d1dc8 100644
--- a/oauthenticator/bitbucket.py
+++ b/oauthenticator/bitbucket.py
@@ -43,23 +43,23 @@ def _userdata_url_default(self):
     async def user_is_authorized(self, auth_model):
         access_token = auth_model["auth_state"]["token_response"]["access_token"]
         token_type = auth_model["auth_state"]["token_response"]["token_type"]
+
         username = auth_model["name"]
+        if username in (self.allowed_users | self.admin_users):
+            return True
 
-        # Check if user is a member of any allowed teams.
-        # This check is performed here, as the check requires `access_token`.
         if self.allowed_teams:
-            user_in_team = await self._check_membership_allowed_teams(
+            return await self._check_membership_allowed_teams(
                 username, access_token, token_type
             )
-            if not user_in_team:
-                self.log.warning(f"{username} not in team allowed list of users")
-                return False
 
-        return True
+        return False
 
     async def _check_membership_allowed_teams(self, username, access_token, token_type):
+        """
+        Verify team membership by calling bitbucket API.
+        """
         headers = self.build_userdata_request_headers(access_token, token_type)
-        # We verify the team membership by calling teams endpoint.
         next_page = url_concat(
             "https://api.bitbucket.org/2.0/workspaces", {'role': 'member'}
         )
@@ -68,8 +68,7 @@ async def _check_membership_allowed_teams(self, username, access_token, token_ty
             next_page = resp_json.get('next', None)
 
             user_teams = {entry["name"] for entry in resp_json["values"]}
-            # check if any of the organizations seen thus far are in the allowed list
-            if len(self.allowed_teams & user_teams) > 0:
+            if any(user_teams & self.allowed_team):
                 return True
         return False
 
diff --git a/oauthenticator/generic.py b/oauthenticator/generic.py
index dc3226dd..54c00837 100644
--- a/oauthenticator/generic.py
+++ b/oauthenticator/generic.py
@@ -7,7 +7,7 @@
 from jupyterhub.auth import LocalAuthenticator
 from jupyterhub.traitlets import Callable
 from tornado.httpclient import AsyncHTTPClient
-from traitlets import Bool, Dict, List, Unicode, Union, default
+from traitlets import Bool, Dict, Set, Unicode, Union, default
 
 from .oauth2 import OAuthenticator
 
@@ -36,13 +36,13 @@ class GenericOAuthenticator(OAuthenticator):
         """,
     )
 
-    allowed_groups = List(
+    allowed_groups = Set(
         Unicode(),
         config=True,
         help="Automatically allow members of selected groups",
     )
 
-    admin_groups = List(
+    admin_groups = Set(
         Unicode(),
         config=True,
         help="Groups whose members should have Jupyterhub admin privileges",
@@ -96,55 +96,58 @@ def user_info_to_username(self, user_info):
         return username
 
     def get_user_groups(self, user_info):
+        """
+        Returns a set of groups the user belongs to based on claim_groups_key
+        and provided user_info.
+
+        - If claim_groups_key is a callable, it is meant to return the groups
+          directly.
+        - If claim_groups_key is a nested dictionary key like
+          "permissions.groups", this function returns
+          user_info["permissions"]["groups"].
+        """
         if callable(self.claim_groups_key):
-            groups = self.claim_groups_key(user_info)
-        else:
-            try:
-                groups = reduce(dict.get, self.claim_groups_key.split("."), user_info)
-            except TypeError:
-                # This happens if a nested key does not exist (reduce trying to call None.get)
-                self.log.error(
-                    f"The key {self.claim_groups_key} does not exist in the user token, or it is set to null"
-                )
-                groups = None
-
-        if not groups:
+            return set(self.claim_groups_key(user_info))
+        try:
+            return reduce(dict.get, self.claim_groups_key.split("."), user_info)
+        except TypeError:
             self.log.error(
-                f"No claim groups found for user! Something wrong with the `claim_groups_key` {self.claim_groups_key}? {user_info}"
+                f"The claim_groups_key {self.claim_groups_key} does not exist in the user token"
             )
-        return groups
+            return set()
 
     async def user_is_authorized(self, auth_model):
+        """
+        A user is authorized by being part of allowed_users, admin_users,
+        allowed_groups, or admin_groups.
+        """
         user_info = auth_model["auth_state"][self.user_auth_state_key]
-        allowed_status = True if auth_model['name'] in self.allowed_users else None
-
-        if not allowed_status and self.allowed_groups:
-            self.log.info(
-                f"Validating if user claim groups match any of {self.allowed_groups}"
-            )
 
-            groups = self.get_user_groups(user_info)
-            if not groups:
-                return False
+        username = auth_model["name"]
+        if username in (self.allowed_users | self.admin_users):
+            return True
 
-            all_allowed_groups = self.allowed_groups
-            if self.admin_groups:
-                all_allowed_groups += self.admin_groups
-            if not self.user_groups_in_allowed_groups(groups, all_allowed_groups):
-                return False
-
-        return True
+        user_groups = self.get_user_groups(user_info)
+        return any(user_groups & (self.allowed_groups | self.admin_groups))
 
     async def update_auth_model(self, auth_model):
+        """
+        Set the admin status based on finding the username in `admin_users` or
+        finding a user group part of `admin_groups`.
+        """
         user_info = auth_model["auth_state"][self.user_auth_state_key]
-        admin_status = True if auth_model['name'] in self.admin_users else None
-        # Check if user has been marked as admin by membership in self.admin_groups
-        if not admin_status and self.admin_groups:
-            groups = self.get_user_groups(user_info)
-            if groups:
-                auth_model['admin'] = self.user_groups_in_allowed_groups(
-                    groups, self.admin_groups
-                )
+
+        username = auth_model["name"]
+        if username in self.admin_users:
+            auth_model["admin"] = True
+            return auth_model
+
+        if self.admin_groups:
+            # admin_groups are declared and the user wasn't part of admin_users,
+            # so we set admin to True or False to allow a user removed from an
+            # admin_groups to no longer be an admin.
+            user_groups = self.get_user_groups(user_info)
+            auth_model["admin"] = any(user_groups & self.admin_groups)
 
         return auth_model
 
diff --git a/oauthenticator/github.py b/oauthenticator/github.py
index 245f5af4..acb3d7da 100644
--- a/oauthenticator/github.py
+++ b/oauthenticator/github.py
@@ -130,9 +130,9 @@ def _github_client_secret_changed(self, name, old, new):
 
     async def user_is_authorized(self, auth_model):
         # Check if user is a member of any allowed organizations.
-        # This check is performed here, as it requires `access_token`.
         access_token = auth_model["auth_state"]["token_response"]["access_token"]
         token_type = auth_model["auth_state"]["token_response"]["token_type"]
+
         if self.allowed_organizations:
             for org in self.allowed_organizations:
                 user_in_org = await self._check_membership_allowed_organizations(
diff --git a/oauthenticator/google.py b/oauthenticator/google.py
index 7439b3ad..6346489a 100644
--- a/oauthenticator/google.py
+++ b/oauthenticator/google.py
@@ -8,7 +8,7 @@
 from jupyterhub.auth import LocalAuthenticator
 from tornado.auth import GoogleOAuth2Mixin
 from tornado.web import HTTPError
-from traitlets import Dict, List, Unicode, default, validate
+from traitlets import Dict, List, Set, Unicode, default, validate
 
 from .oauth2 import OAuthenticator
 
@@ -70,11 +70,11 @@ def _userdata_url_default(self):
     )
 
     allowed_google_groups = Dict(
-        List(Unicode()), help="Automatically allow members of selected groups"
+        Set(Unicode()), help="Automatically allow members of selected groups"
     ).tag(config=True)
 
     admin_google_groups = Dict(
-        List(Unicode()),
+        Set(Unicode()),
         help="Groups whose members should have Jupyterhub admin privileges",
     ).tag(config=True)
 
@@ -117,65 +117,55 @@ def _cast_hosted_domain(self, proposal):
     )
 
     async def user_is_authorized(self, auth_model):
-        user_email = auth_model["auth_state"][self.user_auth_state_key]['email']
-        user_email_domain = user_email.split('@')[1]
+        """
+        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.
+        """
+        user_info = auth_model["auth_state"][self.user_auth_state_key]
+        user_email = user_info["email"]
+        user_domain = user_email.split("@")[1]
 
-        if not auth_model["auth_state"][self.user_auth_state_key]['verified_email']:
+        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:
-            if user_email_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_email_domain} not authorized."
-                )
-
-        allowed_status = True if auth_model['name'] in self.allowed_users else None
-        if not allowed_status and self.allowed_google_groups:
-            google_groups = self._google_groups_for_user(user_email, user_email_domain)
-            if not google_groups:
-                return False
-
-            auth_model['auth_state']['google_user']['google_groups'] = google_groups
-
-            # Check if user is a member of any allowed or admin groups.
-            allowed_groups_per_domain = self.allowed_google_groups.get(
-                user_email_domain, []
-            )
-            if self.admin_google_groups:
-                allowed_groups_per_domain += self.admin_google_groups.get(
-                    user_email_domain, []
-                )
-            if not allowed_groups_per_domain:
-                return False
-            else:
-                return self.user_groups_in_allowed_groups(
-                    google_groups, allowed_groups_per_domain
-                )
-
-        return True
-
-    async def update_auth_model(self, auth_model):
+        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"]
-        admin_status = True if username in self.admin_users else None
-        if not admin_status and self.admin_google_groups:
-            user_email = auth_model["auth_state"][self.user_auth_state_key]['email']
-            user_email_domain = user_email.split('@')[1]
-
-            if user_email_domain in self.admin_google_groups.keys():
-                # Check if user is a member of any admin groups.
-                google_groups = self._google_groups_for_user(
-                    user_email, user_email_domain
-                )
-                if google_groups:
-                    auth_model['admin'] = self.user_groups_in_allowed_groups(
-                        google_groups, self.admin_google_groups[user_email_domain]
-                    )
-
-        return auth_model
+        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)):
+                return True
+
+        return username in (self.allowed_users | self.admin_users)
 
     def _service_client_credentials(self, scopes, user_email_domain):
         """
@@ -227,7 +217,7 @@ def _service_client(self, service_name, service_version, credentials, http=None)
 
     def _google_groups_for_user(self, user_email, user_email_domain, http=None):
         """
-        Return a list with the google groups a given user is a member of
+        Return a set with the google groups a given user is a member of
         """
         credentials = self._service_client_credentials(
             scopes=[f"{self.google_api_url}/auth/admin.directory.group.readonly"],
@@ -241,9 +231,9 @@ def _google_groups_for_user(self, user_email, user_email_domain, http=None):
         )
 
         results = service.groups().list(userKey=user_email).execute()
-        results = [
+        results = {
             g['email'].split('@')[0] for g in results.get('groups', [{'email': None}])
-        ]
+        }
         self.log.debug(f"user_email {user_email} is a member of {results}")
         return results
 
diff --git a/oauthenticator/oauth2.py b/oauthenticator/oauth2.py
index 430aa862..24984660 100644
--- a/oauthenticator/oauth2.py
+++ b/oauthenticator/oauth2.py
@@ -733,6 +733,17 @@ def build_auth_state_dict(self, token_info, user_info):
             self.user_auth_state_key: user_info,
         }
 
+    async def user_is_authorized(self, auth_model):
+        """
+        Checks if the user that is authenticating should be authorized or not and False otherwise.
+        Should be overridden with any relevant logic specific to each oauthenticator.
+
+        Returns True by default.
+
+        Called by the :meth:`oauthenticator.OAuthenticator.authenticate`
+        """
+        return True
+
     async def update_auth_model(self, auth_model):
         """
         Updates `auth_model` dict if any fields have changed or additional information is available
@@ -751,30 +762,17 @@ async def update_auth_model(self, auth_model):
         """
         return auth_model
 
-    async def user_is_authorized(self, auth_model):
+    async def authenticate(self, handler, data=None, **kwargs):
         """
-        Checks if the user that is authenticating should be authorized or not and False otherwise.
-        Should be overridden with any relevant logic specific to each oauthenticator.
+        A JupyterHub Authenticator's authenticate method's job is:
 
-        Returns True by default.
+        - return None if the user isn't successfully authenticated
+        - return a dictionary of if authentication is successful with name,
+          admin (optional), and auth_state (optional)
 
-        Called by the :meth:`oauthenticator.OAuthenticator.authenticate`
+        ref: https://jupyterhub.readthedocs.io/en/stable/reference/authenticators.html#authenticator-authenticate-method
+        ref: https://github.com/jupyterhub/jupyterhub/blob/4.0.0/jupyterhub/auth.py#L581-L611
         """
-        return True
-
-    @staticmethod
-    def user_groups_in_allowed_groups(user_groups, allowed_groups):
-        """
-        Returns True if user is a member of any group in the allowed groups,
-        and False otherwise
-        """
-        if not isinstance(user_groups, set):
-            user_groups = set(user_groups)
-        if not isinstance(allowed_groups, set):
-            allowed_groups = set(allowed_groups)
-        return any((user_groups).intersection(allowed_groups))
-
-    async def authenticate(self, handler, data=None, **kwargs):
         # build the parameters to be used in the request exchanging the oauth code for the access token
         access_token_params = self.build_access_tokens_request_params(handler, data)
         # exchange the oauth code for an access token and get the JSON with info about it
@@ -794,15 +792,17 @@ async def authenticate(self, handler, data=None, **kwargs):
             if refresh_token:
                 token_info["refresh_token"] = refresh_token
 
-        # build the auth model to be persisted if authentication goes right
+        # build the auth model to be read if authentication goes right
         auth_model = {
             "name": username,
+            "admin": None,
             "auth_state": self.build_auth_state_dict(token_info, user_info),
         }
 
         # check if the username that's authenticating should be authorized
         authorized = await self.user_is_authorized(auth_model)
         if not authorized:
+            self.log.warning(f"User {username} wasn't authorized")
             return None
 
         # update the auth model with any info if available
diff --git a/oauthenticator/openshift.py b/oauthenticator/openshift.py
index 9ee17386..b32b43c2 100644
--- a/oauthenticator/openshift.py
+++ b/oauthenticator/openshift.py
@@ -89,42 +89,40 @@ def _userdata_url_default(self):
     def user_info_to_username(self, user_info):
         return user_info['metadata']['name']
 
-    async def update_auth_model(self, auth_model):
+    async def user_is_authorized(self, auth_model):
         """
         Use the group info stored on the OpenShift User object to determine if a user
-        is an admin and update the auth_model with this info.
+        is authorized to login.
         """
-        user_groups = set(auth_model['auth_state']['openshift_user']['groups'])
-        admin_status = True if auth_model['name'] in self.admin_users else None
+        user_info = auth_model["auth_state"][self.user_auth_state_key]
 
-        # Check if user has been marked as admin by membership in self.admin_groups
-        if not admin_status and self.admin_groups:
-            auth_model['admin'] = self.user_groups_in_allowed_groups(
-                user_groups, self.admin_groups
-            )
+        username = auth_model["name"]
+        if username in (self.allowed_users | self.admin_users):
+            return True
 
-        return auth_model
+        user_groups = set(user_info["groups"])
+        return any(user_groups & (self.allowed_groups | self.admin_groups))
 
-    async def user_is_authorized(self, auth_model):
+    async def update_auth_model(self, auth_model):
         """
-        Use the group info stored on the OpenShift User object to determine if a user
-        is authorized to login.
+        Set the admin status based on finding the username in `admin_users` or
+        finding a user group part of `admin_groups`.
         """
-        user_groups = set(auth_model['auth_state']['openshift_user']['groups'])
-        username = auth_model['name']
-        allowed_status = True if username in self.allowed_users else None
-
-        if not allowed_status and self.allowed_groups:
-            msg = f"username:{username} User not in any of the allowed/admin groups"
-            # User is authorized if either in allowed_groups or in admin_groups
-            all_allowed_groups = self.allowed_groups
-            if self.admin_groups:
-                all_allowed_groups = all_allowed_groups.union(self.admin_groups)
-            if not self.user_groups_in_allowed_groups(user_groups, all_allowed_groups):
-                self.log.warning(msg)
-                return False
-
-        return True
+        user_info = auth_model["auth_state"][self.user_auth_state_key]
+
+        username = auth_model["name"]
+        if username in self.admin_users:
+            auth_model["admin"] = True
+            return auth_model
+
+        if self.admin_groups:
+            # admin_groups are declared and the user wasn't part of admin_users,
+            # so we set admin to True or False to allow a user removed from an
+            # admin_groups to no longer be an admin.
+            user_groups = set(user_info["groups"])
+            auth_model["admin"] = any(user_groups & self.admin_groups)
+
+        return auth_model
 
 
 class LocalOpenShiftOAuthenticator(LocalAuthenticator, OpenShiftOAuthenticator):

From 41a0db228f552e6b0e56ff7e932518fe08dc8f86 Mon Sep 17 00:00:00 2001
From: Erik Sundell <erik.i.sundell@gmail.com>
Date: Mon, 24 Apr 2023 01:23:56 +0200
Subject: [PATCH 20/57] Draft work with check_allowed

---
 docs/source/how-to/example-oauthenticator.py |  4 +-
 oauthenticator/bitbucket.py                  | 65 ++++++++++++-----
 oauthenticator/cilogon.py                    | 57 ++++++++-------
 oauthenticator/generic.py                    | 52 ++++++++------
 oauthenticator/oauth2.py                     | 73 +++++++++++---------
 oauthenticator/openshift.py                  | 50 ++++++++------
 6 files changed, 178 insertions(+), 123 deletions(-)

diff --git a/docs/source/how-to/example-oauthenticator.py b/docs/source/how-to/example-oauthenticator.py
index 822aecbc..c139ee14 100644
--- a/docs/source/how-to/example-oauthenticator.py
+++ b/docs/source/how-to/example-oauthenticator.py
@@ -88,9 +88,9 @@ def build_auth_state_dict(self, token_info, user_info):
     # Updates `auth_model` dict if any fields have changed or additional information is available
     # or returns the unchanged `auth_model`.
     # Returns the model unchanged by default.
-    # Should be overridden to take into account  additional checks such as against group/admin/team membership.
+    # Should be overridden to take into account additional checks such as against group/admin/team membership.
     # if the OAuth provider has such a concept
-    async def update_auth_model(self, auth_model, **kwargs):
+    async def update_auth_model(self, username, auth_model):
         pass
 
 
diff --git a/oauthenticator/bitbucket.py b/oauthenticator/bitbucket.py
index 3c2d1dc8..e6dd39fb 100644
--- a/oauthenticator/bitbucket.py
+++ b/oauthenticator/bitbucket.py
@@ -40,37 +40,64 @@ def _userdata_url_default(self):
         config=True, help="Automatically allow members of selected teams"
     )
 
-    async def user_is_authorized(self, auth_model):
+    async def _fetch_user_teams(self, access_token, token_type):
+        """
+        Get user's team memberships via bitbucket's API.
+        """
+        headers = self.build_userdata_request_headers(access_token, token_type)
+        next_page = url_concat(
+            "https://api.bitbucket.org/2.0/workspaces", {'role': 'member'}
+        )
+
+        user_teams = set()
+        while next_page:
+            resp_json = await self.httpfetch(next_page, method="GET", headers=headers)
+            next_page = resp_json.get('next', None)
+            user_teams |= {entry["name"] for entry in resp_json["values"]}
+        return user_teams
+
+    async def update_auth_model(self, auth_model):
+        """
+        Set the admin status based on finding the username in `admin_users` and
+        fetch user teams if `allowed_teams` is configured.
+        """
         access_token = auth_model["auth_state"]["token_response"]["access_token"]
         token_type = auth_model["auth_state"]["token_response"]["token_type"]
 
         username = auth_model["name"]
-        if username in (self.allowed_users | self.admin_users):
-            return True
+        if username in self.admin_users:
+            auth_model["admin"] = True
 
         if self.allowed_teams:
-            return await self._check_membership_allowed_teams(
-                username, access_token, token_type
-            )
+            user_teams = self._fetch_user_teams(access_token, token_type)
+            auth_model["auth_state"]["user_teams"] = user_teams
 
-        return False
+        return auth_model
 
-    async def _check_membership_allowed_teams(self, username, access_token, token_type):
+    async def check_allowed(self, username, auth_model):
         """
-        Verify team membership by calling bitbucket API.
+        Returns True for users allowed to be authorized.
+
+        Overrides the OAuthenticator.check_allowed implementation to allow users
+        either part of `allowed_users` or `allowed_teams`, and not just those
+        part of `allowed_users`.
         """
-        headers = self.build_userdata_request_headers(access_token, token_type)
-        next_page = url_concat(
-            "https://api.bitbucket.org/2.0/workspaces", {'role': 'member'}
-        )
-        while next_page:
-            resp_json = await self.httpfetch(next_page, method="GET", headers=headers)
-            next_page = resp_json.get('next', None)
+        # allow admin users recognized via admin_users or update_auth_model
+        if auth_model["admin"]:
+            return True
 
-            user_teams = {entry["name"] for entry in resp_json["values"]}
-            if any(user_teams & self.allowed_team):
+        # if allowed_users or allowed_teams is configured, we deny users not
+        # part of either
+        if self.allowed_users or self.allowed_teams:
+            user_teams = auth_model["auth_state"]["user_teams"]
+            if username in self.allowed_users:
+                return True
+            if any(user_teams & self.allowed_teams):
                 return True
-        return False
+            return False
+
+        # otherwise, authorize all users
+        return True
 
 
 class LocalBitbucketOAuthenticator(LocalAuthenticator, BitbucketOAuthenticator):
diff --git a/oauthenticator/cilogon.py b/oauthenticator/cilogon.py
index 852f5e72..28be578b 100644
--- a/oauthenticator/cilogon.py
+++ b/oauthenticator/cilogon.py
@@ -275,35 +275,6 @@ def user_info_to_username(self, user_info):
             )
             raise web.HTTPError(500, "Failed to get username from CILogon")
 
-    async def user_is_authorized(self, auth_model):
-        username = auth_model["name"]
-        # Check if selected idp was marked as allowed
-        if self.allowed_idps:
-            selected_idp = auth_model["auth_state"][self.user_auth_state_key].get("idp")
-            # Fail hard if idp wasn't allowed
-            if selected_idp not in self.allowed_idps.keys():
-                self.log.error(
-                    f"Trying to login from an identity provider that was not allowed {selected_idp}",
-                )
-                raise web.HTTPError(
-                    500,
-                    "Trying to login using an identity provider that was not allowed",
-                )
-
-            allowed_domains = self.allowed_idps[selected_idp].get(
-                "allowed_domains", None
-            )
-
-            if allowed_domains:
-                gotten_name, gotten_domain = username.split('@')
-                if gotten_domain not in allowed_domains:
-                    raise web.HTTPError(
-                        500,
-                        "Trying to login using a domain that was not allowed",
-                    )
-
-        return True
-
     async def update_auth_model(self, auth_model):
         selected_idp = auth_model["auth_state"][self.user_auth_state_key].get("idp")
 
@@ -330,6 +301,34 @@ async def update_auth_model(self, auth_model):
         auth_model["name"] = username
         return auth_model
 
+    async def check_allowed(self, username, auth_model):
+        # Check if selected idp was marked as allowed
+        if self.allowed_idps:
+            selected_idp = auth_model["auth_state"][self.user_auth_state_key].get("idp")
+            # Fail hard if idp wasn't allowed
+            if selected_idp not in self.allowed_idps:
+                self.log.error(
+                    f"Trying to login from an identity provider that was not allowed {selected_idp}",
+                )
+                raise web.HTTPError(
+                    500,
+                    "Trying to login using an identity provider that was not allowed",
+                )
+
+            allowed_domains = self.allowed_idps[selected_idp].get(
+                "allowed_domains", None
+            )
+
+            if allowed_domains:
+                gotten_domain = username.split('@')[1]
+                if gotten_domain not in allowed_domains:
+                    raise web.HTTPError(
+                        500,
+                        "Trying to login using a domain that was not allowed",
+                    )
+
+        return True
+
 
 class LocalCILogonOAuthenticator(LocalAuthenticator, CILogonOAuthenticator):
 
diff --git a/oauthenticator/generic.py b/oauthenticator/generic.py
index 54c00837..cc3216af 100644
--- a/oauthenticator/generic.py
+++ b/oauthenticator/generic.py
@@ -70,7 +70,7 @@ class GenericOAuthenticator(OAuthenticator):
     tls_verify = Bool(
         os.environ.get('OAUTH2_TLS_VERIFY', 'True').lower() in {'true', '1'},
         config=True,
-        help="Disable TLS verification on http request",
+        help="Require valid tls certificates in HTTP requests",
     )
 
     @default("basic_auth")
@@ -116,20 +116,6 @@ def get_user_groups(self, user_info):
             )
             return set()
 
-    async def user_is_authorized(self, auth_model):
-        """
-        A user is authorized by being part of allowed_users, admin_users,
-        allowed_groups, or admin_groups.
-        """
-        user_info = auth_model["auth_state"][self.user_auth_state_key]
-
-        username = auth_model["name"]
-        if username in (self.allowed_users | self.admin_users):
-            return True
-
-        user_groups = self.get_user_groups(user_info)
-        return any(user_groups & (self.allowed_groups | self.admin_groups))
-
     async def update_auth_model(self, auth_model):
         """
         Set the admin status based on finding the username in `admin_users` or
@@ -140,17 +126,41 @@ async def update_auth_model(self, auth_model):
         username = auth_model["name"]
         if username in self.admin_users:
             auth_model["admin"] = True
-            return auth_model
-
-        if self.admin_groups:
-            # admin_groups are declared and the user wasn't part of admin_users,
-            # so we set admin to True or False to allow a user removed from an
-            # admin_groups to no longer be an admin.
+        elif self.admin_groups:
+            # if admin_groups is configured, we must either set or unset admin
+            # status and never leave it at None, otherwise removing a user from
+            # the admin_groups won't have an effect
             user_groups = self.get_user_groups(user_info)
             auth_model["admin"] = any(user_groups & self.admin_groups)
 
         return auth_model
 
+    async def check_allowed(self, username, auth_model):
+        """
+        Returns True for users allowed to be authorized.
+
+        Overrides the OAuthenticator.check_allowed implementation to allow users
+        either part of `allowed_users` or `allowed_groups`, 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
+
+        # if allowed_users or allowed_groups is configured, we deny users not
+        # part of either
+        if self.allowed_users or self.allowed_groups:
+            user_info = auth_model["auth_state"][self.user_auth_state_key]
+            user_groups = self.get_user_groups(user_info)
+            if username in self.allowed_users:
+                return True
+            if any(user_groups & self.allowed_groups):
+                return True
+            return False
+
+        # otherwise, authorize all users
+        return True
+
 
 class LocalGenericOAuthenticator(LocalAuthenticator, GenericOAuthenticator):
     """A version that mixes in local system user creation"""
diff --git a/oauthenticator/oauth2.py b/oauthenticator/oauth2.py
index 24984660..a0b4e98a 100644
--- a/oauthenticator/oauth2.py
+++ b/oauthenticator/oauth2.py
@@ -555,7 +555,6 @@ def build_token_info_request_headers(self):
     def user_info_to_username(self, user_info):
         """
         Gets the self.username_claim key's value from the user_info dictionary.
-        This is equivalent to the JupyterHub username.
 
         Should be overridden by the authenticators for which the hub username cannot
         be extracted this way and needs extra processing.
@@ -733,29 +732,17 @@ def build_auth_state_dict(self, token_info, user_info):
             self.user_auth_state_key: user_info,
         }
 
-    async def user_is_authorized(self, auth_model):
+    async def update_auth_model(self, username, auth_model):
         """
-        Checks if the user that is authenticating should be authorized or not and False otherwise.
-        Should be overridden with any relevant logic specific to each oauthenticator.
+        Updates and returns the `auth_model` dict.
 
-        Returns True by default.
+        Should be overridden to collect information required for check_allowed.
 
-        Called by the :meth:`oauthenticator.OAuthenticator.authenticate`
-        """
-        return True
-
-    async def update_auth_model(self, auth_model):
-        """
-        Updates `auth_model` dict if any fields have changed or additional information is available
-        or returns the unchanged `auth_model`.
-
-        Returns the model unchanged by default.
-
-        Should be overridden to take into account changes like group/admin membership.
-
-        Args: auth_model - the auth model dictionary  dict instead, containing:
-            - the `name` key holding the username
-            - the `auth_state` key, the dictionary of of auth state
+        Args: auth_model - the auth model dictionary, containing:
+            - `name`: the normalized username
+            - `admin`: the admin status (True/False/None), where None means it
+                should be unchanged.
+            - `auth_state`: the dictionary of of auth state
                 returned by :meth:`oauthenticator.OAuthenticator.build_auth_state_dict`
 
         Called by the :meth:`oauthenticator.OAuthenticator.authenticate`
@@ -767,8 +754,10 @@ async def authenticate(self, handler, data=None, **kwargs):
         A JupyterHub Authenticator's authenticate method's job is:
 
         - return None if the user isn't successfully authenticated
-        - return a dictionary of if authentication is successful with name,
-          admin (optional), and auth_state (optional)
+        - return a dictionary if authentication is successful with name, admin
+          (optional), and auth_state (optional)
+
+        Subclasses should not override this method.
 
         ref: https://jupyterhub.readthedocs.io/en/stable/reference/authenticators.html#authenticator-authenticate-method
         ref: https://github.com/jupyterhub/jupyterhub/blob/4.0.0/jupyterhub/auth.py#L581-L611
@@ -779,8 +768,9 @@ async def authenticate(self, handler, data=None, **kwargs):
         token_info = await self.get_token_info(handler, access_token_params)
         # use the access_token to get userdata info
         user_info = await self.token_to_user(token_info)
-        # extract the username out of the user_info dict
+        # extract the username out of the user_info dict and normalize it
         username = self.user_info_to_username(user_info)
+        username = self.normalize_username(username)
 
         # check if there any refresh_token in the token_info dict
         refresh_token = token_info.get("refresh_token", None)
@@ -795,19 +785,38 @@ async def authenticate(self, handler, data=None, **kwargs):
         # build the auth model to be read if authentication goes right
         auth_model = {
             "name": username,
-            "admin": None,
+            "admin": True if username in self.admin_users else None,
             "auth_state": self.build_auth_state_dict(token_info, user_info),
         }
 
-        # check if the username that's authenticating should be authorized
-        authorized = await self.user_is_authorized(auth_model)
-        if not authorized:
-            self.log.warning(f"User {username} wasn't authorized")
-            return None
-
-        # update the auth model with any info if available
+        # update the auth_model with info to later authorize the user in
+        # check_allowed, such as admin status and group memberships
         return await self.update_auth_model(auth_model)
 
+    async def check_allowed(self, username, auth_model):
+        """
+        Returns True for users allowed to be authorized
+
+        Overrides Authenticator.check_allowed that is called from
+        `Authenticator.get_authenticated_user` after
+        `OAuthenticator.authenticate` has been called, and therefore also after
+        `update_auth_model` has been called.
+
+        Subclasses with authorization logic involving allowed groups should
+        override this.
+        """
+        # authorize users to become admins by admin_users or logic in
+        # update_auth_model
+        if auth_model["admin"]:
+            return True
+
+        # if allowed_users is configured, authorize/unauthorize based on that
+        if self.allowed_users:
+            return username in self.allowed_users
+
+        # otherwise, authorize all users
+        return True
+
     _deprecated_oauth_aliases = {}
 
     def _deprecated_oauth_trait(self, change):
diff --git a/oauthenticator/openshift.py b/oauthenticator/openshift.py
index b32b43c2..bcfef1a8 100644
--- a/oauthenticator/openshift.py
+++ b/oauthenticator/openshift.py
@@ -89,20 +89,6 @@ def _userdata_url_default(self):
     def user_info_to_username(self, user_info):
         return user_info['metadata']['name']
 
-    async def user_is_authorized(self, auth_model):
-        """
-        Use the group info stored on the OpenShift User object to determine if a user
-        is authorized to login.
-        """
-        user_info = auth_model["auth_state"][self.user_auth_state_key]
-
-        username = auth_model["name"]
-        if username in (self.allowed_users | self.admin_users):
-            return True
-
-        user_groups = set(user_info["groups"])
-        return any(user_groups & (self.allowed_groups | self.admin_groups))
-
     async def update_auth_model(self, auth_model):
         """
         Set the admin status based on finding the username in `admin_users` or
@@ -113,17 +99,41 @@ async def update_auth_model(self, auth_model):
         username = auth_model["name"]
         if username in self.admin_users:
             auth_model["admin"] = True
-            return auth_model
-
-        if self.admin_groups:
-            # admin_groups are declared and the user wasn't part of admin_users,
-            # so we set admin to True or False to allow a user removed from an
-            # admin_groups to no longer be an admin.
+        elif self.admin_groups:
+            # if admin_groups is configured, we must either set or unset admin
+            # status and never leave it at None, otherwise removing a user from
+            # the admin_groups won't have an effect
             user_groups = set(user_info["groups"])
             auth_model["admin"] = any(user_groups & self.admin_groups)
 
         return auth_model
 
+    async def check_allowed(self, username, auth_model):
+        """
+        Returns True for users allowed to be authorized.
+
+        Overrides the OAuthenticator.check_allowed implementation to allow users
+        either part of `allowed_users` or `allowed_groups`, 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
+
+        # if allowed_users or allowed_groups is configured, we deny users not
+        # part of either
+        if self.allowed_users or self.allowed_groups:
+            user_info = auth_model["auth_state"][self.user_auth_state_key]
+            user_groups = set(user_info["groups"])
+            if username in self.allowed_users:
+                return True
+            if any(user_groups & self.allowed_groups):
+                return True
+            return False
+
+        # otherwise, authorize all users
+        return True
+
 
 class LocalOpenShiftOAuthenticator(LocalAuthenticator, OpenShiftOAuthenticator):
 

From a3cf070247fcaebc68170f922330e099a953cd6a Mon Sep 17 00:00:00 2001
From: Erik Sundell <erik.i.sundell@gmail.com>
Date: Tue, 25 Apr 2023 03:15:17 +0200
Subject: [PATCH 21/57] Additional progress on cilogon and mediawiki

---
 oauthenticator/cilogon.py   | 120 +++++++++++++++++++++---------------
 oauthenticator/mediawiki.py |   6 +-
 2 files changed, 71 insertions(+), 55 deletions(-)

diff --git a/oauthenticator/cilogon.py b/oauthenticator/cilogon.py
index 28be578b..4a04ae88 100644
--- a/oauthenticator/cilogon.py
+++ b/oauthenticator/cilogon.py
@@ -249,64 +249,93 @@ def _validate_allowed_idps(self, proposal):
     )
 
     def user_info_to_username(self, user_info):
-        claimlist = [self.username_claim]
-        if self.additional_username_claims:
-            claimlist.extend(self.additional_username_claims)
+        selected_idp = user_info["idp"]
 
+        username_claims = [self.username_claim]
+        if self.additional_username_claims:
+            username_claims.extend(self.additional_username_claims)
         if self.allowed_idps:
-            selected_idp = user_info.get("idp")
-            if selected_idp:
-                # The username_claim which should be used for this idp
-                claimlist = [
-                    self.allowed_idps[selected_idp]["username_derivation"][
-                        "username_claim"
-                    ]
-                ]
-
-        for claim in claimlist:
+            # The username_claim which should be used for this idp
+            username_claims = [
+                self.allowed_idps[selected_idp]["username_derivation"]["username_claim"]
+            ]
+
+        username = None
+        for claim in username_claims:
             username = user_info.get(claim)
             if username:
-                return username
+                break
 
         if not username:
             user_info_keys = sorted(user_info.keys())
             self.log.error(
-                f"No username claim in the list at {claimlist} was found in the response {user_info_keys}"
+                f"No username claim in the list at {username_claims} was found in the response {user_info_keys}"
             )
             raise web.HTTPError(500, "Failed to get username from CILogon")
 
-    async def update_auth_model(self, auth_model):
-        selected_idp = auth_model["auth_state"][self.user_auth_state_key].get("idp")
-
-        # Check if the requested username_claim exists in the response from the provider
-        username = auth_model["name"]
-
-        # Check if we need to strip/prefix username
+        # Optionally strip idp domain or prefix the username
         if self.allowed_idps:
-            username_derivation_config = self.allowed_idps[selected_idp][
-                "username_derivation"
-            ]
-            action = username_derivation_config.get("action", None)
-            allowed_domains = self.allowed_idps[selected_idp].get(
-                "allowed_domains", None
-            )
+            username_derivation = self.allowed_idps[selected_idp]["username_derivation"]
+            action = username_derivation.get("action")
 
             if action == "strip_idp_domain":
-                gotten_name, gotten_domain = username.split('@')
-                username = gotten_name
+                username = username.split("@")[0]
             elif action == "prefix":
-                prefix = username_derivation_config["prefix"]
+                prefix = username_derivation["prefix"]
                 username = f"{prefix}:{username}"
 
-        auth_model["name"] = username
-        return auth_model
+        return username
 
     async def check_allowed(self, username, auth_model):
-        # Check if selected idp was marked as allowed
-        if self.allowed_idps:
-            selected_idp = auth_model["auth_state"][self.user_auth_state_key].get("idp")
-            # Fail hard if idp wasn't allowed
-            if selected_idp not in self.allowed_idps:
+        """
+        Returns True for users allowed to be authorized, raises errors for users
+        denied authorization.
+
+        Overrides the OAuthenticator.check_allowed implementation to allow users
+        either part of `allowed_users` or `allowed_idps`, 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
+
+        # FIXME: This needs to be thought over very carefully.
+        #
+        #        Is there or isn't there a commonly agreed "excepted behavior"
+        #        for when considering a combination of allowed_users,
+        #        allowed_idps, and allowed_idps allowed_domains?
+        #
+
+        # if allowed_users or allowed_idps is configured, we deny users not
+        # part of either
+        if self.allowed_users or self.allowed_idps:
+            user_info = auth_model["auth_state"][self.user_auth_state_key]
+            selected_idp = user_info["idp"]
+
+            if username in self.allowed_users:
+                return True
+
+            if self.allowed_idps and selected_idp in self.allowed_idps:
+                allowed_domains = self.allowed_idps[selected_idp].get("allowed_domains")
+                if allowed_domains:
+                    # FIXME: check allowed is called after username is stripped /
+                    #        prefixed, so we don't know the username's email domain
+                    #        anymore.
+                    #
+                    #        An idea to resolve this is to break apart
+                    #        user_info_to_username in a piece before and after the
+                    #        optional domain stripping operation, allowing us to
+                    #        call it again without the strip operation.
+                    #
+                    user_domain = username.split("@")[1]
+                    if user_domain in allowed_domains:
+                        return True
+                    else:
+                        raise web.HTTPError(
+                            500,
+                            "Trying to login using a domain that was not allowed",
+                        )
+            else:
                 self.log.error(
                     f"Trying to login from an identity provider that was not allowed {selected_idp}",
                 )
@@ -315,18 +344,9 @@ async def check_allowed(self, username, auth_model):
                     "Trying to login using an identity provider that was not allowed",
                 )
 
-            allowed_domains = self.allowed_idps[selected_idp].get(
-                "allowed_domains", None
-            )
-
-            if allowed_domains:
-                gotten_domain = username.split('@')[1]
-                if gotten_domain not in allowed_domains:
-                    raise web.HTTPError(
-                        500,
-                        "Trying to login using a domain that was not allowed",
-                    )
+            return False
 
+        # otherwise, authorize all users
         return True
 
 
diff --git a/oauthenticator/mediawiki.py b/oauthenticator/mediawiki.py
index c4d64e34..49caa90b 100644
--- a/oauthenticator/mediawiki.py
+++ b/oauthenticator/mediawiki.py
@@ -102,6 +102,7 @@ def normalize_username(self, username):
         """
         Override normalize_username to avoid lowercasing usernames
         """
+        username = username.replace(' ', '_')
         return username
 
     def _executor_default(self):
@@ -137,12 +138,7 @@ async def token_to_user(self, token_info):
             self.executor.submit(handshaker.identify, token_info["access_token"])
         )
 
-    async def update_auth_model(self, auth_model):
-        auth_model['name'] = auth_model['name'].replace(' ', '_')
-        return auth_model
-
     def build_auth_state_dict(self, token_info, user_info):
-        username = self.user_info_to_username(user_info)
         # this shouldn't be necessary anymore,
         # but keep for backward-compatibility
         return {

From 071c4116312681684b98de763a66f4bdfb2a8c13 Mon Sep 17 00:00:00 2001
From: Erik Sundell <erik.i.sundell@gmail.com>
Date: Tue, 25 Apr 2023 03:48:14 +0200
Subject: [PATCH 22/57] Update tests

---
 oauthenticator/tests/mocks.py          |  2 +-
 oauthenticator/tests/test_auth0.py     | 14 +++---
 oauthenticator/tests/test_azuread.py   |  8 ++--
 oauthenticator/tests/test_bitbucket.py | 27 +++++------
 oauthenticator/tests/test_cilogon.py   | 62 +++++++++++--------------
 oauthenticator/tests/test_generic.py   | 64 +++++++++++++-------------
 oauthenticator/tests/test_github.py    | 37 ++++++++-------
 oauthenticator/tests/test_gitlab.py    | 56 +++++++++++-----------
 oauthenticator/tests/test_globus.py    | 52 ++++++++++-----------
 oauthenticator/tests/test_google.py    | 32 ++++++-------
 oauthenticator/tests/test_mediawiki.py |  2 +-
 oauthenticator/tests/test_okpy.py      |  4 +-
 oauthenticator/tests/test_openshift.py | 16 +++----
 13 files changed, 178 insertions(+), 198 deletions(-)

diff --git a/oauthenticator/tests/mocks.py b/oauthenticator/tests/mocks.py
index f02870ed..b896c4db 100644
--- a/oauthenticator/tests/mocks.py
+++ b/oauthenticator/tests/mocks.py
@@ -253,5 +253,5 @@ async def no_code_test(authenticator):
     handler = Mock(spec=web.RequestHandler)
     handler.get_argument = Mock(return_value=None)
     with pytest.raises(web.HTTPError) as exc:
-        name = await authenticator.authenticate(handler)
+        auth_model = await authenticator.get_authenticated_user(handler, None)
     assert exc.value.status_code == 400
diff --git a/oauthenticator/tests/test_auth0.py b/oauthenticator/tests/test_auth0.py
index 53146318..7fb28ef4 100644
--- a/oauthenticator/tests/test_auth0.py
+++ b/oauthenticator/tests/test_auth0.py
@@ -42,11 +42,10 @@ async def test_auth0(config, auth0_client):
     authenticator = Auth0OAuthenticator(config=cfg)
 
     handler = auth0_client.handler_for_user(user_model('kaylee@serenity.now'))
-    user_info = await authenticator.authenticate(handler)
-    assert sorted(user_info) == ['auth_state', 'name']
-    name = user_info['name']
-    assert name == 'kaylee@serenity.now'
-    auth_state = user_info['auth_state']
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+    assert sorted(auth_model) == ['admin', 'auth_state', 'name']
+    assert auth_model['name'] == 'kaylee@serenity.now'
+    auth_state = auth_model['auth_state']
     assert 'access_token' in auth_state
     assert 'auth0_user' in auth_state
 
@@ -60,9 +59,8 @@ async def test_username_key(config, auth0_client):
     authenticator = Auth0OAuthenticator(config=cfg)
     authenticator.username_key = 'nickname'
     handler = auth0_client.handler_for_user(user_model('kaylee@serenity.now', 'kayle'))
-    user_info = await authenticator.authenticate(handler)
-
-    assert user_info['name'] == 'kayle'
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+    assert auth_model['name'] == 'kayle'
 
 
 async def test_custom_logout(monkeypatch):
diff --git a/oauthenticator/tests/test_azuread.py b/oauthenticator/tests/test_azuread.py
index d8c9ca77..0070540a 100644
--- a/oauthenticator/tests/test_azuread.py
+++ b/oauthenticator/tests/test_azuread.py
@@ -92,17 +92,17 @@ async def test_azuread(username_claim, azure_client):
         )
     )
 
-    user_info = await authenticator.authenticate(handler)
-    assert sorted(user_info) == ['auth_state', 'name']
+    auth_model = await authenticator.authenticate(handler)
+    assert sorted(auth_model) == ['admin', 'auth_state', 'name']
 
-    auth_state = user_info['auth_state']
+    auth_state = auth_model['auth_state']
     assert 'access_token' in auth_state
     assert 'user' in auth_state
 
     auth_state_user_info = auth_state['user']
     assert auth_state_user_info['aud'] == authenticator.client_id
 
-    username = user_info['name']
+    username = auth_model['name']
     if username_claim:
         assert username == auth_state_user_info[username_claim]
     else:
diff --git a/oauthenticator/tests/test_bitbucket.py b/oauthenticator/tests/test_bitbucket.py
index dfcc4c9e..1cf24b4f 100644
--- a/oauthenticator/tests/test_bitbucket.py
+++ b/oauthenticator/tests/test_bitbucket.py
@@ -28,11 +28,10 @@ def bitbucket_client(client):
 async def test_bitbucket(bitbucket_client):
     authenticator = BitbucketOAuthenticator()
     handler = bitbucket_client.handler_for_user(user_model('yorba'))
-    user_info = await authenticator.authenticate(handler)
-    assert sorted(user_info) == ['auth_state', 'name']
-    name = user_info['name']
-    assert name == 'yorba'
-    auth_state = user_info['auth_state']
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+    assert sorted(auth_model) == ['admin', 'auth_state', 'name']
+    assert auth_model['name'] == 'yorba'
+    auth_state = auth_model['auth_state']
     assert 'access_token' in auth_state
     assert 'bitbucket_user' in auth_state
 
@@ -59,25 +58,23 @@ def list_teams(request):
     client.hosts['api.bitbucket.org'].append(('/2.0/workspaces', list_teams))
 
     handler = client.handler_for_user(user_model('caboose'))
-    user_info = await authenticator.authenticate(handler)
-    name = user_info['name']
-    assert name == 'caboose'
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+    assert auth_model['name'] == 'caboose'
 
     handler = client.handler_for_user(user_model('donut'))
-    name = await authenticator.authenticate(handler)
-    assert name is None
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+    assert auth_model is None
 
     # reverse it, just to be safe
     authenticator.allowed_teams = ['red']
 
     handler = client.handler_for_user(user_model('caboose'))
-    name = await authenticator.authenticate(handler)
-    assert name is None
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+    assert auth_model is None
 
     handler = client.handler_for_user(user_model('donut'))
-    user_info = await authenticator.authenticate(handler)
-    name = user_info['name']
-    assert name == 'donut'
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+    assert auth_model['name'] == 'donut'
 
 
 async def test_deprecated_config(caplog):
diff --git a/oauthenticator/tests/test_cilogon.py b/oauthenticator/tests/test_cilogon.py
index eca7a442..5e0c44a6 100644
--- a/oauthenticator/tests/test_cilogon.py
+++ b/oauthenticator/tests/test_cilogon.py
@@ -38,11 +38,10 @@ def cilogon_client(client):
 async def test_cilogon(cilogon_client):
     authenticator = CILogonOAuthenticator()
     handler = cilogon_client.handler_for_user(user_model('wash'))
-    user_info = await authenticator.authenticate(handler)
-    print(json.dumps(user_info, sort_keys=True, indent=4))
-    name = user_info['name']
-    assert name == 'wash@serenity.space'
-    auth_state = user_info['auth_state']
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+    print(json.dumps(auth_model, sort_keys=True, indent=4))
+    assert auth_model['name'] == 'wash@serenity.space'
+    auth_state = auth_model['auth_state']
     assert 'access_token' in auth_state
     assert 'token_response' in auth_state
     assert auth_state["cilogon_user"] == user_model('wash')
@@ -53,11 +52,10 @@ async def test_cilogon_alternate_claim(cilogon_client):
     handler = cilogon_client.handler_for_user(
         alternative_user_model('jtkirk@ufp.gov', 'uid')
     )
-    user_info = await authenticator.authenticate(handler)
-    print(json.dumps(user_info, sort_keys=True, indent=4))
-    name = user_info['name']
-    assert name == 'jtkirk@ufp.gov'
-    auth_state = user_info['auth_state']
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+    print(json.dumps(auth_model, sort_keys=True, indent=4))
+    assert auth_model['name'] == 'jtkirk@ufp.gov'
+    auth_state = auth_model['auth_state']
     assert 'access_token' in auth_state
     assert 'token_response' in auth_state
     assert auth_state["cilogon_user"] == alternative_user_model('jtkirk@ufp.gov', 'uid')
@@ -68,11 +66,10 @@ async def test_cilogon_additional_claim(cilogon_client):
     handler = cilogon_client.handler_for_user(
         alternative_user_model('jtkirk@ufp.gov', 'uid')
     )
-    user_info = await authenticator.authenticate(handler)
-    print(json.dumps(user_info, sort_keys=True, indent=4))
-    name = user_info['name']
-    assert name == 'jtkirk@ufp.gov'
-    auth_state = user_info['auth_state']
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+    print(json.dumps(auth_model, sort_keys=True, indent=4))
+    assert auth_model['name'] == 'jtkirk@ufp.gov'
+    auth_state = auth_model['auth_state']
     assert 'access_token' in auth_state
     assert 'token_response' in auth_state
     assert auth_state["cilogon_user"] == alternative_user_model('jtkirk@ufp.gov', 'uid')
@@ -84,7 +81,7 @@ async def test_cilogon_missing_alternate_claim(cilogon_client):
         alternative_user_model('jtkirk@ufp.gov', 'uid')
     )
     with raises(HTTPError):
-        user_info = await authenticator.authenticate(handler)
+        auth_model = await authenticator.get_authenticated_user(handler, None)
 
 
 async def test_deprecated_config(caplog):
@@ -273,10 +270,9 @@ async def test_strip_and_prefix_username(cilogon_client):
             'jtkirk@uni.edu', 'email', idp='https://some-idp.com/login/oauth/authorize'
         )
     )
-    user_info = await authenticator.authenticate(handler)
-    print(json.dumps(user_info, sort_keys=True, indent=4))
-    name = user_info['name']
-    assert name == 'jtkirk'
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+    print(json.dumps(auth_model, sort_keys=True, indent=4))
+    assert auth_model['name'] == 'jtkirk'
 
     # Test appending prefixes
     handler = cilogon_client.handler_for_user(
@@ -284,10 +280,9 @@ async def test_strip_and_prefix_username(cilogon_client):
             'jtkirk', 'nickname', idp='https://another-idp.com/login/oauth/authorize'
         )
     )
-    user_info = await authenticator.authenticate(handler)
-    print(json.dumps(user_info, sort_keys=True, indent=4))
-    name = user_info['name']
-    assert name == 'idp:jtkirk'
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+    print(json.dumps(auth_model, sort_keys=True, indent=4))
+    assert auth_model['name'] == 'idp:jtkirk'
 
 
 async def test_no_action_specified(cilogon_client):
@@ -308,9 +303,8 @@ async def test_no_action_specified(cilogon_client):
             'jtkirk@uni.edu', 'email', idp='https://some-idp.com/login/oauth/authorize'
         )
     )
-    user_info = await authenticator.authenticate(handler)
-    name = user_info['name']
-    assert name == 'jtkirk@uni.edu'
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+    assert auth_model['name'] == 'jtkirk@uni.edu'
 
 
 async def test_not_allowed_domains_and_stripping(cilogon_client):
@@ -337,7 +331,7 @@ async def test_not_allowed_domains_and_stripping(cilogon_client):
 
     # The domain to be stripped isn't allowed, so it should fail
     with raises(HTTPError):
-        user_info = await authenticator.authenticate(handler)
+        auth_model = await authenticator.get_authenticated_user(handler, None)
 
 
 async def test_allowed_domains_and_stripping(cilogon_client):
@@ -363,9 +357,8 @@ async def test_allowed_domains_and_stripping(cilogon_client):
     )
 
     # The domain to be stripped is allowed, so it should be stripped
-    user_info = await authenticator.authenticate(handler)
-    name = user_info['name']
-    assert name == 'jtkirk'
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+    assert auth_model['name'] == 'jtkirk'
 
 
 async def test_allowed_domains_no_stripping(cilogon_client):
@@ -389,7 +382,7 @@ async def test_allowed_domains_no_stripping(cilogon_client):
     )
 
     with raises(HTTPError):
-        user_info = await authenticator.authenticate(handler)
+        auth_model = await authenticator.get_authenticated_user(handler, None)
 
     # Test allowed domain login
     handler = cilogon_client.handler_for_user(
@@ -398,6 +391,5 @@ async def test_allowed_domains_no_stripping(cilogon_client):
         )
     )
 
-    user_info = await authenticator.authenticate(handler)
-    name = user_info['name']
-    assert name == 'jtkirk@pink.org'
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+    assert auth_model['name'] == 'jtkirk@pink.org'
diff --git a/oauthenticator/tests/test_generic.py b/oauthenticator/tests/test_generic.py
index 18855bc2..575a6e22 100644
--- a/oauthenticator/tests/test_generic.py
+++ b/oauthenticator/tests/test_generic.py
@@ -48,11 +48,10 @@ async def test_generic(get_authenticator, generic_client):
     authenticator = get_authenticator()
 
     handler = get_simple_handler(generic_client)
-    user_info = await authenticator.authenticate(handler)
-    assert sorted(user_info) == ['auth_state', 'name']
-    name = user_info['name']
-    assert name == 'wash'
-    auth_state = user_info['auth_state']
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+    assert sorted(auth_model) == ['admin', 'auth_state', 'name']
+    assert auth_model['name'] == 'wash'
+    auth_state = auth_model['auth_state']
     assert 'access_token' in auth_state
     assert 'oauth_user' in auth_state
     assert 'refresh_token' in auth_state
@@ -64,10 +63,9 @@ async def test_generic_data(get_authenticator, generic_client):
 
     handler = get_simple_handler(generic_client)
     data = {'testing': 'data'}
-    user_info = await authenticator.authenticate(handler, data)
-    assert sorted(user_info) == ['auth_state', 'name']
-    name = user_info['name']
-    assert name == 'wash'
+    auth_model = await authenticator.authenticate(handler, data)
+    assert sorted(auth_model) == ['admin', 'auth_state', 'name']
+    assert auth_model['name'] == 'wash'
 
 
 async def test_generic_callable_username_key(get_authenticator, generic_client):
@@ -75,8 +73,8 @@ async def test_generic_callable_username_key(get_authenticator, generic_client):
     handler = generic_client.handler_for_user(
         user_model('wash', alternate_username='zoe')
     )
-    user_info = await authenticator.authenticate(handler)
-    assert user_info['name'] == 'zoe'
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+    assert auth_model['name'] == 'zoe'
 
 
 async def test_generic_callable_groups_claim_key_with_allowed_groups(
@@ -90,8 +88,8 @@ async def test_generic_callable_groups_claim_key_with_allowed_groups(
     handler = generic_client.handler_for_user(
         user_model('wash', alternate_username='zoe', policies={'roles': ['super_user']})
     )
-    user_info = await authenticator.authenticate(handler)
-    assert user_info['name'] == 'wash'
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+    assert auth_model['name'] == 'wash'
 
 
 async def test_generic_groups_claim_key_with_allowed_groups(
@@ -105,8 +103,8 @@ async def test_generic_groups_claim_key_with_allowed_groups(
     handler = generic_client.handler_for_user(
         user_model('wash', alternate_username='zoe', groups=['super_user'])
     )
-    user_info = await authenticator.authenticate(handler)
-    assert user_info['name'] == 'wash'
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+    assert auth_model['name'] == 'wash'
 
 
 async def test_generic_groups_claim_key_nested_strings(
@@ -122,8 +120,8 @@ async def test_generic_groups_claim_key_nested_strings(
             'wash', alternate_username='zoe', permissions={"groups": ['super_user']}
         )
     )
-    user_info = await authenticator.authenticate(handler)
-    assert user_info['name'] == 'wash'
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+    assert auth_model['name'] == 'wash'
 
 
 async def test_generic_groups_claim_key_nested_strings_nonexistant_key(
@@ -137,8 +135,8 @@ async def test_generic_groups_claim_key_nested_strings_nonexistant_key(
     handler = generic_client.handler_for_user(
         user_model('wash', alternate_username='zoe')
     )
-    user_info = await authenticator.authenticate(handler)
-    assert user_info is None
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+    assert auth_model is None
 
 
 async def test_generic_groups_claim_key_with_allowed_groups_unauthorized(
@@ -152,8 +150,8 @@ async def test_generic_groups_claim_key_with_allowed_groups_unauthorized(
     handler = generic_client.handler_for_user(
         user_model('wash', alternate_username='zoe', groups=['public'])
     )
-    user_info = await authenticator.authenticate(handler)
-    assert user_info is None
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+    assert auth_model is None
 
 
 async def test_generic_groups_claim_key_with_allowed_groups_and_admin_groups(
@@ -168,9 +166,9 @@ async def test_generic_groups_claim_key_with_allowed_groups_and_admin_groups(
     handler = generic_client.handler_for_user(
         user_model('wash', alternate_username='zoe', groups=['user', 'administrator'])
     )
-    user_info = await authenticator.authenticate(handler)
-    assert user_info['name'] == 'wash'
-    assert user_info['admin'] is True
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+    assert auth_model['name'] == 'wash'
+    assert auth_model['admin'] is True
 
 
 async def test_generic_groups_claim_key_with_allowed_groups_and_admin_groups_not_admin(
@@ -185,9 +183,9 @@ async def test_generic_groups_claim_key_with_allowed_groups_and_admin_groups_not
     handler = generic_client.handler_for_user(
         user_model('wash', alternate_username='zoe', groups=['user'])
     )
-    user_info = await authenticator.authenticate(handler)
-    assert user_info['name'] == 'wash'
-    assert user_info['admin'] is False
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+    assert auth_model['name'] == 'wash'
+    assert auth_model['admin'] is False
 
 
 async def test_generic_groups_claim_key_with_allowed_groups_and_no_admin_groups_but_admin_users(
@@ -206,9 +204,9 @@ async def test_generic_groups_claim_key_with_allowed_groups_and_no_admin_groups_
 
     # Assert that the authenticated user is actually an admin due to being listed in `admin_users`
     # Even though admin_groups is empty
-    user_info = await authenticator.get_authenticated_user(handler, data=None)
-    assert user_info['name'] == 'wash'
-    assert user_info['admin'] is True
+    auth_model = await authenticator.get_authenticated_user(handler, data=None)
+    assert auth_model['name'] == 'wash'
+    assert auth_model['admin'] is True
 
 
 async def test_generic_callable_groups_claim_key_with_allowed_groups_and_admin_groups(
@@ -228,6 +226,6 @@ async def test_generic_callable_groups_claim_key_with_allowed_groups_and_admin_g
             policies={'roles': ['user', 'administrator']},
         )
     )
-    user_info = await authenticator.authenticate(handler)
-    assert user_info['name'] == 'zoe'
-    assert user_info['admin'] is True
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+    assert auth_model['name'] == 'zoe'
+    assert auth_model['admin'] is True
diff --git a/oauthenticator/tests/test_github.py b/oauthenticator/tests/test_github.py
index 3468b00a..03a591d1 100644
--- a/oauthenticator/tests/test_github.py
+++ b/oauthenticator/tests/test_github.py
@@ -39,16 +39,15 @@ def github_client(client):
 async def test_github(github_client):
     authenticator = GitHubOAuthenticator()
     handler = github_client.handler_for_user(user_model('wash'))
-    user_info = await authenticator.authenticate(handler)
-    name = user_info['name']
-    assert name == 'wash'
-    auth_state = user_info['auth_state']
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+    assert auth_model['name'] == 'wash'
+    auth_state = auth_model['auth_state']
     assert 'access_token' in auth_state
     assert 'github_user' in auth_state
     assert auth_state["github_user"] == {
         'email': 'dinosaurs@space',
         'id': 5,
-        'login': name,
+        'login': auth_model['name'],
         'name': 'Hoban Washburn',
     }
 
@@ -157,38 +156,38 @@ def team_membership(request):
         authenticator.allowed_organizations = ['blue']
 
         handler = client.handler_for_user(user_model('caboose'))
-        user = await authenticator.authenticate(handler)
-        assert user['name'] == 'caboose'
+        auth_model = await authenticator.get_authenticated_user(handler, None)
+        assert auth_model['name'] == 'caboose'
 
         handler = client.handler_for_user(user_model('donut'))
-        user = await authenticator.authenticate(handler)
-        assert user is None
+        auth_model = await authenticator.get_authenticated_user(handler, None)
+        assert auth_model is None
 
         # reverse it, just to be safe
         authenticator.allowed_organizations = ['red']
 
         handler = client.handler_for_user(user_model('caboose'))
-        user = await authenticator.authenticate(handler)
-        assert user is None
+        auth_model = await authenticator.get_authenticated_user(handler, None)
+        assert auth_model is None
 
         handler = client.handler_for_user(user_model('donut'))
-        user = await authenticator.authenticate(handler)
-        assert user['name'] == 'donut'
+        auth_model = await authenticator.get_authenticated_user(handler, None)
+        assert auth_model['name'] == 'donut'
 
         # test team membership
         authenticator.allowed_organizations = ['blue:alpha', 'red']
 
         handler = client.handler_for_user(user_model('tucker'))
-        user = await authenticator.authenticate(handler)
-        assert user['name'] == 'tucker'
+        auth_model = await authenticator.get_authenticated_user(handler, None)
+        assert auth_model['name'] == 'tucker'
 
         handler = client.handler_for_user(user_model('grif'))
-        user = await authenticator.authenticate(handler)
-        assert user['name'] == 'grif'
+        auth_model = await authenticator.get_authenticated_user(handler, None)
+        assert auth_model['name'] == 'grif'
 
         handler = client.handler_for_user(user_model('texas'))
-        user = await authenticator.authenticate(handler)
-        assert user is None
+        auth_model = await authenticator.get_authenticated_user(handler, None)
+        assert auth_model is None
 
         client_hosts.pop()
         client_hosts.pop()
diff --git a/oauthenticator/tests/test_gitlab.py b/oauthenticator/tests/test_gitlab.py
index aa4845e3..044d5764 100644
--- a/oauthenticator/tests/test_gitlab.py
+++ b/oauthenticator/tests/test_gitlab.py
@@ -59,11 +59,10 @@ async def test_gitlab(gitlab_client):
     authenticator = GitLabOAuthenticator()
     mock_api_version(gitlab_client, '12.3.1-ee')
     handler = gitlab_client.handler_for_user(user_model('wash'))
-    user_info = await authenticator.authenticate(handler)
-    assert sorted(user_info) == ['auth_state', 'name']
-    name = user_info['name']
-    assert name == 'wash'
-    auth_state = user_info['auth_state']
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+    assert sorted(auth_model) == ['admin', 'auth_state', 'name']
+    assert auth_model['name'] == 'wash'
+    auth_state = auth_model['auth_state']
     assert 'access_token' in auth_state
     assert 'gitlab_user' in auth_state
 
@@ -149,34 +148,31 @@ def groups_paginated(user, page, urlinfo, response):
         authenticator.allowed_gitlab_groups = ['blue']
 
         handler = client.handler_for_user(group_user_model('caboose'))
-        user_info = await authenticator.authenticate(handler)
-        name = user_info['name']
-        assert name == 'caboose'
+        auth_model = await authenticator.get_authenticated_user(handler, None)
+        assert auth_model['name'] == 'caboose'
 
         handler = client.handler_for_user(group_user_model('burns', is_admin=True))
-        user_info = await authenticator.authenticate(handler)
-        name = user_info['name']
-        assert name == 'burns'
+        auth_model = await authenticator.get_authenticated_user(handler, None)
+        assert auth_model['name'] == 'burns'
 
         handler = client.handler_for_user(group_user_model('grif'))
-        name = await authenticator.authenticate(handler)
-        assert name is None
+        auth_model = await authenticator.get_authenticated_user(handler, None)
+        assert auth_model is None
 
         handler = client.handler_for_user(group_user_model('simmons', is_admin=True))
-        name = await authenticator.authenticate(handler)
-        assert name is None
+        auth_model = await authenticator.get_authenticated_user(handler, None)
+        assert auth_model is None
 
         # reverse it, just to be safe
         authenticator.allowed_gitlab_groups = ['red']
 
         handler = client.handler_for_user(group_user_model('caboose'))
-        name = await authenticator.authenticate(handler)
-        assert name is None
+        auth_model = await authenticator.get_authenticated_user(handler, None)
+        assert auth_model is None
 
         handler = client.handler_for_user(group_user_model('grif'))
-        user_info = await authenticator.authenticate(handler)
-        name = user_info['name']
-        assert name == 'grif'
+        auth_model = await authenticator.get_authenticated_user(handler, None)
+        assert auth_model['name'] == 'grif'
 
         client.hosts['gitlab.com'].pop()
 
@@ -237,33 +233,33 @@ def is_member(request):
 
     # Forbidden since John has guest access
     handler = client.handler_for_user(john_user_model)
-    user_info = await authenticator.authenticate(handler)
-    assert user_info is None
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+    assert auth_model is None
 
     # Authenticated since Harry has developer access to the project
     handler = client.handler_for_user(harry_user_model)
-    user_info = await authenticator.authenticate(handler)
-    name = user_info['name']
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+    name = auth_model['name']
     assert name == 'harry'
 
     # Forbidden since Sheila doesn't have access to the project
     handler = client.handler_for_user(sheila_user_model)
-    user_info = await authenticator.authenticate(handler)
-    assert user_info is None
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+    assert auth_model is None
 
     authenticator.allowed_project_ids = [123123152543]
 
     # Forbidden since the project does not exist.
     handler = client.handler_for_user(harry_user_model)
-    user_info = await authenticator.authenticate(handler)
-    assert user_info is None
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+    assert auth_model is None
 
     authenticator.allowed_project_ids = [123123152543, 1231231]
 
     # Authenticated since Harry has developer access to one of the project in the list
     handler = client.handler_for_user(harry_user_model)
-    user_info = await authenticator.authenticate(handler)
-    name = user_info['name']
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+    name = auth_model['name']
     assert name == 'harry'
 
 
diff --git a/oauthenticator/tests/test_globus.py b/oauthenticator/tests/test_globus.py
index 5a2bd397..a06ebf4b 100644
--- a/oauthenticator/tests/test_globus.py
+++ b/oauthenticator/tests/test_globus.py
@@ -187,9 +187,9 @@ async def save_auth_state(self, state):
 async def test_globus(globus_client):
     authenticator = GlobusOAuthenticator()
     handler = globus_client.handler_for_user(user_model('wash@uflightacademy.edu'))
-    data = await authenticator.authenticate(handler)
-    assert data['name'] == 'wash'
-    tokens = list(data['auth_state']['tokens'].keys())
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+    assert auth_model['name'] == 'wash'
+    tokens = list(auth_model['auth_state']['tokens'].keys())
     assert tokens == ['transfer.api.globus.org']
 
 
@@ -229,7 +229,7 @@ async def test_restricted_domain(globus_client):
     authenticator.identity_provider = 'alliance.gov'
     handler = globus_client.handler_for_user(user_model('wash@uflightacademy.edu'))
     with raises(web.HTTPError) as exc:
-        await authenticator.authenticate(handler)
+        authenticator.get_authenticated_user(handler, None)
     assert exc.value.status_code == 403
 
 
@@ -239,8 +239,8 @@ async def test_namespaced_domain(globus_client):
     authenticator.identity_provider = ''
     um = user_model('wash@legitshipping.com@serenity.com')
     handler = globus_client.handler_for_user(um)
-    data = await authenticator.authenticate(handler)
-    assert data['name'] == 'wash'
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+    assert auth_model['name'] == 'wash'
 
 
 async def test_username_from_email(globus_client):
@@ -250,8 +250,8 @@ async def test_username_from_email(globus_client):
     authenticator.username_from_email = True
     um = user_model('wash@legitshipping.com@serenity.com', 'alan@tudyk.org')
     handler = globus_client.handler_for_user(um)
-    data = await authenticator.authenticate(handler)
-    assert data['name'] == 'alan'
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+    assert auth_model['name'] == 'alan'
 
 
 async def test_username_not_from_email(globus_client):
@@ -260,8 +260,8 @@ async def test_username_not_from_email(globus_client):
     authenticator.identity_provider = ''
     um = user_model('wash@legitshipping.com@serenity.com', 'alan@tudyk.org')
     handler = globus_client.handler_for_user(um)
-    data = await authenticator.authenticate(handler)
-    assert data['name'] == 'wash'
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+    assert auth_model['name'] == 'wash'
 
 
 async def test_email_scope_added(globus_client):
@@ -282,8 +282,8 @@ async def test_username_from_email_restricted_pass(globus_client):
     authenticator.username_from_email = True
     um = user_model('wash@serenity.com', 'alan@serenity.com')
     handler = globus_client.handler_for_user(um)
-    data = await authenticator.authenticate(handler)
-    assert data['name'] == 'alan'
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+    assert auth_model['name'] == 'alan'
 
 
 async def test_username_from_email_restricted_fail(globus_client):
@@ -294,7 +294,7 @@ async def test_username_from_email_restricted_fail(globus_client):
     um = user_model('wash@serenity.com', 'alan@tudyk.org')
     handler = globus_client.handler_for_user(um)
     with raises(web.HTTPError) as exc:
-        await authenticator.authenticate(handler)
+        authenticator.get_authenticated_user(handler, None)
     assert exc.value.status_code == 403
 
 
@@ -306,9 +306,9 @@ async def test_token_exclusion(globus_client):
         'groups.api.globus.org',
     ]
     handler = globus_client.handler_for_user(user_model('wash@uflightacademy.edu'))
-    data = await authenticator.authenticate(handler)
-    assert data['name'] == 'wash'
-    assert list(data['auth_state']['tokens'].keys()) == []
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+    assert auth_model['name'] == 'wash'
+    assert list(auth_model['auth_state']['tokens'].keys()) == []
 
 
 async def test_revoke_tokens(globus_client, mock_globus_user):
@@ -408,25 +408,25 @@ async def test_user_in_allowed_group(globus_client):
     authenticator = GlobusOAuthenticator()
     authenticator.allowed_globus_groups = set({'21c6bc5d-fc12-4f60-b999-76766cd596c2'})
     handler = globus_client.handler_for_user(user_model('wash@uflightacademy.edu'))
-    data = await authenticator.authenticate(handler)
-    assert data['name'] == 'wash'
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+    assert auth_model['name'] == 'wash'
 
 
 async def test_user_not_allowed(globus_client):
     authenticator = GlobusOAuthenticator()
     authenticator.allowed_globus_groups = set({'3f1f85c4-f084-4173-9efb-7c7e0b44291a'})
     handler = globus_client.handler_for_user(user_model('wash@uflightacademy.edu'))
-    data = await authenticator.authenticate(handler)
-    assert data == None
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+    assert auth_model == None
 
 
 async def test_user_is_admin(globus_client):
     authenticator = GlobusOAuthenticator()
     authenticator.admin_globus_groups = set({'21c6bc5d-fc12-4f60-b999-76766cd596c2'})
     handler = globus_client.handler_for_user(user_model('wash@uflightacademy.edu'))
-    data = await authenticator.authenticate(handler)
-    assert data['name'] == 'wash'
-    assert data['admin'] == True
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+    assert auth_model['name'] == 'wash'
+    assert auth_model['admin'] == True
 
 
 async def test_user_allowed_not_admin(globus_client):
@@ -434,6 +434,6 @@ async def test_user_allowed_not_admin(globus_client):
     authenticator.allowed_globus_groups = set({'21c6bc5d-fc12-4f60-b999-76766cd596c2'})
     authenticator.admin_globus_groups = set({'3f1f85c4-f084-4173-9efb-7c7e0b44291a'})
     handler = globus_client.handler_for_user(user_model('wash@uflightacademy.edu'))
-    data = await authenticator.authenticate(handler)
-    assert data['name'] == 'wash'
-    assert data['admin'] == False
+    auth_model = await authenticator.get_authenticated_user(handler, None)
+    assert auth_model['name'] == 'wash'
+    assert auth_model['admin'] == False
diff --git a/oauthenticator/tests/test_google.py b/oauthenticator/tests/test_google.py
index 93e5dd79..9a6d7711 100644
--- a/oauthenticator/tests/test_google.py
+++ b/oauthenticator/tests/test_google.py
@@ -35,8 +35,8 @@ 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.authenticate(handler)
-    assert sorted(user_info) == ['auth_state', 'name']
+    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']
@@ -49,8 +49,8 @@ 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.authenticate(handler)
-    assert sorted(user_info) == ['auth_state', 'name']
+    user_info = await authenticator.get_authenticated_user(handler, None)
+    assert sorted(user_info) == ['admin', 'auth_state', 'name']
     name = user_info['name']
     assert name == '724f95667e2fbe903ee1b4cffcae3b25'
 
@@ -58,31 +58,31 @@ async def test_google_username_claim(google_client):
 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.authenticate(handler)
+    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.authenticate(handler)
+        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.authenticate(handler)
+    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.authenticate(handler)
+    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.authenticate(handler)
+        name = await authenticator.get_authenticated_user(handler, None)
     assert exc.value.status_code == 403
 
 
@@ -98,7 +98,7 @@ async def test_admin_google_groups(google_client):
         '_google_groups_for_user',
         return_value=['anotherone', 'fakeadmingroup'],
     ):
-        admin_user_info = await authenticator.authenticate(handler)
+        admin_user_info = await authenticator.get_authenticated_user(handler, None)
         # Make sure the user authenticated successfully
         assert admin_user_info
         # Assert that the user is an admin
@@ -124,7 +124,7 @@ async def test_admin_google_groups(google_client):
         '_google_groups_for_user',
         return_value=['anotherone', 'fakenonallowedgroup'],
     ):
-        allowed_user_groups = await authenticator.authenticate(handler)
+        allowed_user_groups = await authenticator.get_authenticated_user(handler, None)
         assert allowed_user_groups is None
 
 
@@ -158,7 +158,7 @@ async def test_allowed_google_groups(google_client):
         '_google_groups_for_user',
         return_value=['anotherone', 'fakeadmingroup'],
     ):
-        admin_user_info = await authenticator.authenticate(handler)
+        admin_user_info = await authenticator.get_authenticated_user(handler, None)
         assert admin_user_info is None
     handler = google_client.handler_for_user(user_model('fakealloweduser@email.com'))
     with mock.patch.object(
@@ -166,7 +166,7 @@ async def test_allowed_google_groups(google_client):
         '_google_groups_for_user',
         return_value=['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'
         ]
@@ -179,13 +179,13 @@ async def test_allowed_google_groups(google_client):
         '_google_groups_for_user',
         return_value=['anotherone', 'fakenonallowedgroup'],
     ):
-        allowed_user_groups = await authenticator.authenticate(handler)
+        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']
     ):
-        allowed_user_groups = await authenticator.authenticate(handler)
+        allowed_user_groups = await authenticator.get_authenticated_user(handler, None)
         assert allowed_user_groups is None
 
 
@@ -200,7 +200,7 @@ async def test_admin_only_google_groups(google_client):
         '_google_groups_for_user',
         return_value=['anotherone', 'fakeadmingroup'],
     ):
-        admin_user_info = await authenticator.authenticate(handler)
+        admin_user_info = await authenticator.get_authenticated_user(handler, None)
         admin_user = admin_user_info['admin']
         assert admin_user is True
 
diff --git a/oauthenticator/tests/test_mediawiki.py b/oauthenticator/tests/test_mediawiki.py
index 75931ed4..2fe751a4 100644
--- a/oauthenticator/tests/test_mediawiki.py
+++ b/oauthenticator/tests/test_mediawiki.py
@@ -60,7 +60,7 @@ async def test_mediawiki(mediawiki):
         request=Mock(query='oauth_token=key&oauth_verifier=me'),
         find_user=Mock(return_value=None),
     )
-    user = await authenticator.authenticate(handler)
+    user = await authenticator.get_authenticated_user(handler, None)
     assert user['name'] == 'wash'
     auth_state = user['auth_state']
     assert auth_state['ACCESS_TOKEN_KEY'] == 'key'
diff --git a/oauthenticator/tests/test_okpy.py b/oauthenticator/tests/test_okpy.py
index 70235b9d..2452af08 100644
--- a/oauthenticator/tests/test_okpy.py
+++ b/oauthenticator/tests/test_okpy.py
@@ -26,8 +26,8 @@ def okpy_client(client):
 async def test_okpy(okpy_client):
     authenticator = OkpyOAuthenticator()
     handler = okpy_client.handler_for_user(user_model('testing@example.com'))
-    user_info = await authenticator.authenticate(handler)
-    assert sorted(user_info) == ['auth_state', 'name']
+    user_info = await authenticator.get_authenticated_user(handler, None)
+    assert sorted(user_info) == ['admin', 'auth_state', 'name']
     name = user_info['name']
     assert name == 'testing@example.com'
     auth_state = user_info['auth_state']
diff --git a/oauthenticator/tests/test_openshift.py b/oauthenticator/tests/test_openshift.py
index c1951bc1..7ccc6587 100644
--- a/oauthenticator/tests/test_openshift.py
+++ b/oauthenticator/tests/test_openshift.py
@@ -27,8 +27,8 @@ async def test_openshift(openshift_client):
     authenticator = OpenShiftOAuthenticator()
     authenticator.openshift_auth_api_url = "https://openshift.default.svc.cluster.local"
     handler = openshift_client.handler_for_user(user_model('wash'))
-    user_info = await authenticator.authenticate(handler)
-    assert sorted(user_info) == ['auth_state', 'name']
+    user_info = await authenticator.get_authenticated_user(handler, None)
+    assert sorted(user_info) == ['admin', 'auth_state', 'name']
     name = user_info['name']
     assert name == 'wash'
     auth_state = user_info['auth_state']
@@ -41,8 +41,8 @@ async def test_openshift_allowed_groups(openshift_client):
     authenticator.allowed_groups = {'group1'}
     authenticator.openshift_auth_api_url = "https://openshift.default.svc.cluster.local"
     handler = openshift_client.handler_for_user(user_model('wash'))
-    user_info = await authenticator.authenticate(handler)
-    assert sorted(user_info) == ['auth_state', 'name']
+    user_info = await authenticator.get_authenticated_user(handler, None)
+    assert sorted(user_info) == ['admin', 'auth_state', 'name']
     name = user_info['name']
     assert name == 'wash'
     auth_state = user_info['auth_state']
@@ -57,7 +57,7 @@ async def test_openshift_not_in_allowed_groups(openshift_client):
     authenticator.allowed_groups = {'group3'}
     authenticator.openshift_auth_api_url = "https://openshift.default.svc.cluster.local"
     handler = openshift_client.handler_for_user(user_model('wash'))
-    user_info = await authenticator.authenticate(handler)
+    user_info = await authenticator.get_authenticated_user(handler, None)
     assert user_info == None
 
 
@@ -67,7 +67,7 @@ async def test_openshift_not_in_allowed_groups_but_is_admin(openshift_client):
     authenticator.admin_groups = {'group1'}
     authenticator.openshift_auth_api_url = "https://openshift.default.svc.cluster.local"
     handler = openshift_client.handler_for_user(user_model('wash'))
-    user_info = await authenticator.authenticate(handler)
+    user_info = await authenticator.get_authenticated_user(handler, None)
     assert sorted(user_info) == ['admin', 'auth_state', 'name']
     assert user_info['admin'] == True
 
@@ -78,7 +78,7 @@ async def test_openshift_in_allowed_groups_and_is_admin(openshift_client):
     authenticator.admin_groups = {'group1'}
     authenticator.openshift_auth_api_url = "https://openshift.default.svc.cluster.local"
     handler = openshift_client.handler_for_user(user_model('wash'))
-    user_info = await authenticator.authenticate(handler)
+    user_info = await authenticator.get_authenticated_user(handler, None)
     assert sorted(user_info) == ['admin', 'auth_state', 'name']
     assert user_info['admin'] == True
 
@@ -89,7 +89,7 @@ async def test_openshift_in_allowed_groups_and_is_not_admin(openshift_client):
     authenticator.admin_groups = {'group3'}
     authenticator.openshift_auth_api_url = "https://openshift.default.svc.cluster.local"
     handler = openshift_client.handler_for_user(user_model('wash'))
-    user_info = await authenticator.authenticate(handler)
+    user_info = await authenticator.get_authenticated_user(handler, None)
     assert sorted(user_info) == ['admin', 'auth_state', 'name']
     assert user_info['admin'] == False
 

From bfaaa1163a41df577c90489862a97af33b0032d8 Mon Sep 17 00:00:00 2001
From: Erik Sundell <erik.i.sundell@gmail.com>
Date: Tue, 25 Apr 2023 03:49:15 +0200
Subject: [PATCH 23/57] Fixes from running tests

---
 oauthenticator/bitbucket.py | 2 +-
 oauthenticator/cilogon.py   | 3 +--
 oauthenticator/generic.py   | 2 +-
 oauthenticator/oauth2.py    | 2 +-
 4 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/oauthenticator/bitbucket.py b/oauthenticator/bitbucket.py
index e6dd39fb..3c2b0d9f 100644
--- a/oauthenticator/bitbucket.py
+++ b/oauthenticator/bitbucket.py
@@ -69,7 +69,7 @@ async def update_auth_model(self, auth_model):
             auth_model["admin"] = True
 
         if self.allowed_teams:
-            user_teams = self._fetch_user_teams(access_token, token_type)
+            user_teams = await self._fetch_user_teams(access_token, token_type)
             auth_model["auth_state"]["user_teams"] = user_teams
 
         return auth_model
diff --git a/oauthenticator/cilogon.py b/oauthenticator/cilogon.py
index 4a04ae88..8d5ab09f 100644
--- a/oauthenticator/cilogon.py
+++ b/oauthenticator/cilogon.py
@@ -249,12 +249,11 @@ def _validate_allowed_idps(self, proposal):
     )
 
     def user_info_to_username(self, user_info):
-        selected_idp = user_info["idp"]
-
         username_claims = [self.username_claim]
         if self.additional_username_claims:
             username_claims.extend(self.additional_username_claims)
         if self.allowed_idps:
+            selected_idp = user_info["idp"]
             # The username_claim which should be used for this idp
             username_claims = [
                 self.allowed_idps[selected_idp]["username_derivation"]["username_claim"]
diff --git a/oauthenticator/generic.py b/oauthenticator/generic.py
index cc3216af..a143705d 100644
--- a/oauthenticator/generic.py
+++ b/oauthenticator/generic.py
@@ -109,7 +109,7 @@ def get_user_groups(self, user_info):
         if callable(self.claim_groups_key):
             return set(self.claim_groups_key(user_info))
         try:
-            return reduce(dict.get, self.claim_groups_key.split("."), user_info)
+            return set(reduce(dict.get, self.claim_groups_key.split("."), user_info))
         except TypeError:
             self.log.error(
                 f"The claim_groups_key {self.claim_groups_key} does not exist in the user token"
diff --git a/oauthenticator/oauth2.py b/oauthenticator/oauth2.py
index a0b4e98a..8c2d3ef5 100644
--- a/oauthenticator/oauth2.py
+++ b/oauthenticator/oauth2.py
@@ -732,7 +732,7 @@ def build_auth_state_dict(self, token_info, user_info):
             self.user_auth_state_key: user_info,
         }
 
-    async def update_auth_model(self, username, auth_model):
+    async def update_auth_model(self, auth_model):
         """
         Updates and returns the `auth_model` dict.
 

From b8227ac85bbdffc710af67ba810071f0b06b976d Mon Sep 17 00:00:00 2001
From: Erik Sundell <erik.i.sundell@gmail.com>
Date: Tue, 25 Apr 2023 17:37:18 +0200
Subject: [PATCH 24/57] Add and update fixme comments while developing

---
 oauthenticator/cilogon.py | 13 ++++++-------
 oauthenticator/oauth2.py  |  2 ++
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/oauthenticator/cilogon.py b/oauthenticator/cilogon.py
index 8d5ab09f..127118b4 100644
--- a/oauthenticator/cilogon.py
+++ b/oauthenticator/cilogon.py
@@ -298,15 +298,14 @@ async def check_allowed(self, username, auth_model):
         if auth_model["admin"]:
             return True
 
-        # FIXME: This needs to be thought over very carefully.
+        # FIXME: I chatted with Georgiana and we concluded that the user must be
+        #        part of allowed_idps no matter what, following that, the user
+        #        must either be part of allowed_users or allowed_domains to be
+        #        authorized if either is configured, and otherwise all users are
+        #        authorized.
         #
-        #        Is there or isn't there a commonly agreed "excepted behavior"
-        #        for when considering a combination of allowed_users,
-        #        allowed_idps, and allowed_idps allowed_domains?
+        #        The drafted implementation doesn't reflect this yet.
         #
-
-        # if allowed_users or allowed_idps is configured, we deny users not
-        # part of either
         if self.allowed_users or self.allowed_idps:
             user_info = auth_model["auth_state"][self.user_auth_state_key]
             selected_idp = user_info["idp"]
diff --git a/oauthenticator/oauth2.py b/oauthenticator/oauth2.py
index 8c2d3ef5..b2dbb61b 100644
--- a/oauthenticator/oauth2.py
+++ b/oauthenticator/oauth2.py
@@ -785,6 +785,8 @@ async def authenticate(self, handler, data=None, **kwargs):
         # build the auth model to be read if authentication goes right
         auth_model = {
             "name": username,
+            # FIXME: Think about is_admin override, should we call is_admin from
+            #        here or possibly after update_auth_model?
             "admin": True if username in self.admin_users else None,
             "auth_state": self.build_auth_state_dict(token_info, user_info),
         }

From 91dfb038c53287dcc684829429b7e7e354bd5eff Mon Sep 17 00:00:00 2001
From: Erik Sundell <erik.i.sundell@gmail.com>
Date: Wed, 26 Apr 2023 07:59:38 +0200
Subject: [PATCH 25/57] Update where admin status is set and considered

---
 oauthenticator/bitbucket.py | 13 ++++---------
 oauthenticator/generic.py   | 21 ++++++++++-----------
 oauthenticator/openshift.py | 18 +++++++-----------
 3 files changed, 21 insertions(+), 31 deletions(-)

diff --git a/oauthenticator/bitbucket.py b/oauthenticator/bitbucket.py
index 3c2b0d9f..cb54a129 100644
--- a/oauthenticator/bitbucket.py
+++ b/oauthenticator/bitbucket.py
@@ -58,17 +58,12 @@ async def _fetch_user_teams(self, access_token, token_type):
 
     async def update_auth_model(self, auth_model):
         """
-        Set the admin status based on finding the username in `admin_users` and
-        fetch user teams if `allowed_teams` is configured.
+        Fetch and store `user_teams` in auth state if `allowed_teams` is
+        configured.
         """
-        access_token = auth_model["auth_state"]["token_response"]["access_token"]
-        token_type = auth_model["auth_state"]["token_response"]["token_type"]
-
-        username = auth_model["name"]
-        if username in self.admin_users:
-            auth_model["admin"] = True
-
         if self.allowed_teams:
+            access_token = auth_model["auth_state"]["token_response"]["access_token"]
+            token_type = auth_model["auth_state"]["token_response"]["token_type"]
             user_teams = await self._fetch_user_teams(access_token, token_type)
             auth_model["auth_state"]["user_teams"] = user_teams
 
diff --git a/oauthenticator/generic.py b/oauthenticator/generic.py
index a143705d..37a699f7 100644
--- a/oauthenticator/generic.py
+++ b/oauthenticator/generic.py
@@ -118,18 +118,17 @@ def get_user_groups(self, user_info):
 
     async def update_auth_model(self, auth_model):
         """
-        Set the admin status based on finding the username in `admin_users` or
-        finding a user group part of `admin_groups`.
+        Update admin status based on `admin_groups` if its configured.
         """
-        user_info = auth_model["auth_state"][self.user_auth_state_key]
-
-        username = auth_model["name"]
-        if username in self.admin_users:
-            auth_model["admin"] = True
-        elif self.admin_groups:
-            # if admin_groups is configured, we must either set or unset admin
-            # status and never leave it at None, otherwise removing a user from
-            # the admin_groups won't have an effect
+        if auth_model["admin"]:
+            return auth_model
+
+        if self.admin_groups:
+            # if admin_groups is configured and the user wasn't part of
+            # admin_users, we must set the admin status to True or False,
+            # otherwise removing a user from the admin_groups won't have an
+            # effect
+            user_info = auth_model["auth_state"][self.user_auth_state_key]
             user_groups = self.get_user_groups(user_info)
             auth_model["admin"] = any(user_groups & self.admin_groups)
 
diff --git a/oauthenticator/openshift.py b/oauthenticator/openshift.py
index bcfef1a8..bc99b9dd 100644
--- a/oauthenticator/openshift.py
+++ b/oauthenticator/openshift.py
@@ -91,18 +91,14 @@ def user_info_to_username(self, user_info):
 
     async def update_auth_model(self, auth_model):
         """
-        Set the admin status based on finding the username in `admin_users` or
-        finding a user group part of `admin_groups`.
+        Update admin status based on `admin_groups` if its configured.
         """
-        user_info = auth_model["auth_state"][self.user_auth_state_key]
-
-        username = auth_model["name"]
-        if username in self.admin_users:
-            auth_model["admin"] = True
-        elif self.admin_groups:
-            # if admin_groups is configured, we must either set or unset admin
-            # status and never leave it at None, otherwise removing a user from
-            # the admin_groups won't have an effect
+        if self.admin_groups:
+            # if admin_groups is configured and the user wasn't part of
+            # admin_users, we must set the admin status to True or False,
+            # otherwise removing a user from the admin_groups won't have an
+            # effect
+            user_info = auth_model["auth_state"][self.user_auth_state_key]
             user_groups = set(user_info["groups"])
             auth_model["admin"] = any(user_groups & self.admin_groups)
 

From 6d1694679229bacc005406e7b153d6b168d7cf94 Mon Sep 17 00:00:00 2001
From: Georgiana Dolocan <georgiana.dolocan@gmail.com>
Date: Mon, 1 May 2023 11:49:59 +0300
Subject: [PATCH 26/57] Update cilogon to reflect latest discussions about 
 taking priority over  when set

---
 oauthenticator/cilogon.py | 81 ++++++++++++++++++++++++++-------------
 1 file changed, 55 insertions(+), 26 deletions(-)

diff --git a/oauthenticator/cilogon.py b/oauthenticator/cilogon.py
index 127118b4..575f764c 100644
--- a/oauthenticator/cilogon.py
+++ b/oauthenticator/cilogon.py
@@ -248,7 +248,19 @@ def _validate_allowed_idps(self, proposal):
         """,
     )
 
-    def user_info_to_username(self, user_info):
+    def _get_final_username_claim_list(self, user_info):
+        """
+        The username claims that will be used to determine the hub username can be set through:
+         - `CILogonOAutnenticator.username_claim`, that can be extended through `CILogonOAutnenticator.additional_username_claims`
+         or
+         - `CILogonOAuthenticator.allowed_idps.<idp>.username_claim`, that
+            will overwrite any value set through CILogonOAuthenticator.username_claim
+            for this identity provider.
+
+        This function returns the username claim list that will be used for the current user trying to login
+        based on the idp that they have selected. If no `CILogonOAutnenticator.allowed_idps` is set, then
+        `CILogonOAutnenticator.username_claim` will be used.
+        """
         username_claims = [self.username_claim]
         if self.additional_username_claims:
             username_claims.extend(self.additional_username_claims)
@@ -258,13 +270,21 @@ def user_info_to_username(self, user_info):
             username_claims = [
                 self.allowed_idps[selected_idp]["username_derivation"]["username_claim"]
             ]
+        return username_claims
 
+    def _get_username_from_claim_list(self, user_info, username_claims):
         username = None
         for claim in username_claims:
             username = user_info.get(claim)
             if username:
                 break
 
+        return username
+
+    def user_info_to_username(self, user_info):
+        username_claims = self._get_final_username_claim_list(user_info)
+        username = self._get_username_from_claim_list(user_info, username_claims)
+
         if not username:
             user_info_keys = sorted(user_info.keys())
             self.log.error(
@@ -274,6 +294,7 @@ def user_info_to_username(self, user_info):
 
         # Optionally strip idp domain or prefix the username
         if self.allowed_idps:
+            selected_idp = user_info["idp"]
             username_derivation = self.allowed_idps[selected_idp]["username_derivation"]
             action = username_derivation.get("action")
 
@@ -298,34 +319,43 @@ async def check_allowed(self, username, auth_model):
         if auth_model["admin"]:
             return True
 
-        # FIXME: I chatted with Georgiana and we concluded that the user must be
+        # FIXME: Erik and Georgiana chatted and concluded that the user must be
         #        part of allowed_idps no matter what, following that, the user
         #        must either be part of allowed_users or allowed_domains to be
         #        authorized if either is configured, and otherwise all users are
         #        authorized.
         #
-        #        The drafted implementation doesn't reflect this yet.
-        #
-        if self.allowed_users or self.allowed_idps:
+        #        Updated to reflect the discussion.
+        # TODO:  Validate the implementation
+        if self.allowed_idps:
             user_info = auth_model["auth_state"][self.user_auth_state_key]
             selected_idp = user_info["idp"]
+            if selected_idp not in self.allowed_idps.keys():
+                self.log.error(
+                    f"Trying to login from an identity provider that was not allowed {selected_idp}",
+                )
+                raise web.HTTPError(
+                    500,
+                    "Trying to login using an identity provider that was not allowed",
+                )
 
-            if username in self.allowed_users:
-                return True
+            allowed_domains = self.allowed_idps[selected_idp].get("allowed_domains")
+            if self.allowed_users or allowed_domains:
+                if username in self.allowed_users:
+                    return True
 
-            if self.allowed_idps and selected_idp in self.allowed_idps:
-                allowed_domains = self.allowed_idps[selected_idp].get("allowed_domains")
                 if allowed_domains:
-                    # FIXME: check allowed is called after username is stripped /
-                    #        prefixed, so we don't know the username's email domain
-                    #        anymore.
+                    # TODO:  broke apart
+                    #        user_info_to_username in multiple functions
+                    #        allowing us to get the username before the optional
+                    #        stripping operation and use it here.
+                    #        Validate implementation
                     #
-                    #        An idea to resolve this is to break apart
-                    #        user_info_to_username in a piece before and after the
-                    #        optional domain stripping operation, allowing us to
-                    #        call it again without the strip operation.
-                    #
-                    user_domain = username.split("@")[1]
+                    username_claims = self._get_final_username_claim_list(user_info)
+                    username_with_domain = self._get_username_from_claim_list(
+                        user_info, username_claims
+                    )
+                    user_domain = username_with_domain.split("@")[1]
                     if user_domain in allowed_domains:
                         return True
                     else:
@@ -333,15 +363,14 @@ async def check_allowed(self, username, auth_model):
                             500,
                             "Trying to login using a domain that was not allowed",
                         )
-            else:
-                self.log.error(
-                    f"Trying to login from an identity provider that was not allowed {selected_idp}",
-                )
-                raise web.HTTPError(
-                    500,
-                    "Trying to login using an identity provider that was not allowed",
-                )
 
+                return False
+        # Although not recommended, it might be that `allowed_idps` is not specified
+        # In this case we need to make sure we still check `allowed_users` and don't assume
+        # everyone should be authorized
+        elif self.allowed_users:
+            if username in self.allowed_users:
+                return True
             return False
 
         # otherwise, authorize all users

From 26065b8416d2302d3bcba0cf326514bbf9891be7 Mon Sep 17 00:00:00 2001
From: Georgiana Dolocan <georgiana.dolocan@gmail.com>
Date: Mon, 1 May 2023 12:10:49 +0300
Subject: [PATCH 27/57] Change user_is_authorised for check_allowed in github

---
 oauthenticator/github.py | 52 +++++++++++++++++++++++++++-------------
 1 file changed, 36 insertions(+), 16 deletions(-)

diff --git a/oauthenticator/github.py b/oauthenticator/github.py
index acb3d7da..662a16f8 100644
--- a/oauthenticator/github.py
+++ b/oauthenticator/github.py
@@ -128,24 +128,44 @@ def _github_client_secret_changed(self, name, old, new):
         config=True,
     )
 
-    async def user_is_authorized(self, auth_model):
-        # Check if user is a member of any allowed organizations.
-        access_token = auth_model["auth_state"]["token_response"]["access_token"]
-        token_type = auth_model["auth_state"]["token_response"]["token_type"]
+    async def check_allowed(self, username, auth_model):
+        """
+        Returns True for users allowed to be authorized.
 
-        if self.allowed_organizations:
-            for org in self.allowed_organizations:
-                user_in_org = await self._check_membership_allowed_organizations(
-                    org, auth_model["name"], access_token, token_type
-                )
-                if user_in_org:
-                    break
-            else:  # User not found in member list for any organisation
-                self.log.warning(
-                    f"User {auth_model['name']} is not in allowed org list",
-                )
-                return False
+        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
+
+        # if allowed_users or allowed_organizations is configured, we deny users not
+        # part of either
+        if self.allowed_users or self.allowed_organizations:
+            if username in self.allowed_users:
+                return True
+
+            if self.allowed_organizations:
+                access_token = auth_model["auth_state"]["token_response"][
+                    "access_token"
+                ]
+                token_type = auth_model["auth_state"]["token_response"]["token_type"]
+                for org in self.allowed_organizations:
+                    user_in_org = await self._check_membership_allowed_organizations(
+                        org, auth_model["name"], access_token, token_type
+                    )
+                    if user_in_org:
+                        return True
+                else:
+                    # User not found in member list for any organi`ation
+                    self.log.warning(
+                        f"User {auth_model['name']} is not in allowed org list",
+                    )
+
+            return False
 
+        # otherwise, authorize all users
         return True
 
     async def update_auth_model(self, auth_model):

From effc9fdc9c32d20399477e8d786c91d44d5be642 Mon Sep 17 00:00:00 2001
From: Georgiana Dolocan <georgiana.dolocan@gmail.com>
Date: Mon, 1 May 2023 18:29:46 +0300
Subject: [PATCH 28/57] Change user_is_authorised for check_allowed in gitlab

---
 oauthenticator/gitlab.py | 70 +++++++++++++++++++++-------------------
 1 file changed, 36 insertions(+), 34 deletions(-)

diff --git a/oauthenticator/gitlab.py b/oauthenticator/gitlab.py
index 73e9228e..de47acb4 100644
--- a/oauthenticator/gitlab.py
+++ b/oauthenticator/gitlab.py
@@ -7,7 +7,6 @@
 
 from jupyterhub.auth import LocalAuthenticator
 from tornado.escape import url_escape
-from tornado.httpclient import HTTPRequest
 from traitlets import CUnicode, Set, Unicode, default
 
 from .oauth2 import OAuthenticator
@@ -113,47 +112,53 @@ def _userdata_url_default(self):
     )
 
     gitlab_version = None
+    member_api_variant = None
 
-    async def user_is_authorized(self, auth_model):
-        access_token = auth_model["auth_state"]["token_response"]["access_token"]
-        user_id = auth_model["auth_state"][self.user_auth_state_key]["id"]
-
+    async def _set_gitlab_version(self, access_token):
         # memoize gitlab version for class lifetime
         if self.gitlab_version is None:
             self.gitlab_version = await self._get_gitlab_version(access_token)
             self.member_api_variant = 'all/' if self.gitlab_version >= [12, 4] else ''
 
-        # Check if user is a member of any allowed groups or projects.
-        # These checks are performed here, as it requires `access_token`.
-        user_in_group = user_in_project = False
-        is_group_specified = is_project_id_specified = False
+    async def check_allowed(self, username, auth_model):
+        """
+        Returns True for users allowed to be authorized.
 
-        if self.allowed_gitlab_groups:
-            is_group_specified = True
-            user_in_group = await self._check_membership_allowed_groups(
-                user_id, access_token
-            )
+        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
 
-        # We skip project_id check if user is in allowed group.
-        if self.allowed_project_ids and not user_in_group:
-            is_project_id_specified = True
-            user_in_project = await self._check_membership_allowed_project_ids(
-                user_id, access_token
-            )
+        # if allowed_users or allowed_gitlab_groups or allowed_project_ids is configured,
+        # we deny users not part of either
+        if self.allowed_users or self.allowed_gitlab_groups or self.allowed_project_ids:
+            if username in self.allowed_users:
+                return True
 
-        no_config_specified = not (is_group_specified or is_project_id_specified)
+            await self._set_gitlab_version(access_token)
+            access_token = auth_model["auth_state"]["token_response"]["access_token"]
+            user_id = auth_model["auth_state"][self.user_auth_state_key]["id"]
 
-        if (
-            (is_group_specified and user_in_group)
-            or (is_project_id_specified and user_in_project)
-            or no_config_specified
-        ):
-            return True
+            if self.allowed_gitlab_groups:
+                user_in_group = await self._check_membership_allowed_groups(
+                    user_id, access_token
+                )
+                if user_in_group:
+                    return True
 
-        self.log.warning(
-            f"{auth_model['name']} not in group or project allowed list",
-        )
-        return False
+            if self.allowed_project_ids:
+                user_in_project = await self._check_membership_allowed_project_ids(
+                    user_id, access_token
+                )
+                if user_in_project:
+                    return True
+            return False
+
+        # otherwise, authorize all users
+        return True
 
     async def _get_gitlab_version(self, access_token):
         url = f"{self.gitlab_api}/version"
@@ -177,9 +182,6 @@ async def _check_membership_allowed_groups(self, user_id, access_token):
                 self.member_api_variant,
                 user_id,
             )
-            req = HTTPRequest(
-                url,
-            )
             resp = await self.httpfetch(
                 url,
                 parse_json=False,

From d5a3dd9d7a3d8a760769805dbfdb3b6549e14615 Mon Sep 17 00:00:00 2001
From: Georgiana Dolocan <georgiana.dolocan@gmail.com>
Date: Mon, 1 May 2023 20:08:11 +0300
Subject: [PATCH 29/57] Reorder

---
 oauthenticator/gitlab.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/oauthenticator/gitlab.py b/oauthenticator/gitlab.py
index de47acb4..9087db62 100644
--- a/oauthenticator/gitlab.py
+++ b/oauthenticator/gitlab.py
@@ -138,8 +138,8 @@ async def check_allowed(self, username, auth_model):
             if username in self.allowed_users:
                 return True
 
-            await self._set_gitlab_version(access_token)
             access_token = auth_model["auth_state"]["token_response"]["access_token"]
+            await self._set_gitlab_version(access_token)
             user_id = auth_model["auth_state"][self.user_auth_state_key]["id"]
 
             if self.allowed_gitlab_groups:

From 9afefa27eb48388d755718d2b5d375ab6854e51f Mon Sep 17 00:00:00 2001
From: Georgiana Dolocan <georgiana.dolocan@gmail.com>
Date: Tue, 2 May 2023 12:54:22 +0300
Subject: [PATCH 30/57] Change user_is_authorised for check_allowed in globus

---
 oauthenticator/gitlab.py            |  1 +
 oauthenticator/globus.py            | 64 ++++++++++++++++++++---------
 oauthenticator/tests/test_globus.py |  4 +-
 3 files changed, 48 insertions(+), 21 deletions(-)

diff --git a/oauthenticator/gitlab.py b/oauthenticator/gitlab.py
index 9087db62..11123af7 100644
--- a/oauthenticator/gitlab.py
+++ b/oauthenticator/gitlab.py
@@ -155,6 +155,7 @@ async def check_allowed(self, username, auth_model):
                 )
                 if user_in_project:
                     return True
+
             return False
 
         # otherwise, authorize all users
diff --git a/oauthenticator/globus.py b/oauthenticator/globus.py
index fb182cf3..43edfe1e 100644
--- a/oauthenticator/globus.py
+++ b/oauthenticator/globus.py
@@ -182,7 +182,7 @@ async def pre_spawn_start(self, user, spawner):
 
     def get_globus_tokens(self, token_info):
         # Each token should have these attributes. Resource server is optional,
-        # and likely won't be present.
+        # and likely xwon't be present.
         token_attrs = [
             'expires_in',
             'resource_server',
@@ -230,6 +230,9 @@ def build_auth_state_dict(self, token_info, user_info):
             self.user_auth_state_key: user_info,
         }
 
+    # FIXME: Should we persist info about user groups in auth model
+    #        to be consistent with what's happening in bitbucket.py
+    #        where the `auth_model`` is updated with `user_teams`.
     async def get_users_groups_ids(self, tokens):
         user_group_ids = set()
         # Get Groups access token, may not be in dict headed to auth state
@@ -248,33 +251,56 @@ async def get_users_groups_ids(self, tokens):
 
         return user_group_ids
 
-    async def user_is_authorized(self, auth_model):
+    # TODO: Mark this as breaking change because before globus_admin_groups and globus_user_groups
+    #       were the ones that were being considered to determine the admin and access status of a user
+    #       and not their union with the generic admin_users and allowed_users like it is happening now.
+    async def check_allowed(self, username, auth_model):
+        """
+        Returns True for users allowed to be authorized.
+
+        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
+
+        # FIXME: consider overriding the `is_admin`` and add this logic there
         tokens = self.get_globus_tokens(auth_model["auth_state"]["token_response"])
+        user_group_ids = await self.get_users_groups_ids(tokens)
+        if self.admin_globus_groups:
+            if self.check_user_in_groups(user_group_ids, self.admin_globus_groups):
+                auth_model["admin"] = True
+                return True
 
-        if self.allowed_globus_groups or self.admin_globus_groups:
-            # If any of these configurations are set, user must be in the allowed or admin Globus Group
-            user_group_ids = await self.get_users_groups_ids(tokens)
-            if not self.check_user_in_groups(
-                user_group_ids, self.allowed_globus_groups
-            ):
-                if not self.check_user_in_groups(
-                    user_group_ids, self.admin_globus_groups
+        # if allowed_users or allowed_globus_groups is configured, we deny users not part of either
+        if self.allowed_users or self.allowed_globus_groups:
+            if username in self.allowed_users:
+                return True
+
+            if self.allowed_globus_groups:
+                if self.check_user_in_groups(
+                    user_group_ids, self.allowed_globus_groups
                 ):
-                    username = self.user_info_to_username(
-                        auth_model["auth_state"][self.user_auth_state_key]
-                    )
-                    self.log.warning(f"{username} not in an allowed Globus Group")
-                    return False
+                    return True
+
+                self.log.warning(f"{username} not in an allowed Globus Group")
 
+            return False
+
+        # otherwise, authorize all users
         return True
 
     async def update_auth_model(self, auth_model):
-        username = self.user_info_to_username(
-            auth_model["auth_state"][self.user_auth_state_key]
-        )
-        tokens = self.get_globus_tokens(auth_model["auth_state"]["token_response"])
+        # If users is already marked as an admin, it means that
+        # they are present in the `admin_users`` list
+        # no need to check groups membership
+        if auth_model['admin'] is True:
+            return auth_model
 
         if self.admin_globus_groups:
+            tokens = self.get_globus_tokens(auth_model["auth_state"]["token_response"])
             # If any of these configurations are set, user must be in the allowed or admin Globus Group
             user_group_ids = await self.get_users_groups_ids(tokens)
             # Admin users are being managed via Globus Groups
diff --git a/oauthenticator/tests/test_globus.py b/oauthenticator/tests/test_globus.py
index a06ebf4b..6963fe63 100644
--- a/oauthenticator/tests/test_globus.py
+++ b/oauthenticator/tests/test_globus.py
@@ -229,7 +229,7 @@ async def test_restricted_domain(globus_client):
     authenticator.identity_provider = 'alliance.gov'
     handler = globus_client.handler_for_user(user_model('wash@uflightacademy.edu'))
     with raises(web.HTTPError) as exc:
-        authenticator.get_authenticated_user(handler, None)
+        await authenticator.get_authenticated_user(handler, None)
     assert exc.value.status_code == 403
 
 
@@ -294,7 +294,7 @@ async def test_username_from_email_restricted_fail(globus_client):
     um = user_model('wash@serenity.com', 'alan@tudyk.org')
     handler = globus_client.handler_for_user(um)
     with raises(web.HTTPError) as exc:
-        authenticator.get_authenticated_user(handler, None)
+        await authenticator.get_authenticated_user(handler, None)
     assert exc.value.status_code == 403
 
 

From 35338a62b49e15c435784e84fd7179c07897f3d5 Mon Sep 17 00:00:00 2001
From: Georgiana Dolocan <georgiana.dolocan@gmail.com>
Date: Tue, 2 May 2023 12:57:07 +0300
Subject: [PATCH 31/57] Add note about updating auth_model in GitHub

---
 oauthenticator/github.py | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/oauthenticator/github.py b/oauthenticator/github.py
index 662a16f8..acb03938 100644
--- a/oauthenticator/github.py
+++ b/oauthenticator/github.py
@@ -109,6 +109,10 @@ def _github_client_secret_changed(self, name, old, new):
         config=True, help="Automatically allow members of selected organizations"
     )
 
+    # FIXME: when set saves the user teams inside the auth state under a `teams` key.
+    #        Should we standardize the `teams` key?
+    #        (In bitbucket.py the key is called `user_teams` and we set it by default without any flag)
+    #        What about allowed orgs. Should we update the auth_model with these too?
     populate_teams_in_auth_state = Bool(
         False,
         help="""

From fcfe088ac219d7418b89b12b43d55bec940a8221 Mon Sep 17 00:00:00 2001
From: Georgiana Dolocan <georgiana.dolocan@gmail.com>
Date: Tue, 2 May 2023 13:25:16 +0300
Subject: [PATCH 32/57] Small fixes

---
 oauthenticator/cilogon.py |  6 +++---
 oauthenticator/globus.py  | 22 ++++++++++++++--------
 2 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/oauthenticator/cilogon.py b/oauthenticator/cilogon.py
index 575f764c..10da6f47 100644
--- a/oauthenticator/cilogon.py
+++ b/oauthenticator/cilogon.py
@@ -299,7 +299,7 @@ def user_info_to_username(self, user_info):
             action = username_derivation.get("action")
 
             if action == "strip_idp_domain":
-                username = username.split("@")[0]
+                username = username.split("@", 1)[0]
             elif action == "prefix":
                 prefix = username_derivation["prefix"]
                 username = f"{prefix}:{username}"
@@ -335,7 +335,7 @@ async def check_allowed(self, username, auth_model):
                     f"Trying to login from an identity provider that was not allowed {selected_idp}",
                 )
                 raise web.HTTPError(
-                    500,
+                    403,
                     "Trying to login using an identity provider that was not allowed",
                 )
 
@@ -355,7 +355,7 @@ async def check_allowed(self, username, auth_model):
                     username_with_domain = self._get_username_from_claim_list(
                         user_info, username_claims
                     )
-                    user_domain = username_with_domain.split("@")[1]
+                    user_domain = username_with_domain.split("@", 1)[1]
                     if user_domain in allowed_domains:
                         return True
                     else:
diff --git a/oauthenticator/globus.py b/oauthenticator/globus.py
index 43edfe1e..2c878636 100644
--- a/oauthenticator/globus.py
+++ b/oauthenticator/globus.py
@@ -274,6 +274,19 @@ async def check_allowed(self, username, auth_model):
                 auth_model["admin"] = True
                 return True
 
+        if self.identity_provider:
+            user_info = auth_model["auth_state"][self.user_auth_state_key]
+            domain = user_info.get(self.username_claim).split('@', 1)[-1]
+            if domain != self.identity_provider:
+                self.log.error(
+                    f"Trying to login from an identity provider that was not allowed {domain}",
+                )
+                raise HTTPError(
+                    403,
+                    f"This site is restricted to {self.identity_provider} accounts. "
+                    "Please link your account at app.globus.org/account.",
+                )
+
         # if allowed_users or allowed_globus_groups is configured, we deny users not part of either
         if self.allowed_users or self.allowed_globus_groups:
             if username in self.allowed_users:
@@ -320,14 +333,7 @@ def user_info_to_username(self, user_info):
 
         # It's possible for identity provider domains to be namespaced
         # https://docs.globus.org/api/auth/specification/#identity_provider_namespaces # noqa
-        username, domain = user_info.get(self.username_claim).split('@', 1)
-        if self.identity_provider and domain != self.identity_provider:
-            raise HTTPError(
-                403,
-                f"This site is restricted to {self.identity_provider} accounts. "
-                "Please link your account at app.globus.org/account.",
-            )
-        return username
+        return user_info.get(self.username_claim).split('@')[0]
 
     def get_default_headers(self):
         return {"Accept": "application/json", "User-Agent": "JupyterHub"}

From 980fb5e8eb0422996ef3dc55f323388f3a9ecdb2 Mon Sep 17 00:00:00 2001
From: Georgiana Dolocan <georgiana.dolocan@gmail.com>
Date: Wed, 3 May 2023 11:27:41 +0300
Subject: [PATCH 33/57] Change user_is_authorised for check_allowed for google

---
 oauthenticator/google.py            |  96 +++++++++++++++----------
 oauthenticator/tests/test_google.py | 104 ++++++++++++++++------------
 2 files changed, 120 insertions(+), 80 deletions(-)

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"}

From 93b14a926a6bc60a31cad9cbc9751be751c09d27 Mon Sep 17 00:00:00 2001
From: Georgiana <georgiana.dolocan@gmail.com>
Date: Tue, 13 Jun 2023 11:39:19 +0300
Subject: [PATCH 34/57] Raise a 403 instead of 500 when logging in using an
 unallowed domain

Co-authored-by: Erik Sundell <erik.i.sundell@gmail.com>
---
 oauthenticator/cilogon.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/oauthenticator/cilogon.py b/oauthenticator/cilogon.py
index 10da6f47..8bff6e2b 100644
--- a/oauthenticator/cilogon.py
+++ b/oauthenticator/cilogon.py
@@ -360,7 +360,7 @@ async def check_allowed(self, username, auth_model):
                         return True
                     else:
                         raise web.HTTPError(
-                            500,
+                            403,
                             "Trying to login using a domain that was not allowed",
                         )
 

From c9bbbcacf8aa7b0a5cb9eb1c2f3e5389df5c8fa6 Mon Sep 17 00:00:00 2001
From: Georgiana Dolocan <georgiana.dolocan@gmail.com>
Date: Tue, 13 Jun 2023 11:42:04 +0300
Subject: [PATCH 35/57] Use username var directly since it's available

Co-authored-by: Erik Sundell <erik.i.sundell@gmail.com>
---
 oauthenticator/github.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/oauthenticator/github.py b/oauthenticator/github.py
index acb03938..b152c3b9 100644
--- a/oauthenticator/github.py
+++ b/oauthenticator/github.py
@@ -157,14 +157,14 @@ async def check_allowed(self, username, auth_model):
                 token_type = auth_model["auth_state"]["token_response"]["token_type"]
                 for org in self.allowed_organizations:
                     user_in_org = await self._check_membership_allowed_organizations(
-                        org, auth_model["name"], access_token, token_type
+                        org, username, access_token, token_type
                     )
                     if user_in_org:
                         return True
                 else:
                     # User not found in member list for any organi`ation
                     self.log.warning(
-                        f"User {auth_model['name']} is not in allowed org list",
+                        f"User {username} is not in allowed org list",
                     )
 
             return False

From 98692b1415f4a303edb506a19fdb10db1dc339a5 Mon Sep 17 00:00:00 2001
From: Georgiana <georgiana.dolocan@gmail.com>
Date: Tue, 13 Jun 2023 11:43:35 +0300
Subject: [PATCH 36/57] Fix comment

Co-authored-by: Erik Sundell <erik.i.sundell@gmail.com>
---
 oauthenticator/gitlab.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/oauthenticator/gitlab.py b/oauthenticator/gitlab.py
index 11123af7..4dcb23f0 100644
--- a/oauthenticator/gitlab.py
+++ b/oauthenticator/gitlab.py
@@ -125,8 +125,8 @@ async def check_allowed(self, username, auth_model):
         Returns True for users allowed to be authorized.
 
         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`.
+        either part of `allowed_users`, `allowed_gitlab_groups`, or `allowed_project_ids`,
+        and not just those part of `allowed_users`.
         """
         # allow admin users recognized via admin_users or update_auth_model
         if auth_model["admin"]:

From ec139e1a3370cdc28670d86990e855ed9b665f71 Mon Sep 17 00:00:00 2001
From: Georgiana <georgiana.dolocan@gmail.com>
Date: Tue, 13 Jun 2023 11:43:57 +0300
Subject: [PATCH 37/57] Fix typo

Co-authored-by: Erik Sundell <erik.i.sundell@gmail.com>
---
 oauthenticator/globus.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/oauthenticator/globus.py b/oauthenticator/globus.py
index 2c878636..b6287b40 100644
--- a/oauthenticator/globus.py
+++ b/oauthenticator/globus.py
@@ -182,7 +182,7 @@ async def pre_spawn_start(self, user, spawner):
 
     def get_globus_tokens(self, token_info):
         # Each token should have these attributes. Resource server is optional,
-        # and likely xwon't be present.
+        # and likely won't be present.
         token_attrs = [
             'expires_in',
             'resource_server',

From 97cc182fd58ec7966e1579cad5d5d1d5c58f65d2 Mon Sep 17 00:00:00 2001
From: Georgiana <georgiana.dolocan@gmail.com>
Date: Tue, 13 Jun 2023 11:44:42 +0300
Subject: [PATCH 38/57] Rm extra quote

Co-authored-by: Erik Sundell <erik.i.sundell@gmail.com>
---
 oauthenticator/globus.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/oauthenticator/globus.py b/oauthenticator/globus.py
index b6287b40..54ccb9ae 100644
--- a/oauthenticator/globus.py
+++ b/oauthenticator/globus.py
@@ -232,7 +232,7 @@ def build_auth_state_dict(self, token_info, user_info):
 
     # FIXME: Should we persist info about user groups in auth model
     #        to be consistent with what's happening in bitbucket.py
-    #        where the `auth_model`` is updated with `user_teams`.
+    #        where the `auth_model` is updated with `user_teams`.
     async def get_users_groups_ids(self, tokens):
         user_group_ids = set()
         # Get Groups access token, may not be in dict headed to auth state

From a982e1b9670ec336f0fe7a6ac2bac6d47d588368 Mon Sep 17 00:00:00 2001
From: Georgiana <georgiana.dolocan@gmail.com>
Date: Tue, 13 Jun 2023 11:45:04 +0300
Subject: [PATCH 39/57] Fix group related comment

Co-authored-by: Erik Sundell <erik.i.sundell@gmail.com>
---
 oauthenticator/globus.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/oauthenticator/globus.py b/oauthenticator/globus.py
index 54ccb9ae..ea2e7260 100644
--- a/oauthenticator/globus.py
+++ b/oauthenticator/globus.py
@@ -259,7 +259,7 @@ async def check_allowed(self, username, auth_model):
         Returns True for users allowed to be authorized.
 
         Overrides the OAuthenticator.check_allowed implementation to allow users
-        either part of `allowed_users` or `allowed_organizations`, and not just those
+        either part of `allowed_users` or `allowed_globus_groups`, and not just those
         part of `allowed_users`.
         """
         # allow admin users recognized via admin_users or update_auth_model

From 11b35cb7540361bd032fd704500df561ab874f87 Mon Sep 17 00:00:00 2001
From: Georgiana Dolocan <georgiana.dolocan@gmail.com>
Date: Tue, 13 Jun 2023 11:47:25 +0300
Subject: [PATCH 40/57] Move comment to a place where it makes more sense

---
 oauthenticator/globus.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/oauthenticator/globus.py b/oauthenticator/globus.py
index ea2e7260..d871cf06 100644
--- a/oauthenticator/globus.py
+++ b/oauthenticator/globus.py
@@ -275,6 +275,8 @@ async def check_allowed(self, username, auth_model):
                 return True
 
         if self.identity_provider:
+            # It's possible for identity provider domains to be namespaced
+            # https://docs.globus.org/api/auth/specification/#identity_provider_namespaces
             user_info = auth_model["auth_state"][self.user_auth_state_key]
             domain = user_info.get(self.username_claim).split('@', 1)[-1]
             if domain != self.identity_provider:
@@ -331,8 +333,6 @@ def user_info_to_username(self, user_info):
         will have the 'foouser' account in Jupyterhub.
         """
 
-        # It's possible for identity provider domains to be namespaced
-        # https://docs.globus.org/api/auth/specification/#identity_provider_namespaces # noqa
         return user_info.get(self.username_claim).split('@')[0]
 
     def get_default_headers(self):

From b7b87d3f6c9a2b8d25437b6827c005e83134bf8f Mon Sep 17 00:00:00 2001
From: Georgiana <georgiana.dolocan@gmail.com>
Date: Tue, 13 Jun 2023 11:49:01 +0300
Subject: [PATCH 41/57] Rm todo comment

Co-authored-by: Erik Sundell <erik.i.sundell@gmail.com>
---
 oauthenticator/globus.py | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/oauthenticator/globus.py b/oauthenticator/globus.py
index d871cf06..f87b3230 100644
--- a/oauthenticator/globus.py
+++ b/oauthenticator/globus.py
@@ -251,9 +251,6 @@ async def get_users_groups_ids(self, tokens):
 
         return user_group_ids
 
-    # TODO: Mark this as breaking change because before globus_admin_groups and globus_user_groups
-    #       were the ones that were being considered to determine the admin and access status of a user
-    #       and not their union with the generic admin_users and allowed_users like it is happening now.
     async def check_allowed(self, username, auth_model):
         """
         Returns True for users allowed to be authorized.

From 1dcb1eee581b6d4f094e8d9642e5f1196053cfac Mon Sep 17 00:00:00 2001
From: Georgiana Dolocan <georgiana.dolocan@gmail.com>
Date: Tue, 13 Jun 2023 12:18:48 +0300
Subject: [PATCH 42/57] Remove fixme and track it in a todo item

---
 oauthenticator/github.py | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/oauthenticator/github.py b/oauthenticator/github.py
index b152c3b9..32537426 100644
--- a/oauthenticator/github.py
+++ b/oauthenticator/github.py
@@ -109,10 +109,6 @@ def _github_client_secret_changed(self, name, old, new):
         config=True, help="Automatically allow members of selected organizations"
     )
 
-    # FIXME: when set saves the user teams inside the auth state under a `teams` key.
-    #        Should we standardize the `teams` key?
-    #        (In bitbucket.py the key is called `user_teams` and we set it by default without any flag)
-    #        What about allowed orgs. Should we update the auth_model with these too?
     populate_teams_in_auth_state = Bool(
         False,
         help="""

From fb12f5f137aeafe71971d69cc5b0410e86639fd9 Mon Sep 17 00:00:00 2001
From: Georgiana Dolocan <georgiana.dolocan@gmail.com>
Date: Tue, 13 Jun 2023 12:52:22 +0300
Subject: [PATCH 43/57] Set admin status exclusively in  for all authenticators

---
 oauthenticator/globus.py | 18 ++++++------------
 oauthenticator/google.py | 36 +++++++++++++++++-------------------
 2 files changed, 23 insertions(+), 31 deletions(-)

diff --git a/oauthenticator/globus.py b/oauthenticator/globus.py
index f87b3230..f2b8d71c 100644
--- a/oauthenticator/globus.py
+++ b/oauthenticator/globus.py
@@ -263,14 +263,6 @@ async def check_allowed(self, username, auth_model):
         if auth_model["admin"]:
             return True
 
-        # FIXME: consider overriding the `is_admin`` and add this logic there
-        tokens = self.get_globus_tokens(auth_model["auth_state"]["token_response"])
-        user_group_ids = await self.get_users_groups_ids(tokens)
-        if self.admin_globus_groups:
-            if self.check_user_in_groups(user_group_ids, self.admin_globus_groups):
-                auth_model["admin"] = True
-                return True
-
         if self.identity_provider:
             # It's possible for identity provider domains to be namespaced
             # https://docs.globus.org/api/auth/specification/#identity_provider_namespaces
@@ -292,6 +284,9 @@ async def check_allowed(self, username, auth_model):
                 return True
 
             if self.allowed_globus_groups:
+                tokens = self.get_globus_tokens(auth_model["auth_state"]["token_response"])
+                user_group_ids = await self.get_users_groups_ids(tokens)
+
                 if self.check_user_in_groups(
                     user_group_ids, self.allowed_globus_groups
                 ):
@@ -308,7 +303,7 @@ async def update_auth_model(self, auth_model):
         # If users is already marked as an admin, it means that
         # they are present in the `admin_users`` list
         # no need to check groups membership
-        if auth_model['admin'] is True:
+        if auth_model["admin"] is True:
             return auth_model
 
         if self.admin_globus_groups:
@@ -317,10 +312,9 @@ async def update_auth_model(self, auth_model):
             user_group_ids = await self.get_users_groups_ids(tokens)
             # Admin users are being managed via Globus Groups
             # Default to False
-            auth_model['admin'] = False
+            auth_model["admin"] = False
             if self.check_user_in_groups(user_group_ids, self.admin_globus_groups):
-                auth_model['admin'] = True
-
+                auth_model["admin"] = True
         return auth_model
 
     def user_info_to_username(self, user_info):
diff --git a/oauthenticator/google.py b/oauthenticator/google.py
index 86138de0..c0df485b 100644
--- a/oauthenticator/google.py
+++ b/oauthenticator/google.py
@@ -116,24 +116,28 @@ def _cast_hosted_domain(self, proposal):
         help="""Google Apps hosted domain string, e.g. My College""",
     )
 
-    # 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 update_auth_model(self, auth_model):
+        """
+        Updates the `auth_model` dict with info about the admin status.
+        """
+        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))
+        admin_groups = self.admin_google_groups.get(user_domain, set())
+
+        if any(user_groups & admin_groups):
+            auth_model["admin"] = True
+
+        return auth_model
+
+
     async def check_allowed(self, username, auth_model):
         """
         Returns True for users allowed to be authorized.
 
         Overrides the OAuthenticator.check_allowed implementation to allow users
-        either part of `allowed_users` or `allowed_organizations`, and not just those
+        either part of `allowed_users` or `allowed_google_groups`, and not just those
         part of `allowed_users`.
         """
         # allow admin users recognized via admin_users or update_auth_model
@@ -144,12 +148,6 @@ async def check_allowed(self, username, auth_model):
         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}")

From 0d21f601573a8ee139759db15b5724037ae9019b Mon Sep 17 00:00:00 2001
From: "pre-commit-ci[bot]"
 <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Date: Tue, 13 Jun 2023 09:52:39 +0000
Subject: [PATCH 44/57] [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci
---
 oauthenticator/globus.py | 4 +++-
 oauthenticator/google.py | 1 -
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/oauthenticator/globus.py b/oauthenticator/globus.py
index f2b8d71c..5cc9df39 100644
--- a/oauthenticator/globus.py
+++ b/oauthenticator/globus.py
@@ -284,7 +284,9 @@ async def check_allowed(self, username, auth_model):
                 return True
 
             if self.allowed_globus_groups:
-                tokens = self.get_globus_tokens(auth_model["auth_state"]["token_response"])
+                tokens = self.get_globus_tokens(
+                    auth_model["auth_state"]["token_response"]
+                )
                 user_group_ids = await self.get_users_groups_ids(tokens)
 
                 if self.check_user_in_groups(
diff --git a/oauthenticator/google.py b/oauthenticator/google.py
index c0df485b..18c9e247 100644
--- a/oauthenticator/google.py
+++ b/oauthenticator/google.py
@@ -131,7 +131,6 @@ async def update_auth_model(self, auth_model):
 
         return auth_model
 
-
     async def check_allowed(self, username, auth_model):
         """
         Returns True for users allowed to be authorized.

From 25dc2aedbde2acd4eee30e92e0cd1dc6cdf12af3 Mon Sep 17 00:00:00 2001
From: Georgiana Dolocan <georgiana.dolocan@gmail.com>
Date: Tue, 13 Jun 2023 12:58:49 +0300
Subject: [PATCH 45/57] Update comment to match implementation

---
 oauthenticator/cilogon.py | 25 +++++++------------------
 1 file changed, 7 insertions(+), 18 deletions(-)

diff --git a/oauthenticator/cilogon.py b/oauthenticator/cilogon.py
index 8bff6e2b..35e65461 100644
--- a/oauthenticator/cilogon.py
+++ b/oauthenticator/cilogon.py
@@ -308,25 +308,20 @@ def user_info_to_username(self, user_info):
 
     async def check_allowed(self, username, auth_model):
         """
-        Returns True for users allowed to be authorized, raises errors for users
+        Returns True for authorized users, raises errors for users
         denied authorization.
 
-        Overrides the OAuthenticator.check_allowed implementation to allow users
-        either part of `allowed_users` or `allowed_idps`, and not just those
-        part of `allowed_users`.
+        Overrides the `OAuthenticator.check_allowed` implementation to only allow users
+        logging in using a provider that is  part of `allowed_idps`.
+        Following this, the user must either be part of `allowed_users` or `allowed_domains`
+        to be authorized if either is configured, otherwise all users are
+        authorized.
         """
+
         # allow admin users recognized via admin_users or update_auth_model
         if auth_model["admin"]:
             return True
 
-        # FIXME: Erik and Georgiana chatted and concluded that the user must be
-        #        part of allowed_idps no matter what, following that, the user
-        #        must either be part of allowed_users or allowed_domains to be
-        #        authorized if either is configured, and otherwise all users are
-        #        authorized.
-        #
-        #        Updated to reflect the discussion.
-        # TODO:  Validate the implementation
         if self.allowed_idps:
             user_info = auth_model["auth_state"][self.user_auth_state_key]
             selected_idp = user_info["idp"]
@@ -345,12 +340,6 @@ async def check_allowed(self, username, auth_model):
                     return True
 
                 if allowed_domains:
-                    # TODO:  broke apart
-                    #        user_info_to_username in multiple functions
-                    #        allowing us to get the username before the optional
-                    #        stripping operation and use it here.
-                    #        Validate implementation
-                    #
                     username_claims = self._get_final_username_claim_list(user_info)
                     username_with_domain = self._get_username_from_claim_list(
                         user_info, username_claims

From b44d64bb755961f7eb0e1d407dbe563d6fc89bd1 Mon Sep 17 00:00:00 2001
From: Georgiana Dolocan <georgiana.dolocan@gmail.com>
Date: Tue, 13 Jun 2023 14:01:25 +0300
Subject: [PATCH 46/57] Set version from membership check func

---
 oauthenticator/gitlab.py | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/oauthenticator/gitlab.py b/oauthenticator/gitlab.py
index 4dcb23f0..4ea5efc7 100644
--- a/oauthenticator/gitlab.py
+++ b/oauthenticator/gitlab.py
@@ -139,7 +139,6 @@ async def check_allowed(self, username, auth_model):
                 return True
 
             access_token = auth_model["auth_state"]["token_response"]["access_token"]
-            await self._set_gitlab_version(access_token)
             user_id = auth_model["auth_state"][self.user_auth_state_key]["id"]
 
             if self.allowed_gitlab_groups:
@@ -175,6 +174,8 @@ async def _get_gitlab_version(self, access_token):
 
     async def _check_membership_allowed_groups(self, user_id, access_token):
         headers = _api_headers(access_token)
+        await self._set_gitlab_version(access_token)
+
         # Check if user is a member of any group in the allowed list
         for group in map(url_escape, self.allowed_gitlab_groups):
             url = "%s/groups/%s/members/%s%d" % (
@@ -197,6 +198,8 @@ async def _check_membership_allowed_groups(self, user_id, access_token):
 
     async def _check_membership_allowed_project_ids(self, user_id, access_token):
         headers = _api_headers(access_token)
+        await self._set_gitlab_version(access_token)
+
         # Check if user has developer access to any project in the allowed list
         for project in self.allowed_project_ids:
             url = "%s/projects/%s/members/%s%d" % (

From f1f9d7e328873fa1fd322241104091554bf4f0ff Mon Sep 17 00:00:00 2001
From: Georgiana <georgiana.dolocan@gmail.com>
Date: Tue, 13 Jun 2023 14:03:02 +0300
Subject: [PATCH 47/57] Add note about func not being part of the base class

Co-authored-by: Erik Sundell <erik.i.sundell@gmail.com>
---
 oauthenticator/generic.py | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/oauthenticator/generic.py b/oauthenticator/generic.py
index 37a699f7..87bc2f82 100644
--- a/oauthenticator/generic.py
+++ b/oauthenticator/generic.py
@@ -105,6 +105,9 @@ def get_user_groups(self, user_info):
         - If claim_groups_key is a nested dictionary key like
           "permissions.groups", this function returns
           user_info["permissions"]["groups"].
+
+        Note that this method is introduced by GenericOAuthenticator and not
+        present in the base class.
         """
         if callable(self.claim_groups_key):
             return set(self.claim_groups_key(user_info))

From cdd23e26e01b0c38f3550627fb979c41ca09aeb2 Mon Sep 17 00:00:00 2001
From: Georgiana Dolocan <georgiana.dolocan@gmail.com>
Date: Tue, 13 Jun 2023 15:07:01 +0300
Subject: [PATCH 48/57] Add email to default check list of claims  when
 specifics are not set

---
 oauthenticator/cilogon.py | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/oauthenticator/cilogon.py b/oauthenticator/cilogon.py
index 35e65461..ffc15646 100644
--- a/oauthenticator/cilogon.py
+++ b/oauthenticator/cilogon.py
@@ -246,6 +246,7 @@ def _validate_allowed_idps(self, proposal):
         This is useful for linked identities where not all of them return
         the primary username_claim.
         """,
+        default_value=["email"],
     )
 
     def _get_final_username_claim_list(self, user_info):
@@ -266,10 +267,15 @@ def _get_final_username_claim_list(self, user_info):
             username_claims.extend(self.additional_username_claims)
         if self.allowed_idps:
             selected_idp = user_info["idp"]
-            # The username_claim which should be used for this idp
-            username_claims = [
-                self.allowed_idps[selected_idp]["username_derivation"]["username_claim"]
-            ]
+            if selected_idp in self.allowed_idps.keys():
+                # The username_claim which should be used for this idp
+                return [
+                    self.allowed_idps[selected_idp]["username_derivation"][
+                        "username_claim"
+                    ]
+                ]
+            else:
+                return username_claims
         return username_claims
 
     def _get_username_from_claim_list(self, user_info, username_claims):
@@ -295,14 +301,17 @@ def user_info_to_username(self, user_info):
         # Optionally strip idp domain or prefix the username
         if self.allowed_idps:
             selected_idp = user_info["idp"]
-            username_derivation = self.allowed_idps[selected_idp]["username_derivation"]
-            action = username_derivation.get("action")
-
-            if action == "strip_idp_domain":
-                username = username.split("@", 1)[0]
-            elif action == "prefix":
-                prefix = username_derivation["prefix"]
-                username = f"{prefix}:{username}"
+            if selected_idp in self.allowed_idps.keys():
+                username_derivation = self.allowed_idps[selected_idp][
+                    "username_derivation"
+                ]
+                action = username_derivation.get("action")
+
+                if action == "strip_idp_domain":
+                    username = username.split("@", 1)[0]
+                elif action == "prefix":
+                    prefix = username_derivation["prefix"]
+                    username = f"{prefix}:{username}"
 
         return username
 

From efa4e8a037a3ba30f1e469c70961d7620430117b Mon Sep 17 00:00:00 2001
From: Erik Sundell <erik.i.sundell@gmail.com>
Date: Wed, 14 Jun 2023 14:45:49 +0200
Subject: [PATCH 49/57] Workaround edge case when JupyterHub calls
 check_allowed during startup

---
 oauthenticator/bitbucket.py | 6 ++++++
 oauthenticator/cilogon.py   | 5 +++++
 oauthenticator/generic.py   | 6 ++++++
 oauthenticator/github.py    | 6 ++++++
 oauthenticator/gitlab.py    | 6 ++++++
 oauthenticator/globus.py    | 6 ++++++
 oauthenticator/google.py    | 6 ++++++
 oauthenticator/oauth2.py    | 6 ++++++
 oauthenticator/openshift.py | 6 ++++++
 9 files changed, 53 insertions(+)

diff --git a/oauthenticator/bitbucket.py b/oauthenticator/bitbucket.py
index cb54a129..4a537ed2 100644
--- a/oauthenticator/bitbucket.py
+++ b/oauthenticator/bitbucket.py
@@ -77,6 +77,12 @@ async def check_allowed(self, username, auth_model):
         either part of `allowed_users` or `allowed_teams`, and not just those
         part of `allowed_users`.
         """
+        # Workaround situation when JupyterHub.load_roles or
+        # JupyterHub.load_groups is used to create a user, see discussion in
+        # https://github.com/jupyterhub/jupyterhub/issues/4461.
+        if auth_model is None:
+            return True
+
         # allow admin users recognized via admin_users or update_auth_model
         if auth_model["admin"]:
             return True
diff --git a/oauthenticator/cilogon.py b/oauthenticator/cilogon.py
index ffc15646..e8477b20 100644
--- a/oauthenticator/cilogon.py
+++ b/oauthenticator/cilogon.py
@@ -326,6 +326,11 @@ async def check_allowed(self, username, auth_model):
         to be authorized if either is configured, otherwise all users are
         authorized.
         """
+        # Workaround situation when JupyterHub.load_roles or
+        # JupyterHub.load_groups is used to create a user, see discussion in
+        # https://github.com/jupyterhub/jupyterhub/issues/4461.
+        if auth_model is None:
+            return True
 
         # allow admin users recognized via admin_users or update_auth_model
         if auth_model["admin"]:
diff --git a/oauthenticator/generic.py b/oauthenticator/generic.py
index 87bc2f82..ba38f2b4 100644
--- a/oauthenticator/generic.py
+++ b/oauthenticator/generic.py
@@ -145,6 +145,12 @@ async def check_allowed(self, username, auth_model):
         either part of `allowed_users` or `allowed_groups`, and not just those
         part of `allowed_users`.
         """
+        # Workaround situation when JupyterHub.load_roles or
+        # JupyterHub.load_groups is used to create a user, see discussion in
+        # https://github.com/jupyterhub/jupyterhub/issues/4461.
+        if auth_model is None:
+            return True
+
         # allow admin users recognized via admin_users or update_auth_model
         if auth_model["admin"]:
             return True
diff --git a/oauthenticator/github.py b/oauthenticator/github.py
index 32537426..6afbc640 100644
--- a/oauthenticator/github.py
+++ b/oauthenticator/github.py
@@ -136,6 +136,12 @@ async def check_allowed(self, username, auth_model):
         either part of `allowed_users` or `allowed_organizations`, and not just those
         part of `allowed_users`.
         """
+        # Workaround situation when JupyterHub.load_roles or
+        # JupyterHub.load_groups is used to create a user, see discussion in
+        # https://github.com/jupyterhub/jupyterhub/issues/4461.
+        if auth_model is None:
+            return True
+
         # allow admin users recognized via admin_users or update_auth_model
         if auth_model["admin"]:
             return True
diff --git a/oauthenticator/gitlab.py b/oauthenticator/gitlab.py
index 4ea5efc7..ec8bf5a1 100644
--- a/oauthenticator/gitlab.py
+++ b/oauthenticator/gitlab.py
@@ -128,6 +128,12 @@ async def check_allowed(self, username, auth_model):
         either part of `allowed_users`, `allowed_gitlab_groups`, or `allowed_project_ids`,
         and not just those part of `allowed_users`.
         """
+        # Workaround situation when JupyterHub.load_roles or
+        # JupyterHub.load_groups is used to create a user, see discussion in
+        # https://github.com/jupyterhub/jupyterhub/issues/4461.
+        if auth_model is None:
+            return True
+
         # allow admin users recognized via admin_users or update_auth_model
         if auth_model["admin"]:
             return True
diff --git a/oauthenticator/globus.py b/oauthenticator/globus.py
index 5cc9df39..a7cd0fa7 100644
--- a/oauthenticator/globus.py
+++ b/oauthenticator/globus.py
@@ -259,6 +259,12 @@ async def check_allowed(self, username, auth_model):
         either part of `allowed_users` or `allowed_globus_groups`, and not just those
         part of `allowed_users`.
         """
+        # Workaround situation when JupyterHub.load_roles or
+        # JupyterHub.load_groups is used to create a user, see discussion in
+        # https://github.com/jupyterhub/jupyterhub/issues/4461.
+        if auth_model is None:
+            return True
+
         # allow admin users recognized via admin_users or update_auth_model
         if auth_model["admin"]:
             return True
diff --git a/oauthenticator/google.py b/oauthenticator/google.py
index 18c9e247..b0d7ba54 100644
--- a/oauthenticator/google.py
+++ b/oauthenticator/google.py
@@ -139,6 +139,12 @@ async def check_allowed(self, username, auth_model):
         either part of `allowed_users` or `allowed_google_groups`, and not just those
         part of `allowed_users`.
         """
+        # Workaround situation when JupyterHub.load_roles or
+        # JupyterHub.load_groups is used to create a user, see discussion in
+        # https://github.com/jupyterhub/jupyterhub/issues/4461.
+        if auth_model is None:
+            return True
+
         # allow admin users recognized via admin_users or update_auth_model
         if auth_model["admin"]:
             return True
diff --git a/oauthenticator/oauth2.py b/oauthenticator/oauth2.py
index b2dbb61b..da3b899a 100644
--- a/oauthenticator/oauth2.py
+++ b/oauthenticator/oauth2.py
@@ -807,6 +807,12 @@ async def check_allowed(self, username, auth_model):
         Subclasses with authorization logic involving allowed groups should
         override this.
         """
+        # Workaround situation when JupyterHub.load_roles or
+        # JupyterHub.load_groups is used to create a user, see discussion in
+        # https://github.com/jupyterhub/jupyterhub/issues/4461.
+        if auth_model is None:
+            return True
+
         # authorize users to become admins by admin_users or logic in
         # update_auth_model
         if auth_model["admin"]:
diff --git a/oauthenticator/openshift.py b/oauthenticator/openshift.py
index bc99b9dd..248d542b 100644
--- a/oauthenticator/openshift.py
+++ b/oauthenticator/openshift.py
@@ -112,6 +112,12 @@ async def check_allowed(self, username, auth_model):
         either part of `allowed_users` or `allowed_groups`, and not just those
         part of `allowed_users`.
         """
+        # Workaround situation when JupyterHub.load_roles or
+        # JupyterHub.load_groups is used to create a user, see discussion in
+        # https://github.com/jupyterhub/jupyterhub/issues/4461.
+        if auth_model is None:
+            return True
+
         # allow admin users recognized via admin_users or update_auth_model
         if auth_model["admin"]:
             return True

From 66b35f89385f1646099fd1671ffe7cdb2a882a91 Mon Sep 17 00:00:00 2001
From: Erik Sundell <erik.i.sundell@gmail.com>
Date: Wed, 14 Jun 2023 15:00:03 +0200
Subject: [PATCH 50/57] docs: update changelog preliniary

---
 docs/source/reference/changelog.md | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/docs/source/reference/changelog.md b/docs/source/reference/changelog.md
index ed3ab57a..90232a88 100644
--- a/docs/source/reference/changelog.md
+++ b/docs/source/reference/changelog.md
@@ -6,6 +6,20 @@ command line for details.
 
 ## [Unreleased]
 
+### Breaking changes
+
+- [All] Users are now authorized based on *either* being part of
+  `Authenticator.admin_users`, `Authenticator.allowed_users`, an Authenticator
+  specific allowed team/group/organization, or declared in
+  `JupyterHub.load_roles` or `JupyterHub.load_groups`.
+- [Generic, Google] `GenericOAuthenticator.allowed_groups`,
+  `GenericOAuthenticator.allowed_groups`
+  `GoogleOAuthenticator.allowed_google_groups`, and
+  `GoogleOAuthenticatoradmin_google_groups` are now Set based configuration
+  instead of List based configuration. It is still possible to set these with
+  lists as as they are converted to sets automatically, but anyone reading and
+  adding entries must now use set logic and not list logic.
+
 (changelog:version-15)=
 
 ## 15.0

From e70dd313abcf8dad3220b34ce82737eb2d8f5995 Mon Sep 17 00:00:00 2001
From: "pre-commit-ci[bot]"
 <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Date: Wed, 14 Jun 2023 13:01:07 +0000
Subject: [PATCH 51/57] [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci
---
 docs/source/reference/changelog.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/source/reference/changelog.md b/docs/source/reference/changelog.md
index 90232a88..e12d98aa 100644
--- a/docs/source/reference/changelog.md
+++ b/docs/source/reference/changelog.md
@@ -8,7 +8,7 @@ command line for details.
 
 ### Breaking changes
 
-- [All] Users are now authorized based on *either* being part of
+- [All] Users are now authorized based on _either_ being part of
   `Authenticator.admin_users`, `Authenticator.allowed_users`, an Authenticator
   specific allowed team/group/organization, or declared in
   `JupyterHub.load_roles` or `JupyterHub.load_groups`.

From 26d4c75aa0ba4e1b10acf6ff43c11df2d5125634 Mon Sep 17 00:00:00 2001
From: Erik Sundell <erik.i.sundell@gmail.com>
Date: Fri, 23 Jun 2023 11:56:27 +0200
Subject: [PATCH 52/57] refactor: pure update of comments/docstrings/help and
 whitespace

The idea is to offload future commits by making these comment changes
separately.
---
 docs/source/reference/changelog.md |  2 +-
 oauthenticator/auth0.py            |  1 -
 oauthenticator/bitbucket.py        |  5 ++-
 oauthenticator/cilogon.py          |  5 ++-
 oauthenticator/generic.py          | 16 +++++-----
 oauthenticator/github.py           | 50 ++++++++++++++++++------------
 oauthenticator/gitlab.py           |  9 +++---
 oauthenticator/globus.py           | 22 +++++++------
 oauthenticator/google.py           | 30 ++++++++++++++----
 oauthenticator/oauth2.py           |  7 ++---
 oauthenticator/openshift.py        |  5 ++-
 11 files changed, 89 insertions(+), 63 deletions(-)

diff --git a/docs/source/reference/changelog.md b/docs/source/reference/changelog.md
index e12d98aa..ea9d28e1 100644
--- a/docs/source/reference/changelog.md
+++ b/docs/source/reference/changelog.md
@@ -15,7 +15,7 @@ command line for details.
 - [Generic, Google] `GenericOAuthenticator.allowed_groups`,
   `GenericOAuthenticator.allowed_groups`
   `GoogleOAuthenticator.allowed_google_groups`, and
-  `GoogleOAuthenticatoradmin_google_groups` are now Set based configuration
+  `GoogleOAuthenticator.admin_google_groups` are now Set based configuration
   instead of List based configuration. It is still possible to set these with
   lists as as they are converted to sets automatically, but anyone reading and
   adding entries must now use set logic and not list logic.
diff --git a/oauthenticator/auth0.py b/oauthenticator/auth0.py
index 25401779..909551e6 100644
--- a/oauthenticator/auth0.py
+++ b/oauthenticator/auth0.py
@@ -99,5 +99,4 @@ def _userdata_url_default(self):
 
 
 class LocalAuth0OAuthenticator(LocalAuthenticator, Auth0OAuthenticator):
-
     """A version that mixes in local system user creation"""
diff --git a/oauthenticator/bitbucket.py b/oauthenticator/bitbucket.py
index 4a537ed2..1a22f147 100644
--- a/oauthenticator/bitbucket.py
+++ b/oauthenticator/bitbucket.py
@@ -77,9 +77,8 @@ async def check_allowed(self, username, auth_model):
         either part of `allowed_users` or `allowed_teams`, and not just those
         part of `allowed_users`.
         """
-        # Workaround situation when JupyterHub.load_roles or
-        # JupyterHub.load_groups is used to create a user, see discussion in
-        # https://github.com/jupyterhub/jupyterhub/issues/4461.
+        # A workaround for JupyterHub<=4.0.1, described in
+        # https://github.com/jupyterhub/oauthenticator/issues/621
         if auth_model is None:
             return True
 
diff --git a/oauthenticator/cilogon.py b/oauthenticator/cilogon.py
index e8477b20..b1aee5ea 100644
--- a/oauthenticator/cilogon.py
+++ b/oauthenticator/cilogon.py
@@ -326,9 +326,8 @@ async def check_allowed(self, username, auth_model):
         to be authorized if either is configured, otherwise all users are
         authorized.
         """
-        # Workaround situation when JupyterHub.load_roles or
-        # JupyterHub.load_groups is used to create a user, see discussion in
-        # https://github.com/jupyterhub/jupyterhub/issues/4461.
+        # A workaround for JupyterHub<=4.0.1, described in
+        # https://github.com/jupyterhub/oauthenticator/issues/621
         if auth_model is None:
             return True
 
diff --git a/oauthenticator/generic.py b/oauthenticator/generic.py
index ba38f2b4..cae41193 100644
--- a/oauthenticator/generic.py
+++ b/oauthenticator/generic.py
@@ -121,16 +121,17 @@ def get_user_groups(self, user_info):
 
     async def update_auth_model(self, auth_model):
         """
-        Update admin status based on `admin_groups` if its configured.
+        Sets admin status to True or False if `admin_groups` is configured and
+        the user isn't part of `admin_users` or `admin_groups`. Note that
+        leaving it at None makes users able to retain an admin status while
+        setting it to False makes it be revoked.
         """
         if auth_model["admin"]:
+            # auth_model["admin"] being True means the user was in admin_users
             return auth_model
 
         if self.admin_groups:
-            # if admin_groups is configured and the user wasn't part of
-            # admin_users, we must set the admin status to True or False,
-            # otherwise removing a user from the admin_groups won't have an
-            # effect
+            # admin status should in this case be True or False, not None
             user_info = auth_model["auth_state"][self.user_auth_state_key]
             user_groups = self.get_user_groups(user_info)
             auth_model["admin"] = any(user_groups & self.admin_groups)
@@ -145,9 +146,8 @@ async def check_allowed(self, username, auth_model):
         either part of `allowed_users` or `allowed_groups`, and not just those
         part of `allowed_users`.
         """
-        # Workaround situation when JupyterHub.load_roles or
-        # JupyterHub.load_groups is used to create a user, see discussion in
-        # https://github.com/jupyterhub/jupyterhub/issues/4461.
+        # A workaround for JupyterHub<=4.0.1, described in
+        # https://github.com/jupyterhub/oauthenticator/issues/621
         if auth_model is None:
             return True
 
diff --git a/oauthenticator/github.py b/oauthenticator/github.py
index 6afbc640..df2f0dee 100644
--- a/oauthenticator/github.py
+++ b/oauthenticator/github.py
@@ -13,10 +13,6 @@
 
 
 class GitHubOAuthenticator(OAuthenticator):
-    # see github_scopes.md for details about scope config
-    # set scopes via config, e.g.
-    # c.GitHubOAuthenticator.scope = ['read:org']
-
     _deprecated_oauth_aliases = {
         "github_organization_whitelist": ("allowed_organizations", "0.12.0"),
         **OAuthenticator._deprecated_oauth_aliases,
@@ -106,7 +102,14 @@ def _github_client_secret_changed(self, name, old, new):
     )
 
     allowed_organizations = Set(
-        config=True, help="Automatically allow members of selected organizations"
+        help="""
+        Allow members of organizations or organizations' teams by specifying
+        strings like `org-a` and/or `org-b:team-1`.
+
+        Requires `read:org` to be set in `scope` to not just allow based on
+        public membership.
+        """,
+        config=True,
     )
 
     populate_teams_in_auth_state = Bool(
@@ -133,12 +136,11 @@ async def check_allowed(self, username, auth_model):
         Returns True for users allowed to be authorized.
 
         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`.
+        either part of `allowed_users` or `allowed_organizations`, and not just
+        those part of `allowed_users`.
         """
-        # Workaround situation when JupyterHub.load_roles or
-        # JupyterHub.load_groups is used to create a user, see discussion in
-        # https://github.com/jupyterhub/jupyterhub/issues/4461.
+        # A workaround for JupyterHub<=4.0.1, described in
+        # https://github.com/jupyterhub/oauthenticator/issues/621
         if auth_model is None:
             return True
 
@@ -175,6 +177,14 @@ async def check_allowed(self, username, auth_model):
         return True
 
     async def update_auth_model(self, auth_model):
+        """
+        Fetch and store `email` in auth state if the user's only was: private,
+        not part of the initial response, and we was granted a scope to fetch
+        the private email.
+
+        Also fetch and store `teams` in auth state if
+        `populate_teams_in_auth_state` is configured.
+        """
         # If a public email is not available, an extra API call has to be made
         # to a /user/emails using the access token to retrieve emails. The
         # scopes relevant for this are checked based on this documentation:
@@ -205,7 +215,8 @@ async def update_auth_model(self, auth_model):
 
         if self.populate_teams_in_auth_state:
             if "read:org" not in self.scope:
-                # This means the "read:org" scope was not set, and we can"t fetch teams
+                # This means the "read:org" scope was not set, and we can't
+                # fetch teams
                 self.log.error(
                     "read:org scope is required for populate_teams_in_auth_state functionality to work"
                 )
@@ -271,14 +282,16 @@ async def _paginated_fetch(self, api_url, access_token, token_type):
     async def _check_membership_allowed_organizations(
         self, org, username, access_token, token_type
     ):
-        headers = self.build_userdata_request_headers(access_token, token_type)
-        # Check membership of user `username` for organization `org` via api [check-membership](https://docs.github.com/en/rest/orgs/members#check-membership)
-        # With empty scope (even if authenticated by an org member), this
-        # will only await public org members.  You want 'read:org' in order
-        # to be able to iterate through all members. If you would only like to
-        # allow certain teams within an organisation, specify
-        # allowed_organisations = {org_name:team_name}
+        """
+        Checks if a user is part of an organization or organization's team via
+        GitHub's REST API. The `read:org` scope is required to not only check
+        for public org/team membership.
 
+        The `org` parameter accepts values like `org-a` or `org-b:team-1`,
+        and will adjust to use a the relevant REST API to check either org or
+        team membership.
+        """
+        headers = self.build_userdata_request_headers(access_token, token_type)
         check_membership_url = self._build_check_membership_url(org, username)
 
         self.log.debug(f"Checking GitHub organization membership: {username} in {org}?")
@@ -313,5 +326,4 @@ def _build_check_membership_url(self, org: str, username: str) -> str:
 
 
 class LocalGitHubOAuthenticator(LocalAuthenticator, GitHubOAuthenticator):
-
     """A version that mixes in local system user creation"""
diff --git a/oauthenticator/gitlab.py b/oauthenticator/gitlab.py
index ec8bf5a1..e2d36048 100644
--- a/oauthenticator/gitlab.py
+++ b/oauthenticator/gitlab.py
@@ -128,9 +128,8 @@ async def check_allowed(self, username, auth_model):
         either part of `allowed_users`, `allowed_gitlab_groups`, or `allowed_project_ids`,
         and not just those part of `allowed_users`.
         """
-        # Workaround situation when JupyterHub.load_roles or
-        # JupyterHub.load_groups is used to create a user, see discussion in
-        # https://github.com/jupyterhub/jupyterhub/issues/4461.
+        # A workaround for JupyterHub<=4.0.1, described in
+        # https://github.com/jupyterhub/oauthenticator/issues/621
         if auth_model is None:
             return True
 
@@ -138,8 +137,8 @@ async def check_allowed(self, username, auth_model):
         if auth_model["admin"]:
             return True
 
-        # if allowed_users or allowed_gitlab_groups or allowed_project_ids is configured,
-        # we deny users not part of either
+        # if allowed_users, allowed_gitlab_groups, or allowed_project_ids is
+        # configured, we deny users not part of either
         if self.allowed_users or self.allowed_gitlab_groups or self.allowed_project_ids:
             if username in self.allowed_users:
                 return True
diff --git a/oauthenticator/globus.py b/oauthenticator/globus.py
index a7cd0fa7..6f1e45b6 100644
--- a/oauthenticator/globus.py
+++ b/oauthenticator/globus.py
@@ -259,9 +259,8 @@ async def check_allowed(self, username, auth_model):
         either part of `allowed_users` or `allowed_globus_groups`, and not just those
         part of `allowed_users`.
         """
-        # Workaround situation when JupyterHub.load_roles or
-        # JupyterHub.load_groups is used to create a user, see discussion in
-        # https://github.com/jupyterhub/jupyterhub/issues/4461.
+        # A workaround for JupyterHub<=4.0.1, described in
+        # https://github.com/jupyterhub/oauthenticator/issues/621
         if auth_model is None:
             return True
 
@@ -284,11 +283,11 @@ async def check_allowed(self, username, auth_model):
                     "Please link your account at app.globus.org/account.",
                 )
 
-        # if allowed_users or allowed_globus_groups is configured, we deny users not part of either
+        # if allowed_users or allowed_globus_groups is configured, we deny users
+        # not part of either
         if self.allowed_users or self.allowed_globus_groups:
             if username in self.allowed_users:
                 return True
-
             if self.allowed_globus_groups:
                 tokens = self.get_globus_tokens(
                     auth_model["auth_state"]["token_response"]
@@ -299,7 +298,6 @@ async def check_allowed(self, username, auth_model):
                     user_group_ids, self.allowed_globus_groups
                 ):
                     return True
-
                 self.log.warning(f"{username} not in an allowed Globus Group")
 
             return False
@@ -308,9 +306,15 @@ async def check_allowed(self, username, auth_model):
         return True
 
     async def update_auth_model(self, auth_model):
-        # If users is already marked as an admin, it means that
-        # they are present in the `admin_users`` list
-        # no need to check groups membership
+        """
+        Fetch and store `globus_groups` in auth state if `allowed_globus_groups`
+        or `admin_globus_groups` is configured.
+
+        Sets admin status to True or False if `admin_globus_groups` is
+        configured and the user isn't part of `admin_users` or
+        `admin_globus_groups`. Note that leaving it at None makes users able to
+        retain an admin status while setting it to False makes it be revoked.
+        """
         if auth_model["admin"] is True:
             return auth_model
 
diff --git a/oauthenticator/google.py b/oauthenticator/google.py
index b0d7ba54..595f386b 100644
--- a/oauthenticator/google.py
+++ b/oauthenticator/google.py
@@ -118,7 +118,14 @@ def _cast_hosted_domain(self, proposal):
 
     async def update_auth_model(self, auth_model):
         """
-        Updates the `auth_model` dict with info about the admin status.
+        Fetch and store `google_groups` in auth state if `allowed_google_groups`
+        or `admin_google_groups` is configured. Also declare the user an admin
+        if part of `admin_google_groups`.
+
+        Sets admin status to True or False if `admin_google_groups` is
+        configured and the user isn't part of `admin_users` or
+        `admin_google_groups`. Note that leaving it at None makes users able to
+        retain an admin status while setting it to False makes it be revoked.
         """
         user_info = auth_model["auth_state"][self.user_auth_state_key]
         user_email = user_info["email"]
@@ -139,9 +146,8 @@ async def check_allowed(self, username, auth_model):
         either part of `allowed_users` or `allowed_google_groups`, and not just those
         part of `allowed_users`.
         """
-        # Workaround situation when JupyterHub.load_roles or
-        # JupyterHub.load_groups is used to create a user, see discussion in
-        # https://github.com/jupyterhub/jupyterhub/issues/4461.
+        # A workaround for JupyterHub<=4.0.1, described in
+        # https://github.com/jupyterhub/oauthenticator/issues/621
         if auth_model is None:
             return True
 
@@ -158,6 +164,13 @@ async def check_allowed(self, username, auth_model):
             self.log.warning(f"Google OAuth unverified email attempt: {user_email}")
             raise HTTPError(403, f"Google email {user_email} not verified")
 
+        # NOTE: If hosted_domain is configured as ["a.com", "b.com"], and
+        #       allowed_google_groups is declared as {"a.com": {"a-group"}}, a
+        #       "b.com" user won't be authorized unless allowed in another way.
+        #
+        #       This means that its not possible to allow all users of a given
+        #       domain if one wants to restrict another.
+        #
         if self.hosted_domain:
             if user_domain not in self.hosted_domain:
                 self.log.error(
@@ -167,7 +180,8 @@ async def check_allowed(self, username, auth_model):
                     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 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
@@ -184,7 +198,6 @@ async def check_allowed(self, username, auth_model):
                 #
                 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
@@ -244,6 +257,11 @@ def _google_groups_for_user(self, user_email, user_email_domain, http=None):
         """
         Return a set with the google groups a given user is a member of
         """
+        # FIXME: When this function is used and waiting for web request
+        #        responses, JupyterHub gets blocked from doing other things.
+        #        Ideally the web requests should be made using an async client
+        #        that can be awaited while JupyterHub handles other things.
+        #
         credentials = self._service_client_credentials(
             scopes=[f"{self.google_api_url}/auth/admin.directory.group.readonly"],
             user_email_domain=user_email_domain,
diff --git a/oauthenticator/oauth2.py b/oauthenticator/oauth2.py
index da3b899a..564cb2eb 100644
--- a/oauthenticator/oauth2.py
+++ b/oauthenticator/oauth2.py
@@ -785,8 +785,6 @@ async def authenticate(self, handler, data=None, **kwargs):
         # build the auth model to be read if authentication goes right
         auth_model = {
             "name": username,
-            # FIXME: Think about is_admin override, should we call is_admin from
-            #        here or possibly after update_auth_model?
             "admin": True if username in self.admin_users else None,
             "auth_state": self.build_auth_state_dict(token_info, user_info),
         }
@@ -807,9 +805,8 @@ async def check_allowed(self, username, auth_model):
         Subclasses with authorization logic involving allowed groups should
         override this.
         """
-        # Workaround situation when JupyterHub.load_roles or
-        # JupyterHub.load_groups is used to create a user, see discussion in
-        # https://github.com/jupyterhub/jupyterhub/issues/4461.
+        # A workaround for JupyterHub<=4.0.1, described in
+        # https://github.com/jupyterhub/oauthenticator/issues/621
         if auth_model is None:
             return True
 
diff --git a/oauthenticator/openshift.py b/oauthenticator/openshift.py
index 248d542b..a1bcc022 100644
--- a/oauthenticator/openshift.py
+++ b/oauthenticator/openshift.py
@@ -112,9 +112,8 @@ async def check_allowed(self, username, auth_model):
         either part of `allowed_users` or `allowed_groups`, and not just those
         part of `allowed_users`.
         """
-        # Workaround situation when JupyterHub.load_roles or
-        # JupyterHub.load_groups is used to create a user, see discussion in
-        # https://github.com/jupyterhub/jupyterhub/issues/4461.
+        # A workaround for JupyterHub<=4.0.1, described in
+        # https://github.com/jupyterhub/oauthenticator/issues/621
         if auth_model is None:
             return True
 

From 8556e2770bfb49ee53cffd82ac0ed3612e8970e2 Mon Sep 17 00:00:00 2001
From: Erik Sundell <erik.i.sundell@gmail.com>
Date: Fri, 23 Jun 2023 13:13:44 +0200
Subject: [PATCH 53/57] refactor, openshift: plain refactor for readability

---
 oauthenticator/openshift.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/oauthenticator/openshift.py b/oauthenticator/openshift.py
index a1bcc022..c876a7cf 100644
--- a/oauthenticator/openshift.py
+++ b/oauthenticator/openshift.py
@@ -124,10 +124,10 @@ async def check_allowed(self, username, auth_model):
         # if allowed_users or allowed_groups is configured, we deny users not
         # part of either
         if self.allowed_users or self.allowed_groups:
-            user_info = auth_model["auth_state"][self.user_auth_state_key]
-            user_groups = set(user_info["groups"])
             if username in self.allowed_users:
                 return True
+            user_info = auth_model["auth_state"][self.user_auth_state_key]
+            user_groups = set(user_info["groups"])
             if any(user_groups & self.allowed_groups):
                 return True
             return False

From 3c621016e9ba0718084161a0df1823c7cbaaed91 Mon Sep 17 00:00:00 2001
From: Erik Sundell <erik.i.sundell@gmail.com>
Date: Fri, 23 Jun 2023 13:12:11 +0200
Subject: [PATCH 54/57] refactor, github: plain refactor for readability

---
 oauthenticator/github.py            | 63 +++++++++++++----------------
 oauthenticator/tests/test_github.py | 19 +--------
 2 files changed, 30 insertions(+), 52 deletions(-)

diff --git a/oauthenticator/github.py b/oauthenticator/github.py
index df2f0dee..47ba4769 100644
--- a/oauthenticator/github.py
+++ b/oauthenticator/github.py
@@ -165,11 +165,9 @@ async def check_allowed(self, username, auth_model):
                     )
                     if user_in_org:
                         return True
-                else:
-                    # User not found in member list for any organi`ation
-                    self.log.warning(
-                        f"User {username} is not in allowed org list",
-                    )
+                self.log.warning(
+                    f"User {username} is not part of allowed_organizations"
+                )
 
             return False
 
@@ -185,6 +183,8 @@ async def update_auth_model(self, auth_model):
         Also fetch and store `teams` in auth state if
         `populate_teams_in_auth_state` is configured.
         """
+        user_info = auth_model["auth_state"][self.user_auth_state_key]
+
         # If a public email is not available, an extra API call has to be made
         # to a /user/emails using the access token to retrieve emails. The
         # scopes relevant for this are checked based on this documentation:
@@ -194,15 +194,12 @@ async def update_auth_model(self, auth_model):
         # Note that the read:user scope does not imply the user:emails scope!
         access_token = auth_model["auth_state"]["token_response"]["access_token"]
         token_type = auth_model["auth_state"]["token_response"]["token_type"]
-        granted_scopes = []
-        if auth_model["auth_state"]["scope"]:
-            granted_scopes = auth_model["auth_state"]["scope"]
-
-        if not auth_model["auth_state"]["github_user"]["email"] and (
+        granted_scopes = auth_model["auth_state"].get("scope", [])
+        if not user_info["email"] and (
             "user" in granted_scopes or "user:email" in granted_scopes
         ):
             resp_json = await self.httpfetch(
-                self.github_api + "/user/emails",
+                f"{self.github_api}/user/emails",
                 "fetching user emails",
                 method="GET",
                 headers=self.build_userdata_request_headers(access_token, token_type),
@@ -210,7 +207,7 @@ async def update_auth_model(self, auth_model):
             )
             for val in resp_json:
                 if val["primary"]:
-                    auth_model["auth_state"]["github_user"]["email"] = val["email"]
+                    user_info["email"] = val["email"]
                     break
 
         if self.populate_teams_in_auth_state:
@@ -221,15 +218,10 @@ async def update_auth_model(self, auth_model):
                     "read:org scope is required for populate_teams_in_auth_state functionality to work"
                 )
             else:
-                # Number of teams to request per page
-                per_page = 100
-
-                #  https://docs.github.com/en/rest/reference/teams#list-teams-for-the-authenticated-user
-                url = self.github_api + f"/user/teams?per_page={per_page}"
-
-                auth_model["auth_state"]["teams"] = await self._paginated_fetch(
-                    url, access_token, token_type
-                )
+                # https://docs.github.com/en/rest/teams/teams?apiVersion=2022-11-28#list-teams-for-the-authenticated-user
+                url = f"{self.github_api}/user/teams?per_page=100"
+                user_teams = await self._paginated_fetch(url, access_token, token_type)
+                auth_model["auth_state"]["teams"] = user_teams
 
         return auth_model
 
@@ -280,23 +272,33 @@ async def _paginated_fetch(self, api_url, access_token, token_type):
         return content
 
     async def _check_membership_allowed_organizations(
-        self, org, username, access_token, token_type
+        self, org_team, username, access_token, token_type
     ):
         """
         Checks if a user is part of an organization or organization's team via
         GitHub's REST API. The `read:org` scope is required to not only check
         for public org/team membership.
 
-        The `org` parameter accepts values like `org-a` or `org-b:team-1`,
+        The `org_team` parameter accepts values like `org-a` or `org-b:team-1`,
         and will adjust to use a the relevant REST API to check either org or
         team membership.
         """
         headers = self.build_userdata_request_headers(access_token, token_type)
-        check_membership_url = self._build_check_membership_url(org, username)
+
+        if ":" in org_team:
+            # check if user is part of an organization's team
+            # https://docs.github.com/en/rest/teams/members?apiVersion=2022-11-28#get-team-member-legacy
+            org, team = org_team.split(":")
+            api_url = f"{self.github_api}/orgs/{org}/teams/{team}/members/{username}"
+        else:
+            # check if user is part of an organization
+            # https://docs.github.com/en/rest/orgs/members?apiVersion=2022-11-28#check-organization-membership-for-a-user
+            org = org_team
+            api_url = f"{self.github_api}/orgs/{org}/members/{username}"
 
         self.log.debug(f"Checking GitHub organization membership: {username} in {org}?")
         resp = await self.httpfetch(
-            check_membership_url,
+            api_url,
             parse_json=False,
             raise_error=False,
             method="GET",
@@ -304,7 +306,7 @@ async def _check_membership_allowed_organizations(
             validate_cert=self.validate_server_cert,
         )
         if resp.code == 204:
-            self.log.info(f"Allowing {username} as member of {org}")
+            self.log.debug(f"Allowing {username} as member of {org_team}")
             return True
         else:
             try:
@@ -313,17 +315,10 @@ async def _check_membership_allowed_organizations(
             except ValueError:
                 message = ''
             self.log.debug(
-                f"{username} does not appear to be a member of {org} (status={resp.code}): {message}",
+                f"{username} does not appear to be a member of {org_team} (status={resp.code}): {message}",
             )
         return False
 
-    def _build_check_membership_url(self, org: str, username: str) -> str:
-        if ":" in org:
-            org, team = org.split(":")
-            return f"{self.github_api}/orgs/{org}/teams/{team}/members/{username}"
-        else:
-            return f"{self.github_api}/orgs/{org}/members/{username}"
-
 
 class LocalGitHubOAuthenticator(LocalAuthenticator, GitHubOAuthenticator):
     """A version that mixes in local system user creation"""
diff --git a/oauthenticator/tests/test_github.py b/oauthenticator/tests/test_github.py
index 03a591d1..8fd2bbc7 100644
--- a/oauthenticator/tests/test_github.py
+++ b/oauthenticator/tests/test_github.py
@@ -5,7 +5,7 @@
 from io import BytesIO
 from urllib.parse import parse_qs, urlparse
 
-from pytest import fixture, mark
+from pytest import fixture
 from tornado.httpclient import HTTPResponse
 from tornado.httputil import HTTPHeaders
 from traitlets.config import Config
@@ -193,23 +193,6 @@ def team_membership(request):
         client_hosts.pop()
 
 
-@mark.parametrize(
-    "org, username, expected",
-    [
-        ("blue", "texas", "https://api.github.com/orgs/blue/members/texas"),
-        (
-            "blue:alpha",
-            "tucker",
-            "https://api.github.com/orgs/blue/teams/alpha/members/tucker",
-        ),
-        ("red", "grif", "https://api.github.com/orgs/red/members/grif"),
-    ],
-)
-async def test_build_check_membership_url(org, username, expected):
-    output = GitHubOAuthenticator()._build_check_membership_url(org, username)
-    assert output == expected
-
-
 async def test_deprecated_config(caplog):
     cfg = Config()
     cfg.GitHubOAuthenticator.github_organization_whitelist = ["jupy"]

From fd06b4777cf7f824c97990131fd7a90695c845c8 Mon Sep 17 00:00:00 2001
From: Erik Sundell <erik.i.sundell@gmail.com>
Date: Fri, 23 Jun 2023 12:11:09 +0200
Subject: [PATCH 55/57] refactor, google: rename _google_groups_for_user to
 _fetch_user_groups

---
 oauthenticator/google.py            | 14 ++++----
 oauthenticator/tests/test_google.py | 50 ++++++++++++++---------------
 2 files changed, 32 insertions(+), 32 deletions(-)

diff --git a/oauthenticator/google.py b/oauthenticator/google.py
index 595f386b..0c669919 100644
--- a/oauthenticator/google.py
+++ b/oauthenticator/google.py
@@ -130,7 +130,7 @@ async def update_auth_model(self, auth_model):
         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))
+        user_groups = self._fetch_user_groups(user_email, user_domain)
         admin_groups = self.admin_google_groups.get(user_domain, set())
 
         if any(user_groups & admin_groups):
@@ -253,7 +253,7 @@ def _service_client(self, service_name, service_version, credentials, http=None)
             http=http,
         )
 
-    def _google_groups_for_user(self, user_email, user_email_domain, http=None):
+    def _fetch_user_groups(self, user_email, user_email_domain, http=None):
         """
         Return a set with the google groups a given user is a member of
         """
@@ -273,12 +273,12 @@ def _google_groups_for_user(self, user_email, user_email_domain, http=None):
             http=http,
         )
 
-        results = service.groups().list(userKey=user_email).execute()
-        results = {
-            g['email'].split('@')[0] for g in results.get('groups', [{'email': None}])
+        resp = service.groups().list(userKey=user_email).execute()
+        user_groups = {
+            g['email'].split('@')[0] for g in resp.get('groups', [{'email': None}])
         }
-        self.log.debug(f"user_email {user_email} is a member of {results}")
-        return results
+        self.log.debug(f"user_email {user_email} is a member of {user_groups}")
+        return user_groups
 
 
 class LocalGoogleOAuthenticator(LocalAuthenticator, GoogleOAuthenticator):
diff --git a/oauthenticator/tests/test_google.py b/oauthenticator/tests/test_google.py
index 1cb3ef61..0eb34c8c 100644
--- a/oauthenticator/tests/test_google.py
+++ b/oauthenticator/tests/test_google.py
@@ -37,8 +37,8 @@ async def test_google(google_client):
     handler = google_client.handler_for_user(user_model('fake@email.com'))
     with mock.patch.object(
         authenticator,
-        '_google_groups_for_user',
-        lambda *args: ['anotherone', 'fakeadmingroup'],
+        '_fetch_user_groups',
+        lambda *args: {'anotherone', 'fakeadmingroup'},
     ):
         user_info = await authenticator.get_authenticated_user(handler, None)
         assert sorted(user_info) == ['admin', 'auth_state', 'name']
@@ -56,8 +56,8 @@ async def test_google_username_claim(google_client):
     handler = google_client.handler_for_user(user_model('fake@email.com'))
     with mock.patch.object(
         authenticator,
-        '_google_groups_for_user',
-        lambda *args: ['anotherone', 'fakeadmingroup'],
+        '_fetch_user_groups',
+        lambda *args: {'anotherone', 'fakeadmingroup'},
     ):
         user_info = await authenticator.get_authenticated_user(handler, None)
         assert sorted(user_info) == ['admin', 'auth_state', 'name']
@@ -70,8 +70,8 @@ async def test_hosted_domain(google_client):
     handler = google_client.handler_for_user(user_model('fake@email.com'))
     with mock.patch.object(
         authenticator,
-        '_google_groups_for_user',
-        lambda *args: ['anotherone', 'fakeadmingroup'],
+        '_fetch_user_groups',
+        lambda *args: {'anotherone', 'fakeadmingroup'},
     ):
         user_info = await authenticator.get_authenticated_user(handler, None)
         name = user_info['name']
@@ -88,8 +88,8 @@ async def test_multiple_hosted_domain(google_client):
     handler = google_client.handler_for_user(user_model('fake@email.com'))
     with mock.patch.object(
         authenticator,
-        '_google_groups_for_user',
-        lambda *args: ['anotherone', 'fakeadmingroup'],
+        '_fetch_user_groups',
+        lambda *args: {'anotherone', 'fakeadmingroup'},
     ):
         user_info = await authenticator.get_authenticated_user(handler, None)
         name = user_info['name']
@@ -115,8 +115,8 @@ async def test_admin_google_groups(google_client):
     handler = google_client.handler_for_user(user_model('fakeadmin@email.com'))
     with mock.patch.object(
         authenticator,
-        '_google_groups_for_user',
-        lambda *args: ['anotherone', 'fakeadmingroup'],
+        '_fetch_user_groups',
+        lambda *args: {'anotherone', 'fakeadmingroup'},
     ):
         admin_user_info = await authenticator.get_authenticated_user(handler, None)
         # Make sure the user authenticated successfully
@@ -126,8 +126,8 @@ async def test_admin_google_groups(google_client):
     handler = google_client.handler_for_user(user_model('fakealloweduser@email.com'))
     with mock.patch.object(
         authenticator,
-        '_google_groups_for_user',
-        lambda *args: ['anotherone', 'fakegroup'],
+        '_fetch_user_groups',
+        lambda *args: {'anotherone', 'fakegroup'},
     ):
         allowed_user_info = await authenticator.get_authenticated_user(handler, None)
         allowed_user_groups = allowed_user_info['auth_state']['google_user'][
@@ -139,8 +139,8 @@ async def test_admin_google_groups(google_client):
     handler = google_client.handler_for_user(user_model('fakenonalloweduser@email.com'))
     with mock.patch.object(
         authenticator,
-        '_google_groups_for_user',
-        lambda *args: ['anotherone', 'fakenonallowedgroup'],
+        '_fetch_user_groups',
+        lambda *args: {'anotherone', 'fakenonallowedgroup'},
     ):
         allowed_user_groups = await authenticator.get_authenticated_user(handler, None)
         assert allowed_user_groups is None
@@ -155,8 +155,8 @@ async def test_admin_user_but_no_admin_google_groups(google_client):
     handler = google_client.handler_for_user(user_model('fakeadmin@email.com'))
     with mock.patch.object(
         authenticator,
-        '_google_groups_for_user',
-        lambda *args: ['anotherone', 'fakegroup'],
+        '_fetch_user_groups',
+        lambda *args: {'anotherone', 'fakegroup'},
     ):
         admin_user_info = await authenticator.get_authenticated_user(handler, data=None)
         # Make sure the user authenticated successfully
@@ -173,16 +173,16 @@ async def test_allowed_google_groups(google_client):
     handler = google_client.handler_for_user(user_model('fakeadmin@email.com'))
     with mock.patch.object(
         authenticator,
-        '_google_groups_for_user',
-        lambda *args: ['anotherone', 'fakeadmingroup'],
+        '_fetch_user_groups',
+        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('fakealloweduser@email.com'))
     with mock.patch.object(
         authenticator,
-        '_google_groups_for_user',
-        lambda *args: ['anotherone', 'fakegroup'],
+        '_fetch_user_groups',
+        lambda *args: {'anotherone', 'fakegroup'},
     ):
         allowed_user_info = await authenticator.get_authenticated_user(handler, None)
         allowed_user_groups = allowed_user_info['auth_state']['google_user'][
@@ -194,14 +194,14 @@ async def test_allowed_google_groups(google_client):
     handler = google_client.handler_for_user(user_model('fakenonalloweduser@email.com'))
     with mock.patch.object(
         authenticator,
-        '_google_groups_for_user',
-        lambda *args: ['anotherone', 'fakenonallowedgroup'],
+        '_fetch_user_groups',
+        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', lambda *args: ['fakegroup']
+        authenticator, '_fetch_user_groups', lambda *args: {'fakegroup'}
     ):
         allowed_user_groups = await authenticator.get_authenticated_user(handler, None)
         assert allowed_user_groups is None
@@ -215,8 +215,8 @@ async def test_admin_only_google_groups(google_client):
     handler = google_client.handler_for_user(user_model('fakeadmin@email.com'))
     with mock.patch.object(
         authenticator,
-        '_google_groups_for_user',
-        lambda *args: ['anotherone', 'fakeadmingroup'],
+        '_fetch_user_groups',
+        lambda *args: {'anotherone', 'fakeadmingroup'},
     ):
         admin_user_info = await authenticator.get_authenticated_user(handler, None)
         admin_user = admin_user_info['admin']

From 8acf905395a0b60df8c0ff8e7b154daf38231465 Mon Sep 17 00:00:00 2001
From: Erik Sundell <erik.i.sundell@gmail.com>
Date: Fri, 23 Jun 2023 13:02:02 +0200
Subject: [PATCH 56/57] google, breaking fix: revoke admin status, groups are
 now sets in auth state

---
 docs/source/reference/changelog.md  |  4 +++
 oauthenticator/google.py            | 40 ++++++++++++++---------------
 oauthenticator/tests/test_google.py |  2 +-
 3 files changed, 25 insertions(+), 21 deletions(-)

diff --git a/docs/source/reference/changelog.md b/docs/source/reference/changelog.md
index ea9d28e1..d4e7da3b 100644
--- a/docs/source/reference/changelog.md
+++ b/docs/source/reference/changelog.md
@@ -12,6 +12,9 @@ command line for details.
   `Authenticator.admin_users`, `Authenticator.allowed_users`, an Authenticator
   specific allowed team/group/organization, or declared in
   `JupyterHub.load_roles` or `JupyterHub.load_groups`.
+- [Google] If `GoogleOAuthenticator.admin_google_groups` is configured, users
+  logging in not explicitly there or in `Authenticator.admin_users` will get
+  their admin status revoked.
 - [Generic, Google] `GenericOAuthenticator.allowed_groups`,
   `GenericOAuthenticator.allowed_groups`
   `GoogleOAuthenticator.allowed_google_groups`, and
@@ -19,6 +22,7 @@ command line for details.
   instead of List based configuration. It is still possible to set these with
   lists as as they are converted to sets automatically, but anyone reading and
   adding entries must now use set logic and not list logic.
+- [Google] Authentication state's `google_groups` is now a set, not a list.
 
 (changelog:version-15)=
 
diff --git a/oauthenticator/google.py b/oauthenticator/google.py
index 0c669919..f09d9634 100644
--- a/oauthenticator/google.py
+++ b/oauthenticator/google.py
@@ -129,12 +129,23 @@ async def update_auth_model(self, auth_model):
         """
         user_info = auth_model["auth_state"][self.user_auth_state_key]
         user_email = user_info["email"]
-        user_domain = user_email.split("@")[1]
-        user_groups = self._fetch_user_groups(user_email, user_domain)
-        admin_groups = self.admin_google_groups.get(user_domain, set())
+        user_domain = user_info["domain"] = user_email.split("@")[1]
 
-        if any(user_groups & admin_groups):
-            auth_model["admin"] = True
+        user_groups = set()
+        if self.allowed_google_groups or self.admin_google_groups:
+            user_groups = user_info["google_groups"] = self._fetch_user_groups(
+                user_email, user_domain
+            )
+        user_info["google_groups"] = user_groups
+
+        if auth_model["admin"]:
+            # auth_model["admin"] being True means the user was in admin_users
+            return auth_model
+
+        if self.admin_google_groups:
+            # admin status should in this case be True or False, not None
+            admin_groups = self.admin_google_groups.get(user_domain, set())
+            auth_model["admin"] = any(user_groups & admin_groups)
 
         return auth_model
 
@@ -157,8 +168,8 @@ async def check_allowed(self, username, auth_model):
 
         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))
+        user_domain = user_info["domain"]
+        user_groups = user_info["google_groups"]
 
         if not user_info["verified_email"]:
             self.log.warning(f"Google OAuth unverified email attempt: {user_email}")
@@ -173,7 +184,7 @@ async def check_allowed(self, username, auth_model):
         #
         if self.hosted_domain:
             if user_domain not in self.hosted_domain:
-                self.log.error(
+                self.log.warning(
                     f"Google OAuth unauthorized domain attempt: {user_email}"
                 )
                 raise HTTPError(
@@ -185,18 +196,7 @@ async def check_allowed(self, username, auth_model):
         if self.allowed_users or self.allowed_google_groups:
             if username in self.allowed_users:
                 return True
-
-            # 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)
+            if self.allowed_google_groups:
                 allowed_groups = self.allowed_google_groups.get(user_domain, set())
                 if any(user_groups & allowed_groups):
                     return True
diff --git a/oauthenticator/tests/test_google.py b/oauthenticator/tests/test_google.py
index 0eb34c8c..6aa722c7 100644
--- a/oauthenticator/tests/test_google.py
+++ b/oauthenticator/tests/test_google.py
@@ -135,7 +135,7 @@ async def test_admin_google_groups(google_client):
         ]
         admin_user = allowed_user_info['admin']
         assert 'fakegroup' in allowed_user_groups
-        assert not admin_user
+        assert admin_user is False
     handler = google_client.handler_for_user(user_model('fakenonalloweduser@email.com'))
     with mock.patch.object(
         authenticator,

From 8b138c73a2d52f56d2b32891168f8511dd811c85 Mon Sep 17 00:00:00 2001
From: Erik Sundell <erik.i.sundell@gmail.com>
Date: Fri, 23 Jun 2023 13:14:46 +0200
Subject: [PATCH 57/57] globus, maint: persist globus groups in auth state, and
 misc refactor

---
 oauthenticator/globus.py            | 47 +++++++++++------------------
 oauthenticator/tests/test_globus.py | 12 ++++----
 2 files changed, 24 insertions(+), 35 deletions(-)

diff --git a/oauthenticator/globus.py b/oauthenticator/globus.py
index 6f1e45b6..4e225685 100644
--- a/oauthenticator/globus.py
+++ b/oauthenticator/globus.py
@@ -165,10 +165,6 @@ def _revoke_tokens_on_logout_default(self):
         their UUIDs. Setting this will add the Globus Groups scope."""
     ).tag(config=True)
 
-    @staticmethod
-    def check_user_in_groups(member_groups, allowed_groups):
-        return bool(set(member_groups) & set(allowed_groups))
-
     async def pre_spawn_start(self, user, spawner):
         """Add tokens to the spawner whenever the spawner starts a notebook.
         This will allow users to create a transfer client:
@@ -230,11 +226,8 @@ def build_auth_state_dict(self, token_info, user_info):
             self.user_auth_state_key: user_info,
         }
 
-    # FIXME: Should we persist info about user groups in auth model
-    #        to be consistent with what's happening in bitbucket.py
-    #        where the `auth_model` is updated with `user_teams`.
-    async def get_users_groups_ids(self, tokens):
-        user_group_ids = set()
+    async def _fetch_users_groups(self, tokens):
+        user_groups = set()
         # Get Groups access token, may not be in dict headed to auth state
         for token_dict in tokens:
             if token_dict['resource_server'] == 'groups.api.globus.org':
@@ -247,9 +240,9 @@ async def get_users_groups_ids(self, tokens):
         )
         # Build set of Group IDs
         for group in groups_resp:
-            user_group_ids.add(group['id'])
+            user_groups.add(group['id'])
 
-        return user_group_ids
+        return user_groups
 
     async def check_allowed(self, username, auth_model):
         """
@@ -274,7 +267,7 @@ async def check_allowed(self, username, auth_model):
             user_info = auth_model["auth_state"][self.user_auth_state_key]
             domain = user_info.get(self.username_claim).split('@', 1)[-1]
             if domain != self.identity_provider:
-                self.log.error(
+                self.log.warning(
                     f"Trying to login from an identity provider that was not allowed {domain}",
                 )
                 raise HTTPError(
@@ -289,14 +282,8 @@ async def check_allowed(self, username, auth_model):
             if username in self.allowed_users:
                 return True
             if self.allowed_globus_groups:
-                tokens = self.get_globus_tokens(
-                    auth_model["auth_state"]["token_response"]
-                )
-                user_group_ids = await self.get_users_groups_ids(tokens)
-
-                if self.check_user_in_groups(
-                    user_group_ids, self.allowed_globus_groups
-                ):
+                user_groups = auth_model["auth_state"]["globus_groups"]
+                if any(user_groups & self.allowed_globus_groups):
                     return True
                 self.log.warning(f"{username} not in an allowed Globus Group")
 
@@ -315,18 +302,20 @@ async def update_auth_model(self, auth_model):
         `admin_globus_groups`. Note that leaving it at None makes users able to
         retain an admin status while setting it to False makes it be revoked.
         """
-        if auth_model["admin"] is True:
+        user_groups = set()
+        if self.allowed_globus_groups or self.admin_globus_groups:
+            tokens = self.get_globus_tokens(auth_model["auth_state"]["token_response"])
+            user_groups = await self._fetch_users_groups(tokens)
+        auth_model["auth_state"]["globus_groups"] = user_groups
+
+        if auth_model["admin"]:
+            # auth_model["admin"] being True means the user was in admin_users
             return auth_model
 
         if self.admin_globus_groups:
-            tokens = self.get_globus_tokens(auth_model["auth_state"]["token_response"])
-            # If any of these configurations are set, user must be in the allowed or admin Globus Group
-            user_group_ids = await self.get_users_groups_ids(tokens)
-            # Admin users are being managed via Globus Groups
-            # Default to False
-            auth_model["admin"] = False
-            if self.check_user_in_groups(user_group_ids, self.admin_globus_groups):
-                auth_model["admin"] = True
+            # admin status should in this case be True or False, not None
+            auth_model["admin"] = any(user_groups & self.admin_globus_groups)
+
         return auth_model
 
     def user_info_to_username(self, user_info):
diff --git a/oauthenticator/tests/test_globus.py b/oauthenticator/tests/test_globus.py
index 6963fe63..8560e0b0 100644
--- a/oauthenticator/tests/test_globus.py
+++ b/oauthenticator/tests/test_globus.py
@@ -395,7 +395,7 @@ async def test_logout_revokes_tokens(globus_client, monkeypatch, mock_globus_use
 
 async def test_group_scope_added(globus_client):
     authenticator = GlobusOAuthenticator()
-    authenticator.allowed_globus_groups = set({'21c6bc5d-fc12-4f60-b999-76766cd596c2'})
+    authenticator.allowed_globus_groups = {'21c6bc5d-fc12-4f60-b999-76766cd596c2'}
     assert authenticator.scope == [
         'openid',
         'profile',
@@ -406,7 +406,7 @@ async def test_group_scope_added(globus_client):
 
 async def test_user_in_allowed_group(globus_client):
     authenticator = GlobusOAuthenticator()
-    authenticator.allowed_globus_groups = set({'21c6bc5d-fc12-4f60-b999-76766cd596c2'})
+    authenticator.allowed_globus_groups = {'21c6bc5d-fc12-4f60-b999-76766cd596c2'}
     handler = globus_client.handler_for_user(user_model('wash@uflightacademy.edu'))
     auth_model = await authenticator.get_authenticated_user(handler, None)
     assert auth_model['name'] == 'wash'
@@ -414,7 +414,7 @@ async def test_user_in_allowed_group(globus_client):
 
 async def test_user_not_allowed(globus_client):
     authenticator = GlobusOAuthenticator()
-    authenticator.allowed_globus_groups = set({'3f1f85c4-f084-4173-9efb-7c7e0b44291a'})
+    authenticator.allowed_globus_groups = {'3f1f85c4-f084-4173-9efb-7c7e0b44291a'}
     handler = globus_client.handler_for_user(user_model('wash@uflightacademy.edu'))
     auth_model = await authenticator.get_authenticated_user(handler, None)
     assert auth_model == None
@@ -422,7 +422,7 @@ async def test_user_not_allowed(globus_client):
 
 async def test_user_is_admin(globus_client):
     authenticator = GlobusOAuthenticator()
-    authenticator.admin_globus_groups = set({'21c6bc5d-fc12-4f60-b999-76766cd596c2'})
+    authenticator.admin_globus_groups = {'21c6bc5d-fc12-4f60-b999-76766cd596c2'}
     handler = globus_client.handler_for_user(user_model('wash@uflightacademy.edu'))
     auth_model = await authenticator.get_authenticated_user(handler, None)
     assert auth_model['name'] == 'wash'
@@ -431,8 +431,8 @@ async def test_user_is_admin(globus_client):
 
 async def test_user_allowed_not_admin(globus_client):
     authenticator = GlobusOAuthenticator()
-    authenticator.allowed_globus_groups = set({'21c6bc5d-fc12-4f60-b999-76766cd596c2'})
-    authenticator.admin_globus_groups = set({'3f1f85c4-f084-4173-9efb-7c7e0b44291a'})
+    authenticator.allowed_globus_groups = {'21c6bc5d-fc12-4f60-b999-76766cd596c2'}
+    authenticator.admin_globus_groups = {'3f1f85c4-f084-4173-9efb-7c7e0b44291a'}
     handler = globus_client.handler_for_user(user_model('wash@uflightacademy.edu'))
     auth_model = await authenticator.get_authenticated_user(handler, None)
     assert auth_model['name'] == 'wash'