diff --git a/lib/Controller/CommentController.php b/lib/Controller/CommentController.php index 6b25b338..03cce59b 100644 --- a/lib/Controller/CommentController.php +++ b/lib/Controller/CommentController.php @@ -158,11 +158,16 @@ public function resolve(string $id): JSONResponse // Admins may resolve any thread; non-admins are checked against authorship // in CommentService::resolveThread (OWASP A01 — Broken Access Control). - $callerUid = $user->getUID(); + $callerUid = $user->getUID(); $callerIsAdmin = $this->groupManager->isAdmin($callerUid); + $ownerFilter = null; + if ($callerIsAdmin === false) { + $ownerFilter = $callerUid; + } + try { - $comment = $this->commentService->resolveThread($id, $callerIsAdmin ? null : $callerUid); + $comment = $this->commentService->resolveThread($id, $ownerFilter); return new JSONResponse($comment); } catch (\InvalidArgumentException $e) { return new JSONResponse(['message' => $e->getMessage()], Http::STATUS_FORBIDDEN); diff --git a/lib/Controller/MotionCoauthorController.php b/lib/Controller/MotionCoauthorController.php index 2e0bb461..2e6cf3aa 100644 --- a/lib/Controller/MotionCoauthorController.php +++ b/lib/Controller/MotionCoauthorController.php @@ -92,13 +92,17 @@ public function addCoauthor(string $id): JSONResponse $callerUid = $user->getUID(); $callerIsAdmin = $this->groupManager->isAdmin($callerUid); + // Admins bypass ownership check; null callerUid skips the access check in the service. + $accessUid = $callerUid; + if ($callerIsAdmin === true) { + $accessUid = null; + } try { $motion = $this->coauthorService->addCoauthor( motionId: $id, personId: $personId, - callerUid: $callerUid, - callerIsAdmin: $callerIsAdmin, + callerUid: $accessUid, ); return new JSONResponse($motion); } catch (\InvalidArgumentException $e) { @@ -132,13 +136,17 @@ public function removeCoauthor(string $id, string $personId): JSONResponse $callerUid = $user->getUID(); $callerIsAdmin = $this->groupManager->isAdmin($callerUid); + // Admins bypass ownership check; null callerUid skips the access check in the service. + $accessUid = $callerUid; + if ($callerIsAdmin === true) { + $accessUid = null; + } try { $motion = $this->coauthorService->removeCoauthor( motionId: $id, personId: $personId, - callerUid: $callerUid, - callerIsAdmin: $callerIsAdmin, + callerUid: $accessUid, ); return new JSONResponse($motion); } catch (\InvalidArgumentException $e) { @@ -183,6 +191,11 @@ public function updateText(string $id): JSONResponse $callerUid = $user->getUID(); $callerIsAdmin = $this->groupManager->isAdmin($callerUid); + // Admins bypass ownership check; null callerUid skips the access check in the service. + $accessUid = $callerUid; + if ($callerIsAdmin === true) { + $accessUid = null; + } try { $motion = $this->coauthorService->updateMotionText( @@ -190,7 +203,7 @@ public function updateText(string $id): JSONResponse newText: $text, author: $callerUid, changeSummary: $summary, - callerIsAdmin: $callerIsAdmin, + callerUid: $accessUid, ); return new JSONResponse($motion); } catch (\InvalidArgumentException $e) { diff --git a/lib/Controller/TaskController.php b/lib/Controller/TaskController.php index aa3cac41..888222ba 100644 --- a/lib/Controller/TaskController.php +++ b/lib/Controller/TaskController.php @@ -92,11 +92,16 @@ public function status(string $id): JSONResponse // Admins bypass ownership check; regular users are checked inside the service // against assignee/delegator (OWASP A01 — Broken Access Control). - $callerUid = $user->getUID(); + $callerUid = $user->getUID(); $callerIsAdmin = $this->groupManager->isAdmin($callerUid); + $ownerFilter = null; + if ($callerIsAdmin === false) { + $ownerFilter = $callerUid; + } + try { - $task = $this->taskService->updateTaskStatus($id, $newStatus, $callerIsAdmin ? null : $callerUid); + $task = $this->taskService->updateTaskStatus($id, $newStatus, $ownerFilter); return new JSONResponse($task); } catch (\InvalidArgumentException $e) { // Distinguish between "transition not allowed" (422) and "not owner" (403). diff --git a/lib/Mcp/DecideskToolProvider.php b/lib/Mcp/DecideskToolProvider.php index 445778ac..679ddff7 100644 --- a/lib/Mcp/DecideskToolProvider.php +++ b/lib/Mcp/DecideskToolProvider.php @@ -1124,10 +1124,10 @@ private function getCallerMeetingUuids(string $userId): ?array // participant → governance-body → meeting (ParticipantResolver docblock). $meetingUuids = []; foreach ($participants as $raw) { - $p = $this->toArray(item: $raw); - $bodyId = null; + $participant = $this->toArray(item: $raw); + $bodyId = null; - foreach (($p['relations'] ?? []) as $rel) { + foreach (($participant['relations'] ?? []) as $rel) { if (is_array($rel) === true && ($rel['schema'] ?? '') === 'governance-body') { $bodyId = ($rel['id'] ?? null); break; @@ -1150,10 +1150,10 @@ private function getCallerMeetingUuids(string $userId): ?array ); foreach ($meetingEntities as $meetingRaw) { - $m = $this->toArray(item: $meetingRaw); - $mId = ($m['id'] ?? ($m['uuid'] ?? null)); - if ($mId !== null) { - $meetingUuids[] = (string) $mId; + $meeting = $this->toArray(item: $meetingRaw); + $meetingId = ($meeting['id'] ?? ($meeting['uuid'] ?? null)); + if ($meetingId !== null) { + $meetingUuids[] = (string) $meetingId; } } }//end foreach diff --git a/lib/Service/ActionItemExtractionService.php b/lib/Service/ActionItemExtractionService.php index a2640055..07588d4d 100644 --- a/lib/Service/ActionItemExtractionService.php +++ b/lib/Service/ActionItemExtractionService.php @@ -149,14 +149,18 @@ public function saveExtracted(string $minutesId, array $confirmed): int if (empty($candidate['dueDate']) === false) { // Accept only ISO-8601 date strings to prevent strtotime relative-expression injection // (e.g. "yesterday", "next Monday") that produce silently wrong deadline values. - if (preg_match('/^\d{4}-\d{2}-\d{2}(T\d{2}:\d{2}(:\d{2})?Z?)?$/', (string) $candidate['dueDate']) === 1) { - $actionItem['dueDate'] = $candidate['dueDate']; - } else { + $isoDatePattern = '/^\d{4}-\d{2}-\d{2}(T\d{2}:\d{2}(:\d{2})?Z?)?$/'; + $isValidDate = preg_match($isoDatePattern, (string) $candidate['dueDate']) === 1; + if ($isValidDate === false) { $this->logger->warning( 'ActionItemExtractionService: rejected non-ISO-8601 dueDate', ['dueDate' => $candidate['dueDate']] ); } + + if ($isValidDate === true) { + $actionItem['dueDate'] = $candidate['dueDate']; + } } try { diff --git a/lib/Service/AgendaService.php b/lib/Service/AgendaService.php index 6e84db35..6e574972 100644 --- a/lib/Service/AgendaService.php +++ b/lib/Service/AgendaService.php @@ -157,7 +157,7 @@ public function publishAgenda(string $meetingId): void // silently wipe required fields that are not included in the update. $meetingEntity = $this->objectService->find(id: $meetingId, register: 'decidesk', schema: 'meeting'); if ($meetingEntity === null) { - throw new \OCA\Decidesk\Exception\NotFoundException(message: "Meeting {$meetingId} not found"); + throw new NotFoundException(message: "Meeting {$meetingId} not found"); } $meetingData = $this->toArray(item: $meetingEntity); @@ -362,7 +362,7 @@ public function reviseAgenda(string $meetingId): void // #315: Read the full meeting object before saving to avoid wiping required fields. $meetingEntity = $this->objectService->find(id: $meetingId, register: 'decidesk', schema: 'meeting'); if ($meetingEntity === null) { - throw new \OCA\Decidesk\Exception\NotFoundException(message: "Meeting {$meetingId} not found"); + throw new NotFoundException(message: "Meeting {$meetingId} not found"); } $meetingData = $this->toArray(item: $meetingEntity); diff --git a/lib/Service/CommentService.php b/lib/Service/CommentService.php index 66afd9eb..5995d266 100644 --- a/lib/Service/CommentService.php +++ b/lib/Service/CommentService.php @@ -263,7 +263,7 @@ public function findCommentsForTarget(string $target): array * * @spec openspec/changes/p4-collaboration/tasks.md#task-5.1 */ - public function resolveThread(string $commentId, ?string $callerUid = null): array + public function resolveThread(string $commentId, ?string $callerUid=null): array { $comment = $this->findComment(commentId: $commentId); if ($comment === null) { @@ -274,7 +274,7 @@ public function resolveThread(string $commentId, ?string $callerUid = null): arr if ($callerUid !== null) { $author = (string) ($comment['author'] ?? ''); if ($author === '' || $author !== $callerUid) { - throw new \InvalidArgumentException('Only the comment author may resolve this thread'); + throw new InvalidArgumentException('Only the comment author may resolve this thread'); } } diff --git a/lib/Service/DelegationService.php b/lib/Service/DelegationService.php index 7f277b78..03729fd2 100644 --- a/lib/Service/DelegationService.php +++ b/lib/Service/DelegationService.php @@ -27,6 +27,7 @@ use DateTimeImmutable; use DateTimeInterface; +use InvalidArgumentException; use Psr\Container\ContainerInterface; use Psr\Log\LoggerInterface; use RuntimeException; @@ -161,7 +162,7 @@ public function revokeDelegation(string $delegationId, ?string $callerUid=null): if ($callerUid !== null) { $principal = (string) ($delegation['delegatedBy'] ?? ''); if ($principal === '' || $principal !== $callerUid) { - throw new \InvalidArgumentException('Only the delegation principal may revoke this delegation'); + throw new InvalidArgumentException('Only the delegation principal may revoke this delegation'); } } @@ -209,7 +210,7 @@ public function expireDelegation(string $delegationId, ?string $callerUid=null): if ($callerUid !== null) { $principal = (string) ($delegation['delegatedBy'] ?? ''); if ($principal === '' || $principal !== $callerUid) { - throw new \InvalidArgumentException('Only the delegation principal may expire this delegation'); + throw new InvalidArgumentException('Only the delegation principal may expire this delegation'); } } diff --git a/lib/Service/MotionCoauthorService.php b/lib/Service/MotionCoauthorService.php index 79a14e99..c97a385d 100644 --- a/lib/Service/MotionCoauthorService.php +++ b/lib/Service/MotionCoauthorService.php @@ -30,6 +30,7 @@ use DateTimeImmutable; use DateTimeInterface; +use InvalidArgumentException; use Psr\Container\ContainerInterface; use Psr\Log\LoggerInterface; use RuntimeException; @@ -165,7 +166,7 @@ private function checkMotionAccess(array $motion, ?string $callerUid, bool $call return; } - throw new \InvalidArgumentException( + throw new InvalidArgumentException( 'Only the motion proposer or an existing co-author may modify this motion' ); @@ -176,12 +177,11 @@ private function checkMotionAccess(array $motion, ?string $callerUid, bool $call * * Only the motion proposer (owner), an existing co-author, or an admin may * add co-authors (OWASP A01:2021 — Broken Access Control). - * Pass `$callerUid = null` only from admin-only or background-job paths. + * Pass `$callerUid = null` to bypass the ownership check (admin/background-job paths). * - * @param string $motionId Motion UUID - * @param string $personId Person UUID to add as co-author - * @param string|null $callerUid NC UID of the requester (null = skip check) - * @param bool $callerIsAdmin Whether the caller is an NC admin + * @param string $motionId Motion UUID + * @param string $personId Person UUID to add as co-author + * @param string|null $callerUid NC UID of the requester (null = skip access check) * * @return array * @@ -193,10 +193,9 @@ public function addCoauthor( string $motionId, string $personId, ?string $callerUid=null, - bool $callerIsAdmin=false, ): array { $motion = $this->findMotion(motionId: $motionId); - $this->checkMotionAccess(motion: $motion, callerUid: $callerUid, callerIsAdmin: $callerIsAdmin); + $this->checkMotionAccess(motion: $motion, callerUid: $callerUid, callerIsAdmin: false); $coauthors = ($motion['coAuthors'] ?? []); @@ -227,12 +226,11 @@ public function addCoauthor( * * Only the motion proposer (owner), an existing co-author, or an admin may * remove co-authors (OWASP A01:2021 — Broken Access Control). - * Pass `$callerUid = null` only from admin-only or background-job paths. + * Pass `$callerUid = null` to bypass the ownership check (admin/background-job paths). * - * @param string $motionId Motion UUID - * @param string $personId Person UUID to remove - * @param string|null $callerUid NC UID of the requester (null = skip check) - * @param bool $callerIsAdmin Whether the caller is an NC admin + * @param string $motionId Motion UUID + * @param string $personId Person UUID to remove + * @param string|null $callerUid NC UID of the requester (null = skip access check) * * @return array * @@ -244,10 +242,9 @@ public function removeCoauthor( string $motionId, string $personId, ?string $callerUid=null, - bool $callerIsAdmin=false, ): array { $motion = $this->findMotion(motionId: $motionId); - $this->checkMotionAccess(motion: $motion, callerUid: $callerUid, callerIsAdmin: $callerIsAdmin); + $this->checkMotionAccess(motion: $motion, callerUid: $callerUid, callerIsAdmin: false); $coauthors = ($motion['coAuthors'] ?? []); $motion['coAuthors'] = array_values( array_filter( @@ -279,11 +276,11 @@ public function removeCoauthor( * Only the motion proposer (owner), an existing co-author, or an admin may * update the text (OWASP A01:2021 — Broken Access Control). * - * @param string $motionId Motion UUID - * @param string $newText New motion text - * @param string $author NC UID of the author making the change - * @param string $changeSummary Human-readable change summary - * @param bool $callerIsAdmin Whether the caller is an NC admin + * @param string $motionId Motion UUID + * @param string $newText New motion text + * @param string $author NC UID of the author making the change (recorded in history) + * @param string $changeSummary Human-readable change summary + * @param string|null $callerUid NC UID to check for access (null = skip check for admin paths) * * @return array * @@ -297,10 +294,10 @@ public function updateMotionText( string $newText, string $author, string $changeSummary, - bool $callerIsAdmin=false, + ?string $callerUid=null, ): array { $motion = $this->findMotion(motionId: $motionId); - $this->checkMotionAccess(motion: $motion, callerUid: $author, callerIsAdmin: $callerIsAdmin); + $this->checkMotionAccess(motion: $motion, callerUid: $callerUid, callerIsAdmin: false); $previousText = (string) ($motion['text'] ?? ''); $previousHistory = ($motion['versionHistory'] ?? []); diff --git a/lib/Service/VotingService.php b/lib/Service/VotingService.php index 530a3438..81ae84ff 100644 --- a/lib/Service/VotingService.php +++ b/lib/Service/VotingService.php @@ -551,7 +551,6 @@ public function closeVotingRound(string $votingRoundId, bool $anonymise=false): // Transient/infrastructure errors are still logged-and-continued so a network hiccup // does not leave the round un-closed; however they are logged at ERROR level so they // surface in monitoring rather than silently disappearing. - $lifecycleError = null; if ($round !== null) { foreach (($round['relations'] ?? []) as $rel) { if (($rel['schema'] ?? '') === 'motion') { @@ -599,7 +598,6 @@ public function closeVotingRound(string $votingRoundId, bool $anonymise=false): 'Decidesk: lifecycle transition after close failed — round is closed but motion state may be stale', ['votingRoundId' => $votingRoundId, 'motionId' => $motionId, 'error' => $e->getMessage()] ); - $lifecycleError = $e->getMessage(); }//end try }//end if }//end if diff --git a/lib/Service/WorkspaceService.php b/lib/Service/WorkspaceService.php index 9d8e8a55..f9814af2 100644 --- a/lib/Service/WorkspaceService.php +++ b/lib/Service/WorkspaceService.php @@ -244,13 +244,13 @@ public function isMembershipManager(array $workspace, string $callerUid): bool // Existing workspace members may add/remove other members. // members[] stores participant UUIDs — resolve caller's UID first. - $callerParticipantUuid = $this->resolveParticipantUuid(nextcloudUid: $callerUid); - if ($callerParticipantUuid === null) { + $callerPtUuid = $this->resolveParticipantUuid(nextcloudUid: $callerUid); + if ($callerPtUuid === null) { return false; } $members = ($workspace['members'] ?? []); - if (in_array(needle: $callerParticipantUuid, haystack: $members, strict: true) === true) { + if (in_array(needle: $callerPtUuid, haystack: $members, strict: true) === true) { return true; } @@ -283,7 +283,7 @@ public function addMember(string $workspaceId, string $memberRef, ?string $calle } if ($callerUid !== null && $this->isMembershipManager(workspace: $workspace, callerUid: $callerUid) === false) { - throw new \InvalidArgumentException('Only workspace owners and members may add members to this workspace'); + throw new InvalidArgumentException('Only workspace owners and members may add members to this workspace'); } $members = ($workspace['members'] ?? []); @@ -334,7 +334,7 @@ public function removeMember(string $workspaceId, string $memberRef, ?string $ca } if ($callerUid !== null && $this->isMembershipManager(workspace: $workspace, callerUid: $callerUid) === false) { - throw new \InvalidArgumentException('Only workspace owners and members may remove members from this workspace'); + throw new InvalidArgumentException('Only workspace owners and members may remove members from this workspace'); } $members = ($workspace['members'] ?? []); diff --git a/phpmd.baseline.xml b/phpmd.baseline.xml index b1ef36c0..0b4964a2 100644 --- a/phpmd.baseline.xml +++ b/phpmd.baseline.xml @@ -2,6 +2,7 @@ + @@ -11,12 +12,17 @@ + + + + + @@ -27,6 +33,8 @@ + + @@ -38,6 +46,10 @@ + + + + @@ -80,6 +92,8 @@ + + @@ -91,8 +105,13 @@ + + + + +