From 5e1dbb48882fb8da1e942762c5a2fa5e62852733 Mon Sep 17 00:00:00 2001
From: Ali Hadi Mazeh <91922430+alihadimazeh@users.noreply.github.com>
Date: Thu, 19 Dec 2024 15:58:49 -0500
Subject: [PATCH] Changes to Allowed Domains (#5979)

* Changes to Allowed Domains

- improved error displays on front end when a user with a not allowed
  domain signs up for both local and external
- added corresponding tests

* rubocop fixes
---
 .rubocop.yml                                             | 6 +++---
 app/controllers/api/v1/users_controller.rb               | 6 +++---
 app/controllers/external_controller.rb                   | 5 +++--
 .../admin/site_settings/registration/Registration.jsx    | 1 +
 app/javascript/components/home/HomePage.jsx              | 3 +++
 app/javascript/hooks/mutations/users/useCreateUser.jsx   | 2 ++
 spec/controllers/external_controller_spec.rb             | 9 +++++++++
 7 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/.rubocop.yml b/.rubocop.yml
index 0d6f51b2a6..fb29a1ecae 100644
--- a/.rubocop.yml
+++ b/.rubocop.yml
@@ -73,7 +73,7 @@ Metrics/ClassLength:
 # A calculated magnitude based on number of assignments,
 # branches, and conditions.
 Metrics/AbcSize:
-  Max: 90
+  Max: 95
 
 Metrics/ParameterLists:
   CountKeywordArgs: false
@@ -82,10 +82,10 @@ RSpec/AnyInstance:
   Enabled: false
 
 Metrics/CyclomaticComplexity:
-  Max: 20
+  Max: 25
 
 Metrics/PerceivedComplexity:
-  Max: 20
+  Max: 25
 
 Rails/Exit:
   Exclude:
diff --git a/app/controllers/api/v1/users_controller.rb b/app/controllers/api/v1/users_controller.rb
index 09c860e4ed..15ad2f57c4 100644
--- a/app/controllers/api/v1/users_controller.rb
+++ b/app/controllers/api/v1/users_controller.rb
@@ -35,7 +35,7 @@ def show
         render_data data: user, status: :ok
       end
 
-      # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity
+      # rubocop:disable Metrics/AbcSize
       # POST /api/v1/users.json
       # Creates and saves a new user record in the database with the provided parameters
       def create
@@ -62,7 +62,7 @@ def create
         create_user_params[:language] = current_user&.language || I18n.default_locale if create_user_params[:language].blank?
 
         # renders an error if the user is signing up with an invalid domain based off site settings
-        return render_error errors: Rails.configuration.custom_error_msgs[:unauthorized], status: :forbidden unless valid_domain?
+        return render_error errors: Rails.configuration.custom_error_msgs[:banned_user], status: :forbidden unless valid_domain?
 
         user = UserCreator.new(user_params: create_user_params.except(:invite_token), provider: current_provider, role: default_role).call
 
@@ -97,7 +97,7 @@ def create
           render_error errors: Rails.configuration.custom_error_msgs[:record_invalid], status: :bad_request
         end
       end
-      # rubocop:enable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity
+      # rubocop:enable Metrics/AbcSize
 
       # PATCH /api/v1/users/:id.json
       # Updates the values of a user
diff --git a/app/controllers/external_controller.rb b/app/controllers/external_controller.rb
index 8b512663b1..d552dae03a 100644
--- a/app/controllers/external_controller.rb
+++ b/app/controllers/external_controller.rb
@@ -48,9 +48,10 @@ def create_user
       return redirect_to root_path(error: Rails.configuration.custom_error_msgs[:invite_token_invalid])
     end
 
-    return render_error status: :forbidden unless valid_domain?(user_info[:email])
+    # Redirect to root if the user doesn't exist and has an invalid domain
+    return redirect_to root_path(error: Rails.configuration.custom_error_msgs[:banned_user]) if new_user && !valid_domain?(user_info[:email])
 
-    # Create the user if they dont exist
+    # Create the user if they don't exist
     if new_user
       user = UserCreator.new(user_params: user_info, provider: current_provider, role: default_role).call
       user.save!
diff --git a/app/javascript/components/admin/site_settings/registration/Registration.jsx b/app/javascript/components/admin/site_settings/registration/Registration.jsx
index 7d9029666f..764e5b00f5 100644
--- a/app/javascript/components/admin/site_settings/registration/Registration.jsx
+++ b/app/javascript/components/admin/site_settings/registration/Registration.jsx
@@ -108,6 +108,7 @@ export default function Registration() {
           <input
             className="form-control"
             placeholder={t('admin.site_settings.registration.enter_allowed_domains_rule')}
+            defaultValue={siteSettings?.AllowedDomains}
           />
           <Button
             variant="brand"
diff --git a/app/javascript/components/home/HomePage.jsx b/app/javascript/components/home/HomePage.jsx
index f4f520b8e3..80728eb1f5 100644
--- a/app/javascript/components/home/HomePage.jsx
+++ b/app/javascript/components/home/HomePage.jsx
@@ -59,6 +59,9 @@ export default function HomePage() {
       case 'SignupError':
         toast.error(t('toast.error.users.signup_error'));
         break;
+      case 'BannedUser':
+        toast.error(t('toast.error.users.banned'));
+        break;
       default:
     }
     if (error) { setSearchParams(searchParams.delete('error')); }
diff --git a/app/javascript/hooks/mutations/users/useCreateUser.jsx b/app/javascript/hooks/mutations/users/useCreateUser.jsx
index 3f510c9d4f..649e0acdbd 100644
--- a/app/javascript/hooks/mutations/users/useCreateUser.jsx
+++ b/app/javascript/hooks/mutations/users/useCreateUser.jsx
@@ -51,6 +51,8 @@ export default function useCreateUser() {
           toast.error(t('toast.error.users.invalid_invite'));
         } else if (err.response.data.errors === 'EmailAlreadyExists') {
           toast.error(t('toast.error.users.email_exists'));
+        } else if (err.response.data.errors === 'BannedUser') {
+          toast.error(t('toast.error.users.banned'));
         } else {
           toast.error(t('toast.error.problem_completing_action'));
         }
diff --git a/spec/controllers/external_controller_spec.rb b/spec/controllers/external_controller_spec.rb
index 764690dbc1..05b463e6c4 100644
--- a/spec/controllers/external_controller_spec.rb
+++ b/spec/controllers/external_controller_spec.rb
@@ -371,6 +371,15 @@
 
           expect { get :create_user, params: { provider: 'openid_connect' } }.not_to change(User, :count)
         end
+
+        it 'does not affect existing users with different domains' do
+          request.env['omniauth.auth'] = OmniAuth.config.mock_auth[:openid_connect]
+
+          create(:user, external_id: OmniAuth.config.mock_auth[:openid_connect][:uid])
+
+          get :create_user, params: { provider: 'openid_connect' }
+          expect(response).not_to redirect_to(root_path(error: Rails.configuration.custom_error_msgs[:banned_user]))
+        end
       end
 
       context 'restricted domain set to multiple domain' do