Allow curators to create public connectors / document sets #4972
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Extends permissions to allow curators to create and manage public connectors and document sets, with significant changes across both frontend and backend components.
- Modified
backend/ee/onyx/db/user_group.pyto allow curators to create public objects through new ownership validation invalidate_object_creation_for_user - Added object ownership validation in
backend/onyx/server/documents/connector.pyandcc_pair.pyto verify creator permissions before updates - Improved
IsPublicGroupSelectorcomponent to handle curator-specific logic without blocking when no groups are available - Fixed a potential security concern in
CreateCredential.tsxwith temporary debugging code that should be removed before production - Added proper error handling and existence checks in document set API endpoints with nullable user parameter support
13 files reviewed, 5 comments
Edit PR Review Bot Settings | Greptile
| def fetch_connector_credential_pair_for_connector( | ||
| db_session: Session, | ||
| connector_id: int, | ||
| ) -> ConnectorCredentialPair | None: | ||
| stmt = select(ConnectorCredentialPair).where( | ||
| ConnectorCredentialPair.connector_id == connector_id, | ||
| ) | ||
| return db_session.scalar(stmt) |
There was a problem hiding this comment.
logic: This function will only return one random CC pair if multiple exist for the connector. Add ORDER BY clause or handle multiple results explicitly
| ) { | ||
| formikProps.setFieldValue("authentication_method", authMethod); | ||
| } | ||
| console.log(formikProps.isSubmitting); |
There was a problem hiding this comment.
logic: remove debug console.log before merging to production
| # Allow curators and global curators to create public objects | ||
| # w/o associated groups IF the object is new/owned by them | ||
| if ( | ||
| object_is_public | ||
| and user.role in [UserRole.CURATOR, UserRole.GLOBAL_CURATOR] | ||
| and (object_is_new or object_is_owned_by_user) | ||
| ): | ||
| return |
There was a problem hiding this comment.
logic: explicitly check for None to avoid potential false negatives if object_is_new/object_is_owned_by_user is False
| def patch_document_set( | ||
| document_set_update_request: DocumentSetUpdateRequest, | ||
| user: User = Depends(current_curator_or_admin_user), | ||
| user: User | None = Depends(current_curator_or_admin_user), |
There was a problem hiding this comment.
logic: The 'user' parameter is marked as nullable (User | None) but current_curator_or_admin_user should never return None. This could cause confusion.
There was a problem hiding this comment.
greptile is wrong and we should probably make sure it's User | None everywhere
| "onyx.db.user_group", "validate_object_creation_for_user", None | ||
| )( |
There was a problem hiding this comment.
style: Function name 'validate_object_creation_for_user' is misleading since it's being used for deletion validation
There was a problem hiding this comment.
might be worth leaving a comment explaining
backend/ee/onyx/db/user_group.py
Outdated
| object_is_public: bool | None = None, | ||
| object_is_perm_sync: bool | None = None, | ||
| object_is_owned_by_user: bool | None = None, | ||
| object_is_new: bool | None = None, |
There was a problem hiding this comment.
Imo better to make all these default to False unless we truly need a ternary value
| def patch_document_set( | ||
| document_set_update_request: DocumentSetUpdateRequest, | ||
| user: User = Depends(current_curator_or_admin_user), | ||
| user: User | None = Depends(current_curator_or_admin_user), |
There was a problem hiding this comment.
greptile is wrong and we should probably make sure it's User | None everywhere
| "onyx.db.user_group", "validate_object_creation_for_user", None | ||
| )( |
There was a problem hiding this comment.
might be worth leaving a comment explaining
| ) { | ||
| formikProps.setFieldValue("authentication_method", authMethod); | ||
| } | ||
| console.log(formikProps.isSubmitting); |
…app#4972) * Allow curators to create public connectors / document sets * Address EL comments
Description
Fixes https://linear.app/danswer/issue/DAN-2154/allow-curators-to-create-public-connectors-document-sets
How Has This Been Tested?
Locally created connectors/document sets as a curator.
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.