Run Deck as the workflow owner, and drop the job interval

Workflows were being switched off with "its board no longer exists or is
no longer available to you" for boards that were perfectly intact.

Deck does not read the session for permissions. PermissionService -- the
class behind every check, including the ones inside CardService::reorder()
-- takes the current user as a plain `private ?string $userId`, filled
from the app container's `userId` service (ISession::get('user_id'),
registered shared). Pimple resolves that once per process and caches it,
and ServerContainer caches Deck's app container just as long. In cron the
value is null whenever a Deck-owned job ran earlier in the same pass, and
otherwise the first workflow owner touched -- never the user being
impersonated. null fails every check, so Deck answered NoPermissionException
for an untouched board and findStackIds() read that as "the board is gone".

IUserSession::setUser() never had any effect on this path.

- DeckIntegrationService::beginUserContext()/endUserContext() pin
  PermissionService (mandatory), CardService, BoardService and
  ActivityManager (best effort) to the workflow owner, and restore them
  afterwards. If PermissionService cannot be pinned, the runner skips that
  user instead of acting under someone else's permissions.
- findStackIds() now takes the uid as an argument and is assembled from
  pieces that cannot answer for the wrong user: BoardMapper and StackMapper
  carry no user state, and getPermissions() is handed the uid explicitly.
  It no longer goes through StackService::findAll().
- NoPermissionException is no longer treated as "board missing". Unknown
  failures throw, which leaves the workflow enabled.

This also fixes the second half of the same defect: card moves silently
failed for every user except the first one processed in a cron pass.

Separately, RunWorkflowsJob is now a plain Job instead of a TimedJob and
runs on every cron pass. The workflow_deck_automation.interval config key
is gone. Time sensitivity is no longer merely declared but structurally
unreachable: JobList::add() leaves the column at its TIME_SENSITIVE
default, and the ratchet in setLastRun() only fires for TimedJob.

lib/Service/DeckIntegrationService.php

@@ -4,10 +4,13 @@ declare(strict_types=1);
 
 namespace OCA\WorkflowDeckAutomation\Service;
 
+use OCA\Deck\Db\BoardMapper;
 use OCA\Deck\Db\Card;
 use OCA\Deck\Db\CardMapper;
+use OCA\Deck\Db\StackMapper;
 use OCA\Deck\Service\BoardService;
 use OCA\Deck\Service\CardService;
+use OCA\Deck\Service\PermissionService;
 use OCA\Deck\Service\StackService;
 use OCP\App\IAppManager;
 use OCP\AppFramework\Db\DoesNotExistException;
@@ -16,6 +19,7 @@ use OCP\IUser;
 use OCP\IUserManager;
 use OCP\Server;
 use Psr\Log\LoggerInterface;
