Skip to content
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.

Sign up for GitHub

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(clerk-js): Handle new session pending status as authenticated state #5136

Merged
merged 17 commits into from
Feb 18, 2025
Merged

feat(clerk-js): Handle new session pending status as authenticated state #5136

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
ba87fc6
Add `pending` to `SessionStatus` union
LauraBeatris Feb 11, 2025
918bcd1
Plug pending session into main Client resource
LauraBeatris Feb 11, 2025
1a6dffb
Add test coverage for `pending` and `active` statuses
LauraBeatris Feb 12, 2025
99a782f
Update handshake tests
LauraBeatris Feb 12, 2025
7fe2081
Update device section to list for pending
LauraBeatris Feb 12, 2025
76cbf8d
Fix sign in account switcher to look for pending sessions
LauraBeatris Feb 12, 2025
523e920
Consider `pending` as authenticate state for client UAT
LauraBeatris Feb 12, 2025
a22328f
Save JWT token on Expo for pending session status
LauraBeatris Feb 12, 2025
cf8d19a
Add `isAuthenticated` property to main Clerk instance
LauraBeatris Feb 12, 2025
8c88903
Update `SignIn.SessionList` to include pending sessions
LauraBeatris Feb 12, 2025
0b7e5fe
Deprecate `activeSessions`
LauraBeatris Feb 12, 2025
6f3a0a7
Add changeset
LauraBeatris Feb 12, 2025
b344ea5
Introduce separate changesets
LauraBeatris Feb 13, 2025
a79b339
Refactor from `isAuthenticated` to `isSignedIn`
LauraBeatris Feb 17, 2025
760434f
Update changeset
LauraBeatris Feb 18, 2025
4ac18aa
Update clerk-js change to include deprecate property
LauraBeatris Feb 18, 2025
e69c175
Update `isSignedIn` to check against initialized session
LauraBeatris Feb 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions .changeset/nasty-mangos-live.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
---
'@clerk/elements': minor
'@clerk/shared': minor
'@clerk/astro': minor
'@clerk/clerk-react': minor
'@clerk/types': minor
'@clerk/clerk-expo': minor
'@clerk/vue': minor
---

Surface new `pending` session as a signed-in state
20 changes: 20 additions & 0 deletions .changeset/proud-cycles-roll.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
---
'@clerk/clerk-js': minor
---

- Initialize new `pending` session status as an signed-in state
- Deprecate `Clerk.client.activeSessions` in favor of `Clerk.client.signedInSessions`
- Introduce `Clerk.isSignedIn` property as an explicit signed-in state check, instead of `!!Clerk.session` or `!!Clerk.user`:

```ts
- if (Clerk.user) {
+ if (Clerk.isSignedIn) {
Comment on lines +10 to +11
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The isSignedIn property makes sense to me!

In the PR description, you mention that:

Clerk.user shouldn't be used to determine if the user has fully authenticated or not

But the logic in the Client resource tells me that isSignedIn is going to be true if the current session is active or pending, meaning that currently, isSignedIn cannot be used to check if the user is fully authenticated or not.

Is this intentional? If yes, could you please provide an example where Clerk.isSignedIn !== !!Clerk.user?

Copy link
Member Author

@LauraBeatris LauraBeatris Feb 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

meaning that currently, isSignedIn cannot be used to check if the user is fully authenticated or not.

The user has fully authenticated even with a pending status. Still, they have pending tasks to complete, eg: After sign-in, complete all factors but FAPI returns a pending session for tasks such as having to select an org.

Is this intentional? If yes, could you please provide an example where Clerk.isSignedIn !== !!Clerk.user

Currently, they are the same. This PR treats pending exactly like active to maintain current functionality and to incrementally add protections for it since the feature is gated on FAPI and toggled via Dashboard.

That property was added to avoid relying only on the user object, or Clerk.activeSessions.length > 0 for our internal "signed-in" state checks.

Once we introduce tasks (#5170) -> I'd add another property that specifically checks if the user has a session that resolved all pending tasks, something like:

// Would resolve to `false` for session.status === 'pending' 
// Would resolve to `true` for session.status === 'active'
if (Clerk.hasValidSession) { 
  clerk.mountUserButton(userbuttonDiv)
} else {
  clerk.mountSignIn(signInDiv)
}

As a syntax sugar so that developers don't have to manually check for the session statuses on custom flows as well.

Copy link
Member Author

@LauraBeatris LauraBeatris Feb 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually acknowledged that I misplaced "Clerk.user shouldn't be used to determine if the user has fully authenticated or not" statement and updated the changeset here

isSignedIn doesn't necessarily replace Clerk.user, but it does act like a syntax sugar to deprecate any manual references to Clerk.client.activeSessions.length > 0, !!Clerk.user or !!Clerk.session

// Mount user button component
document.getElementById('signed-in').innerHTML = `
<div id="user-button"></div>
`

const userbuttonDiv = document.getElementById('user-button')

clerk.mountUserButton(userbuttonDiv)
}
4 changes: 2 additions & 2 deletions packages/astro/src/stores/internal.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import type {
ActiveSessionResource,
Clerk,
ClientResource,
InitialState,
OrganizationResource,
SignedInSessionResource,
UserResource,
} from '@clerk/types';
import { atom, map } from 'nanostores';
Expand All @@ -12,7 +12,7 @@ export const $csrState = map<{
isLoaded: boolean;
client: ClientResource | undefined | null;
user: UserResource | undefined | null;
session: ActiveSessionResource | undefined | null;
session: SignedInSessionResource | undefined | null;
organization: OrganizationResource | undefined | null;
}>({
isLoaded: false,
Expand Down
2 changes: 1 addition & 1 deletion packages/clerk-js/src/core/__tests__/clerk.redirects.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ describe('Clerk singleton - Redirects', () => {
beforeEach(() => {
mockClientFetch.mockReturnValue(
Promise.resolve({
activeSessions: [],
signedInSessions: [],
}),
);
});
Expand Down
Loading