-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Throw "401 Unauthenticated" when authentication is provided but invalid #26572
Throw "401 Unauthenticated" when authentication is provided but invalid #26572
Conversation
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.
Seems reasonable to me. Let's see what tests etc say :)
For master only - right?
Well the older the better so talk clients get a proper 401 instead of a 404 room not found when the token was revoked, but yeah master only for now |
E.g. with an AppToken that has been revoked Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
82c2e8f
to
4ed296d
Compare
Signed-off-by: Joas Schilling <coding@schilljs.com>
Hey @nickvergessen @LukasReschke @ChristophWurst , this is a critical change: due to this PR, my API tests for the notes app are failing! The notes app provides an API for third-party apps. This is implemented using the Before this PR, the API has returned HTTP status code 401. After this PR, the API returns HTTP status code 403. This applies to both scenarios: a) a request without any authentication credentials and b) a request with invalid authentication credentials. Hence, this PR is a breaking change which definitely should not be backported! Furthermore, I'm unsure if this change was intended. The PR title is about throwing HTTP status code 401. But in fact, HTTP status code 403 is thrown now. Please give an advise if this change will be kept up, since I would have to inform the developers of third-party apps then. This should be also added to the pinned issue regarding critical changes. |
Just for the record, discussing this currently in the community chat. Talk also had failing integration tests, because it was using a user which didn't exist: nextcloud/spreed#5542 |
As requested in the chat, here is my test: https://github.com/nextcloud/notes/blob/master/tests/api/CommonAPITest.php#L257-L261 Example run (failed): Here is the Thanks for having a look! |
Follow up in #26846 |
E.g. with an AppToken that has been revoked