-
Notifications
You must be signed in to change notification settings - Fork 327
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(nextjs): Handle URL <> Session Org Mismatch in Middleware #3977
Conversation
🦋 Changeset detectedLatest commit: bc89068 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
28ece32
to
8dc238f
Compare
cf8ad6c
to
509ada1
Compare
509ada1
to
54e6752
Compare
cff9d48
to
d724695
Compare
d6b3f6b
to
5c575ee
Compare
42612d2
to
aa84f66
Compare
I'll admit, i'm a bit fuzzy here on why it wasn't a dependency before, and also as to why it's only a dev dependency.
Turns out someone in the call chain was redirecting before we hit handshake. That's great!
integration/tests/handshake.test.ts
Outdated
@@ -72,7 +73,7 @@ test.describe('Client handshake @generic', () => { | |||
await new Promise<void>(resolve => jwksServer.close(() => resolve())); | |||
}); | |||
|
|||
test('Test standard signed-in - dev', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My IDE automatically deleted all of these Test
prefixes, citing this:
ESLint: should not have duplicate prefix(playwright/valid-title)
I can add them back if we like them though!
* @returns {HandshakeState | SignedOutState | null} - The function can return the following: | ||
* - {HandshakeState}: If a handshake is needed to resolve the mismatched organization. | ||
* - {SignedOutState}: If a handshake is required but cannot be performed. | ||
* - {null}: If no action is required. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @returns {HandshakeState | SignedOutState | null} - The function can return the following: | |
* - {HandshakeState}: If a handshake is needed to resolve the mismatched organization. | |
* - {SignedOutState}: If a handshake is required but cannot be performed. | |
* - {null}: If no action is required. | |
*/ | |
* @returns {HandshakeState | SignedOutState | null} - The function can return the following: | |
* - {HandshakeState}: If a handshake is needed to resolve the mismatched organization. | |
* - {SignedOutState}: If a handshake is required but cannot be performed. | |
* - {null}: If no action is required. | |
*/ |
We can remove the @returns
tag here since TypeScript already handles that for us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a better place for me to give details on what each of those return types mean?
Co-authored-by: Laura Beatris <48022589+LauraBeatris@users.noreply.github.com>
Co-authored-by: Laura Beatris <48022589+LauraBeatris@users.noreply.github.com>
9738a97
to
5ce428d
Compare
* WARNING: If the organization cannot be activated either because it does not exist or the user lacks access, | ||
* organization-related fields will be set to null. The server component must detect this and respond | ||
* with an appropriate error (e.g., notFound()). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is nice to have here, but it definitely needs to reach the customer facing documentation that we have as well.
packages/backend/src/tokens/types.ts
Outdated
organizationPatterns?: Array<Pattern>; | ||
|
||
/** | ||
* URL patterns for resources in the context of a clerk personal workspace (user-specific, outside any organization). | ||
* If the route also matches the organizationPattern, this takes precedence. | ||
* | ||
* Common examples: | ||
* - ["/user", "/user/(.*)"] | ||
* - ["/user/:any", "/user/:any/(.*)"] | ||
*/ | ||
personalWorkspacePatterns?: Array<Pattern>; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe there is a benefit of redefining those for the nextjs
package and take advantage of our RouteMatcherWithNextTypedRoutes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And just to double check here, we don't wanna use the createRouteMatcher
pattern here, because we are not expecting for folks to use more than one path ? Although the name of the properties hints that you can. If not, I think supporting the route matcher here makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea! I didn't realize we already have RouteMatcherParams, which serves the same purpose.
It looks like there's some duplication between the astro and nextjs RouteMatcherParams. My inclination here is to make the astro version (which doesn't depend on anything astro-specific) shared, and import that for use here. But let me know if you think another path would be better!
r/e createRouteMatcher - We could take a createRouteMatcher-returned function here too, but my inclination is to start with just accepting the pattern syntax for simplicity and expand the the more complex types if necessary in the future - I explored that a bit here (internal only): https://www.notion.so/Sync-Org-from-URL-to-Session-via-Middleware-f41f00865390480ab2279391c5625b27?d=f3738cb7f62c48bb90f8924c653a83a9&pvs=4#c0b57bf937694dd6bf510aeb4dde12af. Again, differing opinions welcome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh you know, digging in a bit further, I think the main difference here between this and RouteMatcherWithNextTypedRoutes
/RouteMatcherRoutes
is that they're geared at returning true/false, not pulling information out of the path. For the organization pattern, we need it to include a group (which I have here as :slug
or :id
). I don't see a clear way to extend the RouteMatcherParams to encompass that without making the type too complex for my taste. I could have the personal workspace pattern use RouteMatcherParams, but I think the mixed types would be worse DX.
And to answer your question:
we are not expecting for folks to use more than one path ?
I am expecting more than one path! It's very tricky to get most practical examples down to just one path expression, so I think allowing multiples is the way to go.
@@ -885,3 +886,524 @@ test.describe('Client handshake @generic', () => { | |||
expect(res.status).toBe(200); | |||
}); | |||
}); | |||
|
|||
test.describe('Client handshake with organization activation @nextjs', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing we've done in the past is pass options via request headers to avoid extra app overhead. Maybe this is something we can also do here?
Thanks @panteliselef! #3977 (comment)
Responding to #3977 (comment). Thanks @brkalow !
Thanks @brkalow! #3977 (comment)
Thanks @brkalow! #3977 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
Co-authored-by: Laura Beatris <48022589+LauraBeatris@users.noreply.github.com>
What problem is this solving?
Today, when developers use organization slugs in URLs, they need to go through contortions to be able to depend on the organization data in the clerk session, especially in server-rendered pages. In short, they need to detect the mismatch between the URL and the session on the server component, punt back to client-side javascript that can call setActive for the org as specified by the URL, and then re-render the server component. See some of that workflow here: https://clerk.com/docs/guides/force-organizations#set-an-active-organization-based-on-the-url
What changed?
This PR introduces a new NextJS middleware option that allows developers to specify their URL structure to the middleware - specifically which url patterns indicate a desire to activate the personal workspace or an organization, and if so which one (by slug or ID).
The middleware then checks the actual URL against that pattern, and if it detects a mismatch between the desired-active organization (from the url) and the actually-active organization (from the session), it performs a handshake with a new query param, which will activate the new organization.
What does the new API look like?
Imagine an application that supports organizations and the personal workspace, but includes both in their URLs. The might have URLs like:
/orgs/bcorp
/orgs/bcorp/settings
/personal-workspace/home
Currently, that application would require special handling in both server and client javascript to detect and resolve an organization mismatch.
Now, they can add middleware config like this:
What's next?
Further Reading:
Clerk internal DX guide: https://www.notion.so/clerkdev/Sync-Org-from-URL-to-Session-via-Middleware-f41f00865390480ab2279391c5625b27?pvs=4