supertokens / prebuiltui

This is a repo that's built on top of supertokens-auth-react that provides a bundle that can be injected into your app to provide the pre built experience of supertokens
MIT License
2 stars 1 forks source link

Can't log back in after logging out via a server-side session revocation until access token expires #4

Closed gwd closed 1 month ago

gwd commented 1 month ago

Thanks for making this little pre-built thing. Found one issue / opportunity for improvement.

How to reproduce:

  1. Use pre-built auth page here to log in
  2. Implement "Logout" using RevokeSession() on the backend, redirecting back to "/" afterwards
  3. Try to log in again

What I expected to happen:

Presented with the login screen

What actually happens:

Redirected to "/", but still not logged in. This happens until the authorization token expires on its own (which in my dev environment is 30s).

Also, if you navigate to a page which requires authorization, you'll get into a loop; e.g., "/study" -> "/auth?redirectTo=/study" -> "/study" -> &c until interrupted (or the auth token expires).

Obviously this is happening because the front-end token hasn't been refreshed. Since:

  1. The auth page is meant to be a bit "heavyweight" anyway
  2. The pre-built UI is meant to be for people who are doing most of the work server-side

it seems like it would be better to actually just unconditionally refresh the token (or check w/ the server to make sure it's still valid).

gwd commented 1 month ago

Hmm, but really it seems like since we do have a connection and are returning a response, the auth token should be deleted from the client. Can you see anything wrong with the code below? The session certainly is being revoked on the server side.

func GetLogout(w http.ResponseWriter, r *http.Request, dbcache *storehouse.Cache) {
    if sess, err := session.GetSession(r, w, nil); err != nil {
        if errors.As(err, &sesserrs.TryRefreshTokenError{}) {
            // Redirect to /refresh-token to refresh the token before we log out
            middlewares.RedirectTokenRefresh(w, r)
            return
        }
        if !errors.As(err, &sesserrs.UnauthorizedError{}) {
            logrus.Errorf("GetLogout: Getting session: %v (type %T)", err, err)
        }
    } else {
        if username, err := middlewares.GetSessionKey[string](w, r, middlewares.SessionUsername); err != nil {
            logrus.Errorf("GetLogout: Getting username: %v", err)
        } else {
            dbcache.CloseUser(username)
        }

        if err := sess.RevokeSession(); err != nil {
            RenderError(w, r, http.StatusInternalServerError,
                "Hmm, sorry that didn't work",
                fmt.Sprintf("INTERNAL ERROR: Revoking session: %v", err))
            return
        }
    }

    http.Redirect(w, r, "/", http.StatusFound)
}
rishabhpoddar commented 1 month ago

Hi,

The above should work. What are the request and response headers from the api? Also, you don't need to make a logout API since you can use the frontend sdk (web-js) to call the revokeSession function.

I suspect that whilst you have setup the prebuiltui SDK, you have not setup the supertokens-web-js SDK on the frontend for other routes, which is needed to handle session management on the frontend. Have a look at our angular / vue guides from our docs to see how the supertokens-web-js SDK is setup.

gwd commented 1 month ago

Hey rp,

I hope this doesn't come off the wrong way, because I really appreciate how helpful you've been, and Supertokens does seem like a pretty solid piece of kit technically. But you guys seem to have a real blind spot when it comes to SSR. Yes, there are actually projects running today in 2024 that aren't SPAs, and don't render anything in the front-end whatsoever. The vast majority of my website is entirely server-side rendered, top to bottom, with JS only used to sprinkle in functionality like tooltips or what-not. I do have a handful of pages which do API fetches because it's not really practical implement the functionality I want any other way; but those are just hand-crafted "vanilla JS". (On those pages I do call supertokens.init(), and it works amazingly, thank you.)

There's quite a big impedance mismatch then, between what the documentation advises, and what I'm trying to do with it. For instance, the golang middleware function is only designed for APIs which return Javascript. But a lot of my routes return full HTML pages, for which I rather need to direct to a "refresh-tokens" JS snippet instead. I've basically had to write this middleware myself.

I've persevered, because I can see that it's a quality library, and once I get things connected properly everything will work great. (In fact, I was thinking of writing up a blog post for how to actually use supertokens with things rendered in the backend in golang.)

I understand that I'm not necessarily your target market, so I'm not necessarily complaining that the library and the documentation isn't built with me in mind. But I don't think it would be that much work to make it work much better for my use case -- mostly it takes tripping over little things like this (which you're not testing because I'm not your target market) and giving you feedback to improve things.

So with that in mind:

I implemented the logout function this way primarily because I'm replacing an existing infrastructure which did the same thing; after all, there's a subsection on "how to revoke a session on the server", so just c&p the code (with some adjustments) should be easier than adding yet another bit of hand-crafted javascript to the frond end.

I did realize that I could implement the logout in the frontend instead as a work-around. But as I said, it does seem like there are two ways in which the system you're offering is behaving in unexpected ways; and if I can get both of those tweaked, it will help the next person doing full SSR who comes along (and perhaps unlike me, doesn't want to do any JS in the front-end at all).

