Skip to content
Jump to bottom

CFM-496_REST_API_v2_DELETE_CO_Group_returns_400 #360

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
+31 −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
2 commits
Select commit Hold shift + click to select a range
3459c4c
Fix groups readonly delete. Improve error information for priviledged…
Ioannis Dec 18, 2025
0f9899d
Fix external identity source title configuration
Ioannis Dec 19, 2025
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 @@ -193,13 +193,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
13 changes: 9 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 @@ -245,12 +245,17 @@ public function beforeFilter(EventInterface $event) {
$this->llog('debug', "User authorization failed: " . $e->getMessage());

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

if($ApiUsers->getUserPrivilege($this->authenticatedUser) === true) {

$priv = $ApiUsers->getUserPrivilege($this->authenticatedUser);
$apiUserCoId = $this->cache['api_user']['co_id'] ?? null;

if ($priv === true || ($apiUserCoId !== null && $priv === $apiUserCoId)) {
Comment on lines +249 to +252
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not immediately obvious to me that a privileged API user should see error messages, especially on a multitenant instance where they may be trying to manipulate a record that isn't within their own CO.

// Platform API user, or privileged API user for this CO:
// let the RecordNotFoundException bubble up.
throw $e;
} else {
throw new UnauthorizedException(__d('error', 'auth.api.failed'));
}

throw new UnauthorizedException(__d('error', 'auth.api.failed'));
}
catch(\Exception $e) {
$this->llog('debug', $e->getMessage());
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.

// 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 @@ -175,11 +175,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