diff --git a/app/controllers/api/v1/api_controller.rb b/app/controllers/api/v1/api_controller.rb index 42325a7e92897492bfbfa8f1e7405eb32e262250..502124b701a86aab21001b1bbe732a9c000675b6 100644 --- a/app/controllers/api/v1/api_controller.rb +++ b/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 diff --git a/app/controllers/api/v1/users_controller.rb b/app/controllers/api/v1/users_controller.rb index 7017e1ed392b2f8545d50e80e95c6f7272e32734..3632789df67588e33572daa9e5a77f3ad50f00ac 100644 --- a/app/controllers/api/v1/users_controller.rb +++ b/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 diff --git a/app/javascript/components/admin/manage_users/ManageUsers.jsx b/app/javascript/components/admin/manage_users/ManageUsers.jsx index b7f896556167c96782063ec70b47506b6706b302..c72d9eef3b67c0272b41080bae8f9422c743b7c3 100644 --- a/app/javascript/components/admin/manage_users/ManageUsers.jsx +++ b/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" />; @@ -46,7 +48,7 @@ export default function ManageUsers() { return ( <div id="admin-panel" className="pb-3"> - <h3 className="py-5">{ t('admin.admin_panel') }</h3> + <h3 className="py-5">{t('admin.admin_panel')}</h3> <Card className="border-0 card-shadow"> <Tab.Container activeKey="users"> <Row> @@ -59,32 +61,37 @@ export default function ManageUsers() { <Tab.Content className="p-0"> <Container className="admin-table p-0"> <div className="p-4 border-bottom"> - <h3>{ t('admin.manage_users.manage_users') }</h3> + <h3>{t('admin.manage_users.manage_users')}</h3> </div> <div className="p-4"> <Stack direction="horizontal" className="mb-4"> <SearchBar searchInput={searchInput} setSearchInput={setSearchInput} /> <div className="ms-auto"> - { registrationMethod === 'invite' + {registrationMethod === 'invite' && ( - <Modal - modalButton={( - <Button variant="brand-outline" className="me-3"> - <EnvelopeIcon className="hi-s me-1" />{ t('admin.manage_users.invite_user') } - </Button> - )} - title={t('admin.manage_users.invite_user')} - body={<InviteUserForm />} - size="md" - /> + <Modal + modalButton={( + <Button variant="brand-outline" className="me-3"> + <EnvelopeIcon className="hi-s me-1" />{t('admin.manage_users.invite_user')} + </Button> + )} + title={t('admin.manage_users.invite_user')} + body={<InviteUserForm />} + size="md" + /> )} - <Modal - modalButton={ - <Button variant="brand"><UserPlusIcon className="hi-s me-1" /> { t('admin.manage_users.add_new_user') }</Button> - } - title={t('admin.manage_users.create_new_user')} - body={<UserSignupForm />} - /> + { + (!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> + } + title={t('admin.manage_users.create_new_user')} + body={<UserSignupForm />} + /> + ) + } </div> </Stack> @@ -92,7 +99,7 @@ export default function ManageUsers() { <Tab eventKey="active" title={t('admin.manage_users.active')}> <VerifiedUsers searchInput={searchInput} /> </Tab> - { registrationMethod === 'approval' + {registrationMethod === 'approval' && ( <Tab eventKey="pending" title={t('admin.manage_users.pending')}> <PendingUsers searchInput={searchInput} /> @@ -101,11 +108,11 @@ export default function ManageUsers() { <Tab eventKey="banned" title={t('admin.manage_users.banned')}> <BannedUsers searchInput={searchInput} /> </Tab> - { registrationMethod === 'invite' + {registrationMethod === 'invite' && ( - <Tab eventKey="invited" title={t('admin.manage_users.invited_tab')}> - <InvitedUsersTable input={searchInput} /> - </Tab> + <Tab eventKey="invited" title={t('admin.manage_users.invited_tab')}> + <InvitedUsersTable input={searchInput} /> + </Tab> )} </Tabs> </div> diff --git a/app/javascript/components/users/authentication/Signup.jsx b/app/javascript/components/users/authentication/Signup.jsx index 90670b39564c2b33ea4e2bd2edc00940619308f2..b5306175dc5447a18e874738d9c5e3fd6577be28 100644 --- a/app/javascript/components/users/authentication/Signup.jsx +++ b/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"> diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index 471507ced1deeeaf350788c1fbc7bd0e5fc7843a..cb97aa817166795af406d6ccf0d081696c72b5b5 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/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 'Admin creation' do - before { sign_in_user(user) } + context 'Authenticated request' do + context 'Not admin creation' do + let(:signed_in_user) { 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) + 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 'User language' do - it 'defaults user language to admin language if the language isn\'t specified' do - user.update! language: 'language' + context 'Admin creation' do + let(:signed_in_user) { user_with_manage_users_permission } - user_params[:user][:language] = nil - post :create, params: user_params - expect(User.find_by(email: user_params[:user][:email]).language).to eq('language') + 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(signed_in_user.session_token) + end + + context 'User language' do + it 'defaults user language to admin language if the language isn\'t specified' do + 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