From 4653248b9dabca5e86b63579b078d544c236f2b9 Mon Sep 17 00:00:00 2001 From: Christian Sutter Date: Thu, 23 Jan 2025 11:44:49 +0000 Subject: [PATCH 1/4] Improve `User` model - Move `ApplicationController#admin_user?` into the model as `User#admin?` - Add some actual tests for `User` model (including the GDS SSO shared example suite) --- app/controllers/application_controller.rb | 4 ---- app/models/user.rb | 6 ++++++ spec/models/user_spec.rb | 21 +++++++++++++++++++++ 3 files changed, 27 insertions(+), 4 deletions(-) create mode 100644 spec/models/user_spec.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index f7c3b413..6e242b26 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -5,8 +5,4 @@ class ApplicationController < ActionController::Base include GDS::SSO::ControllerMethods before_action :authenticate_user! - - def admin_user? - current_user.permissions.include?("admin") - end end diff --git a/app/models/user.rb b/app/models/user.rb index 48dd9f08..00d8e8dc 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1,5 +1,11 @@ class User < ApplicationRecord + ADMIN_PERMISSION_KEY = "admin".freeze + include GDS::SSO::User serialize :permissions, type: Array + + def admin? + permissions.include?(ADMIN_PERMISSION_KEY) + end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb new file mode 100644 index 00000000..890b031b --- /dev/null +++ b/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 From fb18478a2220e763bc0e9d9495838e8113030444 Mon Sep 17 00:00:00 2001 From: Christian Sutter Date: Thu, 23 Jan 2025 11:50:27 +0000 Subject: [PATCH 2/4] Ensure authentication is properly tested - Instead of setting up authentication based on test type, use RSpec shared context - Ensure test user is actually reset after specs - Add request spec to ensure all controllers in the app are authenticated by default --- spec/requests/authentication_spec.rb | 38 +++++++++++++++++++ spec/spec_helper.rb | 10 +---- .../support/shared_contexts/authentication.rb | 25 ++++++++++++ 3 files changed, 65 insertions(+), 8 deletions(-) create mode 100644 spec/requests/authentication_spec.rb create mode 100644 spec/support/shared_contexts/authentication.rb diff --git a/spec/requests/authentication_spec.rb b/spec/requests/authentication_spec.rb new file mode 100644 index 00000000..9bfb51e9 --- /dev/null +++ b/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 diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 976ac090..39b4b5de 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -31,14 +31,8 @@ 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 SharedContexts::Authentication + config.include_context "with an SSO authenticated user", type: :system config.before(:each, type: :system) do driven_by :rack_test diff --git a/spec/support/shared_contexts/authentication.rb b/spec/support/shared_contexts/authentication.rb new file mode 100644 index 00000000..73b473b0 --- /dev/null +++ b/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 From a2a85417325fe5b1066533b5d6bbfd3cbe35e331 Mon Sep 17 00:00:00 2001 From: Christian Sutter Date: Thu, 23 Jan 2025 12:17:00 +0000 Subject: [PATCH 3/4] Move authentication into a controller concern --- app/controllers/application_controller.rb | 3 +-- app/controllers/concerns/authentication.rb | 10 ++++++++++ 2 files changed, 11 insertions(+), 2 deletions(-) create mode 100644 app/controllers/concerns/authentication.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 6e242b26..2772d0d3 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -3,6 +3,5 @@ class ApplicationController < ActionController::Base # For APIs, you may want to use :null_session instead. protect_from_forgery with: :exception - include GDS::SSO::ControllerMethods - before_action :authenticate_user! + include Authentication end diff --git a/app/controllers/concerns/authentication.rb b/app/controllers/concerns/authentication.rb new file mode 100644 index 00000000..584ce457 --- /dev/null +++ b/app/controllers/concerns/authentication.rb @@ -0,0 +1,10 @@ +# Requires a user to be logged in through GDS SSO for any action. +module Authentication + extend ActiveSupport::Concern + + included do + include GDS::SSO::ControllerMethods + + before_action :authenticate_user! + end +end From 49615eb0eec56eae786b5d852ccbf09d534938db Mon Sep 17 00:00:00 2001 From: Christian Sutter Date: Thu, 23 Jan 2025 12:17:50 +0000 Subject: [PATCH 4/4] Create `Current` model as attributes singleton This uses `ActiveSupport::CurrentAttributes` to track the current user across the request. --- app/controllers/concerns/authentication.rb | 11 +++++++++-- app/controllers/recommended_links_controller.rb | 2 +- app/helpers/application_helper.rb | 4 ++-- app/models/current.rb | 3 +++ 4 files changed, 15 insertions(+), 5 deletions(-) create mode 100644 app/models/current.rb diff --git a/app/controllers/concerns/authentication.rb b/app/controllers/concerns/authentication.rb index 584ce457..33f533a7 100644 --- a/app/controllers/concerns/authentication.rb +++ b/app/controllers/concerns/authentication.rb @@ -1,10 +1,17 @@ -# Requires a user to be logged in through GDS SSO for any action. +# Requires a user to be logged in through GDS SSO for any action, and keeps track of the current +# user through `Current`. module Authentication extend ActiveSupport::Concern included do include GDS::SSO::ControllerMethods - before_action :authenticate_user! + before_action :authenticate_user!, :set_current + end + +private + + def set_current + Current.user = current_user end end diff --git a/app/controllers/recommended_links_controller.rb b/app/controllers/recommended_links_controller.rb index 4ccccb99..e479244e 100644 --- a/app/controllers/recommended_links_controller.rb +++ b/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 diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 7bbeb06f..a9b9137c 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -1,6 +1,6 @@ module ApplicationHelper def navigation_items - return [] unless current_user + return [] unless Current.user [ { @@ -9,7 +9,7 @@ def navigation_items active: controller.controller_name == "recommended_links", }, { - text: current_user.name, + text: Current.user.name, href: Plek.new.external_url_for("signon"), }, { diff --git a/app/models/current.rb b/app/models/current.rb new file mode 100644 index 00000000..73a9744b --- /dev/null +++ b/app/models/current.rb @@ -0,0 +1,3 @@ +class Current < ActiveSupport::CurrentAttributes + attribute :user +end