From fcd8466ddf4341c911cb09a0558918cc5009e213 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 23 Sep 2022 18:20:03 +0200 Subject: [PATCH 01/33] Allow using the lvh.me URL in development Rails 6.0 introduced a `hosts` option which, in the development environment, defaults to all IP addresses and the `localhost` domain. However, we can't work with subdomains using `localhost`. For that purpose, the `lvh.me` domain was created [1]. So we're allowing this domain and its subdomains so we can use them while working with multitenancy in the development environment. [1] http://railscasts.com/episodes/123-subdomains-revised --- config/environments/development.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/config/environments/development.rb b/config/environments/development.rb index 42304e757..925734721 100644 --- a/config/environments/development.rb +++ b/config/environments/development.rb @@ -28,6 +28,10 @@ Rails.application.configure do config.cache_store = :null_store end + # Allow accessing the application through a domain so subdomains can be used + config.hosts << "lvh.me" + config.hosts << /.*\.lvh\.me/ + # Don't care if the mailer can't send. config.action_mailer.raise_delivery_errors = false config.action_mailer.default_url_options = { host: "localhost", port: 3000 } From 382abb36665916e4b3c94a9678190634fd9ec599 Mon Sep 17 00:00:00 2001 From: Eduardo Vilar Date: Mon, 30 Jul 2018 08:52:54 +0200 Subject: [PATCH 02/33] Add multitenancy with apartment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Javi Martín --- Gemfile | 1 + Gemfile.lock | 8 +- app/models/tenant.rb | 37 +++++++ config/initializers/apartment.rb | 117 ++++++++++++++++++++ db/migrate/20180502075740_create_tenants.rb | 12 ++ db/schema.rb | 9 ++ spec/factories/administration.rb | 5 + spec/models/tenant_spec.rb | 64 +++++++++++ spec/rails_helper.rb | 11 ++ spec/system/multitenancy_spec.rb | 63 +++++++++++ 10 files changed, 326 insertions(+), 1 deletion(-) create mode 100644 app/models/tenant.rb create mode 100644 config/initializers/apartment.rb create mode 100644 db/migrate/20180502075740_create_tenants.rb create mode 100644 spec/models/tenant_spec.rb create mode 100644 spec/system/multitenancy_spec.rb diff --git a/Gemfile b/Gemfile index 9d247a509..7cc13c7fe 100644 --- a/Gemfile +++ b/Gemfile @@ -49,6 +49,7 @@ gem "recipient_interceptor", "~> 0.3.1" gem "redcarpet", "~> 3.5.1" gem "responders", "~> 3.0.1" gem "rinku", "~> 2.0.6", require: "rails_rinku" +gem "ros-apartment", "~> 2.11.0", require: "apartment" gem "sassc-rails", "~> 2.1.2" gem "savon", "~> 2.13.0" gem "sitemap_generator", "~> 6.3.0" diff --git a/Gemfile.lock b/Gemfile.lock index 5cece524d..9693b05ca 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -472,7 +472,7 @@ GEM pronto-scss (0.11.0) pronto (~> 0.11.0) scss_lint (~> 0.43, >= 0.43.0) - public_suffix (5.0.0) + public_suffix (4.0.7) puma (4.3.12) nio4r (~> 2.0) racc (1.6.0) @@ -534,6 +534,11 @@ GEM retriable (3.1.2) rexml (3.2.5) rinku (2.0.6) + ros-apartment (2.11.0) + activerecord (>= 5.0.0, < 7.1) + parallel (< 2.0) + public_suffix (>= 2.0.5, < 5.0) + rack (>= 1.3.6, < 3.0) rspec-core (3.11.0) rspec-support (~> 3.11.0) rspec-expectations (3.11.0) @@ -784,6 +789,7 @@ DEPENDENCIES redcarpet (~> 3.5.1) responders (~> 3.0.1) rinku (~> 2.0.6) + ros-apartment (~> 2.11.0) rspec-rails (~> 5.1.2) rubocop (~> 1.35.1) rubocop-performance (~> 1.11.4) diff --git a/app/models/tenant.rb b/app/models/tenant.rb new file mode 100644 index 000000000..b3f203727 --- /dev/null +++ b/app/models/tenant.rb @@ -0,0 +1,37 @@ +class Tenant < ApplicationRecord + validates :schema, + presence: true, + uniqueness: true, + exclusion: { in: ->(*) { excluded_subdomains }} + validates :name, presence: true, uniqueness: true + + after_create :create_schema + after_update :rename_schema + after_destroy :destroy_schema + + def self.excluded_subdomains + Apartment::Elevators::Subdomain.excluded_subdomains + %w[mail] + end + + def self.switch(...) + Apartment::Tenant.switch(...) + end + + private + + def create_schema + Apartment::Tenant.create(schema) + end + + def rename_schema + if saved_change_to_schema? + ActiveRecord::Base.connection.execute( + "ALTER SCHEMA \"#{schema_before_last_save}\" RENAME TO \"#{schema}\";" + ) + end + end + + def destroy_schema + Apartment::Tenant.drop(schema) + end +end diff --git a/config/initializers/apartment.rb b/config/initializers/apartment.rb new file mode 100644 index 000000000..a38b8fd3e --- /dev/null +++ b/config/initializers/apartment.rb @@ -0,0 +1,117 @@ +# You can have Apartment route to the appropriate Tenant by adding some Rack middleware. +# Apartment can support many different "Elevators" that can take care of this routing to your data. +# Require whichever Elevator you're using below or none if you have a custom one. +# +# require "apartment/elevators/generic" +# require "apartment/elevators/domain" +require "apartment/elevators/subdomain" +# require "apartment/elevators/first_subdomain" +# require "apartment/elevators/host" + +# +# Apartment Configuration +# +Apartment.configure do |config| + config.seed_after_create = true + + # Add any models that you do not want to be multi-tenanted, but remain in the global (public) namespace. + # A typical example would be a Customer or Tenant model that stores each Tenant's information. + # + config.excluded_models = %w[Tenant] + + # In order to migrate all of your Tenants you need to provide a list of Tenant names to Apartment. + # You can make this dynamic by providing a Proc object to be called on migrations. + # This object should yield either: + # - an array of strings representing each Tenant name. + # - a hash which keys are tenant names, and values custom db config + # (must contain all key/values required in database.yml) + # + # config.tenant_names = lambda{ Customer.pluck(:tenant_name) } + # config.tenant_names = ["tenant1", "tenant2"] + # config.tenant_names = { + # "tenant1" => { + # adapter: "postgresql", + # host: "some_server", + # port: 5555, + # database: "postgres" # this is not the name of the tenant's db + # # but the name of the database to connect to before creating the tenant's db + # # mandatory in postgresql + # }, + # "tenant2" => { + # adapter: "postgresql", + # database: "postgres" # this is not the name of the tenant's db + # # but the name of the database to connect to before creating the tenant's db + # # mandatory in postgresql + # } + # } + # config.tenant_names = lambda do + # Tenant.all.each_with_object({}) do |tenant, hash| + # hash[tenant.name] = tenant.db_configuration + # end + # end + # + config.tenant_names = -> { Tenant.pluck :schema } + + # PostgreSQL: + # Specifies whether to use PostgreSQL schemas or create a new database per Tenant. + # + # MySQL: + # Specifies whether to switch databases by using `use` statement or re-establish connection. + # + # The default behaviour is true. + # + # config.use_schemas = true + + # + # ==> PostgreSQL only options + + # Apartment can be forced to use raw SQL dumps instead of schema.rb for creating new schemas. + # Use this when you are using some extra features in PostgreSQL that can't be represented in + # schema.rb, like materialized views etc. (only applies with use_schemas set to true). + # (Note: this option doesn't use db/structure.sql, it creates SQL dump by executing pg_dump) + # + # config.use_sql = false + + # There are cases where you might want some schemas to always be in your search_path + # 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 } + + # <== PostgreSQL only options + # + + # By default, and only when not using PostgreSQL schemas, Apartment will prepend the environment + # to the tenant name to ensure there is no conflict between your environments. + # This is mainly for the benefit of your development and test environments. + # Uncomment the line below if you want to disable this behaviour in production. + # + # config.prepend_environment = !Rails.env.production? + + # When using PostgreSQL schemas, the database dump will be namespaced, and + # apartment will substitute the default namespace (usually public) with the + # name of the new tenant when creating a new tenant. Some items must maintain + # a reference to the default namespace (ie public) - for instance, a default + # uuid generation. Uncomment the line below to create a list of namespaced + # items in the schema dump that should *not* have their namespace replaced by + # the new tenant + # + # config.pg_excluded_names = ["uuid_generate_v4"] + + # Specifies whether the database and schema (when using PostgreSQL schemas) will prepend in ActiveRecord log. + # Uncomment the line below if you want to enable this behavior. + # + # config.active_record_log = true +end + +# Setup a custom Tenant switching middleware. The Proc should return the name of the Tenant that +# you want to switch to. +# Rails.application.config.middleware.use Apartment::Elevators::Generic, lambda { |request| +# request.host.split(".").first +# } + +# Rails.application.config.middleware.use Apartment::Elevators::Domain +Rails.application.config.middleware.use Apartment::Elevators::Subdomain +# Rails.application.config.middleware.use Apartment::Elevators::FirstSubdomain +# Rails.application.config.middleware.use Apartment::Elevators::Host +Apartment::Elevators::Subdomain.excluded_subdomains = %w[public www] diff --git a/db/migrate/20180502075740_create_tenants.rb b/db/migrate/20180502075740_create_tenants.rb new file mode 100644 index 000000000..323fa3d6f --- /dev/null +++ b/db/migrate/20180502075740_create_tenants.rb @@ -0,0 +1,12 @@ +class CreateTenants < ActiveRecord::Migration[4.2] + def change + create_table :tenants do |t| + t.string :name + t.string :schema + t.timestamps null: false + + t.index :name, unique: true + t.index :schema, unique: true + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 20c37835e..429b1b9fc 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1555,6 +1555,15 @@ ActiveRecord::Schema.define(version: 2022_09_15_154808) do t.index ["proposals_count"], name: "index_tags_on_proposals_count" end + create_table "tenants", id: :serial, force: :cascade do |t| + t.string "name" + t.string "schema" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["name"], name: "index_tenants_on_name", unique: true + t.index ["schema"], name: "index_tenants_on_schema", unique: true + end + create_table "topics", id: :serial, force: :cascade do |t| t.string "title", null: false t.text "description" diff --git a/spec/factories/administration.rb b/spec/factories/administration.rb index 07a110ca5..de6df90be 100644 --- a/spec/factories/administration.rb +++ b/spec/factories/administration.rb @@ -98,4 +98,9 @@ FactoryBot.define do value_es { "Texto en español" } value_en { "Text in english" } end + + factory :tenant do + sequence(:name) { |n| "Tenant #{n}" } + sequence(:schema) { |n| "subdomain#{n}" } + end end diff --git a/spec/models/tenant_spec.rb b/spec/models/tenant_spec.rb new file mode 100644 index 000000000..a6d872e8c --- /dev/null +++ b/spec/models/tenant_spec.rb @@ -0,0 +1,64 @@ +require "rails_helper" + +describe Tenant do + describe "validations" do + let(:tenant) { build(:tenant) } + + it "is valid" do + expect(tenant).to be_valid + end + + it "is not valid without a schema" do + tenant.schema = nil + expect(tenant).not_to be_valid + end + + it "is not valid with an already existing schema" do + expect(create(:tenant, schema: "subdomainx")).to be_valid + expect(build(:tenant, schema: "subdomainx")).not_to be_valid + end + + it "is not valid with an excluded subdomain" do + %w[mail public www].each do |subdomain| + tenant.schema = subdomain + expect(tenant).not_to be_valid + end + end + + it "is not valid without a name" do + tenant.name = "" + expect(tenant).not_to be_valid + end + + it "is not valid with an already existing name" do + expect(create(:tenant, name: "Name X")).to be_valid + expect(build(:tenant, name: "Name X")).not_to be_valid + end + end + + describe "#create_schema" do + it "creates a schema creating a record" do + create(:tenant, schema: "new") + expect { Tenant.switch("new") { nil } }.not_to raise_exception + end + end + + describe "#rename_schema" do + it "renames the schema when updating the schema" do + tenant = create(:tenant, schema: "typo") + tenant.update!(schema: "notypo") + + expect { Tenant.switch("typo") { nil } }.to raise_exception(Apartment::TenantNotFound) + expect { Tenant.switch("notypo") { nil } }.not_to raise_exception + end + end + + describe "#destroy_schema" do + it "drops the schema when destroying a record" do + tenant = create(:tenant, schema: "wrong") + tenant.destroy! + + expect { Tenant.switch("wrong") { nil } }.to raise_exception(Apartment::TenantNotFound) + end + end +end diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 5d8e11d91..08fa8c4c3 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -65,3 +65,14 @@ Capybara.enable_aria_label = true Capybara.disable_animation = true OmniAuth.config.test_mode = true + +def with_subdomain(subdomain, &block) + app_host = Capybara.app_host + + begin + Capybara.app_host = "http://#{subdomain}.lvh.me" + block.call + ensure + Capybara.app_host = app_host + end +end diff --git a/spec/system/multitenancy_spec.rb b/spec/system/multitenancy_spec.rb new file mode 100644 index 000000000..cdf0891ba --- /dev/null +++ b/spec/system/multitenancy_spec.rb @@ -0,0 +1,63 @@ +require "rails_helper" + +describe "Multitenancy" do + before do + create(:tenant, schema: "mars") + create(:tenant, schema: "venus") + end + + scenario "Disabled features", :no_js do + Tenant.switch("mars") { Setting["process.debates"] = true } + Tenant.switch("venus") { Setting["process.debates"] = nil } + + with_subdomain("mars") do + visit debates_path + + expect(page).to have_css "#debates" + end + + with_subdomain("venus") do + expect { visit debates_path }.to raise_exception(FeatureFlags::FeatureDisabled) + end + end + + scenario "Sign up into subdomain" do + with_subdomain("mars") do + visit "/" + click_link "Register" + + fill_in "Username", with: "Marty McMartian" + fill_in "Email", with: "marty@consul.dev" + fill_in "Password", with: "20151021" + fill_in "Confirm password", with: "20151021" + check "By registering you accept the terms and conditions of use" + click_button "Register" + + confirm_email + + expect(page).to have_content "Your account has been confirmed." + end + end + + scenario "Users from another tenant can't sign in" do + Tenant.switch("mars") { create(:user, email: "marty@consul.dev", password: "20151021") } + + with_subdomain("mars") do + visit new_user_session_path + fill_in "Email or username", with: "marty@consul.dev" + fill_in "Password", with: "20151021" + click_button "Enter" + + expect(page).to have_content "You have been signed in successfully." + end + + with_subdomain("venus") do + visit new_user_session_path + fill_in "Email or username", with: "marty@consul.dev" + fill_in "Password", with: "20151021" + click_button "Enter" + + expect(page).to have_content "Invalid Email or username or password." + end + end +end 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 03/33] 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 "/" From d77cf77761a278dffcd012e6273a612e09cbcebc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 25 Sep 2022 16:48:23 +0200 Subject: [PATCH 04/33] Validate format of subdomains / schema names Note we're using the `:HOST` regular expression since subdomains can contain the same characters as domains do. This isn't 100% precise, though, since subdomains have a maximum length of 63 characters, but is good enough for our purposes. --- app/models/tenant.rb | 3 ++- spec/models/tenant_spec.rb | 10 ++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/app/models/tenant.rb b/app/models/tenant.rb index 5d8068cfc..ebfe0fdb4 100644 --- a/app/models/tenant.rb +++ b/app/models/tenant.rb @@ -2,7 +2,8 @@ class Tenant < ApplicationRecord validates :schema, presence: true, uniqueness: true, - exclusion: { in: ->(*) { excluded_subdomains }} + exclusion: { in: ->(*) { excluded_subdomains }}, + format: { with: URI::DEFAULT_PARSER.regexp[:HOST] } validates :name, presence: true, uniqueness: true after_create :create_schema diff --git a/spec/models/tenant_spec.rb b/spec/models/tenant_spec.rb index 497ba4b5f..f22ef0d46 100644 --- a/spec/models/tenant_spec.rb +++ b/spec/models/tenant_spec.rb @@ -25,6 +25,16 @@ describe Tenant do end end + it "is valid with nested subdomains" do + tenant.schema = "multiple.sub.domains" + expect(tenant).to be_valid + end + + it "is not valid with an invalid subdomain" do + tenant.schema = "my sub domain" + expect(tenant).not_to be_valid + end + it "is not valid without a name" do tenant.name = "" expect(tenant).not_to be_valid From 598300665793cec485149f97847f7f7cef42b277 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 30 Sep 2022 23:24:34 +0200 Subject: [PATCH 05/33] Use a custom method to detect the current tenant The subdomain elevator we were using, which is included in apartment, didn't work on hosts already including a subdomain (like demo.consul.dev, for instance). In those cases, we would manually add the subdomain to the list of excluded subdomains. Since these subdomains will be different for different CONSUL installations, it meant each installation had to customize the code. Furthermore, existing installations using subdomains would stop working. So we're using a custom method to find the current tenant, based on the host defined in `default_url_options`. In order to avoid any side-effects on single-tenant applications, we're adding a new configuration option to enable multitenancy We're enabling two ways to handle this configuration option: a) Change the application_custom.rb file, which is under version control b) Change the secrets.yml file, which is not under version control This way people prefering to handle configuration options through version control can do so, while people who prefer handling configuration options through te secrets.yml file can do so as well. We're also disabling the super-annoying warnings mentioning there are no tenants which we got every time we run migrations on single-tenant applications. These messages will only be enabled when the multitenancy feature is enabled too. For this reason, we're also disabling the multitenancy feature in the development environment by default. --- app/models/tenant.rb | 19 ++++- config/application.rb | 3 + config/environments/test.rb | 3 + config/initializers/apartment.rb | 14 ++-- config/secrets.yml.example | 4 + spec/models/tenant_spec.rb | 132 +++++++++++++++++++++++++++++++ 6 files changed, 167 insertions(+), 8 deletions(-) diff --git a/app/models/tenant.rb b/app/models/tenant.rb index ebfe0fdb4..d343f8a61 100644 --- a/app/models/tenant.rb +++ b/app/models/tenant.rb @@ -10,8 +10,25 @@ class Tenant < ApplicationRecord after_update :rename_schema after_destroy :destroy_schema + def self.resolve_host(host) + return nil unless Rails.application.config.multitenancy.present? + return nil if host.blank? || host.match?(Resolv::AddressRegex) + + host_domain = allowed_domains.find { |domain| host == domain || host.ends_with?(".#{domain}") } + host.delete_prefix("www.").sub(/\.?#{host_domain}\Z/, "").presence + end + + def self.allowed_domains + dev_domains = %w[localhost lvh.me example.com] + dev_domains + [default_host] + end + def self.excluded_subdomains - Apartment::Elevators::Subdomain.excluded_subdomains + %w[mail shared_extensions] + %w[mail public shared_extensions www] + end + + def self.default_host + ActionMailer::Base.default_url_options[:host] end def self.switch(...) diff --git a/config/application.rb b/config/application.rb index d18d64a15..82c552a05 100644 --- a/config/application.rb +++ b/config/application.rb @@ -134,6 +134,9 @@ module Consul config.autoload_paths << "#{Rails.root}/app/graphql/custom" config.autoload_paths << "#{Rails.root}/app/models/custom" config.paths["app/views"].unshift(Rails.root.join("app", "views", "custom")) + + # Set to true to enable managing different tenants using the same application + config.multitenancy = Rails.application.secrets.multitenancy end end diff --git a/config/environments/test.rb b/config/environments/test.rb index e58456200..467972d2e 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -59,6 +59,9 @@ Rails.application.configure do Bullet.raise = true # raise an error if n+1 query occurs end end + + # Allow managing different tenants using the same application + config.multitenancy = true end require Rails.root.join("config", "environments", "custom", "test") diff --git a/config/initializers/apartment.rb b/config/initializers/apartment.rb index b4b5d4160..ad022b372 100644 --- a/config/initializers/apartment.rb +++ b/config/initializers/apartment.rb @@ -2,9 +2,9 @@ # Apartment can support many different "Elevators" that can take care of this routing to your data. # Require whichever Elevator you're using below or none if you have a custom one. # -# require "apartment/elevators/generic" +require "apartment/elevators/generic" # require "apartment/elevators/domain" -require "apartment/elevators/subdomain" +# require "apartment/elevators/subdomain" # require "apartment/elevators/first_subdomain" # require "apartment/elevators/host" @@ -12,6 +12,7 @@ require "apartment/elevators/subdomain" # Apartment Configuration # Apartment.configure do |config| + ENV["IGNORE_EMPTY_TENANTS"] = "true" if Rails.env.test? || Rails.application.config.multitenancy.blank? config.seed_after_create = true # Add any models that you do not want to be multi-tenanted, but remain in the global (public) namespace. @@ -106,12 +107,11 @@ end # Setup a custom Tenant switching middleware. The Proc should return the name of the Tenant that # you want to switch to. -# Rails.application.config.middleware.use Apartment::Elevators::Generic, lambda { |request| -# request.host.split(".").first -# } +Rails.application.config.middleware.use Apartment::Elevators::Generic, ->(request) do + Tenant.resolve_host(request.host) +end # Rails.application.config.middleware.use Apartment::Elevators::Domain -Rails.application.config.middleware.use Apartment::Elevators::Subdomain +# Rails.application.config.middleware.use Apartment::Elevators::Subdomain # Rails.application.config.middleware.use Apartment::Elevators::FirstSubdomain # Rails.application.config.middleware.use Apartment::Elevators::Host -Apartment::Elevators::Subdomain.excluded_subdomains = %w[public www] diff --git a/config/secrets.yml.example b/config/secrets.yml.example index dc335a7c5..432ecc587 100644 --- a/config/secrets.yml.example +++ b/config/secrets.yml.example @@ -18,6 +18,7 @@ http_basic_auth: &http_basic_auth development: http_basic_username: "dev" http_basic_password: "pass" + multitenancy: false secret_key_base: 56792feef405a59b18ea7db57b4777e855103882b926413d4afdfb8c0ea8aa86ea6649da4e729c5f5ae324c0ab9338f789174cf48c544173bc18fdc3b14262e4 <<: *maps @@ -48,6 +49,7 @@ staging: http_basic_password: "" managers_url: "" managers_application_key: "" + multitenancy: false <<: *maps <<: *apis @@ -74,6 +76,7 @@ preproduction: http_basic_password: "" managers_url: "" managers_application_key: "" + multitenancy: false twitter_key: "" twitter_secret: "" facebook_key: "" @@ -105,6 +108,7 @@ production: http_basic_password: "" managers_url: "" managers_application_key: "" + multitenancy: false twitter_key: "" twitter_secret: "" facebook_key: "" diff --git a/spec/models/tenant_spec.rb b/spec/models/tenant_spec.rb index f22ef0d46..5a7194eb7 100644 --- a/spec/models/tenant_spec.rb +++ b/spec/models/tenant_spec.rb @@ -1,6 +1,138 @@ require "rails_helper" describe Tenant do + describe ".resolve_host" do + before do + allow(ActionMailer::Base).to receive(:default_url_options).and_return({ host: "consul.dev" }) + end + + it "returns nil for empty hosts" do + expect(Tenant.resolve_host("")).to be nil + expect(Tenant.resolve_host(nil)).to be nil + end + + it "returns nil for IP addresses" do + expect(Tenant.resolve_host("127.0.0.1")).to be nil + end + + it "returns nil using development and test domains" do + expect(Tenant.resolve_host("localhost")).to be nil + expect(Tenant.resolve_host("lvh.me")).to be nil + expect(Tenant.resolve_host("example.com")).to be nil + expect(Tenant.resolve_host("www.example.com")).to be nil + end + + it "treats lvh.me as localhost" do + expect(Tenant.resolve_host("jupiter.lvh.me")).to eq "jupiter" + expect(Tenant.resolve_host("www.lvh.me")).to be nil + end + + it "returns nil for the default host" do + expect(Tenant.resolve_host("consul.dev")).to be nil + end + + it "ignores the www prefix" do + expect(Tenant.resolve_host("www.consul.dev")).to be nil + end + + it "returns subdomains when present" do + expect(Tenant.resolve_host("saturn.consul.dev")).to eq "saturn" + end + + it "ignores the www prefix when subdomains are present" do + expect(Tenant.resolve_host("www.saturn.consul.dev")).to eq "saturn" + end + + it "returns nested additional subdomains" do + expect(Tenant.resolve_host("europa.jupiter.consul.dev")).to eq "europa.jupiter" + end + + it "ignores the www prefix in additional nested subdomains" do + expect(Tenant.resolve_host("www.europa.jupiter.consul.dev")).to eq "europa.jupiter" + end + + it "does not ignore www if it isn't the prefix" do + expect(Tenant.resolve_host("wwwsaturn.consul.dev")).to eq "wwwsaturn" + expect(Tenant.resolve_host("saturn.www.consul.dev")).to eq "saturn.www" + end + + it "returns the host as a subdomain" do + expect(Tenant.resolve_host("consul.dev.consul.dev")).to eq "consul.dev" + end + + it "returns nested subdomains containing the host" do + expect(Tenant.resolve_host("saturn.consul.dev.consul.dev")).to eq "saturn.consul.dev" + end + + it "returns full domains when they don't contain the host" do + expect(Tenant.resolve_host("unrelated.dev")).to eq "unrelated.dev" + expect(Tenant.resolve_host("mercury.anotherconsul.dev")).to eq "mercury.anotherconsul.dev" + end + + it "ignores the www prefix in full domains" do + expect(Tenant.resolve_host("www.unrelated.dev")).to eq "unrelated.dev" + expect(Tenant.resolve_host("www.mercury.anotherconsul.dev")).to eq "mercury.anotherconsul.dev" + end + + context "multitenancy disabled" do + before { allow(Rails.application.config).to receive(:multitenancy).and_return(false) } + + it "always returns nil" do + expect(Tenant.resolve_host("saturn.consul.dev")).to be nil + expect(Tenant.resolve_host("jupiter.lvh.me")).to be nil + end + end + + context "default host contains subdomains" do + before do + allow(ActionMailer::Base).to receive(:default_url_options).and_return({ host: "demo.consul.dev" }) + end + + it "ignores subdomains already present in the default host" do + expect(Tenant.resolve_host("demo.consul.dev")).to be nil + end + + it "ignores the www prefix" do + expect(Tenant.resolve_host("www.demo.consul.dev")).to be nil + end + + it "returns additional subdomains" do + expect(Tenant.resolve_host("saturn.demo.consul.dev")).to eq "saturn" + end + + it "ignores the www prefix in additional subdomains" do + expect(Tenant.resolve_host("www.saturn.demo.consul.dev")).to eq "saturn" + end + + it "returns nested additional subdomains" do + expect(Tenant.resolve_host("europa.jupiter.demo.consul.dev")).to eq "europa.jupiter" + end + + it "ignores the www prefix in additional nested subdomains" do + expect(Tenant.resolve_host("www.europa.jupiter.demo.consul.dev")).to eq "europa.jupiter" + end + + it "does not ignore www if it isn't the prefix" do + expect(Tenant.resolve_host("wwwsaturn.demo.consul.dev")).to eq "wwwsaturn" + expect(Tenant.resolve_host("saturn.www.demo.consul.dev")).to eq "saturn.www" + end + end + + context "default host is similar to development and test domains" do + before do + allow(ActionMailer::Base).to receive(:default_url_options).and_return({ host: "mylvh.me" }) + end + + it "returns nil for the default host" do + expect(Tenant.resolve_host("mylvh.me")).to be nil + end + + it "returns subdomains when present" do + expect(Tenant.resolve_host("neptune.mylvh.me")).to eq "neptune" + end + end + end + describe "validations" do let(:tenant) { build(:tenant) } From 275ddb08d8474c01bb9904bc93d41df61a7a7f3a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 24 Sep 2022 15:14:12 +0200 Subject: [PATCH 06/33] Consider the current tenant in email URLs --- app/mailers/application_mailer.rb | 18 +++++++++++ app/models/tenant.rb | 8 +++++ spec/mailers/application_mailer_spec.rb | 40 +++++++++++++++++++++++++ 3 files changed, 66 insertions(+) diff --git a/app/mailers/application_mailer.rb b/app/mailers/application_mailer.rb index ed9e0b7b2..76541a06c 100644 --- a/app/mailers/application_mailer.rb +++ b/app/mailers/application_mailer.rb @@ -6,7 +6,25 @@ class ApplicationMailer < ActionMailer::Base layout "mailer" before_action :set_asset_host + def default_url_options + if Tenant.default? + super + else + super.merge(host: host_with_subdomain_for(super[:host])) + end + end + def set_asset_host self.asset_host ||= root_url.delete_suffix("/") end + + private + + def host_with_subdomain_for(host) + if host == "localhost" + "#{Tenant.current_schema}.lvh.me" + else + "#{Tenant.current_schema}.#{host}" + end + end end diff --git a/app/models/tenant.rb b/app/models/tenant.rb index d343f8a61..dfc2c1d0c 100644 --- a/app/models/tenant.rb +++ b/app/models/tenant.rb @@ -31,6 +31,14 @@ class Tenant < ApplicationRecord ActionMailer::Base.default_url_options[:host] end + def self.current_schema + Apartment::Tenant.current + end + + def self.default? + current_schema == "public" + end + def self.switch(...) Apartment::Tenant.switch(...) end diff --git a/spec/mailers/application_mailer_spec.rb b/spec/mailers/application_mailer_spec.rb index 6a189c235..665dcca53 100644 --- a/spec/mailers/application_mailer_spec.rb +++ b/spec/mailers/application_mailer_spec.rb @@ -1,6 +1,28 @@ require "rails_helper" describe ApplicationMailer do + describe "#default_url_options" do + it "returns the same options on the default tenant" do + allow(ActionMailer::Base).to receive(:default_url_options).and_return({ host: "consul.dev" }) + + expect(ApplicationMailer.new.default_url_options).to eq({ host: "consul.dev" }) + end + + it "returns the host with a subdomain on other tenants" do + allow(ActionMailer::Base).to receive(:default_url_options).and_return({ host: "consul.dev" }) + allow(Tenant).to receive(:current_schema).and_return("my") + + expect(ApplicationMailer.new.default_url_options).to eq({ host: "my.consul.dev" }) + end + + it "uses lvh.me for subdomains when the host is localhost" do + allow(ActionMailer::Base).to receive(:default_url_options).and_return({ host: "localhost", port: 3000 }) + allow(Tenant).to receive(:current_schema).and_return("dev") + + expect(ApplicationMailer.new.default_url_options).to eq({ host: "dev.lvh.me", port: 3000 }) + end + end + describe "#set_asset_host" do let(:mailer) { ApplicationMailer.new } @@ -24,6 +46,24 @@ describe ApplicationMailer do expect(mailer.asset_host).to eq "https://localhost:3000" end + it "returns the host with a subdomain on other tenants" do + allow(ActionMailer::Base).to receive(:default_url_options).and_return(host: "consul.dev") + allow(Tenant).to receive(:current_schema).and_return("my") + + mailer.set_asset_host + + expect(mailer.asset_host).to eq "http://my.consul.dev" + end + + it "uses lvh.me for subdomains when the host is localhost" do + allow(ActionMailer::Base).to receive(:default_url_options).and_return(host: "localhost", port: 3000) + allow(Tenant).to receive(:current_schema).and_return("dev") + + mailer.set_asset_host + + expect(mailer.asset_host).to eq "http://dev.lvh.me:3000" + end + it "returns the asset host when set manually" do default_asset_host = ActionMailer::Base.asset_host From 52ebeb7ba6c5e88435b2522d998f510677b7da81 Mon Sep 17 00:00:00 2001 From: Eduardo Vilar Date: Wed, 20 Feb 2019 18:45:48 +0100 Subject: [PATCH 07/33] Consider the current tenant with delayed jobs --- config/initializers/apartment.rb | 2 +- config/initializers/delayed_job_config.rb | 12 ++++++++++++ ...0190214103106_add_tenant_to_delayed_job.rb | 5 +++++ db/schema.rb | 1 + spec/mailers/mailer_spec.rb | 19 +++++++++++++++++++ 5 files changed, 38 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20190214103106_add_tenant_to_delayed_job.rb diff --git a/config/initializers/apartment.rb b/config/initializers/apartment.rb index ad022b372..e13c3812a 100644 --- a/config/initializers/apartment.rb +++ b/config/initializers/apartment.rb @@ -18,7 +18,7 @@ Apartment.configure do |config| # Add any models that you do not want to be multi-tenanted, but remain in the global (public) namespace. # A typical example would be a Customer or Tenant model that stores each Tenant's information. # - config.excluded_models = %w[Tenant] + config.excluded_models = %w[Delayed::Job Tenant] # In order to migrate all of your Tenants you need to provide a list of Tenant names to Apartment. # You can make this dynamic by providing a Proc object to be called on migrations. diff --git a/config/initializers/delayed_job_config.rb b/config/initializers/delayed_job_config.rb index 3bcbfa4a1..ce3c6e201 100644 --- a/config/initializers/delayed_job_config.rb +++ b/config/initializers/delayed_job_config.rb @@ -14,3 +14,15 @@ Delayed::Worker.read_ahead = 10 Delayed::Worker.default_queue_name = "default" Delayed::Worker.raise_signal_exceptions = :term Delayed::Worker.logger = Logger.new(File.join(Rails.root, "log", "delayed_job.log")) + +class ApartmentDelayedJobPlugin < Delayed::Plugin + callbacks do |lifecycle| + lifecycle.before(:enqueue) { |job| job.tenant = Tenant.current_schema } + + lifecycle.around(:perform) do |worker, job, *args, &block| + Tenant.switch(job.tenant) { block.call(worker, job, *args) } + end + end +end + +Delayed::Worker.plugins << ApartmentDelayedJobPlugin diff --git a/db/migrate/20190214103106_add_tenant_to_delayed_job.rb b/db/migrate/20190214103106_add_tenant_to_delayed_job.rb new file mode 100644 index 000000000..5e9ec4388 --- /dev/null +++ b/db/migrate/20190214103106_add_tenant_to_delayed_job.rb @@ -0,0 +1,5 @@ +class AddTenantToDelayedJob < ActiveRecord::Migration[4.2] + def change + add_column :delayed_jobs, :tenant, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index 429b1b9fc..5ff1ee3d6 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -561,6 +561,7 @@ ActiveRecord::Schema.define(version: 2022_09_15_154808) do t.string "queue" t.datetime "created_at" t.datetime "updated_at" + t.string "tenant" t.index ["priority", "run_at"], name: "delayed_jobs_priority" end diff --git a/spec/mailers/mailer_spec.rb b/spec/mailers/mailer_spec.rb index 21231ca4d..3db6083c8 100644 --- a/spec/mailers/mailer_spec.rb +++ b/spec/mailers/mailer_spec.rb @@ -51,4 +51,23 @@ describe Mailer do expect(user.subscriptions_token).to eq "subscriptions_token_value" end end + + describe "multitenancy" do + it "uses the current tenant when using delayed jobs", :delay_jobs do + allow(ActionMailer::Base).to receive(:default_url_options).and_return({ host: "consul.dev" }) + create(:tenant, schema: "delay") + + Tenant.switch("delay") do + Setting["org_name"] = "Delayed tenant" + + Mailer.delay.user_invite("test@consul.dev") + end + + Delayed::Worker.new.work_off + body = ActionMailer::Base.deliveries.last.body.to_s + expect(body).to match "Delayed tenant" + expect(body).to match "href=\"http://delay.consul.dev/" + expect(body).to match "src=\"http://delay.consul.dev/" + end + end end From e93b693f717865f876e126ca367d0a63826652e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 28 Sep 2022 13:28:31 +0200 Subject: [PATCH 08/33] Use different cache namespaces for different tenants Since records in different tenants can have the same ID, they can share the same `cache_key`, and so we need a namespace to differentiate them. Without them, records from one tenant could expire the cache of a record from another tenant. --- config/environments/development.rb | 2 +- config/environments/production.rb | 2 +- config/environments/staging.rb | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/config/environments/development.rb b/config/environments/development.rb index 925734721..13479dc34 100644 --- a/config/environments/development.rb +++ b/config/environments/development.rb @@ -18,7 +18,7 @@ Rails.application.configure do config.action_controller.perform_caching = true config.action_controller.enable_fragment_cache_logging = true - config.cache_store = :memory_store + config.cache_store = :memory_store, { namespace: proc { Tenant.current_schema }} config.public_file_server.headers = { "Cache-Control" => "public, max-age=#{2.days.to_i}" } diff --git a/config/environments/production.rb b/config/environments/production.rb index 2eb1a3cdd..47c8ac22a 100644 --- a/config/environments/production.rb +++ b/config/environments/production.rb @@ -58,7 +58,7 @@ Rails.application.configure do config.log_tags = [:request_id] # Use a different cache store in production. - config.cache_store = :mem_cache_store + config.cache_store = :mem_cache_store, { namespace: proc { Tenant.current_schema }} # Use a real queuing backend for Active Job (and separate queues per environment). # config.active_job.queue_adapter = :resque diff --git a/config/environments/staging.rb b/config/environments/staging.rb index 1d947e064..87c57f36d 100644 --- a/config/environments/staging.rb +++ b/config/environments/staging.rb @@ -58,7 +58,7 @@ Rails.application.configure do config.log_tags = [:request_id] # Use a different cache store in production. - config.cache_store = :mem_cache_store + config.cache_store = :mem_cache_store, { namespace: proc { Tenant.current_schema }} # Use a real queuing backend for Active Job (and separate queues per environment). # config.active_job.queue_adapter = :resque From 7189a2368e0d8131432a6221bf27ae2e11afe8b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 2 Oct 2022 17:04:54 +0200 Subject: [PATCH 09/33] Drop all tenant schemas before running db:dev_seed When running this task we truncate all tables; however, doing so doesn't execute the `after_destroy` callback which drops the generated schema. That meant we could run into a situation where there are schemas in the database with no associated tenant, leading to data inconsistencies. So we're now destroying the tenants (alongside their schemas) before truncating the rest of the database tables. --- db/dev_seeds.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/db/dev_seeds.rb b/db/dev_seeds.rb index ebcfeacb5..ff5a658ac 100644 --- a/db/dev_seeds.rb +++ b/db/dev_seeds.rb @@ -1,4 +1,8 @@ -ActiveRecord::Tasks::DatabaseTasks.truncate_all unless Rails.env.test? +unless Rails.env.test? + Tenant.destroy_all + ActiveRecord::Tasks::DatabaseTasks.truncate_all +end + @logger = Logger.new(STDOUT) @logger.formatter = proc do |_severity, _datetime, _progname, msg| msg unless @avoid_log From a98c363d4dfe83d9337c20047aac6af563250df5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 8 Oct 2022 22:10:43 +0200 Subject: [PATCH 10/33] Allow seeding a specific tenant with db:dev_seed Until now, running `db:dev_seed` created development data for the default tenant but it was impossible to create this data for other tenants. Now the tenant can be provided as a parameter. Note that, in order to be able to execute this task twice while running the tests, we need to use `load` instead of `require_relative`, since `require_relative` doesn't load the file again if it's already loaded. Also note that having two optional parameters in a rake task results in a cumbersome syntax to execute it. To avoid this, we're simply removing the `print_log` parameter, which was used mainly for the test environment. Now we use a different logic to get the same result. From now on it won't be possible to pass the option to avoid the log in the development environment. I don't know a developer who's ever used this option, though, and it can always be done using `> /dev/null`. --- db/dev_seeds.rb | 56 ++++++++++++++++++--------------- lib/tasks/db.rake | 5 ++- spec/lib/tasks/dev_seed_spec.rb | 15 ++++++--- 3 files changed, 44 insertions(+), 32 deletions(-) diff --git a/db/dev_seeds.rb b/db/dev_seeds.rb index ff5a658ac..73834c32d 100644 --- a/db/dev_seeds.rb +++ b/db/dev_seeds.rb @@ -1,13 +1,17 @@ unless Rails.env.test? - Tenant.destroy_all + Tenant.destroy_all if Tenant.default? ActiveRecord::Tasks::DatabaseTasks.truncate_all end @logger = Logger.new(STDOUT) @logger.formatter = proc do |_severity, _datetime, _progname, msg| - msg unless @avoid_log + msg unless Rails.env.test? end +def load_dev_seeds(dev_seeds_file) + load Rails.root.join("db", "dev_seeds", "#{dev_seeds_file}.rb") +end + def section(section_title) @logger.info section_title yield @@ -32,28 +36,30 @@ def random_locales_attributes(**attribute_names_with_values) end end -require_relative "dev_seeds/settings" -require_relative "dev_seeds/geozones" -require_relative "dev_seeds/users" -require_relative "dev_seeds/tags_categories" -require_relative "dev_seeds/debates" -require_relative "dev_seeds/proposals" -require_relative "dev_seeds/budgets" -require_relative "dev_seeds/comments" -require_relative "dev_seeds/votes" -require_relative "dev_seeds/flags" -require_relative "dev_seeds/hiddings" -require_relative "dev_seeds/banners" -require_relative "dev_seeds/polls" -require_relative "dev_seeds/communities" -require_relative "dev_seeds/legislation_processes" -require_relative "dev_seeds/newsletters" -require_relative "dev_seeds/notifications" -require_relative "dev_seeds/widgets" -require_relative "dev_seeds/admin_notifications" -require_relative "dev_seeds/legislation_proposals" -require_relative "dev_seeds/milestones" -require_relative "dev_seeds/pages" -require_relative "dev_seeds/sdg" +log "Creating dev seeds for tenant #{Tenant.current_schema}" unless Tenant.default? + +load_dev_seeds "settings" +load_dev_seeds "geozones" +load_dev_seeds "users" +load_dev_seeds "tags_categories" +load_dev_seeds "debates" +load_dev_seeds "proposals" +load_dev_seeds "budgets" +load_dev_seeds "comments" +load_dev_seeds "votes" +load_dev_seeds "flags" +load_dev_seeds "hiddings" +load_dev_seeds "banners" +load_dev_seeds "polls" +load_dev_seeds "communities" +load_dev_seeds "legislation_processes" +load_dev_seeds "newsletters" +load_dev_seeds "notifications" +load_dev_seeds "widgets" +load_dev_seeds "admin_notifications" +load_dev_seeds "legislation_proposals" +load_dev_seeds "milestones" +load_dev_seeds "pages" +load_dev_seeds "sdg" log "All dev seeds created successfuly 👍" diff --git a/lib/tasks/db.rake b/lib/tasks/db.rake index 22597433a..7b38838fa 100644 --- a/lib/tasks/db.rake +++ b/lib/tasks/db.rake @@ -1,9 +1,8 @@ namespace :db do desc "Resets the database and loads it from db/dev_seeds.rb" - task :dev_seed, [:print_log] => [:environment] do |t, args| - @avoid_log = args[:print_log] == "avoid_log" + task :dev_seed, [:tenant] => [:environment] do |_, args| I18n.enforce_available_locales = false - load(Rails.root.join("db", "dev_seeds.rb")) + Tenant.switch(args[:tenant]) { load(Rails.root.join("db", "dev_seeds.rb")) } end desc "Calculates the TSV column for all comments and proposal notifications" diff --git a/spec/lib/tasks/dev_seed_spec.rb b/spec/lib/tasks/dev_seed_spec.rb index 71c816df3..bb2ac5994 100644 --- a/spec/lib/tasks/dev_seed_spec.rb +++ b/spec/lib/tasks/dev_seed_spec.rb @@ -1,11 +1,18 @@ require "rails_helper" describe "rake db:dev_seed" do - let :run_rake_task do - Rake.application.invoke_task("db:dev_seed[avoid_log]") - end + before { Rake::Task["db:dev_seed"].reenable } it "seeds the database without errors" do - expect { run_rake_task }.not_to raise_error + expect { Rake.application.invoke_task("db:dev_seed") }.not_to raise_error + end + + it "can seed a tenant" do + create(:tenant, schema: "democracy") + + Rake.application.invoke_task("db:dev_seed[democracy]") + + expect(Debate.count).to eq 0 + Tenant.switch("democracy") { expect(Debate.count).not_to eq 0 } end end From 796214528ec1fe29ab98ba949b49b281f2f008e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 8 Oct 2022 21:24:15 +0200 Subject: [PATCH 11/33] Send emails to current budget authors in rake tasks We were using `Budget.last`, but the last budget might not be published yet. I must admit I don't know whether these tasks are useful, but I'm not removing them because I'm not sure that won't harm any CONSUL installations. --- lib/tasks/budgets.rake | 4 ++-- spec/lib/tasks/budgets_spec.rb | 31 +++++++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-) create mode 100644 spec/lib/tasks/budgets_spec.rb diff --git a/lib/tasks/budgets.rake b/lib/tasks/budgets.rake index 172944420..008b29bef 100644 --- a/lib/tasks/budgets.rake +++ b/lib/tasks/budgets.rake @@ -2,12 +2,12 @@ namespace :budgets do namespace :email do desc "Sends emails to authors of selected investments" task selected: :environment do - Budget.last.email_selected + Budget.current.email_selected end desc "Sends emails to authors of unselected investments" task unselected: :environment do - Budget.last.email_unselected + Budget.current.email_unselected end end end diff --git a/spec/lib/tasks/budgets_spec.rb b/spec/lib/tasks/budgets_spec.rb new file mode 100644 index 000000000..c58c24ff9 --- /dev/null +++ b/spec/lib/tasks/budgets_spec.rb @@ -0,0 +1,31 @@ +require "rails_helper" + +describe "budget tasks" do + describe "rake budgets:email:selected" do + before { Rake::Task["budgets:email:selected"].reenable } + + it "sends emails to users from the current budget and not the last budget created" do + create(:budget_investment, :selected, author: create(:user, email: "selectme@consul.dev")) + create(:budget, :drafting) + + Rake.application.invoke_task("budgets:email:selected") + + expect(ActionMailer::Base.deliveries.count).to eq 1 + expect(ActionMailer::Base.deliveries.last.to).to eq ["selectme@consul.dev"] + end + end + + describe "rake budgets:email:unselected" do + before { Rake::Task["budgets:email:unselected"].reenable } + + it "sends emails to users from the current budget and not the last budget created" do + create(:budget_investment, author: create(:user, email: "ignorme@consul.dev")) + create(:budget, :drafting) + + Rake.application.invoke_task("budgets:email:unselected") + + expect(ActionMailer::Base.deliveries.count).to eq 1 + expect(ActionMailer::Base.deliveries.last.to).to eq ["ignorme@consul.dev"] + end + end +end From fe9463cb5fda41dd418c04eb7494884ad3897c9a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 8 Oct 2022 21:51:18 +0200 Subject: [PATCH 12/33] Allow specifying the tenant in budget tasks The `budgets:email:selected` and `budgets:email:unselected` tasks are supposed to be run manually because they only make sense at a specific point during the life of a budget. However, they would only run on the default tenant, and it was impossible to run them on a different tenant. So we're introducing an argument in the rake task accepting the name of the tenant whose users we want to send emails to. --- lib/tasks/budgets.rake | 8 ++++---- spec/lib/tasks/budgets_spec.rb | 28 ++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/lib/tasks/budgets.rake b/lib/tasks/budgets.rake index 008b29bef..7143e1e3e 100644 --- a/lib/tasks/budgets.rake +++ b/lib/tasks/budgets.rake @@ -1,13 +1,13 @@ namespace :budgets do namespace :email do desc "Sends emails to authors of selected investments" - task selected: :environment do - Budget.current.email_selected + task :selected, [:tenant] => :environment do |_, args| + Tenant.switch(args[:tenant]) { Budget.current.email_selected } end desc "Sends emails to authors of unselected investments" - task unselected: :environment do - Budget.current.email_unselected + task :unselected, [:tenant] => :environment do |_, args| + Tenant.switch(args[:tenant]) { Budget.current.email_unselected } end end end diff --git a/spec/lib/tasks/budgets_spec.rb b/spec/lib/tasks/budgets_spec.rb index c58c24ff9..ff4419440 100644 --- a/spec/lib/tasks/budgets_spec.rb +++ b/spec/lib/tasks/budgets_spec.rb @@ -13,6 +13,20 @@ describe "budget tasks" do expect(ActionMailer::Base.deliveries.count).to eq 1 expect(ActionMailer::Base.deliveries.last.to).to eq ["selectme@consul.dev"] end + + it "accepts specifying the tenant" do + create(:budget_investment, :selected, author: create(:user, email: "default@consul.dev")) + create(:tenant, schema: "different") + + Tenant.switch("different") do + create(:budget_investment, :selected, author: create(:user, email: "different@consul.dev")) + end + + Rake.application.invoke_task("budgets:email:selected[different]") + + expect(ActionMailer::Base.deliveries.count).to eq 1 + expect(ActionMailer::Base.deliveries.last.to).to eq ["different@consul.dev"] + end end describe "rake budgets:email:unselected" do @@ -27,5 +41,19 @@ describe "budget tasks" do expect(ActionMailer::Base.deliveries.count).to eq 1 expect(ActionMailer::Base.deliveries.last.to).to eq ["ignorme@consul.dev"] end + + it "accepts specifying the tenant" do + create(:budget_investment, author: create(:user, email: "default@consul.dev")) + create(:tenant, schema: "different") + + Tenant.switch("different") do + create(:budget_investment, author: create(:user, email: "different@consul.dev")) + end + + Rake.application.invoke_task("budgets:email:unselected[different]") + + expect(ActionMailer::Base.deliveries.count).to eq 1 + expect(ActionMailer::Base.deliveries.last.to).to eq ["different@consul.dev"] + end end end From 9759288f3bf4d57b76327b9d7dc07ee0c4e27fa0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 24 Sep 2022 19:28:13 +0200 Subject: [PATCH 13/33] Run DB rake tasks on all tenants Some tasks don't have to run on every tenant. The task to calculate the TSV is only done for records which were present before we added the TSV column, and that isn't going to happen in any tenants because we added the TSV column before adding the tenants table. Similarly, the migration needed for existing polls isn't necessary because there weren't any tenants before we allowed to set the starting/ending time to polls. We aren't adding any tests for these tasks because tests for rake tasks are slow and tests creating tenants are also slow, making the combination of the two even slower, particularly if we add tests for every single task we're changing. We're adding tests for the `.run_on_each` method instead. --- app/models/tenant.rb | 6 ++++++ lib/tasks/files.rake | 8 +++++--- lib/tasks/settings.rake | 2 +- lib/tasks/stats.rake | 25 +++++++++++++++---------- lib/tasks/votes.rake | 9 ++++++--- spec/models/tenant_spec.rb | 21 +++++++++++++++++++++ 6 files changed, 54 insertions(+), 17 deletions(-) diff --git a/app/models/tenant.rb b/app/models/tenant.rb index dfc2c1d0c..6e82f3e9a 100644 --- a/app/models/tenant.rb +++ b/app/models/tenant.rb @@ -43,6 +43,12 @@ class Tenant < ApplicationRecord Apartment::Tenant.switch(...) end + def self.run_on_each(&block) + ["public"].union(Apartment.tenant_names).each do |schema| + switch(schema, &block) + end + end + private def create_schema diff --git a/lib/tasks/files.rake b/lib/tasks/files.rake index 978b5f4f1..929eaa951 100644 --- a/lib/tasks/files.rake +++ b/lib/tasks/files.rake @@ -1,8 +1,10 @@ namespace :files do desc "Removes cached attachments which weren't deleted for some reason" task remove_old_cached_attachments: :environment do - ActiveStorage::Blob.unattached - .where("active_storage_blobs.created_at <= ?", 1.day.ago) - .find_each(&:purge_later) + Tenant.run_on_each do + ActiveStorage::Blob.unattached + .where("active_storage_blobs.created_at <= ?", 1.day.ago) + .find_each(&:purge_later) + end end end diff --git a/lib/tasks/settings.rake b/lib/tasks/settings.rake index 11ac39c4b..f98aca240 100644 --- a/lib/tasks/settings.rake +++ b/lib/tasks/settings.rake @@ -2,7 +2,7 @@ namespace :settings do desc "Add new settings" task add_new_settings: :environment do ApplicationLogger.new.info "Adding new settings" - Setting.add_new_settings + Tenant.run_on_each { Setting.add_new_settings } end desc "Rename existing settings" diff --git a/lib/tasks/stats.rake b/lib/tasks/stats.rake index 66456a0b5..b7d397301 100644 --- a/lib/tasks/stats.rake +++ b/lib/tasks/stats.rake @@ -2,23 +2,28 @@ namespace :stats do desc "Generates stats which are not cached yet" task generate: :environment do ApplicationLogger.new.info "Updating budget and poll stats" - admin_ability = Ability.new(Administrator.first.user) - Budget.accessible_by(admin_ability, :read_stats).find_each do |budget| - Budget::Stats.new(budget).generate - print "." - end + Tenant.run_on_each do + admin_ability = Ability.new(Administrator.first.user) - Poll.accessible_by(admin_ability, :stats).find_each do |poll| - Poll::Stats.new(poll).generate - print "." + Budget.accessible_by(admin_ability, :read_stats).find_each do |budget| + Budget::Stats.new(budget).generate + print "." + end + + Poll.accessible_by(admin_ability, :stats).find_each do |poll| + Poll::Stats.new(poll).generate + print "." + end end end desc "Expires stats cache" task expire_cache: :environment do - [Budget, Poll].each do |model_class| - model_class.find_each { |record| record.find_or_create_stats_version.touch } + Tenant.run_on_each do + [Budget, Poll].each do |model_class| + model_class.find_each { |record| record.find_or_create_stats_version.touch } + end end end diff --git a/lib/tasks/votes.rake b/lib/tasks/votes.rake index 59e809cd9..d86bdb4c5 100644 --- a/lib/tasks/votes.rake +++ b/lib/tasks/votes.rake @@ -5,9 +5,12 @@ namespace :votes do models.each do |model| print "Updating votes hot_score for #{model}s" - model.find_each do |resource| - new_hot_score = resource.calculate_hot_score - resource.update_columns(hot_score: new_hot_score, updated_at: Time.current) + + Tenant.run_on_each do + model.find_each do |resource| + new_hot_score = resource.calculate_hot_score + resource.update_columns(hot_score: new_hot_score, updated_at: Time.current) + end end puts " ✅ " end diff --git a/spec/models/tenant_spec.rb b/spec/models/tenant_spec.rb index 5a7194eb7..c8b979023 100644 --- a/spec/models/tenant_spec.rb +++ b/spec/models/tenant_spec.rb @@ -133,6 +133,27 @@ describe Tenant do end end + describe ".run_on_each" do + it "runs the code on all tenants, including the default one" do + create(:tenant, schema: "andromeda") + create(:tenant, schema: "milky-way") + + Tenant.run_on_each do + Setting["org_name"] = "oh-my-#{Tenant.current_schema}" + end + + expect(Setting["org_name"]).to eq "oh-my-public" + + Tenant.switch("andromeda") do + expect(Setting["org_name"]).to eq "oh-my-andromeda" + end + + Tenant.switch("milky-way") do + expect(Setting["org_name"]).to eq "oh-my-milky-way" + end + end + end + describe "validations" do let(:tenant) { build(:tenant) } From 22ffbd4d2b92400a13f71985e9165a6251086edf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 26 Sep 2022 22:00:15 +0200 Subject: [PATCH 14/33] Remove redundant tests in sitemap specs Testing that the sitemap is valid (which we test in the following test) also checks that the sitemap has been generated. The test will also fail with different errors depending on whether no file was generated or the generated file is invalid. --- spec/lib/tasks/sitemap_spec.rb | 8 -------- 1 file changed, 8 deletions(-) diff --git a/spec/lib/tasks/sitemap_spec.rb b/spec/lib/tasks/sitemap_spec.rb index dc2fa7edd..dd6641063 100644 --- a/spec/lib/tasks/sitemap_spec.rb +++ b/spec/lib/tasks/sitemap_spec.rb @@ -11,10 +11,6 @@ describe "rake sitemap:create", type: :system do describe "when processes are enabled" do before { Rake.application.invoke_task("sitemap:create") } - it "generates a sitemap" do - expect(file).to exist - end - it "generates a valid sitemap" do sitemap = Nokogiri::XML(File.open(file)) expect(sitemap.errors).to be_empty @@ -51,10 +47,6 @@ describe "rake sitemap:create", type: :system do Rake.application.invoke_task("sitemap:create") end - it "generates a sitemap" do - expect(file).to exist - end - it "generates a valid sitemap" do sitemap = Nokogiri::XML(File.open(file)) expect(sitemap.errors).to be_empty From cc87bca50053774879a35e07273d54c76afedd5a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 26 Sep 2022 22:31:20 +0200 Subject: [PATCH 15/33] Use have_content in sitemap tests That's what we're using in most of our tests, and we were using it in some expectations in these tests as well. --- spec/lib/tasks/sitemap_spec.rb | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/spec/lib/tasks/sitemap_spec.rb b/spec/lib/tasks/sitemap_spec.rb index dd6641063..0edbf19f4 100644 --- a/spec/lib/tasks/sitemap_spec.rb +++ b/spec/lib/tasks/sitemap_spec.rb @@ -20,16 +20,16 @@ describe "rake sitemap:create", type: :system do sitemap = File.read(file) # Static pages - expect(sitemap).to include(faq_path) - expect(sitemap).to include(help_path) - expect(sitemap).to include(how_to_use_path) + expect(sitemap).to have_content(faq_path) + expect(sitemap).to have_content(help_path) + expect(sitemap).to have_content(how_to_use_path) # Dynamic URLs - expect(sitemap).to include(polls_path) - expect(sitemap).to include(budgets_path) - expect(sitemap).to include(debates_path) - expect(sitemap).to include(proposals_path) - expect(sitemap).to include(legislation_processes_path) + expect(sitemap).to have_content(polls_path) + expect(sitemap).to have_content(budgets_path) + expect(sitemap).to have_content(debates_path) + expect(sitemap).to have_content(proposals_path) + expect(sitemap).to have_content(legislation_processes_path) expect(sitemap).to have_content("0.7", count: 5) expect(sitemap).to have_content("daily", count: 5) @@ -56,16 +56,16 @@ describe "rake sitemap:create", type: :system do sitemap = File.read(file) # Static pages - expect(sitemap).to include(faq_path) - expect(sitemap).to include(help_path) - expect(sitemap).to include(how_to_use_path) + expect(sitemap).to have_content(faq_path) + expect(sitemap).to have_content(help_path) + expect(sitemap).to have_content(how_to_use_path) # Dynamic URLs - expect(sitemap).not_to include(polls_path) - expect(sitemap).not_to include(budgets_path) - expect(sitemap).not_to include(debates_path) - expect(sitemap).not_to include(proposals_path) - expect(sitemap).not_to include(legislation_processes_path) + expect(sitemap).not_to have_content(polls_path) + expect(sitemap).not_to have_content(budgets_path) + expect(sitemap).not_to have_content(debates_path) + expect(sitemap).not_to have_content(proposals_path) + expect(sitemap).not_to have_content(legislation_processes_path) expect(sitemap).not_to have_content("0.7") expect(sitemap).not_to have_content("daily") From 510088411000af5b31b0f996d2315c1627253184 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 26 Sep 2022 22:20:27 +0200 Subject: [PATCH 16/33] Generate one sitemap per tenant Note that the `sitemap:refresh` task only pings search engines at the end, so it only does so for the `Sitemap.default_host` defined last. So we're using the `sitemap:refresh:no_ping` task instead and pinging search engines after creating each sitemap. Note we're pinging search engines in staging and preproduction environments. I'm leaving it that way because that's what we've done until now, but I wonder whether we should only do so on production. Since we're creating a new method to get the current url_options, we're also using it in the dev_seeds. --- .dockerignore | 1 + .gitignore | 1 + app/models/tenant.rb | 4 ++ config/schedule.rb | 2 +- config/sitemap.rb | 86 ++++++++++++++++------------- db/dev_seeds/admin_notifications.rb | 2 +- spec/lib/tasks/sitemap_spec.rb | 44 ++++++++++++++- 7 files changed, 98 insertions(+), 42 deletions(-) diff --git a/.dockerignore b/.dockerignore index a85b5fb6b..41853f5ae 100644 --- a/.dockerignore +++ b/.dockerignore @@ -20,6 +20,7 @@ storage/ # Files generated by scripts or compiled public/sitemap.xml +public/tenants/*/sitemap.xml public/assets/ public/machine_learning/data/ diff --git a/.gitignore b/.gitignore index 7e10963eb..3ac23e93f 100644 --- a/.gitignore +++ b/.gitignore @@ -22,6 +22,7 @@ tmp/ # Files generated by scripts or compiled /public/sitemap.xml +/public/tenants/*/sitemap.xml /public/assets/ /public/machine_learning/data/ diff --git a/app/models/tenant.rb b/app/models/tenant.rb index 6e82f3e9a..b808de150 100644 --- a/app/models/tenant.rb +++ b/app/models/tenant.rb @@ -35,6 +35,10 @@ class Tenant < ApplicationRecord Apartment::Tenant.current end + def self.current_url_options + ApplicationMailer.new.default_url_options + end + def self.default? current_schema == "public" end diff --git a/config/schedule.rb b/config/schedule.rb index 4e7675dc5..b9e3be61c 100644 --- a/config/schedule.rb +++ b/config/schedule.rb @@ -24,7 +24,7 @@ every 1.minute do end every 1.day, at: "5:00 am" do - rake "-s sitemap:refresh" + rake "-s sitemap:refresh:no_ping" end every 2.hours do diff --git a/config/sitemap.rb b/config/sitemap.rb index 3a2664e55..31f973f2b 100644 --- a/config/sitemap.rb +++ b/config/sitemap.rb @@ -6,49 +6,59 @@ class SitemapGenerator::FileAdapter end end SitemapGenerator::Sitemap.namer = SitemapGenerator::SimpleNamer.new(:sitemap, extension: ".xml") - -# default host SitemapGenerator::Sitemap.verbose = false if Rails.env.test? -SitemapGenerator::Sitemap.default_host = root_url(ActionMailer::Base.default_url_options) -# sitemap generator -SitemapGenerator::Sitemap.create do - add help_path - add how_to_use_path - add faq_path +Tenant.run_on_each do + SitemapGenerator::Sitemap.default_host = root_url(Tenant.current_url_options) - if Setting["process.debates"] - add debates_path, priority: 0.7, changefreq: "daily" - Debate.find_each do |debate| - add debate_path(debate), lastmod: debate.updated_at + if Tenant.default? + SitemapGenerator::Sitemap.sitemaps_path = nil + else + SitemapGenerator::Sitemap.sitemaps_path = "tenants/#{Tenant.current_schema}" + end + + SitemapGenerator::Sitemap.create do + add help_path + add how_to_use_path + add faq_path + + if Setting["process.debates"] + add debates_path, priority: 0.7, changefreq: "daily" + Debate.find_each do |debate| + add debate_path(debate), lastmod: debate.updated_at + end + end + + if Setting["process.proposals"] + add proposals_path, priority: 0.7, changefreq: "daily" + Proposal.find_each do |proposal| + add proposal_path(proposal), lastmod: proposal.updated_at + end + end + + if Setting["process.budgets"] + add budgets_path, priority: 0.7, changefreq: "daily" + Budget.find_each do |budget| + add budget_path(budget), lastmod: budget.updated_at + end + end + + if Setting["process.polls"] + add polls_path, priority: 0.7, changefreq: "daily" + Poll.find_each do |poll| + add poll_path(poll), lastmod: poll.starts_at + end + end + + if Setting["process.legislation"] + add legislation_processes_path, priority: 0.7, changefreq: "daily" + Legislation::Process.find_each do |process| + add legislation_process_path(process), lastmod: process.start_date + end end end - if Setting["process.proposals"] - add proposals_path, priority: 0.7, changefreq: "daily" - Proposal.find_each do |proposal| - add proposal_path(proposal), lastmod: proposal.updated_at - end - end - - if Setting["process.budgets"] - add budgets_path, priority: 0.7, changefreq: "daily" - Budget.find_each do |budget| - add budget_path(budget), lastmod: budget.updated_at - end - end - - if Setting["process.polls"] - add polls_path, priority: 0.7, changefreq: "daily" - Poll.find_each do |poll| - add poll_path(poll), lastmod: poll.starts_at - end - end - - if Setting["process.legislation"] - add legislation_processes_path, priority: 0.7, changefreq: "daily" - Legislation::Process.find_each do |process| - add legislation_process_path(process), lastmod: process.start_date - end + unless Rails.env.development? || Rails.env.test? + SitemapGenerator::Sitemap.ping_search_engines end end diff --git a/db/dev_seeds/admin_notifications.rb b/db/dev_seeds/admin_notifications.rb index e75139c7d..132976cf6 100644 --- a/db/dev_seeds/admin_notifications.rb +++ b/db/dev_seeds/admin_notifications.rb @@ -5,7 +5,7 @@ section "Creating Admin Notifications & Templates" do -> { I18n.t("seeds.admin_notifications.proposal.#{attribute}") } end ).merge( - link: Rails.application.routes.url_helpers.proposals_url(ActionMailer::Base.default_url_options), + link: Rails.application.routes.url_helpers.proposals_url(Tenant.current_url_options), segment_recipient: "administrators" ) ).deliver diff --git a/spec/lib/tasks/sitemap_spec.rb b/spec/lib/tasks/sitemap_spec.rb index 0edbf19f4..be989a92c 100644 --- a/spec/lib/tasks/sitemap_spec.rb +++ b/spec/lib/tasks/sitemap_spec.rb @@ -2,6 +2,7 @@ require "rails_helper" describe "rake sitemap:create", type: :system do let(:file) { Rails.root.join("public", "sitemap.xml") } + let(:run_rake_task) { Rake.application.invoke_task("sitemap:create") } before do FileUtils.rm_f(file) @@ -9,7 +10,7 @@ describe "rake sitemap:create", type: :system do end describe "when processes are enabled" do - before { Rake.application.invoke_task("sitemap:create") } + before { run_rake_task } it "generates a valid sitemap" do sitemap = Nokogiri::XML(File.open(file)) @@ -44,7 +45,7 @@ describe "rake sitemap:create", type: :system do Setting["process.polls"] = nil Setting["process.legislation"] = nil - Rake.application.invoke_task("sitemap:create") + run_rake_task end it "generates a valid sitemap" do @@ -71,4 +72,43 @@ describe "rake sitemap:create", type: :system do expect(sitemap).not_to have_content("daily") end end + + it "generates a sitemap for every tenant" do + allow(ActionMailer::Base).to receive(:default_url_options).and_return({ host: "consul.dev" }) + FileUtils.rm_f(Dir.glob(Rails.root.join("public", "tenants", "*", "sitemap.xml"))) + + create(:tenant, schema: "debates") + create(:tenant, schema: "proposals") + + Setting["process.debates"] = false + Setting["process.proposals"] = false + + Tenant.switch("debates") do + Setting["process.budgets"] = false + Setting["process.proposals"] = false + end + + Tenant.switch("proposals") do + Setting["process.budgets"] = false + Setting["process.debates"] = false + end + + run_rake_task + + public_sitemap = File.read(file) + debates_sitemap = File.read(Rails.root.join("public", "tenants", "debates", "sitemap.xml")) + proposals_sitemap = File.read(Rails.root.join("public", "tenants", "proposals", "sitemap.xml")) + + expect(public_sitemap).to have_content budgets_url(host: "consul.dev") + expect(public_sitemap).not_to have_content debates_path + expect(public_sitemap).not_to have_content proposals_path + + expect(debates_sitemap).to have_content debates_url(host: "debates.consul.dev") + expect(debates_sitemap).not_to have_content budgets_path + expect(debates_sitemap).not_to have_content proposals_path + + expect(proposals_sitemap).to have_content proposals_url(host: "proposals.consul.dev") + expect(proposals_sitemap).not_to have_content budgets_path + expect(proposals_sitemap).not_to have_content debates_path + end end From 468761253b5f6847f58788209e8d24c0bd1301a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 30 Sep 2022 16:04:56 +0200 Subject: [PATCH 17/33] Add per-tenant sitemap to robots.txt file While we ping some search engines (currently, only Google) when generating the sitemap files, we weren't telling search engines accessing through the `robots.txt` file where to find the sitemap. Now we're doing so, using the right sitemap file for the right tenant. --- app/controllers/robots_controller.rb | 7 +++++++ .../views/robots/index.text.erb | 6 ++++++ config/routes.rb | 1 + spec/system/robots_spec.rb | 19 +++++++++++++++++++ 4 files changed, 33 insertions(+) create mode 100644 app/controllers/robots_controller.rb rename public/robots.txt => app/views/robots/index.text.erb (69%) create mode 100644 spec/system/robots_spec.rb diff --git a/app/controllers/robots_controller.rb b/app/controllers/robots_controller.rb new file mode 100644 index 000000000..2d445fa01 --- /dev/null +++ b/app/controllers/robots_controller.rb @@ -0,0 +1,7 @@ +class RobotsController < ApplicationController + skip_authorization_check + + def index + respond_to :text + end +end diff --git a/public/robots.txt b/app/views/robots/index.text.erb similarity index 69% rename from public/robots.txt rename to app/views/robots/index.text.erb index 9327c9c1b..8a432d78d 100644 --- a/public/robots.txt +++ b/app/views/robots/index.text.erb @@ -13,3 +13,9 @@ Disallow: /*?*search Disallow: /*?*locale-switcher Disallow: /*?*filter Disallow: user_id + +<% if Tenant.default? %> +Sitemap: <%= "#{root_url}sitemap.xml" %> +<% else %> +Sitemap: <%= "#{root_url}tenants/#{Tenant.current_schema}/sitemap.xml" %> +<% end %> diff --git a/config/routes.rb b/config/routes.rb index d216d1c81..29a4083c6 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -30,6 +30,7 @@ Rails.application.routes.draw do root "welcome#index" get "/welcome", to: "welcome#welcome" get "/consul.json", to: "installation#details" + get "robots.txt", to: "robots#index" resources :stats, only: [:index] resources :images, only: [:destroy] diff --git a/spec/system/robots_spec.rb b/spec/system/robots_spec.rb new file mode 100644 index 000000000..3e728f24b --- /dev/null +++ b/spec/system/robots_spec.rb @@ -0,0 +1,19 @@ +require "rails_helper" + +describe "robots.txt" do + scenario "uses the default sitemap for the default tenant" do + visit "/robots.txt" + + expect(page).to have_content "Sitemap: #{app_host}/sitemap.xml" + end + + scenario "uses a different sitemap for other tenants" do + create(:tenant, schema: "cyborgs") + + with_subdomain("cyborgs") do + visit "/robots.txt" + + expect(page).to have_content "Sitemap: http://cyborgs.lvh.me:#{app_port}/tenants/cyborgs/sitemap.xml" + end + end +end From d904fe8b4fd2cc1562fb0c814a1ee3ea792c84bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 5 Oct 2022 23:37:51 +0200 Subject: [PATCH 18/33] Move subdomain logic to tenant model We had some of the logic in the ApplicationMailer. Since we're going to use this logic in more places, we're moving it to the Tenant model, which is the model handling everything related to hosts. --- app/mailers/application_mailer.rb | 16 +--------------- app/models/tenant.rb | 24 +++++++++++++++++++----- spec/lib/tasks/sitemap_spec.rb | 2 +- spec/models/tenant_spec.rb | 6 +++--- 4 files changed, 24 insertions(+), 24 deletions(-) diff --git a/app/mailers/application_mailer.rb b/app/mailers/application_mailer.rb index 76541a06c..757bc32ff 100644 --- a/app/mailers/application_mailer.rb +++ b/app/mailers/application_mailer.rb @@ -7,24 +7,10 @@ class ApplicationMailer < ActionMailer::Base before_action :set_asset_host def default_url_options - if Tenant.default? - super - else - super.merge(host: host_with_subdomain_for(super[:host])) - end + Tenant.current_url_options end def set_asset_host self.asset_host ||= root_url.delete_suffix("/") end - - private - - def host_with_subdomain_for(host) - if host == "localhost" - "#{Tenant.current_schema}.lvh.me" - else - "#{Tenant.current_schema}.#{host}" - end - end end diff --git a/app/models/tenant.rb b/app/models/tenant.rb index b808de150..59b26591c 100644 --- a/app/models/tenant.rb +++ b/app/models/tenant.rb @@ -27,22 +27,36 @@ class Tenant < ApplicationRecord %w[mail public shared_extensions www] end - def self.default_host - ActionMailer::Base.default_url_options[:host] + def self.default_url_options + ActionMailer::Base.default_url_options end - def self.current_schema - Apartment::Tenant.current + def self.default_host + default_url_options[:host] end def self.current_url_options - ApplicationMailer.new.default_url_options + default_url_options.merge(host: current_host) + end + + def self.current_host + if default? + default_host + elsif default_host == "localhost" + "#{current_schema}.lvh.me" + else + "#{current_schema}.#{default_host}" + end end def self.default? current_schema == "public" end + def self.current_schema + Apartment::Tenant.current + end + def self.switch(...) Apartment::Tenant.switch(...) end diff --git a/spec/lib/tasks/sitemap_spec.rb b/spec/lib/tasks/sitemap_spec.rb index be989a92c..b6cc68c68 100644 --- a/spec/lib/tasks/sitemap_spec.rb +++ b/spec/lib/tasks/sitemap_spec.rb @@ -74,7 +74,7 @@ describe "rake sitemap:create", type: :system do end it "generates a sitemap for every tenant" do - allow(ActionMailer::Base).to receive(:default_url_options).and_return({ host: "consul.dev" }) + allow(Tenant).to receive(:default_url_options).and_return({ host: "consul.dev" }) FileUtils.rm_f(Dir.glob(Rails.root.join("public", "tenants", "*", "sitemap.xml"))) create(:tenant, schema: "debates") diff --git a/spec/models/tenant_spec.rb b/spec/models/tenant_spec.rb index c8b979023..a8f41428f 100644 --- a/spec/models/tenant_spec.rb +++ b/spec/models/tenant_spec.rb @@ -3,7 +3,7 @@ require "rails_helper" describe Tenant do describe ".resolve_host" do before do - allow(ActionMailer::Base).to receive(:default_url_options).and_return({ host: "consul.dev" }) + allow(Tenant).to receive(:default_url_options).and_return({ host: "consul.dev" }) end it "returns nil for empty hosts" do @@ -85,7 +85,7 @@ describe Tenant do context "default host contains subdomains" do before do - allow(ActionMailer::Base).to receive(:default_url_options).and_return({ host: "demo.consul.dev" }) + allow(Tenant).to receive(:default_url_options).and_return({ host: "demo.consul.dev" }) end it "ignores subdomains already present in the default host" do @@ -120,7 +120,7 @@ describe Tenant do context "default host is similar to development and test domains" do before do - allow(ActionMailer::Base).to receive(:default_url_options).and_return({ host: "mylvh.me" }) + allow(Tenant).to receive(:default_url_options).and_return({ host: "mylvh.me" }) end it "returns nil for the default host" do From a71f4d87f881fab6bf939829f08aacdba06f7651 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 25 Sep 2022 16:21:02 +0200 Subject: [PATCH 19/33] Add an interface to manage tenants Note we aren't allowing to delete a tenant because it would delete all its data, so this action is a very dangerous one. We might need to add a warning when creating a tenant, indicating the tenant cannot be destroyed. We can also add an action to delete a tenant which forces the admin to write the name of the tenant before deleting it and with a big warning about the danger of this operation. For now, we're letting administrators of the "main" (default) tenant to create other tenants. However, we're only allowing to manage tenants when the multitenancy configuration option is enabled. This way the interface won't get in the way on single-tenant applications. We've thought about creating a new role to manage tenants or a new URL out of the admin area. We aren't doing so for simplicity purposes and because we want to keep CONSUL working the same way it has for single-tenant installations, but we might change it in the future. There's also the fact that by default we create one user with a known password, and if by default we create a new role and a new user to handle tenants, the chances of people forgetting to change the password of one of these users increases dramatically, particularly if they aren't using multitenancy. --- app/components/admin/menu_component.html.erb | 1 + app/components/admin/menu_component.rb | 15 ++++++- .../admin/tenants/edit_component.html.erb | 3 ++ .../admin/tenants/edit_component.rb | 12 +++++ .../admin/tenants/form_component.html.erb | 7 +++ .../admin/tenants/form_component.rb | 7 +++ .../admin/tenants/index_component.html.erb | 27 +++++++++++ .../admin/tenants/index_component.rb | 18 ++++++++ .../admin/tenants/new_component.html.erb | 3 ++ app/components/admin/tenants/new_component.rb | 12 +++++ app/controllers/admin/tenants_controller.rb | 35 +++++++++++++++ app/models/abilities/administrator.rb | 4 ++ app/models/tenant.rb | 14 ++++-- app/views/admin/tenants/edit.html.erb | 1 + app/views/admin/tenants/index.html.erb | 1 + app/views/admin/tenants/new.html.erb | 1 + config/locales/en/activerecord.yml | 5 +++ config/locales/en/admin.yml | 10 +++++ config/locales/es/activerecord.yml | 5 +++ config/locales/es/admin.yml | 10 +++++ config/routes/admin.rb | 2 + spec/models/abilities/administrator_spec.rb | 33 ++++++++++++++ spec/models/tenant_spec.rb | 20 +++++++++ spec/system/admin/tenants_spec.rb | 45 +++++++++++++++++++ 24 files changed, 286 insertions(+), 5 deletions(-) create mode 100644 app/components/admin/tenants/edit_component.html.erb create mode 100644 app/components/admin/tenants/edit_component.rb create mode 100644 app/components/admin/tenants/form_component.html.erb create mode 100644 app/components/admin/tenants/form_component.rb create mode 100644 app/components/admin/tenants/index_component.html.erb create mode 100644 app/components/admin/tenants/index_component.rb create mode 100644 app/components/admin/tenants/new_component.html.erb create mode 100644 app/components/admin/tenants/new_component.rb create mode 100644 app/controllers/admin/tenants_controller.rb create mode 100644 app/views/admin/tenants/edit.html.erb create mode 100644 app/views/admin/tenants/index.html.erb create mode 100644 app/views/admin/tenants/new.html.erb create mode 100644 spec/system/admin/tenants_spec.rb diff --git a/app/components/admin/menu_component.html.erb b/app/components/admin/menu_component.html.erb index 750ee18ec..8e70d9cb7 100644 --- a/app/components/admin/menu_component.html.erb +++ b/app/components/admin/menu_component.html.erb @@ -110,6 +110,7 @@ <%= t("admin.menu.title_settings") %> <%= link_list( settings_link, + tenants_link, tags_link, geozones_link, images_link, diff --git a/app/components/admin/menu_component.rb b/app/components/admin/menu_component.rb index 4ef5d262f..9fc4c5319 100644 --- a/app/components/admin/menu_component.rb +++ b/app/components/admin/menu_component.rb @@ -1,5 +1,6 @@ class Admin::MenuComponent < ApplicationComponent include LinkListHelper + delegate :can?, to: :helpers private @@ -32,8 +33,8 @@ class Admin::MenuComponent < ApplicationComponent end def settings? - controllers_names = ["settings", "tags", "geozones", "images", "content_blocks", - "local_census_records", "imports"] + controllers_names = ["settings", "tenants", "tags", "geozones", "images", + "content_blocks", "local_census_records", "imports"] controllers_names.include?(controller_name) && controller.class.module_parent != Admin::Poll::Questions::Answers end @@ -300,6 +301,16 @@ class Admin::MenuComponent < ApplicationComponent ] end + def tenants_link + if can?(:read, Tenant) + [ + t("admin.menu.multitenancy"), + admin_tenants_path, + controller_name == "tenants" + ] + end + end + def tags_link [ t("admin.menu.proposals_topics"), diff --git a/app/components/admin/tenants/edit_component.html.erb b/app/components/admin/tenants/edit_component.html.erb new file mode 100644 index 000000000..f83758392 --- /dev/null +++ b/app/components/admin/tenants/edit_component.html.erb @@ -0,0 +1,3 @@ +<%= back_link_to admin_tenants_path %> +<%= header %> +<%= render Admin::Tenants::FormComponent.new(tenant) %> diff --git a/app/components/admin/tenants/edit_component.rb b/app/components/admin/tenants/edit_component.rb new file mode 100644 index 000000000..ee73145d4 --- /dev/null +++ b/app/components/admin/tenants/edit_component.rb @@ -0,0 +1,12 @@ +class Admin::Tenants::EditComponent < ApplicationComponent + include Header + attr_reader :tenant + + def initialize(tenant) + @tenant = tenant + end + + def title + tenant.name + end +end diff --git a/app/components/admin/tenants/form_component.html.erb b/app/components/admin/tenants/form_component.html.erb new file mode 100644 index 000000000..1c01da639 --- /dev/null +++ b/app/components/admin/tenants/form_component.html.erb @@ -0,0 +1,7 @@ +<%= form_for [:admin, tenant] do |f| %> + <%= render "shared/errors", resource: tenant %> + + <%= f.text_field :name %> + <%= f.text_field :schema %> + <%= f.submit %> +<% end %> diff --git a/app/components/admin/tenants/form_component.rb b/app/components/admin/tenants/form_component.rb new file mode 100644 index 000000000..8db70a94b --- /dev/null +++ b/app/components/admin/tenants/form_component.rb @@ -0,0 +1,7 @@ +class Admin::Tenants::FormComponent < ApplicationComponent + attr_reader :tenant + + def initialize(tenant) + @tenant = tenant + end +end diff --git a/app/components/admin/tenants/index_component.html.erb b/app/components/admin/tenants/index_component.html.erb new file mode 100644 index 000000000..005eb377d --- /dev/null +++ b/app/components/admin/tenants/index_component.html.erb @@ -0,0 +1,27 @@ +<%= header do %> + <%= link_to t("admin.tenants.index.create"), new_admin_tenant_path %> +<% end %> + + + + + + + + + + + + <% @tenants.each do |tenant| %> + + + + + + <% end %> + +
<%= attribute_name(:name) %><%= attribute_name(:schema) %><%= t("admin.shared.actions") %>
<%= tenant.name %><%= tenant.schema %> + <%= render Admin::TableActionsComponent.new(tenant, actions: [:edit]) do |actions| %> + <%= actions.action(:show, text: t("admin.shared.view"), path: root_url(host: tenant.host)) %> + <% end %> +
diff --git a/app/components/admin/tenants/index_component.rb b/app/components/admin/tenants/index_component.rb new file mode 100644 index 000000000..eb37a6832 --- /dev/null +++ b/app/components/admin/tenants/index_component.rb @@ -0,0 +1,18 @@ +class Admin::Tenants::IndexComponent < ApplicationComponent + include Header + attr_reader :tenants + + def initialize(tenants) + @tenants = tenants + end + + def title + t("admin.menu.multitenancy") + end + + private + + def attribute_name(attribute) + Tenant.human_attribute_name(attribute) + end +end diff --git a/app/components/admin/tenants/new_component.html.erb b/app/components/admin/tenants/new_component.html.erb new file mode 100644 index 000000000..f83758392 --- /dev/null +++ b/app/components/admin/tenants/new_component.html.erb @@ -0,0 +1,3 @@ +<%= back_link_to admin_tenants_path %> +<%= header %> +<%= render Admin::Tenants::FormComponent.new(tenant) %> diff --git a/app/components/admin/tenants/new_component.rb b/app/components/admin/tenants/new_component.rb new file mode 100644 index 000000000..d0ebacf4d --- /dev/null +++ b/app/components/admin/tenants/new_component.rb @@ -0,0 +1,12 @@ +class Admin::Tenants::NewComponent < ApplicationComponent + include Header + attr_reader :tenant + + def initialize(tenant) + @tenant = tenant + end + + def title + t("admin.tenants.new.title") + end +end diff --git a/app/controllers/admin/tenants_controller.rb b/app/controllers/admin/tenants_controller.rb new file mode 100644 index 000000000..020118ebf --- /dev/null +++ b/app/controllers/admin/tenants_controller.rb @@ -0,0 +1,35 @@ +class Admin::TenantsController < Admin::BaseController + load_and_authorize_resource + + def index + @tenants = @tenants.order(:name) + end + + def new + end + + def edit + end + + def create + if @tenant.save + redirect_to admin_tenants_path, notice: t("admin.tenants.create.notice") + else + render :new + end + end + + def update + if @tenant.update(tenant_params) + redirect_to admin_tenants_path, notice: t("admin.tenants.update.notice") + else + render :edit + end + end + + private + + def tenant_params + params.require(:tenant).permit(:name, :schema) + end +end diff --git a/app/models/abilities/administrator.rb b/app/models/abilities/administrator.rb index f6e2bb038..9e3556e80 100644 --- a/app/models/abilities/administrator.rb +++ b/app/models/abilities/administrator.rb @@ -134,6 +134,10 @@ module Abilities can :manage, LocalCensusRecord can [:create, :read], LocalCensusRecords::Import + + if Rails.application.config.multitenancy && Tenant.default? + can [:create, :read, :update], Tenant + end end end end diff --git a/app/models/tenant.rb b/app/models/tenant.rb index 59b26591c..87b4a2c63 100644 --- a/app/models/tenant.rb +++ b/app/models/tenant.rb @@ -40,12 +40,16 @@ class Tenant < ApplicationRecord end def self.current_host - if default? + host_for(current_schema) + end + + def self.host_for(schema) + if schema == "public" default_host elsif default_host == "localhost" - "#{current_schema}.lvh.me" + "#{schema}.lvh.me" else - "#{current_schema}.#{default_host}" + "#{schema}.#{default_host}" end end @@ -67,6 +71,10 @@ class Tenant < ApplicationRecord end end + def host + self.class.host_for(schema) + end + private def create_schema diff --git a/app/views/admin/tenants/edit.html.erb b/app/views/admin/tenants/edit.html.erb new file mode 100644 index 000000000..710ff10cb --- /dev/null +++ b/app/views/admin/tenants/edit.html.erb @@ -0,0 +1 @@ +<%= render Admin::Tenants::EditComponent.new(@tenant) %> diff --git a/app/views/admin/tenants/index.html.erb b/app/views/admin/tenants/index.html.erb new file mode 100644 index 000000000..51cd3b7a6 --- /dev/null +++ b/app/views/admin/tenants/index.html.erb @@ -0,0 +1 @@ +<%= render Admin::Tenants::IndexComponent.new(@tenants) %> diff --git a/app/views/admin/tenants/new.html.erb b/app/views/admin/tenants/new.html.erb new file mode 100644 index 000000000..be2d4f4ca --- /dev/null +++ b/app/views/admin/tenants/new.html.erb @@ -0,0 +1 @@ +<%= render Admin::Tenants::NewComponent.new(@tenant) %> diff --git a/config/locales/en/activerecord.yml b/config/locales/en/activerecord.yml index 0e9f59e65..c3800181f 100644 --- a/config/locales/en/activerecord.yml +++ b/config/locales/en/activerecord.yml @@ -124,6 +124,9 @@ en: images: one: "Image" other: "Images" + tenant: + one: "tenant" + other: "tenants" topic: one: "Topic" other: "Topics" @@ -378,6 +381,8 @@ en: body: Body tag: name: "Type the name of the topic" + tenant: + schema: "Subdomain" topic: title: "Title" description: "Initial text" diff --git a/config/locales/en/admin.yml b/config/locales/en/admin.yml index b1b68b4b8..68cbbd3d5 100644 --- a/config/locales/en/admin.yml +++ b/config/locales/en/admin.yml @@ -785,6 +785,7 @@ en: comments: "Comments" local_census_records: Manage local census machine_learning: "AI / Machine learning" + multitenancy: Multitenancy administrators: index: title: Administrators @@ -1628,6 +1629,15 @@ en: notice: "Card updated successfully" destroy: notice: "Card removed successfully" + tenants: + create: + notice: Tenant created successfully + index: + create: Create tenant + new: + title: New tenant + update: + notice: Tenant updated successfully homepage: title: Homepage description: The active modules appear in the homepage in the same order as here. diff --git a/config/locales/es/activerecord.yml b/config/locales/es/activerecord.yml index 58305df15..102beb8f4 100644 --- a/config/locales/es/activerecord.yml +++ b/config/locales/es/activerecord.yml @@ -124,6 +124,9 @@ es: images: one: "Imagen" other: "Imágenes" + tenant: + one: "entidad" + other: "entidades" topic: one: "Tema" other: "Temas" @@ -378,6 +381,8 @@ es: body: Contenido tag: name: "Escribe el nombre del tema" + tenant: + schema: "Subdominio" topic: title: "Título" description: "Texto inicial" diff --git a/config/locales/es/admin.yml b/config/locales/es/admin.yml index 08caffd41..cca53ce34 100644 --- a/config/locales/es/admin.yml +++ b/config/locales/es/admin.yml @@ -784,6 +784,7 @@ es: comments: "Comentarios" local_census_records: Gestionar censo local machine_learning: "IA / Machine learning" + multitenancy: Multientidad administrators: index: title: Administradores @@ -1627,6 +1628,15 @@ es: notice: "Tarjeta actualizada con éxito" destroy: notice: "Tarjeta eliminada con éxito" + tenants: + create: + notice: Entidad creada correctamente + index: + create: Crear entidad + new: + title: Nueva entidad + update: + notice: Entidad actualizada correctamente homepage: title: Homepage description: Los módulos activos aparecerán en la homepage en el mismo orden que aquí. diff --git a/config/routes/admin.rb b/config/routes/admin.rb index 56a2cf77e..752942b82 100644 --- a/config/routes/admin.rb +++ b/config/routes/admin.rb @@ -277,6 +277,8 @@ namespace :admin do post :execute, on: :collection delete :cancel, on: :collection end + + resources :tenants, except: [:show, :destroy] end resolve "Milestone" do |milestone| diff --git a/spec/models/abilities/administrator_spec.rb b/spec/models/abilities/administrator_spec.rb index 4579aa4c8..4b8e2376f 100644 --- a/spec/models/abilities/administrator_spec.rb +++ b/spec/models/abilities/administrator_spec.rb @@ -164,4 +164,37 @@ describe Abilities::Administrator do it { should be_able_to(:destroy, SDG::Manager) } it { should be_able_to(:manage, Widget::Card) } + + describe "tenants" do + context "with multitenancy disabled" do + before { allow(Rails.application.config).to receive(:multitenancy).and_return(false) } + + it { should_not be_able_to :create, Tenant } + it { should_not be_able_to :read, Tenant } + it { should_not be_able_to :update, Tenant } + it { should_not be_able_to :destroy, Tenant } + end + + context "with multitenancy enabled" do + before { allow(Rails.application.config).to receive(:multitenancy).and_return(true) } + + it { should be_able_to :create, Tenant } + it { should be_able_to :read, Tenant } + it { should be_able_to :update, Tenant } + it { should_not be_able_to :destroy, Tenant } + + it "does not allow administrators from other tenants to manage tenants " do + create(:tenant, schema: "subsidiary") + + Tenant.switch("subsidiary") do + admin = create(:administrator).user + + expect(admin).not_to be_able_to :create, Tenant + expect(admin).not_to be_able_to :read, Tenant + expect(admin).not_to be_able_to :update, Tenant + expect(admin).not_to be_able_to :destroy, Tenant + end + end + end + end end diff --git a/spec/models/tenant_spec.rb b/spec/models/tenant_spec.rb index a8f41428f..60a658ffa 100644 --- a/spec/models/tenant_spec.rb +++ b/spec/models/tenant_spec.rb @@ -133,6 +133,26 @@ describe Tenant do end end + describe ".host_for" do + before do + allow(Tenant).to receive(:default_url_options).and_return({ host: "consul.dev" }) + end + + it "returns the default host for the default schema" do + expect(Tenant.host_for("public")).to eq "consul.dev" + end + + it "returns the host with a subdomain on other schemas" do + expect(Tenant.host_for("uranus")).to eq "uranus.consul.dev" + end + + it "uses lvh.me for subdomains when the host is localhost" do + allow(Tenant).to receive(:default_url_options).and_return({ host: "localhost" }) + + expect(Tenant.host_for("uranus")).to eq "uranus.lvh.me" + end + end + describe ".run_on_each" do it "runs the code on all tenants, including the default one" do create(:tenant, schema: "andromeda") diff --git a/spec/system/admin/tenants_spec.rb b/spec/system/admin/tenants_spec.rb new file mode 100644 index 000000000..3dabdb7f4 --- /dev/null +++ b/spec/system/admin/tenants_spec.rb @@ -0,0 +1,45 @@ +require "rails_helper" + +describe "Tenants", :admin do + before { allow(Tenant).to receive(:default_host).and_return("localhost") } + + scenario "Create" do + visit admin_root_path + + within("#side_menu") do + click_link "Settings" + click_link "Multitenancy" + end + + click_link "Create tenant" + fill_in "Subdomain", with: "earth" + fill_in "Name", with: "Earthlings" + click_button "Create tenant" + + expect(page).to have_content "Tenant created successfully" + + within("tr", text: "earth") { click_link "View" } + + expect(current_host).to eq "http://earth.lvh.me" + expect(page).to have_current_path root_path + expect(page).to have_link "Sign in" + end + + scenario "Update" do + create(:tenant, schema: "moon") + + visit admin_tenants_path + within("tr", text: "moon") { click_link "Edit" } + + fill_in "Subdomain", with: "the-moon" + click_button "Update tenant" + + expect(page).to have_content "Tenant updated successfully" + + within("tr", text: "the-moon") { click_link "View" } + + expect(current_host).to eq "http://the-moon.lvh.me" + expect(page).to have_current_path root_path + expect(page).to have_link "Sign in" + end +end From 314ce70000701cfa25a8c0f43d96138696e74136 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 7 Oct 2022 19:48:32 +0200 Subject: [PATCH 20/33] Use a different attachments folder per tenant While this is not strictly necessary, it can help moving the data of one tenant to a different server or removing it. Note we're using subfolders inside the `tenants` subfolder. If we simply used subfolders with the schema names, if the schema names were, for instance, language codes like `es`, `en`, `it`, ... they would conflict with the default subfolders used by Active Storage. --- config/storage.yml | 4 ++-- .../service/tenant_disk_service.rb | 13 +++++++++++++ spec/models/attachable_spec.rb | 16 ++++++++++++++++ 3 files changed, 31 insertions(+), 2 deletions(-) create mode 100644 lib/active_storage/service/tenant_disk_service.rb create mode 100644 spec/models/attachable_spec.rb diff --git a/config/storage.yml b/config/storage.yml index 590c3bb76..09c938b65 100644 --- a/config/storage.yml +++ b/config/storage.yml @@ -1,9 +1,9 @@ local: - service: Disk + service: TenantDisk root: <%= Rails.root.join("storage") %> test: - service: Disk + service: TenantDisk root: <%= Rails.root.join("tmp/storage") %> # s3: diff --git a/lib/active_storage/service/tenant_disk_service.rb b/lib/active_storage/service/tenant_disk_service.rb new file mode 100644 index 000000000..63414f295 --- /dev/null +++ b/lib/active_storage/service/tenant_disk_service.rb @@ -0,0 +1,13 @@ +require "active_storage/service/disk_service" + +module ActiveStorage + class Service::TenantDiskService < Service::DiskService + def path_for(key) + if Tenant.default? + super + else + super.sub(root, File.join(root, "tenants", Tenant.current_schema)) + end + end + end +end diff --git a/spec/models/attachable_spec.rb b/spec/models/attachable_spec.rb new file mode 100644 index 000000000..32b208bc8 --- /dev/null +++ b/spec/models/attachable_spec.rb @@ -0,0 +1,16 @@ +require "rails_helper" + +describe Attachable do + it "stores attachments for the default tenant in the default folder" do + file_path = build(:image).file_path + + expect(file_path).to include "storage/" + expect(file_path).not_to include "tenants" + end + + it "stores tenant attachments in a folder for the tenant" do + allow(Tenant).to receive(:current_schema).and_return("image-master") + + expect(build(:image).file_path).to include "storage/tenants/image-master/" + end +end From 2c0ede3aaa19bac1eac52d31363561c66a5e96b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 6 Oct 2022 18:10:35 +0200 Subject: [PATCH 21/33] Use the dir attribute in dashboard and mailer layouts We forgot to do so in commit d827768c0. In order to avoid the same mistake in the future, we're extracting a method to get these attributes. We're also adding tests, since we didn't have any tests to check that the `dir` attribute was properly set. --- .../common_html_attributes_component.html.erb | 1 + .../common_html_attributes_component.rb | 17 ++++++++++ app/helpers/layouts_helper.rb | 4 +++ app/mailers/application_mailer.rb | 4 +-- app/views/layouts/admin.html.erb | 3 +- app/views/layouts/application.html.erb | 2 +- app/views/layouts/dashboard.html.erb | 2 +- app/views/layouts/devise.html.erb | 2 +- app/views/layouts/mailer.html.erb | 2 +- app/views/layouts/management.html.erb | 3 +- .../common_html_attributes_component_spec.rb | 34 +++++++++++++++++++ 11 files changed, 63 insertions(+), 11 deletions(-) create mode 100644 app/components/layout/common_html_attributes_component.html.erb create mode 100644 app/components/layout/common_html_attributes_component.rb create mode 100644 spec/components/layout/common_html_attributes_component_spec.rb diff --git a/app/components/layout/common_html_attributes_component.html.erb b/app/components/layout/common_html_attributes_component.html.erb new file mode 100644 index 000000000..fce00b5b2 --- /dev/null +++ b/app/components/layout/common_html_attributes_component.html.erb @@ -0,0 +1 @@ +<%= attributes -%> diff --git a/app/components/layout/common_html_attributes_component.rb b/app/components/layout/common_html_attributes_component.rb new file mode 100644 index 000000000..41cb8323b --- /dev/null +++ b/app/components/layout/common_html_attributes_component.rb @@ -0,0 +1,17 @@ +class Layout::CommonHTMLAttributesComponent < ApplicationComponent + delegate :rtl?, to: :helpers + + private + + def attributes + sanitize([dir, lang].compact.join(" ")) + end + + def dir + 'dir="rtl"' if rtl? + end + + def lang + "lang=\"#{I18n.locale}\"" + end +end diff --git a/app/helpers/layouts_helper.rb b/app/helpers/layouts_helper.rb index e97727b34..32ddde240 100644 --- a/app/helpers/layouts_helper.rb +++ b/app/helpers/layouts_helper.rb @@ -9,4 +9,8 @@ module LayoutsHelper link_to(text, path, options.merge(title: title)) end end + + def common_html_attributes + render Layout::CommonHTMLAttributesComponent.new + end end diff --git a/app/mailers/application_mailer.rb b/app/mailers/application_mailer.rb index 757bc32ff..4451e348e 100644 --- a/app/mailers/application_mailer.rb +++ b/app/mailers/application_mailer.rb @@ -1,7 +1,5 @@ class ApplicationMailer < ActionMailer::Base - helper :settings - helper :application - helper :mailer + helper :application, :layouts, :mailer, :settings default from: proc { "#{Setting["mailer_from_name"]} <#{Setting["mailer_from_address"]}>" } layout "mailer" before_action :set_asset_host diff --git a/app/views/layouts/admin.html.erb b/app/views/layouts/admin.html.erb index 47cba9f90..48ad4426d 100644 --- a/app/views/layouts/admin.html.erb +++ b/app/views/layouts/admin.html.erb @@ -1,6 +1,5 @@ - lang="<%= I18n.locale %>"> - +> <%= render "layouts/common_head", default_title: "Admin" %> <%= content_for :head %> diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index 04fa2920a..495a18838 100644 --- a/app/views/layouts/application.html.erb +++ b/app/views/layouts/application.html.erb @@ -1,5 +1,5 @@ - lang="<%= I18n.locale %>" data-current-user-id="<%= current_user&.id %>"> + data-current-user-id="<%= current_user&.id %>"> <%= render "layouts/common_head", default_title: setting["org_name"] %> <%= render "layouts/meta_tags" %> diff --git a/app/views/layouts/dashboard.html.erb b/app/views/layouts/dashboard.html.erb index cf33f6f4f..ffdfc9439 100644 --- a/app/views/layouts/dashboard.html.erb +++ b/app/views/layouts/dashboard.html.erb @@ -1,5 +1,5 @@ - + data-current-user-id="<%= current_user&.id %>"> <%= render "layouts/common_head", default_title: setting["org_name"] %> <%= render "layouts/meta_tags" %> diff --git a/app/views/layouts/devise.html.erb b/app/views/layouts/devise.html.erb index e9ebac423..cc4db0fca 100644 --- a/app/views/layouts/devise.html.erb +++ b/app/views/layouts/devise.html.erb @@ -1,5 +1,5 @@ - lang="<%= I18n.locale %>"> +> <%= render "layouts/common_head", default_title: setting["org_name"] %> <%= render "layouts/meta_tags" %> diff --git a/app/views/layouts/mailer.html.erb b/app/views/layouts/mailer.html.erb index 6691fe4fb..c5dcf1fd6 100644 --- a/app/views/layouts/mailer.html.erb +++ b/app/views/layouts/mailer.html.erb @@ -1,5 +1,5 @@ - +> <%= t("mailers.title") %> diff --git a/app/views/layouts/management.html.erb b/app/views/layouts/management.html.erb index 182c12f4a..5bb9f525a 100644 --- a/app/views/layouts/management.html.erb +++ b/app/views/layouts/management.html.erb @@ -1,6 +1,5 @@ - lang="<%= I18n.locale %>"> - +> <%= render "layouts/common_head", default_title: "Management" %> <%= stylesheet_link_tag "print", media: "print" %> diff --git a/spec/components/layout/common_html_attributes_component_spec.rb b/spec/components/layout/common_html_attributes_component_spec.rb new file mode 100644 index 000000000..2d6fc3238 --- /dev/null +++ b/spec/components/layout/common_html_attributes_component_spec.rb @@ -0,0 +1,34 @@ +require "rails_helper" + +describe Layout::CommonHTMLAttributesComponent do + let(:component) { Layout::CommonHTMLAttributesComponent.new } + + it "includes the default language by default" do + render_inline component + + expect(page.text).to eq 'lang="en"' + end + + it "includes the current language" do + I18n.with_locale(:es) { render_inline component } + + expect(page.text).to eq 'lang="es"' + end + + context "RTL languages" do + let!(:default_enforce) { I18n.enforce_available_locales } + + before do + I18n.enforce_available_locales = false + allow(I18n).to receive(:available_locales).and_return(%i[ar en es]) + end + + after { I18n.enforce_available_locales = default_enforce } + + it "includes the dir attribute" do + I18n.with_locale(:ar) { render_inline component } + + expect(page.text).to eq 'dir="rtl" lang="ar"' + end + end +end From 5c61b72d216a55f3ce856ca4da4787fec1f5fc0d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 6 Oct 2022 17:48:42 +0200 Subject: [PATCH 22/33] Identify the current tenant in the tag This way it will be possible to write CSS and JavaScript code that will only apply to specific tenants. Note that CSS customization is still limited because it isn't possible to use different SCSS variables per tenant. --- .../common_html_attributes_component.rb | 6 ++- .../common_html_attributes_component_spec.rb | 50 +++++++++++++++---- spec/system/multitenancy_spec.rb | 4 ++ 3 files changed, 50 insertions(+), 10 deletions(-) diff --git a/app/components/layout/common_html_attributes_component.rb b/app/components/layout/common_html_attributes_component.rb index 41cb8323b..bface8c6c 100644 --- a/app/components/layout/common_html_attributes_component.rb +++ b/app/components/layout/common_html_attributes_component.rb @@ -4,7 +4,7 @@ class Layout::CommonHTMLAttributesComponent < ApplicationComponent private def attributes - sanitize([dir, lang].compact.join(" ")) + sanitize([dir, lang, html_class].compact.join(" ")) end def dir @@ -14,4 +14,8 @@ class Layout::CommonHTMLAttributesComponent < ApplicationComponent def lang "lang=\"#{I18n.locale}\"" end + + def html_class + "class=\"tenant-#{Tenant.current_schema}\"" if Rails.application.config.multitenancy + end end diff --git a/spec/components/layout/common_html_attributes_component_spec.rb b/spec/components/layout/common_html_attributes_component_spec.rb index 2d6fc3238..ded22103c 100644 --- a/spec/components/layout/common_html_attributes_component_spec.rb +++ b/spec/components/layout/common_html_attributes_component_spec.rb @@ -3,16 +3,36 @@ require "rails_helper" describe Layout::CommonHTMLAttributesComponent do let(:component) { Layout::CommonHTMLAttributesComponent.new } - it "includes the default language by default" do - render_inline component + context "with multitenancy disabled" do + before { allow(Rails.application.config).to receive(:multitenancy).and_return(false) } - expect(page.text).to eq 'lang="en"' + it "includes the default language by default" do + render_inline component + + expect(page.text).to eq 'lang="en"' + end + + it "includes the current language" do + I18n.with_locale(:es) { render_inline component } + + expect(page.text).to eq 'lang="es"' + end end - it "includes the current language" do - I18n.with_locale(:es) { render_inline component } + context "with multitenancy enabled" do + it "includes a class with the 'public' suffix for the default tenant" do + render_inline component - expect(page.text).to eq 'lang="es"' + expect(page.text).to eq 'lang="en" class="tenant-public"' + end + + it "includes a class with the schema name as suffix for other tenants" do + allow(Tenant).to receive(:current_schema).and_return "private" + + render_inline component + + expect(page.text).to eq 'lang="en" class="tenant-private"' + end end context "RTL languages" do @@ -25,10 +45,22 @@ describe Layout::CommonHTMLAttributesComponent do after { I18n.enforce_available_locales = default_enforce } - it "includes the dir attribute" do - I18n.with_locale(:ar) { render_inline component } + context "with multitenancy disabled" do + before { allow(Rails.application.config).to receive(:multitenancy).and_return(false) } - expect(page.text).to eq 'dir="rtl" lang="ar"' + it "includes the dir attribute" do + I18n.with_locale(:ar) { render_inline component } + + expect(page.text).to eq 'dir="rtl" lang="ar"' + end + end + + context "with multitenancy enabled" do + it "includes the dir and the class attributes" do + I18n.with_locale(:ar) { render_inline component } + + expect(page.text).to eq 'dir="rtl" lang="ar" class="tenant-public"' + end end end end diff --git a/spec/system/multitenancy_spec.rb b/spec/system/multitenancy_spec.rb index ef1a90564..ef161fb9c 100644 --- a/spec/system/multitenancy_spec.rb +++ b/spec/system/multitenancy_spec.rb @@ -28,12 +28,16 @@ describe "Multitenancy" do visit polls_path expect(page).to have_content "Human rights for Martians?" + expect(page).to have_css "html.tenant-mars" + expect(page).not_to have_css "html.tenant-venus" end with_subdomain("venus") do visit polls_path expect(page).to have_content "There are no open votings" + expect(page).to have_css "html.tenant-venus" + expect(page).not_to have_css "html.tenant-mars" end end From 06d0c2612624a0140d6f1e34b820900e2ee75c01 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 29 Sep 2022 19:38:18 +0200 Subject: [PATCH 23/33] Allow having different SMTP settings per tenant Right now this is configured using the `secrets.yml` file, which is the file we've used in the past to configure SMTP settings. Note that, in the `current_secrets` method, the `if default?` condition is there so in single-tenant applications it returns the exact same object as `Rails.application.secrets`, and it makes it immediately clear for developers reading the code. We're also caching the tenant secrets (using `||=`) so they behave the same way as Rails secrets; for this to work properly 100% of the time (for example, in tests) we need to expire these cached secrets whenever the Rails secrets change. A similar `unless Tenant.default?` condition is present in the ApplicationMailer because there's a chance some CONSUL installations might not be using secrets to define the SMTP settings(they might be using environment variables, for example) and so in this case we don't want to force settings based on the secrets.yml file because it would break the application. The structure of the SMTP settings in the secrets file should be: ``` production: tenants: name_of_the_tenant_subdomain: smtp_settings: address: (...) ``` --- app/mailers/application_mailer.rb | 7 ++++ app/models/tenant.rb | 18 ++++++++++ config/secrets.yml.example | 24 +++++++++++++ spec/mailers/mailer_spec.rb | 36 ++++++++++++++++++++ spec/models/tenant_spec.rb | 56 +++++++++++++++++++++++++++++++ 5 files changed, 141 insertions(+) diff --git a/app/mailers/application_mailer.rb b/app/mailers/application_mailer.rb index 4451e348e..1fc1410e0 100644 --- a/app/mailers/application_mailer.rb +++ b/app/mailers/application_mailer.rb @@ -3,6 +3,7 @@ class ApplicationMailer < ActionMailer::Base default from: proc { "#{Setting["mailer_from_name"]} <#{Setting["mailer_from_address"]}>" } layout "mailer" before_action :set_asset_host + after_action :set_smtp_settings def default_url_options Tenant.current_url_options @@ -11,4 +12,10 @@ class ApplicationMailer < ActionMailer::Base def set_asset_host self.asset_host ||= root_url.delete_suffix("/") end + + def set_smtp_settings + unless Tenant.default? + mail.delivery_method.settings.merge!(Tenant.current_secrets.smtp_settings.to_h) + end + end end diff --git a/app/models/tenant.rb b/app/models/tenant.rb index 87b4a2c63..dbe00b335 100644 --- a/app/models/tenant.rb +++ b/app/models/tenant.rb @@ -53,6 +53,24 @@ class Tenant < ApplicationRecord end end + def self.current_secrets + if default? + Rails.application.secrets + else + @secrets ||= {} + @cached_rails_secrets ||= Rails.application.secrets + + if @cached_rails_secrets != Rails.application.secrets + @secrets = {} + @cached_rails_secrets = nil + end + + @secrets[current_schema] ||= Rails.application.secrets.merge( + Rails.application.secrets.dig(:tenants, current_schema.to_sym).to_h + ) + end + end + def self.default? current_schema == "public" end diff --git a/config/secrets.yml.example b/config/secrets.yml.example index 432ecc587..6d93ec4ab 100644 --- a/config/secrets.yml.example +++ b/config/secrets.yml.example @@ -50,6 +50,14 @@ staging: managers_url: "" managers_application_key: "" multitenancy: false + tenants: + # If you've enabled multitenancy, you can overwrite secrets for a + # specific tenant with: + # + # my_tenant_subdomain: + # secret_key: my_secret_value + # + # Currently you can overwrite SMTP settings. <<: *maps <<: *apis @@ -77,6 +85,14 @@ preproduction: managers_url: "" managers_application_key: "" multitenancy: false + tenants: + # If you've enabled multitenancy, you can overwrite secrets for a + # specific tenant with: + # + # my_tenant_subdomain: + # secret_key: my_secret_value + # + # Currently you can overwrite SMTP settings. twitter_key: "" twitter_secret: "" facebook_key: "" @@ -109,6 +125,14 @@ production: managers_url: "" managers_application_key: "" multitenancy: false + tenants: + # If you've enabled multitenancy, you can overwrite secrets for a + # specific tenant with: + # + # my_tenant_subdomain: + # secret_key: my_secret_value + # + # Currently you can overwrite SMTP settings. twitter_key: "" twitter_secret: "" facebook_key: "" diff --git a/spec/mailers/mailer_spec.rb b/spec/mailers/mailer_spec.rb index 3db6083c8..af991075c 100644 --- a/spec/mailers/mailer_spec.rb +++ b/spec/mailers/mailer_spec.rb @@ -69,5 +69,41 @@ describe Mailer do expect(body).to match "href=\"http://delay.consul.dev/" expect(body).to match "src=\"http://delay.consul.dev/" end + + describe "SMTP settings" do + let(:default_settings) { { address: "mail.consul.dev", username: "main" } } + let(:super_settings) { { address: "super.consul.dev", username: "super" } } + + before do + allow(Rails.application).to receive(:secrets).and_return(ActiveSupport::OrderedOptions.new.merge( + smtp_settings: default_settings, + tenants: { + supermailer: { smtp_settings: super_settings } + } + )) + end + + it "does not overwrite the settings for the default tenant" do + Mailer.user_invite("test@consul.dev").deliver_now + + expect(ActionMailer::Base.deliveries.last.delivery_method.settings).to eq({}) + end + + it "uses specific secret settings for tenants overwriting them" do + allow(Tenant).to receive(:current_schema).and_return("supermailer") + + Mailer.user_invite("test@consul.dev").deliver_now + + expect(ActionMailer::Base.deliveries.last.delivery_method.settings).to eq super_settings + end + + it "uses the default secret settings for other tenants" do + allow(Tenant).to receive(:current_schema).and_return("ultramailer") + + Mailer.user_invite("test@consul.dev").deliver_now + + expect(ActionMailer::Base.deliveries.last.delivery_method.settings).to eq default_settings + end + end end end diff --git a/spec/models/tenant_spec.rb b/spec/models/tenant_spec.rb index 60a658ffa..0d5cdfc8b 100644 --- a/spec/models/tenant_spec.rb +++ b/spec/models/tenant_spec.rb @@ -153,6 +153,62 @@ describe Tenant do end end + describe ".current_secrets" do + context "same secrets for all tenants" do + before do + allow(Rails.application).to receive(:secrets).and_return(ActiveSupport::OrderedOptions.new.merge( + star: "Sun", + volume: "Medium" + )) + end + + it "returns the default secrets for the default tenant" do + allow(Tenant).to receive(:current_schema).and_return("public") + + expect(Tenant.current_secrets.star).to eq "Sun" + expect(Tenant.current_secrets.volume).to eq "Medium" + end + + it "returns the default secrets for other tenants" do + allow(Tenant).to receive(:current_schema).and_return("earth") + + expect(Tenant.current_secrets.star).to eq "Sun" + expect(Tenant.current_secrets.volume).to eq "Medium" + end + end + + context "tenant overwriting secrets" do + before do + allow(Rails.application).to receive(:secrets).and_return(ActiveSupport::OrderedOptions.new.merge( + star: "Sun", + volume: "Medium", + tenants: { proxima: { star: "Alpha Centauri" }} + )) + end + + it "returns the default secrets for the default tenant" do + allow(Tenant).to receive(:current_schema).and_return("public") + + expect(Tenant.current_secrets.star).to eq "Sun" + expect(Tenant.current_secrets.volume).to eq "Medium" + end + + it "returns the overwritten secrets for tenants overwriting them" do + allow(Tenant).to receive(:current_schema).and_return("proxima") + + expect(Tenant.current_secrets.star).to eq "Alpha Centauri" + expect(Tenant.current_secrets.volume).to eq "Medium" + end + + it "returns the default secrets for other tenants" do + allow(Tenant).to receive(:current_schema).and_return("earth") + + expect(Tenant.current_secrets.star).to eq "Sun" + expect(Tenant.current_secrets.volume).to eq "Medium" + end + end + end + describe ".run_on_each" do it "runs the code on all tenants, including the default one" do create(:tenant, schema: "andromeda") From eb2cf00ddfcd560dd3288a19b18a00887118b72d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 29 Sep 2022 19:58:22 +0200 Subject: [PATCH 24/33] Allow different SMS settings for different tenants --- config/secrets.yml.example | 6 +++--- lib/sms_api.rb | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/config/secrets.yml.example b/config/secrets.yml.example index 6d93ec4ab..1da8e5e88 100644 --- a/config/secrets.yml.example +++ b/config/secrets.yml.example @@ -57,7 +57,7 @@ staging: # my_tenant_subdomain: # secret_key: my_secret_value # - # Currently you can overwrite SMTP settings. + # Currently you can overwrite SMTP settings and SMS settings. <<: *maps <<: *apis @@ -92,7 +92,7 @@ preproduction: # my_tenant_subdomain: # secret_key: my_secret_value # - # Currently you can overwrite SMTP settings. + # Currently you can overwrite SMTP settings and SMS settings. twitter_key: "" twitter_secret: "" facebook_key: "" @@ -132,7 +132,7 @@ production: # my_tenant_subdomain: # secret_key: my_secret_value # - # Currently you can overwrite SMTP settings. + # Currently you can overwrite SMTP settings and SMS settings. twitter_key: "" twitter_secret: "" facebook_key: "" diff --git a/lib/sms_api.rb b/lib/sms_api.rb index e3187b710..84389123f 100644 --- a/lib/sms_api.rb +++ b/lib/sms_api.rb @@ -9,11 +9,11 @@ class SMSApi def url return "" unless end_point_available? - URI.parse(Rails.application.secrets.sms_end_point).to_s + URI.parse(Tenant.current_secrets.sms_end_point).to_s end def authorization - Base64.encode64("#{Rails.application.secrets.sms_username}:#{Rails.application.secrets.sms_password}") + Base64.encode64("#{Tenant.current_secrets.sms_username}:#{Tenant.current_secrets.sms_password}") end def sms_deliver(phone, code) From 338f4929ca72ca4268f15dfa3a7362658755c3db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 29 Sep 2022 20:31:55 +0200 Subject: [PATCH 25/33] Allow different manager auth settings per tenant --- config/secrets.yml.example | 6 +++--- lib/manager_authenticator.rb | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/config/secrets.yml.example b/config/secrets.yml.example index 1da8e5e88..99c821160 100644 --- a/config/secrets.yml.example +++ b/config/secrets.yml.example @@ -57,7 +57,7 @@ staging: # my_tenant_subdomain: # secret_key: my_secret_value # - # Currently you can overwrite SMTP settings and SMS settings. + # Currently you can overwrite SMTP, SMS and manager settings. <<: *maps <<: *apis @@ -92,7 +92,7 @@ preproduction: # my_tenant_subdomain: # secret_key: my_secret_value # - # Currently you can overwrite SMTP settings and SMS settings. + # Currently you can overwrite SMTP, SMS and manager settings. twitter_key: "" twitter_secret: "" facebook_key: "" @@ -132,7 +132,7 @@ production: # my_tenant_subdomain: # secret_key: my_secret_value # - # Currently you can overwrite SMTP settings and SMS settings. + # Currently you can overwrite SMTP, SMS and manager settings. twitter_key: "" twitter_secret: "" facebook_key: "" diff --git a/lib/manager_authenticator.rb b/lib/manager_authenticator.rb index 5ad17cb07..d6976470e 100644 --- a/lib/manager_authenticator.rb +++ b/lib/manager_authenticator.rb @@ -31,7 +31,7 @@ class ManagerAuthenticator end def client - @client ||= Savon.client(wsdl: Rails.application.secrets.managers_url) + @client ||= Savon.client(wsdl: Tenant.current_secrets.managers_url) end def parser @@ -39,6 +39,6 @@ class ManagerAuthenticator end def application_key - Rails.application.secrets.managers_application_key.to_s + Tenant.current_secrets.managers_application_key.to_s end end From 18f1d5c1a3f59b94e6a8e89205c1ac122e420f2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 29 Sep 2022 22:58:10 +0200 Subject: [PATCH 26/33] Allow different remote translation keys per tenant Note we don't need to update the tests; the tests themselves help us confirm that `Rails.application.secrets` and `Tenant.current_secrets` return the same object on single-tenant applications. --- app/controllers/concerns/remotely_translatable.rb | 2 +- config/secrets.yml.example | 9 ++++++--- lib/remote_translations/microsoft/available_locales.rb | 2 +- lib/remote_translations/microsoft/client.rb | 2 +- 4 files changed, 9 insertions(+), 6 deletions(-) diff --git a/app/controllers/concerns/remotely_translatable.rb b/app/controllers/concerns/remotely_translatable.rb index 6bbf791d1..7508a29f3 100644 --- a/app/controllers/concerns/remotely_translatable.rb +++ b/app/controllers/concerns/remotely_translatable.rb @@ -26,6 +26,6 @@ module RemotelyTranslatable end def api_key_has_been_set_in_secrets? - Rails.application.secrets.microsoft_api_key.present? + Tenant.current_secrets.microsoft_api_key.present? end end diff --git a/config/secrets.yml.example b/config/secrets.yml.example index 99c821160..a66abb40a 100644 --- a/config/secrets.yml.example +++ b/config/secrets.yml.example @@ -57,7 +57,8 @@ staging: # my_tenant_subdomain: # secret_key: my_secret_value # - # Currently you can overwrite SMTP, SMS and manager settings. + # Currently you can overwrite SMTP, SMS, manager and microsoft API + # settings. <<: *maps <<: *apis @@ -92,7 +93,8 @@ preproduction: # my_tenant_subdomain: # secret_key: my_secret_value # - # Currently you can overwrite SMTP, SMS and manager settings. + # Currently you can overwrite SMTP, SMS, manager and microsoft API + # settings. twitter_key: "" twitter_secret: "" facebook_key: "" @@ -132,7 +134,8 @@ production: # my_tenant_subdomain: # secret_key: my_secret_value # - # Currently you can overwrite SMTP, SMS and manager settings. + # Currently you can overwrite SMTP, SMS, manager and microsoft API + # settings. twitter_key: "" twitter_secret: "" facebook_key: "" diff --git a/lib/remote_translations/microsoft/available_locales.rb b/lib/remote_translations/microsoft/available_locales.rb index 8adee78f7..21ebf05cd 100644 --- a/lib/remote_translations/microsoft/available_locales.rb +++ b/lib/remote_translations/microsoft/available_locales.rb @@ -36,7 +36,7 @@ class RemoteTranslations::Microsoft::AvailableLocales uri = URI(host + path) request = Net::HTTP::Get.new(uri) - request["Ocp-Apim-Subscription-Key"] = Rails.application.secrets.microsoft_api_key + request["Ocp-Apim-Subscription-Key"] = Tenant.current_secrets.microsoft_api_key response = Net::HTTP.start(uri.host, uri.port, use_ssl: uri.scheme == "https") do |http| http.request(request) diff --git a/lib/remote_translations/microsoft/client.rb b/lib/remote_translations/microsoft/client.rb index d7e1bc5d9..a4b256fda 100644 --- a/lib/remote_translations/microsoft/client.rb +++ b/lib/remote_translations/microsoft/client.rb @@ -6,7 +6,7 @@ class RemoteTranslations::Microsoft::Client PREVENTING_TRANSLATION_KEY = "notranslate".freeze def initialize - api_key = Rails.application.secrets.microsoft_api_key + api_key = Tenant.current_secrets.microsoft_api_key @client = TranslatorText::Client.new(api_key) end From a3be1e174bffc77c44752d9dea8643e0b275a3e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 29 Sep 2022 23:07:28 +0200 Subject: [PATCH 27/33] Allow different HTTP basic auth settings per tenant --- app/controllers/application_controller.rb | 5 +++-- config/secrets.yml.example | 12 ++++++------ 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 0a6e67a98..fd9c77a6a 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -26,12 +26,13 @@ class ApplicationController < ActionController::Base def authenticate_http_basic authenticate_or_request_with_http_basic do |username, password| - username == Rails.application.secrets.http_basic_username && password == Rails.application.secrets.http_basic_password + username == Tenant.current_secrets.http_basic_username && + password == Tenant.current_secrets.http_basic_password end end def http_basic_auth_site? - Rails.application.secrets.http_basic_auth + Tenant.current_secrets.http_basic_auth end def verify_lock diff --git a/config/secrets.yml.example b/config/secrets.yml.example index a66abb40a..c6d29ec3b 100644 --- a/config/secrets.yml.example +++ b/config/secrets.yml.example @@ -57,8 +57,8 @@ staging: # my_tenant_subdomain: # secret_key: my_secret_value # - # Currently you can overwrite SMTP, SMS, manager and microsoft API - # settings. + # Currently you can overwrite SMTP, SMS, manager, microsoft API and + # HTTP basic settings. <<: *maps <<: *apis @@ -93,8 +93,8 @@ preproduction: # my_tenant_subdomain: # secret_key: my_secret_value # - # Currently you can overwrite SMTP, SMS, manager and microsoft API - # settings. + # Currently you can overwrite SMTP, SMS, manager, microsoft API and + # HTTP basic settings. twitter_key: "" twitter_secret: "" facebook_key: "" @@ -134,8 +134,8 @@ production: # my_tenant_subdomain: # secret_key: my_secret_value # - # Currently you can overwrite SMTP, SMS, manager and microsoft API - # settings. + # Currently you can overwrite SMTP, SMS, manager, microsoft API and + # HTTP basic settings. twitter_key: "" twitter_secret: "" facebook_key: "" From 0ea61b9b615cf4f27d372f4441047dfc581b9ca5 Mon Sep 17 00:00:00 2001 From: Eduardo Vilar Date: Mon, 13 May 2019 10:34:43 +0200 Subject: [PATCH 28/33] Allow different omniauth settings per tenant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Javi Martín --- .../shared/_social_media_meta_tags.html.erb | 2 +- config/initializers/devise.rb | 20 ++++++-- config/secrets.yml.example | 12 ++--- lib/omniauth_tenant_setup.rb | 47 +++++++++++++++++++ 4 files changed, 70 insertions(+), 11 deletions(-) create mode 100644 lib/omniauth_tenant_setup.rb diff --git a/app/views/shared/_social_media_meta_tags.html.erb b/app/views/shared/_social_media_meta_tags.html.erb index 01811be41..bd7117708 100644 --- a/app/views/shared/_social_media_meta_tags.html.erb +++ b/app/views/shared/_social_media_meta_tags.html.erb @@ -17,4 +17,4 @@ " /> " /> - + diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index aeb30c2f1..e297bfd48 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -245,14 +245,26 @@ Devise.setup do |config| # Add a new OmniAuth provider. Check the wiki for more information on setting # up on your models and hooks. # config.omniauth :github, 'APP_ID', 'APP_SECRET', scope: 'user,public_repo' - config.omniauth :twitter, Rails.application.secrets.twitter_key, Rails.application.secrets.twitter_secret - config.omniauth :facebook, Rails.application.secrets.facebook_key, Rails.application.secrets.facebook_secret, scope: "email", info_fields: "email,name,verified" - config.omniauth :google_oauth2, Rails.application.secrets.google_oauth2_key, Rails.application.secrets.google_oauth2_secret + config.omniauth :twitter, + Rails.application.secrets.twitter_key, + Rails.application.secrets.twitter_secret, + setup: OmniauthTenantSetup.twitter + config.omniauth :facebook, + Rails.application.secrets.facebook_key, + Rails.application.secrets.facebook_secret, + scope: "email", + info_fields: "email,name,verified", + setup: OmniauthTenantSetup.facebook + config.omniauth :google_oauth2, + Rails.application.secrets.google_oauth2_key, + Rails.application.secrets.google_oauth2_secret, + setup: OmniauthTenantSetup.google_oauth2 config.omniauth :wordpress_oauth2, Rails.application.secrets.wordpress_oauth2_key, Rails.application.secrets.wordpress_oauth2_secret, strategy_class: OmniAuth::Strategies::Wordpress, - client_options: { site: Rails.application.secrets.wordpress_oauth2_site } + client_options: { site: Rails.application.secrets.wordpress_oauth2_site }, + setup: OmniauthTenantSetup.wordpress_oauth2 # ==> Warden configuration # If you want to use other strategies, that are not supported by Devise, or diff --git a/config/secrets.yml.example b/config/secrets.yml.example index c6d29ec3b..d04c93778 100644 --- a/config/secrets.yml.example +++ b/config/secrets.yml.example @@ -57,8 +57,8 @@ staging: # my_tenant_subdomain: # secret_key: my_secret_value # - # Currently you can overwrite SMTP, SMS, manager, microsoft API and - # HTTP basic settings. + # Currently you can overwrite SMTP, SMS, manager, microsoft API, + # HTTP basic, twitter, facebook, google and wordpress settings. <<: *maps <<: *apis @@ -93,8 +93,8 @@ preproduction: # my_tenant_subdomain: # secret_key: my_secret_value # - # Currently you can overwrite SMTP, SMS, manager, microsoft API and - # HTTP basic settings. + # Currently you can overwrite SMTP, SMS, manager, microsoft API, + # HTTP basic, twitter, facebook, google and wordpress settings. twitter_key: "" twitter_secret: "" facebook_key: "" @@ -134,8 +134,8 @@ production: # my_tenant_subdomain: # secret_key: my_secret_value # - # Currently you can overwrite SMTP, SMS, manager, microsoft API and - # HTTP basic settings. + # Currently you can overwrite SMTP, SMS, manager, microsoft API, + # HTTP basic, twitter, facebook, google and wordpress settings. twitter_key: "" twitter_secret: "" facebook_key: "" diff --git a/lib/omniauth_tenant_setup.rb b/lib/omniauth_tenant_setup.rb new file mode 100644 index 000000000..57d010947 --- /dev/null +++ b/lib/omniauth_tenant_setup.rb @@ -0,0 +1,47 @@ +module OmniauthTenantSetup + class << self + def twitter + ->(env) do + oauth(env, secrets.twitter_key, secrets.twitter_secret) + end + end + + def facebook + ->(env) do + oauth2(env, secrets.facebook_key, secrets.facebook_secret) + end + end + + def google_oauth2 + ->(env) do + oauth2(env, secrets.google_oauth2_key, secrets.google_oauth2_secret) + end + end + + def wordpress_oauth2 + ->(env) do + oauth2(env, secrets.wordpress_oauth2_key, secrets.wordpress_oauth2_secret) + end + end + + private + + def oauth(env, key, secret) + unless Tenant.default? + env["omniauth.strategy"].options[:consumer_key] = key + env["omniauth.strategy"].options[:consumer_secret] = secret + end + end + + def oauth2(env, key, secret) + unless Tenant.default? + env["omniauth.strategy"].options[:client_id] = key + env["omniauth.strategy"].options[:client_secret] = secret + end + end + + def secrets + Tenant.current_secrets + end + end +end From 58c9e8462ded27d8e2e9ddefc639e628b6fda7cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 13 Oct 2022 03:07:28 +0200 Subject: [PATCH 29/33] Only seed tenants when necessary in tests On my machine, seeding a tenant takes about one second, so skipping this action when it isn't necessary makes tests creating tenants faster (although creating a tenant still takes about 3-4 seconds on my machine). --- config/initializers/apartment.rb | 2 +- spec/lib/tasks/sitemap_spec.rb | 3 +++ spec/spec_helper.rb | 8 ++++++++ spec/system/admin/tenants_spec.rb | 2 +- spec/system/multitenancy_spec.rb | 2 +- 5 files changed, 14 insertions(+), 3 deletions(-) diff --git a/config/initializers/apartment.rb b/config/initializers/apartment.rb index e13c3812a..3088a06c2 100644 --- a/config/initializers/apartment.rb +++ b/config/initializers/apartment.rb @@ -13,7 +13,7 @@ require "apartment/elevators/generic" # Apartment.configure do |config| ENV["IGNORE_EMPTY_TENANTS"] = "true" if Rails.env.test? || Rails.application.config.multitenancy.blank? - config.seed_after_create = true + config.seed_after_create = !Rails.env.test? # Add any models that you do not want to be multi-tenanted, but remain in the global (public) namespace. # A typical example would be a Customer or Tenant model that stores each Tenant's information. diff --git a/spec/lib/tasks/sitemap_spec.rb b/spec/lib/tasks/sitemap_spec.rb index b6cc68c68..379b9c8a8 100644 --- a/spec/lib/tasks/sitemap_spec.rb +++ b/spec/lib/tasks/sitemap_spec.rb @@ -80,15 +80,18 @@ describe "rake sitemap:create", type: :system do create(:tenant, schema: "debates") create(:tenant, schema: "proposals") + Setting["process.budgets"] = true Setting["process.debates"] = false Setting["process.proposals"] = false Tenant.switch("debates") do + Setting["process.debates"] = true Setting["process.budgets"] = false Setting["process.proposals"] = false end Tenant.switch("proposals") do + Setting["process.proposals"] = true Setting["process.budgets"] = false Setting["process.debates"] = false end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 59c87016a..a2f22cf87 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -117,6 +117,14 @@ RSpec.configure do |config| Delayed::Worker.delay_jobs = false end + config.before(:each, :seed_tenants) do + Apartment.seed_after_create = true + end + + config.after(:each, :seed_tenants) do + Apartment.seed_after_create = false + end + config.before(:each, :small_window) do @window_size = Capybara.current_window.size Capybara.current_window.resize_to(639, 479) diff --git a/spec/system/admin/tenants_spec.rb b/spec/system/admin/tenants_spec.rb index 3dabdb7f4..4454fc6fc 100644 --- a/spec/system/admin/tenants_spec.rb +++ b/spec/system/admin/tenants_spec.rb @@ -1,6 +1,6 @@ require "rails_helper" -describe "Tenants", :admin do +describe "Tenants", :admin, :seed_tenants do before { allow(Tenant).to receive(:default_host).and_return("localhost") } scenario "Create" do diff --git a/spec/system/multitenancy_spec.rb b/spec/system/multitenancy_spec.rb index ef161fb9c..dd1f25e84 100644 --- a/spec/system/multitenancy_spec.rb +++ b/spec/system/multitenancy_spec.rb @@ -1,6 +1,6 @@ require "rails_helper" -describe "Multitenancy" do +describe "Multitenancy", :seed_tenants do before do create(:tenant, schema: "mars") create(:tenant, schema: "venus") From 2f312bf4742c87067f023ddf5ad62e83afdc6197 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 19 Oct 2022 01:41:07 +0200 Subject: [PATCH 30/33] Use a different machine learning folder per tenant We're using the "tenants" subfolder for consistency with the folder structure we use in ActiveStorage and because some CONSUL installations might have folders inside the `data` folder which might conflict with the folders created by tenants. Note that the Python scripts have a lot of duplication, meaning we need to change all of them. I'm not refactoring them because I'm not familiar enough with these scripts (or with Python, for that matter). Also note that the scripts folder is still shared by all tenants, meaning it isn't possible to have different scripts for different tenants. I'm not sure how this situation should be handled; again, I'm not familiar enough with this feature. --- .dockerignore | 1 + .gitignore | 1 + app/models/machine_learning.rb | 81 ++++++++++++------- .../budgets_related_content_and_tags_nmf.py | 7 +- .../budgets_summary_comments_textrank.py | 7 +- .../proposals_related_content_and_tags_nmf.py | 7 +- .../proposals_summary_comments_textrank.py | 7 +- spec/models/machine_learning_spec.rb | 22 ++--- spec/system/admin/machine_learning_spec.rb | 6 +- 9 files changed, 89 insertions(+), 50 deletions(-) diff --git a/.dockerignore b/.dockerignore index 41853f5ae..8c75b254c 100644 --- a/.dockerignore +++ b/.dockerignore @@ -23,6 +23,7 @@ public/sitemap.xml public/tenants/*/sitemap.xml public/assets/ public/machine_learning/data/ +public/tenants/*/machine_learning/data/ # Bundler config, cache and gemsets **/.bundle/ diff --git a/.gitignore b/.gitignore index 3ac23e93f..28d1c676c 100644 --- a/.gitignore +++ b/.gitignore @@ -25,6 +25,7 @@ tmp/ /public/tenants/*/sitemap.xml /public/assets/ /public/machine_learning/data/ +/public/tenants/*/machine_learning/data/ # Bundler config, cache and gemsets .bundle/ diff --git a/app/models/machine_learning.rb b/app/models/machine_learning.rb index 76be044b8..0b680e212 100644 --- a/app/models/machine_learning.rb +++ b/app/models/machine_learning.rb @@ -3,7 +3,6 @@ class MachineLearning attr_accessor :job SCRIPTS_FOLDER = Rails.root.join("public", "machine_learning", "scripts").freeze - DATA_FOLDER = Rails.root.join("public", "machine_learning", "data").freeze def initialize(job) @job = job @@ -11,6 +10,10 @@ class MachineLearning @previous_modified_date = set_previous_modified_date end + def data_folder + self.class.data_folder + end + def run begin export_proposals_to_json @@ -81,17 +84,33 @@ class MachineLearning "comments.json" end + def data_folder + Rails.root.join("public", tenant_data_folder) + end + + def tenant_data_folder + File.join(tenant_subfolder, "machine_learning", "data").delete_prefix("/") + end + + def tenant_subfolder + if Tenant.default? + "" + else + File.join("tenants", Tenant.current_schema) + end + end + def data_output_files files = { tags: [], related_content: [], comments_summary: [] } - files[:tags] << proposals_tags_filename if File.exists?(DATA_FOLDER.join(proposals_tags_filename)) - files[:tags] << proposals_taggings_filename if File.exists?(DATA_FOLDER.join(proposals_taggings_filename)) - files[:tags] << investments_tags_filename if File.exists?(DATA_FOLDER.join(investments_tags_filename)) - files[:tags] << investments_taggings_filename if File.exists?(DATA_FOLDER.join(investments_taggings_filename)) - files[:related_content] << proposals_related_filename if File.exists?(DATA_FOLDER.join(proposals_related_filename)) - files[:related_content] << investments_related_filename if File.exists?(DATA_FOLDER.join(investments_related_filename)) - files[:comments_summary] << proposals_comments_summary_filename if File.exists?(DATA_FOLDER.join(proposals_comments_summary_filename)) - files[:comments_summary] << investments_comments_summary_filename if File.exists?(DATA_FOLDER.join(investments_comments_summary_filename)) + files[:tags] << proposals_tags_filename if File.exists?(data_folder.join(proposals_tags_filename)) + files[:tags] << proposals_taggings_filename if File.exists?(data_folder.join(proposals_taggings_filename)) + files[:tags] << investments_tags_filename if File.exists?(data_folder.join(investments_tags_filename)) + files[:tags] << investments_taggings_filename if File.exists?(data_folder.join(investments_taggings_filename)) + files[:related_content] << proposals_related_filename if File.exists?(data_folder.join(proposals_related_filename)) + files[:related_content] << investments_related_filename if File.exists?(data_folder.join(investments_related_filename)) + files[:comments_summary] << proposals_comments_summary_filename if File.exists?(data_folder.join(proposals_comments_summary_filename)) + files[:comments_summary] << investments_comments_summary_filename if File.exists?(data_folder.join(investments_comments_summary_filename)) files end @@ -110,10 +129,10 @@ class MachineLearning proposals_comments_summary_filename, investments_comments_summary_filename ] - json = Dir[DATA_FOLDER.join("*.json")].map do |full_path_filename| + json = Dir[data_folder.join("*.json")].map do |full_path_filename| full_path_filename.split("/").last end - csv = Dir[DATA_FOLDER.join("*.csv")].map do |full_path_filename| + csv = Dir[data_folder.join("*.csv")].map do |full_path_filename| full_path_filename.split("/").last end (json + csv - excluded).sort @@ -152,7 +171,7 @@ class MachineLearning end def data_path(filename) - "/machine_learning/data/" + filename + "/#{tenant_data_folder}/#{filename}" end def script_kinds @@ -196,29 +215,35 @@ class MachineLearning private def create_data_folder - FileUtils.mkdir_p DATA_FOLDER + FileUtils.mkdir_p data_folder end def export_proposals_to_json create_data_folder - filename = DATA_FOLDER.join(MachineLearning.proposals_filename) + filename = data_folder.join(MachineLearning.proposals_filename) Proposal::Exporter.new.to_json_file(filename) end def export_budget_investments_to_json create_data_folder - filename = DATA_FOLDER.join(MachineLearning.investments_filename) + filename = data_folder.join(MachineLearning.investments_filename) Budget::Investment::Exporter.new(Array.new).to_json_file(filename) end def export_comments_to_json create_data_folder - filename = DATA_FOLDER.join(MachineLearning.comments_filename) + filename = data_folder.join(MachineLearning.comments_filename) Comment::Exporter.new.to_json_file(filename) end def run_machine_learning_scripts - output = `cd #{SCRIPTS_FOLDER} && python #{job.script} 2>&1` + command = if Tenant.default? + "python #{job.script}" + else + "CONSUL_TENANT=#{Tenant.current_schema} python #{job.script}" + end + + output = `cd #{SCRIPTS_FOLDER} && #{command} 2>&1` result = $?.success? if result == false job.update!(finished_at: Time.current, error: output) @@ -254,7 +279,7 @@ class MachineLearning end def import_ml_proposals_comments_summary - json_file = DATA_FOLDER.join(MachineLearning.proposals_comments_summary_filename) + json_file = data_folder.join(MachineLearning.proposals_comments_summary_filename) json_data = JSON.parse(File.read(json_file)).each(&:deep_symbolize_keys!) json_data.each do |attributes| attributes.delete(:id) @@ -266,7 +291,7 @@ class MachineLearning end def import_ml_investments_comments_summary - json_file = DATA_FOLDER.join(MachineLearning.investments_comments_summary_filename) + json_file = data_folder.join(MachineLearning.investments_comments_summary_filename) json_data = JSON.parse(File.read(json_file)).each(&:deep_symbolize_keys!) json_data.each do |attributes| attributes.delete(:id) @@ -278,7 +303,7 @@ class MachineLearning end def import_proposals_related_content - json_file = DATA_FOLDER.join(MachineLearning.proposals_related_filename) + json_file = data_folder.join(MachineLearning.proposals_related_filename) json_data = JSON.parse(File.read(json_file)).each(&:deep_symbolize_keys!) json_data.each do |related| id = related.delete(:id) @@ -306,7 +331,7 @@ class MachineLearning end def import_budget_investments_related_content - json_file = DATA_FOLDER.join(MachineLearning.investments_related_filename) + json_file = data_folder.join(MachineLearning.investments_related_filename) json_data = JSON.parse(File.read(json_file)).each(&:deep_symbolize_keys!) json_data.each do |related| id = related.delete(:id) @@ -335,7 +360,7 @@ class MachineLearning def import_ml_proposals_tags ids = {} - json_file = DATA_FOLDER.join(MachineLearning.proposals_tags_filename) + json_file = data_folder.join(MachineLearning.proposals_tags_filename) json_data = JSON.parse(File.read(json_file)).each(&:deep_symbolize_keys!) json_data.each do |attributes| if attributes[:name].present? @@ -348,7 +373,7 @@ class MachineLearning end end - json_file = DATA_FOLDER.join(MachineLearning.proposals_taggings_filename) + json_file = data_folder.join(MachineLearning.proposals_taggings_filename) json_data = JSON.parse(File.read(json_file)).each(&:deep_symbolize_keys!) json_data.each do |attributes| if attributes[:tag_id].present? @@ -365,7 +390,7 @@ class MachineLearning def import_ml_investments_tags ids = {} - json_file = DATA_FOLDER.join(MachineLearning.investments_tags_filename) + json_file = data_folder.join(MachineLearning.investments_tags_filename) json_data = JSON.parse(File.read(json_file)).each(&:deep_symbolize_keys!) json_data.each do |attributes| if attributes[:name].present? @@ -378,7 +403,7 @@ class MachineLearning end end - json_file = DATA_FOLDER.join(MachineLearning.investments_taggings_filename) + json_file = data_folder.join(MachineLearning.investments_taggings_filename) json_data = JSON.parse(File.read(json_file)).each(&:deep_symbolize_keys!) json_data.each do |attributes| if attributes[:tag_id].present? @@ -421,13 +446,13 @@ class MachineLearning end def last_modified_date_for(filename) - return nil unless File.exists? DATA_FOLDER.join(filename) + return nil unless File.exists? data_folder.join(filename) - File.mtime DATA_FOLDER.join(filename) + File.mtime data_folder.join(filename) end def updated_file?(filename) - return false unless File.exists? DATA_FOLDER.join(filename) + return false unless File.exists? data_folder.join(filename) return true unless previous_modified_date[filename].present? last_modified_date_for(filename) > previous_modified_date[filename] diff --git a/public/machine_learning/scripts/budgets_related_content_and_tags_nmf.py b/public/machine_learning/scripts/budgets_related_content_and_tags_nmf.py index da40f73b3..d43cfbc70 100644 --- a/public/machine_learning/scripts/budgets_related_content_and_tags_nmf.py +++ b/public/machine_learning/scripts/budgets_related_content_and_tags_nmf.py @@ -63,14 +63,17 @@ tqdm_notebook = True # In[2]: +import os +if os.environ.get("CONSUL_TENANT"): + data_path = '../../tenants/' + os.environ["CONSUL_TENANT"] + '/machine_learning/data' +else: + data_path = '../data' -data_path = '../data' config_file = 'budgets_related_content_and_tags_nmf.ini' logging_file ='budgets_related_content_and_tags_nmf.log' # Read the configuration file -import os import configparser config = configparser.ConfigParser() check_file(os.path.join(data_path,config_file)) diff --git a/public/machine_learning/scripts/budgets_summary_comments_textrank.py b/public/machine_learning/scripts/budgets_summary_comments_textrank.py index 1c0faf07b..a1dec2b6f 100644 --- a/public/machine_learning/scripts/budgets_summary_comments_textrank.py +++ b/public/machine_learning/scripts/budgets_summary_comments_textrank.py @@ -60,14 +60,17 @@ tqdm_notebook = True # In[ ]: +import os +if os.environ.get("CONSUL_TENANT"): + data_path = '../../tenants/' + os.environ["CONSUL_TENANT"] + '/machine_learning/data' +else: + data_path = '../data' -data_path = '../data' config_file = 'budgets_summary_comments_textrank.ini' logging_file ='budgets_summary_comments_textrank.log' # Read the configuration file -import os import configparser config = configparser.ConfigParser() check_file(os.path.join(data_path,config_file)) diff --git a/public/machine_learning/scripts/proposals_related_content_and_tags_nmf.py b/public/machine_learning/scripts/proposals_related_content_and_tags_nmf.py index 4c303ad28..df0af7945 100644 --- a/public/machine_learning/scripts/proposals_related_content_and_tags_nmf.py +++ b/public/machine_learning/scripts/proposals_related_content_and_tags_nmf.py @@ -63,14 +63,17 @@ tqdm_notebook = True # In[2]: +import os +if os.environ.get("CONSUL_TENANT"): + data_path = '../../tenants/' + os.environ["CONSUL_TENANT"] + '/machine_learning/data' +else: + data_path = '../data' -data_path = '../data' config_file = 'proposals_related_content_and_tags_nmf.ini' logging_file ='proposals_related_content_and_tags_nmf.log' # Read the configuration file -import os import configparser config = configparser.ConfigParser() check_file(os.path.join(data_path,config_file)) diff --git a/public/machine_learning/scripts/proposals_summary_comments_textrank.py b/public/machine_learning/scripts/proposals_summary_comments_textrank.py index 440083558..ac5d2569a 100644 --- a/public/machine_learning/scripts/proposals_summary_comments_textrank.py +++ b/public/machine_learning/scripts/proposals_summary_comments_textrank.py @@ -60,14 +60,17 @@ tqdm_notebook = True # In[3]: +import os +if os.environ.get("CONSUL_TENANT"): + data_path = '../../tenants/' + os.environ["CONSUL_TENANT"] + '/machine_learning/data' +else: + data_path = '../data' -data_path = '../data' config_file = 'proposals_summary_comments_textrank.ini' logging_file ='proposals_summary_comments_textrank.log' # Read the configuration file -import os import configparser config = configparser.ConfigParser() check_file(os.path.join(data_path,config_file)) diff --git a/spec/models/machine_learning_spec.rb b/spec/models/machine_learning_spec.rb index da0788e15..f3f8af8f7 100644 --- a/spec/models/machine_learning_spec.rb +++ b/spec/models/machine_learning_spec.rb @@ -309,7 +309,7 @@ describe MachineLearning do machine_learning = MachineLearning.new(job) machine_learning.send(:export_proposals_to_json) - json_file = MachineLearning::DATA_FOLDER.join("proposals.json") + json_file = MachineLearning.data_folder.join("proposals.json") json = JSON.parse(File.read(json_file)) expect(json).to be_an Array @@ -335,7 +335,7 @@ describe MachineLearning do machine_learning = MachineLearning.new(job) machine_learning.send(:export_budget_investments_to_json) - json_file = MachineLearning::DATA_FOLDER.join("budget_investments.json") + json_file = MachineLearning.data_folder.join("budget_investments.json") json = JSON.parse(File.read(json_file)) expect(json).to be_an Array @@ -359,7 +359,7 @@ describe MachineLearning do machine_learning = MachineLearning.new(job) machine_learning.send(:export_comments_to_json) - json_file = MachineLearning::DATA_FOLDER.join("comments.json") + json_file = MachineLearning.data_folder.join("comments.json") json = JSON.parse(File.read(json_file)) expect(json).to be_an Array @@ -428,7 +428,7 @@ describe MachineLearning do ] filename = "ml_comments_summaries_proposals.json" - json_file = MachineLearning::DATA_FOLDER.join(filename) + json_file = MachineLearning.data_folder.join(filename) expect(File).to receive(:read).with(json_file).and_return data.to_json machine_learning.send(:import_ml_proposals_comments_summary) @@ -450,7 +450,7 @@ describe MachineLearning do ] filename = "ml_comments_summaries_budgets.json" - json_file = MachineLearning::DATA_FOLDER.join(filename) + json_file = MachineLearning.data_folder.join(filename) expect(File).to receive(:read).with(json_file).and_return data.to_json machine_learning.send(:import_ml_investments_comments_summary) @@ -476,7 +476,7 @@ describe MachineLearning do ] filename = "ml_related_content_proposals.json" - json_file = MachineLearning::DATA_FOLDER.join(filename) + json_file = MachineLearning.data_folder.join(filename) expect(File).to receive(:read).with(json_file).and_return data.to_json machine_learning.send(:import_proposals_related_content) @@ -504,7 +504,7 @@ describe MachineLearning do ] filename = "ml_related_content_budgets.json" - json_file = MachineLearning::DATA_FOLDER.join(filename) + json_file = MachineLearning.data_folder.join(filename) expect(File).to receive(:read).with(json_file).and_return data.to_json machine_learning.send(:import_budget_investments_related_content) @@ -538,11 +538,11 @@ describe MachineLearning do ] tags_filename = "ml_tags_proposals.json" - tags_json_file = MachineLearning::DATA_FOLDER.join(tags_filename) + tags_json_file = MachineLearning.data_folder.join(tags_filename) expect(File).to receive(:read).with(tags_json_file).and_return tags_data.to_json taggings_filename = "ml_taggings_proposals.json" - taggings_json_file = MachineLearning::DATA_FOLDER.join(taggings_filename) + taggings_json_file = MachineLearning.data_folder.join(taggings_filename) expect(File).to receive(:read).with(taggings_json_file).and_return taggings_data.to_json machine_learning.send(:import_ml_proposals_tags) @@ -580,11 +580,11 @@ describe MachineLearning do ] tags_filename = "ml_tags_budgets.json" - tags_json_file = MachineLearning::DATA_FOLDER.join(tags_filename) + tags_json_file = MachineLearning.data_folder.join(tags_filename) expect(File).to receive(:read).with(tags_json_file).and_return tags_data.to_json taggings_filename = "ml_taggings_budgets.json" - taggings_json_file = MachineLearning::DATA_FOLDER.join(taggings_filename) + taggings_json_file = MachineLearning.data_folder.join(taggings_filename) expect(File).to receive(:read).with(taggings_json_file).and_return taggings_data.to_json machine_learning.send(:import_ml_investments_tags) diff --git a/spec/system/admin/machine_learning_spec.rb b/spec/system/admin/machine_learning_spec.rb index 03a476026..d058f8eb9 100644 --- a/spec/system/admin/machine_learning_spec.rb +++ b/spec/system/admin/machine_learning_spec.rb @@ -207,7 +207,7 @@ describe "Machine learning" do end scenario "Show output files info on settins page" do - FileUtils.mkdir_p MachineLearning::DATA_FOLDER + FileUtils.mkdir_p MachineLearning.data_folder allow_any_instance_of(MachineLearning).to receive(:run) do MachineLearningJob.first.update!(finished_at: 2.minutes.from_now) @@ -215,9 +215,9 @@ describe "Machine learning" do script: "proposals_summary_comments_textrank.py", kind: "comments_summary", updated_at: 2.minutes.from_now) - comments_file = MachineLearning::DATA_FOLDER.join(MachineLearning.comments_filename) + comments_file = MachineLearning.data_folder.join(MachineLearning.comments_filename) File.write(comments_file, [].to_json) - proposals_comments_summary_file = MachineLearning::DATA_FOLDER.join(MachineLearning.proposals_comments_summary_filename) + proposals_comments_summary_file = MachineLearning.data_folder.join(MachineLearning.proposals_comments_summary_filename) File.write(proposals_comments_summary_file, [].to_json) end From edd47877c2e58d3b655ff4282ab378e961f57b8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 19 Oct 2022 20:18:06 +0200 Subject: [PATCH 31/33] Extract methods to get tenant subfolder/file paths We were using the same logic in many different places, so we're simplifying the code. I'm not convinced about the method names, though, so we might change them in the future. Note using this method for the default tenant in the `TenantDiskService` class resulted in a `//` in the path, which is probably harmless but very ugly and it also generates a different key than the one we got until now. I've added an extra test to make sure that isn't the case. --- app/models/machine_learning.rb | 10 +--------- app/models/tenant.rb | 12 ++++++++++++ app/views/robots/index.text.erb | 6 +----- config/sitemap.rb | 7 +------ lib/active_storage/service/tenant_disk_service.rb | 2 +- spec/models/attachable_spec.rb | 1 + 6 files changed, 17 insertions(+), 21 deletions(-) diff --git a/app/models/machine_learning.rb b/app/models/machine_learning.rb index 0b680e212..a9504c0a4 100644 --- a/app/models/machine_learning.rb +++ b/app/models/machine_learning.rb @@ -89,15 +89,7 @@ class MachineLearning end def tenant_data_folder - File.join(tenant_subfolder, "machine_learning", "data").delete_prefix("/") - end - - def tenant_subfolder - if Tenant.default? - "" - else - File.join("tenants", Tenant.current_schema) - end + Tenant.path_with_subfolder("machine_learning/data") end def data_output_files diff --git a/app/models/tenant.rb b/app/models/tenant.rb index dbe00b335..45d9e2373 100644 --- a/app/models/tenant.rb +++ b/app/models/tenant.rb @@ -71,6 +71,18 @@ class Tenant < ApplicationRecord end end + def self.subfolder_path + if default? + "" + else + File.join("tenants", current_schema) + end + end + + def self.path_with_subfolder(filename_or_folder) + File.join(subfolder_path, filename_or_folder).delete_prefix("/") + end + def self.default? current_schema == "public" end diff --git a/app/views/robots/index.text.erb b/app/views/robots/index.text.erb index 8a432d78d..29c1e6ce3 100644 --- a/app/views/robots/index.text.erb +++ b/app/views/robots/index.text.erb @@ -14,8 +14,4 @@ Disallow: /*?*locale-switcher Disallow: /*?*filter Disallow: user_id -<% if Tenant.default? %> -Sitemap: <%= "#{root_url}sitemap.xml" %> -<% else %> -Sitemap: <%= "#{root_url}tenants/#{Tenant.current_schema}/sitemap.xml" %> -<% end %> +Sitemap: <%= "#{root_url}#{Tenant.path_with_subfolder("sitemap.xml")}" %> diff --git a/config/sitemap.rb b/config/sitemap.rb index 31f973f2b..ecf4378f3 100644 --- a/config/sitemap.rb +++ b/config/sitemap.rb @@ -10,12 +10,7 @@ SitemapGenerator::Sitemap.verbose = false if Rails.env.test? Tenant.run_on_each do SitemapGenerator::Sitemap.default_host = root_url(Tenant.current_url_options) - - if Tenant.default? - SitemapGenerator::Sitemap.sitemaps_path = nil - else - SitemapGenerator::Sitemap.sitemaps_path = "tenants/#{Tenant.current_schema}" - end + SitemapGenerator::Sitemap.sitemaps_path = Tenant.subfolder_path SitemapGenerator::Sitemap.create do add help_path diff --git a/lib/active_storage/service/tenant_disk_service.rb b/lib/active_storage/service/tenant_disk_service.rb index 63414f295..836983d03 100644 --- a/lib/active_storage/service/tenant_disk_service.rb +++ b/lib/active_storage/service/tenant_disk_service.rb @@ -6,7 +6,7 @@ module ActiveStorage if Tenant.default? super else - super.sub(root, File.join(root, "tenants", Tenant.current_schema)) + super.sub(root, File.join(root, Tenant.subfolder_path)) end end end diff --git a/spec/models/attachable_spec.rb b/spec/models/attachable_spec.rb index 32b208bc8..2cb13825b 100644 --- a/spec/models/attachable_spec.rb +++ b/spec/models/attachable_spec.rb @@ -5,6 +5,7 @@ describe Attachable do file_path = build(:image).file_path expect(file_path).to include "storage/" + expect(file_path).not_to include "storage//" expect(file_path).not_to include "tenants" end From 384057cb48b127f1cbdc554f38a656ca42d608f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 19 Oct 2022 20:24:35 +0200 Subject: [PATCH 32/33] Allow different custom images per tenant Note this only affects images which can also be customized using the administration interface; other images like `avatar_admin.png` must be the same for all tenants. I think this is good enough for now, since the images that can't be different are the images that aren't customized that often, and if there's a need to use different images in a certain CONSUL installation, it can be achieved by changing the code which renders that image so it uses `image_path_for`. --- app/helpers/application_helper.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index e1c35eee5..a0ff6f621 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -53,6 +53,8 @@ module ApplicationHelper if image polymorphic_path(image) + elsif AssetFinder.find_asset(File.join(Tenant.subfolder_path, filename)) + File.join(Tenant.subfolder_path, filename) else filename end From e38b860374e786b8a703740942edfd11a2ad11a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 2 Nov 2022 23:24:35 +0100 Subject: [PATCH 33/33] Use the right tenant after Devise redirections This is something we had read about a long time ago, but didn't find how to reproduce the issue until now. As mentioned in the Apartment documentation: > it's important to consider that you may want to maintain the > "selected" tenant through different parts of the Rack application > stack. For example, the Devise gem adds the Warden::Manager middleware > at the end of the stack in the examples above, our > Apartment::Elevators::Subdomain middleware would come after it. > Trouble is, Apartment resets the selected tenant after the request is > finished, so some redirects (e.g. authentication) in Devise will be > run in the context of the "public" tenant. The same issue would also > effect a gem such as the better_errors gem which inserts a middleware > quite early in the Rails middleware stack. > > To resolve this issue, consider adding the Apartment middleware at a > location in the Rack stack that makes sense for your needs, e.g.: > > Rails.application.config.middleware.insert_before Warden::Manager, > Apartment::Elevators::Subdomain > > Now work done in the Warden middleware is wrapped in the > Apartment::Tenant.switch context started in the Generic elevator. --- config/initializers/apartment.rb | 2 +- spec/system/multitenancy_spec.rb | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/config/initializers/apartment.rb b/config/initializers/apartment.rb index 3088a06c2..511699f4b 100644 --- a/config/initializers/apartment.rb +++ b/config/initializers/apartment.rb @@ -107,7 +107,7 @@ end # Setup a custom Tenant switching middleware. The Proc should return the name of the Tenant that # you want to switch to. -Rails.application.config.middleware.use Apartment::Elevators::Generic, ->(request) do +Rails.application.config.middleware.insert_before Warden::Manager, Apartment::Elevators::Generic, ->(request) do Tenant.resolve_host(request.host) end diff --git a/spec/system/multitenancy_spec.rb b/spec/system/multitenancy_spec.rb index dd1f25e84..45f6d473f 100644 --- a/spec/system/multitenancy_spec.rb +++ b/spec/system/multitenancy_spec.rb @@ -156,4 +156,17 @@ describe "Multitenancy", :seed_tenants do expect(page).to have_content "Invalid Email or username or password." end end + + scenario "Uses the right tenant after failing to sign in" do + with_subdomain("mars") do + visit new_user_session_path + fill_in "Email or username", with: "wrong@consul.dev" + fill_in "Password", with: "wrong" + click_button "Enter" + + expect(page).to have_content "Invalid Email or username or password" + expect(page).to have_css "html.tenant-mars" + expect(page).not_to have_css "html.tenant-public" + end + end end