From e1e16d21c33286805df241bc3dd9e59ace5b405b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 5 Oct 2022 18:58:17 +0200 Subject: [PATCH] Allow having tenants with different domains Some institutions using CONSUL have expressed interest in this feature since some of their tenants might already have their own domains. We've considered many options for the user interface to select whether we're using a subdomain or a domain, like having two separate fields, using a check box, ... In the end we've chosen radio buttons because they make it easier to follow a logical sequence: first you decide whether you're introducing a domain or subdomain, and then you enter it. We've also considered hiding this option and assuming "if it's got a dot, it's a domain". However, this wouldn't work with nested subdomains and it wouldn't work with domains which are simply machine names. Note that a group of radio buttons (or check boxes) is difficult to style when the text of the label might expand over more than one line (as is the case here on small screens); in this case, most solutions result in the second line of the label appearing immediately under the radio button, instead of being aligned with the first line of the label. That's why I've added a container for the input+label combination. --- .rubocop.yml | 1 + app/assets/javascripts/admin/tenants/form.js | 20 +++++ app/assets/javascripts/application.js | 1 + .../stylesheets/admin/budgets/form.scss | 10 +-- .../stylesheets/admin/tenants/form.scss | 19 +++++ app/assets/stylesheets/mixins/forms.scss | 12 +++ .../admin/tenants/form_component.html.erb | 16 +++- .../admin/tenants/form_component.rb | 16 ++++ .../admin/tenants/index_component.html.erb | 2 + app/controllers/admin/tenants_controller.rb | 2 +- app/models/tenant.rb | 34 ++++++-- config/locales/en/activerecord.yml | 5 +- config/locales/en/admin.yml | 3 + config/locales/es/activerecord.yml | 5 +- config/locales/es/admin.yml | 3 + ...221120123254_add_schema_type_to_tenants.rb | 5 ++ db/schema.rb | 3 +- .../concerns/tenant_variants_spec.rb | 7 ++ spec/factories/administration.rb | 4 + spec/models/tenant_spec.rb | 85 +++++++++++++++++++ spec/system/admin/tenants_spec.rb | 57 ++++++++++--- 21 files changed, 276 insertions(+), 34 deletions(-) create mode 100644 app/assets/javascripts/admin/tenants/form.js create mode 100644 app/assets/stylesheets/admin/tenants/form.scss create mode 100644 db/migrate/20221120123254_add_schema_type_to_tenants.rb diff --git a/.rubocop.yml b/.rubocop.yml index d44255479..9e252d614 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -248,6 +248,7 @@ Rails/DurationArithmetic: Rails/DynamicFindBy: Enabled: true Whitelist: + - find_by_domain - find_by_slug_or_id - find_by_slug_or_id! - find_by_manager_login diff --git a/app/assets/javascripts/admin/tenants/form.js b/app/assets/javascripts/admin/tenants/form.js new file mode 100644 index 000000000..581f4c5ae --- /dev/null +++ b/app/assets/javascripts/admin/tenants/form.js @@ -0,0 +1,20 @@ +(function() { + "use strict"; + App.AdminTenantsForm = { + initialize: function() { + var form = $(".admin .tenant-form"); + var inputs = $("input[name$='[schema_type]']", form); + var label = $("label[for$='schema']", form); + + inputs.on("change", function() { + label.text(label.data("schema-type-" + $(this).val())); + }); + + inputs.each(function() { + if ($(this).is(":checked")) { + $(this).trigger("change"); + } + }); + } + }; +}).call(this); diff --git a/app/assets/javascripts/application.js b/app/assets/javascripts/application.js index 58f344977..b7c650a5e 100644 --- a/app/assets/javascripts/application.js +++ b/app/assets/javascripts/application.js @@ -167,6 +167,7 @@ var initialize_modules = function() { } App.AdminBudgetsWizardCreationStep.initialize(); App.AdminMachineLearningScripts.initialize(); + App.AdminTenantsForm.initialize(); App.AdminVotationTypesFields.initialize(); App.BudgetEditAssociations.initialize(); App.BudgetHideMoney.initialize(); diff --git a/app/assets/stylesheets/admin/budgets/form.scss b/app/assets/stylesheets/admin/budgets/form.scss index 1a928dd48..bb1a87fd5 100644 --- a/app/assets/stylesheets/admin/budgets/form.scss +++ b/app/assets/stylesheets/admin/budgets/form.scss @@ -9,20 +9,12 @@ } > fieldset { - border-top: 4px solid $admin-border-color; + @include admin-fieldset-separator; clear: both; margin-top: $line-height * 1.5; &:first-of-type { margin-top: 0; } - - legend { - color: $admin-text; - font-size: $small-font-size; - font-weight: bold; - padding-right: $line-height / 2; - text-transform: uppercase; - } } } diff --git a/app/assets/stylesheets/admin/tenants/form.scss b/app/assets/stylesheets/admin/tenants/form.scss new file mode 100644 index 000000000..4e68a4752 --- /dev/null +++ b/app/assets/stylesheets/admin/tenants/form.scss @@ -0,0 +1,19 @@ +.admin .tenant-form { + > fieldset { + @include admin-fieldset-separator; + margin-top: $line-height; + } + + .radio-and-label { + display: flex; + margin-bottom: $line-height / 3; + + &:last-of-type { + margin-bottom: $line-height * 2 / 3; + } + + input { + margin-bottom: 0; + } + } +} diff --git a/app/assets/stylesheets/mixins/forms.scss b/app/assets/stylesheets/mixins/forms.scss index 9a42dda08..4516b8b73 100644 --- a/app/assets/stylesheets/mixins/forms.scss +++ b/app/assets/stylesheets/mixins/forms.scss @@ -187,3 +187,15 @@ %public-form { @include public-form; } + +@mixin admin-fieldset-separator { + border-top: 4px solid $admin-border-color; + + > legend { + color: $admin-text; + font-size: $small-font-size; + font-weight: bold; + padding-right: $line-height / 2; + text-transform: uppercase; + } +} diff --git a/app/components/admin/tenants/form_component.html.erb b/app/components/admin/tenants/form_component.html.erb index 1c01da639..16ba5e51f 100644 --- a/app/components/admin/tenants/form_component.html.erb +++ b/app/components/admin/tenants/form_component.html.erb @@ -1,7 +1,19 @@ -<%= form_for [:admin, tenant] do |f| %> +<%= form_for [:admin, tenant], html: { class: "tenant-form" } do |f| %> <%= render "shared/errors", resource: tenant %> <%= f.text_field :name %> - <%= f.text_field :schema %> + +
+ <%= attribute_name(:url) %> +
+ <%= f.radio_button :schema_type, :subdomain, label: t("admin.tenants.form.use_subdomain", domain: domain) %> +
+
+ <%= f.radio_button :schema_type, :domain, label: t("admin.tenants.form.use_domain") %> +
+ + <%= f.text_field :schema, label_options: { data: schema_labels_per_schema_type } %> +
+ <%= f.submit %> <% end %> diff --git a/app/components/admin/tenants/form_component.rb b/app/components/admin/tenants/form_component.rb index 8db70a94b..153a34a44 100644 --- a/app/components/admin/tenants/form_component.rb +++ b/app/components/admin/tenants/form_component.rb @@ -4,4 +4,20 @@ class Admin::Tenants::FormComponent < ApplicationComponent def initialize(tenant) @tenant = tenant end + + private + + def attribute_name(attribute) + Tenant.human_attribute_name(attribute) + end + + def domain + Tenant.default_domain + end + + def schema_labels_per_schema_type + Tenant.schema_types.keys.to_h do |schema_type| + [:"schema_type_#{schema_type}", attribute_name(schema_type)] + end + end end diff --git a/app/components/admin/tenants/index_component.html.erb b/app/components/admin/tenants/index_component.html.erb index 005eb377d..655603ddc 100644 --- a/app/components/admin/tenants/index_component.html.erb +++ b/app/components/admin/tenants/index_component.html.erb @@ -7,6 +7,7 @@ <%= attribute_name(:name) %> <%= attribute_name(:schema) %> + <%= attribute_name(:url) %> <%= t("admin.shared.actions") %> @@ -16,6 +17,7 @@ <%= tenant.name %> <%= tenant.schema %> + <%= tenant.host %> <%= render Admin::TableActionsComponent.new(tenant, actions: [:edit]) do |actions| %> <%= actions.action(:show, text: t("admin.shared.view"), path: root_url(host: tenant.host)) %> diff --git a/app/controllers/admin/tenants_controller.rb b/app/controllers/admin/tenants_controller.rb index 020118ebf..a43e88166 100644 --- a/app/controllers/admin/tenants_controller.rb +++ b/app/controllers/admin/tenants_controller.rb @@ -30,6 +30,6 @@ class Admin::TenantsController < Admin::BaseController private def tenant_params - params.require(:tenant).permit(:name, :schema) + params.require(:tenant).permit(:name, :schema, :schema_type) end end diff --git a/app/models/tenant.rb b/app/models/tenant.rb index d01f610ba..54a254c39 100644 --- a/app/models/tenant.rb +++ b/app/models/tenant.rb @@ -1,4 +1,6 @@ class Tenant < ApplicationRecord + enum schema_type: %w[subdomain domain] + validates :schema, presence: true, uniqueness: true, @@ -10,12 +12,26 @@ class Tenant < ApplicationRecord after_update :rename_schema after_destroy :destroy_schema + def self.find_by_domain(host) + domain.find_by(schema: host) + end + 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 + host_without_www = host.delete_prefix("www.") + + if find_by_domain(host) + host + elsif find_by_domain(host_without_www) + host_without_www + else + host_domain = allowed_domains.find { |domain| host == domain || host.ends_with?(".#{domain}") } + schema = host_without_www.sub(/\.?#{host_domain}\Z/, "").presence + + schema unless find_by_domain(schema) + end end def self.allowed_domains @@ -35,6 +51,14 @@ class Tenant < ApplicationRecord default_url_options[:host] end + def self.default_domain + if default_host == "localhost" + "lvh.me" + else + default_host + end + end + def self.current_url_options default_url_options.merge(host: current_host) end @@ -46,10 +70,10 @@ class Tenant < ApplicationRecord def self.host_for(schema) if schema == "public" default_host - elsif default_host == "localhost" - "#{schema}.lvh.me" + elsif find_by_domain(schema) + schema else - "#{schema}.#{default_host}" + "#{schema}.#{default_domain}" end end diff --git a/config/locales/en/activerecord.yml b/config/locales/en/activerecord.yml index c3800181f..d6563e507 100644 --- a/config/locales/en/activerecord.yml +++ b/config/locales/en/activerecord.yml @@ -382,7 +382,10 @@ en: tag: name: "Type the name of the topic" tenant: - schema: "Subdomain" + domain: "Domain" + schema: "Domain / Subdomain" + subdomain: "Subdomain" + url: "URL" topic: title: "Title" description: "Initial text" diff --git a/config/locales/en/admin.yml b/config/locales/en/admin.yml index 68cbbd3d5..4f59150ad 100644 --- a/config/locales/en/admin.yml +++ b/config/locales/en/admin.yml @@ -1632,6 +1632,9 @@ en: tenants: create: notice: Tenant created successfully + form: + use_subdomain: "Use a subdomain in the %{domain} domain to access this tenant" + use_domain: "Use a different domain to access this tenant" index: create: Create tenant new: diff --git a/config/locales/es/activerecord.yml b/config/locales/es/activerecord.yml index 102beb8f4..ed149e75b 100644 --- a/config/locales/es/activerecord.yml +++ b/config/locales/es/activerecord.yml @@ -382,7 +382,10 @@ es: tag: name: "Escribe el nombre del tema" tenant: - schema: "Subdominio" + domain: "Dominio" + schema: "Dominio / Subdominio" + subdomain: "Subdominio" + url: "URL" topic: title: "Título" description: "Texto inicial" diff --git a/config/locales/es/admin.yml b/config/locales/es/admin.yml index cca53ce34..948c6cf5c 100644 --- a/config/locales/es/admin.yml +++ b/config/locales/es/admin.yml @@ -1631,6 +1631,9 @@ es: tenants: create: notice: Entidad creada correctamente + form: + use_subdomain: "Utiliza un subdominio en el dominio %{domain} para acceder a esta entidad" + use_domain: "Utiliza un dominio distinto para acceder a esta entidad" index: create: Crear entidad new: diff --git a/db/migrate/20221120123254_add_schema_type_to_tenants.rb b/db/migrate/20221120123254_add_schema_type_to_tenants.rb new file mode 100644 index 000000000..322aabe17 --- /dev/null +++ b/db/migrate/20221120123254_add_schema_type_to_tenants.rb @@ -0,0 +1,5 @@ +class AddSchemaTypeToTenants < ActiveRecord::Migration[6.0] + def change + add_column :tenants, :schema_type, :integer, null: false, default: 0 + end +end diff --git a/db/schema.rb b/db/schema.rb index 5ff1ee3d6..f12f53061 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2022_09_15_154808) do +ActiveRecord::Schema.define(version: 2022_11_20_123254) do # These are extensions that must be enabled in order to support this database enable_extension "pg_trgm" @@ -1561,6 +1561,7 @@ ActiveRecord::Schema.define(version: 2022_09_15_154808) do t.string "schema" t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.integer "schema_type", default: 0, null: false t.index ["name"], name: "index_tenants_on_name", unique: true t.index ["schema"], name: "index_tenants_on_schema", unique: true end diff --git a/spec/controllers/concerns/tenant_variants_spec.rb b/spec/controllers/concerns/tenant_variants_spec.rb index 46e949254..58a9742d4 100644 --- a/spec/controllers/concerns/tenant_variants_spec.rb +++ b/spec/controllers/concerns/tenant_variants_spec.rb @@ -20,4 +20,11 @@ describe TenantVariants do get :index expect(response.body).to eq '[:"random-name"]' end + + it "keeps dots in the variant names" do + allow(Tenant).to receive(:current_schema).and_return("random.domain") + + get :index + expect(response.body).to eq '[:"random.domain"]' + end end diff --git a/spec/factories/administration.rb b/spec/factories/administration.rb index de6df90be..d366cb3a3 100644 --- a/spec/factories/administration.rb +++ b/spec/factories/administration.rb @@ -102,5 +102,9 @@ FactoryBot.define do factory :tenant do sequence(:name) { |n| "Tenant #{n}" } sequence(:schema) { |n| "subdomain#{n}" } + + trait :domain do + schema_type { :domain } + end end end diff --git a/spec/models/tenant_spec.rb b/spec/models/tenant_spec.rb index f4e7e32c5..e6d4bae67 100644 --- a/spec/models/tenant_spec.rb +++ b/spec/models/tenant_spec.rb @@ -74,6 +74,49 @@ describe Tenant do expect(Tenant.resolve_host("www.mercury.anotherconsul.dev")).to eq "mercury.anotherconsul.dev" end + it "returns full domains when there's a tenant with a domain including the host" do + insert(:tenant, :domain, schema: "saturn.consul.dev") + + expect(Tenant.resolve_host("saturn.consul.dev")).to eq "saturn.consul.dev" + end + + it "returns subdomains when there's a subdomain-type tenant with that domain" do + insert(:tenant, schema: "saturn.consul.dev") + + expect(Tenant.resolve_host("saturn.consul.dev")).to eq "saturn" + end + + it "returns nil when a domain is accessed as a subdomain" do + insert(:tenant, :domain, schema: "saturn.dev") + + expect(Tenant.resolve_host("saturn.dev.consul.dev")).to be nil + end + + it "returns nested subdomains when there's a subdomain-type tenant with nested subdomains" do + insert(:tenant, schema: "saturn.dev") + + expect(Tenant.resolve_host("saturn.dev.consul.dev")).to eq "saturn.dev" + end + + it "returns domains when there are two tenants resolving to the same domain" do + insert(:tenant, schema: "saturn") + insert(:tenant, :domain, schema: "saturn.consul.dev") + + expect(Tenant.resolve_host("saturn.consul.dev")).to eq "saturn.consul.dev" + end + + it "returns domains when there's a tenant using the default host" do + insert(:tenant, :domain, schema: "consul.dev") + + expect(Tenant.resolve_host("consul.dev")).to eq "consul.dev" + end + + it "returns domains including www when the tenant contains it" do + insert(:tenant, :domain, schema: "www.consul.dev") + + expect(Tenant.resolve_host("www.consul.dev")).to eq "www.consul.dev" + end + context "multitenancy disabled" do before { allow(Rails.application.config).to receive(:multitenancy).and_return(false) } @@ -151,6 +194,18 @@ describe Tenant do expect(Tenant.host_for("uranus")).to eq "uranus.lvh.me" end + + it "ignores the default host when given a full domain" do + insert(:tenant, :domain, schema: "whole.galaxy") + + expect(Tenant.host_for("whole.galaxy")).to eq "whole.galaxy" + end + + it "uses the default host when given nested subdomains" do + insert(:tenant, schema: "whole.galaxy") + + expect(Tenant.host_for("whole.galaxy")).to eq "whole.galaxy.consul.dev" + end end describe ".current_secrets" do @@ -230,6 +285,22 @@ describe Tenant do end end + describe "scopes" do + describe ".domain" do + it "returns tenants with domain schema type" do + insert(:tenant, schema_type: :domain, schema: "full.domain") + + expect(Tenant.domain.pluck(:schema)).to eq ["full.domain"] + end + + it "does not return tenants with subdomain schema type" do + insert(:tenant, schema_type: :subdomain, schema: "nested.subdomain") + + expect(Tenant.domain).to be_empty + end + end + end + describe "validations" do let(:tenant) { build(:tenant) } @@ -273,6 +344,20 @@ describe Tenant do insert(:tenant, name: "Name X") expect(build(:tenant, name: "Name X")).not_to be_valid end + + context "Domain schema type" do + before { tenant.schema_type = :domain } + + it "is valid with domains" do + tenant.schema = "my.domain" + expect(tenant).to be_valid + end + + it "is valid with domains which are machine names" do + tenant.schema = "localmachine" + expect(tenant).to be_valid + end + end end describe "#create_schema" do diff --git a/spec/system/admin/tenants_spec.rb b/spec/system/admin/tenants_spec.rb index 4454fc6fc..bbc0bbf76 100644 --- a/spec/system/admin/tenants_spec.rb +++ b/spec/system/admin/tenants_spec.rb @@ -3,26 +3,51 @@ require "rails_helper" describe "Tenants", :admin, :seed_tenants do before { allow(Tenant).to receive(:default_host).and_return("localhost") } - scenario "Create" do - visit admin_root_path + describe "Create" do + scenario "Tenant with subdomain" do + visit admin_root_path - within("#side_menu") do - click_link "Settings" - click_link "Multitenancy" + 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") do + expect(page).to have_content "earth.lvh.me" + + click_link "View" + end + + 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 - click_link "Create tenant" - fill_in "Subdomain", with: "earth" - fill_in "Name", with: "Earthlings" - click_button "Create tenant" + scenario "Tenant with domain" do + visit new_admin_tenant_path - expect(page).to have_content "Tenant created successfully" + choose "Use a different domain to access this tenant" + fill_in "Domain", with: "earth.lvh.me" + fill_in "Name", with: "Earthlings" + click_button "Create tenant" - within("tr", text: "earth") { click_link "View" } + within("tr", text: "earth") do + expect(page).to have_content "earth.lvh.me" - expect(current_host).to eq "http://earth.lvh.me" - expect(page).to have_current_path root_path - expect(page).to have_link "Sign in" + click_link "View" + end + + 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 end scenario "Update" do @@ -31,6 +56,10 @@ describe "Tenants", :admin, :seed_tenants do visit admin_tenants_path within("tr", text: "moon") { click_link "Edit" } + expect(page).to have_field "Use a subdomain in the lvh.me domain to access this tenant", + type: :radio, + checked: true + fill_in "Subdomain", with: "the-moon" click_button "Update tenant"