Skip to content

feat: add font management section to the admin settings - #142

Merged
DmySyz merged 1 commit into
mainfrom
feat/font-management
Aug 24, 2026
Merged

feat: add font management section to the admin settings#142
DmySyz merged 1 commit into
mainfrom
feat/font-management

Conversation

@DmySyz

@DmySyz DmySyz commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Adds a section with options to upload and delete custom fonts

New files

lib/Controller/FontController.php

Admin-only Nextcloud controller that proxies font management requests to the EuroOffice DocumentServer AdminPanel API (/admin/api/v1/fonts).

  • Access is restricted to NC admins via both the AppFramework middleware (no #[NoAdminRequired]) and an explicit requireAdmin() guard
  • Authenticates toward the AdminPanel with short-lived JWTs (sub: font-api, 60 s TTL) signed with the DS JWT secret; tokens are cached in APCu to avoid minting on every status poll
  • Upload streams the file directly to the AdminPanel as application/octet-stream via a PHP file handle, avoiding loading the full file into memory
  • Actions: index (list), upload, delete, regenerate, getStatus

src/views/FontManager.vue

Vue component rendered in the EuroOffice admin settings page.

  • File picker restricted to .ttf, .otf, .woff, .woff2; extension validated client-side before upload
  • Font list showing name, size, and per-font delete button with loading state
  • Regenerate button (disabled while running or when no fonts are installed)
  • Status bar (NcNoteCard) for success/error/info feedback
  • Polls /ajax/fonts/status every 2s while regeneration is in progress; stops on terminal state (done or error)

css/eurooffice-listener.css

Built CSS artifact accompanying the Vue component.

Modified files

appinfo/routes.php

Registers five new routes under ajax/fonts/ - index, upload, delete, regenerate, getStatus

lib/DocumentService.php

Added delete to the HTTP methods supported by the shared request() helper.

src/settings.js / templates/settings.php

Wire the FontManager component into the existing EuroOffice admin settings page.

npm-shrinkwrap.json

Lockfile updated to reflect any dependency changes from the Vue component additions.

Notes

Assisted-by: Claude Code (Opus-4.8)

@DmySyz
DmySyz force-pushed the feat/font-management branch from 8a79a2d to 67a0add Compare August 21, 2026 15:20
@moodyjmz

Copy link
Copy Markdown
Member

TL;DR

The controller and UI are competently built — admin-gated twice over, short-lived exp'd service tokens (pairing correctly with the tightened bearer check on the server side), streamed upload that doesn't buffer the file in PHP, sensible polling with terminal-state handling. The blockers are environmental and coordination problems rather than code quality: this only works where the AdminPanel is actually running (it's off by default in the standalone image and never enabled under AIO), it's hard-coupled to server#41's API while server#39 implements the same API incompatibly, and all five routes disable CSRF for no reason. Plus one genuine UX bug: you can't regenerate after deleting the last font — which is exactly when you need to.

Findings, ranked

1. High — the AdminPanel isn't running on the deployments this targets. The standalone image ships ds-adminpanel.conf with autostart=false, gated on ADMINPANEL_ENABLED, and AIO never sets it. On both common deployment shapes every call this controller makes 502s, while templates/settings.php renders the Custom fonts section whenever a DS is configured — so admins get a working-looking uploader that errors on first touch. At minimum, document the ADMINPANEL_ENABLED prerequisite and make the first failed loadFonts say "the DocumentServer AdminPanel is not enabled" instead of a raw error; better, probe once and hide or explain the section.

2. High — hard-coupled to server#41 while server#39 implements the same API differently. This client speaks #41's contract: application/octet-stream + X-Font-Name, GET /status, and Bearer sub: 'font-api' auth. Euro-Office/server#39 (open since 11 July) implements the same endpoints with multipart upload, .ttc instead of .woff/.woff2, /regenerate/status, and cookie-only auth — no bearer path at all. If #39 lands, this PR is dead on arrival. The server-side duplication needs resolving before this merges. It also inherits #41's own gate: regeneration only works once document-server-package#14 ships.

3. Medium (security) — #[NoCSRFRequired] on all five routes is unnecessary and wrong. FontController disables CSRF on state-changing admin endpoints (upload, delete, regenerate). The app's own SettingsController save endpoints — browser-called, same settings page — carry no such attribute, and @nextcloud/axios attaches the requesttoken automatically, so the protection costs nothing. This looks cargo-culted from the editor/callback controllers, which need it because the DS calls them server-side. Nothing calls these routes except the admin's browser. Delete the attribute from all five actions.

4. Medium (functional) — Regenerate is disabled when the font list is empty. FontManager.vue: :disabled="regenerating || fonts.length === 0". Deleting the last custom font is precisely the moment you must regenerate to purge it from the baked AllFonts.js; as shipped, the stale font stays in every editor with no UI path to remove it. Drop the fonts.length === 0 clause — regenerating with zero custom fonts is a legitimate, necessary operation.

5. Low — empty-secret path fails opaquely. With no JWT secret configured on the connector, makeServiceToken() happily signs with ''; the AdminPanel rejects it and the admin sees a generic "DocumentServer error". Fail fast in the controller with an actionable "JWT secret not configured" message.

6. Low — APCu token cache ignores secret rotation. TOKEN_CACHE_KEY is a fixed string, so after changing the DS secret the stale cached token keeps being sent for up to ~45 s of 401s. Either key it on a hash of the secret or skip caching a 60-second token entirely — minting HS256 JWTs is not the expensive part of a 2 s poll loop.

7. Nits. _pollTimer is declared in data(), but Vue 3 doesn't proxy _-prefixed data properties — it works only because the methods assign a plain instance property; declare it outside data() and lose the dev-mode warning. httpOpts() re-implements verify => false and allow_local_address, both of which DocumentService::request() already applies unconditionally.

Fine — not raising: the double admin gate (framework + explicit requireAdmin()) is correct even if the justifying comment oversells it; delete()'s basename($name) !== $name guard is right; the DocumentService::request() change from silently-GET-on-unknown-method to match + throw is a straight improvement and no existing caller passes anything but get/post; the streamed upload via file handle does what the PR description claims.

Bottom line: fix the CSRF attributes and the empty-list regenerate before merge; resolve the #39/#41 server duplication before merge; and decide what the admin experience is on deployments where the AdminPanel is disabled, because right now that's most of them.

@DmySyz
DmySyz force-pushed the feat/font-management branch from 67a0add to 24bf255 Compare August 24, 2026 10:11
@moodyjmz

Copy link
Copy Markdown
Member

TL;DR: 3 of 4 items from the prior review round are cleanly fixed (CSRF attributes removed correctly, empty-list regenerate bug, redundant verify logic). The 4th is a near-miss, plus one new finding worth a decision before merge:

  1. The opaque-error fix only actually surfaces the real error on 1 of 5 endpoints — and not the one that fires first on page load.
  2. Demo mode exposes live font upload/delete against Nextcloud's shared public demo DocumentServer with a shared secret.
  3. A new rawurlencode() on X-Font-Name needs confirming against server#41's decode side.
Detail

1. Opaque-secret-error fix doesn't cover the path that needs it (lib/Controller/FontController.php)

index() (:138-153) and getStatus() (:272-284) both catch (\Exception $e) and unconditionally call proxyError('', 500), which json_decode('')s to null and falls back to the hardcoded "DocumentServer error" — discarding $e->getMessage() entirely. upload()/delete() try method_exists($e, 'getResponse'), but the exception from makeServiceToken() is a plain \Exception with no getResponse(), so it falls to the same generic path. Only regenerate() (:261-263) actually forwards $e->getMessage().

FontManager.vue's mounted() → loadFonts() (calling index()) is the first call on page load — exactly the path most likely to hit a misconfigured secret first, and exactly the one returning the useless message. Compounding it: even if PHP forwarded the message, loadFonts()'s catch block (:116) reads e.message (axios's generic "Request failed with status code 500"), not e.response?.data?.error like the other three handlers do.

Suggested fix: make index/upload/delete/getStatus fall back to $e->getMessage() for non-HTTP exceptions (same as regenerate() already does), and fix loadFonts()'s catch to read e.response?.data?.error first.

2. Custom Fonts UI exposed in demo mode against shared demo infrastructure

templates/settings.php:298 gates the new section with the same condition used for purely cosmetic settings sections. In demo mode, AppConfig::getDocumentServerUrl()/getDocumentServerSecret() return the shared DEMO_PARAM address/secret. That means demo-mode admins get a live upload/delete/regenerate UI pointed at Nextcloud's shared demo DS, authenticated with a secret every demo install shares — a different risk class than the cosmetic sections this boilerplate was written for. Worth an explicit decision (separate gate, or accept as-is) rather than silent inheritance.

3. Unverified cross-repo wire contract change

lib/Controller/FontController.php:196 adds X-Font-Name: rawurlencode($originalName). This header's decoding lives in server#41 — if that side doesn't urldecode() it, font names with spaces/non-ASCII land on disk percent-encoded. Please confirm against the receiving side before merge.

@DmySyz
DmySyz force-pushed the feat/font-management branch from 24bf255 to cfa7529 Compare August 24, 2026 14:00
@DmySyz

DmySyz commented Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

3. Unverified cross-repo wire contract change

lib/Controller/FontController.php:196 adds X-Font-Name: rawurlencode($originalName). This header's decoding lives in server#41 — if that side doesn't urldecode() it, font names with spaces/non-ASCII land on disk percent-encoded. Please confirm against the receiving side before merge.

Verified

@moodyjmz

Copy link
Copy Markdown
Member

TL;DR: The opaque-error fix and the empty-list regenerate fix are both solid — confirmed across all 5 controller endpoints plus the Vue catch block. The demo-mode fix is cosmetic only: the controller itself still has no demo awareness, currently inert only because of an empty-string TODO elsewhere. rawurlencode/decode contract is still unconfirmed. One new finding: a stray build artifact appears to have been committed.

Detail

Fixed — confirmed:

  • index(), getStatus(), upload(), delete() in lib/Controller/FontController.php all now forward $e->getMessage() instead of a generic 500. FontManager.vue's loadFonts() catch now reads e.response?.data?.error || e.message, matching the other three handlers.
  • Empty-list regenerate-button bug: fixed.

Still open — demo-mode gate is template-only, not enforced server-side:

templates/settings.php:298 now hides the fonts section in demo mode, but that's pure CSS (eurooffice-hide { display: none }) — the div and its Vue mount still happen unconditionally in src/settings.js, and FontController has zero useDemo() awareness anywhere. It's currently inert only because AppConfig.php:1415's DEMO_PARAM["ADDR"] is an empty-string TODO — the moment that's filled in, this fires a live, authenticated call against shared demo infrastructure with a shared secret, with no code-level guard and no additional review trigger. Suggest adding a useDemo() check directly in FontController rather than relying on the template hiding the div.

Still unconfirmed:

The rawurlencode() on X-Font-Name (FontController.php:196) — the "Verified" reply doesn't include a file:line or diff pointing at the decode side in server#41. Would like an actual pointer before treating this as resolved, since a mismatch here silently mangles non-ASCII font filenames on disk.

New finding:

css/eurooffice-listener.css looks like an accidentally committed Vite build artifact — its entire content is @imports of three content-hashed *.chunk.css files that .gitignore explicitly excludes (/css/*.chunk.css). Nothing in this PR's scope touches the listener entry point. Recommend dropping it from the PR — those hashes will just rot on the next rebuild.

Nits, non-blocking: upload() mints two JWTs per call (the base httpOpts() header is silently overwritten by a second mint) — harmless, just dead work. regenerate()'s error handling doesn't extract the HTTP response body the way upload/delete do, so an AdminPanel-side error there surfaces a rawer message than the others.

@DmySyz

DmySyz commented Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

Still unconfirmed:

The rawurlencode() on X-Font-Name (FontController.php:196) — the "Verified" reply doesn't include a file:line or diff pointing at the decode side in server#41. Would like an actual pointer before treating this as resolved, since a mismatch here silently mangles non-ASCII font filenames on disk.

PHP's rawurlencode() is RFC 3986 encoding; the server receives it and calls decodeURIComponent() at router.js:318, which is the exact inverse. The comma-split and path.basename() run after decoding.

adds a section with options to upload and delete custom fonts
adds a button to regenerate available fonts based on the list of currently uploaded ones

Signed-off-by: dsyzov <dmytro.syzov@nextcloud.com>
@DmySyz
DmySyz force-pushed the feat/font-management branch from cfa7529 to 44aafe3 Compare August 24, 2026 14:51

@moodyjmz moodyjmz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All three open items resolved. Demo mode now has a real server-side guard (requireNotDemo() on all five controller actions), independent of the unrelated DEMO_PARAM["ADDR"] TODO that made the earlier fix only contingent. The X-Font-Name encode/decode contract with server#41 checks out — traced it into the actual router code this time rather than taking "Verified" at face value. Stray build artifact is gone. Approving.

@DmySyz
DmySyz merged commit 0ea6436 into main Aug 24, 2026
8 checks passed
@DmySyz
DmySyz deleted the feat/font-management branch August 24, 2026 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants