From 09111b9896d4a4354dfad01e69431c6590ec0a12 Mon Sep 17 00:00:00 2001 From: Taylor Downs Date: Wed, 10 Dec 2025 12:54:12 +0100 Subject: [PATCH 1/3] first add per-user unique constraint --- lib/lightning/credentials/credential.ex | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/lightning/credentials/credential.ex b/lib/lightning/credentials/credential.ex index ac02070ae3..5de48f594b 100644 --- a/lib/lightning/credentials/credential.ex +++ b/lib/lightning/credentials/credential.ex @@ -50,6 +50,9 @@ defmodule Lightning.Credentials.Credential do |> unique_constraint([:name, :user_id], message: "you have another credential with the same name" ) + |> unique_constraint([:external_id, :user_id], + message: "you have another credential with the external ID" + ) |> assoc_constraint(:user) |> assoc_constraint(:oauth_client) |> validate_format(:name, ~r/^[a-zA-Z0-9_\- ]*$/, From de171f44b5fa2395d69b72711d87facc35b064af Mon Sep 17 00:00:00 2001 From: Taylor Downs Date: Wed, 10 Dec 2025 15:58:00 +0100 Subject: [PATCH 2/3] close #4170 --- lib/lightning/credentials.ex | 96 ++++++++- .../credentials/keychain_credential_test.exs | 68 ++++++ test/lightning/credentials_test.exs | 198 ++++++++++++++++++ 3 files changed, 360 insertions(+), 2 deletions(-) diff --git a/lib/lightning/credentials.ex b/lib/lightning/credentials.ex index 77873709be..a9dba8dba7 100644 --- a/lib/lightning/credentials.ex +++ b/lib/lightning/credentials.ex @@ -203,7 +203,9 @@ defmodule Lightning.Credentials do credential_bodies = get_credential_bodies(attrs) 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), + true <- changeset.valid? || {:error, %{changeset | action: :insert}} do build_create_multi(changeset, credential_bodies) |> derive_events(changeset) |> Repo.transaction() @@ -266,7 +268,9 @@ defmodule Lightning.Credentials do attrs, credential.schema ), - changeset <- change_credential(credential, attrs) do + changeset <- change_credential(credential, attrs), + changeset <- validate_external_id_uniqueness(changeset), + true <- changeset.valid? || {:error, %{changeset | action: :update}} do build_update_multi(credential, changeset, credential_bodies) |> derive_events(changeset) |> Repo.transaction() @@ -892,6 +896,94 @@ defmodule Lightning.Credentials do Credential.changeset(credential, attrs |> normalize_keys()) end + @doc """ + Validates that a credential's external_id is unique within each project + it's associated with. + + This validation ensures that no two credentials in the same project share + the same external_id, which is required for keychain credential matching. + + ## Parameters + * `changeset` - A credential changeset with project_credentials already cast + + ## Returns + * The changeset, potentially with an error added to :external_id + """ + @spec validate_external_id_uniqueness(Ecto.Changeset.t()) :: Ecto.Changeset.t() + def validate_external_id_uniqueness(changeset) do + external_id = Ecto.Changeset.get_field(changeset, :external_id) + credential_id = Ecto.Changeset.get_field(changeset, :id) + + # Get project_ids from the changeset + # If project_credentials is being changed, use that; otherwise query existing + project_ids = get_project_ids_for_validation(changeset, credential_id) + + if is_nil(external_id) or external_id == "" or project_ids == [] do + changeset + else + if external_id_conflict_exists?(external_id, project_ids, credential_id) do + Ecto.Changeset.add_error( + changeset, + :external_id, + "There is already a credential in the same project that uses this External ID for keychain matching, please choose another." + ) + else + changeset + end + end + end + + defp get_project_ids_for_validation(changeset, credential_id) do + # Check if project_credentials is being changed in this update + case Ecto.Changeset.get_change(changeset, :project_credentials) do + nil -> + # No change to project associations - query existing ones from DB + if credential_id do + from(pc in Lightning.Projects.ProjectCredential, + where: pc.credential_id == ^credential_id, + select: pc.project_id + ) + |> Repo.all() + else + [] + end + + project_credential_changesets -> + # Project associations are being changed - use the changeset data + project_credential_changesets + |> Enum.reject(fn pc_changeset -> + # Reject entries marked for deletion + Ecto.Changeset.get_field(pc_changeset, :delete, false) + end) + |> Enum.map(fn pc_changeset -> + Ecto.Changeset.get_field(pc_changeset, :project_id) + end) + |> Enum.reject(&is_nil/1) + end + end + + defp external_id_conflict_exists?( + external_id, + project_ids, + exclude_credential_id + ) do + query = + from c in Credential, + join: pc in assoc(c, :project_credentials), + where: c.external_id == ^external_id, + where: pc.project_id in ^project_ids, + select: c.id + + query = + if exclude_credential_id do + from [c, _pc] in query, where: c.id != ^exclude_credential_id + else + query + end + + Repo.exists?(query) + end + @doc """ Extracts sensitive values from a credential body map. diff --git a/test/lightning/credentials/keychain_credential_test.exs b/test/lightning/credentials/keychain_credential_test.exs index 9994016a45..022bf29685 100644 --- a/test/lightning/credentials/keychain_credential_test.exs +++ b/test/lightning/credentials/keychain_credential_test.exs @@ -411,4 +411,72 @@ defmodule Lightning.Credentials.KeychainCredentialTest do assert loaded_keychain.default_credential.id == credential.id end end + + describe "external_id uniqueness validation" do + test "keychain resolver raises error when duplicate external_ids exist in same project" do + # This test documents the behavior when duplicate external_ids exist + # (which should be prevented by validation, but could occur from legacy data) + user = insert(:user) + project = insert(:project, project_users: [%{user: user}]) + + # Create credentials directly with insert to bypass validation + # (simulating legacy data or direct DB manipulation) + credential_a = + insert(:credential, + name: "Credential A", + schema: "raw", + external_id: "duplicate-id", + user: user + ) + |> with_body(%{name: "main", body: %{"key" => "value_a"}}) + + credential_b = + insert(:credential, + name: "Credential B", + schema: "raw", + external_id: "duplicate-id", + user: user + ) + |> with_body(%{name: "main", body: %{"key" => "value_b"}}) + + # Associate both credentials with the same project + insert(:project_credential, project: project, credential: credential_a) + insert(:project_credential, project: project, credential: credential_b) + + # Create a keychain credential + keychain_credential = + insert(:keychain_credential, + name: "Test Keychain", + path: "$.user_id", + project: project, + created_by: user + ) + + # Create workflow with the keychain credential + %{jobs: [job]} = + workflow = + build(:workflow, project: project) + |> with_job(%{keychain_credential: keychain_credential}) + |> insert() + + # Create a run with dataclip that matches the duplicate external_id + %{runs: [run]} = + insert(:workorder, workflow: workflow) + |> with_run(%{ + dataclip: + build(:dataclip, %{ + body: %{"user_id" => "duplicate-id"} + }), + starting_job: job + }) + + # The resolver should raise an error because Repo.one() finds multiple results + assert_raise Ecto.MultipleResultsError, fn -> + Lightning.Credentials.Resolver.resolve_credential( + run, + keychain_credential.id + ) + end + end + end end diff --git a/test/lightning/credentials_test.exs b/test/lightning/credentials_test.exs index d062fe0483..64e71d8d00 100644 --- a/test/lightning/credentials_test.exs +++ b/test/lightning/credentials_test.exs @@ -2139,4 +2139,202 @@ defmodule Lightning.CredentialsTest do end end end + + describe "external_id uniqueness per project" do + test "creating two credentials with same external_id in same project fails" do + user = insert(:user) + project = insert(:project, project_users: [%{user: user}]) + + # Create first credential with external_id + {:ok, _credential1} = + Credentials.create_credential(%{ + name: "Credential 1", + user_id: user.id, + external_id: "salesforce-prod", + schema: "raw", + project_credentials: [%{project_id: project.id}], + credential_bodies: [%{name: "production", body: %{}}] + }) + + # Try to create second credential with same external_id in same project + {:error, changeset} = + Credentials.create_credential(%{ + name: "Credential 2", + user_id: user.id, + external_id: "salesforce-prod", + schema: "raw", + project_credentials: [%{project_id: project.id}], + credential_bodies: [%{name: "production", body: %{}}] + }) + + assert %{ + external_id: [ + "There is already a credential in the same project that uses this External ID for keychain matching, please choose another." + ] + } = errors_on(changeset) + end + + test "creating credentials with same external_id in different projects succeeds" do + user = insert(:user) + project1 = insert(:project, project_users: [%{user: user}]) + project2 = insert(:project, project_users: [%{user: user}]) + + # Create first credential in project1 + {:ok, _credential1} = + Credentials.create_credential(%{ + name: "Credential 1", + user_id: user.id, + external_id: "salesforce-prod", + schema: "raw", + project_credentials: [%{project_id: project1.id}], + credential_bodies: [%{name: "production", body: %{}}] + }) + + # Create second credential with same external_id in different project + {:ok, credential2} = + Credentials.create_credential(%{ + name: "Credential 2", + user_id: user.id, + external_id: "salesforce-prod", + schema: "raw", + project_credentials: [%{project_id: project2.id}], + credential_bodies: [%{name: "production", body: %{}}] + }) + + assert credential2.external_id == "salesforce-prod" + end + + test "adding credential to project where another has same external_id fails" do + user = insert(:user) + project = insert(:project, project_users: [%{user: user}]) + + # Create first credential with external_id in project + {:ok, _credential1} = + Credentials.create_credential(%{ + name: "Credential 1", + user_id: user.id, + external_id: "salesforce-prod", + schema: "raw", + project_credentials: [%{project_id: project.id}], + credential_bodies: [%{name: "production", body: %{}}] + }) + + # Create second credential without project association + {:ok, credential2} = + Credentials.create_credential(%{ + name: "Credential 2", + user_id: user.id, + external_id: "salesforce-prod", + schema: "raw", + project_credentials: [], + credential_bodies: [%{name: "production", body: %{}}] + }) + + # Try to add second credential to same project + {:error, changeset} = + Credentials.update_credential(credential2, %{ + project_credentials: [%{project_id: project.id}] + }) + + assert %{ + external_id: [ + "There is already a credential in the same project that uses this External ID for keychain matching, please choose another." + ] + } = errors_on(changeset) + end + + test "credential with nil external_id does not trigger validation error" do + user = insert(:user) + project = insert(:project, project_users: [%{user: user}]) + + # Create credential without external_id + {:ok, credential1} = + Credentials.create_credential(%{ + name: "Credential 1", + user_id: user.id, + external_id: nil, + schema: "raw", + project_credentials: [%{project_id: project.id}], + credential_bodies: [%{name: "production", body: %{}}] + }) + + # Create another credential without external_id in same project + {:ok, credential2} = + Credentials.create_credential(%{ + name: "Credential 2", + user_id: user.id, + external_id: nil, + schema: "raw", + project_credentials: [%{project_id: project.id}], + credential_bodies: [%{name: "production", body: %{}}] + }) + + assert is_nil(credential1.external_id) + assert is_nil(credential2.external_id) + end + + test "updating a credential's own fields does not trigger false positive" do + user = insert(:user) + project = insert(:project, project_users: [%{user: user}]) + + # Create credential with external_id + {:ok, credential} = + Credentials.create_credential(%{ + name: "Original Name", + user_id: user.id, + external_id: "salesforce-prod", + schema: "raw", + project_credentials: [%{project_id: project.id}], + credential_bodies: [%{name: "production", body: %{}}] + }) + + # Preload associations for update + credential = Repo.preload(credential, :project_credentials) + + # Update the credential's name (should not conflict with itself) + {:ok, updated} = + Credentials.update_credential(credential, %{ + name: "Updated Name", + project_credentials: + Enum.map(credential.project_credentials, fn pc -> + %{id: pc.id, project_id: pc.project_id} + end), + credential_bodies: [%{name: "production", body: %{}}] + }) + + assert updated.name == "Updated Name" + assert updated.external_id == "salesforce-prod" + end + + test "different users can have same external_id in different projects" do + user1 = insert(:user) + user2 = insert(:user) + project1 = insert(:project, project_users: [%{user: user1}]) + project2 = insert(:project, project_users: [%{user: user2}]) + + # User1 creates credential with external_id in project1 + {:ok, _credential1} = + Credentials.create_credential(%{ + name: "User1 Credential", + user_id: user1.id, + external_id: "salesforce-prod", + schema: "raw", + project_credentials: [%{project_id: project1.id}], + credential_bodies: [%{name: "production", body: %{}}] + }) + + # User2 creates credential with same external_id in project2 + {:ok, credential2} = + Credentials.create_credential(%{ + name: "User2 Credential", + user_id: user2.id, + external_id: "salesforce-prod", + schema: "raw", + project_credentials: [%{project_id: project2.id}], + credential_bodies: [%{name: "production", body: %{}}] + }) + + assert credential2.external_id == "salesforce-prod" + end + end end From 53683f8d9ed8ca82eefa6a718cfa39a23c76229b Mon Sep 17 00:00:00 2001 From: Taylor Downs Date: Wed, 10 Dec 2025 16:05:43 +0100 Subject: [PATCH 3/3] cl --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index fce59acdec..282a767690 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,9 @@ and this project adheres to ### Fixed +- Fixed bug where two credentials could have the same external ID in the same + project [#4177](https://github.com/OpenFn/lightning/pull/4177) + ## [2.15.0-pre4] - 2025-12-08 ### Added