Skip to content

feat: Resources authorization #3183

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
merged 1 commit into from
Jun 4, 2025
Merged

feat: Resources authorization #3183

merged 1 commit into from
Jun 4, 2025

Conversation

shaohuzhang1
Copy link
Contributor

feat: Resources authorization

Copy link

f2c-ci-robot bot commented Jun 4, 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 Jun 4, 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

@shaohuzhang1 shaohuzhang1 merged commit bc03dea into v2 Jun 4, 2025
3 of 4 checks passed
@shaohuzhang1 shaohuzhang1 deleted the pr@v2@feat_resouce_auth branch June 4, 2025 05:05
ApplicationVersionSerializer.Operate(
data={'application_id': application_id, 'work_flow_version_id': work_flow_version_id,
'user_id': request.user.id}).edit(
request.data))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here are some potential irregularities, issues, and optimization suggestions for the provided Python code:

Potential Irregularities and Issues

  1. Indentation Errors: The file contains inconsistent indentation levels within nested classes (Page and Operate). This can cause parsing errors in certain environments.

  2. Unnecessary Imports: Some imports like rest_framework.request.Request, result, and PermissionConstants could be removed if not utilized directly within the functions they reference (e.g., request.user).

  3. Variable Usage: Ensure that all variables used consistently throughout different parts of the class have appropriate values before being accessed. For example, in def get(self), there might need to ensure workspace_id, application_id, etc., are correctly passed as arguments.

  4. Error Handling: There is no error handling mechanism implemented beyond returning a success response with JSON data. Consider adding more robust exception handling or logging for production use.

  5. Unused Variables: The variable self in some methods appears unused but should ideally refer to the instance of the view class.

  6. Consistency in Namespaces: While not critical here, it's good practice to maintain consistency in namespaces and method naming conventions across similar views or files.

  7. Comments and Docstrings: Comments and docstrings indicate what each section does, which is helpful, but consider refining the comments where necessary.

Optimization Suggestions

  1. Code Refactoring: Break down complex logic inside individual functions into smaller components. This makes the code easier to understand and modify.

  2. Query Parameter Validations: Validate incoming query parameters to prevent unwanted behavior or security vulnerabilities such as SQL injection.

  3. Caching Logic: Implement caching mechanisms if applicable, especially for frequently queried data, to improve performance.

  4. Testing: Unit test your views thoroughly using Django’s testing framework to catch bugs early on during development.

  5. DRF-Spectacular Configuration: Enhance Swagger/OpenAPI documentation configuration by customizing paths, types, descriptions, and other metadata to better explain the API endpoints.

Specific Fixes for Indentation

To fix the indentation issue, you should align each level of nesting under the respective method definitions with consistent spaces:

# ... rest of the module ...

class ApplicationVersionView(APIView):
    authentication_classes = [TokenAuth]

    @extend_schema(
        methods=['POST'],
        description=_("Get the application list"),
        summary=_("Get the application list"),
        operation_id=_("Get the application list"),  # type: ignore
        # parameters=ApplicationCreateAPI.get_parameters(),
        # request=ApplicationCreateAPI.get_request(),
        # responses=ApplicationCreateAPI.get_response(),
        tags=[_('Application/Version')]  # type: ignore
    )
    @has_permissions(PermissionConstants.APPLICATION_READ)
    def get(self, request: Request, workspace_id, application_id: str):
        return result.success(
            ApplicationVersionSerializer.Query(data={
                'name': request.query_params.get('name'),
                'user_id': request.user.id,
                'application_id': application_id}).list(request.data))

    class Page(APIView):
        authentication_classes = [TokenAuth]

        @extend_schema(
            methods=['GET', ],  # Consistent indentation within the brackets
            description=_("Get the list of application versions by page"),
            summary=_("Get the list of application versions by page"),
            operation_id=_("Get the list of application versions by page"),  # type: ignore
            # parameters=ApplicationCreateAPI.get_parameters(),
            # request=ApplicationCreateAPI.get_request(),
            # responses=ApplicationCreateAPI.get_response(),
            tags=[_('Application/Version')]  # type: ignore
        )
        @has_permissions(PermissionConstants.APPLICATION_READ)
        def get(self, request: Request, application_id: str, current_page: int, page_size: int):
            return result.success(
                ApplicationVersionSerializer.Query(data={
                    'name': request.query_params.get('name'),
                    'user_id': request.user,
                    'application_id': application_id}).page(current_page, page_size))

    class Operate(APIView):
        authentication_classes = [TokenAuth]

        @extend_schema(
            methods=['GET'],
            description=_("Get application version details"),
            summary=_("Get application version details"),
            operation_id=_("Get application version details"),  # type: ignore
            # parameters=ApplicationCreateAPI.get_parameters(),
            # request=ApplicationCreateAPI.get_request(),
            # responses=ApplicationCreateAPI.get_response(),
            tags=[_('Application/Version')]  # type: ignore
        )
        @has_permissions(PermissionConstants.APPLICATION_READ)
        def get(self, request: Request, application_id: str, work_flow_version_id: str):
            return result.success(
                ApplicationVersionSerializer.Operate(data={
                    'user_id': request.user,
                    'application_id': application_id, 'work_flow_version_id': work_flow_version_id}).one())

        @extend_schema(
            methods=['PUT'],
            description=_("Modify application version information"),
            summary=_("Modify application version information"),
            operation_id=_("Modify application version information"),  # type: ignore
            # parameters=ApplicationCreateAPI.get_parameters(),
            # request=ApplicationCreateAPI.get_request(),
            # responses=ApplicationCreateAPI.get_response(),
            tags=[_('Application/Version')]  # type: ignore
        )
        def put(self, request: Request, application_id: str, work_flow_version_id: str):
            return result.success(
                ApplicationVersionSerializer.Operate(data={
                    'application_id': application_id, 'work_flow_version_id': work_flow_version_id,
                    'user_id': request.user.id}).edit(request.data))

