🕵️ Add sharereview support by AndyScherzinger · Pull Request #2711 · nextcloud/tables

AndyScherzinger · 2026-06-07T12:59:48Z

🏁 Checklist

⛑️ Tests (unit and/or integration) are included or not needed
🔙 Backport requests are created or not needed: /backport to stableX.X
📅 Milestone is set
🌸 PR title is meaningful (if it should be in the changelog: is it meaningful to users?)

🤖 AI (if applicable)

The content of this PR was partly or fully generated using AI

enjeck

To be sure, it's intentional to be able to delete shares from resources that you don't own/manage? And that usually-secret public share tokens are listed too?

blizzz · 2026-06-17T11:11:28Z

+	private const SHARE_TABLE = 'tables_shares';
+	private const TABLES_TABLE = 'tables_tables';
+	private const VIEWS_TABLE = 'tables_views';
+	private const CONTEXTS_TABLE = 'tables_contexts_context';


I do not like that database resource names leak from their actual place, which are the mappers.

Alternative: make it a public class constant in the respective mapper.

blizzz · 2026-06-17T11:16:15Z

+	/** @return list<array<string, mixed>> */
+	private function fetchAllShares(): array {
+		try {
+			$qb = $this->db->getQueryBuilder();
+			$qb->select(
+				'id', 'sender', 'receiver', 'receiver_type', 'node_id', 'node_type',
+				'token', 'password',
+				'permission_read', 'permission_create', 'permission_update',
+				'permission_delete', 'permission_manage',
+				'created_at', 'last_edit_at'
+			)->from(self::SHARE_TABLE)
+				->orderBy('id', 'ASC');
+			$result = $qb->executeQuery();
+			$rows = $result->fetchAll();
+			$result->closeCursor();
+			return $rows;
+		} catch (Exception $e) {
+			$this->logger->error('Tables ShareReview: failed to fetch shares: {message}', ['message' => $e->getMessage()]);
+			return [];
+		}
+	}


Leaks responsibility, should be provided as a method on (Service and) Mapper level.

A think to consider: can be implemented as Generator (we do it to seldomly) for a smaller memory footprint. Especially since this one reads all shares. Think of big instances. In worst case (not sure it is likely, but depends) this can ran into a memory exhaustion – but also on the callee side if really everything is consumed at once.

blizzz · 2026-06-17T11:22:28Z

+				$qb = $this->db->getQueryBuilder();
+				$qb->select('id', $nameColumn)
+					->from($table)
+					->where($qb->expr()->in('id', $qb->createNamedParameter($chunk, IQueryBuilder::PARAM_INT_ARRAY)));
+				$result = $qb->executeQuery();
+				$rows = $result->fetchAll();
+				$result->closeCursor();
+				foreach ($rows as $row) {
+					$map[(int)$row['id']] = (string)$row[$nameColumn];
+				}
+			} catch (Exception $e) {
+				$this->logger->error('Tables ShareReview: failed to fetch names from {table}: {message}', ['table' => $table, 'message' => $e->getMessage()]);
+			}


as mentioned, database queries should be done in the respectives mappers and exposed perhaps/ideally via Service.

blizzz · 2026-06-17T11:24:53Z

