Skip to content

feat(manifest): inject runtime.user.* context from current-user-schema calculations#1481

Open
rubenvdlinde wants to merge 1 commit into
developmentfrom
feat/scholiq-deps/manifest-user-context
Open

feat(manifest): inject runtime.user.* context from current-user-schema calculations#1481
rubenvdlinde wants to merge 1 commit into
developmentfrom
feat/scholiq-deps/manifest-user-context

Conversation

@rubenvdlinde
Copy link
Copy Markdown
Contributor

Summary

  • Adds ManifestService::getEnrichedManifest(array $manifest): array — OR-side logic that reads the calling app's currentUserSchema declaration, finds the user's profile object (filtered by ncUserId), evaluates all non-materialised x-openregister-calculations.* expressions, and injects the result as runtime.user in the returned manifest.
  • Adds ManifestController at GET /api/manifest/{appId} (PublicPage, NoCSRFRequired) — reads the host app's src/manifest.json from disk via IAppManager::getAppPath, delegates enrichment to ManifestService, returns the result as JSON.
  • Registers the route in appinfo/routes.php.

Four runtime.user scenarios:

  1. No currentUserSchema in manifest → returned unchanged (backwards-compatible).
  2. Anonymous request → runtime.user = null (nc-vue filters to public pages).
  3. Logged-in user, no profile object → runtime.user = { id, roles: ["learner"] }.
  4. Logged-in user, profile found → full context with evaluated calculations.

Closes scholiq deps #8 — replaces cancelled nc-vue roleAware page resolver (ADR-024 §4).

Test plan

  • ManifestServiceTest — 6 unit tests (unchanged manifest, empty slug, anonymous, no profile, full context with calc injection, graceful calc failure)
  • ManifestControllerTest — 4 unit tests (invalid app ID → 400, unknown app → 404, valid app + enrichment → 200, service exception → 500)
  • CI code-quality workflow passes on branch
  • Manual: GET /index.php/apps/openregister/api/manifest/decidesk returns manifest with or without runtime.user depending on auth state

…a calculations

Adds ManifestService + ManifestController (GET /api/manifest/{appId}) that:
- Returns host-app manifest unchanged when no currentUserSchema declared.
- Injects runtime.user=null for anonymous requests.
- Injects runtime.user={id,roles:["learner"]} fallback when no profile object found.
- Injects full runtime.user context (profile payload + non-materialised calculations)
  when the user has a profile object matching ncUserId in the declared schema.

Closes scholiq deps #8 — replaces cancelled nc-vue roleAware page resolver.

4 PHPUnit tests cover all four runtime.user scenarios.
@github-actions
Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ ce32e29

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
composer ✅ 153/153
npm ✅ 598/598
PHPUnit
Newman ⏭️
Playwright ⏭️

Quality workflow — 2026-05-11 20:20 UTC

Download the full PDF report from the workflow artifacts.

try {
$enriched = $this->manifestService->getEnrichedManifest(manifest: $manifest);
return new JSONResponse($enriched);
} catch (Throwable $e) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[BLOCKER] Missing Cache-Control: no-store on user-personalised PublicPage endpoint

#[PublicPage] endpoint returns user-specific runtime.user data yet emits no Cache-Control: no-store header. A reverse proxy, CDN, or shared-machine browser cache can serve user A's personalised manifest to user B. Add $response->addHeader('Cache-Control', 'no-store, no-cache, must-revalidate') before returning the enriched manifest.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[BLOCKER] Full raw profile object dumped into runtime.user without field allowlisting

buildUserContext does array_merge($data, ['id' => $userId]) where $data = $profile->getObject(). This dumps every field stored in the profile — potentially passwords, BSN, email, internal notes — into the public-facing manifest response without any allowlist or field filter. Define which fields are safe to expose and only include those.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[BLOCKER] Profile lookup uses ncUserId as a user-controlled JSON filter — IDOR/spoofing risk

resolveUserProfile filters objects via 'ncUserId' => $userId passed to objectService->findAll. ncUserId is a field in the stored JSON payload, not a system-enforced DB column. A user who can edit their own profile object could set ncUserId to another user's UID, causing that other user's runtime.user to include the attacker's data. The lookup must filter on the system-owned owner column, not a user-settable payload field.

* @version GIT: <git-id>
*
* @link https://OpenRegister.app
*
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[BLOCKER] SPDX machine-readable header missing from ManifestController.php

ManifestController.php uses only the PHPDoc @license tag. Requires two SPDX comment lines: // SPDX-License-Identifier: EUPL-1.2 and // SPDX-FileCopyrightText: 2026 Conduction B.V.. The hydra-gate-spdx gate will block this.

