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

Auth: 401 on invalid credentials #1746

Closed
wants to merge 7 commits into from
Closed

Conversation

Fylax
Copy link
Contributor

@Fylax Fylax commented Nov 21, 2021

With this PR changes some HTTP response codes from hardcoded values to net/http module constants, to achieve improved readability and more uniform APIs.

Moreover, on invalid credentials sent to session API, the HTTP response code changed from 400 (Bad Request) to 401 (Unauthorized).
The rationale of this change is described in RFC 7235, section 3.1, in particular on the sentence

If the request included authentication credentials, then the 401 response indicates that authorization has been refused for those credentials.

@CLAassistant
Copy link

CLAassistant commented Mar 1, 2022

CLA assistant check
All committers have signed the CLA.

@lastzero lastzero self-assigned this Mar 23, 2022
@lastzero lastzero added the enhancement Optimization, improvement or maintenance task label Mar 23, 2022
@lastzero
Copy link
Member

FYI: We can probably take care of this soon! 🥳

lastzero added a commit that referenced this pull request Sep 28, 2022
Signed-off-by: Michael Mayer <michael@photoprism.app>
@lastzero
Copy link
Member

We have almost completely rewritten the session management with the above commit. The response codes should now reflect the actual status, e.g. whether you just don't have authorization or whether the session has expired and you need to re-authenticate.

@lastzero lastzero added the please-test Ready for acceptance test label Sep 28, 2022
@lastzero lastzero changed the title 401 on invalid credentials Auth: 401 on invalid credentials Sep 28, 2022
@lastzero
Copy link
Member

I am closing this as the original PR has conflicts and is probably no longer needed, meaning it would not be merged. Thank you for bringing this up!

@lastzero lastzero closed this Sep 28, 2022
@el-tiuri
Copy link

Hey, this is still on the roadmap in the “development” category. I’m guessing this should be removed from there?

@lastzero
Copy link
Member

We move it to "preview" as soon as a new build with the ":preview" tag has been released and documented. Due to the many changes and the fact that we are continuously developing new code, this may take a little while.

@lastzero lastzero added released Available in the stable release and removed please-test Ready for acceptance test labels Nov 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Optimization, improvement or maintenance task released Available in the stable release
Projects
Status: Release 🌈
Development

Successfully merging this pull request may close these issues.

None yet

4 participants