ITenthusiasm / remix-supertokens

A repository for exemplifying/testing Remix usage with SuperTokens
MIT License
24 stars 0 forks source link

Need to handle errors thrown from the getSession function on the backend #1

Closed rishabhpoddar closed 2 years ago

rishabhpoddar commented 2 years ago

RIght now, errors are ignored if getSession throws. It could throw if the access token has expired, and we need to catch that and send a reply to the frontend to tell it to call the refresh API.

Normally, if you used verifySession middleware, it would send a 401 to the frontend and the supertokens-website repo would do an automatic refresh. But since you are not using these, here is what you could do:

If using JS on the frontend is not an option at all, then I would recommend that you do not use our access + refresh token sessions, and override the session recipe functions on the backend to issue a long lived token (like a JWT). An example of this can be found here: https://github.com/supertokens/supertokens-auth-react/tree/master/examples/with-jwt-localstorage (this example sets the JWT in the response body and localstorage, but you can easily set it to cookies instead).

ITenthusiasm commented 2 years ago

Normally, if you used verifySession middleware, it would send a 401 to the frontend and the supertokens-website repo would do an automatic refresh. But since you are not using these, here is what you could do:

I can't remember why I stopped using verifySession. :thinking: But I think it was because Remix wants to have total control over how routing goes. If I remember correctly, verifySession was able to handle users without tokens and users with valid tokens. But if a 401 response gets sent back by verifySession because of an expired token, then it would return that 401 you mentioned. The problem is that this effectively puts the application in a permanently inaccessible state if the user isn't using JS (or if the JS is missing for some unknown reason). And since the 401 is returned before the actual app middleware can do anything, there isn't anything that Remix (or myself) could do about it. I think I was using getSession so that I can always redirect when necessary.

While I'm thinking about it, if the main difference with verifySession is that it handles expired tokens for developers, then would it perhaps be worthwhile to add an additional options parameter to the function? Something like verifySession({ sessionRequired: false, preventExpired: false }). (That doesn't have to be the exact property name.) This way, SSR frameworks can opt in to have truly full control over routing, and developers don't have to write a custom function just to handle this use case.

ITenthusiasm commented 2 years ago

If using JS on the frontend is not an option at all, then I would recommend that you do not use our access + refresh token sessions, and override the session recipe functions on the backend to issue a long lived token (like a JWT).

Are you saying this from a security standpoint or from a user experience standpoint?

From a security standpoint, I assumed that what I have now would work just fine. If the token is expired, I believe my application will redirect users to the login page, forcing them to login again to get a new token. Is this safe?

From a user experience standpoint, I can understand why users wouldn't want to login repeatedly. The idea of Remix is to have an application that can run without JS, but that is still enhanced by JS. What I'm going for here is something that wouldn't break if the JS was missing for some reason in a special circumstance. But I'm intending/assuming that most users will be using JS. Given that supertokens-website typically handles the refreshing aspect, I was thinking I would include it in the app.

ITenthusiasm commented 2 years ago

(Sorry, I know I'm writing a lot. I really appreciate your help with all of my questions -- both here and on Discord.)

If the request is via a browser navigation (SSR), then you should redirect the user to a page which will call the refresh POST API. This would require you to run JS on the frontend though. If you do not want to do that, then you could create your own GET API that has the same path as the built in refresh API, and redirect the user to that page.

So the second approach to this option is sounding like I wouldn't require JavaScript on the frontend, correct? It sounds like I could:

  1. Detect an expired token
  2. Redirect to a /refresh-token page
  3. On the backend, handle the GET request and translate it to something SuperTokens can understand (just like I do for all other auth-related requests on the server side)
  4. Return the response with the new tokens and redirect the user to where they originally were (similar to what I do for user logins)

It seems like this is doable without needing JS on the frontend, yes?

rishabhpoddar commented 2 years ago

While I'm thinking about it, if the main difference with verifySession is that it handles expired tokens for developers, then would it perhaps be worthwhile to add an additional options parameter to the function? Something like verifySession({ sessionRequired: false, preventExpired: false }). (That doesn't have to be the exact property name.) This way, SSR frameworks can opt in to have truly full control over routing, and developers don't have to write a custom function just to handle this use case.

Well, adding a preventExpired response, is pretty much the same as users using getSession and then catching the exceptions (like when try refresh token exception is thrown) and doing whatever they like. So you should continue to use getSession.

You can see an example of how we use getSession and catch it's errors in the context of a NextJS app (for server side rendering): https://supertokens.com/docs/emailpassword/nextjs/session-verification/in-ssr#1-we-use-getsession-in-getserversideprops

