Skip to content

Commit

Permalink
fix: Bug claim aud is not a string (#9)
Browse files Browse the repository at this point in the history
* fix: Bug claim aud is not a string

* chore: increased test coverage

* chore: updated README

* chore: increase coverage

* chore: increase coverage
  • Loading branch information
lmammino authored Mar 18, 2024
1 parent d3da62d commit 09459db
Show file tree
Hide file tree
Showing 2 changed files with 99 additions and 16 deletions.
4 changes: 2 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,8 @@ The following section describes the steps that are followed to validate a token:
2. The token is decoded and the header is parsed to extract the `kid` (key id) and the `alg` (algorithm) claims. If the `kid` is not found, the token is rejected. If the `alg` is not supported, the token is rejected.
3. The `kid` is used to look up the public key in the JWKS (JSON Web Key Set) provided by the OIDC provider. If the key is not found, the key is refreshed and the lookup is retried. If the key is still not found, the token is rejected. The JWKS cache is optimistic, it does not automatically refresh keys unless a lookup fails. It also does not auto-refresh keys too often (to avoid unnecessary calls to the JWKS endpoint). You can configure the minimum refresh rate (in seconds) using the `MIN_REFRESH_RATE` environment variable.
4. The token is decoded and validated using the public key. If the validation fails, the token is rejected. This validation also checks the `exp` (expiration time) claim and the `nbf` (not before) claim. If the token is expired or not yet valid, the token is rejected.
5. The `iss` (issuer) claim is checked against the list of accepted issuers. If the issuer is not found in the list, the token is rejected. If the list is empty, any issuer is accepted.
6. The `aud` (audience) claim is checked against the list of accepted audiences. If the audience is not found in the list, the token is rejected. If the list is empty, any audience is accepted.
5. The `iss` (issuer) claim is checked against the list of accepted issuers. If the issuer is not found in the list, the token is rejected. If the accept list is empty, any issuer is accepted. If the token contains multiple issuers (array of strings), this check will make sure that at least one of the issuers in the token matches the provided list of accepted issuers.
6. The `aud` (audience) claim is checked against the list of accepted audiences. If the audience is not found in the list, the token is rejected. If the list is empty, any audience is accepted. If the token contains multiple audiences (array of strings), this check will make sure that at least one of the audiences in the token matches the provided list of accepted audiences.
7. If all these checks are passed, the token is considered valid and the request is allowed to proceed. The principal ID is extracted from the token using the list of principal ID claims. If no principal ID claim is found, the default principal ID is used.


Expand Down
111 changes: 97 additions & 14 deletions src/accepted_claims.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,35 @@
use serde_json::Value;
use std::fmt::Display;

#[derive(Debug, Clone, Default, Eq, PartialEq)]
pub struct AcceptedClaims(Vec<String>, String);

pub enum StringOrArray<'a> {
String(&'a str),
Array(Vec<String>),
}

impl Display for StringOrArray<'_> {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
StringOrArray::String(s) => write!(f, "{}", s),
StringOrArray::Array(s) => write!(f, "[{:?}]", s),
}
}
}

impl<'a> From<&'a str> for StringOrArray<'a> {
fn from(s: &'a str) -> Self {
StringOrArray::String(s)
}
}

impl<S: Display> From<&[S]> for StringOrArray<'_> {
fn from(a: &[S]) -> Self {
StringOrArray::Array(a.iter().map(|s| s.to_string()).collect())
}
}

