fix(rbac): lowercase user and group entity references (backstage#1937)

* Introduce LowercaseFileAdapter Signed-off-by: Dominika Zemanovicova <dzemanov@redhat.com> * Introduce transformPolicyGroupToLowercase Signed-off-by: Dominika Zemanovicova <dzemanov@redhat.com> * Introduce transformRolesGroupToLowercase Signed-off-by: Dominika Zemanovicova <dzemanov@redhat.com> * Convert uppercase groups to lowecase in csv watcher Signed-off-by: Dominika Zemanovicova <dzemanov@redhat.com> * Convert uppercase groups to lowercase in providers Signed-off-by: Dominika Zemanovicova <dzemanov@redhat.com> * Add v0 migration of users groups to lowercase Signed-off-by: Dominika Zemanovicova <dzemanov@redhat.com> * Update lowercase-file-adapter reference Signed-off-by: Dominika Zemanovicova <dzemanov@redhat.com> * Fix prettier Signed-off-by: Dominika Zemanovicova <dzemanov@redhat.com> * Introduce lowercasing memberReferences in role endpoints Signed-off-by: Dominika Zemanovicova <dzemanov@redhat.com> * Lowercase admins entityRef from config Signed-off-by: Dominika Zemanovicova <dzemanov@redhat.com> * Fix uppercase role Signed-off-by: Dominika Zemanovicova <dzemanov@redhat.com> * Fix delete of role user Signed-off-by: Dominika Zemanovicova <dzemanov@redhat.com> * Catch invalid calls to enforcer delegate Signed-off-by: Dominika Zemanovicova <dzemanov@redhat.com> * Fix return type of mock implementation Signed-off-by: Dominika Zemanovicova <dzemanov@redhat.com> * Add newline to duplicate-policy.csv Signed-off-by: Dominika Zemanovicova <dzemanov@redhat.com> * Add changeset Signed-off-by: Dominika Zemanovicova <dzemanov@redhat.com> * Unify lowecasing Signed-off-by: Dominika Zemanovicova <dzemanov@redhat.com> Co-authored-by: Oleksandr Andriienko <oandriie@redhat.com> Signed-off-by: Dominika Zemanovicova <dzemanov@redhat.com> --------- Signed-off-by: Dominika Zemanovicova <dzemanov@redhat.com> Co-authored-by: Oleksandr Andriienko <oandriie@redhat.com>
gaelgoth · Jan 23, 2025 · 46ef321 · 46ef321
1 parent 1c10d3a
commit 46ef321
Show file tree

Hide file tree

Showing 16 changed files with 473 additions and 89 deletions.
diff --git a/workspaces/rbac/.changeset/new-moose-nail.md b/workspaces/rbac/.changeset/new-moose-nail.md
@@ -0,0 +1,5 @@
+---
+'@backstage-community/plugin-rbac-backend': minor
+---
+
+Roles and permissions were not correctly applied for users and groups with names containing uppercase letters. To address this issue, we now convert user and group references in all user inputs to lowercase. This change migrates `v0` column in `casbin_rule` table in `backstage_plugin_permission` database. Conditions containing claims with uppercase letters are not resolved yet.
diff --git a/workspaces/rbac/plugins/rbac-backend/__fixtures__/data/invalid-csv/duplicate-policy.csv b/workspaces/rbac/plugins/rbac-backend/__fixtures__/data/invalid-csv/duplicate-policy.csv
@@ -3,10 +3,15 @@ g, user:default/guest, role:default/catalog-deleter
 
 g, user:default/guest, role:default/catalog-updater
 
+g, group:default/READER-GROUP, role:default/CATALOG-USER
+g, group:default/READER-GROUP, role:default/CATALOG-USER
+
 p, role:default/catalog-writer, catalog.entity.create, use, allow
 p, role:default/catalog-writer, catalog.entity.create, use, allow
 
 p, role:default/catalog-writer, catalog-entity, delete, allow
 
 p, role:default/duplication-effect, catalog-entity, update, allow
 p, role:default/duplication-effect, catalog-entity, update, deny
+
+p, role:default/CATALOG-USER, catalog-entity, read, allow
diff --git a/workspaces/rbac/plugins/rbac-backend/__fixtures__/data/valid-csv/rbac-policy.csv b/workspaces/rbac/plugins/rbac-backend/__fixtures__/data/valid-csv/rbac-policy.csv
@@ -8,7 +8,10 @@ p, role:default/legacy, catalog-entity, update, allow
 p, role:default/catalog-writer, catalog-entity, read, allow
 p, role:default/catalog-writer, catalog.entity.create, use, allow
 p, role:default/catalog-deleter, catalog-entity, delete, deny
+p, role:default/CATALOG-USER, catalog-entity, read, allow
 
 p, role:default/known_role, test.resource.deny, use, allow
 
 g, user:default/known_user, role:default/known_role
+g, user:default/TOM, role:default/CATALOG-USER
+g, group:default/READER-GROUP, role:default/CATALOG-USER
diff --git a/workspaces/rbac/plugins/rbac-backend/__fixtures__/data/valid-csv/uppercase-policy.csv b/workspaces/rbac/plugins/rbac-backend/__fixtures__/data/valid-csv/uppercase-policy.csv
@@ -0,0 +1,7 @@
+p, role:default/CATALOG-USER, catalog-entity, read, allow
+p, role:default/known_role, test.resource.deny, use, allow
+
+g, user:default/known_user, role:default/known_role
+g, user:default/TOM, role:default/CATALOG-USER
+g, group:default/READER-GROUP, role:default/CATALOG-USER
+g, group:default/READER-GROUP, role:default/known_role
diff --git a/workspaces/rbac/plugins/rbac-backend/migrations/20241108093910_migrations.js b/workspaces/rbac/plugins/rbac-backend/migrations/20241108093910_migrations.js
@@ -0,0 +1,35 @@
+/*
+ * Copyright 2024 The Backstage Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+exports.up = async function up(knex) {
+  const casbinExists = await knex.schema.hasTable('casbin_rule');
+  if (casbinExists) {
+    await knex('casbin_rule')
+      .whereNotNull('v0')
+      .where(function groups() {
+        this.where('v0', 'like', 'user:%').orWhere('v0', 'like', 'group:%');
+      })
+      .update({
+        v0: knex.raw('LOWER(??)', ['v0']),
+      });
+  }
+};
+
+/**
+ * @param { import("knex").Knex } knex
+ * @returns { Promise<void> }
+ */
+exports.down = async function down(_knex) {};
diff --git a/workspaces/rbac/plugins/rbac-backend/src/admin-permissions/admin-creation.ts b/workspaces/rbac/plugins/rbac-backend/src/admin-permissions/admin-creation.ts
@@ -63,7 +63,7 @@ export const useAdminsFromConfig = async (
   const newGroupPolicies = new Map<string, string>();
 
   for (const admin of admins) {
-    const entityRef = admin.getString('name');
+    const entityRef = admin.getString('name').toLocaleLowerCase('en-US');
     validateEntityReference(entityRef);
 
     addedGroupPolicies.set(entityRef, ADMIN_ROLE_NAME);

diff --git a/workspaces/rbac/plugins/rbac-backend/src/file-permissions/csv-file-watcher.test.ts b/workspaces/rbac/plugins/rbac-backend/src/file-permissions/csv-file-watcher.test.ts
@@ -147,6 +147,7 @@ const currentPermissionPolicies = [
   ['role:default/catalog-writer', 'catalog-entity', 'read', 'allow'],
   ['role:default/catalog-writer', 'catalog.entity.create', 'use', 'allow'],
   ['role:default/catalog-deleter', 'catalog-entity', 'delete', 'deny'],
+  ['role:default/CATALOG-USER', 'catalog-entity', 'read', 'allow'],
   ['role:default/known_role', 'test.resource.deny', 'use', 'allow'],
 ];
 
@@ -156,6 +157,8 @@ const currentRoles = [
   ['user:default/guest', 'role:default/catalog-reader'],
   ['user:default/guest', 'role:default/catalog-deleter'],
   ['user:default/known_user', 'role:default/known_role'],
+  ['user:default/tom', 'role:default/CATALOG-USER'],
+  ['group:default/reader-group', 'role:default/CATALOG-USER'],
 ];
 
 describe('CSVFileWatcher', () => {
@@ -207,6 +210,27 @@ describe('CSVFileWatcher', () => {
     );
   }
 
+  describe('parse', () => {
+    test('should parse users and groups in lowercase', async () => {
+      csvFileName = resolve(
+        __dirname,
+        '../../__fixtures__/data/valid-csv/uppercase-policy.csv',
+      );
+
+      const csvFileWatcher = createCSVFileWatcher(csvFileName);
+      const content = await csvFileWatcher.parse();
+      const expected = [
+        ['p', 'role:default/CATALOG-USER', 'catalog-entity', 'read', 'allow'],
+        ['p', 'role:default/known_role', 'test.resource.deny', 'use', 'allow'],
+        ['g', 'user:default/known_user', 'role:default/known_role'],
+        ['g', 'user:default/tom', 'role:default/CATALOG-USER'],
+        ['g', 'group:default/reader-group', 'role:default/CATALOG-USER'],
+        ['g', 'group:default/reader-group', 'role:default/known_role'],
+      ];
+      expect(content).toStrictEqual(expected);
+    });
+  });
+
   describe('initialize', () => {
     it('should be able to add permission policies during initialization', async () => {
       const csvFileWatcher = createCSVFileWatcher(csvFileName);
@@ -238,6 +262,7 @@ describe('CSVFileWatcher', () => {
           'allow',
         ],
         ['role:default/catalog-deleter', 'catalog-entity', 'delete', 'deny'],
+        ['role:default/CATALOG-USER', 'catalog-entity', 'read', 'allow'],
         ['role:default/known_role', 'test.resource.deny', 'use', 'allow'],
       ];
 
@@ -279,6 +304,8 @@ describe('CSVFileWatcher', () => {
         ['user:default/guest', 'role:default/catalog-reader'],
         ['user:default/guest', 'role:default/catalog-deleter'],
         ['user:default/known_user', 'role:default/known_role'],
+        ['user:default/tom', 'role:default/CATALOG-USER'],
+        ['group:default/reader-group', 'role:default/CATALOG-USER'],
       ];
 
       await enforcerDelegate.addGroupingPolicy(legacyRole, legacyRoleMetadata);
@@ -327,6 +354,10 @@ describe('CSVFileWatcher', () => {
         'user:default/guest',
         'role:default/catalog-deleter',
       ];
+      const duplicateGroupRole = [
+        'group:default/reader-group', // changed to lowercase
+        'role:default/CATALOG-USER',
+      ];
 
       const duplicatePolicyWithDifferentEffect = [
         'role:default/duplication-effect',
@@ -360,6 +391,10 @@ describe('CSVFileWatcher', () => {
         6,
         `Duplicate role: ${duplicateRole} found in the file ${csvFileName}`,
       );
+      expect(mockLoggerService.warn).toHaveBeenNthCalledWith(
+        7,
+        `Duplicate role: ${duplicateGroupRole} found in the file ${csvFileName}`,
+      );
     });
 
     it('should fail to add policies with errors', async () => {

diff --git a/workspaces/rbac/plugins/rbac-backend/src/file-permissions/csv-file-watcher.ts b/workspaces/rbac/plugins/rbac-backend/src/file-permissions/csv-file-watcher.ts
@@ -16,7 +16,7 @@
 import type { LoggerService } from '@backstage/backend-plugin-api';
 
 import type { AuditLogger } from '@janus-idp/backstage-plugin-audit-log-node';
-import { Enforcer, FileAdapter, newEnforcer, newModelFromString } from 'casbin';
+import { Enforcer, newEnforcer, newModelFromString } from 'casbin';
 import { parse } from 'csv-parse/sync';
 import { difference } from 'lodash';
 
@@ -37,6 +37,7 @@ import {
   metadataStringToPolicy,
   policyToString,
   transformArrayToPolicy,
+  transformPolicyGroupToLowercase,
 } from '../helper';
 import { EnforcerDelegate } from '../service/enforcer-delegate';
 import { MODEL } from '../service/permission-model';
@@ -48,6 +49,7 @@ import {
   validateSource,
 } from '../validation/policies-validation';
 import { AbstractFileWatcher } from './file-watcher';
+import { LowercaseFileAdapter } from './lowercase-file-adapter';
 
 export const CSV_PERMISSION_POLICY_FILE_AUTHOR = 'csv permission policy file';
 
@@ -86,13 +88,17 @@ export class CSVFileWatcher extends AbstractFileWatcher<string[][]> {
    */
   parse(): string[][] {
     const content = this.getCurrentContents();
-    const parser = parse(content, {
+    const data = parse(content, {
       skip_empty_lines: true,
       relax_column_count: true,
       trim: true,
     });
 
-    return parser;
+    for (const policy of data) {
+      transformPolicyGroupToLowercase(policy);
+    }
+
+    return data;
   }
 
   /**
@@ -114,7 +120,7 @@ export class CSVFileWatcher extends AbstractFileWatcher<string[][]> {
 
     const tempEnforcer = await newEnforcer(
       newModelFromString(MODEL),
-      new FileAdapter(this.filePath),
+      new LowercaseFileAdapter(this.filePath),
     );
 
     // Check for any old policies that will need to be removed
@@ -197,7 +203,7 @@ export class CSVFileWatcher extends AbstractFileWatcher<string[][]> {
 
     const tempEnforcer = await newEnforcer(
       newModelFromString(MODEL),
-      new FileAdapter(this.filePath!),
+      new LowercaseFileAdapter(this.filePath!),
     );
 
     const currentFlatContent = this.currentContent.flatMap(data => {

diff --git a/workspaces/rbac/plugins/rbac-backend/src/file-permissions/lowercase-file-adapter.ts b/workspaces/rbac/plugins/rbac-backend/src/file-permissions/lowercase-file-adapter.ts
@@ -0,0 +1,55 @@
+/*
+ * Copyright 2024 The Backstage Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+import { FileAdapter, Helper, Model, mustGetDefaultFileSystem } from 'casbin';
+
+export class LowercaseFileAdapter extends FileAdapter {
+  public async loadPolicy(model: Model): Promise<void> {
+    if (!this.filePath) {
+      return;
+    }
+    await this.loadLowercasePolicyFile(model, Helper.loadPolicyLine);
+  }
+
+  private transformLineToLowercaseGroupsUsers(line: string): string {
+    if (line.trim().startsWith('g')) {
+      const policyArray = line.split(',');
+      if (policyArray.length >= 1 && policyArray[0].trim().startsWith('g')) {
+        policyArray[1] = policyArray[1].toLocaleLowerCase('en-US');
+      }
+      return policyArray.join(',');
+    }
+    return line;
+  }
+
+  private async loadLowercasePolicyFile(
+    model: Model,
+    handler: (line: string, model: Model) => void,
+  ): Promise<void> {
+    // Reference: https://github.com/casbin/node-casbin/blob/master/src/persist/fileAdapter.ts#L34-#L43
+    const bodyBuf = await (
+      this.fs ? this.fs : mustGetDefaultFileSystem()
+    ).readFileSync(this.filePath);
+    const lines = bodyBuf.toString().split('\n');
+
+    lines.forEach((line: string) => {
+      if (!line) {
+        return;
+      }
+      const lowercasedLine = this.transformLineToLowercaseGroupsUsers(line);
+      handler(lowercasedLine, model);
+    });
+  }
+}
diff --git a/workspaces/rbac/plugins/rbac-backend/src/helper.test.ts b/workspaces/rbac/plugins/rbac-backend/src/helper.test.ts
@@ -24,6 +24,8 @@ import {
   policyToString,
   removeTheDifference,
   transformArrayToPolicy,
+  transformPolicyGroupToLowercase,
+  transformRolesGroupToLowercase,
   typedPoliciesToString,
   typedPolicyToString,
 } from './helper';
@@ -123,6 +125,63 @@ describe('helper.ts', () => {
     });
   });
 
+  describe('transformPolicyGroupToLowercase', () => {
+    it.each([
+      [
+        ['g', 'user:default/TOM', 'role:default/CATALOG-USER'],
+        ['g', 'user:default/tom', 'role:default/CATALOG-USER'],
+      ],
+      [
+        ['g', 'group:default/Developers', 'role:default/CATALOG-USER'],
+        ['g', 'group:default/developers', 'role:default/CATALOG-USER'],
+      ],
+    ])('should convert group in %s to lowercase', (input, expected) => {
+      transformPolicyGroupToLowercase(input);
+      expect(input).toEqual(expected);
+    });
+
+    it('should not transform policy to lowercase', () => {
+      const policyArray = [
+        'p',
+        'role:default/CATALOG-USER',
+        'catalog-entity',
+        'read',
+        'allow',
+      ];
+      const expected = [...policyArray];
+      transformPolicyGroupToLowercase(policyArray);
+      expect(policyArray).toEqual(expected);
+    });
+
+    it('should handle invalid input', () => {
+      const policyArray = ['g'];
+      transformPolicyGroupToLowercase(policyArray);
+      expect(policyArray).toEqual(['g']);
+    });
+  });
+
+  describe('transformRolesGroupToLowercase', () => {
+    it('should convert users and groups in roles to lowercase', () => {
+      const roles = [
+        ['user:default/test', 'role:default/test-provider'],
+        ['group:default/Developers', 'role:default/Reader'],
+      ];
+      const expectedRoles = [
+        ['user:default/test', 'role:default/test-provider'],
+        ['group:default/developers', 'role:default/Reader'],
+      ];
+      expect(transformRolesGroupToLowercase(roles)).toEqual(expectedRoles);
+    });
+
+    it.each([[[['user:default/test']]], [[[]]]])(
+      'should handle invalid input %d',
+      input => {
+        const result = transformRolesGroupToLowercase(input);
+        expect(result).toEqual(input);
+      },
+    );
+  });
+
   describe('removeTheDifference', () => {
     const mockEnforcerDelegate: Partial<EnforcerDelegate> = {
       removeGroupingPolicies: jest.fn().mockImplementation(),

diff --git a/workspaces/rbac/plugins/rbac-backend/src/helper.ts b/workspaces/rbac/plugins/rbac-backend/src/helper.ts
@@ -135,6 +135,24 @@ export function transformArrayToPolicy(policyArray: string[]): RoleBasedPolicy {
   return { entityReference, permission, policy, effect };
 }
 
+export function transformPolicyGroupToLowercase(policyArray: string[]) {
+  if (
+    policyArray.length > 1 &&
+    policyArray[0].startsWith('g') &&
+    (policyArray[1].startsWith('user') || policyArray[1].startsWith('group'))
+  ) {
+    policyArray[1] = policyArray[1].toLocaleLowerCase('en-US');
+  }
+}
+
+export function transformRolesGroupToLowercase(roles: string[][]) {
+  return roles.map(role =>
+    role.length >= 1
+      ? [role[0].toLocaleLowerCase('en-US'), ...role.slice(1)]
+      : role,
+  );
+}
+
 export function deepSortedEqual(
   obj1: Record<string, any>,
   obj2: Record<string, any>,