Make sure that failed login attempts are returned as 401 Unauthorized

[jhodapp]

Description

This PR ensures that failed login attempts are returned as HTTP statuscode UNAUTHORIZED (401), not a 500 internal server error.

GitHub Issue: Relates to the frontend bug #122

Changes

• Makes the EntityErrorKind::Unauthenticated actually pass through the domain layer

Testing Strategy

1. Try logging in from the frontend with invalid credentials (try both a valid email/invalid password, and an invalid email/password set combination)
2. Observe in the backend logging that you see [WARN] EntityErrorKind::Unauthenticated: Responding with 401 Unauthorized. Error: Domain(Error { source: Some(Error { source: None, error_kind: RecordUnauthenticated }), error_kind: Internal(Entity(Unauthenticated)) })
3. If you're using the paired frontend branch, you should see it display the error Invalid email or password. Please try again.

Concerns

None

[calebbourg]

Looks great! Just the potential for a change depending on what you think.

[calebbourg]

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

[jhodapp]

@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?

[calebbourg]

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,
	}),
	}
	}

[jhodapp]

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

[jhodapp]

@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.

[calebbourg]

@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.