fix(auth): fix overridden default host handling/redirect

Author: adi-herwana-nus

app/controllers/concerns/application_multitenancy.rb

@@ -15,7 +15,7 @@ def deduce_tenant
     tenant_host = deduce_tenant_host
     instance = Instance.find_tenant_by_host_or_default(tenant_host)
 
-    if Rails.env.production? && instance.default? && instance.host.casecmp(tenant_host) != 0
+    if instance.default? && tenant_host.casecmp(Instance.default.host) != 0
       raise ActionController::RoutingError, 'Instance Not Found'
     end
 
@@ -25,16 +25,26 @@ def deduce_tenant
   # Deduces the current host. We strip any leading www from the host.
   # @return [String] The host, with www removed.
   def deduce_tenant_host
-    stripped_host = request.host.downcase.start_with?('www.') ? request.host[4..] : request.host
-    default_host = Application::Application.config.x.default_host&.gsub(/:\d+$/, '')
+    stripped_host = normalize_tenant_host(request.host)
+    default_host = normalize_tenant_host(Instance.default.host)
 
-    if default_host && stripped_host.downcase.ends_with?(default_host)
+    if default_host && stripped_host == default_host
+      Instance.default.host
+    elsif default_host && stripped_host.ends_with?(default_host)
       stripped_host.sub(default_host, 'coursemology.org')
     else
       stripped_host
     end
   end
 
+  # We store "host" strings in database without port modifiers,
+  # even when Coursemology is configured with them as part of the host (in local development).
+  def normalize_tenant_host(host)
+    return nil unless host
+
+    (host.starts_with?('www.') ? host[4..] : host).gsub(/:\d+$/, '').downcase
+  end
+
   module ClassMethods
     def set_current_tenant_through_filter
       super

app/models/instance.rb

@@ -133,7 +133,7 @@ def default?
 
   # Replace the hostname of the default instance.
   def host
-    default_host = Application::Application.config.x.default_host || ENV.fetch('RAILS_HOSTNAME', 'localhost:8080')
+    default_host = Application::Application.config.x.default_host
     return default_host if default?
 
     read_attribute(:host).gsub('coursemology.org', default_host)

client/app/api/ErrorHandling.ts

@@ -1,9 +1,6 @@
 import { AxiosResponse } from 'axios';
 
