Tie workflows to the board owner instead of their creator
Build package / php-lint (8.4) (push) Successful in 2m22s
Build package / php-lint (8.5) (push) Successful in 57s
Build package / xml-lint (push) Successful in 18s
Build package / unit-tests (push) Successful in 1m5s
Build package / package (push) Successful in 1m21s
Build package / php-lint (8.4) (push) Successful in 2m22s
Build package / php-lint (8.5) (push) Successful in 57s
Build package / xml-lint (push) Successful in 18s
Build package / unit-tests (push) Successful in 1m5s
Build package / package (push) Successful in 1m21s
A workflow now belongs to whoever owns its board. The settings page lists, edits and deletes workflows by board ownership alone, using Deck's BoardMapper::findAllByOwner(), a single owner query that never returns shared boards and carries no frozen uid. A transferred board's workflows therefore show up for the new owner immediately. The background job still needs one user to run as and mail, so it looks up the board owner before every run and stores it in user_id when it changed. Archived boards stay listed so their workflows keep a name, but are no longer offered or accepted as a target.
This commit is contained in:
1 parent
ada16cb101
commit
e84123654e
6 files changed
+185
-72
No files matched your search
@@ -25,7 +25,7 @@ Both are undone in a `finally` (`WorkflowRunner::clearImpersonation()`).
|
||||
|
||||
In cron that is *never* the user being impersonated: `null` if any Deck-owned background job ran earlier in the same `cron.php` pass, otherwise the first workflow owner touched. `null` fails every check, so Deck answers `NoPermissionException` for perfectly intact boards. This produced a real bug — see the next section. `beginUserContext()` therefore swaps that private field directly (there is no API), records the old value, and `endUserContext()` puts it back. `PermissionService` is mandatory: if it can't be pinned, `beginUserContext()` throws and the runner **skips that user entirely** rather than acting under someone else's permissions. The other three are best-effort (they only affect activity attribution and unread counts).
|
||||
|
||||
Consequence for `getUserBoards()`: `listBoardsForCurrentUser()` stays **request-only** anyway — Deck may already have cached board lists behind that field, and the job has no use for a board *list*; it asks `findStackIds()` about one known board id. Before using any other `OCA\Deck\*` service from the runner, check whether it carries a frozen `$userId` — the failure mode is silent and user-crossing, not an error.
|
||||
Consequence for board lists: the app does not use `BoardService::getUserBoards()` at all (it reads that frozen field). `listOwnedBoards()` goes through `BoardMapper::findAllByOwner($uid)` instead, which takes the uid explicitly; the job still has no use for a board *list* and asks `findStackIds()` about one known board id. Before using any other `OCA\Deck\*` service from the runner, check whether it carries a frozen `$userId` — the failure mode is silent and user-crossing, not an error.
|
||||
|
||||
The filter logic (`WorkflowRunner::cardMatchesFilters()`, `::isOverdue()`, `::findDeadFilters()`) is deliberately `static` and side-effect-free so it's unit-testable without a real Deck installation — see `tests/Unit/Service/WorkflowRunnerFilterTest.php`. Keep new pure logic in that shape; it is the only part of this app that has tests at all.
|
||||
|
||||
@@ -40,11 +40,16 @@ The filter logic (`WorkflowRunner::cardMatchesFilters()`, `::isOverdue()`, `::fi
|
||||
|
||||
Archived boards deliberately do *not* trigger this (they stay readable, so `findStackIds()` succeeds), even though they're filtered out of the settings dropdowns.
|
||||
|
||||
## Workflows only on boards the user owns
|
||||
## A workflow belongs to its board's owner, not its creator
|
||||
|
||||
A workflow can only be created or saved on a board whose Deck owner is the current user. `DeckIntegrationService::listBoardsForCurrentUser($uid)` returns **only owned boards** (`unwrapUid($board->getOwner()) === $uid`); boards merely shared with the user are dropped. Since `WorkflowController::validate()` checks the board id against that list, the same filter enforces the rule server-side — there is no separate ownership check.
|
||||
The relation is **board owner ↔ workflow**, not creator ↔ workflow. When Deck transfers a board, its workflows move to the new owner.
|
||||
|
||||
Workflows created before this rule may still sit on a shared board. The runner does not check ownership, so they keep running; in the settings table their board is missing from the list, so the row is marked "Board nicht mehr vorhanden oder nicht dein eigenes" and saving it requires picking an owned board.
|
||||
- **Which boards count as "mine"**: `DeckIntegrationService::listOwnedBoards($uid)` → Deck's `BoardMapper::findAllByOwner($uid)`, one `WHERE owner = :uid` query — shared boards never come back, and as a plain QBMapper it carries no frozen uid. Trashed boards are dropped; archived ones are kept with `archived: true` so their workflows stay visible, but they're hidden from the board dropdown and rejected by `validate()`. It *throws* rather than returning `[]`, since `[]` would silently hide all of the user's workflows.
|
||||
- **Listing/editing/deleting** is purely `board_id IN (listOwnedBoards)` (`WorkflowMapper::findAllForBoards()`, `WorkflowController::findOwnWorkflow()`). `user_id` plays no part, so a transferred board's workflows show up for the new owner immediately — no cron pass needed. Workflows whose board is gone disappear from the UI; the runner disables them and mails the last owner.
|
||||
- **Creating/saving** validates the board against the same list. `create()`/`update()` set `user_id` to the current user.
|
||||
- **The runner** needs a single user to impersonate, so it calls `WorkflowRunner::followBoardOwner()` → `DeckIntegrationService::findBoardOwner()` (plain `BoardMapper::find()`) before grouping: a known owner different from `user_id` rewrites and persists it, so the run is impersonated as — and mails go to — the current owner. A gone board keeps its last owner, so `findBrokenTarget()` can still disable it and mail them.
|
||||
|
||||
`user_id` is therefore only "who the job runs as / who gets mail", refreshed from the board owner on every run — not the source of truth for who sees a workflow. Don't reintroduce `user_id` lookups in the controller.
|
||||
|
||||
## Dead filters are surfaced, never disabled
|
||||
|
||||
@@ -188,7 +193,7 @@ Deck's `RelationalEntity` swaps resolved relations for an `OCA\Deck\Db\Relationa
|
||||
|
||||
Two more Deck facts that bit us in the same pass: a board's ACL does **not** contain its owner (a private board has an empty ACL, so participants must be seeded with `$board->getOwner()`), and ACL entries can be groups (`type === 1`) whose members have to be expanded via `IGroupManager`.
|
||||
|
||||
**`BoardService::getUserBoards()` defaults to `$includeArchived = true`, and that flag also gates the `deleted_at = 0` condition** — with the default you get archived *and* trashed boards back. We call `getUserBoards(null, false)` (with a fallback to the no-arg form if Deck ever changes the signature) and still dedupe by id afterwards, because it merges own/group/circle boards and can return the same board twice. Stacks need no such handling: `StackMapper::findAll()` filters `deleted_at = 0` unconditionally and stacks have no archived state.
|
||||
**`BoardMapper::findAllByOwner()` has no archived/`deleted_at` filter at all** — trashed and archived boards come back too, which is why `listOwnedBoards()` drops trashed ones and flags archived ones itself. (`BoardService::getUserBoards()`, used earlier, merges own/shared/group/circle boards and is no longer used.) Stacks need no such handling: `StackMapper::findAll()` filters `deleted_at = 0` unconditionally and stacks have no archived state.
|
||||
|
||||
## Local-only notes
|
||||
|
||||
|
||||
@@ -38,9 +38,14 @@ class WorkflowController extends OCSController {
|
||||
return $user->getUID();
|
||||
}
|
||||
|
||||
/**
|
||||
* Every workflow on a board the user owns, no matter who created it: a
|
||||
* workflow belongs to its board's owner, and follows the board when
|
||||
* Deck transfers it.
|
||||
*/
|
||||
#[NoAdminRequired]
|
||||
public function index(): DataResponse {
|
||||
$workflows = $this->workflowMapper->findAllForUser($this->currentUserId());
|
||||
$workflows = $this->workflowMapper->findAllForBoards($this->ownedBoardIds());
|
||||
return new DataResponse(array_map(static fn (Workflow $w) => $w->jsonSerialize(), $workflows));
|
||||
}
|
||||
|
||||
@@ -85,14 +90,12 @@ class WorkflowController extends OCSController {
|
||||
bool $notifyEmail = false,
|
||||
bool $enabled = true,
|
||||
): DataResponse {
|
||||
try {
|
||||
$workflow = $this->workflowMapper->findForUser($id, $this->currentUserId());
|
||||
} catch (DoesNotExistException $e) {
|
||||
throw new OCSNotFoundException('Workflow not found');
|
||||
}
|
||||
|
||||
$workflow = $this->findOwnWorkflow($id);
|
||||
$this->validate($title, $boardId, $sourceStackId, $targetStackId);
|
||||
|
||||
// Kept in step with the board owner so the job runs as, and mails,
|
||||
// the right user; validate() made sure the editor owns the board.
|
||||
$workflow->setUserId($this->currentUserId());
|
||||
$this->applyFields($workflow, $title, $boardId, $sourceStackId, $targetStackId, $filterUserIds, $filterLabelIds, $notifyEmail, $enabled);
|
||||
$workflow = $this->workflowMapper->update($workflow);
|
||||
|
||||
@@ -101,12 +104,7 @@ class WorkflowController extends OCSController {
|
||||
|
||||
#[NoAdminRequired]
|
||||
public function destroy(int $id): DataResponse {
|
||||
try {
|
||||
$workflow = $this->workflowMapper->findForUser($id, $this->currentUserId());
|
||||
} catch (DoesNotExistException $e) {
|
||||
throw new OCSNotFoundException('Workflow not found');
|
||||
}
|
||||
|
||||
$workflow = $this->findOwnWorkflow($id);
|
||||
$this->workflowMapper->delete($workflow);
|
||||
return new DataResponse([]);
|
||||
}
|
||||
@@ -114,7 +112,7 @@ class WorkflowController extends OCSController {
|
||||
#[NoAdminRequired]
|
||||
public function boards(): DataResponse {
|
||||
$this->assertDeck();
|
||||
return new DataResponse($this->deckService->listBoardsForCurrentUser($this->currentUserId()));
|
||||
return new DataResponse($this->ownedBoards());
|
||||
}
|
||||
|
||||
#[NoAdminRequired]
|
||||
@@ -135,6 +133,40 @@ class WorkflowController extends OCSController {
|
||||
return new DataResponse($this->deckService->listParticipants($boardId));
|
||||
}
|
||||
|
||||
private function findOwnWorkflow(int $id): Workflow {
|
||||
try {
|
||||
$workflow = $this->workflowMapper->findById($id);
|
||||
} catch (DoesNotExistException $e) {
|
||||
throw new OCSNotFoundException('Workflow not found');
|
||||
}
|
||||
|
||||
if (!in_array($workflow->getBoardId(), $this->ownedBoardIds(), true)) {
|
||||
// Same answer as for a missing id: don't reveal other users' workflows.
|
||||
throw new OCSNotFoundException('Workflow not found');
|
||||
}
|
||||
return $workflow;
|
||||
}
|
||||
|
||||
/**
|
||||
* @return array<int, array{id: int, title: string, archived: bool}>
|
||||
*/
|
||||
private function ownedBoards(): array {
|
||||
$userId = $this->currentUserId();
|
||||
$this->assertDeck();
|
||||
try {
|
||||
return $this->deckService->listOwnedBoards($userId);
|
||||
} catch (DeckUnavailableException $e) {
|
||||
throw new OCSPreconditionFailedException('Could not load your Deck boards: ' . $e->getMessage());
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* @return int[]
|
||||
*/
|
||||
private function ownedBoardIds(): array {
|
||||
return array_map(static fn (array $board) => $board['id'], $this->ownedBoards());
|
||||
}
|
||||
|
||||
private function assertDeck(): void {
|
||||
$user = $this->userSession->getUser();
|
||||
if ($user === null) {
|
||||
@@ -162,13 +194,17 @@ class WorkflowController extends OCSController {
|
||||
$this->assertDeck();
|
||||
|
||||
// Only boards the user owns are listed, so this also enforces the
|
||||
// owner rule.
|
||||
$boardIds = array_map(
|
||||
static fn (array $board) => (int)$board['id'],
|
||||
$this->deckService->listBoardsForCurrentUser($this->currentUserId()),
|
||||
);
|
||||
if (!in_array($boardId, $boardIds, true)) {
|
||||
throw new OCSBadRequestException('The selected board does not exist or is not owned by you');
|
||||
// owner rule. Archived boards stay listed (their workflows must stay
|
||||
// visible) but are no target for a new or changed workflow.
|
||||
$board = null;
|
||||
foreach ($this->ownedBoards() as $candidate) {
|
||||
if ($candidate['id'] === $boardId) {
|
||||
$board = $candidate;
|
||||
break;
|
||||
}
|
||||
}
|
||||
if ($board === null || $board['archived']) {
|
||||
throw new OCSBadRequestException('The selected board does not exist, is archived or is not owned by you');
|
||||
}
|
||||
|
||||
$stackIds = array_map(
|
||||
|
||||
@@ -22,24 +22,27 @@ class WorkflowMapper extends QBMapper {
|
||||
* @throws DoesNotExistException
|
||||
* @throws MultipleObjectsReturnedException
|
||||
*/
|
||||
public function findForUser(int $id, string $userId): Workflow {
|
||||
public function findById(int $id): Workflow {
|
||||
$qb = $this->db->getQueryBuilder();
|
||||
$qb->select('*')
|
||||
->from($this->getTableName())
|
||||
->where($qb->expr()->eq('id', $qb->createNamedParameter($id, IQueryBuilder::PARAM_INT)))
|
||||
->andWhere($qb->expr()->eq('user_id', $qb->createNamedParameter($userId)));
|
||||
->where($qb->expr()->eq('id', $qb->createNamedParameter($id, IQueryBuilder::PARAM_INT)));
|
||||
|
||||
return $this->findEntity($qb);
|
||||
}
|
||||
|
||||
/**
|
||||
* @param int[] $boardIds
|
||||
* @return Workflow[]
|
||||
*/
|
||||
public function findAllForUser(string $userId): array {
|
||||
public function findAllForBoards(array $boardIds): array {
|
||||
if ($boardIds === []) {
|
||||
return [];
|
||||
}
|
||||
$qb = $this->db->getQueryBuilder();
|
||||
$qb->select('*')
|
||||
->from($this->getTableName())
|
||||
->where($qb->expr()->eq('user_id', $qb->createNamedParameter($userId)))
|
||||
->where($qb->expr()->in('board_id', $qb->createNamedParameter($boardIds, IQueryBuilder::PARAM_INT_ARRAY)))
|
||||
->orderBy('id', 'ASC');
|
||||
|
||||
return $this->findEntities($qb);
|
||||
|
||||
@@ -172,59 +172,44 @@ class DeckIntegrationService {
|
||||
}
|
||||
|
||||
/**
|
||||
* @return array<int, array{id: int, title: string}>
|
||||
* The boards `$userId` owns — the only boards workflows may live on, and
|
||||
* the boards whose workflows that user sees and manages.
|
||||
*
|
||||
* Only boards owned by `$userId` — workflows may only be created on those,
|
||||
* so boards merely shared with the user are left out entirely.
|
||||
* One query: BoardMapper::findAllByOwner() is `WHERE owner = :uid`, so
|
||||
* shared boards never come back, and as a plain QBMapper it has none of
|
||||
* the frozen-uid state of BoardService::getUserBoards(). It does return
|
||||
* archived and trashed boards; trashed ones are dropped here, archived
|
||||
* ones are kept but flagged — their workflows still exist and must stay
|
||||
* visible, they just aren't offered as a new target.
|
||||
*
|
||||
* Only ever call this from a *request*: getUserBoards() reads the uid
|
||||
* BoardService froze at construction time, and beginUserContext() cannot
|
||||
* help here — it repoints that field, but Deck may already have cached
|
||||
* board lists behind it, and the job has no use for a board *list*
|
||||
* anyway. It asks findStackIds() about one known board id instead.
|
||||
* Throws instead of degrading to `[]`: an empty list would read as "you
|
||||
* own no boards" and hide every workflow the user has.
|
||||
*
|
||||
* @return array<int, array{id: int, title: string, archived: bool}>
|
||||
* @throws DeckUnavailableException
|
||||
*/
|
||||
public function listBoardsForCurrentUser(string $userId): array {
|
||||
$boards = $this->call(function () {
|
||||
$boardService = $this->resolve(BoardService::class);
|
||||
try {
|
||||
// getUserBoards(?int $since, bool $includeArchived, …): with
|
||||
// the default $includeArchived = true, Deck skips the
|
||||
// `archived = false AND deleted_at = 0` conditions entirely,
|
||||
// so archived *and* trashed boards come back. Ask for the
|
||||
// filtered query instead of sorting them out afterwards.
|
||||
return $boardService->getUserBoards(null, false);
|
||||
} catch (Throwable $e) {
|
||||
// Older/newer Deck with a different signature: fall back to
|
||||
// the unfiltered call, isBoardHidden() below still filters.
|
||||
return $boardService->getUserBoards();
|
||||
}
|
||||
}, []);
|
||||
public function listOwnedBoards(string $userId): array {
|
||||
$boards = $this->call(
|
||||
fn () => $this->resolve(BoardMapper::class)->findAllByOwner($userId),
|
||||
[],
|
||||
true,
|
||||
);
|
||||
|
||||
// Deck merges own/group/circle boards, so the same board can come
|
||||
// back more than once; boards in the trash or archived ones are no
|
||||
// useful automation target, and shared ones are not allowed. Keyed
|
||||
// by id => deduplicated.
|
||||
$result = [];
|
||||
foreach ($boards as $board) {
|
||||
$id = (int)$board->getId();
|
||||
if (isset($result[$id]) || $this->isBoardHidden($board) || $this->unwrapUid($board->getOwner()) !== $userId) {
|
||||
if (method_exists($board, 'getDeletedAt') && (int)$board->getDeletedAt() > 0) {
|
||||
continue;
|
||||
}
|
||||
$id = (int)$board->getId();
|
||||
$title = $board->getTitle();
|
||||
$result[$id] = [
|
||||
$result[] = [
|
||||
'id' => $id,
|
||||
'title' => (is_string($title) && $title !== '') ? $title : ('#' . $id),
|
||||
'archived' => method_exists($board, 'getArchived') && (bool)$board->getArchived(),
|
||||
];
|
||||
}
|
||||
|
||||
return array_values($result);
|
||||
}
|
||||
|
||||
private function isBoardHidden(mixed $board): bool {
|
||||
if (method_exists($board, 'getDeletedAt') && (int)$board->getDeletedAt() > 0) {
|
||||
return true;
|
||||
}
|
||||
return method_exists($board, 'getArchived') && $board->getArchived() === true;
|
||||
return $result;
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -297,6 +282,37 @@ class DeckIntegrationService {
|
||||
return array_map(static fn ($stack) => (int)$stack->getId(), $stacks);
|
||||
}
|
||||
|
||||
/**
|
||||
* The uid of the board's current owner — the user a workflow on that
|
||||
* board belongs to. Workflows follow the board: when Deck transfers its
|
||||
* ownership, they move to the new owner along with it.
|
||||
*
|
||||
* Read through BoardMapper, a plain QBMapper without user state, so it
|
||||
* answers the same in a request and in cron. Trashed and archived boards
|
||||
* still report their owner. `null` only if the board row is gone; every
|
||||
* other failure throws, like findStackIds().
|
||||
*
|
||||
* @throws DeckUnavailableException
|
||||
*/
|
||||
public function findBoardOwner(int $boardId): ?string {
|
||||
try {
|
||||
$board = $this->resolve(BoardMapper::class)->find($boardId);
|
||||
} catch (DeckUnavailableException $e) {
|
||||
throw $e;
|
||||
} catch (Throwable $e) {
|
||||
if ($this->isMissing($e)) {
|
||||
return null;
|
||||
}
|
||||
throw new DeckUnavailableException('Could not read owner of board ' . $boardId . ': ' . $e->getMessage(), 0, $e);
|
||||
}
|
||||
|
||||
$owner = $this->unwrapUid($board->getOwner());
|
||||
if ($owner === null) {
|
||||
throw new DeckUnavailableException('Board ' . $boardId . ' has no readable owner');
|
||||
}
|
||||
return $owner;
|
||||
}
|
||||
|
||||
/**
|
||||
* Deck's own exception classes are referenced by name: `is_a()` with a
|
||||
* string simply returns false when the class does not exist, so a missing
|
||||
|
||||
@@ -55,7 +55,9 @@ class WorkflowRunner {
|
||||
}
|
||||
|
||||
$byUser = [];
|
||||
$owners = [];
|
||||
foreach ($workflows as $workflow) {
|
||||
$this->followBoardOwner($workflow, $owners);
|
||||
$byUser[$workflow->getUserId()][] = $workflow;
|
||||
}
|
||||
|
||||
@@ -78,6 +80,55 @@ class WorkflowRunner {
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* A workflow belongs to its board's owner, not to whoever created it: if
|
||||
* Deck transferred the board, the workflow moves along to the new owner —
|
||||
* it then runs as them, and they get its mails.
|
||||
*
|
||||
* Only a known owner rewrites `user_id`. A board that is gone keeps its
|
||||
* last owner, so findBrokenTarget() can disable the workflow and mail
|
||||
* them; a Deck error keeps it too, and the run just goes ahead as before.
|
||||
*
|
||||
* @param array<int, ?string> $owners per-run cache, board id => owner
|
||||
*/
|
||||
private function followBoardOwner(Workflow $workflow, array &$owners): void {
|
||||
$boardId = $workflow->getBoardId();
|
||||
if (!array_key_exists($boardId, $owners)) {
|
||||
try {
|
||||
$owners[$boardId] = $this->deckService->findBoardOwner($boardId);
|
||||
} catch (Throwable $e) {
|
||||
$this->logger->warning('Could not determine the owner of board {board}: ' . $e->getMessage(), [
|
||||
'app' => 'workflow_deck_automation',
|
||||
'board' => $boardId,
|
||||
]);
|
||||
$owners[$boardId] = null;
|
||||
}
|
||||
}
|
||||
|
||||
$owner = $owners[$boardId];
|
||||
if ($owner === null || $owner === $workflow->getUserId()) {
|
||||
return;
|
||||
}
|
||||
|
||||
$this->logger->info('Workflow {id} follows board {board} to its new owner {owner}.', [
|
||||
'app' => 'workflow_deck_automation',
|
||||
'id' => $workflow->getId(),
|
||||
'board' => $boardId,
|
||||
'owner' => $owner,
|
||||
]);
|
||||
$workflow->setUserId($owner);
|
||||
try {
|
||||
$this->workflowMapper->update($workflow);
|
||||
} catch (Throwable $e) {
|
||||
// Not persisted, but this run still goes to the right user; the
|
||||
// next one tries again.
|
||||
$this->logger->warning('Could not store the new owner of workflow {id}: ' . $e->getMessage(), [
|
||||
'app' => 'workflow_deck_automation',
|
||||
'id' => $workflow->getId(),
|
||||
]);
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* @param Workflow[] $workflows
|
||||
*/
|
||||
|
||||
@@ -175,8 +175,10 @@ const loadedBoardId = ref(null)
|
||||
|
||||
const collator = new Intl.Collator(undefined, { numeric: true, sensitivity: 'base' })
|
||||
|
||||
// The backend only returns boards the user owns.
|
||||
// The backend only returns boards the user owns. Archived ones are in there
|
||||
// so their workflows keep a board name, but are no target for new ones.
|
||||
const boardOptions = computed(() => boards.value
|
||||
.filter((board) => !board.archived || board.id === form.boardId)
|
||||
.map((board) => ({ value: board.id, label: board.title }))
|
||||
.sort((a, b) => collator.compare(a.label, b.label)))
|
||||
|
||||
@@ -230,7 +232,7 @@ const workflowIssues = computed(() => {
|
||||
|
||||
for (const workflow of workflows.value) {
|
||||
if (!boards.value.some((board) => board.id === workflow.boardId)) {
|
||||
issues[workflow.id] = 'Board nicht mehr vorhanden oder nicht dein eigenes'
|
||||
issues[workflow.id] = 'Board nicht mehr vorhanden'
|
||||
continue
|
||||
}
|
||||
|
||||
|
||||
Reference in new issue
Block a user