Skip to content
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

Change (de)serialization of UserClaims (specifically claims field) #1115

Open
Django-Fakkeldij opened this issue Dec 29, 2024 · 3 comments · May be fixed by #1159
Open

Change (de)serialization of UserClaims (specifically claims field) #1115

Django-Fakkeldij opened this issue Dec 29, 2024 · 3 comments · May be fixed by #1159

Comments

@Django-Fakkeldij
Copy link

Description

I am building an RBAC system using the built-in JWT support. When using the claims parameter in src/auth/jwt.rs, I encountered unexpected behavior.

Here's the function signature in question:

loco_rs::auth::jwt::JWT::new().generate_token(&self, secret: &str, expiration: &u64, claims: UserJwtClaims)

According to the JWT introduction, claims should correspond to fields within the JWT payload. However, when passing a value to the claims parameter, it serializes as an embedded object under the claims key in the JWT payload instead of merging with the predefined claims. This behavior differs from the expected result of integrating the custom claims directly into the payload.

Here’s the definition of the claims structure from src/auth/jwt.rs:

/// Represents the claims associated with a user JWT.
#[derive(Debug, Serialize, Deserialize)]
pub struct UserClaims {
   pub pid: String,
   exp: u64,
   pub claims: Option<Value>,
}

To Reproduce

#[derive(Debug, Default, Deserialize, Serialize)]
pub struct UserJwtClaims {
    pub roles: Vec<String>,
}

fn main() -> Result<(), Box<dyn Error>> {
    let custom_claims = Some(serde_json::to_value(UserJwtClaims {
        roles: vec!["admin".to_owned(), "non-admin".to_owned()];
    }).unwrap());
    let res = auth::jwt::JWT::new("PqRwLF2rhHe8J22oBeHy").generate_token(&604800, "PID".to_string(), custom_claims)?;
}

Expected Behavior

I expected the following JWT payload:

{
  "pid": "PID",
  "exp": 1736099338,
  "roles": ["admin", "non-admin"]
}

Instead, the actual JWT payload looks like this:

{
  "pid": "PID",
  "exp": 1736099338,
  "claims": {
    "roles": ["admin", "non-admin"]
  }
}

Environment:

Additional Context

I believe this can be resolved with a small adjustment to use the #[serde(flatten)] attribute in the UserClaims struct:

/// Represents the claims associated with a user JWT.
#[derive(Debug, Serialize, Deserialize)]
pub struct UserClaims {
   pub pid: String,
   exp: u64,
   #[serde(flatten)]
   pub claims: Option<Value>,
}

This change would allow the claims to merge seamlessly with the predefined claims, achieving the expected behavior.

@jorgehermo9
Copy link
Contributor

jorgehermo9 commented Jan 5, 2025

I think we should use something like

/// Represents the claims associated with a user JWT.
#[derive(Debug, Serialize, Deserialize)]
pub struct UserClaims {
   pub pid: String,
   exp: u64,
   #[serde(default, flatten)]
   pub claims: HashMap<String,Value>,
}

instead. with your proposal, if the Value is not a map (for example an integer) I wonder how will it behave with flatten

@Django-Fakkeldij
Copy link
Author

I think you're right. If you pass in an integer (or any non-struct) with my proposal, you get a runtime error.

@jorgehermo9
Copy link
Contributor

jorgehermo9 commented Jan 6, 2025

I would like to address this issue. Note that this will involve a breaking change, because of replacement of Option<Value> to serde_json::Map<String,Value>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants