From c483c6036ae1898f1f4a1b879cbb6d20fa70d8e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 3 Jun 2020 01:40:33 +0200 Subject: [PATCH] Install extensions in a shared schema This way all tenants will be able to access them instead of just the default one. The apartment gem recommends using a rake task instead of a migration, but that's a solution which is primarily meant for new installations. Migrations are easier to execute on existing installations. However, since this migration doesn't affect the `schema.rb` file, we still need to make sure the shared schema is created in tasks which do not execute migrations, like `db:schema:load` or `db:test:prepare`, just like the apartment gem recommends. That's why we're enhancing these tasks so they execute this migration. Note that there might be cases where the database user isn't a superuser (as it's usually the case on production environments), meaning commands to create, alter or drop extensions will fail. There's also the case where users don't have permissions to create schemas, which is needed in order to create the shared extensions schema and the schemas used by the tenants. For these reasons, we're minimizing the number of commands, and so we only alter or create extensions when it is really necessary. When users don't have permission, we aren't running the commands but showing a warning with the steps needed to run the migration manually. This is only necessary on installations which are going to use multitenancy; single-tenant applications upgrading don't need to run this migration, and that's why we aren't raising exceptions when we can't run it. For new installations, we'll change the CONSUL installer so extensions are automatically created in the shared schema. Also note the plpgsql extension is not handled here. This is a special extension which must be installed on the pg_catalog schema, which is always in the search path and so is shared by all tenants. Finally, we also need to change the `database.yml` file in order to search for shared extensions while running migrations or model tests, since none of our enabled extensions are executed during migrations; we're also adding a rake task for existing installations. Quoting the apartment documentation: > your database.yml file must mimic what you've set for your default and > persistent schemas in Apartment. When you run migrations with Rails, > it won't know about the extensions schema because Apartment isn't > injected into the default connection, it's done on a per-request > basis. --- app/models/tenant.rb | 2 +- config/database-docker.yml.example | 1 + config/database.yml.example | 1 + config/database.yml.gitlab | 1 + config/initializers/apartment.rb | 2 +- ...2233844_create_shared_extensions_schema.rb | 137 ++++++++++++++++++ lib/application_logger.rb | 4 + lib/tasks/consul.rake | 3 +- .../create_shared_extensions_schema.rake | 9 ++ lib/tasks/db.rake | 26 ++++ spec/models/tenant_spec.rb | 2 +- spec/system/multitenancy_spec.rb | 92 ++++++++++++ 12 files changed, 276 insertions(+), 4 deletions(-) create mode 100644 db/migrate/20200602233844_create_shared_extensions_schema.rb create mode 100644 lib/tasks/create_shared_extensions_schema.rake diff --git a/app/models/tenant.rb b/app/models/tenant.rb index b3f203727..5d8068cfc 100644 --- a/app/models/tenant.rb +++ b/app/models/tenant.rb @@ -10,7 +10,7 @@ class Tenant < ApplicationRecord after_destroy :destroy_schema def self.excluded_subdomains - Apartment::Elevators::Subdomain.excluded_subdomains + %w[mail] + Apartment::Elevators::Subdomain.excluded_subdomains + %w[mail shared_extensions] end def self.switch(...) diff --git a/config/database-docker.yml.example b/config/database-docker.yml.example index 48e1c30db..2fb012a61 100644 --- a/config/database-docker.yml.example +++ b/config/database-docker.yml.example @@ -5,6 +5,7 @@ default: &default host: database #<--the name of the db in the docker-compose pool: 5 port: 5432 + schema_search_path: "public,shared_extensions" username: postgres password: <%= ENV["POSTGRES_PASSWORD"] %> diff --git a/config/database.yml.example b/config/database.yml.example index 2f8b7463f..2e5e8d82d 100644 --- a/config/database.yml.example +++ b/config/database.yml.example @@ -3,6 +3,7 @@ default: &default encoding: unicode host: localhost pool: 5 + schema_search_path: "public,shared_extensions" username: password: diff --git a/config/database.yml.gitlab b/config/database.yml.gitlab index 8ada1adf2..aeaef364b 100644 --- a/config/database.yml.gitlab +++ b/config/database.yml.gitlab @@ -3,6 +3,7 @@ default: &default encoding: unicode host: postgres pool: 5 + schema_search_path: "public,shared_extensions" username: consul password: diff --git a/config/initializers/apartment.rb b/config/initializers/apartment.rb index a38b8fd3e..b4b5d4160 100644 --- a/config/initializers/apartment.rb +++ b/config/initializers/apartment.rb @@ -76,7 +76,7 @@ Apartment.configure do |config| # e.g when using a PostgreSQL extension like hstore. # Any schemas added here will be available along with your selected Tenant. # - # config.persistent_schemas = %w{ hstore } + config.persistent_schemas = ["shared_extensions"] # <== PostgreSQL only options # diff --git a/db/migrate/20200602233844_create_shared_extensions_schema.rb b/db/migrate/20200602233844_create_shared_extensions_schema.rb new file mode 100644 index 000000000..1a35bc80c --- /dev/null +++ b/db/migrate/20200602233844_create_shared_extensions_schema.rb @@ -0,0 +1,137 @@ +class CreateSharedExtensionsSchema < ActiveRecord::Migration[6.0] + def up + unless schema_exists?(extensions_schema) + execute_or_log_create_schema_warning("CREATE SCHEMA #{extensions_schema}") + end + + %w[unaccent pg_trgm].each do |extension| + if extension_enabled?(extension) + unless extension_already_in_extensions_schema?(extension) + execute_or_log_extension_warning("ALTER EXTENSION #{extension} SET SCHEMA #{extensions_schema}") + end + else + execute_or_log_extension_warning("CREATE EXTENSION #{extension} SCHEMA #{extensions_schema}") + end + end + + unless schema_exists?(extensions_schema) && public_has_usage_privilege_on_extensions_schema? + execute_or_log_grant_usage_warning("GRANT usage ON SCHEMA #{extensions_schema} TO public") + end + + show_full_warning_message if warning_messages.any? + end + + def down + %w[unaccent pg_trgm].each do |extension| + unless extension_already_in_public_schema?(extension) + execute "ALTER EXTENSION #{extension} SET SCHEMA public;" + end + end + + execute "DROP SCHEMA #{extensions_schema};" if schema_exists?(extensions_schema) + end + + private + + def extensions_schema + "shared_extensions" + end + + def extension_already_in_extensions_schema?(extension) + associated_schema_id_for(extension) == extensions_schema_id + end + + def extension_already_in_public_schema?(extension) + associated_schema_id_for(extension) == public_schema_id + end + + def associated_schema_id_for(extension) + query_value("SELECT extnamespace FROM pg_extension WHERE extname=#{quote(extension)}") + end + + def extensions_schema_id + schema_id_for(extensions_schema) + end + + def public_schema_id + schema_id_for("public") + end + + def schema_id_for(schema) + query_value("SELECT oid FROM pg_namespace WHERE nspname=#{quote(schema)}") + end + + def execute_or_log_create_schema_warning(statement) + if create_permission? + execute statement + else + log_warning( + "GRANT CREATE ON DATABASE #{query_value("SELECT CURRENT_DATABASE()")} "\ + "TO #{query_value("SELECT CURRENT_USER")}" + ) + log_warning(statement) + end + end + + def execute_or_log_extension_warning(statement) + if superuser? + execute statement + else + log_warning(statement) + end + end + + def execute_or_log_grant_usage_warning(statement) + if schema_exists?(extensions_schema) && grant_usage_permission? + execute statement + else + log_warning(statement) + end + end + + def create_permission? + query_value("SELECT has_database_privilege(CURRENT_USER, CURRENT_DATABASE(), 'CREATE');") + end + + def superuser? + query_value("SELECT usesuper FROM pg_user WHERE usename = CURRENT_USER") + end + + def grant_usage_permission? + query_value("SELECT has_schema_privilege(CURRENT_USER, '#{extensions_schema}', 'CREATE');") + end + + def public_has_usage_privilege_on_extensions_schema? + query_value("SELECT has_schema_privilege('public', '#{extensions_schema}', 'USAGE');") + end + + def log_warning(statement) + warning_messages.push(statement) + end + + def warning_messages + @warning_messages ||= [] + end + + def show_full_warning_message + message = <<~WARNING + ---------------------- Multitenancy Warning ---------------------- + Multitenancy is a feature that allows managing multiple + institutions in a completely independent way using just one + CONSUL installation. + + NOTE: If you aren't going to use multitenancy, you can safely + ignore this warning. + + If you'd like to enable this feature, first run: + #{warning_messages.join(";\n ")}; + using a user with enough database privileges. + + Check the CONSUL release notes for more information. + ------------------------------------------------------------------ + WARNING + + puts message + Rails.logger.warn(message) + end +end diff --git a/lib/application_logger.rb b/lib/application_logger.rb index 44a4a0fde..33b1e0af8 100644 --- a/lib/application_logger.rb +++ b/lib/application_logger.rb @@ -3,6 +3,10 @@ class ApplicationLogger logger.info(message) unless Rails.env.test? end + def warn(message) + logger.warn(message) unless Rails.env.test? + end + def logger @logger ||= Logger.new(STDOUT).tap do |logger| logger.formatter = proc { |severity, _datetime, _progname, msg| "#{severity} #{msg}\n" } diff --git a/lib/tasks/consul.rake b/lib/tasks/consul.rake index 6f5e3724b..f5a57c1c6 100644 --- a/lib/tasks/consul.rake +++ b/lib/tasks/consul.rake @@ -7,6 +7,7 @@ namespace :consul do desc "Runs tasks needed to upgrade from 1.5.0 to 1.6.0" task "execute_release_1.6.0_tasks": [ "db:calculate_tsv", - "polls:set_ends_at_to_end_of_day" + "polls:set_ends_at_to_end_of_day", + "db:add_schema_search_path" ] end diff --git a/lib/tasks/create_shared_extensions_schema.rake b/lib/tasks/create_shared_extensions_schema.rake new file mode 100644 index 000000000..32e52b429 --- /dev/null +++ b/lib/tasks/create_shared_extensions_schema.rake @@ -0,0 +1,9 @@ +require Rails.root.join("db/migrate/20200602233844_create_shared_extensions_schema.rb") + +Rake::Task["db:schema:load"].enhance do + CreateSharedExtensionsSchema.new.up +end + +Rake::Task["db:test:purge"].enhance do + CreateSharedExtensionsSchema.new.up +end diff --git a/lib/tasks/db.rake b/lib/tasks/db.rake index cb6b20f92..22597433a 100644 --- a/lib/tasks/db.rake +++ b/lib/tasks/db.rake @@ -16,4 +16,30 @@ namespace :db do logger.info "Calculating tsvector for proposal notifications" ProposalNotification.with_hidden.find_each(&:calculate_tsvector) end + + desc "Adds shared extensions to the schema search path in the database.yml file" + task add_schema_search_path: :environment do + logger = ApplicationLogger.new + logger.info "Adding search path to config/database.yml" + + config = Rails.application.config.paths["config/database"].first + lines = File.readlines(config) + changes_done = false + + adapter_indices = lines.map.with_index do |line, index| + index if line.start_with?(" adapter: postgresql") + end.compact + + adapter_indices.reverse_each do |index| + unless lines[index + 1]&.match?("schema_search_path") + lines.insert(index + 1, " schema_search_path: \"public,shared_extensions\"\n") + changes_done = true + end + end + + if changes_done + File.write(config, lines.join) + logger.warn "The database search path has been updated. Restart the application to apply the changes." + end + end end diff --git a/spec/models/tenant_spec.rb b/spec/models/tenant_spec.rb index a6d872e8c..497ba4b5f 100644 --- a/spec/models/tenant_spec.rb +++ b/spec/models/tenant_spec.rb @@ -19,7 +19,7 @@ describe Tenant do end it "is not valid with an excluded subdomain" do - %w[mail public www].each do |subdomain| + %w[mail public shared_extensions www].each do |subdomain| tenant.schema = subdomain expect(tenant).not_to be_valid end diff --git a/spec/system/multitenancy_spec.rb b/spec/system/multitenancy_spec.rb index cdf0891ba..ef1a90564 100644 --- a/spec/system/multitenancy_spec.rb +++ b/spec/system/multitenancy_spec.rb @@ -21,6 +21,98 @@ describe "Multitenancy" do end end + scenario "Content is different for differents tenants" do + Tenant.switch("mars") { create(:poll, name: "Human rights for Martians?") } + + with_subdomain("mars") do + visit polls_path + + expect(page).to have_content "Human rights for Martians?" + end + + with_subdomain("venus") do + visit polls_path + + expect(page).to have_content "There are no open votings" + end + end + + scenario "PostgreSQL extensions work for tenants" do + Tenant.switch("mars") { login_as(create(:user)) } + + with_subdomain("mars") do + visit new_proposal_path + fill_in "Proposal title", with: "Use the unaccent extension in Mars" + fill_in "Proposal summary", with: "tsvector for María the Martian" + check "I agree to the Privacy Policy and the Terms and conditions of use" + + click_button "Create proposal" + + expect(page).to have_content "Proposal created successfully." + + click_link "No, I want to publish the proposal" + + expect(page).to have_content "You've created a proposal!" + + visit proposals_path + click_button "Advanced search" + fill_in "With the text", with: "Maria the Martian" + click_button "Filter" + + expect(page).to have_content "Search results" + expect(page).to have_content "María the Martian" + end + end + + scenario "Creating content in one tenant doesn't affect other tenants" do + Tenant.switch("mars") { login_as(create(:user)) } + + with_subdomain("mars") do + visit new_debate_path + fill_in "Debate title", with: "Found any water here?" + fill_in_ckeditor "Initial debate text", with: "Found any water here?" + check "I agree to the Privacy Policy and the Terms and conditions of use" + + click_button "Start a debate" + + expect(page).to have_content "Debate created successfully." + expect(page).to have_content "Found any water here?" + end + + with_subdomain("venus") do + visit debates_path + + expect(page).to have_content "Sign in" + expect(page).not_to have_css ".debate" + + visit new_debate_path + + expect(page).to have_content "You must sign in or register to continue." + end + end + + scenario "Users from another tenant cannot vote" do + Tenant.switch("mars") { create(:proposal, title: "Earth invasion") } + Tenant.switch("venus") { login_as(create(:user)) } + + with_subdomain("venus") do + visit proposals_path + + expect(page).to have_content "Sign out" + expect(page).not_to have_content "Earth invasion" + end + + with_subdomain("mars") do + visit proposals_path + + within(".proposal", text: "Earth invasion") do + click_button "Support" + + expect(page).to have_content "You must sign in or sign up to continue" + end + end + end + scenario "Sign up into subdomain" do with_subdomain("mars") do visit "/"