review fixes

packages/core-api/src/apis/implementations/auth/ObservableSession.tsx

@@ -1,36 +0,0 @@
-/*
- * Copyright 2020 Spotify AB
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *     http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-import { BehaviorSubject } from '../../../lib';
-import { Observable } from '../../../types';
-
-export class ObservableSession<T> {
-  private session: T | undefined;
-  private readonly subject = new BehaviorSubject<T | undefined>(undefined);
-
-  session$(): Observable<T | undefined> {
-    return this.subject;
-  }
-
-  getSession(): T | undefined {
-    return this.session;
-  }
-
-  setSession(session?: T): void {
-    this.session = session;
-    this.subject.next(session);
-  }
-}

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

@@ -21,7 +21,8 @@ import { OAuthApi, AccessTokenOptions } from '../../../definitions/auth';
 import { OAuthRequestApi, AuthProvider } from '../../../definitions';
 import { SessionManager } from '../../../../lib/AuthSessionManager/types';
 import { StaticAuthSessionManager } from '../../../../lib/AuthSessionManager';
-import { ObservableSession } from '../ObservableSession';
+import { BehaviorSubject } from '../../../../lib';
+import { Observable } from '../../../../types';
 
 type CreateOptions = {
   // TODO(Rugvip): These two should be grabbed from global config when available, they're not unique to GithubAuth
@@ -47,7 +48,7 @@ const DEFAULT_PROVIDER = {
   icon: GithubIcon,
 };
 
-class GithubAuth extends ObservableSession<GithubSession> implements OAuthApi {
+class GithubAuth implements OAuthApi {
   static create({
     apiOrigin,
     basePath,
@@ -79,17 +80,23 @@ class GithubAuth extends ObservableSession<GithubSession> implements OAuthApi {
     return new GithubAuth(sessionManager);
   }
 
-  constructor(private readonly sessionManager: SessionManager<GithubSession>) {
-    super();
+  private readonly subject = new BehaviorSubject<boolean | undefined>(
+    undefined,
+  );
+
+  session$(): Observable<boolean | undefined> {
+    return this.subject;
   }
 
+  constructor(private readonly sessionManager: SessionManager<GithubSession>) {}
+
   async getAccessToken(scope?: string, options?: AccessTokenOptions) {
     const normalizedScopes = GithubAuth.normalizeScope(scope);
     const session = await this.sessionManager.getSession({
       ...options,
       scopes: normalizedScopes,
     });
-    this.setSession(session);
+    this.subject.next(!!session);
     if (session) {
       return session.accessToken;
     }
@@ -98,7 +105,7 @@ class GithubAuth extends ObservableSession<GithubSession> implements OAuthApi {
 
   async logout() {
     await this.sessionManager.removeSession();
-    this.setSession();
+    this.subject.next(false);
   }
 
   static normalizeScope(scope?: string): Set<string> {

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

@@ -29,7 +29,8 @@ import {
 import { OAuthRequestApi, AuthProvider } from '../../../definitions';
 import { SessionManager } from '../../../../lib/AuthSessionManager/types';
 import { RefreshingAuthSessionManager } from '../../../../lib/AuthSessionManager';
-import { ObservableSession } from '../ObservableSession';
+import { BehaviorSubject } from '../../../../lib';
+import { Observable } from '../../../../types';
 
 type CreateOptions = {
   // TODO(Rugvip): These two should be grabbed from global config when available, they're not unique to GoogleAuth
@@ -58,8 +59,7 @@ const DEFAULT_PROVIDER = {
 
 const SCOPE_PREFIX = 'https://www.googleapis.com/auth/';
 
-class GoogleAuth extends ObservableSession<GoogleSession>
-  implements OAuthApi, OpenIdConnectApi, ProfileInfoApi {
+class GoogleAuth implements OAuthApi, OpenIdConnectApi, ProfileInfoApi {
   static create({
     apiOrigin,
     basePath,
@@ -101,10 +101,16 @@ class GoogleAuth extends ObservableSession<GoogleSession>
     return new GoogleAuth(sessionManager);
   }
 
-  constructor(private readonly sessionManager: SessionManager<GoogleSession>) {
-    super();
+  private readonly subject = new BehaviorSubject<boolean | undefined>(
+    undefined,
+  );
+
+  session$(): Observable<boolean | undefined> {
+    return this.subject;
   }
 
+  constructor(private readonly sessionManager: SessionManager<GoogleSession>) {}
+
   async getAccessToken(
     scope?: string | string[],
     options?: AccessTokenOptions,
@@ -114,7 +120,7 @@ class GoogleAuth extends ObservableSession<GoogleSession>
       ...options,
       scopes: normalizedScopes,
     });
-    this.setSession(session);
+    this.subject.next(!!session);
     if (session) {
       return session.accessToken;
     }
@@ -123,7 +129,7 @@ class GoogleAuth extends ObservableSession<GoogleSession>
 
   async getIdToken(options: IdTokenOptions = {}) {
     const session = await this.sessionManager.getSession(options);
-    this.setSession(session);
+    this.subject.next(!!session);
     if (session) {
       return session.idToken;
     }
@@ -132,7 +138,7 @@ class GoogleAuth extends ObservableSession<GoogleSession>
 
   async logout() {
     await this.sessionManager.removeSession();
-    this.setSession();
+    this.subject.next(false);
   }
 
   async getProfile(options: ProfileInfoOptions = {}) {

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

@@ -113,8 +113,8 @@ export class RefreshingAuthSessionManager<T> implements SessionManager<T> {
   }
 
   async removeSession() {
+    this.currentSession = undefined;
     await this.connector.removeSession();
-    window.location.reload(); // TODO(Rugvip): make this work without reload?
   }
 
   async getCurrentSession() {

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

@@ -64,7 +64,7 @@ export class StaticAuthSessionManager<T> implements SessionManager<T> {
   }
 
   async removeSession() {
+    this.currentSession = undefined;
     await this.connector.removeSession();
-    window.location.reload(); // TODO(Rugvip): make this work without reload?
   }
 }

packages/core/src/layout/Sidebar/Sidebar.stories.tsx

@@ -22,6 +22,7 @@ import {
   SidebarDivider,
   SidebarSearchField,
   SidebarSpace,
+  SidebarUserSettings,
 } from '.';
 import HomeOutlinedIcon from '@material-ui/icons/HomeOutlined';
 import AddCircleOutlineIcon from '@material-ui/icons/AddCircleOutline';
@@ -54,5 +55,6 @@ export const SampleSidebar = () => (
     <SidebarIntro />
     <SidebarSpace />
     <SidebarDivider />
+    <SidebarUserSettings />
   </Sidebar>
 );

packages/core/src/layout/Sidebar/UserSettings.tsx

@@ -33,6 +33,8 @@ import {
 import { Avatar, IconButton, makeStyles, Tooltip } from '@material-ui/core';
 import PowerButton from '@material-ui/icons/PowerSettingsNew';
 
+type AppAuthProviders = Provider[];
+
 type Provider = {
   title: string;
   api: any;
@@ -44,7 +46,8 @@ type Provider = {
 const useProviders = () => {
   const googleAuth = useApi(googleAuthApiRef);
   const githubAuth = useApi(githubAuthApiRef);
-  const [providers, setProviders] = useState<Provider[]>([
+  // TODO(soapraj): List all the providers supported by the app
+  const [providers, setProviders] = useState<AppAuthProviders>([
     {
       title: 'Google',
       api: googleAuth,
@@ -60,43 +63,40 @@ const useProviders = () => {
     },
   ]);
 
-  useEffect(() => {
-    // On page load we check the status of sign-in/sign-out for all the providers
-    // by making a optional getIdToken or getAccessToken request.
-    const setIsSignedIn = async () => {
-      const signInChecks = await Promise.all(
-        providers.map(provider => {
-          return provider.identity
-            ? provider.api.getIdToken({ optional: true })
-            : provider.api.getAccessToken('', { optional: true });
-        }),
-      );
+  // On page load we check the status of sign-in/sign-out for all the providers
+  // by making a optional getIdToken or getAccessToken request.
+  const setIsSignedIn = async () => {
+    const signInChecks = await Promise.all(
+      providers.map(provider => {
+        return provider.identity
+          ? provider.api.getIdToken({ optional: true })
+          : provider.api.getAccessToken('', { optional: true });
+      }),
+    );
 
-      signInChecks.map((result, i) => {
-        providers[i].isSignedIn = !!result;
-      });
+    signInChecks.map((result, i) => {
+      providers[i].isSignedIn = !!result;
+    });
 
-      setProviders(providers);
-    };
+    setProviders(providers);
+  };
 
-    setIsSignedIn();
-
-    // Any sign-in/sign-out activity on any provider is observed here
+  // Any sign-in/sign-out activity on any provider is observed here
+  const observeProviderState = () => {
     providers.map((provider, index) => {
-      provider.api.session$().subscribe((session: any) => {
-        if (session) {
-          const currentProvider = providers[index];
-          currentProvider.isSignedIn = true;
-          setProviders([
-            ...providers.slice(0, index),
-            currentProvider,
-            ...providers.slice(index + 1, providers.length),
-          ]);
-        }
+      provider.api.session$().subscribe((signedInState: boolean) => {
+        const mutatedProvider = providers[index];
+        mutatedProvider.isSignedIn = signedInState;
+        providers[index] = mutatedProvider;
+        setProviders(providers.slice());
       });
     });
-    // eslint-disable-next-line react-hooks/exhaustive-deps
-  }, []);
+  };
+
+  useEffect(() => {
+    setIsSignedIn();
+    observeProviderState();
+  }, [setIsSignedIn, observeProviderState]);
 
   return providers;
 };
@@ -116,17 +116,19 @@ export function SidebarUserSettings() {
   const [profile, setProfile] = useState<ProfileInfo>();
   const classes = useStyles();
 
-  // TODO(soapraj): List all the providers supported by the app and let user log in from here
-  // TODO(soapraj): How to observe if the user is logged in
   useEffect(() => {
     const identityProvider = providers.find(
       (provider: Provider) => provider.identity,
     );
-    identityProvider?.api
-      .getProfile({ optional: true })
-      .then((userProfile: ProfileInfo) => {
-        setProfile(userProfile);
-      });
+    if (identityProvider?.isSignedIn) {
+      identityProvider?.api
+        .getProfile({ optional: true })
+        .then((userProfile: ProfileInfo) => {
+          setProfile(userProfile);
+        });
+    } else {
+      setProfile(undefined);
+    }
   }, [providers, open]);
 
   const handleClick = () => {
@@ -141,30 +143,27 @@ export function SidebarUserSettings() {
 
   // Handle main auth info that is shown on the collapsible SidebarItem
   let avatar;
-  let displayName;
+  let displayName = 'Guest';
   if (profile) {
     const email = profile.email;
     const name = profile.name;
     const imageUrl = profile.picture;
-    const avatarFallback = email.charAt(0).toUpperCase() + email.slice(1);
     const emailTrimmed = email.split('@')[0];
     const displayEmail =
       emailTrimmed.charAt(0).toUpperCase() + emailTrimmed.slice(1);
     displayName = name ?? displayEmail;
     avatar = imageUrl
-      ? () => <Avatar alt={name} src={imageUrl} className={classes.avatar} />
-      : () => (
-          <Avatar alt={name} className={classes.avatar}>
-            {avatarFallback[0]}
-          </Avatar>
-        );
+      ? () => (
+          <Avatar alt={displayName} src={imageUrl} className={classes.avatar} />
+        )
+      : () => <Avatar alt={displayName} className={classes.avatar} />;
   }
 
   return (
     <>
       <Divider innerRef={ref} />
       <SidebarItem
-        text={displayName || 'Guest'}
+        text={displayName}
         onClick={handleClick}
         icon={avatar || AccountCircleIcon}
         disableSelected
@@ -191,7 +190,7 @@ export function SidebarUserSettings() {
                 arrow
                 title={
                   provider.isSignedIn
-                    ? `Logout from ${provider.title}`
+                    ? `Sign out from ${provider.title}`
                     : `Sign in to ${provider.title}`
                 }
               >