Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enh: db schema change #79

Merged
merged 14 commits into from
Dec 13, 2024
Merged

enh: db schema change #79

merged 14 commits into from
Dec 13, 2024

Conversation

kyteinsky
Copy link
Contributor

I'm sorry for the big PR :(

  • update the existing apis to the new db schema
  • add routes like deleteProvider, deleteUser, updateAccess, updateAccessProvider, updateAccessDeclarative
  • listener for user deletion
  • convert DeleteService and related db schema and job to ActionService and such
  • update group share event handling in file listener
  • add methods to the content manager and deprecate some of them

Signed-off-by: Anupam Kumar <[email protected]>
@@ -77,6 +80,7 @@ public function register(IRegistrationContext $context): void {
$context->registerEventListener(NodeRemovedFromCache::class, FileListener::class);
$context->registerEventListener(NodeWrittenEvent::class, FileListener::class);
$context->registerEventListener(AppDisableEvent::class, AppDisableListener::class);
$context->registerEventListener(UserDeletedEvent::class, UserDeletedListener::class);
Copy link
Member

@marcelklehr marcelklehr Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we also need Group member added/removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about adding it later with complete group based ACLs. We won't need to translate group -> users then, only the list of groups the user is part of, on the fly when querying.
In the backend it would be checked if a file can be accessed by the user or one of the groups the user is part of.
We can maintain a local list in the backend for user-group mappings but fetching it on the fly is not expensive, so we won't even need the listener if a user was added/removed from a group.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mmh. I'd stick with user-based ACLs for this iteration, I think.

$qb = $this->db->getQueryBuilder();
$qb->delete($this->getTableName())
->where($qb->expr()->eq('id', $qb->createPositionalParameter($file->getId())))
->where($qb->expr()->in('id', $qb->createPositionalParameter($fileIds, IQueryBuilder::PARAM_INT_ARRAY)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet!

Copy link
Member

@marcelklehr marcelklehr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woop woop

@kyteinsky kyteinsky marked this pull request as ready for review December 12, 2024 19:11
Signed-off-by: Anupam Kumar <[email protected]>

// the user(s) who have access to the file through file mounts, excluding the user(s)
// who have really lost access to the file and are present in $fileUserIds list
$realFileUserIds = array_diff($fileUserIds, $reallyUnsharedWith);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scenario:

  1. File A is in a groupfolder
  2. File A is shared with group B additionally
  3. File A is unshared with group B but remains in groupfolder
  4. Will user in group B lose access?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is assumed that groupfolders and other non-share mounts do not also have a share.
Either case we'll be allowing more or less than actual access due to mount cache.

$this->logger->error('Could not open file ' . $file->getPath() . ' for reading', ['exception' => $e]);
continue;
} catch (LockedException $e) {
$retryQFiles[] = $queueFile;
$this->logger->info('File ' . $file->getPath() . ' is locked, could not read for indexing. Adding it to the next batch.');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Smart!

@marcelklehr marcelklehr merged commit 9ae31b5 into main Dec 13, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants