Release: merge development into beta - #632
Merged
Merged
Conversation
A successful unlock swapped nothing on screen: the padlock stayed shut and the router replaced the whole view, so the only feedback was the vault appearing. The unlock's own success had no signal. The icon now swaps to LockOpenVariant, turns --color-success green and pops in over 400ms, and every unlock path -- online, the offline-snapshot fallback after a network error, and passkey -- holds $router.push for exactly that long. The hold is the point: the redirect unmounts this screen, so pushing first makes the swap unobservable no matter how the keyframes are written. 400ms is invisible next to the argon2 KDF that just ran, and `loading` clears only afterwards so the form cannot be submitted twice. A rejected unlock gets its own flash, and both outcomes are announced through a live region so the padlock is not the only channel carrying them. Under prefers-reduced-motion the pop is dropped (the colour stays -- that is state, not motion) and the wait is skipped, so that path unlocks faster rather than being delayed for an animation that never plays. The animation lives on the icon rather than its wrapper: the wrapper is a full-width block and scaling it would widen the card's content box while the animation runs. Component tests assert the ordering -- the open padlock is on screen while the push has not happened yet -- which is the part a refactor can reverse silently.
--default-clickable-area is 34px on NC 30 and NcAppNavigationItem sizes its icon column to it, so the 30px tinted disc filled the column almost edge to edge: the label sat 2px from a circled vault but 8px from the default vault's bare glyph. Two different gaps in one list, which is what made the coloured rows look misaligned rather than the default one. The column is now 8px wider and the disc 28px, restoring ~7px on both sides without moving the glyph's centre, and the default (colourless) vault gets the same circle in the theme's neutral surface -- so no row is the odd one out. On the active row that neutral disc becomes a low-alpha scrim in the row's own text colour: a grey disc on the solid-primary highlight reads as a stray pill, the same finding that sent the coloured discs to their opaque main-background variant. The colourless disc is a CSS class rather than an inline style because "no colour" has no hex to derive an active-row variant from. The icon-column rule mirrors NcAppNavigationItem's own three-class selector deliberately: that rule sets `flex: 0 0 var(--default-clickable-area)` at higher specificity than a short `:deep(.app-navigation-entry-icon)` form, so the obvious spelling loses the cascade and leaves the column at 34px with no error anywhere.
The live region added with the padlock animation announces "Vault unlocked. Opening your vault…", and check-l10n.js hard-fails on any t() literal that is not a key in en.json — so the frontend check went red on this branch the moment the string landed. The key ships here rather than in the pending bulk-translation branch because it is this feature's own string: a PR that adds an English literal owns getting it into all 36 locales, and the parity ratchet has no slack to absorb it later. Every locale keeps its existing missing count (the total moves 1125 -> 1126 on both sides), so nothing regressed. Only this one key is added. The bulk translation work stays on its own branch.
Adding the unlock and rejection timers to beforeUnmount made it a CHANGED method with no @SPEC line, which hydra gate-16 fails on. It had no docblock at all before, so the gate had nothing to read. The tag is an exclude: the hook clears timers and nothing else, and what each timer drives is specified on the method that arms it. The prose says which one carries risk -- the unlock timer's resolve drives $router.push, so leaving it armed would navigate after the screen is gone.
Bind-mounts the sibling ../thematiq checkout alongside keepiq and openregister, and enables it from the before-starting hook so every `docker compose up` brings all three apps up together. The header comment is updated to say "all mounted apps" rather than naming the two it used to enable.
RESTYLE-PLAN.md and screens/ are working notes and captured screenshots from the restyling pass — generated locally, never source.
Two problems are fixed together because they land in the same files.
First, the last 16 locale sets reach full parity. et, lv, lt and is had a
complete .json but a .js left behind at 582 keys, so their .js is
regenerated from the .json. The remaining 544 English source strings are
translated into Catalan, Albanian, Maltese, Irish, Luxembourgish and
Romansh for both the frontend (.js) and backend (.json) sets.
Second, nine locales carried a large block of the wrong language. When the
files were first seeded, ca was filled from es, lb from de, rm from it, sv
from da, and cs/sk/sl/sr/mk from hr; only part of that block was ever
translated. 3236 donor-identical values are replaced with real Catalan,
Luxembourgish, Romansh, Swedish, Czech, Slovak, Slovenian, Serbian and
Macedonian. sr and mk move from Croatian Latin to Cyrillic. Values that
were already correct are left untouched, so cross-language coincidences
(URL, PIN, Trezor, Kljuc) legitimately remain.
bs is deliberately unchanged: it shares 84% of its values with hr, but the
544 keys translated independently per locale are still 86% identical, so
that overlap is genuine Bosnian/Croatian similarity rather than seeding.
Every locale keeps the en.json key order and its plural-forms footer, and
all {placeholder} and %n tokens were verified to survive translation. The
.js files match `npm run l10n:build` byte for byte, so check:l10n-js is
clean. Outstanding translation debt goes from 8704 to 0 (keepiq#180).
ENFORCED now defaults to the resolved REQUIRED set instead of the literal 'nl', and tests/l10n/parity-ratchet.json is deleted rather than left holding stale slack. All 36 required locales are at zero missing keys, so the two-tier split the gate ran under while the debt was being paid off no longer buys anything — and the ratchet still tolerated 544 missing keys per locale, which would have let a new English source string regress 35 locales without turning CI red. Defaulting to REQUIRED rather than a hard-coded list of 36 means an override of L10N_REQUIRED_LOCALES carries enforcement with it instead of quietly leaving the newly-required locales unenforced. The ratchet machinery is kept for a locale that is ever added to REQUIRED incomplete; it is dormant while ENFORCED covers everything. Verified by deleting one sv key and blanking another: the gate reports both under [ENFORCED] and exits 1, where previously the ratchet absorbed it. README is updated in the same commit because its Translations section documented the two-tier behaviour and the ratchet file by name.
…260906124641 chore(sync): carry beta back into development
`quality / REUSE compliance` has been red on every run since the job was
added. It is non-blocking (`reuse-blocking` defaults to false in the shared
quality.yml), so it never failed a build — it only showed as REUSE ❌ in the
Quality Report comment, on every branch, including ones that changed nothing
about licensing.
`fsfe/reuse lint` on the tree before this commit:
Files with copyright information: 330 / 1466
Files with license information: 329 / 1466
Two causes. The 329 hand-written source files that DO carry
`SPDX-License-Identifier: EUPL-1.2` pointed at a licence text that was not in
the repo — there was no LICENSES/ directory, only a root LICENSE, which REUSE
does not read as the licence for the identifier. And the remaining ~1140
tracked files carry no header at all: the l10n JSON and its generated .js, the
openspec specs, docs, test fixtures, images and lockfiles.
Adding headers to those 1140 files is the other available fix and it is the
wrong one. An SPDX comment is not valid JSON, so every `l10n/*.json` would need
a `.license` sidecar; the same files are regenerated by
`scripts/build-l10n-js.js` and the lockfiles by the package managers. The REUSE
specification has a blanket declaration for exactly this, so this commit uses
it.
REUSE.toml declares one `path = "**"` annotation at `precedence = "closest"`,
which means a file's own header always wins over the blanket. Nothing about the
existing 329 headers changes, and `.editorconfig` — inherited from Nextcloud —
keeps its AGPL-3.0-or-later. "closest" also completes a half header rather than
rejecting it: `tests/e2e/visual/_visual-helpers.ts` declares a licence and no
copyright, `LICENSE` carries `EUPL © the European Union` and no SPDX tag, and
both now resolve.
LICENSES/EUPL-1.2.txt is byte-identical to the repo's own LICENSE.
LICENSES/AGPL-3.0-or-later.txt covers `.editorconfig` and is the copy already
used across the fleet.
After:
Missing licenses: 0 Unused licenses: 0 Bad licenses: 0
Files with copyright information: 1467 / 1467
Files with license information: 1467 / 1467
Compliant with version 3.3 of the REUSE Specification
The gate stays non-blocking. nextcloud-vue sets `reuse-blocking: true` because
it is a published package whose licensing is consumer-facing; keepiq is an app,
and the blanket is what keeps this correct without a gate. Every file added
from here inherits EUPL-1.2 and the Conduction copyright with nobody having to
remember anything, so the only way to regress is to vendor a file whose own
header names a licence with no text in LICENSES/ — and that shows up as REUSE ❌
in the Quality Report on the PR that does it.
LICENSES/EUPL-1.2.txt was copied from keepiq's own root LICENSE, and that file is not the canonical EUPL text. Every other app in the fleet — OpenRegister, OpenConnector, OpenCatalogi, docudesk — ships a root LICENSE that is byte-identical to the European Commission's text as published in spdx/license-list-data, and so does nextcloud-vue's LICENSES/EUPL-1.2.txt. keepiq's copy has had its typographic quotes stripped to ASCII at some point: `'the EUPL'` where the licence reads `‘the EUPL’`. Eighteen lines differ and the difference is nothing but those quote characters. That is tolerable in a root LICENSE, which people read. It is not what belongs in LICENSES/, which is the text a machine resolves `SPDX-License-Identifier: EUPL-1.2` against, and which should be the same bytes everywhere in the fleet. This replaces it with the canonical text, byte-identical to nextcloud-vue/LICENSES/EUPL-1.2.txt and to the SPDX license list (md5 41fa1e32337666d37d19786aebbf3923). LICENSES/AGPL-3.0-or-later.txt was already canonical and is untouched. `reuse lint` after the swap: 1468 / 1468 files with copyright and licensing information, 0 missing, 0 unused, compliant with version 3.3 of the REUSE Specification. The root LICENSE keeps its ASCII quotes for now; aligning it is a separate change.
Both rm files carried the Italian translation of "Vault unlocked. Opening
your vault…" byte for byte — the value was copied from it.json. The parity
ratchet cannot see that: the key is present and non-empty, so a
wrong-language value counts as translated.
It now reads "Cassaforta debloccada. Vossa cassaforta vegn averta…", using
the noun and verb forms the file's own genuine Romansh already uses
("Debloccescha l'emprim tia cassaforta e cuntinuescha lura.") rather than
introducing a second spelling.
The pre-existing Italian entries elsewhere in rm.json are untouched — that
is a separate cleanup for the bulk-translation branch.
Three signals on this screen did not do what their own docblocks said. Reduced motion dropped the announcement, not just the animation. playUnlockAnimation returned an already-resolved promise there, so `unlocked = true` and `$router.push` landed in the same task and the role="status" region was unmounted within a frame or two of getting its text — far below what a screen reader needs to speak it. Reduced motion is a preference blind screen-reader users commonly have on, so the one viewer who cannot see the open padlock was also the one who heard nothing about it. The hold now shrinks to UNLOCK_ANNOUNCE_MS (500 ms) instead of vanishing: still the faster of the two paths, no longer a silent one. Less motion, not less time. The rejection shake did not replay. flashUnlockRejected set the class false and then true again inside $nextTick — but $nextTick is a microtask, so both writes happened in one task with no style recalculation between them, and a CSS animation restarts only when the computed animation-name changes across one. The browser saw no change and replayed nothing, so a second rejection inside the 1100 ms window re-extended the red without shaking. The closed padlock now takes rejectionSeq as its vnode key: a fresh element animates unconditionally, which sidesteps the restart rule instead of fighting it. Setting the flag synchronously also keeps rejectedTimer inside beforeUnmount's reach — the $nextTick callback could arm it after teardown had already run. liveStatus's `checking` branch never announced anything. suiteCheck starts 'pending', so its text was in the region on the very first render — exactly the region-and-message-arrive-together case that is not announced — and then went empty when the check settled. All it did was put a second copy of the spinner's own visible "Checking your vault…" in the accessibility tree for a screen reader to read twice. Dropped; the visible label already carries it. The reduced-motion test now drives fake timers across the announce beat rather than asserting the push has already happened, and the repeat-rejection test asserts a NEW element rather than only a re-set class — jsdom runs no animations, so element identity is the only handle it has on the restart, and it is the right one: the key bump is what the fix turns on.
The colourless vault's active-row treatment assumed one highlight, and NC 34 has another. @nextcloud/vue gates two active-row rules on isLegacy34 (server major < 34) and both carry !important, so they beat the server's own solid-primary rule in core/css/apps.scss: - legacy (NC 32-33): the row is solid --color-primary-element with a whitened label, which is what the 20% white scrim was written for. - modern (NC 34+): the row is only a 16% primary tint over --color-main-background and the label stays --color-main-text. A 20% white scrim there is about 1.05:1 — an invisible disc, i.e. exactly the asymmetry this rule exists to remove. info.xml declares max-version="34", so that is the newest supported target. The scrim is now legacy-only and the modern row gets an opaque --color-main-background disc, which is the variant the COLOURED discs already take on that row, so the two read as one treatment rather than two. The icon-column colour rule gets the same split for the same reason: pinning it to --color-primary-element-text left the colourless glyph and the nested FolderOutline white-on-near-white (~1.28:1) on a modern row, where they should follow the label to --color-main-text. Both new rules are scoped under .keepiq-nav-tree like every other rule in the stylesheet; the old scrim rule was the one that was not.
`display: inline-block` sat on both .lock-screen__icon-open and .lock-screen__icon-rejected. Those are flashes that come and go, and `display` is a box property, so the padlock's box changed shape every time one cleared — which is how an "the icon jumped" bug gets built, even where the current numbers happen to work out. Measured in Chromium across font sizes, line heights and device scale factors, that inline/inline-block flip moves this glyph 0px: the svg is baseline-anchored either way, and at rest the 48px svg simply overflows its own 17px inline span. So this fixes no visible defect today. What it removes is the dependency — .lock-screen__glyph now owns the box for both padlocks and the state classes carry colour and animation only, so clearing a flash cannot move anything and the closed-to-open swap puts both glyphs in the same box.
…ault-glyph-spacing feat(lock-screen): unlock feedback on the padlock and the vault rail
Drops the ../thematiq bind mount and its `occ app:enable` from the before-starting hook. thematiq is a local-development concern that was never meant to land in this repo's compose file. It also broke the stack for everyone else. With no ../thematiq sibling checkout Docker bind-mounts a freshly created empty root-owned directory over custom_apps/thematiq, `occ app:enable thematiq` fails, `set -eu` aborts the hook, and the official Nextcloud image treats a failed before-starting hook as fatal — so the container never starts. openregister is a genuine dependency and stays; thematiq never was one. docker-compose.yml and docker/nextcloud/enable-apps.sh are restored byte-for-byte to their pre-thematiq state, header comments included.
The comment above ENFORCED said the two-tier split collapsed when "the last 8,704 keys landed" and that enforcement had started at `nl`. Neither survives a look at the merge base. There, parity-ratchet.json carried 72 entries totalling 38,804 missing/empty values — 544 per set for the 35 non-nl locales and 362 per set for nl — and ENFORCED was EMPTY, because no locale was complete enough to put in it. nl was the closest and still 362 keys short in each of its two sets. Left as it was, this comment would have become the repo's permanent record of when the debt cleared, understating it roughly fourfold and crediting a staged rollout that never ran. It also mattered for reviewing the branch: a reader of the old figure believes some twenty locales were untouched, when every one of the 36 in fact receives its missing keys here.
With ENFORCED defaulting to REQUIRED the ratchet is unreachable, not merely dormant. Every row in `failures` is drawn from REQUIRED, and the split sends each ENFORCED locale to hardEnforced before any bound is consulted, so the regressions and withinRatchet branches are dead. A locale added to REQUIRED is enforced along with it and hard-fails; it cannot fall through to a bound of its own. The only way back into the machinery is an explicit L10N_PARITY_ENFORCED naming a proper subset of REQUIRED, which is how a future bulk-translation push would stage itself again. That is a fine design, but roughly 115 lines and the closing advice described behaviour the default never exercises. Worse, the advice pointed at `--write` unconditionally: under the default that writes a parity-ratchet.json the next run does not read, the gate stays red, and whoever followed the line concludes the tool is broken rather than that the listed keys still need translating. So the comments now say what is true, the `--write` line is printed only when enforcement is actually narrowed, and `--write` itself exits 2 rather than leaving behind a dead ratchet file that a later run could mistake for sanctioned slack. README's description of the ratchet is corrected to match.
"Vault unlocked. Opening your vault…" was hand-inserted into en.json mid-file — not appended, so it did not come from `npm run test:l10n:write` — and it appears nowhere in src/, lib/ or templates/. Nothing was going to catch it either: check-l10n.js only asserts used -> present, never present -> used. It was not harmless bookkeeping. With ENFORCED defaulting to REQUIRED the key is permanently enforced across 36 locales x 2 files, so it would have had to be carried and kept non-empty forever for a message that is never displayed. The string belongs to the unlock-animation work, which already carries it in all 37 catalogues alongside the LockScreen call site that renders it, so it lands with the change that uses it rather than here. en.json is back to the 1125 keys it had at the merge base, which means this branch now adds no English source string at all.
Moving sr from Croatian Latin to Serbian Cyrillic rewrote only the values
that were still identical to hr, which left untouched the strings that were
already correct Serbian *Latin*. The catalogue came out at 1,070 Cyrillic
and 51 Latin: `Dashboard` rendered as `Kontrolna tabla` beside `Трезор`,
`Тајне` and `Извештаји`, with `Try again`, `Refresh`, `Admin settings`,
`Create vault`, `Filter by type` and `More actions` equally prominent. The
tour copy embedded Latin labels inside Cyrillic prose. Before the move sr
was at least uniformly Latin; after it, internally mixed.
Serbian is digraphic, so either script alone is fine — mixing them within
one catalogue is not. All 52 remaining Latin values are transliterated,
digraph-aware for lj/nj/dž, leaving only URL, BSN, CVV and
`Keepiq {version}`.
Four of them name a navigation item, and there a letter-for-letter
transliteration would have named something the menu does not show: the
Latin block said "Funkcije i plan razvoja" while sr's own label for
Features & roadmap is "Могућности и планови", and it left Flows in English
where sr calls it "Токови". Those four use the real labels instead.
uk was seeded from ru and only partly translated, and it was described as carrying Russian deliberately. Its own numbers say otherwise: 355 of the 581 seeded keys (61.1%) were still identical to ru, against 13 of the 545 keys added later (2.4%). That step change is the same signature that identified ca, lb, rm and the rest as contaminated — a uniformly high figure, as bs shows, is what a genuinely close language pair looks like. 121 values carried ы/э/ъ/ё, letters the Ukrainian alphabet does not have, 194 occurrences in total, and core UI was affected: Save read Сохранить, Cancel Отмена, Settings Настройки, Vault Хранилище, Secrets Секреты, Loading… Загрузка…. The keys this branch adds are proper Ukrainian, so the file was half correct and half donor text. 319 values are now Ukrainian, following the vocabulary the already-correct part of the file established: сховище, секрет, тека, спільний доступ, застосунок. The 49 that stay identical to ru are words the two languages genuinely share — Назад, Причина, Система, Пароль — plus acronyms and brand names, which is the residual overlap the cleaned locales also show (49 for ca, 68 for lb, 74 for sv). No value contains a non-Ukrainian letter any more.
be was seeded from ru exactly as uk was, and shows the same split: 349 of the 581 seeded keys (60.1%) still identical to ru, against 6 of the 545 keys added later (1.1%). 244 values carried и/щ/ъ — letters absent from the Belarusian alphabet — across 550 occurrences, so more than a fifth of the catalogue was demonstrably not Belarusian. 323 values are now Belarusian, following the vocabulary the clean part of the file established: сховішча, сакрэт, папка, супольны доступ, праграма. Two strings this branch itself added also said ссылка where be uses пасылка; both are corrected while the file is open. The 34 left identical to ru are genuinely shared words and acronyms, in line with the other cleaned locales, and no value contains a non-Belarusian letter any more.
The comment claimed `precedence = "closest"` completes the root LICENSE's half header (copyright, no SPDX tag). It does not: reuse-tool's _IGNORE_FILE_PATTERNS matches `^LICEN[CS]E([-\.].*)?$` before any REUSE.toml annotation is consulted, so LICENSE is never covered and was never counted as non-compliant. `reuse spdx` on this branch emits 1467 FileName: entries and ./LICENSE is not among them. The tests/e2e/visual/_visual-helpers.ts half of the example is correct and stays. Also drops the "of 1470 tracked files" denominator, which was neither the tracked count before this branch (1471) nor reuse's own (1466).
The License section promised EUPL-1.2 but said nothing about REUSE, so a contributor had no way to know that new files need no SPDX header, that their own header wins over the repo-wide blanket, or that vendoring a third-party file also needs that licence's text in LICENSES/. With the REUSE job non-blocking, this note is the control.
feat(l10n): complete all 36 locales and hard-enforce parity
The shared quality.yml defaults `reuse-blocking` to false because most fleet apps ship no REUSE.toml or LICENSES/ directory, and its failure hint says to flip it once an app ships both. This branch ships both and lints at 1467/1467, 0 missing, 0 unused, compliant with REUSE 3.3. The blanket `path = "**"` annotation makes every new file compliant on arrival, so blocking cannot be tripped by ordinary work. It leaves one regression mode: vendoring a file whose own SPDX header names a licence with no text in LICENSES/. CONTRIBUTING.md's REUSE note said the gate "reports" that case; it now fails the build, so the sentence says so.
…tion Two Hydra gate failures. gate-16 (spec-coverage): recipientCertificate()'s body changed in this branch - it now resolves through the batch lookup - which makes it a changed method, and it carried no @SPEC. It has one now: the Recipient Shareability Lookup requirement covers both endpoints, which is the point of routing them through one code path. gate-113 (exclusion-evidence): openspec/specs/application-mgmt/spec.md:158 cites SecretRequestServiceTest for four methods that live in ApplicationRequestAdminServiceTest. Pre-existing, unrelated to this branch, and failing on every PR against development; the correction is unambiguous, since all four are in that one class. SecretRequestServiceTest still exists, which is why the citation reads as plausible - it just holds none of them. Assisted-by: ClaudeCode:claude-opus-5
… by id Three review points from #656. DEDUPLICATION IS NO LONGER A LOOP (Barry). The hand-rolled walk called in_array() against a growing array, so collecting n distinct ids cost O(n^2) and a caller could buy that work before the request was refused. array_unique does the same job - first occurrence wins, original order kept - without the rescan. Not a keyed set, despite that being the obvious fix: PHP coerces a numeric-string array key to int, and Nextcloud user ids may be numeric strings, so "0123" would come back as 123. THE BOUND STAYS ON DISTINCT RECIPIENTS. With the scan gone, the reason to check before deduplicating goes with it, and the limit reads as what it actually is: a cap on how many people one lookup may cover, not a defensive guard against payload size. A list naming the same person a hundred times is asking about one person and is answered, not refused. RESULTS ARE CORRELATED BY ID, NOT POSITION (Wilco). The docblock promised callers could zip the response against the request, which duplicate and malformed entries break: ["alice","alice","","bob"] answers about two people. Every entry already carries its userId; that is now the documented contract, with first-seen order kept as a convenience rather than a guarantee. THE MAPPER TEST NO LONGER SWALLOWS REGRESSIONS (Wilco). Its setUp ran a query and skipped on any Throwable, so a syntax error, a binding mistake or a driver incompatibility in findActiveByOwners() would have reported as "migrations have not run" and turned a red build green. It now tests the missing-schema condition directly with tableExists() and catches nothing else - plus a test asserting that guard names the table the mapper actually reads, so a future rename fails the suite instead of silently skipping it. 1185 tests pass; phpmd, phpstan and phpcs clean. Assisted-by: ClaudeCode:claude-opus-5
Review point from #655: presentedValues() accepted any scalar and cast it to a string, so `aud: 123` and `aud: true` were normalised into "123" and "1" before matching, and a mixed array was filtered down to the members that happened to parse. RFC 7519 section 4.1.3 defines `aud` as a StringOrURI or an array of them. Anything else is a malformed assertion, and the claim is now read as such: non-strings present no audience at all, and an array containing any non-string or empty member is rejected WHOLE rather than filtered. The whole-array rule is the part that mattered. Measured against the previous implementation, of six malformed shapes now covered, three changed verdict: ['keepiq', 123] was accepted, now rejected ['keepiq', ['keepiq']] was accepted, now rejected ['keepiq', ''] was accepted, now rejected Filtering a mixed array authenticated the caller on its well-formed remainder, which is precisely what a malformed claim must not achieve. The bare scalars (123, true, 1.5) already failed, but only by accident: they coerced to values that matched nothing. That is a fact about the accepted set containing no numbers, not a property of the check, and an auth path should not lean on it. Assisted-by: ClaudeCode:claude-opus-5
Review point from #654, and a real gap between two halves of this PR. MoveAttachmentBlobs is deliberately non-fatal: a pre-migration step that throws aborts the upgrade and leaves the app half-migrated, so every failure path leaves the source blob where it is and carries on. But AttachmentService was repointed at the new namespace in the same change, so a blob the step declined to move - a collision, a short copy, a storage error - kept its bytes in a place nothing looked. Bytes intact and unreachable is indistinguishable from lost, which means "we preserved the source" was protecting the data without protecting the attachment. Reads and deletes now resolve the new namespace first and fall back to the old one, so availability does not depend on the relocation having completed. That is what makes a non-fatal migration honest rather than merely quiet. Writes still go only to the new namespace, and the fallback is a read-only tail that retires with the rest of the pre-stable compatibility. The collision case had the same shape and is also fixed. Refusing to overwrite protected the destination but said nothing about whether it held the SAME blob, while reads were being switched onto it regardless. Sizes are now compared: equal means the relocation already ran and the source is redundant, so it goes and a resumed migration converges. Different means two distinct blobs share a ref - both are left alone and reported, because choosing between two ciphertexts is a decision about data. Covered by a download-through-the-fallback test and both collision branches. 1180 tests pass; phpmd, phpstan and phpcs clean. Assisted-by: ClaudeCode:claude-opus-5
NcSelect ships min-width: 260px, so two of them could not share a 600px dialog with a button: the pickers took roughly half the width each and the Add button wrapped onto its own line underneath. The row sizes them itself now — 10rem for the type, the rest to the member field, natural width for the button — since neither picker holds anything long, just "User"/"Group" and an id. 10rem rather than less because NcSelect passes inputLabel to its search field instead of an external label, so the floating "Member type" has to fit inside the control. The row still wraps: under ~512px the dialog goes full width and three across does not fit. What is gone is the wrap on a desktop dialog. While lining them up: NcSelect carries its own bottom margin, so on a bottom-aligned row its control box sits one grid baseline above the row edge. The button and the plain-input fallback take the same offset, which is what actually makes the three bottoms agree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…icates feat(sharing): batch recipient-certificate lookup for share dialogs
…obs-v2 refactor(storage): move attachment blobs to the keepiq AppData namespace
Uses the batch lookup from #656: POST /api/v1/shares/recipient-certificates answers, for ids the caller names, which of them hold an active suite and can therefore receive an encrypted copy. It PROBES rather than enumerates, deliberately — a certificate is a public key and safe to hand out, but "who has a keepiq vault" is a membership fact gated by no sharing permission — so the candidate ids come from Nextcloud's own sharee search, which is already permission-filtered, and the probe narrows them. That is two requests per search term, not one, and no list endpoint is involved. Which retires the placeholder this replaces: encryptionSuite's fetchSuiteOwners() read owner ids off GET /suites and waited for that endpoint to widen its scope. It never will, and now need not. A user who cannot receive secrets is LISTED AND UNSELECTABLE rather than filtered out, using the reason the endpoint returns per recipient: "alice is missing" is a bug report, "alice has no encryption suite" is an answer. Reusing that existing string, so no new translation keys. An unknown user and one without a suite report identically by design, so nothing here treats a reason as proof an account exists. Both member kinds now search the server per keystroke-burst on one debounce, and both produce the same option shape, so the picker has a single template: groups are always selectable (they hold no key of their own — members are resolved and key-checked when the fan-out runs), users only with an active suite behind them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ence Wilco's review on #655: two concerns, three minors. CONCERN - THE DEADLINE WAS DOCUMENTATION ONLY. PRE_STABLE_COMPAT_REMOVED_IN was read in three places and every one of them only PUBLISHED it to consumers as removedInAppVersion / emittedFromAppVersion. Nothing compared it against the running version, so the three shims - including acceptance of a legacy audience on the token-exchange path - would have survived 1.0.0 in silence unless somebody remembered. A promise made to integrators that the codebase could not keep. PreStableCompatDeadlineTest fails the build once info.xml reaches the declared version while any shim is still present, naming each one. Below the deadline it asserts the OPPOSITE - that all three are still findable - so a rename cannot leave it silently guarding nothing, which is the failure mode the test exists to prevent. A second test pins the three published deadlines to the shared constant so they cannot drift and tell consumers different dates. Verified by temporarily setting <version> to 1.0.0: the build fails and lists all three shims. Not a runtime check, deliberately. The shims are correct while they are supposed to exist, and refusing to boot an instance over a version number is worse than refusing to ship one. CONCERN - THE PUBLISHED AUDIENCE FIELDS WERE UNASSERTED. Acceptance mechanics are covered thoroughly in JwtAuthServiceTest, but what a consumer is TOLD to send is a different surface: a swapped constant or a dropped field would break self-configuring clients with every acceptance test still green. Two tests now cover the three fields, one of them asserting the canonical value is not also listed as deprecated - a copy-paste that would read as plausible while telling consumers to migrate away from the value they should adopt. MINORS. The DiscoveryController class docblock still opened on the old canonical path and repeated the superseded "renaming would break every consumer" rationale; it now describes dual-serving and points at the deadline test. readAcceptableClaims() still listed aud="doriath" as a required claim after the check moved out; it now names presence only. The spec sentence I introduced ran on so that "the first stable release" appeared to declare the API version; split into three. 1191 tests pass; phpmd, phpstan and phpcs clean. Assisted-by: ClaudeCode:claude-opus-5
A candidate ruled out by `selectable` came out as a near-white block with dim grey text, and the not-allowed cursor appeared only on the row's padding. Both come from the teleport. NcSelect appends its menu to <body> (vue-select's appendToBody, defaulted true), so the menu sits outside the control and loses every theme variable NcSelect declares on it. @nextcloud/vue re-declares the menu's own set on the teleported `.nc-select__dropdown` — background, text, option padding, active and keyboard-focus colours — but not the disabled ones, so the row fell back to vue-select's `:root` defaults: #f8f8f8 on rgba(60,60,60,.5). Themed in assets/app.css because the teleport puts the menu beyond the reach of any component's scoped styles, and the gap is the same for every NcSelect in the app that rules options out. Transparent background, grey text, unchanged everything else: unavailable is said by the colour and the cursor, not by a block of background. The option content also inherits the cursor now — over the text the topmost box is that span, which is why only the padding changed the pointer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A user with no active suite cannot be a member — there is no public key to encrypt their copies to — so the picker no longer lists them at all. Listing them greyed out was the previous take; it put an unpickable row in front of every choice to explain a case most requesters never hit. Which takes several things with it, all of them only there to render that row: the option objects (both kinds are plain ids again, so the picker needs no reduce and no option template), the shareability flag in the store's state, and the global disabled-option theming in assets/app.css, since nothing in the app rules select options out any more. The endpoint's per-recipient `reason` stays unread on purpose. It says no_active_suite both for a user without a suite and for one that does not exist — deliberately, so it cannot be used as a user-existence oracle — so it can tell a caller nothing beyond "not this one". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adding the last candidate swapped the control: with nothing left to offer, the picker was replaced by a free-text field. A control that turns into a different control underneath you is worse than an empty list, and NcSelect already says "No results" for that state. The free-text form is gone rather than hidden, because there was nothing left for it to do: an id typed by hand is either already in the list or cannot be a member at all — a user with no active suite has no public key to encrypt their copies to, and a typo produced exactly the membership whose copies the reconcile pass then reports as permanently missing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Prettier's own output, no assertion touched: two single-quoted test names holding an apostrophe become double-quoted, and one expectation wraps. They arrived unformatted with 9f5b9ea and fail the format gate for the whole branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
gate-16 (spec-coverage) named 12 changed methods with no @SPEC tag, and gate-46 (spec-anchor-existence) one tag pointing at an anchor that does not exist. Reproduced locally against origin/development with the gate package from ConductionNL/.github; all 79 applicable gates pass now. The missing tags, by what they turned out to be: - The four bulk dialogs' `finished` and `title`. Both are the report half of the two-phase dialog — `title` counts its outcome off the report rather than the emptied selection — so both cite the per-item report requirement. - TeamFolderDialog's `onShareFolder`, which had no docblock at all: it is the share step every membership then hangs off, so it cites the share-a-folder requirement. - `memberIdLabel`, `candidatesLoading`, the `newMemberType` watcher and `beforeUnmount` are excluded with a reason each, naming the method that does carry the specified behaviour. A label, a spinner flag, input hygiene the server re-checks, and a cleared timer. The bad anchor was `#requirement-bulk-delete` in the delete dialog's spec, which the live bulk-actions spec has never had — its requirements are named by mechanism (the four operations, chunked execution with a per-item report), not per operation. Pointed at the two it actually tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…cation Wilco's two P2 findings on #655. Both are cases the tests did not cover, and both undo something this PR claimed to have established. OBJECT-VALUED AUDIENCES PASSED THE STRICT CHECK. readAssertionClaims() decodes with json_decode($raw, true), which erases the difference between a JSON array and a JSON object, so `"aud": {"target": "keepiq"}` arrived as a PHP array whose VALUES were iterated and matched. The strict typing added earlier in this PR rejected `123` and mixed arrays and then accepted an object - the same authenticate-on-the-well-formed-remainder hole, one shape further out. array_is_list() would not have fixed it, as Wilco noted: `{"0": "keepiq"}` decodes to a list. The distinction has to survive decoding, so the payload is now decoded WITHOUT assoc and cast at the top level only. Nested objects stay objects and are rejected; nested arrays stay arrays and are accepted. DEPRECATION WAS REPORTED BEFORE AUTHENTICATION. The audience check runs before replay detection, issuer lookup and signature verification - correctly, since it should reject early - but it also emitted the migration warning there. So a forged or replayed assertion produced the same "issuer X still needs to migrate" line as a successful exchange, naming an issuer nobody had verified. That log is the checklist for deciding when `doriath` can stop being accepted, so anyone could have filled it with issuers of their choosing and kept the shim alive indefinitely. Validation and reporting are now separate: assertNamesThisInstance() throws or returns whether the deprecated value was used, and JwtAuthService reports only after the signature, issuer and replay checks have all passed. Covered by the exchange-level tests Wilco asked for - both object shapes, plus a replayed and a badly signed assertion carrying `aud: doriath` asserting no warning is emitted. Verified by reverting each fix in turn: the object cases fail with the assoc decode restored, and both no-report cases fail with the warning moved back before verification. 1195 tests pass; phpmd, phpstan and phpcs clean. Assisted-by: ClaudeCode:claude-opus-5
Bumps [web-token/jwt-library](https://github.com/web-token/jwt-library) from 4.2.1 to 4.2.2. - [Commits](web-token/jwt-library@4.2.1...4.2.2) --- updated-dependencies: - dependency-name: web-token/jwt-library dependency-version: 4.2.2 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
) Bumps [conduction/hydra-gates](https://github.com/ConductionNL/.github) from 1.16.0 to 1.16.1. - [Release notes](https://github.com/ConductionNL/.github/releases) - [Commits](ConductionNL/.github@v1.16.0...v1.16.1) --- updated-dependencies: - dependency-name: conduction/hydra-gates dependency-version: 1.16.1 dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [phpstan/phpstan](https://github.com/phpstan/phpstan-phar-composer-source) from 2.2.10 to 2.2.13. - [Commits](https://github.com/phpstan/phpstan-phar-composer-source/commits) --- updated-dependencies: - dependency-name: phpstan/phpstan dependency-version: 2.2.13 dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
- the picker offers display names, not raw ids. `searchSharees` carries the
`label` the sharee search returns, `searchShareableRecipients` joins the
probe's id-only answer back onto it, and `fetchGroups` maps a gid to
`{ id, label: gid }` so both directories share one option shape. The
NcSelect gets `label` + `reduce`; on an LDAP or SSO instance the old list
of GUIDs was unusable for the thing the picker is for
- a failed lookup is no longer indistinguishable from "nobody matches":
both stores record `candidatesError` — kept apart from share.js's `error`,
which belongs to the share list — and the picker renders it as its own
helper line. NcSelect's `#no-options` slot would have meant
re-implementing its translated "No results" in keepiq's domain, and only
shows while the dropdown is open
- out-of-order responses cannot win any more: a monotonic seq per store
means a superseded search neither writes its list nor clears a flag the
newer one owns, while still returning its own answer to its own caller
- `onCandidateSearch` ignores the empty term vue-select re-emits when an
option is picked, which used to refetch page 1 under the user ~300 ms
after every member added
- the member field's comment no longer claims the server agrees with it.
The narrowing to the sharee-vouched, suite-holding set IS the point and is
deliberately narrower than assertMemberAddable, which asks only that the
user exists and is not the owner
- group.js's header pointed at `encryptionSuite.fetchSuiteOwners`, removed
in 211ccd3; it now names share.searchShareableRecipients
Not from the review: the edit tripped gate-16 (spec-coverage), which flags
changed methods carrying no @SPEC tag — onUpdateOpen, onRunFanOut, onUnshare
and onGradeChange had none. Tagged, onUpdateOpen excluded with a reason,
every anchor checked against the actual headings.
One new string, translated into all 36 locales; l10n:build re-run.
…ations-v2 feat(api): accept both keepiq and doriath on the machine API wire contract
feat(ui): member pickers, a real request form, and vault creation from the rail
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…ck (#668) Two nits from Wilco's approving review on #656, which merged before they were addressed. Comments only - no shipped behaviour changes. THE NUMERIC-STRING KEY EXAMPLE WAS BACKWARDS. normaliseUserIds() explained why a keyed set is unsafe for user ids and then gave the wrong example: it claimed "0123" would come back from array_keys() as 123. PHP coerces an array key that is a CANONICAL decimal integer string, so it is "123" and "-7" that become ints, while "0123", "007" and "1e3" stay strings. Verified rather than reasoned: "123" -> integer "0123" -> string "-7" -> integer "007" -> string, "1e3" -> string The conclusion held - a keyed set is still the wrong tool here - but a wrong example is worse than none, because it teaches the opposite rule to the next reader. Corrected, and it now notes the sharper reason: coercion depends on the SHAPE of the id, so some ids would survive and others would not, which is worse than breaking uniformly. The same error appeared in the PR discussion; a correction is posted there. THE TEST DOCBLOCK STILL DESCRIBED THE OLD CONTRACT. testRecipientCertificatesPreservesOrderAndDeduplicates said the caller zips the response against the request and that order is "part of the contract" - the claim that was removed from the endpoint's own documentation in the same PR. The test still asserts order, which is right; it just is not a guarantee anyone may rely on. Assisted-by: ClaudeCode:claude-opus-5
…) (#676) * docs(encryption-suites): spec — suite self-service ops are owner-scoped (#675) OpenSpec change fix-app-suite-ownership-guard: proposal, tasks, and a new encryption-suites requirement ("Suite Self-Service Operations Are Owner-Scoped"). Records the authorization boundary the Revocation requirement left implicit — the gap that allowed an application suite to be acted on by any session. Spec only — the fix lands in the next commit. Refs #675 Assisted-by: ClaudeCode:claude-opus-5 * fix(encryption-suites): scope suite self-service ops to the owner (#675) validateOwnership() guarded `ownerType === 'user' && ownerId !== $userId`, which passes for every non-user suite: an application suite has ownerType === 'application', so the clause is false and the check is skipped. Any authenticated non-admin could revoke an application's suite by id — a revoked suite blocks every read, locking the application out of its own vault — as well as read it or overwrite its private-key envelope, through show/updatePrivateKey/revoke. Refuse unless the caller owns the suite as a user (ownerType === 'user' && ownerId === $userId), matching the contract CertificateLifecycleService::reissueSuite already enforces. Application suites are managed through the admin application-lifecycle endpoints. Adds regression tests for the application-suite case on revoke and updatePrivateKey (both proven to fail against the old guard). Refs #675 Assisted-by: ClaudeCode:claude-opus-5 * docs(encryption-suites): mark completed tasks for #675 Records the verification done this session: caller audit (1.2), phpmd clean + the baseline linter-debt note (3.2), and the AI-disclosure/ public-path decision (3.4). The three remaining items are merge-time human/CI actions — CI gates (3.1), DCO sign-off (3.3), review-at-merge (3.5) — completed by merging. PR set to close this issue. Refs #675 Assisted-by: ClaudeCode:claude-opus-5
…260912131746 chore(sync): carry beta back into development
The shared Nextcloud on localhost:8080 bind-mounts host checkouts and holds data colleagues are working on. An e2e suite that seeds and deletes objects there is editing someone else's environment, not running tests. tests/e2e/shared-instance.ts adds the opt-in: aiming at localhost port 80 or 8080 off CI now needs KEEPIQ_E2E_ALLOW_SHARED_INSTANCE (or the fleet-wide E2E_ALLOW_SHARED_INSTANCE) to hold that same origin. The flag holds an origin rather than a boolean, so a value left in a shell profile stops permitting every instance the suite ever meets. CI is exempt: there localhost:8080 is the runner's own throwaway Nextcloud. The guard sits in resolveBaseUrl(), which is the one place a target enters the suite: both playwright.config.ts and tests/e2e/playwright.config.ts import BASE_URL from it. The unit test lives under tests/vitest/ because vitest.config.js excludes tests/e2e/**, so a test file left there would never run.
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
4 participants
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.
Automated PR to sync development changes to beta for beta release.
Merging this PR will trigger the beta release workflow.