feat(adr-compliance): close gaps against 17 org ADRs#20
Open
rubenvdlinde wants to merge 8 commits into
Open
Conversation
…DR mandate Audits the template against the 17 org-wide ADRs (hydra/openspec/architecture/adr-*.md) and closes every demonstrable gap so builders who copy from this template get the right patterns by default. Full audit table: docs/adr-audit.md. Key additions / fixes: - ADR-001 seed data: add 3 realistic @self-enveloped objects to app_template_register.json so installs are not empty. - ADR-002 CORS: register OPTIONS route for public /api/health. - ADR-003 @SPEC tags: add file-level + method-level @SPEC tags to every class in lib/ and to the test class (was only on Application.php). - ADR-004 frontend: - switch settings store from raw fetch() to @nextcloud/axios (CSRF). - route all @nextcloud/vue imports through @conduction/nextcloud-vue (App.vue, MainMenu, Settings, UserSettings, ItemDetail). - replace window.confirm() in ItemDetail with NcDialog. - fix deep link URL template from hash (#/examples/{uuid}) to path format (/examples/{uuid}) per history-mode requirement. - wrap every await store.action() in try/catch with user feedback. - ADR-005 security: SettingsController now try/catches every action and returns a static "Operation failed" message on exception; logs the real error server-side. SettingsService::loadConfiguration no longer leaks $e->getMessage() to clients. - ADR-006 observability: add HealthController (public JSON, verifies OpenRegister connectivity) and MetricsController (admin-only Prometheus text, includes {app}_health_status + {app}_info). Routes already declared them; controllers were missing (500 on call). - ADR-008 testing: add error-path unit test for SettingsController. All PHP files pass php -l. Follow-ups (documented in audit): @Licence vs @license sweep, private readonly DI params, SettingsServiceTest.
2 tasks
…ompliance PHP files carry licence + copyright via PHPDoc @tags in the main docblock (ADR-014). Duplicating that as SPDX-FileCopyrightText / SPDX-License-Identifier inside every docblock was redundant — the @copyright / @license / @author tags already say the same thing. - Remove the SPDX-FileCopyrightText + SPDX-License-Identifier lines from every PHP file header. @copyright / @license stay. - Add REUSE.toml at repo root so `reuse lint` still passes — it covers **/*.php, non-PHP sources, and configs/data in a handful of rules. New apps scaffolded from this template inherit the same pattern. This aligns the template with the hydra spdx-headers gate rewrite (ConductionNL/hydra#66) which now enforces @license + @copyright PHPDoc tags, and the ADR-014 update that mandates @tag-only on PHP.
Renames the template's demo schema from 'example' to 'article' and aligns its properties to schema.org/Article (name, description, identifier, dateCreated, author) per ADR-011. Updates the deep-link listener to point at the new 'article' schema slug and renames seed objects to match. The template is the canonical reference apps copy from, so it must demonstrate the ADR-011 rule (use schema.org vocabulary instead of ad-hoc domain types whenever a standard type fits).
Adds PHPUnit coverage for SettingsService, mirroring the shape of the existing SettingsControllerTest. All 6 dependencies (IAppConfig, IAppManager, ContainerInterface, IGroupManager, IUserSession, LoggerInterface) are mocked and every method is exercised: - isOpenRegisterAvailable() — both installed and not-installed branches - getSettings() — isAdmin=true (admin user), isAdmin=false (non-admin), and the no-user/guest branch that must skip groupManager->isAdmin() - updateSettings() — persists known CONFIG_KEYS via IAppConfig, ignores unknown keys - loadConfiguration() — success path (force=true), OpenRegister-missing short-circuit, and the Throwable-caught branch per ADR-005 (logs the real exception server-side, returns a static generic message) 10 test methods / 32 assertions — well above the ADR-008 floor of >= 3 methods per logical unit. Also extends tests/bootstrap-unit.php to autoload NC core classes (OCP\IAppConfig, OCP\IRequest, etc.) without triggering OC::init(). This allows the existing SettingsControllerTest to run too (previously it failed with "Class OCP\\IRequest does not exist") so both test classes now execute: 15 tests, 49 assertions, all passing.
Demonstrates the ADR-005 mandate that #[NoAdminRequired] mutation
endpoints MUST carry an explicit backend auth check. The template had
no domain mutation endpoint to demo this on, so the audit listed it as
'partial' — this commit adds a minimal, copy-pasteable example.
- lib/Service/ItemService::delete($id, $userId) enforces an admin-OR-owner
check via IGroupManager::isAdmin() and the OpenRegister @self.owner
metadata, returning a STATUS_* enum the controller maps to HTTP codes.
- lib/Controller/ItemController::destroy($id) declares #[NoAdminRequired],
derives the acting UID from IUserSession (never a request param), and
maps STATUS_DELETED→204, STATUS_FORBIDDEN→403, STATUS_NOT_FOUND→404,
STATUS_UNAVAILABLE→503, Throwable→generic 500 with server-side log.
- appinfo/routes.php registers DELETE /api/items/{id} before the SPA
catch-all, per existing route-ordering comments.
- Follows OWASP A01: returns 404 (not 403) for non-existent IDs so
response codes don't leak existence.
- ItemServiceTest (6 methods) and ItemControllerTest (5 methods) lock
down all auth branches + the HTTP mapping. Full unit suite now runs
26 tests / 78 assertions, all passing.
Audits the whole repo for PHPDoc @Licence (UK) and confirms every PHP file already uses @license (US) — which is the PHPCS standard. The stale ADR-014 audit entry wrongly flagged this as a partial finding and as a follow-up; this commit resolves both. Also rolls up the audit with the three other ADR follow-ups now resolved this branch: - ADR-005: DELETE /api/items/{id} + ItemService auth → ✅ - ADR-008: SettingsServiceTest + ItemServiceTest + ItemControllerTest → ✅ - ADR-011: schema.org/Article alignment → ✅ - ADR-002 pagination: reclassified as N/A (OpenRegister facilitates this automatically via useListView — apps don't need list endpoints) `<licence>agpl</licence>` in appinfo/info.xml is intentionally kept — it's a Nextcloud-schema XML element, not PHPDoc.
Fixes lint violations surfaced while running composer phpcs + phpmd on this branch: - lib/Service/ItemService.php — inline IFs in extractOwner() rewritten as explicit if-blocks, extractOwner() call updated to use named parameter (ADR custom sniff: internal calls must use named params), docblock alignment tweak. - lib/Controller/HealthController.php — pre-existing inline IFs in index() replaced with a guarded mutation pattern (sets degraded default, overrides on healthy) to also clear the PHPMD ElseExpression. - lib/Controller/MetricsController.php — pre-existing equals-sign alignment between $prefix and $healthy. `composer phpcs` + `composer phpmd` (minus a pre-existing BooleanArgumentFlag warning on SettingsService::loadConfiguration($force) which would require an API change) + `composer psalm` + `composer phpstan` are now clean. `composer test:unit` still reports 26 / 26 passing.
Contributor
Author
ADR follow-ups — finished (4 commits + 1 quality sweep)Picked up the prior agent's in-progress schema.org work, finished it, and knocked out the three other follow-ups from Commits on this branch (on top of
|
| SHA | Subject | Notes |
|---|---|---|
95433f9 |
feat(adr-011): align example schema to schema.org/Article | Finished the prior agent's work: schema renamed example → article, properties aligned to schema.org/Article (name, description, identifier, dateCreated, author) with x-schema-org annotation. Deep-link listener + seed objects updated. |
e185a8e |
feat(adr-008): add SettingsServiceTest with all branches | 10 test methods / 32 assertions covering isOpenRegisterAvailable() (installed/missing), getSettings() (admin/non-admin/no-user), updateSettings() (persist/ignore-unknown), loadConfiguration() (success/missing/Throwable-caught per ADR-005). Also taught tests/bootstrap-unit.php to autoload NC core classes without OC::init() so the existing SettingsControllerTest starts passing too. |
0ba8184 |
feat(adr-005): per-object auth demo on DELETE /api/items/{id} | New ItemController::destroy() (#[NoAdminRequired]) + ItemService::delete() with admin-OR-owner check via IGroupManager::isAdmin() and the OpenRegister @self.owner metadata. Returns 204/403/404/503 with generic messages per ADR-005. Route registered before SPA catch-all. ItemServiceTest (6 methods) + ItemControllerTest (5 methods) lock down every auth branch. |
34c57a6 |
fix(adr-014): @Licence → @license PHPDoc spelling | Audited repo — no PHP files actually use @licence; all use @license. Rolled up docs/adr-audit.md to mark ADR-005/008/011/014 resolved and reclassified pagination as N/A. |
7ee06aa |
chore(quality): fix PHPCS inline IFs, named-param, alignment | Pre-existing lint violations on HealthController + MetricsController plus violations on the new ItemService, per the team rule of fixing pre-existing quality issues when encountered. |
Dropped from scope
- Pagination demo (ADR-002) — OpenRegister facilitates
_page/_limit/total/pagesautomatically throughcreateObjectStore+useListView. The template doesn't need its own list endpoint.docs/adr-audit.mdrow reclassified N/A.
Verify
php -lclean on every touched PHP filecomposer phpcs12/12 files cleancomposer psalm— no errorscomposer phpstan— no errorscomposer phpmd— clean (one pre-existingBooleanArgumentFlagwarning onSettingsService::loadConfiguration(\$force)left alone; would require an API break to fix)composer test:unit(via the Nextcloud container) — 26 tests, 78 assertions, all passing (1 placeholder + 4 SettingsController + 10 SettingsService + 5 ItemController + 6 ItemService)
Audit roll-up
docs/adr-audit.md summary updated: 49 demonstrated, 17 added/fixed this branch, 2 partial, 7 N/A.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #19. Audits the template against every MUST/SHALL/REQUIRED rule in the 17 org-wide ADRs (
hydra/openspec/architecture/adr-*.md) and adds minimal concrete examples for every demonstrable gap, so builders who copy from this template get the right patterns by default.Full audit table:
docs/adr-audit.md— 56 rules checked, 45 already demonstrated, 13 added/fixed, 5 partial with documented exceptions, 6 N/A.What's added/fixed
Seed data (ADR-001): 3 realistic
@self-enveloped objects inlib/Settings/app_template_register.jsonso installs are not empty.Observability (ADR-006): new
HealthController(public JSON, verifies OpenRegister) +MetricsController(admin Prometheus text with{app}_health_status+{app}_info). Routes declared them already — controllers were missing so/api/metricsand/api/healthreturned 500.API + security (ADR-002 / ADR-005):
/api/health.SettingsControllernowtry/catchesevery action, returns staticOperation failedmessage, logs real error server-side.SettingsService::loadConfigurationno longer leaks$e->getMessage()to clients.Frontend (ADR-004 / ADR-015):
fetch()→@nextcloud/axios(CSRF).App.vue,MainMenu,Settings,UserSettings,ItemDetailno longer import directly from@nextcloud/vue— all go through@conduction/nextcloud-vue.ItemDetailreplaceswindow.confirm()withNcDialog.#/examples/{uuid}) to path format (/examples/{uuid}) per Vue router history mode.await store.action()now wrapped intry/catchwith user-facing feedback.t(…)→this.t(…)inSettings.vue.Spec traceability (ADR-003): every class and public method in
lib/now has@spec openspec/changes/example-change/tasks.md#task-N(was only onApplication.php). Explicitly illustrative — copies get replaced with real paths.Testing (ADR-008): added error-path unit test
testIndexReturnsGenericErrorOnServiceException— asserts 500 + generic message + no leak of exception text.Base branch
Stacked on top of
fix/header-consistency-info-email(PR #19) so both sets of fixes land coherently. Retarget base todevelopmentonce #19 merges, or merge #19 first and this will fast-forward.Test plan
php -lclean on all 10 modified/added PHP filescomposer check:strictclean locallycurl /api/healthreturns JSON,curl /api/metricsreturns Prometheus text, and the 3 seed objects appear in the list view