Skip to content

Raise validation error if badly access-limiting draft on-create [WHIT-3458]#11476

Merged
ChrisBAshton merged 1 commit into
mainfrom
fix-access-limiting-bug
May 21, 2026
Merged

Raise validation error if badly access-limiting draft on-create [WHIT-3458]#11476
ChrisBAshton merged 1 commit into
mainfrom
fix-access-limiting-bug

Conversation

@ChrisBAshton
Copy link
Copy Markdown
Contributor

@ChrisBAshton ChrisBAshton commented May 20, 2026

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

Screenshots

Screenshot 2026-05-20 at 10 42 41

⚠️ This repo is Continuously Deployed: make sure you follow the guidance ⚠️

This application is owned by the Whitehall Experience team. Please let us know in #govuk-whitehall-experience-tech when you raise any PRs.

Follow these steps if you are doing a Rails upgrade.

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
@ChrisBAshton ChrisBAshton marked this pull request as ready for review May 20, 2026 09:58
@ChrisBAshton ChrisBAshton changed the title Raise validation error if badly access-limiting draft on-create Raise validation error if badly access-limiting draft on-create [WHIT-3458] May 20, 2026
Copy link
Copy Markdown
Contributor

@GDSNewt GDSNewt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I think if we do end up consolidating the form in one place we may wish to do some renaming/commenting in the tests. At the mo it's a bit confusing to think of where the user is in the flow.

@ChrisBAshton ChrisBAshton merged commit d60aea6 into main May 21, 2026
26 checks passed
@ChrisBAshton ChrisBAshton deleted the fix-access-limiting-bug branch May 21, 2026 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants