From d48a5d07dfcb7160872ff04ef1e50b1e76ac6a08 Mon Sep 17 00:00:00 2001
From: baarkerlounger <5101747+baarkerlounger@users.noreply.github.com>
Date: Fri, 3 Dec 2021 14:33:28 +0000
Subject: [PATCH] CLDC-769: Scope authentication to the correct resources
 (#143)

* Scope auth for org pages

* Scope user methods

* Refactor form UI controller

* More tests for access

* Consistently return not found for scoped auth

* Add ADR

* Authenticate bulk uploads

* Authenticate soft validations controller

* Scope bulk upload to user's org for now

* Pass through current user

* Update docs/adr/adr-012-controller-http-return-statuses.md

Co-authored-by: Dushan <47317567+dushan-madetech@users.noreply.github.com>

* Update spec/requests/case_log_controller_spec.rb

Co-authored-by: Dushan <47317567+dushan-madetech@users.noreply.github.com>

Co-authored-by: Dushan <47317567+dushan-madetech@users.noreply.github.com>
---
 app/controllers/bulk_upload_controller.rb     |   6 +-
 app/controllers/case_logs_controller.rb       |  94 +-----
 app/controllers/form_controller.rb            |  83 ++++++
 app/controllers/organisations_controller.rb   |   9 +-
 .../soft_validations_controller.rb            |   2 +
 app/controllers/users_controller.rb           |  16 +-
 app/models/bulk_upload.rb                     |   6 +-
 config/routes.rb                              |   6 +-
 ...adr-012-controller-http-return-statuses.md |  15 +
 spec/controllers/case_logs_controller_spec.rb | 172 -----------
 spec/requests/bulk_upload_controller_spec.rb  |  91 ++++--
 spec/requests/case_log_controller_spec.rb     | 200 +++----------
 spec/requests/form_controller_spec.rb         | 278 ++++++++++++++++++
 .../requests/organisations_controller_spec.rb | 233 ++++++++++-----
 .../soft_validations_controller_spec.rb       |  57 ++--
 spec/requests/user_controller_spec.rb         | 129 +++++++-
 16 files changed, 832 insertions(+), 565 deletions(-)
 create mode 100644 app/controllers/form_controller.rb
 create mode 100644 docs/adr/adr-012-controller-http-return-statuses.md
 delete mode 100644 spec/controllers/case_logs_controller_spec.rb
 create mode 100644 spec/requests/form_controller_spec.rb

diff --git a/app/controllers/bulk_upload_controller.rb b/app/controllers/bulk_upload_controller.rb
index ba552cb49..28fe7bbae 100644
--- a/app/controllers/bulk_upload_controller.rb
+++ b/app/controllers/bulk_upload_controller.rb
@@ -1,4 +1,6 @@
 class BulkUploadController < ApplicationController
+  before_action :authenticate_user!
+
   def show
     @bulk_upload = BulkUpload.new(nil, nil)
     render "case_logs/bulk_upload"
@@ -8,7 +10,7 @@ class BulkUploadController < ApplicationController
     file = upload_params.tempfile
     content_type = upload_params.content_type
     @bulk_upload = BulkUpload.new(file, content_type)
-    @bulk_upload.process
+    @bulk_upload.process(current_user)
     if @bulk_upload.errors.present?
       render "case_logs/bulk_upload", status: :unprocessable_entity
     else
@@ -16,6 +18,8 @@ class BulkUploadController < ApplicationController
     end
   end
 
+private
+
   def upload_params
     params.require("bulk_upload")["case_log_bulk_upload"]
   end
diff --git a/app/controllers/case_logs_controller.rb b/app/controllers/case_logs_controller.rb
index 069100299..77ccc3d6d 100644
--- a/app/controllers/case_logs_controller.rb
+++ b/app/controllers/case_logs_controller.rb
@@ -2,6 +2,7 @@ class CaseLogsController < ApplicationController
   skip_before_action :verify_authenticity_token, if: :json_api_request?
   before_action :authenticate, if: :json_api_request?
   before_action :authenticate_user!, unless: :json_api_request?
+  before_action :find_resource, except: %i[create index edit]
 
   def index
     @completed_case_logs = current_user.completed_case_logs
@@ -23,11 +24,11 @@ class CaseLogsController < ApplicationController
   end
 
   def update
-    if (case_log = CaseLog.find_by(id: params[:id]))
-      if case_log.update(api_case_log_params)
-        render json: case_log, status: :ok
+    if @case_log
+      if @case_log.update(api_case_log_params)
+        render json: @case_log, status: :ok
       else
-        render json: { errors: case_log.errors.messages }, status: :unprocessable_entity
+        render json: { errors: @case_log.errors.messages }, status: :unprocessable_entity
       end
     else
       render_not_found_json("Case log", params[:id])
@@ -39,8 +40,8 @@ class CaseLogsController < ApplicationController
       # We don't have a dedicated non-editable show view
       format.html { edit }
       format.json do
-        if (case_log = CaseLog.find_by(id: params[:id]))
-          render json: case_log, status: :ok
+        if @case_log
+          render json: @case_log, status: :ok
         else
           render_not_found_json("Case log", params[:id])
         end
@@ -58,93 +59,22 @@ class CaseLogsController < ApplicationController
     end
   end
 
-  def submit_form
-    form = FormHandler.instance.get_form("2021_2022")
-    @case_log = current_user.case_logs.find_by(id: params[:id])
-    if @case_log
-      page = form.get_page(params[:case_log][:page])
-      responses_for_page = responses_for_page(page)
-      if @case_log.update(responses_for_page) && @case_log.has_no_unresolved_soft_errors?
-        redirect_path = form.next_page_redirect_path(page, @case_log)
-        redirect_to(send(redirect_path, @case_log))
-      else
-        subsection = form.subsection_for_page(page)
-        render "form/page", locals: { form: form, page: page, subsection: subsection.label }, status: :unprocessable_entity
-      end
-    else
-      render_not_found_html
-    end
-  end
-
   def destroy
-    if (case_log = CaseLog.find_by(id: params[:id]))
-      if case_log.discard
+    if @case_log
+      if @case_log.discard
         head :no_content
       else
-        render json: { errors: case_log.errors.messages }, status: :unprocessable_entity
+        render json: { errors: @case_log.errors.messages }, status: :unprocessable_entity
       end
     else
       render_not_found_json("Case log", params[:id])
     end
   end
 
-  def check_answers
-    form = FormHandler.instance.get_form("2021_2022")
-    @case_log = current_user.case_logs.find_by(id: params[:case_log_id])
-    if @case_log
-      current_url = request.env["PATH_INFO"]
-      subsection = form.get_subsection(current_url.split("/")[-2])
-      render "form/check_answers", locals: { subsection: subsection, form: form }
-    else
-      render_not_found_html
-    end
-  end
-
-  form = FormHandler.instance.get_form("2021_2022")
-  form.pages.map do |page|
-    define_method(page.id) do |_errors = {}|
-      @case_log = current_user.case_logs.find_by(id: params[:case_log_id])
-      if @case_log
-        subsection = form.subsection_for_page(page)
-        render "form/page", locals: { form: form, page: page, subsection: subsection.label }
-      else
-        render_not_found_html
-      end
-    end
-  end
-
 private
 
   API_ACTIONS = %w[create show update destroy].freeze
 
-  def responses_for_page(page)
-    page.expected_responses.each_with_object({}) do |question, result|
-      question_params = params["case_log"][question.id]
-      if question.type == "date"
-        day = params["case_log"]["#{question.id}(3i)"]
-        month = params["case_log"]["#{question.id}(2i)"]
-        year = params["case_log"]["#{question.id}(1i)"]
-        next unless [day, month, year].any?(&:present?)
-
-        result[question.id] = if day.to_i.between?(1, 31) && month.to_i.between?(1, 12) && year.to_i.between?(2000, 2200)
-                                Date.new(year.to_i, month.to_i, day.to_i)
-                              else
-                                Date.new(0, 1, 1)
-                              end
-      end
-      next unless question_params
-
-      if %w[checkbox validation_override].include?(question.type)
-        question.answer_options.keys.reject { |x| x.match(/divider/) }.each do |option|
-          result[option] = question_params.include?(option) ? 1 : 0
-        end
-      else
-        result[question.id] = question_params
-      end
-      result
-    end
-  end
-
   def json_api_request?
     API_ACTIONS.include?(request["action"]) && request.format.json?
   end
@@ -173,4 +103,8 @@ private
 
     params.require(:case_log).permit(CaseLog.editable_fields)
   end
+
+  def find_resource
+    @case_log = CaseLog.find_by(id: params[:id])
+  end
 end
diff --git a/app/controllers/form_controller.rb b/app/controllers/form_controller.rb
new file mode 100644
index 000000000..93dc32e84
--- /dev/null
+++ b/app/controllers/form_controller.rb
@@ -0,0 +1,83 @@
+class FormController < ApplicationController
+  before_action :authenticate_user!
+  before_action :find_resource, only: [:submit_form]
+  before_action :find_resource_by_named_id, except: [:submit_form]
+
+  def submit_form
+    form = FormHandler.instance.get_form("2021_2022")
+    if @case_log
+      page = form.get_page(params[:case_log][:page])
+      responses_for_page = responses_for_page(page)
+      if @case_log.update(responses_for_page) && @case_log.has_no_unresolved_soft_errors?
+        redirect_path = form.next_page_redirect_path(page, @case_log)
+        redirect_to(send(redirect_path, @case_log))
+      else
+        subsection = form.subsection_for_page(page)
+        render "form/page", locals: { form: form, page: page, subsection: subsection.label }, status: :unprocessable_entity
+      end
+    else
+      render_not_found_html
+    end
+  end
+
+  def check_answers
+    form = FormHandler.instance.get_form("2021_2022")
+    if @case_log
+      current_url = request.env["PATH_INFO"]
+      subsection = form.get_subsection(current_url.split("/")[-2])
+      render "form/check_answers", locals: { subsection: subsection, form: form }
+    else
+      render_not_found_html
+    end
+  end
+
+  form = FormHandler.instance.get_form("2021_2022")
+  form.pages.map do |page|
+    define_method(page.id) do |_errors = {}|
+      if @case_log
+        subsection = form.subsection_for_page(page)
+        render "form/page", locals: { form: form, page: page, subsection: subsection.label }
+      else
+        render_not_found_html
+      end
+    end
+  end
+
+private
+
+  def responses_for_page(page)
+    page.expected_responses.each_with_object({}) do |question, result|
+      question_params = params["case_log"][question.id]
+      if question.type == "date"
+        day = params["case_log"]["#{question.id}(3i)"]
+        month = params["case_log"]["#{question.id}(2i)"]
+        year = params["case_log"]["#{question.id}(1i)"]
+        next unless [day, month, year].any?(&:present?)
+
+        result[question.id] = if day.to_i.between?(1, 31) && month.to_i.between?(1, 12) && year.to_i.between?(2000, 2200)
+                                Date.new(year.to_i, month.to_i, day.to_i)
+                              else
+                                Date.new(0, 1, 1)
+                              end
+      end
+      next unless question_params
+
+      if %w[checkbox validation_override].include?(question.type)
+        question.answer_options.keys.reject { |x| x.match(/divider/) }.each do |option|
+          result[option] = question_params.include?(option) ? 1 : 0
+        end
+      else
+        result[question.id] = question_params
+      end
+      result
+    end
+  end
+
+  def find_resource
+    @case_log = current_user.case_logs.find_by(id: params[:id])
+  end
+
+  def find_resource_by_named_id
+    @case_log = current_user.case_logs.find_by(id: params[:case_log_id])
+  end
+end
diff --git a/app/controllers/organisations_controller.rb b/app/controllers/organisations_controller.rb
index dcf1aa34a..a9081dd3f 100644
--- a/app/controllers/organisations_controller.rb
+++ b/app/controllers/organisations_controller.rb
@@ -1,6 +1,7 @@
 class OrganisationsController < ApplicationController
   before_action :authenticate_user!
-  before_action :find_organisation
+  before_action :find_resource
+  before_action :authenticate_scope!
 
   def show
     redirect_to details_organisation_path(@organisation)
@@ -20,7 +21,11 @@ class OrganisationsController < ApplicationController
 
 private
 
-  def find_organisation
+  def authenticate_scope!
+    head :not_found if current_user.organisation != @organisation
+  end
+
+  def find_resource
     @organisation = Organisation.find(params[:id])
   end
 end
diff --git a/app/controllers/soft_validations_controller.rb b/app/controllers/soft_validations_controller.rb
index 4f9881de6..0f0baf376 100644
--- a/app/controllers/soft_validations_controller.rb
+++ b/app/controllers/soft_validations_controller.rb
@@ -1,4 +1,6 @@
 class SoftValidationsController < ApplicationController
+  before_action :authenticate_user!
+
   def show
     @case_log = CaseLog.find(params[:case_log_id])
     page_id = request.env["PATH_INFO"].split("/")[-2]
diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index 30afa501b..9ebdae068 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -2,12 +2,14 @@ class UsersController < ApplicationController
   include Devise::Controllers::SignInOut
   include Helpers::Email
   before_action :authenticate_user!
+  before_action :find_resource, except: %i[new create]
+  before_action :authenticate_scope!, except: %i[new create]
 
   def update
-    if current_user.update(user_params)
-      bypass_sign_in current_user
+    if @user.update(user_params)
+      bypass_sign_in @user
       flash[:notice] = I18n.t("devise.passwords.updated")
-      redirect_to user_path(current_user)
+      redirect_to user_path(@user)
     end
   end
 
@@ -48,4 +50,12 @@ private
   def user_params
     params.require(:user).permit(:email, :name, :password, :role)
   end
+
+  def find_resource
+    @user = User.find(params[:id])
+  end
+
+  def authenticate_scope!
+    head :not_found if current_user != @user
+  end
 end
diff --git a/app/models/bulk_upload.rb b/app/models/bulk_upload.rb
index 8bf0e2131..4eb558c10 100644
--- a/app/models/bulk_upload.rb
+++ b/app/models/bulk_upload.rb
@@ -15,7 +15,7 @@ class BulkUpload
     @content_type = content_type
   end
 
-  def process
+  def process(current_user)
     return unless valid_content_type?
 
     xlsx = Roo::Spreadsheet.open(@file, extension: :xlsx)
@@ -30,8 +30,8 @@ class BulkUpload
         owning_organisation = Organisation.find(row[111])
         managing_organisation = Organisation.find(row[113])
         case_log = CaseLog.create!(
-          owning_organisation: owning_organisation,
-          managing_organisation: managing_organisation,
+          owning_organisation: current_user.organisation,
+          managing_organisation: current_user.organisation,
         )
         map_row(row).each do |attr_key, attr_val|
           update = case_log.update(attr_key => attr_val)
diff --git a/config/routes.rb b/config/routes.rb
index 72e1ef208..0cc0065c0 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -38,16 +38,16 @@ Rails.application.routes.draw do
     end
 
     member do
-      post "form", to: "case_logs#submit_form"
+      post "form", to: "form#submit_form"
     end
 
     form.pages.map do |page|
-      get page.id.to_s.dasherize, to: "case_logs##{page.id}"
+      get page.id.to_s.dasherize, to: "form##{page.id}"
       get "#{page.id.to_s.dasherize}/soft-validations", to: "soft_validations#show" if page.has_soft_validations?
     end
 
     form.subsections.map do |subsection|
-      get "#{subsection.id.to_s.dasherize}/check-answers", to: "case_logs#check_answers"
+      get "#{subsection.id.to_s.dasherize}/check-answers", to: "form#check_answers"
     end
   end
 end
diff --git a/docs/adr/adr-012-controller-http-return-statuses.md b/docs/adr/adr-012-controller-http-return-statuses.md
new file mode 100644
index 000000000..d28ee39d8
--- /dev/null
+++ b/docs/adr/adr-012-controller-http-return-statuses.md
@@ -0,0 +1,15 @@
+### ADR - 012: Controller HTTP return statuses
+
+Controllers assess authentication by 3 criteria:
+
+1. Are you signed in at all?
+2. Are you signed in and requesting an action that your role/user type has access to?
+3. Are you signed in, requesting an action that your role/user type has access to and requesting a resource that your user has access to.
+
+When these aren't met they fail with the following response types:
+
+1. 401: Unauthorized. Redirect to sign-in page.
+2. 401: Unauthorized
+3. 404: Not found.
+
+This helps make it harder to determine whether a resource exists or not just by enumerating ids.
diff --git a/spec/controllers/case_logs_controller_spec.rb b/spec/controllers/case_logs_controller_spec.rb
deleted file mode 100644
index c2340d4ef..000000000
--- a/spec/controllers/case_logs_controller_spec.rb
+++ /dev/null
@@ -1,172 +0,0 @@
-require "rails_helper"
-
-RSpec.describe CaseLogsController, type: :controller do
-  let(:valid_session) { {} }
-  let(:user) { FactoryBot.create(:user) }
-  let(:case_log) do
-    FactoryBot.create(
-      :case_log,
-      owning_organisation: user.organisation,
-      managing_organisation: user.organisation,
-    )
-  end
-  let(:id) { case_log.id }
-
-  before do
-    sign_in user
-  end
-
-  context "Collection routes" do
-    describe "GET #index" do
-      it "returns a success response" do
-        get :index, params: {}, session: valid_session
-        expect(response).to be_successful
-      end
-    end
-
-    describe "Post #create" do
-      let(:owning_organisation) { FactoryBot.create(:organisation) }
-      let(:managing_organisation) { owning_organisation }
-      let(:params) do
-        {
-          "owning_organisation_id": owning_organisation.id,
-          "managing_organisation_id": managing_organisation.id,
-        }
-      end
-
-      it "creates a new case log record" do
-        expect {
-          post :create, params: params, session: valid_session
-        }.to change(CaseLog, :count).by(1)
-      end
-
-      it "redirects to that case log" do
-        post :create, params: params, session: valid_session
-        expect(response.status).to eq(302)
-      end
-    end
-  end
-
-  context "Instance routes" do
-    describe "GET #show" do
-      it "returns a success response" do
-        get :show, params: { id: id }
-        expect(response).to be_successful
-      end
-    end
-
-    describe "GET #edit" do
-      it "returns a success response" do
-        get :edit, params: { id: id }
-        expect(response).to be_successful
-      end
-    end
-  end
-
-  describe "submit_form" do
-    let(:case_log_form_params) do
-      { accessibility_requirements:
-                             %w[ housingneeds_a
-                                 housingneeds_b
-                                 housingneeds_c],
-        page: "accessibility_requirements" }
-    end
-
-    let(:new_case_log_form_params) do
-      {
-        accessibility_requirements: %w[housingneeds_c],
-        page: "accessibility_requirements",
-      }
-    end
-
-    it "sets checked items to true" do
-      post :submit_form, params: { id: id, case_log: case_log_form_params }
-      case_log.reload
-
-      expect(case_log.housingneeds_a).to eq("Yes")
-      expect(case_log.housingneeds_b).to eq("Yes")
-      expect(case_log.housingneeds_c).to eq("Yes")
-    end
-
-    it "sets previously submitted items to false when resubmitted with new values" do
-      post :submit_form, params: { id: id, case_log: new_case_log_form_params }
-      case_log.reload
-
-      expect(case_log.housingneeds_a).to eq("No")
-      expect(case_log.housingneeds_b).to eq("No")
-      expect(case_log.housingneeds_c).to eq("Yes")
-    end
-
-    context "given a page with checkbox and non-checkbox questions" do
-      let(:tenant_code) { "BZ355" }
-      let(:case_log_form_params) do
-        { accessibility_requirements:
-                               %w[ housingneeds_a
-                                   housingneeds_b
-                                   housingneeds_c],
-          tenant_code: tenant_code,
-          page: "accessibility_requirements" }
-      end
-      let(:questions_for_page) do
-        [
-          Form::Question.new(
-            "accessibility_requirements",
-            {
-              "type" => "checkbox",
-              "answer_options" =>
-              { "housingneeds_a" => "Fully wheelchair accessible housing",
-                "housingneeds_b" => "Wheelchair access to essential rooms",
-                "housingneeds_c" => "Level access housing",
-                "housingneeds_f" => "Other disability requirements",
-                "housingneeds_g" => "No disability requirements",
-                "divider_a" => true,
-                "housingneeds_h" => "Do not know",
-                "divider_b" => true,
-                "accessibility_requirements_prefer_not_to_say" => "Prefer not to say" },
-            }, nil
-          ),
-          Form::Question.new("tenant_code", { "type" => "text" }, nil),
-        ]
-      end
-
-      it "updates both question fields" do
-        allow_any_instance_of(Form::Page).to receive(:expected_responses).and_return(questions_for_page)
-        post :submit_form, params: { id: id, case_log: case_log_form_params }
-        case_log.reload
-
-        expect(case_log.housingneeds_a).to eq("Yes")
-        expect(case_log.housingneeds_b).to eq("Yes")
-        expect(case_log.housingneeds_c).to eq("Yes")
-        expect(case_log.tenant_code).to eq(tenant_code)
-      end
-    end
-
-    context "conditional routing" do
-      before do
-        allow_any_instance_of(CaseLogValidator).to receive(:validate_pregnancy).and_return(true)
-      end
-
-      let(:case_log_form_conditional_question_yes_params) do
-        {
-          preg_occ: "Yes",
-          page: "conditional_question",
-        }
-      end
-
-      let(:case_log_form_conditional_question_no_params) do
-        {
-          preg_occ: "No",
-          page: "conditional_question",
-        }
-      end
-
-      it "routes to the appropriate conditional page based on the question answer of the current page" do
-        post :submit_form, params: { id: id, case_log: case_log_form_conditional_question_yes_params }
-        expect(response).to redirect_to("/case-logs/#{id}/conditional-question-yes-page")
-
-        post :submit_form, params: { id: id, case_log: case_log_form_conditional_question_no_params }
-        expect(response).to redirect_to("/case-logs/#{id}/conditional-question-no-page")
-      end
-    end
-  end
-end
diff --git a/spec/requests/bulk_upload_controller_spec.rb b/spec/requests/bulk_upload_controller_spec.rb
index 52233906c..e19247162 100644
--- a/spec/requests/bulk_upload_controller_spec.rb
+++ b/spec/requests/bulk_upload_controller_spec.rb
@@ -2,62 +2,89 @@ require "rails_helper"
 
 RSpec.describe BulkUploadController, type: :request do
   let(:url) { "/case-logs/bulk-upload" }
-  let(:organisation) { FactoryBot.create(:organisation) }
+  let(:user) { FactoryBot.create(:user) }
+  let(:organisation) { user.organisation }
   before do
     allow(Organisation).to receive(:find).with(107_242).and_return(organisation)
   end
 
-  describe "GET #show" do
-    before do
-      get url, params: {}
+  context "a not signed in user" do
+    describe "GET #show" do
+      it "does not let you see the bulk upload page" do
+        get url, headers: headers, params: {}
+        expect(response).to redirect_to("/users/sign-in")
+      end
     end
 
-    it "returns a success response" do
-      expect(response).to be_successful
-    end
+    describe "POST #bulk upload" do
+      before do
+        @file = fixture_file_upload("2021_22_lettings_bulk_upload.xlsx", "application/vnd.ms-excel")
+      end
 
-    it "returns a page with a file upload form" do
-      expect(response.body).to match(/<input id="bulk-upload-case-log-bulk-upload-field" class="govuk-file-upload"/)
-      expect(response.body).to match(/<button type="submit" formnovalidate="formnovalidate" class="govuk-button"/)
+      it "does not let you submit bulk uploads" do
+        post url, params: { bulk_upload: { case_log_bulk_upload: @file } }
+        expect(response).to redirect_to("/users/sign-in")
+      end
     end
   end
 
-  describe "POST #bulk upload" do
-    subject { post url, params: { bulk_upload: { case_log_bulk_upload: @file } } }
+  context "a signed in user" do
+    before do
+      sign_in user
+    end
 
-    context "given a valid file based on the upload template" do
+    describe "GET #show" do
       before do
-        @file = fixture_file_upload("2021_22_lettings_bulk_upload.xlsx", "application/vnd.ms-excel")
+        get url, params: {}
       end
 
-      it "creates case logs for each row in the template" do
-        expect { subject }.to change(CaseLog, :count).by(9)
+      it "returns a success response" do
+        expect(response).to be_successful
       end
 
-      it "redirects to the case log index page" do
-        expect(subject).to redirect_to(case_logs_path)
+      it "returns a page with a file upload form" do
+        expect(response.body).to match(/<input id="bulk-upload-case-log-bulk-upload-field" class="govuk-file-upload"/)
+        expect(response.body).to match(/<button type="submit" formnovalidate="formnovalidate" class="govuk-button"/)
       end
     end
 
-    context "given an invalid file type" do
-      before do
-        @file = fixture_file_upload("random.txt", "text/plain")
-        subject
-      end
+    describe "POST #bulk upload" do
+      subject { post url, params: { bulk_upload: { case_log_bulk_upload: @file } } }
+
+      context "given a valid file based on the upload template" do
+        before do
+          @file = fixture_file_upload("2021_22_lettings_bulk_upload.xlsx", "application/vnd.ms-excel")
+        end
+
+        it "creates case logs for each row in the template" do
+          expect { subject }.to change(CaseLog, :count).by(9)
+        end
 
-      it "displays an error message" do
-        expect(response.body).to match(/Invalid file type/)
+        it "redirects to the case log index page" do
+          expect(subject).to redirect_to(case_logs_path)
+        end
       end
-    end
 
-    context "given an empty file" do
-      before do
-        @file = fixture_file_upload("2021_22_lettings_bulk_upload_empty.xlsx", "application/vnd.ms-excel")
-        subject
+      context "given an invalid file type" do
+        before do
+          @file = fixture_file_upload("random.txt", "text/plain")
+          subject
+        end
+
+        it "displays an error message" do
+          expect(response.body).to match(/Invalid file type/)
+        end
       end
 
-      it "displays an error message" do
-        expect(response.body).to match(/No data found/)
+      context "given an empty file" do
+        before do
+          @file = fixture_file_upload("2021_22_lettings_bulk_upload_empty.xlsx", "application/vnd.ms-excel")
+          subject
+        end
+
+        it "displays an error message" do
+          expect(response.body).to match(/No data found/)
+        end
       end
     end
   end
diff --git a/spec/requests/case_log_controller_spec.rb b/spec/requests/case_log_controller_spec.rb
index deeb6840c..8c7757cc6 100644
--- a/spec/requests/case_log_controller_spec.rb
+++ b/spec/requests/case_log_controller_spec.rb
@@ -180,84 +180,63 @@ RSpec.describe CaseLogsController, type: :request do
           allow(FormHandler.instance).to receive(:get_form).and_return(form)
         end
 
-        context "case logs that are owned or managed by your organisation" do
-          before do
-            sign_in user
+        context "a user that is not signed in" do
+          it "does not let the user get case log tasklist pages they don't have access to" do
             get "/case-logs/#{case_log.id}", headers: headers, params: {}
-          end
-
-          it "shows the tasklist for case logs you have access to" do
-            expect(response.body).to match("Case log")
-            expect(response.body).to match(case_log.id.to_s)
-          end
-
-          it "displays a section status for a case log" do
-            assert_select ".govuk-tag", text: /Not started/, count: 8
-            assert_select ".govuk-tag", text: /Completed/, count: 0
-            assert_select ".govuk-tag", text: /Cannot start yet/, count: 1
-          end
-        end
-
-        context "case log with a single section complete" do
-          let(:section_completed_case_log) do
-            FactoryBot.create(
-              :case_log,
-              :conditional_section_complete,
-              owning_organisation: organisation,
-              managing_organisation: organisation,
-            )
-          end
-
-          before do
-            sign_in user
-            get "/case-logs/#{section_completed_case_log.id}", headers: headers, params: {}
-          end
-
-          it "displays a section status for a case log" do
-            assert_select ".govuk-tag", text: /Not started/, count: 7
-            assert_select ".govuk-tag", text: /Completed/, count: 1
-            assert_select ".govuk-tag", text: /Cannot start yet/, count: 1
+            expect(response).to redirect_to("/users/sign-in")
           end
         end
 
-        context "case logs that are not owned or managed by your organisation" do
-          before do
-            sign_in user
-            get "/case-logs/#{unauthorized_case_log.id}", headers: headers, params: {}
+        context "a signed in user" do
+          context "case logs that are owned or managed by your organisation" do
+            before do
+              sign_in user
+              get "/case-logs/#{case_log.id}", headers: headers, params: {}
+            end
+
+            it "shows the tasklist for case logs you have access to" do
+              expect(response.body).to match("Case log")
+              expect(response.body).to match(case_log.id.to_s)
+            end
+
+            it "displays a section status for a case log" do
+              assert_select ".govuk-tag", text: /Not started/, count: 8
+              assert_select ".govuk-tag", text: /Completed/, count: 0
+              assert_select ".govuk-tag", text: /Cannot start yet/, count: 1
+            end
           end
 
-          it "does not show the tasklist for case logs you don't have access to" do
-            expect(response).to have_http_status(:not_found)
+          context "case log with a single section complete" do
+            let(:section_completed_case_log) do
+              FactoryBot.create(
+                :case_log,
+                :conditional_section_complete,
+                owning_organisation: organisation,
+                managing_organisation: organisation,
+              )
+            end
+
+            before do
+              sign_in user
+              get "/case-logs/#{section_completed_case_log.id}", headers: headers, params: {}
+            end
+
+            it "displays a section status for a case log" do
+              assert_select ".govuk-tag", text: /Not started/, count: 7
+              assert_select ".govuk-tag", text: /Completed/, count: 1
+              assert_select ".govuk-tag", text: /Cannot start yet/, count: 1
+            end
           end
-        end
-      end
 
-      context "form pages" do
-        let(:headers) { { "Accept" => "text/html" } }
+          context "case logs that are not owned or managed by your organisation" do
+            before do
+              sign_in user
+              get "/case-logs/#{unauthorized_case_log.id}", headers: headers, params: {}
+            end
 
-        context "case logs that are not owned or managed by your organisation" do
-          before do
-            sign_in user
-            get "/case-logs/#{unauthorized_case_log.id}/person-1-age", headers: headers, params: {}
-          end
-
-          it "does not show form pages for case logs you don't have access to" do
-            expect(response).to have_http_status(:not_found)
-          end
-        end
-      end
-
-      context "check answers pages" do
-        let(:headers) { { "Accept" => "text/html" } }
-
-        context "case logs that are not owned or managed by your organisation" do
-          before do
-            sign_in user
-            get "/case-logs/#{unauthorized_case_log.id}/household-characteristics/check-answers", headers: headers, params: {}
-          end
-
-          it "does not show a check answers for case logs you don't have access to" do
-            expect(response).to have_http_status(:not_found)
+            it "does not show the tasklist for case logs you don't have access to" do
+              expect(response).to have_http_status(:not_found)
+            end
           end
         end
       end
@@ -414,87 +393,4 @@ RSpec.describe CaseLogsController, type: :request do
       end
     end
   end
-
-  describe "Submit Form" do
-    let(:user) { FactoryBot.create(:user) }
-    let(:form) { Form.new("spec/fixtures/forms/test_form.json") }
-    let(:organisation) { user.organisation }
-    let(:case_log) do
-      FactoryBot.create(
-        :case_log,
-        owning_organisation: organisation,
-        managing_organisation: organisation,
-      )
-    end
-    let(:page_id) { "person_1_age" }
-    let(:params) do
-      {
-        id: case_log.id,
-        case_log: {
-          page: page_id,
-          age1: answer,
-        },
-      }
-    end
-
-    before do
-      allow(FormHandler.instance).to receive(:get_form).and_return(form)
-      sign_in user
-      post "/case-logs/#{case_log.id}/form", params: params
-    end
-
-    context "invalid answers" do
-      let(:answer) { 2000 }
-
-      it "re-renders the same page with errors if validation fails" do
-        expect(response).to have_http_status(:unprocessable_entity)
-      end
-    end
-
-    context "valid answers" do
-      let(:answer) { 20 }
-
-      it "re-renders the same page with errors if validation fails" do
-        expect(response).to have_http_status(:redirect)
-      end
-
-      let(:params) do
-        {
-          id: case_log.id,
-          case_log: {
-            page: page_id,
-            age1: answer,
-            age2: 2000,
-          },
-        }
-      end
-
-      it "only updates answers that apply to the page being submitted" do
-        case_log.reload
-        expect(case_log.age1).to eq(answer)
-        expect(case_log.age2).to be nil
-      end
-    end
-
-    context "case logs that are not owned or managed by your organisation" do
-      let(:answer) { 25 }
-      let(:other_organisation) { FactoryBot.create(:organisation) }
-      let(:unauthorized_case_log) do
-        FactoryBot.create(
-          :case_log,
-          owning_organisation: other_organisation,
-          managing_organisation: other_organisation,
-        )
-      end
-
-      before do
-        sign_in user
-        post "/case-logs/#{unauthorized_case_log.id}/form", params: params
-      end
-
-      it "does not let you post form answers to case logs you don't have access to" do
-        expect(response).to have_http_status(:not_found)
-      end
-    end
-  end
 end
diff --git a/spec/requests/form_controller_spec.rb b/spec/requests/form_controller_spec.rb
new file mode 100644
index 000000000..f88b1219b
--- /dev/null
+++ b/spec/requests/form_controller_spec.rb
@@ -0,0 +1,278 @@
+require "rails_helper"
+
+RSpec.describe FormController, type: :request do
+  let(:user) { FactoryBot.create(:user) }
+  let(:organisation) { user.organisation }
+  let(:other_organisation) { FactoryBot.create(:organisation) }
+  let!(:case_log) do
+    FactoryBot.create(
+      :case_log,
+      owning_organisation: organisation,
+      managing_organisation: organisation,
+    )
+  end
+  let!(:unauthorized_case_log) do
+    FactoryBot.create(
+      :case_log,
+      owning_organisation: other_organisation,
+      managing_organisation: other_organisation,
+    )
+  end
+  let(:headers) { { "Accept" => "text/html" } }
+
+  context "a not signed in user" do
+    describe "GET" do
+      it "does not let you get case logs pages you don't have access to" do
+        get "/case-logs/#{case_log.id}/person-1-age", headers: headers, params: {}
+        expect(response).to redirect_to("/users/sign-in")
+      end
+
+      it "does not let you get case log check answer pages you don't have access to" do
+        get "/case-logs/#{case_log.id}/household-characteristics/check-answers", headers: headers, params: {}
+        expect(response).to redirect_to("/users/sign-in")
+      end
+    end
+
+    describe "POST" do
+      it "does not let you post form answers to case logs you don't have access to" do
+        post "/case-logs/#{case_log.id}/form", params: {}
+        expect(response).to redirect_to("/users/sign-in")
+      end
+    end
+  end
+
+  context "a signed in user" do
+    before do
+      sign_in user
+    end
+
+    describe "GET" do
+      context "form pages" do
+        context "case logs that are not owned or managed by your organisation" do
+          it "does not show form pages for case logs you don't have access to" do
+            get "/case-logs/#{unauthorized_case_log.id}/person-1-age", headers: headers, params: {}
+            expect(response).to have_http_status(:not_found)
+          end
+        end
+      end
+
+      context "check answers pages" do
+        context "case logs that are not owned or managed by your organisation" do
+          it "does not show a check answers for case logs you don't have access to" do
+            get "/case-logs/#{unauthorized_case_log.id}/household-characteristics/check-answers", headers: headers, params: {}
+            expect(response).to have_http_status(:not_found)
+          end
+        end
+      end
+    end
+
+    describe "Submit Form" do
+      context "a form page" do
+        let(:user) { FactoryBot.create(:user) }
+        let(:form) { Form.new("spec/fixtures/forms/test_form.json") }
+        let(:organisation) { user.organisation }
+        let(:case_log) do
+          FactoryBot.create(
+            :case_log,
+            owning_organisation: organisation,
+            managing_organisation: organisation,
+          )
+        end
+        let(:page_id) { "person_1_age" }
+        let(:params) do
+          {
+            id: case_log.id,
+            case_log: {
+              page: page_id,
+              age1: answer,
+            },
+          }
+        end
+
+        before do
+          allow(FormHandler.instance).to receive(:get_form).and_return(form)
+          post "/case-logs/#{case_log.id}/form", params: params
+        end
+
+        context "invalid answers" do
+          let(:answer) { 2000 }
+
+          it "re-renders the same page with errors if validation fails" do
+            expect(response).to have_http_status(:unprocessable_entity)
+          end
+        end
+
+        context "valid answers" do
+          let(:answer) { 20 }
+
+          it "re-renders the same page with errors if validation fails" do
+            expect(response).to have_http_status(:redirect)
+          end
+
+          let(:params) do
+            {
+              id: case_log.id,
+              case_log: {
+                page: page_id,
+                age1: answer,
+                age2: 2000,
+              },
+            }
+          end
+
+          it "only updates answers that apply to the page being submitted" do
+            case_log.reload
+            expect(case_log.age1).to eq(answer)
+            expect(case_log.age2).to be nil
+          end
+        end
+      end
+
+      context "checkbox questions" do
+        let(:case_log_form_params) do
+          {
+            id: case_log.id,
+            case_log: {
+              page: "accessibility_requirements",
+              accessibility_requirements:
+                                     %w[ housingneeds_a
+                                         housingneeds_b
+                                         housingneeds_c],
+            },
+          }
+        end
+
+        let(:new_case_log_form_params) do
+          {
+            id: case_log.id,
+            case_log: {
+              page: "accessibility_requirements",
+              accessibility_requirements: %w[housingneeds_c],
+            },
+          }
+        end
+
+        it "sets checked items to true" do
+          post "/case-logs/#{case_log.id}/form", params: case_log_form_params
+          case_log.reload
+
+          expect(case_log.housingneeds_a).to eq("Yes")
+          expect(case_log.housingneeds_b).to eq("Yes")
+          expect(case_log.housingneeds_c).to eq("Yes")
+        end
+
+        it "sets previously submitted items to false when resubmitted with new values" do
+          post "/case-logs/#{case_log.id}/form", params: new_case_log_form_params
+          case_log.reload
+
+          expect(case_log.housingneeds_a).to eq("No")
+          expect(case_log.housingneeds_b).to eq("No")
+          expect(case_log.housingneeds_c).to eq("Yes")
+        end
+
+        context "given a page with checkbox and non-checkbox questions" do
+          let(:tenant_code) { "BZ355" }
+          let(:case_log_form_params) do
+            {
+              id: case_log.id,
+              case_log: {
+                page: "accessibility_requirements",
+                accessibility_requirements:
+                                       %w[ housingneeds_a
+                                           housingneeds_b
+                                           housingneeds_c],
+                tenant_code: tenant_code,
+              },
+            }
+          end
+          let(:questions_for_page) do
+            [
+              Form::Question.new(
+                "accessibility_requirements",
+                {
+                  "type" => "checkbox",
+                  "answer_options" =>
+                  { "housingneeds_a" => "Fully wheelchair accessible housing",
+                    "housingneeds_b" => "Wheelchair access to essential rooms",
+                    "housingneeds_c" => "Level access housing",
+                    "housingneeds_f" => "Other disability requirements",
+                    "housingneeds_g" => "No disability requirements",
+                    "divider_a" => true,
+                    "housingneeds_h" => "Do not know",
+                    "divider_b" => true,
+                    "accessibility_requirements_prefer_not_to_say" => "Prefer not to say" },
+                }, nil
+              ),
+              Form::Question.new("tenant_code", { "type" => "text" }, nil),
+            ]
+          end
+
+          it "updates both question fields" do
+            allow_any_instance_of(Form::Page).to receive(:expected_responses).and_return(questions_for_page)
+            post "/case-logs/#{case_log.id}/form", params: case_log_form_params
+            case_log.reload
+
+            expect(case_log.housingneeds_a).to eq("Yes")
+            expect(case_log.housingneeds_b).to eq("Yes")
+            expect(case_log.housingneeds_c).to eq("Yes")
+            expect(case_log.tenant_code).to eq(tenant_code)
+          end
+        end
+      end
+
+      context "conditional routing" do
+        before do
+          allow_any_instance_of(CaseLogValidator).to receive(:validate_pregnancy).and_return(true)
+        end
+
+        let(:case_log_form_conditional_question_yes_params) do
+          {
+            id: case_log.id,
+            case_log: {
+              page: "conditional_question",
+              preg_occ: "Yes",
+            },
+          }
+        end
+
+        let(:case_log_form_conditional_question_no_params) do
+          {
+            id: case_log.id,
+            case_log: {
+              page: "conditional_question",
+              preg_occ: "No",
+            },
+          }
+        end
+
+        it "routes to the appropriate conditional page based on the question answer of the current page" do
+          post "/case-logs/#{case_log.id}/form", params: case_log_form_conditional_question_yes_params
+          expect(response).to redirect_to("/case-logs/#{case_log.id}/conditional-question-yes-page")
+
+          post "/case-logs/#{case_log.id}/form", params: case_log_form_conditional_question_no_params
+          expect(response).to redirect_to("/case-logs/#{case_log.id}/conditional-question-no-page")
+        end
+      end
+
+      context "case logs that are not owned or managed by your organisation" do
+        let(:answer) { 25 }
+        let(:other_organisation) { FactoryBot.create(:organisation) }
+        let(:unauthorized_case_log) do
+          FactoryBot.create(
+            :case_log,
+            owning_organisation: other_organisation,
+            managing_organisation: other_organisation,
+          )
+        end
+
+        before do
+          post "/case-logs/#{unauthorized_case_log.id}/form", params: {}
+        end
+
+        it "does not let you post form answers to case logs you don't have access to" do
+          expect(response).to have_http_status(:not_found)
+        end
+      end
+    end
+  end
+end
diff --git a/spec/requests/organisations_controller_spec.rb b/spec/requests/organisations_controller_spec.rb
index be49b8c9d..d086452ac 100644
--- a/spec/requests/organisations_controller_spec.rb
+++ b/spec/requests/organisations_controller_spec.rb
@@ -2,110 +2,181 @@ require "rails_helper"
 
 RSpec.describe OrganisationsController, type: :request do
   let(:organisation) { user.organisation }
+  let(:unauthorised_organisation) { FactoryBot.create(:organisation) }
   let(:headers) { { "Accept" => "text/html" } }
   let(:page) { Capybara::Node::Simple.new(response.body) }
+  let(:user) { FactoryBot.create(:user, :data_coordinator) }
 
-  describe "#show" do
-    let(:user) { FactoryBot.create(:user, :data_coordinator) }
-
-    before do
-      sign_in user
-      get "/organisations/#{organisation.id}", headers: headers, params: {}
-    end
-
-    it "redirects to details" do
-      expect(response).to have_http_status(:redirect)
-    end
-  end
-
-  context "As a data coordinator user" do
-    let(:user) { FactoryBot.create(:user, :data_coordinator) }
-
-    context "details tab" do
-      before do
-        sign_in user
-        get "/organisations/#{organisation.id}/details", headers: headers, params: {}
-      end
-
-      it "shows the tab navigation" do
-        expected_html = "<nav class=\"app-tab-navigation\""
-        expect(response.body).to include(expected_html)
+  context "a not signed in user" do
+    describe "#show" do
+      it "does not let you see organisation details from org route" do
+        get "/organisations/#{organisation.id}", headers: headers, params: {}
+        expect(response).to redirect_to("/users/sign-in")
       end
 
-      it "shows a summary list of org details" do
-        expected_html = "<dl class=\"govuk-summary-list\""
-        expect(response.body).to include(expected_html)
-        expect(response.body).to include(organisation.name)
-      end
-
-      it "has a hidden header title" do
-        expected_html = "<h2 class=\"govuk-visually-hidden\">  Details"
-        expect(response.body).to include(expected_html)
+      it "does not let you see organisation details from details route" do
+        get "/organisations/#{organisation.id}/details", headers: headers, params: {}
+        expect(response).to redirect_to("/users/sign-in")
       end
-    end
 
-    context "users tab" do
-      before do
-        sign_in user
+      it "does not let you see organisation users" do
         get "/organisations/#{organisation.id}/users", headers: headers, params: {}
-      end
-
-      it "shows the tab navigation" do
-        expected_html = "<nav class=\"app-tab-navigation\""
-        expect(response.body).to include(expected_html)
-      end
-
-      it "shows a new user button" do
-        expect(page).to have_link("Invite user")
-      end
-
-      it "shows a table of users" do
-        expected_html = "<table class=\"govuk-table\""
-        expect(response.body).to include(expected_html)
-        expect(response.body).to include(user.email)
-      end
-
-      it "has a hidden header title" do
-        expected_html = "<h2 class=\"govuk-visually-hidden\">  Users"
-        expect(response.body).to include(expected_html)
+        expect(response).to redirect_to("/users/sign-in")
       end
     end
   end
 
-  context "As a data provider user" do
-    let(:user) { FactoryBot.create(:user) }
-
-    context "details tab" do
-      before do
-        sign_in user
-        get "/organisations/#{organisation.id}/details", headers: headers, params: {}
+  context "a signed in user" do
+    describe "#show" do
+      context "organisation that the user belongs to" do
+        before do
+          sign_in user
+          get "/organisations/#{organisation.id}", headers: headers, params: {}
+        end
+
+        it "redirects to details" do
+          expect(response).to have_http_status(:redirect)
+        end
       end
 
-      it "shows the tab navigation" do
-        expected_html = "<nav class=\"app-tab-navigation\""
-        expect(response.body).to include(expected_html)
+      context "organisation that are not in scope for the user, i.e. that they do not belong to" do
+        before do
+          sign_in user
+          get "/organisations/#{unauthorised_organisation.id}", headers: headers, params: {}
+        end
+
+        it "returns not found 404 from org route" do
+          expect(response).to have_http_status(:not_found)
+        end
       end
+    end
 
-      it "shows a summary list of org details" do
-        expected_html = "<dl class=\"govuk-summary-list\""
-        expect(response.body).to include(expected_html)
-        expect(response.body).to include(organisation.name)
+    context "As a data coordinator user" do
+      context "details tab" do
+        context "organisation that the user belongs to" do
+          before do
+            sign_in user
+            get "/organisations/#{organisation.id}/details", headers: headers, params: {}
+          end
+
+          it "shows the tab navigation" do
+            expected_html = "<nav class=\"app-tab-navigation\""
+            expect(response.body).to include(expected_html)
+          end
+
+          it "shows a summary list of org details" do
+            expected_html = "<dl class=\"govuk-summary-list\""
+            expect(response.body).to include(expected_html)
+            expect(response.body).to include(organisation.name)
+          end
+
+          it "has a hidden header title" do
+            expected_html = "<h2 class=\"govuk-visually-hidden\">  Details"
+            expect(response.body).to include(expected_html)
+          end
+        end
+
+        context "organisation that are not in scope for the user, i.e. that they do not belong to" do
+          before do
+            sign_in user
+            get "/organisations/#{unauthorised_organisation.id}/details", headers: headers, params: {}
+          end
+
+          it "returns not found 404 from org details route" do
+            expect(response).to have_http_status(:not_found)
+          end
+        end
       end
 
-      it "has a hidden header title" do
-        expected_html = "<h2 class=\"govuk-visually-hidden\">  Details"
-        expect(response.body).to include(expected_html)
+      context "users tab" do
+        context "organisation that the user belongs to" do
+          before do
+            sign_in user
+            get "/organisations/#{organisation.id}/users", headers: headers, params: {}
+          end
+
+          it "shows the tab navigation" do
+            expected_html = "<nav class=\"app-tab-navigation\""
+            expect(response.body).to include(expected_html)
+          end
+
+          it "shows a new user button" do
+            expect(page).to have_link("Invite user")
+          end
+
+          it "shows a table of users" do
+            expected_html = "<table class=\"govuk-table\""
+            expect(response.body).to include(expected_html)
+            expect(response.body).to include(user.email)
+          end
+
+          it "has a hidden header title" do
+            expected_html = "<h2 class=\"govuk-visually-hidden\">  Users"
+            expect(response.body).to include(expected_html)
+          end
+        end
+
+        context "organisation that are not in scope for the user, i.e. that they do not belong to" do
+          before do
+            sign_in user
+            get "/organisations/#{unauthorised_organisation.id}/users", headers: headers, params: {}
+          end
+
+          it "returns not found 404 from users page" do
+            expect(response).to have_http_status(:not_found)
+          end
+        end
       end
     end
 
-    context "users tab" do
-      before do
-        sign_in user
-        get "/organisations/#{organisation.id}/users", headers: headers, params: {}
+    context "As a data provider user" do
+      let(:user) { FactoryBot.create(:user) }
+
+      context "details tab" do
+        context "organisation that the user belongs to" do
+          before do
+            sign_in user
+            get "/organisations/#{organisation.id}/details", headers: headers, params: {}
+          end
+
+          it "shows the tab navigation" do
+            expected_html = "<nav class=\"app-tab-navigation\""
+            expect(response.body).to include(expected_html)
+          end
+
+          it "shows a summary list of org details" do
+            expected_html = "<dl class=\"govuk-summary-list\""
+            expect(response.body).to include(expected_html)
+            expect(response.body).to include(organisation.name)
+          end
+
+          it "has a hidden header title" do
+            expected_html = "<h2 class=\"govuk-visually-hidden\">  Details"
+            expect(response.body).to include(expected_html)
+          end
+        end
+
+        context "organisation that are not in scope for the user, i.e. that they do not belong to" do
+          before do
+            sign_in user
+            get "/organisations/#{unauthorised_organisation.id}/details", headers: headers, params: {}
+          end
+
+          it "returns not found 404" do
+            expect(response).to have_http_status(:not_found)
+          end
+        end
       end
 
-      it "should return unauthorised 401" do
-        expect(response).to have_http_status(:unauthorized)
+      context "users tab" do
+        before do
+          sign_in user
+          get "/organisations/#{organisation.id}/users", headers: headers, params: {}
+        end
+
+        it "should return unauthorized 401" do
+          expect(response).to have_http_status(:unauthorized)
+        end
       end
     end
   end
diff --git a/spec/requests/soft_validations_controller_spec.rb b/spec/requests/soft_validations_controller_spec.rb
index d387ded97..83ffc3d58 100644
--- a/spec/requests/soft_validations_controller_spec.rb
+++ b/spec/requests/soft_validations_controller_spec.rb
@@ -3,36 +3,51 @@ require "rails_helper"
 RSpec.describe SoftValidationsController, type: :request do
   let(:params) { { case_log_id: case_log.id } }
   let(:url) { "/case-logs/#{case_log.id}/net-income/soft-validations" }
+  let(:user) { FactoryBot.create(:user) }
 
-  before do
-    get url, params: {}
-  end
-
-  describe "GET #show" do
-    context "Soft validation overide required" do
-      let(:case_log) { FactoryBot.create(:case_log, :soft_validations_triggered) }
+  context "a not signed in user" do
+    let(:case_log) { FactoryBot.create(:case_log, :in_progress) }
 
-      it "returns a success response" do
-        expect(response).to be_successful
+    describe "GET #show" do
+      it "redirects to the sign in page" do
+        get url, headers: headers, params: {}
+        expect(response).to redirect_to("/users/sign-in")
       end
+    end
+  end
 
-      it "returns a json with the soft validation fields" do
-        json_response = JSON.parse(response.body)
-        expect(json_response["show"]).to eq(true)
-        expect(json_response["label"]).to match(/Are you sure this is correct?/)
-      end
+  context "a signed in user" do
+    before do
+      sign_in user
+      get url, params: {}
     end
 
-    context "Soft validation overide not required" do
-      let(:case_log) { FactoryBot.create(:case_log, :in_progress) }
+    describe "GET #show" do
+      context "Soft validation overide required" do
+        let(:case_log) { FactoryBot.create(:case_log, :soft_validations_triggered) }
+
+        it "returns a success response" do
+          expect(response).to be_successful
+        end
 
-      it "returns a success response" do
-        expect(response).to be_successful
+        it "returns a json with the soft validation fields" do
+          json_response = JSON.parse(response.body)
+          expect(json_response["show"]).to eq(true)
+          expect(json_response["label"]).to match(/Are you sure this is correct?/)
+        end
       end
 
-      it "returns a json with the soft validation fields" do
-        json_response = JSON.parse(response.body)
-        expect(json_response["show"]).to eq(false)
+      context "Soft validation overide not required" do
+        let(:case_log) { FactoryBot.create(:case_log, :in_progress) }
+
+        it "returns a success response" do
+          expect(response).to be_successful
+        end
+
+        it "returns a json with the soft validation fields" do
+          json_response = JSON.parse(response.body)
+          expect(json_response["show"]).to eq(false)
+        end
       end
     end
   end
diff --git a/spec/requests/user_controller_spec.rb b/spec/requests/user_controller_spec.rb
index bccd2dd31..26f7bd959 100644
--- a/spec/requests/user_controller_spec.rb
+++ b/spec/requests/user_controller_spec.rb
@@ -3,39 +3,138 @@ require_relative "../support/devise"
 
 RSpec.describe UsersController, type: :request do
   let(:user) { FactoryBot.create(:user) }
+  let(:unauthorised_user) { FactoryBot.create(:user) }
   let(:headers) { { "Accept" => "text/html" } }
   let(:page) { Capybara::Node::Simple.new(response.body) }
+  let(:new_value) { "new test name" }
+  let(:params) { { id: user.id, user: { name: new_value } } }
+
+  context "a not signed in user" do
+    describe "#show" do
+      it "does not let you see user details" do
+        get "/users/#{user.id}", headers: headers, params: {}
+        expect(response).to redirect_to("/users/sign-in")
+      end
+    end
+
+    describe "#edit" do
+      it "does not let you edit user details" do
+        get "/users/#{user.id}/edit", headers: headers, params: {}
+        expect(response).to redirect_to("/users/sign-in")
+      end
+    end
+
+    describe "#password" do
+      it "does not let you edit user passwords" do
+        get "/users/#{user.id}/password/edit", headers: headers, params: {}
+        expect(response).to redirect_to("/users/sign-in")
+      end
+    end
+
+    describe "#patch" do
+      it "does not let you update user details" do
+        patch "/case-logs/#{user.id}", params: {}
+        expect(response).to redirect_to("/users/sign-in")
+      end
+    end
+  end
 
   describe "#show" do
-    before do
-      sign_in user
-      get "/users/#{user.id}", headers: headers, params: {}
+    context "current user is user" do
+      before do
+        sign_in user
+        get "/users/#{user.id}", headers: headers, params: {}
+      end
+
+      it "show the user details" do
+        expect(page).to have_content("Your account")
+      end
     end
 
-    it "show the user details" do
-      expect(page).to have_content("Your account")
+    context "current user is another user" do
+      before do
+        sign_in user
+        get "/users/#{unauthorised_user.id}", headers: headers, params: {}
+      end
+
+      it "returns not found 404" do
+        expect(response).to have_http_status(:not_found)
+      end
     end
   end
 
   describe "#edit" do
-    before do
-      sign_in user
-      get "/users/#{user.id}/edit", headers: headers, params: {}
+    context "current user is user" do
+      before do
+        sign_in user
+        get "/users/#{user.id}/edit", headers: headers, params: {}
+      end
+
+      it "show the edit personal details page" do
+        expect(page).to have_content("Change your personal details")
+      end
     end
 
-    it "show the edit personal details page" do
-      expect(page).to have_content("Change your personal details")
+    context "current user is another user" do
+      before do
+        sign_in user
+        get "/users/#{unauthorised_user.id}/edit", headers: headers, params: {}
+      end
+
+      it "returns not found 404" do
+        expect(response).to have_http_status(:not_found)
+      end
     end
   end
 
   describe "#edit_password" do
-    before do
-      sign_in user
-      get "/users/#{user.id}/password/edit", headers: headers, params: {}
+    context "current user is user" do
+      before do
+        sign_in user
+        get "/users/#{user.id}/password/edit", headers: headers, params: {}
+      end
+
+      it "show the edit password page" do
+        expect(page).to have_content("Change your password")
+      end
+    end
+
+    context "current user is another user" do
+      before do
+        sign_in user
+        get "/users/#{unauthorised_user.id}/edit", headers: headers, params: {}
+      end
+
+      it "returns not found 404" do
+        expect(response).to have_http_status(:not_found)
+      end
     end
+  end
+
+  describe "#update" do
+    context "current user is user" do
+      before do
+        sign_in user
+        patch "/users/#{user.id}", headers: headers, params: params
+      end
+
+      it "updates the user" do
+        user.reload
+        expect(user.name).to eq(new_value)
+      end
+    end
+
+    context "current user is another user" do
+      let(:params) { { id: unauthorised_user.id, user: { name: new_value } } }
+
+      before do
+        sign_in user
+        patch "/users/#{unauthorised_user.id}", headers: headers, params: params
+      end
 
-    it "show the edit password page" do
-      expect(page).to have_content("Change your password")
+      it "returns not found 404" do
+        expect(response).to have_http_status(:not_found)
+      end
     end
   end
 end