*/
private function buildUserContext(string $userId, ObjectEntity $profile, string $schemaSlug): array
{
$data = $profile->getObject() ?? [];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[BLOCKER] SPDX machine-readable header missing from ManifestService.php

ManifestService.php uses only the PHPDoc @license tag, not the required SPDX-License-Identifier / SPDX-FileCopyrightText comment lines required by hydra-gate-spdx. Both new lib/ files must be fixed before CI can pass.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[BLOCKER] Gate 7: catch (Throwable) silently swallows schema-lookup errors in getCalculations()

getCalculations() uses catch (Throwable) { return null; } with no logging. If the schema mapper throws due to DB error or schema misconfiguration, the calculation context is silently dropped and the manifest returns without calculations — indistinguishable from 'no calculations defined'. Log at warning level before returning null.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[CONCERN] SchemaMapper queried twice per request — resolveUserProfile + getCalculations

resolveUserProfile calls schemaMapper->findBySlug() at line 321, and buildUserContext calls it again via getCalculations at line 445 — two DB round trips for the same slug per request. Pass the already-resolved Schema object through instead of re-fetching.

Copy link
Copy Markdown
Contributor

@WilcoLouwerse WilcoLouwerse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

(Inline comments already posted above — this is the summary verdict.)

🔴 Blockers (6)

  • Missing Cache-Control: no-store on user-personalised PublicPage endpoint (lib/Controller/ManifestController.php:109)
  • Full raw profile object dumped into runtime.user without field allowlisting (lib/Service/ManifestService.php:379)
  • Profile lookup uses ncUserId as a user-controlled JSON filter — IDOR/spoofing risk (lib/Service/ManifestService.php:338)
  • SPDX machine-readable header missing from ManifestController.php (lib/Controller/ManifestController.php:20)
  • SPDX machine-readable header missing from ManifestService.php (lib/Service/ManifestService.php:190)
  • Gate 7: catch (Throwable) silently swallows schema-lookup errors in getCalculations() (lib/Service/ManifestService.php:446)

🟡 Concerns (5)

  • SchemaMapper queried twice per request — resolveUserProfile + getCalculations (lib/Service/ManifestService.php:321)
  • objectService->findAll called with _rbac=false and _multitenancy=false — bypasses all access controls (lib/Service/ManifestService.php:338)
  • No cross-user isolation test — two simultaneous users must get different runtime.user (tests/Unit/Service/ManifestServiceTest.php:659)
  • Manifest-defined runtime.user silently overwritten — no precedence strategy documented (lib/Service/ManifestService.php:272)
  • loadBundledManifest reads filesystem paths via IAppManager::getAppPath — path traversal surface (lib/Controller/ManifestController.php:158)

🟢 Minor (1)

  • @self added to $data after $context is built — @self appears in runtime.user output unexpectedly (lib/Service/ManifestService.php:394)

Reviewed by WilcoLouwerse via automated batch review.

Copy link
Copy Markdown
Contributor

@WilcoLouwerse WilcoLouwerse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

🔴 Blockers (6)

🟡 Concerns (5)

  • SchemaMapper queried twice per request — resolveUserProfile + getCalculationslib/Service/ManifestService.php:321
  • objectService->findAll called with _rbac=false and _multitenancy=false — bypasses all access controls (lib/Service/ManifestService.php:338)
    Both RBAC and multi-tenancy checks are explicitly disabled when fetching the user's profile object. In a multi-tenant deployment schema slugs can collide across tenants, meaning a user in org A could be matched against a profile object in org B. Enable multi-tenancy filtering or add an explicit organisation-scoped filter.
  • No cross-user isolation test — two simultaneous users must get different runtime.user (tests/Unit/Service/ManifestServiceTest.php:659)
    Test suite covers four separate users in isolated methods but has no test that calls getEnrichedManifest with two different IUserSession configurations back-to-back, asserting that runtime.user.id differs. Such a test would catch any static/class-level caching or session-state bleed.
  • Manifest-defined runtime.user silently overwritten — no precedence strategy documented (lib/Service/ManifestService.php:272)
    If the calling app's manifest.json already contains a runtime.user key, the service unconditionally overwrites it at lines 281, 291, and 299 with no warning or merge. This shadowing is not documented and could silently drop manifest-defined values the calling app relies on.
  • loadBundledManifest reads filesystem paths via IAppManager::getAppPath — path traversal surface (lib/Controller/ManifestController.php:158)
    getAppPath($appId) is called after a regex check on $appId, but the constructed path $appPath.'/src/manifest.json' is never canonicalised. While the appId regex mitigates direct traversal, add an assertion that the resolved manifest path is still within the expected app directory.

🟢 Minor (1)

  • @self added to $data after $context is built — @self appears in runtime.user output unexpectedly (lib/Service/ManifestService.php:394)
    $context = array_merge($data, ['id' => $userId]) (line 382) is built before $data['@self'] = [...] (line 394). The @self object is silently merged into $context and surfaces in runtime.user. Downstream consumers may not expect a raw @self object there. Exclude it from context output or document the intent.

Reviewed by WilcoLouwerse via automated batch review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants