Fix RUBY-1948 Default authentication options are set on URI and not on client (#1550)

* remove duplicitious uri_spec tests

* dont change uri spec

* get all client integration tests working with default client options

* cleaned up some code

* uncomment another test

* for now add back ssl_key default

* change some variable names

* make changes to client options to differentiate ssl_key and ssl_cert

* update documentation

* add comments back to uri

* tests failing because variable doesnt exist

* revert auth_source to nil on connection_spec

* fix tests by being explicit about authentication mechanism

* include auth mech when auth is set up

* use the right environment variables

* fix mmap tests

* only specify auth mech for versions that enable scram256

* whoops wrong env variable

* use connection to server to determine whether scram256 is supported

* make sure to close client

* fix connection spec

* make comments more specific

* committed something by mistake

* update the comment in client registry

* update comment again

* scram256 was introduced in 4.0

* respond to pr feedback

* comment

* App metadata needs to take server options into account

* Remove scram256 workaround

* Clarify why sometimes features are instantiated once and sometimes twice

Author: p-mongo

.evergreen/functions.sh

@@ -46,7 +46,7 @@ set_env_vars() {
   AUTH=${AUTH:-noauth}
   SSL=${SSL:-nossl}
   MONGODB_URI=${MONGODB_URI:-}
-  
+
   # drivers-evergreen-tools do not set tls parameter in URI when the
   # deployment uses TLS, repair this
   if test $SSL = ssl && ! echo $MONGODB_URI |grep -q tls=; then
@@ -58,20 +58,20 @@ set_env_vars() {
       MONGODB_URI="$MONGODB_URI/?tls=true"
     fi
   fi
-  
+
   TOPOLOGY=${TOPOLOGY:-server}
   DRIVERS_TOOLS=${DRIVERS_TOOLS:-}
 
   if [ "$AUTH" != "noauth" ]; then
     export ROOT_USER_NAME="bob"
     export ROOT_USER_PWD="pwd123"
   fi
-  
+
   export MONGODB_URI
   export COMPRESSOR
-  
+
   export CI=evergreen
-  
+
   # JRUBY_OPTS were initially set for Mongoid
   export JRUBY_OPTS="--server -J-Xms512m -J-Xmx2G"
 }
@@ -81,7 +81,7 @@ setup_ruby() {
     echo "Empty RVM_RUBY, aborting"
     exit 2
   fi
-  
+
   ls -l /opt
 
   # Necessary for jruby
@@ -90,7 +90,7 @@ setup_ruby() {
     export JAVACMD=/opt/java/jdk8/bin/java
     export PATH=$PATH:/opt/java/jdk8/bin
   fi
-    
+
   # ppc64le has it in a different place
   if test -z "$JAVACMD" && [ -f /usr/lib/jvm/java-1.8.0/bin/java ]; then
     export JAVACMD=/usr/lib/jvm/java-1.8.0/bin/java
@@ -104,22 +104,22 @@ setup_ruby() {
     export PATH=`pwd`/ruby-head/bin:`pwd`/ruby-head/lib/ruby/gems/2.6.0/bin:$PATH
     ruby --version
     ruby --version |grep dev
-    
+
     #rvm reinstall $RVM_RUBY
   else
     if true; then
-    
+
     # For testing toolchains:
     toolchain_url=https://s3.amazonaws.com//mciuploads/mongo-ruby-toolchain/`host_arch`/ce62fbb005213564a3da1041854da54df6615b2a/mongo_ruby_driver_toolchain_`host_arch |tr - _`_patch_ce62fbb005213564a3da1041854da54df6615b2a_5cfacdc857e85a3ef6647ad9_19_06_07_20_49_15.tar.gz
     curl -fL $toolchain_url |tar zxf -
     export PATH=`pwd`/rubies/$RVM_RUBY/bin:$PATH
-    
+
     # Attempt to get bundler to report all errors - so far unsuccessful
     #curl -o bundler-openssl.diff https://github.com/bundler/bundler/compare/v2.0.1...p-mongo:report-errors.diff
     #find . -path \*/lib/bundler/fetcher.rb -exec patch {} bundler-openssl.diff \;
-    
+
     else
-    
+
     # Normal operation
     if ! test -d $HOME/.rubies/$RVM_RUBY/bin; then
       echo "Ruby directory does not exist: $HOME/.rubies/$RVM_RUBY/bin" 1>&2
@@ -132,9 +132,9 @@ setup_ruby() {
       exit 2
     fi
     export PATH=$HOME/.rubies/$RVM_RUBY/bin:$PATH
-    
+
     fi
-    
+
     ruby --version
 
     # Ensure we're using the right ruby
@@ -178,7 +178,7 @@ kill_jruby() {
 prepare_server() {
   arch=$1
   version=$2
-  
+
   url=http://downloads.10gen.com/linux/mongodb-linux-x86_64-enterprise-$arch-$version.tgz
   mongodb_dir="$MONGO_ORCHESTRATION_HOME"/mdb
   mkdir -p "$mongodb_dir"

docs/tutorials/ruby-driver-authentication.txt

@@ -88,6 +88,13 @@ derived from the distinguished subject name of this certificate.
 This authentication method requires the use of SSL connections with
 certificate validation.
 
+To authenticate the client, you will need a valid SSL certificate
+and private encryption key. These can be stored in separate files,
+or together in one file (in the PEM format). Even if the certificate
+and private key are stored in the same file, you must specify the path to
+that file by passing both the ``ssl_cert`` and ``ssl_key`` options
+to the client.
+
 For more information about configuring X.509 authentication in MongoDB,
 see the :manual:`X.509 tutorial in the MongoDB Manual
 </tutorial/configure-x509/>`.
@@ -98,6 +105,7 @@ see the :manual:`X.509 tutorial in the MongoDB Manual
                              auth_mech: :mongodb_x509,
                              ssl: true,
                              ssl_cert: '/path/to/client.pem',
+                             ssl_key: '/path/to/client.pem',
                              ssl_ca_cert: '/path/to/ca.pem' )
 
 

lib/mongo/client.rb

@@ -398,18 +398,12 @@ def initialize(addresses_or_uri, options = nil)
         @srv_records = nil
       end
 
-      unless options[:retry_reads] == false
-        options[:retry_reads] = true
-      end
-      unless options[:retry_writes] == false
-        options[:retry_writes] = true
-      end
-
       # Special handling for sdam_proc as it is only used during client
       # construction
       sdam_proc = options.delete(:sdam_proc)
 
-      @options = validate_new_options!(Database::DEFAULT_OPTIONS.merge(options))
+      options = default_options(options).merge(options)
+      @options = validate_new_options!(options)
 =begin WriteConcern object support
       if @options[:write_concern].is_a?(WriteConcern::Base)
         # Cache the instance so that we do not needlessly reconstruct it.
@@ -811,6 +805,35 @@ def watch(pipeline = [], options = {})
 
     private
 
+    # Generate default client options based on the URI and options
+    # passed into the Client constructor.
+    def default_options(options)
+      Database::DEFAULT_OPTIONS.dup.tap do |default_options|
+        if options[:auth_mech] || options[:user]
+          default_options[:auth_source] = default_auth_source(options)
+        end
+
+        if options[:auth_mech] == :gssapi
+          default_options[:auth_mech_properties] = { service_name: 'mongodb' }
+        end
+
+        default_options[:retry_reads] = true
+        default_options[:retry_writes] = true
+      end
+    end
+
+    # Generate default auth source based on the URI and options
+    def default_auth_source(options)
+      case options[:auth_mech]
+      when :gssapi, :mongodb_x509
+        '$external'
+      when :plain
+        options[:database] || '$external'
+      else
+        options[:database] || Database::ADMIN
+      end
+    end
+
     # If options[:session] is set, validates that session and returns it.
     # If deployment supports sessions, creates a new session and returns it.
     # The session is implicit unless options[:implicit] is given.

lib/mongo/uri.rb

@@ -241,7 +241,10 @@ def self.get(string, opts = {})
     #
     # @since 2.0.0
     def client_options
-      opts = default_client_options.merge(uri_options)
+      opts = uri_options.tap do |opts|
+        opts[:database] = @database if @database
+      end
+
       @user ? opts.merge(credentials) : opts
     end
 
@@ -442,35 +445,6 @@ def parse_database!(string)
       decode(string) if string.length > 0
     end
 
-    def default_client_options
-      opts = Options::Redacted.new(database: database)
-
-      if @uri_options[:auth_mech] || @user
-        opts[:auth_source] = default_auth_source
-      end
-
-      if @uri_options[:auth_mech] == :gssapi
-        opts[:auth_mech_properties] = default_auth_mech_properties
-      end
-
-      opts
-    end
-
-    def default_auth_mech_properties
-      { service_name: 'mongodb' }
-    end
-
-    def default_auth_source
-      case @uri_options[:auth_mech]
-      when :gssapi, :mongodb_x509
-        '$external'
-      when :plain
-        @database || '$external'
-      else
-        @database || Database::ADMIN
-      end
-    end
-
     def raise_invalid_error!(details)
       raise Error::InvalidURI.new(@string, details, FORMAT)
     end

spec/integration/client_options_spec.rb

@@ -49,20 +49,19 @@
         end
       end
 
-      # TODO: get this test passing
-      # context 'with client options' do
-      #   let(:client_opts) do
-      #     {
-      #       auth_mech: auth_mech_sym,
-      #       user: user,
-      #       password: pwd,
-      #     }
-      #   end
-
-      #   it 'creates a client with default auth source' do
-      #     expect(client.options['auth_source']).to eq(default_auth_source)
-      #   end
-      # end
+      context 'with client options' do
+        let(:client_opts) do
+          {
+            auth_mech: auth_mech_sym,
+            user: user,
+            password: pwd,
+          }
+        end
+
+        it 'creates a client with default auth source' do
+          expect(client.options['auth_source']).to eq(default_auth_source)
+        end
+      end
     end
 
     context 'where database is provided' do
@@ -77,21 +76,20 @@
         end
       end
 
-      # TODO: get this test passing
-      # context 'with client options' do
-      #   let(:client_opts) do
-      #     {
-      #       auth_mech: auth_mech_sym,
-      #       user: user,
-      #       password: pwd,
-      #       database: database
-      #     }
-      #   end
-
-      #   it 'creates a client with database as auth source' do
-      #     expect(client.options['auth_source']).to eq(database)
-      #   end
-      # end
+      context 'with client options' do
+        let(:client_opts) do
+          {
+            auth_mech: auth_mech_sym,
+            user: user,
+            password: pwd,
+            database: database
+          }
+        end
+
+        it 'creates a client with database as auth source' do
+          expect(client.options['auth_source']).to eq(database)
+        end
+      end
     end
   end
 
@@ -117,6 +115,7 @@
           auth_mech: auth_mech_sym,
           ssl: true,
           ssl_cert: cert_path,
+          ssl_key: cert_path,
           ssl_ca_cert: ca_file_path,
           user: user,
           password: pwd
@@ -127,8 +126,7 @@
         expect(client.options[:ssl]).to be true
         expect(client.options[:ssl_cert]).to eq(cert_path)
         expect(client.options[:ssl_ca_cert]).to eq(ca_file_path)
-        # TODO: get this expectation passing
-        # expect(client.options[:ssl_key]).to eq(cert_path)
+        expect(client.options[:ssl_key]).to eq(cert_path)
       end
     end
   end
@@ -245,20 +243,19 @@
       end
     end
 
-    # TODO: get this test passing
-    # context 'with client options' do
-    #   let(:client_opts) do
-    #     {
-    #       auth_mech: :gssapi,
-    #       user: user,
-    #       password: pwd
-    #     }
-    #   end
-
-    #   it 'sets default auth mech properties' do
-    #     expect(client.options[:auth_mech_properties]).to eq({ 'service_name' => 'mongodb' })
-    #   end
-    # end
+    context 'with client options' do
+      let(:client_opts) do
+        {
+          auth_mech: :gssapi,
+          user: user,
+          password: pwd
+        }
+      end
+
+      it 'sets default auth mech properties' do
+        expect(client.options[:auth_mech_properties]).to eq({ 'service_name' => 'mongodb' })
+      end
+    end
   end
 
   context 'with PLAIN auth mechanism' do
@@ -312,10 +309,9 @@
     context 'with client options' do
       let(:client_opts) { { auth_mech: :mongodb_x509, user: user } }
 
-      # TODO: get this test passing
-      # it 'sets default auth source' do
-      #   expect(client.options[:auth_source]).to eq('$external')
-      # end
+      it 'sets default auth source' do
+        expect(client.options[:auth_source]).to eq('$external')
+      end
 
       context 'when username is not provided' do
         let(:client_opts) { { auth_mech: :mongodb_x509} }

spec/integration/connection_pool_populator_spec.rb

@@ -21,6 +21,10 @@
 
   declare_topology_double
 
+  let(:app_metadata) do
+    Mongo::Server::AppMetadata.new(options)
+  end
+
   let(:cluster) do
     double('cluster').tap do |cl|
       allow(cl).to receive(:topology).and_return(topology)

spec/integration/connection_spec.rb

@@ -158,8 +158,10 @@
         RSpec::Mocks.with_temporary_scope do
           # now pretend an ismaster returned a different range
           features = Mongo::Server::Description::Features.new(0..3)
-          # the second Features instantiation is for SDAM event publication
-          expect(Mongo::Server::Description::Features).to receive(:new).twice.and_return(features)
+          # One Features instantiation is for SDAM event publication, this
+          # one always happens. The second one happens on servers
+          # where we do not negotiate auth mechanism.
+          expect(Mongo::Server::Description::Features).to receive(:new).at_least(:once).and_return(features)
 
           connection = Mongo::Server::Connection.new(server, server.options)
           expect(connection.connect!).to be true

spec/mongo/server/connection_pool_spec.rb

@@ -24,6 +24,10 @@
 
   declare_topology_double
 
+  let(:app_metadata) do
+    Mongo::Server::AppMetadata.new(server_options)
+  end
+
   let(:cluster) do
     double('cluster').tap do |cl|
       allow(cl).to receive(:topology).and_return(topology)