feat: prevent wildcard deletion in deleteUser/deleteRole APIs (#519)

Author: hsluoyz

src/enforcer.ts

@@ -165,6 +165,12 @@ export class Enforcer extends ManagementEnforcer {
    * @return succeeds or not.
    */
   public async deleteRoleForUser(user: string, role: string, domain?: string): Promise<boolean> {
+    if (!user) {
+      throw new Error('user must not be empty');
+    }
+    if (!role) {
+      throw new Error('role must not be empty');
+    }
     if (domain === undefined) {
       return this.removeGroupingPolicy(user, role);
     } else {
@@ -181,6 +187,9 @@ export class Enforcer extends ManagementEnforcer {
    * @return succeeds or not.
    */
   public async deleteRolesForUser(user: string, domain?: string): Promise<boolean> {
+    if (!user) {
+      throw new Error('user must not be empty');
+    }
     if (domain === undefined) {
       const subIndex = this.getFieldIndex('p', FieldIndex.Subject);
       return this.removeFilteredGroupingPolicy(subIndex, user);
@@ -197,6 +206,9 @@ export class Enforcer extends ManagementEnforcer {
    * @return succeeds or not.
    */
   public async deleteUser(user: string): Promise<boolean> {
+    if (!user) {
+      throw new Error('user must not be empty');
+    }
     const subIndex = this.getFieldIndex('p', FieldIndex.Subject);
     const res1 = await this.removeFilteredGroupingPolicy(subIndex, user);
     const res2 = await this.removeFilteredPolicy(subIndex, user);
@@ -211,6 +223,9 @@ export class Enforcer extends ManagementEnforcer {
    * @return succeeds or not.
    */
   public async deleteRole(role: string): Promise<boolean> {
+    if (!role) {
+      throw new Error('role must not be empty');
+    }
     const subIndex = this.getFieldIndex('p', FieldIndex.Subject);
     const res1 = await this.removeFilteredGroupingPolicy(subIndex, role);
     const res2 = await this.removeFilteredPolicy(subIndex, role);
@@ -225,6 +240,9 @@ export class Enforcer extends ManagementEnforcer {
    * @return succeeds or not.
    */
   public async deletePermission(...permission: string[]): Promise<boolean> {
+    if (permission.length === 0) {
+      throw new Error('permission must not be empty');
+    }
     return this.removeFilteredPolicy(1, ...permission);
   }
 
@@ -250,6 +268,9 @@ export class Enforcer extends ManagementEnforcer {
    * @return succeeds or not.
    */
   public async deletePermissionForUser(user: string, ...permission: string[]): Promise<boolean> {
+    if (!user) {
+      throw new Error('user must not be empty');
+    }
     permission.unshift(user);
     return this.removePolicy(...permission);
   }
@@ -262,6 +283,9 @@ export class Enforcer extends ManagementEnforcer {
    * @return succeeds or not.
    */
   public async deletePermissionsForUser(user: string): Promise<boolean> {
+    if (!user) {
+      throw new Error('user must not be empty');
+    }
     const subIndex = this.getFieldIndex('p', FieldIndex.Subject);
     return this.removeFilteredPolicy(subIndex, user);
   }

test/rbacAPI.test.ts

@@ -219,3 +219,81 @@ test('test rbac with multiple policy definitions', async () => {
     ['admin', 'create'],
   ]);
 });
+
+test('test deleteUser with empty string should throw error', async () => {
+  const e = await newEnforcer('examples/rbac_model.conf', 'examples/rbac_with_hierarchy_policy.csv');
+
+  // Store initial state
+  const initialGPolicies = await e.getGroupingPolicy();
+  const initialPPolicies = await e.getPolicy();
+  expect(initialGPolicies.length).toBeGreaterThan(0);
+  expect(initialPPolicies.length).toBeGreaterThan(0);
+
+  // Attempt to delete with empty string should throw
+  await expect(e.deleteUser('')).rejects.toThrow('user must not be empty');
+
+  // Verify nothing was deleted
+  expect(await e.getGroupingPolicy()).toEqual(initialGPolicies);
+  expect(await e.getPolicy()).toEqual(initialPPolicies);
+});
+
+test('test deleteRole with empty string should throw error', async () => {
+  const e = await newEnforcer('examples/rbac_model.conf', 'examples/rbac_with_hierarchy_policy.csv');
+
+  const initialGPolicies = await e.getGroupingPolicy();
+  const initialPPolicies = await e.getPolicy();
+  expect(initialGPolicies.length).toBeGreaterThan(0);
+  expect(initialPPolicies.length).toBeGreaterThan(0);
+
+  await expect(e.deleteRole('')).rejects.toThrow('role must not be empty');
+
+  expect(await e.getGroupingPolicy()).toEqual(initialGPolicies);
+  expect(await e.getPolicy()).toEqual(initialPPolicies);
+});
+
+test('test deletePermissionsForUser with empty string should throw error', async () => {
+  const e = await newEnforcer('examples/rbac_model.conf', 'examples/rbac_with_hierarchy_policy.csv');
+
+  const initialPPolicies = await e.getPolicy();
+  expect(initialPPolicies.length).toBeGreaterThan(0);
+
+  await expect(e.deletePermissionsForUser('')).rejects.toThrow('user must not be empty');
+
+  expect(await e.getPolicy()).toEqual(initialPPolicies);
+});
+
+test('test deleteRolesForUser with empty string should throw error', async () => {
+  const e = await newEnforcer('examples/rbac_model.conf', 'examples/rbac_with_hierarchy_policy.csv');
+
+  const initialGPolicies = await e.getGroupingPolicy();
+  expect(initialGPolicies.length).toBeGreaterThan(0);
+
+  await expect(e.deleteRolesForUser('')).rejects.toThrow('user must not be empty');
+
+  expect(await e.getGroupingPolicy()).toEqual(initialGPolicies);
+});
+
+test('test deleteRoleForUser with empty strings should throw error', async () => {
+  const e = await newEnforcer('examples/rbac_model.conf', 'examples/rbac_with_hierarchy_policy.csv');
+
+  await expect(e.deleteRoleForUser('', 'admin')).rejects.toThrow('user must not be empty');
+  await expect(e.deleteRoleForUser('alice', '')).rejects.toThrow('role must not be empty');
+  await expect(e.deleteRoleForUser('', '')).rejects.toThrow('user must not be empty');
+});
+
+test('test deletePermissionForUser with empty string should throw error', async () => {
+  const e = await newEnforcer('examples/rbac_model.conf', 'examples/rbac_with_hierarchy_policy.csv');
+
+  await expect(e.deletePermissionForUser('', 'data1', 'read')).rejects.toThrow('user must not be empty');
+});
+
+test('test deletePermission with empty array should throw error', async () => {
+  const e = await newEnforcer('examples/rbac_model.conf', 'examples/rbac_with_hierarchy_policy.csv');
+
+  const initialPPolicies = await e.getPolicy();
+  expect(initialPPolicies.length).toBeGreaterThan(0);
+
+  await expect(e.deletePermission()).rejects.toThrow('permission must not be empty');
+
+  expect(await e.getPolicy()).toEqual(initialPPolicies);
+});