+
+		$formatted = [];
+		foreach ($rawShares as $share) {
+			$formatted[] = [


Ideally this is not a random associated array, but a specific class (can be a DTO/data containers without further logic). This is way more maintainable and also has a better memory footprint.

A factory class or callback could be provided by the ShareReview app when calling, then you have it more formalized/API-like as well.

blizzz · 2026-06-17T11:28:45Z

+			return $this->l10n->t('%s (View)', [$viewNames[$nodeId] ?? $this->l10n->t('View %s', [$nodeId])]);
+		}
+		if ($nodeType === self::NODE_TYPE_CONTEXT) {
+			return $this->l10n->t('%s (Context)', [$contextNames[$nodeId] ?? $this->l10n->t('Context %s', [$nodeId])]);


Application is the user facing term, Context is only the internal-technical name.

blizzz · 2026-06-17T11:35:58Z

+		try {
+			$this->contextNavigationMapper->deleteByShareId((int)$shareId);
+		} catch (Exception $e) {
+			$this->logger->error('Tables ShareReview: failed to clean up context navigation for share {id}: {message}', ['id' => $shareId, 'message' => $e->getMessage()]);


This will be more rule than exception, as we can expect this being called for Tables and Views way more often than for Contexts. One or more entries will only be against shared contexts.

Hence it would result in confusing, at worst also spammy, log messages.

AndyScherzinger · 2026-06-19T19:10:28Z

@enjeck regarding:

To be sure, it's intentional to be able to delete shares from resources that you don't own/manage? And that usually-secret public share tokens are listed too?

Yes BUT I will change the implementation in the way that it uses an event mechanism like in other places with similar calling patterns, to ensure the user would have that permission as checked then by the share review app. SO I reset the PR to draft, to reflect the need for further improvements.

Assisted-by: Claude Code:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

…ayer Assisted-by: ClaudeCode:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

Assisted-by: ClaudeCode:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

… shares Assisted-by: ClaudeCode:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

…share review Use last_edit_at as the share time when it is more recent than created_at, falling back to created_at otherwise. This ensures that shares whose permissions were modified after creation show the more recent timestamp, allowing the ShareReview app to surface recently-changed shares accurately. Assisted-by: ClaudeCode:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

Database resource names and raw query logic no longer leak from ShareReviewSource into the application layer. ShareMapper gains findAllRaw() to fetch all shares without exposing the table name. TableMapper and ViewMapper gain findIdToTitleMap(), and ContextMapper gains findIdToNameMap() — each mapper owns its own SQL and table name. ShareReviewSource now depends on the three node-type mappers instead of IDBConnection directly. The tests are updated to mock at the mapper level, eliminating all raw IQueryBuilder/IDBConnection stub setup. Assisted-by: ClaudeCode:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

Replace the anonymous associative array built inline in getShares() with a proper ShareInfo value object. ShareReviewSource now constructs ShareInfo instances via buildShareInfo() and converts them to the array format required by ISource::getShares() through ShareInfo::toArray(). This improves maintainability and gives static analysis a concrete type to reason about instead of an ad-hoc array shape. Assisted-by: ClaudeCode:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

'Context' is an internal technical term. The user-facing name for the same concept is 'Application'. Update the share review object label so users see e.g. 'My App (Application)' instead of 'My App (Context)'. Assisted-by: ClaudeCode:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

Calling contextNavigationMapper::deleteByShareId() for every share deletion would be misleading since context navigation entries only exist for context-type shares. Fetching the share entity first lets us guard the cleanup call behind a node-type check, so it only runs when the deleted share actually belongs to a context. This removes the confusing and potentially spammy error log path that could appear for ordinary table and view share deletions. Assisted-by: ClaudeCode:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

Assisted-by: Claude Code:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

AndyScherzinger added this to the v2.3.0 milestone Jun 7, 2026

AndyScherzinger added enhancement New feature or request 2. developing Work in progress AI assisted labels Jun 7, 2026

AndyScherzinger force-pushed the feat/noid/sharereview branch from a74331b to 411e1f7 Compare June 7, 2026 13:34

AndyScherzinger marked this pull request as ready for review June 7, 2026 18:35

AndyScherzinger requested review from blizzz and enjeck as code owners June 7, 2026 18:35

AndyScherzinger changed the title ~~Add sharereview support~~ 🕵️ Add sharereview support Jun 7, 2026

AndyScherzinger added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jun 7, 2026

AndyScherzinger force-pushed the feat/noid/sharereview branch 2 times, most recently from 569f563 to b35b188 Compare June 11, 2026 15:39

enjeck reviewed Jun 15, 2026

View reviewed changes

Comment thread lib/ShareReview/ShareReviewSource.php Outdated

Comment thread lib/ShareReview/ShareReviewSource.php Outdated

Comment thread lib/ShareReview/ShareReviewSource.php Outdated

Comment thread lib/ShareReview/ShareReviewSource.php Outdated

AndyScherzinger force-pushed the feat/noid/sharereview branch 5 times, most recently from d53e2d8 to d2e8862 Compare June 15, 2026 22:08

enjeck reviewed Jun 17, 2026

View reviewed changes

blizzz requested changes Jun 17, 2026

View reviewed changes

AndyScherzinger added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jun 19, 2026

AndyScherzinger marked this pull request as draft June 19, 2026 19:57

AndyScherzinger force-pushed the feat/noid/sharereview branch 2 times, most recently from 575300f to 873b425 Compare June 22, 2026 16:30

AndyScherzinger added 3 commits June 22, 2026 19:09

feat(sharereview): add listener registering Tables as a share source

2c6378f

Assisted-by: Claude Code:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

feat(sharereview): add ShareReviewSource with constructor and getName()

daf6bfd

Assisted-by: Claude Code:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

feat(sharereview): implement getShares() with batched name lookups

2d10049

Assisted-by: Claude Code:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

AndyScherzinger added 15 commits June 22, 2026 19:09

feat(sharereview): implement deleteShare() via direct SQL with logging

43d53b2

Assisted-by: Claude Code:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

feat(sharereview): register ShareReview listener on SourceEvent

ba3f40a

Assisted-by: Claude Code:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

style(sharereview): apply coding standards and Psalm fixes

7a4b934

Assisted-by: Claude Code:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

test(sharereview): add unit tests for ShareReviewSource

9b9dcee

Assisted-by: Claude Code:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

fix(sharereview): harden and optimize implementation and testing

e4eac87

Assisted-by: Claude Code:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

refactor(sharereview): replace raw SQL in deleteShare() with mapper l…

dd63e33

…ayer Assisted-by: ClaudeCode:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

fix(sharereview): remove redundant app key from getShares() output

d1c08a9

Assisted-by: ClaudeCode:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

fix(sharereview): localize user-facing strings in ShareReviewSource

274136b

Assisted-by: ClaudeCode:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

fix(sharereview): log warnings for unknown node and receiver types of…

f01f586

… shares Assisted-by: ClaudeCode:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

feat(sharereview): gate ACL deletion behind event-based access check

e8dc75a

Assisted-by: Claude Code:claude-sonnet-4-6 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>

AndyScherzinger force-pushed the feat/noid/sharereview branch from 873b425 to e8dc75a Compare June 22, 2026 17:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🕵️ Add sharereview support#2711

🕵️ Add sharereview support#2711
AndyScherzinger wants to merge 18 commits into
mainfrom
feat/noid/sharereview

AndyScherzinger commented Jun 7, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

enjeck left a comment

Uh oh!

blizzz Jun 17, 2026

Uh oh!

blizzz Jun 17, 2026

Uh oh!

blizzz Jun 17, 2026

Uh oh!

blizzz Jun 17, 2026

Uh oh!

blizzz Jun 17, 2026

Uh oh!

blizzz Jun 17, 2026

Uh oh!

AndyScherzinger commented Jun 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

AndyScherzinger commented Jun 7, 2026

🏁 Checklist

🤖 AI (if applicable)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

enjeck left a comment

Choose a reason for hiding this comment

Uh oh!

blizzz Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

blizzz Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

blizzz Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

blizzz Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

blizzz Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

blizzz Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

AndyScherzinger commented Jun 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants