improve pre-auth dialog

Signed-off-by: Matthias Wippich <mfwippich@gmail.com>

Author: Tsche

runtime/src/auth/prompt.ts

@@ -0,0 +1,88 @@
+import type { AccessToken } from "../api/client.js";
+import { AuthenticationError } from "./controller.js";
+import type { Authentication } from "./controller.js";
+
+export interface AuthenticationPromptOptions {
+  authentication: Authentication;
+  dialog: HTMLDialogElement;
+  continueButton: HTMLButtonElement;
+  closeButtons?: readonly HTMLElement[];
+  message?: HTMLElement;
+}
+
+export interface AuthenticationPrompt {
+  authenticate(message?: string): Promise<AccessToken>;
+  destroy(): void;
+}
+
+export function createAuthenticationPrompt(options: AuthenticationPromptOptions): AuthenticationPrompt {
+  let pending: Promise<AccessToken> | undefined;
+  let finish: ((error?: unknown, token?: AccessToken) => void) | undefined;
+  let started = false;
+  let destroyed = false;
+
+  const cancelled = (): void => {
+    if (!started) finish?.(new AuthenticationError("cancelled"));
+  };
+  const close = (): void => {
+    cancelled();
+    if (options.dialog.open) options.dialog.close();
+  };
+  const continueAuthentication = (): void => {
+    if (!pending || started) return;
+    started = true;
+    // Start OAuth in the click handler so the popup retains browser user activation.
+    let operation: Promise<AccessToken>;
+    try {
+      operation = options.authentication.authenticate();
+    } catch (error) {
+      finish?.(error);
+      return;
+    }
+    if (options.dialog.open) options.dialog.close();
+    void operation.then(
+      (token) => { finish?.(undefined, token); },
+      (error: unknown) => { finish?.(error); },
+    );
+  };
+  options.continueButton.addEventListener("click", continueAuthentication);
+  options.dialog.addEventListener("cancel", cancelled);
+  options.dialog.addEventListener("close", cancelled);
+  for (const button of options.closeButtons ?? []) button.addEventListener("click", close);
+
+  return {
+    authenticate(message?: string): Promise<AccessToken> {
+      if (destroyed) return Promise.reject(new AuthenticationError("cancelled"));
+      if (pending) return pending;
+      const token = options.authentication.token();
+      if (token) return Promise.resolve(token);
+      if (message !== undefined && options.message) options.message.textContent = message;
+      started = false;
+      const result = new Promise<AccessToken>((resolve, reject) => {
+        finish = (error, authenticated) => {
+          finish = undefined;
+          pending = undefined;
+          if (options.dialog.open) options.dialog.close();
+          if (error !== undefined) reject(error instanceof Error ? error : new Error("Authentication failed"));
+          else if (authenticated) resolve(authenticated);
+        };
+      });
+      pending = result;
+      try {
+        options.dialog.showModal();
+      } catch (error) {
+        finish?.(error);
+      }
+      return result;
+    },
+    destroy(): void {
+      if (destroyed) return;
+      destroyed = true;
+      options.continueButton.removeEventListener("click", continueAuthentication);
+      options.dialog.removeEventListener("cancel", cancelled);
+      options.dialog.removeEventListener("close", cancelled);
+      for (const button of options.closeButtons ?? []) button.removeEventListener("click", close);
+      finish?.(new AuthenticationError("cancelled"));
+    },
+  };
+}

runtime/src/auth/status.ts

