-
-
Notifications
You must be signed in to change notification settings - Fork 284
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.
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
feat: flatten (de)serialization of custom user claims #1159
base: master
Are you sure you want to change the base?
Changes from all commits
b3bc611
a4bddb6
a96dd62
97aa482
3767c4d
a825698
5a3283c
6a9ab79
2d52d02
36565ed
4f3ccce
89ac951
b3968aa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -2,23 +2,28 @@ | |||||
//! | ||||||
//! This module provides functionality for working with JSON Web Tokens (JWTs) | ||||||
//! and password hashing. | ||||||
|
||||||
use jsonwebtoken::{ | ||||||
decode, encode, errors::Result as JWTResult, get_current_timestamp, Algorithm, DecodingKey, | ||||||
EncodingKey, Header, TokenData, Validation, | ||||||
}; | ||||||
use serde::{Deserialize, Serialize}; | ||||||
use serde_json::Value; | ||||||
use serde_json::{Map, Value}; | ||||||
|
||||||
/// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like |
||||||
pub struct UserClaims { | ||||||
pub pid: String, | ||||||
exp: u64, | ||||||
pub claims: Option<Value>, | ||||||
#[serde(default, flatten)] | ||||||
// 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 | ||||||
Comment on lines
+21
to
+24
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you please remove the TODOs? |
||||||
// use generics to serialize/deserialize it? | ||||||
pub claims: Map<String, Value>, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The advantage of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. serde_json::Map for sure! Didn't know that existed. |
||||||
} | ||||||
|
||||||
/// Represents the JWT configuration and operations. | ||||||
|
@@ -61,17 +66,18 @@ impl JWT { | |||||
/// | ||||||
/// # Example | ||||||
/// ```rust | ||||||
/// use serde_json::Map; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should I hide this import in the doctest or not?
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wouldn't hide it. I think it makes clear to the user what to import to use this feature. |
||||||
/// use loco_rs::auth; | ||||||
/// | ||||||
/// auth::jwt::JWT::new("PqRwLF2rhHe8J22oBeHy").generate_token(&604800, "PID".to_string(), None); | ||||||
/// auth::jwt::JWT::new("PqRwLF2rhHe8J22oBeHy").generate_token(604800, "PID".to_string(), Map::new()); | ||||||
/// ``` | ||||||
pub fn generate_token( | ||||||
&self, | ||||||
expiration: &u64, | ||||||
expiration: u64, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only thing I can say is that it won't be a biggie for me :) |
||||||
pid: String, | ||||||
claims: Option<Value>, | ||||||
claims: Map<String, Value>, | ||||||
) -> JWTResult<String> { | ||||||
let exp = get_current_timestamp().saturating_add(*expiration); | ||||||
let exp = get_current_timestamp().saturating_add(expiration); | ||||||
|
||||||
let claims = UserClaims { pid, exp, claims }; | ||||||
|
||||||
|
@@ -119,18 +125,27 @@ mod tests { | |||||
use super::*; | ||||||
|
||||||
#[rstest] | ||||||
#[case("valid token", 60, None)] | ||||||
#[case("token expired", 1, None)] | ||||||
#[case("valid token and custom claims", 60, Some(json!({})))] | ||||||
#[tokio::test] | ||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. are those cases enough? too much? any specific case that you miss? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep, any JSON value should be valid as the key's value :) |
||||||
#[case("valid token and custom boolean claims",60, json!({ "custom": true,}))] | ||||||
#[case("valid token and custom number claims",60, json!({ "custom": 123,}))] | ||||||
#[case("valid token and custom nested claims",60, json!({ "level1": { "level2": { "level3": "claim" } } }))] | ||||||
#[case("valid token and custom array claims",60, json!({ "array": [1, 2, 3] }))] | ||||||
#[case("valid token and custom nested array claims",60, json!({ "level1": { "level2": { "level3": [1, 2, 3] } } }))] | ||||||
fn can_generate_token( | ||||||
#[case] test_name: &str, | ||||||
#[case] expiration: u64, | ||||||
#[case] claims: Option<Value>, | ||||||
#[case] json_claims: Value, | ||||||
) { | ||||||
let claims = json_claims | ||||||
.as_object() | ||||||
.expect("case input claims must be an object") | ||||||
.clone(); | ||||||
let jwt = JWT::new("PqRwLF2rhHe8J22oBeHy"); | ||||||
|
||||||
let token = jwt | ||||||
.generate_token(&expiration, "pid".to_string(), claims) | ||||||
.generate_token(expiration, "pid".to_string(), claims) | ||||||
.unwrap(); | ||||||
|
||||||
std::thread::sleep(std::time::Duration::from_secs(3)); | ||||||
|
@@ -140,4 +155,76 @@ mod tests { | |||||
assert_debug_snapshot!(test_name, jwt.validate(&token)); | ||||||
}); | ||||||
} | ||||||
|
||||||
#[rstest] | ||||||
#[case::without_custom_claims(json!({}))] | ||||||
#[case::with_custom_string_claims(json!({ "custom": "claim",}))] | ||||||
#[case::with_custom_boolean_claims(json!({ "custom": true,}))] | ||||||
#[case::with_custom_number_claims(json!({ "custom": 123,}))] | ||||||
#[case::with_custom_nested_claims(json!({ "level1": { "level2": { "level3": "claim" } } }))] | ||||||
#[case::with_custom_array_claims(json!({ "array": [1, 2, 3] }))] | ||||||
#[case::with_custom_nested_array_claims(json!({ "level1": { "level2": { "level3": [1, 2, 3] } } }))] | ||||||
// we use `Value` to reduce code duplicity in the case inputs | ||||||
fn serialize_user_claims(#[case] json_claims: Value) { | ||||||
let claims = json_claims | ||||||
.as_object() | ||||||
.expect("case input claims must be an object") | ||||||
.clone(); | ||||||
let input_user_claims = UserClaims { | ||||||
pid: "pid".to_string(), | ||||||
exp: 60, | ||||||
claims: claims.clone(), | ||||||
}; | ||||||
|
||||||
let mut expected_claim = Map::new(); | ||||||
expected_claim.insert("pid".to_string(), "pid".into()); | ||||||
expected_claim.insert("exp".to_string(), 60.into()); | ||||||
// we add the claims in a flattened way | ||||||
expected_claim.extend(claims); | ||||||
let expected_value = Value::from(expected_claim); | ||||||
|
||||||
// We check between `Value` instead of `String` to avoid key ordering issues when serializing. | ||||||
// It is because `expected_value` has all the keys in alphabetical order, as the `Value` serialization ensures that. | ||||||
// But when serializing `input_user_claims`, first the `pid` and `exp` fields are serialized (in that order), | ||||||
// and then the claims are serialized in alfabetic order. So, the resulting JSON string from the `input_user_claims` serialization | ||||||
// may have the `pid` and `exp` fields unordered which differs from the `Value` serialization. | ||||||
assert_eq!( | ||||||
expected_value, | ||||||
serde_json::to_value(&input_user_claims).unwrap() | ||||||
); | ||||||
} | ||||||
|
||||||
#[rstest] | ||||||
#[case::without_custom_claims(json!({}))] | ||||||
#[case::with_custom_string_claims(json!({ "custom": "claim",}))] | ||||||
#[case::with_custom_boolean_claims(json!({ "custom": true,}))] | ||||||
#[case::with_custom_number_claims(json!({ "custom": 123,}))] | ||||||
#[case::with_custom_nested_claims(json!({ "level1": { "level2": { "level3": "claim" } } }))] | ||||||
#[case::with_custom_array_claims(json!({ "array": [1, 2, 3] }))] | ||||||
#[case::with_custom_nested_array_claims(json!({ "level1": { "level2": { "level3": [1, 2, 3] } } }))] | ||||||
// we use `Value` to reduce code duplicity in the case inputs | ||||||
fn deserialize_user_claims(#[case] json_claims: Value) { | ||||||
let claims = json_claims | ||||||
.as_object() | ||||||
.expect("case input claims must be an object") | ||||||
.clone(); | ||||||
|
||||||
let mut input_claims = Map::new(); | ||||||
input_claims.insert("pid".to_string(), "pid".into()); | ||||||
input_claims.insert("exp".to_string(), 60.into()); | ||||||
// we add the claims in a flattened way | ||||||
input_claims.extend(claims.clone()); | ||||||
let input_json = Value::from(input_claims).to_string(); | ||||||
|
||||||
let expected_user_claims = UserClaims { | ||||||
pid: "pid".to_string(), | ||||||
exp: 60, | ||||||
claims, | ||||||
}; | ||||||
|
||||||
assert_eq!( | ||||||
expected_user_claims, | ||||||
serde_json::from_str(&input_json).unwrap() | ||||||
); | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
--- | ||
source: src/auth/jwt.rs | ||
expression: jwt.validate(&token) | ||
--- | ||
Ok( | ||
TokenData { | ||
header: Header { | ||
typ: Some( | ||
"JWT", | ||
), | ||
alg: HS512, | ||
cty: None, | ||
jku: None, | ||
jwk: None, | ||
kid: None, | ||
x5u: None, | ||
x5c: None, | ||
x5t: None, | ||
x5t_s256: None, | ||
}, | ||
claims: UserClaims { | ||
pid: "pid", | ||
exp: EXP, | ||
claims: { | ||
"array": Array [ | ||
Number(1), | ||
Number(2), | ||
Number(3), | ||
], | ||
}, | ||
}, | ||
}, | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,5 @@ | ||
--- | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this snapshot file's diff is marked as "renamed file", but actually I deleted the old |
||
source: src/auth/jwt.rs | ||
assertion_line: 133 | ||
expression: jwt.validate(&token) | ||
--- | ||
Ok( | ||
|
@@ -22,9 +21,9 @@ Ok( | |
claims: UserClaims { | ||
pid: "pid", | ||
exp: EXP, | ||
claims: Some( | ||
Object {}, | ||
), | ||
claims: { | ||
"custom": Bool(true), | ||
}, | ||
}, | ||
}, | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
--- | ||
source: src/auth/jwt.rs | ||
expression: jwt.validate(&token) | ||
--- | ||
Ok( | ||
TokenData { | ||
header: Header { | ||
typ: Some( | ||
"JWT", | ||
), | ||
alg: HS512, | ||
cty: None, | ||
jku: None, | ||
jwk: None, | ||
kid: None, | ||
x5u: None, | ||
x5c: None, | ||
x5t: None, | ||
x5t_s256: None, | ||
}, | ||
claims: UserClaims { | ||
pid: "pid", | ||
exp: EXP, | ||
claims: { | ||
"level1": Object { | ||
"level2": Object { | ||
"level3": Array [ | ||
Number(1), | ||
Number(2), | ||
Number(3), | ||
], | ||
}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added Eq and PartialEq so we can use
assert_eq!
with this struct