Skip to content

Commit 805fe70

Browse files
authored
It's compliant if you're on your first attempt to fetch a session token (#4957)
1 parent 6ed217c commit 805fe70

File tree

6 files changed

+184
-33
lines changed

6 files changed

+184
-33
lines changed

packages/cloud/src/AuthService.ts

Lines changed: 36 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import { RefreshTimer } from "./RefreshTimer"
1111
import { getUserAgent } from "./utils"
1212

1313
export interface AuthServiceEvents {
14+
"attempting-session": [data: { previousState: AuthState }]
1415
"inactive-session": [data: { previousState: AuthState }]
1516
"active-session": [data: { previousState: AuthState }]
1617
"logged-out": [data: { previousState: AuthState }]
@@ -26,7 +27,7 @@ type AuthCredentials = z.infer<typeof authCredentialsSchema>
2627

2728
const AUTH_STATE_KEY = "clerk-auth-state"
2829

29-
type AuthState = "initializing" | "logged-out" | "active-session" | "inactive-session"
30+
type AuthState = "initializing" | "logged-out" | "active-session" | "attempting-session" | "inactive-session"
3031

3132
const clerkSignInResponseSchema = z.object({
3233
response: z.object({
@@ -93,6 +94,7 @@ export class AuthService extends EventEmitter<AuthServiceEvents> {
9394
private credentials: AuthCredentials | null = null
9495
private sessionToken: string | null = null
9596
private userInfo: CloudUserInfo | null = null
97+
private isFirstRefreshAttempt: boolean = false
9698

9799
constructor(context: vscode.ExtensionContext, log?: (...args: unknown[]) => void) {
98100
super()
@@ -129,7 +131,7 @@ export class AuthService extends EventEmitter<AuthServiceEvents> {
129131
this.credentials.clientToken !== credentials.clientToken ||
130132
this.credentials.sessionId !== credentials.sessionId
131133
) {
132-
this.transitionToInactiveSession(credentials)
134+
this.transitionToAttemptingSession(credentials)
133135
}
134136
} else {
135137
if (this.state !== "logged-out") {
@@ -156,19 +158,32 @@ export class AuthService extends EventEmitter<AuthServiceEvents> {
156158
this.log("[auth] Transitioned to logged-out state")
157159
}
158160

159-
private transitionToInactiveSession(credentials: AuthCredentials): void {
161+
private transitionToAttemptingSession(credentials: AuthCredentials): void {
160162
this.credentials = credentials
161163

162164
const previousState = this.state
163-
this.state = "inactive-session"
165+
this.state = "attempting-session"
164166

165167
this.sessionToken = null
166168
this.userInfo = null
169+
this.isFirstRefreshAttempt = true
167170

168-
this.emit("inactive-session", { previousState })
171+
this.emit("attempting-session", { previousState })
169172

170173
this.timer.start()
171174

175+
this.log("[auth] Transitioned to attempting-session state")
176+
}
177+
178+
private transitionToInactiveSession(): void {
179+
const previousState = this.state
180+
this.state = "inactive-session"
181+
182+
this.sessionToken = null
183+
this.userInfo = null
184+
185+
this.emit("inactive-session", { previousState })
186+
172187
this.log("[auth] Transitioned to inactive-session state")
173188
}
174189

@@ -329,16 +344,27 @@ export class AuthService extends EventEmitter<AuthServiceEvents> {
329344
/**
330345
* Check if the user is authenticated
331346
*
332-
* @returns True if the user is authenticated (has an active or inactive session)
347+
* @returns True if the user is authenticated (has an active, attempting, or inactive session)
333348
*/
334349
public isAuthenticated(): boolean {
335-
return this.state === "active-session" || this.state === "inactive-session"
350+
return (
351+
this.state === "active-session" || this.state === "attempting-session" || this.state === "inactive-session"
352+
)
336353
}
337354

338355
public hasActiveSession(): boolean {
339356
return this.state === "active-session"
340357
}
341358

359+
/**
360+
* Check if the user has an active session or is currently attempting to acquire one
361+
*
362+
* @returns True if the user has an active session or is attempting to get one
363+
*/
364+
public hasOrIsAcquiringActiveSession(): boolean {
365+
return this.state === "active-session" || this.state === "attempting-session"
366+
}
367+
342368
/**
343369
* Refresh the session
344370
*
@@ -364,6 +390,9 @@ export class AuthService extends EventEmitter<AuthServiceEvents> {
364390
if (error instanceof InvalidClientTokenError) {
365391
this.log("[auth] Invalid/Expired client token: clearing credentials")
366392
this.clearCredentials()
393+
} else if (this.isFirstRefreshAttempt && this.state === "attempting-session") {
394+
this.isFirstRefreshAttempt = false
395+
this.transitionToInactiveSession()
367396
}
368397
this.log("[auth] Failed to refresh session", error)
369398
throw error

packages/cloud/src/CloudService.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ export class CloudService {
4040
this.authService = new AuthService(this.context, this.log)
4141
await this.authService.initialize()
4242

43+
this.authService.on("attempting-session", this.authListener)
4344
this.authService.on("inactive-session", this.authListener)
4445
this.authService.on("active-session", this.authListener)
4546
this.authService.on("logged-out", this.authListener)
@@ -89,6 +90,11 @@ export class CloudService {
8990
return this.authService!.hasActiveSession()
9091
}
9192

93+
public hasOrIsAcquiringActiveSession(): boolean {
94+
this.ensureInitialized()
95+
return this.authService!.hasOrIsAcquiringActiveSession()
96+
}
97+
9298
public getUserInfo(): CloudUserInfo | null {
9399
this.ensureInitialized()
94100
return this.authService!.getUserInfo()
@@ -152,6 +158,8 @@ export class CloudService {
152158

153159
public dispose(): void {
154160
if (this.authService) {
161+
this.authService.off("attempting-session", this.authListener)
162+
this.authService.off("inactive-session", this.authListener)
155163
this.authService.off("active-session", this.authListener)
156164
this.authService.off("logged-out", this.authListener)
157165
this.authService.off("user-info", this.authListener)

packages/cloud/src/SettingsService.ts

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@ import { RefreshTimer } from "./RefreshTimer"
1414
const ORGANIZATION_SETTINGS_CACHE_KEY = "organization-settings"
1515

1616
export class SettingsService {
17-
1817
private context: vscode.ExtensionContext
1918
private authService: AuthService
2019
private settings: OrganizationSettings | undefined = undefined
@@ -43,6 +42,10 @@ export class SettingsService {
4342
this.removeSettings()
4443
}
4544

45+
this.authService.on("attempting-session", () => {
46+
this.timer.start()
47+
})
48+
4649
this.authService.on("active-session", () => {
4750
this.timer.start()
4851
})
@@ -52,7 +55,7 @@ export class SettingsService {
5255
this.removeSettings()
5356
})
5457

55-
if (this.authService.hasActiveSession()) {
58+
if (this.authService.hasOrIsAcquiringActiveSession()) {
5659
this.timer.start()
5760
}
5861
}
@@ -120,5 +123,4 @@ export class SettingsService {
120123
public dispose(): void {
121124
this.timer.stop()
122125
}
123-
124126
}

packages/cloud/src/__tests__/AuthService.spec.ts

Lines changed: 114 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -173,17 +173,17 @@ describe("AuthService", () => {
173173
expect(loggedOutSpy).toHaveBeenCalledWith({ previousState: "initializing" })
174174
})
175175

176-
it("should transition to inactive-session when valid credentials exist", async () => {
176+
it("should transition to attempting-session when valid credentials exist", async () => {
177177
const credentials = { clientToken: "test-token", sessionId: "test-session" }
178178
mockContext.secrets.get.mockResolvedValue(JSON.stringify(credentials))
179179

180-
const inactiveSessionSpy = vi.fn()
181-
authService.on("inactive-session", inactiveSessionSpy)
180+
const attemptingSessionSpy = vi.fn()
181+
authService.on("attempting-session", attemptingSessionSpy)
182182

183183
await authService.initialize()
184184

185-
expect(authService.getState()).toBe("inactive-session")
186-
expect(inactiveSessionSpy).toHaveBeenCalledWith({ previousState: "initializing" })
185+
expect(authService.getState()).toBe("attempting-session")
186+
expect(attemptingSessionSpy).toHaveBeenCalledWith({ previousState: "initializing" })
187187
expect(mockTimer.start).toHaveBeenCalled()
188188
})
189189

@@ -213,13 +213,13 @@ describe("AuthService", () => {
213213
const newCredentials = { clientToken: "new-token", sessionId: "new-session" }
214214
mockContext.secrets.get.mockResolvedValue(JSON.stringify(newCredentials))
215215

216-
const inactiveSessionSpy = vi.fn()
217-
authService.on("inactive-session", inactiveSessionSpy)
216+
const attemptingSessionSpy = vi.fn()
217+
authService.on("attempting-session", attemptingSessionSpy)
218218

219219
onDidChangeCallback!({ key: "clerk-auth-credentials" })
220220
await new Promise((resolve) => setTimeout(resolve, 0)) // Wait for async handling
221221

222-
expect(inactiveSessionSpy).toHaveBeenCalled()
222+
expect(attemptingSessionSpy).toHaveBeenCalled()
223223
})
224224
})
225225

@@ -451,6 +451,26 @@ describe("AuthService", () => {
451451

452452
expect(authService.getSessionToken()).toBe("test-jwt")
453453
})
454+
455+
it("should return correct values for new methods", async () => {
456+
await authService.initialize()
457+
expect(authService.hasOrIsAcquiringActiveSession()).toBe(false)
458+
459+
// Create a new service instance with credentials (attempting-session)
460+
const credentials = { clientToken: "test-token", sessionId: "test-session" }
461+
mockContext.secrets.get.mockResolvedValue(JSON.stringify(credentials))
462+
463+
const attemptingService = new AuthService(mockContext as unknown as vscode.ExtensionContext, mockLog)
464+
await attemptingService.initialize()
465+
466+
expect(attemptingService.hasOrIsAcquiringActiveSession()).toBe(true)
467+
expect(attemptingService.hasActiveSession()).toBe(false)
468+
469+
// Manually set state to active-session for testing
470+
attemptingService["state"] = "active-session"
471+
expect(attemptingService.hasOrIsAcquiringActiveSession()).toBe(true)
472+
expect(attemptingService.hasActiveSession()).toBe(true)
473+
})
454474
})
455475

456476
describe("session refresh", () => {
@@ -497,7 +517,7 @@ describe("AuthService", () => {
497517
expect(authService.getState()).toBe("active-session")
498518
expect(authService.hasActiveSession()).toBe(true)
499519
expect(authService.getSessionToken()).toBe("new-jwt-token")
500-
expect(activeSessionSpy).toHaveBeenCalledWith({ previousState: "inactive-session" })
520+
expect(activeSessionSpy).toHaveBeenCalledWith({ previousState: "attempting-session" })
501521
expect(userInfoSpy).toHaveBeenCalledWith({
502522
userInfo: {
503523
name: "John Doe",
@@ -530,6 +550,82 @@ describe("AuthService", () => {
530550
await expect(timerCallback()).rejects.toThrow("Network error")
531551
expect(mockLog).toHaveBeenCalledWith("[auth] Failed to refresh session", expect.any(Error))
532552
})
553+
554+
it("should transition to inactive-session on first attempt failure", async () => {
555+
// Mock failed token creation response
556+
mockFetch.mockResolvedValue({
557+
ok: false,
558+
status: 500,
559+
statusText: "Internal Server Error",
560+
})
561+
562+
const inactiveSessionSpy = vi.fn()
563+
authService.on("inactive-session", inactiveSessionSpy)
564+
565+
// Verify we start in attempting-session state
566+
expect(authService.getState()).toBe("attempting-session")
567+
expect(authService["isFirstRefreshAttempt"]).toBe(true)
568+
569+
const timerCallback = vi.mocked(RefreshTimer).mock.calls[0][0].callback
570+
571+
await expect(timerCallback()).rejects.toThrow()
572+
573+
// Should transition to inactive-session after first failure
574+
expect(authService.getState()).toBe("inactive-session")
575+
expect(authService["isFirstRefreshAttempt"]).toBe(false)
576+
expect(inactiveSessionSpy).toHaveBeenCalledWith({ previousState: "attempting-session" })
577+
})
578+
579+
it("should not transition to inactive-session on subsequent failures", async () => {
580+
// First, transition to inactive-session by failing the first attempt
581+
mockFetch.mockResolvedValue({
582+
ok: false,
583+
status: 500,
584+
statusText: "Internal Server Error",
585+
})
586+
587+
const timerCallback = vi.mocked(RefreshTimer).mock.calls[0][0].callback
588+
await expect(timerCallback()).rejects.toThrow()
589+
590+
// Verify we're now in inactive-session
591+
expect(authService.getState()).toBe("inactive-session")
592+
expect(authService["isFirstRefreshAttempt"]).toBe(false)
593+
594+
const inactiveSessionSpy = vi.fn()
595+
authService.on("inactive-session", inactiveSessionSpy)
596+
597+
// Subsequent failure should not trigger another transition
598+
await expect(timerCallback()).rejects.toThrow()
599+
600+
expect(authService.getState()).toBe("inactive-session")
601+
expect(inactiveSessionSpy).not.toHaveBeenCalled()
602+
})
603+
604+
it("should clear credentials on 401 during first refresh attempt (bug fix)", async () => {
605+
// Mock 401 response during first refresh attempt
606+
mockFetch.mockResolvedValue({
607+
ok: false,
608+
status: 401,
609+
statusText: "Unauthorized",
610+
})
611+
612+
const loggedOutSpy = vi.fn()
613+
authService.on("logged-out", loggedOutSpy)
614+
615+
const timerCallback = vi.mocked(RefreshTimer).mock.calls[0][0].callback
616+
await expect(timerCallback()).rejects.toThrow()
617+
618+
// Should clear credentials (not just transition to inactive-session)
619+
expect(mockContext.secrets.delete).toHaveBeenCalledWith("clerk-auth-credentials")
620+
expect(mockLog).toHaveBeenCalledWith("[auth] Invalid/Expired client token: clearing credentials")
621+
622+
// Simulate credentials cleared event
623+
mockContext.secrets.get.mockResolvedValue(undefined)
624+
await authService["handleCredentialsChange"]()
625+
626+
expect(authService.getState()).toBe("logged-out")
627+
expect(loggedOutSpy).toHaveBeenCalledWith({ previousState: "attempting-session" })
628+
})
533629
})
534630

535631
describe("user info", () => {
@@ -654,16 +750,16 @@ describe("AuthService", () => {
654750
expect(loggedOutSpy).toHaveBeenCalledWith({ previousState: "initializing" })
655751
})
656752

657-
it("should emit inactive-session event", async () => {
753+
it("should emit attempting-session event", async () => {
658754
const credentials = { clientToken: "test-token", sessionId: "test-session" }
659755
mockContext.secrets.get.mockResolvedValue(JSON.stringify(credentials))
660756

661-
const inactiveSessionSpy = vi.fn()
662-
authService.on("inactive-session", inactiveSessionSpy)
757+
const attemptingSessionSpy = vi.fn()
758+
authService.on("attempting-session", attemptingSessionSpy)
663759

664760
await authService.initialize()
665761

666-
expect(inactiveSessionSpy).toHaveBeenCalledWith({ previousState: "initializing" })
762+
expect(attemptingSessionSpy).toHaveBeenCalledWith({ previousState: "initializing" })
667763
})
668764

669765
it("should emit active-session event", async () => {
@@ -701,7 +797,7 @@ describe("AuthService", () => {
701797
// Wait for async operations to complete
702798
await new Promise((resolve) => setTimeout(resolve, 0))
703799

704-
expect(activeSessionSpy).toHaveBeenCalledWith({ previousState: "inactive-session" })
800+
expect(activeSessionSpy).toHaveBeenCalledWith({ previousState: "attempting-session" })
705801
})
706802

707803
it("should emit user-info event", async () => {
@@ -803,7 +899,7 @@ describe("AuthService", () => {
803899
expect(mockTimer.stop).toHaveBeenCalled()
804900
})
805901

806-
it("should start timer on inactive-session transition", async () => {
902+
it("should start timer on attempting-session transition", async () => {
807903
const credentials = { clientToken: "test-token", sessionId: "test-session" }
808904
mockContext.secrets.get.mockResolvedValue(JSON.stringify(credentials))
809905

@@ -892,13 +988,13 @@ describe("AuthService", () => {
892988
const newCredentials = { clientToken: "new-token", sessionId: "new-session" }
893989
mockContext.secrets.get.mockResolvedValue(JSON.stringify(newCredentials))
894990

895-
const inactiveSessionSpy = vi.fn()
896-
service.on("inactive-session", inactiveSessionSpy)
991+
const attemptingSessionSpy = vi.fn()
992+
service.on("attempting-session", attemptingSessionSpy)
897993

898994
onDidChangeCallback!({ key: `clerk-auth-credentials-${customUrl}` })
899995
await new Promise((resolve) => setTimeout(resolve, 0)) // Wait for async handling
900996

901-
expect(inactiveSessionSpy).toHaveBeenCalled()
997+
expect(attemptingSessionSpy).toHaveBeenCalled()
902998
})
903999

9041000
it("should not respond to changes on different scoped keys", async () => {

src/services/mdm/MdmService.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -85,8 +85,8 @@ export class MdmService {
8585
return { compliant: true }
8686
}
8787

88-
// Check if cloud service is available and authenticated
89-
if (!CloudService.hasInstance() || !CloudService.instance.hasActiveSession()) {
88+
// Check if cloud service is available and has active or attempting session
89+
if (!CloudService.hasInstance() || !CloudService.instance.hasOrIsAcquiringActiveSession()) {
9090
return {
9191
compliant: false,
9292
reason: "Your organization requires Roo Code Cloud authentication. Please sign in to continue.",

0 commit comments

Comments
 (0)