fix(files_sharing): stop ignoring shares without a usergroup entry when filtering by path #57930
Conversation
artonge
requested review from
ArtificialOwl,
CarlSchwan,
come-nc and
icewind1991
and removed request for
a team
artonge
added
bug
3. to review
come-nc
left a comment
come-nc
left a comment
There was a problem hiding this comment.
Won’t that cause an issue with non-accepted group shares?
Or if the user left the share?
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
salmart-dev
dismissed
their stale review
Moving the path filtering to the resolve step would remove the filtering on the "parent" share, having the result of returning all shares received by a user where the only resolved ones are the ones where the path condition matches. This is not what we want.
Previously, this was preventing users without a specific USERGROUP entry to see GROUP shares. Signed-off-by: Louis Chmn <[email protected]> Signed-off-by: Salvatore Martire <[email protected]>
salmart-dev
force-pushed
the
artonge/fix/share_resolution
branch
from
f4a8d15 to
073d980
Compare
|
Edited the PR to avoid filtering by path only during the resolution step, as that would cause only the shares with a specific path to be resolved + would return shares unrelated to the path, since the first step would not filter by path any longer. The new changes:
This fixes the case where a share's permission get reduced because some usergroup rows may be missing for a specific share/user and others may be present resulting in incorrect permissions. |
Try the same on stable33, there seem to be some things working on master that aren't working on stable33 for some reason. |
salmart-dev
changed the title
Signed-off-by: Salvatore Martire <[email protected]>
Signed-off-by: Salvatore Martire <[email protected]>
Do you have more context? I can reproduce this issue also on stable33 |
| $qb->expr()->like('s.file_target', $qb->createNamedParameter($this->dbConn->escapeLikeParameter($path) . '_%')), | ||
| $qb->expr()->like('sc.file_target', $qb->createNamedParameter($this->dbConn->escapeLikeParameter($path) . '_%')), | ||
| ) | ||
| ); | ||
| } else { | ||
| $qb->andWhere($qb->expr()->eq('sc.file_target', $qb->createNamedParameter($path))); | ||
| $qb->andWhere( | ||
| $qb->expr()->orX( | ||
| $qb->expr()->eq('s.file_target', $qb->createNamedParameter($path)), | ||
| $qb->expr()->eq('sc.file_target', $qb->createNamedParameter($path)), |
There was a problem hiding this comment.
Shouldn't these only match on sc.file_target? The file_target of the share parent is irrelevant, because only the usergroup share will dictate where the mount will actually be. The current filter could return some false-positives where the group share matches, while the usergroup share doesn't.
There was a problem hiding this comment.
I think I agree, see #51602 , the target of the USERGROUP share is the one holding the actual path to the share.
But then that breaks again when the USERGROUP share is missing 🫨
There was a problem hiding this comment.
But then that breaks again when the USERGROUP share is missing
But by matching against the group share it's also broken.
So do we need to sync all missing usergroup shares?
There was a problem hiding this comment.
Sorry, GitHub keeps not refreshing fast enough (not sure if it's Firefox or just GH) so I am seeing this only now.
Shouldn't these only match on sc.file_target? The file_target of the share parent is irrelevant, because only the usergroup share will dictate where the mount will actually be. The current filter could return some false-positives where the group share matches, while the usergroup share doesn't.
Yes, the problem would happen in the resolution step though, as if the share was renamed and for any reason the previous target is used, the code here would return the GROUP row, but then the resolution step would not return the USERGROUP row and we'd fail to patch the share object.
I will change the query again, Louis had a good suggestion for it: WHERE ... (sc.file_target = ... OR (sc.file_target IS NULL AND s.file_target = ...)), this should fix the problem, as we will only use s.file_target if the user actually doesn't have a USERGROUP row.
|
/backport to stable33 |
Signed-off-by: Salvatore Martire <[email protected]>
come-nc
left a comment
come-nc
left a comment
There was a problem hiding this comment.
I cannot see any problem anymore, does not mean there are none 🙈
Previously, this was preventing users without a specific USERGROUP entry to see GROUP shares.
The path filtering is now done at resolution time. This is less efficient, but keeps the old behavior.(crossed out from Salvatore)I think we should probably update the other share providers to follow the same logic.