More specific error responses when authorization fails #46
Conversation
| @@ -4,6 +4,7 @@ use serde::{Deserialize, Serialize}; | |||
| use std::{collections::HashSet, time::SystemTime}; | |||
|
|
|||
| pub use jsonwebtoken::errors::Error as JwtError; | |||
| pub use jsonwebtoken::errors::ErrorKind as JwtErrorKind; | |||
There was a problem hiding this comment.
Sidenote: when I worked on the Rust compiler, I noticed that these sort of re-exports tend to make it harder to split crates and can be detrimental for refactoring. I would in the future recommend more direct imports instead.
There was a problem hiding this comment.
This might be true for a compiler, but I don’t agree it is in general. Re-exporting can help ensure consistent dependency resolution, for example.
In this particular case, it is a reasonable API choice (consumers should work against the core crate, not a particular implementation of JWT), and it is a reasonable choice to enable improvement: the chosen library is of somewhat questionable quality, and it is arguably easier to replace it in only one place.
Admittedly I blinked when I arrived here, but decided to leave it as is for these reasons.
| pub struct AuthorizationRejection { | ||
| reason: AuthorizationRejectionReason, | ||
| } |
There was a problem hiding this comment.
| pub struct AuthorizationRejection { | |
| reason: AuthorizationRejectionReason, | |
| } | |
| /// A response by the API signifying that an authorization was rejected with the `reason` for this. | |
| pub struct AuthorizationRejection { | |
| /// The reason the authorization was rejected. | |
| reason: AuthorizationRejectionReason, | |
| } |
There was a problem hiding this comment.
These docstrings do not add any information over just the types. So what is the value of doing this, even for non public items?
There was a problem hiding this comment.
They do add a bit of context on the type which can be useful when hovering over the type. Also, I think this is a good habit to get into to document too much rather than too little.
There was a problem hiding this comment.
Then we will have to agree to disagree
| impl IntoResponse for AuthorizationRejection { | ||
| fn into_response(self) -> axum::response::Response { | ||
| (StatusCode::BAD_REQUEST, "Authorization is invalid - malformed token.").into_response() | ||
| // Most likely, the server key was rotated |
There was a problem hiding this comment.
| // Most likely, the server key was rotated | |
| // Most likely, the server key was rotated. |
Nit
There was a problem hiding this comment.
How to you feel about authoring a style guide?
| ); | ||
| // JWT is hard bruh | ||
| const INVALID: (StatusCode, &str) = (StatusCode::BAD_REQUEST, "Authorization is invalid: malformed token"); | ||
| // Sensible fallback if no auth header is present |
There was a problem hiding this comment.
| // Sensible fallback if no auth header is present | |
| // Sensible fallback if no auth header is present. |
nit
| AuthorizationRejectionReason::Jwt(JwtErrorKind::InvalidSignature) => ROTATED.into_response(), | ||
| AuthorizationRejectionReason::Header(rejection) => match rejection.reason() { | ||
| TypedHeaderRejectionReason::Missing => REQUIRED.into_response(), | ||
| _ => rejection.into_response(), |
There was a problem hiding this comment.
| _ => rejection.into_response(), | |
| TypedHeaderRejectionReason::Error(_) => rejection.into_response(), |
There was a problem hiding this comment.
In general a good suggestion, here I don’t think. I should’ve used matches!.
| StatusCode::UNAUTHORIZED, | ||
| "Authorization failed: token not signed by this instance", | ||
| ); | ||
| // JWT is hard bruh |
There was a problem hiding this comment.
Would be helpful to expand this comment for someone who has no context on JWT.
| TypedHeaderRejectionReason::Missing => Ok(Self { auth: None }), | ||
| _ => Err(AuthorizationRejection), | ||
| _ => Err(AuthorizationRejection { |
There was a problem hiding this comment.
| _ => Err(AuthorizationRejection { | |
| TypedHeaderRejectionReason::Error(_) => Err(AuthorizationRejection { |
| let auth = SpacetimeAuth { | ||
| creds, | ||
| identity: claims.hex_identity, | ||
| }; | ||
| Ok(Self { auth: Some(auth) }) | ||
| } | ||
| Err(e) => match e.reason() { | ||
| // Leave it to handlers to decide on unauthorized requests |
There was a problem hiding this comment.
| // Leave it to handlers to decide on unauthorized requests | |
| // Leave it to handlers to decide on unauthorized requests. |
Description of Changes
API
If the API is breaking, please state below what will break