feat: Folder Permission

[shaohuzhang1]

feat: Folder Permission

[f2c-ci-robot]

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[f2c-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[shaohuzhang1]

After reviewing the code provided, I've identified several potential issues and some improvements:

1. Duplicate Permissions: The folderCreate, folderRead, folderEdit, folderAuth, and folderManage methods now have duplicate implementations with only the function signature differing. It would be more efficient to consolidate these into a single method using conditional statements.

2. Complexity Reduction: Some permissions involve multiple conditions, which can make the logic harder to read and maintain. Consider breaking down these complex permission checks into more manageable parts.

3. Code Duplication: The creation of ComplexPermission objects appears to be repeated across all folder operations. If we can factor this out, it will clean up the code.

4. Functionality Consistency: Ensure that each operation's functionality is aligned; for example, folderDelete should not be identical but rather perform specific actions based on permissions.

Based on these observations, here are suggestions for improvement:

Suggested Improvements

Consolidate Similar Methods

const workspace = (
    // ... existing fields ...
) => ({
    folderAccess: (functionName) => (folderId: string) => {
        return hasPermission([
            new ComplexPermission([RoleConst.USER], [permissions[fn].getToolWorkspaceResourcePermission(folderId)], [], 'AND'),
            RoleConst.WORKSPACE_MANAGE.getWorkspaceRole,
            permissions[fn].getToolWorkspaceResourcePermission(folderId),
            permissions[fn].getWorkspacePermissionWorkspaceManageRole
        ], 'OR');
    },

    create: (...folders: { source_id: string }) => folders.forEach(fn => ({ type: fn })),
    delete: (...folders: { source_id: string }) => folders.forEach(fn => ({ type: fn })), 
    update: (...folders: { source_id: string }) => folders.forEach(fn => ({ type: fn })),

    switch: (...spaces: { user_id?: string, space_id?: string[] }) => spaces.forEach(spaces),
    shareUser: (...roles: { workspace_access_roles: WorkspaceAccessRoles }) => roles.map(role => ({ type: role.type })),
});

Explanation

1. Consolidated Method (folderAccess):

   • This method takes a function name as an argument and combines common components into a concise form.
   • Each operation (create, delete, update) delegates the task to folderAccess.

2. Destructuring Parameters (...args):

   • Destructures arguments when calling functions like forEach.

3. Map Functions:

   • Maps over arrays of functions (shareUser).

These changes improve readability and reduce code duplication while maintaining consistency in access control logic.

[shaohuzhang1]

The provided code appears to be implementing permission checks using a complex hasPermission function with nested ComplexPermission objects. However, there are several improvements that can be made:

1. Consistency: The method names for permissions (folderRead, folderCreate, etc.) should match the resource name they operate on.

2. Duplicated Logic: Many of the methods share similar logic for checking permissions. This could be consolidated into fewer functions or abstracted away.

3. Typo Correction: There is a typo in _rolePermissions which might be unintentional.

4. Error Handling: Consider adding error handling to manage cases where permissions cannot be retrieved or checked due to exceptions.

Here's an optimized version of your code:

const workspace = {
  knowledgeRead: (workspaceId: string) => {
    return hasPermission([
      hasPermission(RoleConst.USER, WorkspaceRoles.workspaceManager),
      hasPermission(
        PermissionConst.KNOWLEDGE_WORKSPACE_RESOURCE_PERMISSION,
        { resourceIdType: ResourceTypeEnum.WORKSPACE, resourceId: workspaceId }
      )
    ]).OR;
  },
  // Consolidated folders management methods:
  folderPermissions(basePermissions: IHasBasePermissions): IFolderPermissions {
    [RoleConst.USER.getWorkspaceRole(), RoleConst.WORKSPACE_MANAGE.getWorkspaceRole()].
      forEach(role => basePermissions.addRole(role));

    ['read', 'create', 'edit', 'delete'].forEach(action => {
      const actionPermissionKey = PermissionConst[`${action.toUpperCase()}${capitalize('knowledge')}_${ResourceTypeEnum.FOLDER}_${action}`];
      if (!Object.keys(PermissionConst).includes(actionPermissionKey))
        throw new Error(`Missing ${action} permission key in PermissionConst`);

      basePermissions.addPermission({
        role: '',
        permissionKey: actionPermissionKey,
        target: ResourceTypeEnum.FOLDER,
        action
      });
    });

    return basePermissions;
  },

  folderAccess: (folderId: string) => this.folderPermissions(new HasBasePermissions())
    .checkPermission(ResourceTypeEnum.FOLDER),
};

Key Changes:

• Logical Grouping: Folder operations have been consolidated into a single method folderPermissions.
• Error Checking: A check is added at runtime to ensure all necessary permission keys exist in PermissionConst.
• Template Literals: Used for constructing permission keys dynamically.
• Method Naming: Renamed methods to be more descriptive and consistent with IHasBasePermissions.

These changes improve maintainability, reduce redundancy, and add better structure to the permission logic.

[shaohuzhang1]

The provided TypeScript code snippet is a configuration file that defines permissions for various actions related to folders within a system. The hasPermission function checks if an actor has the necessary roles or permissions specified in the role-based access control (RBAC) setup.

Overall, there are no significant irregularities in the structure of the code but some optimizations can be made:

1. Code Duplication: Functions like folderCreate, folderEdit, folderAuth, folderDelete, and overview_embed have almost identical logic except for different parameters and permission IDs. Consider creating a helper function to handle these repeated sections.

2. Documentation: Adding comments explaining each part of the RBAC rules could improve maintainability and understanding.

Here's an optimized version with minimal changes and additions:

const workspace = {
    // Function prototypes
    folderCreate: (folder_id: string) => {},
    folderEdit: (folder_id: string) => {},
    folderAuth: (folder_id: string) => {},
    folderDelete: (folder_id: string) => {},
    overview_embed: (source_id: string) => {},

    // Helper function for common permissions
    sharedPermissions: (...args) => [
        ...[RoleConst.USER],
        new ComplexPermission([RoleConst.USER], args.permissions, [], 'AND'),
        RoleConst.WORKSPACE_MANAGE.getWorkspaceRole,
        ...args.workspacePermissionList.map(permissionId =>
            PermissionConst.APPLICATION.getApplicationWorkspaceResourcePermission(permissionId)
        ),
        ...args.workspaceManageRolePermissionList.map(roleId => `${roleName}.${roleId}`)
    ],

    // Permissions definitions
    folder_create_role_list: ['ROLE_USER', 'ROLE_WORKSPACE_MANAGER'],
    folder_edit_role_list: ['ROLE_USER', 'ROLE_WORKSPACE_MANAGER'], // Assuming ROLE_APPLICATION_EDITOR and similar exist
    folder_auth_role_list: [...this.folder_edit_role_list, ROLE_APPLICATION_ACCESS_CONTROL], 
    folder_delete_role_list: [...this.folder_edit_role_list, ROLE_PERMISSION_DELETE],

    folder_create_permission_list: [PERMISSION_ID_APPLICATION_ADD_FILE],
    folder_edit_workspace_management_permissions_list: ['workspace.application.edit.permission'],

    /**
     * Check permission to create a folder within a specific workspace.
     */
    async checkFolderCreatePermission(actor, workspace): Promise<boolean> {
        const params = this.sharedPermissions(this.constructor.name, {
            permissions: this.folder_create_permission_list,
            workspacePermissionList: this.folder_create_workspace ManagementPermissionsLiST,
            workspaceManageRolePermissionList: this.folder_create_role_list
        });
        return await hasPermission(params);
    },

    /**
     * Check permission to edit a folder within a specific workspace.
     */
    async checkFolderEditPermission(actor, workspace): Promise<boolean> {
        const params = this.sharedPermissions(this.constructor.name, {
            permissions: [
                PERMISSION_ID_APPLICATION_MODIFY_FILE,
                PERMISSION_ID_APPLICATION_VIEW_ALL_FILES
            ],
            workspacePermissionList: this.folder_edit_workspaceManagementPermissionsList,
            workspaceManageRolePermissionList: this.folder_edit_role_list
        });
        return await hasPermission(params);
    },
};

// Example usage:
async function exampleUsage() {
    const userActor = getActo('USER');
    const workspace = getWorkspace('EXAMPLE');

    console.log(await workspace.checkFolderCreatePermission(userActor, workspace));
    console.log(await workspace.checkFolderEditPermission(userActor, workspace));
}

exampleUsage();

Key Changes and Additions:

• Reduced duplication by extracting common permission logic into a helper function.
• Added documentation through annotations and comments.
• Created method names for checking permissions (checkFolderxxxPermission) which are more descriptive and easier to understand when using the class methods.