-
Notifications
You must be signed in to change notification settings - Fork 61
Fix keychain credential :external_id bug
#4177
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4177 +/- ##
==========================================
+ Coverage 89.16% 89.17% +0.01%
==========================================
Files 425 425
Lines 19733 19757 +24
==========================================
+ Hits 17594 17618 +24
Misses 2139 2139 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| |> unique_constraint([:external_id, :user_id], | ||
| message: "you have another credential with the external ID" | ||
| ) |
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.
This constraint enforces uniqueness per-user, but the bug we're fixing requires uniqueness per-project, right ? Also, this needs a corresponding database index/migration to actually work. I scanned a little bit the migrations and the changes in this PR but I haven't seen one. Without a db migration for the constraint, this probably won't work.
| with :ok <- validate_credential_bodies(credential_bodies, attrs), | ||
| changeset <- change_credential(%Credential{}, attrs) do | ||
| changeset <- change_credential(%Credential{}, attrs), | ||
| changeset <- validate_external_id_uniqueness(changeset), |
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.
This validation here runs outside the multi and transaction. This means there are high chances of a race condition. In fact, two concurrent requests could both pass validation before either inserts. This flow for example:
1. Request A: checks DB → no conflict
2. Request B: checks DB → no conflict
3. Request A: inserts credential with external_id "foo"
4. Request B: inserts credential with external_id "foo" → duplicate!
Now this may be very rare situation but it can happen, and when it happens it will break things.
We should move this validation inside the Multi so the check and insert happen in the same transaction.
| end | ||
| end | ||
|
|
||
| describe "external_id uniqueness validation" do |
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.
Nice test documenting the failure mode for legacy data! But this makes me think, users in prod might already have duplicates in their database, right? They'd hit this Ecto.MultipleResultsError at runtime. Maybe worth a follow-up issue to add a migration or mix task that detects and reports existing duplicates so admins know to clean them up?
This PR fixes #4170 by adding validation on credential create or update (including project_credential addition) that prevents two different credentials that are both shared in the same project from having the same external ID.
The implementation was based on the Keychain Credential feature from OpenFn v1.
Validation steps
AI Usage
Please disclose how you've used AI in this work (it's cool, we just want to know!):
You can read more details in our Responsible AI Policy
Pre-submission checklist
:owner,:admin,:editor,:viewer)