Skip to content

CFM-496_REST_API_v2_DELETE_CO_Group_returns_400 #360

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

Ioannis
Copy link
Contributor

@Ioannis Ioannis commented Dec 18, 2025

  • Fix groups read-only delete.
  • Fix ApiUser Key title calculation bug.

@Ioannis Ioannis requested a review from skoranda December 18, 2025 18:29
Comment on lines +255 to +261
// Align with REST API: do not allow delete of read-only records
if (method_exists($obj, "isReadOnly") && $obj->isReadOnly()) {
$this->Flash->error(__d('error', 'edit.readonly'));
// Redirect to view, as we do for read-only edits
return $this->redirect(['action' => 'view', $obj->id]);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

How does one trigger the delete of a read only record? The UI shouldn't offer that option in the first place.

Copy link
Contributor Author

@Ioannis Ioannis Jan 13, 2026

Choose a reason for hiding this comment

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

@benno Previously the delete logic ran before we checked isReadOnly, so a crafted DELETE or POST could actually delete a read‑only record even though the API then returned an error.
The UI already hides the delete button for read‑only entities, so normal users could not trigger this through the interface.
The issue only appeared when someone called the delete endpoint directly. The change moves the read‑only check ahead of the delete call, so those crafted requests are rejected before any data is removed.
In short, the UI blocks delete for read‑only records, and this change closes the remaining gap for manually crafted delete requests.

@Ioannis Ioannis force-pushed the CFM-496_REST_API_v2_DELETE_CO_Group_returns_400 branch from 0f9899d to 386d9de Compare January 12, 2026 13:40
@Ioannis Ioannis requested a review from benno January 13, 2026 10:35
@Ioannis
Copy link
Contributor Author

Ioannis commented Jan 13, 2026

@benno I’ve updated the PR based on your feedback; please continue the review when convenient. Thanks.

Sign in to join this conversation on GitHub.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants