Merge pull request #981 from alphagov/improve-handling-of-invalid-sso…

…-requests

Improve handling of invalid SSO requests

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

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
 

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

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: "[email protected]" }
+        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