Skip to content

Restrict API access to non-public Assistants #4581

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

patrickfweston
Copy link
Contributor

@patrickfweston patrickfweston commented Apr 22, 2025

Description

I'm using an API key to access Onyx, sending it queries and returning the response. I ran into two issues when doing this:

API Keys can access all Assistants

API keys show up as users in the "Access" section for an Assistant:
image
If an Assistant is not marked as public and is granted only to specific users, they can still access and use that Assistant through calls to /create-chat-session. This could grant them access to Document Sets and knowledge they should not have access to through the front end.

This was addressed by adding a check to ensure the user making the request to /create-chat-session has access to the persona they are trying to request. It also ensures the Persona is marked as visible – this may not be desired, but there is no other way to disable the built-in Assistants without adding this check.

How Has This Been Tested?

I've tested this by:

  1. Create an API key that is granted "Basic" access
  2. Create a custom Assistant. Ensure it is not public. Grant your API key user access.
  3. Visit /create-chat-session using your API key as a bearer token. You should get an ID.
  4. Edit the custom Assistant. Ensure your API key user is visible. Previously, it would not show up here. Revoke your API key's user access.
  5. Visit /create-chat-session using your API key as a bearer token. You should not get an ID and should get an error.

Backporting (check the box to trigger backport action)

Note: You have to check that the action passes, otherwise resolve the conflicts manually and tag the patches.

  • This PR should be backported (make sure to check that the backport attempt succeeds)
  • [Optional] Override Linear Check

@patrickfweston patrickfweston requested a review from a team as a code owner April 22, 2025 17:23
Copy link

vercel bot commented Apr 22, 2025

@patrickfweston is attempting to deploy a commit to the Danswer Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR implements persona permission validation for API calls, ensuring proper access control when users interact with personas through chat sessions and other endpoints.

  • Added users field to PersonaSnapshot and FullPersonaSnapshot in /backend/onyx/server/features/persona/models.py to track authorized users with their IDs and emails
  • Implemented permission validation in /backend/onyx/server/query_and_chat/chat_backend.py to verify user access before creating chat sessions
  • Added proper error handling and logging for unauthorized persona access attempts
  • Ensures personas are both accessible and visible before allowing chat session creation
  • Maintains strict typing with Pydantic models for the new user permission fields

💡 (3/5) Reply to the bot's comments like "Can you suggest a fix for this @greptileai?" or ask follow-up questions!

2 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile

Comment on lines +275 to +277
# Ensure the persona is visible
if not persona.is_visible:
raise Exception("Persona is not visible to the user.")
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Redundant visibility check - get_persona_by_id already checks visibility

Copy link
Contributor Author

@patrickfweston patrickfweston Apr 22, 2025

Choose a reason for hiding this comment

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

I don't believe this is actually true. I'm not seeing anything where get_persona_by_id() checks for the is_visible property. Is that the case @greptileai?

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right - looking at the code more carefully, I don't see where get_persona_by_id() checks for visibility. The visibility check is only being done in create_new_chat_session(). We should remove this redundant check since it's not actually redundant.

Suggested change
# Ensure the persona is visible
if not persona.is_visible:
raise Exception("Persona is not visible to the user.")
# Check if the persona exists and is valid for this user.
# An exception will be thrown if the persona is not valid.
persona = get_persona_by_id(
persona_id=chat_session_creation_request.persona_id,
user=user,
db_session=db_session,
is_for_edit=False,
)

This removes the unnecessary visibility check while keeping the important persona validation logic. The comment about visibility in my previous review was incorrect.

@patrickfweston patrickfweston changed the title Enforce persona permissions for api calls Restrict API access to non-public Assistants and Fix user listing on Assistant edit page Apr 22, 2025
@patrickfweston patrickfweston changed the title Restrict API access to non-public Assistants and Fix user listing on Assistant edit page Restrict API access to non-public Assistants Apr 22, 2025
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