From e1d3e604a5e71eb16422eca6440eca4cad421742 Mon Sep 17 00:00:00 2001 From: Gasan Guseinov Date: Tue, 16 Jul 2024 13:12:26 +0200 Subject: [PATCH 01/10] create session if refresh fails Signed-off-by: Gasan Guseinov --- .../RefreshingAuthSessionManager.ts | 37 ++++++++++++++----- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.ts b/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.ts index 216409a528..b6ce65cccd 100644 --- a/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.ts +++ b/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.ts @@ -64,6 +64,21 @@ export class RefreshingAuthSessionManager implements SessionManager { } async getSession(options: GetSessionOptions): Promise { + // eslint-disable-next-line consistent-this + const that = this; + const createSession = async () => { + // We can call authRequester multiple times, the returned session will contain all requested scopes. + that.currentSession = await that.connector.createSession({ + ...options, + scopes: that.helper.getExtendedScope( + that.currentSession, + options.scopes, + ), + }); + that.stateTracker.setIsSignedIn(true); + return that.currentSession; + }; + if ( this.helper.sessionExistsAndHasScope(this.currentSession, options.scopes) ) { @@ -83,10 +98,17 @@ export class RefreshingAuthSessionManager implements SessionManager { } return refreshedSession; } catch (error) { - if (options.optional) { - return undefined; + try { + return createSession(); + } catch (err) { + if (options.optional) { + return undefined; + } + throw new Error( + `Cannot create a new session after failing to refresh the session. Session refresh error: ${error}`, + err, + ); } - throw error; } } @@ -113,13 +135,8 @@ export class RefreshingAuthSessionManager implements SessionManager { return undefined; } - // We can call authRequester multiple times, the returned session will contain all requested scopes. - this.currentSession = await this.connector.createSession({ - ...options, - scopes: this.helper.getExtendedScope(this.currentSession, options.scopes), - }); - this.stateTracker.setIsSignedIn(true); - return this.currentSession; + return createSession(); + // return this.currentSession; } async removeSession() { From 29f7d6280638c655cb53e5a35b9e97d7e2ba45f5 Mon Sep 17 00:00:00 2001 From: Gasan Guseinov Date: Tue, 16 Jul 2024 13:17:34 +0200 Subject: [PATCH 02/10] udpate Signed-off-by: Gasan Guseinov --- .../src/lib/AuthSessionManager/RefreshingAuthSessionManager.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.ts b/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.ts index b6ce65cccd..6f650fa52a 100644 --- a/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.ts +++ b/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.ts @@ -136,7 +136,6 @@ export class RefreshingAuthSessionManager implements SessionManager { } return createSession(); - // return this.currentSession; } async removeSession() { From 2a951d6fe57973b189d40432bf99dff23ac411c7 Mon Sep 17 00:00:00 2001 From: Gasan Guseinov Date: Fri, 26 Jul 2024 17:44:26 +0200 Subject: [PATCH 03/10] update Signed-off-by: Gasan Guseinov --- .../src/lib/AuthSessionManager/RefreshingAuthSessionManager.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.ts b/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.ts index 6f650fa52a..72c4687fd6 100644 --- a/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.ts +++ b/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.ts @@ -106,7 +106,7 @@ export class RefreshingAuthSessionManager implements SessionManager { } throw new Error( `Cannot create a new session after failing to refresh the session. Session refresh error: ${error}`, - err, + { cause: err }, ); } } From 2f56fd30e8e6f2185d72e5ba2a4ad3fe1d447dd6 Mon Sep 17 00:00:00 2001 From: Gasan Guseinov Date: Fri, 30 Aug 2024 16:33:19 +0200 Subject: [PATCH 04/10] code from master Signed-off-by: Gasan Guseinov --- .../RefreshingAuthSessionManager.ts | 36 ++++++------------- .../src/stages/publish/googleStorage.ts | 1 + 2 files changed, 11 insertions(+), 26 deletions(-) diff --git a/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.ts b/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.ts index 72c4687fd6..216409a528 100644 --- a/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.ts +++ b/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.ts @@ -64,21 +64,6 @@ export class RefreshingAuthSessionManager implements SessionManager { } async getSession(options: GetSessionOptions): Promise { - // eslint-disable-next-line consistent-this - const that = this; - const createSession = async () => { - // We can call authRequester multiple times, the returned session will contain all requested scopes. - that.currentSession = await that.connector.createSession({ - ...options, - scopes: that.helper.getExtendedScope( - that.currentSession, - options.scopes, - ), - }); - that.stateTracker.setIsSignedIn(true); - return that.currentSession; - }; - if ( this.helper.sessionExistsAndHasScope(this.currentSession, options.scopes) ) { @@ -98,17 +83,10 @@ export class RefreshingAuthSessionManager implements SessionManager { } return refreshedSession; } catch (error) { - try { - return createSession(); - } catch (err) { - if (options.optional) { - return undefined; - } - throw new Error( - `Cannot create a new session after failing to refresh the session. Session refresh error: ${error}`, - { cause: err }, - ); + if (options.optional) { + return undefined; } + throw error; } } @@ -135,7 +113,13 @@ export class RefreshingAuthSessionManager implements SessionManager { return undefined; } - return createSession(); + // We can call authRequester multiple times, the returned session will contain all requested scopes. + this.currentSession = await this.connector.createSession({ + ...options, + scopes: this.helper.getExtendedScope(this.currentSession, options.scopes), + }); + this.stateTracker.setIsSignedIn(true); + return this.currentSession; } async removeSession() { diff --git a/plugins/techdocs-node/src/stages/publish/googleStorage.ts b/plugins/techdocs-node/src/stages/publish/googleStorage.ts index 3eac9834d2..e0da6ab528 100644 --- a/plugins/techdocs-node/src/stages/publish/googleStorage.ts +++ b/plugins/techdocs-node/src/stages/publish/googleStorage.ts @@ -348,6 +348,7 @@ export class GoogleGCSPublish implements PublisherBase { * can be used to verify if there are any pre-generated docs available to serve. */ async hasDocsBeenGenerated(entity: Entity): Promise { + Promise.resolve(); return new Promise(resolve => { const entityTriplet = `${entity.metadata.namespace}/${entity.kind}/${entity.metadata.name}`; const entityDir = this.legacyPathCasing From 979cf5d7d89f07d1f686d29b52f99aa2e332d933 Mon Sep 17 00:00:00 2001 From: Gasan Guseinov Date: Thu, 28 Nov 2024 11:19:30 +0100 Subject: [PATCH 05/10] create session if refresh did not succeed Signed-off-by: Gasan Guseinov --- .../lib/AuthSessionManager/RefreshingAuthSessionManager.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.ts b/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.ts index 216409a528..1978e14d2d 100644 --- a/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.ts +++ b/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.ts @@ -83,10 +83,7 @@ export class RefreshingAuthSessionManager implements SessionManager { } return refreshedSession; } catch (error) { - if (options.optional) { - return undefined; - } - throw error; + // If the refresh attempt fails we assume we don't have a session, so continue to create one. } } From a6334098a207687bc681e1a821168cf55fbeeeea Mon Sep 17 00:00:00 2001 From: Gasan Guseinov Date: Thu, 28 Nov 2024 11:21:09 +0100 Subject: [PATCH 06/10] update Signed-off-by: Gasan Guseinov --- plugins/techdocs-node/src/stages/publish/googleStorage.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/plugins/techdocs-node/src/stages/publish/googleStorage.ts b/plugins/techdocs-node/src/stages/publish/googleStorage.ts index e0da6ab528..3eac9834d2 100644 --- a/plugins/techdocs-node/src/stages/publish/googleStorage.ts +++ b/plugins/techdocs-node/src/stages/publish/googleStorage.ts @@ -348,7 +348,6 @@ export class GoogleGCSPublish implements PublisherBase { * can be used to verify if there are any pre-generated docs available to serve. */ async hasDocsBeenGenerated(entity: Entity): Promise { - Promise.resolve(); return new Promise(resolve => { const entityTriplet = `${entity.metadata.namespace}/${entity.kind}/${entity.metadata.name}`; const entityDir = this.legacyPathCasing From 5ddc0fedb041ca7eeb77bb03e33ffa0157d60ddd Mon Sep 17 00:00:00 2001 From: Gasan Guseinov Date: Thu, 28 Nov 2024 11:31:11 +0100 Subject: [PATCH 07/10] changeset Signed-off-by: Gasan Guseinov --- .changeset/tame-kings-eat.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/tame-kings-eat.md diff --git a/.changeset/tame-kings-eat.md b/.changeset/tame-kings-eat.md new file mode 100644 index 0000000000..814cebf73c --- /dev/null +++ b/.changeset/tame-kings-eat.md @@ -0,0 +1,5 @@ +--- +'@backstage/core-app-api': minor +--- + +create session if refresh session fails From 7bf209736d86d3f8c99ab6d571cd3c62d7dbfe14 Mon Sep 17 00:00:00 2001 From: Gasan Guseinov Date: Mon, 27 Jan 2025 23:51:03 +0100 Subject: [PATCH 08/10] 1. create session if not instant popup if refresh fails 2. add optimization to not call refresh session twice 3. add unit test Signed-off-by: Gasan Guseinov --- .../RefreshingAuthSessionManager.test.ts | 22 +++++++++++++ .../RefreshingAuthSessionManager.ts | 33 +++++++++++++++++-- 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.test.ts b/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.test.ts index 9afc710bdd..302eb82de0 100644 --- a/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.test.ts +++ b/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.test.ts @@ -275,4 +275,26 @@ describe('RefreshingAuthSessionManager', () => { expect(refreshSession).toHaveBeenCalledTimes(1); expect(createSession).toHaveBeenCalledTimes(1); }); + + it('should create a new session if refresh fails with existing expired session', async () => { + const createSession = jest.fn(); + const refreshSession = jest.fn().mockRejectedValue(new Error('NOPE')); + const manager = new RefreshingAuthSessionManager({ + connector: { createSession, refreshSession }, + ...defaultOptions, + } as any); + + createSession.mockResolvedValue({ + scopes: new Set(['a']), + expired: true, + }); + await manager.getSession({ scopes: new Set(['a']) }); + expect(refreshSession).toHaveBeenCalledTimes(1); + expect(createSession).toHaveBeenCalledTimes(1); + + await manager.getSession({ scopes: new Set(['a']) }); + // call refresh session only once + expect(refreshSession).toHaveBeenCalledTimes(2); + expect(createSession).toHaveBeenCalledTimes(2); + }); }); diff --git a/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.ts b/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.ts index 1978e14d2d..64a1fe2170 100644 --- a/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.ts +++ b/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.ts @@ -64,6 +64,7 @@ export class RefreshingAuthSessionManager implements SessionManager { } async getSession(options: GetSessionOptions): Promise { + let alreadyTriedToRefreshSession = false; if ( this.helper.sessionExistsAndHasScope(this.currentSession, options.scopes) ) { @@ -72,6 +73,8 @@ export class RefreshingAuthSessionManager implements SessionManager { return this.currentSession!; } + alreadyTriedToRefreshSession = true; + try { const refreshedSession = await this.collapsedSessionRefresh( options.scopes, @@ -83,7 +86,20 @@ export class RefreshingAuthSessionManager implements SessionManager { } return refreshedSession; } catch (error) { - // If the refresh attempt fails we assume we don't have a session, so continue to create one. + this.removeLocalSession(); + + if (options.optional) { + return undefined; + } + + if (options.instantPopup) { + // if `instantPopup`, then can't continue because + // we are in asynchronous control flow. Therefore, the application must not try to + // open the popup later in `connector#createSession(...)` because the browser may block the popup. + // Immediately return error. + throw error; + } + // If the refresh attempt fails we assume we don't have a session, so continue to create one } } @@ -94,13 +110,14 @@ export class RefreshingAuthSessionManager implements SessionManager { // stay in a synchronous call stack from the user interaction. The downside // is that the user will sometimes be requested to log in even if they // already had an existing session. - if (!options.instantPopup) { + if (!options.instantPopup && !alreadyTriedToRefreshSession) { try { const newSession = await this.collapsedSessionRefresh(options.scopes); this.currentSession = newSession; // The session might not have the scopes requested so go back and check again return this.getSession(options); } catch { + this.removeLocalSession(); // If the refresh attempt fails we assume we don't have a session, so continue to create one. } } @@ -119,6 +136,18 @@ export class RefreshingAuthSessionManager implements SessionManager { return this.currentSession; } + /** + * Sets `undefined` to this.{@link currentSession} and tells this.{@link stateTracker}, session state tracker, + * that a user has signed out. + * + * Does not propagate session removal to the connector like {@link removeSession}(). + * + */ + removeLocalSession() { + this.currentSession = undefined; + this.stateTracker.setIsSignedIn(false); + } + async removeSession() { this.currentSession = undefined; await this.connector.removeSession(); From 58c4ce203dc1eb82bd7521e7828356342f636448 Mon Sep 17 00:00:00 2001 From: Gasan Guseinov Date: Mon, 27 Jan 2025 23:56:35 +0100 Subject: [PATCH 09/10] update change description Signed-off-by: Gasan Guseinov --- .changeset/tame-kings-eat.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/tame-kings-eat.md b/.changeset/tame-kings-eat.md index 814cebf73c..74dfc25f3d 100644 --- a/.changeset/tame-kings-eat.md +++ b/.changeset/tame-kings-eat.md @@ -2,4 +2,4 @@ '@backstage/core-app-api': minor --- -create session if refresh session fails +if session exists and refresh fails, then create a new session if not instant popup From ad23eb9ff4cbba84d3a8599de84242182cbce99b Mon Sep 17 00:00:00 2001 From: Gasan Guseinov Date: Fri, 31 Jan 2025 15:28:55 +0100 Subject: [PATCH 10/10] allow refresh session when client requests instant popup Signed-off-by: Gasan Guseinov --- .../RefreshingAuthSessionManager.test.ts | 4 ++-- .../RefreshingAuthSessionManager.ts | 19 ++++++------------- 2 files changed, 8 insertions(+), 15 deletions(-) diff --git a/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.test.ts b/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.test.ts index 302eb82de0..d5e72a2365 100644 --- a/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.test.ts +++ b/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.test.ts @@ -138,7 +138,7 @@ describe('RefreshingAuthSessionManager', () => { expect(refreshSession).toHaveBeenCalledWith({ scopes: new Set() }); }); - it('should forward option to instantly show auth popup and not attempt refresh', async () => { + it('should forward option to instantly show auth popup after attempting refresh', async () => { const createSession = jest.fn(); const refreshSession = jest.fn().mockRejectedValue(new Error('NOPE')); const manager = new RefreshingAuthSessionManager({ @@ -152,7 +152,7 @@ describe('RefreshingAuthSessionManager', () => { scopes: new Set(), instantPopup: true, }); - expect(refreshSession).toHaveBeenCalledTimes(0); + expect(refreshSession).toHaveBeenCalledTimes(1); }); it('should remove session straight away', async () => { diff --git a/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.ts b/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.ts index 64a1fe2170..fbb57b4a5e 100644 --- a/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.ts +++ b/packages/core-app-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.ts @@ -91,14 +91,6 @@ export class RefreshingAuthSessionManager implements SessionManager { if (options.optional) { return undefined; } - - if (options.instantPopup) { - // if `instantPopup`, then can't continue because - // we are in asynchronous control flow. Therefore, the application must not try to - // open the popup later in `connector#createSession(...)` because the browser may block the popup. - // Immediately return error. - throw error; - } // If the refresh attempt fails we assume we don't have a session, so continue to create one } } @@ -106,11 +98,12 @@ export class RefreshingAuthSessionManager implements SessionManager { // The user may still have a valid refresh token in their cookies. Attempt to // initiate a fresh session through the backend using that refresh token. // - // We skip this check if an instant login popup is requested, as we need to - // stay in a synchronous call stack from the user interaction. The downside - // is that the user will sometimes be requested to log in even if they - // already had an existing session. - if (!options.instantPopup && !alreadyTriedToRefreshSession) { + // We can still try to refresh even if client requested instant popup. + // With instant popup option, the client is responsible for providing the user login prompt modal window. + // If control flow executes this code and client requested instant popup, it means that + // must have clicked sign in on the login prompt. The browser allows asynchronous code to open a popup + // if it is caused by a user interaction, clicking on a sign-in button, for example. + if (!alreadyTriedToRefreshSession) { try { const newSession = await this.collapsedSessionRefresh(options.scopes); this.currentSession = newSession;