Fix and improve scope extraction implementation

mrrajan · claude · mrrajan · commit 42d4fff47cf2 · 2026-03-19T20:22:09.000+05:30
Co-Authored-By: Claude Sonnet 4.5 &lt;noreply@anthropic.com&gt;
Signed-off-by: mrrajan &lt;86094767+mrrajan@users.noreply.github.com.&gt;
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/common/auth/Cargo.toml b/common/auth/Cargo.toml
@@ -37,6 +37,9 @@ url = { workspace = true }
 utoipa = { workspace = true, features = ["actix_extras"], optional = true }
 utoipa-swagger-ui = { workspace = true, features = ["actix-web"], optional = true }
 
+[dev-dependencies]
+rstest = { workspace = true }
+
 [features]
 actix = ["actix-web", "actix-http", "actix-web-httpauth"]
 swagger = ["utoipa", "utoipa-swagger-ui", "actix"]
diff --git a/common/auth/schema/auth.json b/common/auth/schema/auth.json
@@ -77,6 +77,14 @@
           ],
           "default": null
         },
+        "scopeSelector": {
+          "description": "JSON path extracting scopes from the access token (default: $['scope','scp'])",
+          "type": [
+            "string",
+            "null"
+          ],
+          "default": null
+        },
         "groupMappings": {
           "description": "Mapping table for groups returned found through the `groups_selector` to permissions.",
           "type": "object",
diff --git a/common/auth/src/authenticator/claims.rs b/common/auth/src/authenticator/claims.rs
@@ -27,11 +27,6 @@ pub struct AccessTokenClaims {
 
     #[serde(default, skip_serializing_if = "String::is_empty")]
     pub scope: String,
-
-    /// Azure Entra ID uses 'scp' instead of 'scope'
-    /// This can be either a single string or an array of strings
-    #[serde(default, skip_serializing_if = "Option::is_none")]
-    pub scp: Option<SingleOrMultiple<String>>,
 }
 
 impl CompactJson for AccessTokenClaims {}
diff --git a/common/auth/src/authenticator/config.rs b/common/auth/src/authenticator/config.rs
@@ -44,6 +44,7 @@ impl AuthenticatorConfig {
                     additional_permissions: Default::default(),
                     required_audience: None,
                     group_selector: None,
+                    scope_selector: None,
                     group_mappings: Default::default(),
                     tls_insecure: false,
                     tls_ca_certificates: Default::default(),
@@ -132,6 +133,10 @@ pub struct AuthenticatorClientConfig {
     #[serde(default)]
     pub group_selector: Option<String>,
 
+    /// JSON path extracting scopes from the access token (default: $['scope','scp'])
+    #[serde(default)]
+    pub scope_selector: Option<String>,
+
     /// Mapping table for groups returned found through the `groups_selector` to permissions.
     #[serde(default, skip_serializing_if = "HashMap::is_empty")]
     pub group_mappings: HashMap<String, Vec<String>>,
@@ -157,6 +162,7 @@ impl SingleAuthenticatorClientConfig {
                 required_audience: self.required_audience.clone(),
                 scope_mappings: default_scope_mappings(),
                 group_selector: None,
+                scope_selector: None,
                 group_mappings: Default::default(),
                 additional_permissions: Default::default(),
             })
diff --git a/common/auth/src/authenticator/mod.rs b/common/auth/src/authenticator/mod.rs
@@ -16,7 +16,7 @@ use crate::{
     authenticator::claims::ValidatedAccessToken, authenticator::config::AuthenticatorConfig,
 };
 use anyhow::anyhow;
-use biscuit::{SingleOrMultiple, jws::Compact};
+use biscuit::jws::Compact;
 use claims::AccessTokenClaims;
 use config::AuthenticatorClientConfig;
 use error::AuthenticationError;
@@ -31,6 +31,8 @@ use std::{collections::HashMap, ops::Deref};
 use tracing::instrument;
 use trustify_common::reqwest::ClientFactory;
 
+const DEFAULT_SCOPE_SELECTOR: &str = "$['scope','scp']";
+
 /// An authenticator to authenticate incoming requests.
 #[derive(Clone)]
 pub struct Authenticator {
@@ -192,13 +194,28 @@ async fn create_client(config: AuthenticatorClientConfig) -> anyhow::Result<Auth
         })
         .transpose()?;
 
+    let scope_selector = parse_json_path(
+        config
+            .scope_selector
+            .as_deref()
+            .unwrap_or(DEFAULT_SCOPE_SELECTOR),
+    )
+    .map_err(|err| {
+        anyhow!(
+            "Unable to parse JSON path scope selector for client '{}' / '{}': {err}",
+            config.issuer_url,
+            client.client_id,
+        )
+    })?;
+
     Ok(AuthenticatorClient {
         client,
         audience: config.required_audience,
         scope_mappings: config.scope_mappings,
         additional_permissions: config.additional_permissions,
         group_selector,
         group_mappings: config.group_mappings,
+        scope_selector,
     })
 }
 
