Adhere to specification for default mechanism selection

xp-forge · Aug 18, 2023 · 48370fb · 48370fb
1 parent 88d768d
commit 48370fb
Show file tree

Hide file tree

Showing 3 changed files with 22 additions and 31 deletions.
diff --git a/src/main/php/com/mongodb/Authentication.class.php b/src/main/php/com/mongodb/Authentication.class.php
@@ -5,7 +5,7 @@
 
 /** @test com.mongodb.unittest.AuthenticationTest */
 abstract class Authentication {
-  const MECHANISMS= ['SCRAM-SHA-1', 'SCRAM-SHA-256'];
+  const MECHANISMS= ['SCRAM-SHA-256', 'SCRAM-SHA-1'];
 
   /**
    * Returns an authentication by a given mechanism name
@@ -24,10 +24,18 @@ public static function mechanism(string $name): Mechanism {
    * Negotiates one of the supported authentication mechansim from a list
    * of given mechanisms.
    *
+   * If SCRAM-SHA-256 is present in the list of mechanism, then it MUST be used as
+   * the default; otherwise, SCRAM-SHA-1 MUST be used as the default, regardless of
+   * whether SCRAM-SHA-1 is in the list. If saslSupportedMechs is not present in the
+   * handshake response for mechanism negotiation, then SCRAM-SHA-1 MUST be used
+   *
+   * @see    https://github.com/mongodb/specifications/blob/master/source/auth/auth.rst#defaults
    * @throws lang.IllegalArgumentException if none are supported
    */
   public static function negotiate(array $mechanisms): Mechanism {
-    if ($supported= array_intersect($mechanisms, self::MECHANISMS)) {
+    if (empty($mechanisms)) {
+      return self::mechanism('SCRAM-SHA-1');
+    } else if ($supported= array_intersect(self::MECHANISMS, $mechanisms)) {
       return self::mechanism(current($supported));
     }
 

diff --git a/src/main/php/com/mongodb/io/Connection.class.php b/src/main/php/com/mongodb/io/Connection.class.php
@@ -150,14 +150,7 @@ public function establish($options= [], $auth= null) {
     if (null === $authSource) return;
 
     try {
-      if ($auth) {
-        // Use this explicitely specified mechanism
-      } else if ($supported= $document['saslSupportedMechs'] ?? null) {
-        $auth= Authentication::negotiate($supported);
-      } else {
-        $auth= Authentication::mechanism(Authentication::MECHANISMS[0]);
-      }
-
+      $auth ?? $auth= Authentication::negotiate($document['saslSupportedMechs'] ?? []);
       $conversation= $auth->conversation($user, $pass, $authSource);
       do {
         $result= $this->message($conversation->current(), null);

diff --git a/src/test/php/com/mongodb/unittest/AuthenticationTest.class.php b/src/test/php/com/mongodb/unittest/AuthenticationTest.class.php
@@ -3,15 +3,10 @@
 use com\mongodb\Authentication;
 use com\mongodb\auth\Mechanism;
 use lang\IllegalArgumentException;
-use test\{Assert, Expect, Test};
+use test\{Assert, Expect, Test, Values};
 
 class AuthenticationTest {
 
-  #[Test]
-  public function sha1_is_default_mechanism() {
-    Assert::equals('SCRAM-SHA-1', Authentication::MECHANISMS[0]);
-  }
-
   #[Test]
   public function scram_sha_1() {
     Assert::instance(Mechanism::class, Authentication::mechanism('SCRAM-SHA-1'));
@@ -27,33 +22,28 @@ public function unknown() {
     Authentication::mechanism('unknown');
   }
 
-  #[Test]
-  public function negotiate_sha1() {
-    Assert::equals('SCRAM-SHA-1', Authentication::negotiate(['SCRAM-SHA-1'])->name());
+  #[Test, Values([[['SCRAM-SHA-1']], [['SCRAM-SHA-256']]])]
+  public function negotiate_only_option($supplied) {
+    Assert::equals($supplied[0], Authentication::negotiate($supplied)->name());
   }
 
-  #[Test]
-  public function negotiate_sha1_preferred() {
-    Assert::equals('SCRAM-SHA-1', Authentication::negotiate(['SCRAM-SHA-1', 'SCRAM-SHA-256'])->name());
+  #[Test, Values([[['SCRAM-SHA-256', 'SCRAM-SHA-1']], [['SCRAM-SHA-1', 'SCRAM-SHA-256']]])]
+  public function negotiate_sha256_is_default_if_contained($supplied) {
+    Assert::equals('SCRAM-SHA-256', Authentication::negotiate($supplied)->name());
   }
 
   #[Test]
-  public function negotiate_sha256_preferred() {
-    Assert::equals('SCRAM-SHA-256', Authentication::negotiate(['SCRAM-SHA-256', 'SCRAM-SHA-1'])->name());
+  public function negotiate_with_unsupported_plain() {
+    Assert::equals('SCRAM-SHA-1', Authentication::negotiate(['PLAIN', 'SCRAM-SHA-1'])->name());
   }
 
   #[Test]
-  public function negotiate_unsupported_plain_preferred() {
-    Assert::equals('SCRAM-SHA-1', Authentication::negotiate(['PLAIN', 'SCRAM-SHA-1'])->name());
+  public function negotiate_returns_sha1_if_empty() {
+    Assert::equals('SCRAM-SHA-1', Authentication::negotiate([])->name());
   }
 
   #[Test, Expect(IllegalArgumentException::class)]
   public function negotiate_none_supported() {
     Authentication::negotiate(['PLAIN', 'AWS'])->name();
   }
-
-  #[Test, Expect(IllegalArgumentException::class)]
-  public function negotiate_empty() {
-    Authentication::negotiate([])->name();
-  }
 }