github-personal/firezone
1
0
Fork 0
mirror of https://github.com/outbackdingo/firezone.git synced 2026-01-27 10:18:54 +00:00
Code Issues Packages Projects Releases Wiki Activity

refactor(portal): Allow breaking changes in Resources/Policies (#8267)

Browse Source 
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 <bmanifold@users.noreply.github.com>
Co-authored-by: Jamil <jamilbk@users.noreply.github.com>
This commit is contained in:
Brian Manifold
2025-02-26 09:05:34 -08:00
committed by GitHub
parent 10314e2540
commit d0f0de0f8d
18 changed files with 160 additions and 429 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
elixir/apps/api/lib/api/controllers/policy_controller.ex
View File

@@ -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

5
elixir/apps/api/lib/api/controllers/resource_controller.ex
View File

@@ -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

4
elixir/apps/api/test/api/client/channel_test.exs
View File

@@ -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

6
elixir/apps/api/test/api/gateway/channel_test.exs
View File

@@ -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

26
elixir/apps/domain/lib/domain/policies.ex
View File

@@ -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

29
elixir/apps/domain/lib/domain/policies/policy/changeset.ex
View File

@@ -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

84
elixir/apps/domain/lib/domain/repo.ex
View File

@@ -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}

36
elixir/apps/domain/lib/domain/resources.ex
View File

@@ -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

34
elixir/apps/domain/lib/domain/resources/resource/changeset.ex
View File

@@ -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

115
elixir/apps/domain/test/domain/policies_test.exs
View File

@@ -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

103
elixir/apps/domain/test/domain/resources_test.exs
View File

@@ -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,

23
elixir/apps/domain/test/support/fixtures/policies.ex
View File

@@ -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

10
elixir/apps/web/lib/web/live/policies/edit.ex
View File

@@ -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

19
elixir/apps/web/lib/web/live/resources/edit.ex
View File

@@ -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))}

2
elixir/apps/web/lib/web/live/sites/index.ex
View File

@@ -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} ->

12
elixir/apps/web/test/web/live/policies/edit_test.exs
View File

@@ -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

52
elixir/apps/web/test/web/live/policies/show_test.exs
View File

@@ -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,

24
elixir/apps/web/test/web/live/resources/edit_test.exs
View File

@@ -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", %{