-import {
-  AUTH_USER_MANAGER,
-  oidcConfig,
-} from 'lib/components/wrappers/AuthProvider';
+import { AUTH_USER_MANAGER } from 'lib/components/wrappers/AuthProvider';
 import {
   redirectToForbidden,
   redirectToNotFound,
@@ -33,7 +30,7 @@ const isSuspendedResponse = (response?: AxiosResponse): boolean =>
 
 export const redirectIfMatchesErrorIn = (response?: AxiosResponse): void => {
   if (isUnauthenticatedResponse(response))
-    AUTH_USER_MANAGER.signinRedirect({ redirect_uri: oidcConfig.redirect_uri });
+    AUTH_USER_MANAGER.signinRedirect({ redirect_uri: window.location.href });
   if (isSuspendedResponse(response)) redirectToSuspended();
   if (isUnauthorizedResponse(response))
     // Should open a new window and login

config/environments/production.rb

@@ -26,7 +26,7 @@
   # NGINX, varnish or squid.
   # config.action_dispatch.rack_cache = true
   # We will configure the host from the environment.
-  config.action_mailer.default_url_options = { host: ENV['RAILS_HOSTNAME'] }
+  config.action_mailer.default_url_options = { host: ENV.fetch('RAILS_HOSTNAME') }
 
   # Ensures that a master key has been made available in either ENV["RAILS_MASTER_KEY"]
   # or in config/master.key. This key is used to decrypt credentials (and other encrypted files).
@@ -134,7 +134,7 @@
   # config.active_record.database_resolver = ActiveRecord::Middleware::DatabaseSelector::Resolver
   # config.active_record.database_resolver_context = ActiveRecord::Middleware::DatabaseSelector::Resolver::Session
   #
-  config.x.default_host = ENV['RAILS_HOSTNAME']
+  config.x.default_host = ENV.fetch('RAILS_HOSTNAME')
   config.active_job.queue_adapter = :sidekiq
 
   # Rails 6.0.5.1 security patch

spec/controllers/application_controller_spec.rb

@@ -16,6 +16,7 @@ def publicly_accessible?
     end
   end
 
+  let(:instance) { create(:instance) }
   describe ApplicationControllerMultitenancyConcern do
     # These variables override the hostname in the selected tenant object before with_tenant gets
     # to execute. This effectively changes the host requested in the mock request.
@@ -25,10 +26,8 @@ def publicly_accessible?
     with_tenant(:instance) do
       context 'when a nonexistent instance is specified' do
         let(:instance) { build_stubbed(:instance) }
-        it 'falls back to the default instance' do
-          get :index
-
-          expect(ActsAsTenant.current_tenant).to eq(Instance.default)
+        it 'raises a RoutingError' do
+          expect { get :index }.to raise_error(ActionController::RoutingError)
         end
       end
 
@@ -54,11 +53,8 @@ def publicly_accessible?
 
         context 'when the host has a subdomain other than www' do
           let(:instance_host) { "random.#{instance.host.upcase}" }
-          it 'finds the actual host' do
-            get :index
-
-            expect(ActsAsTenant.current_tenant).not_to eq(instance)
-            expect(ActsAsTenant.current_tenant).to eq(Instance.default)
+          it 'raises a RoutingError' do
+            expect { get :index }.to raise_error(ActionController::RoutingError)
           end
         end
       end
@@ -69,8 +65,8 @@ def publicly_accessible?
     subject { controller.send(:deduce_tenant_host) }
 
     before do
-      allow(Application::Application.config.x).to receive(:default_host).
-        and_return('staging.coursemology.org')
+      allow(Instance).to receive(:default).
+        and_return(instance_double(Instance, host: 'staging.coursemology.org'))
     end
 
     context 'when the host has a www prefix' do
@@ -79,10 +75,10 @@ def publicly_accessible?
       it { is_expected.to eq('example.com') }
     end
 
-    context 'when the host matches the default host' do
+    context 'when the host exactly matches the default host' do
       before { @request.host = 'staging.coursemology.org' }
 
-      it { is_expected.to eq('coursemology.org') }
+      it { is_expected.to eq('staging.coursemology.org') }
     end
 
     context 'when the host is a subdomain of the default host' do
@@ -105,82 +101,88 @@ def publicly_accessible?
   end
 
   describe ApplicationInternationalizationConcern do
-    before { @old_i18n_locale = I18n.locale }
-    after { I18n.locale = @old_i18n_locale }
+    with_tenant(:instance) do
+      before { @old_i18n_locale = I18n.locale }
+      after { I18n.locale = @old_i18n_locale }
 
-    pending 'when http accept language is present' do
-      before { @request.env['HTTP_ACCEPT_LANGUAGE'] = 'zh-cn' }
+      context 'when http accept language is present' do
+        before { @request.env['HTTP_ACCEPT_LANGUAGE'] = 'zh' }
 
-      it 'sets the correct locale' do
-        get :index
-        expect(I18n.locale).to eq(:'zh-CN')
+        it 'sets the correct locale' do
+          get :index
+          expect(I18n.locale).to eq(:zh)
+        end
       end
-    end
 
-    context 'when http accept language is not present' do
-      before { @request.env['HTTP_ACCEPT_LANGUAGE'] = nil }
+      context 'when http accept language is not present' do
+        before { @request.env['HTTP_ACCEPT_LANGUAGE'] = nil }
 
-      it 'sets the locale to default' do
-        get :index
-        expect(I18n.locale).to eq(I18n.default_locale)
+        it 'sets the locale to default' do
+          get :index
+          expect(I18n.locale).to eq(I18n.default_locale)
+        end
       end
-    end
 
-    context 'when http accept language is not available' do
-      before do
-        @request.env['HTTP_ACCEPT_LANGUAGE'] = 'jp'
-        get :index
-      end
+      context 'when http accept language is not available' do
+        before do
+          @request.env['HTTP_ACCEPT_LANGUAGE'] = 'jp'
+          get :index
+        end
 
-      it 'sets the locale to default' do
-        expect(I18n.locale).to eq(I18n.default_locale)
+        it 'sets the locale to default' do
+          expect(I18n.locale).to eq(I18n.default_locale)
+        end
       end
-    end
 
-    pending 'when http accept language belongs to the same region' do
-      before { @request.env['HTTP_ACCEPT_LANGUAGE'] = 'zh-tw' }
+      pending 'when http accept language belongs to the same region' do
+        before { @request.env['HTTP_ACCEPT_LANGUAGE'] = 'zh-tw' }
 
-      it 'sets the nearest locale' do
-        get :index
-        expect(I18n.locale).to eq(:'zh-CN')
+        it 'sets the nearest locale' do
+          get :index
+          expect(I18n.locale).to eq(:'zh-CN')
+        end
       end
-    end
 
-    pending 'when multiple http accept languages are present' do
-      before { @request.env['HTTP_ACCEPT_LANGUAGE'] = 'jp, zh-tw' }
+      pending 'when multiple http accept languages are present' do
+        before { @request.env['HTTP_ACCEPT_LANGUAGE'] = 'jp, zh-tw' }
 
-      it 'sets the nearest available locale' do
-        get :index
-        expect(I18n.locale).to eq(:'zh-CN')
+        it 'sets the nearest available locale' do
+          get :index
+          expect(I18n.locale).to eq(:'zh-CN')
+        end
       end
     end
   end
 
   describe ApplicationUserConcern do
-    context 'when the action raises a CanCan::AccessDenied' do
-      run_rescue
+    with_tenant(:instance) do
+      context 'when the action raises a CanCan::AccessDenied' do
+        run_rescue
 
-      it 'returns HTTP status 403' do
-        post :create
-        expect(response.status).to eq(403)
+        it 'returns HTTP status 403' do
+          post :create
+          expect(response.status).to eq(403)
+        end
       end
     end
   end
 
   describe ApplicationComponentsConcern do
-    context 'when the action raises a Coursemology::ComponentNotFoundError' do
-      run_rescue
+    with_tenant(:instance) do
+      context 'when the action raises a Coursemology::ComponentNotFoundError' do
+        run_rescue
 
-      before do
-        def controller.index
-          raise ComponentNotFoundError
+        before do
+          def controller.index
+            raise ComponentNotFoundError
+          end
         end
-      end
 
-      it 'returns HTTP status 404' do
-        get :index
-        expect(response.status).to eq(404)
-        expect(response.body).to include('Component not found')
+        it 'returns HTTP status 404' do
+          get :index
+          expect(response.status).to eq(404)
+          expect(response.body).to include('Component not found')
+        end
       end
     end
   end
@@ -194,34 +196,38 @@ def controller.index
   end
 
   context 'when the action raises an IllegalStateError' do
-    run_rescue
+    with_tenant(:instance) do
+      run_rescue
 
-    before do
-      def controller.index
-        raise IllegalStateError
+      before do
+        def controller.index
+          raise IllegalStateError
+        end
       end
-    end
 
-    it 'returns HTTP status 422' do
-      get :index
-      expect(response.status).to eq(422)
+      it 'returns HTTP status 422' do
+        get :index
+        expect(response.status).to eq(422)
+      end
     end
   end
 
   context 'when the action raises ActionController::InvalidAuthenticityToken' do
-    run_rescue
+    with_tenant(:instance) do
+      run_rescue
 
-    before do
-      def controller.index
-        raise ActionController::InvalidAuthenticityToken
+      before do
+        def controller.index
+          raise ActionController::InvalidAuthenticityToken
+        end
       end
-    end
 
-    it 'returns HTTP status 403' do
-      # Replaced specific error check due to potential false positives
-      # expect { get :index }.to_not raise_error ActionController::InvalidAuthenticityToken
-      expect { get :index }.to_not raise_error
-      expect(response.status).to eq(403)
+      it 'returns HTTP status 403' do
+        # Replaced specific error check due to potential false positives
+        # expect { get :index }.to_not raise_error ActionController::InvalidAuthenticityToken
+        expect { get :index }.to_not raise_error
+        expect(response.status).to eq(403)
+      end
     end
   end
 end