summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--changelogs/unreleased/check-trigger-permissions.yml5
-rw-r--r--lib/gitlab/database.rb8
-rw-r--r--lib/gitlab/database/grant.rb34
-rw-r--r--lib/gitlab/database/migration_helpers.rb36
-rw-r--r--spec/lib/gitlab/database/grant_spec.rb30
-rw-r--r--spec/lib/gitlab/database/migration_helpers_spec.rb32
6 files changed, 137 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 5e2c6cc5cad..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.
@@ -625,6 +629,30 @@ module Gitlab
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 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