From a security standpoint, I assumed that what I have now would work just fine. If the token is expired, I believe my application will redirect users to the login page, forcing them to login again to get a new token. Is this safe?

Yes. This is totally fine from a security point of view (assuming that the access token's lifetime is kept "small"), but terrible from a UX point of view.

Given that supertokens-website typically handles the refreshing aspect, I was thinking I would include it in the app.

I don't think you should do that. If you are going with the route of redirecting on refresh, then stick pattern to that everywhere. If you use this package, then you will have to make all API calls (sign up, sign in, sign out, etc) use axios or fetch and that requires JS. That being said, if you can make make all API calls via form submissions (in case JS is disabled), and use axios or fetch in case JS is enabled, then using supertokens-website would be a good idea.

I say it's a good idea cause let's say a user is filling in a long form post login, and they have JS disabled. Now when they click on the submit button, and if the access token has expired already, then they will be redirected for a session refresh and then redirected back - this way, they would loose their filled in form and would have to refill it all (which can be quite annoying). So in this case, if the user has enabled JS, and use use axios or fetch for that situation, then it would refresh the session automatically (without a redirect), and the UX would be good.

It seems like this is doable without needing JS on the frontend, yes?

Yes, you are on the right track. it is indeed doable. The only issue with the steps you mentioned is that the page the user needs to be redirected to, must have have the same path as the refresh token API (that is provided by us). That path is = /{apiBasePath}/session/refresh (/auth/session/refresh by default). The reason for this is that the refresh token cookie is restricted to be sent to only that exact path (for security reasons).

ITenthusiasm commented 2 years ago

That being said, if you can make make all API calls via form submissions (in case JS is disabled), and use axios or fetch in case JS is enabled, then using supertokens-website would be a good idea.

Yes. When using Remix's Form component, it will use fetch when JS is available and fallback to the native browser behavior when JS is unavailable.

That path is = /{apiBasePath}/session/refresh (/auth/session/refresh by default). The reason for this is that the refresh token cookie is restricted to be sent to only that exact path (for security reasons).

Okay. This is helpful to know. Is there an easy way to make access tokens expire early to test these use cases? Or can refreshing only be tested by making the access token's life incredibly short?

rishabhpoddar commented 2 years ago

Yes. When using Remix's Form component, it will use fetch when JS is available and fallback to the native browser behavior when JS is unavailable.

Ah right! Then using supertokens-website should work well (is JS is enabled).

Is there an easy way to make access tokens expire early to test these use cases? Or can refreshing only be tested by making the access token's life incredibly short?

You can change the access token's lifetime: See https://supertokens.com/docs/session/common-customizations/sessions/change-session-timeout

You can also manually delete the access token from the cookie store in chrome and that would trigger a refresh.

ITenthusiasm commented 2 years ago

The only issue with the steps you mentioned is that the page the user needs to be redirected to, must have have the same path as the refresh token API (that is provided by us).

Just wanted to double check something here. Does the API provide a way to redirect users after a token has been refreshed? I'm assuming not?

rishabhpoddar commented 2 years ago

Just wanted to double check something here. Does the API provide a way to redirect users after a token has been refreshed? I'm assuming not?

You can override the refresh API in the session.init part on the backend and use that to send a custom status code like this:

Session.init({
    override: {
        apis: (oI) => {
            return {
                ...oI,
                refreshPOST: async function (input) {
                    // when redirecting to the refresh API, we append a custom query param
                    // to it "redirectTo" which tells it where to redirect the user back to,
                    let redirectTarget = input.options.req.getKeyValueFromQuery("redirectTo");

                    let response = input.options.res.original; // express response object

                    // do the actual refreshing.
                    try {
                        await oI.refreshPOST!(input);
                    } catch (err) {
                        if (Session.Error.isErrorFromSuperTokens(err)) {
                            if (err.type === Session.Error.UNAUTHORISED) {
                                // the session is not valid anymore.
                                return response.redirect("/to-login-page")
                            }
                        }
                        throw err;
                    }

                    return response.redirect(redirectTarget)
                }
            }
        }
    }
})

Im not completely sure that this will work, since we are sending a non 200 response to the browser, so maybe it doesn't set the cookies, but i think it will work.

ITenthusiasm commented 2 years ago

Okay cool. I'll try to see how this goes and then give an update. Thanks!

rishabhpoddar commented 2 years ago

hey @ITenthusiasm can this issue be closed?

ITenthusiasm commented 2 years ago

Yes! If you find any problems, you can reopen this issue.

Should be fixed by faf6238985d180cdf9aa164ff56a8fa7fa1770a0. Still working on a basic localStorage example.