feat(templates): let admins share pad templates with everyone - #181
Merged
Conversation
Nextcloud only ever looks at one Templates folder per account, so a team template had to be set up by every member individually – and a read-only share is not a supported source at all. An admin can now upload .pad files under Administration settings, and they appear in the picker for every account with no per-user setup. The templates are ordinary .pad files, so creating from one takes the path that already exists: content copied, placeholders resolved, pad provisioned, binding written. Only the storage and the listing are new. They live in the app's appdata folder, which IAppData owns. That folder has no versioning and no trash, so overwriting an existing name is refused unless the request says it means to replace, and the settings page asks first. The check and the write happen together under an exclusive lock on the name: Folder::newFile() overwrites what is already there, so asking "does it exist?" beforehand would leave a window for a second upload to create the file and this one to destroy it unnoticed. Deleting takes the same lock. What can be rejected is rejected at upload time, where an admin can act on it – not a pad file, no pad in its frontmatter, a pad on another Etherpad server, empty, or over 2 MiB. A failure to list is deliberately not swallowed on the settings page: an empty list must not be able to mean "appdata unreadable". The picker is the one place that tolerates it, logs and carries on, because a provider that throws would take every other app's templates down with it. The "New pad" entry is now registered while either pad type is enabled. It was tied to protected pads alone, so an instance with only public pads had no entry – and would have had no way to reach the shared templates.
Nextcloud shows the picker whenever there is something to pick. Until now this instance had nothing, so "+ New pad" created the file directly and the helper never had to confirm anything. The shared templates change that: the picker appears, nobody presses its button, and the file is never created. Matched by the control itself — it is an <input type="submit">, and matching its label would also find the "+ New" menu entries still in the page behind the modal.
The lock key was the full path. Nextcloud's database locking writes that into a 64-character column, and an ordinary instance id plus file name is already 68 – so on any instance not backed by Redis the upload would fail with a database error, invisible on a Redis test server. The key is hashed now, prefixed with the app id, and stays 63 characters for any name. Three tests pin it: the bound, that two names never collide, and that delete takes the very same key as the write. Filename rules beyond our own are Nextcloud's: control characters, the configured forbidden characters and reserved names. IFilenameValidator gets the name before it is stored, so the admin reads a sentence instead of a 500 from deep in the storage. The .pad suffix, the leading dot and the path check stay ours. A name over the length limit reported "Template name is required", which is not what happened. It has its own message, and counts characters rather than bytes – the name is what an admin reads in the picker. Adds the missing half of the folder race: another request creating it first was handled but not tested, only the failure that keeps its exception. The creator comment described a second tile the picker does not offer on this branch – that is the follow-up.
delete() validated the name like a new upload. Those rules say what may be stored; applying them to removal means a template that is already there becomes unremovable the moment the rules tighten – ours or the ones Nextcloud enforces – while it stays visible in the list and in the picker. The name is now only trimmed. What makes that safe is the exact match against the listing in the storage, not the shape of the name. The lock recorder in the tests kept only the key, so an accidental LOCK_SHARED would have passed unnoticed. It records the type too, and the write test asserts it.
Splitting the branch left lib/Settings/AdminSettings.php behind, so the section rendered as an empty box: the template reads its headings, button labels and the two API URLs out of that array by name, and a missing key renders as an empty string. Nothing failed, nothing was logged – the section was simply blank between two others. Neither test level could see it. The PHPUnit tests never render the template; the Vitest tests build their own DOM around the script. So two checks now cover exactly that seam: - AdminSettingsTest compares every `$_['…']` and `$_['l10n']['…']` the template reads against what the provider hands over. Removing a single key fails it by name. - An e2e case opens the settings page as an admin and asserts the heading, the button label and that the list actually loads – the view the bug was found in. Also brings over the nine translations that came with those labels.
Nextcloud points a template tile at /core/preview, which has nothing to render for a .pad — the request 400s and the picker falls back to its generic document icon. The tiles carry the app icon directly now, the same way the pad file type does everywhere else. This covers the templates this app offers. A user's own .pad in their Templates folder is built by Nextcloud itself and still shows the generic icon; that would need a listener on the templates event, which is a separate decision.
Two things this PR left to its follow-up, which each make it unstable on its own: The app labels its own picker tiles "Public pad" and "Public pad from URL". Until now an admin could upload a template of exactly that name, and the follow-up would then show two tiles reading the same — indistinguishable to whoever picks one, and no reservation added later can undo a file that is already there. The names are reserved from here on. A pad type can be switched off while the picker is open. Materialising then fails correctly, but the failure was treated as a transient one: the blank fallback ran, failed for the very same reason, and left an empty .pad in the user's files that never opens. Both the template path and Nextcloud's blank entry now remove the file and let the exception travel. This is not hypothetical for the public-only instances this PR unblocks. Three things the settings page got wrong: - A failed listing showed the error and "No shared templates yet" at once, claiming an empty list when we simply could not read one. The empty note is hidden while loading and after a failure, and the test that promised to tell the two apart now actually checks it. - The file input is visually hidden but stayed focusable and nameless; the visible button operates it, so it leaves the tab order. Every delete button read just "Delete" — each now names its template. - Uploading a template wiped the connection test and save results, which it says nothing about. The template status is its own group; verified by putting the coupling back and watching the test fail. Adds the end-to-end contract this feature exists for: an admin uploads a template, it appears as a tile, and creating from it yields a pad with the template's content and a pad id of its own — the last part is what keeps two files from sharing one pad. It cleans up after itself so the tile does not leak into the next run.
The e2e spec uploaded the template before the try block, so an upload that reached the server but failed the assertion on the page left the template on the test instance – and the next run would have seen it as an extra tile. Everything the cleanup covers now happens inside the try. The accessible name of a delete button was the button label glued to the file name, which reads "Löschen Meeting.pad" in German and cannot be fixed from the parts. It is one translatable sentence with a placeholder now, read from the page rather than assembled in the script — the test uses a German sentence to prove exactly that. The reserved-name test checked one spelling of one name. It runs over both constants and a mixed-case variant now.
Three refinements from the second review, none of them behaviour the tests missed: - The lock key was 63 characters against a 64-character column — one to spare, and a longer prefix later would have broken it silently, on non-Redis instances only. Half the hash is plenty for telling file names in one folder apart, which brings it to 55 with the arithmetic written down next to it. The comment also says what the namespace does not do: coordinate with anything else reaching into appdata. - addGlobalTemplate() says that it expects a validated name. The read paths match against the listing and need no such promise; this one hands the name to get() and newFile(), so a second caller bypassing validateName() would have direct reach. - The 2 MiB limit existed only on the server, so a mis-picked large file was read into memory and posted, to fail against post_max_size with a message that explains nothing. The page now says the same sentence the server would, before reading the file — verified by removing the check and watching the test fail.
Code Review by Qodo
1.
|
PR Summary by QodoAdd instance-wide shared pad templates managed by admins
AI Description
Diagram
High-Level Assessment
Files changed (49)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Review of #181 suspected an upload could store a template whose frontmatter names no pad, which would then fail for everyone who picks the tile. It cannot: the parser rejects both a missing and an empty pad_id, so add() answers with its validation error long before the storage sees it. Nothing to fix, but nothing said so either — a data provider now feeds both shapes through the real parser at the service boundary.
…othing Every upload and delete starts another listing while the first may still be in flight, and each answer rendered unconditionally. Whichever arrived last won — putting a deleted template back on the page, or replacing a fresh result with an older error. Answers now carry a generation and a superseded one is dropped, with a test that lets the second request answer first. Two e2e refinements: - The cleanup deleted the uploaded template unconditionally. When the upload itself failed there was no row to click, so the cleanup timed out and buried the real error under its own. It waits for the row and only deletes what is there. - The blank-pad helper waited ten seconds for a picker that never opens on an instance without templates — twice per run, times retries. It races the picker against the created file instead, so whichever happens decides.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
An admin can now define pad templates for the whole instance: upload a
.padfile under Administration settings → Pads and it appears in the template picker for every account, with no per-user setup – the same idea Collabora offers for its own document templates.The templates are ordinary
.padfiles, so creating from one takes the existing path: content copied, placeholders resolved, pad provisioned, binding written. Only the storage and the listing are new.Storage. In the app's
appdatafolder (IAppDataowns it; the nodes come fromIRootFolder, because the template API needs a realOCP\Files\File). No versioning and no trash there, so overwriting is refused unless the request says it means to replace, and the page asks first. Check and write happen together under an exclusive lock on the name –Folder::newFile()overwrites silently, so asking beforehand would leave a window for a second upload. Deleting takes the same lock. The key is hashed and namespaced: Nextcloud's DB locking stores it in a 64-character column, which the plain path exceeds.Validation at upload time, where an admin can act on it: not a pad file, no pad in its frontmatter, external pad, empty, over 2 MiB, a name with a path, or a name the instance forbids (
IFilenameValidator). A failing listing is not swallowed – an empty list must never mean "appdata unreadable". Only the picker tolerates it and logs, because a throwing provider takes every other app's templates down with it.Two fixes that belong here: the
New padentry was tied to protected pads alone, so a public-only instance had no way to reach the templates; and a pad type switched off mid-picker left an empty.padbehind instead of failing. Also reservesPublic pad.padandPublic pad from URL.pad– a later PR labels its own picker tiles with those names, and reserving them afterwards cannot undo a file that is already there.Follow-up: the templates introduced here can be used to move the public-pad and external-pad creation onto Nextcloud's template flow in a later PR, replacing some of the app's own menu and dialog code.
Testing
main(GroupFolders trashbin HTTP 500, three timeouts), verified by re-running them againstmainNotes for review
nextcloud/ocpis a dependency. The bootstrap is built on stubs throughout; switching it over belongs in its own PR.global:<file name>, not<fileId>: the name is matched against the directory listing instead of passed toget(), which is what keeps a crafted value inside the folder.