Skip to content
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

Implement Permissions for Conversations #1545

Merged
merged 2 commits into from
Oct 31, 2024
Merged

Conversation

NolanTrem
Copy link
Collaborator

@NolanTrem NolanTrem commented Oct 31, 2024

Important

Implement permission checks for conversation operations, restricting non-superusers from accessing or modifying conversations they do not own, and update database schema to include user ownership.

  • Behavior:
    • Implement permission checks in management_router.py for conversation-related endpoints, restricting non-superusers from accessing or modifying conversations they do not own.
    • Superusers can access all conversations without restrictions.
  • Database:
    • Update conversations table in r2r_logger.py to include user_id and name columns.
    • Add verify_conversation_access() in r2r_logger.py to check user access to conversations.
  • Services:
    • Add verify_conversation_access() in management_service.py to delegate access checks to the logging provider.
    • Modify create_conversation(), get_conversation(), add_message(), and edit_message() in management_service.py to include user ID in operations.

This description was created by Ellipsis for 5874be4. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Reviewed everything up to 49c87ee in 1 minute and 54 seconds

More details
  • Looked at 357 lines of code in 4 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. py/core/providers/logger/r2r_logger.py:267
  • Draft comment:
    Ensure that user_ids are consistently treated as TEXT in the database queries to avoid type mismatches.
  • Reason this comment was not posted:
    Comment did not seem useful.
2. py/core/main/services/management_service.py:677
  • Draft comment:
    This function is a duplicate of an existing function in r2r_logger.py. Consider using the existing function directly instead of creating a new wrapper.

  • r2r_logger.py

  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment claims duplication, but I cannot verify this without seeing the other file. The function is newly added in this diff, so the comment is about a change. However, without strong evidence of duplication, the comment may not be useful. The comment could be speculative if the duplication is not confirmed.
    I might be missing the context from the other file, which could confirm the duplication. Without seeing the other file, I cannot be certain if the comment is correct.
    Given the rules, I should only keep comments with strong evidence of correctness. Without access to the other file, I cannot confirm the duplication claim.
    Delete the comment due to lack of strong evidence confirming the duplication claim.

Workflow ID: wflow_Qk6GyBj4HIt4dAUY


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

py/core/main/api/management_router.py Outdated Show resolved Hide resolved
@@ -832,6 +868,8 @@
message: str = Body(..., description="New content"),
auth_user=Depends(self.service.providers.auth.auth_wrapper),
) -> dict:
# TODO: Add a check to see if the user has access to the message
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a check to verify if the user has access to edit the message. This is crucial for maintaining security and preventing unauthorized edits.

Suggested change
# TODO: Add a check to see if the user has access to the message
if not await self.service.verify_message_access(message_id, auth_user.id): raise R2RException("You do not have access to edit this message.", 403)

Copy link

vercel bot commented Oct 31, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
yc_demo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 31, 2024 7:32pm
yc-demo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 31, 2024 7:32pm
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
recommendation_platform ⬜️ Ignored (Inspect) Oct 31, 2024 7:32pm

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on 5874be4 in 15 seconds

More details
  • Looked at 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. py/core/main/api/management_router.py:788
  • Draft comment:
    The change from user_ids = auth_user.id to user_ids = [auth_user.id] is correct. The conversations_overview function expects a list of user IDs, not a single ID.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The change from user_ids = auth_user.id to user_ids = [auth_user.id] is correct because the conversations_overview function expects a list of user IDs, not a single ID.

Workflow ID: wflow_g3SdFT6RUDxcWplL


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@NolanTrem NolanTrem merged commit 9b25a5e into main Oct 31, 2024
14 of 15 checks passed
@NolanTrem NolanTrem deleted the Nolan/ConversationPermissions branch October 31, 2024 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant