Skip password hash migration on read-only databases

What does this MR do and why?

On a Geo secondary, the first Git over HTTP request that authenticates a user whose stored password hash is due for migration answers 500. EncryptedUserPassword#valid_password? verifies the password and then calls migrate_password!, which rewrites the hash with update_column whenever its algorithm, BCrypt cost or PBKDF2 salt length differs from the configured one. The secondary's database is a read-only replica, so that UPDATE raises PG::ReadOnlySqlTransaction, which is the log in the issue (Repositories::GitHttpController#info_refs).

This MR makes migrate_password! return true without the UPDATE when Gitlab::Database.read_only? is true, right after the existing migrated_password? check. The password has been verified by then, so the sign-in succeeds with the stored hash as it is, and the hash is migrated the next time the user signs in where the database is writable. It is the guard the other sign-in writes in User already carry: update_tracked_fields!, increment_failed_attempts! and consume_otp!, whose return true if Gitlab::Database.read_only? has the same shape, plus reset_failed_attempts! in EE::User since !251370 (merged). invalidate_otp_backup_code! reads the same predicate but refuses the code instead, since a backup code must not be reusable.

I used Gitlab::Database.read_only? rather than ::Gitlab::Geo.secondary_or_org_migration_target?, which the issue's implementation plan suggests:

• Gitlab::Geo exists only in EE (ee/lib/gitlab/geo.rb), while this concern and its spec are CE code. The Geo development guide asks for this predicate: "When some write actions are not allowed because the site is a secondary, consider adding the Gitlab::Database.read_only? or Gitlab::Database.read_write? guard, instead of Gitlab::Geo.secondary?." Gitlab::Database.read_only? is defined in CE (always false) and overridden in EE as Gitlab::Geo.secondary? || Gitlab.maintenance_mode?, and it is what the sibling guards read. The maintainer review on !251370 (merged) suggested this same predicate, for the same consistency.
• On an org migration target nothing changes: GeoNode#secondary? excludes it, so read_only? is false there and migrate_password! behaves as it does today, as every sibling write above does. I have not checked whether a target cell's database refuses writes. I would rather the sign-in writes on such a cell be decided together than have this one write behave differently. If you prefer the issue's predicate here, I can switch to it with an EE override.

read_only? is also true in maintenance mode on a primary, where the UPDATE would succeed (Maintenance Mode internals: "The database itself is not in read-only mode (except in a Geo secondary site)"). The migration is skipped there too, as the sibling writes already are. It is housekeeping after a successful password check, so skipping it never blocks a sign-in, and the hash is migrated on the first sign-in after maintenance mode ends.

References

• Closes #543223 (closed)
• !251370 (merged) added the same guard to reset_failed_attempts!, and its review discussed read_only? against a Geo-only check and the maintenance mode case.
• The sibling guards on master at a20857c3: app/models/user.rb lines 153 (update_tracked_fields!), 1428 (invalidate_otp_backup_code!), 2643 (increment_failed_attempts!) and 3084 (consume_otp!), and ee/app/models/ee/user.rb line 929 (reset_failed_attempts!).

Screenshots or screen recordings

No UI change. The new examples in spec/models/concerns/encrypted_user_password_spec.rb stub Gitlab::Database.read_only? and cover the four hash migrations the concern performs: BCrypt cost 4 to the configured cost, PBKDF2 to BCrypt, and in FIPS mode BCrypt to PBKDF2 and a 16 character PBKDF2 salt to 64 characters.

Run	master at a20857c3	This branch
spec/models/concerns/encrypted_user_password_spec.rb, 24 examples	4 failures, the 4 new examples	0 failures
Reproduction inside a read-only transaction, 2 examples, not part of this MR	2 failures, PG::ReadOnlySqlTransaction	0 failures

Failure of a new example on master

The other three fail the same way.

