Merge branch '2982-semantics-of-postgres_user-with-password-nil-are-unclear' into 'master'

Resolve "Semantics of `postgres_user` with unstated passwords password are unclear"

Closes #2982

See merge request gitlab-org/omnibus-gitlab!2123

files/gitlab-cookbooks/gitlab/libraries/helpers/base_pg_helper.rb

@@ -35,6 +35,27 @@ class BasePgHelper
       "|grep -x #{db_user}"])
   end
 
+  def user_options(db_user)
+    query = "SELECT usecreatedb, usesuper, userepl, usebypassrls FROM pg_shadow WHERE usename='#{db_user}'"
+    values = do_shell_out(
+      %(/opt/gitlab/bin/#{service_cmd} -d template1 -c "#{query}" -tA)
+    ).stdout.chomp.split('|').map { |v| v == 't' }
+    options = %w(CREATEDB SUPERUSER REPLICATION BYPASSRLS)
+    Hash[options.zip(values)]
+  end
+
+  def user_options_set?(db_user, options)
+    active_options = user_options(db_user)
+    options.map(&:upcase).each do |option|
+      if option =~ /^NO(.*)/
+        return false if active_options[$1]
+      else
+        return false if !active_options[option]
+      end
+   end
+   true
+  end
+
   def user_hashed_password(db_user)
     db_user_safe = db_user.scan(/[a-z_][a-z0-9_-]*[$]?/).first
     do_shell_out(

files/gitlab-cookbooks/gitlab/resources/postgresql_user.rb

@@ -2,13 +2,13 @@ resource_name :postgresql_user
 
 property :username, String, name_property: true
 property :password, String
-property :options, Array, default: []
+property :options, Array
 property :helper, default: PgHelper.new(node)
 
 action :create do
   account_helper = AccountHelper.new(node)
 
-  query = %(CREATE USER \\"#{username}\\" #{options.join(' ')})
+  query = %(CREATE USER \\"#{username}\\")
 
   execute "create #{username} postgresql user" do
     command %(/opt/gitlab/bin/#{helper.service_cmd} -d template1 -c "#{query}")
@@ -18,14 +18,32 @@ action :create do
     not_if { !helper.is_running? || helper.user_exists?(username) }
   end
 
-  query = %(ALTER USER \\"#{username}\\" #{options.join(' ')})
-  query << %( WITH PASSWORD '#{password}') unless password.nil?
+  if property_is_set?(:password)
+    query = %(ALTER USER \\"#{username}\\")
+    query << if password.nil?
+               %( WITH PASSWORD NULL )
+             else
+               %( WITH PASSWORD '#{password}')
+             end
 
-  execute "set password for #{username} postgresql user" do
-    command %(/opt/gitlab/bin/#{helper.service_cmd} -d template1 -c "#{query}")
-    user account_helper.postgresql_user
-    # Added retries to give the service time to start on slower systems
-    retries 20
-    not_if { !helper.is_running? || !helper.user_exists?(username) || helper.user_password_match?(username, password) }
+    execute "set password for #{username} postgresql user" do
+      command %(/opt/gitlab/bin/#{helper.service_cmd} -d template1 -c "#{query}")
+      user account_helper.postgresql_user
+      # Added retries to give the service time to start on slower systems
+      retries 20
+      not_if { !helper.is_running? || !helper.user_exists?(username) || helper.user_password_match?(username, password) }
+    end
+  end
+
+  if property_is_set?(:options)
+    query = %(ALTER USER \\"#{username}\\" #{options.join(' ')})
+
+    execute "set options for #{username} postgresql user" do
+      command %(/opt/gitlab/bin/#{helper.service_cmd} -d template1 -c "#{query}")
+      user account_helper.postgresql_user
+      # Added retries to give the service time to start on slower systems
+      retries 20
+      not_if { !helper.is_running? || !helper.user_exists?(username) || helper.user_options_set?(username, options) }
+    end
   end
 end

spec/chef/resources/postgresql_user_spec.rb

@@ -0,0 +1,77 @@
+require 'chef_helper'
+
+describe 'postgresql_user' do
+  before do
+    allow_any_instance_of(PgHelper).to receive(:is_running?).and_return(true)
+    allow_any_instance_of(PgHelper).to receive(:user_exists?).and_return(false, true)
+    allow_any_instance_of(PgHelper).to receive(:user_password_match?).and_return(false)
+    allow_any_instance_of(PgHelper).to receive(:user_options_set?).and_return(false)
+  end
+
+  let(:runner) { ChefSpec::SoloRunner.new(step_into: ['postgresql_user']) }
+
+  context 'create' do
+    let(:chef_run) { runner.converge('test_gitlab_postgresql_user::create') }
+
+    it 'creates a user' do
+      expect(chef_run).to run_execute('create example postgresql user')
+    end
+  end
+
+  context 'password' do
+    context 'not specified' do
+      let(:chef_run) { runner.converge('test_gitlab_postgresql_user::password_unspecified') }
+
+      it 'does not set the password of the no_password user' do
+        expect(chef_run).not_to run_execute('set password for no_password postgresql user')
+      end
+    end
+
+    context 'nil' do
+      let(:chef_run) { runner.converge('test_gitlab_postgresql_user::password_nil') }
+
+      it 'does set the password of the nil_password user' do
+        pending 'upgrade to chef 13'
+        expect(chef_run).to run_execute('set password for nil_password postgresql user')
+          .with(command: /PASSWORD NULL/)
+      end
+    end
+
+    context 'md5' do
+      let(:chef_run) { runner.converge('test_gitlab_postgresql_user::password_md5') }
+
+      it 'does set the password of the md5_password user' do
+        expect(chef_run).to run_execute('set password for md5_password postgresql user')
+          .with(command: /PASSWORD 'e99b79fbdf9b997e6918df2385e60f5c'/)
+      end
+    end
+
+    context 'empty' do
+      let(:chef_run) { runner.converge('test_gitlab_postgresql_user::password_empty') }
+
+      it 'does set the password of the empty_password user' do
+        expect(chef_run).to run_execute('set password for empty_password postgresql user')
+          .with(command: /PASSWORD ''/)
+      end
+    end
+  end
+
+  context 'options' do
+    context 'unspecified' do
+      let(:chef_run) { runner.converge('test_gitlab_postgresql_user::options_unspecified') }
+
+      it 'does not set options' do
+        expect(chef_run).not_to run_execute('set options for example postgresql user')
+      end
+    end
+
+    context 'SUPERUSER' do
+      let(:chef_run) { runner.converge('test_gitlab_postgresql_user::options_superuser') }
+
+      it 'does set SUPERUSER' do
+        expect(chef_run).to run_execute('set options for example postgresql user')
+          .with(command: /\bSUPERUSER\b/)
+      end
+    end
+  end
+end

spec/chef_helper.rb

@@ -35,7 +35,7 @@ RSpec.configure do |config|
   config.platform = platform
   config.version = version
 
-  config.cookbook_path = ['files/gitlab-cookbooks/']
+  config.cookbook_path = ['files/gitlab-cookbooks/', 'spec/fixtures/cookbooks']
   config.log_level = :error
 
   config.filter_run focus: true

spec/fixtures/cookbooks/test_gitlab_postgresql_user/metadata.rb

@@ -0,0 +1,2 @@
+name 'test_gitlab_postgresql_user'
+depends 'gitlab'

spec/fixtures/cookbooks/test_gitlab_postgresql_user/recipes/create.rb

@@ -0,0 +1,2 @@
+postgresql_user 'example' do
+end

spec/fixtures/cookbooks/test_gitlab_postgresql_user/recipes/options_superuser.rb

@@ -0,0 +1,3 @@
+postgresql_user 'example' do
+  options ['SUPERUSER']
+end

spec/fixtures/cookbooks/test_gitlab_postgresql_user/recipes/options_unspecified.rb

@@ -0,0 +1,2 @@
+postgresql_user 'example' do
+end

spec/fixtures/cookbooks/test_gitlab_postgresql_user/recipes/password_empty.rb

@@ -0,0 +1,3 @@
+postgresql_user 'empty_password' do
+  password ''
+end

spec/fixtures/cookbooks/test_gitlab_postgresql_user/recipes/password_md5.rb

@@ -0,0 +1,5 @@
+postgresql_user 'md5_password' do
+  # This is not a secret password:
+  #   ruby -rdigest -e 'puts Digest::MD5.hexdigest("toomanysecrets" + "md5_password")'
+  password 'e99b79fbdf9b997e6918df2385e60f5c'
+end

spec/fixtures/cookbooks/test_gitlab_postgresql_user/recipes/password_nil.rb

@@ -0,0 +1,3 @@
+postgresql_user 'nil_password' do
+  password nil
+end

spec/fixtures/cookbooks/test_gitlab_postgresql_user/recipes/password_unspecified.rb

@@ -0,0 +1,2 @@
+postgresql_user 'no_password' do
+end

spec/libraries/helpers/base_pg_helper_spec.rb

@@ -10,6 +10,64 @@ describe BasePgHelper do
     allow(subject).to receive(:service_cmd) { 'gitlab-psql' }
   end
 
+  describe '#user_options' do
+    before do
+      result = spy('shellout')
+      allow(result).to receive(:stdout).and_return("f|f|t|f\n")
+      allow(subject).to receive(:do_shell_out).and_return(result)
+    end
+
+    it 'returns hash from query' do
+      expect(subject.user_options('')).to eq(
+        {
+          'SUPERUSER' => false,
+          'CREATEDB' => false,
+          'REPLICATION' => true,
+          'BYPASSRLS' => false
+        }
+      )
+    end
+  end
+
+  describe '#user_options_set?' do
+    let(:default_options) do
+      {
+        'SUPERUSER' => false,
+        'CREATEDB' => false,
+        'REPLICATION' => true,
+        'BYPASSRLS' => false
+      }
+    end
+
+    context 'default user options' do
+      before do
+        allow(subject).to receive(:user_options).and_return(default_options)
+      end
+
+      it 'returns true when no options are asked about' do
+        expect(subject.user_options_set?('', [])).to be_truthy
+      end
+
+      it 'returns true when options are set to their defaults' do
+        expect(subject.user_options_set?('', ['NOSUPERUSER'])).to be_truthy
+      end
+
+      it 'returns false when options are set away from their defaults' do
+        expect(subject.user_options_set?('', ['SUPERUSER'])).to be_falsey
+      end
+    end
+
+    context 'modified user' do
+      before do
+        allow(subject).to receive(:user_options).and_return(default_options.merge({ 'SUPERUSER' => true }))
+      end
+
+      it 'returns false when options is not what we expect' do
+        expect(subject.user_options_set?('', ['NOSUPERUSER'])).to be_falsey
+      end
+    end
+  end
+
   describe '#user_password_match?' do
     before do
       # user: gitlab pass: test123
@@ -31,5 +89,20 @@ describe BasePgHelper do
     it 'returns false when wrong password is in MD5 format' do
       expect(subject.user_password_match?('gitlab', 'md5b599de4332636c03a60fca13be1edb5f')).to be_falsey
     end
+
+    it 'returns false when password is not supplied' do
+      expect(subject.user_password_match?('gitlab', nil)).to be_falsey
+    end
+
+    context 'nil password' do
+      before do
+        # user: gitlab pass: unset
+        allow(subject).to receive(:user_hashed_password) { '' }
+      end
+
+      it 'returns true when the password is nil' do
+        expect(subject.user_password_match?('gitlab', nil)).to be_truthy
+      end
+    end
   end
 end

spec/libraries/roles_spec.rb

@@ -22,11 +22,11 @@ describe 'GitLabRoles' do
     end
 
     it 'supports providing a single role as a string' do
-      stub_gitlab_rb(roles: 'geo_secondary_role')
+      stub_gitlab_rb(roles: 'application_role')
 
       Gitlab.load_roles
 
-      expect(Gitlab['geo_secondary_role']['enable']).to be true
+      expect(Gitlab['application_role']['enable']).to be true
     end
 
     it 'handles users specifying hyphens instead of underscores' do