Fix keychain credential :external_id bug

CHANGELOG.md

@@ -24,6 +24,8 @@ 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)
 - Fix AI Assistant disclaimer not persisting after acceptance
   [#4158](https://github.com/OpenFn/lightning/issues/4158)
 - Credential form now shows inline validation errors for JSON schema and raw

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()
@@ -921,6 +925,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.
 

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_\- ]*$/,

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