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

@@ -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"

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)

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

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

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

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

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

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:) }

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

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

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

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