Add logging to /login failure
#4688
Conversation
We currently alert if we see too many errors, so we should write to the logs what is going on.
Deploying matrix-authentication-service-docs with
|
| Latest commit: |
35a8d6c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://babb9a92.matrix-authentication-service-docs.pages.dev |
| Branch Preview URL: | https://erikj-user-password-logging.matrix-authentication-service-docs.pages.dev |
erikjohnston
force-pushed
the
erikj/user_password_logging
branch
from
3b8d1f9 to
b6595ae
Compare
This gets a bit involved, but should help us separate "expected" errors (password mismatch) vs "unexpected" errors (wrong hash algorithm, etc).
erikjohnston
force-pushed
the
erikj/user_password_logging
branch
from
b6595ae to
485a577
Compare
|
Oh, also bonus third commit: differentiating between errors and password mismatches in the metrics, as I think we care differently about them. |
| pepper: Option<&[u8]>, | ||
| ) -> Result<(), anyhow::Error> { | ||
| match self { | ||
| ) -> Result<PasswordVerificationResult, anyhow::Error> { |
There was a problem hiding this comment.
This should be simplified with just a boolean
There was a problem hiding this comment.
The reason for using PasswordVerificationResult is so that we can use #[must_use], which a) helped a lot in the refactor to ensure we changed all call sites, and b) ensures future uses don't forget to actually check the result (this is a common issue with verify functions returning bools).
There was a problem hiding this comment.
We could potentially change it to be type PasswordVerificationResult<T> = Result<T, ()> too, but that just might be even more confusing to use.
| tracing::warn!("Invalid login form: {form_state:?}"); | ||
| PASSWORD_LOGIN_COUNTER.add(1, &[KeyValue::new(RESULT, "error")]); |
There was a problem hiding this comment.
I think it makes sense to add an additional label on the metric to attach the reason on each failure, which will be easier to look in graphs rather than logs
There was a problem hiding this comment.
I'm not sure. It'd only be useful if we can capture enough information in the metrics to be able to debug it, but we also can't add much cardinality to the metrics without causing issues for Prometheus.
TBH, so long as we actually have what is going wrong in the logs then its probably easy enough to just check in there (or in tracing, etc) 🤷
crates/data-model/src/users.rs
Outdated
| } | ||
| } | ||
|
|
||
| impl Display for User { |
There was a problem hiding this comment.
I don't like the idea of adding a display implementation for things in datamodel, unless the whole thing has an obvious string representation
There was a problem hiding this comment.
Mmm, have removed for now. Although I do suspect we'll run into this later where we want to ergonomically log the user, so not sure what the best plan there is
crates/handlers/src/compat/login.rs
Outdated
|
|
||
| #[error("password verification failed")] | ||
| PasswordVerificationFailed(#[source] anyhow::Error), | ||
| PasswordVerificationFailed, |
There was a problem hiding this comment.
To be consistent with the rest, split this into PasswordMismatch and PasswordVerificationFailed(#[source] anyhow::Error), and mark the second as an internal server error like we do for ProvisionDeviceFailed
There was a problem hiding this comment.
Have renamed. Errors raised during verification are currently going to InternalError, is it helpful to split that up into a separate variant? I wouldn't have thought so as they're unexpected internal errors at that point.
| password: Zeroizing<String>, | ||
| hashed_password: String, | ||
| ) -> Result<Option<(SchemeVersion, String)>, anyhow::Error> { | ||
| ) -> Result<PasswordVerificationResult<Option<(SchemeVersion, String)>>, anyhow::Error> { |
There was a problem hiding this comment.
Not the prettiest, but this could return Result<bool, Option<(SchemaVersion, String)>>. I don't think adding this new result type makes sense
erikjohnston
changed the title
/login failure
|
Sorry that I've missed the re-review request |
sandhose
added
A-Logging
2 participants
Two commits:
The second has ended up being a fairly large change, as I opted to add a new struct
PasswordVerificationResultfor the return type. I did look instead into making the returned error being structured, but that proved a bit of a PITA, and generally I think we want to handle the error vs password mismatch cases differently anyway.