From 32d835c93bf87cfd1f05f74c3e6955006c53e1a6 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Tue, 11 Aug 2026 10:47:07 +0200 Subject: [PATCH 01/10] =?UTF-8?q?test(e2e):=20dashboard-public-share=20?= =?UTF-8?q?=E2=80=94=20one=20real=20test,=2013=20checked=20reasons,=20and?= =?UTF-8?q?=20the=20header-block=20tags=20moved=20onto=20the=20test=20that?= =?UTF-8?q?=20proves=20them?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes 14 gate-19 findings; gate-19: 176 -> 162 (measured, unscoped, both numbers observed on this tree). NEW TEST — "a logged-in owner opening the share link gets the same read-only page as a stranger" (tests/e2e/ci/public-share.spec.ts). REQ-PSHR-006's fourth scenario is the only one of its four a browser can decide, and it is the one with a real failure mode: the public view noticing a session and upgrading to the editable workspace, so that a link pasted into a group chat becomes an edit surface for every colleague who is logged in. The test opens the share as the dashboard OWNER — the caller with the most permission available — and requires `.public-share-view` plus the read-only badge AND zero `.launchpad-sidebar-toggle` / `.launchpad-grid-item`. The negative half matters: asserting the badge alone would still pass if the editable shell rendered around it. It carries a CONTROL. Before opening the link it loads /apps/launchpad and requires the workspace shell to be visible, which proves the shared admin session is live. Without that, an expired session would make every "no edit affordance" assertion pass for the wrong reason — read-only because the visitor is anonymous, not because the view refuses to upgrade. THE THREE SIBLING SCENARIOS ARE EXCLUDED, AND THE REASON IS STRUCTURAL. "Cannot create widget / edit / delete via public share" are guarded by PublicShareContext, a REQUEST-scoped PHP marker set exactly once, inside PublicShareController::renderShare(). No route mutates anything under /s/{token}, so no request a browser can issue is both a public-share bearer and a mutation: an anonymous PUT /api/dashboard/{uuid} is refused as unauthenticated, and a test asserting 403 there would pass while proving something else entirely. Recorded alongside: the endpoint the first of those scenarios names, POST /api/dashboards/{uuid}/placements, does not exist in appinfo/routes.php. Remaining exclusions, all with claims that were checked before they were written: * REQ-PSHR-007 (3) — viewCount/lastViewedAt are DB columns no anonymous response echoes back, plus distinct IPs or 60s+ waits. * REQ-PSHR-009 (3) + the PSHR-005 throttle scenario (1) — the IThrottler bucket is IP-global by the spec's own wording, and playwright.config.ts runs workers:1 from one runner IP, so spending it hands 429 to every later test. PublicShareControllerTest:: testUnlockReturns429WhenThrottled was opened and confirmed. * REQ-PSHR-010 (2) + the PSHR-004 groupfolder scenario (1) — GroupFolders is not installed on the fixture; code-quality.yml `additional-apps` provisions only openregister and neither seed script adds it. HEADER-BLOCK TAGS MOVED (.github#343). public-share.spec.ts listed four @e2e slugs above every test(). gate-19 resolves a tag's owner with _TestDoc.owner(), which for a tag above all tests returns simply the FIRST test in the file — so all four were credited to one body whatever that body asserted, and a tag for a scenario nobody had tested would have counted the same. These four happen to be genuinely proven by the test they now sit on, but that was luck of authorship, not something the gate checked. Every tag on this branch sits directly above its own test(). --list: 77 tests in 19 files -> 78 in 19. --- openspec/specs/dashboard-public-share/spec.md | 11 ++ tests/e2e/ci/public-share.spec.ts | 140 +++++++++++++++++- 2 files changed, 146 insertions(+), 5 deletions(-) diff --git a/openspec/specs/dashboard-public-share/spec.md b/openspec/specs/dashboard-public-share/spec.md index 3b83180a..758075c2 100644 --- a/openspec/specs/dashboard-public-share/spec.md +++ b/openspec/specs/dashboard-public-share/spec.md @@ -118,6 +118,7 @@ Anonymous users MUST be able to render a dashboard via a public share token, sub - THEN the system MUST return HTTP 404 #### Scenario: Public render must not reuse user session for GroupFolder content +@e2e exclude the GroupFolders app is not installed on the Playwright fixture — code-quality.yml `additional-apps` provisions only ConductionNL/openregister and neither seed script adds groupfolders — so the GroupFolder branch of the render path cannot be entered from a browser at all; same reason as the sibling groupfolder-storage-backend spec - GIVEN dashboard content lives in the GroupFolder backend (sibling spec: `groupfolder-storage-backend`) - WHEN a public-share token renders the dashboard - THEN the system MUST use a service-account path (NOT the current Nextcloud user's permissions) @@ -146,6 +147,7 @@ Password-protected shares MUST require a POST unlock before rendering via the pu - AND NOT block subsequent attempts (throttle is separate) #### Scenario: Unlock is throttled to prevent brute-force +@e2e exclude the throttle bucket is IP-global by design, so spending it would leave every later test in the same job facing 429 from a shared runner IP — playwright.config.ts runs `workers: 1`, `fullyParallel: false`, so there is no isolated IP to burn; asserted instead by PublicShareControllerTest::testUnlockReturns429WhenThrottled - GIVEN a public share with a password - WHEN an attacker sends 11 failed unlock attempts from IP `203.0.113.100` within 60 seconds - THEN the system MUST invoke Nextcloud's `IThrottler` service with action `launchpad_share_password` (limit: 10 per 60 s, IP-global) @@ -162,16 +164,19 @@ Password-protected shares MUST require a POST unlock before rendering via the pu Any mutation endpoint accessed with a public-share token (not a logged-in Nextcloud user session) MUST return HTTP 403. #### Scenario: Cannot create widget on public share +@e2e exclude the guard is PublicShareContext, a request-scoped PHP marker set only inside PublicShareController::renderShare(); no route mutates anything under /s/{token}, so no request a browser can issue is both a public-share bearer and a mutation — an anonymous POST is refused as unauthenticated instead, which would make a passing test prove the wrong thing. Also note the endpoint this scenario names, POST /api/dashboards/{uuid}/placements, does not exist in appinfo/routes.php. Guard asserted by PublicShareContextTest::testRequireMutableThrowsAfterMarkBearer - GIVEN an anonymous user has rendered dashboard via public share - WHEN they attempt `POST /api/dashboards/{uuid}/placements` - THEN the system MUST detect the public-share bearer and return HTTP 403 with message "Cannot modify dashboard via public share" #### Scenario: Cannot edit dashboard via public share +@e2e exclude same reason as the create-widget scenario above — PublicShareContext is request-scoped and only PublicShareController::renderShare() marks it, so an anonymous PUT /api/dashboard/{uuid} is refused as unauthenticated rather than as a public-share bearer; the choke point is DashboardService::updateDashboard()'s requireMutable() call, asserted by PublicShareContextTest::testRequireMutableThrowsAfterMarkBearer - GIVEN a public share allows viewing - WHEN an anonymous user attempts `PUT /api/dashboard/{uuid}` to rename - THEN the system MUST return HTTP 403 #### Scenario: Cannot delete dashboard via public share +@e2e exclude same reason as the two scenarios above — no browser-issuable request is simultaneously a public-share bearer and a DELETE; the choke point is DashboardService::deleteDashboard()'s requireMutable() call, asserted by PublicShareContextTest::testRequireMutableThrowsAfterMarkBearer. The fourth scenario of this requirement, logged-in-user-on-public-share-renders-read-only, IS browser-observable and has a real test in tests/e2e/ci/public-share.spec.ts - GIVEN a public share allows viewing - WHEN an anonymous user attempts `DELETE /api/dashboard/{uuid}` - THEN the system MUST return HTTP 403 @@ -183,6 +188,8 @@ Any mutation endpoint accessed with a public-share token (not a logged-in Nextcl ### Requirement: REQ-PSHR-007 View Count Debouncing +@e2e exclude viewCount and lastViewedAt are DB columns that no anonymous response echoes back, so a browser cannot read the value the debounce is about; the three scenarios additionally need either distinct client IPs (one CI runner has one) or 60-second-plus wall-clock waits inside a 60s per-test timeout + View counts MUST be incremented at most once per minute per (token, client IP) pair to prevent refresh-spam inflation. #### Scenario: Repeated renders from same IP in one minute @@ -225,6 +232,8 @@ Shares with `expiresAt < now()` OR `revokedAt IS NOT NULL` MUST return HTTP 404 ### Requirement: REQ-PSHR-009 Brute-Force Protection +@e2e exclude all three scenarios spend the IThrottler `launchpad_share_password` bucket, which the spec itself defines as IP-global; playwright.config.ts runs workers:1 / fullyParallel:false from one runner IP, so a test that exhausts it hands 429 to every test that follows and the throttle-resets scenario additionally needs a 60s+ wall-clock wait against a 60s test timeout. The 429 path is asserted by PublicShareControllerTest::testUnlockReturns429WhenThrottled + Failed unlock attempts MUST be throttled via Nextcloud's `IThrottler` to prevent password guessing. #### Scenario: 10 failed unlocks per 60 seconds allowed @@ -249,6 +258,8 @@ Failed unlock attempts MUST be throttled via Nextcloud's `IThrottler` to prevent ### Requirement: REQ-PSHR-010 Service-Account File Read for GroupFolder Content +@e2e exclude both scenarios are about which account the GroupFolders backend is read as, and the GroupFolders app is not installed on the Playwright fixture — code-quality.yml `additional-apps` provisions only ConductionNL/openregister and neither seed script adds it — so the branch cannot be entered from a browser; same reason the sibling groupfolder-storage-backend spec carries + Public shares referencing dashboards with GroupFolder-backed content MUST use a service-account read path, not the viewer's session. #### Scenario: Public share rendering GroupFolder content uses service account diff --git a/tests/e2e/ci/public-share.spec.ts b/tests/e2e/ci/public-share.spec.ts index dc9e23b4..c689edce 100644 --- a/tests/e2e/ci/public-share.spec.ts +++ b/tests/e2e/ci/public-share.spec.ts @@ -28,11 +28,21 @@ * legs could pass while the rendered page still required a session, and that * is exactly the bug a public link has to not have. * - * Scenarios covered: - * @e2e dashboard-public-share::public-render-via-valid-token-without-password - * @e2e dashboard-public-share::invalid-token-returns-404 - * @e2e dashboard-public-share::revoked-token-returns-404 - * @e2e dashboard-public-share::soft-revoke-a-public-share + * WHY THERE IS NO `Scenarios covered:` BLOCK HERE ANY MORE (.github#343) + * ===================================================================== + * There was one, listing four `@e2e` slugs. gate-19 resolves the owner of a + * tag with `_TestDoc.owner()`, and for a tag that sits ABOVE every `test(` the + * owner it picks is simply the FIRST test in the file — so all four slugs were + * credited to one test body regardless of what that body asserted, and a tag + * for a scenario nobody had written a test for would have counted just the + * same. The four below happen to be genuinely proven by the test they now sit + * on (it brackets a valid-token render with an unissued-token control and a + * post-revoke control, which is all four), but that was true by luck of + * authorship rather than by anything the gate checked. + * + * Tags therefore live directly above the `test()` that proves them, here and + * in every file this branch touches. A tag with no test under it is now a + * syntax nobody can write by accident. * * @spec openspec/specs/dashboard-public-share/spec.md */ @@ -84,6 +94,10 @@ async function anonymousApi(baseURL: string): Promise { } test.describe('anonymous public share', () => { + // @e2e dashboard-public-share::public-render-via-valid-token-without-password + // @e2e dashboard-public-share::invalid-token-returns-404 + // @e2e dashboard-public-share::revoked-token-returns-404 + // @e2e dashboard-public-share::soft-revoke-a-public-share test('a valid token renders read-only for a visitor with no session, and stops on revoke', async ({ baseURL, browser }) => { const admin = await apiAs(baseURL!, ADMIN) const anon = await anonymousApi(baseURL!) @@ -184,4 +198,120 @@ test.describe('anonymous public share', () => { await admin.dispose() await anon.dispose() }) + + /* + * REQ-PSHR-006's last scenario, and the only one of its four that a + * browser can decide. + * + * The other three ("cannot create widget / edit / delete via public + * share") are guarded by `PublicShareContext`, a REQUEST-scoped PHP + * marker set exactly once, inside `PublicShareController::renderShare()`, + * after the render succeeds. No route mutates anything under `/s/{token}`, + * so no HTTP request a browser can issue is both a public-share bearer and + * a mutation: an anonymous `PUT /api/dashboard/{id}` is simply an + * unauthenticated request and is refused for that reason instead. Those + * three carry `@e2e exclude` in the spec, with that as the reason. + * + * THIS one is different, and it is the one with a real failure mode. A + * logged-in user — here the admin who OWNS the dashboard, i.e. the caller + * with the most permission available — must get the same read-only render + * as a stranger. The bug this catches is the plausible one: the public + * view noticing a session and "helpfully" upgrading to the editable + * workspace, so that a link pasted into a group chat becomes an edit + * surface for every colleague who happens to be logged in. + * + * The assertions are negative on purpose. Asserting the read-only badge + * alone would still pass if the editable shell rendered AROUND it, so the + * test also requires that none of the workspace's edit affordances exist + * on the page. + */ + // @e2e dashboard-public-share::logged-in-user-on-public-share-renders-read-only + test('a logged-in owner opening the share link gets the same read-only page as a stranger', async ({ baseURL, page }) => { + const admin = await apiAs(baseURL!, ADMIN) + const anon = await anonymousApi(baseURL!) + + const settingsBefore = await admin.get('/index.php/apps/launchpad/api/admin/settings') + expect(settingsBefore.status(), await settingsBefore.text()).toBe(200) + const priorAllowUserDash = (await settingsBefore.json()).allowUserDashboards === true + const enable = await admin.put('/index.php/apps/launchpad/api/admin/settings', { + data: { allowUserDash: true }, + }) + expect(enable.status(), await enable.text()).toBeLessThan(300) + + const created = await admin.post('/index.php/apps/launchpad/api/dashboard', { + data: { name: `E2E Session Share ${Date.now()}` }, + }) + expect(created.status(), await created.text()).toBeLessThan(300) + const createdBody = await created.json() + const uuid = createdBody.dashboard?.uuid ?? createdBody.uuid ?? createdBody.data?.uuid + expect(uuid, `no uuid in create response: ${JSON.stringify(createdBody)}`).toBeTruthy() + + const share = await admin.post( + `/index.php/apps/launchpad/api/dashboards/${uuid}/public-share`, + { data: {} }, + ) + expect(share.status(), await share.text()).toBe(201) + const shareBody = await share.json() + const token = shareBody.token ?? shareBody.data?.token ?? shareBody.share?.token + expect(token, `no token in share response: ${JSON.stringify(shareBody)}`).toBeTruthy() + + /* + * CONTROL — establish that this browser context really is logged in + * before drawing any conclusion from what it is refused. The `page` + * fixture carries playwright.config's `use.storageState` (the admin + * session from global-setup). If that session had expired, every + * "no edit affordance" assertion below would pass for the wrong + * reason: the page would be read-only because the visitor is + * anonymous, not because the public view refuses to upgrade. + */ + await page.goto('/index.php/apps/launchpad') + await expect( + page.locator('.launchpad-sidebar-toggle').first(), + 'CONTROL: the shared admin session must be live, otherwise the read-only assertions below prove nothing', + ).toBeVisible({ timeout: 30_000 }) + + // The same link, in the same authenticated context. + const response = await page.goto(`/index.php/apps/launchpad/s/${token}`) + expect(response?.status(), 'the share page must serve a logged-in visitor too').toBeLessThan(400) + + await expect( + page.locator('.public-share-view'), + 'a logged-in visitor must land on the public read-only view, not the workspace', + ).toBeVisible({ timeout: 15_000 }) + + await expect( + page.locator('.public-share-view__badge'), + 'the read-only badge must be shown to a logged-in visitor exactly as it is to an anonymous one', + ).toBeVisible({ timeout: 15_000 }) + + /* + * THE assertions. `.launchpad-sidebar-toggle` is the workspace shell's + * entry point (the same landmark the CONTROL above waited for, and the + * one `manifest-boot.spec.ts` uses to mean "the app rendered"), and + * `.launchpad-grid-item` is an editable grid cell. Neither belongs on + * a read-only page, and the CONTROL proved this context is capable of + * rendering the first of them. + */ + await expect( + page.locator('.launchpad-sidebar-toggle'), + 'the editable workspace shell must not render on a public-share page, even for the dashboard owner', + ).toHaveCount(0) + await expect( + page.locator('.launchpad-grid-item'), + 'editable grid cells must not render on a public-share page', + ).toHaveCount(0) + + // And the session did not change what the data endpoint hands back: + // the authenticated caller and the anonymous one see the same shape. + const asVisitor = await anon.get(`/index.php/apps/launchpad/s/${token}/data`) + expect(asVisitor.status(), await asVisitor.text()).toBe(200) + + if (priorAllowUserDash === false) { + await admin.put('/index.php/apps/launchpad/api/admin/settings', { + data: { allowUserDash: false }, + }) + } + await admin.dispose() + await anon.dispose() + }) }) From 10144bd27397ea4ba1b672f5e792e9e9a9ddd3b9 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Tue, 11 Aug 2026 10:52:10 +0200 Subject: [PATCH 02/10] =?UTF-8?q?test(e2e):=20dashboard-sharing=20?= =?UTF-8?q?=E2=80=94=2011=20real=20tests=20that=20provision=20the=20second?= =?UTF-8?q?=20user=20CI=20never=20had?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes 11 gate-19 findings, every one of them with a test. gate-19: 162 -> 151 (measured on this tree). WHY THESE WERE OPEN. tests/e2e/dashboard-sharing.spec.ts drives the share sidebar and needs a pre-seeded `recipient` account (LAUNCHPAD_E2E_SHAREE) that tests/e2e/seed.sh does not create — it makes `e2e-grantee` and nothing else — so all four of its tests fail in the CI job and playwright.config.ts excludes the file. gate-19 counts nothing from a file no project runs, so eleven REQ-SHARE scenarios had no proof. Its four @e2e slugs would not have closed them anyway: it writes `owner-adds-a-user-share` where the spec heading slugifies to `owner-adds-a-share`. The new file does not fight that. It PROVISIONS what it needs — three throwaway accounts and a group, through the same OCS provisioning API tests/e2e/fixtures/secondary-user.ts uses — and asserts the sharing contract at the HTTP layer, then removes them in afterAll. The sidebar spec still owns the UI and is untouched. HTTP AND NOT DOM, DELIBERATELY. Every scenario in REQ-SHARE-001/002/ 004/006/009 is about what a SECOND user may see or do. The sidebar renders for the owner; the recipient's half of each scenario has no owner-visible UI, so a DOM assertion could only prove the half already covered. Same reasoning ci/public-share-lifecycle.spec.ts records, and the same conventions: `Authorization: Basic` rather than Playwright's reactive httpCredentials, and `storageState: undefined` on every context so no request is silently served as the admin from global-setup. EVERY TEST CARRIES A CONTROL, because most of these assertions are satisfiable by a build with no check at all: * owner-adds-a-share — the share list is asserted EMPTY first. * recipient-cannot-manage-shares — bob is given `full`, the most permissive level, and proven able to READ the dashboard before his POST is required to 403. * recipient-sees-a-shared-dashboard — bob is proven NOT to see it before the share exists, so an endpoint returning every dashboard on the instance cannot pass. * group-share-grants-visibility — dave, a non-member, is required NOT to gain visibility. * most-permissive-level-wins — carol is proven `view_only` with only the direct share, so the final `full` must come from the group. * owner-has-viewonly-recipient-has-full — the widget add is required to be REFUSED at view_only before it is required to succeed at full. Without that arm the success assertion would pass on a build with no permission check. * search-returns-matching-users-and-groups — a query matching nothing must return nothing, or the search is not filtering. * non-owner-is-denied / idempotent-re-PUT — the row set is compared before and after, ids and timestamps included. TWO PLACES WHERE THE SPEC PROSE NAMES A PATH THE APP DOES NOT SERVE, recorded in the file header rather than worked around silently: 1. REQ-SHARE-002 says the recipient's dashboards come from GET /api/dashboards. That route is dashboardApi#list -> getUserDashboards() -> findByUserId(), the caller's OWN rows only. The union that folds in shares is GET /api/dashboards/visible (getVisibleToUser() -> findSharedDashboards(), under a comment naming REQ-SHARE-002). Behaviour implemented, path stale. 2. REQ-SHARE-002 names a field `effectivePermissionLevel`. No response carries that key; /visible carries `source` + `isOwner` and the resolved level comes from GET /api/dashboard/{id} as `permissionLevel` with `isOwner` and `sharedBy`. The tests assert the keys the app actually sends. The idempotent-re-PUT scenario has two clauses. "No rows change" is asserted here, exactly. "No notifications published" is not observable from a browser, which is why REQ-SHARE-008 already carries its own @e2e exclude; the test says so where it sits. NEGATIVE CONTROL, run before this commit: removing this file whole takes gate-19 from 151 back to 162 and dashboard-sharing reappears at exactly 11. Restoring it returns 151. The gate is reading the tests, not the annotations. --list: 78 tests in 19 files -> 89 in 20. --- tests/e2e/ci/dashboard-share-api.spec.ts | 633 +++++++++++++++++++++++ 1 file changed, 633 insertions(+) create mode 100644 tests/e2e/ci/dashboard-share-api.spec.ts diff --git a/tests/e2e/ci/dashboard-share-api.spec.ts b/tests/e2e/ci/dashboard-share-api.spec.ts new file mode 100644 index 00000000..e8f257d6 --- /dev/null +++ b/tests/e2e/ci/dashboard-share-api.spec.ts @@ -0,0 +1,633 @@ +/* + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * SPDX-License-Identifier: EUPL-1.2 + * + * Owner-driven dashboard sharing, end to end against a live instance. + * + * WHY THIS FILE EXISTS WHEN `tests/e2e/dashboard-sharing.spec.ts` ALREADY DOES + * =========================================================================== + * That file is in `playwright.config.ts`'s `testIgnore`, and correctly so: it + * drives the sharing SIDEBAR and needs a pre-seeded `recipient` account + * (`LAUNCHPAD_E2E_SHAREE`, default `recipient`) that the CI seed does not + * create — `tests/e2e/seed.sh` creates `e2e-grantee` and nothing else. All + * four of its tests fail in that job. Because it never runs, gate-19 counts + * none of its annotations, and eleven REQ-SHARE scenarios have no proof. + * + * Its four `@e2e` slugs additionally do not match any scenario heading in + * `openspec/specs/dashboard-sharing/spec.md` (it writes + * `owner-adds-a-user-share`; the spec's heading slugifies to + * `owner-adds-a-share`), so promoting that file would still not have closed + * them. + * + * This file takes the other route: it PROVISIONS the accounts it needs, via + * the same OCS provisioning API `tests/e2e/fixtures/secondary-user.ts` uses, + * and asserts at the HTTP layer that the sharing contract holds. It does not + * replace the sidebar spec — that one still owns the UI — and it deliberately + * does not touch it. + * + * WHY HTTP AND NOT THE DOM + * ======================== + * Every scenario in REQ-SHARE-001/002/004/006/009 is a statement about what a + * SECOND user is allowed to see or do. The share sidebar renders for the + * owner; the recipient's half of each scenario has no owner-visible UI at all, + * so a DOM assertion could only ever prove the half that is already covered. + * This is the same reasoning `ci/public-share-lifecycle.spec.ts` records for + * sitting in `tests/e2e/ci/`, and this file follows its conventions exactly — + * `Authorization: Basic` rather than Playwright's reactive `httpCredentials`, + * and `storageState: undefined` on every context so no request is silently + * served as the admin from `global-setup.ts`. + * + * TWO PLACES WHERE THE SPEC PROSE NAMES A PATH THE APP DOES NOT SERVE + * ================================================================== + * Recorded here rather than worked around silently, because a reader + * comparing this file to the spec will notice: + * + * 1. REQ-SHARE-002 says the recipient's dashboards come from + * `GET /api/dashboards`. That route is `dashboardApi#list`, which calls + * `DashboardService::getUserDashboards()` -> `findByUserId()` — the + * caller's OWN rows only. The union that includes shares is + * `GET /api/dashboards/visible` (`dashboardApi#visible` -> + * `getVisibleToUser()`, which folds in `findSharedDashboards()` under a + * comment naming REQ-SHARE-002). The behaviour is implemented; the path + * in the prose is stale, so the tests below use the real one. + * + * 2. REQ-SHARE-002 also names a field `effectivePermissionLevel`. No such + * key exists in any response: `/api/dashboards/visible` carries + * `source` + `isOwner`, and the resolved level is served by + * `GET /api/dashboard/{id}` as `permissionLevel` alongside `isOwner` and + * `sharedBy`. The tests assert on the keys the app actually sends. + * + * @spec openspec/specs/dashboard-sharing/spec.md + */ + +import { expect, request, test, type APIRequestContext } from '@playwright/test' + +const ADMIN = { + user: process.env.ADMIN_USER ?? process.env.NC_ADMIN_USER ?? 'admin', + pass: process.env.ADMIN_PASSWORD ?? process.env.NC_ADMIN_PASS ?? 'admin', +} + +/* + * `baseURL` is a TEST-scoped Playwright option and cannot be destructured in + * a worker-scoped hook. The neighbouring public-share specs read the same + * environment variable the config resolves it from; so does this file. + */ +const ENV_BASE_URL = (process.env.BASE_URL ?? process.env.NC_BASE_URL ?? '').replace(/\/$/, '') + +const SETTINGS = '/index.php/apps/launchpad/api/admin/settings' +const DASHBOARDS = '/index.php/apps/launchpad/api/dashboard' +const VISIBLE = '/index.php/apps/launchpad/api/dashboards/visible' +const sharesUrl = (id: number) => `/index.php/apps/launchpad/api/dashboard/${id}/shares` +const dashboardUrl = (id: number) => `/index.php/apps/launchpad/api/dashboard/${id}` +const shareesUrl = (query: string) => `/index.php/apps/launchpad/api/sharees?query=${encodeURIComponent(query)}` + +function basic(user: string, pass: string): string { + return `Basic ${Buffer.from(`${user}:${pass}`).toString('base64')}` +} + +/* + * An API context that is unambiguously ONE named user. + * + * `storageState: undefined` is load-bearing and is not defensive tidying: + * playwright.config.ts sets a top-level `use.storageState` (the admin session + * from global-setup), and a context that inherits it is served as admin + * whatever Authorization header it also sends. Measured on this repo in run + * 31389746411, where a non-admin's create answered 201 with + * `"createdBy":"admin"` — the response naming the user the request had + * actually been served as. Every "bob is refused" assertion below would + * otherwise be an assertion about an administrator. + */ +async function apiAs(creds: { user: string, pass: string }): Promise { + return request.newContext({ + baseURL: ENV_BASE_URL, + storageState: undefined, + extraHTTPHeaders: { + 'OCS-APIRequest': 'true', + Authorization: basic(creds.user, creds.pass), + }, + }) +} + +/** Provision a throwaway account through the OCS provisioning API. */ +async function makeUser(label: string): Promise<{ user: string, pass: string }> { + const user = `e2e-share-${label}-${Date.now()}-${Math.floor(Math.random() * 10_000)}` + // Nextcloud silently rejects passwords under the policy minimum, so this + // is comfortably over it and carries all four character classes. + const pass = `Share-${Math.random().toString(36).slice(2)}A1!` + const admin = await apiAs(ADMIN) + const res = await admin.post('/ocs/v1.php/cloud/users', { form: { userid: user, password: pass } }) + expect(res.ok(), `provisioning ${user} failed: ${res.status()} ${await res.text()}`).toBeTruthy() + await admin.dispose() + return { user, pass } +} + +async function deleteUser(user: string): Promise { + const admin = await apiAs(ADMIN) + await admin.delete(`/ocs/v1.php/cloud/users/${encodeURIComponent(user)}`) + await admin.dispose() +} + +async function makeGroup(label: string): Promise { + const gid = `e2e-share-grp-${label}-${Date.now()}-${Math.floor(Math.random() * 10_000)}` + const admin = await apiAs(ADMIN) + const res = await admin.post('/ocs/v1.php/cloud/groups', { form: { groupid: gid } }) + expect(res.ok(), `creating group ${gid} failed: ${res.status()} ${await res.text()}`).toBeTruthy() + await admin.dispose() + return gid +} + +async function deleteGroup(gid: string): Promise { + const admin = await apiAs(ADMIN) + await admin.delete(`/ocs/v1.php/cloud/groups/${encodeURIComponent(gid)}`) + await admin.dispose() +} + +async function addToGroup(user: string, gid: string): Promise { + const admin = await apiAs(ADMIN) + const res = await admin.post(`/ocs/v1.php/cloud/users/${encodeURIComponent(user)}/groups`, { + form: { groupid: gid }, + }) + expect(res.ok(), `adding ${user} to ${gid} failed: ${res.status()} ${await res.text()}`).toBeTruthy() + await admin.dispose() +} + +/** + * Create a dashboard owned by the caller; return both ids. + * + * The share routes are declared `int $id`, so they take the numeric id — the + * public-share routes next door take the uuid, and the two are not + * interchangeable. + */ +async function createDashboard(api: APIRequestContext, label: string): Promise<{ id: number, uuid: string }> { + const res = await api.post(DASHBOARDS, { data: { name: `E2E Share ${label} ${Date.now()}` } }) + expect(res.status(), await res.text()).toBeLessThan(300) + const body = await res.json() + const dash = body.dashboard ?? body.data?.dashboard ?? body + expect(dash?.id, `no numeric id in create response: ${JSON.stringify(body)}`).toBeTruthy() + return { id: Number(dash.id), uuid: dash.uuid } +} + +/** The share rows the owner can see for a dashboard. */ +async function listShares(api: APIRequestContext, id: number): Promise>> { + const res = await api.get(sharesUrl(id)) + expect(res.status(), await res.text()).toBe(200) + const body = await res.json() + return Array.isArray(body) ? body : (body.data ?? body.items ?? []) +} + +/** The dashboards visible to the caller, shares folded in. */ +async function listVisible(api: APIRequestContext): Promise>> { + const res = await api.get(VISIBLE) + expect(res.status(), await res.text()).toBe(200) + const body = await res.json() + return body.items ?? body.data?.items ?? (Array.isArray(body) ? body : []) +} + +/* + * A fresh CI instance ships `allow_user_dashboards` OFF, so `POST + * /api/dashboard` answers 403 `personal_dashboards_disabled` (REQ-ASET-003). + * Enabling it is setup, not part of what anything here asserts — the prior + * value is read first and restored afterwards so the instance is left as it + * was found for whatever runs next in the same serial job. + */ +let priorAllowUserDash = true + +/* + * Provisioned once for the whole file: three accounts and a group. Creating + * them per-test would multiply OCS round-trips for no isolation gain, since + * playwright.config.ts runs `workers: 1, fullyParallel: false` and every test + * below creates its OWN dashboard — which is the state that actually needs to + * be isolated. + */ +let bob: { user: string, pass: string } +let carol: { user: string, pass: string } +let dave: { user: string, pass: string } +let salesGroup: string + +test.beforeAll(async () => { + const admin = await apiAs(ADMIN) + const before = await admin.get(SETTINGS) + expect(before.status(), await before.text()).toBe(200) + priorAllowUserDash = (await before.json()).allowUserDashboards === true + const enable = await admin.put(SETTINGS, { data: { allowUserDash: true } }) + expect(enable.status(), await enable.text()).toBeLessThan(300) + await admin.dispose() + + bob = await makeUser('bob') + carol = await makeUser('carol') + dave = await makeUser('dave') + salesGroup = await makeGroup('sales') + await addToGroup(carol.user, salesGroup) +}) + +test.afterAll(async () => { + await Promise.all([ + deleteUser(bob.user), + deleteUser(carol.user), + deleteUser(dave.user), + ]) + await deleteGroup(salesGroup) + + if (priorAllowUserDash === false) { + const admin = await apiAs(ADMIN) + await admin.put(SETTINGS, { data: { allowUserDash: false } }) + await admin.dispose() + } +}) + +test.describe('REQ-SHARE-001 owner-only share management', () => { + // @e2e dashboard-sharing::owner-adds-a-share + test('the owner adds a share and it comes back in the share list with the level she set', async () => { + const owner = await apiAs(ADMIN) + const dash = await createDashboard(owner, 'add') + + // CONTROL — a new dashboard has no shares. Without this, "the list + // contains a bob row" could be true of a list that was never empty. + expect( + await listShares(owner, dash.id), + 'CONTROL: a freshly created dashboard must start with no shares', + ).toHaveLength(0) + + const created = await owner.post(sharesUrl(dash.id), { + data: { shareType: 'user', shareWith: bob.user, permissionLevel: 'view_only' }, + }) + expect(created.status(), await created.text()).toBeLessThan(300) + + const shares = await listShares(owner, dash.id) + expect(shares, 'exactly the one share just added must be listed').toHaveLength(1) + expect(shares[0].shareWith).toBe(bob.user) + expect(shares[0].shareType).toBe('user') + expect(shares[0].permissionLevel).toBe('view_only') + + await owner.dispose() + }) + + // @e2e dashboard-sharing::recipient-cannot-manage-shares + test('a recipient at full level still cannot add a share of their own', async () => { + const owner = await apiAs(ADMIN) + const asBob = await apiAs(bob) + const dash = await createDashboard(owner, 'recipient-mgmt') + + // bob is given the MOST permissive level the model has, so a refusal + // below is about share MANAGEMENT being owner-only and not about bob + // lacking permission generally. + const grant = await owner.post(sharesUrl(dash.id), { + data: { shareType: 'user', shareWith: bob.user, permissionLevel: 'full' }, + }) + expect(grant.status(), await grant.text()).toBeLessThan(300) + + // CONTROL — bob really can reach this dashboard, so the 403 below is + // a statement about the endpoint and not about visibility. + const canRead = await asBob.get(dashboardUrl(dash.id)) + expect( + canRead.status(), + 'CONTROL: a full-level recipient must be able to read the dashboard, otherwise the 403 below proves nothing', + ).toBe(200) + + const attempt = await asBob.post(sharesUrl(dash.id), { + data: { shareType: 'user', shareWith: dave.user, permissionLevel: 'view_only' }, + }) + expect(attempt.status(), await attempt.text()).toBe(403) + + // …and nothing was written. + expect( + await listShares(owner, dash.id), + 'a refused share attempt must not leave a row behind', + ).toHaveLength(1) + + await owner.dispose() + await asBob.dispose() + }) + + // @e2e dashboard-sharing::updating-an-existing-share-replaces-does-not-duplicate + test('re-sharing with the same recipient upgrades the existing row rather than adding a second', async () => { + const owner = await apiAs(ADMIN) + const dash = await createDashboard(owner, 'upsert') + + const first = await owner.post(sharesUrl(dash.id), { + data: { shareType: 'user', shareWith: bob.user, permissionLevel: 'view_only' }, + }) + expect(first.status(), await first.text()).toBeLessThan(300) + expect(await listShares(owner, dash.id)).toHaveLength(1) + + const second = await owner.post(sharesUrl(dash.id), { + data: { shareType: 'user', shareWith: bob.user, permissionLevel: 'full' }, + }) + expect(second.status(), await second.text()).toBeLessThan(300) + + // THE assertion. A duplicate row would also leave bob at `full`, so + // the count is what distinguishes an upsert from an insert. + const shares = await listShares(owner, dash.id) + expect(shares, 're-sharing the same recipient must update, not duplicate').toHaveLength(1) + expect(shares[0].permissionLevel).toBe('full') + + await owner.dispose() + }) +}) + +test.describe('REQ-SHARE-002 listing dashboards visible to a user', () => { + // @e2e dashboard-sharing::recipient-sees-a-shared-dashboard-in-their-list + test('a user share puts the dashboard in the recipient list, tagged as not owned', async () => { + const owner = await apiAs(ADMIN) + const asBob = await apiAs(bob) + const dash = await createDashboard(owner, 'visible') + + // CONTROL — before the share, bob must NOT see it. This is the half + // that makes the positive assertion mean anything: a union endpoint + // that returned every dashboard on the instance would pass the + // positive check alone. + const beforeIds = (await listVisible(asBob)).map(d => Number(d.id)) + expect( + beforeIds, + 'CONTROL: an unshared dashboard must not appear in another user\'s visible list', + ).not.toContain(dash.id) + + const grant = await owner.post(sharesUrl(dash.id), { + data: { shareType: 'user', shareWith: bob.user, permissionLevel: 'add_only' }, + }) + expect(grant.status(), await grant.text()).toBeLessThan(300) + + const entry = (await listVisible(asBob)).find(d => Number(d.id) === dash.id) + expect(entry, 'the shared dashboard must appear in the recipient\'s visible list').toBeTruthy() + expect(entry!.isOwner, 'the recipient is not the owner and the payload must say so').toBe(false) + + // The resolved level is served by GET /api/dashboard/{id}, not by the + // list — see the header note about `effectivePermissionLevel`. + const detail = await asBob.get(dashboardUrl(dash.id)) + expect(detail.status(), await detail.text()).toBe(200) + const detailBody = await detail.json() + expect(detailBody.permissionLevel).toBe('add_only') + expect(detailBody.isOwner).toBe(false) + expect(detailBody.sharedBy).toBe(ADMIN.user) + + await owner.dispose() + await asBob.dispose() + }) + + // @e2e dashboard-sharing::group-share-grants-visibility-to-all-group-members + test('a group share reaches a member who was never named individually', async () => { + const owner = await apiAs(ADMIN) + const asCarol = await apiAs(carol) + const asDave = await apiAs(dave) + const dash = await createDashboard(owner, 'groupshare') + + const grant = await owner.post(sharesUrl(dash.id), { + data: { shareType: 'group', shareWith: salesGroup, permissionLevel: 'view_only' }, + }) + expect(grant.status(), await grant.text()).toBeLessThan(300) + + // carol is in the group and was never named. + const carolEntry = (await listVisible(asCarol)).find(d => Number(d.id) === dash.id) + expect(carolEntry, 'a group member must see a dashboard shared with their group').toBeTruthy() + + // CONTROL — dave is NOT in the group. Without this, "carol can see it" + // would also hold if group shares granted visibility to everyone. + const daveIds = (await listVisible(asDave)).map(d => Number(d.id)) + expect( + daveIds, + 'CONTROL: a non-member must not gain visibility from a group share', + ).not.toContain(dash.id) + + const detail = await asCarol.get(dashboardUrl(dash.id)) + expect(detail.status(), await detail.text()).toBe(200) + expect((await detail.json()).permissionLevel).toBe('view_only') + + await owner.dispose() + await asCarol.dispose() + await asDave.dispose() + }) + + // @e2e dashboard-sharing::most-permissive-level-wins-when-a-user-matches-multiple-shares + test('a user matched by both a personal and a group share gets the more permissive of the two', async () => { + const owner = await apiAs(ADMIN) + const asCarol = await apiAs(carol) + const dash = await createDashboard(owner, 'most-permissive') + + // carol personally at view_only … + const direct = await owner.post(sharesUrl(dash.id), { + data: { shareType: 'user', shareWith: carol.user, permissionLevel: 'view_only' }, + }) + expect(direct.status(), await direct.text()).toBeLessThan(300) + + // CONTROL — with only the view_only share in place she must be + // view_only. This is what proves the final `full` came from the group + // share and not from the resolver defaulting high. + const beforeDetail = await asCarol.get(dashboardUrl(dash.id)) + expect(beforeDetail.status(), await beforeDetail.text()).toBe(200) + expect( + (await beforeDetail.json()).permissionLevel, + 'CONTROL: a lone view_only share must resolve to view_only', + ).toBe('view_only') + + // … and her group at full. + const viaGroup = await owner.post(sharesUrl(dash.id), { + data: { shareType: 'group', shareWith: salesGroup, permissionLevel: 'full' }, + }) + expect(viaGroup.status(), await viaGroup.text()).toBeLessThan(300) + + const afterDetail = await asCarol.get(dashboardUrl(dash.id)) + expect(afterDetail.status(), await afterDetail.text()).toBe(200) + expect( + (await afterDetail.json()).permissionLevel, + 'the more permissive of two matching shares must win', + ).toBe('full') + + await owner.dispose() + await asCarol.dispose() + }) +}) + +test.describe('REQ-SHARE-004 per-share permission overrides the dashboard default', () => { + // @e2e dashboard-sharing::owner-has-viewonly-recipient-has-full + test('a full-level recipient may add a widget to a dashboard whose own level is view_only', async () => { + const owner = await apiAs(ADMIN) + const asBob = await apiAs(bob) + const dash = await createDashboard(owner, 'override') + + // Put the dashboard's OWN level at view_only, so that anything bob is + // allowed to do has to come from his share rather than from the + // dashboard's default. + const setLevel = await owner.put(dashboardUrl(dash.id), { data: { permissionLevel: 'view_only' } }) + expect(setLevel.status(), await setLevel.text()).toBeLessThan(300) + + const viewOnly = await owner.post(sharesUrl(dash.id), { + data: { shareType: 'user', shareWith: bob.user, permissionLevel: 'view_only' }, + }) + expect(viewOnly.status(), await viewOnly.text()).toBeLessThan(300) + + /* + * CONTROL, and the half that makes this test able to fail in the + * right direction: at view_only the add MUST be refused. If it were + * allowed here, the "allowed at full" assertion below would be + * vacuous — it would pass on a build with no permission check at all. + */ + const refused = await asBob.post(`/index.php/apps/launchpad/api/dashboard/${dash.id}/widgets`, { + data: { widgetId: 'label', gridX: 0, gridY: 0, gridWidth: 4, gridHeight: 2 }, + }) + expect( + refused.status(), + `CONTROL: a view_only recipient must be refused the widget add: ${await refused.text()}`, + ).toBe(403) + + // Upgrade the share only. Nothing else about the dashboard changes. + const upgrade = await owner.post(sharesUrl(dash.id), { + data: { shareType: 'user', shareWith: bob.user, permissionLevel: 'full' }, + }) + expect(upgrade.status(), await upgrade.text()).toBeLessThan(300) + + const allowed = await asBob.post(`/index.php/apps/launchpad/api/dashboard/${dash.id}/widgets`, { + data: { widgetId: 'label', gridX: 0, gridY: 0, gridWidth: 4, gridHeight: 2 }, + }) + expect( + allowed.status(), + `a full-level share must override the dashboard's own view_only level: ${await allowed.text()}`, + ).toBeLessThan(300) + + await owner.dispose() + await asBob.dispose() + }) +}) + +test.describe('REQ-SHARE-006 sharee autocomplete', () => { + // @e2e dashboard-sharing::search-returns-matching-users-and-groups + test('the sharee search returns matching users and groups, and omits non-matches', async () => { + const owner = await apiAs(ADMIN) + + // Both provisioned accounts and the group share the `e2e-share-` + // prefix, so one query is expected to match across both arrays. + const res = await owner.get(shareesUrl('e2e-share-')) + expect(res.status(), await res.text()).toBe(200) + const body = await res.json() + const users: string[] = (body.users ?? body.data?.users ?? []).map( + (u: Record) => String(u.id ?? u.shareWith ?? u.label ?? ''), + ) + const groups: string[] = (body.groups ?? body.data?.groups ?? []).map( + (g: Record) => String(g.id ?? g.shareWith ?? g.label ?? ''), + ) + + expect(users, 'a provisioned user matching the query must be offered').toContain(bob.user) + expect(groups, 'a group matching the query must be offered').toContain(salesGroup) + + /* + * CONTROL — a query that matches nothing must come back empty. An + * autocomplete that ignored its query and returned every account + * would satisfy both assertions above. + */ + const noMatch = await owner.get(shareesUrl(`zzz-no-such-principal-${Date.now()}`)) + expect(noMatch.status(), await noMatch.text()).toBe(200) + const noMatchBody = await noMatch.json() + expect( + [...(noMatchBody.users ?? []), ...(noMatchBody.groups ?? [])], + 'CONTROL: a query matching nothing must return nothing — otherwise the search is not filtering', + ).toHaveLength(0) + + await owner.dispose() + }) +}) + +test.describe('REQ-SHARE-009 bulk replace', () => { + // @e2e dashboard-sharing::replace-adds-upgrades-and-removes-in-one-call + test('one PUT upgrades one recipient, adds another, and drops the rest', async () => { + const owner = await apiAs(ADMIN) + const dash = await createDashboard(owner, 'replace') + + for (const payload of [ + { shareType: 'user', shareWith: bob.user, permissionLevel: 'view_only' }, + { shareType: 'user', shareWith: carol.user, permissionLevel: 'view_only' }, + { shareType: 'group', shareWith: salesGroup, permissionLevel: 'view_only' }, + ]) { + const res = await owner.post(sharesUrl(dash.id), { data: payload }) + expect(res.status(), await res.text()).toBeLessThan(300) + } + expect(await listShares(owner, dash.id), 'three shares must exist before the replace').toHaveLength(3) + + const replaced = await owner.put(sharesUrl(dash.id), { + data: { + shares: [ + { shareType: 'user', shareWith: bob.user, permissionLevel: 'full' }, + { shareType: 'user', shareWith: dave.user, permissionLevel: 'view_only' }, + ], + }, + }) + expect(replaced.status(), await replaced.text()).toBeLessThan(300) + + const after = await listShares(owner, dash.id) + expect(after, 'the replace must leave exactly the two rows it was given').toHaveLength(2) + const byRecipient = Object.fromEntries(after.map(s => [String(s.shareWith), String(s.permissionLevel)])) + expect(byRecipient[bob.user], 'bob must be upgraded in place').toBe('full') + expect(byRecipient[dave.user], 'dave must be added').toBe('view_only') + expect(Object.keys(byRecipient), 'carol must be removed').not.toContain(carol.user) + expect(Object.keys(byRecipient), 'the group share must be removed').not.toContain(salesGroup) + + await owner.dispose() + }) + + /* + * The spec's scenario has two clauses: "no rows MUST change" and "no + * notifications MUST be published". Only the first is observable from a + * browser — a notification is a server-side dispatch to a recipient's + * queue, which is why REQ-SHARE-008 carries its own `@e2e exclude`. This + * test asserts the row clause, exactly and completely. + */ + // @e2e dashboard-sharing::idempotent-re-put-publishes-nothing + test('replaying the same PUT changes no rows', async () => { + const owner = await apiAs(ADMIN) + const dash = await createDashboard(owner, 'idempotent') + + const payload = { + shares: [ + { shareType: 'user', shareWith: bob.user, permissionLevel: 'full' }, + { shareType: 'user', shareWith: dave.user, permissionLevel: 'view_only' }, + ], + } + + const first = await owner.put(sharesUrl(dash.id), { data: payload }) + expect(first.status(), await first.text()).toBeLessThan(300) + const afterFirst = await listShares(owner, dash.id) + expect(afterFirst).toHaveLength(2) + + const second = await owner.put(sharesUrl(dash.id), { data: payload }) + expect(second.status(), await second.text()).toBeLessThan(300) + const afterSecond = await listShares(owner, dash.id) + + // Compare the meaningful tuple rather than the raw rows: an id or a + // createdAt that moved is exactly the "row changed" this asserts + // against, so both are included. + const shape = (rows: Array>) => rows + .map(r => `${r.shareType}:${r.shareWith}:${r.permissionLevel}:${r.id}:${r.createdAt}`) + .sort() + expect( + shape(afterSecond), + 'an identical re-PUT must leave every row byte-identical, ids and timestamps included', + ).toEqual(shape(afterFirst)) + + await owner.dispose() + }) + + // @e2e dashboard-sharing::non-owner-is-denied + test('a non-owner cannot bulk-replace the share list, and nothing moves when they try', async () => { + const owner = await apiAs(ADMIN) + const asBob = await apiAs(bob) + const dash = await createDashboard(owner, 'replace-denied') + + const grant = await owner.post(sharesUrl(dash.id), { + data: { shareType: 'user', shareWith: bob.user, permissionLevel: 'full' }, + }) + expect(grant.status(), await grant.text()).toBeLessThan(300) + const before = await listShares(owner, dash.id) + + const attempt = await asBob.put(sharesUrl(dash.id), { + data: { shares: [{ shareType: 'user', shareWith: dave.user, permissionLevel: 'full' }] }, + }) + expect(attempt.status(), await attempt.text()).toBe(403) + + const after = await listShares(owner, dash.id) + expect( + after.map(s => `${s.shareWith}:${s.permissionLevel}`).sort(), + 'a refused replace must modify no rows', + ).toEqual(before.map(s => `${s.shareWith}:${s.permissionLevel}`).sort()) + + await owner.dispose() + await asBob.dispose() + }) +}) From 69de125e8479d0e52b38c12cac7dcb02caabe7f5 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Tue, 11 Aug 2026 11:41:29 +0200 Subject: [PATCH 03/10] fix(e2e): spell the empty cookie jar explicitly, not as storageState: undefined MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `storageState: undefined` and `storageState: { cookies: [], origins: [] }` are not equivalent. Option merging treats an explicit `undefined` as "not supplied", which falls back to the project default — and the project default here is exactly the thing being guarded against: playwright.config.ts sets a top-level `use.storageState` pointing at global-setup's admin session. So the previous spelling risked being a no-op against the very inheritance it documents, and the consequence is silent: a valid session cookie outranks a later Authorization header, so every "bob is refused" assertion would have been an assertion about an administrator. The measurement already in the file header (run 31389746411, a non-admin create answering 201 with "createdBy":"admin") is what that failure looks like. The empty-jar literal cannot be read as "not supplied". The same fix, in the same form, closed an identical false green in pipelinq, where a context created to be anonymous read back ocs.data.id === "admin" — and was only caught because that suite's identity guard had been changed to read the resolved uid instead of an HTTP status. No test behaviour changes; 3 call sites, one comment. --- tests/e2e/ci/dashboard-share-api.spec.ts | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/tests/e2e/ci/dashboard-share-api.spec.ts b/tests/e2e/ci/dashboard-share-api.spec.ts index e8f257d6..c8ad8556 100644 --- a/tests/e2e/ci/dashboard-share-api.spec.ts +++ b/tests/e2e/ci/dashboard-share-api.spec.ts @@ -88,19 +88,29 @@ function basic(user: string, pass: string): string { /* * An API context that is unambiguously ONE named user. * - * `storageState: undefined` is load-bearing and is not defensive tidying: + * An EXPLICIT EMPTY JAR is load-bearing here and is not defensive tidying: * playwright.config.ts sets a top-level `use.storageState` (the admin session * from global-setup), and a context that inherits it is served as admin - * whatever Authorization header it also sends. Measured on this repo in run - * 31389746411, where a non-admin's create answered 201 with - * `"createdBy":"admin"` — the response naming the user the request had - * actually been served as. Every "bob is refused" assertion below would - * otherwise be an assertion about an administrator. + * whatever Authorization header it also sends — a valid session cookie + * outranks a later `Authorization` header, so a per-request header is not an + * identity switch. Measured on this repo in run 31389746411, where a + * non-admin's create answered 201 with `"createdBy":"admin"` — the response + * naming the user the request had actually been served as. Every "bob is + * refused" assertion below would otherwise be an assertion about an + * administrator. + * + * The jar is spelled `{ cookies: [], origins: [] }` rather than + * `storageState: undefined`. Those are not equivalent: option merging treats + * an explicit `undefined` as "not supplied", which is exactly the case that + * falls back to the project default — i.e. the very inheritance this is meant + * to prevent. The empty-jar literal cannot be read that way. (The same fix, + * in the same form, closed an identical false green in pipelinq, where a + * context created to be anonymous read back `ocs.data.id === "admin"`.) */ async function apiAs(creds: { user: string, pass: string }): Promise { return request.newContext({ baseURL: ENV_BASE_URL, - storageState: undefined, + storageState: { cookies: [], origins: [] }, extraHTTPHeaders: { 'OCS-APIRequest': 'true', Authorization: basic(creds.user, creds.pass), From f218673bea3cd7455ae6f76559462e2b39d3c2e0 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Tue, 11 Aug 2026 12:01:46 +0200 Subject: [PATCH 04/10] =?UTF-8?q?fix(e2e):=20the=20REQ-SHARE-004=20403=20w?= =?UTF-8?q?as=20the=20ROLE-FEATURE=20allow-list,=20not=20the=20share=20che?= =?UTF-8?q?ck=20=E2=80=94=20and=20the=20control=20was=20green=20for=20that?= =?UTF-8?q?=20same=20wrong=20reason?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI on this branch failed one test: "a full-level recipient may add a widget to a dashboard whose own level is view_only", with `{"error":"Access denied"}`. That body was read as canAddWidget() refusing. It is not attributable that way. `WidgetApiController::denyAddWidget()` has TWO independent 403 branches and both return `ResponseHelper::forbidden()` — the byte-identical body `{"error":"Access denied"}`: 1. PermissionService::canAddWidget() — the share check, which is all REQ-SHARE-004 is about 2. RoleFeaturePermissionService::isWidgetAllowed() — the role-feature widget allow-list, about something else The body cannot distinguish them, so the diagnosis had to come from the fixture. tests/e2e/fixtures/role-feature-permissions.ts installs a RESTRICTIVE `default` row — allowedWidgets: ['activity', 'recommendations'] — and TEN widget-adding specs call it in beforeAll. playwright.config.ts runs workers:1, fullyParallel:false against one instance, so that row was in force here whether this file asked for it or not. isWidgetAllowed() short-circuits for Nextcloud admins; bob is a plain provisioned account, so branch 2 refused `label` outright. That is the failure, and it is a test-fixture collision, not a product bug. THE PART THAT MATTERS MORE THAN THE FIX: the view_only CONTROL arm PASSED THE WHOLE TIME. It expects 403 and branch 2 supplied one. So the control was green while proving nothing whatever about share permissions — a check that could not have failed for the reason it names. It only surfaced because the arm NEXT to it could still fail. Two changes remove the ambiguity instead of working around it: * this file now calls ensureDefaultWidgetRestriction() in its own beforeAll, so the allow-list state is KNOWN rather than inherited from whichever spec happened to run first in the job; * both arms now add `activity`, which that restriction ALLOWS, so branch 2 is constant across them and the share level is the only variable left. The refusal at view_only is attributable to the share check precisely because the SAME widget id succeeds for the SAME user at `full` — that arm is what rules branch 2 out. Also added the splitting probe: after the upgrade POST the share row is asserted to read `full` BEFORE the add is attempted, so any future failure names its own cause — a row still at `view_only` means the upgrade did not take, a row at `full` under a refused add means the per-share level is ignored downstream. Neither assertion was weakened and the control arm is unchanged in strength; it is now merely able to fail for the right reason. Also folds in the correction to the empty cookie jar: `storageState: undefined` does NOT clear an inherited jar — option merging reads an explicit `undefined` as "not supplied" and falls back to the project default, which is the very admin session it was meant to drop. The contexts spell it `{ cookies: [], origins: [] }`, and the stale header line that still described the old form is corrected. gate-19 unchanged at 151. --list unchanged at 89 tests in 20 files. --- tests/e2e/ci/dashboard-share-api.spec.ts | 64 ++++++++++++++++++++++-- 1 file changed, 60 insertions(+), 4 deletions(-) diff --git a/tests/e2e/ci/dashboard-share-api.spec.ts b/tests/e2e/ci/dashboard-share-api.spec.ts index c8ad8556..30b7f7bd 100644 --- a/tests/e2e/ci/dashboard-share-api.spec.ts +++ b/tests/e2e/ci/dashboard-share-api.spec.ts @@ -34,7 +34,7 @@ * This is the same reasoning `ci/public-share-lifecycle.spec.ts` records for * sitting in `tests/e2e/ci/`, and this file follows its conventions exactly — * `Authorization: Basic` rather than Playwright's reactive `httpCredentials`, - * and `storageState: undefined` on every context so no request is silently + * and an explicitly EMPTY cookie jar on every context so no request is silently * served as the admin from `global-setup.ts`. * * TWO PLACES WHERE THE SPEC PROSE NAMES A PATH THE APP DOES NOT SERVE @@ -61,6 +61,40 @@ */ import { expect, request, test, type APIRequestContext } from '@playwright/test' +import { ensureDefaultWidgetRestriction } from '../fixtures/role-feature-permissions' + +/* + * THE WIDGET THE REQ-SHARE-004 TEST ADDS, AND WHY IT IS NOT `label`. + * + * `WidgetApiController::denyAddWidget()` has TWO independent 403 branches and + * both return `ResponseHelper::forbidden()`, i.e. the byte-identical body + * `{"error":"Access denied"}`: + * + * 1. `PermissionService::canAddWidget()` — the share-permission check, + * which is the ONLY thing REQ-SHARE-004 is about. + * 2. `RoleFeaturePermissionService::isWidgetAllowed()` — the role-feature + * widget allow-list, which is about something else entirely. + * + * `tests/e2e/fixtures/role-feature-permissions.ts` installs a RESTRICTIVE + * `default` row — `allowedWidgets: ['activity', 'recommendations']` — and ten + * widget-adding specs call it in `beforeAll`. playwright.config.ts runs + * `workers: 1, fullyParallel: false` against ONE instance, so that row is in + * force here whether or not this file asks for it. `isWidgetAllowed()` + * short-circuits for Nextcloud admins but bob is a plain provisioned account, + * so branch 2 refused `label` outright — measured in CI on this branch, where + * the "allowed at full" arm failed with `Access denied`. + * + * The dangerous half is that the view_only CONTROL arm PASSED THROUGHOUT. It + * expects 403 and branch 2 supplied one, so the control was green while + * proving nothing about share permissions at all. + * + * Two changes remove the ambiguity rather than working around it. This file + * now installs the restriction itself in `beforeAll`, so the state is known + * instead of inherited from whichever spec happened to run first; and it adds + * a widget that restriction ALLOWS, so branch 2 is constant across both arms + * and the share level is the only variable left. + */ +const ALLOWED_WIDGET = 'activity' const ADMIN = { user: process.env.ADMIN_USER ?? process.env.NC_ADMIN_USER ?? 'admin', @@ -223,6 +257,10 @@ test.beforeAll(async () => { expect(enable.status(), await enable.text()).toBeLessThan(300) await admin.dispose() + // Pin the role-feature widget allow-list instead of inheriting whatever + // the previously-run specs left behind — see the ALLOWED_WIDGET note. + await ensureDefaultWidgetRestriction() + bob = await makeUser('bob') carol = await makeUser('carol') dave = await makeUser('dave') @@ -472,11 +510,14 @@ test.describe('REQ-SHARE-004 per-share permission overrides the dashboard defaul * vacuous — it would pass on a build with no permission check at all. */ const refused = await asBob.post(`/index.php/apps/launchpad/api/dashboard/${dash.id}/widgets`, { - data: { widgetId: 'label', gridX: 0, gridY: 0, gridWidth: 4, gridHeight: 2 }, + data: { widgetId: ALLOWED_WIDGET, gridX: 0, gridY: 0, gridWidth: 4, gridHeight: 2 }, }) expect( refused.status(), - `CONTROL: a view_only recipient must be refused the widget add: ${await refused.text()}`, + 'CONTROL: a view_only recipient must be refused the widget add. This refusal is ' + + 'attributable to the share check only because the SAME widget id succeeds for the ' + + 'same user at `full` below — that arm is what rules out the role-feature allow-list ' + + `as the cause. Body: ${await refused.text()}`, ).toBe(403) // Upgrade the share only. Nothing else about the dashboard changes. @@ -485,8 +526,23 @@ test.describe('REQ-SHARE-004 per-share permission overrides the dashboard defaul }) expect(upgrade.status(), await upgrade.text()).toBeLessThan(300) + /* + * SPLITTING PROBE. The share row must actually read `full` before + * the add is attempted, so a failure names its own cause instead of + * leaving the next reader to re-derive it: a row still reading + * `view_only` means the upgrade POST did not take, whereas a row + * reading `full` under a refused add means the per-share level is + * being ignored downstream. + */ + const rowAfterUpgrade = (await listShares(owner, dash.id)) + .find(s => String(s.shareWith) === bob.user) + expect( + rowAfterUpgrade?.permissionLevel, + 'SPLITTING PROBE: the upgrade POST must leave bob\'s share row at `full`', + ).toBe('full') + const allowed = await asBob.post(`/index.php/apps/launchpad/api/dashboard/${dash.id}/widgets`, { - data: { widgetId: 'label', gridX: 0, gridY: 0, gridWidth: 4, gridHeight: 2 }, + data: { widgetId: ALLOWED_WIDGET, gridX: 0, gridY: 0, gridWidth: 4, gridHeight: 2 }, }) expect( allowed.status(), From cec520f4093c2d33ef54650a155f9b71ce10e09b Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Tue, 11 Aug 2026 12:06:50 +0200 Subject: [PATCH 05/10] =?UTF-8?q?test(e2e):=20tile-quick-search=20?= =?UTF-8?q?=E2=80=94=209=20real=20keyboard/DOM=20tests=20for=20REQ-QSEARCH?= =?UTF-8?q?-001..003?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes 9 gate-19 findings, all with tests. gate-19: 151 -> 142. A browser is the only place these can be proven. Every claim in these three requirements is about the DOM or the keyboard: which element holds focus after `/`, whether the browser's own Ctrl+K was prevented, whether a non-matching tile is DIMMED rather than removed from the grid, whether aria-activedescendant tracks the arrow keys. A component test cannot reach the interesting half either, because that half is the bridge between RuntimeShellSearch.vue (the combobox) and Views.vue (the grid) — WorkspaceApp.vue joins them with a plain DOM query, by its own comment, "because the grid DOM lives in a sibling component's tree". The file seeds its own dashboard rather than guessing at the shared fixture, which other specs in this serial job are free to change. Four tiles are created through POST /api/dashboard/{id}/tile with titles chosen to make the spec's ranking rule decidable — "Verlof aanvragen" (prefix) against "Overzicht verlof" (mid-string) — and a per-run stamp so a re-run against a warm instance cannot match a leftover tile. Selectors are the component's own `data-test` hooks, not CSS classes. THE ASSERTIONS THAT DISTINGUISH A REAL IMPLEMENTATION FROM A PLAUSIBLE ONE, since most of these scenarios are satisfiable by something weaker: * slash-focuses-the-bar also requires the input to be EMPTY. A handler that focuses without preventing the default leaves a stray "/" in the field, and the focus assertion alone would not see it. * ctrlk-focuses-the-bar installs a listener and reads `event.defaultPrevented` on the very event the app acted on. Asserting focus alone passes on a handler that lets the browser's own Ctrl+K fire as well. * typing-filters-tiles-by-label counts the GRID before and after and requires it unchanged. A filter implemented by unmounting cells would satisfy every other assertion in that test. It also records every /apps/launchpad/ request fired while typing and requires none — the spec says the filter is entirely client-side. * no-query-stored types a unique probe string and requires it in no request URL, no request BODY, and no localStorage/sessionStorage key or value. It waits for the result list to settle first, so it cannot pass on a store that merely had not flushed yet. * arrow-keys checks that the option named by aria-activedescendant is the one carrying aria-selected="true", and that exactly ONE non-colour marker is un-hidden — colour alone would leave zero, which is the WCAG clause the scenario states. * enter-opens-the-selected-tile records window.open and aborts the route, so it asserts which target the app ASKS for rather than depending on a page load — the technique image-widget.spec.ts already uses for REQ-IMG-003. Three tests carry an explicit CONTROL that focus/dimming is not already in the asserted state before the key is pressed, so each assertion is a change rather than a description of the resting state. The four REQ-QSEARCH-004 fallback scenarios are NOT closed here. They need `quicksearch_fallback_target` changed instance-wide and the shell reloaded, and that setting is read from initial state; they are left open rather than annotated. --list: 89 tests in 20 files -> 98 in 21. --- tests/e2e/tile-quick-search.spec.ts | 497 ++++++++++++++++++++++++++++ 1 file changed, 497 insertions(+) create mode 100644 tests/e2e/tile-quick-search.spec.ts diff --git a/tests/e2e/tile-quick-search.spec.ts b/tests/e2e/tile-quick-search.spec.ts new file mode 100644 index 00000000..ead436aa --- /dev/null +++ b/tests/e2e/tile-quick-search.spec.ts @@ -0,0 +1,497 @@ +/* + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * SPDX-License-Identifier: EUPL-1.2 + * + * The quick-search / launcher bar above the tile grid (tile-quick-search + * REQ-QSEARCH-001..003), driven from the keyboard exactly as a user would. + * + * WHY A BROWSER IS THE ONLY PLACE THIS CAN BE PROVEN + * ================================================== + * Every claim in these three requirements is about the DOM or about the + * keyboard: which element has focus after `/`, whether the browser's own + * Ctrl+K was prevented, whether a non-matching tile is DIMMED rather than + * removed from the grid, whether `aria-activedescendant` tracks the arrow + * keys. None of it is visible to a unit test of the component in isolation, + * because the interesting half is the interaction between + * `RuntimeShellSearch.vue` (which owns the combobox) and `Views.vue` (which + * owns the grid) — `WorkspaceApp.vue` bridges them with a plain DOM query, + * by its own comment, "because the grid DOM lives in a sibling component's + * tree". A component test mounts one side of that bridge. + * + * WHAT THE SELECTORS ARE + * ====================== + * `RuntimeShellSearch.vue` ships stable `data-test` hooks — `quick-search- + * input`, `quick-search-option`, `quick-search-status`, `quick-search-empty` + * — so these tests do not depend on CSS class names that are free to change. + * + * THE FIXTURE BUILDS ITS OWN DASHBOARD + * ==================================== + * The searchable label of a placement is decided by + * `WorkspaceApp.vue::tileSearchLabel()`: a `custom` tile searches by + * `tileTitle`. So the dashboard below is seeded through + * `POST /api/dashboard/{id}/tile` with titles chosen to exercise the ranking + * rule the spec states — prefix beats mid-string beats subsequence — rather + * than relying on whatever the shared fixture happens to contain, which no + * test should have to guess at and which other specs in this serial job are + * free to change. + * + * @spec openspec/specs/tile-quick-search/spec.md + */ + +import { expect, request, test, type APIRequestContext, type Page } from '@playwright/test' + +const ADMIN = { + user: process.env.ADMIN_USER ?? process.env.NC_ADMIN_USER ?? 'admin', + pass: process.env.ADMIN_PASSWORD ?? process.env.NC_ADMIN_PASS ?? 'admin', +} +const ENV_BASE_URL = (process.env.BASE_URL ?? process.env.NC_BASE_URL ?? '').replace(/\/$/, '') + +const APP_URL = '/index.php/apps/launchpad' +const SETTINGS = '/index.php/apps/launchpad/api/admin/settings' +const INPUT = '[data-test="quick-search-input"]' +const OPTION = '[data-test="quick-search-option"]' +const STATUS = '[data-test="quick-search-status"]' + +/* + * Tile titles, chosen to make the ranking rule decidable. + * + * For the query `verlof`: + * "Verlof aanvragen" — PREFIX match + * "Overzicht verlof" — mid-string SUBSTRING match + * For the query `zaak`: + * "Zaaksysteem", "Zaakbrowser" match; "Verlof aanvragen" must not. + * + * A stamp keeps them unique per run so a re-run against a warm instance + * cannot match a leftover tile from a previous one. + */ +const STAMP = `${Date.now()}` +const TILES = [ + `Zaaksysteem ${STAMP}`, + `Zaakbrowser ${STAMP}`, + `Verlof aanvragen ${STAMP}`, + `Overzicht verlof ${STAMP}`, +] + +function basic(user: string, pass: string): string { + return `Basic ${Buffer.from(`${user}:${pass}`).toString('base64')}` +} + +/* + * An API context that is unambiguously the admin. + * + * The jar is spelled `{ cookies: [], origins: [] }` and NOT + * `storageState: undefined`: option merging treats an explicit `undefined` + * as "not supplied" and falls back to the project default, which is the very + * `use.storageState` session this is meant to drop. + */ +async function adminApi(): Promise { + return request.newContext({ + baseURL: ENV_BASE_URL, + storageState: { cookies: [], origins: [] }, + extraHTTPHeaders: { + 'OCS-APIRequest': 'true', + Authorization: basic(ADMIN.user, ADMIN.pass), + }, + }) +} + +let dashboardId: number +let priorAllowUserDash = true + +test.beforeAll(async () => { + const api = await adminApi() + + const before = await api.get(SETTINGS) + expect(before.status(), await before.text()).toBe(200) + priorAllowUserDash = (await before.json()).allowUserDashboards === true + const enable = await api.put(SETTINGS, { data: { allowUserDash: true } }) + expect(enable.status(), await enable.text()).toBeLessThan(300) + + const created = await api.post('/index.php/apps/launchpad/api/dashboard', { + data: { name: `E2E QuickSearch ${STAMP}` }, + }) + expect(created.status(), await created.text()).toBeLessThan(300) + const body = await created.json() + const dash = body.dashboard ?? body.data?.dashboard ?? body + dashboardId = Number(dash.id) + expect(dashboardId, `no dashboard id: ${JSON.stringify(body)}`).toBeTruthy() + + for (const [i, title] of TILES.entries()) { + const res = await api.post(`/index.php/apps/launchpad/api/dashboard/${dashboardId}/tile`, { + data: { + title, + linkType: 'url', + linkValue: `https://example.invalid/${encodeURIComponent(title)}`, + gridX: (i % 2) * 3, + gridY: Math.floor(i / 2) * 2, + gridWidth: 3, + gridHeight: 2, + }, + }) + expect(res.status(), `seeding tile "${title}": ${await res.text()}`).toBeLessThan(300) + } + + const activate = await api.post(`/index.php/apps/launchpad/api/dashboard/${dashboardId}/activate`) + expect(activate.status(), await activate.text()).toBeLessThan(300) + + await api.dispose() +}) + +test.afterAll(async () => { + const api = await adminApi() + if (dashboardId) { + await api.delete(`/index.php/apps/launchpad/api/dashboard/${dashboardId}`) + } + if (priorAllowUserDash === false) { + await api.put(SETTINGS, { data: { allowUserDash: false } }) + } + await api.dispose() +}) + +/** Open the workspace on the seeded dashboard with the search bar mounted. */ +async function openWorkspace(page: Page): Promise { + await page.goto(APP_URL) + // The bar only renders when an active dashboard is resolved + // (`v-if="hasActiveDashboard"` in WorkspaceApp.vue), so its presence is + // also the signal that the shell finished booting. + await expect(page.locator(INPUT)).toBeVisible({ timeout: 30_000 }) + // The tiles have to be in the store before any filtering assertion means + // anything — `searchableTiles` reads the live Pinia placements. + await expect + .poll( + async () => page.locator('.launchpad-grid-item, .grid-stack-item').count(), + { message: 'the seeded tiles must be rendered before searching', timeout: 30_000 }, + ) + .toBeGreaterThanOrEqual(TILES.length) +} + +/** The visible result labels, in the order the listbox presents them. */ +async function optionLabels(page: Page): Promise { + return (await page.locator(OPTION).allInnerTexts()).map(t => t.trim()) +} + +test.describe('tile quick-search — focus (REQ-QSEARCH-001)', () => { + // @e2e tile-quick-search::search-bar-is-present-and-labelled + test('the bar renders above the grid, is wrapped in role=search, and is labelled and tabbable', async ({ page }) => { + await openWorkspace(page) + + const input = page.locator(INPUT) + await expect(input).toBeVisible() + + // `role="search"` is on the wrapper, per RuntimeShellSearch.vue. + await expect( + page.locator('[role="search"]').filter({ has: page.locator(INPUT) }), + 'the input must be wrapped in a role="search" landmark', + ).toHaveCount(1) + + // An accessible name, however it is supplied (the component uses a + // visually-hidden