diff --git a/.dialyzer_ignore.exs b/.dialyzer_ignore.exs index b5b60c5f..e9a9046e 100644 --- a/.dialyzer_ignore.exs +++ b/.dialyzer_ignore.exs @@ -29,11 +29,6 @@ # containing gen_server.ex path is a CI build artifact, not a local dep path. ~r/gen_server\.ex/, - # :ranch is a transitive dep (via Bandit/Cowboy); :ranch.get_addr/1 exists - # but ranch isn't in the default PLT graph. Adding :ranch to plt_add_apps - # destabilises the PLT, so suppress the single call site instead. - {"lib/towerops_web/remote_ip.ex", :unknown_function}, - # Vendored SnmpKit — out of scope for typing cleanup ~r/lib\/snmpkit/, ~r/lib\/snmp_lib/, diff --git a/lib/towerops/accounts.ex b/lib/towerops/accounts.ex index 8f791c1e..8ba69d00 100644 --- a/lib/towerops/accounts.ex +++ b/lib/towerops/accounts.ex @@ -122,18 +122,22 @@ defmodule Towerops.Accounts do {:error, %Ecto.Changeset{}} """ - # Ecto.Multi uses an opaque type that dialyzer can't see through when - # composing with local helpers; suppress the false-positive opaque warning. + # `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() - multi = Ecto.Multi.insert(multi, :user, user_changeset) - - # Grant consents if provided - multi = grant_consents_on_registration(multi, 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} @@ -154,16 +158,16 @@ defmodule Towerops.Accounts do {:error, %Ecto.Changeset{}} """ - @dialyzer {:nowarn_function, register_user_with_organization: 1} + # 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() - multi = Ecto.Multi.insert(multi, :user, user_changeset) - multi = - Ecto.Multi.run(multi, :organization, fn _repo, %{user: user} -> + 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( @@ -171,9 +175,7 @@ defmodule Towerops.Accounts do user.id ) end) - - # Grant consents if provided - multi = grant_consents_on_registration(multi, attrs) + |> grant_consents_on_registration(attrs) case Repo.transaction(multi) do {:ok, %{user: user}} -> {:ok, user} @@ -183,29 +185,26 @@ defmodule Towerops.Accounts do 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 = - if privacy_consent in ["true", true, "on"] do - Ecto.Multi.run(multi, :privacy_policy_consent, fn _repo, %{user: user} -> - grant_consent(user.id, "privacy_policy") - end) - else - multi - end - - if terms_consent in ["true", true, "on"] do - Ecto.Multi.run(multi, :terms_of_service_consent, fn _repo, %{user: user} -> - grant_consent(user.id, "terms_of_service") - end) - else - multi - end + 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) + end) + end + + defp maybe_grant_consent(multi, _flag, _step_name, _consent_type), do: multi + ## TOTP / Two-Factor Authentication @doc """ @@ -1774,25 +1773,31 @@ defmodule Towerops.Accounts do @doc """ Confirms a user by the given token. """ - # Ecto.Multi uses an opaque type that dialyzer can't always see through. - @dialyzer {:no_opaque, [confirm_user: 1]} + @spec confirm_user(String.t()) :: {:ok, User.t()} | :error def confirm_user(token) do with {:ok, query} <- UserToken.verify_email_token_query(token, "confirm"), %User{} = user <- Repo.one(query), - {:ok, %{user: user}} <- - 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() do - {:ok, user} + {:ok, %{user: 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()} + 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() + end + ## Account Deletion (GDPR Right to Erasure) @doc """ diff --git a/lib/towerops/devices.ex b/lib/towerops/devices.ex index f0fd9403..50d30b2b 100644 --- a/lib/towerops/devices.ex +++ b/lib/towerops/devices.ex @@ -19,8 +19,19 @@ defmodule Towerops.Devices do @device_preloads [:organization, site: :organization] - # Ecto.Multi uses opaque types that dialyzer can't see through when composed - # with local helper functions for locking and quota checks. + # 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, diff --git a/lib/towerops/organizations.ex b/lib/towerops/organizations.ex index 15c2a15e..272c8e3e 100644 --- a/lib/towerops/organizations.ex +++ b/lib/towerops/organizations.ex @@ -85,11 +85,8 @@ defmodule Towerops.Organizations do ## Options * `:bypass_limits` - When true, bypasses free organization limit (for superusers) """ - @dialyzer {:nowarn_function, create_organization: 2} - @dialyzer {:nowarn_function, create_organization: 3} - # Ecto.Multi/Changeset opaqueness trips dialyzer on this helper too. - @dialyzer {:no_opaque, [do_create_organization: 4, set_default_organization: 2]} - @spec create_organization(map(), String.t(), Keyword.t()) :: {:ok, Organization.t()} | {:error, Ecto.Changeset.t()} + @spec create_organization(map(), String.t(), Keyword.t()) :: + {:ok, Organization.t()} | {:error, Ecto.Changeset.t()} def create_organization(attrs, user_id, opts \\ []) do bypass_limits = Keyword.get(opts, :bypass_limits, false) subscription_plan = Map.get(attrs, :subscription_plan) || Map.get(attrs, "subscription_plan") || "free" @@ -97,6 +94,14 @@ 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() @@ -346,6 +351,10 @@ 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} 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 @@ -443,20 +452,20 @@ defmodule Towerops.Organizations do @doc """ Accepts an invitation and creates a membership. """ - @dialyzer {:nowarn_function, accept_invitation: 2} + # 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() - multi = - Ecto.Multi.update(multi, :invitation, fn _ -> + Ecto.Multi.new() + |> Ecto.Multi.update(:invitation, fn _ -> Ecto.Changeset.change(invitation, %{ accepted_at: Towerops.Time.now(), accepted_by_id: user_id }) end) - - multi = - Ecto.Multi.insert(multi, :membership, fn _ -> + |> Ecto.Multi.insert(:membership, fn _ -> Membership.changeset(%Membership{}, %{ organization_id: invitation.organization_id, user_id: user_id, diff --git a/lib/towerops/snmp/discovery.ex b/lib/towerops/snmp/discovery.ex index 1af7484b..9a4ea4e1 100644 --- a/lib/towerops/snmp/discovery.ex +++ b/lib/towerops/snmp/discovery.ex @@ -1154,7 +1154,8 @@ defmodule Towerops.Snmp.Discovery do end) end - @spec sync_ip_addresses(Device.t(), [map()]) :: :ok + @type ip_sync_device :: Device.t() | %{required(:device_id) => String.t(), required(:interfaces) => list()} + @spec sync_ip_addresses(ip_sync_device(), [map()]) :: :ok @doc """ Sync IP addresses from discovery/polling results to database. @@ -1238,7 +1239,8 @@ defmodule Towerops.Snmp.Discovery do end end - @spec sync_processors(Device.t(), [map()]) :: :ok + @type snmp_device_ref :: Device.t() | %{required(:id) => String.t()} + @spec sync_processors(snmp_device_ref(), [map()]) :: :ok @doc """ Sync processors from discovery/polling results to database. """ @@ -1305,7 +1307,7 @@ defmodule Towerops.Snmp.Discovery do Logger.error("Failed to upsert processor: #{inspect(changeset)}") end - @spec sync_storage(Device.t(), [map()]) :: :ok + @spec sync_storage(snmp_device_ref(), [map()]) :: :ok @doc """ Sync storage from discovery/polling results to database. """ diff --git a/lib/towerops/snmp/mib_translator.ex b/lib/towerops/snmp/mib_translator.ex index 6ca49c7b..5e769c88 100644 --- a/lib/towerops/snmp/mib_translator.ex +++ b/lib/towerops/snmp/mib_translator.ex @@ -27,10 +27,6 @@ defmodule Towerops.Snmp.MibTranslator do iex> MibTranslator.translate("INVALID-MIB::badObject") {:error, :translation_failed} """ - # The Rust NIF stub returns a narrow type but at runtime the real NIF returns - # either a binary OID on success or {:error, reason} on failure. Dialyzer sees - # only the stub's narrow type, causing a false guard_fail on the is_binary clause. - @dialyzer {:nowarn_function, translate: 1} @spec translate(String.t()) :: {:ok, String.t()} | {:error, :translation_failed} def translate(mib_name) when is_binary(mib_name) do cond do diff --git a/lib/towerops_native.ex b/lib/towerops_native.ex index 3bd567d1..0563eafb 100644 --- a/lib/towerops_native.ex +++ b/lib/towerops_native.ex @@ -22,10 +22,6 @@ defmodule ToweropsNative do @on_load :load_nif - # NIF stubs return fallback values when the NIF fails to load (defensive). - # Dialyzer only sees the stub bodies, so tell it to trust the @spec instead. - @dialyzer {:nowarn_function, [resolve_oid: 1, init_mib_library: 0, load_mib_directory: 1]} - def load_nif do nif_path = :filename.join(:code.priv_dir(:towerops), ~c"towerops_nif") @@ -62,7 +58,7 @@ defmodule ToweropsNative do {:error, "Failed to resolve MIB name"} """ @spec resolve_oid(String.t()) :: String.t() | {:error, String.t()} - def resolve_oid(_mib_name), do: {:error, "NIF not loaded"} + def resolve_oid(_mib_name), do: :erlang.nif_error(:nif_not_loaded) @doc """ Add a MIB directory to the search path. @@ -78,7 +74,7 @@ defmodule ToweropsNative do :ok """ @spec load_mib_directory(Path.t()) :: :ok - def load_mib_directory(_path), do: :ok + def load_mib_directory(_path), do: :erlang.nif_error(:nif_not_loaded) @doc """ Initialize the net-snmp library and load all MIBs into memory. @@ -101,5 +97,5 @@ defmodule ToweropsNative do "already_initialized" """ @spec init_mib_library() :: String.t() - def init_mib_library, do: "initialized" + def init_mib_library, do: :erlang.nif_error(:nif_not_loaded) end diff --git a/lib/towerops_web/channels/agent_channel.ex b/lib/towerops_web/channels/agent_channel.ex index 3005dd02..7cc192de 100644 --- a/lib/towerops_web/channels/agent_channel.ex +++ b/lib/towerops_web/channels/agent_channel.ex @@ -62,11 +62,6 @@ defmodule ToweropsWeb.AgentChannel do @typep base64_string :: String.t() @typep protobuf_binary :: binary() - # These helpers pass lightweight maps (not full Device structs) to - # Discovery.sync_* functions; the sync functions only need a few fields - # (`:id`, `:device_id`, `:interfaces`), so this is safe at runtime. - @dialyzer {:nowarn_function, [process_ip_addresses: 3, process_processors: 2, process_storage: 2]} - # Heartbeat timeout: close channel if no heartbeat in 5 minutes @heartbeat_timeout_seconds 300 # Check heartbeat every 2 minutes diff --git a/lib/towerops_web/channels/agent_socket.ex b/lib/towerops_web/channels/agent_socket.ex index 98d63829..6bb1f605 100644 --- a/lib/towerops_web/channels/agent_socket.ex +++ b/lib/towerops_web/channels/agent_socket.ex @@ -14,9 +14,12 @@ defmodule ToweropsWeb.AgentSocket do @impl true @spec connect(map(), Phoenix.Socket.t(), map()) :: {:ok, Phoenix.Socket.t()} - def connect(_params, socket, _connect_info) do - # Allow all connections - authentication happens at channel join - {:ok, socket} + def connect(_params, socket, connect_info) do + # Allow all connections - authentication happens at channel join. + # Stash peer_data so we can extract the client IP later without + # reaching into the transport process (which is adapter-specific). + peer_data = Map.get(connect_info, :peer_data) + {:ok, Phoenix.Socket.assign(socket, :peer_data, peer_data)} end @impl true diff --git a/lib/towerops_web/remote_ip.ex b/lib/towerops_web/remote_ip.ex index 0ad4a814..053c49e1 100644 --- a/lib/towerops_web/remote_ip.ex +++ b/lib/towerops_web/remote_ip.ex @@ -21,18 +21,13 @@ defmodule ToweropsWeb.RemoteIp do @spec from_socket(Phoenix.Socket.t()) :: String.t() | nil def from_socket(socket) do - case socket.transport_pid do - nil -> - nil - - pid when is_pid(pid) -> - case :ranch.get_addr(pid) do - {ip, _port} when is_tuple(ip) -> format(ip) - _ -> nil - end + # Use peer_data stashed into socket assigns at connect time via + # `connect_info: [:peer_data]`. This is the documented Phoenix API + # and is adapter-agnostic (works with both Bandit and Cowboy). + case socket.assigns[:peer_data] do + %{address: ip} when is_tuple(ip) -> format(ip) + _ -> nil end - rescue - _ -> nil end @spec format(tuple() | nil) :: String.t() | nil diff --git a/test/towerops_web/remote_ip_test.exs b/test/towerops_web/remote_ip_test.exs index 293ec1f9..a6c85ce5 100644 --- a/test/towerops_web/remote_ip_test.exs +++ b/test/towerops_web/remote_ip_test.exs @@ -15,4 +15,24 @@ defmodule ToweropsWeb.RemoteIpTest do test "formats IPv6 addresses in uppercase hex" do assert RemoteIp.format({8193, 3512, 0, 0, 0, 0, 0, 1}) == "2001:DB8:0:0:0:0:0:1" end + + describe "from_socket/1" do + test "returns formatted IP from peer_data in assigns" do + socket = %Phoenix.Socket{ + assigns: %{peer_data: %{address: {192, 168, 1, 42}, port: 12_345}} + } + + assert RemoteIp.from_socket(socket) == "192.168.1.42" + end + + test "returns nil when peer_data is missing" do + socket = %Phoenix.Socket{assigns: %{}} + assert RemoteIp.from_socket(socket) == nil + end + + test "returns nil when peer_data has no address tuple" do + socket = %Phoenix.Socket{assigns: %{peer_data: %{}}} + assert RemoteIp.from_socket(socket) == nil + end + end end