RUST-1954 Disallow commas in authMechanismProperties values (#1315)

Author: isabelatkinson

src/client/auth.rs

@@ -401,6 +401,10 @@ pub struct Credential {
     pub mechanism: Option<AuthMechanism>,
 
     /// Additional properties for the given mechanism.
+    ///
+    /// If any value in the properties contains a comma, this field must be set directly on
+    /// [`ClientOptions`](crate::options::ClientOptions) and cannot be parsed from a connection
+    /// string.
     pub mechanism_properties: Option<Document>,
 
     /// The token callback for OIDC authentication.

src/client/options.rs

@@ -1345,17 +1345,6 @@ fn split_once_right<'a>(s: &'a str, delimiter: &str) -> (Option<&'a str>, &'a st
     }
 }
 
-/// Splits a string into a section before a given index and a section exclusively after the index.
-/// Empty portions are returned as `None`.
-fn exclusive_split_at(s: &str, i: usize) -> (Option<&str>, Option<&str>) {
-    let (l, r) = s.split_at(i);
-
-    let lout = if !l.is_empty() { Some(l) } else { None };
-    let rout = if r.len() > 1 { Some(&r[1..]) } else { None };
-
-    (lout, rout)
-}
-
 fn percent_decode(s: &str, err_message: &str) -> Result<String> {
     match percent_encoding::percent_decode_str(s).decode_utf8() {
         Ok(result) => Ok(result.to_string()),
@@ -1817,47 +1806,26 @@ impl ConnectionString {
             }
             "authsource" => parts.auth_source = Some(value.to_string()),
             "authmechanismproperties" => {
-                let mut doc = Document::new();
-                let err_func = || {
-                    ErrorKind::InvalidArgument {
-                        message: "improperly formatted authMechanismProperties".to_string(),
-                    }
-                    .into()
-                };
+                let mut properties = Document::new();
 
-                for kvp in value.split(',') {
-                    match kvp.find(':') {
-                        Some(index) => {
-                            let (k, v) = exclusive_split_at(kvp, index);
-                            let key = k.ok_or_else(err_func)?;
-                            match key {
-                                "ALLOWED_HOSTS" => {
-                                    return Err(Error::invalid_argument(
-                                        "ALLOWED_HOSTS must only be specified through client \
-                                         options",
-                                    ));
-                                }
-                                "OIDC_CALLBACK" => {
-                                    return Err(Error::invalid_argument(
-                                        "OIDC_CALLBACK must only be specified through client \
-                                         options",
-                                    ));
-                                }
-                                "OIDC_HUMAN_CALLBACK" => {
-                                    return Err(Error::invalid_argument(
-                                        "OIDC_HUMAN_CALLBACK must only be specified through \
-                                         client options",
-                                    ));
-                                }
-                                _ => {}
-                            }
-                            let value = v.ok_or_else(err_func)?;
-                            doc.insert(key, value);
-                        }
-                        None => return Err(err_func()),
+                for property in value.split(",") {
+                    let Some((k, v)) = property.split_once(":") else {
+                        return Err(Error::invalid_argument(format!(
+                            "each entry in authMechanismProperties must be a colon-separated \
+                             key-value pair, got {}",
+                            property
+                        )));
                     };
+                    if k == "ALLOWED_HOSTS" || k == "OIDC_CALLBACK" || k == "OIDC_HUMAN_CALLBACK" {
+                        return Err(Error::invalid_argument(format!(
+                            "{} must only be specified through client options",
+                            k
+                        )));
+                    }
+                    properties.insert(k, v);
                 }
-                parts.auth_mechanism_properties = Some(doc);
+
+                parts.auth_mechanism_properties = Some(properties);
             }
             #[cfg(any(
                 feature = "zstd-compression",

src/client/options/test.rs

@@ -22,9 +22,6 @@ static SKIPPED_TESTS: Lazy<Vec<&'static str>> = Lazy::new(|| {
         "maxPoolSize=0 does not error",
         #[cfg(not(feature = "cert-key-password"))]
         "Valid tlsCertificateKeyFilePassword is parsed correctly",
-        // TODO RUST-1954: unskip these tests
-        "Colon in a key value pair",
-        "Comma in a key value pair causes a warning",
     ];
 
     // TODO RUST-1896: unskip this test when openssl-tls is enabled

src/test/spec/auth.rs

@@ -106,6 +106,8 @@ async fn run_auth_test(test_file: TestFile) {
 }
 
 #[tokio::test]
-async fn run() {
-    run_spec_test(&["auth"], run_auth_test).await;
+async fn run_legacy() {
+    run_spec_test(&["auth", "legacy"], run_auth_test).await;
 }
+
+// TODO RUST-1665: run unified tests

src/test/spec/json/auth/README.md

@@ -0,0 +1,47 @@
+# Auth Tests
+
+## Introduction
+
+This document describes the format of the driver spec tests included in the JSON and YAML files included in the `legacy`
+sub-directory. Tests in the `unified` directory are written using the
+[Unified Test Format](../../unified-test-format/unified-test-format.md).
+
+The YAML and JSON files in the `legacy` directory tree are platform-independent tests that drivers can use to prove
+their conformance to the Auth Spec at least with respect to connection string URI input.
+
+Drivers should do additional unit testing if there are alternate ways of configuring credentials on a client.
+
+Driver must also conduct the prose tests in the Auth Spec test plan section.
+
+## Format
+
+Each YAML file contains an object with a single `tests` key. This key is an array of test case objects, each of which
+have the following keys:
+
+- `description`: A string describing the test.
+- `uri`: A string containing the URI to be parsed.
+- `valid:` A boolean indicating if the URI should be considered valid.
+- `credential`: If null, the credential must not be considered configured for the the purpose of deciding if the driver
+    should authenticate to the topology. If non-null, it is an object containing one or more of the following properties
+    of a credential:
+    - `username`: A string containing the username. For auth mechanisms that do not utilize a password, this may be the
+        entire `userinfo` token from the connection string.
+    - `password`: A string containing the password.
+    - `source`: A string containing the authentication database.
+    - `mechanism`: A string containing the authentication mechanism. A null value for this key is used to indicate that a
+        mechanism wasn't specified and that mechanism negotiation is required. Test harnesses should modify the mechanism
+        test as needed to assert this condition.
+    - `mechanism_properties`: A document containing mechanism-specific properties. It specifies a subset of properties
+        that must match. If a key exists in the test data, it must exist with the corresponding value in the credential.
+        Other values may exist in the credential without failing the test.
+
+If any key is missing, no assertion about that key is necessary. Except as specified explicitly above, if a key is
+present, but the test value is null, the observed value for that key must be uninitialized (whatever that means for a
+given driver and data type).
+
+## Implementation notes
+
+Testing whether a URI is valid or not should simply be a matter of checking whether URI parsing (or MongoClient
+construction) raises an error or exception.
+
+If a credential is configured, its properties must be compared to the `credential` field.

src/test/spec/json/auth/README.rst

@@ -648,4 +648,4 @@
       "credential": null
     }
   ]
-}
+}

src/test/spec/json/auth/connection-string.json renamed to src/test/spec/json/auth/legacy/connection-string.json

@@ -468,4 +468,4 @@ tests:
     (MONGODB-OIDC)
   uri: mongodb://user:pass@localhost/?authMechanism=MONGODB-OIDC&authMechanismProperties=ENVIRONMENT:k8s
   valid: false
-  credential: null
+  credential: null