fix(auth): fix deduce_tenant logic, fix authentication recovery flow

Author: adi-herwana-nus

app/controllers/concerns/application_multitenancy.rb

@@ -25,18 +25,13 @@ 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
-    if Rails.env.development?
-      default_app_host = Application::Application.config.x.default_app_host
+    stripped_host = request.host.downcase.start_with?('www.') ? request.host[4..] : request.host
+    default_host = Application::Application.config.x.default_host&.gsub(/:\d+$/, '')
 
-      if request.host.downcase.ends_with?(default_app_host)
-        request.host.sub(default_app_host, 'coursemology.org')
-      else
-        'coursemology.org'
-      end
-    elsif request.host.downcase.start_with?('www.')
-      request.host[4..]
+    if default_host && stripped_host.downcase.ends_with?(default_host)
+      stripped_host.sub(default_host, 'coursemology.org')
     else
-      request.host
+      stripped_host
     end
   end
 

client/app/bundles/authentication/pages/AuthenticationRedirection/index.tsx

@@ -11,7 +11,7 @@ const AuthenticationRedirection = (): JSX.Element | null => {
     ? `${window.origin}${nextURL}`
     : oidcConfig.redirect_uri;
 
-  if (auth.isAuthenticated) <Redirectable />;
+  if (auth.isAuthenticated) return <Redirectable />;
 
   auth.signinRedirect({ redirect_uri: redirectUri });
 

client/app/lib/components/wrappers/AuthProvider.tsx

@@ -20,7 +20,18 @@ interface AuthProviderProps {
 
 export const INVALID_GRANT_ERROR = 'invalid_grant';
 
+const RECOVERY_ATTEMPTED_KEY = 'oidc_recovery_attempted';
+
+const getCleanCallbackUrl = (): string => {
+  const url = new URL(window.location.href);
+  ['code', 'state', 'session_state', 'iss'].forEach((p) =>
+    url.searchParams.delete(p),
+  );
+  return url.toString();
+};
+
 const onSigninCallback = (_user: User | void): void => {
+  sessionStorage.removeItem(RECOVERY_ATTEMPTED_KEY);
   const url = new URL(window.location.pathname, window.location.origin);
   url.searchParams.set('from', 'auth');
   window.history.replaceState({}, document.title, url.toString());
@@ -40,18 +51,29 @@ export const AUTH_USER_MANAGER = new UserManager(oidcConfig);
 /**
  * Recovers from auth errors that occur during the signin callback, typically
  * caused by a stale or mismatched `state` param (e.g. a bookmarked callback
- * URL, or localStorage cleared between redirect and return). Clears the stale
- * OIDC state and returns the user to the home page to start a fresh login.
+ * URL, or localStorage cleared between redirect and return).
+ *
+ * Clears the stale OIDC state and attempts to re-authenticate the user.
+ * If re-authentication fails again, returns the user to the home page to start a fresh login.
  */
 const AuthErrorRecovery = (): null => {
-  const { error, clearStaleState } = useAuth();
+  const { error, clearStaleState, signinRedirect } = useAuth();
 
   useEffect(() => {
     if (error?.source !== 'signinCallback') return;
-    clearStaleState().finally(() =>
-      window.location.replace(window.location.origin),
-    );
-  }, [error, clearStaleState]);
+
+    const alreadyAttempted = sessionStorage.getItem(RECOVERY_ATTEMPTED_KEY);
+
+    clearStaleState().finally(() => {
+      if (alreadyAttempted) {
+        sessionStorage.removeItem(RECOVERY_ATTEMPTED_KEY);
+        window.location.replace(window.location.origin);
+      } else {
+        sessionStorage.setItem(RECOVERY_ATTEMPTED_KEY, '1');
+        signinRedirect({ redirect_uri: getCleanCallbackUrl() });
+      }
+    });
+  }, [error, clearStaleState, signinRedirect]);
 
   return null;
 };

spec/controllers/application_controller_spec.rb

@@ -65,6 +65,45 @@ def publicly_accessible?
     end
   end
 
+  describe 'ApplicationControllerMultitenancyConcern#deduce_tenant_host' do
+    subject { controller.send(:deduce_tenant_host) }
+
+    before do
+      allow(Application::Application.config.x).to receive(:default_host).
+        and_return('staging.coursemology.org')
+    end
+
+    context 'when the host has a www prefix' do
+      before { @request.host = 'www.example.com' }
+
+      it { is_expected.to eq('example.com') }
+    end
+
+    context 'when the host matches the default host' do
+      before { @request.host = 'staging.coursemology.org' }
+
+      it { is_expected.to eq('coursemology.org') }
+    end
+
+    context 'when the host is a subdomain of the default host' do
+      before { @request.host = 'tenant.staging.coursemology.org' }
+
+      it { is_expected.to eq('tenant.coursemology.org') }
+    end
+
+    context 'when the host has a www prefix and is a subdomain of the default host' do
+      before { @request.host = 'www.tenant.staging.coursemology.org' }
+
+      it { is_expected.to eq('tenant.coursemology.org') }
+    end
+
+    context 'when the host is unrelated to the default host' do
+      before { @request.host = 'tenant.example.com' }
+
+      it { is_expected.to eq('tenant.example.com') }
+    end
+  end
+
   describe ApplicationInternationalizationConcern do
     before { @old_i18n_locale = I18n.locale }
     after { I18n.locale = @old_i18n_locale }