From c7db5102f6ee17d32af4bf5087d976e054ff83dc Mon Sep 17 00:00:00 2001 From: Phil Lee Date: Tue, 2 May 2023 09:50:14 +0100 Subject: [PATCH] CLDC-2294 Add authorization to bulk upload results (#1591) # Context - https://digital.dclg.gov.uk/jira/browse/CLDC-2294 - When bulk upload returns a results page it would be useful if colleagues of the uploader can see this page to help fix errors - It would also be useful if support users can see these reports to help diagnose bulk upload errors # Changes - Added `pundit` gem to handle authorization - Bulk upload results previously only accessible to the bulk uploader. Now they can be seen by users in the same org as the uploader and also support users --- Gemfile | 3 ++ Gemfile.lock | 3 ++ app/controllers/application_controller.rb | 8 ++++ ...bulk_upload_lettings_results_controller.rb | 8 +++- .../bulk_upload_sales_results_controller.rb | 10 ++++- app/policies/bulk_upload_policy.rb | 26 ++++++++++++ .../application_controller_spec.rb | 23 +++++++++++ ...upload_lettings_results_controller_spec.rb | 13 ++++++ ...lk_upload_sales_results_controller_spec.rb | 33 +++++++++++++++ spec/policies/bulk_upload_policy_spec.rb | 39 ++++++++++++++++++ spec/rails_helper.rb | 1 + ...upload_lettings_results_controller_spec.rb | 41 ++++++++++++++++++- 12 files changed, 203 insertions(+), 5 deletions(-) create mode 100644 app/policies/bulk_upload_policy.rb create mode 100644 spec/controllers/application_controller_spec.rb create mode 100644 spec/controllers/bulk_upload_sales_results_controller_spec.rb create mode 100644 spec/policies/bulk_upload_policy_spec.rb diff --git a/Gemfile b/Gemfile index f23a4db6f..5322a25b2 100644 --- a/Gemfile +++ b/Gemfile @@ -48,6 +48,9 @@ gem "aws-sdk-s3" gem "paper_trail" # Store active record objects in version whodunnits gem "paper_trail-globalid" + +gem "pundit" + # Request rate limiting gem "rack", ">= 2.2.6.3" gem "rack-attack" diff --git a/Gemfile.lock b/Gemfile.lock index 78fe1e157..98267f775 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -277,6 +277,8 @@ GEM public_suffix (5.0.1) puma (5.6.5) nio4r (~> 2.0) + pundit (2.3.0) + activesupport (>= 3.0.0) raabro (1.4.0) racc (1.6.2) rack (2.2.6.4) @@ -478,6 +480,7 @@ DEPENDENCIES propshaft pry-byebug puma (~> 5.0) + pundit rack (>= 2.2.6.3) rack-attack rack-mini-profiler (~> 2.0) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 5685796af..ff085e6dc 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -1,10 +1,18 @@ class ApplicationController < ActionController::Base + include Pundit::Authorization + + rescue_from Pundit::NotAuthorizedError, with: :render_not_authorized + before_action :set_paper_trail_whodunnit def render_not_found render "errors/not_found", status: :not_found end + def render_not_authorized + render "errors/not_found", status: :unauthorized + end + def render_not_found_json(class_name, id) render json: { error: "#{class_name} #{id} not found" }, status: :not_found end diff --git a/app/controllers/bulk_upload_lettings_results_controller.rb b/app/controllers/bulk_upload_lettings_results_controller.rb index 6f92c0375..90008bcd2 100644 --- a/app/controllers/bulk_upload_lettings_results_controller.rb +++ b/app/controllers/bulk_upload_lettings_results_controller.rb @@ -4,7 +4,9 @@ class BulkUploadLettingsResultsController < ApplicationController rescue_from ActiveRecord::RecordNotFound, with: :render_not_found def show - @bulk_upload = current_user.bulk_uploads.lettings.find(params[:id]) + @bulk_upload = BulkUpload.lettings.find(params[:id]) + + authorize @bulk_upload end def resume @@ -20,7 +22,9 @@ class BulkUploadLettingsResultsController < ApplicationController end def summary - @bulk_upload = current_user.bulk_uploads.lettings.find(params[:id]) + @bulk_upload = BulkUpload.lettings.find(params[:id]) + + authorize @bulk_upload end private diff --git a/app/controllers/bulk_upload_sales_results_controller.rb b/app/controllers/bulk_upload_sales_results_controller.rb index c70907bbe..feb7b3e06 100644 --- a/app/controllers/bulk_upload_sales_results_controller.rb +++ b/app/controllers/bulk_upload_sales_results_controller.rb @@ -4,7 +4,9 @@ class BulkUploadSalesResultsController < ApplicationController rescue_from ActiveRecord::RecordNotFound, with: :render_not_found def show - @bulk_upload = current_user.bulk_uploads.sales.find(params[:id]) + @bulk_upload = BulkUpload.sales.find(params[:id]) + + authorize @bulk_upload end def resume @@ -20,9 +22,13 @@ class BulkUploadSalesResultsController < ApplicationController end def summary - @bulk_upload = current_user.bulk_uploads.sales.find(params[:id]) + @bulk_upload = BulkUpload.sales.find(params[:id]) + + authorize @bulk_upload end +private + def reset_logs_filters session["logs_filters"] = {}.to_json end diff --git a/app/policies/bulk_upload_policy.rb b/app/policies/bulk_upload_policy.rb new file mode 100644 index 000000000..8c609e1d8 --- /dev/null +++ b/app/policies/bulk_upload_policy.rb @@ -0,0 +1,26 @@ +class BulkUploadPolicy + attr_reader :user, :bulk_upload + + def initialize(user, bulk_upload) + @user = user + @bulk_upload = bulk_upload + end + + def summary? + owner? || same_org? || user.support? + end + + def show? + owner? || same_org? || user.support? + end + +private + + def owner? + bulk_upload.user == user + end + + def same_org? + bulk_upload.user.organisation.users.include?(user) + end +end diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb new file mode 100644 index 000000000..f120eb02a --- /dev/null +++ b/spec/controllers/application_controller_spec.rb @@ -0,0 +1,23 @@ +require "rails_helper" + +RSpec.describe ApplicationController do + describe "when Pundit::NotAuthorizedError raised" do + render_views + + controller do + def index + raise Pundit::NotAuthorizedError, "error goes here" + end + end + + it "returns status 401 unauthorized" do + get :index + expect(response).to be_unauthorized + end + + it "renders page not found" do + get :index + expect(response.body).to have_content("Page not found") + end + end +end diff --git a/spec/controllers/bulk_upload_lettings_results_controller_spec.rb b/spec/controllers/bulk_upload_lettings_results_controller_spec.rb index 7fb50a76a..c49511425 100644 --- a/spec/controllers/bulk_upload_lettings_results_controller_spec.rb +++ b/spec/controllers/bulk_upload_lettings_results_controller_spec.rb @@ -5,6 +5,19 @@ RSpec.describe BulkUploadLettingsResultsController do sign_in user end + describe "#show" do + let(:user) { create(:user) } + let(:bulk_upload) { create(:bulk_upload, :lettings, user:) } + + it "passes thru pundit" do + allow(controller).to receive(:authorize) + + get :show, params: { id: bulk_upload.id } + + expect(controller).to have_received(:authorize) + end + end + describe "GET #resume /lettings-logs/bulk-upload-results/:ID/resume" do let(:user) { create(:user) } let(:bulk_upload) { create(:bulk_upload, :lettings, user:) } diff --git a/spec/controllers/bulk_upload_sales_results_controller_spec.rb b/spec/controllers/bulk_upload_sales_results_controller_spec.rb new file mode 100644 index 000000000..e2cc46291 --- /dev/null +++ b/spec/controllers/bulk_upload_sales_results_controller_spec.rb @@ -0,0 +1,33 @@ +require "rails_helper" + +RSpec.describe BulkUploadSalesResultsController do + before do + sign_in user + end + + describe "#show" do + let(:user) { create(:user) } + let(:bulk_upload) { create(:bulk_upload, :sales, user:) } + + it "passes thru pundit" do + allow(controller).to receive(:authorize) + + get :show, params: { id: bulk_upload.id } + + expect(controller).to have_received(:authorize) + end + end + + describe "#summary" do + let(:user) { create(:user) } + let(:bulk_upload) { create(:bulk_upload, :sales, user:) } + + it "passes thru pundit" do + allow(controller).to receive(:authorize) + + get :summary, params: { id: bulk_upload.id } + + expect(controller).to have_received(:authorize) + end + end +end diff --git a/spec/policies/bulk_upload_policy_spec.rb b/spec/policies/bulk_upload_policy_spec.rb new file mode 100644 index 000000000..a99af5584 --- /dev/null +++ b/spec/policies/bulk_upload_policy_spec.rb @@ -0,0 +1,39 @@ +require "rails_helper" + +RSpec.describe BulkUploadPolicy do + subject(:policy) { described_class } + + permissions :summary?, :show? do + it "grants access to owner" do + user = build(:user) + bulk_upload = build(:bulk_upload, user:) + + expect(policy).to permit(user, bulk_upload) + end + + it "grants access to user from same org as uploader" do + user = create(:user) + organisation = user.organisation + other_user = create(:user, organisation:) + bulk_upload = create(:bulk_upload, user:) + + expect(policy).to permit(other_user, bulk_upload) + end + + it "grants access to support" do + user = create(:user) + support_user = create(:user, :support) + bulk_upload = create(:bulk_upload, user:) + + expect(policy).to permit(support_user, bulk_upload) + end + + it "denies access to random users" do + user = create(:user) + other_user = create(:user) + bulk_upload = create(:bulk_upload, user:) + + expect(policy).not_to permit(other_user, bulk_upload) + end + end +end diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 8e175ccf6..36cf81b99 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -9,6 +9,7 @@ require "capybara/rspec" require "capybara-screenshot/rspec" require "selenium-webdriver" require "view_component/test_helpers" +require "pundit/rspec" Capybara.register_driver :headless do |app| options = Selenium::WebDriver::Firefox::Options.new diff --git a/spec/requests/bulk_upload_lettings_results_controller_spec.rb b/spec/requests/bulk_upload_lettings_results_controller_spec.rb index c15fef5b9..91d7a0742 100644 --- a/spec/requests/bulk_upload_lettings_results_controller_spec.rb +++ b/spec/requests/bulk_upload_lettings_results_controller_spec.rb @@ -2,11 +2,13 @@ require "rails_helper" RSpec.describe BulkUploadLettingsResultsController, type: :request do let(:user) { create(:user) } + let(:support_user) { create(:user, :support) } let(:bulk_upload) { create(:bulk_upload, :lettings, user:, bulk_upload_errors:) } let(:bulk_upload_errors) { create_list(:bulk_upload_error, 2) } + let(:viewing_user) { user } before do - sign_in user + sign_in viewing_user end describe "GET /lettings-logs/bulk-upload-results/:ID/summary" do @@ -22,6 +24,43 @@ RSpec.describe BulkUploadLettingsResultsController, type: :request do expect(response.body).to include(bulk_upload.filename) end + + context "when viewed by support user" do + before do + allow(support_user).to receive(:need_two_factor_authentication?).and_return(false) + end + + let(:viewing_user) { support_user } + + it "is accessible" do + get "/lettings-logs/bulk-upload-results/#{bulk_upload.id}/summary" + + expect(response).to be_successful + expect(response.body).to include(bulk_upload.filename) + end + end + + context "when viewed by some other random user" do + let(:other_user) { create(:user) } + let(:viewing_user) { other_user } + + it "is not accessible" do + get "/lettings-logs/bulk-upload-results/#{bulk_upload.id}/summary" + expect(response).to be_unauthorized + end + end + + context "when viewed by another user in the same org" do + let(:other_user) { create(:user, organisation: user.organisation) } + let(:viewing_user) { other_user } + + it "is accessible" do + get "/lettings-logs/bulk-upload-results/#{bulk_upload.id}/summary" + + expect(response).to be_successful + expect(response.body).to include(bulk_upload.filename) + end + end end describe "GET /lettings-logs/bulk-upload-results/:ID" do