diff options
author | Douwe Maan <douwe@gitlab.com> | 2017-08-30 17:55:15 +0000 |
---|---|---|
committer | Jose Ivan Vargas <jvargas@gitlab.com> | 2017-09-08 12:05:01 -0500 |
commit | 531e89f25a2791928e826a560498eb05598efb0a (patch) | |
tree | be7c2a349aba07e5a39a90c9bba5d1173f0a164e | |
parent | 9c88832e9cdb6714e0d029a1eb743a993c170409 (diff) | |
download | gitlab-ce-531e89f25a2791928e826a560498eb05598efb0a.tar.gz |
Merge branch 'check-trigger-permissions-mysql' into 'master'
Improve migrations using triggers
Closes #36633
See merge request !13666
-rw-r--r-- | changelogs/unreleased/check-trigger-permissions.yml | 5 | ||||
-rw-r--r-- | lib/gitlab/database.rb | 8 | ||||
-rw-r--r-- | lib/gitlab/database/grant.rb | 34 | ||||
-rw-r--r-- | lib/gitlab/database/migration_helpers.rb | 50 | ||||
-rw-r--r-- | spec/lib/gitlab/database/grant_spec.rb | 30 | ||||
-rw-r--r-- | spec/lib/gitlab/database/migration_helpers_spec.rb | 79 |
6 files changed, 198 insertions, 8 deletions
diff --git a/changelogs/unreleased/check-trigger-permissions.yml b/changelogs/unreleased/check-trigger-permissions.yml new file mode 100644 index 00000000000..e0809cea9bf --- /dev/null +++ b/changelogs/unreleased/check-trigger-permissions.yml @@ -0,0 +1,5 @@ +--- +title: Improve migrations using triggers +merge_request: +author: +type: fixed diff --git a/lib/gitlab/database.rb b/lib/gitlab/database.rb index e001d25e7b7..a6ec75da385 100644 --- a/lib/gitlab/database.rb +++ b/lib/gitlab/database.rb @@ -9,6 +9,14 @@ module Gitlab ActiveRecord::Base.configurations[Rails.env] end + def self.username + config['username'] || ENV['USER'] + end + + def self.database_name + config['database'] + end + def self.adapter_name config['adapter'] end diff --git a/lib/gitlab/database/grant.rb b/lib/gitlab/database/grant.rb new file mode 100644 index 00000000000..aee3981e79a --- /dev/null +++ b/lib/gitlab/database/grant.rb @@ -0,0 +1,34 @@ +module Gitlab + module Database + # Model that can be used for querying permissions of a SQL user. + class Grant < ActiveRecord::Base + self.table_name = + if Database.postgresql? + 'information_schema.role_table_grants' + else + 'mysql.user' + end + + def self.scope_to_current_user + if Database.postgresql? + where('grantee = user') + else + where("CONCAT(User, '@', Host) = current_user()") + end + end + + # Returns true if the current user can create and execute triggers on the + # given table. + def self.create_and_execute_trigger?(table) + priv = + if Database.postgresql? + where(privilege_type: 'TRIGGER', table_name: table) + else + where(Trigger_priv: 'Y') + end + + priv.scope_to_current_user.any? + end + end + end +end diff --git a/lib/gitlab/database/migration_helpers.rb b/lib/gitlab/database/migration_helpers.rb index b83e633c7ed..fb14798efe6 100644 --- a/lib/gitlab/database/migration_helpers.rb +++ b/lib/gitlab/database/migration_helpers.rb @@ -358,6 +358,8 @@ module Gitlab raise 'rename_column_concurrently can not be run inside a transaction' end + check_trigger_permissions!(table) + old_col = column_for(table, old) new_type = type || old_col.type @@ -430,6 +432,8 @@ module Gitlab def cleanup_concurrent_column_rename(table, old, new) trigger_name = rename_trigger_name(table, old, new) + check_trigger_permissions!(table) + if Database.postgresql? remove_rename_triggers_for_postgresql(table, trigger_name) else @@ -485,14 +489,14 @@ module Gitlab # Removes the triggers used for renaming a PostgreSQL column concurrently. def remove_rename_triggers_for_postgresql(table, trigger) - execute("DROP TRIGGER #{trigger} ON #{table}") - execute("DROP FUNCTION #{trigger}()") + execute("DROP TRIGGER IF EXISTS #{trigger} ON #{table}") + execute("DROP FUNCTION IF EXISTS #{trigger}()") end # Removes the triggers used for renaming a MySQL column concurrently. def remove_rename_triggers_for_mysql(trigger) - execute("DROP TRIGGER #{trigger}_insert") - execute("DROP TRIGGER #{trigger}_update") + execute("DROP TRIGGER IF EXISTS #{trigger}_insert") + execute("DROP TRIGGER IF EXISTS #{trigger}_update") end # Returns the (base) name to use for triggers when renaming columns. @@ -611,6 +615,44 @@ module Gitlab remove_foreign_key(*args) rescue ArgumentError end + + def sidekiq_queue_migrate(queue_from, to:) + while sidekiq_queue_length(queue_from) > 0 + Sidekiq.redis do |conn| + conn.rpoplpush "queue:#{queue_from}", "queue:#{to}" + end + end + end + + def sidekiq_queue_length(queue_name) + Sidekiq.redis do |conn| + conn.llen("queue:#{queue_name}") + end + end + + def check_trigger_permissions!(table) + unless Grant.create_and_execute_trigger?(table) + dbname = Database.database_name + user = Database.username + + raise <<-EOF +Your database user is not allowed to create, drop, or execute triggers on the +table #{table}. + +If you are using PostgreSQL you can solve this by logging in to the GitLab +database (#{dbname}) using a super user and running: + + ALTER #{user} WITH SUPERUSER + +For MySQL you instead need to run: + + GRANT ALL PRIVILEGES ON *.* TO #{user}@'%' + +Both queries will grant the user super user permissions, ensuring you don't run +into similar problems in the future (e.g. when new tables are created). + EOF + end + end end end end diff --git a/spec/lib/gitlab/database/grant_spec.rb b/spec/lib/gitlab/database/grant_spec.rb new file mode 100644 index 00000000000..651da3e8476 --- /dev/null +++ b/spec/lib/gitlab/database/grant_spec.rb @@ -0,0 +1,30 @@ +require 'spec_helper' + +describe Gitlab::Database::Grant do + describe '.scope_to_current_user' do + it 'scopes the relation to the current user' do + user = Gitlab::Database.username + column = Gitlab::Database.postgresql? ? :grantee : :User + names = described_class.scope_to_current_user.pluck(column).uniq + + expect(names).to eq([user]) + end + end + + describe '.create_and_execute_trigger' do + it 'returns true when the user can create and execute a trigger' do + # We assume the DB/user is set up correctly so that triggers can be + # created, which is necessary anyway for other tests to work. + expect(described_class.create_and_execute_trigger?('users')).to eq(true) + end + + it 'returns false when the user can not create and/or execute a trigger' do + allow(described_class).to receive(:scope_to_current_user) + .and_return(described_class.none) + + result = described_class.create_and_execute_trigger?('kittens') + + expect(result).to eq(false) + end + end +end diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb index ec2274a70aa..40ac4ef319d 100644 --- a/spec/lib/gitlab/database/migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers_spec.rb @@ -452,6 +452,8 @@ describe Gitlab::Database::MigrationHelpers do it 'renames a column concurrently' do allow(Gitlab::Database).to receive(:postgresql?).and_return(false) + expect(model).to receive(:check_trigger_permissions!).with(:users) + expect(model).to receive(:install_rename_triggers_for_mysql) .with(trigger_name, 'users', 'old', 'new') @@ -479,6 +481,8 @@ describe Gitlab::Database::MigrationHelpers do it 'renames a column concurrently' do allow(Gitlab::Database).to receive(:postgresql?).and_return(true) + expect(model).to receive(:check_trigger_permissions!).with(:users) + expect(model).to receive(:install_rename_triggers_for_postgresql) .with(trigger_name, 'users', 'old', 'new') @@ -508,6 +512,8 @@ describe Gitlab::Database::MigrationHelpers do it 'cleans up the renaming procedure for PostgreSQL' do allow(Gitlab::Database).to receive(:postgresql?).and_return(true) + expect(model).to receive(:check_trigger_permissions!).with(:users) + expect(model).to receive(:remove_rename_triggers_for_postgresql) .with(:users, /trigger_.{12}/) @@ -519,6 +525,8 @@ describe Gitlab::Database::MigrationHelpers do it 'cleans up the renaming procedure for MySQL' do allow(Gitlab::Database).to receive(:postgresql?).and_return(false) + expect(model).to receive(:check_trigger_permissions!).with(:users) + expect(model).to receive(:remove_rename_triggers_for_mysql) .with(/trigger_.{12}/) @@ -575,8 +583,8 @@ describe Gitlab::Database::MigrationHelpers do describe '#remove_rename_triggers_for_postgresql' do it 'removes the function and trigger' do - expect(model).to receive(:execute).with('DROP TRIGGER foo ON bar') - expect(model).to receive(:execute).with('DROP FUNCTION foo()') + expect(model).to receive(:execute).with('DROP TRIGGER IF EXISTS foo ON bar') + expect(model).to receive(:execute).with('DROP FUNCTION IF EXISTS foo()') model.remove_rename_triggers_for_postgresql('bar', 'foo') end @@ -584,8 +592,8 @@ describe Gitlab::Database::MigrationHelpers do describe '#remove_rename_triggers_for_mysql' do it 'removes the triggers' do - expect(model).to receive(:execute).with('DROP TRIGGER foo_insert') - expect(model).to receive(:execute).with('DROP TRIGGER foo_update') + expect(model).to receive(:execute).with('DROP TRIGGER IF EXISTS foo_insert') + expect(model).to receive(:execute).with('DROP TRIGGER IF EXISTS foo_update') model.remove_rename_triggers_for_mysql('foo') end @@ -845,4 +853,67 @@ describe Gitlab::Database::MigrationHelpers do end end end + + describe 'sidekiq migration helpers', :sidekiq, :redis do + let(:worker) do + Class.new do + include Sidekiq::Worker + sidekiq_options queue: 'test' + end + end + + describe '#sidekiq_queue_length' do + context 'when queue is empty' do + it 'returns zero' do + Sidekiq::Testing.disable! do + expect(model.sidekiq_queue_length('test')).to eq 0 + end + end + end + + context 'when queue contains jobs' do + it 'returns correct size of the queue' do + Sidekiq::Testing.disable! do + worker.perform_async('Something', [1]) + worker.perform_async('Something', [2]) + + expect(model.sidekiq_queue_length('test')).to eq 2 + end + end + end + end + + describe '#migrate_sidekiq_queue' do + it 'migrates jobs from one sidekiq queue to another' do + Sidekiq::Testing.disable! do + worker.perform_async('Something', [1]) + worker.perform_async('Something', [2]) + + expect(model.sidekiq_queue_length('test')).to eq 2 + expect(model.sidekiq_queue_length('new_test')).to eq 0 + + model.sidekiq_queue_migrate('test', to: 'new_test') + + expect(model.sidekiq_queue_length('test')).to eq 0 + expect(model.sidekiq_queue_length('new_test')).to eq 2 + end + end + end + end + + describe '#check_trigger_permissions!' do + it 'does nothing when the user has the correct permissions' do + expect { model.check_trigger_permissions!('users') } + .not_to raise_error(RuntimeError) + end + + it 'raises RuntimeError when the user does not have the correct permissions' do + allow(Gitlab::Database::Grant).to receive(:create_and_execute_trigger?) + .with('kittens') + .and_return(false) + + expect { model.check_trigger_permissions!('kittens') } + .to raise_error(RuntimeError, /Your database user is not allowed/) + end + end end |