From 26f3347ad364fc45d7e1b660f1bb5327ee26ad6b Mon Sep 17 00:00:00 2001 From: Tanner Bennett Date: Tue, 7 Jul 2026 16:01:07 -0500 Subject: [PATCH 1/2] Attempt to recover OAuth credentials marked invalid instead of requiring a manual re-login MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Once refreshAccessToken persists AuthInfoState.Invalid, softRefreshOAuth and refreshAccessToken both skip all future refresh attempts, so a credential invalidated by a single transient 401 from the token endpoint stays broken forever and ClientManager.createClient shows the 'There was an error connecting to... Please log in again.' modal in every new window — even though the stored refresh token still works. Fixes the roach motel by attempting a refresh for invalid OAuth credentials (still bounded by the existing failed-refresh backoff) and marking them Valid again when the refresh succeeds. --- src/atlclients/authStore.test.ts | 89 ++++++++++++++++++++++---------- src/atlclients/authStore.ts | 23 ++++----- 2 files changed, 72 insertions(+), 40 deletions(-) diff --git a/src/atlclients/authStore.test.ts b/src/atlclients/authStore.test.ts index f1e4bc7a8..3c067f7ef 100644 --- a/src/atlclients/authStore.test.ts +++ b/src/atlclients/authStore.test.ts @@ -1169,8 +1169,7 @@ describe('CredentialManager', () => { expect(Logger.debug).toHaveBeenCalledWith(expect.stringContaining('permanent previous failure')); }); - it('should skip refreshAccessToken when OAuth credentials are already invalid', async () => { - const Logger = require('../logger').Logger; + it('should attempt a recovery refresh when OAuth credentials are marked invalid', async () => { const site = { ...mockJiraSite, isCloud: true }; const invalidOAuthInfo: OAuthInfo = { ...mockOAuthInfo, @@ -1182,46 +1181,82 @@ describe('CredentialManager', () => { const saveAuthInfoSpy = jest.spyOn(credentialManager as any, 'saveAuthInfo'); saveAuthInfoSpy.mockResolvedValue(true); - Logger.debug.mockClear(); - mockRefresher.getNewTokens.mockClear(); + mockRefresher.getNewTokens.mockResolvedValue({ + tokens: { + accessToken: 'new-access-token', + refreshToken: 'new-refresh-token', + expiration: Date.now() + 3600000, + receivedAt: Date.now(), + }, + shouldSlowDown: false, + shouldInvalidate: false, + }); - const result = await (credentialManager as any).refreshAccessToken(site); + await (credentialManager as any).refreshAccessToken(site); - expect(result).toBeUndefined(); - expect(mockRefresher.getNewTokens).not.toHaveBeenCalled(); - expect(saveAuthInfoSpy).not.toHaveBeenCalled(); - expect(Logger.debug).toHaveBeenCalledWith(expect.stringContaining('credentials are invalid')); + expect(mockRefresher.getNewTokens).toHaveBeenCalled(); + expect(saveAuthInfoSpy).toHaveBeenCalledWith( + site, + expect.objectContaining({ + state: AuthInfoState.Valid, + access: 'new-access-token', + }), + ); + }); + + it('should keep credentials invalid when the recovery refresh fails permanently', async () => { + const site = { ...mockJiraSite, isCloud: true }; + const invalidOAuthInfo: OAuthInfo = { + ...mockOAuthInfo, + state: AuthInfoState.Invalid, + }; + + const getAuthInfoSpy = jest.spyOn(credentialManager as any, 'getAuthInfoForProductAndCredentialId'); + getAuthInfoSpy.mockResolvedValue(invalidOAuthInfo); + const saveAuthInfoSpy = jest.spyOn(credentialManager as any, 'saveAuthInfo'); + saveAuthInfoSpy.mockResolvedValue(true); + mockRefresher.getNewTokens.mockResolvedValue({ + tokens: undefined, + shouldInvalidate: true, + shouldSlowDown: false, + }); + + await (credentialManager as any).refreshAccessToken(site); + + expect(saveAuthInfoSpy).toHaveBeenCalledWith( + site, + expect.objectContaining({ state: AuthInfoState.Invalid }), + ); const failedRefresh = (credentialManager as any)._failedRefreshCache.get(site.credentialId); expect(failedRefresh?.permanentFailure).toBe(true); }); - it('should skip token refresh when credentials state is Invalid', async () => { - const Logger = require('../logger').Logger; + it('should not retry a recovery refresh after a permanent failure in the same session', async () => { const site = { ...mockJiraSite, isCloud: true }; - const invalidOAuthInfo = { + const invalidOAuthInfo: OAuthInfo = { ...mockOAuthInfo, state: AuthInfoState.Invalid, }; - const getAuthInfoForProductSpy = jest.spyOn( - credentialManager as any, - 'getAuthInfoForProductAndCredentialId', - ); - getAuthInfoForProductSpy.mockResolvedValue(invalidOAuthInfo); - const softRefreshSpy = jest.spyOn(credentialManager as any, 'softRefreshOAuth'); - softRefreshSpy.mockImplementation(async (site: DetailedSiteInfo, authInfo: AuthInfo) => { - if (!authInfo || authInfo.state === AuthInfoState.Invalid) { - Logger.debug(`Skipping token refresh for ${site.baseApiUrl}; credentials are invalid.`); - return authInfo; - } - return authInfo; + const getAuthInfoSpy = jest.spyOn(credentialManager as any, 'getAuthInfoForProductAndCredentialId'); + getAuthInfoSpy.mockResolvedValue(invalidOAuthInfo); + const saveAuthInfoSpy = jest.spyOn(credentialManager as any, 'saveAuthInfo'); + saveAuthInfoSpy.mockResolvedValue(true); + + mockRefresher.getNewTokens.mockResolvedValue({ + tokens: undefined, + shouldInvalidate: true, + shouldSlowDown: false, }); - const result = await credentialManager.getAuthInfo(site); + await (credentialManager as any).refreshAccessToken(site); - expect(Logger.debug).toHaveBeenCalledWith(expect.stringContaining('credentials are invalid')); - expect(result?.state).toBe(AuthInfoState.Invalid); + mockRefresher.getNewTokens.mockClear(); + const result = await (credentialManager as any).refreshAccessToken(site); + + expect(result).toBeUndefined(); + expect(mockRefresher.getNewTokens).not.toHaveBeenCalled(); }); }); }); diff --git a/src/atlclients/authStore.ts b/src/atlclients/authStore.ts index 49c4955c9..50a4fa815 100644 --- a/src/atlclients/authStore.ts +++ b/src/atlclients/authStore.ts @@ -394,14 +394,13 @@ export class CredentialManager implements Disposable { return authInfo; // not an OAuth info, no need to refresh } - if (credentials.state === AuthInfoState.Invalid) { - Logger.debug(`Skipping token refresh for ${site.baseApiUrl}; credentials are invalid.`); - return credentials; - } - const GRACE_PERIOD = 30 * Time.MINUTES; - if (credentials.expirationDate) { + if (credentials.state === AuthInfoState.Invalid) { + // Credentials may have been invalidated spuriously (e.g. a transient 401 from the token + // endpoint), so attempt a refresh anyway; a successful one marks them Valid again. + Logger.debug(`Credentials for ${site.baseApiUrl} are marked invalid; attempting to recover.`); + } else if (credentials.expirationDate) { const diff = credentials.expirationDate - Date.now(); Logger.debug( `${Math.floor(diff / 1000)} seconds remaining for ${site.name} refresh token. ${diff > GRACE_PERIOD ? 'No refresh needed yet.' : 'refreshing...'}`, @@ -567,13 +566,9 @@ export class CredentialManager implements Disposable { } if (credentials.state === AuthInfoState.Invalid) { - Logger.debug(`Skipping token refresh for credentialID: ${site.credentialId}; credentials are invalid.`); - this._failedRefreshCache.set(site.credentialId, { - attemptsCount: this._failedRefreshCache.get(site.credentialId)?.attemptsCount ?? 0, - lastAttemptAt: new Date(), - permanentFailure: true, - }); - return undefined; + Logger.debug( + `Credentials for credentialID: ${site.credentialId} are marked invalid; attempting to recover.`, + ); } const failedRefresh = this._failedRefreshCache.get(site.credentialId); @@ -618,6 +613,8 @@ export class CredentialManager implements Disposable { credentials.refresh = newTokens.refreshToken; credentials.iat = newTokens.iat ?? 0; } + // A successful refresh proves the credentials work, even if they were marked invalid before + credentials.state = AuthInfoState.Valid; await this.saveAuthInfo(site, credentials); if (this._failedRefreshCache.has(site.credentialId)) { From ef26512cfd7a2121c1ada5e4f528aa6a8f72d9eb Mon Sep 17 00:00:00 2001 From: Tanner Bennett Date: Tue, 14 Jul 2026 01:45:35 -0500 Subject: [PATCH 2/2] Add CHANGELOG entry for OAuth credential recovery --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 69337d08a..fcf124467 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### Bug Fixes +- **Authentication**: OAuth credentials that were previously marked invalid are now re-validated by attempting a token refresh instead of being skipped forever. A credential invalidated by a transient token-endpoint error now self-heals on the next refresh, so users no longer see a persistent "There was an error connecting to ... Please log in again." message in every new window despite still being signed in. - **RovoDev**: Hid stack traces, stderr, and log details from external users while preserving them for Atlassian users. - **RovoDev (BBY)**: Fixed `ROVODEV_REBRAND_JCA` env var handling so the "Jira Coding Agent" rebrand works correctly in webviews. - **Notifications**: Fixed `atlassianNotificationNotifier` to correctly flush all promise levels, resolving a test reliability issue.