Skip to content

Make sure that failed login attempts are returned as 401 Unauthorized #160

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jun 20, 2025
Merged

Make sure that failed login attempts are returned as 401 Unauthorized #160

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
248e05c
Make sure that failed login attempts are returned as HTTP statuscode …
jhodapp Jun 18, 2025
432ccbc
Run cargo fmt
jhodapp Jun 18, 2025
0a648e7
Merge branch 'main' into 122-fe-bug-unsuccessful-login-returns-500
jhodapp Jun 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions domain/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ pub enum InternalErrorKind {
pub enum EntityErrorKind {
NotFound,
Invalid,
Unauthenticated,
DbTransaction,
Other(String),
}
Expand Down Expand Up @@ -71,6 +72,7 @@ impl From<EntityApiError> for Error {
let entity_error_kind = match err.error_kind {
EntityApiErrorKind::RecordNotFound => EntityErrorKind::NotFound,
EntityApiErrorKind::InvalidQueryTerm => EntityErrorKind::Invalid,
EntityApiErrorKind::RecordUnauthenticated => EntityErrorKind::Unauthenticated,
_ => EntityErrorKind::Other("EntityErrorKind".to_string()),
};

Expand Down
38 changes: 33 additions & 5 deletions web/src/controller/user_session_controller.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use crate::controller::ApiResponse;
use crate::error::{Error as WebError, Result as WebResult};
use axum::{http::StatusCode, response::IntoResponse, Form, Json};
use domain::user::{AuthSession, Credentials};
use log::*;
Expand Down Expand Up @@ -36,17 +37,44 @@ pub struct NextUrl {
pub async fn login(
mut auth_session: AuthSession,
Form(creds): Form<Credentials>,
) -> impl IntoResponse {
) -> WebResult<impl IntoResponse> {
debug!("UserSessionController::login()");

let user = match auth_session.authenticate(creds.clone()).await {
Ok(Some(user)) => user,
Ok(None) => return Err(StatusCode::UNAUTHORIZED.into_response()),
Err(_) => return Err(StatusCode::INTERNAL_SERVER_ERROR.into_response()),
Ok(None) => {
// No user found - this should also be treated as an authentication error
return Err(WebError::from(domain::error::Error {
source: None,
error_kind: domain::error::DomainErrorKind::Internal(
domain::error::InternalErrorKind::Entity(
domain::error::EntityErrorKind::Unauthenticated,
),
),
}));
}
Err(auth_error) => {
// axum_login errors contain our entity_api::Error in the error field
warn!("Authentication failed: {:?}", auth_error);
return Err(WebError::from(domain::error::Error {
source: Some(Box::new(auth_error)),
error_kind: domain::error::DomainErrorKind::Internal(
domain::error::InternalErrorKind::Entity(
domain::error::EntityErrorKind::Unauthenticated,
),
),
}));
}
Comment on lines +45 to +67
Copy link
Collaborator

@calebbourg calebbourg Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great change! I think it makes sense to return a 401 in all non-success cases here for security purposes.

Here is a small suggestion and my thought process

Suggested change
Ok(None) => {
// No user found - this should also be treated as an authentication error
return Err(WebError::from(domain::error::Error {
source: None,
error_kind: domain::error::DomainErrorKind::Internal(
domain::error::InternalErrorKind::Entity(
domain::error::EntityErrorKind::Unauthenticated,
),
),
}));
}
Err(auth_error) => {
// axum_login errors contain our entity_api::Error in the error field
warn!("Authentication failed: {:?}", auth_error);
return Err(WebError::from(domain::error::Error {
source: Some(Box::new(auth_error)),
error_kind: domain::error::DomainErrorKind::Internal(
domain::error::InternalErrorKind::Entity(
domain::error::EntityErrorKind::Unauthenticated,
),
),
}));
}
Ok(None) => { return Err(StatusCode::UNAUTHORIZED.into_response()) }
Err(auth_error) => {
// axum_login errors contain our entity_api::Error in the error field
warn!("Authentication failed: {:?}", auth_error);
Err(auth_error)
}

To me this feels more correct semantically. The absence of a record, in my mind, is not really an error even though we want to translate it into a 401. The translation is more of an obfuscation similar to an actual error. This is as opposed to the alternatives:

  • The record does exist but is not able to be authenticated for whatever reason. More of a pure 401
  • There was an actual error somewhere along the call stack. Translated, obfuscated 401

The main reason this stands out to me is that we're reaching into the other abstraction layers and explicitly returning an error that is defined in entity_api. Even though it's re-exported through domain it feels slightly like we're leaking concepts between the boundaries. I think that returning the Err(StatusCode::UNAUTHORIZED.into_response() feels more appropriate for the something the web layer's responsibility. The entity_api and lower level constructs should, if possible, be returned from those layers and only translated in web.

For Err(auth_error) I think we could just return that as it already includes the information and would be just a pass through. I may be totally wrong there.

This is somewhat nit picky but I think it's important to try to maintain the boundaries if possible. Certainly open to your take and philosophy on the matter. In the end it's not really a blocker

Copy link
Member Author

@jhodapp jhodapp Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@calebbourg Great suggestion Caleb. So for the Err case, it would either map to something that exists, or turn into a 500 internal server error if an error was raised that didn't map to something we anticipate.

The only risk here is that we are now exposing a security difference to the world on our front page. This conversation is public, this source code is public, so someone just needs to figure out how to trigger this situation and now they know a distinct difference. I've read a lot of informed thoughts about login forms and consistency if the highest value. They even recommend that we always respond with the same constant time whether there's a successful login vs an unsuccessful one because of this "having distinct knowledge" risk. Thoughts?

Copy link
Collaborator

@calebbourg calebbourg Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you and thank you for your thoughts as well!

I completely agree with all of the security implications.
I envision it either returning a 401 in the case where the user queried doesn't exist (theOk(None) case and then ensuring that auth_session.authenticate(creds.clone()) returns an error from the lower layers that web translates to a 401. Which would mean in all non-authenticated scenarios (error, user doesn't exist, etc.) we will always return a 401.

Let me know if that makes sense and if you think it's feasible.

It may already be accomplished here:

refactor-platform-rs/entity_api/src/user.rs

Lines 127 to 135 in 46355c6

async fn authenticate_user(creds: Credentials, user: Model) -> Result<Option<Model>, Error> {
match password_auth::verify_password(creds.password, &user.password) {
Ok(_) => Ok(Some(user)),
Err(_) => Err(Error {
source: None,
error_kind: EntityApiErrorKind::RecordUnauthenticated,
}),
}
}

Copy link
Member Author

@jhodapp jhodapp Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll look into this a little more deeply this evening. Thanks for your thoughts.

Copy link
Member Author

@jhodapp jhodapp Jun 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@calebbourg It looks like I must provide an Ok(None) => {} branch to satisfy the let user = match auth_session.authenticate(creds.clone()).await {}…it's expected and if it's not there rustc complains.

Hopefully I'm not completely misunderstanding what you're intending here.

Copy link
Collaborator

@calebbourg calebbourg Jun 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jhodapp no worries! I may not be describing my suggestion very well.

My hypothesis is that the goal I outlined can be accomplished by something like this (adding comments):

Suggested change
Ok(None) => {
// No user found - this should also be treated as an authentication error
return Err(WebError::from(domain::error::Error {
source: None,
error_kind: domain::error::DomainErrorKind::Internal(
domain::error::InternalErrorKind::Entity(
domain::error::EntityErrorKind::Unauthenticated,
),
),
}));
}
Err(auth_error) => {
// axum_login errors contain our entity_api::Error in the error field
warn!("Authentication failed: {:?}", auth_error);
return Err(WebError::from(domain::error::Error {
source: Some(Box::new(auth_error)),
error_kind: domain::error::DomainErrorKind::Internal(
domain::error::InternalErrorKind::Entity(
domain::error::EntityErrorKind::Unauthenticated,
),
),
}));
}
Ok(None) => {
// here we use StatusCode explicitly because it belongs within the web abstraction layer
// whereas EntityApiErrorKind and it's cousins have less of an explicit place here.
return Err(StatusCode::UNAUTHORIZED.into_response())
}
Err(auth_error) => {
// Here we simply pass through the error that is emitted from the lower layer (entity_api) which I think
// is EntityApiErrorKind::RecordUnauthenticated based on what authenticate_user() emits.
// EntityApiErrorKind::RecordUnauthenticated gets automatically translated to a 401 by the web layer and so we can just make it a pass through here and avoid any explicit naming of the actual underlying EntityApiErrorKind::RecordUnauthenticated in this part of the code thus avoiding the explicit direct dependency on entity_api.
// We only indirectly depend on it via domain which is ok and part of our design
warn!("Authentication failed: {:?}", auth_error);
return Err(auth_error);
}

Let me know if that helps clarify my thought process. I haven't tested the error translation yet and so it may be that either authenticate_user() needs to be updated to actually return an error that the web layer will automatically translate to a 401 or, if that isn't possible, leave the code like this for now.

};

if auth_session.login(&user).await.is_err() {
return Err(StatusCode::INTERNAL_SERVER_ERROR.into_response());
if let Err(login_error) = auth_session.login(&user).await {
warn!("Session login failed: {:?}", login_error);
return Err(WebError::from(domain::error::Error {
source: Some(Box::new(login_error)),
error_kind: domain::error::DomainErrorKind::Internal(
domain::error::InternalErrorKind::Other("Session login failed".to_string()),
),
}));
}

let user_session_json = json!({
Expand Down
7 changes: 7 additions & 0 deletions web/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,13 @@ impl Error {
);
(StatusCode::NOT_FOUND, "NOT FOUND").into_response()
}
EntityErrorKind::Unauthenticated => {
warn!(
"EntityErrorKind::Unauthenticated: Responding with 401 Unauthorized. Error: {:?}",
self
);
(StatusCode::UNAUTHORIZED, "UNAUTHORIZED").into_response()
}
EntityErrorKind::DbTransaction => {
warn!(
"EntityErrorKind::DbTransaction: Responding with 500 Internal Server Error. Error: {:?}",
Expand Down
Loading