From 8998a7e6b48e239a8d05718c3a1dce4a68577222 Mon Sep 17 00:00:00 2001 From: Andrew White Date: Tue, 3 Sep 2024 17:28:33 +0100 Subject: [PATCH 1/2] Fix false positive spec The 302 redirect was for the login page and not the petition page. --- spec/requests/bad_requests_spec.rb | 30 +++++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/spec/requests/bad_requests_spec.rb b/spec/requests/bad_requests_spec.rb index df982a87e..e7603e15c 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,9 @@ 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 From 3480480953bb0c4a560d7f3c84c222c75c177b9a Mon Sep 17 00:00:00 2001 From: Andrew White Date: Tue, 3 Sep 2024 18:20:43 +0100 Subject: [PATCH 2/2] Improve handling of invalid SSO requests Return an invalid login message for unknown SSO providers and invalid authentication data. --- .../admin/omniauth_callbacks_controller.rb | 41 +++++++++- app/models/identity_provider.rb | 4 +- config/application.rb | 1 + spec/requests/bad_requests_spec.rb | 78 +++++++++++++++++++ 4 files changed, 119 insertions(+), 5 deletions(-) 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 e7603e15c..a92e1351d 100644 --- a/spec/requests/bad_requests_spec.rb +++ b/spec/requests/bad_requests_spec.rb @@ -69,4 +69,82 @@ 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 end