From 87f6e2564b0fc980ec6799d245f167d05e427e65 Mon Sep 17 00:00:00 2001 From: Lucio Lelii Date: Sun, 20 Sep 2026 11:29:58 +0200 Subject: [PATCH] Drop cached projects/flows on login or logout, not just app reload ProjectsService and FlowsService are root singletons that load their list once and cache it for the app's lifetime. Logout never cleared that cache, so a second person signing in on the same tab without a hard reload still saw whoever's projects and flows were loaded before them - no backend request was even made. Authorization now emits userChanged whenever the signed-in identity actually changes (never on the initial hydration from storage), and both services reset their cache on it. Co-Authored-By: Claude Sonnet 5 --- .../authorization/authorization.spec.ts | 66 +++++++++++++++++++ .../services/authorization/authorization.ts | 19 +++++- src/app/services/flows/flows.spec.ts | 20 ++++++ src/app/services/flows/flows.ts | 14 +++- src/app/services/projects/projects.spec.ts | 20 ++++++ src/app/services/projects/projects.ts | 16 ++++- .../flows-list/flow-item/flow-item.spec.ts | 3 +- 7 files changed, 154 insertions(+), 4 deletions(-) diff --git a/src/app/services/authorization/authorization.spec.ts b/src/app/services/authorization/authorization.spec.ts index ab9be71..67be71f 100644 --- a/src/app/services/authorization/authorization.spec.ts +++ b/src/app/services/authorization/authorization.spec.ts @@ -3,6 +3,8 @@ // Attribution term under AGPL-3.0 section 7(b): see LICENSE-ADDENDUM. import { TestBed } from '@angular/core/testing'; +import { of, throwError } from 'rxjs'; +import { vi } from 'vitest'; import { Authorization } from './authorization'; @@ -17,4 +19,68 @@ describe('Authorization', () => { it('should be created', () => { expect(service).toBeTruthy(); }); + + describe('userChanged', () => { + /** + * A per-user cache (ProjectsService, FlowsService) subscribes to this to know when to drop + * what it is holding. If it fired on every persisted state, or missed a real transition, that + * cache would either thrash on every session check or leak one user's data into another's. + */ + it('fires with the new username on login', () => { + service.authCall.login = vi.fn(() => of({ username: 'alice', email: null, role: 'USER' })) as any; + const seen: (string | null)[] = []; + service.userChanged.subscribe((username) => seen.push(username)); + + service.login('alice', 'secret').subscribe(); + + expect(seen).toEqual(['alice']); + }); + + it('fires with null on logout', () => { + service.authCall.login = vi.fn(() => of({ username: 'alice', email: null, role: 'USER' })) as any; + service.authCall.logout = vi.fn(() => of(void 0)) as any; + service.login('alice', 'secret').subscribe(); + const seen: (string | null)[] = []; + service.userChanged.subscribe((username) => seen.push(username)); + + service.logout().subscribe(); + + expect(seen).toEqual([null]); + }); + + it('fires again when a different account logs in over the current one', () => { + service.authCall.login = vi.fn() + .mockReturnValueOnce(of({ username: 'alice', email: null, role: 'USER' })) + .mockReturnValueOnce(of({ username: 'bob', email: null, role: 'USER' })) as any; + service.login('alice', 'secret').subscribe(); + const seen: (string | null)[] = []; + service.userChanged.subscribe((username) => seen.push(username)); + + service.login('bob', 'secret').subscribe(); + + expect(seen).toEqual(['bob']); + }); + + it('does not fire when a session check confirms the same user is still signed in', () => { + service.authCall.login = vi.fn(() => of({ username: 'alice', email: null, role: 'USER' })) as any; + service.authCall.currentUser = vi.fn(() => of({ username: 'alice', email: null, role: 'USER' })) as any; + service.login('alice', 'secret').subscribe(); + const seen: (string | null)[] = []; + service.userChanged.subscribe((username) => seen.push(username)); + + service.validateSession().subscribe(); + + expect(seen).toEqual([]); + }); + + it('does not fire for a failed login attempt', () => { + service.authCall.login = vi.fn(() => throwError(() => new Error('bad credentials'))) as any; + const seen: (string | null)[] = []; + service.userChanged.subscribe((username) => seen.push(username)); + + service.login('alice', 'wrong').subscribe({ error: () => {} }); + + expect(seen).toEqual([]); + }); + }); }); diff --git a/src/app/services/authorization/authorization.ts b/src/app/services/authorization/authorization.ts index 5df3b9f..859e10b 100644 --- a/src/app/services/authorization/authorization.ts +++ b/src/app/services/authorization/authorization.ts @@ -10,7 +10,7 @@ import { } from '@models/user'; import { AuthorizationCallServiceBase } from './authorization-call.base'; import { environment } from '@environment'; -import { Observable, catchError, finalize, map, of, shareReplay, take, tap, throwError } from 'rxjs'; +import { Observable, Subject, catchError, finalize, map, of, shareReplay, take, tap, throwError } from 'rxjs'; @Injectable({ providedIn: 'root', @@ -25,6 +25,17 @@ export class Authorization { loggedInUser = this.user.asReadonly(); + private readonly userChanged$ = new Subject(); + /** + * Emits the new username (or null) whenever the signed-in identity actually changes - login, + * logout, or one account replacing another - but never for the initial hydration from storage on + * app boot, since that is a page load continuing a session, not a change. A per-user client cache + * (`ProjectsService`, `FlowsService`, ...) subscribes to invalidate itself: those services are + * `providedIn: 'root'` and outlive any one login, so without this a second person signing in on + * the same tab without a hard reload would still see whoever was logged in before them. + */ + readonly userChanged: Observable = this.userChanged$.asObservable(); + constructor() { this.restoreUserFromStorage(); } @@ -114,8 +125,14 @@ export class Authorization { private persistUserState(user: User | null) { const normalizedUser = this.normalizeUser(user); + const previousUsername = this.user()?.username ?? null; this.user.set(normalizedUser); + const nextUsername = normalizedUser?.username ?? null; + if (previousUsername !== nextUsername) { + this.userChanged$.next(nextUsername); + } + try { if (typeof localStorage === 'undefined') return; if (normalizedUser) { diff --git a/src/app/services/flows/flows.spec.ts b/src/app/services/flows/flows.spec.ts index 725c194..fa69bd9 100644 --- a/src/app/services/flows/flows.spec.ts +++ b/src/app/services/flows/flows.spec.ts @@ -4,6 +4,7 @@ import { TestBed } from '@angular/core/testing'; import { Flow, FlowData } from '@models/flow'; +import { Authorization } from '@services/authorization/authorization'; import { firstValueFrom, of, throwError } from 'rxjs'; import { vi } from 'vitest'; import { FlowsCallServiceBase } from './flows-call.base'; @@ -97,6 +98,25 @@ describe('FlowsService', () => { }); }); + it('drops the cached list when a different user signs in, so the next read refetches', async () => { + // Regression: this service is providedIn 'root' and outlives any one login. Without this, a + // second person signing in on the same tab kept seeing whoever's flows were loaded before. + callServiceSpy.retrieveAllFlows + .mockReturnValueOnce(of([makeFlow('alice-1')])) + .mockReturnValueOnce(of([makeFlow('bob-1')])); + await service.getAllFlows(); + expect(service.flows().map((flow) => flow.id)).toEqual(['alice-1']); + + const authorization = TestBed.inject(Authorization); + authorization.authCall.login = vi.fn(() => of({ username: 'bob', email: null, role: 'USER' })) as any; + authorization.login('bob', 'secret').subscribe(); + + expect(service.flows()).toEqual([]); + + await service.getAllFlows(); + expect(service.flows().map((flow) => flow.id)).toEqual(['bob-1']); + }); + describe('refresh', () => { it('should update flows signal', async () => { const flows = [makeFlow('r1')]; diff --git a/src/app/services/flows/flows.ts b/src/app/services/flows/flows.ts index f9de3a4..4fbb598 100644 --- a/src/app/services/flows/flows.ts +++ b/src/app/services/flows/flows.ts @@ -2,9 +2,10 @@ // SPDX-License-Identifier: AGPL-3.0-or-later // Attribution term under AGPL-3.0 section 7(b): see LICENSE-ADDENDUM. -import { Injectable, signal } from '@angular/core'; +import { inject, Injectable, signal } from '@angular/core'; import { environment } from '@environment'; import { Flow, FlowData } from '@models/flow'; +import { Authorization } from '@services/authorization/authorization'; import { FlowsCallServiceBase } from './flows-call.base'; import { catchError, firstValueFrom, Observable, of, switchMap, tap, throwError } from 'rxjs'; @@ -21,6 +22,17 @@ export class FlowsService { private _flows = signal([]); readonly flows = this._flows.asReadonly(); + constructor() { + // See Authorization.userChanged / ProjectsService: same cache-outlives-login leak, same fix. + inject(Authorization).userChanged.subscribe(() => this.reset()); + } + + reset() { + this._flows.set([]); + this.toInit = true; + this.loadingPromise = null; + } + hasLoadedFlows() { return this._flows().length > 0 || !this.toInit; } diff --git a/src/app/services/projects/projects.spec.ts b/src/app/services/projects/projects.spec.ts index 09833f5..20f4ac7 100644 --- a/src/app/services/projects/projects.spec.ts +++ b/src/app/services/projects/projects.spec.ts @@ -4,6 +4,7 @@ import { TestBed } from '@angular/core/testing'; import { Project } from '@models/project'; +import { Authorization } from '@services/authorization/authorization'; import { of, throwError } from 'rxjs'; import { vi } from 'vitest'; @@ -73,6 +74,25 @@ describe('ProjectsService', () => { expect(service.hasLoadedProjects()).toBe(false); }); + it('drops the cached list when a different user signs in, so the next read refetches', async () => { + // Regression: this service is providedIn 'root' and outlives any one login. Without this, a + // second person signing in on the same tab kept seeing whoever's projects were loaded before. + callServiceSpy.retrieveAllProjects + .mockReturnValueOnce(of([makeProject('p1', "Alice's project")])) + .mockReturnValueOnce(of([makeProject('p2', "Bob's project")])); + await service.getAllProjects(); + expect(service.projects().map((project) => project.id)).toEqual(['p1']); + + const authorization = TestBed.inject(Authorization); + authorization.authCall.login = vi.fn(() => of({ username: 'bob', email: null, role: 'USER' })) as any; + authorization.login('bob', 'secret').subscribe(); + + expect(service.projects()).toEqual([]); + + await service.getAllProjects(); + expect(service.projects().map((project) => project.id)).toEqual(['p2']); + }); + it('indexes projects by id for O(1) lookup from a flow', async () => { callServiceSpy.retrieveAllProjects.mockReturnValue(of([makeProject('p1', 'Recruiting')])); await service.getAllProjects(); diff --git a/src/app/services/projects/projects.ts b/src/app/services/projects/projects.ts index 4415005..a641dc8 100644 --- a/src/app/services/projects/projects.ts +++ b/src/app/services/projects/projects.ts @@ -2,9 +2,10 @@ // SPDX-License-Identifier: AGPL-3.0-or-later // Attribution term under AGPL-3.0 section 7(b): see LICENSE-ADDENDUM. -import { computed, Injectable, signal } from '@angular/core'; +import { computed, inject, Injectable, signal } from '@angular/core'; import { environment } from '@environment'; import { Project, ProjectContext, ProjectDraft } from '@models/project'; +import { Authorization } from '@services/authorization/authorization'; import { catchError, firstValueFrom, Observable, of, tap, throwError } from 'rxjs'; import { ProjectsCallServiceBase } from './projects-call.base'; @@ -24,6 +25,19 @@ export class ProjectsService { /** Lets a flow resolve its project in O(1) without threading inputs through the tree. */ readonly projectById = computed(() => new Map(this._projects().map((project) => [project.id, project]))); + constructor() { + // See Authorization.userChanged: without this, a second person signing in on the same tab + // would still see whoever loaded the page before them, since this service outlives any one login. + inject(Authorization).userChanged.subscribe(() => this.reset()); + } + + /** Drops the cached list so the next read has to ask the server again, as who is asking has changed. */ + reset() { + this._projects.set([]); + this.toInit = true; + this.loadingPromise = null; + } + hasLoadedProjects() { return this._projects().length > 0 || !this.toInit; } diff --git a/src/app/shared/flows-list/flow-item/flow-item.spec.ts b/src/app/shared/flows-list/flow-item/flow-item.spec.ts index d0c5698..5f978f0 100644 --- a/src/app/shared/flows-list/flow-item/flow-item.spec.ts +++ b/src/app/shared/flows-list/flow-item/flow-item.spec.ts @@ -51,7 +51,8 @@ describe('FlowItem', () => { { provide: Authorization, useValue: { - loggedInUser: vi.fn().mockReturnValue({ username: 'author' }) + loggedInUser: vi.fn().mockReturnValue({ username: 'author' }), + userChanged: of(null) } }, {