fix logout #92

jacobcoro · 2023-02-14T05:53:55Z

What's happening:

When we click 'logout' new requests from SWR (like to subscriptions) get sent out, and the cookie that just got deleted resets again.

Also, if we redirect the use to the login page right after signing out, the browser still somehow has the cookie, and our middleware detects that the user is still logged in and redirects right back to the dashboard.

I tried calling supabase.auth.logOut on the server side but that doesn't actually work. because supabase uses jwt tokens, all the serverside logout does it say 'I wont accept refresh tokens' but doesn't invalidate the current token.

Why the swr calls still have a token after it was just deleted is a mystery to me.

references:

https://stackoverflow.com/questions/72972086/cannot-log-out-user-on-the-server-side-next-js-and-supabase

signOut not deleting cookie

Closedsupabase/gotrue-js · #46 · Issue opened by gomflo on Jan 17 2021

I then tried manually setting the cookie from the backend during the 'logout' call. That also did not work. (in the screenshot above, notice deleted and expires are set.

So it turns out doing a fully server-side log out is not possible with supabase as far as I can tell.

But if we do the logout page redirect, at least we should be able to redirect to the login page and not have it immediately sign back in

Tomorrow I will try redirecting the user to a logout page, that then redirects to login after signing out. We should probably still call the server logOut function even though it really doesnt do much.

Using a logout redirect page seemed to work, after fixing up some of the logic around 'profile' fetching. I think that the profile not getting set to null properly before logout might have been the culprit for the lingering fetch calls that re-validated the cookie.

I updated to use SWR for the profile fetching, which might fix some of the other bugs we've been seeing around signin status.

Solution

The way I finally solved the the 'inflight requests are resetting cookie' issue is:
calling window.location.href= "/logout" Then inside an empty pages/logout.tsx page, I call the auth.signOut function again and then finally redirect back to "/login" or "/signup"

The reason this works is that window.location.href will cancel in-flight requests, whereas router.push() in next.js will not.

also had to add an abort controller on one fetch request that I couldn't stop because its called from the top level _app.tsx
https://frontend-digest.com/cancelling-fetch-requests-in-react-applications-58a52a048e8e

vercel · 2023-02-14T05:53:58Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated
relay-supa-club	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Feb 15, 2023 at 9:01AM (UTC)

pullrequest

✅ This pull request was sent to the PullRequest network.

Check the status or cancel PullRequest code review here.

pullrequest

PullRequest Breakdown

Reviewable lines of change

+ 193
- 72

55% TSX
45% TypeScript

Type of change

Fix - These changes are likely to be fixing a bug or issue.

pullrequest

These changes—primarily related to logout—look good to me. I had a few questions and comments inline.

Ryan B

Was this helpful? Yes | No

Reviewers will be notified any time you reply to their comments or commit new changes.
You can also request a full follow-up review from your PullRequest dashboard.

pullrequest · 2023-02-14T06:27:49Z

middleware.ts

         */
-        '/((?!_next/static|_next/image|favicon.ico|assets/*|api/company/accept-invite*|api/company/create-employee*).*)',
+        '/((?!_next/static|_next/image|favicon.ico|assets/*|api/company/accept-invite*|api/company/create-employee*|logout/*).*)',


This is a bit unfortunate, since it likely won't be the last bug that gets created due to this approach. It's too bad Next doesn't seem to give many other options if you want to do the config approach, and it can't be from dynamic values.

If this list continues to grow, it might be worth handling these routes in the middleware function. As part of that, something should be done about the fact that this is tribal knowledge.

Meaning—other developers working on this code base have nothing telling them that adding a new "public / unauthed" route will break unless they know to come and add a regex to this line hiding at the bottom of the middleware file. How that gets addressed depends on the project conventions, but a comment in a well-placed location would at least help a little.

🔸 Reduce Future Bugs (Important)

Ryan B

good point. I made a note in the middleware jsdoc, although it isnt ideal, someone new will definitely have to have a glance at this file during onboarding

That sounds like a good improvement!

Ryan B

pullrequest · 2023-02-14T06:27:50Z

pages/api/profiles/index.ts

+    if (req.method === 'GET') {
+        const { id } = req.query as ProfileGetQuery;
+
+        if (!id) {


It's good to see this pattern of validation after using a type assertion continuing to be applied.

◽ Compliment

Ryan B

pullrequest · 2023-02-14T06:27:50Z

pages/api/profiles/index.ts

+        if (!id) {
+            return res.status(httpCodes.BAD_REQUEST).json({ message: 'missing id' });
+        }
+        try {


There are some opportunities for code reuse in this handler.

🔹 Code Reuse (Nice to have)

Ryan B

pullrequest · 2023-02-14T06:27:50Z

pages/logout/[email].tsx

@@ -0,0 +1,14 @@
+import { GetServerSidePropsContext } from 'next';
+import { logoutRedirect } from 'src/utils/auth';


What exactly is this route accomplishing that's different from just /logout?

🔸 Clarify Intent (Important)

Ryan B

redirecting to the login page with the old email pre-filled

pullrequest · 2023-02-14T06:27:50Z

pages/signup/index.tsx

    const [loading, setLoading] = useState(false);
    const [signupSuccess, setSignupSuccess] = useState(false);
-    const [initialLoad, setInitialLoad] = useState(true);


It's great to see this being removed.

◽ Compliment

Ryan B

pullrequest · 2023-02-14T06:27:50Z

src/components/layout.tsx


    useEffect(() => {
        // this fixes a bug where the profile is not loaded on the first page load when coming from signup
        if (!loading && !profile?.id) refreshProfile();
    }, [refreshProfile, profile, loading]);

-    const supabase = useSupabaseClient();


Awesome to see supabase being abstracted away from here into an auth layer.

◽ Compliment

Ryan B

pullrequest · 2023-02-14T06:27:50Z

src/utils/auth.ts

@@ -27,3 +48,23 @@ export const isCompanyOwnerOrRelayEmployee = async (req: NextApiRequest, res: Ne

    return isAdmin(profile?.role);
 };
+
+export const logoutRedirect = async (ctx: GetServerSidePropsContext, email?: string) => {


This is a nicely written function.

◽ Compliment

Ryan B

…tside

pullrequest

The reason this works is that window.location.href will cancel in-flight requests, whereas router.push() in next.js will not.

This is expected, since the router is just for client-side routing within the same document, whereas window.location.href is an actual navigation event in which a new document is loaded.

Ryan B

Was this helpful? Yes | No

Reviewers will be notified any time you reply to their comments or commit new changes.
You can also request a full follow-up review from your PullRequest dashboard.

pullrequest · 2023-02-14T13:52:31Z

src/hooks/use-user.tsx

-        await supabaseClient.auth.signOut();
-        setProfile(null);
+        const email = session?.user?.email;
+        // cannot use router.push() here because it won't cancel in-flight requests which wil re-set the cookie


Great comment here. I was just getting ready to say "why can't we use the router" as I saw it.

◽ Compliment

Ryan B

…tside

pullrequest

Hey @jacobcoro,
Was the feedback from PullRequest helpful? Yes | No

fix logout

27a27a5

vercel bot deployed to Preview February 14, 2023 05:54 View deployment

pullrequest bot reviewed Feb 14, 2023

View reviewed changes

allow id query from onboarding

5d649e3

vercel bot deployed to Preview February 14, 2023 06:09 View deployment

pullrequest bot reviewed Feb 14, 2023

View reviewed changes

kill persistent cookie

0b68bbc

vercel bot deployed to Preview February 14, 2023 09:21 View deployment

jacobcoro and others added 2 commits February 14, 2023 17:24

Merge branch 'main' into jacob/v2-72-logout-user-serverside-not-clien…

c0193d6

…tside

note on middleware use

312253e

vercel bot deployed to Preview February 14, 2023 09:26 View deployment

document nicer

461744e

vercel bot deployed to Preview February 14, 2023 09:44 View deployment

jacobcoro requested a review from 2woohoo February 14, 2023 09:56

pullrequest bot reviewed Feb 14, 2023

View reviewed changes

Merge branch 'main' into jacob/v2-72-logout-user-serverside-not-clien…

b3ae1b1

…tside

vercel bot deployed to Preview February 15, 2023 05:44 View deployment

2woohoo approved these changes Feb 15, 2023

View reviewed changes

Merge branch 'main' into jacob/v2-72-logout-user-serverside-not-clien…

f458d95

…tside

vercel bot deployed to Preview February 15, 2023 09:01 View deployment

jacobcoro merged commit 0491557 into main Feb 15, 2023

jacobcoro deleted the jacob/v2-72-logout-user-serverside-not-clientside branch February 15, 2023 09:06

pullrequest bot reviewed Feb 15, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix logout #92

fix logout #92

jacobcoro commented Feb 14, 2023 •

edited

Loading

vercel bot commented Feb 14, 2023 •

edited

Loading

pullrequest bot left a comment

pullrequest bot left a comment •

edited

Loading

pullrequest bot left a comment

pullrequest bot Feb 14, 2023

jacobcoro Feb 14, 2023

pullrequest bot Feb 14, 2023

pullrequest bot Feb 14, 2023

pullrequest bot Feb 14, 2023

pullrequest bot Feb 14, 2023

jacobcoro Feb 14, 2023

pullrequest bot Feb 14, 2023

pullrequest bot Feb 14, 2023

pullrequest bot Feb 14, 2023

pullrequest bot left a comment

pullrequest bot Feb 14, 2023

pullrequest bot left a comment

		@@ -0,0 +1,14 @@
		import { GetServerSidePropsContext } from 'next';
		import { logoutRedirect } from 'src/utils/auth';

fix logout #92

fix logout #92

Conversation

jacobcoro commented Feb 14, 2023 • edited Loading

Solution

vercel bot commented Feb 14, 2023 • edited Loading

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot left a comment • edited Loading

Choose a reason for hiding this comment

PullRequest Breakdown

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot Feb 14, 2023

Choose a reason for hiding this comment

🔸 Reduce Future Bugs (Important)

jacobcoro Feb 14, 2023

Choose a reason for hiding this comment

pullrequest bot Feb 14, 2023

Choose a reason for hiding this comment

pullrequest bot Feb 14, 2023

Choose a reason for hiding this comment

◽ Compliment

pullrequest bot Feb 14, 2023

Choose a reason for hiding this comment

🔹 Code Reuse (Nice to have)

pullrequest bot Feb 14, 2023

Choose a reason for hiding this comment

🔸 Clarify Intent (Important)

jacobcoro Feb 14, 2023

Choose a reason for hiding this comment

pullrequest bot Feb 14, 2023

Choose a reason for hiding this comment

◽ Compliment

pullrequest bot Feb 14, 2023

Choose a reason for hiding this comment

◽ Compliment

pullrequest bot Feb 14, 2023

Choose a reason for hiding this comment

◽ Compliment

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot Feb 14, 2023

Choose a reason for hiding this comment

◽ Compliment

pullrequest bot left a comment

Choose a reason for hiding this comment

jacobcoro commented Feb 14, 2023 •

edited

Loading

vercel bot commented Feb 14, 2023 •

edited

Loading

pullrequest bot left a comment •

edited

Loading