From d0f0de0f8d689374053f16dc9a7aafa1a5ada74f Mon Sep 17 00:00:00 2001 From: Brian Manifold Date: Wed, 26 Feb 2025 09:05:34 -0800 Subject: [PATCH] refactor(portal): Allow breaking changes in Resources/Policies (#8267) Why: * Rather than using a persistent_id field in Resources/Policies, it was decided that we should allow "breaking changes" to these entities. This means that Resources/Policies will now be able to update all fields on the schema without changing the primary key ID of the entity. * This change will greatly help the API and Terraform provider development. @jamilbk, would you like me to put a migration in this PR to actually get rid of all of the existing soft deleted entities? @thomaseizinger, I tagged you on this, because I wanted to make sure that these changes weren't going to break any expectations in the client and/or gateways. --------- Signed-off-by: Brian Manifold Co-authored-by: Jamil --- .../lib/api/controllers/policy_controller.ex | 5 +- .../api/controllers/resource_controller.ex | 5 +- .../apps/api/test/api/client/channel_test.exs | 4 +- .../api/test/api/gateway/channel_test.exs | 6 +- elixir/apps/domain/lib/domain/policies.ex | 26 ++-- .../lib/domain/policies/policy/changeset.ex | 29 ++--- elixir/apps/domain/lib/domain/repo.ex | 84 ++++--------- elixir/apps/domain/lib/domain/resources.ex | 36 ++---- .../domain/resources/resource/changeset.ex | 34 ++---- .../apps/domain/test/domain/policies_test.exs | 115 ++++++------------ .../domain/test/domain/resources_test.exs | 103 ++++++---------- .../domain/test/support/fixtures/policies.ex | 23 ---- elixir/apps/web/lib/web/live/policies/edit.ex | 10 +- .../apps/web/lib/web/live/resources/edit.ex | 19 +-- elixir/apps/web/lib/web/live/sites/index.ex | 2 +- .../web/test/web/live/policies/edit_test.exs | 12 +- .../web/test/web/live/policies/show_test.exs | 52 -------- .../web/test/web/live/resources/edit_test.exs | 24 ++-- 18 files changed, 160 insertions(+), 429 deletions(-) diff --git a/elixir/apps/api/lib/api/controllers/policy_controller.ex b/elixir/apps/api/lib/api/controllers/policy_controller.ex index 32e2da09c..4acf772ee 100644 --- a/elixir/apps/api/lib/api/controllers/policy_controller.ex +++ b/elixir/apps/api/lib/api/controllers/policy_controller.ex @@ -92,13 +92,10 @@ defmodule API.PolicyController do subject = conn.assigns.subject with {:ok, policy} <- Policies.fetch_policy_by_id_or_persistent_id(id, subject) do - case Policies.update_or_replace_policy(policy, params, subject) do + case Policies.update_policy(policy, params, subject) do {:updated, updated_policy} -> render(conn, :show, policy: updated_policy) - {:replaced, _replaced_policy, replacement_policy} -> - render(conn, :show, policy: replacement_policy) - {:error, reason} -> {:error, reason} end diff --git a/elixir/apps/api/lib/api/controllers/resource_controller.ex b/elixir/apps/api/lib/api/controllers/resource_controller.ex index 74ef30e05..677f8918d 100644 --- a/elixir/apps/api/lib/api/controllers/resource_controller.ex +++ b/elixir/apps/api/lib/api/controllers/resource_controller.ex @@ -93,13 +93,10 @@ defmodule API.ResourceController do attrs = set_param_defaults(params) with {:ok, resource} <- Resources.fetch_active_resource_by_id_or_persistent_id(id, subject) do - case Resources.update_or_replace_resource(resource, attrs, subject) do + case Resources.update_resource(resource, attrs, subject) do {:updated, updated_resource} -> render(conn, :show, resource: updated_resource) - {:replaced, _updated_resource, created_resource} -> - render(conn, :show, resource: created_resource) - {:error, reason} -> {:error, reason} end diff --git a/elixir/apps/api/test/api/client/channel_test.exs b/elixir/apps/api/test/api/client/channel_test.exs index deaaa776c..ca1615a8f 100644 --- a/elixir/apps/api/test/api/client/channel_test.exs +++ b/elixir/apps/api/test/api/client/channel_test.exs @@ -498,7 +498,7 @@ defmodule API.Client.ChannelTest do assert_push "init", %{} {:updated, _resource} = - Domain.Resources.update_or_replace_resource(resource, %{name: "foobar"}, subject) + Domain.Resources.update_resource(resource, %{name: "foobar"}, subject) assert_push "resource_created_or_updated", %{} end @@ -1799,7 +1799,7 @@ defmodule API.Client.ChannelTest do ) {:updated, resource} = - Domain.Resources.update_or_replace_resource( + Domain.Resources.update_resource( resource, %{connections: [%{gateway_group_id: gateway_group.id}]}, subject diff --git a/elixir/apps/api/test/api/gateway/channel_test.exs b/elixir/apps/api/test/api/gateway/channel_test.exs index cd5e70140..ec85b161f 100644 --- a/elixir/apps/api/test/api/gateway/channel_test.exs +++ b/elixir/apps/api/test/api/gateway/channel_test.exs @@ -269,7 +269,7 @@ defmodule API.Gateway.ChannelTest do assert_push "allow_access", %{} {:updated, resource} = - Domain.Resources.update_or_replace_resource( + Domain.Resources.update_resource( resource, %{"name" => Ecto.UUID.generate()}, subject @@ -664,7 +664,7 @@ defmodule API.Gateway.ChannelTest do assert_push "request_connection", %{}, 200 {:updated, resource} = - Domain.Resources.update_or_replace_resource( + Domain.Resources.update_resource( resource, %{"name" => Ecto.UUID.generate()}, subject @@ -884,7 +884,7 @@ defmodule API.Gateway.ChannelTest do assert_push "authorize_flow", %{} {:updated, resource} = - Domain.Resources.update_or_replace_resource( + Domain.Resources.update_resource( resource, %{"name" => Ecto.UUID.generate()}, subject diff --git a/elixir/apps/domain/lib/domain/policies.ex b/elixir/apps/domain/lib/domain/policies.ex index 86a5780c6..5b6f6e072 100644 --- a/elixir/apps/domain/lib/domain/policies.ex +++ b/elixir/apps/domain/lib/domain/policies.ex @@ -79,30 +79,24 @@ defmodule Domain.Policies do end end - def change_policy(%Policy{} = policy, attrs, %Auth.Subject{} = subject) do - case Policy.Changeset.update_or_replace(policy, attrs, subject) do - {update_changeset, nil} -> update_changeset - {_update_changeset, create_changeset} -> create_changeset - end + def change_policy(%Policy{} = policy, attrs) do + {update_changeset, _breaking_update} = Policy.Changeset.update(policy, attrs) + update_changeset end - def update_or_replace_policy(%Policy{} = policy, attrs, %Auth.Subject{} = subject) do + def update_policy(%Policy{} = policy, attrs, %Auth.Subject{} = subject) do with :ok <- Auth.ensure_has_permissions(subject, Authorizer.manage_policies_permission()), :ok <- ensure_has_access_to(subject, policy) do Policy.Query.not_deleted() |> Policy.Query.by_id(policy.id) |> Authorizer.for_subject(subject) - |> Repo.fetch_and_update_or_replace(Policy.Query, - with: &Policy.Changeset.update_or_replace(&1, attrs, subject), - on_replace: fn repo, updated_policy, created_policy -> - Ecto.Changeset.change(updated_policy, replaced_by_policy_id: created_policy.id) - |> repo.update() - end, + |> Repo.fetch_and_update_breakable(Policy.Query, + with: &Policy.Changeset.update(&1, attrs), after_update_commit: &broadcast_policy_events(:update, &1), - after_replace_commit: fn {replaced_policy, created_policy}, _changesets -> - {:ok, _flows} = Flows.expire_flows_for(replaced_policy, subject) - :ok = broadcast_policy_events(:delete, replaced_policy) - :ok = broadcast_policy_events(:create, created_policy) + after_breaking_update_commit: fn updated_policy, _changeset -> + {:ok, _flows} = Flows.expire_flows_for(updated_policy, subject) + :ok = broadcast_policy_events(:delete, updated_policy) + :ok = broadcast_policy_events(:create, updated_policy) end ) end diff --git a/elixir/apps/domain/lib/domain/policies/policy/changeset.ex b/elixir/apps/domain/lib/domain/policies/policy/changeset.ex index a61b6260c..f859498b7 100644 --- a/elixir/apps/domain/lib/domain/policies/policy/changeset.ex +++ b/elixir/apps/domain/lib/domain/policies/policy/changeset.ex @@ -19,38 +19,23 @@ defmodule Domain.Policies.Policy.Changeset do |> put_change(:persistent_id, Ecto.UUID.generate()) end - def update_or_replace(%Policy{} = policy, attrs, %Auth.Subject{} = subject) do + def update(%Policy{} = policy, attrs) do policy |> cast(attrs, @update_fields) |> validate_required(@required_fields) |> cast_embed(:conditions, with: &Domain.Policies.Condition.Changeset.changeset/3) |> changeset() - |> maybe_replace(policy, subject) + |> maybe_breaking_update() end - defp maybe_replace(%{valid?: false} = changeset, _policy, _subject), - do: {changeset, nil} + defp maybe_breaking_update(%{valid?: false} = changeset), + do: {changeset, false} - defp maybe_replace(changeset, policy, subject) do + defp maybe_breaking_update(changeset) do if any_field_changed?(changeset, @replace_fields) do - new_changeset = - changeset - |> apply_changes() - |> Map.from_struct() - |> Map.update(:conditions, [], fn conditions -> - Enum.map(conditions, &Map.from_struct/1) - end) - |> create(subject) - |> put_change(:persistent_id, policy.persistent_id) - |> put_change(:disabled_at, if(policy.disabled_at, do: DateTime.utc_now())) - - changeset = - policy - |> change(deleted_at: DateTime.utc_now()) - - {changeset, new_changeset} + {changeset, true} else - {changeset, nil} + {changeset, false} end end diff --git a/elixir/apps/domain/lib/domain/repo.ex b/elixir/apps/domain/lib/domain/repo.ex index 907d6481b..a36ec6b7d 100644 --- a/elixir/apps/domain/lib/domain/repo.ex +++ b/elixir/apps/domain/lib/domain/repo.ex @@ -158,72 +158,54 @@ defmodule Domain.Repo do @typedoc """ A callback which is executed after the transaction is committed that - has replaced a record in the database. + has a breaking change to the record in the database. The callback is either a function that takes the schema as an argument or a function that takes the schema and the changeset as arguments. It must return `:ok`. """ - @type replace_after_commit :: ({replaced_schema :: term(), created_schema :: term()}, - {update_changeset :: Ecto.Changeset.t(), - create_changeset :: Ecto.Changeset.t()} -> - :ok) + @type break_after_commit :: (updated_schema :: term(), update_changeset :: Ecto.Changeset.t() -> + :ok) @typedoc """ - A callback which takes a schema and returns a changeset that is then used to update the schema and either `nil` if update needs to be performed or - another changeset that is then used to create a new schema. + A callback which takes a schema and returns a changeset that is then used to update the schema and a boolean indicating whether the update is a breaking change. """ - @type fetch_update_or_replace_changeset_fun :: (term() -> - {update_changeset :: Ecto.Changeset.t(), - create_replacement_changeset :: - Ecto.Changeset.t() | nil}) - - @typedoc """ - A callback which is executed after the transaction is committed that - has replaced a record in the database. It allows to set an additional - field on the updated schema (like reference to a replacement schema). - - The function will be executed within the same transaction as the replace - operation and must return `{:ok, updated_schema}` or `{:error, reason}`. - """ - @type on_replace :: (repo :: Ecto.Repo.t(), - updated_schema :: Ecto.Schema.t(), - replaced_schema :: Ecto.Schema.t() -> - :ok) + @type fetch_update_changeset_fun :: (term() -> + {update_changeset :: Ecto.Changeset.t(), + breaking_change :: true | false}) @doc """ Uses query to fetch a single result from the database, locks it for update and - then either updates or replaces it using a changesets within a database transaction. + then updates it using a changesets within a database transaction. Different callbacks can + be used for a breaking change to the record. """ - @spec fetch_and_update_or_replace( + @spec fetch_and_update_breakable( queryable :: Ecto.Queryable.t(), query_module :: module(), opts :: [ - {:with, fetch_update_or_replace_changeset_fun()} + {:with, fetch_update_changeset_fun()} | {:preload, term()} | {:filter, Domain.Repo.Filter.filters()} | {:after_update_commit, update_after_commit() | [update_after_commit()]} - | {:after_replace_commit, replace_after_commit() | [replace_after_commit()]} - | {:on_replace, on_replace()} + | {:after_breaking_update_commit, break_after_commit() | [break_after_commit()]} ] | Keyword.t() ) :: {:updated, Ecto.Schema.t()} - | {:replaced, Ecto.Schema.t(), Ecto.Schema.t()} + | {:breaking_update, Ecto.Schema.t(), Ecto.Schema.t()} | {:error, :not_found} | {:error, {:unknown_filter, metadata :: Keyword.t()}} | {:error, {:invalid_type, metadata :: Keyword.t()}} | {:error, {:invalid_value, metadata :: Keyword.t()}} | {:error, Ecto.Changeset.t()} | {:error, term()} - def fetch_and_update_or_replace(queryable, query_module, opts) do + def fetch_and_update_breakable(queryable, query_module, opts) do {preload, opts} = Keyword.pop(opts, :preload, []) {filter, opts} = Keyword.pop(opts, :filter, []) {after_update_commit, opts} = Keyword.pop(opts, :after_update_commit, []) - {after_replace_commit, opts} = Keyword.pop(opts, :after_replace_commit, []) - {on_replace, opts} = Keyword.pop(opts, :on_replace, []) + {after_breaking_update_commit, opts} = Keyword.pop(opts, :after_breaking_update_commit, []) {changeset_fun, transaction_opts} = Keyword.pop!(opts, :with) with {:ok, queryable} <- Filter.filter(queryable, query_module, filter) do @@ -232,43 +214,25 @@ defmodule Domain.Repo do _effects_so_far -> Ecto.Query.lock(queryable, "FOR NO KEY UPDATE") end) - |> Ecto.Multi.run(:changesets, fn - _repo, %{fetch_and_lock: schema} -> - {%Ecto.Changeset{} = update_changeset, replace_changeset_or_nil} = - changeset_fun.(schema) + |> Ecto.Multi.run(:changeset, fn _repo, %{fetch_and_lock: schema} -> + {%Ecto.Changeset{} = update_changeset, breaking} = + changeset_fun.(schema) - {:ok, {update_changeset, replace_changeset_or_nil}} + {:ok, {update_changeset, breaking}} end) |> Ecto.Multi.update(:update, fn - %{changesets: {update_changeset, _replace_changeset_or_nil}} -> + %{changeset: {update_changeset, _breaking}} -> update_changeset end) - |> Ecto.Multi.run(:maybe_create, fn - _repo, %{changesets: {_update_changeset, nil}} -> - {:ok, nil} - - repo, %{changesets: {_update_changeset, replace_changeset_or_nil}} -> - repo.insert(replace_changeset_or_nil) - end) - |> Ecto.Multi.run(:replace, fn - _repo, %{update: _updated, maybe_create: nil} -> - {:ok, nil} - - repo, %{update: updated, maybe_create: created} -> - on_replace.(repo, updated, created) - end) |> transaction(transaction_opts) |> case do - {:ok, %{update: updated, maybe_create: nil, changesets: {update_changeset, nil}}} -> + {:ok, %{update: updated, changeset: {update_changeset, false}}} -> :ok = execute_after_commit(updated, update_changeset, after_update_commit) {:updated, execute_preloads(updated, query_module, preload)} - {:ok, %{replace: replaced, maybe_create: created, changesets: changesets}} -> - :ok = execute_after_commit({replaced, created}, changesets, after_replace_commit) - {:replaced, replaced, execute_preloads(created, query_module, preload)} - - {:error, :maybe_create, changeset, _changes_so_far} -> - {:error, changeset} + {:ok, %{update: updated, changeset: {update_changeset, true}}} -> + :ok = execute_after_commit(updated, update_changeset, after_breaking_update_commit) + {:updated, execute_preloads(updated, query_module, preload)} {:error, :fetch_and_lock, reason, _changes_so_far} -> {:error, reason} diff --git a/elixir/apps/domain/lib/domain/resources.ex b/elixir/apps/domain/lib/domain/resources.ex index bdac1b779..9a5fe0a89 100644 --- a/elixir/apps/domain/lib/domain/resources.ex +++ b/elixir/apps/domain/lib/domain/resources.ex @@ -252,26 +252,21 @@ defmodule Domain.Resources do end def change_resource(%Resource{} = resource, attrs \\ %{}, %Auth.Subject{} = subject) do - case Resource.Changeset.update_or_replace(resource, attrs, subject) do - {update_changeset, nil} -> update_changeset - {_update_changeset, create_changeset} -> create_changeset + case Resource.Changeset.update(resource, attrs, subject) do + {update_changeset, _} -> update_changeset end end - def update_or_replace_resource(%Resource{} = resource, attrs, %Auth.Subject{} = subject) do + def update_resource(%Resource{} = resource, attrs, %Auth.Subject{} = subject) do with :ok <- Auth.ensure_has_permissions(subject, Authorizer.manage_resources_permission()) do Resource.Query.not_deleted() |> Resource.Query.by_id(resource.id) |> Authorizer.for_subject(Resource, subject) - |> Repo.fetch_and_update_or_replace(Resource.Query, + |> Repo.fetch_and_update_breakable(Resource.Query, with: fn resource -> resource |> Repo.preload(:connections) - |> Resource.Changeset.update_or_replace(attrs, subject) - end, - on_replace: fn repo, updated_resource, created_resource -> - Ecto.Changeset.change(updated_resource, replaced_by_resource_id: created_resource.id) - |> repo.update() + |> Resource.Changeset.update(attrs, subject) end, after_update_commit: fn resource, changeset -> if Map.has_key?(changeset.changes, :connections) do @@ -280,22 +275,13 @@ defmodule Domain.Resources do broadcast_resource_events(:update, resource) end, - after_replace_commit: fn {replaced_resource, created_resource}, _changesets -> - replaced_resource = Repo.preload(replaced_resource, :policies) + after_breaking_update_commit: fn updated_resource, _changeset -> + # The :delete resource event broadcast is a no-op. + # This is used to reset the resource on the client and gateway in case filters, conditions, etc are changed. + {:ok, _flows} = Flows.expire_flows_for(updated_resource, subject) - :ok = - Enum.each(replaced_resource.policies, fn policy -> - {:replaced, _replaced_policy, _created_policy} = - Policies.update_or_replace_policy( - policy, - %{resource_id: created_resource.id}, - subject - ) - end) - - {:ok, _flows} = Flows.expire_flows_for(replaced_resource, subject) - :ok = broadcast_resource_events(:delete, replaced_resource) - :ok = broadcast_resource_events(:create, created_resource) + :ok = broadcast_resource_events(:delete, updated_resource) + :ok = broadcast_resource_events(:create, updated_resource) end ) end diff --git a/elixir/apps/domain/lib/domain/resources/resource/changeset.ex b/elixir/apps/domain/lib/domain/resources/resource/changeset.ex index 8ff82d829..f5250a0ca 100644 --- a/elixir/apps/domain/lib/domain/resources/resource/changeset.ex +++ b/elixir/apps/domain/lib/domain/resources/resource/changeset.ex @@ -209,7 +209,7 @@ defmodule Domain.Resources.Resource.Changeset do end end - def update_or_replace(%Resource{} = resource, attrs, %Auth.Subject{} = subject) do + def update(%Resource{} = resource, attrs, %Auth.Subject{} = subject) do resource |> cast(attrs, @update_fields) |> validate_required(@required_fields) @@ -218,36 +218,18 @@ defmodule Domain.Resources.Resource.Changeset do with: &Connection.Changeset.changeset(resource.account_id, &1, &2, subject), required: true ) - |> maybe_replace(resource, subject) + |> maybe_breaking_change() end - defp maybe_replace(%{valid?: false} = changeset, _policy, _subject), - do: {changeset, nil} + defp maybe_breaking_change(%{valid?: false} = changeset), + do: {changeset, false} - defp maybe_replace(changeset, resource, subject) do + # NOTE: Kept for backwards compatibility. + defp maybe_breaking_change(changeset) do if any_field_changed?(changeset, @replace_fields) do - resource = Domain.Repo.preload(resource, :account) - - new_attrs = - changeset - |> apply_changes() - |> Map.from_struct() - |> Map.update(:connections, [], fn connections -> - Enum.map(connections, &Map.from_struct/1) - end) - |> Map.update(:filters, [], fn filters -> - Enum.map(filters, &Map.from_struct/1) - end) - - new_changeset = - create(resource.account, new_attrs, subject) - |> put_change(:persistent_id, resource.persistent_id) - - changeset = delete(resource) - - {changeset, new_changeset} + {changeset, true} else - {changeset, nil} + {changeset, false} end end diff --git a/elixir/apps/domain/test/domain/policies_test.exs b/elixir/apps/domain/test/domain/policies_test.exs index a268d10c9..4e694fcfd 100644 --- a/elixir/apps/domain/test/domain/policies_test.exs +++ b/elixir/apps/domain/test/domain/policies_test.exs @@ -424,7 +424,7 @@ defmodule Domain.PoliciesTest do end end - describe "update_or_replace_policy/3" do + describe "update_policy/3" do setup context do policy = Fixtures.Policies.create_policy( @@ -443,20 +443,20 @@ defmodule Domain.PoliciesTest do end test "does nothing on empty params", %{policy: policy, subject: subject} do - assert {:updated, _policy} = update_or_replace_policy(policy, %{}, subject) + assert {:updated, _policy} = update_policy(policy, %{}, subject) end test "returns changeset error on invalid params", %{account: account, subject: subject} do policy = Fixtures.Policies.create_policy(account: account, subject: subject) attrs = %{description: String.duplicate("a", 1025)} - assert {:error, changeset} = update_or_replace_policy(policy, attrs, subject) + assert {:error, changeset} = update_policy(policy, attrs, subject) assert errors_on(changeset) == %{description: ["should be at most 1024 character(s)"]} end test "allows update to description", %{policy: policy, subject: subject} do attrs = %{description: "updated policy description"} - assert {:updated, updated_policy} = update_or_replace_policy(policy, attrs, subject) + assert {:updated, updated_policy} = update_policy(policy, attrs, subject) assert updated_policy.description == attrs.description end @@ -468,7 +468,7 @@ defmodule Domain.PoliciesTest do :ok = subscribe_to_events_for_account(account) attrs = %{description: "updated policy description"} - assert {:updated, policy} = update_or_replace_policy(policy, attrs, subject) + assert {:updated, policy} = update_policy(policy, attrs, subject) assert_receive {:update_policy, policy_id} assert policy_id == policy.id @@ -481,7 +481,7 @@ defmodule Domain.PoliciesTest do :ok = subscribe_to_events_for_policy(policy) attrs = %{description: "updated policy description"} - assert {:updated, updated_policy} = update_or_replace_policy(policy, attrs, subject) + assert {:updated, updated_policy} = update_policy(policy, attrs, subject) assert updated_policy.id == policy.id assert_receive {:update_policy, policy_id} @@ -495,13 +495,13 @@ defmodule Domain.PoliciesTest do :ok = subscribe_to_events_for_actor_group(policy.actor_group_id) attrs = %{description: "updated policy description"} - assert {:updated, _policy} = update_or_replace_policy(policy, attrs, subject) + assert {:updated, _policy} = update_policy(policy, attrs, subject) refute_receive {:allow_access, _policy_id, _actor_group_id, _resource_id} refute_receive {:reject_access, _policy_id, _actor_group_id, _resource_id} end - test "creates a replacement policy when resource_id is changed", %{ + test "updates a policy when resource_id is changed", %{ policy: policy, account: account, subject: subject @@ -510,24 +510,15 @@ defmodule Domain.PoliciesTest do attrs = %{resource_id: new_resource.id} - assert {:replaced, replaced_policy, replacement_policy} = - update_or_replace_policy(policy, attrs, subject) + assert {:updated, updated_policy} = update_policy(policy, attrs, subject) - assert replaced_policy.resource_id == policy.resource_id - assert replaced_policy.actor_group_id == policy.actor_group_id - assert replaced_policy.conditions == policy.conditions - assert not is_nil(replaced_policy.deleted_at) - assert replaced_policy.replaced_by_policy_id == replacement_policy.id - - assert replacement_policy.resource_id == new_resource.id - assert replacement_policy.actor_group_id == policy.actor_group_id - assert replacement_policy.conditions == policy.conditions - assert is_nil(replacement_policy.deleted_at) - - assert replaced_policy.persistent_id == replacement_policy.persistent_id + assert updated_policy.resource_id != policy.resource_id + assert updated_policy.resource_id == attrs[:resource_id] + assert updated_policy.actor_group_id == policy.actor_group_id + assert updated_policy.conditions == policy.conditions end - test "creates a replacement policy when actor_group_id is changed", %{ + test "updates policy when actor_group_id is changed", %{ policy: policy, account: account, subject: subject @@ -536,24 +527,17 @@ defmodule Domain.PoliciesTest do attrs = %{actor_group_id: new_actor_group.id} - assert {:replaced, replaced_policy, replacement_policy} = - update_or_replace_policy(policy, attrs, subject) + assert {:updated, updated_policy} = + update_policy(policy, attrs, subject) - assert replaced_policy.resource_id == policy.resource_id - assert replaced_policy.actor_group_id == policy.actor_group_id - assert replaced_policy.conditions == policy.conditions - assert not is_nil(replaced_policy.deleted_at) - assert replaced_policy.replaced_by_policy_id == replacement_policy.id - - assert replacement_policy.resource_id == policy.resource_id - assert replacement_policy.actor_group_id == new_actor_group.id - assert replacement_policy.conditions == policy.conditions - assert is_nil(replacement_policy.deleted_at) - - assert replaced_policy.persistent_id == replacement_policy.persistent_id + assert updated_policy.id == policy.id + assert updated_policy.resource_id == policy.resource_id + assert updated_policy.actor_group_id != policy.actor_group_id + assert updated_policy.actor_group_id == attrs[:actor_group_id] + assert updated_policy.conditions == policy.conditions end - test "creates a replacement policy when conditions are changed", %{ + test "updates a policy when conditions are changed", %{ policy: policy, subject: subject } do @@ -567,15 +551,14 @@ defmodule Domain.PoliciesTest do ] } - assert {:replaced, replaced_policy, replacement_policy} = - update_or_replace_policy(policy, attrs, subject) + assert {:updated, updated_policy} = + update_policy(policy, attrs, subject) - assert replaced_policy.id == policy.id - assert not is_nil(replaced_policy.deleted_at) - assert replaced_policy.resource_id == policy.resource_id - assert replaced_policy.actor_group_id == policy.actor_group_id + assert updated_policy.id == policy.id + assert updated_policy.resource_id == policy.resource_id + assert updated_policy.actor_group_id == policy.actor_group_id - assert replaced_policy.conditions == [ + refute updated_policy.conditions == [ %Domain.Policies.Condition{ property: :remote_ip, operator: :is_in_cidr, @@ -583,12 +566,7 @@ defmodule Domain.PoliciesTest do } ] - assert replacement_policy.id != policy.id - assert is_nil(replacement_policy.deleted_at) - assert replacement_policy.resource_id == policy.resource_id - assert replacement_policy.actor_group_id == policy.actor_group_id - - assert replacement_policy.conditions == [ + assert updated_policy.conditions == [ %Domain.Policies.Condition{ property: :remote_ip, operator: :is_in_cidr, @@ -597,24 +575,7 @@ defmodule Domain.PoliciesTest do ] end - test "keeps replaced policy disabled if original was disabled too", %{ - policy: policy, - account: account, - subject: subject - } do - policy = Fixtures.Policies.disable_policy(policy) - new_resource = Fixtures.Resources.create_resource(account: account) - - attrs = %{resource_id: new_resource.id} - - assert {:replaced, replaced_policy, replacement_policy} = - update_or_replace_policy(policy, attrs, subject) - - assert replaced_policy.disabled_at - assert replacement_policy.disabled_at - end - - test "broadcasts events and expires flow for replaced policy", %{ + test "broadcasts events and expires flow for updated policy", %{ policy: policy, account: account, subject: subject @@ -628,16 +589,16 @@ defmodule Domain.PoliciesTest do attrs = %{resource_id: new_resource.id} - assert {:replaced, replaced_policy, _replacement_policy} = - update_or_replace_policy(policy, attrs, subject) + assert {:updated, updated_policy} = + update_policy(policy, attrs, subject) assert_receive {:delete_policy, policy_id} - assert policy_id == replaced_policy.id + assert policy_id == updated_policy.id assert_receive {:reject_access, policy_id, actor_group_id, resource_id} - assert policy_id == replaced_policy.id - assert actor_group_id == replaced_policy.actor_group_id - assert resource_id == replaced_policy.resource_id + assert policy_id == updated_policy.id + assert actor_group_id == updated_policy.actor_group_id + assert resource_id == updated_policy.resource_id assert_received {:expire_flow, flow_id, _flow_client_id, _flow_resource_id} assert flow_id == flow.id @@ -650,7 +611,7 @@ defmodule Domain.PoliciesTest do subject = Fixtures.Auth.remove_permissions(subject) attrs = %{description: "Name Change Attempt"} - assert update_or_replace_policy(policy, attrs, subject) == + assert update_policy(policy, attrs, subject) == {:error, {:unauthorized, reason: :missing_permissions, @@ -666,7 +627,7 @@ defmodule Domain.PoliciesTest do other_identity = Fixtures.Auth.create_identity(account: other_account, actor: other_actor) other_subject = Fixtures.Auth.create_subject(identity: other_identity) - assert update_or_replace_policy( + assert update_policy( policy, %{description: "Should not be allowed"}, other_subject diff --git a/elixir/apps/domain/test/domain/resources_test.exs b/elixir/apps/domain/test/domain/resources_test.exs index e4464c691..0b918b36c 100644 --- a/elixir/apps/domain/test/domain/resources_test.exs +++ b/elixir/apps/domain/test/domain/resources_test.exs @@ -1328,7 +1328,7 @@ defmodule Domain.ResourcesTest do end end - describe "update_or_replace_resource/3" do + describe "update_resource/3" do setup context do resource = Fixtures.Resources.create_resource( @@ -1340,7 +1340,7 @@ defmodule Domain.ResourcesTest do end test "does nothing on empty attrs", %{resource: resource, subject: subject} do - assert {:updated, _resource} = update_or_replace_resource(resource, %{}, subject) + assert {:updated, _resource} = update_resource(resource, %{}, subject) end test "returns error on invalid attrs", %{resource: resource, subject: subject} do @@ -1351,7 +1351,7 @@ defmodule Domain.ResourcesTest do "connections" => :bar } - assert {:error, changeset} = update_or_replace_resource(resource, attrs, subject) + assert {:error, changeset} = update_resource(resource, attrs, subject) assert errors_on(changeset) == %{ name: ["should be at most 255 character(s)"], @@ -1369,7 +1369,7 @@ defmodule Domain.ResourcesTest do :ok = subscribe_to_events_for_account(account) attrs = %{"name" => "foo"} - assert {:updated, resource} = update_or_replace_resource(resource, attrs, subject) + assert {:updated, resource} = update_resource(resource, attrs, subject) assert_receive {:update_resource, resource_id} assert resource_id == resource.id @@ -1382,7 +1382,7 @@ defmodule Domain.ResourcesTest do :ok = subscribe_to_events_for_resource(resource) attrs = %{"name" => "foo"} - assert {:updated, resource} = update_or_replace_resource(resource, attrs, subject) + assert {:updated, resource} = update_resource(resource, attrs, subject) assert_receive {:update_resource, resource_id} assert resource_id == resource.id @@ -1390,13 +1390,13 @@ defmodule Domain.ResourcesTest do test "allows to update name", %{resource: resource, subject: subject} do attrs = %{"name" => "foo"} - assert {:updated, resource} = update_or_replace_resource(resource, attrs, subject) + assert {:updated, resource} = update_resource(resource, attrs, subject) assert resource.name == "foo" end test "allows to update client address", %{resource: resource, subject: subject} do attrs = %{"address_description" => "http://#{resource.address}:1234/foo"} - assert {:updated, resource} = update_or_replace_resource(resource, attrs, subject) + assert {:updated, resource} = update_resource(resource, attrs, subject) assert resource.address_description == attrs["address_description"] end @@ -1409,7 +1409,7 @@ defmodule Domain.ResourcesTest do :ok = Domain.Flows.subscribe_to_flow_expiration_events(flow) attrs = %{"name" => "foo"} - assert {:updated, _resource} = update_or_replace_resource(resource, attrs, subject) + assert {:updated, _resource} = update_resource(resource, attrs, subject) refute_receive {:expire_flow, _flow_id, _client_id, _resource_id} end @@ -1419,7 +1419,7 @@ defmodule Domain.ResourcesTest do gateway1 = Fixtures.Gateways.create_gateway(account: account, group: group) attrs = %{"connections" => [%{gateway_group_id: gateway1.group_id}]} - assert {:updated, resource} = update_or_replace_resource(resource, attrs, subject) + assert {:updated, resource} = update_resource(resource, attrs, subject) gateway_group_ids = Enum.map(resource.connections, & &1.gateway_group_id) assert gateway_group_ids == [gateway1.group_id] @@ -1435,12 +1435,12 @@ defmodule Domain.ResourcesTest do ] } - assert {:updated, resource} = update_or_replace_resource(resource, attrs, subject) + assert {:updated, resource} = update_resource(resource, attrs, subject) gateway_group_ids = Enum.map(resource.connections, & &1.gateway_group_id) assert Enum.sort(gateway_group_ids) == Enum.sort([gateway1.group_id, gateway2.group_id]) attrs = %{"connections" => [%{gateway_group_id: gateway2.group_id}]} - assert {:updated, resource} = update_or_replace_resource(resource, attrs, subject) + assert {:updated, resource} = update_resource(resource, attrs, subject) gateway_group_ids = Enum.map(resource.connections, & &1.gateway_group_id) assert gateway_group_ids == [gateway2.group_id] @@ -1451,24 +1451,24 @@ defmodule Domain.ResourcesTest do test "does not allow to remove all connections", %{resource: resource, subject: subject} do attrs = %{"connections" => []} - assert {:error, changeset} = update_or_replace_resource(resource, attrs, subject) + assert {:error, changeset} = update_resource(resource, attrs, subject) assert errors_on(changeset) == %{ connections: ["can't be blank"] } end - test "replaces the resource when address is changed", %{resource: resource, subject: subject} do + test "updates the resource when address is changed", %{resource: resource, subject: subject} do attrs = %{"address" => "foo"} - assert {:replaced, updated_resource, created_resource} = - update_or_replace_resource(resource, attrs, subject) + assert {:updated, updated_resource} = + update_resource(resource, attrs, subject) - assert updated_resource.address == resource.address - assert created_resource.address == attrs["address"] + assert updated_resource.address == attrs["address"] + refute updated_resource.address == resource.address end - test "broadcasts events and expires flows when resource is replaced", %{ + test "broadcasts events and expires flows when resource is updated", %{ account: account, resource: resource, subject: subject @@ -1479,71 +1479,36 @@ defmodule Domain.ResourcesTest do attrs = %{"address" => "foo"} - assert {:replaced, updated_resource, created_resource} = - update_or_replace_resource(resource, attrs, subject) + assert {:updated, updated_resource} = + update_resource(resource, attrs, subject) flow_id = flow.id updated_resource_id = updated_resource.id assert_receive {:expire_flow, ^flow_id, _client_id, ^updated_resource_id} assert_receive {:delete_resource, ^updated_resource_id} - - created_resource_id = created_resource.id - assert_receive {:create_resource, ^created_resource_id} + assert_receive {:create_resource, ^updated_resource_id} end - test "replaces resource policies when resource is replaced", %{ - account: account, - resource: resource, - subject: subject - } do - policy1 = Fixtures.Policies.create_policy(account: account, resource: resource) - policy2 = Fixtures.Policies.create_policy(account: account, resource: resource) - - :ok = Domain.Policies.subscribe_to_events_for_account(account) - - attrs = %{"address" => "foo"} - - assert {:replaced, _updated_resource, created_resource} = - update_or_replace_resource(resource, attrs, subject) - - assert Repo.get_by(Domain.Policies.Policy, id: policy1.id).deleted_at - assert Repo.get_by(Domain.Policies.Policy, id: policy2.id).deleted_at - - assert_receive {:delete_policy, deleted_policy_id} - assert deleted_policy_id in [policy1.id, policy2.id] - - assert_receive {:delete_policy, deleted_policy_id} - assert deleted_policy_id in [policy1.id, policy2.id] - - assert_receive {:create_policy, created_policy_id} - - assert Repo.get_by(Domain.Policies.Policy, id: created_policy_id).resource_id == - created_resource.id - - assert_receive {:create_policy, created_policy_id} - - assert Repo.get_by(Domain.Policies.Policy, id: created_policy_id).resource_id == - created_resource.id - end - - test "replaces the resource when type is changed", %{resource: resource, subject: subject} do + test "updates the resource when type is changed", %{resource: resource, subject: subject} do attrs = %{"type" => "ip", "address" => "10.0.10.1"} - assert {:replaced, updated_resource, created_resource} = - update_or_replace_resource(resource, attrs, subject) + assert {:updated, updated_resource} = + update_resource(resource, attrs, subject) - assert updated_resource.type == resource.type - assert created_resource.type == :ip + assert updated_resource.id == resource.id + refute updated_resource.type == resource.type + assert updated_resource.type == :ip + assert updated_resource.address == attrs["address"] end - test "replaces the resource when filters are changed", %{resource: resource, subject: subject} do + test "updates the resource when filters are changed", %{resource: resource, subject: subject} do attrs = %{"filters" => []} - assert {:replaced, updated_resource, created_resource} = - update_or_replace_resource(resource, attrs, subject) + assert {:updated, updated_resource} = + update_resource(resource, attrs, subject) - assert updated_resource.filters == resource.filters - assert created_resource.filters == attrs["filters"] + refute updated_resource.filters == resource.filters + assert updated_resource.filters == attrs["filters"] end test "returns error when subject has no permission to create resources", %{ @@ -1552,7 +1517,7 @@ defmodule Domain.ResourcesTest do } do subject = Fixtures.Auth.remove_permissions(subject) - assert update_or_replace_resource(resource, %{}, subject) == + assert update_resource(resource, %{}, subject) == {:error, {:unauthorized, reason: :missing_permissions, diff --git a/elixir/apps/domain/test/support/fixtures/policies.ex b/elixir/apps/domain/test/support/fixtures/policies.ex index fa06af318..e1230eb37 100644 --- a/elixir/apps/domain/test/support/fixtures/policies.ex +++ b/elixir/apps/domain/test/support/fixtures/policies.ex @@ -69,27 +69,4 @@ defmodule Domain.Fixtures.Policies do {:ok, policy} = Policies.delete_policy(policy, subject) policy end - - def replace_policy(policy, attrs \\ %{}) do - attrs = policy_attrs(attrs) - policy = Repo.preload(policy, :account) - - subject = - Fixtures.Auth.create_subject( - account: policy.account, - actor: [type: :account_admin_user] - ) - - {resource_id, attrs} = - pop_assoc_fixture_id(attrs, :resource, fn -> - Fixtures.Resources.create_resource(account: policy.account, subject: subject) - end) - - attrs = Map.put(attrs, :resource_id, resource_id) - - {:replaced, replaced_policy, replacement_policy} = - Policies.update_or_replace_policy(policy, attrs, subject) - - {replaced_policy, replacement_policy} - end end diff --git a/elixir/apps/web/lib/web/live/policies/edit.ex b/elixir/apps/web/lib/web/live/policies/edit.ex index 5f694f2c0..666da8d55 100644 --- a/elixir/apps/web/lib/web/live/policies/edit.ex +++ b/elixir/apps/web/lib/web/live/policies/edit.ex @@ -13,7 +13,7 @@ defmodule Web.Policies.Edit do form = policy - |> Policies.change_policy(%{}, socket.assigns.subject) + |> Policies.change_policy(%{}) |> to_form() socket = @@ -206,7 +206,7 @@ defmodule Web.Policies.Edit do |> maybe_drop_unsupported_conditions(socket) changeset = - Policies.change_policy(socket.assigns.policy, params, socket.assigns.subject) + Policies.change_policy(socket.assigns.policy, params) |> Map.put(:action, :validate) {:noreply, assign(socket, form: to_form(changeset))} @@ -218,15 +218,11 @@ defmodule Web.Policies.Edit do |> map_condition_params(empty_values: :drop) |> maybe_drop_unsupported_conditions(socket) - case Policies.update_or_replace_policy(socket.assigns.policy, params, socket.assigns.subject) do + case Policies.update_policy(socket.assigns.policy, params, socket.assigns.subject) do {:updated, updated_policy} -> {:noreply, push_navigate(socket, to: ~p"/#{socket.assigns.account}/policies/#{updated_policy}")} - {:replaced, _replaced_policy, created_policy} -> - {:noreply, - push_navigate(socket, to: ~p"/#{socket.assigns.account}/policies/#{created_policy}")} - {:error, changeset} -> {:noreply, assign(socket, form: to_form(changeset))} end diff --git a/elixir/apps/web/lib/web/live/resources/edit.ex b/elixir/apps/web/lib/web/live/resources/edit.ex index e6858ea3b..55e429413 100644 --- a/elixir/apps/web/lib/web/live/resources/edit.ex +++ b/elixir/apps/web/lib/web/live/resources/edit.ex @@ -258,7 +258,7 @@ defmodule Web.Resources.Edit do |> map_connections_form_attrs() |> maybe_delete_connections(socket.assigns.params) - case Resources.update_or_replace_resource( + case Resources.update_resource( socket.assigns.resource, attrs, socket.assigns.subject @@ -275,23 +275,6 @@ defmodule Web.Resources.Edit do {:noreply, push_navigate(socket, to: ~p"/#{socket.assigns.account}/resources")} end - {:replaced, _updated_resource, created_resource} -> - socket = - put_flash( - socket, - :info, - "New version of resource #{created_resource.name} is created successfully." - ) - - if site_id = socket.assigns.params["site_id"] do - {:noreply, - push_navigate(socket, - to: ~p"/#{socket.assigns.account}/sites/#{site_id}" - )} - else - {:noreply, push_navigate(socket, to: ~p"/#{socket.assigns.account}/resources")} - end - {:error, changeset} -> changeset = Map.put(changeset, :action, :validate) {:noreply, assign(socket, form: to_form(changeset))} diff --git a/elixir/apps/web/lib/web/live/sites/index.ex b/elixir/apps/web/lib/web/live/sites/index.ex index fe9683957..e043caab6 100644 --- a/elixir/apps/web/lib/web/live/sites/index.ex +++ b/elixir/apps/web/lib/web/live/sites/index.ex @@ -378,7 +378,7 @@ defmodule Web.Sites.Index do Domain.Repo.transaction(fn -> with {:ok, _count} <- Domain.Resources.delete_connections_for(internet_resource, subject), {:updated, resource} <- - Domain.Resources.update_or_replace_resource(internet_resource, attrs, subject) do + Domain.Resources.update_resource(internet_resource, attrs, subject) do resource else {:error, changeset} -> diff --git a/elixir/apps/web/test/web/live/policies/edit_test.exs b/elixir/apps/web/test/web/live/policies/edit_test.exs index 085c59496..ea3e3a101 100644 --- a/elixir/apps/web/test/web/live/policies/edit_test.exs +++ b/elixir/apps/web/test/web/live/policies/edit_test.exs @@ -188,7 +188,7 @@ defmodule Web.Live.Policies.EditTest do assert policy.description == attrs.description end - test "replaces a policy on valid attrs", %{ + test "updates a policy on valid breaking change attrs", %{ account: account, identity: identity, policy: policy, @@ -206,14 +206,8 @@ defmodule Web.Live.Policies.EditTest do |> form("form", policy: attrs) |> render_submit() - assert policy = Repo.get_by(Domain.Policies.Policy, id: policy.id) - assert policy.replaced_by_policy_id + assert updated_policy = Repo.get_by(Domain.Policies.Policy, id: policy.id) - assert_redirected(lv, ~p"/#{account}/policies/#{policy.replaced_by_policy_id}") - - assert replacement_policy = - Repo.get_by(Domain.Policies.Policy, id: policy.replaced_by_policy_id) - - assert replacement_policy.resource_id == new_resource.id + assert_redirected(lv, ~p"/#{account}/policies/#{updated_policy.id}") end end diff --git a/elixir/apps/web/test/web/live/policies/show_test.exs b/elixir/apps/web/test/web/live/policies/show_test.exs index d17d2ad33..17f6b1798 100644 --- a/elixir/apps/web/test/web/live/policies/show_test.exs +++ b/elixir/apps/web/test/web/live/policies/show_test.exs @@ -92,58 +92,6 @@ defmodule Web.Live.Policies.ShowTest do assert active_buttons(html) == [] end - test "renders replaced policy", %{ - account: account, - policy: policy, - identity: identity, - conn: conn - } do - {replaced_policy, replacement_policy} = Fixtures.Policies.replace_policy(policy) - - {:ok, lv, html} = - conn - |> authorize_conn(identity) - |> live(~p"/#{account}/policies/#{replaced_policy}") - - assert html =~ "(replaced)" - assert active_buttons(html) == [] - - table = - lv - |> element("#policy") - |> render() - |> vertical_table_to_map() - - replacement_policy = Repo.preload(replacement_policy, [:actor_group, :resource]) - assert table["replaced by policy"] =~ replacement_policy.actor_group.name - assert table["replaced by policy"] =~ replacement_policy.resource.name - end - - # For now we don't show prev/next version buttons in the UI of the latest version of a policy - # test "renders replacement policy", %{ - # account: account, - # policy: policy, - # identity: identity, - # conn: conn - # } do - # {replaced_policy, replacement_policy} = Fixtures.Policies.replace_policy(policy) - - # {:ok, lv, _html} = - # conn - # |> authorize_conn(identity) - # |> live(~p"/#{account}/policies/#{replacement_policy}") - - # table = - # lv - # |> element("#policy") - # |> render() - # |> vertical_table_to_map() - - # replaced_policy = Repo.preload(replaced_policy, [:actor_group, :resource]) - # assert table["replaced policy"] =~ replaced_policy.actor_group.name - # assert table["replaced policy"] =~ replaced_policy.resource.name - # end - test "renders breadcrumbs item", %{ account: account, policy: policy, diff --git a/elixir/apps/web/test/web/live/resources/edit_test.exs b/elixir/apps/web/test/web/live/resources/edit_test.exs index 9ea2e0896..a0e6405a7 100644 --- a/elixir/apps/web/test/web/live/resources/edit_test.exs +++ b/elixir/apps/web/test/web/live/resources/edit_test.exs @@ -209,7 +209,7 @@ defmodule Web.Live.Resources.EditTest do } end - test "replaces a resource on valid attrs", %{ + test "updates a resource on valid breaking change attrs", %{ account: account, identity: identity, resource: resource, @@ -238,20 +238,22 @@ defmodule Web.Live.Resources.EditTest do |> render_submit() |> follow_redirect(conn, ~p"/#{account}/resources") - assert_receive {:create_resource, created_resource_id} + assert_receive {:delete_resource, delete_msg_resource_id} + assert_receive {:create_resource, create_msg_resource_id} + assert delete_msg_resource_id == create_msg_resource_id - assert saved_resource = Repo.get_by(Domain.Resources.Resource, id: created_resource_id) - assert saved_resource.name == attrs.name - assert html =~ "New version of resource #{saved_resource.name} is created successfully." + assert updated_resource = Repo.get_by(Domain.Resources.Resource, id: resource.id) + assert updated_resource.name == attrs.name + assert html =~ "Resource #{updated_resource.name} updated successfully" - saved_filters = - for filter <- saved_resource.filters, into: %{} do + updated_filters = + for filter <- updated_resource.filters, into: %{} do {filter.protocol, %{ports: Enum.join(filter.ports, ", ")}} end - assert Map.keys(saved_filters) == Map.keys(attrs.filters) - assert saved_filters.tcp == attrs.filters.tcp - assert saved_filters.udp == attrs.filters.udp + assert Map.keys(updated_filters) == Map.keys(attrs.filters) + assert updated_filters.tcp == attrs.filters.tcp + assert updated_filters.udp == attrs.filters.udp end test "redirects to a site when site_id query param is set", %{ @@ -282,7 +284,7 @@ defmodule Web.Live.Resources.EditTest do |> render_submit() |> follow_redirect(conn, ~p"/#{account}/sites/#{group}") - assert html =~ "New version of resource #{attrs.name} is created successfully." + assert html =~ "Resource #{attrs.name} updated successfully" end test "shows disabled traffic filter form when traffic filters disabled", %{