Skip to content

fix: force V1 auth when ecdhXprv is absent for addAccessToken - #9432

Open
danielpeng1 wants to merge 1 commit into
masterfrom
WCN-1790/fix-sso-access-token
Open

fix: force V1 auth when ecdhXprv is absent for addAccessToken#9432
danielpeng1 wants to merge 1 commit into
masterfrom
WCN-1790/fix-sso-access-token

Conversation

@danielpeng1

Copy link
Copy Markdown
Contributor

Problem

SSO users (Okta, Entra ID, etc.) were getting undefined when copying a newly created access token. This only affected SSO users whose account had an ECDH keychain registered, either because they originally signed up with a password, or had a support-initiated password reset.

Root cause

Commit c578250 (ANT-963) changed the condition that tells the server "return the token in plaintext" from:

if (!this._ecdhXprv)   // if I have no decryption key → ask for plaintext

to:

if (!this._ecdhXprv && !strategyAuthenticated)   // also bail if HMAC is active

Browser users always have an active HMAC session, so the plaintext request was never sent. The server then encrypted the token with the user's ECDH key. SSO users have no password → no decryption key → token came back as undefined.

Fix

Revert to the original condition: if (!this._ecdhXprv). HMAC request signing and ECDH response encryption are completely independent — whether the request is HMAC-signed has no bearing on whether the response token needs to be encrypted.

Ticket: WCN-1790

@danielpeng1 danielpeng1 self-assigned this Aug 5, 2026
@linear-code

linear-code Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

WCN-1790

@danielpeng1

Copy link
Copy Markdown
Contributor Author

@claude review, and verify if this is the correct fix, given the changes from the past commit referenced in the PR description

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Claude finished @danielpeng1's task —— View job


Code Review and Verification

I'll review this PR and verify if the fix correctly addresses the SSO access token issue.

Todo List:

  • Read repository guidelines (CLAUDE.md)
  • Examine the current changes in the PR
  • Investigate the referenced commit c578250 (ANT-963) that caused the issue
  • Analyze the logic to verify the fix is correct
  • Review test changes for proper coverage
  • Provide comprehensive review feedback

Analysis

This PR correctly fixes a critical bug in SSO access token generation. Here's my detailed analysis:

Problem Context

The bug affected SSO users (Okta, Entra ID, etc.) who had ECDH keychains registered but no password (hence no _ecdhXprv decryption key). When these users tried to copy newly created access tokens, they received undefined instead of the token.

Root Cause Analysis

The previous condition was:

if (!this._ecdhXprv && !strategyAuthenticated)

This meant that when:

  1. _ecdhXprv is absent (SSO user has no decryption key) ✓
  2. BUT strategyAuthenticated is true (browser has active HMAC session) ✓

The condition evaluated to false, so forceV1Auth was NOT set. The server then encrypted the token with the user's ECDH key, but the client had no _ecdhXprv to decrypt it with, resulting in undefined.

Fix Verification ✅

The fix correctly reverts to:

if (!this._ecdhXprv)

This is the right approach because:

  • ECDH response encryption and HMAC request signing are orthogonal concerns
  • forceV1Auth = true tells the server to return plaintext tokens (no ECDH encryption)
  • This should be set whenever the client lacks decryption capability, regardless of request signing method
  • SSO users need plaintext tokens since they have no password-derived decryption keys

Code Quality Assessment

Strengths:

  1. Clear problem identification - The PR description accurately explains the root cause
  2. Minimal, targeted fix - Only changes what's necessary
  3. Improved documentation - Better comments explain the HMAC/ECDH orthogonality
  4. Comprehensive test updates - Tests now properly verify the SSO scenario

Test Coverage Analysis:

  • ✅ Tests SSO users with authenticated HMAC strategy
  • ✅ Tests SSO users without authenticated HMAC strategy
  • ✅ Tests that handleTokenIssuance is not called when ecdhXprv is absent
  • ✅ Test descriptions are updated with regression test context

Security Considerations

This fix does not introduce security concerns:

  • V1 auth fallback is legitimate for users without decryption keys
  • HMAC request signing continues to work normally
  • Only affects token response format, not authentication strength

Recommendation

✅ APPROVED - This is the correct fix. The logic is sound, the implementation is clean, and the test coverage is thorough.

The fix properly addresses the specific SSO use case while maintaining backward compatibility and security.


Job run

@danielpeng1
danielpeng1 marked this pull request as ready for review August 5, 2026 19:51
@danielpeng1
danielpeng1 requested review from a team as code owners August 5, 2026 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants