From 373ab101ebce72e22821d5c3044c2452ec02ece3 Mon Sep 17 00:00:00 2001 From: Keith Lawrence Date: Tue, 30 Jul 2024 07:33:57 +0100 Subject: [PATCH 01/12] Add ADR about department permissions --- docs/adr/adr-001-department-permissions.md | 41 ++++++++++++++++++++++ 1 file changed, 41 insertions(+) create mode 100644 docs/adr/adr-001-department-permissions.md diff --git a/docs/adr/adr-001-department-permissions.md b/docs/adr/adr-001-department-permissions.md new file mode 100644 index 000000000..5a1c59b17 --- /dev/null +++ b/docs/adr/adr-001-department-permissions.md @@ -0,0 +1,41 @@ +# Decision Record: Department Permissions + +## Introduction + +In July 2024 Places Manager was opened up to departments to allow them to +directly control their own datasets. It had long been suggested that the +same openness should be a feature of Local Links Manager, since departments +often have more information about particular services than GDS. This would form +part of a future possible three-way access system in which GDS Editors could +edit any link, departments could edit links in particular services, and local +authorities could edit links in their authority. This ADR moves towards this +by opening up the second of these three ways. + +## Requirements + +Each service would need to be owned by zero, one, or more than one +departments. Only editors with Local Links Managers access permission in +Signon should be allowed to view and edit those services, with GDS editors +allowed to access all services for troubleshooting and incident response. + +We followed the pattern from Places Manager, which was itself based on the +style of permission in Whitehall and other publishing apps, where Signon +provides the current user's organisational slug and access can be limited +based on that. A "GDS Editor" special permission is also typical of these +apps. + +## Resulting changes + +- Add an `organisational_slugs` field to each service, to be filled in by + us for existing services before departments are given access. +- Add a `GDS Editor` permission. Anyone with this permission can see and + edit links for all services. Anyone without this permission can + only edit links in services whose organisational_slugs field contains + the same slug as reported for them by Signon. +- Add a UI to edit the organisational slugs. Like Places Manager, this + will be a simple string field, with space separation for multiple owners, + editable only by someone with the `GDS Editor` permission. We will not at + the moment add in an organisational drop-down, so any GDS Editor + making changes to the organisation slug will need to know the correct one, + but it is assumed that people with this permission will know how to find + that out. \ No newline at end of file From 698c8105bab41c4ef1b5f9b2063259b032b94e8c Mon Sep 17 00:00:00 2001 From: Keith Lawrence Date: Mon, 12 Aug 2024 15:11:17 +0100 Subject: [PATCH 02/12] Add documentation for new permission, feature --- docs/permissions.md | 17 +++++++++++++++++ docs/service-owners.md | 5 +++++ 2 files changed, 22 insertions(+) create mode 100644 docs/permissions.md create mode 100644 docs/service-owners.md diff --git a/docs/permissions.md b/docs/permissions.md new file mode 100644 index 000000000..d1750b9f9 --- /dev/null +++ b/docs/permissions.md @@ -0,0 +1,17 @@ +# Permissions + +## Named Permissions + +- `GDS Editor`: gives the user permission to do all actions in the app. + +## Department Permissions + +Other permissions are based on the organisation_slug of the current user. If a user does not have the `GDS Editor` permission they will be able to: + +- visit the Services page `/services`, which will only show services where the current user's organisation slug is contained in the service's organisation_slugs array. +- visit the specific service pages of those services +- download a csv of links for those services +- download a csv of new links for those services +- edit links in those services. + +The organisation slugs array is editable only by people with the `GDS Editor` permission. diff --git a/docs/service-owners.md b/docs/service-owners.md new file mode 100644 index 000000000..ba5be4b7b --- /dev/null +++ b/docs/service-owners.md @@ -0,0 +1,5 @@ +# Service Owners + +Most services currently do not have an owner, but a service can be assigned one or more owning organisations by a user with the `GDS Editor` [permission](/docs/permissions.md). Visit the service, and select the "Update Owner" action from the sidebar. You can then enter one or more organisations by their organisation slug (the final part of their organisation URL on gov.uk, eg: 'government-digital-service' from https://www.gov.uk/government/organisations/government-digital-service), separating organisations with a space if there are more than one. + +This allows users from that department with access to Local Links Manager to edit the service as detailed in the [Permissions page](/docs/permissions.md) From e505bdc27358b231850a1cecbec33364f380e888 Mon Sep 17 00:00:00 2001 From: ramya vidapanakal Date: Tue, 30 Jul 2024 15:41:20 +0100 Subject: [PATCH 03/12] Add organisation_slugs to service record --- .../20240730142812_modify_services_add_organisation_slug.rb | 5 +++++ db/schema.rb | 3 ++- 2 files changed, 7 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20240730142812_modify_services_add_organisation_slug.rb diff --git a/db/migrate/20240730142812_modify_services_add_organisation_slug.rb b/db/migrate/20240730142812_modify_services_add_organisation_slug.rb new file mode 100644 index 000000000..2ee3da71e --- /dev/null +++ b/db/migrate/20240730142812_modify_services_add_organisation_slug.rb @@ -0,0 +1,5 @@ +class ModifyServicesAddOrganisationSlug < ActiveRecord::Migration[7.1] + def change + add_column :services, :organisation_slugs, :string, array: true, default: [] + end +end diff --git a/db/schema.rb b/db/schema.rb index 09e4793f5..a3ed1d99b 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.1].define(version: 2024_03_18_105241) do +ActiveRecord::Schema[7.1].define(version: 2024_07_30_142812) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" @@ -106,6 +106,7 @@ t.string "slug", null: false t.boolean "enabled", default: false, null: false t.integer "broken_link_count", default: 0 + t.string "organisation_slugs", default: [], array: true t.index ["label"], name: "index_services_on_label", unique: true t.index ["lgsl_code"], name: "index_services_on_lgsl_code", unique: true end From 6fbaa2913694751d50b23ce76e3eca4addc82890 Mon Sep 17 00:00:00 2001 From: Keith Lawrence Date: Wed, 14 Aug 2024 12:12:07 +0100 Subject: [PATCH 04/12] Update main menu - Add Switch app option - Add check so that without the GDS Editor permission, a user won't see the Broken Links or Councils links - Add new System test to check this - Update authentication support file to simplify it - Include authentication by default in all test types - Add infer test type to RSpec Co-authored-by: Ramya Vidapanakal --- app/controllers/application_controller.rb | 4 ++ .../concerns/service_permissions.rb | 5 +++ app/views/layouts/application.html.erb | 36 ++++++++++++------ spec/rails_helper.rb | 6 +++ spec/support/authentication.rb | 17 ++++----- spec/system/main_menu_spec.rb | 38 +++++++++++++++++++ 6 files changed, 84 insertions(+), 22 deletions(-) create mode 100644 app/controllers/concerns/service_permissions.rb create mode 100644 spec/system/main_menu_spec.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 094220635..1999b934e 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -1,5 +1,9 @@ class ApplicationController < ActionController::Base include GDS::SSO::ControllerMethods + include ServicePermissions + + helper_method :gds_editor? + before_action :authenticate_user! # Prevent CSRF attacks by raising an exception. # For APIs, you may want to use :null_session instead. diff --git a/app/controllers/concerns/service_permissions.rb b/app/controllers/concerns/service_permissions.rb new file mode 100644 index 000000000..ef85e7f67 --- /dev/null +++ b/app/controllers/concerns/service_permissions.rb @@ -0,0 +1,5 @@ +module ServicePermissions + def gds_editor? + current_user.permissions.include?("GDS Editor") + end +end diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index 9cfcb91ce..158035f9c 100644 --- a/app/views/layouts/application.html.erb +++ b/app/views/layouts/application.html.erb @@ -16,11 +16,11 @@ browser_title: (yield :page_title), } do %> -<%= render "govuk_publishing_components/components/layout_header", { - product_name: "Local Links Manager", - environment:, - navigation_items: [ - { +<% + menu_option = [] + + if gds_editor? + menu_option += [{ text: "Broken Links", href: root_path, active: current_page?(root_path), @@ -29,13 +29,25 @@ text: "Councils", href: local_authorities_path(filter: %w[only_active]), active: current_page?(local_authorities_path), - }, - { - text: "Services", - href: services_path, - active: current_page?(services_path), - }, - ], + }] + end + + menu_option << { + text: "Services", + href: services_path, + active: current_page?(services_path), + } + + menu_option << { + text: "Switch app", + href: Plek.external_url_for("signon"), + } +%> + +<%= render "govuk_publishing_components/components/layout_header", { + product_name: "Local Links Manager", + environment:, + navigation_items: menu_option, } %>
diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index f867784ab..27afc88fd 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -39,6 +39,10 @@ Rails.application.load_tasks RSpec.configure do |config| + config.infer_spec_type_from_file_location! + config.before(:each, type: :system) do + driven_by :rack_test + end # Remove this line if you're not using ActiveRecord or ActiveRecord fixtures config.fixture_paths = [Rails.root.join("spec/fixtures")] @@ -67,6 +71,8 @@ # arbitrary gems may also be filtered via: # config.filter_gems_from_backtrace("gem name") + config.include AuthenticationControllerHelpers + config.before do stub_publishing_api_for_external_content end diff --git a/spec/support/authentication.rb b/spec/support/authentication.rb index 0bf2cd92f..2d13b71df 100644 --- a/spec/support/authentication.rb +++ b/spec/support/authentication.rb @@ -1,18 +1,15 @@ module AuthenticationControllerHelpers def login_as(user) - request.env["warden"] = double( - authenticate!: true, - authenticated?: true, - user:, - ) + allow_any_instance_of(ApplicationController).to receive(:authenticate_user!).and_return(true) + allow_any_instance_of(ApplicationController).to receive(:user_signed_in?).and_return(true) + allow_any_instance_of(ApplicationController).to receive(:current_user).and_return(user) end - def stub_user - create(:user) + def login_as_department_user(organisation_slug: "random-department") + login_as(create(:user, organisation_slug:)) end - def login_as_stub_user - login_as stub_user + def login_as_gds_editor + login_as(create(:user, permissions: ["GDS Editor"], organisation_slug: "government-digital-service")) end end -RSpec.configuration.include AuthenticationControllerHelpers, type: :controller diff --git a/spec/system/main_menu_spec.rb b/spec/system/main_menu_spec.rb new file mode 100644 index 000000000..ec9c9bf5f --- /dev/null +++ b/spec/system/main_menu_spec.rb @@ -0,0 +1,38 @@ +require "gds_api/test_helpers/organisations" + +RSpec.describe "Main menu" do + include GdsApi::TestHelpers::Organisations + + context "as a normal user" do + before do + login_as_department_user + stub_organisations_api_has_organisations_with_bodies([{ "title" => "Department of Randomness", "details" => { "slug" => "random-department" } }]) + end + + it "shows only the Services/Switch app menu items" do + visit "/" + + within(".govuk-header__container") do + expect(page).not_to have_link("Broken Links") + expect(page).not_to have_link("Councils") + expect(page).to have_link("Services") + expect(page).to have_link("Switch app") + end + end + end + + context "as a GDS Editor" do + before { login_as_gds_editor } + + it "shows all four menu options" do + visit "/" + + within(".govuk-header__container") do + expect(page).to have_link("Broken Links") + expect(page).to have_link("Councils") + expect(page).to have_link("Services") + expect(page).to have_link("Switch app") + end + end + end +end From 0483412389239cfed28cfc5520c687c4c6a92ca4 Mon Sep 17 00:00:00 2001 From: Keith Lawrence Date: Mon, 19 Aug 2024 11:01:46 +0100 Subject: [PATCH 05/12] Use permissions in links controller - Auto-redirect to services page if not a GDS Editor, or forbid if it's not a navigable page. - Edit link can be accessed by an owner. - Add request tests to enforce these checks. - Add shared examples to make tests simpler. Co-authored-by: Ramya Vidapanakal --- .../concerns/service_permissions.rb | 20 ++++ app/controllers/links_controller.rb | 5 + spec/requests/broken_links_page_spec.rb | 3 + spec/requests/edit_link_page_spec.rb | 100 ++++++++++++++++++ spec/requests/link_status_csv_spec.rb | 5 + spec/support/authentication.rb | 76 +++++++++++++ 6 files changed, 209 insertions(+) create mode 100644 spec/requests/broken_links_page_spec.rb create mode 100644 spec/requests/edit_link_page_spec.rb create mode 100644 spec/requests/link_status_csv_spec.rb diff --git a/app/controllers/concerns/service_permissions.rb b/app/controllers/concerns/service_permissions.rb index ef85e7f67..334324a3f 100644 --- a/app/controllers/concerns/service_permissions.rb +++ b/app/controllers/concerns/service_permissions.rb @@ -2,4 +2,24 @@ module ServicePermissions def gds_editor? current_user.permissions.include?("GDS Editor") end + + def service_owner?(service) + service.organisation_slugs.include?(current_user.organisation_slug) + end + + def permission_for_service?(service) + gds_editor? || service_owner?(service) + end + + def redirect_unless_gds_editor + redirect_to services_path unless gds_editor? + end + + def forbid_unless_permission + raise GDS::SSO::PermissionDeniedError, "You do not have permission to view this page" unless permission_for_service?(@service) + end + + def forbid_unless_gds_editor + raise GDS::SSO::PermissionDeniedError, "You do not have permission to view this page" unless gds_editor? + end end diff --git a/app/controllers/links_controller.rb b/app/controllers/links_controller.rb index b9a0ae7b0..badc5801e 100644 --- a/app/controllers/links_controller.rb +++ b/app/controllers/links_controller.rb @@ -1,6 +1,11 @@ class LinksController < ApplicationController before_action :load_dependencies, only: %i[edit update destroy] before_action :set_back_url_before_post_request, only: %i[edit update destroy] + + before_action :forbid_unless_permission, only: %i[edit update] + before_action :forbid_unless_gds_editor, only: %i[destroy homepage_links_status_csv links_status_csv bad_links_url_and_status_csv] + before_action :redirect_unless_gds_editor, only: %i[index] + helper_method :back_url def index diff --git a/spec/requests/broken_links_page_spec.rb b/spec/requests/broken_links_page_spec.rb new file mode 100644 index 000000000..f648f1427 --- /dev/null +++ b/spec/requests/broken_links_page_spec.rb @@ -0,0 +1,3 @@ +RSpec.describe "Broken links page" do + it_behaves_like "redirects non-GDS Editors to services page", "/" +end diff --git a/spec/requests/edit_link_page_spec.rb b/spec/requests/edit_link_page_spec.rb new file mode 100644 index 000000000..cdf490efe --- /dev/null +++ b/spec/requests/edit_link_page_spec.rb @@ -0,0 +1,100 @@ +RSpec.describe "Edit link page" do + let(:owning_department) { "department-of-aardvarks" } + let!(:service) { create(:service, label: "Aardvark Wardens", organisation_slugs: [owning_department]) } + let!(:interaction) { create(:interaction, label: "reporting") } + let!(:service_interaction) { create(:service_interaction, service:, interaction:) } + let!(:local_authority) { create(:district_council, slug: "north-midlands") } + let!(:link) { create(:link, local_authority:, service_interaction:) } + + describe "GET /local_authorities/:local_authority_slug/services/:service_slug/:interaction_slug/edit" do + it_behaves_like "it is forbidden to non-owners", "/local_authorities/north-midlands/services/aardvark-wardens/reporting/edit", "department-of-aardvarks" + end + + describe "PUT /local_authorities/:local_authority_slug/services/:service_slug/:interaction_slug" do + let(:path) { "/local_authorities/north-midlands/services/aardvark-wardens/reporting" } + let(:url) { "http://www.example.com/new" } + + context "as a GDS Editor" do + before { login_as_gds_editor } + + it "returns 302 Found" do + put path, params: { url: } + + expect(response).to have_http_status(:found) + end + + it "updates the link" do + put path, params: { url: } + + expect(link.reload.url).to eq(url) + end + end + + context "as a department user from the owning department" do + before { login_as_department_user(organisation_slug: owning_department) } + + it "returns 302 Found" do + put path, params: { url: } + + expect(response).to have_http_status(:found) + end + + it "updates the link" do + put path, params: { url: } + + expect(link.reload.url).to eq(url) + end + end + + context "as a department user" do + before { login_as_department_user } + + it "returns 403 Forbidden" do + put path, params: { url: } + + expect(response).to have_http_status(:forbidden) + end + + it "does not update the link" do + original_link = link.url + put path, params: { url: } + + expect(link.reload.url).to eq(original_link) + end + end + end + + describe "DELETE /local_authorities/:local_authority_slug/services/:service_slug/:interaction_slug" do + let(:path) { "/local_authorities/north-midlands/services/aardvark-wardens/reporting" } + + context "as a GDS Editor" do + before { login_as_gds_editor } + + it "returns 302 Found" do + delete path + + expect(response).to have_http_status(:found) + end + end + + context "as a department user from the owning department" do + before { login_as_department_user(organisation_slug: owning_department) } + + it "returns 403 Forbidden" do + delete path + + expect(response).to have_http_status(:forbidden) + end + end + + context "as a department user" do + before { login_as_department_user } + + it "returns 403 Forbidden" do + delete path + + expect(response).to have_http_status(:forbidden) + end + end + end +end diff --git a/spec/requests/link_status_csv_spec.rb b/spec/requests/link_status_csv_spec.rb new file mode 100644 index 000000000..a535799c5 --- /dev/null +++ b/spec/requests/link_status_csv_spec.rb @@ -0,0 +1,5 @@ +RSpec.describe "Link Status CSV" do + it_behaves_like "it is forbidden to non-GDS Editors", "/check_homepage_links_status.csv" + it_behaves_like "it is forbidden to non-GDS Editors", "/check_links_status.csv" + it_behaves_like "it is forbidden to non-GDS Editors", "/bad_links_url_status.csv" +end diff --git a/spec/support/authentication.rb b/spec/support/authentication.rb index 2d13b71df..8455f6ad7 100644 --- a/spec/support/authentication.rb +++ b/spec/support/authentication.rb @@ -13,3 +13,79 @@ def login_as_gds_editor login_as(create(:user, permissions: ["GDS Editor"], organisation_slug: "government-digital-service")) end end + +RSpec.shared_examples "redirects non-GDS Editors to services page" do |path| + context "as a GDS Editor" do + before { login_as_gds_editor } + + it "shows the page" do + get path + + expect(response).to have_http_status(:ok) + end + end + + context "as a department user" do + before { login_as_department_user } + + it "does not show the page" do + get path + + expect(response).to redirect_to(services_path) + end + end +end + +RSpec.shared_examples "it is forbidden to non-GDS Editors" do |path| + context "as a GDS Editor" do + before { login_as_gds_editor } + + it "shows the page" do + get path + + expect(response).to have_http_status(:ok) + end + end + + context "as a department user" do + before { login_as_department_user } + + it "does not show the page" do + get path + + expect(response).to have_http_status(:forbidden) + end + end +end + +RSpec.shared_examples "it is forbidden to non-owners" do |path, owning_department| + context "as a GDS Editor" do + before { login_as_gds_editor } + + it "returns 200 OK" do + get path + + expect(response).to have_http_status(:ok) + end + end + + context "as a department user from the owning department" do + before { login_as_department_user(organisation_slug: owning_department) } + + it "returns 200 OK" do + get path + + expect(response).to have_http_status(:ok) + end + end + + context "as a department user" do + before { login_as_department_user } + + it "returns 403 Forbidden" do + get path + + expect(response).to have_http_status(:forbidden) + end + end +end From f1f583f27c58f8c2c2ca0b8d790b55ffcf468046 Mon Sep 17 00:00:00 2001 From: Keith Lawrence Date: Wed, 14 Aug 2024 12:19:26 +0100 Subject: [PATCH 06/12] Only show DELETE button to gds editors - Add system test --- app/views/links/edit.html.erb | 2 +- spec/system/edit_link_page_spec.rb | 28 ++++++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) create mode 100644 spec/system/edit_link_page_spec.rb diff --git a/app/views/links/edit.html.erb b/app/views/links/edit.html.erb index f648fd942..94071c426 100644 --- a/app/views/links/edit.html.erb +++ b/app/views/links/edit.html.erb @@ -32,7 +32,7 @@ <%= render "govuk_publishing_components/components/button", { text: "Update" } %> <% end %> - <% if @link.url %> + <% if @link.url && gds_editor? %>
<%= form_for @link, url: link_path(@local_authority.slug, @service.slug, @interaction.slug), method: "delete", data: { confirm: "Are you sure you wish to delete this link?" } do |f| %>

