diff --git a/lib/towerops/accounts.ex b/lib/towerops/accounts.ex index 8ba69d00..a478de3f 100644 --- a/lib/towerops/accounts.ex +++ b/lib/towerops/accounts.ex @@ -122,28 +122,18 @@ defmodule Towerops.Accounts do {:error, %Ecto.Changeset{}} """ - # `Ecto.Multi.new/0` returns a `%Multi{names: MapSet.new()}` whose - # `names` field dialyzer's success-typing sees as `%MapSet{map: %{}}`. - # That concrete value violates MapSet's `@opaque t` when passed to - # subsequent `Ecto.Multi.*` calls — an upstream typing quirk in Ecto's - # `@spec new :: t` that loses information vs the actual struct literal. - # See elixir-ecto/ecto: `Ecto.Multi` uses `MapSet` internally but exposes - # it in the type, so `:no_opaque` is the correct narrow suppression. - @dialyzer {:no_opaque, [register_user: 1]} @spec register_user(map()) :: {:ok, User.t()} | {:error, Ecto.Changeset.t()} def register_user(attrs) do user_changeset = User.registration_changeset(%User{}, attrs) - multi = - Ecto.Multi.new() - |> Ecto.Multi.insert(:user, user_changeset) - |> grant_consents_on_registration(attrs) - - case Repo.transaction(multi) do - {:ok, %{user: user}} -> {:ok, user} - {:error, :user, changeset, _} -> {:error, changeset} - {:error, _name, _changeset, _} -> {:error, user_changeset} - end + Repo.transaction(fn -> + with {:ok, user} <- Repo.insert(user_changeset), + :ok <- grant_consents_sequential(user, attrs) do + user + else + {:error, changeset} -> Repo.rollback(changeset) + end + end) end @doc """ @@ -158,52 +148,41 @@ defmodule Towerops.Accounts do {:error, %Ecto.Changeset{}} """ - # Same MapSet-opacity issue as register_user/1 — see its comment above. - @dialyzer {:no_opaque, [register_user_with_organization: 1]} @spec register_user_with_organization(map()) :: {:ok, User.t()} | {:error, Ecto.Changeset.t()} def register_user_with_organization(attrs) do user_changeset = User.registration_changeset(%User{}, attrs) - multi = - Ecto.Multi.new() - |> Ecto.Multi.insert(:user, user_changeset) - |> Ecto.Multi.run(:organization, fn _repo, %{user: user} -> - org_name = attrs["organization_name"] || "Personal" - - Towerops.Organizations.create_organization( - %{name: org_name}, - user.id - ) - end) - |> grant_consents_on_registration(attrs) - - case Repo.transaction(multi) do - {:ok, %{user: user}} -> {:ok, user} - {:error, :user, changeset, _} -> {:error, changeset} - {:error, :organization, changeset, _} -> {:error, changeset} - {:error, _name, _changeset, _} -> {:error, user_changeset} - end - end - - @spec grant_consents_on_registration(Ecto.Multi.t(), map()) :: Ecto.Multi.t() - defp grant_consents_on_registration(multi, attrs) do - # Only grant consents if checkboxes were checked - privacy_consent = attrs["privacy_policy_consent"] || attrs[:privacy_policy_consent] - terms_consent = attrs["terms_of_service_consent"] || attrs[:terms_of_service_consent] - - multi - |> maybe_grant_consent(privacy_consent, :privacy_policy_consent, "privacy_policy") - |> maybe_grant_consent(terms_consent, :terms_of_service_consent, "terms_of_service") - end - - @spec maybe_grant_consent(Ecto.Multi.t(), term(), atom(), String.t()) :: Ecto.Multi.t() - defp maybe_grant_consent(multi, flag, step_name, consent_type) when flag in ["true", true, "on"] do - Ecto.Multi.run(multi, step_name, fn _repo, %{user: user} -> - grant_consent(user.id, consent_type) + Repo.transaction(fn -> + with {:ok, user} <- Repo.insert(user_changeset), + org_name = attrs["organization_name"] || "Personal", + {:ok, _org} <- Towerops.Organizations.create_organization(%{name: org_name}, user.id), + :ok <- grant_consents_sequential(user, attrs) do + user + else + {:error, %Ecto.Changeset{} = changeset} -> Repo.rollback(changeset) + {:error, reason} -> Repo.rollback(reason) + end end) end - defp maybe_grant_consent(multi, _flag, _step_name, _consent_type), do: multi + @spec grant_consents_sequential(User.t(), map()) :: :ok | {:error, Ecto.Changeset.t()} + defp grant_consents_sequential(user, attrs) do + privacy_consent = attrs["privacy_policy_consent"] || attrs[:privacy_policy_consent] + terms_consent = attrs["terms_of_service_consent"] || attrs[:terms_of_service_consent] + + with :ok <- maybe_grant_consent(user, privacy_consent, "privacy_policy") do + maybe_grant_consent(user, terms_consent, "terms_of_service") + end + end + + defp maybe_grant_consent(user, flag, consent_type) when flag in ["true", true, "on"] do + case grant_consent(user.id, consent_type) do + {:ok, _} -> :ok + {:error, _} = err -> err + end + end + + defp maybe_grant_consent(_user, _flag, _consent_type), do: :ok ## TOTP / Two-Factor Authentication @@ -1777,25 +1756,25 @@ defmodule Towerops.Accounts do def confirm_user(token) do with {:ok, query} <- UserToken.verify_email_token_query(token, "confirm"), %User{} = user <- Repo.one(query), - {:ok, %{user: confirmed_user}} <- confirm_user_transaction(user) do + {:ok, confirmed_user} <- confirm_user_transaction(user) do {:ok, confirmed_user} else _ -> :error end end - # Same MapSet-opacity issue as register_user/1 — see its comment above. - @dialyzer {:no_opaque, [confirm_user_transaction: 1]} - @spec confirm_user_transaction(User.t()) :: - {:ok, map()} | {:error, any(), any(), map()} + @spec confirm_user_transaction(User.t()) :: {:ok, User.t()} | {:error, Ecto.Changeset.t()} defp confirm_user_transaction(user) do - Ecto.Multi.new() - |> Ecto.Multi.update(:user, User.confirm_changeset(user)) - |> Ecto.Multi.delete_all( - :tokens, - UserToken.by_user_and_contexts_query(user, ["confirm"]) - ) - |> Repo.transaction() + Repo.transaction(fn -> + case Repo.update(User.confirm_changeset(user)) do + {:ok, confirmed_user} -> + _ = Repo.delete_all(UserToken.by_user_and_contexts_query(user, ["confirm"])) + confirmed_user + + {:error, changeset} -> + Repo.rollback(changeset) + end + end) end ## Account Deletion (GDPR Right to Erasure) diff --git a/lib/towerops/devices.ex b/lib/towerops/devices.ex index 50d30b2b..996adfb6 100644 --- a/lib/towerops/devices.ex +++ b/lib/towerops/devices.ex @@ -11,6 +11,7 @@ defmodule Towerops.Devices do alias Towerops.Devices.Event alias Towerops.Monitoring alias Towerops.Organizations + alias Towerops.Organizations.Organization alias Towerops.Organizations.SubscriptionLimits alias Towerops.Repo alias Towerops.Sites @@ -19,29 +20,6 @@ defmodule Towerops.Devices do @device_preloads [:organization, site: :organization] - # Ecto.Multi's struct is declared as an opaque type in Ecto, but - # Ecto.Multi.new/0 and the Ecto.Multi.{insert,run}/* functions return - # the concrete struct shape (not the opaque Ecto.Multi.t()). Flow analysis - # sees a mismatch between the concrete struct we have in hand and the - # opaque type the subsequent Ecto.Multi.* calls declare in their - # signatures, raising call_without_opaque on every step of the pipeline. - # Passing the Multi through private helpers that take/return - # Ecto.Multi.t() compounds the issue. Adding @spec Ecto.Multi.t() on the - # helpers doesn't help — it just moves the warning to contract_with_opaque. - # Inlining the whole pipeline (even without helpers) still warns on - # `Ecto.Multi.new() |> Ecto.Multi.insert(...)` alone, confirming the - # problem is in Ecto's opaque-type declarations, not our helper shape. - # Suppression is the least-worst option until Ecto loosens the opaque type. - @dialyzer {:no_opaque, - [ - create_device: 1, - create_device: 2, - add_organization_lock: 2, - add_quota_check: 2, - maybe_lock_organization_for_quota: 3, - maybe_check_device_quota: 3 - ]} - @doc """ Returns the list of devices for a site. Ordered by custom display_order (if set), then alphabetically by name. @@ -570,28 +548,29 @@ defmodule Towerops.Devices do require Logger bypass_limits = Keyword.get(opts, :bypass_limits, false) - - # Build changeset with credential resolution changeset = build_device_changeset(attrs) - # Use transaction to prevent race condition on quota check - multi = - Ecto.Multi.new() - |> maybe_lock_organization_for_quota(changeset, bypass_limits) - |> maybe_check_device_quota(changeset, bypass_limits) - |> Ecto.Multi.insert(:device, changeset) + result = + Repo.transaction(fn -> + # Lock organization to prevent concurrent device creation race + _ = maybe_lock_organization(changeset, bypass_limits) - case Repo.transaction(multi) do - {:ok, %{device: device}} -> - # Start monitoring/polling if enabled + with :ok <- maybe_check_device_quota(changeset, bypass_limits), + {:ok, device} <- Repo.insert(changeset) do + device + else + {:error, %Ecto.Changeset{} = changeset} -> Repo.rollback(changeset) + {:error, reason} -> Repo.rollback(reason) + end + end) + + case result do + {:ok, device} -> _ = if device.monitoring_enabled do DeviceMonitorWorker.start_monitoring(device.id) end - # Auto-create ICMP ping check for all devices - # Best-effort: if this fails (e.g., transient DB contention), - # JobHealthCheckWorker will recover the missing check job. try do _ = Monitoring.ensure_default_ping_check(device) rescue @@ -600,16 +579,9 @@ defmodule Towerops.Devices do end _ = broadcast_device_change(device.organization_id, :device_created) - {:ok, device} - {:error, :quota_check, error_changeset, _} -> - {:error, error_changeset} - - {:error, :device, changeset, _} -> - {:error, changeset} - - {:error, _step, reason, _} -> + {:error, reason} -> {:error, reason} end end @@ -619,13 +591,17 @@ defmodule Towerops.Devices do apply_credential_resolution(changeset) end - # Lock organization to prevent concurrent device creation race condition - defp maybe_lock_organization_for_quota(multi, changeset, bypass_limits) do + defp maybe_lock_organization(changeset, bypass_limits) do if bypass_limits or no_organization_id?(changeset) do - multi + :ok else organization_id = Ecto.Changeset.get_change(changeset, :organization_id) - add_organization_lock(multi, organization_id) + + Repo.one!( + from o in Organization, + where: o.id == ^organization_id, + lock: "FOR UPDATE" + ) end end @@ -633,28 +609,20 @@ defmodule Towerops.Devices do match?(:error, Ecto.Changeset.fetch_change(changeset, :organization_id)) end - defp add_organization_lock(multi, organization_id) do - Ecto.Multi.run(multi, :lock_org, fn repo, _changes -> - org = - repo.one!(from o in Towerops.Organizations.Organization, where: o.id == ^organization_id, lock: "FOR UPDATE") + defp maybe_check_device_quota(_changeset, true), do: :ok - {:ok, org} - end) - end - - # Check device quota inside transaction (after locking organization) - defp maybe_check_device_quota(multi, changeset, bypass_limits) do - if bypass_limits do - multi + defp maybe_check_device_quota(changeset, false) do + if no_organization_id?(changeset) do + :ok else - add_quota_check(multi, changeset) - end - end + organization_id = Ecto.Changeset.get_change(changeset, :organization_id) + organization = Repo.get!(Organization, organization_id) - defp add_quota_check(multi, changeset) do - Ecto.Multi.run(multi, :quota_check, fn _repo, %{lock_org: organization} -> - validate_quota_limit(organization, changeset) - end) + case validate_quota_limit(organization, changeset) do + {:ok, _} -> :ok + {:error, _} = err -> err + end + end end defp validate_quota_limit(organization, changeset) do diff --git a/lib/towerops/organizations.ex b/lib/towerops/organizations.ex index 272c8e3e..1886e126 100644 --- a/lib/towerops/organizations.ex +++ b/lib/towerops/organizations.ex @@ -94,51 +94,31 @@ defmodule Towerops.Organizations do do_create_organization(attrs, user_id, bypass_limits, subscription_plan) end - # `Ecto.Multi.new/0` returns `%Multi{names: MapSet.new()}`. Dialyzer's - # success-typing sees the concrete `%MapSet{map: %{}}` value, which - # violates MapSet's `@opaque t` when passed to subsequent `Ecto.Multi.*` - # calls. This is an upstream quirk of Ecto exposing MapSet in the struct - # without an opacity-preserving `@spec`. - @dialyzer {:no_opaque, [do_create_organization: 4]} @spec do_create_organization(map(), String.t(), boolean(), String.t()) :: {:ok, Organization.t()} | {:error, Ecto.Changeset.t()} defp do_create_organization(attrs, user_id, bypass_limits, subscription_plan) do - multi = - Ecto.Multi.new() - # Step 1: Lock user to prevent concurrent org creation - |> Ecto.Multi.run(:lock_user, fn repo, _changes -> - user = repo.one!(from u in User, where: u.id == ^user_id, lock: "FOR UPDATE") - {:ok, user} - end) - # Step 2: Check free org limit inside transaction (prevents race condition) - |> Ecto.Multi.run(:check_limit, fn _repo, _changes -> - check_free_org_limit_in_transaction(bypass_limits, subscription_plan, user_id, attrs) - end) - # Step 3: Check if user has existing orgs (inside transaction, prevents default org race) - |> Ecto.Multi.run(:check_existing, fn repo, _changes -> - has_existing = repo.exists?(from m in Membership, where: m.user_id == ^user_id) - {:ok, has_existing} - end) - # Step 4: Create organization - |> Ecto.Multi.insert(:organization, Organization.changeset(%Organization{}, attrs)) - # Step 5: Create membership with correct is_default flag - |> Ecto.Multi.insert(:membership, fn %{organization: organization, check_existing: has_existing} -> - Membership.changeset(%Membership{}, %{ - organization_id: organization.id, - user_id: user_id, - role: :owner, - # Set as default if this is the user's first organization - is_default: !has_existing - }) - end) + Repo.transaction(fn -> + # Lock user to prevent concurrent org creation + _user = Repo.one!(from u in User, where: u.id == ^user_id, lock: "FOR UPDATE") - case Repo.transaction(multi) do - {:ok, %{organization: organization}} -> {:ok, organization} - {:error, :check_limit, changeset, _} -> {:error, changeset} - {:error, :organization, changeset, _} -> {:error, changeset} - {:error, :membership, changeset, _} -> {:error, changeset} - {:error, _step, reason, _} -> {:error, reason} - end + with {:ok, _} <- + check_free_org_limit_in_transaction(bypass_limits, subscription_plan, user_id, attrs), + has_existing = Repo.exists?(from m in Membership, where: m.user_id == ^user_id), + {:ok, organization} <- Repo.insert(Organization.changeset(%Organization{}, attrs)), + membership_changeset = + Membership.changeset(%Membership{}, %{ + organization_id: organization.id, + user_id: user_id, + role: :owner, + is_default: !has_existing + }), + {:ok, _membership} <- Repo.insert(membership_changeset) do + organization + else + {:error, %Ecto.Changeset{} = changeset} -> Repo.rollback(changeset) + {:error, reason} -> Repo.rollback(reason) + end + end) end # Check free org limit - for use inside transaction @@ -351,26 +331,15 @@ defmodule Towerops.Organizations do Sets an organization as the user's default. Clears any existing default and sets the new one. """ - # Same MapSet-opacity issue as do_create_organization/4 above. - @dialyzer {:no_opaque, [set_default_organization: 2]} @spec set_default_organization(String.t(), String.t()) :: - {:ok, Organization.t()} | {:error, Ecto.Changeset.t() | :not_found} + {:ok, Organization.t()} | {:error, :not_found} def set_default_organization(user_id, organization_id) do - # Verify user has access to this organization if user_has_access?(user_id, organization_id) do - multi = - Ecto.Multi.new() - |> Ecto.Multi.update_all(:clear_old_default, clear_default_query(user_id), set: [is_default: false]) - |> Ecto.Multi.update_all( - :set_new_default, - set_default_query(user_id, organization_id), - set: [is_default: true] - ) - - case Repo.transaction(multi) do - {:ok, _} -> {:ok, get_organization!(organization_id)} - {:error, _, changeset, _} -> {:error, changeset} - end + Repo.transaction(fn -> + _ = Repo.update_all(clear_default_query(user_id), set: [is_default: false]) + _ = Repo.update_all(set_default_query(user_id, organization_id), set: [is_default: true]) + get_organization!(organization_id) + end) else {:error, :not_found} end @@ -452,31 +421,30 @@ defmodule Towerops.Organizations do @doc """ Accepts an invitation and creates a membership. """ - # Same MapSet-opacity issue as do_create_organization/4 above. - @dialyzer {:no_opaque, [accept_invitation: 2]} @spec accept_invitation(Invitation.t(), String.t()) :: {:ok, Membership.t()} | {:error, Ecto.Changeset.t()} def accept_invitation(invitation, user_id) do - multi = - Ecto.Multi.new() - |> Ecto.Multi.update(:invitation, fn _ -> + Repo.transaction(fn -> + invitation_changeset = Ecto.Changeset.change(invitation, %{ accepted_at: Towerops.Time.now(), accepted_by_id: user_id }) - end) - |> Ecto.Multi.insert(:membership, fn _ -> + + membership_changeset = Membership.changeset(%Membership{}, %{ organization_id: invitation.organization_id, user_id: user_id, role: invitation.role }) - end) - case Repo.transaction(multi) do - {:ok, %{membership: membership}} -> {:ok, membership} - {:error, _failed_operation, changeset, _changes_so_far} -> {:error, changeset} - end + with {:ok, _} <- Repo.update(invitation_changeset), + {:ok, membership} <- Repo.insert(membership_changeset) do + membership + else + {:error, changeset} -> Repo.rollback(changeset) + end + end) end @doc """