fix(gate-7): an authentication check is not an authorisation guard (#365)

hydra-gates/scripts/run-hydra-gates.sh

@@ -2389,7 +2389,17 @@ fi
 #     ->forbiddenWords( is not a false guard)
 #   - TemplateResponse return type / instantiation — SPA page renderers
 #   - (delegated guards, see check_no_admin_idor.py: Pattern 1 same-class
-#     guard-helper; Pattern 2 OpenRegister ObjectService/*Mapper RBAC)
+#     guard-helper; Pattern 2 OpenRegister ObjectService/*Mapper RBAC;
+#     Pattern 6 session-identity hand-off; Pattern 7 in-body ownership
+#     comparison, including the deliberate 404 anti-oracle refusal)
+#
+# NOT a guard (.github#365): an AUTHENTICATION check. `if ($user === null)
+# { ... 401 ... }` asks whether anyone is logged in; this gate asks whether
+# THIS caller may touch THIS object. Under #[NoAdminRequired] the framework
+# has already answered the first question, so the clause cannot fail — and it
+# was clearing the gate in all eighteen fleet apps at once. Such a clause is
+# now ignored whatever status it answers with, because authentication-ness is
+# a property of the CONDITION, not of the status code.
 #
 # Exemptions (never IDOR vectors):
 #   - __construct — not a routed action endpoint
@@ -2483,7 +2493,7 @@ if [ "${_idor_ran}" -eq 1 ]; then
         # service layer — which is where reading naturally stops, because the
         # service is what the controller calls. Saying so here costs nothing
         # and is the difference between a triage and a wrong security report.
-        _fail 7 "no-admin-idor" "${_idor_fail} method(s) with NoAdminRequired + no guard — see ${_idor_log}. BEFORE treating any of these as real: the checker sees ONLY the controller method body. If the endpoint reaches storage through a service or mapper, check whether the guard is enforced THERE (e.g. an organisation/tenant filter applied in the query builder) — and note that a deliberate 404-style tenancy refusal IS a guard, chosen so a 403 cannot become an existence oracle for another tenant's ids."
+        _fail 7 "no-admin-idor" "${_idor_fail} method(s) with NoAdminRequired + no guard — see ${_idor_log}. BEFORE treating any of these as real: the checker sees ONLY the controller method body. If the endpoint reaches storage through a service or mapper, check whether the guard is enforced THERE (e.g. an organisation/tenant filter applied in the query builder) — and note that a deliberate 404-style tenancy refusal IS a guard, chosen so a 403 cannot become an existence oracle for another tenant's ids. AND BEFORE 'fixing' any of these: a 'no user -> 401' preamble is NOT the fix (.github#365). It answers whether anyone is logged in, which #[NoAdminRequired] has already settled; the gate ignores it whatever status it returns. The fix is to scope the object to the caller — compare ownership, pass the session identity into the query, or call the app's existing per-object predicate."
     fi
 fi
 

hydra-gates/scripts/test-fixtures/gate-acceptance/UNCOVERED.md

@@ -40,9 +40,17 @@ A reason is a testable claim, so each row states which of four kinds it is.
 
 Two notes on authoring, both measured rather than assumed:
 
-1. `scripts/test-fixtures/gate-acceptance/auth-guards/` exists but carries no
+1. ~~`scripts/test-fixtures/gate-acceptance/auth-guards/` exists but carries no
    `expect.conf` and no source files beyond `appinfo/info.xml`, so it currently
-   contributes **zero** covered gates. It is a stub, not coverage.
+   contributes **zero** covered gates. It is a stub, not coverage.~~
+   **Superseded.** `auth-guards/` is now a real bundle (gate-7, `#353` —
+   verb-object guard predicates), and `authn-vs-authz/` joins it (gate-7,
+   `#365` — an authentication check is not an authorisation guard). Two bundles
+   assert the same gate from opposite directions on purpose: `auth-guards`
+   pins that a real guard is RECOGNISED (the false-positive failure mode),
+   `authn-vs-authz` pins that a non-guard is REFUSED (the false-negative one).
+   A single bundle could be passed by a checker that is broken in the other
+   direction — which is exactly how `#365` survived `#353` and `#360`.
 2. `_enum_tracked` prefers `git ls-files`, and a fixture directory sits inside
    this repository's own work tree — so a planted file must be **committed** to
    be enumerated at all. An untracked plant reproduces the very silence these

hydra-gates/scripts/test-fixtures/gate-acceptance/authn-vs-authz/clean/appinfo/info.xml

@@ -0,0 +1,13 @@
+<?xml version="1.0"?>
+<info xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
+	<id>authnfixture</id>
+	<name>Authn vs Authz Fixture</name>
+	<summary>Fixture app for the gate scope matrix. Not a real app.</summary>
+	<description>Exists so the gate suite can express push / full / diff scopes against a real git history.</description>
+	<version>1.0.0</version>
+	<licence>agpl</licence>
+	<author>Conduction</author>
+	<namespace>AuthnFixture</namespace>
+	<category>tools</category>
+	<dependencies><nextcloud min-version="30" max-version="32"/></dependencies>
+</info>

hydra-gates/scripts/test-fixtures/gate-acceptance/authn-vs-authz/clean/appinfo/routes.php

@@ -0,0 +1,14 @@
+<?php
+/**
+ * @license EUPL-1.2
+ * @copyright Conduction B.V.
+ */
+
+return [
+	'routes' => [
+		['name' => 'ledger#ownershipCheck', 'url' => '/api/entries/{entryId}',              'verb' => 'GET'],
+		['name' => 'ledger#tenancy404',     'url' => '/api/entries/{entryId}/tenancy',      'verb' => 'GET'],
+		['name' => 'ledger#collaborator',   'url' => '/api/entries/{entryId}/collaborator', 'verb' => 'GET'],
+		['name' => 'ledger#handoff',        'url' => '/api/entries/{entryId}/handoff',      'verb' => 'GET'],
+	],
+];

hydra-gates/scripts/test-fixtures/gate-acceptance/authn-vs-authz/clean/lib/Controller/LedgerController.php

@@ -0,0 +1,124 @@
+<?php
+/**
+ * The CLEAN arm for `.github#365` — every method here is genuinely guarded,
+ * and every one of them ALSO carries the `no user -> 401` preamble.
+ *
+ * THE PREAMBLE IS DELIBERATELY KEPT. Without it, this arm would prove only
+ * that a file with no preamble passes, and the fix could have been "flag any
+ * method containing a 401" — which would turn all four of these into findings
+ * and re-create the false-positive problem that cost gate-7 its credibility
+ * (`#353`, `#360`). The clean arm's job is to pin that the preamble is not
+ * being PUNISHED; it is being IGNORED. What clears each method is the guard
+ * that comes after it.
+ *
+ * Four guard shapes, one per method, chosen because each is a distinct route
+ * through the checker and a fix that widens one of them must not blind another:
+ *
+ *   ownershipCheck()  in-body comparison, answered 403   (`_GUARD_BODY_RE`)
+ *   tenancy404()      in-body comparison, answered 404   (Pattern 7 — the
+ *                     anti-oracle refusal gate-7's own FAIL message endorses)
+ *   collaborator()    resolved collaborator predicate    (Pattern 4a)
+ *   handoff()         session identity passed into the data call (Pattern 6 —
+ *                     doriath's shape, hand-verified as zero real exposure)
+ *
+ * This file must produce ZERO findings. Under the pre-`#365` checker it also
+ * produced zero — but for the WRONG reason: the preamble alone cleared all
+ * four, so the guards below were never consulted. That is why the planted arm
+ * is the load-bearing half of this bundle and this one is the abuse control.
+ *
+ * @license EUPL-1.2
+ * @copyright Conduction B.V.
+ */
+
+namespace OCA\AuthnFixture\Controller;
+
+use OCA\AuthnFixture\Service\LedgerService;
+use OCP\AppFramework\Controller;
+use OCP\AppFramework\Http;
+use OCP\AppFramework\Http\Attribute\NoAdminRequired;
+use OCP\AppFramework\Http\JSONResponse;
+use OCP\IRequest;
+use OCP\IUserSession;
+
+class LedgerController extends Controller {
+
+	public function __construct(
+		string $appName,
+		IRequest $request,
+		private readonly LedgerService $ledger,
+		private readonly IUserSession $userSession,
+	) {
+		parent::__construct($appName, $request);
+	}
+
+	/**
+	 * Shape 1 — in-body ownership comparison, answered with 403.
+	 */
+	#[NoAdminRequired]
+	public function ownershipCheck(string $entryId): JSONResponse {
+		$user = $this->userSession->getUser();
+		if ($user === null) {
+			return new JSONResponse(['error' => 'Unauthorized'], Http::STATUS_UNAUTHORIZED);
+		}
+		$entry = $this->ledger->find($entryId);
+		if ($entry['ownerId'] !== $user->getUID()) {
+			return new JSONResponse(['error' => 'Forbidden'], Http::STATUS_FORBIDDEN);
+		}
+		return new JSONResponse($entry);
+	}
+
+	/**
+	 * Shape 2 — the same comparison answered with 404 ON PURPOSE, so that a 403
+	 * cannot become an existence oracle for another owner's ids.
+	 */
+	#[NoAdminRequired]
+	public function tenancy404(string $entryId): JSONResponse {
+		$user = $this->userSession->getUser();
+		if ($user === null) {
+			return new JSONResponse(['error' => 'Unauthorized'], Http::STATUS_UNAUTHORIZED);
+		}
+		$entry = $this->ledger->find($entryId);
+		if ($entry['ownerId'] !== $user->getUID()) {
+			return new JSONResponse(['error' => 'Not found'], Http::STATUS_NOT_FOUND);
+		}
+		return new JSONResponse($entry);
+	}
+
+	/**
+	 * Shape 3 — the predicate lives on an injected collaborator and is read out
+	 * of that class's own source.
+	 */
+	#[NoAdminRequired]
+	public function collaborator(string $entryId): JSONResponse {
+		$user = $this->userSession->getUser();
+		if ($user === null) {
+			return new JSONResponse(['error' => 'Unauthorized'], Http::STATUS_UNAUTHORIZED);
+		}
+		if ($this->ledger->canAccessEntry($entryId, $user->getUID()) === false) {
+			return new JSONResponse(['error' => 'Not found'], Http::STATUS_NOT_FOUND);
+		}
+		return new JSONResponse($this->ledger->find($entryId));
+	}
+
+	/**
+	 * Shape 4 — the caller's identity is handed to the data layer alongside the
+	 * caller-supplied id, so the lookup is scoped to a value the caller cannot
+	 * forge. This is how doriath writes almost every endpoint.
+	 */
+	#[NoAdminRequired]
+	public function handoff(string $entryId): JSONResponse {
+		$userId = $this->sessionUserId();
+		if ($userId === null) {
+			return new JSONResponse(['error' => 'Unauthorized'], Http::STATUS_UNAUTHORIZED);
+		}
+		return new JSONResponse($this->ledger->findOwned(entryId: $entryId, userId: $userId));
+	}
+
+	private function sessionUserId(): ?string {
+		$user = $this->userSession->getUser();
+		if ($user === null) {
+			return null;
+		}
+		return $user->getUID();
+	}
+}

hydra-gates/scripts/test-fixtures/gate-acceptance/authn-vs-authz/clean/lib/Service/LedgerService.php

@@ -0,0 +1,26 @@
+<?php
+/**
+ * @license EUPL-1.2
+ * @copyright Conduction B.V.
+ */
+
+namespace OCA\AuthnFixture\Service;
+
+class LedgerService {
+
+	public function find(string $entryId): array {
+		return ['id' => $entryId, 'ownerId' => 'alice'];
+	}
+
+	public function findOwned(string $entryId, string $userId): array {
+		$entry = $this->find($entryId);
+		if ($entry['ownerId'] !== $userId) {
+			throw new \RuntimeException('Not found');
+		}
+		return $entry;
+	}
+
+	public function canAccessEntry(string $entryId, string $userId): bool {
+		return $this->find($entryId)['ownerId'] === $userId;
+	}
+}

hydra-gates/scripts/test-fixtures/gate-acceptance/authn-vs-authz/expect.conf

@@ -0,0 +1,20 @@
+# expect.conf — read by scripts/lib/test_gate_acceptance_matrix.sh
+#
+# Columns:
+#   gate <n> <log-basename|-> <planted-verdict> <clean-verdict> <subject-substring>
+#
+# Bundle: authn-vs-authz — gate-7 (no-admin-idor), .github#365.
+#
+# AUTHENTICATION IS NOT AUTHORISATION. The planted arm's three unguarded
+# methods differ from each other only in how they answer, and from the clean
+# arm's four only in whether a per-object guard follows the `no user -> 401`
+# preamble. Both arms KEEP that preamble, which is the point: the clean arm
+# proves the fix ignores it rather than punishing it.
+#
+# The subject is `method=preamble`, not the file path, and that is deliberate.
+# The file path is named by the bare unguarded method too, so a checker that
+# regressed to pre-#365 behaviour would still FAIL this arm (on `bare()`) and
+# still NAME the file — and would pass this bundle without ever having noticed
+# the 401 arm. Requiring the METHOD name is what makes this fixture a control
+# for `#365` specifically rather than a second copy of the auth-guards bundle.
+gate 7 hydra-gate-no-admin-idor.log FAIL PASS method=preamble

hydra-gates/scripts/test-fixtures/gate-acceptance/authn-vs-authz/planted/appinfo/info.xml

@@ -0,0 +1,13 @@
+<?xml version="1.0"?>
+<info xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
+	<id>authnfixture</id>
+	<name>Authn vs Authz Fixture</name>
+	<summary>Fixture app for the gate scope matrix. Not a real app.</summary>
+	<description>Exists so the gate suite can express push / full / diff scopes against a real git history.</description>
+	<version>1.0.0</version>
+	<licence>agpl</licence>
+	<author>Conduction</author>
+	<namespace>AuthnFixture</namespace>
+	<category>tools</category>
+	<dependencies><nextcloud min-version="30" max-version="32"/></dependencies>
+</info>

hydra-gates/scripts/test-fixtures/gate-acceptance/authn-vs-authz/planted/appinfo/routes.php

@@ -0,0 +1,14 @@
+<?php
+/**
+ * @license EUPL-1.2
+ * @copyright Conduction B.V.
+ */
+
+return [
+	'routes' => [
+		['name' => 'ledger#bare',                  'url' => '/api/entries/{entryId}/bare',      'verb' => 'GET'],
+		['name' => 'ledger#preamble',              'url' => '/api/entries/{entryId}/preamble',  'verb' => 'GET'],
+		['name' => 'ledger#preambleForbiddenCode', 'url' => '/api/entries/{entryId}/forbidden', 'verb' => 'GET'],
+		['name' => 'ledger#readAsOwner',           'url' => '/api/entries/{entryId}',           'verb' => 'GET'],
+	],
+];

hydra-gates/scripts/test-fixtures/gate-acceptance/authn-vs-authz/planted/lib/Controller/LedgerController.php

@@ -0,0 +1,110 @@
+<?php
+/**
+ * The PLANTED arm for `.github#365` — AUTHENTICATION IS NOT AUTHORISATION.
+ *
+ * THE THREE-ARM CONTROL THIS BUNDLE EXISTS FOR
+ * --------------------------------------------
+ * Arm 1 lives here as `bare()`   — an unguarded IDOR. gate-7 always found it.
+ * Arm 2 lives here as `preamble()` — the SAME data-access body, preceded only
+ *        by the house-style `no user -> 401` clause. gate-7 reported ZERO on
+ *        this, in all eighteen fleet apps, for as long as the gate has existed.
+ * Arm 3 lives in `clean/`        — the SAME body, the SAME 401 preamble, PLUS a
+ *        real per-object guard. It must stay silent.
+ *
+ * Arms 1 and 2 differ by the preamble alone; arms 2 and 3 differ by the guard
+ * alone. That is what makes the pair a control rather than two samples: the
+ * only thing that can explain a verdict difference is the thing that changed.
+ *
+ * `readAsOwner()` KEEPS a real guard, so this arm is not uniformly guilty — a
+ * checker that simply flagged every `#[NoAdminRequired]` method would score 4/4
+ * here and look correct. It must find exactly the three that guard nothing.
+ *
+ * @license EUPL-1.2
+ * @copyright Conduction B.V.
+ */
+
+namespace OCA\AuthnFixture\Controller;
+
+use OCA\AuthnFixture\Service\LedgerService;
+use OCP\AppFramework\Controller;
+use OCP\AppFramework\Http;
+use OCP\AppFramework\Http\Attribute\NoAdminRequired;
+use OCP\AppFramework\Http\JSONResponse;
+use OCP\IRequest;
+use OCP\IUserSession;
+
+class LedgerController extends Controller {
+
+	public function __construct(
+		string $appName,
+		IRequest $request,
+		private readonly LedgerService $ledger,
+		private readonly IUserSession $userSession,
+	) {
+		parent::__construct($appName, $request);
+	}
+
+	/**
+	 * ARM 1 — bare unguarded IDOR. The control that proves the gate is live on
+	 * this file and this shape. If this one stops firing, nothing else here
+	 * means anything.
+	 */
+	#[NoAdminRequired]
+	public function bare(string $entryId): JSONResponse {
+		$entry = $this->ledger->find($entryId);
+		return new JSONResponse($entry);
+	}
+
+	/**
+	 * ARM 2 — THE DEFECT. Byte-identical to `bare()` below the preamble.
+	 *
+	 * "Is anyone logged in?" is AUTHENTICATION. Under `#[NoAdminRequired]`
+	 * Nextcloud's middleware has already answered that question, so this clause
+	 * cannot even fail — and it still bought the method a PASS.
+	 */
+	#[NoAdminRequired]
+	public function preamble(string $entryId): JSONResponse {
+		$user = $this->userSession->getUser();
+		if ($user === null) {
+			return new JSONResponse(['error' => 'Unauthorized'], Http::STATUS_UNAUTHORIZED);
+		}
+		$entry = $this->ledger->find($entryId);
+		return new JSONResponse($entry);
+	}
+
+	/**
+	 * ARM 2b — the same authentication clause answering 403 instead of 401.
+	 *
+	 * `#365` as filed proposes deleting `401`/`UNAUTHORIZED` from the guard
+	 * regex. This method is what that repair leaves behind: one token of edit,
+	 * made by someone chasing a green gate, and the silence is back — bought by
+	 * making the response WORSE. It is here so that a future token-level
+	 * "simplification" of the fix goes red.
+	 */
+	#[NoAdminRequired]
+	public function preambleForbiddenCode(string $entryId): JSONResponse {
+		$user = $this->userSession->getUser();
+		if ($user === null) {
+			return new JSONResponse(['error' => 'Forbidden'], Http::STATUS_FORBIDDEN);
+		}
+		$entry = $this->ledger->find($entryId);
+		return new JSONResponse($entry);
+	}
+
+	/**
+	 * NOT planted — a real per-object ownership check. Keeps the planted arm
+	 * from being uniformly guilty.
+	 */
+	#[NoAdminRequired]
+	public function readAsOwner(string $entryId): JSONResponse {
+		$user = $this->userSession->getUser();
+		if ($user === null) {
+			return new JSONResponse(['error' => 'Unauthorized'], Http::STATUS_UNAUTHORIZED);
+		}
+		$entry = $this->ledger->find($entryId);
+		if ($entry['ownerId'] !== $user->getUID()) {
+			return new JSONResponse(['error' => 'Forbidden'], Http::STATUS_FORBIDDEN);
+		}
+		return new JSONResponse($entry);
+	}
+}