fix(auth): Fix repeat auth calls on logout (#321)

* Fix repeat auth calls on logout * Move auth method query to login service * Extract checkAuth logic from Login page * fixup! Extract checkAuth logic from Login page * Control display logic with sessionState * Refactor reconnect logic * Refactor reconnect logic * Remove redundant variable * fixup! Remove redundant variable * Remove unused cached method * Stay on Login page after refresh * Fix formatting * fixup! Stay on Login page after refresh
cryostatio · Oct 18, 2021 · 80cf1f3 · 80cf1f3
1 parent 7f15186
commit 80cf1f3
Show file tree

Hide file tree

Showing 7 changed files with 78 additions and 97 deletions.
diff --git a/src/app/AppLayout/AppLayout.tsx b/src/app/AppLayout/AppLayout.tsx
@@ -52,6 +52,7 @@ import { AuthModal } from './AuthModal';
 import { SslErrorModal } from './SslErrorModal';
 import { AboutCryostatModal } from '@app/About/AboutCryostatModal';
 import cryostatLogoHorizontal from '@app/assets/logo-cryostat-3-horizontal.svg';
+import { SessionState } from '@app/Shared/Services/Login.service';
 
 interface IAppLayout {
   children: React.ReactNode;
@@ -150,7 +151,9 @@ const AppLayout: React.FunctionComponent<IAppLayout> = ({children}) => {
   };
 
   React.useEffect(() => {
-    const sub = serviceContext.login.isAuthenticated().subscribe(setShowUserIcon);
+    const sub = serviceContext.login.getSessionState().subscribe(sessionState => {
+      setShowUserIcon(sessionState === SessionState.USER_SESSION);
+    });
     return () => sub.unsubscribe();
   }, [serviceContext.target]);
 

diff --git a/src/app/Login/Login.tsx b/src/app/Login/Login.tsx
@@ -38,78 +38,33 @@
 import * as React from 'react';
 import { ServiceContext } from '@app/Shared/Services/Services';
 import { NotificationsContext } from '../Notifications/Notifications';
-import { CloseStatus } from '@app/Shared/Services/NotificationChannel.service';
-import { useSubscriptions } from '@app/utils/useSubscriptions';
 import { Card, CardBody, CardFooter, CardHeader, PageSection, Title } from '@patternfly/react-core';
-import { combineLatest, timer } from 'rxjs';
-import { debounceTime, first } from 'rxjs/operators';
-import { Base64 } from 'js-base64';
 import { BasicAuthDescriptionText, BasicAuthForm } from './BasicAuthForm';
 import { BearerAuthDescriptionText, BearerAuthForm } from './BearerAuthForm';
 
 export const Login = () => {
   const serviceContext = React.useContext(ServiceContext);
   const notifications = React.useContext(NotificationsContext);
   const [authMethod, setAuthMethod] = React.useState('');
-  const addSubscription = useSubscriptions();
 
-  const checkAuth = React.useCallback((token, authMethod, rememberMe = false, userSubmission = false) => {
-    let tok = token;
-
-    if (authMethod === 'Basic') {
-      tok = Base64.encodeURL(token);
-    } // else this is Bearer auth and the token is sent as-is
-    addSubscription(
-      serviceContext.login.checkAuth(tok, authMethod)
-      .pipe(first())
-      .subscribe(v => {
-        if(v && rememberMe) {
-          serviceContext.login.rememberToken(tok);
-        } else if (v && !rememberMe && userSubmission) {
-          serviceContext.login.forgetToken();
-        }
+  const handleSubmit = React.useCallback((evt, token, authMethod, rememberMe) => {
+    setAuthMethod(authMethod);
 
-        if (!v && userSubmission) {
+    const sub = serviceContext.login.checkAuth(token, authMethod, rememberMe)
+      .subscribe(authSuccess => {
+        if(!authSuccess) {
           notifications.danger('Authentication Failure', `${authMethod} authentication failed`);
         }
-      })
-    );
-  }, [serviceContext, serviceContext.login, addSubscription,  notifications, authMethod]);
+      });
+    () => sub.unsubscribe();
 
-  const handleSubmit = React.useCallback((evt, token, authMethod, rememberMe) => {
-    setAuthMethod(authMethod);
-    checkAuth(token, authMethod, rememberMe, true);
     evt.preventDefault();
-  }, [setAuthMethod, checkAuth]);
+  }, [serviceContext, serviceContext.login, setAuthMethod]);
 
   React.useEffect(() => {
     const sub = serviceContext.login.getAuthMethod().subscribe(setAuthMethod);
-    checkAuth('', 'Basic'); // check auth once at component load to query the server's auth method
-    return () => sub.unsubscribe();
-  }, [serviceContext, serviceContext.login, setAuthMethod, checkAuth]);
-
-  React.useEffect(() => {
-    const sub =
-      combineLatest(serviceContext.login.getToken(), serviceContext.login.getAuthMethod(),  serviceContext.notificationChannel.isReady(), timer(0, 5000))
-      .pipe(debounceTime(1000))
-      .subscribe(parts => {
-        let token = parts[0];
-        let authMethod = parts[1];
-        let ready = parts[2];
-        if (authMethod === 'Basic') {
-          token = Base64.decode(token);
-        }
-
-        const hasInvalidCredentials = !!ready.code && ready.code === CloseStatus.PROTOCOL_FAILURE;
-        const shouldRetryLogin = (!hasInvalidCredentials && !ready.ready)
-          || (!!token && ready.ready);
-
-        if (shouldRetryLogin) {
-          checkAuth(token, authMethod);
-        }
-    });
     return () => sub.unsubscribe();
-  }, [serviceContext, serviceContext.login, checkAuth]);
+  }, [serviceContext, serviceContext.login, setAuthMethod]);
 
   return (
     <PageSection>

diff --git a/src/app/Shared/Services/Api.service.tsx b/src/app/Shared/Services/Api.service.tsx
@@ -40,7 +40,7 @@ import { fromFetch } from 'rxjs/fetch';
 import { catchError, concatMap, first, map, mergeMap, tap } from 'rxjs/operators';
 import { Target, TargetService } from './Target.service';
 import { Notifications } from '@app/Notifications/Notifications';
-import { LoginService } from './Login.service';
+import { LoginService, SessionState } from './Login.service';
 
 type ApiVersion = "v1" | "v2";
 
@@ -74,8 +74,8 @@ export class ApiService {
   ) {
 
     // show recording archives when recordings available
-    login.isAuthenticated().pipe(
-    concatMap((authenticated) => authenticated ? this.doGet('recordings') : EMPTY)
+    login.getSessionState().pipe(
+    concatMap((sessionState) => sessionState === SessionState.USER_SESSION ? this.doGet('recordings') : EMPTY)
     )
     .subscribe({
       next: () => {

diff --git a/src/app/Shared/Services/Login.service.tsx b/src/app/Shared/Services/Login.service.tsx
@@ -35,20 +35,26 @@
  * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
  * SOFTWARE.
  */
+import { Base64 } from 'js-base64';
 import { Observable, ObservableInput, of, ReplaySubject } from 'rxjs';
 import { fromFetch } from 'rxjs/fetch';
 import { catchError, concatMap, first, map, tap } from 'rxjs/operators';
 
+export enum SessionState {
+  NO_USER_SESSION,
+  CREATING_USER_SESSION,
+  USER_SESSION
+}
+
 export class LoginService {
 
   private readonly TOKEN_KEY: string = 'token';
-  private readonly METHOD_KEY: string = 'method';
   private readonly USER_KEY: string = 'user';
   private readonly token = new ReplaySubject<string>(1);
   private readonly authMethod = new ReplaySubject<string>(1);
   private readonly logout = new ReplaySubject<void>(1);
   private readonly username = new ReplaySubject<string>(1);
-  private readonly authenticated = new ReplaySubject<boolean>(1);
+  private readonly sessionState = new ReplaySubject<SessionState>(1);
   readonly authority: string;
 
   constructor() {
@@ -58,12 +64,22 @@ export class LoginService {
     }
     this.authority = apiAuthority;
     this.token.next(this.getCacheItem(this.TOKEN_KEY));
-    this.authMethod.next(this.getCacheItem(this.METHOD_KEY));
     this.username.next(this.getCacheItem(this.USER_KEY));
-    this.authenticated.next(false);
+    this.sessionState.next(SessionState.NO_USER_SESSION);
+    this.queryAuthMethod();
   }
 
-  checkAuth(token: string, method: string): Observable<boolean> {
+  queryAuthMethod(): void {
+      this.checkAuth('', '').subscribe(() => {
+        ; // check auth once at component load to query the server's auth method
+      });
+  }
+
+  checkAuth(token: string, method: string, rememberMe = false): Observable<boolean> {
+
+    if (method === 'Basic') {
+      token = Base64.encodeURL(token);
+    }
 
     token = this.useCacheItemIfAvailable(this.TOKEN_KEY, token);
 
@@ -85,9 +101,9 @@ export class LoginService {
       tap((jsonResp: AuthV2Response) => {
         if(jsonResp.meta.status === 'OK') {
           this.completeAuthMethod(method);
+          this.decideRememberToken(token, rememberMe);
           this.setUsername(jsonResp.data.result.username);
-          this.token.next(token);
-          this.authenticated.next(true);
+          this.sessionState.next(SessionState.CREATING_USER_SESSION);
         }
       }),
       map((jsonResp: AuthV2Response) => {
@@ -122,8 +138,8 @@ export class LoginService {
     return this.username.asObservable();
   }
 
-  isAuthenticated(): Observable<boolean> {
-    return this.authenticated.asObservable();
+  getSessionState(): Observable<SessionState> {
+    return this.sessionState.asObservable();
   }
 
   loggedOut(): Observable<void> {
@@ -132,19 +148,25 @@ export class LoginService {
 
   setLoggedOut(): void {
     this.removeCacheItem(this.USER_KEY);
-    this.forgetToken();
+    this.removeCacheItem(this.TOKEN_KEY);
     this.token.next('');
     this.username.next('');
     this.logout.next();
-    this.authenticated.next(false);
+    this.sessionState.next(SessionState.NO_USER_SESSION);
   }
 
-  rememberToken(token: string): void {
-    this.setCacheItem(this.TOKEN_KEY, token);
+  setSessionState(state: SessionState): void {
+    this.sessionState.next(state);
   }
 
-  forgetToken(): void {
-    this.removeCacheItem(this.TOKEN_KEY);
+  private decideRememberToken(token: string, rememberMe: boolean): void {
+    this.token.next(token);
+
+    if(rememberMe && !!token) {
+      this.setCacheItem(this.TOKEN_KEY, token);
+    } else {
+      this.removeCacheItem(this.TOKEN_KEY);
+    }
   }
 
   private setUsername(username: string): void {
@@ -154,7 +176,6 @@ export class LoginService {
 
   private completeAuthMethod(method: string): void {
     this.authMethod.next(method);
-    this.setCacheItem(this.METHOD_KEY, method);
     this.authMethod.complete();
   }
 

diff --git a/src/app/Shared/Services/NotificationChannel.service.tsx b/src/app/Shared/Services/NotificationChannel.service.tsx
@@ -36,13 +36,13 @@
  * SOFTWARE.
  */
 import { Notifications } from '@app/Notifications/Notifications';
-import { BehaviorSubject, combineLatest, Observable, Subject } from 'rxjs';
+import { BehaviorSubject, combineLatest, Observable, Subject, timer } from 'rxjs';
 import { fromFetch } from 'rxjs/fetch';
 import { webSocket, WebSocketSubject } from 'rxjs/webSocket';
 import { concatMap, distinctUntilChanged, filter } from 'rxjs/operators';
 import { Base64 } from 'js-base64';
 import * as _ from 'lodash';
-import { LoginService } from './Login.service';
+import { LoginService, SessionState } from './Login.service';
 
 interface RecordingNotificationEvent {
   recording: string;
@@ -119,18 +119,21 @@ export class NotificationChannel {
         })
       );
 
-    combineLatest(notificationsUrl, this.login.getToken(), this.login.getAuthMethod())
-      .pipe(distinctUntilChanged(_.isEqual))
-      .subscribe({
+    combineLatest([notificationsUrl, this.login.getToken(), this.login.getAuthMethod(), this.login.getSessionState(), timer(0, 5000)])
+    .pipe(distinctUntilChanged(_.isEqual))
+    .subscribe({
         next: (parts: string[]) => {
           const url = parts[0];
           const token = parts[1];
           const authMethod = parts[2];
+          const sessionState = parseInt(parts[3]);
           let subprotocol: string | undefined = undefined;
 
-          if(!token) {
+          if(sessionState !== SessionState.CREATING_USER_SESSION) {
             return;
-          } else if (authMethod === 'Bearer') {
+          }
+
+          if (authMethod === 'Bearer') {
             subprotocol = `base64url.bearer.authorization.cryostat.${Base64.encodeURL(token)}`;
           } else if (authMethod === 'Basic') {
             subprotocol = `basic.authorization.cryostat.${token}`;
@@ -146,35 +149,42 @@ export class NotificationChannel {
             openObserver: {
               next: () => {
                 this._ready.next({ ready: true });
+                this.login.setSessionState(SessionState.USER_SESSION);
               }
             },
             closeObserver: {
               next: (evt) => {
                 let code: CloseStatus;
                 let msg: string | undefined = undefined;
                 let fn: Function;
+                let sessionState: SessionState;
                 switch (evt.code) {
                   case CloseStatus.LOGGED_OUT:
                     code = CloseStatus.LOGGED_OUT;
                     msg = 'Logout success';
                     fn = this.notifications.info;
+                    sessionState = SessionState.NO_USER_SESSION;
                     break;
                   case CloseStatus.PROTOCOL_FAILURE:
                     code = CloseStatus.PROTOCOL_FAILURE;
                     msg = 'Authentication failed';
                     fn = this.notifications.danger;
+                    sessionState = SessionState.NO_USER_SESSION;
                     break;
                   case CloseStatus.INTERNAL_ERROR:
                     code = CloseStatus.INTERNAL_ERROR;
                     msg = 'Internal server error';
                     fn = this.notifications.danger;
+                    sessionState = SessionState.CREATING_USER_SESSION;
                     break;
                   default:
                     code = CloseStatus.UNKNOWN;
                     fn = this.notifications.info;
+                    sessionState = SessionState.CREATING_USER_SESSION;
                     break;
                 }
                 this._ready.next({ ready: false, code });
+                this.login.setSessionState(sessionState);
                 fn.apply(this.notifications, ['WebSocket connection lost', msg, NotificationCategory.WsClientActivity]);
               }
             }
@@ -198,6 +208,7 @@ export class NotificationChannel {
       },
       error: (err: any) => this.logError('Notifications URL configuration', err)
     });
+
   }
 
   isReady(): Observable<ReadyState> {

diff --git a/src/app/Shared/Services/Targets.service.tsx b/src/app/Shared/Services/Targets.service.tsx
@@ -43,7 +43,7 @@ import { Notifications } from '@app/Notifications/Notifications';
 import { NotificationCategory, NotificationChannel } from './NotificationChannel.service';
 import { Observable, BehaviorSubject, of, EMPTY } from 'rxjs';
 import { catchError, concatMap, first, map, tap } from 'rxjs/operators';
-import { LoginService } from './Login.service';
+import { LoginService, SessionState } from './Login.service';
 
 export interface TargetDiscoveryEvent {
   kind: 'LOST' | 'FOUND';
@@ -59,8 +59,8 @@ export class TargetsService {
     private readonly login: LoginService,
     notificationChannel: NotificationChannel,
     ) {
-    login.isAuthenticated().pipe(
-      concatMap((authenticated) => authenticated ? this.queryForTargets() : EMPTY)
+    login.getSessionState().pipe(
+      concatMap((sessionState) => sessionState === SessionState.USER_SESSION ? this.queryForTargets() : EMPTY)
       )
       .subscribe(() => {
       ; // just trigger a startup query

diff --git a/src/app/routes.tsx b/src/app/routes.tsx
@@ -50,7 +50,7 @@ import { accessibleRouteChangeHandler } from '@app/utils/utils';
 import { Route, RouteComponentProps, Switch } from 'react-router-dom';
 import { LastLocationProvider, useLastLocation } from 'react-router-last-location';
 import { About } from './About/About';
-import { combineLatest } from 'rxjs';
+import { SessionState } from './Shared/Services/Login.service';
 
 let routeFocusTimer: number;
 const OVERVIEW = 'Overview';
@@ -174,18 +174,9 @@ const AppRoutes = () => {
   const [showDashboard, setShowDashboard] = React.useState(false);
 
   React.useEffect(() => {
-    const sub = combineLatest(context.notificationChannel.isReady(), context.login.isAuthenticated()).subscribe(
-      (parts) => {
-        const connected = parts[0].ready;
-        const authenticated = parts[1];
-
-        if (connected && authenticated) {
-          setShowDashboard(true);
-        } else {
-          setShowDashboard(false);
-        }
-      }
-    );
+    const sub = context.login
+      .getSessionState()
+      .subscribe(sessionState => setShowDashboard(sessionState === SessionState.USER_SESSION));
     return () => sub.unsubscribe();
   }, [context, context.login, setShowDashboard]);