Skip to content

fix: When other users download the public model, they cannot obtain the download data #2785

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.

Sign up for GitHub

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
shaohuzhang1 merged 1 commit into from
Apr 2, 2025
Merged

fix: When other users download the public model, they cannot obtain the download data #2785

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion apps/setting/serializers/provider_serializers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,12 @@ def one(self, with_valid=False):

def one_meta(self, with_valid=False):
if with_valid:
self.is_valid(raise_exception=True)
super().is_valid(raise_exception=True)
model = QuerySet(Model).filter(id=self.data.get("id")).first()
if model is None:
raise AppApiException(500, _('Model does not exist'))
if model.permission_type == 'PRIVATE' and str(model.user_id) != str(self.data.get("user_id")):
raise Exception(_('No permission to use this model') + f"{model.name}")
model = QuerySet(Model).get(id=self.data.get('id'), user_id=self.data.get('user_id'))
return {'id': str(model.id), 'provider': model.provider, 'name': model.name, 'model_type': model.model_type,
'model_name': model.model_name,
Copy link
Copy Markdown
Contributor Author

@shaohuzhang1 shaohuzhang1 Apr 2, 2025

Choose a reason for hiding this comment

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

The provided code snippet has several issues and opportunities for improvement:

  1. Redundancy: The one_meta method calls self.is_valid() multiple times. This can lead to unnecessary validation checks during each call.
  2. Security Risk: If with_valid=False, there's no safety margin against unauthorized access since the data is directly retrieved from self.data.
  3. Duplicate Code: The if model_permission_type... logic is duplicated at two different points (both checking for existence and permissions).
  4. Error Handling: The error messages are missing when raising exceptions, which could make debugging difficult.
  5. Code Duplication: The retrieval of the model after checking its validity should be moved to a more reusable function.

Here’s a revised version with improvements:

from django.core.exceptions import ObjectDoesNotExist
from .exceptions import AppApiException

class OneMixin:
    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)

    def one_meta(self, with_valid=False):
        if with_valid:
            self.check_validity(raise_exception=True)
            
            # Ensure that the model exists first
            try:
                model = Model.objects.filter(id=self.data.get("id")).get()
            except ObjectDoesNotExist:
                raise AppApiException(500, _('Model does not exist'))
                
            # Check permission type
            if model.permission_type == 'PRIVATE':
                current_user_id = str(get_current_user_id())  # Assuming get_current_user_id() returns the authenticated user ID
                if current_user_id != str(model.user_id):
                    raise Exception(_('No permission to use this model') + f" {model.name}")
        
        return self.get_model()

    def check_validity(self, raise_exception=False):
        # Implement your validation logic here
        pass

    def get_model(self):
        try:
            model = Model.objects.get(
                id=int(self.data.get('id')), 
                user_id=get_current_user_id())
        except IntegrityError as e:
            raise AppApiException(500, _('Database integrity error')) from e
        except ValueError as e:
            raise AppApiException(400, _('Invalid input')) from e
        else:
            return {'id': str(model.id), 'provider': model.provider, 'name': model.name,
                        'model_type': model.model_type, 'model_name': model.model_name}

Key Improvements:

  • Simplified Validation Logic: Removed redundant is_valid() calls by delegating validation to a separate check_validity method.
  • Check for Existence Early: Ensured that the model exists before proceeding with the permission check.
  • Reused Functionality: Moved the duplicate code around the model fetching process into the get_model method for better readability and maintainability.
  • Added Error Messages: Included detailed error messages in exception instances where necessary.
  • User Authentication: Used a placeholder function get_current_user_id() assuming it retrieves the currently authenticated user's ID. You need to replace this with an actual implementation based on your authentication system.

Expand Down