@@ -210,14 +227,17 @@ pub struct AuthenticatorClient {
     additional_permissions: Vec<String>,
     group_selector: Option<JpQuery>,
     group_mappings: HashMap<String, Vec<String>>,
+    scope_selector: JpQuery,
 }
 
 impl AuthenticatorClient {
     /// Convert from a set of (verified!) access token claims into a [`ValidatedAccessToken`] struct.
     pub fn convert_token(&self, access_token: AccessTokenClaims) -> ValidatedAccessToken {
-        // Combine scopes from both 'scope' and 'scp' claims
-        // Azure Entra ID uses 'scp', while other providers use 'scope'
-        let scopes = Self::extract_scopes(&access_token);
+        let token_value = serde_json::to_value(&access_token).unwrap_or_else(|err| {
+            log::warn!("Failed to serialize access token for scope extraction: {err}");
+            Value::Null
+        });
+        let scopes = Self::extract_scopes(&token_value, &self.scope_selector);
         let mut permissions = Self::map_scopes(&scopes, &self.scope_mappings);
         permissions.extend(self.additional_permissions.clone());
         let groups = self
@@ -234,18 +254,27 @@ impl AuthenticatorClient {
         }
     }
 
-    /// Extract scopes from both 'scope' and 'scp' claims
-    fn extract_scopes(access_token: &AccessTokenClaims) -> String {
-        let scp_part = access_token.scp.as_ref().map(|scp| match scp {
-            SingleOrMultiple::Single(s) => s.clone(),
-            SingleOrMultiple::Multiple(list) => list.join(" "),
-        });
-
-        match (access_token.scope.is_empty(), scp_part) {
-            (true, Some(scp)) => scp,
-            (false, Some(scp)) => format!("{} {}", access_token.scope, scp),
-            (_, None) => access_token.scope.clone(),
-        }
+    /// Extract scopes from the value/access token
+    fn extract_scopes(value: &Value, selector: &JpQuery) -> String {
+        js_path_process(selector, value)
+            .ok()
+            .into_iter()
+            .flatten()
+            .flat_map(|qr| match qr.val() {
+                Value::String(s) => s
+                    .split_ascii_whitespace()
+                    .map(|s| s.to_string())
+                    .collect::<Vec<_>>(),
+                Value::Array(arr) => arr
+                    .iter()
+                    .filter_map(|v| v.as_str())
+                    .flat_map(|s| s.split_ascii_whitespace())
+                    .map(|s| s.to_string())
+                    .collect(),
+                _ => vec![],
+            })
+            .collect::<Vec<_>>()
+            .join(" ")
     }
 
     /// Extract the groups from the value/access token
@@ -298,7 +327,8 @@ impl Deref for AuthenticatorClient {
 #[cfg(test)]
 mod test {
     use super::*;
-    use biscuit::SingleOrMultiple;
+    use rstest::rstest;
+    use serde_json::json;
 
     fn assert_scope_mapping(scopes: &str, mappings: &[(&str, &[&str])], expected: &[&str]) {
         let mappings = mappings
@@ -356,89 +386,55 @@ mod test {
         assert_eq!(&groups, &["manager", "reader"]);
     }
 
-    #[test]
-    fn test_extract_scopes_from_scope_claim() {
-        let claims = AccessTokenClaims {
-            azp: None,
-            sub: "user123".to_string(),
-            iss: "https://example.com".parse().unwrap(),
-            aud: None,
-            exp: 0,
-            iat: 0,
-            auth_time: None,
-            extended_claims: Value::Object(Default::default()),
-            scope: "read:document create:document".to_string(),
-            scp: None,
-        };
-
-        let scopes = AuthenticatorClient::extract_scopes(&claims);
-        assert_eq!(scopes, "read:document create:document");
+    #[rstest]
+    #[case::scope_only("read:document create:document", json!({}), "read:document create:document")]
+    #[case::scp_string("", json!({"scp": "read:document"}), "read:document")]
+    #[case::scp_array("", json!({"scp": ["api://app/read:document", "api://app/create:document"]}), "api://app/read:document api://app/create:document")]
+    #[case::both_claims("openid profile", json!({"scp": ["api://app/read:document", "api://app/create:document"]}), "openid profile api://app/read:document api://app/create:document")]
+    fn test_extract_scopes(#[case] scope: &str, #[case] mut extra: Value, #[case] expected: &str) {
+        let selector = parse_json_path(DEFAULT_SCOPE_SELECTOR).unwrap();
+        if !scope.is_empty() {
+            extra["scope"] = json!(scope);
+        }
+        assert_eq!(
+            AuthenticatorClient::extract_scopes(&extra, &selector),
+            expected
+        );
     }
 
     #[test]
-    fn test_extract_scopes_from_scp_claim_single() {
-        let claims = AccessTokenClaims {
-            azp: None,
-            sub: "user123".to_string(),
-            iss: "https://example.com".parse().unwrap(),
-            aud: None,
-            exp: 0,
-            iat: 0,
-            auth_time: None,
-            extended_claims: Value::Object(Default::default()),
-            scope: "".to_string(),
-            scp: Some(SingleOrMultiple::Single("read:document".to_string())),
-        };
-
-        let scopes = AuthenticatorClient::extract_scopes(&claims);
-        assert_eq!(scopes, "read:document");
+    fn test_extract_scopes_custom_selector() {
+        let selector = parse_json_path("$.scp").unwrap();
+        let value = json!({
+            "scope": "openid profile",
+            "scp": ["read:document", "create:document"]
+        });
+        // Should extract only from scp, ignoring scope
+        assert_eq!(
+            AuthenticatorClient::extract_scopes(&value, &selector),
+            "read:document create:document"
+        );
     }
 
-    #[test]
-    fn test_extract_scopes_from_scp_claim_multiple() {
-        let claims = AccessTokenClaims {
-            azp: None,
-            sub: "user123".to_string(),
-            iss: "https://example.com".parse().unwrap(),
-            aud: None,
-            exp: 0,
-            iat: 0,
-            auth_time: None,
-            extended_claims: Value::Object(Default::default()),
-            scope: "".to_string(),
-            scp: Some(SingleOrMultiple::Multiple(vec![
-                "api://app/read:document".to_string(),
-                "api://app/create:document".to_string(),
-            ])),
-        };
-
-        let scopes = AuthenticatorClient::extract_scopes(&claims);
-        assert_eq!(scopes, "api://app/read:document api://app/create:document");
+    #[rstest]
+    #[case::null_value(json!({"scope": null}), "")]
+    #[case::number_value(json!({"scope": 42}), "")]
+    #[case::boolean_value(json!({"scope": true}), "")]
+    #[case::object_value(json!({"scope": {"nested": "value"}}), "")]
+    #[case::empty_string(json!({"scope": ""}), "")]
+    #[case::empty_array(json!({"scp": []}), "")]
+    #[case::no_matching_fields(json!({"other": "value"}), "")]
+    fn test_extract_scopes_non_string_and_empty(#[case] value: Value, #[case] expected: &str) {
+        let selector = parse_json_path(DEFAULT_SCOPE_SELECTOR).unwrap();
+        assert_eq!(AuthenticatorClient::extract_scopes(&value, &selector), expected);
     }
 
-    // Combine scopes from both claims (additive, not fallback)
-    #[test]
-    fn test_extract_scopes_from_both_claims() {
-        let claims = AccessTokenClaims {
-            azp: None,
-            sub: "user123".to_string(),
-            iss: "https://example.com".parse().unwrap(),
-            aud: None,
-            exp: 0,
-            iat: 0,
-            auth_time: None,
-            extended_claims: Value::Object(Default::default()),
-            scope: "openid profile".to_string(),
-            scp: Some(SingleOrMultiple::Multiple(vec![
-                "api://app/read:document".to_string(),
-                "api://app/create:document".to_string(),
-            ])),
-        };
-
-        let scopes = AuthenticatorClient::extract_scopes(&claims);
-        assert_eq!(
-            scopes,
-            "openid profile api://app/read:document api://app/create:document"
-        );
+    #[rstest]
+    #[case::mixed_types(json!({"scp": ["read:document", 42, null, "write:document", true]}), "read:document write:document")]
+    #[case::string_multiple_whitespace(json!({"scope": "  read:document   write:document  "}), "read:document write:document")]
+    #[case::array_with_whitespace(json!({"scp": ["  read:document write:document  ", "  delete:document  "]}), "read:document write:document delete:document")]
+    fn test_extract_scopes_edge_cases(#[case] value: Value, #[case] expected: &str) {
+        let selector = parse_json_path(DEFAULT_SCOPE_SELECTOR).unwrap();
+        assert_eq!(AuthenticatorClient::extract_scopes(&value, &selector), expected);
     }
 }
