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
+30 −16
Open

CFM-496_REST_API_v2_DELETE_CO_Group_returns_400 #360

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
75b3f31
Fix groups readonly delete. Improve error information for priviledged…
Ioannis Dec 18, 2025
386d9de
Fix external identity source title configuration
Ioannis Dec 19, 2025
ec496d4
Revert detailed RecordNotFound errors for CO-privileged API users; re…
Ioannis Jan 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions app/src/Controller/ApiUsersController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,11 @@ public function generate(string $id) {
$this->Flash->error($e->getMessage());
}

[$title, , ] = StringUtilities::entityAndActionToTitle(null,
'api.key',
$this->request->getParam('action'));
[$title, , ] = StringUtilities::entityAndActionToTitle(
null,
null,
'api.key.' . $this->request->getParam('action'),
);
$this->set('vv_title', $title);

// Render the view.
Expand Down
7 changes: 3 additions & 4 deletions app/src/Controller/ApiV2Controller.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,13 +200,12 @@ public function delete($id) {
try {
$obj = $table->findById($id)->firstOrFail();

// XXX document AR-CO-1 when we implement hard delete/changelog
// note similar logic in StandardController
$table->deleteOrFail($obj);

if(method_exists($obj, "isReadOnly") && $obj->isReadOnly()) {
throw new BadRequestException(__d('error', 'edit.readonly'));
}
// XXX document AR-CO-1 when we implement hard delete/changelog
// note similar logic in StandardController
$table->deleteOrFail($obj);

// Trigger provisioning, letting errors bubble up (AR-GMR-5)
if(method_exists($table, "requestProvisioning")) {
Expand Down
12 changes: 8 additions & 4 deletions app/src/Controller/Component/RegistryAuthComponent.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,12 +240,16 @@ public function beforeFilter(EventInterface $event) {
throw new ForbiddenException(__d('error', 'perm'));
}
catch(RecordNotFoundException $e) {
// Requested record does not exist. For platform API users, we can return
// a RecordNotFoundException, otherwise we recast to generate permission denied.
// The requested record does not exist. Because CO IDs are derived from existing
// records (via primary links / CO resolution helpers), we cannot reliably
// determine whether a privileged CO-scoped API user is actually related to
// a record that failed with RecordNotFoundException. To avoid leaking
// cross-CO information, we only allow platform API users to see the original
// exception and return a generic API failure for all other API users.
$this->llog('debug', "User authorization failed: " . $e->getMessage());

$ApiUsers = TableRegistry::getTableLocator()->get('ApiUsers');

if($ApiUsers->getUserPrivilege($this->authenticatedUser) === true) {
throw $e;
} else {
Expand Down
8 changes: 5 additions & 3 deletions app/src/Controller/ExternalIdentitySourcesController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,9 +187,11 @@ public function search(string $id) {

$this->set('vv_search_attrs', $this->ExternalIdentitySources->searchableAttributes((int)$id));

[$title, , ] = StringUtilities::entityAndActionToTitle(null,
$this->getName(),
$this->request->getParam('action'));
[$title, , ] = StringUtilities::entityAndActionToTitle(
null,
null,
$this->getName() . '.search',
);
$this->set('vv_title', $title);
}

Expand Down
7 changes: 7 additions & 0 deletions app/src/Controller/StandardController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,13 @@ public function delete($id) {
try {
$obj = $table->findById($id)->firstOrFail();

// 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]);
}

Comment on lines +255 to +261
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 •
edited
Loading

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.

// By default, a delete is a soft delete. The exceptions is when
// deleting a CO (AR-CO-1). In v4, we permitted a controller level
// flag to be set, but the only controller this really applies to
Expand Down
4 changes: 2 additions & 2 deletions app/src/Model/Table/ApiUsersTable.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,11 +177,11 @@ public function generateKey(int $id) {
*
* @since COmanage Registry v5.0.0
* @param string $username API Username
* @return mixed true if $username is a platform API user, an integer (the CO ID) if the user is a privileged API user within that CO, or false otherwise
* @return bool|int true if $username is a platform API user, an integer (the CO ID) if the user is a privileged API user within that CO, or false otherwise
* @throws InvalidArgumentException
*/

public function getUserPrivilege(string $username) {
public function getUserPrivilege(string $username): bool|int {
$apiUser = $this->find()->where(['username' => $username])->contain('Cos')->first();

if(empty($apiUser)) {
Expand Down