feat: Time Tracking MVP — CRUD backend + TimeLog frontend#22
feat: Time Tracking MVP — CRUD backend + TimeLog frontend#22rubenvdlinde wants to merge 8 commits intodevelopmentfrom
Conversation
Implements the backend for time tracking MVP: - TimeEntryService: CRUD operations via OpenRegister ObjectService - TimeEntryController: REST endpoints POST/GET/DELETE /api/time-entries - Routes registered in appinfo/routes.php - Validation: duration > 0, date required, taskId must exist - Owner-only delete enforcement - Authentication check (403 for unauthenticated users) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- TimeEntryControllerTest: 10 test methods covering auth, CRUD, validation - TimeEntryServiceTest: 6 test methods covering user session, validation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- TimeLog.vue: displays time entries for a task with log form and delete - TaskDetail.vue: task detail view with breadcrumb, meta, and TimeLog - timeEntries.js: Pinia store for time entry CRUD via OpenRegister - Router: added /tasks/:taskId route for task detail Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix docblock parameter order in TimeEntryController - Use named parameters for all internal method calls in TimeEntryService - Replace inline ternary with if/return block in findObject() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
No description provided. |
[WARNING] IDOR —
|
Quality Report
Summary
PHP Quality
Vue Quality
Security
License Compliance
composer dependencies (100 total)
npm dependencies (215 total)
PHPUnit Tests
Code coverage: 0% (0 / 3 statements) Integration Tests (Newman)Newman integration tests were not enabled for this run. E2E Tests (Playwright)Playwright E2E tests were not enabled for this run. Generated automatically by the Quality workflow.
|
[WARNING] Insufficient input validation —
|
[SUGGESTION] Frontend store bypasses the new TimeEntryControllerFiles: The // timeEntries.js:87-89
const entry = await objectStore.saveObject(TIME_ENTRY_SCHEMA, {
...data,
user: uid, // set client-side
})Consequences:
Recommended fix: Have
|
{
"tool": "hydra-security-reviewer",
"reviewer": "Clyde Barcode",
"pr": "https://github.com/ConductionNL/planix/pull/22",
"reviewed_at": "2026-04-05T00:00:00Z",
"verdict": "CHANGES_REQUESTED",
"sast": {
"semgrep": { "status": "clean", "findings": 0 },
"gitleaks": { "status": "clean", "findings": 0 },
"trivy": { "status": "not_run" }
},
"findings": [
{
"id": "SEC-001",
"severity": "WARNING",
"title": "IDOR — listTimeEntries does not enforce task/project membership",
"file": "lib/Service/TimeEntryService.php",
"line": 227,
"description": "Any authenticated user can retrieve all time entries for any task UUID, including entries and descriptions belonging to users in projects they are not members of.",
"owasp": "A01:2021 – Broken Access Control"
},
{
"id": "SEC-002",
"severity": "WARNING",
"title": "Missing format validation on date field and UUID parameters",
"file": "lib/Service/TimeEntryService.php",
"line": 278,
"description": "The date field is only checked for emptiness; arbitrary strings are persisted. Route parameter id in destroy() and taskId in createTimeEntry() are not validated as UUID-formatted strings before being passed to OpenRegister.",
"owasp": "A03:2021 – Injection"
},
{
"id": "SEC-003",
"severity": "SUGGESTION",
"title": "Frontend store bypasses TimeEntryController — client sets user field",
"file": "src/store/timeEntries.js",
"line": 87,
"description": "The Pinia store calls OpenRegister directly, bypassing task-existence checks and allowing the client to supply an arbitrary user value. The three controller endpoints are dead code for the bundled frontend.",
"owasp": "A01:2021 – Broken Access Control"
}
],
"summary": "Two WARNING-level findings must be addressed before merge: an IDOR that leaks time entries across project boundaries, and insufficient input validation on the date field and UUID parameters. One SUGGESTION is noted regarding the frontend bypassing the controller layer."
} |
🔍 Hydra Code Review — Juan Claude van DammePR: feat: Time Tracking MVP — CRUD backend + TimeLog frontend ✅ Quality Gate Results
The PHPCS failure in Individual findings follow in separate comments below. |
[CRITICAL]
|
[CRITICAL]
|
[WARNING]
|
[WARNING]
|
[WARNING]
|
[WARNING]
|
[SUGGESTION]
|
[SUGGESTION]
|
Hydra Code Review — Final Verdict{
"reviewer": "Juan Claude van Damme (Hydra Code Review Agent)",
"pr": "ConductionNL/planix#22",
"reviewed_at": "2026-04-05",
"verdict": "CHANGES_REQUESTED",
"quality_gates": {
"composer_cs_check_lib": "PASS",
"composer_cs_check_tests": "FAIL",
"phpunit": "ENVIRONMENT_UNAVAILABLE",
"eslint": "ENVIRONMENT_UNAVAILABLE"
},
"findings": [
{
"id": "F-01",
"severity": "CRITICAL",
"file": "src/store/timeEntries.js",
"title": "Frontend bypasses TimeEntryController — validation and owner ACL never invoked",
"summary": "Store calls OpenRegister objectStore directly. TimeEntryController and TimeEntryService are dead code from the primary user flow. Owner-only delete is not enforced for frontend requests."
},
{
"id": "F-02",
"severity": "CRITICAL",
"file": "lib/Controller/TimeEntryController.php",
"title": "Missing @NoCSRFRequired on all API methods",
"summary": "Without @NoCSRFRequired, POST and DELETE AJAX calls from the SPA will be blocked by Nextcloud CSRF protection."
},
{
"id": "F-03",
"severity": "WARNING",
"file": "tests/unit/Service/TimeEntryServiceTest.php",
"title": "16 PHPCS violations — breaks composer cs:check",
"summary": "expectException/expectExceptionMessage must use named params per project standard. Multi-line call formatting violations. Run composer cs:fix + manual named-param fixes."
},
{
"id": "F-04",
"severity": "WARNING",
"file": "tests/unit/Controller/TimeEntryControllerTest.php",
"title": "Missing test: destroy() returns 404 when entry not found",
"summary": "The InvalidArgumentException -> 404 path in destroy() has no test coverage."
},
{
"id": "F-05",
"severity": "WARNING",
"file": "lib/Service/TimeEntryService.php",
"title": "date field not validated for format",
"summary": "validateTimeEntryData() only checks non-empty. Invalid date strings (e.g. 'not-a-date') are accepted and persisted."
},
{
"id": "F-06",
"severity": "WARNING",
"file": "src/components/TimeLog.vue",
"title": "No confirmation dialog before destructive delete",
"summary": "deleteEntry() fires immediately on button click with no confirm guard. Risk of accidental data loss."
},
{
"id": "F-07",
"severity": "SUGGESTION",
"file": "src/components/TimeLog.vue",
"title": "No success feedback after time entry submission",
"summary": "Form resets silently. Add showSuccess() toast on successful createEntry()."
},
{
"id": "F-08",
"severity": "SUGGESTION",
"file": "lib/Service/TimeEntryService.php",
"title": "No pagination on listTimeEntries",
"summary": "All entries for a task returned in one response. Add limit/offset before heavy use."
}
],
"summary": {
"critical": 2,
"warning": 4,
"suggestion": 2,
"must_fix_before_merge": ["F-01", "F-02", "F-03", "F-04", "F-05", "F-06"]
}
}Two CRITICAL blockers must be resolved before this can merge:
All 4 WARNINGs (F-03 through F-06) must also be addressed per the Hydra review policy. |
Quality Report
Summary
PHP Quality
Vue Quality
Security
License Compliance
composer dependencies (100 total)
npm dependencies (215 total)
PHPUnit Tests
Code coverage: 0% (0 / 3 statements) Integration Tests (Newman)Newman integration tests were not enabled for this run. E2E Tests (Playwright)Playwright E2E tests were not enabled for this run. Generated automatically by the Quality workflow.
|
[WARNING] Wrong HTTP status for access-denial in
|
[SUGGESTION] Raw exception messages returned to client in
|
[INFO]
|
Security Review Verdict{
"verdict": "PASS_WITH_WARNINGS",
"pr": "https://github.com/ConductionNL/planix/pull/22",
"reviewer": "Clyde Barcode — Hydra Security Reviewer",
"review_date": "2026-04-05",
"sast": {
"semgrep": { "rules": ["p/security-audit", "p/secrets", "p/owasp-top-ten"], "findings": 0 },
"gitleaks": { "findings": 0 },
"trivy": { "status": "not_present" }
},
"manual_findings": [
{
"id": "SEC-TIME-001",
"severity": "WARNING",
"title": "Wrong HTTP status for access-denial in create() — RuntimeException mapped to 500 instead of 403",
"file": "lib/Controller/TimeEntryController.php",
"lines": "82-87",
"description": "The RuntimeException catch block in create() returns HTTP 500 instead of HTTP 403, causing access-control rejections to surface as internal server errors. The authorization logic itself is correct — no entry is created — but monitoring/alerting tools will treat this as an infrastructure failure rather than a normal denied request.",
"recommendation": "Change Http::STATUS_INTERNAL_SERVER_ERROR to Http::STATUS_FORBIDDEN in the RuntimeException catch block of create().",
"must_fix_before_merge": true
},
{
"id": "SEC-TIME-002",
"severity": "SUGGESTION",
"title": "Raw exception messages returned to client in internal-error path",
"file": "lib/Controller/TimeEntryController.php",
"lines": "77-87",
"description": "RuntimeException messages (e.g. 'OpenRegister is not available.') are forwarded directly to the API response body. No currently-exploitable disclosure exists, but the pattern creates fragility for future exceptions that might carry internal details.",
"recommendation": "Log the full exception server-side; return a generic error string for the 500/internal path.",
"must_fix_before_merge": false
},
{
"id": "SEC-TIME-003",
"severity": "INFO",
"title": "@NoCSRFRequired on index() GET endpoint",
"file": "lib/Controller/TimeEntryController.php",
"lines": "96-99",
"description": "Standard Nextcloud practice for read-only GET endpoints. All state-changing endpoints require a CSRF token. No action needed.",
"must_fix_before_merge": false
}
],
"positive_observations": [
"UUID validation on all path and query parameters before hitting service layer",
"Project-membership gate (assertProjectMembership) enforced on list and create — fails closed by design",
"Owner-only enforcement on update and delete — server-side, not client-gated",
"user field on time entry is server-set from IUserSession, never from client input — mass-assignment prevented",
"Whitelist merge in updateTimeEntry() — only duration, date, description accepted from client",
"No secrets, credentials, or tokens found in diff",
"CSRF protection active on POST/PUT/DELETE endpoints",
"All Vue template output uses interpolation or NcTextField — no raw innerHTML / v-html",
"isOwner() check in frontend is UI-only; real ownership enforced server-side"
],
"summary": "The implementation is security-conscious overall. IDOR guards, project-membership checks, owner-only enforcement, and input validation are all in place and well-structured. One WARNING must be fixed before merge: the create() endpoint returns HTTP 500 for access-denial RuntimeExceptions instead of HTTP 403. This does not create an exploitable vulnerability but is semantically incorrect and will interfere with monitoring."
} |
| ['name' => 'settings#load', 'url' => '/api/settings/load', 'verb' => 'POST'], | ||
|
|
||
| // Time entry CRUD endpoints. | ||
| ['name' => 'time_entry#create', 'url' => '/api/time-entries', 'verb' => 'POST'], |
There was a problem hiding this comment.
[CRITICAL] Missing OAS 3.0 OpenAPI specification
The Conduction company-wide conventions require an OAS 3.0 OpenAPI Specification for all new or changed endpoints. This PR adds four new REST endpoints (POST /api/time-entries, GET /api/time-entries, PUT /api/time-entries/{id}, DELETE /api/time-entries/{id}) with no accompanying OpenAPI spec file. Without the spec, downstream consumers, API gateways, and the Common Ground ecosystem cannot discover or validate against these endpoints.
Required action: Add an openapi.yaml documenting all four endpoints — request bodies, query parameters, response schemas, and error responses.
| { | ||
| $taskId = 'task-uuid-1'; | ||
| $entries = [ | ||
| ['id' => 'e1', 'task' => $taskId, 'duration' => 30, 'date' => '2026-04-01'], |
There was a problem hiding this comment.
[CRITICAL] Test testIndexReturnsEntriesForTask uses an invalid UUID — will assert 400, not 200
The test supplies 'task-uuid-1' as taskId, but the controller validates UUID format at line 119 before delegating to the service:
if (preg_match('/^[0-9a-f]{8}-...$/i', $taskId) !== 1) {
return new JSONResponse(['error' => 'taskId must be a valid UUID.'], Http::STATUS_BAD_REQUEST);
}'task-uuid-1' does not match the pattern, so the controller returns 400. The test then calls listTimeEntries via mock (which would never be reached) and asserts 200.
This test documents incorrect behaviour and would fail if run inside the Nextcloud environment.
Required action: Replace 'task-uuid-1' with a valid UUID v4, e.g. 'a1b2c3d4-0000-4000-a000-000000000001'.
| // Time entry CRUD endpoints. | ||
| ['name' => 'time_entry#create', 'url' => '/api/time-entries', 'verb' => 'POST'], | ||
| ['name' => 'time_entry#index', 'url' => '/api/time-entries', 'verb' => 'GET'], | ||
| ['name' => 'time_entry#update', 'url' => '/api/time-entries/{id}', 'verb' => 'PUT'], |
There was a problem hiding this comment.
[WARNING] PUT used for partial-update semantics — should be PATCH (NL API strategie)
The route registers PUT /api/time-entries/{id} and the service only updates the fields that are present in $data (partial update). Per the NL API strategie (Dutch government API design guidelines) and RFC 7231:
- PUT replaces the entire resource. The client must send the full representation.
- PATCH applies a partial modification. Only supplied fields are changed.
The implementation is partial-update behaviour, which means the correct HTTP verb is PATCH.
Required action: Change the route verb from 'PUT' to 'PATCH' in routes.php and update the store (axios.put → axios.patch) accordingly.
| throw new \InvalidArgumentException('Time entry not found.'); | ||
| } | ||
|
|
||
| $userId = $this->getCurrentUserId(); |
There was a problem hiding this comment.
[WARNING] deleteTimeEntry() lacks explicit $userId === null guard
Unlike createTimeEntry() (line 251) and listTimeEntries() (line 292) which both explicitly check $userId === null and throw with a clear message, deleteTimeEntry() jumps straight to the ownership comparison:
$userId = $this->getCurrentUserId();
if (($entry['user'] ?? '') !== $userId) { // null !== 'owner' → throws (fine)
throw new \RuntimeException('Only the owner may delete a time entry.');
}This is correct for the normal case (unauthenticated → null → throws), but:
- It masks the true reason for denial — the error message says "only the owner" rather than "authentication required".
- If an entry ever has
user = null(e.g. a legacy record), an unauthenticated user would pass the check (null !== nullis false) and delete the entry.
Required action: Add the same null guard present in the other service methods:
if ($userId === null) {
throw new \RuntimeException('Access denied: authentication required.');
}
| * @param {string} taskId Task UUID | ||
| * @return {Promise<Array>} | ||
| */ | ||
| async fetchEntries(taskId) { |
There was a problem hiding this comment.
[WARNING] No frontend tests for new Vue components and Pinia store
The CLAUDE.md quality gates for JS/TS projects require:
npm ci
npm test # Vitest / Jest
npm run lint
Three new frontend units are introduced with zero test coverage:
src/store/timeEntries.js— all five actions (fetchEntries,createEntry,updateEntry,deleteEntry) and thetotalDurationgettersrc/components/TimeLog.vue— form validation, sort order, owner-only delete visibility, edit flowsrc/views/TaskDetail.vue— task fetch, breadcrumb, error state
Required action: Add Vitest unit tests covering at minimum the store actions (using vi.mock for axios) and the isOwner/isFormValid computed properties in TimeLog.vue.
| if (this.task?.project && !this.projectsStore.activeProject) { | ||
| await this.projectsStore.fetchProject(this.task.project) | ||
| } | ||
| } catch (err) { |
There was a problem hiding this comment.
[SUGGESTION] Silent error on task load — no user-visible feedback
When fetchTask() fails (network error, 403, 404), the error is only sent to console.error. The task stays null, which shows the "Task not found" empty state — but the user gets no indication of why (network failure vs. genuine missing task).
Consider calling showError(t('planix', 'Failed to load task')) from @nextcloud/dialogs inside the catch block, consistent with how the store handles create/update/delete errors.
| const response = await axios.get(API_BASE, { params: { taskId } }) | ||
| this.entries = response.data || [] | ||
| return this.entries | ||
| } catch (err) { |
There was a problem hiding this comment.
[SUGGESTION] fetchEntries silently swallows errors — inconsistent with other store actions
createEntry, updateEntry, and deleteEntry all call showError() on failure. fetchEntries only logs to console.error and returns an empty array, leaving the user staring at the empty state with no explanation.
Consider adding showError(t('planix', 'Failed to load time entries')) in the catch block for consistency.
Hydra Code Review — Juan Claude van Damme{
"reviewer": "Juan Claude van Damme (Hydra Code Reviewer)",
"pr": "ConductionNL/planix#22",
"reviewed_at": "2026-04-05",
"verdict": "REQUEST_CHANGES",
"quality_gates": {
"phpcs_new_files": "PASS",
"phpcs_routes_php": "PRE_EXISTING_VIOLATION (not introduced by this PR)",
"phpunit": "CANNOT_RUN_OUTSIDE_NEXTCLOUD_ENV (pre-existing constraint)",
"npm_lint": "NOT_RUN (no JS linter changes in scope)"
},
"findings": [
{
"id": "F-001",
"severity": "CRITICAL",
"file": "appinfo/routes.php",
"line": 14,
"title": "Missing OAS 3.0 OpenAPI specification",
"description": "Company-wide convention requires an OpenAPI 3.0 spec for all new or changed endpoints. Four new endpoints are introduced with no spec file."
},
{
"id": "F-002",
"severity": "CRITICAL",
"file": "tests/unit/Controller/TimeEntryControllerTest.php",
"line": 212,
"title": "testIndexReturnsEntriesForTask uses invalid UUID — test asserts wrong status code",
"description": "taskId 'task-uuid-1' fails the controller's UUID regex at line 119 (returns 400). The test mocks listTimeEntries and asserts 200, documenting incorrect behavior. Would fail in the Nextcloud environment."
},
{
"id": "F-003",
"severity": "WARNING",
"file": "appinfo/routes.php",
"line": 16,
"title": "PUT used for partial-update semantics — NL API strategie requires PATCH",
"description": "The update endpoint only changes provided fields (partial update), but uses PUT which per HTTP/NL API strategie must replace the entire resource. Use PATCH."
},
{
"id": "F-004",
"severity": "WARNING",
"file": "lib/Service/TimeEntryService.php",
"line": 366,
"title": "deleteTimeEntry() lacks explicit null-userId guard",
"description": "Unlike createTimeEntry() and listTimeEntries(), deleteTimeEntry() skips the null check on $userId. If an entry has user=null a null-authenticated request would bypass the ownership check and delete the entry."
},
{
"id": "F-005",
"severity": "WARNING",
"file": "src/store/timeEntries.js",
"line": 44,
"title": "No frontend tests for new Vue components and Pinia store",
"description": "timeEntries.js, TimeLog.vue, and TaskDetail.vue have zero test coverage. Quality gates require npm test to pass."
},
{
"id": "F-006",
"severity": "SUGGESTION",
"file": "src/views/TaskDetail.vue",
"line": 124,
"title": "Silent error on task fetch failure",
"description": "fetchTask() catch block only calls console.error. Should call showError() for user-visible feedback, consistent with store error handling."
},
{
"id": "F-007",
"severity": "SUGGESTION",
"file": "src/store/timeEntries.js",
"line": 51,
"title": "fetchEntries silently swallows errors",
"description": "Unlike create/update/delete actions, fetchEntries does not call showError() on failure. Inconsistent UX."
}
],
"summary": {
"critical": 2,
"warning": 3,
"suggestion": 2,
"positives": [
"PHPCS passes cleanly on all new PHP files",
"Project membership IDOR guard (assertProjectMembership) correctly enforces read access — closes F-002/SEC-001",
"UUID format validation in controller before delegating to service",
"Owner-only edit and delete correctly enforced with 403 responses",
"NcDialog confirmation before destructive delete — good UX",
"EUPL-1.2 license headers present on all new files",
"PHPDoc annotations complete and accurate throughout",
"Test coverage for controller and service is structurally sound (10 + 7 methods)",
"Pinia store does not send user field from client — server always sets it (correct)"
]
}
} |
|
Closing — new test run. |
1 participant
Summary
Implements the Time Tracking MVP for Planix. Users can log time entries against tasks and view a per-task time log. The backend provides CRUD endpoints via a TimeEntryController that delegates to OpenRegister's ObjectService through a TimeEntryService. The frontend adds a TimeLog.vue component (with log form, entry list, and delete capability) embedded in a new TaskDetail view, backed by a dedicated Pinia store.
Spec Reference
openspec/changes/spec/design.md
Changes
lib/Controller/TimeEntryController.php— new controller with POST/GET/DELETE endpoints for time entries, authentication checks, validation error handlinglib/Service/TimeEntryService.php— new service encapsulating OpenRegister CRUD for timeEntry objects, input validation, owner-only delete enforcementappinfo/routes.php— added/api/time-entriesroutes (POST, GET, DELETE)src/store/timeEntries.js— new Pinia store for time entry CRUD via @conduction/nextcloud-vue objectStoresrc/components/TimeLog.vue— time log component with entry list, log form (minutes + date + description), empty state, owner-only deletesrc/views/TaskDetail.vue— task detail view with breadcrumb, task meta, and embedded TimeLog componentsrc/router/index.js— added/tasks/:taskIdroute for task detail viewopenspec/changes/spec/— copied spec files, marked all tasks complete, status set to pr-createdTest Coverage
tests/unit/Controller/TimeEntryControllerTest.php— 10 test methods: auth checks (403 for unauthenticated on create/index/destroy), successful create (201), validation error (400), missing taskId (400), list entries, owner delete success, non-owner delete forbiddentests/unit/Service/TimeEntryServiceTest.php— 6 test methods: getCurrentUserId with/without user, validation throws on missing taskId, zero duration, negative duration, missing date🤖 Generated with Claude Code