-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Implement Directory Check #50258
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
Implement Directory Check #50258
Conversation
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
|
@susnux @joshtrichards before trying to resolve the conflicts, are you willing to accept this feature or is there anything else I can provide from my side? |
Not my area, better to align with @blizzz |
|
Watching this PR. Recently added a similar path check to a plugin I maintain (cwilby/nextcloud-workflow-media-converter@1412580). When this gets merged (and it should be, to enable a lot of folder-structure driven automation use cases) I'll remove it from there. |
|
@blizzz any news on this, please? |
4654ddb to
7defb64
Compare
|
Thanks for your feedback @susnux. I just incorporated your suggested changes and rebased onto current master. |
7defb64 to
29f57db
Compare
29f57db to
a3a7f2e
Compare
d681c89 to
124b13b
Compare
|
Please help me understand why this is needed, and why not working with tags instead. Name and Paths are not reliable. They change. They might be different across users are shares can be moved and renamed individually. |
|
@blizzz So in my use case I would use the Using tags would be possible, of course, but would introduce another step for the user. I totally agree that directory paths are somehow fragile. Nevertheless, in my case the path will never change and I would see this as a feature for advanced users to keep track of their configured workflows. @cwilby maybe you would like to add some comments, too? |
|
I think @R0Wi addressed most of the reasons why this might be a useful feature to have, @blizzz is right to question from maintenance perspective. On my end it's something that's bubbled up from frequent issues/feature requests on https://github.com/cwilby/nextcloud-workflow-media-converter, where similar folder structures are used frequently (e.g. by Year). |
|
/compile rebase |
* Partially implements #27591 Signed-off-by: Robin Windey <[email protected]>
Signed-off-by: nextcloud-command <[email protected]>
124b13b to
69a3604
Compare
nickvergessen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This filter was intentionally never added as it adds a security issue:
- Create a folder
Finance - Set up Files Access Control rule with
/^Finance\/.+$/i(and for easier testing "Not member of admin group", real case would be not from internal network or something a like) - Share a document from in there with a person that is not an admin
Expectation:
Person can not see the preview nor download nor online edit the document
Actual:
Person can see the preview, download and online edit the document
That is exactly why we have to go via systemtags at the moment: https://docs.nextcloud.com/server/latest/admin_manual/file_workflows/access_control.html#denying-access-to-folders
Similarly autotagging would unintentionally not trigger workflows if someone did something as a share recipient with a different path.
Unless a proposal how to fix this was provided by end of next week, I'll have to revert this on the last day before my vacation, to prevent accidentally shipping it with 32 creating a security fiasco
| operators: stringOrRegexOperators, | ||
| placeholder: (check) => { | ||
| if (check.operator === 'matches' || check.operator === '!matches') { | ||
| return '/^myfolder/.+$/i' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an invalid placeholder.
| return '/^myfolder/.+$/i' | |
| return '/^myfolder\/.+$/i' |
|
Thanks for your feedback, @nickvergessen! Indeed I didn't take into consideration the A slightly different approach would be to use directory ids instead of paths. I didn't verify it but I could imagine that using a Nextcloud directory picker UI control (instead of letting the user type in some free text for the path) would help here - assuming that the directory picker uses ids under the hood. Of course this might be a bigger change so my proposal would be:
For the latter I would kindly ask the code owners to give some feedback here. A little comment on this PR: I don't want to complain since I know that all of you guys are pretty busy with other more important stuff, but nevertheless this PR has been created in January this year. Even though the change might seem to be small, it took me a couple of hours of my spare time to implement this and especially keep the code up to date and accommodate changes being made to the code base in the meantime. So next time I would really love to get some quick feedback earlier in the development phase to decide if this feature should be implemented at all or if there are arguments against it. I guess this would have saved time for all of us. Thank you! |
|
Just a quick thought, maybe you can get inspiration from https://github.com/nextcloud/server/blob/master/apps/workflowengine/lib/Check/FileSystemTags.php#L86-L145
Very sorry about this. If I would have noticed the PR I would have left the same comment much earlier. I will try to set myself as a code owner for the PHP code of the workflowengine as I know quite a bit about it. |
|
I will try to put something together soon - maybe let's connect once again on this after your vacation 👍 (enjoy btw. 😄)
Great! Thank you so much |
Summary
Partially implements #27591 by introducing a new
Directory-check. This check can be used by a user to determine if a file trigger has been executed inside of a certain directory. Also, if one uses thematchor!matchoperators, a recursive/nested check can be realized (something like "if the trigger has been executed in any subfolder of ...")Checklist