Merge pull request #1348 from alphagov/improve-auth

Improve authentication

app/controllers/application_controller.rb

@@ -1,12 +1,7 @@
 class ApplicationController < ActionController::Base
+  include AuthenticatesUser
+
   # Prevent CSRF attacks by raising an exception.
   # For APIs, you may want to use :null_session instead.
   protect_from_forgery with: :exception
-
-  include GDS::SSO::ControllerMethods
-  before_action :authenticate_user!
-
-  def admin_user?
-    current_user.permissions.include?("admin")
-  end
 end

app/controllers/concerns/authenticates_user.rb

@@ -0,0 +1,14 @@
+# Enforces user authentication for a controller and stores the authenticated user in `Current`
+module AuthenticatesUser
+  extend ActiveSupport::Concern
+
+  included do
+    include GDS::SSO::ControllerMethods
+
+    before_action do
+      authenticate_user!
+
+      Current.user = current_user
+    end
+  end
+end

app/controllers/recommended_links_controller.rb

@@ -59,6 +59,6 @@ def set_recommended_link
   def recommended_link_params
     params
       .expect(recommended_link: %i[link title description keywords comment])
-      .merge(user_id: current_user.id)
+      .merge(user_id: Current.user.id)
   end
 end

app/helpers/application_helper.rb

@@ -1,6 +1,6 @@
 module ApplicationHelper
   def navigation_items
-    return [] unless current_user
+    return [] unless Current.user?
 
     [
       {
@@ -14,7 +14,7 @@ def navigation_items
         active: controller.controller_name == "controls",
       },
       {
-        text: current_user.name,
+        text: Current.user.name,
         href: Plek.new.external_url_for("signon"),
       },
       {

app/models/current.rb

@@ -0,0 +1,8 @@
+# Stores cross-cutting concerns for every request
+class Current < ActiveSupport::CurrentAttributes
+  attribute :user
+
+  def user?
+    user.present?
+  end
+end

app/models/user.rb

@@ -1,5 +1,12 @@
 class User < ApplicationRecord
+  # GDS SSO key for administrative permissions
+  ADMIN_PERMISSION_KEY = "admin".freeze
+
   include GDS::SSO::User
 
   serialize :permissions, type: Array
+
+  def admin?
+    permissions.include?(ADMIN_PERMISSION_KEY)
+  end
 end

spec/models/user_spec.rb

@@ -0,0 +1,21 @@
+require "gds-sso/lint/user_spec"
+
+RSpec.describe User, type: :model do
+  it_behaves_like "a gds-sso user class"
+
+  describe "#admin?" do
+    subject(:user) { build_stubbed(:user, permissions:) }
+
+    context "when the user has the `admin` permission" do
+      let(:permissions) { %w[admin and others] }
+
+      it { is_expected.to be_admin }
+    end
+
+    context "when the user does not have the `admin` permission" do
+      let(:permissions) { %w[blah] }
+
+      it { is_expected.not_to be_admin }
+    end
+  end
+end

spec/requests/authentication_spec.rb

@@ -0,0 +1,38 @@
+class FakeController < ApplicationController
+  def hello
+    render plain: "Hello, world!"
+  end
+end
+
+RSpec.describe "Authentication", type: :request do
+  before(:all) do
+    Rails.application.routes.draw do
+      get "hello" => "fake#hello"
+    end
+  end
+
+  after(:all) do
+    Rails.application.reload_routes!
+  end
+
+  context "when the user is authenticated" do
+    include_context "with an SSO authenticated user"
+
+    it "allows the request to proceed" do
+      get hello_path
+
+      expect(response).to have_http_status(:ok)
+      expect(response.body).to eq("Hello, world!")
+    end
+  end
+
+  context "when the user is unauthenticated" do
+    include_context "without an SSO authenticated user"
+
+    it "redirects to the GDS SSO page" do
+      get hello_path
+
+      expect(response).to redirect_to("http://www.example.com/auth/gds")
+    end
+  end
+end

spec/spec_helper.rb

@@ -38,14 +38,7 @@
 
   config.include FactoryBot::Syntax::Methods
 
-  config.before(:all, type: :system) do
-    @user = create(:user)
-    GDS::SSO.test_user = @user
-  end
-
-  config.after(:all, type: :system) do
-    @user.destroy!
-  end
+  config.include_context "with an SSO authenticated user", type: :system
 
   config.before(:each, type: :system) do
     driven_by :rack_test

spec/support/shared_contexts/authentication.rb

@@ -0,0 +1,25 @@
+module SharedContexts
+  module Authentication
+    RSpec.shared_context "with an SSO authenticated user" do
+      let(:sso_user) { create(:user) }
+
+      before do
+        GDS::SSO.test_user = sso_user
+      end
+
+      after do
+        GDS::SSO.test_user = nil
+      end
+    end
+
+    RSpec.shared_context "without an SSO authenticated user" do
+      before do
+        # Unfortunately gds-sso assumes that a nil `test_user` is undesirable, and loads the first
+        # user from the database. Pretending to set this environment variable is the only way to
+        # force an unauthenticated state.
+        allow(ENV).to receive(:[]).and_call_original
+        allow(ENV).to receive(:[]).with("GDS_SSO_MOCK_INVALID").and_return("1")
+      end
+    end
+  end
+end