@@ -1,8 +1,10 @@
+import type { AccessToken } from "../api/client.js";
 import type { Authentication } from "./controller.js";
 
 export interface AuthenticationStatusOptions {
   mount: HTMLElement;
   authentication: Authentication;
+  authenticate?: () => Promise<AccessToken>;
   signedOutLabel?: string;
   signedInLabel?: (viewerId?: string) => string;
   explanation?: string;
@@ -69,7 +71,7 @@ export function createAuthenticationStatus(
         return;
       }
       action.disabled = true;
-      void options.authentication.authenticate()
+      void (options.authenticate ? options.authenticate() : options.authentication.authenticate())
         .then(() => {
           refresh();
           options.onChange?.(true);

runtime/src/example/page.ts

@@ -3,6 +3,7 @@ import type { ReactionState } from "../api/client.js";
 import { trustedOrigin } from "../auth/origin.js";
 import { Authentication } from "../auth/controller.js";
 import { createAuthenticationStatus } from "../auth/status.js";
+import { createAuthenticationPrompt } from "../auth/prompt.js";
 import {
   setPollVote,
   setReaction,
@@ -38,6 +39,13 @@ const otherReactions = reactionTypes.filter((reaction) =>
 const client = new FeedbackClient({ apiOrigin, site });
 const authentication = new Authentication({ site, callbackOrigin: location.origin, service: client });
 const status = required("status");
+const authenticationPrompt = createAuthenticationPrompt({
+  authentication,
+  dialog: required("authentication-dialog") as HTMLDialogElement,
+  continueButton: required("authentication-continue") as HTMLButtonElement,
+  closeButtons: [...required("authentication-dialog").querySelectorAll<HTMLElement>(".dialog-close")],
+  message: required("authentication-message"),
+});
 let replyTo: { key: string; id: string } | undefined;
 let replyOrder: "oldest" | "newest" = parameters.get("replyOrder") === "newest" ? "newest" : "oldest";
 let authDialogEnabled = parameters.get("authDialog") !== "off";
@@ -57,6 +65,7 @@ required("api-origin").textContent = apiOrigin;
 const authenticationStatus = createAuthenticationStatus({
   mount: required("authentication-status"),
   authentication,
+  authenticate: () => requireAuthentication("Sign in to continue."),
   explanation: "Sign in is required to add comments or replies.",
   onError: (error) => { status.textContent = error instanceof Error ? error.message : "Authentication failed."; },
   onChange: () => { updateAuthenticationUi(); void render(); },
@@ -796,27 +805,9 @@ function sortedComments(values: unknown[]): unknown[] {
 async function requireAuthentication(message: string): Promise<NonNullable<ReturnType<Authentication["token"]>>> {
   const existing = authentication.token();
   if (existing) return existing;
-  if (authDialogEnabled) {
-    const dialog = required("authentication-dialog") as HTMLDialogElement;
-    required("authentication-message").textContent = message;
-    await new Promise<void>((resolve, reject) => {
-      const button = required("authentication-continue") as HTMLButtonElement;
-      let started = false;
-      const close = (): void => {
-        dialog.removeEventListener("close", close);
-        button.onclick = null;
-        if (!started) reject(new Error("Sign in was cancelled."));
-      };
-      button.onclick = () => {
-        started = true;
-        dialog.close();
-        resolve();
-      };
-      dialog.addEventListener("close", close);
-      dialog.showModal();
-    });
-  }
-  const token = await authentication.authenticate();
+  const token = await (authDialogEnabled
+    ? authenticationPrompt.authenticate(message)
+    : authentication.authenticate());
   authenticationStatus.refresh();
   updateAuthenticationUi();
   const activeReply = replyTo;

runtime/src/feedback/votes.ts

@@ -1,4 +1,4 @@
-import type { FeedbackClient, ReactionState } from "../api/client.js";
+import type { AccessToken, FeedbackClient, ReactionState } from "../api/client.js";
 import type { Authentication } from "../auth/controller.js";
 import type { Resource } from "./resources.js";
 import { viewerSubjectStates } from "../protocol/github.js";
@@ -12,6 +12,7 @@ export interface VoteItem {
 export interface VoteControlsOptions {
   client: FeedbackClient;
   authentication: Authentication;
+  authenticate?: () => Promise<AccessToken>;
   items: readonly VoteItem[];
   format?: (count: number, selected: boolean, direction: "up" | "down") => string;
   onError?: (error: unknown) => void;
@@ -75,7 +76,8 @@ export function createVoteControls(options: VoteControlsOptions): VoteControls {
       if (currentItem.resource.key === item.resource.key) currentButton.disabled = true;
     }
     try {
-      const token = options.authentication.token() ?? await options.authentication.authenticate();
+      const token = options.authentication.token() ?? await (options.authenticate
+        ? options.authenticate() : options.authentication.authenticate());
       const resource = item.resource;
       let state = states.get(resource.key);
       if (!state?.id) {

runtime/src/index.ts

@@ -9,6 +9,8 @@ export type {
 } from "./api/client.js";
 export { Authentication, AuthenticationError } from "./auth/controller.js";
 export type { AuthenticationOptions } from "./auth/controller.js";
+export { createAuthenticationPrompt } from "./auth/prompt.js";
+export type { AuthenticationPrompt, AuthenticationPromptOptions } from "./auth/prompt.js";
 export { createAuthenticationStatus } from "./auth/status.js";
 export type { AuthenticationStatusController, AuthenticationStatusOptions } from "./auth/status.js";
 export { PendingVoteStore } from "./auth/pending-vote.js";

runtime/test/authentication-prompt.test.ts

@@ -0,0 +1,107 @@
+import assert from "node:assert/strict";
+import test from "node:test";
+import type { AccessToken } from "../src/api/client.js";
+import { AuthenticationError } from "../src/auth/controller.js";
+import type { Authentication } from "../src/auth/controller.js";
+import { createAuthenticationPrompt } from "../src/auth/prompt.js";
+
+class Dialog extends EventTarget {
+  open = false;
+  showModal(): void { this.open = true; }
+  close(): void {
+    this.open = false;
+    this.dispatchEvent(new Event("close"));
+  }
+}
+
+const token = { value: "token", expiresAt: 2_000_000_000, creationGrant: "grant" } as AccessToken;
+
+function fixture(existing: AccessToken | null = null) {
+  const dialog = new Dialog();
+  const continueButton = new EventTarget();
+  const closeButton = new EventTarget();
+  const message = { textContent: "" };
+  let calls = 0;
+  let resolveOperation!: (value: AccessToken) => void;
+  let rejectOperation!: (reason: Error) => void;
+  const authentication = {
+    token: () => existing,
+    authenticate: () => {
+      calls++;
+      return new Promise<AccessToken>((resolve, reject) => {
+        resolveOperation = resolve;
+        rejectOperation = reject;
+      });
+    },
+  } as unknown as Authentication;
+  const prompt = createAuthenticationPrompt({
+    authentication,
+    dialog: dialog as unknown as HTMLDialogElement,
+    continueButton: continueButton as unknown as HTMLButtonElement,
+    closeButtons: [closeButton as unknown as HTMLElement],
+    message: message as unknown as HTMLElement,
+  });
+  return { dialog, continueButton, closeButton, message, prompt,
+    calls: () => calls, resolve: (value = token) => { resolveOperation(value); },
+    reject: (error: Error) => { rejectOperation(error); } };
+}
+
+void test("existing session bypasses the prompt", async () => {
+  const f = fixture(token);
+  assert.equal(await f.prompt.authenticate("Hello"), token);
+  assert.equal(f.dialog.open, false);
+  assert.equal(f.calls(), 0);
+  f.prompt.destroy();
+});
+
+void test("Continue starts one OAuth operation for simultaneous callers", async () => {
+  const f = fixture();
+  const first = f.prompt.authenticate("Sign in to vote");
+  const second = f.prompt.authenticate("Other message");
+  assert.equal(first, second);
+  assert.equal(f.message.textContent, "Sign in to vote");
+  f.continueButton.dispatchEvent(new Event("click"));
+  assert.equal(f.calls(), 1);
+  assert.equal(f.dialog.open, false);
+  f.resolve();
+  assert.equal(await first, token);
+  f.prompt.destroy();
+});
+
+void test("close button, native cancel, and external close cancel the prompt", async () => {
+  for (const method of ["button", "cancel", "close"] as const) {
+    const f = fixture();
+    const pending = f.prompt.authenticate();
+    if (method === "button") f.closeButton.dispatchEvent(new Event("click"));
+    if (method === "cancel") f.dialog.dispatchEvent(new Event("cancel"));
+    if (method === "close") f.dialog.close();
+    await assert.rejects(pending, (error: unknown) =>
+      error instanceof AuthenticationError && error.code === "cancelled");
+    f.prompt.destroy();
+  }
+});
+
+void test("destroy settles pending work and removes listeners", async () => {
+  const f = fixture();
+  const pending = f.prompt.authenticate();
+  f.prompt.destroy();
+  await assert.rejects(pending, (error: unknown) =>
+    error instanceof AuthenticationError && error.code === "cancelled");
+  f.continueButton.dispatchEvent(new Event("click"));
+  assert.equal(f.calls(), 0);
+});
+
+void test("failed OAuth closes the prompt and permits retry", async () => {
+  const f = fixture();
+  const first = f.prompt.authenticate();
+  f.continueButton.dispatchEvent(new Event("click"));
+  f.reject(new Error("OAuth failed"));
+  await assert.rejects(first, /OAuth failed/);
+  const second = f.prompt.authenticate();
+  assert.equal(f.dialog.open, true);
+  f.continueButton.dispatchEvent(new Event("click"));
+  f.resolve();
+  assert.equal(await second, token);
+  assert.equal(f.calls(), 2);
+  f.prompt.destroy();
+});

runtime/test/authentication.test.ts

@@ -4,6 +4,7 @@ import test from "node:test";
 import { Authentication, type AuthEnvironment, type AuthMessage } from "../src/auth/controller.js";
 import type { OAuthTransport } from "../src/auth/controller.js";
 import type { KeyValueStorage } from "../src/storage.js";
+import { createAuthenticationStatus } from "../src/auth/status.js";
 
 void test("authentication verifies the callback and GitHub viewer before storing a token", async () => {
   const values = new Map<string, string>();
@@ -124,3 +125,46 @@ void test("authentication permits loopback HTTP but rejects other insecure origi
     storage,
   }), /loopback/);
 });
+
+void test("authentication status uses the supplied login callback", async () => {
+  const previousDocument = globalThis.document;
+  class Element extends EventTarget {
+    className = "";
+    type = "";
+    textContent = "";
+    disabled = false;
+    onclick: (() => void) | null = null;
+    classList = { toggle: () => undefined };
+    append(): void { /* children are not needed here */ }
+    replaceChildren(): void { /* children are not needed here */ }
+    setAttribute(): void { /* attributes are not needed here */ }
+  }
+  const elements: Element[] = [];
+  globalThis.document = {
+    createElement: () => { const element = new Element(); elements.push(element); return element; },
+    createTextNode: (value: string) => ({ textContent: value }),
+  } as unknown as Document;
+  try {
+    const token = { value: "token", expiresAt: 2_000_000_000, creationGrant: "grant" };
+    let current: typeof token | null = null;
+    let directCalls = 0;
+    let customCalls = 0;
+    const authentication = {
+      token: () => current,
+      authenticate: () => { directCalls++; return Promise.resolve(token); },
+      clear: () => { current = null; },
+    } as unknown as Authentication;
+    const mount = { replaceChildren: () => undefined } as unknown as HTMLElement;
+    createAuthenticationStatus({ mount, authentication,
+      authenticate: () => { customCalls++; current = token; return Promise.resolve(token); },
+    });
+    const action = elements[1];
+    assert.ok(action);
+    action.onclick?.();
+    await new Promise((resolve) => setImmediate(resolve));
+    assert.equal(customCalls, 1);
+    assert.equal(directCalls, 0);
+  } finally {
+    globalThis.document = previousDocument;
+  }
+});