test(frontend): close the user, hub and resource-registry service gaps

frontend/src/app/common/service/user/email-request-modal/email-request-modal.component.spec.ts

@@ -140,6 +140,35 @@ describe("EmailRequestModalComponent", () => {
     expect(code.getAttribute("autocomplete")).toBe("one-time-code");
   });
 
+  /**
+   * The spec above reads the code box's rendered attributes; nothing had ever typed into it, so its
+   * [(ngModel)] write-back had never fired. That is the direction that matters: the dialog's caller
+   * reads the code out of getValues(), and a one-way binding here would hand it a blank code with
+   * no visible symptom on screen.
+   */
+  it("writes the typed verification code back through ngModel", async () => {
+    const fixture = await createFixture({ name: "Sofia" });
+    fixture.detectChanges();
+    fixture.componentInstance.email = "sofia@example.com";
+    fixture.componentInstance.step = "code";
+    fixture.detectChanges();
+
+    const [, code] = inputs(fixture);
+    code.value = "246810";
+    code.dispatchEvent(new Event("input"));
+    fixture.detectChanges();
+
+    expect(fixture.componentInstance.code).toBe("246810");
+    // The two boxes are not crossed: the frozen address is still the one it was mailed to.
+    expect(fixture.componentInstance.getValues()).toEqual({ email: "sofia@example.com", code: "246810" });
+    // ...and not only in the fields. This is the one test that renders the code step with a
+    // non-empty code, so it is the only place the label naming where the code went can be told
+    // apart from the code itself; through getValues() the two are indistinguishable on screen.
+    const label = (fixture.nativeElement as HTMLElement).querySelector(".email-modal-code-label");
+    expect(label?.textContent).toContain("sofia@example.com");
+    expect(label?.textContent).not.toContain("246810");
+  });
+
   function inputs(fixture: ComponentFixture<EmailRequestModalComponent>): HTMLInputElement[] {
     return Array.from(fixture.nativeElement.querySelectorAll("input"));
   }

frontend/src/app/common/service/user/user.service.spec.ts

@@ -106,6 +106,34 @@ describe("UserService", () => {
     expect(service.isLogin()).toBe(true);
   });
 
