Correctly look up filename when logged in but inaccessible #48295
Conversation
If the user is not logged in, and we can't find the file owner, we guess the file owner from the path. Most likely the username in the path has some access to the file, so we can find the filename. However, in the case where we are logged in, and the file is a group share, both the username and the owner will be the same UID. Which is fine as long as we have access to the file. But in the case where we are actually writing a spreadsheet update for a form submission, the logged in user may not have access to the file. However, it is still a legitimate write by the forms app, so it is safe to use the owner in the path, just as if we were logged out. I'm not 100% sure about the security implications here and request review. However it provides a simple workaround for our use case: - Form with an attached spreadsheet - User is logged in - User has access to the form but not the spreadsheet - The spreadsheet is in a group folder - The versioning app is enabled The alternative is to fix the underlying problem in the forms app, which would be a much bigger diff. See the forms bug here: nextcloud/forms#2067 However, if I am right about the security model here, this is a safe workaround, and may actually be correct. Signed-off-by: Matthew Toseland <matthew@toselandcs.co.uk>
toad
force-pushed
the
mjt-forms-versions-error-forms-bug-2067
branch
from
0d7674a
to
e92817d
Compare
There was a problem hiding this comment.
This does not look like something we should do, or not this way.
Do you have a stack trace for the permission error in forms app?
Why does the files write pass but not the versions write 🤔
The issue is specific to groupfolders where there is no owner so getOwner can return a user with no write permission I think. Maybe that’s the root problem to solve, not sure.
|
@icewind1991 @artonge I’m interested in your input on this. Part of the problem is that files_version needs a path to work with, and the event contains a Node instance, and the logic to extract a path from the node fails. It’s not clear to me if the fix for this lies in groupfolders or files_version, or a bit of both. |
|
In the error case I was debugging, it's actually in files_exists. If the user has read-only access to the file then it will work. If the user does not have access to the file at all, it fails. Which is not unreasonable - as they said earlier in the ticket, ideally Forms should use a worker thread with its own user. On the other hand I have to assume that running without files_versions doesn't create a massive security hole? Is files_versions actually enforcing security constraints here, or is it just crashing when something legal but confusing happens? Given that it allows you to write files when you're not even logged in, I have to assume the latter. Anyway, the stack trace is: |
|
The There might be a better way to implement EDIT: Also happy to have a better way to get the path. @icewind1991 might know. |
So that Storage can do what exactly? As I understand it, permissions checks apply at a higher level. Versions does not replace higher level permissions checks, it just hooks in after they've taken place, creates some metadata for the new version (which will be written to disk anyway), and presumably prevents the old one being deleted? So it's safe to deploy this as a temporary fix, even though it's probably not the right fix? |
|
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.) |
|
Hi @toad , from what I read it seems the the preferred approach here is to migrate from hooks to events for this part in forms. We'd rather not merge something temporary as that risks committing less attention to the real source of the issue, and we'd rather work on that. Nevertheless your PR brought our attention to this issue, so thank you very much for that! |
Is there a public ticket I can follow to monitor the status of the correct fix? Thanks! |
Fixes a forms bug by changing how files_versions gets the path for a Node to tolerate the forms app writing files that are not even visible to the logged-in user.
In FileEventsListener::getPathForNode(), if the user is not logged in, and we can't find the file owner, we guess the file owner from the path. Most likely the username in the path has some access to the file, so we can find the filename. This allows writing files when the user is not logged in, for instance where we are filling in a form from a public link.
However, in the case where there is a logged-in user, and the file is on a group share, both the username and the owner will be the same UID. Which is fine as long as we have access to the file.
But in the case where we are actually writing a spreadsheet update for a form submission, the logged in user may not have access to the file. However, it is still a legitimate write by the forms app, so it is safe to use the owner in the path, just as if we were logged out.
I'm not 100% sure about the security implications here and request review. However it provides a simple workaround for our use case:
The result is an error every time, see the forms bug nextcloud/forms#2067 (note that the actual error has changed since the initial report!)
The alternative is to fix the underlying problem in the forms app, which would be a much bigger diff, see the above.
However, if I am right about the security model here, this is a safe workaround, and may actually be correct.
Summary
Fixes a reproducible form submission error due to a conflict between how forms and versioning see permissions.
At this stage I'm asking for review that this is basically the right solution and there are no security implications. I believe it is safe but request review on that assumption!
An alternative fix in forms would be much more work, see their bug.
TODO
Checklist