1) User#valid_password? when the database is read-only when the user password is BCrypt with work factor 4 behaves like accepts the password without migrating its hash returns true and does not write a new hash
   Failure/Error: update_column(:encrypted_password, hash_this_password(password))

     (#<User id:12 @user20>).update_column(:encrypted_password, "$2a$05$kxuRKaGy.cUZE/.sQVhEZe9NKFS5qpwlZaZbmP7xiy46nGZ4KVdCu")
         expected: 0 times with any arguments
         received: 1 time with arguments: (:encrypted_password, "$2a$05$kxuRKaGy.cUZE/.sQVhEZe9NKFS5qpwlZaZbmP7xiy46nGZ4KVdCu")
   # ./app/models/concerns/encrypted_user_password.rb:117:in `migrate_password!'
   # ./app/models/concerns/encrypted_user_password.rb:32:in `valid_password?'
   # ./app/models/user.rb:1408:in `valid_password?'

Reproduction inside a read-only transaction, not part of this MR

To see the error from the issue rather than a stubbed expectation, I ran valid_password? inside a savepoint that PostgreSQL holds read-only, once with read_only? stubbed and once with the current node stubbed as a Geo secondary so the EE predicate answers by itself:

RSpec.describe User, feature_category: :system_access do
  include ::EE::GeoHelpers

  let(:encrypted_password) { '$2a$04$ThzqXSFnlW3uH86uQ79puOU7vARSFuuNzb1nUGfsBeYtCLkdymAQW' }
  let!(:user) { create(:user, encrypted_password: encrypted_password) }

  def valid_password_in_read_only_transaction
    result = nil

    User.transaction(requires_new: true) do
      User.connection.execute('SET TRANSACTION READ ONLY')
      result = user.valid_password?('security')
      raise ActiveRecord::Rollback
    end

    result
  end

  it 'accepts the password when read_only? is true and the database refuses writes' do
    allow(Gitlab::Database).to receive(:read_only?).and_return(true)

    expect(valid_password_in_read_only_transaction).to be(true)
    expect(user.reload.encrypted_password).to eq(encrypted_password)
  end

  context 'on a Geo secondary', :geo do
    let_it_be(:primary) { create(:geo_node, :primary) }
    let_it_be(:secondary) { create(:geo_node) }

    before do
      stub_current_geo_node(secondary)
    end

    it 'accepts the password when the database refuses writes' do
      expect(Gitlab::Database.read_only?).to be(true)

      expect(valid_password_in_read_only_transaction).to be(true)
      expect(user.reload.encrypted_password).to eq(encrypted_password)
    end
  end
end

On master both examples fail with the error the issue logged:

2) User on a Geo secondary accepts the password when the database refuses writes
   Failure/Error: return super unless block_given? &&

   ActiveRecord::StatementInvalid:
     PG::ReadOnlySqlTransaction: ERROR:  cannot execute UPDATE in a read-only transaction
   # ./lib/gitlab/patch/postgresql_adapter_returning_values.rb:20:in `log'
   # ./app/models/concerns/encrypted_user_password.rb:117:in `migrate_password!'
   # ./app/models/concerns/encrypted_user_password.rb:32:in `valid_password?'
   # ./app/models/user.rb:1408:in `valid_password?'

On this branch both pass.

How to set up and validate locally

I have not run these steps on a Geo GDK; the specs above are what I ran.

1. On the primary, in a Rails console, give a user without two-factor authentication a hash with a lower BCrypt cost than the configured one, which the next password sign-in migrates:

   user = User.find_by_username('<username>')
   user.update_column(:encrypted_password, BCrypt::Password.create('<password>', cost: 10).to_s)

2. Once the row has replicated, authenticate a Git over HTTP request against the secondary with that password:

   curl --user '<username>:<password>' "<secondary URL>/<project path>.git/info/refs?service=git-upload-pack"

3. On master the request answers 500 and the secondary's PostgreSQL log shows cannot execute UPDATE in a read-only transaction. On this branch it is served, and the hash on the secondary still starts with $2a$10$.

4. Sign in with the same password on the primary: the hash is migrated to the configured cost there, as before.

bundle exec rspec spec/models/concerns/encrypted_user_password_spec.rb passes on the branch, as do the 13 examples bundle exec rspec spec/models/user_spec.rb -e valid_password selects. RuboCop and both commit message linters report nothing.

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.