fix(auth): guard the aggregate user-groups route and align 10 auth annotations (#456)

TWO SEPARATE THINGS, both authorisation.

1. A PRIVILEGE BYPASS BY SIBLING ROUTE.

Four routes each return one slice of the user-groups configuration and
each carries an explicit `isAdmin() === false -> 403` guard:

    GET /api/settings/user-groups/generic
    GET /api/settings/user-groups/organization-admin
    GET /api/settings/user-groups/super-user
    GET /api/settings/user-groups/all

A fifth returns all four at once and checked only that the caller was
logged in:

    GET /api/user-groups/config   -> SettingsController::getUserGroupsConfig()

SettingsService::getUserGroupsConfig() is literally the union of the four
guarded getters, so any authenticated user could read through it exactly
what the four dedicated routes refuse them — including `allGroups`, the
full group list of the instance.

Which side to fix was NOT obvious from the finding. The four guarded
getters have ZERO callers in src/: every consumer, including
UserGroupsConfiguration.vue via the settings store, hits
/api/user-groups/config. Read as "dead code", the tempting move is to
delete the four guarded endpoints — which would have left the UNGUARDED
aggregate as the only surviving reader. Tracing the sibling seam gives
the opposite answer: the four are the correct implementation, and the
live route is the one missing the guard. So the guard was added there,
and to updateUserGroupsConfig() alongside it, which was admin-only
through the ABSENCE of @NoAdminRequired — middleware alone, with no
in-body backstop, unlike every one of its siblings.

Neither gate could see this. gate-7 (no-admin-idor) counts
`getUser() === null` as an auth guard and passes. gate-9 (semantic-auth)
compares the annotation against the body and finds no mismatch, because
a @NoAdminRequired method with no admin check is self-consistent. The
defect lives in the relationship between two endpoints, which neither
gate models.

2. TEN ANNOTATION/BODY MISMATCHES (gate-9, full-tree).

Eight SettingsController methods declared @NoAdminRequired while their
bodies return 403 to non-admins. Their own sibling setters
(setGenericUserGroups, setSuperUserGroups, ...) already omit the
annotation, so the file's intended pattern was unambiguous: drop it, and
let the middleware refuse before the controller runs. Same effective
policy, enforced one layer earlier.

Two more — AanbodController::getAanbod and
AangebodenGebruikController::getGebruiksWhereAfnemer — carried
@publicpage while their specs (REQ-009, REQ-004) require an
authenticated caller and their bodies enforce it. These are NOT the
self-authenticating webhook/portal shape that makes gate-9's advice
dangerous: nothing authenticates the caller from the request, the body
tests the SESSION, so @publicpage only ever admitted callers the body
would then reject. Replaced with @NoAdminRequired; the in-body guard
stays as deny-before-grant.

Full-tree gate-9: 10 findings -> PASS.

EVIDENCE. SettingsControllerUserGroupsConfigAuthTest, five arms:
non-admin refused (403, payload absent, service never consulted), admin
still served the config itself and not merely a 200, anonymous still 401
rather than promoted to 403, and both arms again for the write half.
Reverting SettingsController.php to origin/development turns exactly the
two non-admin arms red — "Failed asserting that 200 is identical to 403"
— i.e. a non-admin received the configuration.

The deny-before-grant check uses a call counter, not
expects($this->never()): the controller wraps its body in
catch (\Exception), which swallows a PHPUnit expectation failure into a
500 and reports the leak as an unrelated server error. Measured — the
first version of this test failed with "500 is identical to 403".

phpcs: lib/ is 0 errors / 87 warnings, unchanged. Unit suite: 486 green.

Author: rubenvdlinde

lib/Controller/AanbodController.php

@@ -80,8 +80,16 @@ public function __construct(
      *
      * @return JSONResponse JSON response with aanbod objects array
      *
+     * REQ-009 requires an authenticated caller, and the body enforces it.
+     * The PublicPage annotation told Nextcloud's middleware to admit
+     * anonymous requests anyway, so declared and enforced contract disagreed:
+     * every anonymous call was admitted, routed, and only then rejected by
+     * the controller. @NoAdminRequired is the annotation that matches —
+     * any logged-in user, rejected at the middleware. The in-body guard
+     * stays as deny-before-grant (REQ-001).
+     *
      * @NoCSRFRequired
-     * @PublicPage
+     * @NoAdminRequired
      *
      * @spec openspec/specs/aanbod-listings/spec.md
      * @spec openspec/specs/vendor-visibility-rbac/spec.md#requirement-the-aanbod-listing-endpoint-must-require-authentication-explicitly-not-implicitly-req-009

lib/Controller/AangebodenGebruikController.php

@@ -87,10 +87,18 @@ public function __construct(
      *
      * @return JSONResponse JSON response with gebruiks array where org is afnemer
      *
+     * REQ-004 requires an authenticated caller, and the body enforces it.
+     * The PublicPage annotation told Nextcloud's middleware to admit
+     * anonymous requests anyway, so declared and enforced contract disagreed:
+     * every anonymous call was admitted, routed, and only then rejected by
+     * the controller. @NoAdminRequired is the annotation that matches —
+     * any logged-in user, rejected at the middleware. The in-body guard
+     * stays as deny-before-grant (REQ-001).
+     *
      * @NoCSRFRequired
-     * @PublicPage
-     * @spec           openspec/specs/aangeboden-gebruik-api/spec.md
-     * @spec           openspec/specs/vendor-visibility-rbac/spec.md#requirement-the-offered-usage-afnemer-endpoint-must-require-authentication-explicitly-not-implicitly-req-004
+     * @NoAdminRequired
+     * @spec            openspec/specs/aangeboden-gebruik-api/spec.md
+     * @spec            openspec/specs/vendor-visibility-rbac/spec.md#requirement-the-offered-usage-afnemer-endpoint-must-require-authentication-explicitly-not-implicitly-req-004
      */
     public function getGebruiksWhereAfnemer(): JSONResponse
     {

lib/Controller/SettingsController.php

@@ -2290,7 +2290,10 @@ public function getEmailTemplateVariables(string $templateName): JSONResponse
     /**
      * Get generic user groups
      *
-     * @NoAdminRequired
+     * Admin-only: the body rejects a non-admin with 403, so the endpoint
+     * must not declare @NoAdminRequired. Its sibling setGenericUserGroups()
+     * already omits it.
+     *
      * @NoCSRFRequired
      *
      * @return JSONResponse Generic user groups
@@ -2387,7 +2390,10 @@ public function setGenericUserGroups(): JSONResponse
     /**
      * Get organization admin groups
      *
-     * @NoAdminRequired
+     * Admin-only: the body rejects a non-admin with 403, so the endpoint
+     * must not declare @NoAdminRequired. Its sibling
+     * setOrganizationAdminGroups() already omits it.
+     *
      * @NoCSRFRequired
      *
      * @return JSONResponse Organization admin groups
@@ -2484,7 +2490,10 @@ public function setOrganizationAdminGroups(): JSONResponse
     /**
      * Get super user groups
      *
-     * @NoAdminRequired
+     * Admin-only: the body rejects a non-admin with 403, so the endpoint
+     * must not declare @NoAdminRequired. Its sibling setSuperUserGroups()
+     * already omits it.
+     *
      * @NoCSRFRequired
      *
      * @return JSONResponse Super user groups
@@ -2581,7 +2590,10 @@ public function setSuperUserGroups(): JSONResponse
     /**
      * Get all user groups
      *
-     * @NoAdminRequired
+     * Admin-only: the body rejects a non-admin with 403, and the payload is
+     * the full group list of the instance — an enumeration surface. The
+     * endpoint must not declare @NoAdminRequired.
+     *
      * @NoCSRFRequired
      *
      * @return JSONResponse All user groups
@@ -2631,7 +2643,9 @@ public function getAllGroups(): JSONResponse
     /**
      * Clear ArchiMate import status
      *
-     * @NoAdminRequired
+     * Admin-only: the body rejects a non-admin with 403, and importArchiMate()
+     * — the operation whose status this clears — is already admin-only.
+     *
      * @NoCSRFRequired
      *
      * @return JSONResponse Clear result
@@ -2682,9 +2696,11 @@ public function clearArchiMateImportStatus(): JSONResponse
      *
      * @deprecated Use cancelArchiMateImport() instead.
      *
-     * @NoAdminRequired
+     * Admin-only: the body rejects a non-admin with 403, and importArchiMate()
+     * — the process this kills — is already admin-only.
+     *
      * @NoCSRFRequired
-     * @spec            openspec/specs/settings-admin-controller/spec.md
+     * @spec           openspec/specs/settings-admin-controller/spec.md
      */
     public function killArchiMateImport(): JSONResponse
     {
@@ -2728,7 +2744,9 @@ public function killArchiMateImport(): JSONResponse
      * Cancel a running ArchiMate import
      * This combines force clearing and process killing for complete cancellation
      *
-     * @NoAdminRequired
+     * Admin-only: the body rejects a non-admin with 403, and importArchiMate()
+     * — the process this cancels — is already admin-only.
+     *
      * @NoCSRFRequired
      *
      * @return JSONResponse Cancellation result
@@ -2773,7 +2791,8 @@ public function cancelArchiMateImport(): JSONResponse
     /**
      * Clear ArchiMate export status
      *
-     * @NoAdminRequired
+     * Admin-only: the body rejects a non-admin with 403.
+     *
      * @NoCSRFRequired
      *
      * @return JSONResponse Clear result
@@ -3322,18 +3341,30 @@ public function getObjectsStatistics(): JSONResponse
     /**
      * Get user groups configuration only
      *
-     * @NoAdminRequired
+     * This endpoint returns the union of getGenericUserGroups(),
+     * getOrganizationAdminGroups(), getSuperUserGroups() and getAllGroups().
+     * Each of those is exposed on its own route behind an explicit
+     * `isAdmin() === false -> 403` guard, so this aggregate MUST carry the
+     * same guard: without it any authenticated user reads through it the
+     * data the four dedicated routes refuse them, and the guards on those
+     * routes protect nothing.
+     *
      * @NoCSRFRequired
      *
      * @return JSONResponse User groups configuration
      * @spec   openspec/specs/settings-admin-controller/spec.md
      */
     public function getUserGroupsConfig(): JSONResponse
     {
-        if ($this->userSession->getUser() === null) {
+        $currentUser = $this->userSession->getUser();
+        if ($currentUser === null) {
             return new JSONResponse(['message' => 'Not authenticated'], Http::STATUS_UNAUTHORIZED);
         }
 
+        if ($this->groupManager->isAdmin($currentUser->getUID()) === false) {
+            return new JSONResponse(['message' => 'Admin privileges required'], Http::STATUS_FORBIDDEN);
+        }
+
         try {
             $config = $this->settingsService->getUserGroupsConfig();
 
@@ -3365,6 +3396,15 @@ public function getUserGroupsConfig(): JSONResponse
      */
     public function updateUserGroupsConfig(): JSONResponse
     {
+        $currentUser = $this->userSession->getUser();
+        if ($currentUser === null) {
+            return new JSONResponse(['message' => 'Not authenticated'], Http::STATUS_UNAUTHORIZED);
+        }
+
+        if ($this->groupManager->isAdmin($currentUser->getUID()) === false) {
+            return new JSONResponse(['message' => 'Admin privileges required'], Http::STATUS_FORBIDDEN);
+        }
+
         try {
             $data   = $this->request->getParams();
             $result = $this->settingsService->updateUserGroupsConfig($data);