Skip to content

Conversation

@iceljc
Copy link
Collaborator

@iceljc iceljc commented Jun 12, 2025

PR Type

Bug fix, Enhancement


Description

• Fix user permission retrieval in profile endpoint
• Add ContentType property to file upload models
• Improve error messages for multi-modal operations
• Standardize error handling across file processing methods


Changes walkthrough 📝

Relevant files
Enhancement
InstructModeController.cs
Enhance file processing and error messages                             

src/Infrastructure/BotSharp.OpenAPI/Controllers/InstructModeController.cs

• Add ContentType property to InstructFileModel objects in file upload
methods
• Update error messages to be more generic and accurate

Change "reading images" to "reading multi-modal files"
• Simplify PDF
and speech-to-text error messages

+8/-6     
Bug fix
UserController.cs
Fix user permission retrieval in profile                                 

src/Infrastructure/BotSharp.OpenAPI/Controllers/UserController.cs

• Add BotSharp.Abstraction.Roles import
• Retrieve user authorizations
and set permissions in GetMyUserProfile method
• Add null check before
setting user permissions

+8/-0     

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @GGHansome
    Copy link

    Auto Review Result:

    Code Review Summary

    Overview of Changes: The submitted code includes:

    • Updates to error messages to use more generalized terms such as "multi-modal files" instead of specifics like "images" or "pdf uploads."
    • Addition of a ContentType field to InstructFileModel during list creation.
    • Modification in UserController to include user permissions in the user profile.

    Issues Found

    Issue 1: Consistency in Error Messaging

    • Description: The error messages are updated to use "multi-modal files," which is a more accurate reflection of the function capabilities and can improve clarity when logging errors.
    • Recommendation: Ensure that similar updates in terminology are consistently applied across the project for coherence.
    • Example:
      // Before
      var error = $"Error in reading image upload. {ex.Message}";
      // After
      var error = $"Error in reading multi-modal files. {ex.Message}";

    Issue 2: Addition of ContentType to File Models

    • Description: The ContentType property is now added to InstructFileModel, which aids in maintaining file metadata.
    • Recommendation: Verify that ContentType is handled or validated wherever necessary to avoid undesired behavior when processing files.
    • Example:
      // Before
      FileData = FileUtility.BuildFileDataFromFile(x)
      // After
      FileData = FileUtility.BuildFileDataFromFile(x),
      ContentType = x.ContentType

    Issue 3: Addition of User Permissions in User Profiles

    • Description: User profiles now include permissions extracted from authorizations, providing enhanced access control visibility.
    • Recommendation: Consider caching or lazy-loading permissions for better performance if the authorization data is large or complex.
    • Example:
      if (user != null)
      {
          var auth = await _userService.GetUserAuthorizations();
          user.Permissions = auth?.Permissions ?? [];
      }

    Overall Assessment

    The code changes improve the flexibility and maintainability of the project by using more generalized terms in error messages and enhancing file and user metadata management. It is crucial to ensure that these changes integrate smoothly with existing logic, particularly in handling new ContentType data and user permissions.

    @qodo-code-review
    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Missing Property

    The FileData property assignment was removed from InstructFileModel creation but may still be required for proper file processing functionality

        FileData = FileUtility.BuildFileDataFromFile(x),
        ContentType = x.ContentType
    }).ToList();
    Potential Issue

    The GetUserAuthorizations method is called without any user context or parameters, which may not return the correct authorizations for the current user

    var auth = await _userService.GetUserAuthorizations();
    user.Permissions = auth?.Permissions ?? [];

    @qodo-code-review
    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Pass user ID to authorization method

    The authorization retrieval should be scoped to the specific user rather than
    getting generic authorizations. This could lead to incorrect permissions being
    assigned to users.

    src/Infrastructure/BotSharp.OpenAPI/Controllers/UserController.cs [110-114]

     if (user != null)
     {
    -    var auth = await _userService.GetUserAuthorizations();
    +    var auth = await _userService.GetUserAuthorizations(user.Id);
         user.Permissions = auth?.Permissions ?? [];
     }
    • Apply / Chat
    Suggestion importance[1-10]: 9

    __

    Why: The suggestion correctly points out a potential security issue. Calling _userService.GetUserAuthorizations() without a specific user ID might fetch permissions for the wrong user, or generic permissions. Passing user.Id as suggested makes the intent explicit and ensures the correct permissions are retrieved for the specific user profile being constructed.

    High
    • More

    @iceljc iceljc merged commit 8e44bca into SciSharp:master Jun 12, 2025
    3 of 4 checks passed
    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