Merge pull request #29256 from artfulrobot/standalone-issue-4862

standalone: fix ACLs on Roles re core #4862

ext/standaloneusers/CRM/Standaloneusers/BAO/Role.php

@@ -20,16 +20,38 @@ public static function self_hook_civicrm_post(\Civi\Core\Event\PostEvent $event)
   /**
    * Check access permission
    *
+   * Note that $e->getRecord() returns the data passed INTO the API, e.g. if it
+   * has a 'name' key, then the value is that passed into the API (e.g. for an
+   * update action), not the current value.
+   *
    * @param \Civi\Api4\Event\AuthorizeRecordEvent $e
    * @see \Civi\Api4\Utils\CoreUtil::checkAccessRecord
    */
   public static function self_civi_api4_authorizeRecord(AuthorizeRecordEvent $e): void {
     $record = $e->getRecord();
     $action = $e->getActionName();
-    // Prevent users from updating or deleting the admin and everyone roles
-    if (in_array($action, ['delete', 'update'], TRUE)) {
-      $name = $record['name'] ?? CRM_Core_DAO::getFieldValue(self::class, $record['id']);
-      if (in_array($name, ['admin', 'everyone'], TRUE)) {
+    if (!in_array($action, ['delete', 'update'], TRUE)) {
+      // We only care about these actions.
+      return;
+    }
+
+    // Load the role name from the record that is to be updated/deleted.
+    $storedRoleName = CRM_Core_DAO::getFieldValue(self::class, $record['id'], 'name');
+
+    // Protect the admin role: it must have access to everything.
+    if ($storedRoleName === 'admin') {
+      $e->setAuthorized(FALSE);
+      return;
+    }
+
+    // Protect the everyone role
+    if ($storedRoleName === 'everyone') {
+      if ($action === 'delete') {
+        // Do not allow deletion of the everyone role.
+        $e->setAuthorized(FALSE);
+      }
+      // Updates: Disallow changing name and is_active
+      if (array_intersect(['name', 'is_active'], array_keys($record))) {
         $e->setAuthorized(FALSE);
       }
     }

ext/standaloneusers/tests/phpunit/Civi/Api4/Action/UserTest.php

@@ -758,4 +758,51 @@ public function testOtherApiAccess() {
     }
   }
 
+  public function testEveryoneRoleProtections() {
+    $this->loginUser($this->adminUserID);
+    $this->assertRoleCannotBeDeleted('everyone');
+    $this->assertRoleUpdateFails('everyone', ['name' => 'everyone']);
+    $this->assertRoleUpdateFails('everyone', ['is_active' => FALSE]);
+    // Check we can change permissions
+    $user = Role::update(TRUE)
+      ->addWhere('name', '=', 'everyone')
+      ->setValues([
+        'permissions' => ['access CiviMail subscribe/unsubscribe pages'],
+        'label' => 'Y’all',
+      ])
+      ->execute();
+  }
+
+  public function testAdminRoleProtections() {
+    $this->loginUser($this->adminUserID);
+    $this->assertRoleCannotBeDeleted('admin');
+    $this->assertRoleUpdateFails('admin', ['name' => 'admin']);
+    $this->assertRoleUpdateFails('admin', ['is_active' => FALSE]);
+  }
+
+  protected function assertRoleCannotBeDeleted($roleName) {
+    try {
+      $user = Role::delete(TRUE)
+        ->addWhere('name', '=', $roleName)
+        ->execute();
+      $this->fail("We were able to delete the '$roleName' role and we should not be allowed to.");
+    }
+    catch (\Civi\API\Exception\UnauthorizedException $e) {
+      $this->assertEquals('ACL check failed', $e->getMessage());
+    }
+  }
+
+  protected function assertRoleUpdateFails($roleName, array $updates) {
+    try {
+      $user = Role::update(TRUE)
+        ->addWhere('name', '=', $roleName)
+        ->setValues($updates)
+        ->execute();
+      $this->fail("We were able to update the '$roleName' role and we should not be allowed to.");
+    }
+    catch (\Civi\API\Exception\UnauthorizedException $e) {
+      $this->assertEquals('ACL check failed', $e->getMessage());
+    }
+  }
+
 }