clerk / javascript

Official Javascript repository for Clerk authentication
https://clerk.com
MIT License
1.04k stars 235 forks source link

Session revocation has no effect for client side state. #874

Closed cy6eria closed 1 year ago

cy6eria commented 1 year ago

Package + Version

Version:

4.11.1

Browser/OS

Chrome latest

Description

For example, I have this middleware. In case of user doesn't have an 'admin' role, I want to revoke his session.

import { withClerkMiddleware, getAuth, clerkClient } from '@clerk/nextjs/server';
import { NextResponse } from 'next/server';
import type { NextRequest } from 'next/server';

export default withClerkMiddleware(async (req: NextRequest) => {
   const { userId, sessionId } = getAuth(req);

    if (userId && sessionId) {
        const user = await clerkClient.users.getUser(userId);

        if (user.privateMetadata.role !== 'admin') {
            await clerkClient.sessions.revokeSession(sessionId);

            const session = await clerkClient.sessions.getSession(sessionId);

            console.log(session.status); // getting 'revoked'
        }
    }

    return NextResponse.next();
});

// Stop Middleware running on static files
export const config = { matcher: '/((?!.*\\.).*)' }

In my component I get a session data with useSession hook and check current user's sessions status and get 'active'.

import React from 'react';
import { useSession, SignIn } from '@clerk/nextjs';

export const IsAdmin = ({ children }) => {
    const { session } = useSession();

    return session?.status === 'active' ? <>{children}</> : <SignIn />;
}
dimkl commented 1 year ago

hello @cy6eria. thank you for reporting this. I will take a look and come back to you with more information.

dimkl commented 1 year ago

@cy6eria do you use SSR ?

cy6eria commented 1 year ago

@cy6eria do you use SSR ?

Yes

dimkl commented 1 year ago

First of all i would suggest to avoid mutating the user/organisation/session resources in the middleware level. Instead of revoking the session i would advise to change the isAdmin() policy you use to check for admin role:

import React from 'react';
import { useUser, SignIn } from '@clerk/nextjs';

export const IsAdmin = ({ children }) => {
    const { user } = useUser();
    const hasAdminRole = user.privateMetadata.role === 'admin';
    return hasAdminRole ?  <>{children}</> : <SignIn />;
}

I think you don't have access to privateMetadata in this part of the code. I will check it out and provide with a better example. I will also investigate why the session is not updated in the SSR case.

cy6eria commented 1 year ago

First of all i would suggest to avoid mutating the user/organisation/session resources in the middleware level. Instead of revoking the session i would advise to change the isAdmin() policy you use to check for admin role:

import React from 'react';
import { useUser, SignIn } from '@clerk/nextjs';

export const IsAdmin = ({ children }) => {
    const { user } = useUser();
    const hasAdminRole = user.privateMetadata.role === 'admin';
    return hasAdminRole ?  <>{children}</> : <SignIn />;
}

I think you don't have access to privateMetadata in this part of the code. I will check it out and provide with a better example. I will also investigate why the session is not updated in the SSR case.

I'm 100% sure that I don't.

dimkl commented 1 year ago

What version of nextJS are you using? Could you provide a sample project?

cy6eria commented 1 year ago

@dimkl you can find my app here https://github.com/cy6eria/admin.cy6eria.ru (clerk branch).

dimkl commented 1 year ago

The issue is more complex that i thought. It is related to the client side routing and the session revocation being triggered in the server. We will investigate it further and come back to you. One hacky way to avoid this is to redirect (full page reload) the user to the target page after sign-in instead.

dimkl commented 1 year ago

In your implementation there are various of issues with using ClerkJS. Some of which are:

  1. middleware config matcher should be updated to exclude nextJS prefetching routes export const config = { matcher: '/((?!_next/image|_next/static|favicon.ico).*)',};
  2. mutating (eg revoking) session in a XHR request cannot update the current context of your application in the browser

In your current implementation after sign-in requests to _next/* are triggered which cause the session to be revoked since there are not excluded from the middleware whitelist. The session revocation does not update the browser causing an inconsistent behaviour to the logs of browser and server. In the repo provided you can see logs of isAdmin in server (which are always undefined since the session resource is not prefetched) and in client (which session.status === 'active' because it uses the session populated from after sign-in).

The use case of using a role for authorization is valid and you could use 2 different approaches to handle your case:

  1. customize session token to include the role using Sessions > Customize session token in Clerk Dashboard
  2. set role as user.public_metadata (accessible as readonly both in browser & server) and add use the isAdmin check to validate against role instead of revoking the session. (recommended)

Here you can find more information about user metadata.

Even though we currently don't have an example for role based access control, i think you could check this example from our docs for "protecting your pages": https://beta-docs.clerk.com/quickstarts/next/next-beta#protecting-your-pages

@cy6eria WDYT ?

cy6eria commented 1 year ago

Hi @dimikl. Sorry for long time response. I decided to try use organizations for this case (I want to use the same userbase for two apps). I think it should works like solution with a public metadata. But I got an issue with this approach. I used hooks use Auth and use Organization. When I loged in as a user from the organization I receive no data about that during SSR and on client side. What do I do wrong?

dimkl commented 1 year ago

@cy6eria This is a different issue. I will close the current Github issue since there seems to be no problem. For your last question, since it's not related with the current issue, could you ask in our discord channel and provide the necessary information?