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

feat: allow setting public brain status to private #1258

Merged
merged 9 commits into from
Sep 26, 2023
Merged

Conversation

mamadoudicko
Copy link
Contributor

@mamadoudicko mamadoudicko commented Sep 25, 2023

Refetch brains list on when new brain is added
Update BrainConfig type
Update useSettingsTab, add usebrainFormState, and useSettings tab
Add modal
Update translations
Handle brain status change to private
Validate chat access
Fix failing tests and remove deprecated

#1213

@vercel
Copy link

vercel bot commented Sep 25, 2023

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

Name Status Preview Comments Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 26, 2023 8:38am
quivr-strapi ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 26, 2023 8:38am
quivrapp ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 26, 2023 8:38am

@dosubot dosubot bot added the area: backend Related to backend functionality or under the /backend directory label Sep 25, 2023
@mamadoudicko mamadoudicko changed the title feat: set public brain status to private feat: allow setting public brain status to private Sep 25, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Sep 25, 2023

Risk Level 2 - /home/runner/work/quivr/quivr/backend/routes/authorizations/brain_authorization.py

The validate_brain_authorization function is checking if the user has at least one of the required roles. This might not be the intended behavior if all the required roles are necessary. Consider changing the logic to ensure the user has all the required roles.

Example:

# Instead of this
if user_brain.rights not in required_roles:

# Do this
if not all(role in user_brain.rights for role in required_roles):

Risk Level 3 - /home/runner/work/quivr/quivr/backend/routes/chat_routes.py

  1. Exception Handling: The delete_chat_from_db function catches all exceptions and simply prints them. This is not a good practice as it can make debugging difficult. It would be better to log the exceptions with more context or re-raise them after logging. For example:
try:
    supabase_db.delete_chat_history(chat_id)
except Exception as e:
    logger.error(f'Error deleting chat history for chat_id {chat_id}: {e}')
    raise
  1. Error Messages: The check_user_requests_limit function raises an HTTPException with a status code of 429 when the user has reached the maximum number of requests for the day. However, the error message does not include the limit or the current count, which would be useful information for the user.
  2. Code Duplication: There is a lot of duplicated code in the create_question_handler and create_stream_question_handler functions. This could be refactored into a separate function to improve readability and maintainability.
  3. Security: The create_question_handler and create_stream_question_handler functions retrieve the user's OpenAI API key from the request headers. This could be a security risk if the request is not over a secure connection. Consider using a more secure method of transmitting sensitive information.

Risk Level 3 - /home/runner/work/quivr/quivr/backend/models/databases/supabase/brains.py

  1. The openai_api_key is being stored as plain text which is a security risk. Consider using a secure method to store sensitive information like API keys.

  2. The delete_brain_users method is deleting all users with the role 'Viewer'. This might not be the intended behavior if there are other types of subscribers. Consider adding a check to ensure only the intended users are deleted.

  3. The delete_brain_vector and delete_brain_users methods are returning the entire result of the delete operation. This could potentially leak sensitive information. Consider returning only the necessary information.

Example:

# Instead of this
return results

# Do this
return {'status': 'success', 'deleted_count': results.count}

🔒🔍🔄


Powered by Code Review GPT

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: backend Related to backend functionality or under the /backend directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants