From 51c4e72d024bf276221a9bd7b0a3e3da0e05891a Mon Sep 17 00:00:00 2001 From: ChrisBAshton Date: Wed, 20 May 2026 10:43:25 +0100 Subject: [PATCH] Raise validation error if badly access-limiting draft on-create Prior to this commit, it was possible to: 1. Start drafting a new document 2. Change the lead organisation to an org other than the one the user is in 3. Check the "Limit access" checkbox 4. Click "Save" This would result in the publisher seeing a "You do not have permission to access this page" 403 response. They'd have to hit the 'back' button in the browser to return to their form and correct their inputs. In my local testing, getting into this state did not create the document, but Zendesk ticket 6597730 implies that a document _did_ get created when a user did this in the wild. As a GDS Admin, the bug is even stranger, leading to a Server Error (https://govuk.sentry.io/issues/7494056061/?project=202259&query=is%3Aunresolved&referrer=issue-stream): > No route matches {:action=>"edit", :controller=>"admin/edition_access_limited", :id=>nil}, missing required keys: [:id] (ActionView::Template::Error) This was because of a recent change to the forbidden.html.erb template, whereby GDS Admins are linked through to the `edit_access_limited_admin_edition_path` of the Edition. That path requires an edition ID, but there is no ID if the Edition failed to persist, hence we saw a runtime error instead. And fixing that error led to another one: ``` undefined method `id' for nil (NoMethodError) ./app/services/draft_edition_updater.rb:31:in `access_limit_excludes_current_user?' ``` ^ This one was because by default, any user in Cucumber has not been configured to have an Organisation, so the `access_limit_excludes_current_user?` check would raise an exception when trying to query the user's organisations. We know from [WHIT-3179](https://gov-uk.atlassian.net/browse/WHIT-3179) that it _is_ possible for some users to lack an Organisation, so we do need to account for that too rather than just patch up the fixture data. The fix in this commit is to: 1. Only enforce the 'create' permission on Editions if the Edition is persisted 2. Ensure that creation failures are passed through to the view via the `flash.now[:alert]` mechanism 3. "Fail closed" on the access limiting check, for any user who is not part of an organisation 4. Only attempt to render the helper link for GDS Admins if the edition is persisted. Jira: https://gov-uk.atlassian.net/browse/WHIT-3458 --- app/controllers/admin/editions_controller.rb | 3 ++- app/services/draft_edition_updater.rb | 3 +++ app/views/admin/errors/forbidden.html.erb | 2 +- features/admin-limit-access-editions.feature | 8 ++++++ .../admin_limit_access_steps.rb | 27 +++++++++++++++++++ 5 files changed, 41 insertions(+), 2 deletions(-) diff --git a/app/controllers/admin/editions_controller.rb b/app/controllers/admin/editions_controller.rb index 0eab4e8a34b..ecd16de83b8 100644 --- a/app/controllers/admin/editions_controller.rb +++ b/app/controllers/admin/editions_controller.rb @@ -32,7 +32,7 @@ def enforce_permissions! when "choose_type", "change_type", "change_type_preview", "apply_change_type" enforce_permission!(:create, edition_class || Edition) when "create" - enforce_permission!(:create, @edition) + enforce_permission!(:create, @edition) if @edition.persisted? when "edit", "update", "revise", "diff", "update_bypass_id", "features" enforce_permission!(:update, @edition) when "destroy", "confirm_destroy" @@ -95,6 +95,7 @@ def create updater.perform! redirect_to show_or_edit_path, saved_confirmation_notice else + flash.now[:alert] = updater.failure_reason build_edition_dependencies render :new end diff --git a/app/services/draft_edition_updater.rb b/app/services/draft_edition_updater.rb index 04011ecceb6..210d1567cd7 100644 --- a/app/services/draft_edition_updater.rb +++ b/app/services/draft_edition_updater.rb @@ -28,6 +28,9 @@ def should_check_current_user_will_retain_access? end def access_limit_excludes_current_user? + # Some users may not necessarily belong to an organisation. We should fail-closed for them. + return true unless @options[:current_user].organisation + edition.organisation_association_enabled? && edition.edition_organisations.map(&:organisation_id).exclude?(@options[:current_user].organisation.id) end end diff --git a/app/views/admin/errors/forbidden.html.erb b/app/views/admin/errors/forbidden.html.erb index bdf47a30494..742899b2f63 100644 --- a/app/views/admin/errors/forbidden.html.erb +++ b/app/views/admin/errors/forbidden.html.erb @@ -5,7 +5,7 @@

You do not have permission to access this page.

- <% if @edition && can?(:perform_administrative_tasks, Edition) %> + <% if @edition&.persisted? && can?(:perform_administrative_tasks, Edition) %>

As an administrator, you can update the access permissions of the page. diff --git a/features/admin-limit-access-editions.feature b/features/admin-limit-access-editions.feature index 3eb474be4b1..594243629fb 100644 --- a/features/admin-limit-access-editions.feature +++ b/features/admin-limit-access-editions.feature @@ -15,3 +15,11 @@ Feature: Viewing most recent editions in admin And I view the current edition of document "Road accidents" Then I am told I do not have permissions to access this page And I should see a link to edit the access + + Scenario: Attempting to access limit a new (unpersisted) document against a different org + Given I am an editor + When I begin drafting a new document + And I set the Lead organisation to an org I am not in + And I check the "Limit access to publishers from organisations associated with this document before you publish" box + When I click "Save" + Then I should see the validation error "Access can only be limited by users belonging to an organisation tagged to the document" diff --git a/features/step_definitions/admin_limit_access_steps.rb b/features/step_definitions/admin_limit_access_steps.rb index 22c624e9b80..2ead9af659e 100644 --- a/features/step_definitions/admin_limit_access_steps.rb +++ b/features/step_definitions/admin_limit_access_steps.rb @@ -14,3 +14,30 @@ Then(/^I should see a link to edit the access/) do expect(page).to have_link("As an administrator, you can update the access permissions of the page.", href: edit_access_limited_admin_edition_path(@edition)) end + +When(/^I begin drafting a new document$/) do + @org = create(:organisation, name: "Some made up org") + + visit "/government/admin/publications/new" + select "Policy paper", from: "edition_publication_type_id" + fill_in "edition[title]", with: "Test publication" + fill_in "edition[summary]", with: "Test summary" + fill_in "edition[body]", with: "Test body" + check "Applies to all UK nations" +end + +When(/^I set the Lead organisation to an org I am not in$/) do + select @org.name, from: "edition_lead_organisation_ids_1" +end + +When(/^I check the "(.+)" box$/) do |box_name| + check box_name +end + +When(/^I click "(.+)"$/) do |button_name| + click_on button_name +end + +Then(/^I should see the validation error "(.+)"$/) do |string| + expect(page).to have_content string +end