Merge pull request #1431 from spotify/rugvip/retrack

core-api: move session state tracking to auth session managers

packages/core-api/src/apis/implementations/auth/github/GithubAuth.ts

@@ -29,7 +29,6 @@ import { OAuthRequestApi, AuthProvider } from '../../../definitions';
 import { SessionManager } from '../../../../lib/AuthSessionManager/types';
 import { StaticAuthSessionManager } from '../../../../lib/AuthSessionManager';
 import { Observable } from '../../../../types';
-import { SessionStateTracker } from '../../../../lib/AuthSessionManager/SessionStateTracker';
 
 type CreateOptions = {
   // TODO(Rugvip): These two should be grabbed from global config when available, they're not unique to GithubAuth
@@ -95,44 +94,34 @@ class GithubAuth implements OAuthApi, SessionStateApi {
     return new GithubAuth(sessionManager);
   }
 
-  private readonly sessionStateTracker = new SessionStateTracker();
-
   sessionState$(): Observable<SessionState> {
-    return this.sessionStateTracker.observable;
+    return this.sessionManager.sessionState$();
   }
 
   constructor(private readonly sessionManager: SessionManager<GithubSession>) {}
 
   async getAccessToken(scope?: string, options?: AuthRequestOptions) {
-    const normalizedScopes = GithubAuth.normalizeScope(scope);
     const session = await this.sessionManager.getSession({
       ...options,
-      scopes: normalizedScopes,
+      scopes: GithubAuth.normalizeScope(scope),
     });
-    this.sessionStateTracker.setIsSignedId(!!session);
-    if (session) {
-      return session.providerInfo.accessToken;
-    }
-    return '';
+    return session?.providerInfo.accessToken ?? '';
   }
 
   async getBackstageIdentity(
     options: AuthRequestOptions = {},
   ): Promise<BackstageIdentity | undefined> {
     const session = await this.sessionManager.getSession(options);
-    this.sessionStateTracker.setIsSignedId(!!session);
     return session?.backstageIdentity;
   }
 
   async getProfile(options: AuthRequestOptions = {}) {
     const session = await this.sessionManager.getSession(options);
-    this.sessionStateTracker.setIsSignedId(!!session);
     return session?.profile;
   }
 
   async logout() {
     await this.sessionManager.removeSession();
-    this.sessionStateTracker.setIsSignedId(false);
   }
 
   static normalizeScope(scope?: string): Set<string> {

packages/core-api/src/apis/implementations/auth/google/GoogleAuth.ts

@@ -32,7 +32,6 @@ import { OAuthRequestApi, AuthProvider } from '../../../definitions';
 import { SessionManager } from '../../../../lib/AuthSessionManager/types';
 import { RefreshingAuthSessionManager } from '../../../../lib/AuthSessionManager';
 import { Observable } from '../../../../types';
-import { SessionStateTracker } from '../../../../lib/AuthSessionManager/SessionStateTracker';
 
 type CreateOptions = {
   // TODO(Rugvip): These two should be grabbed from global config when available, they're not unique to GoogleAuth
@@ -117,10 +116,8 @@ class GoogleAuth
     return new GoogleAuth(sessionManager);
   }
 
-  private readonly sessionStateTracker = new SessionStateTracker();
-
   sessionState$(): Observable<SessionState> {
-    return this.sessionStateTracker.observable;
+    return this.sessionManager.sessionState$();
   }
 
   constructor(private readonly sessionManager: SessionManager<GoogleSession>) {}
@@ -129,43 +126,31 @@ class GoogleAuth
     scope?: string | string[],
     options?: AuthRequestOptions,
   ) {
-    const normalizedScopes = GoogleAuth.normalizeScopes(scope);
     const session = await this.sessionManager.getSession({
       ...options,
-      scopes: normalizedScopes,
+      scopes: GoogleAuth.normalizeScopes(scope),
     });
-    this.sessionStateTracker.setIsSignedId(!!session);
-    if (session) {
-      return session.providerInfo.accessToken;
-    }
-    return '';
+    return session?.providerInfo.accessToken ?? '';
   }
 
   async getIdToken(options: AuthRequestOptions = {}) {
     const session = await this.sessionManager.getSession(options);
-    this.sessionStateTracker.setIsSignedId(!!session);
-    if (session) {
-      return session.providerInfo.idToken;
-    }
-    return '';
+    return session?.providerInfo.idToken ?? '';
   }
 
   async logout() {
     await this.sessionManager.removeSession();
-    this.sessionStateTracker.setIsSignedId(false);
   }
 
   async getBackstageIdentity(
     options: AuthRequestOptions = {},
   ): Promise<BackstageIdentity | undefined> {
     const session = await this.sessionManager.getSession(options);
-    this.sessionStateTracker.setIsSignedId(!!session);
     return session?.backstageIdentity;
   }
 
   async getProfile(options: AuthRequestOptions = {}) {
     const session = await this.sessionManager.getSession(options);
-    this.sessionStateTracker.setIsSignedId(!!session);
     return session?.profile;
   }
 

packages/core-api/src/lib/AuthSessionManager/AuthSessionStore.test.ts

@@ -38,6 +38,7 @@ class LocalStorage {
 class MockManager implements SessionManager<string> {
   getSession = jest.fn();
   removeSession = jest.fn();
+  sessionState$ = jest.fn();
 }
 
 describe('GheAuth AuthSessionStore', () => {
@@ -119,4 +120,11 @@ describe('GheAuth AuthSessionStore', () => {
 
     expect(localStorage.getItem('my-key')).toBe(null);
   });
+
+  it('should forward sessionState calls', () => {
+    const manager = new MockManager();
+    const store = new AuthSessionStore({ manager, ...defaultOptions });
+    store.sessionState$();
+    expect(manager.sessionState$).toHaveBeenCalled();
+  });
 });

packages/core-api/src/lib/AuthSessionManager/AuthSessionStore.ts

@@ -82,6 +82,10 @@ export class AuthSessionStore<T> implements SessionManager<T> {
     await this.manager.removeSession();
   }
 
+  sessionState$() {
+    return this.manager.sessionState$();
+  }
+
   private loadSession(): T | undefined {
     try {
       const sessionJson = localStorage.getItem(this.storageKey);

packages/core-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.test.ts

@@ -15,6 +15,7 @@
  */
 
 import { RefreshingAuthSessionManager } from './RefreshingAuthSessionManager';
+import { SessionState } from '../../apis';
 
 const defaultOptions = {
   sessionScopes: (session: { scopes: Set<string> }) => session.scopes,
@@ -22,21 +23,44 @@ const defaultOptions = {
 };
 
 describe('RefreshingAuthSessionManager', () => {
-  it('should save result form createSession', async () => {
+  it('should save result from createSession', async () => {
     const createSession = jest.fn().mockResolvedValue({ expired: false });
     const refreshSession = jest.fn().mockRejectedValue(new Error('NOPE'));
+    const removeSession = jest.fn();
     const manager = new RefreshingAuthSessionManager({
-      connector: { createSession, refreshSession },
+      connector: { createSession, refreshSession, removeSession },
       ...defaultOptions,
     } as any);
+    const stateSubscriber = jest.fn();
+    manager.sessionState$().subscribe(stateSubscriber);
 
+    await Promise.resolve(); // Wait a tick for observer to post a value
+
+    expect(stateSubscriber.mock.calls).toEqual([[SessionState.SignedOut]]);
     await manager.getSession({});
     expect(createSession).toBeCalledTimes(1);
 
+    expect(stateSubscriber.mock.calls).toEqual([
+      [SessionState.SignedOut],
+      [SessionState.SignedIn],
+    ]);
     await manager.getSession({});
     expect(createSession).toBeCalledTimes(1);
 
     expect(refreshSession).toBeCalledTimes(1);
+    expect(stateSubscriber.mock.calls).toEqual([
+      [SessionState.SignedOut],
+      [SessionState.SignedIn],
+    ]);
+
+    expect(removeSession).toHaveBeenCalledTimes(0);
+    await manager.removeSession();
+    expect(removeSession).toHaveBeenCalledTimes(1);
+    expect(stateSubscriber.mock.calls).toEqual([
+      [SessionState.SignedOut],
+      [SessionState.SignedIn],
+      [SessionState.SignedOut],
+    ]);
   });
 
   it('should ask consent only if scopes have changed', async () => {
@@ -130,7 +154,7 @@ describe('RefreshingAuthSessionManager', () => {
     expect(refreshSession).toBeCalledTimes(1);
   });
 
-  it('should remove session and reload', async () => {
+  it('should remove session straight away', async () => {
     const removeSession = jest.fn();
     const manager = new RefreshingAuthSessionManager({
       connector: { removeSession },

packages/core-api/src/lib/AuthSessionManager/RefreshingAuthSessionManager.ts

@@ -22,6 +22,7 @@ import {
 } from './types';
 import { AuthConnector } from '../AuthConnector';
 import { SessionScopeHelper, hasScopes } from './common';
+import { SessionStateTracker } from './SessionStateTracker';
 
 type Options<T> = {
   /** The connector used for acting on the auth session */
@@ -43,6 +44,7 @@ export class RefreshingAuthSessionManager<T> implements SessionManager<T> {
   private readonly helper: SessionScopeHelper<T>;
   private readonly sessionScopesFunc: SessionScopesFunc<T>;
   private readonly sessionShouldRefreshFunc: SessionShouldRefreshFunc<T>;
+  private readonly stateTracker = new SessionStateTracker();
 
   private refreshPromise?: Promise<T>;
   private currentSession: T | undefined;
@@ -109,16 +111,18 @@ export class RefreshingAuthSessionManager<T> implements SessionManager<T> {
       ...options,
       scopes: this.helper.getExtendedScope(this.currentSession, options.scopes),
     });
+    this.stateTracker.setIsSignedIn(true);
     return this.currentSession;
   }
 
   async removeSession() {
     this.currentSession = undefined;
     await this.connector.removeSession();
+    this.stateTracker.setIsSignedIn(false);
   }
 
-  async getCurrentSession() {
-    return this.currentSession;
+  sessionState$() {
+    return this.stateTracker.sessionState$();
   }
 
   private async collapsedSessionRefresh(): Promise<T> {
@@ -129,7 +133,9 @@ export class RefreshingAuthSessionManager<T> implements SessionManager<T> {
     this.refreshPromise = this.connector.refreshSession();
 
     try {
-      return await this.refreshPromise;
+      const session = await this.refreshPromise;
+      this.stateTracker.setIsSignedIn(true);
+      return session;
     } finally {
       delete this.refreshPromise;
     }

packages/core-api/src/lib/AuthSessionManager/SessionStateTracker.ts

@@ -16,17 +16,25 @@
 
 import { BehaviorSubject } from '..';
 import { SessionState } from '../../apis';
+import { Observable } from '../../types';
 
 export class SessionStateTracker {
-  private signedIn: boolean = false;
-  observable = new BehaviorSubject<SessionState>(SessionState.SignedOut);
+  private readonly subject = new BehaviorSubject<SessionState>(
+    SessionState.SignedOut,
+  );
 
-  setIsSignedId(isSignedIn: boolean) {
+  private signedIn: boolean = false;
+
+  setIsSignedIn(isSignedIn: boolean) {
     if (this.signedIn !== isSignedIn) {
       this.signedIn = isSignedIn;
-      this.observable.next(
+      this.subject.next(
         this.signedIn ? SessionState.SignedIn : SessionState.SignedOut,
       );
     }
   }
+
+  sessionState$(): Observable<SessionState> {
+    return this.subject;
+  }
 }

packages/core-api/src/lib/AuthSessionManager/StaticAuthSessionManager.ts

@@ -17,6 +17,7 @@
 import { SessionManager, GetSessionOptions } from './types';
 import { AuthConnector } from '../AuthConnector';
 import { SessionScopeHelper } from './common';
+import { SessionStateTracker } from './SessionStateTracker';
 
 type Options<T> = {
   /** The connector used for acting on the auth session */
@@ -33,6 +34,7 @@ type Options<T> = {
 export class StaticAuthSessionManager<T> implements SessionManager<T> {
   private readonly connector: AuthConnector<T>;
   private readonly helper: SessionScopeHelper<T>;
+  private readonly stateTracker = new SessionStateTracker();
 
   private currentSession: T | undefined;
 
@@ -60,11 +62,17 @@ export class StaticAuthSessionManager<T> implements SessionManager<T> {
       ...options,
       scopes: this.helper.getExtendedScope(this.currentSession, options.scopes),
     });
+    this.stateTracker.setIsSignedIn(true);
     return this.currentSession;
   }
 
   async removeSession() {
     this.currentSession = undefined;
     await this.connector.removeSession();
+    this.stateTracker.setIsSignedIn(false);
+  }
+
+  sessionState$() {
+    return this.stateTracker.sessionState$();
   }
 }

packages/core-api/src/lib/AuthSessionManager/types.ts

@@ -14,6 +14,9 @@
  * limitations under the License.
  */
 
+import { Observable } from '../../types';
+import { SessionState } from '../../apis';
+
 export type GetSessionOptions = {
   optional?: boolean;
   instantPopup?: boolean;
@@ -29,6 +32,8 @@ export type SessionManager<T> = {
   getSession(options: GetSessionOptions): Promise<T | undefined>;
 
   removeSession(): Promise<void>;
+
+  sessionState$(): Observable<SessionState>;
 };
 
 /**