You can also delete this link if you are certain it is no longer necessary (NB: this cannot be undone)

diff --git a/spec/system/edit_link_page_spec.rb b/spec/system/edit_link_page_spec.rb new file mode 100644 index 000000000..006c7814b --- /dev/null +++ b/spec/system/edit_link_page_spec.rb @@ -0,0 +1,28 @@ +RSpec.describe "Edit link page" do + let(:owning_department) { "department-of-aardvarks" } + let!(:service) { create(:service, label: "Aardvark Wardens", organisation_slugs: [owning_department]) } + let!(:interaction) { create(:interaction, label: "reporting") } + let!(:service_interaction) { create(:service_interaction, service:, interaction:) } + let!(:local_authority) { create(:district_council, slug: "north-midlands") } + let!(:link) { create(:link, local_authority:, service_interaction:) } + + context "as an owning user" do + before { login_as_department_user(organisation_slug: owning_department) } + + it "doesn't show the delete link" do + visit "/local_authorities/north-midlands/services/aardvark-wardens/reporting/edit" + + expect(page).not_to have_button("Delete") + end + end + + context "as a GDS Editor" do + before { login_as_gds_editor } + + it "shows the delete link" do + visit "/local_authorities/north-midlands/services/aardvark-wardens/reporting/edit" + + expect(page).to have_button("Delete") + end + end +end From f7349ad8ef2fed1f2a20ee8fdb38777f58c70cc6 Mon Sep 17 00:00:00 2001 From: Keith Lawrence Date: Mon, 19 Aug 2024 11:37:10 +0100 Subject: [PATCH 07/12] Enforce permissions on councils pages - Only GDS Editors can see local authority list, or use local authority endpoints. - Add request tests Co-authored-by: Ramya Vidapanakal --- .../local_authorities_controller.rb | 3 + spec/requests/bad_homepage_csv_spec.rb | 3 + spec/requests/council_page_spec.rb | 79 +++++++++++++++++++ 3 files changed, 85 insertions(+) create mode 100644 spec/requests/bad_homepage_csv_spec.rb create mode 100644 spec/requests/council_page_spec.rb diff --git a/app/controllers/local_authorities_controller.rb b/app/controllers/local_authorities_controller.rb index 248061034..bb8d96b80 100644 --- a/app/controllers/local_authorities_controller.rb +++ b/app/controllers/local_authorities_controller.rb @@ -4,6 +4,9 @@ class LocalAuthoritiesController < ApplicationController before_action :set_authority, except: %i[index bad_homepage_url_and_status_csv] + before_action :forbid_unless_gds_editor, only: %i[update download_links_csv upload_links_csv bad_homepage_url_and_status_csv] + before_action :redirect_unless_gds_editor, only: %i[index show edit_url download_links_form upload_links_form] + def index Rails.logger.info(params) diff --git a/spec/requests/bad_homepage_csv_spec.rb b/spec/requests/bad_homepage_csv_spec.rb new file mode 100644 index 000000000..25c2e7b02 --- /dev/null +++ b/spec/requests/bad_homepage_csv_spec.rb @@ -0,0 +1,3 @@ +RSpec.describe "Bad homepage CSV" do + it_behaves_like "it is forbidden to non-GDS Editors", "/bad_homepage_url_status.csv" +end diff --git a/spec/requests/council_page_spec.rb b/spec/requests/council_page_spec.rb new file mode 100644 index 000000000..2a02bd3c4 --- /dev/null +++ b/spec/requests/council_page_spec.rb @@ -0,0 +1,79 @@ +RSpec.describe "Council page" do + let!(:local_authority) { create(:district_council, slug: "north-midlands") } + + it_behaves_like "redirects non-GDS Editors to services page", "/local_authorities" + it_behaves_like "redirects non-GDS Editors to services page", "/local_authorities/north-midlands" + it_behaves_like "redirects non-GDS Editors to services page", "/local_authorities/north-midlands/edit_url" + it_behaves_like "redirects non-GDS Editors to services page", "/local_authorities/north-midlands/download_links_form" + it_behaves_like "redirects non-GDS Editors to services page", "/local_authorities/north-midlands/upload_links_form" + + describe "PATCH local_authorities/:local_authority_slug" do + context "as a GDS Editor" do + before { login_as_gds_editor } + + it "does the update and returns 200 OK" do + patch "/local_authorities/north-midlands", params: { homepage_url: "https://www.changed.com" } + + expect(response).to redirect_to("/local_authorities/north-midlands") + expect(local_authority.reload.homepage_url).to eq("https://www.changed.com") + end + end + + context "as a department user" do + before { login_as_department_user } + + it "does not update and returns 403 Forbidden " do + patch "/local_authorities/north-midlands", params: { homepage_url: "https://www.changed.com" } + + expect(response).to have_http_status(:forbidden) + expect(local_authority.reload.homepage_url).to eq("http://www.angus.gov.uk") + end + end + end + + describe "POST local_authorities/:local_authority_slug/download_links_csv" do + context "as a GDS Editor" do + before { login_as_gds_editor } + + it "returns 200 OK" do + post "/local_authorities/north-midlands/download_links_csv", params: { links_status_checkbox: %w[ok] } + + expect(response).to have_http_status(:ok) + end + end + + context "as a department user" do + before { login_as_department_user } + + it "returns 403 Forbidden " do + post "/local_authorities/north-midlands/download_links_csv", params: { links_status_checkbox: %w[ok] } + + expect(response).to have_http_status(:forbidden) + end + end + end + + describe "POST local_authorities/:local_authority_slug/upload_links_csv" do + context "with an empty upload" do + context "as a GDS Editor" do + before { login_as_gds_editor } + + it "returns 302 found" do + post "/local_authorities/north-midlands/upload_links_csv", params: {} + + expect(response).to have_http_status(:found) + end + end + + context "as a department user" do + before { login_as_department_user } + + it "returns 403 Forbidden" do + post "/local_authorities/north-midlands/upload_links_csv", params: {} + + expect(response).to have_http_status(:forbidden) + end + end + end + end +end From 620e5188c18a7e70d2ea9ac6b617a86046bdef4f Mon Sep 17 00:00:00 2001 From: Keith Lawrence Date: Mon, 19 Aug 2024 11:40:11 +0100 Subject: [PATCH 08/12] Enforce permissions on services page - GDS Editors can see list of all services, department users can only see their own services. - All actions on services can only be performed by GDS Editors or users from owning departments. - It's now legit for the services page not to have any services appearing because the current user might not belong to a department that owns any yet, and for GDS Editors a catastrophic loss of data shouldn't be a thing that a user has to detect. - Make the upload services CSV work like the same one in the services controller (redirect to the service on a good import, redirect to the form on a bad one) Co-authored-by: Ramya Vidapanakal 2 --- app/controllers/services_controller.rb | 16 +++- spec/controllers/services_controller_spec.rb | 19 ++--- spec/requests/services_page_spec.rb | 86 ++++++++++++++++++++ spec/system/services_page_spec.rb | 77 ++++++++++++++++++ 4 files changed, 180 insertions(+), 18 deletions(-) create mode 100644 spec/requests/services_page_spec.rb create mode 100644 spec/system/services_page_spec.rb diff --git a/app/controllers/services_controller.rb b/app/controllers/services_controller.rb index af00e9060..3b4880aa9 100644 --- a/app/controllers/services_controller.rb +++ b/app/controllers/services_controller.rb @@ -4,9 +4,10 @@ class ServicesController < ApplicationController before_action :set_service, except: :index + before_action :forbid_unless_permission, except: %i[index] + def index - @services = Service.enabled.order(broken_link_count: :desc) - raise "Missing Data" if @services.empty? + @services = services_for_user(current_user).enabled.order(broken_link_count: :desc) @breadcrumbs = index_breadcrumbs end @@ -34,12 +35,19 @@ def upload_links_form end def upload_links_csv - attempt_import(:service, @service) - redirect_to service_path(@service) + return redirect_to service_path(@service) if attempt_import(:service, @service) + + redirect_to(upload_links_form_service_path(@service)) end private + def services_for_user(user) + return Service.all if gds_editor? + + Service.where(":organisation_slugs = ANY(organisation_slugs)", organisation_slugs: user.organisation_slug) + end + def set_service @service = Service.find_by!(slug: params[:service_slug]) end diff --git a/spec/controllers/services_controller_spec.rb b/spec/controllers/services_controller_spec.rb index 5a1636be7..6ec27547e 100644 --- a/spec/controllers/services_controller_spec.rb +++ b/spec/controllers/services_controller_spec.rb @@ -1,19 +1,10 @@ RSpec.describe ServicesController, type: :controller do describe "GET #index" do - context "when there is missing data" do - it "returns http server error" do - login_as_stub_user - expect { get :index }.to raise_error "Missing Data" - end - end - - context "when there is sufficient data" do - it "returns http succcess" do - login_as_stub_user - create(:service) - get :index - expect(response).to have_http_status(200) - end + it "returns http succcess" do + login_as_stub_user + create(:service) + get :index + expect(response).to have_http_status(200) end end diff --git a/spec/requests/services_page_spec.rb b/spec/requests/services_page_spec.rb new file mode 100644 index 000000000..937370848 --- /dev/null +++ b/spec/requests/services_page_spec.rb @@ -0,0 +1,86 @@ +RSpec.describe "Services page" do + let(:owning_department) { "department-of-aardvarks" } + let!(:service) { create(:service, label: "Aardvark Wardens", organisation_slugs: [owning_department]) } + + describe "GET /services/:service_slug" do + it_behaves_like "it is forbidden to non-owners", "/services/aardvark-wardens", "department-of-aardvarks" + end + + describe "GET /services/:service_slug/download_links_form" do + it_behaves_like "it is forbidden to non-owners", "/services/aardvark-wardens/download_links_form", "department-of-aardvarks" + end + + describe "GET /services/:service_slug/upload_links_form" do + it_behaves_like "it is forbidden to non-owners", "/services/aardvark-wardens/upload_links_form", "department-of-aardvarks" + end + + describe "POST /services/:service_slug/download_links_csv" do + let(:exported_data) { "some_data" } + + before do + allow_any_instance_of(LocalLinksManager::Export::ServiceLinksExporter).to receive(:export_links).and_return(exported_data) + end + + context "as a GDS Editor" do + before { login_as_gds_editor } + + it "returns 200 OK" do + post "/services/aardvark-wardens/download_links_csv" + + expect(response).to have_http_status(:ok) + end + end + + context "as a department user from the owning department" do + before { login_as_department_user(organisation_slug: owning_department) } + + it "returns 200 OK" do + post "/services/aardvark-wardens/download_links_csv" + + expect(response).to have_http_status(:ok) + end + end + + context "as a department user" do + before { login_as_department_user } + + it "returns 403 Forbidden" do + post "/services/aardvark-wardens/download_links_csv" + + expect(response).to have_http_status(:forbidden) + end + end + end + + describe "POST /services/:service_slug/upload_links_csv" do + context "as a GDS Editor" do + before { login_as_gds_editor } + + it "returns 302 Found" do + post "/services/aardvark-wardens/upload_links_csv" + + expect(response).to have_http_status(:found) + end + end + + context "as a department user from the owning department" do + before { login_as_department_user(organisation_slug: owning_department) } + + it "returns 302 Found" do + post "/services/aardvark-wardens/upload_links_csv" + + expect(response).to have_http_status(:found) + end + end + + context "as a department user" do + before { login_as_department_user } + + it "returns 403 Forbidden" do + post "/services/aardvark-wardens/upload_links_csv" + + expect(response).to have_http_status(:forbidden) + end + end + end +end diff --git a/spec/system/services_page_spec.rb b/spec/system/services_page_spec.rb new file mode 100644 index 000000000..4db6b89a2 --- /dev/null +++ b/spec/system/services_page_spec.rb @@ -0,0 +1,77 @@ +require "gds_api/test_helpers/organisations" + +RSpec.describe "Services page" do + include GdsApi::TestHelpers::Organisations + + before do + create(:service, label: "Aardvark Wardens", organisation_slugs: %w[department-of-aardvarks]) + end + + context "as a gds editor" do + before { login_as_gds_editor } + + it "shows all the services" do + visit "/services" + + expect(page).to have_content("Aardvark Wardens") + end + + it "shows a generic title" do + visit "/services" + + expect(page).to have_content("Services (1)") + end + end + + context "as a department user" do + before do + stub_organisations_api_has_organisations_with_bodies([{ "title" => "Department of Randomness", "details" => { "slug" => "random-department" } }]) + login_as_department_user + end + + it "does not show Aardvark Wardens service" do + visit "/services" + + expect(page).not_to have_content("Aardvark Wardens") + end + + it "shows a department title" do + visit "/services" + + expect(page).to have_content("Services for Department of Randomness (0)") + end + + context "if the Organisations API fails" do + before do + stub_request(:get, "#{Plek.new.website_root}/api/organisations/random-department").to_raise(GdsApi::HTTPUnavailable) + end + + it "shows a generic title" do + visit "/services" + + expect(page).to have_content("Services for random-department (0)") + end + end + end + + context "as a user from an owning department" do + before do + stub_organisations_api_has_organisations_with_bodies([{ "title" => "Department of Aardvarks", "details" => { "slug" => "department-of-aardvarks" } }]) + login_as_department_user(organisation_slug: "department-of-aardvarks") + end + + before { login_as_department_user(organisation_slug: "department-of-aardvarks") } + + it "shows the related services only" do + visit "/services" + + expect(page).to have_content("Aardvark Wardens") + end + + it "shows a department title" do + visit "/services" + + expect(page).to have_content("Services for Department of Aardvarks (1)") + end + end +end From 4ca1c64eb48b1c768dce6b4c0096b131781f467e Mon Sep 17 00:00:00 2001 From: Keith Lawrence Date: Wed, 14 Aug 2024 14:03:38 +0100 Subject: [PATCH 09/12] Show owning department name in services list Co-authored-by: Ramya Vidapanakal --- app/controllers/concerns/service_permissions.rb | 6 ++++++ app/controllers/services_controller.rb | 2 ++ app/views/services/index.html.erb | 5 +++-- 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/app/controllers/concerns/service_permissions.rb b/app/controllers/concerns/service_permissions.rb index 334324a3f..e6f734b68 100644 --- a/app/controllers/concerns/service_permissions.rb +++ b/app/controllers/concerns/service_permissions.rb @@ -11,6 +11,12 @@ def permission_for_service?(service) gds_editor? || service_owner?(service) end + def org_name_for_current_user + GdsApi.organisations.organisation(current_user.organisation_slug).to_hash["title"] + rescue GdsApi::HTTPUnavailable + current_user.organisation_slug + end + def redirect_unless_gds_editor redirect_to services_path unless gds_editor? end diff --git a/app/controllers/services_controller.rb b/app/controllers/services_controller.rb index 3b4880aa9..47553e3a5 100644 --- a/app/controllers/services_controller.rb +++ b/app/controllers/services_controller.rb @@ -6,6 +6,8 @@ class ServicesController < ApplicationController before_action :forbid_unless_permission, except: %i[index] + helper_method :org_name_for_current_user + def index @services = services_for_user(current_user).enabled.order(broken_link_count: :desc) diff --git a/app/views/services/index.html.erb b/app/views/services/index.html.erb index 5fb4181d6..74aca3c8d 100644 --- a/app/views/services/index.html.erb +++ b/app/views/services/index.html.erb @@ -1,9 +1,10 @@ -<% content_for :page_title, "Services" %> +<% title = gds_editor? ? "Services" : "Services for #{org_name_for_current_user}" %> +<% content_for :page_title, title %> <% breadcrumb :services %> <%= render "govuk_publishing_components/components/heading", { - text: "Services (#{@services.count})", + text: "#{title} (#{@services.count})", heading_level: 1, font_size: "l", margin_bottom: 5, From 6cfe14f51f3b8698be5d638502f070c224554f0f Mon Sep 17 00:00:00 2001 From: Keith Lawrence Date: Mon, 19 Aug 2024 11:42:38 +0100 Subject: [PATCH 10/12] Add update_owners form - Only GDS Editors can see this for the moment Co-authored-by: Ramya Vidapanakal --- app/controllers/services_controller.rb | 13 +++- app/views/services/show.html.erb | 13 ++-- app/views/services/update_owner_form.html.erb | 23 +++++++ config/routes.rb | 2 + spec/requests/services_page_spec.rb | 62 +++++++++++++++++++ spec/system/service_page_spec.rb | 59 ++++++++++++++++++ 6 files changed, 166 insertions(+), 6 deletions(-) create mode 100644 app/views/services/update_owner_form.html.erb create mode 100644 spec/system/service_page_spec.rb diff --git a/app/controllers/services_controller.rb b/app/controllers/services_controller.rb index 47553e3a5..0bd3270cb 100644 --- a/app/controllers/services_controller.rb +++ b/app/controllers/services_controller.rb @@ -4,7 +4,8 @@ class ServicesController < ApplicationController before_action :set_service, except: :index - before_action :forbid_unless_permission, except: %i[index] + before_action :forbid_unless_permission, only: %i[show download_links_form download_links_csv upload_links_form upload_links_csv] + before_action :forbid_unless_gds_editor, only: %i[update_owner_form update_owner] helper_method :org_name_for_current_user @@ -21,6 +22,16 @@ def show @breadcrumbs = service_breadcrumbs(@service) end + def update_owner_form + @breadcrumbs = service_breadcrumbs(@service) + [{ title: "Update Owner", url: update_owner_form_service_path(@service) }] + end + + def update_owner + @service.update!(organisation_slugs: params["service"]["organisation_slugs"].split(" ")) + + redirect_to service_path(@service, filter: "broken_links") + end + def download_links_form @breadcrumbs = service_breadcrumbs(@service) + [{ title: "Download Links", url: download_links_form_service_path(@service) }] end diff --git a/app/views/services/show.html.erb b/app/views/services/show.html.erb index 725c9fd52..fcdfc6aa8 100644 --- a/app/views/services/show.html.erb +++ b/app/views/services/show.html.erb @@ -15,12 +15,15 @@