diff options
author | Yorick Peterse <yorickpeterse@gmail.com> | 2017-08-18 14:26:23 +0200 |
---|---|---|
committer | Yorick Peterse <yorickpeterse@gmail.com> | 2017-08-29 13:02:44 +0200 |
commit | 5eab624d3ce7500b3cc19230b5ce86125b88015b (patch) | |
tree | cbfd2c470c9009fce1449a837d60058bed026aa0 /spec/lib/gitlab | |
parent | 9e7e0496ff639d1eec65dcbf1b51edb2262456e2 (diff) | |
download | gitlab-ce-5eab624d3ce7500b3cc19230b5ce86125b88015b.tar.gz |
Improve migrations using triggerscheck-trigger-permissions-mysql
This adds a bunch of checks to migrations that may create or drop
triggers. Dropping triggers/functions is done using "IF EXISTS" so we
don't throw an error if the object in question has already been dropped.
We now also raise a custom error (message) when the user does not have
TRIGGER privileges. This should prevent the schema from entering an
inconsistent state while also providing the user with enough information
on how to solve the problem.
The recommendation of using SUPERUSER permissions is a bit extreme but
we require this anyway (Omnibus also configures users with this
permission).
Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/36633
Diffstat (limited to 'spec/lib/gitlab')
-rw-r--r-- | spec/lib/gitlab/database/grant_spec.rb | 30 | ||||
-rw-r--r-- | spec/lib/gitlab/database/migration_helpers_spec.rb | 32 |
2 files changed, 58 insertions, 4 deletions
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 c25fd459dd7..1bcdc369c44 100644 --- a/spec/lib/gitlab/database/migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers_spec.rb @@ -450,6 +450,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') @@ -477,6 +479,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') @@ -506,6 +510,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}/) @@ -517,6 +523,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}/) @@ -573,8 +581,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 @@ -582,8 +590,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 @@ -890,4 +898,20 @@ describe Gitlab::Database::MigrationHelpers do 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 |