-
Notifications
You must be signed in to change notification settings - Fork 452
fix: [ENG-2941] tolerate missing body in auth:getState #709
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -13,7 +13,7 @@ export const AuthEvents = { | |||||||||||
| } as const | ||||||||||||
|
|
||||||||||||
| export interface AuthGetStateRequest { | ||||||||||||
| projectPath: string | ||||||||||||
| projectPath?: string | ||||||||||||
| } | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion (non-blocking, if-minor): Widening
Suggested change
|
||||||||||||
|
|
||||||||||||
| export interface AuthGetStateResponse { | ||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -82,6 +82,39 @@ function createTestBrvConfig(): BrvConfig { | |
|
|
||
| // ==================== Tests ==================== | ||
|
|
||
| function makeValidTokenStoreFixture(): ITokenStore { | ||
| return { | ||
| clear: stub().resolves(), | ||
| load: stub().resolves(createValidToken()), | ||
| save: stub().resolves(), | ||
| } as unknown as ITokenStore | ||
| } | ||
|
|
||
| function makeMissingTokenStoreFixture(): ITokenStore { | ||
| return { | ||
| clear: stub().resolves(), | ||
| load: stub().resolves(), | ||
| save: stub().resolves(), | ||
| } as unknown as ITokenStore | ||
| } | ||
|
|
||
| function makeExpiredTokenStoreFixture(): ITokenStore { | ||
| const expired = new AuthToken({ | ||
| accessToken: 'expired-access', | ||
| expiresAt: new Date(Date.now() - 60_000), | ||
| refreshToken: 'expired-refresh', | ||
| sessionKey: 'expired-session', | ||
| tokenType: 'Bearer', | ||
| userEmail: 'test@example.com', | ||
| userId: 'user-123', | ||
| }) | ||
| return { | ||
| clear: stub().resolves(), | ||
| load: stub().resolves(expired), | ||
| save: stub().resolves(), | ||
| } as unknown as ITokenStore | ||
| } | ||
|
|
||
| function createMockProviderConfigStore( | ||
| options: {isConnected?: boolean} = {}, | ||
| ): SinonStubbedInstance<IProviderConfigStore> { | ||
|
|
@@ -524,4 +557,62 @@ describe('AuthHandler — setupExternalAuthSync', () => { | |
| .to.be.lessThan(callOrder.indexOf('LOGIN_COMPLETED')) | ||
| }) | ||
| }) | ||
|
|
||
| describe('setupGetState', () => { | ||
| it('returns isAuthorized=true and skips brvConfig when body is undefined (TUI sends no body)', async () => { | ||
| createHandler({tokenStore: makeValidTokenStoreFixture()}) | ||
| const handler = transport._handlers.get(AuthEvents.GET_STATE)! | ||
|
|
||
|
|
||
| const result = await handler(undefined, 'client-1') | ||
|
|
||
| expect(result.isAuthorized).to.equal(true) | ||
| expect(result.user).to.deep.include({email: 'test@example.com', id: 'user-123'}) | ||
| expect(result.brvConfig).to.equal(undefined) | ||
| expect(result.authToken).to.have.property('accessToken', 'test-access-token') | ||
| expect(projectConfigStore.read.called, 'projectConfigStore.read should not be called without projectPath').to.be | ||
| .false | ||
| }) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. praise: Good ENG-2941 regression guard. The |
||
|
|
||
| it('returns full state including brvConfig when body has projectPath (WebUI happy path)', async () => { | ||
| createHandler({tokenStore: makeValidTokenStoreFixture()}) | ||
| const handler = transport._handlers.get(AuthEvents.GET_STATE)! | ||
|
|
||
| const result = await handler({projectPath: '/foo'}, 'client-1') | ||
|
|
||
| expect(result.isAuthorized).to.equal(true) | ||
| expect(result.brvConfig).to.deep.include({spaceId: 'space-1', teamId: 'team-1'}) | ||
| expect(projectConfigStore.read.calledOnceWith('/foo')).to.be.true | ||
| }) | ||
|
|
||
| it('returns isAuthorized=true and skips brvConfig when body is empty object', async () => { | ||
| createHandler({tokenStore: makeValidTokenStoreFixture()}) | ||
| const handler = transport._handlers.get(AuthEvents.GET_STATE)! | ||
|
|
||
| const result = await handler({}, 'client-1') | ||
|
|
||
| expect(result.isAuthorized).to.equal(true) | ||
| expect(result.brvConfig).to.equal(undefined) | ||
| expect(projectConfigStore.read.called).to.be.false | ||
| }) | ||
|
|
||
| it('returns isAuthorized=false when token is missing, regardless of body', async () => { | ||
| createHandler({tokenStore: makeMissingTokenStoreFixture()}) | ||
| const handler = transport._handlers.get(AuthEvents.GET_STATE)! | ||
|
|
||
|
|
||
| const result = await handler(undefined, 'client-1') | ||
|
|
||
| expect(result).to.deep.equal({isAuthorized: false}) | ||
| }) | ||
|
|
||
| it('returns isAuthorized=false when token is expired, regardless of body', async () => { | ||
| createHandler({tokenStore: makeExpiredTokenStoreFixture()}) | ||
| const handler = transport._handlers.get(AuthEvents.GET_STATE)! | ||
|
|
||
| const result = await handler({projectPath: '/foo'}, 'client-1') | ||
|
|
||
| expect(result).to.deep.equal({isAuthorized: false}) | ||
| }) | ||
| }) | ||
| }) | ||
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.
note (non-blocking): The fix here is exactly right for ENG-2941. Flagging for a follow-up (already acknowledged in the PR description): the broad
} catch { return {isAuthorized: false} }on lines 249–251 still converts any unexpected throw fromuserService.getCurrentUser(e.g. transient network error) into{isAuthorized: false}, which re-creates the exact "stuck on provider chooser" symptom this PR fixes — just on flaky networks instead of every startup. Worth narrowing the catch in a follow-up so token validity (the actual auth signal) is decoupled from user-info fetch failures. The newbroadcastAuthStateChanged(lines 117–122) already does this — broadcasts{isAuthorized: true}without user details ongetCurrentUserfailure — so the pattern is already in this file.