lms icon indicating copy to clipboard operation
lms copied to clipboard

Admin login fails if Google Drive permissions have been granted for the same Google user

Open robertknight opened this issue 1 year ago • 3 comments

Google login to the admin pages at https://lms.staging.hypothes.is/admin breaks if the same Google account used for the login has previously been used to authorize Google Drive access as part of creating an assignment.

Steps to reproduce:

  1. Create a Google Drive PDF assignment using the Hypothesis LMS staging app. Grant Google Drive access as part of this flow.
  2. Go to https://lms.staging.hypothes.is/admin and attempt to login with the same Google account

Expected: Login succeeds Admin: Login fails with 500 error

In this scenario, visiting https://lms.staging.hypothes.is/admin redirects to a Google login page with a URL that includes openid, userinfo.email and userinfo.profile scopes. After approving access, Google redirects back to the LMS app at https://lms.staging.hypothes.is/googleauth/login/callback with an authorization code. In the redirect URL the scopes include the requested ones but also have the existing Google Drive scope added.

The presence of these existing extra permissions causes an exception on our end (Sentry).

A workaround is to go to your Google account settings and revoke Google Drive permissions for the Hypothesis LMS app under security settings.

robertknight avatar Oct 18 '24 11:10 robertknight

The Warning exception is being raised by oauthlib, which in turn is called by pyramid_googleauth. See https://github.com/oauthlib/oauthlib/blob/bf75322aad61785501405308f3c4cd020983d17d/oauthlib/oauth2/rfc6749/parameters.py#L463.

robertknight avatar Oct 18 '24 11:10 robertknight

oauthlib throws an exception if the OAuth callback URL includes different scopes than what were requested, even if the returned scopes are a superset of the requested scopes. The warning can be globally disabled by setting the OAUTHLIB_RELAX_TOKEN_SCOPE environment variable. See https://github.com/oauthlib/oauthlib/issues/562. Another workaround documented in https://github.com/oauthlib/oauthlib/commit/ca4811b3087f9d34754d3debf839e247593b8a39 is to catch the Warning exception and extract the OAuth2Token object from in, if the revised scopes are acceptable.

robertknight avatar Oct 18 '24 11:10 robertknight

Catching the Warning exception thrown by oauthlib is problematic because we're not directly calling the library that is throwing it. The call chain looks like:

lms -> pyramid_googleauth -> google_auth_oauthlib -> requests_oauthlib -> oauthlib

Here requests_oauthlib.OAuth2Session has a fetch_token method that prepares a request for the authorization token, fetches it, validates the response, saves the token in the OAuth2Session and returns it. The Warning exception is thrown during validation, before the token has been saved on the session. If we catch the token at this point, it won't be saved on the session which could lead to unexpected behavior later. See also https://github.com/requests/requests-oauthlib/issues/507 which is a different issue about scopes changing.

robertknight avatar Oct 18 '24 12:10 robertknight