Commit 5eab624d authored by Yorick Peterse's avatar Yorick Peterse

Improve migrations using triggers

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
parent 9e7e0496
---
title: Improve migrations using triggers
merge_request:
author:
type: fixed
...@@ -9,6 +9,14 @@ module Gitlab ...@@ -9,6 +9,14 @@ module Gitlab
ActiveRecord::Base.configurations[Rails.env] ActiveRecord::Base.configurations[Rails.env]
end end
def self.username
config['username'] || ENV['USER']
end
def self.database_name
config['database']
end
def self.adapter_name def self.adapter_name
config['adapter'] config['adapter']
end end
......
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
...@@ -358,6 +358,8 @@ module Gitlab ...@@ -358,6 +358,8 @@ module Gitlab
raise 'rename_column_concurrently can not be run inside a transaction' raise 'rename_column_concurrently can not be run inside a transaction'
end end
check_trigger_permissions!(table)
old_col = column_for(table, old) old_col = column_for(table, old)
new_type = type || old_col.type new_type = type || old_col.type
...@@ -430,6 +432,8 @@ module Gitlab ...@@ -430,6 +432,8 @@ module Gitlab
def cleanup_concurrent_column_rename(table, old, new) def cleanup_concurrent_column_rename(table, old, new)
trigger_name = rename_trigger_name(table, old, new) trigger_name = rename_trigger_name(table, old, new)
check_trigger_permissions!(table)
if Database.postgresql? if Database.postgresql?
remove_rename_triggers_for_postgresql(table, trigger_name) remove_rename_triggers_for_postgresql(table, trigger_name)
else else
...@@ -485,14 +489,14 @@ module Gitlab ...@@ -485,14 +489,14 @@ module Gitlab
# Removes the triggers used for renaming a PostgreSQL column concurrently. # Removes the triggers used for renaming a PostgreSQL column concurrently.
def remove_rename_triggers_for_postgresql(table, trigger) def remove_rename_triggers_for_postgresql(table, trigger)
execute("DROP TRIGGER #{trigger} ON #{table}") execute("DROP TRIGGER IF EXISTS #{trigger} ON #{table}")
execute("DROP FUNCTION #{trigger}()") execute("DROP FUNCTION IF EXISTS #{trigger}()")
end end
# Removes the triggers used for renaming a MySQL column concurrently. # Removes the triggers used for renaming a MySQL column concurrently.
def remove_rename_triggers_for_mysql(trigger) def remove_rename_triggers_for_mysql(trigger)
execute("DROP TRIGGER #{trigger}_insert") execute("DROP TRIGGER IF EXISTS #{trigger}_insert")
execute("DROP TRIGGER #{trigger}_update") execute("DROP TRIGGER IF EXISTS #{trigger}_update")
end end
# Returns the (base) name to use for triggers when renaming columns. # Returns the (base) name to use for triggers when renaming columns.
...@@ -625,6 +629,30 @@ module Gitlab ...@@ -625,6 +629,30 @@ module Gitlab
conn.llen("queue:#{queue_name}") conn.llen("queue:#{queue_name}")
end end
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 end
end end
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
...@@ -450,6 +450,8 @@ describe Gitlab::Database::MigrationHelpers do ...@@ -450,6 +450,8 @@ describe Gitlab::Database::MigrationHelpers do
it 'renames a column concurrently' do it 'renames a column concurrently' do
allow(Gitlab::Database).to receive(:postgresql?).and_return(false) 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) expect(model).to receive(:install_rename_triggers_for_mysql)
.with(trigger_name, 'users', 'old', 'new') .with(trigger_name, 'users', 'old', 'new')
...@@ -477,6 +479,8 @@ describe Gitlab::Database::MigrationHelpers do ...@@ -477,6 +479,8 @@ describe Gitlab::Database::MigrationHelpers do
it 'renames a column concurrently' do it 'renames a column concurrently' do
allow(Gitlab::Database).to receive(:postgresql?).and_return(true) 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) expect(model).to receive(:install_rename_triggers_for_postgresql)
.with(trigger_name, 'users', 'old', 'new') .with(trigger_name, 'users', 'old', 'new')
...@@ -506,6 +510,8 @@ describe Gitlab::Database::MigrationHelpers do ...@@ -506,6 +510,8 @@ describe Gitlab::Database::MigrationHelpers do
it 'cleans up the renaming procedure for PostgreSQL' do it 'cleans up the renaming procedure for PostgreSQL' do
allow(Gitlab::Database).to receive(:postgresql?).and_return(true) 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) expect(model).to receive(:remove_rename_triggers_for_postgresql)
.with(:users, /trigger_.{12}/) .with(:users, /trigger_.{12}/)
...@@ -517,6 +523,8 @@ describe Gitlab::Database::MigrationHelpers do ...@@ -517,6 +523,8 @@ describe Gitlab::Database::MigrationHelpers do
it 'cleans up the renaming procedure for MySQL' do it 'cleans up the renaming procedure for MySQL' do
allow(Gitlab::Database).to receive(:postgresql?).and_return(false) 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) expect(model).to receive(:remove_rename_triggers_for_mysql)
.with(/trigger_.{12}/) .with(/trigger_.{12}/)
...@@ -573,8 +581,8 @@ describe Gitlab::Database::MigrationHelpers do ...@@ -573,8 +581,8 @@ describe Gitlab::Database::MigrationHelpers do
describe '#remove_rename_triggers_for_postgresql' do describe '#remove_rename_triggers_for_postgresql' do
it 'removes the function and trigger' 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 TRIGGER IF EXISTS foo ON bar')
expect(model).to receive(:execute).with('DROP FUNCTION foo()') expect(model).to receive(:execute).with('DROP FUNCTION IF EXISTS foo()')
model.remove_rename_triggers_for_postgresql('bar', 'foo') model.remove_rename_triggers_for_postgresql('bar', 'foo')
end end
...@@ -582,8 +590,8 @@ describe Gitlab::Database::MigrationHelpers do ...@@ -582,8 +590,8 @@ describe Gitlab::Database::MigrationHelpers do
describe '#remove_rename_triggers_for_mysql' do describe '#remove_rename_triggers_for_mysql' do
it 'removes the triggers' do it 'removes the triggers' do
expect(model).to receive(:execute).with('DROP TRIGGER foo_insert') expect(model).to receive(:execute).with('DROP TRIGGER IF EXISTS foo_insert')
expect(model).to receive(:execute).with('DROP TRIGGER foo_update') expect(model).to receive(:execute).with('DROP TRIGGER IF EXISTS foo_update')
model.remove_rename_triggers_for_mysql('foo') model.remove_rename_triggers_for_mysql('foo')
end end
...@@ -890,4 +898,20 @@ describe Gitlab::Database::MigrationHelpers do ...@@ -890,4 +898,20 @@ describe Gitlab::Database::MigrationHelpers do
end end
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 end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment