Skip to content
This repository was archived by the owner on May 29, 2026. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions lib/Controller/CommentController.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
23 changes: 18 additions & 5 deletions lib/Controller/MotionCoauthorController.php
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -183,14 +191,19 @@ 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(
motionId: $id,
newText: $text,
author: $callerUid,
changeSummary: $summary,
callerIsAdmin: $callerIsAdmin,
callerUid: $accessUid,
);
return new JSONResponse($motion);
} catch (\InvalidArgumentException $e) {
Expand Down
9 changes: 7 additions & 2 deletions lib/Controller/TaskController.php
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down
14 changes: 7 additions & 7 deletions lib/Mcp/DecideskToolProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand Down
10 changes: 7 additions & 3 deletions lib/Service/ActionItemExtractionService.php
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
4 changes: 2 additions & 2 deletions lib/Service/AgendaService.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
4 changes: 2 additions & 2 deletions lib/Service/CommentService.php
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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');
}
}

Expand Down
5 changes: 3 additions & 2 deletions lib/Service/DelegationService.php
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@

use DateTimeImmutable;
use DateTimeInterface;
use InvalidArgumentException;
use Psr\Container\ContainerInterface;
use Psr\Log\LoggerInterface;
use RuntimeException;
Expand Down Expand Up @@ -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');
}
}

Expand Down Expand Up @@ -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');
}
}

Expand Down
41 changes: 19 additions & 22 deletions lib/Service/MotionCoauthorService.php
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@

use DateTimeImmutable;
use DateTimeInterface;
use InvalidArgumentException;
use Psr\Container\ContainerInterface;
use Psr\Log\LoggerInterface;
use RuntimeException;
Expand Down Expand Up @@ -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'
);

Expand All @@ -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<string, mixed>
*
Expand All @@ -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'] ?? []);

Expand Down Expand Up @@ -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<string, mixed>
*
Expand All @@ -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(
Expand Down Expand Up @@ -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<string, mixed>
*
Expand All @@ -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'] ?? []);

Expand Down
2 changes: 0 additions & 2 deletions lib/Service/VotingService.php
Original file line number Diff line number Diff line change
Expand Up @@ -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') {
Expand Down Expand Up @@ -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
Expand Down
10 changes: 5 additions & 5 deletions lib/Service/WorkspaceService.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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'] ?? []);
Expand Down Expand Up @@ -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'] ?? []);
Expand Down
Loading