Also, I think that the "prebuiltui" page not checking the token with the front-end would still be a latent bug even if I did logout in the frontend: suppose someone implemented a feature to log out all sessions -- for instance, suppose a company had a policy that passwords had to change every 3 months, and that on a password change all existing sessions were revoked. Then imagine someone is logged in on two separate browsers A and B; and they reset their password on browser A, which removes the token in browser A. Then later they sit down at browser B, which has the stale token, and end up in a redirection loop as described above (the server saying, "You can't access this page because you're not logged in, go to the auth page" and the pre-built auth page saying, "You're already logged in, go back to the page you were trying to access").

And of course, sess.RevokeSession() really should delete the token from the front-end, and it's rather curious that it's not; switching to logging out on the front-end would just paper over that.

Let me turn on supertokens logging and see if that turns anything up.

Again, thank you for your library and for all your help! Hopefully the work I'm doing will help open up another market segment to you, rather than being pure overhead. 😃

gwd commented 1 month ago

Sorry that there doesn't be a good way to c&p the browser dev info here. The upshot is that the result of calling sess.RevokeSession() on the backend is that sAccessToken and sRefreshToken are removed from the client, but that st-last-access-token-update is not. The latter one seems to be the only token passed with the request to /auth which erroneously redirects back to / without logging in.

Going through the logs, it looks like st-last-access-token-update is set by the client. So presumably what needs to happen is that whatever checks that first checks to see if sAccessToken exists at all, and then check st-last-access-token-update to do whatever.

gwd commented 1 month ago

I've verified that the following work-around fixes the issue:

        if err := sess.RevokeSession(); err != nil {
            RenderError(w, r, http.StatusInternalServerError,
                "Hmm, sorry that didn't work",
                fmt.Sprintf("INTERNAL ERROR: Revoking session: %v", err))
            return
        }

        // Workaround for https://github.com/supertokens/prebuiltui/issues/4
        http.SetCookie(w, &http.Cookie{
            Name:     "st-last-access-token-update",
            Value:    "",
            Domain:   r.Host,
            Path:     "/",
            MaxAge:   -1,
            HttpOnly: true,
        })
rishabhpoddar commented 1 month ago

Hmm. You should not delete the st-last-access-token-update, but instead, delete the sFrontToken cookie. The way the library is designed is that it expects frontend JS to be running when you make API calls. The JS would add interceptors on the frontend would catch the response from the api calling RevokeSession. and clear the sFrontToken cookie. This would indicate to the pre built UI logic that the session no longer exists. The prebuiltui JS can't really check if the sAccessToken exists or not cause it can't access httpOnly cookies.

The way to properly solve this would be to override the frontend session.init in the prebuiltui init to check if a session exists by making an API call to the backend which does session verification and returns a boolean. This way, clearing only the sAccessToken would solve the problem and you won't have to do the hack you did.

You can implement this as follows on the frontend:

window.supertokensUISession.init({
    override: {
        functions: (oI) => {
            return {
                ...oI,
                doesSessionExist: async () => {
                    let response = await fetch("https://mybackend.com/get-session-info");
                    return response.ok
                },
                getAccessTokenPayloadSecurely: async () => {
                    let response = await fetch("https://mybackend.com/get-session-info");
                    if (response.ok) {
                        return response.json()
                    }
                    throw new Error("Session does not exist");
                }
            }
        }
    }
})

Now in your golang server, create an API like this:

func GetSessionInfo(w http.ResponseWriter, r *http.Request, dbcache *storehouse.Cache) {
    if sess, err := session.GetSession(r, w, nil); err != nil {
        if errors.As(err, &sesserrs.TryRefreshTokenError{}) {
            // Redirect to /refresh-token to refresh the token before we log out
            middlewares.RedirectTokenRefresh(w, r)
            return
        }
        if !errors.As(err, &sesserrs.UnauthorizedError{}) {
            // TODO: return a 401
        }
    } else {
        accessTokenPayload = session.GetAccessTokenPayload()
                // TODO: return a 200 JSON response with the access token payload
    }
}

This type of flow is not fully tested on our side, but i think it should be fine. Do try it out and let us know! Thanks.

rishabhpoddar commented 1 month ago

Since this is not really an issue with this repo, i am closing this issue, but feel free to continue this conversation here,