Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle cases where the database connection does not use a password #763

Merged
merged 1 commit into from
Oct 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions db/migrate/20241017013023_reencrypt_password_scramsha.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ class ReencryptPasswordScramsha < ActiveRecord::Migration[6.1]
def up
say_with_time('Reencrypting database user password with scram-sha-256') do
db_config = ActiveRecord::Base.connection_db_config.configuration_hash
return if db_config[:username].blank? || db_config[:password].blank?

username = db_config[:username]
password = connection.raw_connection.encrypt_password(db_config[:password], username, "scram-sha-256")

Expand Down
27 changes: 20 additions & 7 deletions spec/migrations/20241017013023_reencrypt_password_scramsha_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,31 @@
describe ReencryptPasswordScramsha do
migration_context :up do
it "Ensures that the user password is stored as scram-sha-256" do
migrate
allow(ActiveRecord::Base.connection).to receive(:execute).and_call_original

username = ActiveRecord::Base.connection_db_config.configuration_hash[:username]

users_and_passwords = ActiveRecord::Base.connection.execute <<-SQL
SELECT rolname, rolpassword FROM pg_authid WHERE rolcanlogin;
SQL
expect(ActiveRecord::Base.connection_db_config).to receive(:configuration_hash).exactly(10).times.and_call_original
Copy link
Member Author

@bdunne bdunne Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was an interesting find. It feels a little fragile to me, anyone have a better idea?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't love adding "real" code only to support tests, but maybe worth it in this case. You could add a method that returns the connection_db_config in the migration class and stub that method in this spec

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why exactly 10 times as opposed to something like at least once?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing because on the 11th time he needs to return a different payload

expect(ActiveRecord::Base.connection_db_config).to receive(:configuration_hash).and_wrap_original do |original_method, *args, &block|
original_method.call(*args, &block).dup.tap { |i| i[:password] ||= "abc" }
end
expect(ActiveRecord::Base.connection).to receive(:execute).with(a_string_matching(/ALTER ROLE #{username} WITH PASSWORD \'SCRAM-SHA-256.*\'\;/))

migrate
end

it "Handles connections with no password" do
allow(ActiveRecord::Base.connection).to receive(:execute).and_call_original

username = ActiveRecord::Base.connection_db_config.configuration_hash[:username]

record = users_and_passwords.to_a.detect { |i| i["rolname"] == username }
expect(ActiveRecord::Base.connection_db_config).to receive(:configuration_hash).exactly(10).times.and_call_original
expect(ActiveRecord::Base.connection_db_config).to receive(:configuration_hash).and_wrap_original do |original_method, *args, &block|
original_method.call(*args, &block).dup.tap { |i| i.delete(:password) }
end
expect(ActiveRecord::Base.connection).not_to receive(:execute).with(a_string_matching(/ALTER ROLE.*\'\;/))

expect(record["rolname"]).to eq(username)
expect(record["rolpassword"]).to match(/^SCRAM-SHA-256.*/)
migrate
end
end
end