Redirect for reauth should not present login form

[janus-reith]

Issue Description

Currently, when the storefront recieves a Status 401 from the backend over GraphQL, it will redirect to the /signin route. This will then redirect to the meteor backend to get a new authentication token. The assumption here seems to be, that the Meteor User still is logged in. If the User is logged out on the Backend, this will just present a login form. Skipping the auth redirect won't solve anything, as the page will just stay at its current point and display a Status 500 then.

example-storefront/src/lib/apollo/initApollo.js

The desired behaviour should be: If GraphQL gives a 401, get an actual indication if there is a logged in backend user. If there is, do the redirect to get a new token. If not, ditch the current token and reload (Although I'd be unhappy with a real reload, maybe Apollo Client could just try to connect a second time )

Steps to Reproduce

Please provide starting context, i.e. logged in as a user, configure a particular payment method.

1. Log in to the storefront
2. Wait until the token expires
   2.5 Im not sure when actually the backend logout happens
3. Visit the storefront again, and see that it will redirect to the backend login form.

Possible Solution

I'm unsure if that can be fixed in the backend on the route that also provides the silent reauth, or has to be done in the storefront.

We currently don't separate actual logins from silent reauths in the /loginendpoint on the Backend:
https://github.com/reactioncommerce/reaction/blob/746f5acf8eb2a9e877d7ede93f46a595b8013348/imports/plugins/core/hydra-oauth/server/oauthEndpoints.js#L18

We could instead provide both a /login and /refresh (or /reauth as it isn't really a refresh in oauth terminology) endpoint. While the behaviour for both would be the same when getLoginRequestRes.skip is set, for the other case /login could still redirect to the login form as usual, while /refresh would redirect back to the storefront instead. I The storefront would have dropped the session cookie by then so it won't try the reauth again.

I think it would be even better if the unsuccessful redirect could be completely avoided, as it would happen quite often for logged in users. But the storefront would need to know beforehand if there is an authenticated backend user, and I'm not sure if that could work, as I think the Meteor Client bundle will always need to load once and grab the token from localStorage to which no other page has access. A hidden iframe solution could maybe work, that would only trigger a redirect if silent re-authentication is possible.

Im still need to figure out why exactly this issue occurs where the backend doesn't have valid authentication anymore while the frontend still thinks it is logged in.

Versions

2.1.0

[aldeed]

@janus-reith I am looking at this for 3.0.0 release. If we were going to change this, I think the correct solution is to create and redirect to a new storefront server route such as /silent-sign-in, which would do the auth call with a new { loginAction: "silentSignIn" }, which the identity service can then use to skip showing the login page.

However, I'm not sure this is what we want.

First, if they were logged in and now they aren't because their token expired, isn't it typical to show a login form? Maybe it should say "Your login has expired" on the form to avoid confusion, but it would still show the form. This would be only if Hydra forgets who they are, which it should be remembering in its own session cookie for 24 hours by default.

Second, Hydra docs are pretty clear about what the identity /login route needs to do:

The endpoint handler at /login must not remember previous sessions. This task is solved by ORY Hydra. If the REST API call tells you to show the login ui, you must show it. If the REST API tells you to not show the login ui, you must not show it. Again, do not implement any type of session here.

Part of the problem here might be a lack of understanding about when exactly this happens and why. If anyone can post specific examples, that we can reproduce easily, where it's not behaving as a user would expect, this would help.

Another part of the problem may be that storefront does not seem to do anything with the refresh tokens. At the moment I'm not sure if we are intentionally not doing refresh or if this was an oversight in the original implementation. Possibly we should be attempting a token refresh when we get a 401 instead of a silent re-auth.

More to investigate here.

[janus-reith]

@aldeed The issue here seems to be indeed related to the unused refresh token in the storefront.
While we could skip its usage like done currently with the redirect to the meteor login, I wonder if that is even how it is supposed to work, as it relies on the meteor login data still being present.

Therefore, a "silent" reauth in the meteor app like you described would somehow solve the initial issue, but seems like a workaround which migth be unnecessary. (Avoiding the clunky meteor load time for each refresh is another plus here.)

If I got things right, the IDP shouldn't be responsible for the token refresh at all, and the storefront could call hydra directly. The token refresh involves the clientId and clientSecret, so it should probably be called from the storefront express server, not the client.

A basic implementation could look like this:

1. The Storefront passport session needs to also store the refreshToken(to be discussed) in addition to the accessToken.
   To keep things stateless and work with loadbalancers, the storefront currently stores the session in an unencrypted cookie.
   While this might be fine for just the accessToken, we should probably switch out cookie-session with https://github.com/mozilla/node-client-sessions (tested it, works) to have an encrypted cookie, so the refreshToken could only be read by the express server but not the client itself.
   I'd love to hear better suggestions on how to handle the refreshToken in a simple but scaleable manner, but for now this seems to be the most simple way while still reasonable secure.

2. Create a "/refresh" route on the storefront server which takes the refreshToken from the encrypted cookie session and sends a POST call to hydra.
   If the response is succesfull, store the new accessToken + refreshToken in the encrypted cookie session, if not log out. Then in either case, no matter if successfull, redirect back.

3. In the withApollo HOC, for the client side executon just replace the redict to /signin with /refresh in the errorLink when a status 401 occurs.
   For the server side, do the same but remove the call to options.req.logout() which currently is in place to prevent infinite loops if the new signin doesn't work.
   Instead, just let the /refresh express endpoint handle that logic as described initially.

[janus-reith]

What I did not take into account here is the still unused expiresAt value, but I assume we don't need it.

A refresh would be triggered on each authenticated GraphQL call when neccessary so this should also work on cases without page load, like when polling is used. Also there is no third party service involved that would be called with an neccesarily up-to-date token, so we're good here.

[janus-reith]

I just finished the implementaton and will test now if everything checks out as expected (Really should've done this sooner, this issue annoyed me way too long ).

Let me know what you think about this approach and if I should submit a PR.

For technical reasons there now is no "storefront-session" and "storefront-session.sig" cookie anymore but just one called "session" (needed this name so passport + client-sessions work together) but this has the added benefit that everything should stay compatible. Previously logged in users would just be not logged in on their next visit.

[janus-reith]

Just tested it, everything seems to work:

1. Hydra reports an expiry time expires_At of 3600s (= 1 hours) so I saved a copy of my cookie and waited one hour.
2. After one hour, the page reloaded and I had a new cookie with token and I still was authenticated. The automatic reload happended for me because I have queries with polling in use, so that case is covered aswell.
3. Next I saved a copy of that new cookie aswell and put my old one back into place.
   After reopening or reloading the storefront I am not logged in anymore, and the session cookie is reset aswell (~10% of the size compared to when it contains tokens).

One thing that made we wonder at first is that I'm logged out here and no refresh is done with my previous, outdated cookie data. The reason is, that appareantly hydra issues new refreshTokens aswell on a refresh, invalidating the previous old one.
Maybe we need to investigate if that behaviour is desired and if it could/should be disabled in Hydra. One issue I could think of is, that if you are logged in on multiple clients, the others would be logged out on the next page visit as they didn't recieve the new one.

[janus-reith]

Another thing to note: When I had the chrome Applications Tab open, during the refresh I noticed that my session cookie data was changed before the actual page reload happened. This leads me to believe that maybe the refresh could be triggered without a redirect at all, simply by doing a POST call from the storefront client to server, then making sure the returned new accessToken (only access, refresh is never returned to the client) is stored in the withApollo HOC, replacing the outdated one fetched in getInitialProps.

But for now I would like to keep it the way it is, as I really consider it a rather small inconvience that the page reloads once when it was left open for one hour, as this refresh also keeps things less prone to errors for now as any potentially stale data e.g. from previosly authenticated queries is removed on reload.

Please move this to the exampe-storefront repo, when I opened it initially it was not clear where it would belong

[aldeed]

@janus-reith Moved the issue to this repo, thanks.

This is great investigation work. It seems to mostly align with what I was thinking.

• Your implementation sounds correct to me, so a PR would be great.
• Getting a new refresh token with every refresh is how it always works in oauth as far as I remember. It should be updated and used along with the auth token. That way you will remain logged in (by default) for 30 days from last refresh, rather than 30 days from first login.
• If you need to test more quickly, you can set TTL_ACCESS_TOKEN and TTL_REFRESH_TOKEN env on Hydra service to smaller values. I believe these are both set in number of seconds.
• Without a page refresh, I think it would get a new access token on the client, but the server would have the old one, which could lead to weirdness on the next user-initiated refresh. Maybe not, but it's something to check.
• Make sure whatever you do works correctly when ENABLE_SPA_ROUTING env is set to true and also when set to false. With SPA routing, most route changes are intercepted on the client. Without it, all route changes do a full page refresh.

[janus-reith]

@aldeed Thanks, will keep on testing and prepare a PR.
Had this in use in a dev environment for the past week and didn‘t encounter any errors yet.

Regarding the refresh token: I think it should only issue a new one when necessary. The (semi-)critical part I see here also is not the issuing of a new refresh token, but the invalidation of the previous one. If I’m logged in with client a and client b and both share the same login, as soon as client a would refresh, the token of client b would be considered invalid and client b would be logged out on the next visit after being redirected to my refresh endpoint.
Client B might have an outdated Access Token, but the refresh token should still be valid to allow for a token refresh.

However, I don’t consider this a blocker and would still favor this over the current implementation, also I think that issue is not related to the changes supposed here, as the token endpoint I call in my implementation seems to be the correct one, for reference: https://www.oauth.com/oauth2-servers/access-tokens/refreshing-access-tokens/
Still something to keep in mind and research.

[aldeed]

@janus-reith Is this still on your "make a PR when I get a chance" list?

[janus-reith]

@aldeed Hi, sorry, yes it is, altough I focused on the storefront reimplementation and fixed it there in one go.
I`m done with most of the stuff there and could quickly do a PR for the token refresh here if this still is relevant,

[aldeed]

It would be nice to fix here if it’s not a lot of work.

[janus-reith]

Will prepare something

[janus-reith]

Just to give an update, I basically finished this,
but faced a weird error here that I couldn't resolve yet properly:
https://github.com/reactioncommerce/example-storefront/blob/trunk/src/lib/apollo/initApollo.js#L68-L72

I had to remove options.req.logout(), as now the /refresh route will determine wether a refresh was successful, or the user should be logged out to prevent an infinite loop.
The weird thing: If that predecending logout call on req is not present, and I call options.res.writeHead, the app will crash, stating "Cannot set headers after they are sent to the client". This is however expected to only execute if process.browser is false.

This is not related to my implementation, but also is present on the default one when you remove the options.req.logout() part. The expectation would then be, that the app may take an infinite loop of trying to log in, but instead it will crash right away with that error.
I think this is somehow related to the fact that multiple Apollo Queries are executing in parallel, and this leads me to believe that this redirecting in there should never have worked in the first place. I'm really curious why that call on req migitates that error, I don't think it is because the actual logout.
I tried several things to fix this, but without success, the closest I could get was the wrap this in a try-catch block so only the first executes properly, but I don't really consider this a solution.

In my own usecase for the new storefront I dont handle any authenticated queries on SSR Render anymore and therefore didn't have that happening for me before, as only the Router.pushRoute clientside routing is left and I also believe that to be the way to go.

What I'm trying now is to get rid of the redirect alltogether and make this a completely silent refresh implementation, by doing a fetch call to the refresh endpoint inside the onError Link, that endpoint would now return the new acessToken as JSON instead of a redirect, rest is the same, new tokens including refresh token are storen encrypted in the cookie.
But that turned out to be really complex: The default example in the apollo docs doesn't seem to mimic a real usecase, as this only works as a sync call, which wouldn't work for fetching tokens.

What I need to do is to stall the query in there, try a token refresh, then execute it, either with a new token or unauthenicated without one.
I also found a solution for that - But the next issue is that by default, the storefront will have around 3-4 Queries executing in a short time frame, and I don't want each of them to trigger another token refresh plus the added delay of another fetch, in case the previous call doing that didn't finish yet.

So I have to batch all pending Queries, wait for the token refresh, then execute them, and I still have some issues at this point.
I still think it is worth to figure this out, as the new storefront would also benefit from the silent refresh instead of a clientside redirect.

I'm really baffled BTW that I couldn't find any official or complete example on how to handle the refresh properly in Apollo Client.

[aldeed]

@janus-reith Seems like you've put a lot of work into this, so thank you for that. Some of your issues bring to mind the same roadblocks we hit in the initial implementation.

I haven't looked at it closely, but there might be some techniques in this package that could help: https://www.npmjs.com/package/apollo-link-token-refresh

The logout call does nothing except clear the auth cookie. I would need to debug a lot more to fully understand how the multiple requests are clashing with each other, but it makes sense that would happen. Attempting to track the expiration time and reauth before the token expires (i.e., not in onError) might be an option, but it might miss some situations.

You might try posting an issue with some of this info in the Apollo Client repo. I'm sure they'd love to have good docs on how to do this with a standard OAuth flow, which they don't currently have. Maybe some minimal changes to the core packages would simplify what you're trying to do. It also seems like a situation where creating a minimal reproduction app would be helpful. An app that just has a single page that looks different depending on user and tries to fulfill all the requirements of the storefront.

[janus-reith]

@aldeed Thanks, I also noticed that package, and I think it basically does the same thing, which is to queue upcoming queries until the fetch is resolved.
I will compare implementations, because that's where I got stuck and didn't have time left last time. Also I did't want to just plug that in, as I didn't want to introduce another unknown here and instead want to be able to actually keep track of what happens at what point in the chain.