fix: read policy conditions and session tags in name order on all builds

crates/auth/src/policy/document.rs

@@ -390,12 +390,12 @@ pub fn parse_conditions(value: &Value) -> Result<Vec<Condition>, PolicyParseErro
     };
 
     let mut conditions = Vec::new();
-    for (op_name, keys_obj) in obj {
+    for (op_name, keys_obj) in by_name(obj) {
         let operator = parse_operator(op_name)?;
         let keys = keys_obj
             .as_object()
             .ok_or_else(|| PolicyParseError(format!("condition {op_name} must be an object")))?;
-        for (key, vals) in keys {
+        for (key, vals) in by_name(keys) {
             let values = parse_string_or_array(vals)?;
             conditions.push(Condition {
                 operator: operator.clone(),
@@ -407,6 +407,19 @@ pub fn parse_conditions(value: &Value) -> Result<Vec<Condition>, PolicyParseErro
     Ok(conditions)
 }
 
+/// The entries of `map` in name order. Any build that compiles the MongoDB
+/// backend crate, which includes a build of the whole workspace, gets
+/// serde_json's `preserve_order` feature through `bson`, so there a map keeps
+/// the order it was written in; a build without that crate sorts it by name.
+/// A parse or a check that stops at the first fault reports the same fault on
+/// every build when it reads the entries in name order.
+#[must_use]
+pub fn by_name(map: &serde_json::Map<String, Value>) -> Vec<(&String, &Value)> {
+    let mut entries: Vec<_> = map.iter().collect();
+    entries.sort_unstable_by(|a, b| a.0.cmp(b.0));
+    entries
+}
+
 /// Parse a condition operator name, handling `ForAllValues:`, `ForAnyValue:` prefixes
 /// and `IfExists` suffix.
 fn parse_operator(name: &str) -> Result<ConditionOperator, PolicyParseError> {
@@ -512,6 +525,45 @@ mod tests {
         ));
     }
 
+    /// The fault reported for a Condition with two unknown operators is the
+    /// one whose name sorts first, in either order of the document (#780).
+    /// `cargo test --workspace` builds this crate with `preserve_order`,
+    /// which keeps the written order, so there the test tells the two apart.
+    #[test]
+    fn two_unknown_operators_report_the_first_in_name_order() {
+        for (first, second) in [("BadA", "BadB"), ("BadB", "BadA")] {
+            let json = format!(
+                r#"{{"Version":"2012-10-17","Statement":[{{"Effect":"Allow",
+                "Action":"dynamodb:GetItem","Resource":"*","Condition":{{
+                "{first}":{{"aws:k":"v"}},"{second}":{{"aws:k":"v"}}}}}}]}}"#
+            );
+            let err = PolicyDocument::from_json(&json).unwrap_err();
+            assert_eq!(err.0, "unknown operator: BadA", "{first} before {second}");
+        }
+    }
+
+    /// Conditions come out in name order of operator, then key, in either
+    /// order of the document.
+    #[test]
+    fn conditions_are_read_in_name_order() {
+        for condition in [
+            r#"{"StringLike":{"aws:b":"1","aws:a":"2"},"Bool":{"aws:c":"true"}}"#,
+            r#"{"Bool":{"aws:c":"true"},"StringLike":{"aws:a":"2","aws:b":"1"}}"#,
+        ] {
+            let json = format!(
+                r#"{{"Version":"2012-10-17","Statement":[{{"Effect":"Allow",
+                "Action":"dynamodb:GetItem","Resource":"*","Condition":{condition}}}]}}"#
+            );
+            let doc = PolicyDocument::from_json(&json).unwrap();
+            let keys: Vec<&str> = doc.statements[0]
+                .conditions
+                .iter()
+                .map(|c| c.key.as_str())
+                .collect();
+            assert_eq!(keys, ["aws:c", "aws:a", "aws:b"], "{condition}");
+        }
+    }
+
     #[test]
     fn parse_deny_with_not_action() {
         let json = r#"{

crates/server/src/management/assume_role.rs

@@ -22,7 +22,7 @@ use super::crypto::{
 };
 use extenddb_auth::policy::condition::{ConditionMatch, evaluate_statement_conditions};
 use extenddb_auth::policy::context::AssumeRoleContext;
-use extenddb_auth::policy::document::parse_conditions;
+use extenddb_auth::policy::document::{by_name, parse_conditions};
 
 #[derive(Deserialize)]
 pub struct AssumeRoleRequest {
@@ -624,7 +624,9 @@ fn session_tag_pairs(session_tags: &Value) -> Vec<(String, String)> {
     let mut pairs = Vec::new();
     match session_tags {
         Value::Object(obj) => {
-            for (k, v) in obj {
+            // In name order, so the first bad key and the later of two keys
+            // that collide by case are the same on every build.
+            for (k, v) in by_name(obj) {
                 if let Some(s) = scalar_to_string(v) {
                     pairs.push((k.clone(), s));
                 }
@@ -816,7 +818,9 @@ fn validate_trust_condition(cond: &Value) -> Result<(), String> {
     let obj = cond
         .as_object()
         .ok_or_else(|| "Condition must be an object".to_owned())?;
-    for (op_name, keys) in obj {
+    // In name order, so a block with two faults reports the same one on every
+    // build.
+    for (op_name, keys) in by_name(obj) {
         if !is_known_iam_condition_operator(op_name) {
             return Err(format!("invalid Condition operator: {op_name}"));
         }
@@ -1664,6 +1668,45 @@ mod tests {
         }
     }
 
+    /// A trust policy `Condition` with two unknown operators reports the one
+    /// whose name sorts first, in either order of the document (#780).
+    /// `cargo test --workspace` builds `serde_json` with `preserve_order`,
+    /// which keeps the written order, so there the test tells the two apart.
+    #[test]
+    fn validate_reports_the_first_bad_operator_in_name_order() {
+        for (first, second) in [("BadA", "BadB"), ("BadB", "BadA")] {
+            let doc: Value = serde_json::from_str(&format!(
+                r#"{{"Version":"2012-10-17","Statement":[{{"Effect":"Allow",
+                "Principal":{{"AWS":"{CALLER}"}},"Action":"sts:AssumeRole",
+                "Condition":{{"{first}":{{"aws:k":"v"}},"{second}":{{"aws:k":"v"}}}}}}]}}"#
+            ))
+            .unwrap();
+            assert_eq!(
+                validate_trust_policy(&doc).unwrap_err(),
+                "invalid Condition operator: BadA",
+                "{first} before {second}"
+            );
+        }
+    }
+
+    /// Object-form session tags are checked in name order: of two keys that
+    /// collide by case, the later in name order is the one reported, in either
+    /// order of the request.
+    #[test]
+    fn session_tag_pairs_are_in_name_order() {
+        for tags in [r#"{"team":"a","Team":"b"}"#, r#"{"Team":"b","team":"a"}"#] {
+            let tags: Value = serde_json::from_str(tags).unwrap();
+            let pairs = session_tag_pairs(&tags);
+            let keys: Vec<&str> = pairs.iter().map(|(k, _)| k.as_str()).collect();
+            assert_eq!(keys, ["Team", "team"], "{tags}");
+            assert_eq!(
+                super::super::duplicate_tag_key_ci(&pairs).as_deref(),
+                Some("team"),
+                "{tags}"
+            );
+        }
+    }
+
     #[test]
     fn validate_accepts_alphanumeric_or_empty_sid() {
         for sid in ["Abc123", "", "aaaaaaaaaa"] {