feat(pad): suppress deletion token for durable identities + relabel recovery action (#7926)

[JohnMcLear]

Stacked on #7929 (base branch is that PR's branch — review/merge #7929 first; I'll rebase this onto develop once it lands).

Relates to #7926 — the follow-up "other bits" discussed on the issue, after #7929 handled the primary ask (suppress the modal when allowPadDeletionByAllUsers).

What this adds

1. Suppress the token for durable identities (the OIDC/auth idea)

A creator's deletion token (and its modal) is now also withheld when the creator has a durable identity: authenticated (req.session.user.username) AND the deployment maps that identity to a stable authorID via a getAuthorId hook. Only then does the creator path (author === revision-0 author) survive a cookie clear or a different device, so the recovery token is genuinely redundant.

2. Why not requireAuthentication alone

This tightens the old requireAuthentication ⇒ always suppress rule. Without a getAuthorId hook the authorID still comes from the per-browser author-token cookie (AuthorManager.getAuthorId → getAuthor4Token), so an authenticated user on a second device is not recognised as the creator. Withholding the token there would strand them — so those deployments now keep issuing one. Deployments using the documented getAuthorId pattern (e.g. SSO) get the clean no-modal experience. (This is the one debatable behaviour change — flagging it explicitly; easy to scope down if undesired.)

3. Relabel the recovery action

New canDeleteWithoutToken clientVar (allowPadDeletionByAllUsers || durable identity) drives the UI: when the user can already delete without a token, the recovery disclosure summary reads plainly "Delete Pad" (reusing the already-translated pad.settings.deletePad key — German etc. included) instead of the jargon "Delete with token".

Tests

src/tests/backend/specs/socketio.ts — Pad deletion token issuance (#7926):

• anonymous creator → token, canDeleteWithoutToken=false
• allowPadDeletionByAllUsers → no token, true
• authenticated without a getAuthorId hook → still a token, false
• authenticated with a getAuthorId hook → no token, true

43 passing. Also verified live in Chromium on both branches of canDeleteWithoutToken: modal hidden + summary "Delete Pad" when suppressed; modal shown + recovery disclosure intact otherwise.

🤖 Generated with Claude Code

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0)

1. Token disclosure still shown ✓ Resolved 📎 Requirement gap ≡ Correctness

Description

With allowPadDeletionByAllUsers=true, the UI still renders the token-based deletion disclosure
(including the Pad deletion token field), which conflicts with the requirement to suppress
token-related UI/wording in this mode.

Code

src/static/js/pad_editor.ts[R152-161]

+      // When the user can already delete the pad without a token (everyone may
+      // delete, or they have a durable authenticated identity — issue #7926),
+      // the recovery-token disclosure is redundant: label it plainly "Delete
+      // Pad" rather than the jargon "Delete with token". Reuses the existing,
+      // already-translated pad.settings.deletePad key.
+      if ((window as any).clientVars?.canDeleteWithoutToken) {
+        const $summary = $('#delete-pad-with-token > summary');
+        $summary.attr('data-l10n-id', 'pad.settings.deletePad')
+            .text(html10n.get('pad.settings.deletePad'));
+      }

Evidence

The checklist forbids token UI/wording when allowPadDeletionByAllUsers=true. The PR adds logic
that, in this mode, sets canDeleteWithoutToken and only relabels the disclosure summary, but the
underlying token disclosure UI (token label/input) still exists and therefore still appears in the
UI.

Disable pad deletion token UI when allowPadDeletionByAllUsers is true
src/static/js/pad_editor.ts[152-161]
src/templates/pad.html[390-397]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
PR Compliance ID 1 requires that when `allowPadDeletionByAllUsers=true`, token-related deletion UI (modal and token wording/controls) must not appear. The PR suppresses token issuance and relabels the disclosure summary, but the token disclosure still renders a `Pad deletion token` label and input, so token UI/wording is still present.
## Issue Context
- The template always renders the token disclosure when `!settings.requireAuthentication`, regardless of `allowPadDeletionByAllUsers`.
- The new client-side change only renames the disclosure summary when `clientVars.canDeleteWithoutToken` is true; it does not remove/hide the token label/input.
## Fix Focus Areas
- src/templates/pad.html[390-398]
- src/static/js/pad_editor.ts[152-191]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Auth token UI mismatch 🐞 Bug ≡ Correctness

Description

handleClientReady can now issue a padDeletionToken even when settings.requireAuthentication is true
(if there is no durable getAuthorId mapping), but the pad UI hides the token-based deletion controls
whenever requireAuthentication is enabled. This can strand authenticated creators on other
devices/cookie-loss scenarios because handlePadDelete requires creator identity or a valid deletion
token.

Code

src/node/handler/PadMessageHandler.ts[R1310-1316]

+    const hasGetAuthorIdHook = (plugins.hooks.getAuthorId || []).length > 0;
+    const hasDurableIdentity = hasGetAuthorIdHook && !!(user && user.username);
+    const canDeleteWithoutToken = settings.allowPadDeletionByAllUsers || hasDurableIdentity;
   const padDeletionToken =
-        isCreator && !settings.requireAuthentication && !settings.allowPadDeletionByAllUsers
+        isCreator && !canDeleteWithoutToken
       ? await padDeletionManager.createDeletionTokenIfAbsent(sessionInfo.padId)
       : null;

Evidence

The server now issues a deletion token whenever isCreator && !canDeleteWithoutToken, where
canDeleteWithoutToken no longer depends on requireAuthentication, so requireAuth+no-hook can
produce a token. But the token-based delete UI is gated by !settings.requireAuthentication, so an
authenticated user cannot submit the token, even though the server-side delete path accepts a token
and otherwise relies on creator authorID equality which is cookie-derived without a hook.

src/node/handler/PadMessageHandler.ts[1284-1316]
src/templates/pad.html[390-398]
src/node/handler/PadMessageHandler.ts[290-313]
src/node/db/AuthorManager.ts[161-165]
src/node/utils/ensureAuthorTokenCookie.ts[11-34]
src/node/db/API.ts[552-560]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`handleClientReady` may emit `clientVars.padDeletionToken` (and `canDeleteWithoutToken=false`) in configurations where `settings.requireAuthentication=true` but there is no durable `getAuthorId` mapping. However, the token-based delete UI (`#delete-pad-with-token`) is currently not rendered at all when `requireAuthentication` is enabled, so users cannot supply the recovery token from another browser/device to delete the pad.
## Issue Context
- Server-side delete authorization allows deletion with a valid token regardless of creator cookie, but only if the client can send `PAD_DELETE` with `deletionToken`.
- The EJS template currently removes the token-delete form under `requireAuthentication`, which conflicts with the new behavior that can still require a token.
- The HTTP API `createPad` path still unconditionally suppresses `deletionToken` when `requireAuthentication` is enabled, which diverges from the socket/UI path.
## Fix Focus Areas
- Decide on one consistent contract:
- **Option A (preferred if token recovery should work under requireAuth without durable identity):** Render the token-delete UI even when `requireAuthentication` is true, and hide it dynamically (client-side) when `clientVars.canDeleteWithoutToken` is true.
- **Option B:** If you do *not* want token recovery under `requireAuthentication`, restore the old server-side suppression and remove the new backend test expectations.
- If Option A:
- Update `src/templates/pad.html` to always include (or conditionally include based on a new server-provided flag) the `#delete-pad-with-token` form.
- Update `src/static/js/pad_editor.ts` to hide/show the disclosure based on `clientVars.canDeleteWithoutToken` (and/or presence of `clientVars.padDeletionToken`).
- Consider aligning `src/node/db/API.ts#createPad` to the same durable-identity rules so API-created pads aren’t treated differently.
### Fix Focus Areas (exact locations)
- src/node/handler/PadMessageHandler.ts[1284-1316]
- src/templates/pad.html[390-398]
- src/static/js/pad_editor.ts[152-191]
- src/node/db/API.ts[552-560]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Durable identity misdetected 🐞 Bug ☼ Reliability

Description

The server treats an authenticated user as having a durable identity if any getAuthorId hook is
registered, but hooks are allowed to return undefined (falling back to token-cookie mapping) for
some users (for example guest/partial auth). This can incorrectly suppress the deletion token for a
creator whose getAuthorId hook does not actually pin their authorID, leaving them unable to delete
after cookie loss/device change.

Code

src/node/handler/PadMessageHandler.ts[R1310-1312]

+    const hasGetAuthorIdHook = (plugins.hooks.getAuthorId || []).length > 0;
+    const hasDurableIdentity = hasGetAuthorIdHook && !!(user && user.username);
+    const canDeleteWithoutToken = settings.allowPadDeletionByAllUsers || hasDurableIdentity;

Evidence

The getAuthorId hook contract allows hook functions to return undefined (falling through) and/or
only change context.dbKey; if no hook returns a usable authorId, AuthorManager falls back to
token-derived identity. Therefore, checking only that a hook exists does not guarantee durable
identity for the current authenticated user.

src/node/handler/PadMessageHandler.ts[1310-1312]
src/node/db/AuthorManager.ts[161-165]
doc/api/hooks_server-side.md[211-264]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`hasDurableIdentity` is currently inferred from `(plugins.hooks.getAuthorId || []).length > 0` and `user.username`, but `getAuthorId` hooks can be conditional and return `undefined` (or otherwise not modify `context.dbKey`) for some authenticated users. In those cases, Etherpad will fall back to token-cookie mapping and the identity is *not* durable, but the deletion token would still be suppressed.
## Issue Context
- `AuthorManager.getAuthorId()` falls back to token mapping when the hook returns a falsy/undefined authorId.
- The official hook contract explicitly allows returning `undefined` to fall through (or returning falsy while relying on `context.dbKey`), and the example shows conditional behavior.
## Fix Focus Areas
- Determine durability based on the *effective* hook behavior for the current request’s `user`, not just hook registration. For example:
- Create a probe context `{dbKey: token, token, user}` and run `hooks.aCallFirst('getAuthorId', ctx)`; treat the identity as durable only if the hook returns a non-`undefined` value **or** mutates `ctx.dbKey` away from the token.
- (Alternative) Add an explicit plugin-provided flag/hook response indicating durability, to avoid extra hook invocation.
### Fix Focus Areas (exact locations)
- src/node/handler/PadMessageHandler.ts[1298-1312]
- src/node/db/AuthorManager.ts[161-165]
- doc/api/hooks_server-side.md[211-264]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Pad: suppress deletion token for durable identities; relabel recovery action
✨ Enhancement 🧪 Tests 🕐 20-40 Minutes

Walkthroughs

User Description

Stacked on #7929 (base branch is that PR's branch — review/merge #7929 first; I'll rebase this onto develop once it lands).

Relates to #7926 — the follow-up "other bits" discussed on the issue, after #7929 handled the primary ask (suppress the modal when allowPadDeletionByAllUsers).

What this adds

1. Suppress the token for durable identities (the OIDC/auth idea)

A creator's deletion token (and its modal) is now also withheld when the creator has a durable identity: authenticated (req.session.user.username) AND the deployment maps that identity to a stable authorID via a getAuthorId hook. Only then does the creator path (author === revision-0 author) survive a cookie clear or a different device, so the recovery token is genuinely redundant.

2. Why not requireAuthentication alone

This tightens the old requireAuthentication ⇒ always suppress rule. Without a getAuthorId hook the authorID still comes from the per-browser author-token cookie (AuthorManager.getAuthorId → getAuthor4Token), so an authenticated user on a second device is not recognised as the creator. Withholding the token there would strand them — so those deployments now keep issuing one. Deployments using the documented getAuthorId pattern (e.g. SSO) get the clean no-modal experience. (This is the one debatable behaviour change — flagging it explicitly; easy to scope down if undesired.)

3. Relabel the recovery action

New canDeleteWithoutToken clientVar (allowPadDeletionByAllUsers || durable identity) drives the UI: when the user can already delete without a token, the recovery disclosure summary reads plainly "Delete Pad" (reusing the already-translated pad.settings.deletePad key — German etc. included) instead of the jargon "Delete with token".

Tests

src/tests/backend/specs/socketio.ts — Pad deletion token issuance (#7926):

• anonymous creator → token, canDeleteWithoutToken=false
• allowPadDeletionByAllUsers → no token, true
• authenticated without a getAuthorId hook → still a token, false
• authenticated with a getAuthorId hook → no token, true

43 passing. Also verified live in Chromium on both branches of canDeleteWithoutToken: modal hidden + summary "Delete Pad" when suppressed; modal shown + recovery disclosure intact otherwise.

🤖 Generated with Claude Code

AI Description

• Suppress pad deletion-token issuance when the creator has a durable authenticated identity.
• Expose canDeleteWithoutToken to the client and relabel the recovery action to "Delete Pad".
• Extend backend Socket.IO tests to cover all token issuance/suppression combinations.

Diagram

graph TD
  A["PadMessageHandler.handleClientReady"] --> B["clientVars.canDeleteWithoutToken"] --> C["pad_editor.ts (settings UI)"]
  A --> D["PadDeletionManager.createDeletionTokenIfAbsent"]
  A --> E["settings.allowPadDeletionByAllUsers"]
  A --> F["plugins.hooks.getAuthorId"]
  G["socketio.spec.ts"] --> A

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Always suppress when requireAuthentication is true

• ➕ Simpler rule; fewer moving parts to understand.
• ➖ Incorrect for deployments without getAuthorId mapping: authenticated users on another device may not be recognized as creator and would lose the only recovery path (token).

2. Keep issuing tokens but suppress only the modal client-side

• ➕ Avoids any behavior change around token availability; purely UX-driven.
• ➖ Still creates/returns sensitive recovery tokens unnecessarily; does not reduce token-management surface area; requires careful handling to avoid leaking token beyond creator.

3. Introduce an explicit config flag (e.g., suppressDeletionTokenForAuthenticatedCreators)

• ➕ Opt-in/opt-out behavior for admins; avoids implicit coupling to hook presence.
• ➖ More configuration complexity; higher support burden; still needs guidance on when it is safe to enable (which effectively mirrors the durable-identity logic).

Recommendation: Current approach is the safest default: only suppress when the creator identity is durable (username + getAuthorId hook) or deletion is already universally allowed. That preserves cross-device recovery in non-SSO deployments while eliminating redundant tokens/modals for deployments that have stable identity mapping.

File Changes

Enhancement (2)

PadMessageHandler.ts Gate deletion-token issuance on durable identity and export canDeleteWithoutToken +28/-7

Gate deletion-token issuance on durable identity and export canDeleteWithoutToken

• Refines the deletion token suppression logic: instead of suppressing on requireAuthentication alone, it suppresses only when either allowPadDeletionByAllUsers is enabled or the creator has a durable identity (username + getAuthorId hook present). Adds canDeleteWithoutToken to clientVars to drive client-side UX.

src/node/handler/PadMessageHandler.ts

---

pad_editor.ts Relabel recovery delete disclosure when token is unnecessary +11/-0

Relabel recovery delete disclosure when token is unnecessary

• Uses clientVars.canDeleteWithoutToken to rename the recovery disclosure summary from "Delete with token" to the existing localized "Delete Pad" label when a token is not needed.

src/static/js/pad_editor.ts

---

Tests (1)

socketio.ts Add backend coverage for token issuance across auth/hook configurations +47/-0

Add backend coverage for token issuance across auth/hook configurations

• Adds a test helper to temporarily install a getAuthorId hook and validates both padDeletionToken and canDeleteWithoutToken across: anonymous creator, allowPadDeletionByAllUsers, authenticated without hook, and authenticated with hook. Ensures hook state is restored between specs to avoid leakage.

src/tests/backend/specs/socketio.ts

---

[qodo-free-for-open-source-projects]

2. Auth token ui mismatch 🐞 Bug ≡ Correctness

handleClientReady can now issue a padDeletionToken even when settings.requireAuthentication is true
(if there is no durable getAuthorId mapping), but the pad UI hides the token-based deletion controls
whenever requireAuthentication is enabled. This can strand authenticated creators on other
devices/cookie-loss scenarios because handlePadDelete requires creator identity or a valid deletion
token.

Agent Prompt

## Issue description
`handleClientReady` may emit `clientVars.padDeletionToken` (and `canDeleteWithoutToken=false`) in configurations where `settings.requireAuthentication=true` but there is no durable `getAuthorId` mapping. However, the token-based delete UI (`#delete-pad-with-token`) is currently not rendered at all when `requireAuthentication` is enabled, so users cannot supply the recovery token from another browser/device to delete the pad.

## Issue Context
- Server-side delete authorization allows deletion with a valid token regardless of creator cookie, but only if the client can send `PAD_DELETE` with `deletionToken`.
- The EJS template currently removes the token-delete form under `requireAuthentication`, which conflicts with the new behavior that can still require a token.
- The HTTP API `createPad` path still unconditionally suppresses `deletionToken` when `requireAuthentication` is enabled, which diverges from the socket/UI path.

## Fix Focus Areas
- Decide on one consistent contract:
  - **Option A (preferred if token recovery should work under requireAuth without durable identity):** Render the token-delete UI even when `requireAuthentication` is true, and hide it dynamically (client-side) when `clientVars.canDeleteWithoutToken` is true.
  - **Option B:** If you do *not* want token recovery under `requireAuthentication`, restore the old server-side suppression and remove the new backend test expectations.
- If Option A:
  - Update `src/templates/pad.html` to always include (or conditionally include based on a new server-provided flag) the `#delete-pad-with-token` form.
  - Update `src/static/js/pad_editor.ts` to hide/show the disclosure based on `clientVars.canDeleteWithoutToken` (and/or presence of `clientVars.padDeletionToken`).
  - Consider aligning `src/node/db/API.ts#createPad` to the same durable-identity rules so API-created pads aren’t treated differently.

### Fix Focus Areas (exact locations)
- src/node/handler/PadMessageHandler.ts[1284-1316]
- src/templates/pad.html[390-398]
- src/static/js/pad_editor.ts[152-191]
- src/node/db/API.ts[552-560]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[JohnMcLear]

Good catch — real bug, fixed in 8e923f0 (your Option A).

• The recovery disclosure is no longer gated on !requireAuthentication: pad.html now renders #delete-pad-with-token for every session (default hidden), and pad_editor.ts shows it only when clientVars.canDeleteWithoutToken is false. So an authenticated creator who is issued a token (requireAuthentication + no durable getAuthorId mapping) now has the UI to enter it on another device.
• API.createPad aligned: it also returns deletionToken: null under allowPadDeletionByAllUsers now (it already suppressed under requireAuthentication). I left the requireAuthentication suppression on the API path as-is otherwise — the API caller is apikey-trusted and has no req.session.user/hook context, so the durable-identity rules don't map there; the relevant consistency fix is the allowAll suppression.

Verified live in Chromium: allowAll=true → disclosure hidden, no modal; default anonymous → disclosure visible, modal shown. Added a createPad-under-allowPadDeletionByAllUsers backend test.

[JohnMcLear]

/review

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[qodo-free-for-open-source-projects]

Looking for bugs?

Check back in a few minutes. An AI review agent is analyzing this pull request.