refactor, github: plain refactor for readability

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,23 +194,20 @@ 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),
                 validate_cert=self.validate_server_cert,
             )
             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,31 +272,41 @@ 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",
             headers=headers,
             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"""

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