Skip to content

Conversation

@ArtificialOwl
Copy link
Member

@ArtificialOwl ArtificialOwl commented Oct 31, 2022

Beware, spooky review required 🎃

when running with activity_use_cached_mountpoints on a file:

  • get all users [1] that have basic groupfolders' access
  • get rules related to path to said file
  • get list of users that might be affected by said rules
  • verify access to those users
  • remove users without read permission on said file from [1]

Copy link
Member

@icewind1991 icewind1991 left a comment

Choose a reason for hiding this comment

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

I'm not a fan of having additional explicit cross-app dependencies.

We can maybe add an storage extention interface to core that has a filterUserAccess or something similar to decouple things, but that can be done later.

Overall the approach of the filtering is fine and shouldn't be to bad for performance, once the listed issues are addressed this is good from my pov

@ArtificialOwl ArtificialOwl force-pushed the enh/noid/check-groupfolders-acl branch from e7c53f9 to e87988f Compare November 9, 2022 11:18
@PVince81
Copy link
Member

not a fan of cross-app either.
still, for backportability let's stick with this solution for now

once merged and backported we can look into make it cleaner if low effort

Signed-off-by: Maxence Lange <[email protected]>
@ArtificialOwl ArtificialOwl force-pushed the enh/noid/check-groupfolders-acl branch from e87988f to 946aecb Compare November 23, 2022 16:45
@ArtificialOwl
Copy link
Member Author

  • fixed the 2nd location for Constants::PERMISSION_READ
  • squashed
  • rebased

Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍

@PVince81 PVince81 merged commit 26a23b3 into master Nov 23, 2022
@delete-merged-branch delete-merged-branch bot deleted the enh/noid/check-groupfolders-acl branch November 23, 2022 17:49
@PVince81
Copy link
Member

/backport to stable25

@fschrempf
Copy link
Contributor

We can maybe add an storage extention interface to core that has a filterUserAccess or something similar to decouple things, but that can be done later.

once merged and backported we can look into make it cleaner if low effort

This is a real problem and it's one example for why NC code probably gets harder and harder to maintain. You are adding technical debt and "can be done later" and "make it cleaner if low effort" effectively means that it will never be done unless there's really someone who tracks these issues.

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.

6 participants