feat(UserGroup): allow updating groups with system scope #822

Author: jaredhendrickson13

pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Models/UserGroup.inc

@@ -9,6 +9,7 @@ use RESTAPI\Fields\IntegerField;
 use RESTAPI\Fields\StringField;
 use RESTAPI\Responses\ForbiddenError;
 use RESTAPI\Responses\NotFoundError;
+use RESTAPI\Responses\UnprocessableContentError;
 use RESTAPI\Responses\ValidationError;
 use RESTAPI\Validators\RegexValidator;
 
@@ -48,9 +49,10 @@ class UserGroup extends Model {
         );
         $this->scope = new StringField(
             default: 'local',
-            choices: ['local', 'remote'],
+            choices: ['local', 'remote', 'system'],
             help_text: 'The scope of this user group. Use `local` for user groups that only apply to this system. use ' .
-                '`remote` for groups that also apply to remote authentication servers.',
+                '`remote` for groups that also apply to remote authentication servers. Please note the `system` scope ' .
+                'is reserved for built-in, system-defined user groups and cannot be assigned manually.',
         );
         $this->member = new ForeignModelField(
             model_name: 'User',
@@ -95,6 +97,26 @@ class UserGroup extends Model {
         return $name;
     }
 
+    /**
+     * Adds additional validation to the `scope` field.
+     * @param string $scope The incoming value to be validated.
+     * @return string The validated value to be assigned.
+     * @throws ValidationError When `scope` is set to `system` and this is a newly created object or the existing
+     * `scope` is not already `system`.
+     */
+    public function validate_scope(string $scope): string {
+        $existing_scope = $this->initial_object->scope->value ?? null;
+
+        if ($scope === 'system' and $scope !== $existing_scope) {
+            throw new UnprocessableContentError(
+                message: 'Field `scope` cannot be set to `system` as it is reserved for system-defined user groups only.',
+                response_id: 'USER_GROUP_SCOPE_CANNOT_BE_SET_TO_SYSTEM',
+            );
+        }
+
+        return $scope;
+    }
+
     /**
      * Adds additional validation to the `priv` field.
      * @param string $priv The incoming value to be validated

pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Responses/UnprocessableContentError.inc

@@ -15,7 +15,7 @@ use RESTAPI\Core\Response;
  */
 class UnprocessableContentError extends Response {
     public $code = 422;
-    public string $help_text = 'The client has requested a resource that requires a dependency which is not installed.';
+    public string $help_text = 'The client has requested a resource that requires a dependency or other requirement that was not met.';
 
     public function __construct(
         $message,

pfSense-pkg-RESTAPI/files/usr/local/pkg/RESTAPI/Tests/APIModelsUserGroupTestCase.inc

@@ -123,4 +123,50 @@ class APIModelsUserGroupTestCase extends TestCase {
             },
         );
     }
+
+    /**
+     * Ensures that UserGroups cannot be created or updated with the 'system' scope unless the existing object also has
+     * the 'system' scope.
+     */
+    public function test_cannot_create_or_update_user_groups_with_system_scope(): void {
+        # Create a UserGroup to test with
+        $user_group = new UserGroup();
+
+        # Ensure attempts to create UserGroups with the system scope are rejected
+        $this->assert_throws_response(
+            response_id: 'USER_GROUP_SCOPE_CANNOT_BE_SET_TO_SYSTEM',
+            code: 422,
+            callable: function () use ($user_group) {
+                $user_group->scope->value = 'system';
+                $user_group->create();
+            },
+        );
+
+        # Ensure attempts to update existing UserGroups to have the system scope are rejected
+        $this->assert_throws_response(
+            response_id: 'USER_GROUP_SCOPE_CANNOT_BE_SET_TO_SYSTEM',
+            code: 422,
+            callable: function () use ($user_group) {
+                # Create a local UserGroup to test with
+                $user_group = new UserGroup(data: ['name' => 'testgroup', 'scope' => 'local']);
+                $user_group->create();
+
+                # Attempt to update the scope to `system`
+                $user_group->scope->value = 'system';
+                $user_group->update();
+            },
+        );
+
+        # Delete the test UserGroup
+        $user_group->delete();
+
+        # Ensure we CAN update an existing UserGroup with the system scope to still have the system scope
+        $this->assert_does_not_throw(
+            callable: function () {
+                # Create a UserGroup with the system scope to test with
+                $admin_group = UserGroup::query(name: 'Admins')->first();
+                $admin_group->validate_scope('system');
+            },
+        );
+    }
 }