[Google] Make looking up google groups far less blocking #764
Conversation
There was a problem hiding this comment.
We could use aiohttp, but there is also the OAuthenticator base class function below that makes use of tornado.HTTPRequest which can be done async. If using this could work, that would be preferred as we avoid introducing another way of making HTTP requests.
async def httpfetch(
self, url, label="fetching", parse_json=True, raise_error=True, **kwargs
):
"""Wrapper for creating and fetching http requests
Includes http_request_kwargs in request kwargs
logs error responses, parses successful JSON responses
Args:
url (str): url to fetch
label (str): label describing what is happening,
used in log message when the request fails.
parse_json (bool): whether to parse the response as JSON
raise_error (bool): whether to raise an exception on HTTP errors
**kwargs: remaining keyword args
passed to underlying `tornado.HTTPRequest`, overrides
`http_request_kwargs`
Returns:
parsed JSON response if `parse_json=True`, else `tornado.HTTPResponse`
"""
request_kwargs = self.http_request_kwargs.copy()
request_kwargs.update(kwargs)
req = HTTPRequest(url, **request_kwargs)
return await self.fetch(
req, label=label, parse_json=parse_json, raise_error=raise_error
)
|
Thanks for your guidance here on using |
jrdnbradford
changed the title
|
Oh beautiful!!! Have you been able to test that this works as well? I think our test coverage won't be able to tell so well if it does |
|
I've tested this on $ lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Description: Ubuntu 24.04.1 LTS
Release: 24.04
Codename: nobleIf you like, I can probably test with several logins at the same time sometime soon. 🚀 |
|
@jrdnbradford I pushed a commit removing a dependency I think wasn't used more, do you think that was correct? I really appreciate those complexity reductions, thanks for working this!!! |
I'm pretty sure you're right, now that we don't build the service that isn't needed. I'll test to make sure. I've also scheduled a time tomorrow to get several semi-simultaneous logins on a dev TLJH with the oauthenticator changes. I'll report back after. |
oauthenticator/google.py
Outdated
| 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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
If multiple people login at the same time _setup_credentials could be called multiple times in parallel. Is this likely to cause any problems?
There was a problem hiding this comment.
@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.
|
I have added support so that creds/tokens are saved on a domain-basis in a dictionary and are refreshed only if necessary. My testing with a single domain works, but I'll need some more time to test multiple Google workspace domains in a JupyterHub installation to ensure it works. |
consideRatio
changed the title
There was a problem hiding this comment.
This is brilliant, really pleased about the refresh logic detail and the removal of now unused python dependency.
If this passes your end to end testing I figure we go for a merge!
@manics what do you think?
consideRatio
changed the title
Co-authored-by: Simon Li <[email protected]>
for more information, see https://pre-commit.ci
|
Awesome, will report back after I setup another Workspace domain for testing this. |
|
Turns out I don't have the access to another workspace domain to do this testing. @consideRatio and @manics, any contacts that use Google with multiple domains that could install the dev version for a test? |
| # WARNING: There's a race condition here if multiple users login at the same time. | ||
| # This is currently ignored. | ||
| credentials = credentials or self._get_service_credentials(user_email_domain) | ||
| token = credentials[user_email_domain].token |
There was a problem hiding this comment.
@manics I've tried to understand this race condition, but I've failed to see it. Are you sure there is one?
My understanding made explicit: whenever an await is showing up, then and only then is Python possibly switching to execute something else.
I'm not able to get this done, but I'm willing to go for a merge anyhow I think. As @manics wrote:
I'll go for it! Thank you a lot for thorough work on this @jrdnbradford ! |
|
If there are any issues please @ me and I'd be happy to fix! |
Closes #607 and should only be merged after #763 as it is stacked. There might be some conflicts that I will resolve.
The main change was removing the creation of the
serviceand instead retrieving an oauth token and using an asynchronousaiohttpsession to get the groups. If Google ever releases an asynchronous client that uses theservicethen we should switch to that because it's more readable.