Share panel: people and groups first, links split internal/public (#529, 2/3) #571
Labels
No labels
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: Cordy/Cairn#571
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Second slice of #529. The UI for what 1/3 (#570) makes possible. Approved from
sharing-rework-529-mockup.htmlrev 1.Blocked by #570 — the panel has nothing to render without the addressee.
Scope
Share with people and groups, first section of the panel: recipient picker with autocomplete over accounts and groups (reusing
GET /api/v1/admin/groups/candidates, the mechanism built in v0.6.204), a role select, and the list of current grantees with role and remove.Links, subdivided into two clearly distinct kinds:
/s/surface unchanged in behaviour: password, expiry, upload, and the existingModeselector (plaintext / fragment / password).Roles as three labels over primitives that already exist — Can view →
read, Can edit →write, Can upload →AllowUpload, the last shown for folders only.States, all drawn in the mockup: nothing shared yet; public links disabled by instance policy (the admin switches already exist — the panel must not offer what the instance forbids); an addressee that no longer resolves, shown as a dead row that says so rather than vanishing; and the recipient picker on a local-only instance, which must offer local accounts and app-owned groups without implying a directory that is not there (the #552 lesson).
Decided, not to be built: recipients cannot re-share — only the owner manages the grant list. Re-sharing is where Nextcloud's permission model becomes genuinely confusing, and adding it later is easier than removing it.
Also here
The handbook sentence from the mockup's block 4, saying what the encryption does and does not protect against now that a share means the server opens the owner's file for the recipient. Same wording should serve #238.
House rules
i18n ×4; #389 desktop-invariant fence gate for any
index.htmlwork; phone treatment follows the #389 sheet pattern rather than a squeezed panel. Mockup is approved, so implementation may proceed once #570 lands.Backend slice landed — #575 merged.
/sharedis live: grants can be created, listed and revoked, and the overlay is wired to the share store.One finding that changes this issue's scope. The issue says the recipient picker reuses
GET /api/v1/admin/groups/candidates. It cannot: that route is registered asrequireAdmin(h.groupCandidates), so the picker would 403 for every non-admin — which is almost everyone who shares a file.Decided with Nikola: a new search-only
GET /api/v1/recipients?q=, minimum 2 characters, capped results, no blank-query listing. The admin endpoint enumerates every account and group in one unauthenticated-by-query call; handing that to every session — or to anything holding a stolen one — is a user-enumeration surface we would be adding on purpose. Requiring a query means you can still find anyone you are roughly looking for, without anyone being able to download the staff list.Second decision: internal sharing gets its own switch.
ShareLinksDisabledsays public share links are turned off on this server — a statement about the public internet, not a request that staff stop sending each other files. Reusing it would have meant an admin closing the public hole silently also stopping internal sharing, with no way to separate the two.ShareInternalDisabledis stored negative like its neighbour, so an instance upgrading into the field keeps sharing on.TestPublicLinkPolicyDoesNotGovernGrantspins the separation.The re-share refusal is now structural, not intended. This was the sharpest edge in the slice. The overlay resolving
/shared/alice/q3.pdfto Alice's real backend path is correct for reading — and is exactly what would have let Bob POST a grant on Alice's file and become a second grantor of it.scope.Driver.Delegatedanswers "was this path reached through someone else's grant", and the API refuses before writing anything. The test asserts both the 403 and that the store stayed empty.Also worth recording: the audit callback in
main.gouses literal verbs in a switch rather than forwarding the verb string. That keeps the #433 registry scanner able to see them, and means an unregistered verb cannot be emitted from the composition root at all.Remaining on #571:
GET /api/v1/recipients?q=— search-only picker source.ShareInternalDisabled, beside the existing sharing switches (i18n ×4).Mockup requirement for the panel and the admin toggle, recorded before that slice starts:
Both mockups must show the whole page the element lives in, not the element on its own. For the share panel that means the file browser with a file selected and the panel open in place — what the user actually sees, including how the panel sits against the list, the toolbar and the details pane. For the toggle it means the admin sharing section as a whole, so the new switch can be judged against the switches already beside it rather than in isolation.
The
sharing-rework-529-mockup.htmlblocks were component-level by design — they existed to settle the model. The implementation mockup is a different artefact and answers a different question: does this fit the page.Picker source landed — #576 merged.
GET /api/v1/recipients?q=replaces the admin candidates endpoint for this feature.Decisions worth keeping, since they shape what the panel can and cannot do:
It cannot be asked for everything. A query is mandatory, minimum 2 characters, and a shorter one is a 400 that says so rather than an empty list. An empty list would let a client treat
""as "list everyone" and get a plausible-looking answer back; a refusal cannot be misread.Prefix-only, on token boundaries. The approved decision was "prefix matches only", and a strict whole-name prefix would have made anyone with a
firstname.surnameaccount unfindable by surname — which is how people actually search. A prefix of any token (.,-,_, space,@) counts; mid-token matching stays out, soanikadoes not matchniand nobody walks the directory two characters at a time. Both halves are pinned by tests, because the useful half and the safe half can each be broken without the other failing.The cap runs after ranking, not before. Exact match, then whole-name prefix, then token prefix, then alphabetical — then cut to 20. Truncating before ranking would have dropped the exact match whenever twenty other names happened to sort ahead of it.
truncatedis returned so the panel can ask for another character rather than silently understating the count.Three smaller ones: the caller is excluded from their own results; a name offered by two sources is one recipient, but a user and a group of the same name are two, because they are two different things to share with; and an unreachable directory degrades the picker to what it can still see rather than failing it open or closed.
The route is registered before the group-store guard in
registerGroups, so the picker works on a local-only instance where the accounts alone are the picker.Remaining on #571: the panel itself, and the admin toggle for
ShareInternalDisabled. Both get a whole-page mockup first, per the note above.UI landed and released — #578, #580, #581, #582, #583 merged, cut as v0.6.212, live on the dogfood.
The admin switch has been checked on the live instance: "Sharing with people and groups" renders beside "Public share links" with the right copy. That was the #580 failure mode (a merged switch that never reached the page, because I did not know about
SET_SLOTS), so it is the one worth confirming by eye — confirmed.Still unverified: the share dialog and the two Shares tabs. Neither has been looked at, and neither has had a live round-trip. Expect a fix round.
Item 4 has moved to #585. The handbook sentence on what the encryption does and does not protect against cannot land on
docs/handbook/sharing.mdas it stands — that page opens by saying there are two ways to send a file and that the page is about links, so a sentence premised on grants has nothing to attach to. #585 is the docs run for the whole slice and carries the sentence, along with the #238 cross-reference it was always meant to serve.That leaves #571 with nothing outstanding but the visual review.
Fix round 1 — #586 merged, v0.6.213 live on the dogfood.
The live pass over v0.6.212 found the sharing policy toggles moving without saving, no save bar, and
Load failed: f is not definedin the console. Root cause was one character inloadSettingsValues(): it read the new switch offf, the parameter name ofsharingPolicyFromFormeleven lines below, instead ofshp.The scope was wider than the switch. That line sits near the top of the function, so the ReferenceError aborted every field below it — audit, OCM, download marking, retention, trash, both quotas, lock max, notification and transfer retention, space folder locks. None of them got
dataset.base, which is whatadmDirtySync()diffs against, so the save bar never appeared for any admin setting in v0.6.212. Nothing was ever written, so there is nothing to undo, but the pages were showing markup defaults rather than stored values.This is the third failure in this slice of the same shape — merged, tests green, broken on screen. #580 was a switch that never reached the page, #582 was a grant rendered as a link, this one is a save bar that never appeared. All three were invisible to CI and obvious within a minute of looking.
The common thread is that
sharing-policy.test.jscovers the pure block and passed the whole time, correctly — every one of these lived in the DOM glue outside the fence.web/test/settings-load.test.jsnow covers that glue for the settings form: it liftsloadSettingsValues, stubs the DOM, and asserts both that it completes and that it assigns everyset-*field it references, with the field list read from source so it does not go stale. Red was witnessed on all four assertions before the fix.Still unverified on the dogfood: the share dialog's people-and-groups path with a real recipient, and the two Shares tabs. The dialog renders — confirmed in the v0.6.212 pass — but nothing has been shared with anyone yet, so the picker, the role select, the grantee list,
via {group}, and the/sharedround trip are all still unexercised.Worth noting for whoever reviews next: with the save bar fixed, the admin switch can finally be turned off, which means the two states the mockup drew for a disabled instance (
share-sections.test.jscovers the logic) can now actually be seen.Live pass done on v0.6.213, both directions, with a real second account.
Verified working end to end:
moverini/ shared by nikola-test /Can view./sharedround trip works. Asnikola-test2, the folder opens at#/shared/nikola-test/moveriniwith a View only badge, and the contents decrypt and list correctly —GET /api/v1/sharedreturns the grant with a server-suppliedvpath, exactly as #582 intended.That closes the substantive question this slice existed to answer: a share can be addressed to a person, and the recipient can actually read it.
Three follow-ups cut, all on v0.7:
Share,Send to peer,Move,Rename,Delete,Lock, plusUpload file/New) beside the View only badge. Backend refuses all of it; the menu is the thing that is wrong. This is the significant one.Can uploadrole was never built; the select offers only view and edit, on a folder. Straight scope gap against this issue.One decision still open, not filed as a bug: the share dialog's policy notices use
class="peernotice"(neutral grey), while the rev-2 mockup draws them amber. The house amber box exists as.notebox.warnovervar(--warn). But v0.6.211 deliberately reserved amber "for a fault the administrator can act on", on the grounds that a supported configuration should not wear a permanent warning colour — and "public links are off by instance policy" is a supported configuration, not a fault. So the mockup and that rule disagree, and which one gives is Nikola's call rather than a defect.Nothing else outstanding on #571 itself. Once #588–#590 are closed, this can close with them.
Decided: the policy notices stay neutral.
class="peernotice", not amber.The v0.6.211 rule holds — the amber style stays reserved for a fault an administrator can act on. "Public links are turned off on this server" is a supported configuration someone chose deliberately, so it is a notice, not a warning, and should not wear a permanent warning colour.
The rev-2 mockup draws these amber and is therefore out of date on this point. Recording it here so the discrepancy is not re-opened as a defect next time someone compares the two.
Fix round 2 shipped — v0.6.214 live on both dogfoods (
cairn-encandcairn-openbao, per the both-instances rule)./sharedoffers what the grant allows — re-sharing never, mutations by role — and the same gate covers read-only space members. The toolbar turned out to be already disabled and merely unstyled; it now looks disabled too.resolveOwnedduring the docs run, parked as a decision rather than decided by documentation.Every gate is covered by lifted-glue tests now (
row-menu.test.js,shares-view.test.js, besidesettings-load.test.js), so the pure-block-right-wiring-wrong class that produced #580/#582/#586/#590 fails the build instead of the page.What remains on this issue is one thing: an eyeball on v0.6.214 as the recipient. Sessions are in-memory, so the rollout logged everyone out — after a fresh login as
nikola-test2, the moverini grant is still in place: the row menu inside/shared/nikola-test/moverinishould offer Open, Download, Copy to, favourites and Details, nothing else; Upload/New should look as dead as they are; and My shares (asnikola-test) should head the group "moverini". If that matches, this issue can close.matches, can be closed now
Live eyeball on the enc dogfood (v0.6.215), signed in as
nikola-test2(recipient):/ shared / nikola-test / moveriniwith a View only chip; Upload file and New both visibly disabled./api/v1/fileson the vpath returnsreadOnly: true.Owner-side heading basename (#590) is covered by the glue tests; not re-eyeballed since this session is the recipient account.
One unrelated find while testing, split out as its own issue: stale search text silently filters a freshly opened folder view down to zero rows while the count footer still reports the unfiltered totals.
Closing — the delegated view behaves as specified.