Skip to content

feat: Front end permissions for resource authorization #3878

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

shaohuzhang1
Copy link
Contributor

feat: Front end permissions for resource authorization

Copy link

f2c-ci-robot bot commented Aug 18, 2025

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.

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.

Copy link

f2c-ci-robot bot commented Aug 18, 2025

[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.

Needs approval from an approver in each of these files:

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

PermissionConst.APPLICATION_RESOURCE_AUTHORIZATION.getApplicationWorkspaceResourcePermission(source_id)
],
'OR'
),
folderEdit: () =>
hasPermission(
[
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The provided TypeScript code snippet appears to be part of an API definition that defines several permission checks within a workspace object. Some key points to consider:

  1. Code Structure: The structure is clear with multiple function definitions and permissions, but it looks like there might not be any actual logic implemented for these functions yet.

  2. Function Names:

    • auth: This suggests this function should return a boolean value indicating if the user/auth source has the necessary permissions.
    • folderEdit: Similarily, this function should also return a boolean value indicating permission for editing folders.
  3. Permissions:

    • Each check involves different arrays and conditions. It's unclear what exactly each array represents without further context on the hasPermission, ComplexPermission, RoleConst, and PermissionConst.
  4. Documentation and Comments: There seems to be some intended documentation or comments around these methods, but they are incomplete (// at the end of lines).

  5. Suggested Improvements:

    • Comments and Documentation: Add more detailed explanations and comments about the purpose and functionality of each method and associated constants.
    • Code Clarity: Make sure each function name corresponds clearly to its intended use and actions (e.g., checkUserAuth, canEditFolder).
    • Type Annotations: Consider adding type annotations if using JavaScript/TypeScript to help clarify parameter types and expected return values.

Here’s a revised version with added comments and improved clarity:

const workspace = {
    /**
     * Checks if the user/source has the specified permissions for authentication.
     * @param {string} sourceId - ID of the user source.
     * @returns {boolean} True if all requirements are met, false otherwise.
     */
    auth: (sourceId: string): boolean => {
        return hasPermission([
            new ComplexPermission([RoleConst.USER], [PermissionConst.APPLICATION.getApplicationWorkspaceResourcePermission(sourceId)], [], 'AND'),
            RoleConst.WORKSPACE_MANAGE.getWorkspaceRole,
            PermissionConst.APPLICATION_RESOURCE_AUTHORIZATION.getWorkspacePermissionWorkspaceManageRole,
            PermissionConst.APPLICATION_RESOURCE_AUTHORIZATION.getApplicationWorkspaceResourcePermission(sourceId)
        ], 'OR');
    },

    /**
     * Check if the user/source has the necessary permissions to edit folders.
     * @returns {boolean} True if allowed, false otherwise.
     */
    folderEdit: (): boolean => {
        return hasPermission([
            // Add permission checks here if needed
            // For example:
            // PermissionConst.FOLDER_EDIT.getFolderPermission,
            // ...
        ], 'OR');
    }
};

/**
 * Utility function to determine if user/auth meets given permissions.
 * @param {Array<IPermissionCheck>} checks - Array of permission checks to evaluate.
 * @returns {boolean} True if all checks PASS, else false.
 */
function hasPermission(checks: IPermissionCheck[], logicalOperator: string = 'AND'): boolean {
    // Implementation details would go here
}

interface IComplexPermission extends IPermissionCheck {
    roles: Role[];
    resources: Resource[];
    operators?: Operator;
    logicalOperands?: LogicalOperand[];
}

enum PermissionConst {
    APPLICATION = 'application',
    WORKSPACE_MANAGE = 'workspaceManage',
    APPLICATION_RESOURCE_AUTHORIZATION = 'applicationResourceAuthorization',
    FOLDER_EDIT = 'folderEdit',
    ...

}

This revision includes comments, docstrings, and placeholders for the implementation of the hasPermission function, which could include more complex logic based on your application's permission system.

@zhanweizhang7 zhanweizhang7 merged commit 31d71d1 into v2 Aug 18, 2025
3 of 5 checks passed
@zhanweizhang7 zhanweizhang7 deleted the pr@v2@feat_front_end_permissions_for_resource_authorization branch August 18, 2025 10:16
PermissionConst.KNOWLEDGE_RESOURCE_AUTHORIZATION.getWorkspacePermissionWorkspaceManageRole,
],
'OR',
),
folderEdit: () =>
hasPermission(
[
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The provided JavaScript code defines an object workspace with several methods that handle permission checks using a hypothetical function hasPermission. The methods include "search", "details", "delete", and "view". Each method takes parameters related to permissions.

Here's a summary of the issues, potential improvements, and some suggestions:

Issues:

  1. Missing Parameter Handling: Some methods do not explicitly handle missing parameters, which could lead to errors if called without proper arguments.

  2. Redundant Permissions: There are multiple uses of similar permission combinations across different functions, which might indicate redundancy that can be streamlined.

  3. Complexity in Permission Logic: The logic for defining complex conditions is complex, making it hard to read and understand at a glance.

  4. Comments: While comments explain how specific permissions work, they don't clarify where other parts of the logic should go or why certain decisions were made about individual permissions.

  5. Consistency: The overall structure of the methods does not fully follow best practices for readability and maintainability.

Potential Improvements/Cleanups:

  1. Parameter Validation: Ensure all parameter validation before calling hasPermission to prevent runtime errors related to unexpected input types. For example:

    if (!source_id) throw new Error("source_id is required");
  2. Inline Comments or Inline Documentation Blocks Instead of placing comments after each line, consider adding inline doc blocks above each method or function call explaining what it does. This makes it easier to understand the role of each part without additional scrolling through many lines of comments.

  3. Simplify Complex Conditions: Use named constants or variables instead of string literals for roles and permissions names. This improves readability and reduces errors due to mispellings.

  4. Refactor Redundant Code: Extract common permission logic into shared utility functions or reuse existing permission definitions when applicable.

  5. Documentation: Consider creating API documentation outside the source code for developers who will interact with these functions. A comprehensive guide on how to use them would also help users.

Example Refactored Snippet:

const { RoleConst, PermissionConst } = require('./constants');

const workspace = {
  search: () => hasPermission([
    // ... (existing permission array)
  ]),

  delete: () => hasPermission([
    // ... (existing permission array)
  ]),

  view: () => hasPermission([
    // ... (existing permission array)
  ]),
};

// Utility Function for Creating Complex Permissions
function makeComplexPermission(roleOrRoles, permissions, exclusions = [], conjunction = 'AND') {
  return { roleOrRoles, permissions, exclusions, conjunction };
}

// Example Usage in 'auth'
workspace.auth = (sourceId) => {
  if (!sourceId) throw new Error("source_id is required");

  const permissions = [makeComplexPermission(RoleConst.USER, [
    PermissionConst.KNOWLEDGE.getKnowledgeWorkspaceResourcePermission(sourceId),
  ], [])];

  permissions.push(RoleConst.WORKSPACE_MANAGE.getWorkspaceRole);
  permissions.push(PermissionConst.KNOWLEDGE_RESOURCE_AUTHORIZATION.getKnowledgeWorkspaceResourcePermission(sourceId));
  permissions.push(PermissionConst.KNOWLEDGE_RESOURCE_AUTHORIZATION.getWorkspacePermissionWorkspaceManageRole);

  return hasPermission(permissions, 'OR');
};

In this refactored version:

  • A utility function makeComplexPermission is used to encapsulate common permission structures.
  • Specific error handling is implemented directly in the method calls.
  • Overall clarity improves by organizing the permission logic more logically within each method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants