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.

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
+23 −1
Open

Restrict API access to non-public Assistants #4581

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f70583d
Ensure requests to /create-chat-session check the user's permissions …
patrickfweston Apr 21, 2025
884d022
Make sure /persona/{id} lists granted users so they can display on /a…
patrickfweston Apr 22, 2025
2e78586
Code linting; add groups in addition to users per greptile suggestion
patrickfweston Apr 22, 2025
5e2aeb8
Merge branch 'main' into enforce-persona-permissions-for-api-calls
patrickfweston Apr 22, 2025
d54eb69
Merge in upstream fix to FullPersonaSnapshot
patrickfweston Apr 22, 2025
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
24 changes: 23 additions & 1 deletion backend/onyx/server/query_and_chat/chat_backend.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,13 +261,35 @@ def create_new_chat_session(
db_session: Session = Depends(get_session),
) -> CreateChatSessionID:
user_id = user.id if user is not None else None

try:
# 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,
)

# Ensure the persona is visible
if not persona.is_visible:
raise Exception("Persona is not visible to the user.")
Comment on lines +275 to +277
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 •
edited
Loading

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.


except Exception as e:
logger.exception(e)
raise HTTPException(
status_code=400,
detail="The given user does not have access to this Persona.",
)

try:
new_chat_session = create_chat_session(
db_session=db_session,
description=chat_session_creation_request.description
or "", # Leave the naming till later to prevent delay
user_id=user_id,
persona_id=chat_session_creation_request.persona_id,
persona_id=persona.id,
)
except Exception as e:
logger.exception(e)
Expand Down
Loading