fix(auth): centralize token handling in auth service (#1534)

Author: zachyale

frontend/src/app/app.routes.spec.ts

@@ -1,7 +1,7 @@
 import {describe, expect, it} from 'vitest';
 
 import {routes} from './app.routes';
-import {AuthGuard} from './core/security/auth.guard';
+import {AuthChildGuard, AuthGuard} from './core/security/auth.guard';
 import {BookdropGuard} from './core/security/guards/bookdrop.guard';
 import {EditMetadataGuard} from './core/security/guards/edit-metdata.guard';
 import {LibraryStatsGuard} from './core/security/guards/library-stats.guard';
@@ -28,10 +28,11 @@ describe('app routes', () => {
     const children = shellRoute?.children ?? [];
 
     expect(children).toHaveLength(17);
-    expect(children.find(route => route.path === 'dashboard')?.canActivate).toEqual([AuthGuard]);
-    expect(children.find(route => route.path === 'all-books')?.canActivate).toEqual([AuthGuard]);
-    expect(children.find(route => route.path === 'magic-shelf/:magicShelfId/books')?.canActivate).toEqual([AuthGuard]);
-    expect(children.find(route => route.path === 'notebook')?.canActivate).toEqual([AuthGuard]);
+    expect(shellRoute?.canActivateChild).toEqual([AuthChildGuard]);
+    expect(children.find(route => route.path === 'dashboard')?.canActivate).toBeUndefined();
+    expect(children.find(route => route.path === 'all-books')?.canActivate).toBeUndefined();
+    expect(children.find(route => route.path === 'magic-shelf/:magicShelfId/books')?.canActivate).toBeUndefined();
+    expect(children.find(route => route.path === 'notebook')?.canActivate).toBeUndefined();
     expect(typeof children.find(route => route.path === 'all-books')?.loadComponent).toBe('function');
     expect(typeof children.find(route => route.path === 'library/:libraryId/books')?.loadComponent).toBe('function');
     expect(typeof children.find(route => route.path === 'shelf/:shelfId/books')?.loadComponent).toBe('function');

frontend/src/app/app.routes.ts

@@ -1,7 +1,7 @@
 import {Routes} from '@angular/router';
 import {AppLayoutComponent} from './shared/layout/layout-main/app.layout.component';
 import {LoginComponent} from './shared/components/login/login.component';
-import {AuthGuard} from './core/security/auth.guard';
+import {AuthChildGuard, AuthGuard} from './core/security/auth.guard';
 import {ChangePasswordComponent} from './shared/components/change-password/change-password.component';
 import {SetupComponent} from './shared/components/setup/setup.component';
 import {SetupGuard} from './shared/components/setup/setup.guard';
@@ -34,24 +34,25 @@ export const routes: Routes = [
   {
     path: '',
     component: AppLayoutComponent,
+    canActivateChild: [AuthChildGuard],
     children: [
-      {path: 'dashboard', component: MainDashboardComponent, canActivate: [AuthGuard]},
-      {path: 'all-books', loadComponent: loadBookBrowserComponent, canActivate: [AuthGuard]},
-      {path: 'settings', loadComponent: () => import('./features/settings/settings.component').then(m => m.SettingsComponent), canActivate: [AuthGuard]},
-      {path: 'library/:libraryId/books', loadComponent: loadBookBrowserComponent, canActivate: [AuthGuard]},
-      {path: 'shelf/:shelfId/books', loadComponent: loadBookBrowserComponent, canActivate: [AuthGuard]},
-      {path: 'unshelved-books', loadComponent: loadBookBrowserComponent, canActivate: [AuthGuard]},
-      {path: 'series', loadComponent: () => import('./features/series-browser/components/series-browser/series-browser.component').then(m => m.SeriesBrowserComponent), canActivate: [AuthGuard]},
-      {path: 'series/:seriesName', loadComponent: () => import('./features/book/components/series-page/series-page.component').then(m => m.SeriesPageComponent), canActivate: [AuthGuard]},
-      {path: 'authors', loadComponent: () => import('./features/author-browser/components/author-browser/author-browser.component').then(m => m.AuthorBrowserComponent), canActivate: [AuthGuard]},
-      {path: 'author/:authorId', loadComponent: () => import('./features/author-browser/components/author-detail/author-detail.component').then(m => m.AuthorDetailComponent), canActivate: [AuthGuard]},
-      {path: 'magic-shelf/:magicShelfId/books', loadComponent: loadBookBrowserComponent, canActivate: [AuthGuard]},
-      {path: 'book/:bookId', loadComponent: () => import('./features/metadata/component/book-metadata-center/book-metadata-center.component').then(m => m.BookMetadataCenterComponent), canActivate: [AuthGuard]},
+      {path: 'dashboard', component: MainDashboardComponent},
+      {path: 'all-books', loadComponent: loadBookBrowserComponent},
+      {path: 'settings', loadComponent: () => import('./features/settings/settings.component').then(m => m.SettingsComponent)},
+      {path: 'library/:libraryId/books', loadComponent: loadBookBrowserComponent},
+      {path: 'shelf/:shelfId/books', loadComponent: loadBookBrowserComponent},
+      {path: 'unshelved-books', loadComponent: loadBookBrowserComponent},
+      {path: 'series', loadComponent: () => import('./features/series-browser/components/series-browser/series-browser.component').then(m => m.SeriesBrowserComponent)},
+      {path: 'series/:seriesName', loadComponent: () => import('./features/book/components/series-page/series-page.component').then(m => m.SeriesPageComponent)},
+      {path: 'authors', loadComponent: () => import('./features/author-browser/components/author-browser/author-browser.component').then(m => m.AuthorBrowserComponent)},
+      {path: 'author/:authorId', loadComponent: () => import('./features/author-browser/components/author-detail/author-detail.component').then(m => m.AuthorDetailComponent)},
+      {path: 'magic-shelf/:magicShelfId/books', loadComponent: loadBookBrowserComponent},
+      {path: 'book/:bookId', loadComponent: () => import('./features/metadata/component/book-metadata-center/book-metadata-center.component').then(m => m.BookMetadataCenterComponent)},
       {path: 'bookdrop', loadComponent: () => import('./features/bookdrop/component/bookdrop-file-review/bookdrop-file-review.component').then(m => m.BookdropFileReviewComponent), canActivate: [BookdropGuard]},
       {path: 'metadata-manager', loadComponent: () => import('./features/metadata/component/metadata-manager/metadata-manager.component').then(m => m.MetadataManagerComponent), canActivate: [EditMetadataGuard]},
       {path: 'library-stats', loadComponent: () => import('./features/stats/component/library-stats/library-stats.component').then(m => m.LibraryStatsComponent), canActivate: [LibraryStatsGuard]},
       {path: 'reading-stats', loadComponent: () => import('./features/stats/component/user-stats/user-stats.component').then(m => m.UserStatsComponent), canActivate: [UserStatsGuard]},
-      {path: 'notebook', loadComponent: () => import('./features/notebook/components/notebook/notebook.component').then(m => m.NotebookComponent), canActivate: [AuthGuard]},
+      {path: 'notebook', loadComponent: () => import('./features/notebook/components/notebook/notebook.component').then(m => m.NotebookComponent)},
     ]
   },
   {

frontend/src/app/core/security/auth-interceptor.service.spec.ts

@@ -10,8 +10,7 @@ import { AuthInterceptorService } from './auth-interceptor.service';
 describe('AuthInterceptorService', () => {
   const authService = {
     getInternalAccessToken: vi.fn<() => string | null>(),
-    internalRefreshToken: vi.fn<() => Observable<{ accessToken: string; refreshToken: string }>>(),
-    saveInternalTokens: vi.fn<(accessToken: string, refreshToken: string) => void>(),
+    ensureAccessToken: vi.fn<(options?: {forceRefresh?: boolean}) => Observable<string>>(),
     logout: vi.fn<() => void>(),
   };
 
@@ -21,8 +20,7 @@ describe('AuthInterceptorService', () => {
   beforeEach(() => {
     vi.restoreAllMocks();
     authService.getInternalAccessToken.mockReset();
-    authService.internalRefreshToken.mockReset();
-    authService.saveInternalTokens.mockReset();
+    authService.ensureAccessToken.mockReset();
     authService.logout.mockReset();
 
     TestBed.resetTestingModule();
@@ -82,7 +80,7 @@ describe('AuthInterceptorService', () => {
 
   it('retries a 401 request after a successful refresh', async () => {
     authService.getInternalAccessToken.mockReturnValue('expired-token');
-    authService.internalRefreshToken.mockReturnValue(of({ accessToken: 'fresh-token', refreshToken: 'refresh-token' }));
+    authService.ensureAccessToken.mockReturnValue(of('fresh-token'));
 
     const next = vi.fn((request: HttpRequest<unknown>) => {
       const authHeader = request.headers.get('Authorization');
@@ -97,34 +95,47 @@ describe('AuthInterceptorService', () => {
       next
     ));
 
-    expect(authService.internalRefreshToken).toHaveBeenCalledOnce();
-    expect(authService.saveInternalTokens).toHaveBeenCalledWith('fresh-token', 'refresh-token');
+    expect(authService.ensureAccessToken).toHaveBeenCalledWith({forceRefresh: true});
     expect((response as HttpResponse<string>).body).toBe('Bearer fresh-token');
   });
 
-  it('retries a 401 request without persisting tokens when the refresh response is incomplete', async () => {
+  it('logs out when AuthService cannot refresh a complete token pair', async () => {
     authService.getInternalAccessToken.mockReturnValue('expired-token');
-    authService.internalRefreshToken.mockReturnValue(of({ accessToken: 'fresh-token', refreshToken: '' }));
+    authService.ensureAccessToken.mockReturnValue(throwError(() => new Error('Authentication response did not include a complete token pair')));
+    const next = vi.fn(() => throwError(() => new HttpErrorResponse({ status: 401 })));
+
+    await expect(firstValueFrom(interceptor(
+      new HttpRequest('GET', `${apiUrl}/books`),
+      next
+    ))).rejects.toBeInstanceOf(Error);
+
+    expect(authService.ensureAccessToken).toHaveBeenCalledWith({forceRefresh: true});
+    expect(authService.logout).toHaveBeenCalledOnce();
+  });
+
+  it('does not log out when the retried request fails after refresh succeeds', async () => {
+    authService.getInternalAccessToken.mockReturnValue('expired-token');
+    authService.ensureAccessToken.mockReturnValue(of('fresh-token'));
 
     const next = vi.fn((request: HttpRequest<unknown>) => {
       if (request.headers.get('Authorization') === 'Bearer fresh-token') {
-        return of(new HttpResponse({ status: 200, body: request.headers.get('Authorization') }));
+        return throwError(() => new HttpErrorResponse({ status: 500 }));
       }
       return throwError(() => new HttpErrorResponse({ status: 401 }));
     });
 
-    const response = await firstValueFrom(interceptor(
+    await expect(firstValueFrom(interceptor(
       new HttpRequest('GET', `${apiUrl}/books`),
       next
-    ));
+    ))).rejects.toBeInstanceOf(HttpErrorResponse);
 
-    expect(authService.saveInternalTokens).not.toHaveBeenCalled();
-    expect((response as HttpResponse<string>).body).toBe('Bearer fresh-token');
+    expect(authService.ensureAccessToken).toHaveBeenCalledWith({forceRefresh: true});
+    expect(authService.logout).not.toHaveBeenCalled();
   });
 
   it('logs out when the refresh request fails', async () => {
     authService.getInternalAccessToken.mockReturnValue('expired-token');
-    authService.internalRefreshToken.mockReturnValue(
+    authService.ensureAccessToken.mockReturnValue(
       throwError(() => new HttpErrorResponse({ status: 500 }))
     );
 
@@ -147,14 +158,14 @@ describe('AuthInterceptorService', () => {
       next
     ))).rejects.toBeInstanceOf(HttpErrorResponse);
 
-    expect(authService.internalRefreshToken).not.toHaveBeenCalled();
+    expect(authService.ensureAccessToken).not.toHaveBeenCalled();
     expect(authService.logout).not.toHaveBeenCalled();
   });
 
-  it('queues concurrent 401s behind a single refresh operation', async () => {
+  it('delegates concurrent 401 refresh coordination to AuthService', async () => {
     authService.getInternalAccessToken.mockReturnValue('expired-token');
-    const refreshSubject = new Subject<{ accessToken: string; refreshToken: string }>();
-    authService.internalRefreshToken.mockReturnValue(refreshSubject.asObservable());
+    const refreshSubject = new Subject<string>();
+    authService.ensureAccessToken.mockReturnValue(refreshSubject.asObservable());
 
     const next = vi.fn((request: HttpRequest<unknown>) => {
       const authHeader = request.headers.get('Authorization');
@@ -167,12 +178,13 @@ describe('AuthInterceptorService', () => {
     const firstRequest = firstValueFrom(interceptor(new HttpRequest('GET', `${apiUrl}/books`), next));
     const secondRequest = firstValueFrom(interceptor(new HttpRequest('GET', `${apiUrl}/libraries`), next));
 
-    refreshSubject.next({ accessToken: 'refreshed-token', refreshToken: 'refresh-token' });
+    refreshSubject.next('refreshed-token');
     refreshSubject.complete();
 
     const [firstResponse, secondResponse] = await Promise.all([firstRequest, secondRequest]);
 
-    expect(authService.internalRefreshToken).toHaveBeenCalledOnce();
+    expect(authService.ensureAccessToken).toHaveBeenCalledTimes(2);
+    expect(authService.ensureAccessToken).toHaveBeenCalledWith({forceRefresh: true});
     expect((firstResponse as HttpResponse<string>).body).toBe('Bearer refreshed-token');
     expect((secondResponse as HttpResponse<string>).body).toBe('Bearer refreshed-token');
   });

frontend/src/app/core/security/auth-interceptor.service.ts

@@ -1,13 +1,10 @@
 import { HttpErrorResponse, HttpEvent, HttpHandlerFn, HttpInterceptorFn, HttpRequest } from '@angular/common/http';
 import { inject } from '@angular/core';
-import { catchError, filter, switchMap, take } from 'rxjs/operators';
-import { BehaviorSubject, Observable, throwError, defer } from 'rxjs';
+import { catchError, switchMap } from 'rxjs/operators';
+import { Observable, throwError, defer } from 'rxjs';
 import { AuthService } from '../../shared/service/auth.service';
 import { API_CONFIG } from '../config/api-config';
 
-let isRefreshing = false;
-const refreshTokenSubject = new BehaviorSubject<string | null>(null);
-
 export const AuthInterceptorService: HttpInterceptorFn = (req, next: HttpHandlerFn) => {
   const authService = inject(AuthService);
 
@@ -42,36 +39,14 @@ export const AuthInterceptorService: HttpInterceptorFn = (req, next: HttpHandler
 
 function handle401Error(authService: AuthService, request: HttpRequest<unknown>, next: HttpHandlerFn): Observable<HttpEvent<unknown>> {
   return defer(() => {
-    if (!isRefreshing) {
-      isRefreshing = true;
-      refreshTokenSubject.next(null);
-
-      return authService.internalRefreshToken().pipe(
-        switchMap(response => {
-          isRefreshing = false;
-          const { accessToken, refreshToken } = response;
-          if (accessToken && refreshToken) {
-            authService.saveInternalTokens(accessToken, refreshToken);
-            refreshTokenSubject.next(accessToken);
-          }
-          return next(request.clone({
-            setHeaders: { Authorization: `Bearer ${accessToken}` }
-          }));
-        }),
-        catchError(err => {
-          isRefreshing = false;
-          forceLogout(authService);
-          return throwError(() => err);
-        })
-      );
-    }
-
-    return refreshTokenSubject.pipe(
-      filter(token => token !== null),
-      take(1),
-      switchMap(token =>
+    return authService.ensureAccessToken({forceRefresh: true}).pipe(
+      catchError(err => {
+        forceLogout(authService);
+        return throwError(() => err);
+      }),
+      switchMap(accessToken =>
         next(request.clone({
-          setHeaders: { Authorization: `Bearer ${token}` }
+          setHeaders: { Authorization: `Bearer ${accessToken}` }
         }))
       )
     );