feat: The folder creator manages permissions and the root directory displays all resources

[shaohuzhang1]

feat: The folder creator manages permissions and the root directory displays all resources

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

The provided code has several potential issues and areas for improvement:

1. Security Concerns: The select_list function is used to query the database directly using dynamic SQL with user-provided input (user_resource_permission_list). This can lead to SQL injection attacks if the input data is not properly validated.

2. Code Duplication: There's a duplication of logic in the auth_resource method, which checks based on auth_type. Consider refactoring this into a separate utility function called get_auth_targets.

3. Unnecessary Checks: The if isinstance(auth_target_type, dict) condition is unnecessary within the auth_resource method because it always returns the same value regardless of whether it's passed as a string or dictionary.

4. Inconsistent Code Style: Some lines have inconsistent spacing between operators (==) and arguments (,).

5. Cache Version Handling: While updating cache versions seems necessary, ensure that the version system handles concurrent updates correctly to avoid race conditions, especially before committing changes.

Here’s an optimized version of the code addressing these points:

import os

from django.core.cache import cache

from apps.system_manage.models import DatabaseModelManage, UserResourcePermissions

def get_auth_targets(auth_type):
    # Placeholder logic to determine authorization targets based on auth_type
    return ['view', 'manage'] if auth_type == ResourceAuthType.RESOURCE_PERMISSION_GROUP else ['role']

class YourClass:
    def __init__(self, data):
        self.data = data

    @staticmethod
    def select_list(query_sql_path, params):
        # Securely execute SQL query
        # Example implementation using Django ORM
        pass

    def is_valid(self, *, raise_exception=False):
        user_resource_permission_list = self.data.get('user_resource_permission_list')
        workspace_id = self.data.get('workspace_id')
        if isinstance(user_resource_permission_list, str):
            illegal_target_id_list = self.select_list(os.path.join(PROJECT_DIR, "apps", "system_manage", 'sql', 'check_member_permission_target_exists.sql'), json.dumps([user_resource_permission_list], ensure_ascii=False))
        elif isinstance(user_resource_permission_list, list):
            illegal_target_id_list = self.select_list(os.path.join(PROJECT_DIR, "apps", "system_manage", 'sql', 'check_member_permission_target_exists_aliased_groups.sql').ensure_ascii(False))
        else:
            illegal_target_id_list = []

        if illegal_target_id_list is not None and len(illegal_target_id_list) > 0:
            raise AppApiException(500, _('Non-existent id[') + str(illegal_target_id_list) + ']')

    def auth_resource_batch(self, resource_id_list: list):
        # Optimized batch authentication logic
        try:
            for resource_id in resource_id_list:
                self.auth_resource(resource_id)
            cache.delete_many(["resource_permissions_" + str(workspace_id) for workspace_id in set(self.data["workspace_ids"])])
        except Exception as e:
            return False
        return True

    def auth_resource(self, resource_id: str, is_folder: bool = False):
        self.is_valid(True)

        auth_target_type = self.data.get('auth_target_type')
        permission_list = get_auth_targets(auth_target_type)
        user_id = self.data.get('user_id')

        permissions_to_save = UserResourcePermissions.objects.create(
            target=resource_id,
            auth_target_type=auth_target_type,
            permissions=permission_list,
            workspace_id=self.data.get('workspace_id'),
            user_id=user_id,
            is_group=is_folder
        )

        # Refresh cached permissions
        Cache_Version.PERMISSION_LIST.refresh()

    def get_queryset(self, instance):
        ...  # Existing queryset logic without significant issues

Key Changes:

• SQL Injection Prevention: Used a placeholder example for securely executing SQL queries, ensuring proper parameterization.
• Utility Function: Refactored auth_resource into get_auth_targets.
• Simplified Conditions: Removed redundant checks.
• Consistent Formatting: Improved readability through consistent indentation and spacing.
• Error Handling in Batch Operations: Added error handling when deleting from cache during batch operations.

[shaohuzhang1]

The provided code snippet contains several enhancements and modifications to improve performance, maintainability, and functionality:

1. Import Statements: The import statement for UserResourcePermissionSerializer has been added, which may be necessary for handling user resource permissions related operations after creating a new folder.

2. Folder Creation: A new instance of the folder is created using the get_or_create method in the insert function. This approach helps in avoiding potential issues with duplicate entries if there's already an existing folder with the same name under the specified parent. However, ensure that this logic aligns with business requirements (e.g., whether duplicates are allowed).

3. Access Verification: After creating the new folder, a call is made to UserResourcePermissionSerializer within insert. Assuming this serializer handles the creation or update of permission records specifically targeting folders (auth_target_type='folder'). Ensure that the data being passed to UserResourcePermissionSerializer is correct and relevant to your use-case.

4. Comments and Documentation: It'd be beneficial to include comments explaining each part of the operation, especially if it's not immediately clear what the code is doing, such as why certain checks or actions are taken.

5. Security Considerations: Make sure that sensitive information, like workspace IDs and user IDs, is handled securely by validating them against authorized resources before performing database updates or API calls.

6. Performance Optimization: If there're many instances of this insertion process happening concurrently, consider adding locking mechanisms to prevent concurrent access scenarios from affecting integrity or performance.

Overall, these changes generally enhance the robustness and usability of the code. Always test thoroughly after making such substantial modifications to ensure all functionalities remain intact and perform as expected.

[shaohuzhang1]

There are no specific irregularities or issues with the provided code snippet. However, I suggest making a small optimization:

            folder_query_set = folder_query_set.filter(workspace_id=workspace_id)
            application_query_set = application_query_set.filter(workspace_id=workspace_id)

        folder_id = instance.get('folder_id')

        if folder_id is not None and folder_id != workspace_id:
            folder_query_set = folder_query_set.filter(parent=folder_id).distinct()
            application_query_set = application_query_set.filter(folder_id=folder_id)

The distinct() method is added to the folder_query_set filter operation when filtering by parent, which can increase efficiency slightly.

This change ensures that only unique entries from the folder_query_set filtered by parent are included in the results. If multiple sub-folders have the same parent at different levels of nesting, this might prevent duplicates. But please note that using distinct in a query may negatively impact performance on very large datasets due to increased database scanning. Always balance between readability and performance based on practical use cases.