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

fix(isLegitimatedForUserId): Setup mountpoints to check file access #40482

Merged
merged 2 commits into from
Oct 23, 2023

Conversation

mejo-
Copy link
Member

@mejo- mejo- commented Sep 18, 2023

This fixes workflows on groupfolders, as it will consider access to files in groupfolders.

It also fixes false positives where access to files was limited by other means not taken into account before, e.g. access control.

For postDelete events, check for permissions of the parent folder instead, as the file itself no longer exists.

Fixes: nextcloud/flow_notifications#71

Idea taken from #38946

Checklist

@blizzz
Copy link
Member

blizzz commented Sep 18, 2023

@icewind1991 do you think this can be a performance killer?

@mejo- mejo- force-pushed the fix/workflowengine_fileaccess branch from fe3d6f4 to 76a156d Compare September 18, 2023 14:35
@mejo- mejo- force-pushed the fix/workflowengine_fileaccess branch from 76a156d to a4d98c8 Compare September 20, 2023 10:29
@icewind1991
Copy link
Member

@icewind1991 do you think this can be a performance killer?

It isn't worse than the previous code (getAccessList is probably just as bad), but if this gets called in a loop (like for every file in a folder that is being accessed) than performance will be dead yes.
I do not know enough about workflow to know how this code is called

@mejo- mejo- force-pushed the fix/workflowengine_fileaccess branch from a4d98c8 to 9df4da5 Compare September 20, 2023 10:46
@mejo- mejo- requested a review from icewind1991 September 20, 2023 10:46
@mejo- mejo- force-pushed the fix/workflowengine_fileaccess branch 2 times, most recently from dac55c5 to a07df9a Compare September 20, 2023 11:26
@mejo- mejo- marked this pull request as ready for review September 20, 2023 12:36
@mejo- mejo- added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Sep 20, 2023
@mejo- mejo- added this to the Nextcloud 28 milestone Sep 20, 2023
@mejo-
Copy link
Member Author

mejo- commented Sep 20, 2023

Cypress test failures seem unrelated. PR should be ready for review 😊

@mejo-
Copy link
Member Author

mejo- commented Sep 20, 2023

By the way, I manually tested the following scenarios (all with a flow_notification flow defined by admin for files with a system tag):

  • User jane creates/changes a file in a tagged folder shared with her by admin -> isLegitimatedForUserId returns true
  • User jane deletes a file in a tagged folder shared with her by admin -> isLegitimatedForUserId returns true
  • User jane creates/changes a file in a tagged groupfolder where admin has access -> isLegitimatedForUserId returns true
  • User jane deletes a file in a tagged groupfolder where admin has access -> -> isLegitimatedForUserId returns true
  • User jane creates/changes a file in a tagged groupfolder where admin doesn't have access -> isLegitimatedForUserId returns false
  • User jane deletes a file in a tagged groupfolder where admin doesn't have access -> -> isLegitimatedForUserId returns false

apps/workflowengine/lib/Entity/File.php Outdated Show resolved Hide resolved
apps/workflowengine/lib/Entity/File.php Outdated Show resolved Hide resolved
@mejo- mejo- force-pushed the fix/workflowengine_fileaccess branch 2 times, most recently from 28fdcb9 to 5858970 Compare September 25, 2023 16:27
apps/workflowengine/lib/Entity/File.php Fixed Show fixed Hide fixed
lib/private/Files/Mount/Manager.php Fixed Show fixed Hide fixed
lib/public/Files/Mount/IMountManager.php Fixed Show fixed Hide fixed
lib/public/Files/Mount/IMountManager.php Fixed Show fixed Hide fixed
lib/public/Files/Mount/IMountManager.php Fixed Show fixed Hide fixed
apps/workflowengine/lib/Entity/File.php Fixed Show fixed Hide fixed
@mejo- mejo- force-pushed the fix/workflowengine_fileaccess branch 2 times, most recently from 74b0139 to 576eba9 Compare September 26, 2023 12:59
@mejo- mejo- force-pushed the fix/workflowengine_fileaccess branch 2 times, most recently from a72a31d to ec31c90 Compare October 20, 2023 15:32
@mejo- mejo- requested a review from icewind1991 October 20, 2023 15:32
@mejo- mejo- force-pushed the fix/workflowengine_fileaccess branch from ec31c90 to c7d209d Compare October 23, 2023 14:14
@mejo- mejo- requested a review from juliusknorr October 23, 2023 17:16
Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

One psalm complaint, but otherwise seems reasonable 👍

mejo- added 2 commits October 23, 2023 20:47
This fixes workflows on groupfolders, as it will consider access to
files in groupfolders.

It also fixes false positives where access to files was limited by other
means not taken into account before, e.g. access control.

For postDelete events, check for permissions of the parent folder
instead, as the file itself no longer exists.

Fixes: nextcloud/flow_notifications#71

Signed-off-by: Jonas <[email protected]>
@mejo- mejo- force-pushed the fix/workflowengine_fileaccess branch from c7d209d to 7441ce2 Compare October 23, 2023 18:50
@mejo-
Copy link
Member Author

mejo- commented Oct 23, 2023

Thanks @juliushaertl. I addressed the psalm complaint now.

apps/workflowengine/lib/Entity/File.php Dismissed Show dismissed Hide dismissed
@mejo-
Copy link
Member Author

mejo- commented Oct 23, 2023

Will need manual backports without the public interface changes to stable27 and stable26.

@juliusknorr juliusknorr merged commit 45d7612 into master Oct 23, 2023
39 checks passed
@juliusknorr juliusknorr deleted the fix/workflowengine_fileaccess branch October 23, 2023 21:25
@mejo-
Copy link
Member Author

mejo- commented Oct 23, 2023

/backport to stable27

1 similar comment
@mejo-
Copy link
Member Author

mejo- commented Oct 23, 2023

/backport to stable27

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No notification from groupfolders
4 participants