core-api: add MutableSessionManager and fix AuthSessionStore not prop…

…erly updating session state
backstage · Dec 17, 2020 · d681db2 · d681db2
1 parent ebbc133
commit d681db2
Show file tree

Hide file tree

Showing 5 changed files with 30 additions and 4 deletions.
diff --git a/.changeset/giant-rice-jump.md b/.changeset/giant-rice-jump.md
@@ -0,0 +1,5 @@
+---
+'@backstage/core-api': patch
+---
+
+Fix for GitHub and SAML auth not properly updating session state when already logged in.
diff --git a/packages/core-api/src/lib/AuthSessionManager/AuthSessionStore.test.ts b/packages/core-api/src/lib/AuthSessionManager/AuthSessionStore.test.ts
@@ -36,6 +36,7 @@ class LocalStorage {
 }
 
 class MockManager implements SessionManager<string> {
+  setSession = jest.fn();
   getSession = jest.fn();
   removeSession = jest.fn();
   sessionState$ = jest.fn();
@@ -59,6 +60,7 @@ describe('GheAuth AuthSessionStore', () => {
 
     await expect(store.getSession({})).resolves.toBe('a b c');
     expect(manager.getSession).not.toHaveBeenCalled();
+    expect(manager.setSession).toHaveBeenCalledWith('a b c');
   });
 
   it('should not use session without enough scope', async () => {
@@ -72,6 +74,7 @@ describe('GheAuth AuthSessionStore', () => {
       'a b c d',
     );
     expect(manager.getSession).toHaveBeenCalledTimes(1);
+    expect(manager.setSession).not.toHaveBeenCalled();
   });
 
   it('should not use expired session', async () => {
@@ -87,6 +90,7 @@ describe('GheAuth AuthSessionStore', () => {
 
     await expect(store.getSession({})).resolves.toBe('123');
     expect(manager.getSession).toHaveBeenCalledTimes(1);
+    expect(manager.setSession).not.toHaveBeenCalled();
   });
 
   it('should not load missing session', async () => {
@@ -96,6 +100,7 @@ describe('GheAuth AuthSessionStore', () => {
 
     await expect(store.getSession({})).resolves.toBe('123');
     expect(manager.getSession).toHaveBeenCalledTimes(1);
+    expect(manager.setSession).not.toHaveBeenCalled();
 
     expect(localStorage.getItem('my-key')).toBe('"123"');
   });
@@ -109,6 +114,7 @@ describe('GheAuth AuthSessionStore', () => {
 
     await expect(store.getSession({})).resolves.toBe('123');
     expect(manager.getSession).toHaveBeenCalledTimes(1);
+    expect(manager.setSession).not.toHaveBeenCalled();
   });
 
   it('should clear session', () => {
@@ -119,6 +125,7 @@ describe('GheAuth AuthSessionStore', () => {
     store.removeSession();
 
     expect(localStorage.getItem('my-key')).toBe(null);
+    expect(manager.removeSession).toHaveBeenCalled();
   });
 
   it('should forward sessionState calls', () => {

diff --git a/packages/core-api/src/lib/AuthSessionManager/AuthSessionStore.ts b/packages/core-api/src/lib/AuthSessionManager/AuthSessionStore.ts
@@ -16,6 +16,7 @@
 
 import {
   SessionManager,
+  MutableSessionManager,
   SessionScopesFunc,
   SessionShouldRefreshFunc,
   GetSessionOptions,
@@ -24,7 +25,7 @@ import { SessionScopeHelper } from './common';
 
 type Options<T> = {
   /** The connector used for acting on the auth session */
-  manager: SessionManager<T>;
+  manager: MutableSessionManager<T>;
   /** Storage key to use to store sessions */
   storageKey: string;
   /** Used to get the scope of the session */
@@ -40,7 +41,7 @@ type Options<T> = {
  * Session is serialized to JSON with special support for following types: Set.
  */
 export class AuthSessionStore<T> implements SessionManager<T> {
-  private readonly manager: SessionManager<T>;
+  private readonly manager: MutableSessionManager<T>;
   private readonly storageKey: string;
   private readonly sessionShouldRefreshFunc: SessionShouldRefreshFunc<T>;
   private readonly helper: SessionScopeHelper<T>;
@@ -70,6 +71,7 @@ export class AuthSessionStore<T> implements SessionManager<T> {
       const shouldRefresh = this.sessionShouldRefreshFunc(session!);
 
       if (!shouldRefresh) {
+        this.manager.setSession(session!);
         return session!;
       }
     }

diff --git a/packages/core-api/src/lib/AuthSessionManager/StaticAuthSessionManager.ts b/packages/core-api/src/lib/AuthSessionManager/StaticAuthSessionManager.ts
@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-import { SessionManager, GetSessionOptions } from './types';
+import { MutableSessionManager, GetSessionOptions } from './types';
 import { AuthConnector } from '../AuthConnector';
 import { SessionScopeHelper } from './common';
 import { SessionStateTracker } from './SessionStateTracker';
@@ -31,7 +31,7 @@ type Options<T> = {
 /**
  * StaticAuthSessionManager manages an underlying session that does not expire.
  */
-export class StaticAuthSessionManager<T> implements SessionManager<T> {
+export class StaticAuthSessionManager<T> implements MutableSessionManager<T> {
   private readonly connector: AuthConnector<T>;
   private readonly helper: SessionScopeHelper<T>;
   private readonly stateTracker = new SessionStateTracker();
@@ -45,6 +45,11 @@ export class StaticAuthSessionManager<T> implements SessionManager<T> {
     this.helper = new SessionScopeHelper({ sessionScopes, defaultScopes });
   }
 
+  setSession(session: T | undefined): void {
+    this.currentSession = session;
+    this.stateTracker.setIsSignedIn(Boolean(session));
+  }
+
   async getSession(options: GetSessionOptions): Promise<T | undefined> {
     if (
       this.helper.sessionExistsAndHasScope(this.currentSession, options.scopes)

diff --git a/packages/core-api/src/lib/AuthSessionManager/types.ts b/packages/core-api/src/lib/AuthSessionManager/types.ts
@@ -36,6 +36,13 @@ export type SessionManager<T> = {
   sessionState$(): Observable<SessionState>;
 };
 
+/**
+ * An extension of the session manager where the session can also be pushed from the manager.
+ */
+export interface MutableSessionManager<T> extends SessionManager<T> {
+  setSession(session: T | undefined): void;
+}
+
 /**
  * A function called to determine the scopes of a session.
  */