[Feature Request] Allow to disable default password log in after SSO is configured #406 #502
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…is configured hoarder-app#406 changed the flag to also disallow logging in via password The extensions will also no longer be allowed to log in via username/password then
|
Nice! Was looking for this. |
MohamedBassem
requested changes
Oct 7, 2024
| if (serverConfig.auth.disablePasswordAuth) { | ||
| throw new TRPCError({ | ||
| message: "Password authentication is currently disabled", | ||
| code: "FORBIDDEN", |
There was a problem hiding this comment.
I think UNAUTHORIZED is more suitable here.
https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/401
A 401 Unauthorized is similar to the 403 Forbidden response, except that a 403 is returned when a request contains valid credentials, but the client does not have permissions to perform a certain action.
In that case, we don't have valid creds. So I think a 401 is more suitable. What do you think?
There was a problem hiding this comment.
- I put the reason for this in the comment above. If we return 401, the extension will show "Wrong username or password", which is not correct, since they might be correct, they might be wrong. We did not check. Additionally it will make problem solving harder, since it is very misleading to tell the users that their credentials are wrong, when they are possibly correct, but the admin decided to turn off the login via credentials. I have looked at changing the way we show the message in the extension, but that would mean just taking whatever the server sends as a message, which is currently very revealing (e.g. user exists, but password is wrong), so that is not good from a security perspective.
- I find "forbidden" much better as it is actively forbidden to sign in this way. It does not fulfill the first part of the spec though (valid credentials, as we never check), but the second part is fulfilled. But I also do not find 401 correct, since you might have valid credentials, but we did not check.
…is configured hoarder-app#406 added the error message for OAuth
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
changed the flag to also disallow logging in via password
The extensions will also no longer be allowed to log in via username/password then