+  // The verify endpoint answers with `accessToken: string | null`, so a backend that accepted the
+  // code but declined to mint a session is representable. registerVerify substitutes "" for the
+  // absent token. What is pinned here is narrow and deliberate: no *usable* token is left behind,
+  // so any substituted placeholder fails.
+  //
+  // What is deliberately NOT pinned is whether the absent-token path runs `handleAccessToken` at
+  // all. It does today, and that is the defect: it writes "" over whatever token was there and
+  // fires the post-login @RolesAllowed config fetch for a session that was never granted. With a
+  // token already in localStorage that path reaches `AuthService.loginWithExistingToken`, whose
+  // `token == null` guard does not catch "", so an unrelated failed verification pops "Access
+  // token is expired!" and signs the current user out. `register()` above handles the same
+  // `string | null` shape by short-circuiting instead. Asserting either symptom — the "" write, or
+  // the config fetch firing — would cement that, so this test stays silent about it and accepts
+  // that a short-circuiting rewrite of line 108 is not distinguishable from here.
+  it("leaves no usable session when registerVerify answers without a token", async () => {
+    const auth = TestBed.inject(AuthService) as unknown as StubAuthService;
+    vi.spyOn(auth, "registerVerify").mockReturnValue(of({ accessToken: null }));
+
+    await firstValueFrom(service.registerVerify("pending", "pending@example.com", "password", "123456"));
+
+    // Tolerant of both "" (today) and null (a short-circuiting fix), intolerant of any substituted
+    // placeholder. `isLogin()` is deliberately not asserted next to it: the stub's
+    // loginWithExistingToken answers with a user only for MOCK_TOKEN, which no mutation of this
+    // path can synthesise out of `{ accessToken: null }`, so that assertion could never have
+    // failed and read as assurance it did not provide.
+    expect(AuthService.getAccessToken() ?? "").toBe("");
+  });
+
   it("should not login after login failed", () => {
     expect((service as any).currentUser).toBeFalsy();
     service
@@ -260,30 +288,42 @@ describe("UserService", () => {
     // so stub them deterministically and restore the originals afterwards.
     let originalFetch: typeof globalThis.fetch;
     let originalCreateObjectURL: typeof URL.createObjectURL;
+    let originalRevokeObjectURL: typeof URL.revokeObjectURL;
 
     beforeEach(() => {
       originalFetch = globalThis.fetch;
       originalCreateObjectURL = URL.createObjectURL;
+      originalRevokeObjectURL = URL.revokeObjectURL;
     });
 
     afterEach(() => {
       globalThis.fetch = originalFetch;
       URL.createObjectURL = originalCreateObjectURL;
+      URL.revokeObjectURL = originalRevokeObjectURL;
     });
 
     it("fetches the avatar, wraps the blob in an object URL, and caches it", async () => {
       const blob = new Blob(["img"]);
       globalThis.fetch = vi.fn().mockResolvedValue({ ok: true, blob: () => Promise.resolve(blob) }) as any;
       URL.createObjectURL = vi.fn().mockReturnValue("blob:fetched");
 
-      const result = await firstValueFrom(service.getAvatar(AVATAR_URL));
+      // Held rather than subscribed inline, and subscribed twice: the returned observable replays
+      // its one value to every later subscriber. Under a refCounted share the first subscriber's
+      // unsubscribe would tear the buffer down, so the second would refetch and mint a second
+      // object URL over the cache entry — orphaning the first blob with nothing left holding the
+      // reference needed to revoke it. Same leak class as the expired-entry revoke below.
+      const avatar$ = service.getAvatar(AVATAR_URL);
+
+      expect(await firstValueFrom(avatar$)).toBe("blob:fetched");
+      expect(await firstValueFrom(avatar$)).toBe("blob:fetched");
 
-      expect(result).toBe("blob:fetched");
       // fetched verbatim — no CDN prefix is reconstructed here any more
       expect(globalThis.fetch).toHaveBeenCalledWith(AVATAR_URL, {
         referrerPolicy: "no-referrer",
       });
       expect(URL.createObjectURL).toHaveBeenCalledWith(blob);
+      expect(globalThis.fetch).toHaveBeenCalledTimes(1);
+      expect(URL.createObjectURL).toHaveBeenCalledTimes(1);
     });
 
     it("fetches an avatar hosted anywhere the backend allowed, not just Google's CDN", async () => {
@@ -296,6 +336,45 @@ describe("UserService", () => {
       expect(globalThis.fetch).toHaveBeenCalledWith(otherHost, { referrerPolicy: "no-referrer" });
     });
 
+    // The freshness check has an expiry side to it: an object URL that outlives its entry has to be
+    // handed back to the browser, or every avatar refresh leaks the blob it was holding. The
+    // "returns the cached object URL while the entry is still fresh" spec above only ever exercised
+    // the other side of the same comparison.
+    it("revokes the stale object URL and refetches once the cached entry has expired", async () => {
+      const blob = new Blob(["fresh-img"]);
+      globalThis.fetch = vi.fn().mockResolvedValue({ ok: true, blob: () => Promise.resolve(blob) }) as any;
+      URL.createObjectURL = vi.fn().mockReturnValue("blob:fresh");
+      URL.revokeObjectURL = vi.fn();
+      (service as any).cache.set(AVATAR_URL, { url: "blob:stale", expiry: Date.now() - 1 });
+
+      const result = await firstValueFrom(service.getAvatar(AVATAR_URL));
+
+      // Released by its own url, not by the cache key it was filed under.
+      expect(URL.revokeObjectURL).toHaveBeenCalledWith("blob:stale");
+      expect(globalThis.fetch).toHaveBeenCalledWith(AVATAR_URL, { referrerPolicy: "no-referrer" });
+      expect(result).toBe("blob:fresh");
+      // The whole entry, not just its url. Both freshness tests hand the comparison an expiry the
+      // test itself supplied, so without this nothing anywhere reads back the expiry getAvatar
+      // *writes*: `Date.now() + cacheDuration` could read `- cacheDuration` and every entry would
+      // be born stale, turning every avatar render into a revoke-and-refetch.
+      const entry = (service as any).cache.get(AVATAR_URL);
+      expect(entry.url).toBe("blob:fresh");
+      expect(entry.expiry).toBeGreaterThan(Date.now());
+    });
+
+    it("drops the revoked entry from the cache even when the refetch then fails", async () => {
+      URL.revokeObjectURL = vi.fn();
+      globalThis.fetch = vi.fn().mockResolvedValue({ ok: false, status: 500 }) as any;
+      (service as any).cache.set(AVATAR_URL, { url: "blob:stale", expiry: Date.now() - 1 });
+
+      expect(await firstValueFrom(service.getAvatar(AVATAR_URL))).toBeUndefined();
+
+      // The url has been handed back to the browser, so the entry holding it is dead. Keeping it
+      // would leave a revoked object URL reachable and revoke it a second time on the next call.
+      expect((service as any).cache.has(AVATAR_URL)).toBe(false);
+      expect(URL.revokeObjectURL).toHaveBeenCalledTimes(1);
+    });
+
     it("returns undefined when the avatar fetch fails", async () => {
       globalThis.fetch = vi.fn().mockResolvedValue({ ok: false, status: 500 }) as any;
       expect(await firstValueFrom(service.getAvatar("https://lh3.googleusercontent.com/a/BAD"))).toBeUndefined();

frontend/src/app/dashboard/service/user/resource-registry/resource-registry.service.spec.ts

@@ -27,6 +27,7 @@ import { DatasetService } from "../dataset/dataset.service";
 import { ModelService } from "../model/model.service";
 import { WorkflowPersistService } from "../../../../common/service/workflow-persist/workflow-persist.service";
 import { DownloadService } from "../download/download.service";
+import { FileResourceDescriptor } from "./file-resource.descriptor";
 import {
   HUB_DATASET_RESULT_DETAIL,
   HUB_MODEL_RESULT_DETAIL,
@@ -247,4 +248,41 @@ describe("ResourceRegistryService", () => {
     expect(registry.entryLink(entry({ type: EntityType.Dataset, id: undefined }), 42)).toEqual([]);
     expect(registry.entryLink(entry({ type: EntityType.Workflow, id: "draft" }), 42)).toEqual([]);
   });
+
+  /**
+   * `hubRoute` is optional on the descriptor contract, and the shipped kinds happen to declare
+   * both routes or neither — so nothing had ever asked what a private-page-only kind links to.
+   * The answer must not depend on the viewer: with nowhere else to send them, the private page is
+   * the only link there is, and the access check further down would otherwise route an outsider to
+   * `undefined`. Descriptors reach the registry by injection, so the kind is supplied as one.
+   */
+  it("links a kind with a private page and no hub page straight to its private page", () => {
+    TestBed.resetTestingModule();
+    TestBed.configureTestingModule({
+      imports: [HttpClientTestingModule],
+      providers: [
+        { provide: DownloadService, useValue: downloadService },
+        { provide: WorkflowPersistService, useValue: workflowPersistService },
+        { provide: DatasetService, useValue: datasetService },
+        { provide: ModelService, useValue: modelService },
+        {
+          provide: FileResourceDescriptor,
+          useValue: {
+            type: EntityType.File,
+            iconType: "folder-open",
+            privateRoute: "/private-files",
+            isOwner: () => true,
+          },
+        },
+        ...commonTestProviders,
+      ],
+    });
+    const privatePageOnly = TestBed.inject(ResourceRegistryService);
+    const file = entry({ type: EntityType.File, id: 7, accessibleUserIds: [42] });
+
+    expect(privatePageOnly.entryLink(file, 42)).toEqual(["/private-files", "7"]);
+    // Same link for a viewer with no access, and for an anonymous one.
+    expect(privatePageOnly.entryLink(file, 99)).toEqual(["/private-files", "7"]);
+    expect(privatePageOnly.entryLink(file, undefined)).toEqual(["/private-files", "7"]);
+  });
 });

frontend/src/app/hub/component/browse-section/browse-section.component.spec.ts

@@ -128,7 +128,12 @@ describe("BrowseSectionComponent", () => {
   });
 
   describe("cover images", () => {
-    it("caches the cover URL the descriptor resolves for an entity that has a cover", () => {
+    it("caches the cover URL the descriptor resolves for an entity that has a cover, and asks once", () => {
+      // Spied, not replaced, so the double still answers: the call count is what pins the
+      // `!coverImageUrls.has(cacheKey(entity))` filter. ngOnChanges runs on every input change of
+      // every one of the landing page's four sections, so without that filter each pass would
+      // re-resolve every cover — a fresh presigned-URL request per card per change-detection run.
+      const cover = vi.spyOn(TestBed.inject(DatasetService) as any, "getDatasetCoverUrl");
       const entity = {
         id: 5,
         type: "dataset",
@@ -137,8 +142,10 @@ describe("BrowseSectionComponent", () => {
       } as unknown as DashboardEntry;
       component.entities = [entity];
       component.ngOnInit();
+      component.ngOnChanges({} as any);
 
       expect(component.getCoverImage(entity)).toBe(PRESIGNED_COVER);
+      expect(cover).toHaveBeenCalledTimes(1);
     });
 
     it("falls back to the default background when no cover was cached", () => {
@@ -149,6 +156,58 @@ describe("BrowseSectionComponent", () => {
 
       expect(component.getCoverImage(entity)).toBe(component.defaultBackground);
     });
+
+    // `this.resourceRegistry.find(entity.type)?.coverUrl` carries two guards, and a mixed section
+    // can trip either. A workflow's cover is a data URL carried on the entry itself, so
+    // WorkflowResourceDescriptor deliberately declares no `coverUrl`; and a kind the registry does
+    // not carry at all has no descriptor to ask, which is why this is `find`, not `get` — one such
+    // row must not take the whole section's covers down, exactly as `routeFor` five lines up
+    // already promises for links.
+    it("skips an entity whose descriptor resolves no cover, rather than calling undefined", () => {
+      const workflow = {
+        id: 10,
+        type: "workflow",
+        coverImageUrl: "carried-on-the-entry",
+        accessibleUserIds: [],
+      } as unknown as DashboardEntry;
+      const unregistered = {
+        id: 12,
+        type: "computing-unit",
+        coverImageUrl: "carried-on-the-entry",
+        accessibleUserIds: [],
+      } as unknown as DashboardEntry;
+      component.entities = [workflow, unregistered];
+
+      expect(() => component.ngOnInit()).not.toThrow();
+      expect(coverCache(component).has("workflow:10")).toBe(false);
+      expect(coverCache(component).has("computing-unit:12")).toBe(false);
+      expect(component.getCoverImage(workflow)).toBe(component.defaultBackground);
+    });
+
+    it("caches nothing when the descriptor resolves an empty cover url", () => {
+      // A presigned-URL endpoint with nothing to sign answers with an empty string; caching that
+      // would put an <img src=""> on the card, which the browser resolves to the page itself.
+      vi.spyOn(TestBed.inject(DatasetService) as any, "getDatasetCoverUrl").mockReturnValue(of({ url: "" }));
+      const entity = {
+        id: 11,
+        type: "dataset",
+        coverImageUrl: "has-cover",
+        accessibleUserIds: [],
+      } as unknown as DashboardEntry;
+      component.entities = [entity];
+      component.ngOnInit();
+
+      // White-box on purpose: getCoverImage's `|| defaultBackground` makes "cached an empty string"
+      // and "cached nothing" indistinguishable through the public API, so only the map itself can
+      // say whether the guard ran.
+      expect(coverCache(component).has("dataset:11")).toBe(false);
+      expect(component.getCoverImage(entity)).toBe(component.defaultBackground);
+    });
+
+    /** The component's cover cache, which no public member exposes. */
+    function coverCache(c: BrowseSectionComponent): Map<string, string> {
+      return (c as unknown as { coverImageUrls: Map<string, string> }).coverImageUrls;
+    }
   });
 });
 /**