Skip to content

Conversation

@taylordowns2000
Copy link
Member

@taylordowns2000 taylordowns2000 commented Dec 10, 2025

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

  1. Attempt to create a credential with the same external ID as another in the same project
  2. Attempt to update a credential to use the same external ID as another in the same project
  3. Attempt to add a credential with the same external ID to a project in which there is another credential that already has that external ID

AI Usage

Please disclose how you've used AI in this work (it's cool, we just want to know!):

  • Code generation (copilot but not intellisense)
  • Learning or fact checking
  • Strategy / design
  • Optimisation / refactoring
  • Translation / spellchecking / doc gen
  • Other
  • I have not used AI

You can read more details in our Responsible AI Policy

Pre-submission checklist

  • I have performed a self-review of my code.
  • I have implemented and tested all related authorization policies. (e.g., :owner, :admin, :editor, :viewer)
  • I have updated the changelog.
  • I have ticked a box in "AI usage" in this PR

@github-project-automation github-project-automation bot moved this to New Issues in v2 Dec 10, 2025
@taylordowns2000 taylordowns2000 marked this pull request as ready for review December 10, 2025 15:04
@taylordowns2000 taylordowns2000 requested review from elias-ba and stuartc and removed request for stuartc December 10, 2025 15:04
@codecov
Copy link

codecov bot commented Dec 10, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.17%. Comparing base (baf3ae2) to head (4417bb0).

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment on lines +53 to +55
|> unique_constraint([:external_id, :user_id],
message: "you have another credential with the external ID"
)
Copy link
Contributor

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),
Copy link
Contributor

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
Copy link
Contributor

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?

@github-project-automation github-project-automation bot moved this from New Issues to In review in v2 Dec 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In review

Development

Successfully merging this pull request may close these issues.

Credentials can have the same external id

3 participants