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

Check if user can update node #286

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

XueSheng-GIT
Copy link
Contributor

...and change to an alternative user if not (fixes ocr for filedrop and groupfolder acl).

@R0Wi This is just a rough proof of concept to fix #273
In comparison to the discussion at #110 (#116) this approach does first check whether the current user has the permissions to update the node. If not, an alternative user is chosen (but not owner, because this may fail for groupfolders).

This draft does not include a setting, test or further documentation. I'm also wondering whether the manual implementation for createNewFileVersion is really required.

$this->createNewFileVersion($newFilePath, $fileContent, $fileId, $fileMtime);

Wouldn't $node->putContentbe enough?
https://github.com/nextcloud/server/blob/2651d4964ec26696b5f8b1f5db48588a5d09ab01/lib/private/Files/Node/File.php#L49

...and change to an alternative user if not.

Signed-off-by: XueSheng-GIT <[email protected]>
@R0Wi
Copy link
Contributor

R0Wi commented Dec 17, 2024

Thanks for opening this PR. I like the idea of using a fallback user in case the original user doesn't have the proper permissions to overwrite the file. Although I'd prefer to not pick any random user, this could lead to confusion.

Maybe let's do some testing again with $node->getOwner() instead? I could also imagine having an admin global config option to set a fallback user explicitly.

To answer your question: the implementation for creating a new file version is needed since it ensures that we're not stuck in an endless loop. Also it covers the case where a new file is created instead of creating a new file version (for example when processing jpg to pdf).

@XueSheng-GIT
Copy link
Contributor Author

Maybe let's do some testing again with $node->getOwner() instead? I could also imagine having an admin global config option to set a fallback user explicitly.

I started with getOwner and failed with groupfolders (enabled ACL with restriction to modify). The user provided by getOwner may not have the necessary rights to modify the file.
It seems there's no single owner for groupfolders (with guaranteed access to the file), so that I decided just to go for a random user who has the rights to modify the file.
I totally agree that this is not ideal and could lead to confusion.

To minimize this risk, it could be a two stage fallback.

  1. getOwner
  2. random user

@R0Wi
Copy link
Contributor

R0Wi commented Dec 19, 2024

I'm still not happy with picking a random user tbh

Option 1: we need to introduce a new setting "fallback user", which is globally applied and can be only set by the admin
Option 2: since the user running the OCR process obviously has the permissions to create new files but not to update them, we could also just create a new file next to the original and suffix it with something like <filename>_OCR.pdf.

@XueSheng-GIT
Copy link
Contributor Author

I haven't had Option 2 in mind yet. Could be a good alternative.

Must it be option 1 or 2? I could imagine that each approach has it's advantages and disadvantages so that it may depend on the specific use case which one makes more sense.
Why not adding this admin setting for handling restricted file modification rights to choose between (i) do nothing (log a warning), (ii) create new file (with appendix "_OCR") and (iii) force file versioning with random user?

If you really feel unhappy with this forced versioning (random user) because of potential confusion, it could be helpful to provide some clarity via the label of the current version. Instead of just adding "Before OCR", it could be an addition like "Before OCR (OCR will be done with random user)".

@R0Wi
Copy link
Contributor

R0Wi commented Dec 24, 2024

To keep things simple I would aim for appending a suffix to the file. Otherwise features like for example "notifications" (which are bound to a user) would need to be reworked as well. Could be an option for the future but for now I'd go with the suffix approach.

@XueSheng-GIT would you feel comfortable to implement this?

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.

OCR fails on file drop (shared upload folder)
2 participants