Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Google] Make looking up google groups far less blocking #764

Merged
merged 13 commits into from
Oct 8, 2024
29 changes: 28 additions & 1 deletion oauthenticator/google.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

class GoogleOAuthenticator(OAuthenticator, GoogleOAuth2Mixin):
user_auth_state_key = "google_user"
_credentials = None
jrdnbradford marked this conversation as resolved.
Show resolved Hide resolved

@default("login_service")
def _login_service_default(self):
Expand Down Expand Up @@ -314,6 +315,31 @@ async def check_allowed(self, username, auth_model):
# users should be explicitly allowed via config, otherwise they aren't
return False

def _get_credentials(self, user_email_domain):
"""
Returns the stored credentials or fetches and stores new ones.

Checks if the credentials are valid before returning them. Refreshes
if necessary and stores the refreshed credentials.
"""
if not self._credentials or not self._is_token_valid():
self._credentials = self._setup_credentials(user_email_domain)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Credentials seems to be received for a user_email_domain, doesn't that makes them different between users? If so, isn't it causing issues to re-use them for any user?

I figure we would at least need to have a dictionary of credentials, with one for each user_email_domain, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If multiple people login at the same time _setup_credentials could be called multiple times in parallel. Is this likely to cause any problems?

Copy link
Contributor Author

@jrdnbradford jrdnbradford Oct 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@manics this is a good question. On the off-chance this does happen and there multiple tokens fetched, those tokens will still be valid for making the request.


return self._credentials

def _is_token_valid(self):
"""
Checks if the stored token is valid.
"""
if not self._credentials:
return False
if not self._credentials.token:
return False
if self._credentials.expired:
return False

return True

def _service_client_credentials(self, scopes, user_email_domain):
"""
Return a configured service client credentials for the API.
Expand Down Expand Up @@ -357,6 +383,7 @@ def _setup_credentials(self, user_email_domain):

request = requests.Request()
credentials.refresh(request)
consideRatio marked this conversation as resolved.
Show resolved Hide resolved
self.log.debug("Credentials refreshed")
return credentials

async def _fetch_member_groups(
Expand All @@ -372,7 +399,7 @@ async def _fetch_member_groups(
Return a set with the google groups a given user/group is a member of, including nested groups if allowed.
"""

jrdnbradford marked this conversation as resolved.
Show resolved Hide resolved
credentials = credentials or self._setup_credentials(user_email_domain)
credentials = credentials or self._get_credentials(user_email_domain)
checked_groups = checked_groups or set()
processed_groups = processed_groups or set()

Expand Down