Support receiving JSON values from Webhooks/JWT/NoAuth instead of jus…

…t strings (#1257)

### What

Previously, we only supported String as the type that contained your
session variable value when they are provided from webhooks/JWT/NoAuth.
We then coerced that value into whatever type was actually expected (eg
a float) later.

However, when we added support for array-typed session variables (#1221)
we didn't actually allow you to provide a JSON array of values as a
session variable value. You had to provide a string that contained a
JSON-encoded array of values. This meant that webhooks/JWT/NoAuth had to
double JSON-encode their session variables when returning them.

This PR fixes this and makes it so that webhooks/JWT/NoAuth can return
JSON values for session variables and that JSON is respected. So if a
session variable needs to be an array of integers, they can simply
return the JSON array of integers as the value for that session
variable.

### How

Instead of holding a `SessionVariableValue` as a `String`, we now turn
that into an enum where we have an "unparsed" String (used for when we
don't receive JSON, we just receive a string value (ie. http headers)),
or a "parsed" JSON value. When we receive session variables from
webhooks/JWT/NoAuth, we relax the restriction that they can only return
us JSON strings, and instead allow them to return JSON Values, which we
put in the new `SessionVariableValue::Parsed` enum variant. HTTP headers
go into `SessionVariableValue::Unparsed`.

Then, when we go to get the required value from the
`SessionVariableValue` based on the desired type, we either parse it out
of the "unparsed" String, or we expect that the value is already in the
correct form in the "parsed" JSON value. This is the behaviour you will
get if JSON session variables are turned on in the Flags.

If JSON session variables are not turned on, then we expect that only
String session variables (parsed or unparsed) are provided from
headers/webhooks/JWT/NoAuth, and so we run the old logic of always
expecting a String and parsing the correct value out of it.

V3_GIT_ORIGIN_REV_ID: b6734ad5443b7d68065f91aea71386c893aa7eba

v3/changelog.md

@@ -9,6 +9,15 @@
 
 ### Fixed
 
+- When the `CompatibilityConfig` date is set to `2024-10-16` or newer, session
+  variables returned by webhooks, set in `noAuth` config in `AuthConfig` or set
+  in JWT claims are now correctly allowed to be full JSON values, not just JSON
+  strings. This fixes the bug where you were incorrectly required to JSON-encode
+  your JSON value inside a string. For example, you were previously incorrectly
+  required to return session variables like this
+  `{ "X-Hasura-AllowedUserIds": "[1,2,3]" }`, but now you can correctly return
+  them like this: `{ "X-Hasura-AllowedUserIds": [1,2,3] }`.
+
 ### Changed
 
 ## [v2024.10.21]

v3/crates/auth/hasura-authn-core/Cargo.toml

@@ -13,9 +13,11 @@ open-dds = { path = "../../open-dds" }
 
 axum = { workspace = true }
 axum-core = { workspace = true }
+derive_more = { workspace = true }
 http = { workspace = true }
 schemars = { workspace = true }
 serde = { workspace = true }
+serde_json = { workspace = true }
 thiserror = { workspace = true }
 
 [dev-dependencies]

v3/crates/auth/hasura-authn-core/src/lib.rs

@@ -26,16 +26,73 @@ pub use open_dds::{
     session_variables::{SessionVariableName, SessionVariableReference, SESSION_VARIABLE_ROLE},
 };
 
-#[derive(Clone, Debug, Eq, PartialEq, serde::Serialize, serde::Deserialize, JsonSchema)]
-/// Value of a session variable
-pub struct SessionVariableValue(pub String);
+#[derive(Clone, Debug, Eq, PartialEq, serde::Serialize, derive_more::Display)]
+/// Value of a session variable, used to capture session variable input from parsed sources (jwt, webhook, etc)
+/// and unparsed sources (http headers)
+pub enum SessionVariableValue {
+    /// An unparsed session variable value as a string. Might be a raw string, might be a number, might be json.
+    /// How we interpret it depends on what type we're trying to coerce to from the string
+    #[display(fmt = "{_0}")]
+    Unparsed(String),
+    /// A parsed JSON session variable value. We know what the type is because we parsed it from JSON.
+    #[display(fmt = "{_0}")]
+    Parsed(serde_json::Value),
+}
 
 impl SessionVariableValue {
     pub fn new(value: &str) -> Self {
-        SessionVariableValue(value.to_string())
+        SessionVariableValue::Unparsed(value.to_string())
+    }
+
+    /// Assert that a session variable represents a string, regardless of encoding
+    pub fn as_str(&self) -> Option<&str> {
+        match self {
+            SessionVariableValue::Unparsed(s) => Some(s.as_str()),
+            SessionVariableValue::Parsed(value) => value.as_str(),
+        }
+    }
+
+    pub fn as_i64(&self) -> Option<i64> {
+        match self {
+            SessionVariableValue::Unparsed(s) => s.parse::<i64>().ok(),
+            SessionVariableValue::Parsed(value) => value.as_i64(),
+        }
+    }
+
+    pub fn as_f64(&self) -> Option<f64> {
+        match self {
+            SessionVariableValue::Unparsed(s) => s.parse::<f64>().ok(),
+            SessionVariableValue::Parsed(value) => value.as_f64(),
+        }
+    }
+
+    pub fn as_bool(&self) -> Option<bool> {
+        match self {
+            SessionVariableValue::Unparsed(s) => s.parse::<bool>().ok(),
+            SessionVariableValue::Parsed(value) => value.as_bool(),
+        }
+    }
+
+    pub fn as_value(&self) -> serde_json::Result<serde_json::Value> {
+        match self {
+            SessionVariableValue::Unparsed(s) => serde_json::from_str(s),
+            SessionVariableValue::Parsed(value) => Ok(value.clone()),
+        }
+    }
+}
+
+impl From<JsonSessionVariableValue> for SessionVariableValue {
+    fn from(value: JsonSessionVariableValue) -> Self {
+        SessionVariableValue::Parsed(value.0)
     }
 }
 
+/// JSON value of a session variable
+// This is used instead of SessionVariableValue when only JSON session variable values are accepted
+#[derive(Debug, serde::Serialize, serde::Deserialize, PartialEq, Clone, JsonSchema)]
+#[schemars(rename = "SessionVariableValue")] // Renamed to keep json schema compatibility
+pub struct JsonSessionVariableValue(pub serde_json::Value);
+
 #[derive(Clone, Debug, Eq, PartialEq, serde::Serialize)]
 pub struct SessionVariables(HashMap<SessionVariableName, SessionVariableValue>);
 
@@ -184,16 +241,17 @@ pub fn authorize_identity(
     // traverse through the headers and collect role and session variables
     for (header_name, header_value) in headers {
         if let Ok(session_variable) = SessionVariableName::from_str(header_name.as_str()) {
-            let variable_value = match header_value.to_str() {
+            let variable_value_str = match header_value.to_str() {
                 Err(e) => Err(SessionError::InvalidHeaderValue {
                     header_name: header_name.to_string(),
                     error: e.to_string(),
                 })?,
-                Ok(h) => SessionVariableValue::new(h),
+                Ok(h) => h,
             };
+            let variable_value = SessionVariableValue::Unparsed(variable_value_str.to_string());
 
             if session_variable == SESSION_VARIABLE_ROLE {
-                role = Some(Role::new(&variable_value.0));
+                role = Some(Role::new(variable_value_str));
             } else {
                 // TODO: Handle the duplicate case?
                 session_variables.insert(session_variable, variable_value);

v3/crates/auth/hasura-authn-jwt/src/auth.rs

@@ -16,7 +16,11 @@ fn build_allowed_roles(
             // Note: The same `custom_claims` is being cloned
             // for every role present in the allowed roles.
             // We should think of having common claims.
-            session_variables: hasura_claims.custom_claims.clone(),
+            session_variables: hasura_claims
+                .custom_claims
+                .iter()
+                .map(|(k, v)| (k.clone(), v.clone().into()))
+                .collect(),
             allowed_session_variables_from_request: auth_base::SessionVariableList::Some(
                 HashSet::new(),
             ),
@@ -75,7 +79,14 @@ pub async fn authenticate_request(
                                 let role = hasura_claims
                                     .custom_claims
                                     .get(&SESSION_VARIABLE_ROLE)
-                                    .map(|v| Role::new(v.0.as_str()));
+                                    .map(|v| {
+                                        Ok::<_, Error>(Role::new(v.0.as_str().ok_or_else(
+                                            || Error::ClaimMustBeAString {
+                                                claim_name: SESSION_VARIABLE_ROLE.to_string(),
+                                            },
+                                        )?))
+                                    })
+                                    .transpose()?;
                                 match role {
                                     // `x-hasura-role` is found, check if it's the
                                     // role that can emulate by comparing it to
@@ -111,6 +122,7 @@ mod tests {
     use std::str::FromStr;
 
     use auth_base::{RoleAuthorization, SessionVariableValue};
+    use hasura_authn_core::JsonSessionVariableValue;
     use jsonwebtoken as jwt;
     use jsonwebtoken::Algorithm;
     use jwt::{encode, EncodingKey};
@@ -168,7 +180,7 @@ mod tests {
         let mut hasura_custom_claims = HashMap::new();
         hasura_custom_claims.insert(
             SessionVariableName::from_str("x-hasura-user-id").unwrap(),
-            SessionVariableValue("1".to_string()),
+            JsonSessionVariableValue(json!("1")),
         );
         HasuraClaims {
             default_role: Role::new("user"),
@@ -225,7 +237,7 @@ mod tests {
 
         role_authorization_session_variables.insert(
             SessionVariableName::from_str("x-hasura-user-id").unwrap(),
-            SessionVariableValue::new("1"),
+            SessionVariableValue::Parsed(json!("1")),
         );
         expected_allowed_roles.insert(
             test_role.clone(),
@@ -274,7 +286,7 @@ mod tests {
         let mut hasura_claims = get_default_hasura_claims();
         hasura_claims.custom_claims.insert(
             SessionVariableName::from_str("x-hasura-role").unwrap(),
-            SessionVariableValue::new("admin"),
+            JsonSessionVariableValue(json!("admin")),
         );
         let encoded_claims = get_encoded_claims(Algorithm::HS256, &hasura_claims)?;
 

v3/crates/auth/hasura-authn-jwt/src/jwt.rs

@@ -4,7 +4,7 @@ use std::time::Duration;
 use axum::http::{HeaderMap, HeaderValue};
 use axum::response::IntoResponse;
 use cookie::{self, Cookie};
-use hasura_authn_core::{Role, SessionVariableValue};
+use hasura_authn_core::{JsonSessionVariableValue, Role};
 use jsonptr::Pointer;
 use jsonwebtoken::{self as jwt, decode, DecodingKey, Validation};
 use jwt::decode_header;
@@ -40,6 +40,8 @@ pub enum Error {
         claim_name: String,
         err: serde_json::Error,
     },
+    #[error("Expected string value for claim {claim_name}")]
+    ClaimMustBeAString { claim_name: String },
     #[error("Required claim {claim_name} not found")]
     RequiredClaimNotFound { claim_name: String },
     #[error("JWT Authorization token source: Header name {header_name} not found.")]
@@ -114,7 +116,8 @@ impl Error {
                 header_name: _,
             }
             | Error::CookieParseError { err: _ }
-            | Error::MissingCookieValue { cookie_name: _ } => StatusCode::BAD_REQUEST,
+            | Error::MissingCookieValue { cookie_name: _ }
+            | Error::ClaimMustBeAString { claim_name: _ } => StatusCode::BAD_REQUEST,
         }
     }
 }
@@ -254,7 +257,7 @@ pub struct JWTClaimsMap {
     /// A dictionary of the custom claims, where the key is the name of the claim and the value
     /// is the JSON pointer to lookup the custom claims within the decoded JWT.
     pub custom_claims:
-        Option<HashMap<SessionVariableName, JWTClaimsMappingEntry<SessionVariableValue>>>,
+        Option<HashMap<SessionVariableName, JWTClaimsMappingEntry<JsonSessionVariableValue>>>,
 }
 
 #[derive(Serialize, Deserialize, PartialEq, Clone, JsonSchema, Debug)]
@@ -391,7 +394,7 @@ pub struct HasuraClaims {
     /// as per the user's defined permissions.
     /// For example, things like `x-hasura-user-id` can go here.
     #[serde(flatten)]
-    pub custom_claims: HashMap<SessionVariableName, SessionVariableValue>,
+    pub custom_claims: HashMap<SessionVariableName, JsonSessionVariableValue>,
 }
 
 #[derive(Debug, Serialize, Deserialize)]
@@ -735,7 +738,7 @@ mod tests {
         let mut hasura_custom_claims = HashMap::new();
         hasura_custom_claims.insert(
             SessionVariableName::from_str("x-hasura-user-id").unwrap(),
-            SessionVariableValue("1".to_string()),
+            JsonSessionVariableValue(json!("1")),
         );
         HasuraClaims {
             default_role: Role::new("user"),

v3/crates/auth/hasura-authn-noauth/src/lib.rs

@@ -1,4 +1,4 @@
-use hasura_authn_core::{Role, SessionVariableName, SessionVariableValue};
+use hasura_authn_core::{JsonSessionVariableValue, Role, SessionVariableName};
 use schemars::JsonSchema;
 use serde::{Deserialize, Serialize};
 use serde_json::json;
@@ -15,7 +15,7 @@ pub struct NoAuthConfig {
     pub role: Role,
     /// static session variables to use whilst running the engine
     #[schemars(title = "SessionVariables")]
-    pub session_variables: HashMap<SessionVariableName, SessionVariableValue>,
+    pub session_variables: HashMap<SessionVariableName, JsonSessionVariableValue>,
 }
 
 impl NoAuthConfig {
@@ -40,7 +40,11 @@ pub fn identity_from_config(no_auth_config: &NoAuthConfig) -> hasura_authn_core:
         no_auth_config.role.clone(),
         hasura_authn_core::RoleAuthorization {
             role: no_auth_config.role.clone(),
-            session_variables: no_auth_config.session_variables.clone(),
+            session_variables: no_auth_config
+                .session_variables
+                .iter()
+                .map(|(k, v)| (k.clone(), v.clone().into()))
+                .collect(),
             allowed_session_variables_from_request: hasura_authn_core::SessionVariableList::Some(
                 HashSet::new(),
             ),

v3/crates/auth/hasura-authn-webhook/src/webhook.rs

@@ -46,6 +46,8 @@ pub enum InternalError {
     ReqwestError(reqwest::Error),
     #[error("'x-hasura-role' session variable not found in the webhook response.")]
     RoleSessionVariableNotFound,
+    #[error("'x-hasura-role' session variable in the webhook response was not a string.")]
+    RoleSessionVariableMustBeString,
 }
 
 impl TraceableError for InternalError {
@@ -202,14 +204,14 @@ async fn make_auth_hook_request(
     match response.status() {
         reqwest::StatusCode::UNAUTHORIZED => Err(Error::AuthenticationFailed),
         reqwest::StatusCode::OK => {
-            let auth_hook_response: HashMap<String, String> =
+            let auth_hook_response: HashMap<String, serde_json::Value> =
                 response.json().await.map_err(InternalError::ReqwestError)?;
             let mut session_variables = HashMap::new();
             for (k, v) in &auth_hook_response {
                 match SessionVariableName::from_str(k) {
                     Ok(session_variable) => {
                         session_variables
-                            .insert(session_variable, SessionVariableValue(v.to_string()));
+                            .insert(session_variable, SessionVariableValue::Parsed(v.clone()));
                     }
                     Err(_e) => {}
                 }
@@ -218,8 +220,8 @@ async fn make_auth_hook_request(
                 session_variables
                     .get(&session_variables::SESSION_VARIABLE_ROLE)
                     .ok_or(InternalError::RoleSessionVariableNotFound)?
-                    .0
-                    .as_str(),
+                    .as_str()
+                    .ok_or_else(|| InternalError::RoleSessionVariableMustBeString)?,
             );
             let role_authorization = RoleAuthorization {
                 role: role.clone(),
@@ -324,6 +326,7 @@ mod tests {
     use mockito;
     use rand::{thread_rng, Rng};
     use reqwest::header::CONTENT_TYPE;
+    use serde_json::json;
 
     #[tokio::test]
     // This test emulates a successful authentication by the webhook using Get method
@@ -367,11 +370,11 @@ mod tests {
         let mut role_authorization_session_variables = HashMap::new();
         role_authorization_session_variables.insert(
             SessionVariableName::from_str("x-hasura-role").unwrap(),
-            SessionVariableValue::new("test-role"),
+            SessionVariableValue::Parsed(json!("test-role")),
         );
         role_authorization_session_variables.insert(
             SessionVariableName::from_str("x-hasura-test-role-id").unwrap(),
-            SessionVariableValue::new("1"),
+            SessionVariableValue::Parsed(json!("1")),
         );
         expected_allowed_roles.insert(
             test_role.clone(),
@@ -438,11 +441,11 @@ mod tests {
         let mut role_authorization_session_variables = HashMap::new();
         role_authorization_session_variables.insert(
             SessionVariableName::from_str("x-hasura-role").unwrap(),
-            SessionVariableValue::new("test-role"),
+            SessionVariableValue::Parsed(json!("test-role")),
         );
         role_authorization_session_variables.insert(
             SessionVariableName::from_str("x-hasura-test-role-id").unwrap(),
-            SessionVariableValue::new("1"),
+            SessionVariableValue::Parsed(json!("1")),
         );
         expected_allowed_roles.insert(
             test_role.clone(),
@@ -510,15 +513,15 @@ mod tests {
         let mut role_authorization_session_variables = HashMap::new();
         role_authorization_session_variables.insert(
             SessionVariableName::from_str("x-hasura-role").unwrap(),
-            SessionVariableValue::new("test-role"),
+            SessionVariableValue::Parsed(json!("test-role")),
         );
         role_authorization_session_variables.insert(
             SessionVariableName::from_str("x-hasura-test-role-id").unwrap(),
-            SessionVariableValue::new("1"),
+            SessionVariableValue::Parsed(json!("1")),
         );
         role_authorization_session_variables.insert(
             SessionVariableName::from_str("status").unwrap(),
-            SessionVariableValue::new("true"),
+            SessionVariableValue::Parsed(json!("true")),
         );
         expected_allowed_roles.insert(
             test_role.clone(),
@@ -638,11 +641,11 @@ mod tests {
         let mut role_authorization_session_variables = HashMap::new();
         role_authorization_session_variables.insert(
             SessionVariableName::from_str("x-hasura-role").unwrap(),
-            SessionVariableValue::new("user"),
+            SessionVariableValue::Parsed(json!("user")),
         );
         role_authorization_session_variables.insert(
             SessionVariableName::from_str("x-hasura-user-id").unwrap(),
-            SessionVariableValue::new("1"),
+            SessionVariableValue::Parsed(json!("1")),
         );
         expected_allowed_roles.insert(
             test_role.clone(),