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

Allow partner sites to provide grant tokens asynchronously #330

Open
6 tasks
seanh opened this issue Jun 22, 2017 · 2 comments
Open
6 tasks

Allow partner sites to provide grant tokens asynchronously #330

seanh opened this issue Jun 22, 2017 · 2 comments

Comments

@seanh
Copy link

seanh commented Jun 22, 2017

Problem

TLDR: The benefits of fixing this issue should be:

  1. Faster loading of the Hypothesis client on eLife pages. (How much faster this would be we don't know - the difference may not be noticeable to users.)

  2. A nicer API for partner developers who're integrating the Hypothesis client into their sites. (Not end-user visible but nicer for partners.)

  3. Partner pages that have Hypothesis embedded and use third-party accounts might become faster to load because they'd become cacheable because they don't contain a grant token. This does not apply to eLife (they already have to fetch their grant tokens asynchronously), and it would only apply to other partner sites if there's nothing else, besides our grant token, preventing pages for logged-in users from being cached.

Details of the problem:

Partner sites that embed the Hypothesis client and use third-party accounts currently need to provide a grant token, representing the third-party user who should be logged in to the Hypothesis client (e.g. a fred@elifesciences.org Hypothesis account), as part of the HTML of the page at page render time. They have to put a JavaScript function in the page that looks like this:

window.hypothesisConfig = function () {
  return {
    ...
    services: [{
      ...
      grantToken: 'xyzabc123789',
    }],
  };
};

Our client then calls this function during its boot process to get the grant token and other configuration settings.

(See http://h.readthedocs.io/projects/client/en/latest/publishers/config/#cmdoption-arg-services).

One problem with this is that if the grant token is part of the rendered page then, because the grant token is different for every user, it prevents the HTML file from being cacheable.

A second problem with this is that our partner site, https://elifesciences.org/, does not have access to the grant token value at page render time, the page actually needs to make an AJAX request to get the grant token.

A workaround is that eLife can actually defer injecting the Hypothesis client's embed.js script into the page until their AJAX response comes back and they're ready to return the grant token, then they inject our embed.js script and (as soon as it's ready) our client calls their hypothesisConfig() function and gets the grant token.

The problem with this workaround is that our client doesn't begin loading (loading its javascript, css and image assets, booting up...) during the time when their AJAX request is in-flight, the whole client load and boot doesn't happen until after the AJAX request for the grant token has finished.

And also having to defer injecting of our embed.js script into the page (and therefore having to inject the script via JavaScript) is a pain for partners to have to do.

It would be better if the partner could render the embed.js script into the page in the first place, our client could start loading immediately, but our client provided a nice way for the partner to provide the grant token asynchronously.

Separate issue: login and logout

We've also talked about the situation when the user is not logged in to the partner site when the page loads, but then after page load the user does log in, and the partner site page's login mechanism enables the user to log in without reloading the page. (Or vice-versa: the user is logged in at page load and then they log out, and then maybe log in again, all without reloading the page...). Currently there is no way for the partner site to log the user in to or out of the embedded Hypothesis client without reloading the whole page.

I'm just noting here that this login logout issue is a separate issue, it will have its own complexities, we're not going to try and solve it here. This issue is just for allowing partner sites to provide a single grant token configuration value asynchronously.

Proposed solution

The proposed solution is to allow the services[].grantToken value that the partner site's window.hypothesisConfig() function returns to be a Promise that resolves to either a string or (when no user is logged in) null:

window.hypothesisConfig = function(){
  return {
    ...
    services: [{
      ...
      grantToken: new Promise(function(resolve, reject){
      	// ...do my async thing
        resolve(grantToken);
      }),
    }],
  };
};

It should still be possible for the grant token to just be plain null or a string as well, this is necessary for example when the grant token is provided in a js-hypothesis-config script. So the client will have to detect whether the grant token is a Promise or not, and treat it accordingly.

Separate issue: getting a new grant token when the client's refresh token is expired

A known issue is that our client can sometimes end up with an expired refresh token, and has to ask the user to reload the page so that it can get a new grant token. With this new asynchronous way of providing grant tokens, if the partner site provided our client with a function that returns a grant token or a promise of a grant token, then our client could potentially call this function again later if it needed to get a new grant token, instead of asking the user to reload the page. (And we could maintain backwards compatibility - hypothesisConfig() could be allowed to return any of string|null, Promise<string|null>, () -> string|null, or () -> Promise<string|null> and the client could detect each case and react accordingly.)

This would potentially actually mean that the client wouldn't need OAuth refresh tokens at all when embedded in a site with third-party accounts, and could use only access tokens and grant tokens, and (at least in the case of embedded clients using third-party accounts) the client's OAuth-related code could be much simpler while also being more robust.

If the partner provided us with a URL that we call to get the grant token instead of getting the token via JavaScript (See Other solutions below), that would also be re-callable.

We're not going to implement this (the client requesting a fresh grant token when it needs a new one) as part of this issue, I'm just noting it as a potential future benefit.

Also note that there's potentially a different fix for this issue anyway.

Other solutions

Other solutions that we considered and rejected:

  1. The partner could provide is with a URL that we call to get the grant token. Actually if the partner is providing us with a Promise that makes an AJAX request and then resolves to the grant token from the response - then it seems that they already have this URL, and we're just making them write more code to wrap the URL in a Promise for us. On the other hand if we call the URL ourselves then we need to specify to partners what the HTTP request and response would look like. To avoid potential CORS issues we'd have to call this URL from our src/annotator/ code not from our iframe, but that's also true of a Promise returned by hypothesisConfig(). We rejected this idea of the partner providing us with a URL because hypothesisConfig() returning a Promise seems simpler and more flexible.

  2. We provide a callback function, window.something(), that the partner calls. But partner would need to wait for our code to boot and the function to be defined before calling it. It seems to make more sense to pass a callback in to the hypothesisConfig() func that we already call (see 3 below).

  3. We could pass in a client object as an argument to hypothesisConfig(), and the partner could call a client.login(grantToken) method when they're ready to pass the grant token. In fact we may want to add this to support the separate asynchronous login and logout issue mentioned above. For this issue though we rejected the idea of a client object as we'd have to tell partners to make sure to call the client object soon after page load, because our client would have to defer (some of) its initialisation until it has the grant token. The partner just returning a Promise that resolves ASAP seems simpler.

Tasks

TODO: This section needs to be fleshed out with more detail about the client code changes that would be needed, in order to get a better idea of how much work this is.

  • Change the annotator code to send the grant token over the bridge into the sidebar, rather than putting it in the sidebar's boot-time config (?config=... param on sidebar's URL) as it does currently.

    It should omit grantToken from the normal services config, wait on the grantToken Promise that hypothesisConfig() will now return, and send the grant token over the bridge when the Promise resolves. It also needs to detect when the grantToken returned by hypothesisConfig() is a plain string or null rather than a Promise, wrap it in a resolved Promise, and continue.

  • Change the sidebar to receive the grant token over the bridge and do a login in response. We already have mechanisms to unload and reload the user's state based on logins and logouts, so this part hopefully shouldn't be too hard.

  • The client should look like a normal, logged out client when it first loads and then change to a logged-in client when it receives the grant token

  • Some parts of the client need to be hidden until it receives the grant token.

    When the grant token is received it will either be a token for a logged-in user, in which case the client changes to its normal logged-in user UI, or it will be null, in which case the client shows its normal logged-out UI. In the meantime, when the client hasn't received the grant token yet, it doesn't know whether the user is logged-in or logged-out. Certain parts of the normal logged-out UI should be hidden because they don't make sense in these circumstances - for example the Sign up / Log in button and the the create a free account or log in banner:

    screenshot from 2017-06-22 13-30-22

    (If the user clicks on the login button for example, this is supposed to call eLife's login function, but the user may already be logged in to eLife.)

    Apparently this neither-logged-in-or-out state already exists in the client (it's used in the current third-party accounts OAuth implementation while the client is busy exchanging the grant token with the Hypothesis server for an access token) so all we have to do is add a new Promise for the grant token itself, and plug that in to the existing Promise chain.

  • The sidebar should defer loading any annotations until the grant token is received over the bridge.

    We want to avoid the sidebar first loading as logged out and requesting all the public annotations from the API, and then milliseconds later receiving the grant token and re-requesting all the annotations again with the user's auth.

    Again, the client already does this while it's exchanging the grant token for an access token, so we just need to plug the wait for the asynchronous grant token into the chain.

  • The client should time out if it waits too long for the publisher's Promise to resolve, and should revert to the normal logged-out state. The publisher site's Promise may never resolve, and there's nothing we can do about this

See Also

@seanh
Copy link
Author

seanh commented Jun 28, 2017

Just noting that @nickstenning argues that an HTTP API for a grant token is actually simpler than a Promise for a grant token (and this seems like a good argument to me): https://hypothes-is.slack.com/archives/C5JB5AL11/p1498651684908617

@seanh
Copy link
Author

seanh commented Jan 17, 2018

@ajpeddakotla This only applies to "third-party groups" (i.e. eLife) and won't apply to the new first-party open and restricted groups. In this case, should we remove the groups label from this? (Maybe we need a separate 3rd-party groups label or just use the eLife label.)

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

No branches or pull requests

4 participants