diff --git a/CHANGELOG.md b/CHANGELOG.md index d6b8502074..186535ce8e 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/lib/lightning/credentials.ex b/lib/lightning/credentials.ex index f922c43327..7b059559f8 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() @@ -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. 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_\- ]*$/, 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 07b16d1335..408747d39a 100644 --- a/test/lightning/credentials_test.exs +++ b/test/lightning/credentials_test.exs @@ -2180,4 +2180,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