From a21088d7adc7cdaa2c39004b7fae48713a8de8aa Mon Sep 17 00:00:00 2001 From: Ahmad Farhat <ahmad.af.farhat@gmail.com> Date: Mon, 20 Nov 2023 14:17:26 -0500 Subject: [PATCH] Prevent local users from signing in if external accounts is enabled (#5542) --- app/controllers/api/v1/sessions_controller.rb | 3 +- app/controllers/concerns/authorizable.rb | 5 + spec/controllers/sessions_controller_spec.rb | 110 +++++++++++------- 3 files changed, 77 insertions(+), 41 deletions(-) diff --git a/app/controllers/api/v1/sessions_controller.rb b/app/controllers/api/v1/sessions_controller.rb index 41eb19cb..bbf1372a 100644 --- a/app/controllers/api/v1/sessions_controller.rb +++ b/app/controllers/api/v1/sessions_controller.rb @@ -20,6 +20,7 @@ module Api module V1 class SessionsController < ApiController skip_before_action :ensure_authenticated, only: %i[index create] + before_action :ensure_unauthenticated, only: :create # GET /api/v1/sessions # Returns the current_user @@ -43,7 +44,7 @@ module Api return render_error if user.blank? # Will return an error if the user is NOT from the current provider and if the user is NOT a super admin - return render_error if user.provider != current_provider && !user.super_admin? + return render_error status: :forbidden if !user.super_admin? && (user.provider != current_provider || external_auth?) # Password is not set (local user migrated from v2) if user.external_id.blank? && user.password_digest.blank? diff --git a/app/controllers/concerns/authorizable.rb b/app/controllers/concerns/authorizable.rb index cea926b7..4b8b4049 100644 --- a/app/controllers/concerns/authorizable.rb +++ b/app/controllers/concerns/authorizable.rb @@ -29,6 +29,11 @@ module Authorizable render_error status: :unauthorized unless current_user end + # Ensures that the user is NOT logged in + def ensure_unauthenticated + render_error status: :unauthorized if current_user + end + # PermissionsChecker service will return a true or false depending on whether the current_user's role has the provided permission_name def ensure_authorized(permission_names, user_id: nil, friendly_id: nil, record_id: nil) render_error status: :forbidden unless PermissionsChecker.new( diff --git a/spec/controllers/sessions_controller_spec.rb b/spec/controllers/sessions_controller_spec.rb index e2202d26..4b234930 100644 --- a/spec/controllers/sessions_controller_spec.rb +++ b/spec/controllers/sessions_controller_spec.rb @@ -54,46 +54,6 @@ RSpec.describe Api::V1::SessionsController, type: :controller do expect(session[:session_token]).to eq(user.session_token) end - it 'returns UnverifiedUser error if the user is not verified' do - unverified_user = create(:user, password: 'Password1!', verified: false) - - post :create, params: { - session: { - email: unverified_user.email, - password: 'Password1!' - } - } - - expect(response.parsed_body['data']).to eq(unverified_user.id) - expect(response.parsed_body['errors']).to eq('UnverifiedUser') - end - - it 'returns BannedUser error if the user is banned' do - banned_user = create(:user, password: 'Password1!', status: :banned) - - post :create, params: { - session: { - email: banned_user.email, - password: 'Password1!' - } - } - - expect(response.parsed_body['errors']).to eq('BannedUser') - end - - it 'returns Pending error if the user is banned' do - banned_user = create(:user, password: 'Password1!', status: :pending) - - post :create, params: { - session: { - email: banned_user.email, - password: 'Password1!' - } - } - - expect(response.parsed_body['errors']).to eq('PendingUser') - end - it 'logs in with greenlight account before bn account' do post :create, params: { session: { email: user.email, password: 'Password1!' } } expect(response).to have_http_status(:ok) @@ -107,6 +67,76 @@ RSpec.describe Api::V1::SessionsController, type: :controller do expect(response).to have_http_status(:ok) expect(session[:session_token]).to eq(super_admin.reload.session_token) end + + context 'errors' do + it 'returns unauthorized if the user is already signed in' do + sign_in_user(user) + + post :create, params: { + session: { + email: 'email@email.com', + password: 'Password1!', + extend_session: false + } + }, as: :json + + expect(response).to be_unauthorized + end + + it 'returns forbidden if the external auth is enabled' do + allow(controller).to receive(:external_auth?).and_return(true) + + post :create, params: { + session: { + email: 'email@email.com', + password: 'Password1!', + extend_session: false + } + }, as: :json + + expect(response).to be_forbidden + end + + it 'returns UnverifiedUser error if the user is not verified' do + unverified_user = create(:user, password: 'Password1!', verified: false) + + post :create, params: { + session: { + email: unverified_user.email, + password: 'Password1!' + } + } + + expect(response.parsed_body['data']).to eq(unverified_user.id) + expect(response.parsed_body['errors']).to eq('UnverifiedUser') + end + + it 'returns BannedUser error if the user is banned' do + banned_user = create(:user, password: 'Password1!', status: :banned) + + post :create, params: { + session: { + email: banned_user.email, + password: 'Password1!' + } + } + + expect(response.parsed_body['errors']).to eq('BannedUser') + end + + it 'returns Pending error if the user is banned' do + banned_user = create(:user, password: 'Password1!', status: :pending) + + post :create, params: { + session: { + email: banned_user.email, + password: 'Password1!' + } + } + + expect(response.parsed_body['errors']).to eq('PendingUser') + end + end end describe '#destroy' do -- GitLab