This refactored approach ensures clean, structured code with proper indentation.

work_flow_version.save()
return ApplicationVersionModelSerializer(work_flow_version).data
else:
raise AppApiException(500, _('Workflow version does not exist'))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There aren't any major issues or performance optimizations needed in this code at present. Here's a brief review:

Code Review

  1. Imports: The page_search function from the common.db.search module is imported but isn't used anywhere else. It might be redundant.

  2. Serializer Class Documentation: The docstring for each serializer class and method starts with "##", which doesn't seem appropriate for Python docstrings (""" """).

  3. Method Signatures:

    • The list method takes two optional parameters (with_valid), which could potentially confuse users about their purpose.
    • The page method has a similar issue with its parameter description.
  4. Exception Handling:

    • All exceptions are handled within the respective methods using raise AppApiException.
    • This suggests that all invalid operations should lead to an API exception, which is reasonable given the business logic involved.

Overall, the code looks well-structured and implements the required functionality for querying and editing workflow versions associated with applications. However, minor improvements can be made in documentation and handling signatures.

Possible Improvements

  1. Docstring Formatting and Clarity:

    • Ensure consistent docstring formatting using triple quotes (''' ... ''') instead of double hash symbols.
  2. Parameter Descriptions:

    • Clarify the use of with_valid parameters in both the list and page methods. Perhaps rename them to something like validate_input.
  3. Code Readability:

    • Consider breaking down larger methods into smaller functions when they become too complex.

Here's a revised version of the first few lines based on these suggestions:

#coding=utf-8
"""
project: MaxKB
Author:虎虎
file: application_version.py
date:2025/6/3 16:25
desc:
"""

from typing import Dict
from django.db.models import QuerySet
from rest_framework import serializers
from django.utils.translation import gettext_lazy as _

from application.models import WorkFlowVersion
from common.db.search import page_search
from common.exception.app_exception import AppApiException

This adjustment makes it easier for anyone reading the code to understand what each part represents.

item.operate[AuthorizationEnum.MANAGE] = false
item.permission[AuthorizationEnum.VIEW] = true
} else if (Name === AuthorizationEnum.VIEW && !e) {
item.permission[AuthorizationEnum.MANAGE] = false
}
}
})
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 code appears to be related to managing permissions for different types of resources in an application, such as datasets or knowledge. Here are some points and potential improvements:

Code Points Review

  1. Type Checking:

    const isDataset = computed(() => props.type === AuthorizationEnum.DATASET)

    Ensure that AuthorizationEnum is properly defined elsewhere in your project.

  2. Variable Naming Consistency:

    • In template conditions (v-if, etc.), you use icon but store it in type. Make sure these correspond correctly.
  3. Permission Array Structure:

    type: String, // Assuming this should be Number for enum values
    operate: {
      type: Object,
      default() {
        return {};
      },
    },
  4. Template Updates:

    • Corrected variable names from operate to permission.
    • Adjusted logic to handle role checks based on permission properties rather than operation properties.
  5. State Management:

    • Ensure manage prop is properly passed down to control checkbox behavior when editing mode is off.
  6. Logic Flow Corrections:

    • Fixed logic involving checkedOperateChange to ensure correct handling of user roles based on view and manage permissions.
  7. Validation Check:
    Verify if there's a validation component being used somewhere since you're referring to @/components/auto-tooltip.

Optimization Suggestion

  • Use More Descriptive Variable Names:
    Instead of using generic variables like all_indeterminate and all_checked, consider more descriptive names that clearly indicate their purpose (e.g., datasetAllChecked, knowledgeAllChecked).

Potential Improvements

  1. Refactored Logic for Role Checks:
    The current logic around managing and usage toggling can be simplified:

    • If Manage is enabled, always enable Use.
    • Only enable Manage if both View and Use are checked.
  2. Code DRYing:
    Extract common operations into helper methods to reduce duplicates.

  3. Validation Enhancements:
    Ensure that any input validations (like ensuring that each resource has at least one permission checked) are implemented where applicable.

Overall, the code structure looks mostly consistent with expected conventions for Vue.js. However, reviewing specific contexts may uncover additional areas needing attention depending on your full implementation details.

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.

1 participant