diff --git a/app/controllers/admin/omniauth_callbacks_controller.rb b/app/controllers/admin/omniauth_callbacks_controller.rb index 78a4fb705..f7442b5e5 100644 --- a/app/controllers/admin/omniauth_callbacks_controller.rb +++ b/app/controllers/admin/omniauth_callbacks_controller.rb @@ -2,12 +2,23 @@ class Admin::OmniauthCallbacksController < Devise::OmniauthCallbacksController skip_before_action :require_admin skip_before_action :verify_authenticity_token, only: %i[saml] + before_action :find_identity_provider, only: :saml + before_action :verify_authentication_data, only: :saml + rescue_from ActiveRecord::RecordNotFound do redirect_to admin_login_url, alert: :login_failed end + rescue_from ActionController::BadRequest do + redirect_to admin_login_url, alert: :invalid_login + end + + def passthru + raise ActionController::BadRequest, "Couldn't find the provider '#{provider_name}'" + end + def saml - @user = AdminUser.find_or_create_from!(provider, auth_data) + @user = AdminUser.find_or_create_from!(@provider, auth_data) if @user.present? sign_in @user, event: :authentication @@ -35,8 +46,32 @@ def auth_data request.env["omniauth.auth"] end - def provider - IdentityProvider.find_by!(name: auth_data.provider) + def provider_name + params.fetch(:provider) + end + + def find_identity_provider + @provider = IdentityProvider.find_by!(name: provider_name) + rescue IdentityProvider::NotFoundError => e + raise ActionController::BadRequest, "Couldn't find the provider '#{provider_name}'" + end + + def verify_authentication_data + unless auth_data.present? + raise ActionController::BadRequest, "Missing authentication data" + end + + %w[uid provider].each do |key| + unless auth_data[key].present? + raise ActionController::BadRequest, "Missing authentication parameter: '#{key}'" + end + end + + %w[first_name last_name groups].each do |key| + unless auth_data.info[key].present? + raise ActionController::BadRequest, "Missing authentication info: '#{key}'" + end + end end def set_refresh_header diff --git a/app/models/identity_provider.rb b/app/models/identity_provider.rb index e49a18987..29a6ce6c3 100644 --- a/app/models/identity_provider.rb +++ b/app/models/identity_provider.rb @@ -1,5 +1,5 @@ class IdentityProvider - class ProviderNotFound < ArgumentError; end + class NotFoundError < ArgumentError; end class << self delegate :each, to: :providers @@ -27,7 +27,7 @@ def load_providers end def raise_provider_not_found(name) - raise ProviderNotFound, "Couldn't find the provider '#{name}'" + raise NotFoundError, "Couldn't find the provider '#{name}'" end end diff --git a/config/application.rb b/config/application.rb index 247b0d9b1..69549c493 100644 --- a/config/application.rb +++ b/config/application.rb @@ -70,6 +70,7 @@ class Application < Rails::Application # Add additional exceptions to the rescue responses config.action_dispatch.rescue_responses.merge!( + "IdentityProvider::NotFoundError" => :bad_request, "Site::PetitionRemoved" => :gone, "Site::ServiceUnavailable" => :service_unavailable, "BulkVerification::InvalidBulkRequest" => :bad_request diff --git a/spec/requests/bad_requests_spec.rb b/spec/requests/bad_requests_spec.rb index df982a87e..a92e1351d 100644 --- a/spec/requests/bad_requests_spec.rb +++ b/spec/requests/bad_requests_spec.rb @@ -21,26 +21,40 @@ expect(response.status).to eq 400 end - context "when logged in as an admin" do + context "when logged in as an admin", csrf: false do let(:user_attributes) do { first_name: "System", last_name: "Administrator", - email: "admin@petition.parliament.uk" + email: "admin@example.com" } end let(:login_params) do - { email: "admin@petition.parliament.uk", password: "L3tme1n!" } + { email: "admin@example.com" } end - let!(:user) { FactoryBot.create(:sysadmin_user, user_attributes) } - before do host! "moderate.petition.parliament.uk" https! - post "/admin/user_sessions", params: { admin_user_session: login_params } + sso_user = FactoryBot.create(:sysadmin_sso_user, **user_attributes) + OmniAuth.config.mock_auth[:example] = sso_user + + post "/admin/login", params: { user: login_params } + + expect(response.status).to eq(307) + expect(response.location).to eq("https://moderate.petition.parliament.uk/admin/auth/example") + + follow_redirect!(params: request.POST) + + expect(response.status).to eq(302) + expect(response.location).to eq("https://moderate.petition.parliament.uk/admin/auth/example/callback") + + follow_redirect! + + expect(response.status).to eq(200) + expect(response).to have_header("Refresh", "0; url=https://moderate.petition.parliament.uk/admin") end context "and uploading a debate outcome image" do @@ -49,7 +63,87 @@ it 'does not return 400 for an image containing null bytes' do patch "/admin/petitions/#{petition.id}/debate-outcome", params: { debate_outcome: { image: image } } - expect(response.status).to eq 302 + + expect(response.status).to eq(302) + expect(response.location).to eq("https://moderate.petition.parliament.uk/admin/petitions/#{petition.id}") + end + end + end + + context "when attempting to login as an admin", csrf: false do + before do + host! "moderate.petition.parliament.uk" + https! + end + + context "on an unknown provider" do + let(:provider) { "/admin/auth/unknown" } + + it "redirects to the login page on the passthru url" do + get "#{provider}" + + expect(response.status).to eq(302) + expect(response.location).to eq("https://moderate.petition.parliament.uk/admin/login") + expect(flash[:alert]).to eq("Invalid login details") + end + + it "redirects to the login page on the callback url" do + get "#{provider}/callback" + + expect(response.status).to eq(302) + expect(response.location).to eq("https://moderate.petition.parliament.uk/admin/login") + expect(flash[:alert]).to eq("Invalid login details") + end + end + + context "on a known provider" do + let(:provider) { "/admin/auth/example" } + + it "redirects to the login page on the passthru url" do + get "#{provider}" + + expect(response.status).to eq(302) + expect(response.location).to eq("https://moderate.petition.parliament.uk/admin/login") + expect(flash[:alert]).to eq("Invalid login details") + end + + it "redirects to the login page on the callback url" do + get "#{provider}/callback" + + expect(response.status).to eq(302) + expect(response.location).to eq("https://moderate.petition.parliament.uk/admin/login") + expect(flash[:alert]).to eq("Invalid login details") + end + + context "with invalid auth data" do + let(:user_attributes) do + { first_name: "", last_name: "", email: "" } + end + + let(:login_params) do + { email: "admin@example.com" } + end + + it "redirects to the login page" do + sso_user = FactoryBot.create(:sso_user, **user_attributes) + OmniAuth.config.mock_auth[:example] = sso_user + + post "/admin/login", params: { user: login_params } + + expect(response.status).to eq(307) + expect(response.location).to eq("https://moderate.petition.parliament.uk/admin/auth/example") + + follow_redirect!(params: request.POST) + + expect(response.status).to eq(302) + expect(response.location).to eq("https://moderate.petition.parliament.uk/admin/auth/example/callback") + + follow_redirect! + + expect(response.status).to eq(302) + expect(response.location).to eq("https://moderate.petition.parliament.uk/admin/login") + expect(flash[:alert]).to eq("Invalid login details") + end end end end