From 19245102694f10c5af07d48c14da13aaa5e4c65b Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Sat, 12 Sep 2026 11:26:28 +0200 Subject: [PATCH 1/9] feat(archive): add per-user archive schema, entity and mapper Add the tables_archive_user table and the contexts.archived column, the UserArchive entity and mapper, and the archived flag on the Table and Context entities. Non-owner archive decisions are stored as per-user override rows, owner decisions live on the entity flag. Assisted-by: ClaudeCode:claude-fable-5 Signed-off-by: Andy Scherzinger --- lib/Db/Context.php | 7 +- lib/Db/ContextMapper.php | 1 + lib/Db/Table.php | 1 + lib/Db/UserArchive.php | 35 +++ lib/Db/UserArchiveMapper.php | 206 ++++++++++++++++++ .../Version2500Date20261031000000.php | 87 ++++++++ 6 files changed, 336 insertions(+), 1 deletion(-) create mode 100644 lib/Db/UserArchive.php create mode 100644 lib/Db/UserArchiveMapper.php create mode 100644 lib/Migration/Version2500Date20261031000000.php diff --git a/lib/Db/Context.php b/lib/Db/Context.php index 3fa14f56b5..40bf6ba0b1 100644 --- a/lib/Db/Context.php +++ b/lib/Db/Context.php @@ -21,6 +21,8 @@ * @method setOwnerId(string $value): void * @method getOwnerType(): int * @method setOwnerType(int $value): void + * @method isArchived(): bool + * @method setArchived(bool $value): void * * @method getSharing(): array * @method setSharing(array $value): void @@ -35,6 +37,7 @@ class Context extends EntitySuper implements JsonSerializable { protected ?string $description = null; protected ?string $ownerId = null; protected ?int $ownerType = null; + protected bool $archived = false; // virtual properties protected ?array $sharing = null; @@ -46,6 +49,7 @@ class Context extends EntitySuper implements JsonSerializable { public function __construct() { $this->addType('id', 'integer'); $this->addType('owner_type', 'integer'); + $this->addType('archived', 'boolean'); } public function jsonSerialize(): array { @@ -56,7 +60,8 @@ public function jsonSerialize(): array { 'iconName' => $this->getIcon(), 'description' => $this->getDescription(), 'owner' => $this->getOwnerId(), - 'ownerType' => $this->getOwnerType() + 'ownerType' => $this->getOwnerType(), + 'archived' => $this->isArchived(), ]; // extended data diff --git a/lib/Db/ContextMapper.php b/lib/Db/ContextMapper.php index 0da199335e..e3fcd68b98 100644 --- a/lib/Db/ContextMapper.php +++ b/lib/Db/ContextMapper.php @@ -84,6 +84,7 @@ protected function formatResultRows(array $rows, ?string $userId) { 'description' => $rows[0]['description'], 'owner_id' => $rows[0]['owner_id'], 'owner_type' => $rows[0]['owner_type'], + 'archived' => (bool)($rows[0]['archived'] ?? false), ]; $formatted['sharing'] = array_reduce($rows, function (array $carry, array $item) use ($userId) { diff --git a/lib/Db/Table.php b/lib/Db/Table.php index 74dafd7667..175aaad104 100644 --- a/lib/Db/Table.php +++ b/lib/Db/Table.php @@ -28,6 +28,7 @@ * @method getEmoji(): string * @method setEmoji(string $emoji) * @method getArchived(): bool + * @method isArchived(): bool * @method setArchived(bool $archived) * @method getDescription(): string * @method setDescription(string $description) diff --git a/lib/Db/UserArchive.php b/lib/Db/UserArchive.php new file mode 100644 index 0000000000..720645d517 --- /dev/null +++ b/lib/Db/UserArchive.php @@ -0,0 +1,35 @@ +addType('id', 'integer'); + $this->addType('nodeType', 'integer'); + $this->addType('nodeId', 'integer'); + $this->addType('archived', 'boolean'); + } +} diff --git a/lib/Db/UserArchiveMapper.php b/lib/Db/UserArchiveMapper.php new file mode 100644 index 0000000000..dee17ddc9c --- /dev/null +++ b/lib/Db/UserArchiveMapper.php @@ -0,0 +1,206 @@ + */ +class UserArchiveMapper extends QBMapper { + /** + * Oracle enforces a hard limit of 1000 items per IN clause, which is the + * chunk size every other mapper in this app uses. + */ + private const DB_CHUNK_SIZE = 1_000; + + protected string $table = 'tables_archive_user'; + + public function __construct(IDBConnection $db) { + parent::__construct($db, $this->table, UserArchive::class); + } + + /** + * Look up a single per-user archive override. + * + * @throws Exception + */ + public function findForUser(string $userId, int $nodeType, int $nodeId): ?UserArchive { + $qb = $this->db->getQueryBuilder(); + $qb->select('*') + ->from($this->table) + ->where($qb->expr()->eq('user_id', $qb->createNamedParameter($userId, IQueryBuilder::PARAM_STR))) + ->andWhere($qb->expr()->eq('node_type', $qb->createNamedParameter($nodeType, IQueryBuilder::PARAM_INT))) + ->andWhere($qb->expr()->eq('node_id', $qb->createNamedParameter($nodeId, IQueryBuilder::PARAM_INT))); + + $entities = $this->findEntities($qb); + return $entities[0] ?? null; + } + + /** + * Which of the given users hold an override on which of the given nodes. + * + * Answers for every user in one query rather than one query per user, which + * matters where the caller holds the whole inherited membership of a team. + * + * @param string[] $userIds users to restrict the lookup to + * @param int[] $nodeIds IDs to restrict the lookup to + * @return array> user IDs, keyed by node_id + * @throws Exception + */ + public function findOverrideUsersForNodes(array $userIds, int $nodeType, array $nodeIds): array { + $userIds = array_values(array_unique($userIds)); + $nodeIds = array_values(array_unique($nodeIds)); + if ($userIds === [] || $nodeIds === []) { + return []; + } + + $qb = $this->db->getQueryBuilder(); + $qb->select('user_id', 'node_id') + ->from($this->table) + ->where($qb->expr()->eq('node_type', $qb->createNamedParameter($nodeType, IQueryBuilder::PARAM_INT))) + ->andWhere($qb->expr()->in('user_id', $qb->createParameter('userChunk'))) + ->andWhere($qb->expr()->in('node_id', $qb->createParameter('nodeChunk'))); + + $userIdsByNode = []; + foreach (array_chunk($userIds, self::DB_CHUNK_SIZE) as $userChunk) { + $qb->setParameter('userChunk', $userChunk, IQueryBuilder::PARAM_STR_ARRAY); + foreach (array_chunk($nodeIds, self::DB_CHUNK_SIZE) as $nodeChunk) { + $qb->setParameter('nodeChunk', $nodeChunk, IQueryBuilder::PARAM_INT_ARRAY); + $result = $qb->executeQuery(); + while ($row = $result->fetchAssociative()) { + $userIdsByNode[(int)$row['node_id']][] = (string)$row['user_id']; + } + $result->closeCursor(); + } + } + + return $userIdsByNode; + } + + /** + * Fetch all per-user archive overrides for a given user and node type, + * filtered to a specific set of node IDs. + * + * Chunks $nodeIds into batches of DB_CHUNK_SIZE and merges results in + * PHP to stay within the Oracle IN-clause limit transparently. + * + * @param int[] $nodeIds IDs to restrict the lookup to + * @return array Keyed by node_id for O(1) map lookup + * @throws Exception + */ + public function findAllOverridesForUser(string $userId, int $nodeType, array $nodeIds): array { + if (empty($nodeIds)) { + return []; + } + $nodeIds = array_values(array_unique($nodeIds)); + + $results = []; + $qb = $this->db->getQueryBuilder(); + $qb->select('*') + ->from($this->table) + ->where($qb->expr()->eq('user_id', $qb->createNamedParameter($userId, IQueryBuilder::PARAM_STR))) + ->andWhere($qb->expr()->eq('node_type', $qb->createNamedParameter($nodeType, IQueryBuilder::PARAM_INT))) + ->andWhere($qb->expr()->in('node_id', $qb->createParameter('chunk'))); + + foreach (array_chunk($nodeIds, self::DB_CHUNK_SIZE) as $chunk) { + $qb->setParameter('chunk', $chunk, IQueryBuilder::PARAM_INT_ARRAY); + foreach ($this->findEntities($qb) as $entity) { + $results[$entity->getNodeId()] = $entity; + } + } + + return $results; + } + + /** + * Insert or update a per-user archive override. + * + * Uses IDBConnection::setValues() so insert-or-update happens as one + * portable atomic operation instead of a read-modify-write cycle. + * + * @throws Exception + */ + public function upsert(string $userId, int $nodeType, int $nodeId, bool $archived): void { + $this->db->setValues($this->table, [ + 'user_id' => $userId, + 'node_type' => $nodeType, + 'node_id' => $nodeId, + ], [ + 'archived' => $archived, + ]); + } + + /** + * Fetch the IDs of all users holding an archive override for a node. + * + * @return string[] + * @throws Exception + */ + public function findUserIdsForNode(int $nodeType, int $nodeId): array { + $qb = $this->db->getQueryBuilder(); + $qb->select('user_id') + ->from($this->table) + ->where($qb->expr()->eq('node_type', $qb->createNamedParameter($nodeType, IQueryBuilder::PARAM_INT))) + ->andWhere($qb->expr()->eq('node_id', $qb->createNamedParameter($nodeId, IQueryBuilder::PARAM_INT))); + + $result = $qb->executeQuery(); + $userIds = array_map(static fn (array $row) => (string)$row['user_id'], $result->fetchAllAssociative()); + $result->closeCursor(); + return $userIds; + } + + /** + * Remove every archive override a user holds, on any node. + * + * Called when the user account is deleted, so no orphaned rows remain + * for nodes owned by other users. + * + * @throws Exception + */ + public function deleteAllForUser(string $userId): void { + $qb = $this->db->getQueryBuilder(); + $qb->delete($this->table) + ->where($qb->expr()->eq('user_id', $qb->createNamedParameter($userId, IQueryBuilder::PARAM_STR))); + + $qb->executeStatement(); + } + + /** + * Remove the per-user archive override for a single user. + * + * @throws Exception + */ + public function deleteForUser(string $userId, int $nodeType, int $nodeId): void { + $qb = $this->db->getQueryBuilder(); + $qb->delete($this->table) + ->where($qb->expr()->eq('user_id', $qb->createNamedParameter($userId, IQueryBuilder::PARAM_STR))) + ->andWhere($qb->expr()->eq('node_type', $qb->createNamedParameter($nodeType, IQueryBuilder::PARAM_INT))) + ->andWhere($qb->expr()->eq('node_id', $qb->createNamedParameter($nodeId, IQueryBuilder::PARAM_INT))); + + $qb->executeStatement(); + } + + /** + * Remove all per-user archive overrides for a node, which only happens once + * the node itself is gone. An owner archiving or unarchiving sets the shared + * flag and leaves every personal choice standing. + * + * @throws Exception + */ + public function deleteAllForNode(int $nodeType, int $nodeId): void { + $qb = $this->db->getQueryBuilder(); + $qb->delete($this->table) + ->where($qb->expr()->eq('node_type', $qb->createNamedParameter($nodeType, IQueryBuilder::PARAM_INT))) + ->andWhere($qb->expr()->eq('node_id', $qb->createNamedParameter($nodeId, IQueryBuilder::PARAM_INT))); + + $qb->executeStatement(); + } +} diff --git a/lib/Migration/Version2500Date20261031000000.php b/lib/Migration/Version2500Date20261031000000.php new file mode 100644 index 0000000000..d2df33add5 --- /dev/null +++ b/lib/Migration/Version2500Date20261031000000.php @@ -0,0 +1,87 @@ +hasTable('tables_contexts_context')) { + $table = $schema->getTable('tables_contexts_context'); + if (!$table->hasColumn('archived')) { + $table->addColumn('archived', Types::BOOLEAN, [ + 'default' => false, + 'notnull' => true, + ]); + } + } + + // Step 2: Create `tables_archive_user` table for per-user archive overrides + if (!$schema->hasTable('tables_archive_user')) { + $table = $schema->createTable('tables_archive_user'); + $table->addColumn('id', Types::BIGINT, [ + 'notnull' => true, + 'autoincrement' => true, + 'unsigned' => true, + ]); + $table->addColumn('user_id', Types::STRING, [ + 'notnull' => true, + 'length' => 64, + ]); + $table->addColumn('node_type', Types::SMALLINT, [ + 'notnull' => true, + ]); + $table->addColumn('node_id', Types::BIGINT, [ + 'notnull' => true, + ]); + // `archived` = true means the user archived this node; + // `archived` = false means the user explicitly unarchived an owner-archived node. + // No index on the `archived` column: it is a low-cardinality boolean evaluated + // after a higher-selectivity filter (ownership / join) is already applied. + // Adding an index here would waste write overhead without measurable read benefit. + $table->addColumn('archived', Types::BOOLEAN, [ + 'notnull' => true, + 'default' => true, + ]); + $table->setPrimaryKey(['id']); + + // Unique index: one override row per (user, node_type, node_id) triple + $table->addUniqueIndex(['user_id', 'node_type', 'node_id'], 'archive_user_unique_idx'); + + // Secondary index to support deleteAllForNode() queries + // that filter on (node_type, node_id) without a leading user_id. + $table->addIndex(['node_type', 'node_id'], 'archive_user_node_idx'); + } + + return $schema; + } +} From 7889cf8db28ebe3e1b32f89f626ca41729fe963b Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Sat, 12 Sep 2026 11:26:39 +0200 Subject: [PATCH 2/9] feat(archive): add ArchiveService for owner and per-user archive logic Owner archiving toggles the shared entity flag inside a transaction and clears every per-user override; non-owners store or drop a personal override. The service also resolves the per-user flag onto entity collections and migrates overrides when ownership is transferred. Assisted-by: ClaudeCode:claude-fable-5 Signed-off-by: Andy Scherzinger --- lib/Db/EntitySuper.php | 21 ++++ lib/Service/ArchiveService.php | 220 +++++++++++++++++++++++++++++++++ 2 files changed, 241 insertions(+) create mode 100644 lib/Service/ArchiveService.php diff --git a/lib/Db/EntitySuper.php b/lib/Db/EntitySuper.php index ad2c90a53c..263c09a4b0 100644 --- a/lib/Db/EntitySuper.php +++ b/lib/Db/EntitySuper.php @@ -13,6 +13,27 @@ class EntitySuper extends Entity { protected const VIRTUAL_PROPERTIES = []; + /** + * Write a value that only describes the entity for the requesting user, such as + * the archived state resolved from their personal override. + * + * The column keeps its stored value for everyone else, so the write must not mark + * the field for persistence: the mappers cache an entity for the whole request and + * hand the same instance to every later consumer, one of which may well persist it. + * Whatever was pending before stays pending. + * + * @param string $property the entity property, as its setter names it + */ + public function setResolved(string $property, mixed $value): void { + $pendingFields = $this->getUpdatedFields(); + $this->{'set' . ucfirst($property)}($value); + + $this->resetUpdatedFields(); + foreach (array_keys($pendingFields) as $field) { + $this->markFieldUpdated($field); + } + } + protected function markFieldUpdated(string $attribute): void { if (in_array($attribute, static::VIRTUAL_PROPERTIES, true)) { return; diff --git a/lib/Service/ArchiveService.php b/lib/Service/ArchiveService.php new file mode 100644 index 0000000000..122e2c3b7c --- /dev/null +++ b/lib/Service/ArchiveService.php @@ -0,0 +1,220 @@ +setEntityArchived($nodeType, $nodeId, true); + } + } else { + $this->userArchiveMapper->upsert($userId, $nodeType, $nodeId, true); + } + } + + /** + * Unarchive a table or context for a user. + * + * The owner moves the entity-level flag, which is the default for users + * without a personal override. Existing overrides are left untouched: a + * user who archived it for themselves keeps it archived regardless of the + * owner. A non-owner removes their override when the entity is not + * owner-archived, or upserts an explicit unarchive override otherwise. + * + * @throws Exception + * @throws InternalError + */ + public function unarchiveForUser(string $userId, int $nodeType, int $nodeId, bool $isOwner, bool $entityArchived): void { + if ($isOwner) { + if ($entityArchived) { + $this->setEntityArchived($nodeType, $nodeId, false); + } + } elseif (!$entityArchived) { + // Entity is not owner-archived — just remove the personal override. + $this->userArchiveMapper->deleteForUser($userId, $nodeType, $nodeId); + } else { + // Entity is owner-archived — store an explicit unarchive override so + // the user's active list shows the item while the owner's state is + // preserved for everyone else. + $this->userArchiveMapper->upsert($userId, $nodeType, $nodeId, false); + } + } + + /** + * Overwrite the `archived` property on each table with the per-user + * resolved value for $userId. + * + * @param Table[] $tables + * @throws Exception + */ + public function enrichTablesWithArchiveState(array $tables, string $userId): void { + $this->overrideArchivedFlags($tables, $userId, Application::NODE_TYPE_TABLE); + } + + /** + * Overwrite the `archived` property on each context with the per-user + * resolved value for $userId. + * + * @param Context[] $contexts + * @throws Exception + */ + public function enrichContextsWithArchiveState(array $contexts, string $userId): void { + $this->overrideArchivedFlags($contexts, $userId, Application::NODE_TYPE_CONTEXT); + } + + /** + * Overwrite the `archived` property on each entity with the per-user + * resolved value for $userId. + * + * Uses a single bulk DB query (chunked for Oracle compatibility) regardless + * of how many entities are in the array. + * + * @param list $entities + * @throws Exception + */ + private function overrideArchivedFlags(array $entities, string $userId, int $nodeType): void { + $nodeIds = array_map(static fn (Table|Context $entity) => $entity->getId(), $entities); + $overrides = $this->userArchiveMapper->findAllOverridesForUser($userId, $nodeType, $nodeIds); + + foreach ($entities as $entity) { + $override = $overrides[$entity->getId()] ?? null; + $archived = $override !== null ? $override->isArchived() : $entity->isArchived(); + + // The resolved value describes the entity for this user only, so it must + // never be persisted: the mappers cache the entity for the whole request + // and a later update would write one user's state to the shared column. + $entity->setResolved('archived', $archived); + } + } + + /** + * Migrate per-user archive overrides when ownership is transferred. + * + * Two invariants are maintained: + * 1. If the incoming owner held a personal override, that value is promoted + * to the entity-level flag and their override row is deleted. + * 2. If the entity flag changes as a result, the outgoing owner receives a + * preservation record so their view is unchanged after the transfer. + * Callers must clean this up via removeUserOverride() when the transfer + * leaves the outgoing owner without access to the node. + * + * Must be called inside the same atomic block as the ownership-column + * update so that archive state and ownership are always consistent. + * + * Returns the new entity-level archived value. The caller must call + * `$entity->setArchived($newArchived)` when the returned value differs + * from $entityArchived, before persisting the entity. + * + * @throws Exception + */ + public function prepareOwnershipTransfer( + string $oldOwnerId, + string $newOwnerId, + int $nodeType, + int $nodeId, + bool $entityArchived, + ): bool { + $newOwnerOverride = $this->userArchiveMapper->findForUser($newOwnerId, $nodeType, $nodeId); + + if ($newOwnerOverride !== null) { + $newArchived = $newOwnerOverride->isArchived(); + // Remove the override — entity flag becomes authoritative for the new owner + $this->userArchiveMapper->deleteForUser($newOwnerId, $nodeType, $nodeId); + } else { + $newArchived = $entityArchived; + } + + // Preserve the outgoing owner's view if the entity flag will change. + // Any existing row is deleted first: this method runs inside the + // caller's transaction, where the insert-then-update-on-conflict + // recovery of setValues() would abort the transaction on PostgreSQL. + if ($newArchived !== $entityArchived) { + $this->userArchiveMapper->deleteForUser($oldOwnerId, $nodeType, $nodeId); + $this->userArchiveMapper->upsert($oldOwnerId, $nodeType, $nodeId, $entityArchived); + } + + return $newArchived; + } + + /** + * Remove a single user's archive override for a node. + * + * Used after ownership transfers when the outgoing owner no longer has + * access to the node, so no orphaned override rows are left behind. + * + * @throws Exception + */ + public function removeUserOverride(string $userId, int $nodeType, int $nodeId): void { + $this->userArchiveMapper->deleteForUser($userId, $nodeType, $nodeId); + } + + /** + * Remove all per-user archive overrides for a node. + * + * Called when a table or context is permanently deleted so that + * `tables_archive_user` does not accumulate orphaned rows. + * + * @throws Exception + */ + public function deleteNodeArchiveOverrides(int $nodeType, int $nodeId): void { + $this->userArchiveMapper->deleteAllForNode($nodeType, $nodeId); + } + + /** + * Directly set the entity-level `archived` flag for a table or context row. + * + * Intentionally bypasses TableService / ContextService to avoid a circular + * dependency: those services will eventually call ArchiveService, so + * ArchiveService must not call them back for this low-level write. + * + * @throws Exception + * @throws InternalError + */ + private function setEntityArchived(int $nodeType, int $nodeId, bool $archived): void { + $tableName = match ($nodeType) { + Application::NODE_TYPE_TABLE => 'tables_tables', + Application::NODE_TYPE_CONTEXT => 'tables_contexts_context', + default => throw new InternalError('Unsupported node type for archiving: ' . $nodeType), + }; + + $qb = $this->connection->getQueryBuilder(); + $qb->update($tableName) + ->set('archived', $qb->createNamedParameter($archived, IQueryBuilder::PARAM_BOOL)) + ->where($qb->expr()->eq('id', $qb->createNamedParameter($nodeId, IQueryBuilder::PARAM_INT))) + ->executeStatement(); + } +} From ccfd23ba205d93766b180dda0f81f98e62a4f662 Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Sat, 12 Sep 2026 11:26:39 +0200 Subject: [PATCH 3/9] feat(archive): apply per-user archive state in table and context services Enrich read paths with the per-user resolved archived flag, add archive/unarchive, keep the state consistent across ownership transfer and deletion, hide archived contexts from the navigation, and expose the archived field in the API response definitions. Assisted-by: ClaudeCode:claude-fable-5 Signed-off-by: Andy Scherzinger --- lib/AuditLog/Operation.php | 4 + lib/ResponseDefinitions.php | 1 + lib/Service/ContextService.php | 131 ++++++++- lib/Service/TableService.php | 255 ++++++++++++++++-- .../unit/Service/TableServiceAuditLogTest.php | 2 + 5 files changed, 368 insertions(+), 25 deletions(-) diff --git a/lib/AuditLog/Operation.php b/lib/AuditLog/Operation.php index e0bca428e7..7941cdd62c 100644 --- a/lib/AuditLog/Operation.php +++ b/lib/AuditLog/Operation.php @@ -27,6 +27,8 @@ enum Operation: string { case ColumnDeleted = 'tables.column.deleted'; case ImportCompleted = 'tables.import.completed'; case ContextCreated = 'tables.context.created'; + case ContextArchived = 'tables.context.archived'; + case ContextUnarchived = 'tables.context.unarchived'; case ContextUpdated = 'tables.context.updated'; case ContextDeleted = 'tables.context.deleted'; case ContextTransferred = 'tables.context.transferred'; @@ -54,6 +56,8 @@ public function description(): string { self::ColumnDeleted => 'A column was deleted with its values in all rows', self::ImportCompleted => 'Rows were imported into a table or view from a spreadsheet, possibly creating columns', self::ContextCreated => 'An application was created', + self::ContextArchived => 'An application was archived for all users it is shared with', + self::ContextUnarchived => 'An application was unarchived', self::ContextUpdated => 'An application was changed, including tables and views added to or removed from it and changes to their permissions in it, which exposes them to the people the application is shared with', self::ContextDeleted => 'An application was deleted with its pages and shares', self::ContextTransferred => 'The ownership of an application was transferred to another user', diff --git a/lib/ResponseDefinitions.php b/lib/ResponseDefinitions.php index dd41a0bf4f..7aba108c32 100644 --- a/lib/ResponseDefinitions.php +++ b/lib/ResponseDefinitions.php @@ -241,6 +241,7 @@ * description: string, * owner: string, * ownerType: int, + * archived: bool, * } * * @psalm-type TablesContextNavigation = array{ diff --git a/lib/Service/ContextService.php b/lib/Service/ContextService.php index 828897ad6f..9a77877548 100644 --- a/lib/Service/ContextService.php +++ b/lib/Service/ContextService.php @@ -57,11 +57,13 @@ public function __construct( private AuditLogServiceInterface $auditLogService, private IDBConnection $dbc, private ShareService $shareService, + private ArchiveCleanupService $archiveCleanupService, private bool $isCLI, protected INavigationManager $navigationManager, protected IURLGenerator $urlGenerator, private TableMapper $tableMapper, private ViewMapper $viewMapper, + private ArchiveService $archiveService, ) { } @@ -81,11 +83,28 @@ public function findAll(?string $userId): array { $this->logger->warning($error); throw new InternalError($error); } - return $this->contextMapper->findAll($userId); + $contexts = $this->contextMapper->findAll($userId); + if ($userId !== null) { + try { + $this->archiveService->enrichContextsWithArchiveState($contexts, $userId); + } catch (Exception $e) { + $this->logger->error($e->getMessage(), ['exception' => $e]); + } + } + return $contexts; } + /** + * @return Context[] + */ public function findForNavigation(string $userId): array { - return $this->contextMapper->findForNavBar($userId); + $contexts = $this->contextMapper->findForNavBar($userId); + try { + $this->archiveService->enrichContextsWithArchiveState($contexts, $userId); + } catch (Exception $e) { + $this->logger->error($e->getMessage(), ['exception' => $e]); + } + return array_values(array_filter($contexts, static fn (Context $context) => !$context->isArchived())); } public function addToNavigation(string $userId): void { @@ -126,7 +145,86 @@ public function findById(int $id, ?string $userId): Context { throw new InternalError($error); } - return $this->contextMapper->findById($id, $userId); + $context = $this->contextMapper->findById($id, $userId); + if ($userId !== null) { + try { + $this->archiveService->enrichContextsWithArchiveState([$context], $userId); + } catch (Exception $e) { + $this->logger->error($e->getMessage(), ['exception' => $e]); + } + } + return $context; + } + + /** + * Archive a context for the given user. + * + * If the user is the owner the entity flag is set and all per-user + * overrides are cleared; otherwise a personal override is stored. + * Access is validated by the mapper (NotFoundError if no access). + * + * @throws Exception + * @throws NotFoundError + * @throws InternalError + */ + public function archiveContext(int $contextId, string $userId): Context { + return $this->setArchivedForUser($contextId, $userId, true); + } + + /** + * Unarchive a context for the given user. + * + * If the user is the owner the entity flag is cleared and all per-user + * overrides are reset; otherwise the personal override is removed or + * set to false if the owner has archived the context. + * Access is validated by the mapper (NotFoundError if no access). + * + * @throws Exception + * @throws NotFoundError + * @throws InternalError + */ + public function unarchiveContext(int $contextId, string $userId): Context { + return $this->setArchivedForUser($contextId, $userId, false); + } + + /** + * Shared implementation of archiveContext() and unarchiveContext(). + * + * An owner-level change alters what every user with access sees, so it is + * recorded in the audit log. A personal override is invisible to others + * and stays silent. + * + * @throws Exception + * @throws NotFoundError + * @throws InternalError + */ + private function setArchivedForUser(int $contextId, string $userId, bool $archived): Context { + // Load directly from mapper to get entity-level archived (bypasses per-user enrichment) + $context = $this->contextMapper->findById($contextId, $userId); + $isOwner = $context->getOwnerId() === $userId; + $entityArchived = $context->isArchived(); // entity-level flag, not per-user + + try { + if ($archived) { + $this->archiveService->archiveForUser($userId, Application::NODE_TYPE_CONTEXT, $contextId, $isOwner, $entityArchived); + } else { + $this->archiveService->unarchiveForUser($userId, Application::NODE_TYPE_CONTEXT, $contextId, $isOwner, $entityArchived); + } + } catch (Exception $e) { + $this->logger->error($e->getMessage(), ['exception' => $e]); + throw new InternalError(static::class . ' - ' . __FUNCTION__ . ': ' . $e->getMessage()); + } + + // a per-user override stays private, so only an owner-level flip is audited + if ($isOwner && $entityArchived !== $archived) { + $this->auditLogService->log( + $archived ? Operation::ContextArchived : Operation::ContextUnarchived, + $archived ? 'Tables application with ID %s was archived' : 'Tables application with ID %s was unarchived', + ['contextId' => $contextId], + ); + } + + return $this->findById($contextId, $userId); } /** @@ -327,6 +425,14 @@ public function update(int $contextId, string $userId, ?string $name, ?string $i 'updatedNodes' => $updatedNodes, ]); + // The response must report the requesting user's archive state, not the + // shared flag, exactly as the read paths and TableService::update() do. + try { + $this->archiveService->enrichContextsWithArchiveState([$context], $userId); + } catch (Exception $e) { + $this->logger->error($e->getMessage(), ['exception' => $e]); + } + return $context; } @@ -346,6 +452,7 @@ public function delete(int $contextId, string $userId): Context { $this->pageMapper->deleteByPageId($pageId); } $this->contextMapper->delete($context); + $this->archiveService->deleteNodeArchiveOverrides(Application::NODE_TYPE_CONTEXT, $context->getId()); }, $this->dbc); $this->auditLogService->log(Operation::ContextDeleted, 'Tables application with ID %s was deleted', [ 'contextId' => $context->getId(), @@ -386,13 +493,21 @@ public function transfer(int $contextId, string $newOwnerId, int $newOwnerType): } $oldOwnerId = $context->getOwnerId(); + $oldArchived = $context->isArchived(); $context->setOwnerId($newOwnerId); $context->setOwnerType($newOwnerType); try { - $context = $this->atomic(function () use ($context, $contextId, $newOwnerId, $oldOwnerId) { + $context = $this->atomic(function () use ($context, $contextId, $newOwnerId, $oldOwnerId, $oldArchived) { + $newArchived = $this->archiveService->prepareOwnershipTransfer( + $oldOwnerId, $newOwnerId, Application::NODE_TYPE_CONTEXT, $contextId, $oldArchived + ); + if ($newArchived !== $oldArchived) { + $context->setArchived($newArchived); + } $context = $this->contextMapper->update($context); $this->shareService->transferSharesForContext($contextId, $newOwnerId, $oldOwnerId); + $this->archiveCleanupService->removeOverrideIfStale($oldOwnerId, Application::NODE_TYPE_CONTEXT, $contextId); return $context; }, $this->dbc); } catch (\Exception $e) { @@ -406,6 +521,14 @@ public function transfer(int $contextId, string $newOwnerId, int $newOwnerType): 'fromUserId' => $oldOwnerId, ]); + // Only the owner may transfer, and the transfer may have just written them a + // preservation override, so the response has to carry their own state. + try { + $this->archiveService->enrichContextsWithArchiveState([$context], $oldOwnerId); + } catch (Exception $e) { + $this->logger->error($e->getMessage(), ['exception' => $e]); + } + return $context; } diff --git a/lib/Service/TableService.php b/lib/Service/TableService.php index 11d2ca1173..88f0f2acac 100644 --- a/lib/Service/TableService.php +++ b/lib/Service/TableService.php @@ -75,6 +75,7 @@ public function __construct( private ActivityManager $activityManager, private FederationService $federationService, private AuditLogServiceInterface $auditLogService, + private ArchiveService $archiveService, ) { parent::__construct($logger, $userId, $permissionsService); } @@ -159,6 +160,14 @@ public function findAll(?string $userId = null, bool $skipTableEnhancement = fal } } + if ($userId !== '') { + try { + $this->archiveService->enrichTablesWithArchiveState(array_values($allTables), $userId); + } catch (OcpDbException $e) { + $this->logger->error($e->getMessage(), ['exception' => $e]); + } + } + return array_values($allTables); } @@ -279,6 +288,160 @@ public function find(int $id, bool $skipTableEnhancement = false, ?string $userI } } + /** + * Fetch a single table and resolve the per-user `archived` flag. + * + * Use this instead of `find()` when the caller needs the correct per-user + * archive state (e.g. GET /tables/{id} API endpoints). + * + * @throws InternalError + * @throws NotFoundError + * @throws PermissionError + */ + public function getTableForUser(int $id, string $userId): Table { + $table = $this->find($id, false, $userId); + try { + $this->archiveService->enrichTablesWithArchiveState([$table], $userId); + } catch (OcpDbException $e) { + $this->logger->error($e->getMessage(), ['exception' => $e]); + } + return $table; + } + + /** + * Apply the legacy `archived` flag of the update endpoints with the same + * per-user semantics the dedicated archive endpoints use. + * + * An owner toggles the entity-level flag, which everyone who never made a + * choice of their own follows; a stored override keeps its user on their + * own choice. Anyone else only changes their own override and leaves the + * shared flag alone. + * + * Returns whether this touched the shared table, i.e. an owner actually + * flipped the entity flag. A non-owner's private override, or an owner + * repeating the current state, changes nothing anyone else can observe and + * must not surface as a shared edit (bumped metadata, activity, federation). + * + * @throws InternalError + */ + private function applyArchivedOnUpdate(Table $table, bool $archived, string $userId): bool { + $nodeId = $table->getId(); + $entityArchived = $table->isArchived(); + // an empty user id is a CLI call (occ) and acts with owner-level rights + $isOwner = $userId === '' || $table->getOwnership() === $userId; + try { + if ($archived) { + $this->archiveService->archiveForUser($userId, Application::NODE_TYPE_TABLE, $nodeId, $isOwner, $entityArchived); + } else { + $this->archiveService->unarchiveForUser($userId, Application::NODE_TYPE_TABLE, $nodeId, $isOwner, $entityArchived); + } + } catch (OcpDbException $e) { + $this->logger->error($e->getMessage(), ['exception' => $e]); + throw new InternalError(static::class . ' - ' . __FUNCTION__ . ': ' . $e->getMessage()); + } + + if ($isOwner) { + // ArchiveService already wrote the shared flag; keep the in-memory + // entity consistent for the response and the activity diff. + $table->setArchived($archived); + return $entityArchived !== $archived; + } + return false; + } + + /** + * Archive a table for the given user. + * + * If the user is the owner the entity flag is set and all per-user + * overrides are cleared; otherwise a personal override is stored. + * + * @throws InternalError + * @throws NotFoundError + * @throws PermissionError + */ + public function archiveTable(int $id, string $userId): Table { + return $this->setArchivedForUser($id, $userId, true); + } + + /** + * Unarchive a table for the given user. + * + * If the user is the owner the entity flag is cleared and all per-user + * overrides are reset; otherwise the personal override is removed or + * set to false if the owner has archived the table. + * + * @throws InternalError + * @throws NotFoundError + * @throws PermissionError + */ + public function unarchiveTable(int $id, string $userId): Table { + return $this->setArchivedForUser($id, $userId, false); + } + + /** + * Shared implementation of archiveTable() and unarchiveTable(). + * + * An owner changes the shared flag, which is a change to the table like + * any other: it bumps the edit metadata, reaches the activity stream and + * is announced to federated receivers. A personal override changes nothing + * anyone else can observe, so it stays silent. + * + * @throws InternalError + * @throws NotFoundError + * @throws PermissionError + */ + private function setArchivedForUser(int $id, string $userId, bool $archived): Table { + $table = $this->find($id, true, $userId); + $isOwner = $table->getOwnership() === $userId; + $entityArchived = $table->isArchived(); // entity-level flag, not per-user + $changes = new ChangeSet($table); + + try { + if ($archived) { + $this->archiveService->archiveForUser($userId, Application::NODE_TYPE_TABLE, $id, $isOwner, $entityArchived); + } else { + $this->archiveService->unarchiveForUser($userId, Application::NODE_TYPE_TABLE, $id, $isOwner, $entityArchived); + } + } catch (OcpDbException $e) { + $this->logger->error($e->getMessage(), ['exception' => $e]); + throw new InternalError(static::class . ' - ' . __FUNCTION__ . ': ' . $e->getMessage()); + } + + if ($isOwner && $entityArchived !== $archived) { + $this->announceArchiveChange($table, $userId, $archived, $changes); + } + + return $this->getTableForUser($id, $userId); + } + + /** + * Record an owner-level archive change the way TableService::update() does. + * + * The flag itself was already written by ArchiveService, so this only + * refreshes the edit metadata and emits the notifications. Failures are + * logged rather than raised: the archive state is already stored and must + * not be rolled back because a notification could not be delivered. + */ + private function announceArchiveChange(Table $table, string $userId, bool $archived, ChangeSet $changes): void { + try { + $table->setArchived($archived); + $table->setLastEditBy($userId); + $table->setLastEditAt((new DateTime())->format('Y-m-d H:i:s')); + $table = $this->mapper->update($table); + + $this->federationService->notifyNodeUpdate($table, 'table'); + + $changes->setAfter($table); + $this->activityManager->triggerUpdateEvents( + objectType: ActivityManager::TABLES_OBJECT_TABLE, + changeSet: $changes, + subject: ActivityManager::SUBJECT_TABLE_UPDATE + ); + } catch (\Throwable $e) { + $this->logger->error('Could not announce the archive change: ' . $e->getMessage(), ['exception' => $e]); + } + } + /** * @param string $title * @param string $template @@ -391,12 +554,22 @@ public function setOwner(int $id, string $newOwnerUserId, ?string $userId = null } $previousOwnerUserId = $table->getOwnership(); + $wasOwnerArchived = $table->isArchived(); $table->setOwnership($newOwnerUserId); try { - $table = $this->atomic(function () use ($table, $id, $newOwnerUserId, $userId) { + $table = $this->atomic(function () use ($table, $id, $newOwnerUserId, $userId, $previousOwnerUserId, $wasOwnerArchived) { + $newArchived = $this->archiveService->prepareOwnershipTransfer( + $previousOwnerUserId, $newOwnerUserId, Application::NODE_TYPE_TABLE, $id, $wasOwnerArchived + ); + if ($newArchived !== $wasOwnerArchived) { + $table->setArchived($newArchived); + } $table = $this->mapper->update($table); $this->shareService->changeSenderForNode('table', $id, $newOwnerUserId, $userId); + if (!$this->permissionsService->canReadTable($table, $previousOwnerUserId)) { + $this->archiveService->removeUserOverride($previousOwnerUserId, Application::NODE_TYPE_TABLE, $id); + } return $table; }, $this->dbc); } catch (\Exception $e) { @@ -492,6 +665,13 @@ public function delete(int $id, ?string $userId = null): Table { throw new InternalError(static::class . ' - ' . __FUNCTION__ . ': ' . $e->getMessage()); } + // remove per-user archive overrides for this table + try { + $this->archiveService->deleteNodeArchiveOverrides(Application::NODE_TYPE_TABLE, $id); + } catch (OcpDbException $e) { + $this->logger->error($e->getMessage(), ['exception' => $e]); + } + $event = new TableDeletedEvent(table: $item); $this->eventDispatcher->dispatchTyped($event); @@ -539,36 +719,48 @@ public function update(int $id, ?string $title, ?string $emoji, ?string $descrip $changes = new ChangeSet($table); $wasArchived = $table->getArchived(); $time = new DateTime(); + $hasSharedChange = false; if ($title !== null) { $title = (string)new Title($title); $table->setTitle($title); + $hasSharedChange = true; } if ($emoji !== null) { $table->setEmoji($emoji); + $hasSharedChange = true; } if ($archived !== null) { - $table->setArchived($archived); + // a non-owner's archive is private; only an owner-level flag flip is shared + $hasSharedChange = $this->applyArchivedOnUpdate($table, $archived, $userId) || $hasSharedChange; } if ($description !== null) { $table->setDescription($description); + $hasSharedChange = true; } if ($columnSettings !== null) { $table->setColumnOrder(\json_encode($columnSettings->jsonSerialize())); + $hasSharedChange = true; } if ($sort !== null) { $table->setSort(\json_encode($sort->jsonSerialize())); + $hasSharedChange = true; } - $table->setLastEditBy($userId); - $table->setLastEditAt($time->format('Y-m-d H:i:s')); - try { - $table = $this->mapper->update($table); - } catch (OcpDbException $e) { - $this->logger->error($e->getMessage(), ['exception' => $e]); - throw new InternalError(static::class . ' - ' . __FUNCTION__ . ': ' . $e->getMessage()); - } - // notify federated shares about table update - $this->federationService->notifyNodeUpdate($table, 'table'); + // A private per-user archive override leaves the shared table untouched, + // so skip the edit-metadata bump, persist, federation notice and activity. + if ($hasSharedChange) { + $table->setLastEditBy($userId); + $table->setLastEditAt($time->format('Y-m-d H:i:s')); + try { + $table = $this->mapper->update($table); + } catch (OcpDbException $e) { + $this->logger->error($e->getMessage(), ['exception' => $e]); + throw new InternalError(static::class . ' - ' . __FUNCTION__ . ': ' . $e->getMessage()); + } + + // notify federated shares about table update + $this->federationService->notifyNodeUpdate($table, 'table'); + } try { $this->enhanceTable($table, $userId); @@ -576,19 +768,32 @@ public function update(int $id, ?string $title, ?string $emoji, ?string $descrip $this->logger->error($e->getMessage(), ['exception' => $e]); throw new InternalError(static::class . ' - ' . __FUNCTION__ . ': ' . $e->getMessage()); } - $changes->setAfter($table); - $this->activityManager->triggerUpdateEvents( - objectType: ActivityManager::TABLES_OBJECT_TABLE, - changeSet: $changes, - subject: ActivityManager::SUBJECT_TABLE_UPDATE - ); - if ($archived !== null && $archived !== $wasArchived) { + if ($hasSharedChange) { + $changes->setAfter($table); + $this->activityManager->triggerUpdateEvents( + objectType: ActivityManager::TABLES_OBJECT_TABLE, + changeSet: $changes, + subject: ActivityManager::SUBJECT_TABLE_UPDATE + ); + } + // A private per-user override leaves the shared flag alone, so only an + // owner-level flip is worth an audit entry. + if ($table->getArchived() !== $wasArchived) { $this->auditLogService->log( - $archived ? Operation::TableArchived : Operation::TableUnarchived, - $archived ? 'Table with ID: %s was archived' : 'Table with ID: %s was unarchived', + $table->getArchived() ? Operation::TableArchived : Operation::TableUnarchived, + $table->getArchived() ? 'Table with ID: %s was archived' : 'Table with ID: %s was unarchived', ['tableId' => $table->getId()], ); } + // Resolve the per-user archived state only after the activity diff and the + // audit entry are built, so the response reflects the requesting user's state. + if ($userId !== '') { + try { + $this->archiveService->enrichTablesWithArchiveState([$table], $userId); + } catch (OcpDbException $e) { + $this->logger->error($e->getMessage(), ['exception' => $e]); + } + } return $table; } @@ -836,6 +1041,14 @@ public function updateTableStructure(int $tableId, array $columns, array $views, $this->viewService->delete($viewData['id']); } + // The caller returns this table straight to the client, which must see its own + // archived state rather than the shared flag. + try { + $this->archiveService->enrichTablesWithArchiveState([$table], $userId); + } catch (OcpDbException $e) { + $this->logger->error($e->getMessage(), ['exception' => $e]); + } + return $table; } diff --git a/tests/unit/Service/TableServiceAuditLogTest.php b/tests/unit/Service/TableServiceAuditLogTest.php index 5445bbc6fa..c450bd7aac 100644 --- a/tests/unit/Service/TableServiceAuditLogTest.php +++ b/tests/unit/Service/TableServiceAuditLogTest.php @@ -14,6 +14,7 @@ use OCA\Tables\Db\Table; use OCA\Tables\Db\TableMapper; use OCA\Tables\Helper\UserHelper; +use OCA\Tables\Service\ArchiveService; use OCA\Tables\Service\ColumnService; use OCA\Tables\Service\ContextService; use OCA\Tables\Service\FavoritesService; @@ -69,6 +70,7 @@ protected function setUp(): void { $this->createMock(ActivityManager::class), $this->createMock(FederationService::class), $this->auditLogService, + $this->createMock(ArchiveService::class), ); } From 2f62664b972bc708e0bd961aa639865e18b2dc90 Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Sat, 12 Sep 2026 11:26:50 +0200 Subject: [PATCH 4/9] feat(archive): add archive/unarchive endpoints and routes Add the v2 table and context archive/unarchive endpoints (rate-limited, permission-checked, contexts addressed via the string typeParam) and their routes, resolve the per-user state in the single-item GET endpoints, and leave the archived state unchanged on partial updates through API v1 and the occ command. Assisted-by: ClaudeCode:claude-fable-5 Signed-off-by: Andy Scherzinger --- appinfo/routes.php | 4 ++ lib/Command/RenameTable.php | 6 ++- lib/Controller/Api1Controller.php | 9 ++-- lib/Controller/ApiTablesController.php | 61 +++++++++++++++++++++++++- lib/Controller/ContextController.php | 59 +++++++++++++++++++++++++ lib/Controller/TableController.php | 2 +- tests/stub.phpstub | 1 + 7 files changed, 135 insertions(+), 7 deletions(-) diff --git a/appinfo/routes.php b/appinfo/routes.php index ad9b79e77e..724c413a2a 100644 --- a/appinfo/routes.php +++ b/appinfo/routes.php @@ -138,6 +138,8 @@ ['name' => 'ApiTables#transfer', 'url' => '/api/2/tables/{id}/transfer', 'verb' => 'PUT'], ['name' => 'ApiTables#previewSchemeChanges', 'url' => '/api/2/tables/{id}/scheme/preview-changes', 'verb' => 'POST'], ['name' => 'ApiTables#importScheme', 'url' => '/api/2/tables/{id}/scheme/import', 'verb' => 'POST'], + ['name' => 'ApiTables#archiveTable', 'url' => '/api/2/tables/{id}/archive', 'verb' => 'POST'], + ['name' => 'ApiTables#unarchiveTable', 'url' => '/api/2/tables/{id}/archive', 'verb' => 'DELETE'], ['name' => 'ApiColumns#index', 'url' => '/api/2/columns/{nodeType}/{nodeId}', 'verb' => 'GET'], ['name' => 'ApiColumns#show', 'url' => '/api/2/columns/{id}', 'verb' => 'GET'], @@ -159,6 +161,8 @@ ['name' => 'Context#previewSchemeChanges', 'url' => '/api/2/contexts/{contextId}/scheme/preview-changes', 'verb' => 'POST'], ['name' => 'Context#importScheme', 'url' => '/api/2/contexts/{contextId}/scheme/import', 'verb' => 'POST'], ['name' => 'Context#transfer', 'url' => '/api/2/contexts/{contextId}/transfer', 'verb' => 'PUT'], + ['name' => 'Context#archiveContext', 'url' => '/api/2/contexts/{contextId}/archive', 'verb' => 'POST'], + ['name' => 'Context#unarchiveContext', 'url' => '/api/2/contexts/{contextId}/archive', 'verb' => 'DELETE'], ['name' => 'Context#updateContentOrder', 'url' => '/api/2/contexts/{contextId}/pages/{pageId}', 'verb' => 'PUT'], ['name' => 'Config#getTableConfig', 'url' => '/api/2/config/table/{id}', 'verb' => 'GET'], diff --git a/lib/Command/RenameTable.php b/lib/Command/RenameTable.php index eac168904e..2d87e76a23 100644 --- a/lib/Command/RenameTable.php +++ b/lib/Command/RenameTable.php @@ -55,8 +55,8 @@ protected function configure(): void { ->addOption( 'archived', 'a', - InputOption::VALUE_NONE, - 'Archived' + InputOption::VALUE_NEGATABLE, + 'Archive (--archived) or unarchive (--no-archived) the table; the archived state is kept unchanged when omitted' ) ; } @@ -70,6 +70,8 @@ protected function execute(InputInterface $input, OutputInterface $output): int $id = $input->getArgument('ID'); $title = $input->getArgument('title'); $emoji = $input->getOption('emoji'); + // A negatable option is null when neither form was given, which leaves the + // archived state unchanged rather than unarchiving the table. $archived = $input->getOption('archived'); $description = $input->getOption('description'); diff --git a/lib/Controller/Api1Controller.php b/lib/Controller/Api1Controller.php index 82cfc11450..56c2ecf831 100644 --- a/lib/Controller/Api1Controller.php +++ b/lib/Controller/Api1Controller.php @@ -191,7 +191,10 @@ public function showScheme(int $tableId): JSONResponse|DataResponse { #[OpenAPI(scope: OpenAPI::SCOPE_DEFAULT)] public function getTable(int $tableId): DataResponse { try { - return new DataResponse($this->tableService->find($tableId)->jsonSerialize()); + $table = $this->userId === null + ? $this->tableService->find($tableId) + : $this->tableService->getTableForUser($tableId, $this->userId); + return new DataResponse($table->jsonSerialize()); } catch (PermissionError $e) { $this->logger->warning('A permission error occurred: ' . $e->getMessage(), ['exception' => $e]); $message = ['message' => $e->getMessage()]; @@ -213,7 +216,7 @@ public function getTable(int $tableId): DataResponse { * @param int $tableId Table ID * @param string|null $title New table title * @param string|null $emoji New table emoji - * @param bool $archived Whether the table is archived + * @param bool|null $archived Whether the table is archived; the state is kept unchanged when omitted * @return DataResponse|DataResponse * * 200: Tables returned @@ -226,7 +229,7 @@ public function getTable(int $tableId): DataResponse { #[CORS] #[RequirePermission(permission: Application::PERMISSION_MANAGE, type: Application::NODE_TYPE_TABLE, idParam: 'tableId')] #[OpenAPI(scope: OpenAPI::SCOPE_DEFAULT)] - public function updateTable(int $tableId, ?string $title = null, ?string $emoji = null, ?bool $archived = false): DataResponse { + public function updateTable(int $tableId, ?string $title = null, ?string $emoji = null, ?bool $archived = null): DataResponse { try { return new DataResponse($this->tableService->update($tableId, $title, $emoji, null, $archived, $this->userId)->jsonSerialize()); } catch (InvalidArgumentException $e) { diff --git a/lib/Controller/ApiTablesController.php b/lib/Controller/ApiTablesController.php index 3cf2e1e86f..3302d96a55 100644 --- a/lib/Controller/ApiTablesController.php +++ b/lib/Controller/ApiTablesController.php @@ -29,6 +29,7 @@ use OCP\App\IAppManager; use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\NoAdminRequired; +use OCP\AppFramework\Http\Attribute\UserRateLimit; use OCP\AppFramework\Http\DataResponse; use OCP\IDBConnection; use OCP\IL10N; @@ -87,7 +88,7 @@ public function index(): DataResponse { #[RequirePermission(permission: Application::PERMISSION_READ, type: Application::NODE_TYPE_TABLE, idParam: 'id')] public function show(int $id): DataResponse { try { - return new DataResponse($this->service->find($id)->jsonSerialize()); + return new DataResponse($this->service->getTableForUser($id, $this->userId)->jsonSerialize()); } catch (PermissionError $e) { return $this->handlePermissionError($e); } catch (InternalError $e) { @@ -479,6 +480,64 @@ public function destroy(int $id): DataResponse { } } + /** + * [api v2] Archive a table for the requesting user + * + * Owners set the archived default for everyone; a user who made their own + * choice keeps it. Non-owners archive only for themselves. + * + * @param int $id Table ID + * @return DataResponse|DataResponse + * + * 200: Table returned with updated archived state + * 403: No permissions + * 404: Not found + */ + #[NoAdminRequired] + #[UserRateLimit(limit: 20, period: 60)] + #[RequirePermission(permission: Application::PERMISSION_READ, type: Application::NODE_TYPE_TABLE, idParam: 'id')] + public function archiveTable(int $id): DataResponse { + return $this->archiveResponse($id, true); + } + + /** + * [api v2] Unarchive a table for the requesting user + * + * Owners set the unarchived default for everyone; a user who made their own + * choice keeps it. Non-owners remove only their personal archive override. + * + * @param int $id Table ID + * @return DataResponse|DataResponse + * + * 200: Table returned with updated archived state + * 403: No permissions + * 404: Not found + */ + #[NoAdminRequired] + #[UserRateLimit(limit: 20, period: 60)] + #[RequirePermission(permission: Application::PERMISSION_READ, type: Application::NODE_TYPE_TABLE, idParam: 'id')] + public function unarchiveTable(int $id): DataResponse { + return $this->archiveResponse($id, false); + } + + /** + * @return DataResponse|DataResponse + */ + private function archiveResponse(int $id, bool $archived): DataResponse { + try { + $table = $archived + ? $this->service->archiveTable($id, $this->userId) + : $this->service->unarchiveTable($id, $this->userId); + return new DataResponse($table->jsonSerialize()); + } catch (PermissionError $e) { + return $this->handlePermissionError($e); + } catch (InternalError $e) { + return $this->handleError($e); + } catch (NotFoundError $e) { + return $this->handleNotFoundError($e); + } + } + /** * [api v2] Transfer table * diff --git a/lib/Controller/ContextController.php b/lib/Controller/ContextController.php index e87f8ca440..c9365b57de 100644 --- a/lib/Controller/ContextController.php +++ b/lib/Controller/ContextController.php @@ -25,6 +25,7 @@ use OCP\AppFramework\Db\MultipleObjectsReturnedException; use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\NoAdminRequired; +use OCP\AppFramework\Http\Attribute\UserRateLimit; use OCP\AppFramework\Http\DataResponse; use OCP\DB\Exception; use OCP\IDBConnection; @@ -255,6 +256,64 @@ public function transfer(int $contextId, string $newOwnerId, int $newOwnerType = } } + /** + * [api v2] Archive a context for the requesting user + * + * Owners set the archived default for everyone; a user who made their own + * choice keeps it. Non-owners archive only for themselves. + * + * @param int $contextId ID of the context + * @return DataResponse|DataResponse + * + * 200: Context returned with updated archived state + * 403: No permissions + * 404: Context not found or not available + */ + #[NoAdminRequired] + #[UserRateLimit(limit: 20, period: 60)] + #[RequirePermission(permission: Application::PERMISSION_READ, typeParam: 'context', idParam: 'contextId')] + public function archiveContext(int $contextId): DataResponse { + return $this->archiveResponse($contextId, true); + } + + /** + * [api v2] Unarchive a context for the requesting user + * + * Owners set the unarchived default for everyone; a user who made their own + * choice keeps it. Non-owners remove only their personal archive override. + * + * @param int $contextId ID of the context + * @return DataResponse|DataResponse + * + * 200: Context returned with updated archived state + * 403: No permissions + * 404: Context not found or not available + */ + #[NoAdminRequired] + #[UserRateLimit(limit: 20, period: 60)] + #[RequirePermission(permission: Application::PERMISSION_READ, typeParam: 'context', idParam: 'contextId')] + public function unarchiveContext(int $contextId): DataResponse { + return $this->archiveResponse($contextId, false); + } + + /** + * @return DataResponse|DataResponse + */ + private function archiveResponse(int $contextId, bool $archived): DataResponse { + try { + $context = $archived + ? $this->contextService->archiveContext($contextId, $this->userId) + : $this->contextService->unarchiveContext($contextId, $this->userId); + return new DataResponse($context->jsonSerialize()); + } catch (PermissionError $e) { + return $this->handlePermissionError($e); + } catch (NotFoundError $e) { + return $this->handleNotFoundError($e); + } catch (InternalError|Exception $e) { + return $this->handleError($e); + } + } + /** * [api v2] Update the order on a page of a context * diff --git a/lib/Controller/TableController.php b/lib/Controller/TableController.php index cd7c3963f3..1e42475b32 100644 --- a/lib/Controller/TableController.php +++ b/lib/Controller/TableController.php @@ -38,7 +38,7 @@ public function index(): DataResponse { #[NoAdminRequired] #[RequirePermission(permission: Application::PERMISSION_READ, type: Application::NODE_TYPE_TABLE, idParam: 'id')] public function show(int $id): DataResponse { - return $this->handleError(fn () => $this->service->find($id)); + return $this->handleError(fn () => $this->service->getTableForUser($id, $this->userId)); } #[NoAdminRequired] diff --git a/tests/stub.phpstub b/tests/stub.phpstub index 1abb26a1ec..7b4502c333 100644 --- a/tests/stub.phpstub +++ b/tests/stub.phpstub @@ -139,6 +139,7 @@ namespace Symfony\Component\Console\Input { const VALUE_NONE = 1; const VALUE_REQUIRED = 1; const VALUE_OPTIONAL = 1; + const VALUE_NEGATABLE = 1; } } From e11ebac2ba0631f60d83542d049cb4679c5f2286 Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Sat, 12 Sep 2026 11:26:50 +0200 Subject: [PATCH 5/9] feat(archive): clean up per-user overrides when access is lost Remove stale archive overrides when a share is deleted, a group or circle membership ends, a receiver is deleted, or a user account is removed. Per-user access re-checks are handed to a background job so an unrelated request does not scale with the number of override holders. Assisted-by: ClaudeCode:claude-fable-5 Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Andy Scherzinger --- lib/AppInfo/Application.php | 6 + .../CleanupArchiveOverridesJob.php | 59 ++++ lib/Db/ShareMapper.php | 22 ++ lib/Federation/FederationProvider.php | 7 + lib/Helper/ConversionHelper.php | 17 ++ lib/Listener/ArchiveCleanupListener.php | 82 ++++++ lib/Listener/ReceiverCleanupListener.php | 31 ++- lib/Service/ArchiveCleanupService.php | 255 ++++++++++++++++++ lib/Service/ContextService.php | 17 +- lib/Service/PermissionsService.php | 12 + lib/Service/ShareService.php | 3 + lib/Service/TableService.php | 36 ++- tests/unit/Circles/Stubs.php | 66 +++++ .../Federation/FederationProviderTest.php | 2 + .../Listener/ArchiveCleanupListenerTest.php | 120 +++++++++ .../unit/Service/TableServiceAuditLogTest.php | 2 + tests/unit/bootstrap.php | 4 + 17 files changed, 725 insertions(+), 16 deletions(-) create mode 100644 lib/BackgroundJob/CleanupArchiveOverridesJob.php create mode 100644 lib/Listener/ArchiveCleanupListener.php create mode 100644 lib/Service/ArchiveCleanupService.php create mode 100644 tests/unit/Circles/Stubs.php create mode 100644 tests/unit/Listener/ArchiveCleanupListenerTest.php diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index 110a878d8b..9376a9e1ce 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -11,6 +11,7 @@ use OC\OCM\OCMSignatoryManager; use OCA\Analytics\Datasource\DatasourceEvent; use OCA\Circles\Events\CircleDestroyedEvent; +use OCA\Circles\Events\CircleMemberRemovedEvent; use OCA\Tables\Capabilities; use OCA\Tables\Config\ConfigLexicon; use OCA\Tables\Event\RowDeletedEvent; @@ -20,6 +21,7 @@ use OCA\Tables\Federation\FederationProvider; use OCA\Tables\Listener\AddMissingIndicesListener; use OCA\Tables\Listener\AnalyticsDatasourceListener; +use OCA\Tables\Listener\ArchiveCleanupListener; use OCA\Tables\Listener\LoadAdditionalEntriesListener; use OCA\Tables\Listener\LoadAdditionalListener; use OCA\Tables\Listener\ReceiverCleanupListener; @@ -50,6 +52,7 @@ use OCP\Federation\ICloudFederationProvider; use OCP\Federation\ICloudFederationProviderManager; use OCP\Group\Events\GroupDeletedEvent; +use OCP\Group\Events\UserRemovedEvent; use OCP\Navigation\Events\LoadAdditionalEntriesEvent; use OCP\OCM\Events\LocalOCMDiscoveryEvent; use OCP\Security\Signature\ISignatoryManager; @@ -64,6 +67,7 @@ class Application extends App implements IBootstrap { public const NODE_TYPE_TABLE = 0; public const NODE_TYPE_VIEW = 1; + public const NODE_TYPE_CONTEXT = 2; public const OWNER_TYPE_USER = 0; @@ -108,6 +112,8 @@ public function register(IRegistrationContext $context): void { $context->registerEventListener(UserDeletedEvent::class, ReceiverCleanupListener::class); $context->registerEventListener(GroupDeletedEvent::class, ReceiverCleanupListener::class); $context->registerEventListener(CircleDestroyedEvent::class, ReceiverCleanupListener::class); + $context->registerEventListener(UserRemovedEvent::class, ArchiveCleanupListener::class); + $context->registerEventListener(CircleMemberRemovedEvent::class, ArchiveCleanupListener::class); $context->registerEventListener(LocalOCMDiscoveryEvent::class, ResourceTypeRegisterListener::class); $context->registerSearchProvider(SearchTablesProvider::class); diff --git a/lib/BackgroundJob/CleanupArchiveOverridesJob.php b/lib/BackgroundJob/CleanupArchiveOverridesJob.php new file mode 100644 index 0000000000..fb229d64f7 --- /dev/null +++ b/lib/BackgroundJob/CleanupArchiveOverridesJob.php @@ -0,0 +1,59 @@ +} + */ + protected function run($argument): void { + if (!is_array($argument)) { + $this->logger->warning('Cannot clean up archive overrides: invalid argument'); + return; + } + + $nodeType = (int)($argument['nodeType'] ?? -1); + $nodeId = (int)($argument['nodeId'] ?? 0); + $userIds = $argument['userIds'] ?? []; + + if ($nodeId <= 0 || !is_array($userIds) || $userIds === []) { + return; + } + if (!in_array($nodeType, [Application::NODE_TYPE_TABLE, Application::NODE_TYPE_CONTEXT], true)) { + $this->logger->warning('Cannot clean up archive overrides: unsupported node type ' . $nodeType); + return; + } + + foreach ($userIds as $userId) { + $this->archiveCleanupService->removeOverrideIfStale((string)$userId, $nodeType, $nodeId); + } + } +} diff --git a/lib/Db/ShareMapper.php b/lib/Db/ShareMapper.php index c8a42db6bc..ee34f1fa58 100644 --- a/lib/Db/ShareMapper.php +++ b/lib/Db/ShareMapper.php @@ -301,6 +301,28 @@ public function changeReceiverForNode(string $nodeType, int $nodeId, string $new ->executeStatement(); } + /** + * Find the distinct nodes shared with a given receiver. + * + * @return list + * @throws Exception + */ + public function findNodesByReceiver(string $receiver, string $receiverType): array { + $qb = $this->db->getQueryBuilder(); + $qb->selectDistinct(['node_type', 'node_id']) + ->from($this->table) + ->where($qb->expr()->eq('receiver', $qb->createNamedParameter($receiver, IQueryBuilder::PARAM_STR))) + ->andWhere($qb->expr()->eq('receiver_type', $qb->createNamedParameter($receiverType, IQueryBuilder::PARAM_STR))); + + $result = $qb->executeQuery(); + $nodes = []; + while ($row = $result->fetchAssociative()) { + $nodes[] = ['nodeType' => (string)$row['node_type'], 'nodeId' => (int)$row['node_id']]; + } + $result->closeCursor(); + return $nodes; + } + /** * @throws Exception */ diff --git a/lib/Federation/FederationProvider.php b/lib/Federation/FederationProvider.php index 06ebe7563b..48b7e798a8 100644 --- a/lib/Federation/FederationProvider.php +++ b/lib/Federation/FederationProvider.php @@ -9,6 +9,7 @@ namespace OCA\Tables\Federation; +use OCA\Tables\AppInfo\Application; use OCA\Tables\AuditLog\Operation; use OCA\Tables\Constants\ShareReceiverType; use OCA\Tables\Constants\ViewLayout; @@ -20,6 +21,7 @@ use OCA\Tables\Db\ViewMapper; use OCA\Tables\Errors\FederationDisabledError; use OCA\Tables\Errors\InternalError; +use OCA\Tables\Service\ArchiveService; use OCA\Tables\Service\FederationService; use OCA\Tables\Service\Support\AuditLogServiceInterface; use OCA\Tables\Service\ValueObject\ShareToken; @@ -45,6 +47,7 @@ public function __construct( private readonly ICloudIdManager $cloudIdManager, private readonly FederationService $federationService, private readonly AuditLogServiceInterface $auditLogService, + private readonly ArchiveService $archiveService, ) { } @@ -234,6 +237,10 @@ private function handleNodeDelete(string $providerId, array $notification): void $nodeType = $notification['nodeType']; $node = $this->findNodeByExternalIdAndToken($providerId, $notification, $nodeType); $this->shareMapper->deleteByNode($node->getId(), $nodeType); + if ($nodeType === 'table') { + // The share rows go first, so no later event could reach these. + $this->archiveService->deleteNodeArchiveOverrides(Application::NODE_TYPE_TABLE, $node->getId()); + } $this->getMapperForNodeType($nodeType)->delete($node); } diff --git a/lib/Helper/ConversionHelper.php b/lib/Helper/ConversionHelper.php index 04b0015343..b941232029 100644 --- a/lib/Helper/ConversionHelper.php +++ b/lib/Helper/ConversionHelper.php @@ -36,6 +36,23 @@ public static function stringNodeType2Const(string $nodeType): int { }; } + /** + * Map a node type as stored in `tables_shares`, which unlike the node + * types above also covers contexts. + * + * Returns null for anything unknown so callers can skip it. Kept separate + * from stringNodeType2Const() on purpose: that method doubles as a + * validation gate for endpoints that only handle tables and views. + */ + public static function shareNodeType2Const(string $nodeType): ?int { + return match ($nodeType) { + 'table', 'tables' => Application::NODE_TYPE_TABLE, + 'view', 'views' => Application::NODE_TYPE_VIEW, + 'context', 'contexts' => Application::NODE_TYPE_CONTEXT, + default => null, + }; + } + public static function object2String(Table|View $node): string { if ($node instanceof Table) { return 'table'; diff --git a/lib/Listener/ArchiveCleanupListener.php b/lib/Listener/ArchiveCleanupListener.php new file mode 100644 index 0000000000..141e4580c2 --- /dev/null +++ b/lib/Listener/ArchiveCleanupListener.php @@ -0,0 +1,82 @@ + + */ +class ArchiveCleanupListener implements IEventListener { + public function __construct( + private readonly ArchiveCleanupService $archiveCleanupService, + private readonly LoggerInterface $logger, + ) { + } + + public function handle(Event $event): void { + if ($event instanceof UserRemovedEvent) { + $this->archiveCleanupService->cleanupAfterMembershipLoss( + [$event->getUser()->getUID()], + $event->getGroup()->getGID(), + ShareReceiverType::GROUP, + ); + return; + } + + if ($event instanceof CircleMemberRemovedEvent) { + $this->handleCircleMemberRemoved($event); + } + } + + private function handleCircleMemberRemoved(CircleMemberRemovedEvent $event): void { + try { + $member = $event->getMember(); + if ($member === null) { + return; + } + + // A member that stands for a group or a nested team has to be expanded + // into the accounts behind it; only those hold archive overrides. + $basedOn = $member->getUserType() === Member::TYPE_USER ? null : $member->getBasedOn(); + $members = $basedOn !== null ? $basedOn->getInheritedMembers() : [$member]; + + $userIds = []; + foreach ($members as $affectedMember) { + if ($affectedMember->getUserType() === Member::TYPE_USER) { + $userIds[] = $affectedMember->getUserId(); + } + } + + if ($userIds === []) { + return; + } + + $this->archiveCleanupService->cleanupAfterMembershipLoss( + $userIds, + $event->getCircle()->getSingleId(), + ShareReceiverType::CIRCLE, + ); + } catch (\Throwable $e) { + $this->logger->warning('cleanup of archive overrides after circle member removal failed: ' . $e->getMessage(), [ + 'exception' => $e, + ]); + } + } +} diff --git a/lib/Listener/ReceiverCleanupListener.php b/lib/Listener/ReceiverCleanupListener.php index a5cfe3a470..0919584e3d 100644 --- a/lib/Listener/ReceiverCleanupListener.php +++ b/lib/Listener/ReceiverCleanupListener.php @@ -10,6 +10,7 @@ use OCA\Circles\Events\CircleDestroyedEvent; use OCA\Tables\Constants\ShareReceiverType; use OCA\Tables\Db\ShareMapper; +use OCA\Tables\Service\ArchiveCleanupService; use OCP\EventDispatcher\Event; use OCP\EventDispatcher\IEventListener; use OCP\Group\Events\GroupDeletedEvent; @@ -20,6 +21,7 @@ class ReceiverCleanupListener implements IEventListener { public function __construct( private readonly ShareMapper $shareMapper, + private readonly ArchiveCleanupService $archiveCleanupService, private readonly LoggerInterface $logger, ) { } @@ -27,10 +29,35 @@ public function __construct( public function handle(Event $event): void { if ($event instanceof UserDeletedEvent) { $this->cleanupByParticipant(ShareReceiverType::USER, $event->getUser()->getUID()); + $this->archiveCleanupService->cleanupDeletedUser($event->getUser()->getUID()); } elseif ($event instanceof GroupDeletedEvent) { - $this->cleanupByParticipant(ShareReceiverType::GROUP, $event->getGroup()->getGID()); + $this->cleanupReceiverGone(ShareReceiverType::GROUP, $event->getGroup()->getGID()); } elseif ($event instanceof CircleDestroyedEvent) { - $this->cleanupByParticipant(ShareReceiverType::CIRCLE, $event->getCircle()->getSingleId()); + $this->cleanupReceiverGone(ShareReceiverType::CIRCLE, $event->getCircle()->getSingleId()); + } + } + + /** + * Delete all shares of a removed group or circle receiver and drop the + * archive overrides of every ex-member who lost access to the affected + * nodes with those shares. + */ + private function cleanupReceiverGone(string $type, string $participant): void { + try { + $nodes = $this->shareMapper->findNodesByReceiver($participant, $type); + } catch (\Throwable $e) { + $this->logger->warning('collecting nodes shared with deleted receiver has failed: ' . $e->getMessage(), [ + 'exception' => $e, + 'receiver_type' => $type, + 'receiver' => $participant, + ]); + $nodes = []; + } + + $this->cleanupByParticipant($type, $participant); + + foreach ($nodes as $node) { + $this->archiveCleanupService->purgeShareNodeOverrides((string)$node['nodeType'], $node['nodeId']); } } diff --git a/lib/Service/ArchiveCleanupService.php b/lib/Service/ArchiveCleanupService.php new file mode 100644 index 0000000000..a27d92e4cb --- /dev/null +++ b/lib/Service/ArchiveCleanupService.php @@ -0,0 +1,255 @@ +shareMapper->findNodesByReceiver($receiver, $receiverType); + } catch (\Throwable $e) { + $this->logNonFatal(__FUNCTION__, $e); + return; + } + + $nodeIdsByType = [ + Application::NODE_TYPE_TABLE => [], + Application::NODE_TYPE_CONTEXT => [], + ]; + foreach ($nodes as $node) { + $nodeType = $this->shareNodeType2Const($node['nodeType']); + if ($nodeType !== null) { + $nodeIdsByType[$nodeType][] = $node['nodeId']; + } + } + + $affectedUsersByNode = []; + foreach ($nodeIdsByType as $nodeType => $nodeIds) { + if ($nodeIds === []) { + continue; + } + try { + // One query for the whole membership: removing a nested team hands us + // every inherited member at once, and that set has no upper bound. + $affectedUsersByNode[$nodeType] = $this->userArchiveMapper->findOverrideUsersForNodes($userIds, $nodeType, $nodeIds); + } catch (\Throwable $e) { + $this->logNonFatal(__FUNCTION__, $e); + } + } + + foreach ($affectedUsersByNode as $nodeType => $usersByNodeId) { + foreach ($usersByNodeId as $nodeId => $affectedUserIds) { + $this->scheduleCleanup((int)$nodeType, (int)$nodeId, $affectedUserIds); + } + } + } + + /** + * Remove the stale archive overrides of a node addressed by its share + * node-type string; share types without archive support (views) are + * ignored. + */ + public function purgeShareNodeOverrides(string $shareNodeType, int $nodeId): void { + $nodeType = $this->shareNodeType2Const($shareNodeType); + if ($nodeType === null) { + return; + } + $this->purgeNodeOverrides($nodeType, $nodeId); + } + + /** + * Remove the archive overrides of every user who can no longer access + * the given node. + * + * Called when a group or circle receiver of the node is deleted, or when + * a group or circle share of the node is removed. + */ + public function purgeNodeOverrides(int $nodeType, int $nodeId): void { + try { + $userIds = $this->userArchiveMapper->findUserIdsForNode($nodeType, $nodeId); + } catch (\Throwable $e) { + $this->logNonFatal(__FUNCTION__, $e); + return; + } + + $this->scheduleCleanup($nodeType, $nodeId, $userIds); + } + + /** + * Hand the per-user access checks to a background job. + * + * Each check is a full permission resolution, so doing them inline would + * make an unrelated request (deleting one share, removing one member) + * scale with the number of users holding an override. + * + * @param list $userIds + */ + private function scheduleCleanup(int $nodeType, int $nodeId, array $userIds): void { + if ($userIds === []) { + return; + } + + // A job argument is stored as JSON and rejected above 4000 bytes, so a + // widely shared node is split across several jobs rather than silently + // failing to schedule. + foreach (array_chunk(array_values(array_unique($userIds)), self::JOB_USER_CHUNK_SIZE) as $chunk) { + try { + $this->jobList->add(CleanupArchiveOverridesJob::class, [ + 'nodeType' => $nodeType, + 'nodeId' => $nodeId, + 'userIds' => $chunk, + ]); + } catch (\Throwable $e) { + $this->logNonFatal(__FUNCTION__, $e); + } + } + } + + /** + * Remove archive overrides that became stale because a share was deleted. + */ + public function cleanupAfterShareDeletion(Share $share): void { + $nodeType = $this->shareNodeType2Const((string)$share->getNodeType()); + if ($nodeType === null) { + return; + } + + $receiverType = $share->getReceiverType(); + if ($receiverType === ShareReceiverType::USER) { + // The share this request resolved on the way in is gone now, and the + // access check below must not be answered from that memo. + $this->permissionsService->forgetCachedPermissions( + (int)$share->getNodeId(), + (string)$share->getNodeType(), + (string)$share->getReceiver(), + ); + $this->removeOverrideIfStale((string)$share->getReceiver(), $nodeType, (int)$share->getNodeId()); + } elseif ($receiverType === ShareReceiverType::GROUP || $receiverType === ShareReceiverType::CIRCLE) { + $this->purgeNodeOverrides($nodeType, (int)$share->getNodeId()); + } + } + + /** + * Remove every archive override of a deleted user account. + */ + public function cleanupDeletedUser(string $userId): void { + try { + $this->userArchiveMapper->deleteAllForUser($userId); + } catch (\Throwable $e) { + $this->logNonFatal(__FUNCTION__, $e); + } + } + + /** + * Drop a user's archive override for a node they can no longer reach. + * + * Team shares are taken into account, which plain context access checks do + * not do, so this is the only correct way to decide the question. + */ + public function removeOverrideIfStale(string $userId, int $nodeType, int $nodeId): void { + try { + if ($this->hasAccess($userId, $nodeType, $nodeId)) { + return; + } + $this->userArchiveMapper->deleteForUser($userId, $nodeType, $nodeId); + } catch (\Throwable $e) { + $this->logNonFatal(__FUNCTION__, $e); + } + } + + private function hasAccess(string $userId, int $nodeType, int $nodeId): bool { + if ($nodeType !== Application::NODE_TYPE_CONTEXT) { + return $this->permissionsService->canAccessNodeById($nodeType, $nodeId, $userId); + } + if ($this->permissionsService->canAccessContextById($nodeId, $userId)) { + return true; + } + + // Context access resolves through ContextMapper, which only considers + // the owner plus user and group shares, never team shares. Ask the + // share table directly, otherwise a user whose only access is a team + // share looks like they lost it and their override gets deleted. + return $this->hasTeamShareFor($userId, $nodeId); + } + + private function hasTeamShareFor(string $userId, int $nodeId): bool { + $circleIds = $this->circleHelper->getCircleIdsForUser($userId); + if (empty($circleIds)) { + return false; + } + + $shares = $this->shareMapper->findAllSharesForNodeTo('context', $nodeId, $userId, [], $circleIds); + foreach ($shares as $share) { + if ($share->getReceiverType() === ShareReceiverType::CIRCLE) { + return true; + } + } + return false; + } + + private function shareNodeType2Const(string $nodeType): ?int { + $nodeTypeConst = ConversionHelper::shareNodeType2Const($nodeType); + if ($nodeTypeConst === null) { + return null; + } + // Only tables and contexts can carry archive overrides; view shares + // must not trigger pointless lookup or access-check queries. + return in_array($nodeTypeConst, [Application::NODE_TYPE_TABLE, Application::NODE_TYPE_CONTEXT], true) + ? $nodeTypeConst + : null; + } + + private function logNonFatal(string $method, \Throwable $e): void { + $this->logger->warning(static::class . ' - ' . $method . ': archive override cleanup failed: ' . $e->getMessage(), [ + 'exception' => $e, + ]); + } +} diff --git a/lib/Service/ContextService.php b/lib/Service/ContextService.php index 9a77877548..6cb7d80bf7 100644 --- a/lib/Service/ContextService.php +++ b/lib/Service/ContextService.php @@ -159,8 +159,9 @@ public function findById(int $id, ?string $userId): Context { /** * Archive a context for the given user. * - * If the user is the owner the entity flag is set and all per-user - * overrides are cleared; otherwise a personal override is stored. + * If the user is the owner the entity flag is set, which is the default for + * everyone who never made a choice of their own; a stored override keeps its + * user on their own choice. Anyone else only stores a personal override. * Access is validated by the mapper (NotFoundError if no access). * * @throws Exception @@ -174,9 +175,10 @@ public function archiveContext(int $contextId, string $userId): Context { /** * Unarchive a context for the given user. * - * If the user is the owner the entity flag is cleared and all per-user - * overrides are reset; otherwise the personal override is removed or - * set to false if the owner has archived the context. + * If the user is the owner the entity flag is cleared, which is the default for + * everyone who never made a choice of their own; a stored override keeps its + * user on their own choice. Anyone else has their personal override removed, or + * set to false while the owner has archived the context. * Access is validated by the mapper (NotFoundError if no access). * * @throws Exception @@ -507,7 +509,6 @@ public function transfer(int $contextId, string $newOwnerId, int $newOwnerType): } $context = $this->contextMapper->update($context); $this->shareService->transferSharesForContext($contextId, $newOwnerId, $oldOwnerId); - $this->archiveCleanupService->removeOverrideIfStale($oldOwnerId, Application::NODE_TYPE_CONTEXT, $contextId); return $context; }, $this->dbc); } catch (\Exception $e) { @@ -515,6 +516,10 @@ public function transfer(int $contextId, string $newOwnerId, int $newOwnerType): throw new InternalError('Failed to transfer context ownership: ' . $e->getMessage()); } + // Outside the transaction on purpose: the access check reads shares and team + // memberships, and a failure there must not take the completed transfer with it. + $this->archiveCleanupService->removeOverrideIfStale($oldOwnerId, Application::NODE_TYPE_CONTEXT, $contextId); + $this->auditLogService->log(Operation::ContextTransferred, 'Tables application with ID %s was transferred to user %s', [ 'contextId' => $contextId, 'toUserId' => $newOwnerId, diff --git a/lib/Service/PermissionsService.php b/lib/Service/PermissionsService.php index 68803539c4..4c4353fa5c 100644 --- a/lib/Service/PermissionsService.php +++ b/lib/Service/PermissionsService.php @@ -613,6 +613,18 @@ private function buildPermissionCacheKey(int $nodeId, string $nodeType, string $ return $nodeType . ':' . $nodeId . ':' . $userId; } + /** + * Drop what this request already resolved about a user's access to a node. + * + * The memo is only sound while the shares it was built from are unchanged, and + * a request that deletes a share usually resolved the permissions first, to be + * allowed to delete it at all. + */ + public function forgetCachedPermissions(int $nodeId, string $nodeType, string $userId): void { + $cacheKey = $this->buildPermissionCacheKey($nodeId, $nodeType, $userId); + unset($this->sharedPermissionsByNode[$cacheKey], $this->contextPermissionsByNode[$cacheKey]); + } + private function hasPermission(int $existingPermissions, string $permissionName): bool { $constantName = 'PERMISSION_' . strtoupper($permissionName); try { diff --git a/lib/Service/ShareService.php b/lib/Service/ShareService.php index 1d5408c188..b48242cb88 100644 --- a/lib/Service/ShareService.php +++ b/lib/Service/ShareService.php @@ -74,6 +74,7 @@ public function __construct( private readonly IShareManager $shareManager, private readonly FederationService $federationService, private readonly AuditLogServiceInterface $auditLogService, + private readonly ArchiveCleanupService $archiveCleanupService, ) { parent::__construct($logger, $userId, $permissionsService); } @@ -740,6 +741,7 @@ public function delete(int $id): Share { throw new InternalError(static::class . ' - ' . __FUNCTION__ . ': ' . $e->getMessage()); } $this->logShareChange(Operation::ShareDeleted, 'Share %s of %s %s for %s "%s" was deleted', $item); + $this->archiveCleanupService->cleanupAfterShareDeletion($item); return $item; } @@ -847,6 +849,7 @@ public function deleteForShareReview(int $id): void { $this->contextNavigationMapper->deleteByShareId($share->getId()); } $this->logShareChange(Operation::ShareDeleted, 'Share %s of %s %s for %s "%s" was deleted through the share review', $share); + $this->archiveCleanupService->cleanupAfterShareDeletion($share); } public function deleteAllForTable(Table $table):void { diff --git a/lib/Service/TableService.php b/lib/Service/TableService.php index 88f0f2acac..e3dfb3d9aa 100644 --- a/lib/Service/TableService.php +++ b/lib/Service/TableService.php @@ -76,6 +76,7 @@ public function __construct( private FederationService $federationService, private AuditLogServiceInterface $auditLogService, private ArchiveService $archiveService, + private ArchiveCleanupService $archiveCleanupService, ) { parent::__construct($logger, $userId, $permissionsService); } @@ -352,8 +353,9 @@ private function applyArchivedOnUpdate(Table $table, bool $archived, string $use /** * Archive a table for the given user. * - * If the user is the owner the entity flag is set and all per-user - * overrides are cleared; otherwise a personal override is stored. + * If the user is the owner the entity flag is set, which is the default for + * everyone who never made a choice of their own; a stored override keeps its + * user on their own choice. Anyone else only stores a personal override. * * @throws InternalError * @throws NotFoundError @@ -366,9 +368,10 @@ public function archiveTable(int $id, string $userId): Table { /** * Unarchive a table for the given user. * - * If the user is the owner the entity flag is cleared and all per-user - * overrides are reset; otherwise the personal override is removed or - * set to false if the owner has archived the table. + * If the user is the owner the entity flag is cleared, which is the default for + * everyone who never made a choice of their own; a stored override keeps its + * user on their own choice. Anyone else has their personal override removed, or + * set to false while the owner has archived the table. * * @throws InternalError * @throws NotFoundError @@ -408,6 +411,14 @@ private function setArchivedForUser(int $id, string $userId, bool $archived): Ta } if ($isOwner && $entityArchived !== $archived) { + // Logged before the announcement, which swallows its own failures: an + // owner-level flip is a shared change and must leave an audit trail even + // when the activity or federation notice cannot be delivered. + $this->auditLogService->log( + $archived ? Operation::TableArchived : Operation::TableUnarchived, + $archived ? 'Table with ID: %s was archived' : 'Table with ID: %s was unarchived', + ['tableId' => $id], + ); $this->announceArchiveChange($table, $userId, $archived, $changes); } @@ -567,9 +578,6 @@ public function setOwner(int $id, string $newOwnerUserId, ?string $userId = null } $table = $this->mapper->update($table); $this->shareService->changeSenderForNode('table', $id, $newOwnerUserId, $userId); - if (!$this->permissionsService->canReadTable($table, $previousOwnerUserId)) { - $this->archiveService->removeUserOverride($previousOwnerUserId, Application::NODE_TYPE_TABLE, $id); - } return $table; }, $this->dbc); } catch (\Exception $e) { @@ -577,6 +585,10 @@ public function setOwner(int $id, string $newOwnerUserId, ?string $userId = null throw new InternalError('Failed to transfer table ownership: ' . $e->getMessage()); } + // Outside the transaction on purpose: the access check reads shares and group + // memberships, and a failure there must not take the completed transfer with it. + $this->archiveCleanupService->removeOverrideIfStale($previousOwnerUserId, Application::NODE_TYPE_TABLE, $id); + $event = new TableOwnershipTransferredEvent( table: $table, toUserId: $newOwnerUserId, @@ -585,6 +597,14 @@ public function setOwner(int $id, string $newOwnerUserId, ?string $userId = null $this->eventDispatcher->dispatchTyped($event); + // The transfer may have promoted the new owner's override to the shared flag, + // so the response has to carry the requesting user's own state, not that flag. + try { + $this->archiveService->enrichTablesWithArchiveState([$table], $userId); + } catch (OcpDbException $e) { + $this->logger->error($e->getMessage(), ['exception' => $e]); + } + return $table; } diff --git a/tests/unit/Circles/Stubs.php b/tests/unit/Circles/Stubs.php new file mode 100644 index 0000000000..c25577c724 --- /dev/null +++ b/tests/unit/Circles/Stubs.php @@ -0,0 +1,66 @@ + */ + public function getInheritedMembers(): array { + return []; + } + } + + class Member { + public const TYPE_SINGLE = 0; + public const TYPE_USER = 1; + public const TYPE_GROUP = 2; + public const TYPE_MAIL = 4; + public const TYPE_CONTACT = 8; + public const TYPE_CIRCLE = 16; + public const TYPE_APP = 10000; + + public function getUserId(): string { + return ''; + } + + public function getUserType(): int { + return self::TYPE_USER; + } + + public function getBasedOn(): ?Circle { + return null; + } + } +} + +namespace OCA\Circles\Events { + + use OCA\Circles\Model\Circle; + use OCA\Circles\Model\Member; + use OCP\EventDispatcher\Event; + + class CircleMemberRemovedEvent extends Event { + public function getCircle(): Circle { + return new Circle(); + } + + public function getMember(): ?Member { + return null; + } + } +} diff --git a/tests/unit/Federation/FederationProviderTest.php b/tests/unit/Federation/FederationProviderTest.php index 8fdc4a5ede..ad12a9bc13 100644 --- a/tests/unit/Federation/FederationProviderTest.php +++ b/tests/unit/Federation/FederationProviderTest.php @@ -19,6 +19,7 @@ use OCA\Tables\Db\ViewMapper; use OCA\Tables\Errors\FederationDisabledError; use OCA\Tables\Federation\FederationProvider; +use OCA\Tables\Service\ArchiveService; use OCA\Tables\Service\FederationService; use OCA\Tables\Service\Support\AuditLogServiceInterface; use OCP\Federation\Exceptions\ProviderCouldNotAddShareException; @@ -52,6 +53,7 @@ protected function setUp(): void { $this->cloudIdManager, $this->federationService, $this->auditLogService, + $this->createMock(ArchiveService::class), ); } diff --git a/tests/unit/Listener/ArchiveCleanupListenerTest.php b/tests/unit/Listener/ArchiveCleanupListenerTest.php new file mode 100644 index 0000000000..0c8ae7813a --- /dev/null +++ b/tests/unit/Listener/ArchiveCleanupListenerTest.php @@ -0,0 +1,120 @@ +cleanupService = $this->createMock(ArchiveCleanupService::class); + $this->listener = new ArchiveCleanupListener( + $this->cleanupService, + $this->createMock(LoggerInterface::class), + ); + } + + public function testUserRemovedFromGroupSchedulesGroupCleanup(): void { + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('bob'); + $group = $this->createMock(IGroup::class); + $group->method('getGID')->willReturn('team-a'); + $event = $this->createMock(UserRemovedEvent::class); + $event->method('getUser')->willReturn($user); + $event->method('getGroup')->willReturn($group); + + $this->cleanupService->expects($this->once()) + ->method('cleanupAfterMembershipLoss') + ->with(['bob'], 'team-a', ShareReceiverType::GROUP); + + $this->listener->handle($event); + } + + public function testCircleMemberRemovedDirectUserSchedulesCircleCleanup(): void { + $member = $this->mockMember(Member::TYPE_USER, 'bob'); + $event = $this->mockCircleEvent($member, 'circle-single-id'); + + $this->cleanupService->expects($this->once()) + ->method('cleanupAfterMembershipLoss') + ->with(['bob'], 'circle-single-id', ShareReceiverType::CIRCLE); + + $this->listener->handle($event); + } + + public function testCircleMemberRemovedSubCircleExpandsInheritedUsers(): void { + $inherited = [ + $this->mockMember(Member::TYPE_USER, 'alice'), + $this->mockMember(Member::TYPE_CIRCLE, ''), // nested non-user, must be skipped + $this->mockMember(Member::TYPE_USER, 'bob'), + ]; + $basedOn = $this->createMock(Circle::class); + $basedOn->method('getInheritedMembers')->willReturn($inherited); + + $member = $this->createMock(Member::class); + $member->method('getUserType')->willReturn(Member::TYPE_CIRCLE); + $member->method('getBasedOn')->willReturn($basedOn); + $event = $this->mockCircleEvent($member, 'circle-single-id'); + + $this->cleanupService->expects($this->once()) + ->method('cleanupAfterMembershipLoss') + ->with(['alice', 'bob'], 'circle-single-id', ShareReceiverType::CIRCLE); + + $this->listener->handle($event); + } + + public function testCircleMemberRemovedWithNullMemberDoesNothing(): void { + $event = $this->mockCircleEvent(null, 'circle-single-id'); + + $this->cleanupService->expects($this->never())->method('cleanupAfterMembershipLoss'); + + $this->listener->handle($event); + } + + public function testCircleMemberRemovedSubCircleWithNullBasedOnDoesNothing(): void { + $member = $this->createMock(Member::class); + $member->method('getUserType')->willReturn(Member::TYPE_CIRCLE); + $member->method('getBasedOn')->willReturn(null); + $event = $this->mockCircleEvent($member, 'circle-single-id'); + + $this->cleanupService->expects($this->never())->method('cleanupAfterMembershipLoss'); + + $this->listener->handle($event); + } + + private function mockMember(int $type, string $userId): Member&MockObject { + $member = $this->createMock(Member::class); + $member->method('getUserType')->willReturn($type); + $member->method('getUserId')->willReturn($userId); + return $member; + } + + private function mockCircleEvent(?Member $member, string $singleId): CircleMemberRemovedEvent&MockObject { + $circle = $this->createMock(Circle::class); + $circle->method('getSingleId')->willReturn($singleId); + $event = $this->createMock(CircleMemberRemovedEvent::class); + $event->method('getMember')->willReturn($member); + $event->method('getCircle')->willReturn($circle); + return $event; + } +} diff --git a/tests/unit/Service/TableServiceAuditLogTest.php b/tests/unit/Service/TableServiceAuditLogTest.php index c450bd7aac..77639693ca 100644 --- a/tests/unit/Service/TableServiceAuditLogTest.php +++ b/tests/unit/Service/TableServiceAuditLogTest.php @@ -14,6 +14,7 @@ use OCA\Tables\Db\Table; use OCA\Tables\Db\TableMapper; use OCA\Tables\Helper\UserHelper; +use OCA\Tables\Service\ArchiveCleanupService; use OCA\Tables\Service\ArchiveService; use OCA\Tables\Service\ColumnService; use OCA\Tables\Service\ContextService; @@ -71,6 +72,7 @@ protected function setUp(): void { $this->createMock(FederationService::class), $this->auditLogService, $this->createMock(ArchiveService::class), + $this->createMock(ArchiveCleanupService::class), ); } diff --git a/tests/unit/bootstrap.php b/tests/unit/bootstrap.php index ce471574ad..113ac4e91a 100644 --- a/tests/unit/bootstrap.php +++ b/tests/unit/bootstrap.php @@ -27,6 +27,10 @@ require_once __DIR__ . '/ShareReview/Stubs.php'; } +if (!class_exists(\OCA\Circles\Model\Member::class)) { + require_once __DIR__ . '/Circles/Stubs.php'; +} + if (!class_exists(TestCase::class)) { require_once('PHPUnit/Autoload.php'); } From bc4309f6b24db856c282c584a56896f71317fcdc Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Sat, 12 Sep 2026 11:27:02 +0200 Subject: [PATCH 6/9] feat(archive): add archiving to the navigation and store Add archive/unarchive actions to the table and context navigation items, a collapsible archived-applications section, and the matching store actions that guard against a missing local item. Assisted-by: ClaudeCode:claude-fable-5 Signed-off-by: Andy Scherzinger --- src/modules/modals/EditContext.vue | 9 ++- .../partials/NavigationContextItem.vue | 36 ++++++++- .../partials/NavigationTableItem.vue | 15 ++-- .../navigation/sections/Navigation.vue | 23 +++++- src/store/store.js | 74 ++++++++++++++++++- 5 files changed, 144 insertions(+), 13 deletions(-) diff --git a/src/modules/modals/EditContext.vue b/src/modules/modals/EditContext.vue index b096360b74..4d8d36fc9e 100644 --- a/src/modules/modals/EditContext.vue +++ b/src/modules/modals/EditContext.vue @@ -37,11 +37,11 @@
- + {{ t('tables', 'Show in app list') }}

- {{ t('tables', 'This can be overridden by a per-account preference') }} + {{ isArchived ? t('tables', 'An archived application is never listed in the app list') : t('tables', 'This can be overridden by a per-account preference') }}

@@ -130,6 +130,11 @@ export default { localContext() { return this.getContext(this.contextId) }, + // An archived application is filtered out of the app list entirely, so the + // setting would read as a promise the menu does not keep. + isArchived() { + return this.localContext?.archived ?? false + }, }, watch: { title() { diff --git a/src/modules/navigation/partials/NavigationContextItem.vue b/src/modules/navigation/partials/NavigationContextItem.vue index 73c0416a30..da1f6908ab 100644 --- a/src/modules/navigation/partials/NavigationContextItem.vue +++ b/src/modules/navigation/partials/NavigationContextItem.vue @@ -42,6 +42,22 @@ {{ t('tables', 'Transfer application') }} + + + + {{ t('tables', 'Archive application') }} + + + + + + {{ t('tables', 'Unarchive application') }} + + {{ t('tables', 'Delete application') }} - + + {{ t('tables', 'Show in app list') }} @@ -65,6 +83,8 @@ import PlaylistEdit from 'vue-material-design-icons/PlaylistEdit.vue' import FileSwapOutline from 'vue-material-design-icons/FileSwapOutline.vue' import DeleteOutline from 'vue-material-design-icons/TrashCanOutline.vue' import TrayArrowDown from 'vue-material-design-icons/TrayArrowDown.vue' +import ArchiveArrowDownOutline from 'vue-material-design-icons/ArchiveArrowDownOutline.vue' +import ArchiveArrowUpOutline from 'vue-material-design-icons/ArchiveArrowUpOutline.vue' import permissionsMixin from '../../../shared/components/ncTable/mixins/permissionsMixin.js' import svgHelper from '../../../shared/components/ncIconPicker/mixins/svgHelper.js' import { NAV_ENTRY_MODE } from '../../../shared/constants.ts' @@ -83,6 +103,8 @@ export default { FileSwapOutline, TableIcon, DeleteOutline, + ArchiveArrowDownOutline, + ArchiveArrowUpOutline, NcIconSvgWrapper, NcAppNavigationItem, NcActionButton, @@ -119,7 +141,7 @@ export default { }, methods: { - ...mapActions(useTablesStore, ['updateDisplayMode']), + ...mapActions(useTablesStore, ['updateDisplayMode', 'archiveContext', 'unarchiveContext']), emit, async editContext() { emit('tables:context:edit', this.context.id) @@ -156,6 +178,16 @@ export default { } return false }, + async toggleArchiveContext(archived) { + if (archived) { + await this.archiveContext({ id: this.context.id }) + } else { + await this.unarchiveContext({ id: this.context.id }) + } + // An archived application drops out of the app list, which the server + // renders, so the top bar keeps the stale entry until it is rebuilt. + await rebuildNavigation() + }, async changeDisplayMode() { const value = !this.showInNavigation const displayMode = value ? NAV_ENTRY_MODE.NAV_ENTRY_MODE_ALL : NAV_ENTRY_MODE.NAV_ENTRY_MODE_HIDDEN diff --git a/src/modules/navigation/partials/NavigationTableItem.vue b/src/modules/navigation/partials/NavigationTableItem.vue index 14df41fbc2..92651faf71 100644 --- a/src/modules/navigation/partials/NavigationTableItem.vue +++ b/src/modules/navigation/partials/NavigationTableItem.vue @@ -109,7 +109,7 @@ - {{ t('tables', 'Archive table') }}