+use ReflectionProperty;
 use Throwable;
 
 /**
@@ -39,6 +43,36 @@ class DeckIntegrationService {
 	private const ACL_TYPE_USER = 0;
 	private const ACL_TYPE_GROUP = 1;
 
+	/**
+	 * Deck's Acl::PERMISSION_READ, inlined for the same reason. It is the key
+	 * under which PermissionService::getPermissions() reports read access.
+	 */
+	private const ACL_PERMISSION_READ = 0;
+
+	/**
+	 * The property through which Deck carries "the current user" — a plain
+	 * string frozen at construction rather than a session lookup. See
+	 * beginUserContext().
+	 */
+	private const IDENTITY_PROPERTY = 'userId';
+
+	/**
+	 * Further Deck classes holding that same frozen uid. Unlike
+	 * PermissionService these do not decide access, only whose name ends up
+	 * on an activity entry or whose unread-comment counts are computed, so
+	 * failing to pin them is logged and shrugged off.
+	 */
+	private const OPTIONAL_IDENTITY_CLASSES = [
+		CardService::class,
+		BoardService::class,
+		'OCA\Deck\Activity\ActivityManager',
+	];
+
+	/**
+	 * @var list<array{ReflectionProperty, object, ?string}>
+	 */
+	private array $identityRestore = [];
+
 	public function __construct(
 		private IAppManager $appManager,
 		private IUserManager $userManager,
@@ -47,6 +81,87 @@ class DeckIntegrationService {
 	) {
 	}
 
+	/**
+	 * Pins Deck to $user until endUserContext() — mandatory before any Deck
+	 * call made on behalf of someone who is not the logged-in web user.
+	 *
+	 * IUserSession::setUser() alone is not enough, and that is not obvious:
+	 * Deck's PermissionService — the class behind *every* permission check,
+	 * including the ones inside CardService::reorder() — receives the current
+	 * user as `private ?string $userId`. The app container fills that from
+	 * `ISession::get('user_id')` through a **shared** service, so Pimple
+	 * computes it once and caches it for the rest of the process, and
+	 * ServerContainer keeps the app container just as long. Whoever was
+	 * active when Deck's container first built PermissionService is therefore
+	 * the user every later check is evaluated against.
+	 *
+	 * In the background job that is never the user we are impersonating: it
+	 * is `null` whenever a Deck-owned job ran earlier in the same cron pass
+	 * (cron.php works through many jobs in one process), and otherwise the
+	 * first workflow owner we happened to touch. `null` fails every check,
+	 * which is how intact boards started reporting NoPermissionException and
+	 * got their workflows switched off.
+	 *
+	 * Deck exposes no API for this, so the frozen value is swapped directly
+	 * and put back in endUserContext(). PermissionService is required: if it
+	 * cannot be pinned this throws, and the caller has to skip the user
+	 * rather than run them under someone else's permissions.
+	 *
+	 * @throws DeckUnavailableException
+	 */
+	public function beginUserContext(IUser $user): void {
+		if ($this->identityRestore !== []) {
+			throw new DeckUnavailableException('A Deck user context is already active');
+		}
+
+		$uid = $user->getUID();
+
+		try {
+			$this->pinDeckIdentity($this->resolve(PermissionService::class), $uid);
+		} catch (Throwable $e) {
+			$this->endUserContext();
+			throw new DeckUnavailableException(
+				'Could not run Deck as ' . $uid . ': ' . $e->getMessage(),
+				0,
+				$e,
+			);
+		}
+
+		foreach (self::OPTIONAL_IDENTITY_CLASSES as $class) {
+			try {
+				$this->pinDeckIdentity($this->resolve($class), $uid);
+			} catch (Throwable $e) {
+				$this->logger->debug('Could not pin ' . $class . ' to ' . $uid . ': ' . $e->getMessage(), [
+					'app' => 'workflow_deck_automation',
+				]);
+			}
+		}
+	}
+
+	/**
+	 * Restores what beginUserContext() replaced. Idempotent, so it can be
+	 * called unconditionally from a finally block.
+	 */
+	public function endUserContext(): void {
+		foreach (array_reverse($this->identityRestore) as [$property, $service, $previous]) {
+			try {
+				$property->setValue($service, $previous);
+			} catch (Throwable $e) {
+				$this->logger->warning('Could not restore Deck identity on ' . $service::class . ': ' . $e->getMessage(), [
+					'app' => 'workflow_deck_automation',
+				]);
+			}
+		}
+		$this->identityRestore = [];
+	}
+
+	private function pinDeckIdentity(object $service, string $uid): void {
+		$property = new ReflectionProperty($service, self::IDENTITY_PROPERTY);
+		$previous = $property->isInitialized($service) ? $property->getValue($service) : null;
+		$property->setValue($service, $uid);
+		$this->identityRestore[] = [$property, $service, is_string($previous) ? $previous : null];
+	}
+
 	public function assertDeckAvailable(IUser $user): void {
 		if (!$this->appManager->isEnabledForUser(self::DECK_APP_ID, $user)) {
 			throw new DeckUnavailableException('Deck is not enabled for user ' . $user->getUID());
@@ -59,12 +174,11 @@ class DeckIntegrationService {
 	/**
 	 * @return array<int, array{id: int, title: string}>
 	 *
-	 * Only ever call this from a *request*: Deck injects the current user id
-	 * into BoardService as a plain string frozen at construction time, so in
-	 * the background job (one process, many users, a container-cached
-	 * BoardService) this would answer for the wrong user — or for none at
-	 * all. The job uses findStackIds() instead, which goes through Deck's
-	 * PermissionService and therefore reads the live session.
+	 * 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.
 	 */
 	public function listBoardsForCurrentUser(): array {
 		$boards = $this->call(function () {
@@ -127,23 +241,46 @@ class DeckIntegrationService {
 	 * Ids of the board's stacks, for deciding whether a stored workflow still
 	 * points at anything real.
 	 *
-	 * Returns `null` when the board itself is gone or not readable for the
-	 * current user — `StackService::findAll()` runs a Deck permission check
-	 * first, and that one reads the *live* session, which is what makes this
-	 * usable from the impersonating background job. Any other failure throws,
+	 * Returns `null` only on positive knowledge that the board is gone, in the
+	 * trash, or no longer readable for `$userId`. Every other failure throws,
 	 * because "Deck is broken right now" must never be mistaken for "the user
-	 * deleted this board".
+	 * deleted this board" — the caller reacts by switching the workflow off
+	 * and mailing its owner.
+	 *
+	 * Deliberately assembled from pieces that cannot answer for the wrong
+	 * user. BoardMapper and StackMapper are plain QBMappers with no user
+	 * state (StackMapper::findAll() filters `deleted_at = 0` itself), and
+	 * getPermissions() is handed the uid explicitly instead of falling back
+	 * to PermissionService's construction-time `$userId`. Leaning on that
+	 * frozen value — which is exactly what `StackService::findAll()` does
+	 * internally — is what disabled workflows whose board was perfectly
+	 * intact, so this must not be "simplified" back to one service call even
+	 * once beginUserContext() has pinned the identity.
 	 *
 	 * @return int[]|null
 	 * @throws DeckUnavailableException
 	 */
-	public function findStackIds(int $boardId): ?array {
+	public function findStackIds(int $boardId, string $userId): ?array {
 		try {
-			$stacks = $this->resolve(StackService::class)->findAll($boardId);
+			$board = $this->resolve(BoardMapper::class)->find($boardId);
+			if (method_exists($board, 'getDeletedAt') && (int)$board->getDeletedAt() > 0) {
+				return null;
+			}
+
+			$permissions = $this->resolve(PermissionService::class)->getPermissions($boardId, $userId);
+			if (!array_key_exists(self::ACL_PERMISSION_READ, $permissions)) {
+				// Unknown shape: say nothing rather than something wrong.
+				throw new DeckUnavailableException('Deck reported no read permission for board ' . $boardId);
+			}
+			if ($permissions[self::ACL_PERMISSION_READ] !== true) {
+				return null;
+			}
+
+			$stacks = $this->resolve(StackMapper::class)->findAll($boardId);
 		} catch (DeckUnavailableException $e) {
 			throw $e;
 		} catch (Throwable $e) {
-			if ($this->isMissingOrForbidden($e)) {
+			if ($this->isMissing($e)) {
 				return null;
 			}
 			$this->logger->error('Could not list stacks of board ' . $boardId . ': ' . $e->getMessage(), [
@@ -160,17 +297,18 @@ class DeckIntegrationService {
 	 * 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
 	 * or renamed Deck degrades to "unknown error" instead of fataling.
+	 *
+	 * `OCA\Deck\NoPermissionException` is deliberately *not* treated as "the
+	 * board is gone" here. It used to be, and that is what switched intact
+	 * workflows off: Deck raises it for any check that fails, including every
+	 * check evaluated against the wrong frozen uid — which says nothing at
+	 * all about whether the board still exists.
 	 */
-	private function isMissingOrForbidden(Throwable $e): bool {
+	private function isMissing(Throwable $e): bool {
 		if ($e instanceof DoesNotExistException) {
 			return true;
 		}
-		foreach (['OCA\Deck\NoPermissionException', 'OCA\Deck\NotFoundException'] as $class) {
-			if (is_a($e, $class)) {
-				return true;
-			}
-		}
-		return false;
+		return is_a($e, 'OCA\Deck\NotFoundException');
 	}
 
 	/**

lib/Service/WorkflowRunner.php

@@ -18,12 +18,15 @@ use Throwable;
 /**
  * Evaluates all enabled workflows and moves overdue cards.
  *
- * Deck's permission checks read the *live* user session, not a value
- * frozen at construction, and this runner has to evaluate rules for many
- * different users within a single background-job process. So workflows
- * are grouped by owner and, for each owner, we temporarily impersonate
- * that user (push their IUser onto the session, restore afterwards) —
- * the standard Nextcloud pattern for multi-user cron jobs.
+ * This runner has to evaluate rules for many different users within a
+ * single background-job process, so workflows are grouped by owner and
+ * each owner is impersonated in turn. Impersonation here means two
+ * things, and the second one is the load-bearing half: pushing the IUser
+ * onto the session (the standard Nextcloud cron pattern), *and* pinning
+ * Deck itself to that user via
+ * DeckIntegrationService::beginUserContext(), because Deck's permission
+ * checks never consult the session — they read a uid frozen once per
+ * process. Both are undone in a finally.
  */
 class WorkflowRunner {
 	public const TARGET_BOARD = 'board';
@@ -93,7 +96,20 @@ class WorkflowRunner {
 			return;
 		}
 
-		$this->impersonate($user);
+		try {
+			$this->impersonate($user);
+		} catch (DeckUnavailableException $e) {
+			// Deck could not be pinned to this user, so every permission
+			// check would be answered for somebody else. Skipping costs one
+			// run; continuing would move the wrong cards or, worse, read a
+			// denial as "the board is gone" and disable a working workflow.
+			$this->logger->error('Could not impersonate {user} towards Deck: ' . $e->getMessage(), [
+				'app' => 'workflow_deck_automation',
+				'user' => $userId,
+			]);
+			return;
+		}
+
 		try {
 			foreach ($workflows as $workflow) {
 				// Same reasoning as in run(): one broken workflow must not
@@ -115,7 +131,7 @@ class WorkflowRunner {
 
 	private function runWorkflow(IUser $user, Workflow $workflow): void {
 		try {
-			$brokenTarget = $this->findBrokenTarget($workflow);
+			$brokenTarget = $this->findBrokenTarget($user, $workflow);
 		} catch (DeckUnavailableException $e) {
 			// Could not determine it either way — leave the workflow alone
 			// rather than disabling it over a temporary Deck problem.
@@ -173,8 +189,8 @@ class WorkflowRunner {
 	 * @return self::TARGET_*|null
 	 * @throws DeckUnavailableException if Deck could not be asked
 	 */
-	private function findBrokenTarget(Workflow $workflow): ?string {
-		$stackIds = $this->deckService->findStackIds($workflow->getBoardId());
+	private function findBrokenTarget(IUser $user, Workflow $workflow): ?string {
+		$stackIds = $this->deckService->findStackIds($workflow->getBoardId(), $user->getUID());
 		if ($stackIds === null) {
 			return self::TARGET_BOARD;
 		}
@@ -257,6 +273,9 @@ class WorkflowRunner {
 		return true;
 	}
 
+	/**
+	 * @throws DeckUnavailableException if Deck cannot be pinned to $user
+	 */
 	private function impersonate(IUser $user): void {
 		$this->impersonationRestore = $this->userSession->getUser();
 		$this->userSession->setUser($user);
@@ -269,9 +288,20 @@ class WorkflowRunner {
 				'app' => 'workflow_deck_automation',
 			]);
 		}
+
+		try {
+			// The session switch above is necessary but nowhere near
+			// sufficient — Deck's permission checks never look at it. See
+			// DeckIntegrationService::beginUserContext().
+			$this->deckService->beginUserContext($user);
+		} catch (DeckUnavailableException $e) {
+			$this->clearImpersonation();
+			throw $e;
+		}
 	}
 
 	private function clearImpersonation(): void {
+		$this->deckService->endUserContext();
 		$this->userSession->setUser($this->impersonationRestore);
 		$this->impersonationRestore = null;
 	}