🕵️ Add sharereview support#2711
Conversation
AndyScherzinger
added
enhancement
a74331b to
411e1f7
Compare
AndyScherzinger
added
3. to review
569f563 to
b35b188
Compare
d53e2d8 to
d2e8862
Compare
enjeck
left a comment
enjeck
left a comment
There was a problem hiding this comment.
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?
| 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'; |
There was a problem hiding this comment.
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.
| /** @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 []; | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
| $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()]); | ||
| } |
There was a problem hiding this comment.
as mentioned, database queries should be done in the respectives mappers and exposed perhaps/ideally via Service.
|
|
||
| $formatted = []; | ||
| foreach ($rawShares as $share) { | ||
| $formatted[] = [ |
There was a problem hiding this comment.
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.
| 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])]); |
There was a problem hiding this comment.
Application is the user facing term, Context is only the internal-technical name.
| 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()]); |
There was a problem hiding this comment.
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
added
2. developing
|
@enjeck regarding:
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. |
873b425 to
e8dc75a
Compare
e8dc75a to
affa711
Compare
15ceadf to
b0444a4
Compare
77c0ec2 to
ad3985f
Compare
…review apps via OCP\Share\ShareReview Implement IShareReviewSource listing all tables(table/view/context) shares with their capabilities mapped to ShareReviewPermission entries, streaming share rows from the database via a generator to keep the memory footprint low on large instances, gate deletions behind the ShareReviewAccessCheckEvent authorization check with share-delete activity entries for auditability, and register the source via RegisterShareReviewSourceEvent. Assisted-by: Claude Code:claude-fable-5 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
ad3985f to
390d769
Compare
3 participants
🏁 Checklist
/backport to stableX.X🤖 AI (if applicable)