-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Fix: NotFoundException for anonymous users #50958
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: NotFoundException for anonymous users #50958
Conversation
Signed-off-by: Kostiantyn Miakshyn <[email protected]>
|
/backport to stable31 |
|
/backport to stable30 |
|
/backport to stable29 |
|
|
||
| $owner = $node->getOwner()?->getUid(); | ||
| try { | ||
| $owner = $node->getOwner()?->getUid(); |
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.
Is this not a deeper problem @come-nc ?
So this method is not marked as throwing, but just to return null if not found.
Meaning should we fix the getOwner method or at least add the throwing to the docs?
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 how we manage non-existing files, see https://github.com/nextcloud/server/blob/master/lib/private/Files/Node/NonExistingFile.php
We could mark all methods as throwing but that would trigger a lot of false positive.
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.
Actually for getOwner the throw comes from Node::getFileInfo directly I think.
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.
But in this case the file exists
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.
No it does not.
Look at the trace in nextcloud/forms#2435
It’s triggered by rename hook, so either source or target does not exist, depending if we are in the pre- or post- rename hook/event.
Checklist