impl AcceptedClaims {
pub fn new(accepted_values: Vec<String>, claim_name: String) -> Self {
Self(accepted_values, claim_name)
Expand All @@ -18,8 +45,14 @@ impl AcceptedClaims {
Self::new(accepted_values, claim_name)
}

pub fn is_accepted(&self, claim_value: &str) -> bool {
self.0.is_empty() || self.0.contains(&claim_value.to_string())
pub fn is_accepted(&self, claim_value: &StringOrArray) -> bool {
self.0.is_empty()
|| match claim_value {
StringOrArray::String(claim_value) => self.0.contains(&claim_value.to_string()),
StringOrArray::Array(claim_values) => claim_values
.iter()
.any(|claim_value| self.0.contains(claim_value)),
}
}

pub fn assert(&self, claims: &Value) -> Result<(), String> {
Expand All @@ -29,14 +62,32 @@ impl AcceptedClaims {
}

let claim_value = match claims.get(&self.1) {
Some(claim_value) => match claim_value.as_str() {
Some(claim_value) => claim_value,
None => return Err(format!("Claim '{}' is not a string", self.1)),
Some(claim_value) => match claim_value {
Value::String(claim_value) => StringOrArray::String(claim_value),
Value::Array(claim_values) => {
let claim_values = claim_values
.iter()
.map(|claim_value| match claim_value {
Value::String(claim_value) => Ok(claim_value.to_string()),
_ => Err(format!(
"Claim '{}' is not a string or an array of strings",
self.1
)),
})
.collect::<Result<Vec<_>, _>>()?;
StringOrArray::Array(claim_values)
}
_ => {
return Err(format!(
"Claim '{}' is not a string or an array of strings",
self.1
))
}
},
None => return Err(format!("Missing claim '{}'", self.1)),
};

match self.is_accepted(claim_value) {
match self.is_accepted(&claim_value) {
true => Ok(()),
false => Err(format!(
"Unsupported value for claim '{}' (found='{}', supported={:?})",
Expand Down Expand Up @@ -73,41 +124,62 @@ mod tests {

#[test]
fn it_should_accept_expected_claims_and_reject_others() {
// example.com and example.org are accepted
// example.net is not accepted

let accepted_claims = AcceptedClaims::from_comma_separated_values(
"https://example.com, https://example.org",
"iss".to_string(),
);

assert!(accepted_claims.is_accepted("https://example.com"));
assert!(accepted_claims.is_accepted(&"https://example.com".into()));
assert!(accepted_claims
.assert(&json!({"iss": "https://example.com"}))
.is_ok());
assert!(accepted_claims.is_accepted("https://example.org"));
assert!(accepted_claims.is_accepted(&"https://example.org".into()));
assert!(accepted_claims
.assert(&json!({"iss": "https://example.org"}))
.is_ok());
assert!(!accepted_claims.is_accepted("https://example.net"));

// if the claim is an array, it should accept if at least one of the values is accepted
assert!(accepted_claims
.assert(&json!({"iss": ["https://example.net", "https://example.com"]}))
.is_ok());

// do not accept example.net (not listed)
assert!(!accepted_claims.is_accepted(&"https://example.net".into()));
assert!(accepted_claims
.assert(&json!({"iss": "https://example.net"}))
.is_err());

// do not accept example.net (not listed) even when an array of claims was provided in the token
assert!(!accepted_claims
.is_accepted(&["https://example.net", "https://example.tld"][..].into()));
assert!(accepted_claims
.assert(&json!({"iss": ["https://example.net", "https://example.tld"]}))
.is_err(),);
}

#[test]
fn it_should_accept_everything_when_using_an_empty_list() {
let accepted_claims = AcceptedClaims::from_comma_separated_values("", "iss".to_string());

assert!(accepted_claims.is_accepted("https://example.com"));
assert!(accepted_claims.is_accepted(&"https://example.com".into()));
assert!(accepted_claims
.assert(&json!({"iss": "https://example.com"}))
.is_ok());
assert!(accepted_claims.is_accepted("https://example.org"));
assert!(accepted_claims.is_accepted(&"https://example.org".into()));
assert!(accepted_claims
.assert(&json!({"iss": "https://example.org"}))
.is_ok());
assert!(accepted_claims.is_accepted("https://example.net"));
assert!(accepted_claims.is_accepted(&"https://example.net".into()));
assert!(accepted_claims
.assert(&json!({"iss": "https://example.net"}))
.is_ok());
// it should accept an array of strings
assert!(accepted_claims
.assert(&json!({"iss": ["https://example.net", "https://example.com"]}))
.is_ok());
// it should also accept tokens with the missing claim
assert!(accepted_claims
.assert(&json!({"some_other_claim": "some_value"}))
Expand All @@ -129,19 +201,30 @@ mod tests {
}

#[test]
fn it_should_reject_if_the_claim_is_not_a_string() {
fn it_should_reject_if_the_claim_is_not_a_string_or_an_array_of_strings() {
let accepted_claims = AcceptedClaims::from_comma_separated_values(
"https://example.com, https://example.org",
"iss".to_string(),
);

// not a string
let result = accepted_claims.assert(&json!({
"iss": 22
}));
assert!(result.is_err());
assert_eq!(
result.unwrap_err(),
"Claim 'iss' is not a string".to_string()
"Claim 'iss' is not a string or an array of strings".to_string()
);

// not an array of string
let result = accepted_claims.assert(&json!({
"iss": ["https://example.com", 22, "https://example.org"]
}));
assert!(result.is_err());
assert_eq!(
result.unwrap_err(),
"Claim 'iss' is not a string or an array of strings".to_string()
);
}
}

0 comments on commit 09459db

Please sign in to comment.