-
Notifications
You must be signed in to change notification settings - Fork 2k
perm sync validation framework #4958
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
Implements a new permission sync validation framework that enables connectors to validate their permission sync capabilities during CC pair setup and credential swapping.
- Added
validate_perm_sync
method toBaseConnector
inbackend/onyx/connectors/interfaces.py
allowing connectors to implement custom validation logic - Added
access_type
parameter to credential-related functions across the stack to enforce proper permission validation during CC pair creation and modification - Modified
CredentialSwapRequest
model inbackend/onyx/server/documents/models.py
to include requiredaccess_type
field for validation - Updated frontend components to properly pass and handle
accessType
during credential operations, maintaining type safety - Standardized permission validation across credential creation, modification, and swapping flows
11 files reviewed, 2 comments
Edit PR Review Bot Settings | Greptile
validate_ccpair_for_user( | ||
connector_id=connector_id, | ||
credential_id=credential_id, | ||
access_type=connector_data.access_type, | ||
db_session=db_session, | ||
) | ||
response = add_credential_to_connector( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: New access_type validation might throw ConnectorValidationError - add error handling here similar to line 990
if (onSwap && attachedConnector) { | ||
onSwap(selectedCredential!, attachedConnector.id); | ||
onSwap(selectedCredential!, attachedConnector.id, accessType); | ||
if (close) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: non-null assertion (!) used here could be replaced with a proper type guard to ensure type safety
960739c
to
bfdcc5c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable to me!
* perm synce validation framework * frontend fixes * validate perm sync when getting runner * attempt to fix integration tests * added new file * oops * skipping salesforce test due to creds * add todo
Description
Addresses https://linear.app/danswer/issue/DAN-2149/permission-sync-validation-framework
Adds a new validation function to connectors that connector implementers can implement. The validation function is called for all CC pairs set up to use perm sync
How Has This Been Tested?
n/a
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.