From b6b6d9d75d6d62baf8f2b59d8c04d7ec5430d612 Mon Sep 17 00:00:00 2001 From: davidgisbey Date: Mon, 8 Jan 2024 16:37:20 +0000 Subject: [PATCH 1/4] Ensure GDS Admin can search for limited access documents We're doing some work to allow GDS Admins to remove limited access/and or update the organisations for one. In order to do this GDS Admins will need to be able to search for the document before being able to make the updates. This updates the EditionFilter to permit access limited documents for GDS Admins. --- app/models/admin/edition_filter.rb | 2 ++ .../authority/rules/edition_rules.rb | 2 ++ .../admin/editions_controller_test.rb | 12 +++++++++ .../app/models/admin/edition_filter_test.rb | 26 +++++++++++++++++++ 4 files changed, 42 insertions(+) diff --git a/app/models/admin/edition_filter.rb b/app/models/admin/edition_filter.rb index e294fd8f5b8..46ba0ac0c00 100644 --- a/app/models/admin/edition_filter.rb +++ b/app/models/admin/edition_filter.rb @@ -139,6 +139,8 @@ def editions_with_translations(locale = nil) end def permitted_only(requested_editions) + return requested_editions if Whitehall::Authority::Enforcer.new(@current_user, Edition).can?(:perform_administrative_tasks) + requested_editions.select do |edition| Whitehall::Authority::Enforcer.new(@current_user, edition).can?(:see) end diff --git a/lib/whitehall/authority/rules/edition_rules.rb b/lib/whitehall/authority/rules/edition_rules.rb index f5d768418f0..8d92336a85a 100644 --- a/lib/whitehall/authority/rules/edition_rules.rb +++ b/lib/whitehall/authority/rules/edition_rules.rb @@ -134,6 +134,8 @@ def not_publishing_scheduled_edition_without_authority? def can_with_a_class?(action) case action + when :perform_administrative_tasks + actor.gds_admin? when :export, :confirm_export actor.gds_admin? || actor.gds_editor? || actor.managing_editor? || actor.departmental_editor? when :create, :see diff --git a/test/functional/admin/editions_controller_test.rb b/test/functional/admin/editions_controller_test.rb index c0a00a4534f..914ae8d2e3b 100644 --- a/test/functional/admin/editions_controller_test.rb +++ b/test/functional/admin/editions_controller_test.rb @@ -254,6 +254,18 @@ class Admin::EditionsControllerTest < ActionController::TestCase end end + view_test "GET :index GDS Admins should be able to view all access limited documents" do + login_as create(:gds_admin) + publication = create(:draft_publication, access_limited: true) + + get :index, params: { state: :active } + + assert_select ".govuk-table__row" do + assert_select ".govuk-table__header:nth-child(1)", text: publication.title + assert_select ".govuk-table__cell:nth-child(3)", text: "Draft Limited access" + end + end + test "prevents revising of access-limited editions" do my_organisation = create(:organisation) other_organisation = create(:organisation) diff --git a/test/unit/app/models/admin/edition_filter_test.rb b/test/unit/app/models/admin/edition_filter_test.rb index 43ad9ec6544..91eaf78b520 100644 --- a/test/unit/app/models/admin/edition_filter_test.rb +++ b/test/unit/app/models/admin/edition_filter_test.rb @@ -5,6 +5,32 @@ class Admin::EditionFilterTest < ActiveSupport::TestCase @current_user = build(:gds_editor) end + test "should return limited access editions when the edition is published by the users organisation" do + user = create(:user) + edition = create( + :news_article, + access_limited: true, + ) + edition.organisations.first.users << user + + filter = Admin::EditionFilter.new(Edition, user) + assert_equal edition, filter.editions.first + end + + test "should not return editions which have limited access for other orgs for non-gds admins" do + create(:news_article, access_limited: true) + + filter = Admin::EditionFilter.new(Edition, build(:user)) + assert_equal 0, filter.editions.count + end + + test "should return limited access editions for GDS admins" do + edition = create(:news_article, access_limited: true) + + filter = Admin::EditionFilter.new(Edition, build(:gds_admin)) + assert_equal edition, filter.editions.first + end + test "can preload unpublishing data if asked to" do news_article = create(:news_article) create(:unpublishing, edition: news_article) From 7f39b731c3632180c37df0379e89d0768d5e1b13 Mon Sep 17 00:00:00 2001 From: davidgisbey Date: Tue, 9 Jan 2024 09:31:29 +0000 Subject: [PATCH 2/4] Render link to edit an organisations access on doc search page This ensures that links render in the following way: When the user is a GDS Admin: - renders the view document and edit access links if edition belongs to their org (GDS) - renders the edit access link if the edition doesn't belong to their org When the user is not a GDS Admin: - renders the view link when the edition belongs to their org - never renders the edit access link --- app/helpers/admin/editions_helper.rb | 21 +++++++++++++++++++ .../admin/editions/_search_results.html.erb | 2 +- .../admin/editions_controller_test.rb | 4 +++- 3 files changed, 25 insertions(+), 2 deletions(-) diff --git a/app/helpers/admin/editions_helper.rb b/app/helpers/admin/editions_helper.rb index 08526e9197c..9d4b42dc8e9 100644 --- a/app/helpers/admin/editions_helper.rb +++ b/app/helpers/admin/editions_helper.rb @@ -318,4 +318,25 @@ def reset_search_fields_query_string_params(user, filter_action, anchor) filter_action + query_string_params + anchor end + + def search_results_table_actions(edition) + actions = "" + if can?(:see, edition) + actions << link_to( + sanitize("View #{tag.span(edition.title, class: 'govuk-visually-hidden')}"), + admin_edition_path(edition), + class: "govuk-link", + ) + end + + if can?(:perform_administrative_tasks, Edition) && edition.access_limited + actions << link_to( + sanitize("Edit access #{tag.span("for #{edition.title}", class: 'govuk-visually-hidden')}"), + edit_access_limited_admin_edition_path(edition), + class: "govuk-link", + ) + end + + sanitize(actions) + end end diff --git a/app/views/admin/editions/_search_results.html.erb b/app/views/admin/editions/_search_results.html.erb index 1d6e0cfdf38..978ad8e6ab5 100644 --- a/app/views/admin/editions/_search_results.html.erb +++ b/app/views/admin/editions/_search_results.html.erb @@ -40,7 +40,7 @@ text: render(Admin::Editions::TagsComponent.new(edition)), }, { - text: link_to(sanitize("View #{tag.span(edition.title, class: 'govuk-visually-hidden')}"), admin_edition_path(edition), class: "govuk-link"), + text: search_results_table_actions(edition), }, ] end, diff --git a/test/functional/admin/editions_controller_test.rb b/test/functional/admin/editions_controller_test.rb index 914ae8d2e3b..b29c149afa7 100644 --- a/test/functional/admin/editions_controller_test.rb +++ b/test/functional/admin/editions_controller_test.rb @@ -254,7 +254,7 @@ class Admin::EditionsControllerTest < ActionController::TestCase end end - view_test "GET :index GDS Admins should be able to view all access limited documents" do + view_test "GET :index admin do not see a view link, but are given a link to edit acccess controls when an access limited edition doesn't belong to their org" do login_as create(:gds_admin) publication = create(:draft_publication, access_limited: true) @@ -263,6 +263,8 @@ class Admin::EditionsControllerTest < ActionController::TestCase assert_select ".govuk-table__row" do assert_select ".govuk-table__header:nth-child(1)", text: publication.title assert_select ".govuk-table__cell:nth-child(3)", text: "Draft Limited access" + assert_select ".govuk-table__cell:nth-child(4) a[href='#{edit_access_limited_admin_edition_path(publication)}']", text: "Edit access for #{publication.title}" + assert_select ".govuk-table__cell:nth-child(4) .govuk-link", text: "View #{publication.title}", count: 0 end end From 8318fcc07bd7cd52cbf7ee0c5391fb0695464451 Mon Sep 17 00:00:00 2001 From: davidgisbey Date: Tue, 9 Jan 2024 15:19:09 +0000 Subject: [PATCH 3/4] Add endpoint that allows an editions access controls to be updated This adds a route, controller and view which handle updating an editions access_limited, lead organisations and supporting organisations. These endpoints are only available to GDS Admins. --- .../edition_access_limited_controller.rb | 76 ++++++++ .../edition_access_limited/edit.html.erb | 39 ++++ config/routes.rb | 2 + .../edition_access_limited_controller_test.rb | 175 ++++++++++++++++++ 4 files changed, 292 insertions(+) create mode 100644 app/controllers/admin/edition_access_limited_controller.rb create mode 100644 app/views/admin/edition_access_limited/edit.html.erb create mode 100644 test/functional/admin/edition_access_limited_controller_test.rb diff --git a/app/controllers/admin/edition_access_limited_controller.rb b/app/controllers/admin/edition_access_limited_controller.rb new file mode 100644 index 00000000000..ec8baf9adeb --- /dev/null +++ b/app/controllers/admin/edition_access_limited_controller.rb @@ -0,0 +1,76 @@ +class Admin::EditionAccessLimitedController < Admin::BaseController + before_action :find_edition + before_action :enforce_permissions! + before_action :clean_organisation_params, only: %i[update] + + def edit; end + + def update + editorial_remark = edition_params.delete(:editorial_remark) + @edition.assign_attributes(edition_params) + + if changed? + if editorial_remark.blank? + @edition.errors.add(:editorial_remark, "can't be blank") + + render :edit + else + @edition.save! + + EditorialRemark.create!( + edition: @edition, + body: "Access options updated by GDS Admin: #{editorial_remark}", + author: current_user, + created_at: Time.zone.now, + updated_at: Time.zone.now, + ) + + redirect_to admin_editions_path, notice: "Access updated for #{@edition.title}" + end + else + redirect_to admin_editions_path, notice: "Access updated for #{@edition.title}" + end + end + +private + + def find_edition + @edition = Edition.find(params[:id]) + end + + def enforce_permissions! + enforce_permission!(:perform_administrative_tasks, Edition) + end + + def edition_params + @edition_params ||= params + .fetch(:edition, {}) + .permit( + :access_limited, + :editorial_remark, + { + lead_organisation_ids: [], + supporting_organisation_ids: [], + }, + ) + end + + def clean_organisation_params + if edition_params[:lead_organisation_ids] + edition_params[:lead_organisation_ids] = edition_params[:lead_organisation_ids].reject(&:blank?) + end + if edition_params[:supporting_organisation_ids] + edition_params[:supporting_organisation_ids] = edition_params[:supporting_organisation_ids].reject(&:blank?) + end + end + + def changed? + if @edition.can_be_related_to_organisations? + @edition.changed? || + @edition.lead_organisation_ids.map(&:to_s) != edition_params[:lead_organisation_ids] || + @edition.supporting_organisations.map(&:id).map(&:to_s) != edition_params[:supporting_organisation_ids] + else + @edition.changed? + end + end +end diff --git a/app/views/admin/edition_access_limited/edit.html.erb b/app/views/admin/edition_access_limited/edit.html.erb new file mode 100644 index 00000000000..bf5eed7edaa --- /dev/null +++ b/app/views/admin/edition_access_limited/edit.html.erb @@ -0,0 +1,39 @@ +<% content_for :page_title, "#{@edition.title}: Update access" %> +<% content_for :context, @edition.title %> +<% content_for :title, "Update access" %> +<% content_for :error_summary, render(Admin::ErrorSummaryComponent.new(object: @edition, parent_class: "edition")) %> +<% content_for :back_link do %> + <%= render "govuk_publishing_components/components/back_link", { + href: admin_editions_path, + } %> +<% end %> + +
+
+ <%= form_for @edition, url: update_access_limited_admin_edition_path(@edition.id), as: :edition, method: :patch do |form| %> + + <%= render "govuk_publishing_components/components/warning_text", { + text: "This page in only available to GDS Admins. Information in this document could be sensitive and access controls should only be changed at the request of the user.", + } %> + + <%= render "admin/editions/organisation_fields", form: form, edition: @edition %> + <%= render "admin/editions/access_limiting_fields", form: form, edition: @edition %> + + <%= render "govuk_publishing_components/components/textarea", { + label: { + text: "Editorial remark (required)", + heading_size: "l", + }, + name: "edition[editorial_remark]", + id: "edition_editorial_remark", + error_items: errors_for(@edition.errors, :editorial_remark), + hint: "Please explain why this change is required", + rows: 20, + } %> + + <%= render "govuk_publishing_components/components/button", { + text: "Save", + } %> + <% end %> +
+
diff --git a/config/routes.rb b/config/routes.rb index 1ea693d71e7..c562b768a71 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -200,6 +200,8 @@ def redirect(path, options = { prefix: Whitehall.router_prefix }) patch :update_bypass_id patch :update_image_display_option, controller: "case_studies" get :confirm_destroy + get :edit_access_limited, to: "edition_access_limited#edit" + patch :update_access_limited, to: "edition_access_limited#update" end resources :link_check_reports resource :unpublishing, controller: "edition_unpublishing", only: %i[edit update] diff --git a/test/functional/admin/edition_access_limited_controller_test.rb b/test/functional/admin/edition_access_limited_controller_test.rb new file mode 100644 index 00000000000..cf1350f06ca --- /dev/null +++ b/test/functional/admin/edition_access_limited_controller_test.rb @@ -0,0 +1,175 @@ +require "test_helper" + +class Admin::EditionAccessLimitedControllerTest < ActionController::TestCase + setup do + login_as :gds_admin + end + + should_be_an_admin_controller + + test "GET :edit should be forbidden unless user is a GDS Admin" do + edition = create(:consultation) + login_as :gds_editor + get :edit, params: { id: edition } + assert_response :forbidden + end + + view_test "GET :edit should display the correct fields" do + organisation = create(:organisation) + + edition = create( + :consultation, + access_limited: true, + create_default_organisation: false, + lead_organisations: [organisation], + ) + + get :edit, params: { id: edition } + + assert_select "form[action='#{update_access_limited_admin_edition_path(edition.id)}']" do + assert_select "input[name='edition[access_limited]'][type=checkbox][checked=checked]" + assert_select "textarea[name='edition[editorial_remark]']" + + (1..4).each do |i| + assert_select "label[for=edition_lead_organisation_ids_#{i}]", text: "Lead organisation #{i}" + assert_select("#edition_lead_organisation_ids_#{i}") + end + + assert_select("#edition_lead_organisation_ids_1") do + assert_select "option[selected='selected']", value: organisation.id + end + + refute_select "#edition_lead_organisation_ids_5" + assert_select("#edition_supporting_organisation_ids_") + end + end + + test "PATCH :update should be forbidden unless user is a GDS Admin" do + edition = create(:consultation) + login_as :gds_editor + get :edit, params: { id: edition } + assert_response :forbidden + end + + test "PATCH :update should update editions organisations correctly and creates an editorial remark" do + first_organisation = create(:organisation) + second_organisation = create(:organisation) + third_organisation = create(:organisation) + + edition = create( + :consultation, + access_limited: true, + create_default_organisation: false, + lead_organisations: [first_organisation], + supporting_organisations: [second_organisation], + ) + + editorial_remark = "Updating the organisations at the users request." + + put :update, + params: { + id: edition, + edition: { + lead_organisation_ids: [second_organisation.id], + supporting_organisation_ids: [third_organisation.id], + editorial_remark:, + access_limited: "1", + }, + } + + assert_equal [second_organisation], edition.reload.lead_organisations + assert_equal [third_organisation], edition.supporting_organisations + assert_redirected_to admin_editions_path + assert_equal "Access updated for #{edition.title}", flash[:notice] + assert_equal "Access options updated by GDS Admin: #{editorial_remark}", edition.editorial_remarks.last.body + end + + test "PATCH :update allows access_limited to be updated and creates an editorial remark" do + first_organisation = create(:organisation) + second_organisation = create(:organisation) + + edition = create( + :consultation, + access_limited: true, + create_default_organisation: false, + lead_organisations: [first_organisation], + supporting_organisations: [second_organisation], + ) + + editorial_remark = "Removing access limited at the users request." + + put :update, + params: { + id: edition, + edition: { + lead_organisation_ids: [first_organisation.id], + supporting_organisation_ids: [second_organisation.id], + editorial_remark:, + access_limited: "0", + }, + } + + assert_equal [first_organisation], edition.reload.lead_organisations + assert_equal [second_organisation], edition.supporting_organisations + assert_not edition.access_limited + assert_redirected_to admin_editions_path + assert_equal "Access updated for #{edition.title}", flash[:notice] + assert_equal "Access options updated by GDS Admin: #{editorial_remark}", edition.editorial_remarks.last.body + end + + test "PATCH :update re-renders edit template if editorial remark is not provided" do + first_organisation = create(:organisation) + second_organisation = create(:organisation) + + edition = create( + :consultation, + access_limited: true, + create_default_organisation: false, + lead_organisations: [first_organisation], + supporting_organisations: [second_organisation], + ) + + put :update, + params: { + id: edition, + edition: { + lead_organisation_ids: [first_organisation.id], + supporting_organisation_ids: [second_organisation.id], + editorial_remark: "", + access_limited: "0", + }, + } + + assert_template :edit + assert_equal ["Editorial remark can't be blank"], assigns(:edition).errors.full_messages + assert edition.reload.access_limited + end + + test "PATCH :update doesn't create an editorial remark or re-render with an error when nothing has changed" do + first_organisation = create(:organisation) + second_organisation = create(:organisation) + + edition = create( + :consultation, + access_limited: true, + create_default_organisation: false, + lead_organisations: [first_organisation], + supporting_organisations: [second_organisation], + ) + + put :update, + params: { + id: edition, + edition: { + lead_organisation_ids: [first_organisation.id], + supporting_organisation_ids: [second_organisation.id], + editorial_remark: "", + access_limited: "1", + }, + } + + assert_redirected_to admin_editions_path + assert_equal "Access updated for #{edition.title}", flash[:notice] + assert_equal 0, edition.editorial_remarks.count + end +end From 63c01b79ba5865587705878e761504ee6da86608 Mon Sep 17 00:00:00 2001 From: davidgisbey Date: Wed, 10 Jan 2024 13:45:58 +0000 Subject: [PATCH 4/4] Add documentation for updating access controls --- docs/access_controls.md | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 docs/access_controls.md diff --git a/docs/access_controls.md b/docs/access_controls.md new file mode 100644 index 00000000000..757fda84979 --- /dev/null +++ b/docs/access_controls.md @@ -0,0 +1,22 @@ +# Access controls for editions + +Publishers can limit access for a document on current editions edit page. This is used to keep sensitive data private prior to the publication +of documents. + +When access is limited, only users that belong one of the lead or supporting organisations assigned to the document will be able to access it. + +On 2ndline, we often get requests involving documents which have limited access. These tend to fall into 2 categories: + +- A Publisher has limited the access to the document but limited it to the wrong organisation +- There is an issue/bug with a document that has limited access + +Previously we have dealt with these requests by investigating the issues via console. + +GDS Admins can now update access control for these documents by searching for the document on the document search page and clicking the "Edit access" link. + +This page allows you to update the editions associated organisations and/or remove limited access completely. If you make changes on this +page you will be required to provide an editorial remark that will be used to audit the change. + +If you need to investigate an issue with the document you can add temporarily gain access to the document by adding GDS as a supporting organisation, then removing it +once you've completed your investigation. +