create read-only "secondary" db user for dev/test
and activate this user while on the "secondary" in tests Canvas uses read-only secondary database replicas, but before now, GuardRail.activate(:secondary) had no effect in specs. The result is that specs wouldn't catch attempts to write to a secondary, and the error would be discovered in production, often requiring a hotfix. This patchset sets up a migration that creates a `canvas_readonly_user` in the database and sets up SELECT permissions for it in each shard's schema. (The migration does nothing in production.) It also stubs out GuardRail in specs to run `SET ROLE canvas_readonly_user` when activating the secondary, and `RESET ROLE` when returning to the primary. test plan: - specs pass (this PS includes specs that attempt to write to the secondary and verify the correct error is raised) - use the read-only user in development by adding the following to the development section in config/database.yml: secondary: username: canvas_readonly_user then try to write to the secondary in the rails console and ensure you get a permission denied error. for example, GuardRail.activate(:secondary) { User.create! } should result in PG::InsufficientPrivilege: ERROR: permission denied for table users (ActiveRecord::StatementInvalid) flag = none closes LS-2818 Change-Id: Ibfa75af821eb7f5d65f6b26aea03417378ab255a Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/161086 QA-Review: Isaac Moore <isaac.moore@instructure.com> Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com> Reviewed-by: Cody Cutrer <cody@instructure.com> Product-Review: Cody Cutrer <cody@instructure.com>
This commit is contained in:
parent
5c370bd705
commit
26fc3d406a
|
@ -1589,8 +1589,10 @@ class Account < ActiveRecord::Base
|
|||
t '#account.default_site_administrator_account_name', 'Site Admin'
|
||||
t '#account.default_account_name', 'Default Account'
|
||||
account = special_accounts[special_account_type] = Account.new(:name => default_account_name)
|
||||
account.save!
|
||||
Setting.set("#{special_account_type}_account_id", account.id)
|
||||
GuardRail.activate(:primary) do
|
||||
account.save!
|
||||
Setting.set("#{special_account_type}_account_id", account.id)
|
||||
end
|
||||
special_account_ids[special_account_type] = account.id
|
||||
end
|
||||
account
|
||||
|
@ -1958,13 +1960,15 @@ class Account < ActiveRecord::Base
|
|||
display_name = t('#account.manually_created_courses', "Manually-Created Courses")
|
||||
acct = manually_created_courses_account_from_settings
|
||||
if acct.blank?
|
||||
transaction do
|
||||
lock!
|
||||
acct = manually_created_courses_account_from_settings
|
||||
acct ||= self.sub_accounts.where(name: display_name).first_or_create! # for backwards compatibility
|
||||
if acct.id != self.settings[:manually_created_courses_account_id]
|
||||
self.settings[:manually_created_courses_account_id] = acct.id
|
||||
self.save!
|
||||
GuardRail.activate(:primary) do
|
||||
transaction do
|
||||
lock!
|
||||
acct = manually_created_courses_account_from_settings
|
||||
acct ||= self.sub_accounts.where(name: display_name).first_or_create! # for backwards compatibility
|
||||
if acct.id != self.settings[:manually_created_courses_account_id]
|
||||
self.settings[:manually_created_courses_account_id] = acct.id
|
||||
self.save!
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
@ -14,6 +14,8 @@ development:
|
|||
database: canvas_development
|
||||
password: your_password
|
||||
timeout: 5000
|
||||
secondary:
|
||||
username: canvas_readonly_user
|
||||
|
||||
production:
|
||||
adapter: postgresql
|
||||
|
|
|
@ -0,0 +1,66 @@
|
|||
# frozen_string_literal: true
|
||||
|
||||
#
|
||||
# Copyright (C) 2021 - present Instructure, Inc.
|
||||
#
|
||||
# This file is part of Canvas.
|
||||
#
|
||||
# Canvas is free software: you can redistribute it and/or modify it under
|
||||
# the terms of the GNU Affero General Public License as published by the Free
|
||||
# Software Foundation, version 3 of the License.
|
||||
#
|
||||
# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY
|
||||
# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
|
||||
# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
|
||||
# details.
|
||||
#
|
||||
# You should have received a copy of the GNU Affero General Public License along
|
||||
# with this program. If not, see <http://www.gnu.org/licenses/>.
|
||||
|
||||
class CreateReadOnlyDatabaseUser < ActiveRecord::Migration[6.0]
|
||||
tag :predeploy
|
||||
|
||||
def up
|
||||
# this user is *not* used in production! it's only used to simulate a read-only secondary database in dev/test
|
||||
return if ::Rails.env.production?
|
||||
|
||||
# the user is cluster-wide ...
|
||||
unless readonly_user_exists?
|
||||
execute("CREATE USER canvas_readonly_user")
|
||||
end
|
||||
|
||||
# ... but needs permissions on each shard's schema
|
||||
execute("GRANT USAGE ON SCHEMA #{quoted_schema} TO canvas_readonly_user")
|
||||
execute("GRANT SELECT ON ALL TABLES IN SCHEMA #{quoted_schema} TO canvas_readonly_user")
|
||||
execute("ALTER DEFAULT PRIVILEGES IN SCHEMA #{quoted_schema} GRANT SELECT ON TABLES TO canvas_readonly_user")
|
||||
end
|
||||
|
||||
def down
|
||||
return if ::Rails.env.production?
|
||||
|
||||
return unless readonly_user_exists?
|
||||
|
||||
# the user's privileges must be revoked before it can be dropped
|
||||
execute("ALTER DEFAULT PRIVILEGES IN SCHEMA #{quoted_schema} REVOKE SELECT ON TABLES FROM canvas_readonly_user")
|
||||
execute("REVOKE SELECT ON ALL TABLES IN SCHEMA #{quoted_schema} FROM canvas_readonly_user")
|
||||
execute("REVOKE USAGE ON SCHEMA #{quoted_schema} FROM canvas_readonly_user")
|
||||
|
||||
# attempt to drop the user. this will fail if another schema references it, but should succeed when this
|
||||
# migration runs on the last shard in the cluster
|
||||
begin
|
||||
connection.transaction(requires_new: true) do
|
||||
execute("DROP USER canvas_readonly_user")
|
||||
end
|
||||
rescue ActiveRecord::StatementInvalid => e
|
||||
raise unless e.cause.is_a?(PG::DependentObjectsStillExist)
|
||||
end
|
||||
end
|
||||
|
||||
def readonly_user_exists?
|
||||
!!connection.select_value("SELECT 1 AS one FROM pg_roles WHERE rolname='canvas_readonly_user'")
|
||||
end
|
||||
|
||||
def quoted_schema
|
||||
@quoted_schema ||= connection.quote_local_table_name(Shard.current.name)
|
||||
end
|
||||
end
|
|
@ -103,4 +103,37 @@ describe "spec_helper" do
|
|||
expect(root).not_to encompass([{ :key2 => :val2, :key3 => :val3 }, { :key4 => :val4 }])
|
||||
end
|
||||
end
|
||||
|
||||
context "ReadOnlyTestStub" do
|
||||
it "switches to a read-only secondary" do
|
||||
GuardRail.activate(:secondary) do
|
||||
expect { User.create! }.to raise_error(ActiveRecord::StatementInvalid, /PG::InsufficientPrivilege/)
|
||||
end
|
||||
end
|
||||
|
||||
it "nests primary inside secondary" do
|
||||
GuardRail.activate(:secondary) do
|
||||
expect { User.last }.not_to raise_error
|
||||
GuardRail.activate(:primary) do
|
||||
expect { User.create! }.not_to raise_error
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
it "works with after-transaction-commit hooks" do
|
||||
GuardRail.activate(:secondary) do
|
||||
User.transaction do
|
||||
User.connection.after_transaction_commit do
|
||||
User.last.touch
|
||||
end
|
||||
n = User.count
|
||||
GuardRail.activate(:primary) do
|
||||
expect { User.create! }.not_to raise_error
|
||||
end
|
||||
expect(GuardRail.environment).to eq :secondary
|
||||
expect(User.count).to eq n + 1
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
@ -0,0 +1,70 @@
|
|||
# frozen_string_literal: true
|
||||
|
||||
# Copyright (C) 2018 - present Instructure, Inc.
|
||||
#
|
||||
# This file is part of Canvas.
|
||||
#
|
||||
# Canvas is free software: you can redistribute it and/or modify it under
|
||||
# the terms of the GNU Affero General Public License as published by the Free
|
||||
# Software Foundation, version 3 of the License.
|
||||
#
|
||||
# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY
|
||||
# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
|
||||
# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
|
||||
# details.
|
||||
#
|
||||
# You should have received a copy of the GNU Affero General Public License along
|
||||
# with this program. If not, see <http://www.gnu.org/licenses/>.
|
||||
|
||||
describe 'read-only database role' do
|
||||
def with_read_only_role
|
||||
ActiveRecord::Base.connection.execute("SET ROLE canvas_readonly_user")
|
||||
yield
|
||||
ensure
|
||||
ActiveRecord::Base.connection.execute("RESET ROLE")
|
||||
end
|
||||
|
||||
it 'allows select' do
|
||||
user_factory(name: 'blah')
|
||||
with_read_only_role {
|
||||
expect(User.where(id: @user).pluck(:name)).to eq(['blah'])
|
||||
}
|
||||
end
|
||||
|
||||
it 'allows switching from read-only to read-write' do
|
||||
user_factory(name: 'blah')
|
||||
name = nil
|
||||
with_read_only_role {
|
||||
name = User.take.name
|
||||
}
|
||||
expect {
|
||||
@user.update name: name.succ
|
||||
}.not_to raise_error
|
||||
end
|
||||
|
||||
it 'disallows insert' do
|
||||
expect {
|
||||
with_read_only_role {
|
||||
user_factory
|
||||
}
|
||||
}.to raise_error(ActiveRecord::StatementInvalid, /PG::InsufficientPrivilege/)
|
||||
end
|
||||
|
||||
it 'disallows update' do
|
||||
user_factory(name: 'blah')
|
||||
expect {
|
||||
with_read_only_role {
|
||||
@user.update_attribute(:name, 'bleh')
|
||||
}
|
||||
}.to raise_error(ActiveRecord::StatementInvalid, /PG::InsufficientPrivilege/)
|
||||
end
|
||||
|
||||
it 'disallows delete' do
|
||||
user_factory(name: 'blah')
|
||||
expect {
|
||||
with_read_only_role {
|
||||
@user.destroy_permanently!
|
||||
}
|
||||
}.to raise_error(ActiveRecord::StatementInvalid, /PG::InsufficientPrivilege/)
|
||||
end
|
||||
end
|
|
@ -434,21 +434,6 @@ describe Attachment do
|
|||
a = attachment_model(:uploaded_data => default_uploaded_data)
|
||||
expect(a.filename).to eql("doc.doc")
|
||||
end
|
||||
|
||||
context "uploading and db transactions" do
|
||||
before :once do
|
||||
attachment_model(:context => Account.default.groups.create!, :filename => 'test.mp4', :content_type => 'video')
|
||||
end
|
||||
|
||||
it "delays upload until the #save transaction is committed" do
|
||||
allow(Rails.env).to receive(:test?).and_return(false)
|
||||
@attachment.uploaded_data = default_uploaded_data
|
||||
expect(Attachment.connection).to receive(:after_transaction_commit).twice
|
||||
expect(@attachment).not_to receive(:touch_context_if_appropriate)
|
||||
expect(@attachment).not_to receive(:ensure_media_object)
|
||||
@attachment.save
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
context "ensure_media_object" do
|
||||
|
|
|
@ -146,6 +146,7 @@ describe MicrosoftSync::PartialSyncChange do
|
|||
# simulate updating a record that hasn't made it to the secondary by changing the updated_at
|
||||
# after we fetch the last updated record from the secondary
|
||||
orig_secondary_method = GuardRail.method(:activate)
|
||||
allow(GuardRail).to receive(:activate).with(:primary)
|
||||
allow(GuardRail).to receive(:activate).with(:secondary) do |*args, &blk|
|
||||
res = orig_secondary_method.call(*args, &blk)
|
||||
pscs[2].update!(updated_at: 1.hour.from_now)
|
||||
|
|
|
@ -182,6 +182,49 @@ module RSpec::Rails
|
|||
end
|
||||
end
|
||||
|
||||
module ReadOnlySecondaryStub
|
||||
def self.reset
|
||||
ActiveRecord::Base.connection.execute("RESET ROLE")
|
||||
Thread.current[:stubbed_guard_rail_env] = nil
|
||||
end
|
||||
|
||||
def switch_role!(env)
|
||||
ActiveRecord::Base.connection.execute(env == :secondary ? "SET ROLE canvas_readonly_user" : "RESET ROLE")
|
||||
end
|
||||
|
||||
def environment
|
||||
Thread.current[:stubbed_guard_rail_env] || super
|
||||
end
|
||||
|
||||
def activate(env)
|
||||
return super if environment == :deploy
|
||||
return super unless [:primary, :secondary].include?(env)
|
||||
|
||||
previous_stub = Thread.current[:stubbed_guard_rail_env]
|
||||
previous_env = previous_stub || :primary
|
||||
return yield if previous_env == env
|
||||
|
||||
begin
|
||||
switch_role!(env)
|
||||
Thread.current[:stubbed_guard_rail_env] = env
|
||||
yield
|
||||
ensure
|
||||
switch_role!(previous_env)
|
||||
Thread.current[:stubbed_guard_rail_env] = previous_stub
|
||||
end
|
||||
end
|
||||
end
|
||||
GuardRail.singleton_class.prepend ReadOnlySecondaryStub
|
||||
|
||||
module ForceTransactionCommitCallbacksToPrimary
|
||||
def commit_records
|
||||
GuardRail.activate(:primary) do
|
||||
super
|
||||
end
|
||||
end
|
||||
end
|
||||
ActiveRecord::ConnectionAdapters::Transaction.prepend ForceTransactionCommitCallbacksToPrimary
|
||||
|
||||
module RenderWithHelpers
|
||||
def assign(key, value)
|
||||
@assigned_variables ||= {}
|
||||
|
@ -344,6 +387,7 @@ RSpec.configure do |config|
|
|||
end
|
||||
|
||||
def reset_all_the_things!
|
||||
ReadOnlySecondaryStub.reset
|
||||
I18n.locale = :en
|
||||
Time.zone = 'UTC'
|
||||
LoadAccount.force_special_account_reload = true
|
||||
|
|
Loading…
Reference in New Issue