Signup: Adding restrictions for edge cases. (#5206)

* Restricted UsersController#create API when external AuthN is enabled.

* Restricted access to Signup page when external AuthN is enabled.

* Restricted authenticated access to signup API only to admins.

* Hid New User form on admin panel when external AuthN is enabled.

---------

Co-authored-by: Ahmad Farhat <ahmad.af.farhat@gmail.com>

app/controllers/api/v1/api_controller.rb

@@ -88,6 +88,11 @@ module Api
 
         { sort_column => sort_direction }
       end
+
+      # Checks if external authentication is enabled
+      def external_authn_enabled?
+        ENV['OPENID_CONNECT_ISSUER'].present?
+      end
     end
   end
 end

app/controllers/api/v1/users_controller.rb

@@ -39,12 +39,13 @@ module Api
       # POST /api/v1/users.json
       # Creates and saves a new user record in the database with the provided parameters
       def create
-        smtp_enabled = ENV['SMTP_SERVER'].present?
+        return render_error status: :forbidden if external_authn_enabled?
+
         # Check if this is an admin creating a user
         admin_create = current_user && PermissionsChecker.new(current_user:, permission_names: 'ManageUsers', current_provider:).call
 
-        # Users created by a user will have the creator language by default with a fallback to the server configured default_locale.
-        create_user_params[:language] = current_user&.language || I18n.default_locale if create_user_params[:language].blank?
+        # Allow only administrative access for authenticated requests
+        return render_error status: :forbidden if current_user && !admin_create
 
         registration_method = SettingGetter.new(setting_name: 'RegistrationMethod', provider: current_provider).call
 
@@ -52,15 +53,20 @@ module Api
           return render_error errors: Rails.configuration.custom_error_msgs[:invite_token_invalid]
         end
 
-        user = UserCreator.new(user_params: create_user_params.except(:invite_token), provider: current_provider, role: default_role).call
-
-        user.verify! unless smtp_enabled
-
         # TODO: Add proper error logging for non-verified token hcaptcha
         if !admin_create && hcaptcha_enabled? && !verify_hcaptcha(response: params[:token])
           return render_error errors: Rails.configuration.custom_error_msgs[:hcaptcha_invalid]
         end
 
+        # Users created by a user will have the creator language by default with a fallback to the server configured default_locale.
+        create_user_params[:language] = current_user&.language || I18n.default_locale if create_user_params[:language].blank?
+
+        user = UserCreator.new(user_params: create_user_params.except(:invite_token), provider: current_provider, role: default_role).call
+
+        smtp_enabled = ENV['SMTP_SERVER'].present?
+
+        user.verify! unless smtp_enabled
+
         # Set to pending if registration method is approval
         user.pending! if !admin_create && registration_method == SiteSetting::REGISTRATION_METHODS[:approval]
 
@@ -77,9 +83,9 @@ module Api
           return render_data data: user, serializer: CurrentUserSerializer, status: :created unless user.verified?
 
           user.generate_session_token!
-          session[:session_token] = user.session_token unless current_user # if this is NOT an admin creating a user
+          session[:session_token] = user.session_token unless admin_create # if this is NOT an admin creating a user
 
-          render_data data: current_user, serializer: CurrentUserSerializer, status: :created
+          render_data data: user, serializer: CurrentUserSerializer, status: :created
         elsif user.errors.size == 1 && user.errors.of_kind?(:email, :taken)
           render_error errors: Rails.configuration.custom_error_msgs[:email_exists], status: :bad_request
         else

app/javascript/components/admin/manage_users/ManageUsers.jsx

@@ -33,12 +33,14 @@ import InvitedUsersTable from './InvitedUsersTable';
 import PendingUsers from './PendingUsers';
 import BannedUsers from './BannedUsers';
 import { useAuth } from '../../../contexts/auth/AuthProvider';
+import useEnv from '../../../hooks/queries/env/useEnv';
 
 export default function ManageUsers() {
   const { t } = useTranslation();
   const [searchInput, setSearchInput] = useState();
   const { data: registrationMethod } = useSiteSetting('RegistrationMethod');
   const currentUser = useAuth();
+  const envAPI = useEnv();
 
   if (currentUser.permissions?.ManageUsers !== 'true') {
     return <Navigate to="/404" />;
@@ -78,6 +80,9 @@ export default function ManageUsers() {
                               size="md"
                             />
                           )}
+                        {
+                          (!envAPI.isLoading && !envAPI.data?.OPENID_CONNECT)
+                          && (
                             <Modal
                               modalButton={
                                 <Button variant="brand"><UserPlusIcon className="hi-s me-1" /> {t('admin.manage_users.add_new_user')}</Button>
@@ -85,6 +90,8 @@ export default function ManageUsers() {
                               title={t('admin.manage_users.create_new_user')}
                               body={<UserSignupForm />}
                             />
+                          )
+                        }
 
                       </div>
                     </Stack>

app/javascript/components/users/authentication/Signup.jsx

@@ -22,18 +22,29 @@ import { toast } from 'react-toastify';
 import SignupForm from './forms/SignupForm';
 import Logo from '../../shared_components/Logo';
 import useSiteSetting from '../../../hooks/queries/site_settings/useSiteSetting';
+import useEnv from '../../../hooks/queries/env/useEnv';
 
 export default function Signup() {
   const { t } = useTranslation();
   const [searchParams] = useSearchParams();
   const inviteToken = searchParams.get('inviteToken');
-  const { data: registrationMethod } = useSiteSetting('RegistrationMethod');
+  const registrationMethodSettingAPI = useSiteSetting('RegistrationMethod');
+  const envAPI = useEnv();
+  const isLoading = envAPI.isLoading || registrationMethodSettingAPI.isLoading;
 
-  if (registrationMethod === 'invite' && !inviteToken) {
+  if (envAPI.data?.OPENID_CONNECT) {
+    return <Navigate to="/" replace />;
+  }
+
+  if (registrationMethodSettingAPI.data === 'invite' && !inviteToken) {
     toast.error(t('toast.error.users.invalid_invite'));
     return <Navigate to="/" replace />;
   }
 
+  if (isLoading) {
+    return null;
+  }
+
   return (
     <div className="vertical-center">
       <div className="text-center pb-4">

spec/controllers/users_controller_spec.rb

@@ -25,6 +25,7 @@ RSpec.describe Api::V1::UsersController, type: :controller do
 
   before do
     ENV['SMTP_SERVER'] = 'test.com'
+    allow(controller).to receive(:external_authn_enabled?).and_return(false)
     request.headers['ACCEPT'] = 'application/json'
   end
 
@@ -112,28 +113,49 @@ RSpec.describe Api::V1::UsersController, type: :controller do
             user = User.find_by email: user_params[:user][:email]
             expect(user).to be_verified
             expect(session[:session_token]).to eq(user.session_token)
+            expect(ActionMailer::MailDeliveryJob).not_to have_been_enqueued
+          end
+        end
       end
+
+      context 'Authenticated request' do
+        context 'Not admin creation' do
+          let(:signed_in_user) { user }
+
+          before { sign_in_user(signed_in_user) }
+
+          it 'returns :forbidden and does NOT create the user' do
+            expect { post :create, params: user_params }.not_to change(User, :count)
+            expect(ActionMailer::MailDeliveryJob).not_to have_been_enqueued
+
+            expect(response).to have_http_status(:forbidden)
+            expect(session[:session_token]).to eql(signed_in_user.session_token)
           end
         end
 
         context 'Admin creation' do
-        before { sign_in_user(user) }
+          let(:signed_in_user) { user_with_manage_users_permission }
+
+          before { sign_in_user(signed_in_user) }
 
           it 'sends activation email to but does NOT signin the created user' do
             expect { post :create, params: user_params }.to change(User, :count).by(1)
             expect(ActionMailer::MailDeliveryJob).to have_been_enqueued.at(:no_wait).exactly(:once).with('UserMailer', 'activate_account_email',
                                                                                                          'deliver_now', Hash)
             expect(response).to have_http_status(:created)
-          expect(session[:session_token]).to eql(user.session_token)
+            expect(session[:session_token]).to eql(signed_in_user.session_token)
           end
 
           context 'User language' do
             it 'defaults user language to admin language if the language isn\'t specified' do
-            user.update! language: 'language'
+              signed_in_user.update! language: 'language'
 
               user_params[:user][:language] = nil
               post :create, params: user_params
               expect(User.find_by(email: user_params[:user][:email]).language).to eq('language')
+              expect(response).to have_http_status(:created)
+              expect(session[:session_token]).to eql(signed_in_user.session_token)
+            end
           end
         end
       end
@@ -255,6 +277,20 @@ RSpec.describe Api::V1::UsersController, type: :controller do
         end
       end
     end
+
+    context 'External AuthN enabled' do
+      before do
+        allow(controller).to receive(:external_authn_enabled?).and_return(true)
+      end
+
+      it 'returns :forbidden without creating the user account' do
+        expect { post :create, params: user_params }.not_to change(User, :count)
+
+        expect(response).to have_http_status(:forbidden)
+        expect(JSON.parse(response.body)['data']).to be_blank
+        expect(JSON.parse(response.body)['errors']).not_to be_nil
+      end
+    end
   end
 
   describe '#show' do
@@ -417,4 +453,32 @@ RSpec.describe Api::V1::UsersController, type: :controller do
       expect(response).to have_http_status(:forbidden)
     end
   end
+
+  context 'private methods' do
+    describe '#external_authn_enabled?' do
+      before do
+        allow(controller).to receive(:external_authn_enabled?).and_call_original
+      end
+
+      context 'OPENID_CONNECT_ISSUER is present?' do
+        before do
+          ENV['OPENID_CONNECT_ISSUER'] = 'issuer'
+        end
+
+        it 'returns true' do
+          expect(controller).to be_external_authn_enabled
+        end
+      end
+
+      context 'OPENID_CONNECT_ISSUER is NOT present?' do
+        before do
+          ENV['OPENID_CONNECT_ISSUER'] = ''
+        end
+
+        it 'returns false' do
+          expect(controller).not_to be_external_authn_enabled
+        end
+      end
+    end
+  end
 end