Block supporting org editors from locking themselves out [WHIT - 3458]

[GDSNewt]

Fixes a bug where users could unintentionally remove their own organisation’s access to a draft without receiving a warning, due to how empty multi-select fields are handled in HTML forms.

---

Problem

While working on support, I encountered the following scenario:

1. An Org B user creates a draft publication with:

   • Org A as the lead organisation
   • Org B as a supporting organisation

2. The user later edits the draft and:

   • Enables "Limit access"
   • Changes the lead organisation to Org A only
   • Removes Org B from supporting organisations

3. Due to HTML behavior:

   • <select multiple> fields submit nothing when empty
   • supporting_organisation_ids is missing from the payload entirely

---

Root Cause

• In the controller:

  • delete_absent_edition_organisations detects the missing key
  • It skips processing
  • Org B remains in the in-memory edition_organisations

• In DraftEditionUpdater:

  • The "will this user retain access?" check runs against this in-memory state
  • Org B is still present, so access is assumed to be retained

• The save is allowed:

  • access_limited = true
  • Only Org A is persisted as the lead organisation

---

Result

• The Org B user loses access
• No warning is shown during the edit process

---

Fix

1. Handle missing multi-select params explicitly

   • Treat absence of supporting_organisation_ids as "clear all values", not "no change"

2. Wrap update flow in a transaction

   • Encloses assign_attributes and save
   • Ensures that if can_perform? returns false, all changes are rolled back, including side effects from attribute assignment

JIRA

[TonyGDS]

It's not clear reading the code why we are doing this. We could create a separate method to describe it e.g.

  if editions_form_cleared_supporting_orgs?
    edition_params[:supporting_organisation_ids] = []
  elsif edition_params[:supporting_organisation_ids]
    edition_params[:supporting_organisation_ids] = edition_params[:supporting_organisation_ids].reject(&:blank?)
  end

  def editions_form_cleared_supporting_orgs?
    edition_params.key?(:lead_organisation_ids) &&
      !edition_params.key?(:supporting_organisation_ids)
  end

[ChrisBAshton]

Can we also add a corresponding test?

[TonyGDS]

DraftEditionUpdater has a method access_limit_excludes_current_user? could we not do something similar here with the input params rather than wrapping the action in a transaction? Reading the code it's unclear why we need to do it. If we call access_limit_excludes_current_user? before @edition.assign_attributes(edition_params) we wouldn't have to roll anything back.

[ChrisBAshton]

Closing this PR. It reads like an AI guess that hasn't been validated:

1. Following the steps outlined in the PR description doesn't reproduce the bug
2. The explanation doesn't make sense either. If supporting_organisation_ids is missing from the payload, it would be skipped over in the delete_absent_edition_organisations method - so wouldn't actually be removed from the Edition. As it happens, supporting_organisation_ids is included in the payload when no orgs are selected (it's sent as an empty array).

In addition:

3. Rolling back isn't a safe mechanism as far as draft updating goes, because we may already have made calls to Publishing API at the point we roll back the Whitehall changes. We need to try to prevent the bad change from happening in the first place.

[ChrisBAshton]

UPDATE:

This PR was superseded by #11476. But through pairing with Alex, we've found separate bugs that affect post-document-creation access limiting:

First:

1. Create access-limited Publication draft
2. Attempt to change the org. Save. See validation error.
3. Refresh the page (or take any action) - already locked out. Permissions error.

Second:

1. Create access-limited Publication draft, setting current org as the supporting org, and Lead Org as a different org.
2. Attempt to remove the supporting org. Save. See validation error.
3. Refresh the page (or take any action) - already locked out. Permissions error.

Same root cause: dirty data on the 'update' path. We're trying to assess whether a user will be locked out based on edition attributes some of which are from the DB and some of which are overwritten in memory by form inputs. We're having a wider think about what to do with this.