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

Show error messages when OAuth access token and refresh token requests fail #376

Closed
wants to merge 2 commits into from

Conversation

seanh
Copy link
Contributor

@seanh seanh commented Apr 27, 2017

Show an error if the initial request for an OAuth access token or a later request to refresh an access token fails.

When the client is embedded in a page that includes a services
configuration setting (see http://h.readthedocs.io/projects/client/en/latest/publishers/config/#cmdoption-arg-services)
then it tries to login to Hypothesis using OAuth, exchanging the grant
token from the services config setting for an access token from the
Hypothesis API.

If this request for an access token fails (for example, if the
Hypothesis server responds with an error) then the client ends up in an
invalid state. Some of the sidebar components are not fully rendered,
with visible parts missing, no annotations are shown, and actions such
as trying to create an annotation fail in broken ways.

For example you can hack h to always return an error for the access token request like this:

diff --git a/h/services/oauth.py b/h/services/oauth.py
index ca2745c3d..140dc9f9b 100644
--- a/h/services/oauth.py
+++ b/h/services/oauth.py
@@ -69,6 +69,8 @@ class OAuthService(object):
         :returns: a tuple with the user and authclient
         :rtype: tuple
         """
+        raise OAuthTokenError('Not a valid grant token',
+                              'invalid_request')
         if 'assertion' not in body:
             raise OAuthTokenError('required assertion parameter is missing',
                                   'invalid_request')

and then the client will look like this:

peek 2017-04-27 13-30

We should really fix the completely broken appearance and behavior of
the client in this state, but that'll be a big job. For now, at least show an
error message to the user telling them that they need to reload the page:

peek 2017-04-27 13-33

(This error message doesn't disappear until the page reloads, not even if the user clicks on it.)

There's currently no facility to retry the access token request but if
the user reloads the page the whole process will begin again and the
request will be tried again.

Additionally, when embedded in a publisher site like this the client needs to refresh the access token every hour. If the refresh request fails then things stop working. For example here trying to dismiss the sidebar tutorial fails silently, while trying to create an annotation gives an unfriendly error message:

peek 2017-04-27 14-44

You can hack h to make access tokens expire after 5 seconds and to make all requests to refresh them fail like this:

diff --git a/h/services/oauth.py b/h/services/oauth.py
index ca2745c3d..f0e8e098c 100644
--- a/h/services/oauth.py
+++ b/h/services/oauth.py
@@ -12,7 +12,7 @@ from h.exceptions import OAuthTokenError
 from h._compat import text_type
 
 # TTL of an OAuth token
-TOKEN_TTL = datetime.timedelta(hours=1)
+TOKEN_TTL = datetime.timedelta(seconds=5)
 
 
 class OAuthService(object):
@@ -103,6 +103,9 @@ class OAuthService(object):
                 raise
 
     def _verify_refresh_token(self, body):
+        raise OAuthTokenError('not valid',
+                              'invalid_request')
+
         refresh_token = body.get('refresh_token')
 
         if not refresh_token:

Again, the user has to reload the page. We don't retry the refresh request (and even if we did, once the access token has expired it can no longer succeed). When a refresh request fails show a permanent error message telling the user to reload the page:

peek 2017-04-27 14-45

@seanh seanh added the WIP label Apr 27, 2017
@seanh seanh changed the title Show error on fail to get OAuth access token Show error messages when OAuth access token and refresh token requests fail Apr 27, 2017
seanh added 2 commits April 27, 2017 15:54
Show an error if the initial request for an OAuth access token fails.

When the client is embedded in a page that includes a services
configuration setting (see <http://h.readthedocs.io/projects/client/en/latest/publishers/config/#cmdoption-arg-services>)
then it tries to login to Hypothesis using OAuth, exchanging the grant
token from the services config setting for an access token from the
Hypothesis API.

If this request for an access token fails (for example, if the
Hypothesis server responds with an error) then the client ends up in an
invalid state. Some of the sidebar components are not fully rendered,
with visible parts missing, no annotations are shown, and actions such
as trying to create an annotation fail in broken ways.

We should really fix the completely broken appearance and behavior of
the client in this state, but that'll be a big job.

For now, at least show an error message to the user telling them that
they need to reload the page.

There's currently no facility to retry the access token request but if
the user reloads the page the whole process will begin again and the
request will be tried again.
When the client is embedded in a page that includes a services
configuration setting then it logs in to Hypothesis using OAuth,
getting an access token from the Hypothesis API.

The Hypothesis API's OAuth access tokens currently expire after an hour
and must be refreshed by the client before then. When the request to
refresh an access token fails the client continues to look and work
exactly as before from the user's point of view, except that all actions
that require communicating with the API such as creating an annotation
fail and show error messages.

Some actions, such as dismissing the sidebar tutorial, fail with no
error message.

Currently we only try the refresh request once for each access token and
don't retry is the request fails. Even if we did retry the refresh
request can't succeed after the access token has expired - it's always
possible for the client to get into a situation where the access token
is expired and can no longer be refreshed.

The only fix (currently) is for the user to reload the page which will
re-do the initial grant token request to get a new access token.

So when a refresh request fails show an error message to the user
telling them that they need to reload the page.
@seanh seanh force-pushed the show-error-on-oauth-token-fail branch from 1191f66 to d826fd7 Compare April 27, 2017 14:56
@seanh
Copy link
Contributor Author

seanh commented Apr 27, 2017

This is done but just needs tests.

When refreshing the access token fails I'd like it to show the error when the token expires, rather than when the request fails (when the access token could still have 5 mins before expiry) but I think doing that would require a little refactoring so I'd do it in a follow-up.

I'm gonna mark this as needs review as I'd like to check that this is an acceptable fix for hypothesis/product-backlog#169 before taking the time to write the tests for it.

@seanh
Copy link
Contributor Author

seanh commented May 12, 2017

Discussed in Slack, I'll finish up this PR with an aim to getting it reviewed and merged. It may not be the final fix that we want for this problem but it's an improvement over what's on master now and it shouldn't be much work to finish it

@seanh
Copy link
Contributor Author

seanh commented May 15, 2017

I'm gonna close this because I've realised that when I wrote the refresh token code (that is already on master) I missed something about how setTimeout works: if I, say, use setTimeout to call a function in 10 minutes time, then I wait 5 minutes, then put the laptop to sleep for an hour, then wake it up, it will wait another 5 minutes and then call the function. That is, the time that the laptop spent asleep does not count towards the timeout, it pauses the timer and then continues the timer again when the laptop wakes. This will also happen if the browser or OS suspends the tab or app, not just if the laptop goes to sleep. I've verified this behaviour in Chrome and Firefox on Linux, and it also agrees with this StackOverflow answer and the HTML5 spec on setTimeout: "wait until the Document associated with the method context has been fully active for a further timeout milliseconds (not necessarily consecutively)." (It's not mentioned at all in places like the MDN docs for setTimeout though, hence I missed it.)

It shouldn't be too difficult to fix (I think we need to use setInterval and test, every few seconds, whether the access token is close to timing out) so I'll send a PR for that first and then we can add the error message on top of that code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants