feat: flatten (de)serialization of custom user claims #1159
Conversation
| /// ``` | ||
| pub fn generate_token( | ||
| &self, | ||
| expiration: &u64, | ||
| expiration: u64, |
There was a problem hiding this comment.
u64 implements Copy (https://doc.rust-lang.org/std/marker/trait.Copy.html) so there is no need to use references here, we can simply copy it.
This may be a bit out of scope, I can address this in another PR if required, but seemed to me too little change and not worth of a separate PR
There was a problem hiding this comment.
Only thing I can say is that it won't be a biggie for me :)
| // TODO: serde_json::Map or std::collections::HashMap? | ||
| // TODO: is it ok to use a generic Map<String, Value> here? Or should we let the user specify their desired typed claim and | ||
| // use generics to serialize/deserialize it? | ||
| pub claims: Map<String, Value>, |
There was a problem hiding this comment.
serde_json::Map? HashMap? BtreeMap?
The advantage of serde_json::Map is that users can use the serde_json's preserve_order flag to control how the claims are serialized https://docs.rs/serde_json/latest/serde_json/enum.Value.html#variant.Object
There was a problem hiding this comment.
serde_json::Map for sure! Didn't know that existed.
| @@ -61,17 +66,18 @@ impl JWT { | |||
| /// | |||
| /// # Example | |||
| /// ```rust | |||
| /// use serde_json::Map; | |||
There was a problem hiding this comment.
Should I hide this import in the doctest or not?
| /// use serde_json::Map; | |
| /// # use serde_json::Map; |
There was a problem hiding this comment.
I wouldn't hide it. I think it makes clear to the user what to import to use this feature.
| async fn can_generate_token( | ||
| #[case("valid token", 60, json!({}))] | ||
| #[case("token expired", 1, json!({}))] | ||
| #[case("valid token and custom string claims", 60, json!({ "custom": "claim",}))] |
There was a problem hiding this comment.
are those cases enough? too much? any specific case that you miss?
There was a problem hiding this comment.
Think this is more than enough. You could have a second look at the link below but it doesn't dictate much about the specific form of claims in a JWT.
There was a problem hiding this comment.
yep, any JSON value should be valid as the key's value :)
| @@ -1,6 +1,5 @@ | |||
| --- | |||
There was a problem hiding this comment.
this snapshot file's diff is marked as "renamed file", but actually I deleted the old valid token and custom claims snapshot, as it was unreferenced, and added a new valid token and custom boolean claims snapshot, such as the other from this PR.
|
|
||
| /// Represents the default JWT algorithm used by the [`JWT`] struct. | ||
| const JWT_ALGORITHM: Algorithm = Algorithm::HS512; | ||
|
|
||
| /// Represents the claims associated with a user JWT. | ||
| #[derive(Debug, Serialize, Deserialize)] | ||
| #[derive(Debug, Serialize, Deserialize, Eq, PartialEq)] |
There was a problem hiding this comment.
added Eq and PartialEq so we can use assert_eq! with this struct
|
Think this looks great! Thank you for taking the time to fix this issue ❤️ |
|
Fixed failed CI |
Co-authored-by: Jorge Hermo <[email protected]>
| // TODO: should we wrap this in an Option? `Option<Map<String, Value>>` | ||
| // so we can use `auth::jwt::JWT::new("PqRwLF2rhHe8J22oBeHy").generate_token(&604800, "PID".to_string(), None); | ||
| // TODO: serde_json::Map or std::collections::HashMap? | ||
| // TODO: is it ok to use a generic Map<String, Value> here? Or should we let the user specify their desired typed claim and |
There was a problem hiding this comment.
Can you please remove the TODOs?
|
|
||
| /// Represents the default JWT algorithm used by the [`JWT`] struct. | ||
| const JWT_ALGORITHM: Algorithm = Algorithm::HS512; | ||
|
|
||
| /// Represents the claims associated with a user JWT. | ||
| #[derive(Debug, Serialize, Deserialize)] | ||
| #[derive(Debug, Serialize, Deserialize, Eq, PartialEq)] |
There was a problem hiding this comment.
It looks like Eq and PartialEq are only needed for testing purposes. To address this, you can use #[cfg_attr(test, derive(Eq, PartialEq))] to conditionally derive them only in test builds.
| @@ -2,6 +2,7 @@ use async_trait::async_trait; | |||
| use chrono::offset::Local; | |||
| use loco_rs::{auth::jwt, hash, prelude::*}; | |||
| use serde::{Deserialize, Serialize}; | |||
| use serde_json::Map; | |||
There was a problem hiding this comment.
Starters folder are deprecated and move the loco-new.
please revert all the changes under this path
Closes #1115
Left some doubts as
// TODO:in the code, please take a look into that comments when reviewing.CC @Django-Fakkeldij