From 138151ade6bdc03536d53ee29863ef4a6fe3944d Mon Sep 17 00:00:00 2001 From: Graham McIntire Date: Tue, 10 Feb 2026 17:11:37 -0600 Subject: [PATCH] Replace conditionals with functional pattern matching Refactor cond/if/nested-case blocks into idiomatic multi-clause functions across permissions, organizations, credential resolver, SNMP client, and monitoring modules. Extract shared log_snmp_error helper and dispatch_snmp for SNMP adapter routing. --- lib/towerops/devices/credential_resolver.ex | 132 ++++++------ lib/towerops/monitoring.ex | 25 +-- lib/towerops/organizations.ex | 27 ++- lib/towerops/snmp/client.ex | 216 +++++--------------- lib/towerops_web/permissions.ex | 64 +++--- 5 files changed, 161 insertions(+), 303 deletions(-) diff --git a/lib/towerops/devices/credential_resolver.ex b/lib/towerops/devices/credential_resolver.ex index 7bc3bd20..870860b1 100644 --- a/lib/towerops/devices/credential_resolver.ex +++ b/lib/towerops/devices/credential_resolver.ex @@ -60,30 +60,9 @@ defmodule Towerops.Devices.CredentialResolver do def resolve_snmp_credentials(changeset, organization, site) do # If user explicitly set community, keep it and mark as "device" # Otherwise resolve from site → org and track source - case_result = - case get_field(changeset, :snmp_community) do - nil -> - # Resolve from hierarchy - {community, source} = resolve_with_source(site, organization, :snmp_community) - - changeset - |> put_change(:snmp_community, community) - |> put_change(:snmp_community_source, source) - - "" -> - # Empty string means inherit from site/org - {community, source} = resolve_with_source(site, organization, :snmp_community) - - changeset - |> put_change(:snmp_community, community) - |> put_change(:snmp_community_source, source) - - _value -> - # User set a value, mark as device-specific - put_change(changeset, :snmp_community_source, "device") - end - - case_result + changeset + |> get_field(:snmp_community) + |> resolve_snmp_community(changeset, organization, site) |> resolve_snmp_version(organization, site) |> resolve_snmp_port(organization, site) end @@ -121,36 +100,9 @@ defmodule Towerops.Devices.CredentialResolver do @spec resolve_snmpv3_credentials(Ecto.Changeset.t(), Organization.t(), Site.t() | nil) :: Ecto.Changeset.t() def resolve_snmpv3_credentials(changeset, organization, site) do - # Only resolve if SNMP version is "3" - version = get_field(changeset, :snmp_version) - - if version == "3" do - case get_field(changeset, :snmpv3_username) do - nil -> - # Resolve all SNMPv3 fields from hierarchy - {username, source} = resolve_with_source(site, organization, :snmpv3_username) - security_level = resolve_value(site, organization, :snmpv3_security_level) - auth_protocol = resolve_value(site, organization, :snmpv3_auth_protocol) || "SHA-256" - auth_password = resolve_value(site, organization, :snmpv3_auth_password) - priv_protocol = resolve_value(site, organization, :snmpv3_priv_protocol) || "AES" - priv_password = resolve_value(site, organization, :snmpv3_priv_password) - - changeset - |> put_change(:snmpv3_username, username) - |> put_change(:snmpv3_security_level, security_level) - |> put_change(:snmpv3_auth_protocol, auth_protocol) - |> put_change(:snmpv3_auth_password, auth_password) - |> put_change(:snmpv3_priv_protocol, priv_protocol) - |> put_change(:snmpv3_priv_password, priv_password) - |> put_change(:snmpv3_credential_source, source) - - _value -> - # User set credentials, mark as device-specific - put_change(changeset, :snmpv3_credential_source, "device") - end - else - changeset - end + changeset + |> get_field(:snmp_version) + |> do_resolve_snmpv3(changeset, organization, site) end @doc """ @@ -194,26 +146,70 @@ defmodule Towerops.Devices.CredentialResolver do # Private helpers - # Resolves a field from site → org, returns {value, source} - defp resolve_with_source(site, organization, field) do - cond do - site && Map.get(site, field) != nil -> - {Map.get(site, field), "site"} + # Resolves SNMP community from field value, collapsing nil and "" (both mean "inherit") + defp resolve_snmp_community(value, changeset, organization, site) when value in [nil, ""] do + {community, source} = resolve_with_source(site, organization, :snmp_community) - Map.get(organization, field) != nil -> - {Map.get(organization, field), "organization"} + changeset + |> put_change(:snmp_community, community) + |> put_change(:snmp_community_source, source) + end - true -> - {nil, "organization"} + defp resolve_snmp_community(_value, changeset, _organization, _site) do + put_change(changeset, :snmp_community_source, "device") + end + + # Only resolve SNMPv3 fields when version is "3" + defp do_resolve_snmpv3("3", changeset, organization, site) do + case get_field(changeset, :snmpv3_username) do + nil -> + {username, source} = resolve_with_source(site, organization, :snmpv3_username) + security_level = resolve_value(site, organization, :snmpv3_security_level) + auth_protocol = resolve_value(site, organization, :snmpv3_auth_protocol) || "SHA-256" + auth_password = resolve_value(site, organization, :snmpv3_auth_password) + priv_protocol = resolve_value(site, organization, :snmpv3_priv_protocol) || "AES" + priv_password = resolve_value(site, organization, :snmpv3_priv_password) + + changeset + |> put_change(:snmpv3_username, username) + |> put_change(:snmpv3_security_level, security_level) + |> put_change(:snmpv3_auth_protocol, auth_protocol) + |> put_change(:snmpv3_auth_password, auth_password) + |> put_change(:snmpv3_priv_protocol, priv_protocol) + |> put_change(:snmpv3_priv_password, priv_password) + |> put_change(:snmpv3_credential_source, source) + + _value -> + put_change(changeset, :snmpv3_credential_source, "device") end end + defp do_resolve_snmpv3(_version, changeset, _organization, _site), do: changeset + + # Resolves a field from site → org, returns {value, source} + defp resolve_with_source(%Site{} = site, organization, field) do + case Map.get(site, field) do + nil -> resolve_with_source(nil, organization, field) + value -> {value, "site"} + end + end + + defp resolve_with_source(_site, %Organization{} = organization, field) do + case Map.get(organization, field) do + nil -> {nil, "organization"} + value -> {value, "organization"} + end + end + + defp resolve_with_source(_site, _organization, _field), do: {nil, "organization"} + # Resolves a field from site → org, returns just the value - defp resolve_value(site, organization, field) do - if site && Map.get(site, field) != nil do - Map.get(site, field) - else - Map.get(organization, field) + defp resolve_value(%Site{} = site, organization, field) do + case Map.get(site, field) do + nil -> resolve_value(nil, organization, field) + value -> value end end + + defp resolve_value(_site, organization, field), do: Map.get(organization, field) end diff --git a/lib/towerops/monitoring.ex b/lib/towerops/monitoring.ex index 7aaaf4ae..a424cddb 100644 --- a/lib/towerops/monitoring.ex +++ b/lib/towerops/monitoring.ex @@ -72,12 +72,7 @@ defmodule Towerops.Monitoring do {:ok, %{rows: rows} = result} when rows != [] -> result - {:ok, _empty_result} -> - # Continuous aggregate may not have recent data yet, fall back to raw query - get_hourly_stats_raw(device_id, start_time, end_time) - - {:error, _} -> - # Fallback to raw query if continuous aggregate doesn't exist + _empty_or_error -> get_hourly_stats_raw(device_id, start_time, end_time) end end @@ -137,12 +132,7 @@ defmodule Towerops.Monitoring do {:ok, %{rows: rows} = result} when rows != [] -> result - {:ok, _empty_result} -> - # Continuous aggregate may not have recent data yet, fall back to raw query - get_daily_stats_raw(device_id, start_time, end_time) - - {:error, _} -> - # Fallback to raw query if continuous aggregate doesn't exist + _empty_or_error -> get_daily_stats_raw(device_id, start_time, end_time) end end @@ -195,16 +185,7 @@ defmodule Towerops.Monitoring do {:ok, %{rows: [[%Decimal{} = uptime]]}} -> uptime |> Decimal.to_float() |> Float.round(2) - {:ok, %{rows: [[nil]]}} -> - # Continuous aggregate may not have recent data yet, fall back to raw query - get_uptime_percentage_raw(device_id, start_time) - - {:ok, %{rows: []}} -> - # Continuous aggregate may not have recent data yet, fall back to raw query - get_uptime_percentage_raw(device_id, start_time) - - {:error, _} -> - # Fallback to raw query if continuous aggregate doesn't exist + _empty_or_error -> get_uptime_percentage_raw(device_id, start_time) end end diff --git a/lib/towerops/organizations.ex b/lib/towerops/organizations.ex index b6a98189..22df4477 100644 --- a/lib/towerops/organizations.ex +++ b/lib/towerops/organizations.ex @@ -78,23 +78,22 @@ defmodule Towerops.Organizations do end end - defp check_free_org_limit(bypass_limits, subscription_plan, user_id, attrs) do - if bypass_limits or subscription_plan != "free" do + defp check_free_org_limit(true, _plan, _user_id, _attrs), do: :ok + defp check_free_org_limit(_bypass, plan, _user_id, _attrs) when plan != "free", do: :ok + + defp check_free_org_limit(false, "free", user_id, attrs) do + if SubscriptionLimits.can_create_free_organization?(user_id) do :ok else - if SubscriptionLimits.can_create_free_organization?(user_id) do - :ok - else - changeset = - %Organization{} - |> Organization.changeset(attrs) - |> Ecto.Changeset.add_error( - :base, - "You already have a free organization. Upgrade your existing organization to create additional ones." - ) + changeset = + %Organization{} + |> Organization.changeset(attrs) + |> Ecto.Changeset.add_error( + :base, + "You already have a free organization. Upgrade your existing organization to create additional ones." + ) - {:error, changeset} - end + {:error, changeset} end end diff --git a/lib/towerops/snmp/client.ex b/lib/towerops/snmp/client.ex index 72a1bc8f..189c1ebb 100644 --- a/lib/towerops/snmp/client.ex +++ b/lib/towerops/snmp/client.ex @@ -43,22 +43,7 @@ defmodule Towerops.Snmp.Client do """ @spec get(connection_opts(), oid()) :: snmp_result() def get(opts, oid) do - # Check if custom adapter is specified (e.g., Replay adapter) - case Keyword.get(opts, :adapter) do - nil -> - # No adapter - check if Phoenix SNMP is disabled - if phoenix_snmp_disabled() do - log_disabled_call("get", opts, oid) - {:error, :phoenix_snmp_disabled} - else - # Default behavior - use SnmpKit - do_get_with_snmpkit(opts, oid) - end - - adapter -> - # Custom adapter - delegate directly (bypass Phoenix SNMP check) - adapter.get(opts, oid) - end + dispatch_snmp(:get, opts, oid, fn -> do_get_with_snmpkit(opts, oid) end) end # Original get implementation using SnmpKit @@ -74,30 +59,7 @@ defmodule Towerops.Snmp.Client do {:ok, extract_snmp_value(value)} {:error, reason} = error -> - # Only log warnings for actual communication errors, not missing OIDs - case reason do - :no_such_object -> - Logger.debug("SNMP object not found for #{target}: #{inspect(oid)}") - - :no_such_instance -> - Logger.debug("SNMP instance not found for #{target}: #{inspect(oid)}") - - :no_such_name -> - Logger.debug("SNMP name not found for #{target}: #{inspect(oid)}") - - :end_of_mib_view -> - Logger.debug("End of MIB view reached for #{target}: #{inspect(oid)}") - - _ -> - # Actual errors like timeout, network issues, etc. - timeout = Keyword.get(opts, :timeout, @default_timeout) - version = Keyword.fetch!(opts, :version) - - Logger.warning( - "SNMP GET failed for #{target} (v#{version}, timeout: #{timeout}ms) OID #{inspect(oid)}: #{inspect(reason)}" - ) - end - + log_snmp_error("GET", reason, target, oid, opts) error end end @@ -186,29 +148,7 @@ defmodule Towerops.Snmp.Client do {:ok, %{oid: next_oid, value: value}} {:error, reason} = error -> - # Only log warnings for actual communication errors - case reason do - :no_such_object -> - Logger.debug("SNMP GET-NEXT: object not found for #{target} at #{inspect(oid)}") - - :no_such_instance -> - Logger.debug("SNMP GET-NEXT: instance not found for #{target} at #{inspect(oid)}") - - :no_such_name -> - Logger.debug("SNMP GET-NEXT: name not found for #{target} at #{inspect(oid)}") - - :end_of_mib_view -> - Logger.debug("SNMP GET-NEXT: end of MIB view for #{target} at #{inspect(oid)}") - - _ -> - timeout = Keyword.get(opts, :timeout, @default_timeout) - version = Keyword.fetch!(opts, :version) - - Logger.warning( - "SNMP GET-NEXT failed for #{target} (v#{version}, timeout: #{timeout}ms) OID #{inspect(oid)}: #{inspect(reason)}" - ) - end - + log_snmp_error("GET-NEXT", reason, target, oid, opts) error end end @@ -227,32 +167,9 @@ defmodule Towerops.Snmp.Client do """ @spec walk(connection_opts(), oid()) :: {:ok, %{String.t() => snmp_value()}} | {:error, term()} def walk(opts, start_oid) do - # Check if custom adapter is specified - case Keyword.get(opts, :adapter) do - nil -> - # No adapter - check if Phoenix SNMP is disabled - if phoenix_snmp_disabled() do - log_disabled_call("walk", opts, start_oid) - {:error, :phoenix_snmp_disabled} - else - # Default behavior - use SnmpKit - do_walk_with_snmpkit(opts, start_oid) - end - - adapter -> - # Custom adapter - delegate and convert to map format (bypass Phoenix SNMP check) - convert_adapter_walk_to_map(adapter.walk(opts, start_oid)) - end + dispatch_snmp(:walk, opts, start_oid, fn -> do_walk_with_snmpkit(opts, start_oid) end) end - # Convert adapter's list result to map format (for backward compatibility) - defp convert_adapter_walk_to_map({:ok, results}) when is_list(results) do - walked_data = Map.new(results, fn {oid, value} -> {oid, value} end) - {:ok, walked_data} - end - - defp convert_adapter_walk_to_map(error), do: error - # Original walk implementation using SnmpKit defp do_walk_with_snmpkit(opts, start_oid) do target = build_target(opts) @@ -263,39 +180,13 @@ defmodule Towerops.Snmp.Client do case snmp_adapter().walk(target, resolved_oid, snmp_opts) do {:ok, results} when is_list(results) -> - # snmpkit returns a list of maps, convert to OID -> value map walked_data = Map.new(results, fn %{oid: oid, value: value} -> {oid, extract_snmp_value(value)} end) {:ok, walked_data} {:error, reason} = error -> - # Only log warnings for actual communication errors - case reason do - :no_such_object -> - Logger.debug("SNMP WALK: object not found for #{target} at #{inspect(start_oid)}") - - :no_such_instance -> - Logger.debug("SNMP WALK: instance not found for #{target} at #{inspect(start_oid)}") - - :no_such_name -> - Logger.debug("SNMP WALK: name not found for #{target} at #{inspect(start_oid)}") - - :end_of_mib_view -> - Logger.debug("SNMP WALK: end of MIB view for #{target} at #{inspect(start_oid)}") - - :not_found -> - Logger.debug("SNMP WALK: MIB not found for #{target} at #{inspect(start_oid)}") - - _ -> - timeout = Keyword.get(opts, :timeout, @default_timeout) - version = Keyword.fetch!(opts, :version) - - Logger.warning( - "SNMP WALK failed for #{target} (v#{version}, timeout: #{timeout}ms) OID #{inspect(start_oid)}: #{inspect(reason)}" - ) - end - + log_snmp_error("WALK", reason, target, start_oid, opts) error end end @@ -331,29 +222,7 @@ defmodule Towerops.Snmp.Client do {:ok, bulk_data} {:error, reason} = error -> - # Only log warnings for actual communication errors - case reason do - :no_such_object -> - Logger.debug("SNMP GET-BULK: object not found for #{target} at #{inspect(start_oid)}") - - :no_such_instance -> - Logger.debug("SNMP GET-BULK: instance not found for #{target} at #{inspect(start_oid)}") - - :no_such_name -> - Logger.debug("SNMP GET-BULK: name not found for #{target} at #{inspect(start_oid)}") - - :end_of_mib_view -> - Logger.debug("SNMP GET-BULK: end of MIB view for #{target} at #{inspect(start_oid)}") - - _ -> - timeout = Keyword.get(opts, :timeout, @default_timeout) - version = Keyword.fetch!(opts, :version) - - Logger.warning( - "SNMP GET-BULK failed for #{target} (v#{version}, timeout: #{timeout}ms) OID #{inspect(start_oid)}: #{inspect(reason)}" - ) - end - + log_snmp_error("GET-BULK", reason, target, start_oid, opts) error end end @@ -371,24 +240,14 @@ defmodule Towerops.Snmp.Client do """ @spec test_connection(connection_opts()) :: {:ok, String.t()} | {:error, term()} def test_connection(opts) do - # Check if custom adapter is specified - adapter = Keyword.get(opts, :adapter) + # get/2 already handles adapter dispatch and disabled check + case get(opts, "1.3.6.1.2.1.1.3.0") do + {:ok, _uptime} -> + {:ok, "Connection successful"} - if adapter == nil and phoenix_snmp_disabled() do - # No adapter and Phoenix SNMP is disabled - log_disabled_call("test_connection", opts, "sysUpTime.0") - {:error, :phoenix_snmp_disabled} - else - # Try to get sysUpTime (1.3.6.1.2.1.1.3.0) as a connectivity test - # This will use the adapter if one is specified, or SnmpKit if not - case get(opts, "1.3.6.1.2.1.1.3.0") do - {:ok, _uptime} -> - {:ok, "Connection successful"} - - {:error, reason} = error -> - Logger.warning("SNMP connection test failed: #{inspect(reason)}") - error - end + {:error, reason} = error -> + Logger.warning("SNMP connection test failed: #{inspect(reason)}") + error end end @@ -400,15 +259,52 @@ defmodule Towerops.Snmp.Client do Always returns false in test environment. """ @spec phoenix_snmp_disabled() :: boolean() - def phoenix_snmp_disabled do - # Only disable in non-test environments (allow tests to run normally) - if Application.get_env(:towerops, :env) == :test do - false - else - Application.get_env(:towerops, :disable_phoenix_snmp, true) + def phoenix_snmp_disabled, do: do_phoenix_snmp_disabled(Application.get_env(:towerops, :env)) + + defp do_phoenix_snmp_disabled(:test), do: false + defp do_phoenix_snmp_disabled(_env), do: Application.get_env(:towerops, :disable_phoenix_snmp, true) + + # Dispatches SNMP operations: custom adapter → disabled check → default implementation + defp dispatch_snmp(operation, opts, oid, default_fn) do + case Keyword.get(opts, :adapter) do + nil -> + if phoenix_snmp_disabled() do + log_disabled_call(Atom.to_string(operation), opts, oid) + {:error, :phoenix_snmp_disabled} + else + default_fn.() + end + + adapter when operation == :walk -> + opts |> adapter.walk(oid) |> convert_adapter_walk_to_map() + + adapter -> + adapter.get(opts, oid) end end + # Convert adapter's list result to map format (for backward compatibility) + defp convert_adapter_walk_to_map({:ok, results}) when is_list(results) do + {:ok, Map.new(results, fn {oid, value} -> {oid, value} end)} + end + + defp convert_adapter_walk_to_map(error), do: error + + # Shared SNMP error logging with pattern-matched clauses for known vs unknown errors + defp log_snmp_error(operation, reason, target, oid, _opts) + when reason in [:no_such_object, :no_such_instance, :no_such_name, :end_of_mib_view, :not_found] do + Logger.debug("SNMP #{operation}: #{reason} for #{target} at #{inspect(oid)}") + end + + defp log_snmp_error(operation, reason, target, oid, opts) do + timeout = Keyword.get(opts, :timeout, @default_timeout) + version = Keyword.fetch!(opts, :version) + + Logger.warning( + "SNMP #{operation} failed for #{target} (v#{version}, timeout: #{timeout}ms) OID #{inspect(oid)}: #{inspect(reason)}" + ) + end + defp log_disabled_call(operation, opts, oid_or_param) do ip = Keyword.get(opts, :ip, "unknown") version = Keyword.get(opts, :version, "unknown") diff --git a/lib/towerops_web/permissions.ex b/lib/towerops_web/permissions.ex index 166819be..95d06749 100644 --- a/lib/towerops_web/permissions.ex +++ b/lib/towerops_web/permissions.ex @@ -64,20 +64,7 @@ defmodule ToweropsWeb.Permissions do end def can?(%Scope{} = scope, action, resource) do - cond do - # Superusers can do everything - Scope.superuser?(scope) -> - true - - # No organization context - cannot perform action - is_nil(scope.organization) -> - false - - # Check organization membership role permissions - true -> - membership = get_membership(scope) - Policy.can?(membership, action, resource) - end + if Scope.superuser?(scope), do: true, else: do_can?(scope, action, resource) end def can?(nil, _action, _resource), do: false @@ -100,17 +87,7 @@ defmodule ToweropsWeb.Permissions do end def owner?(%Scope{} = scope) do - cond do - Scope.superuser?(scope) -> - true - - is_nil(scope.organization) -> - false - - true -> - membership = get_membership(scope) - membership && membership.role == :owner - end + if Scope.superuser?(scope), do: true, else: do_owner?(scope) end def owner?(nil), do: false @@ -133,27 +110,36 @@ defmodule ToweropsWeb.Permissions do end def admin?(%Scope{} = scope) do - cond do - Scope.superuser?(scope) -> - true - - is_nil(scope.organization) -> - false - - true -> - membership = get_membership(scope) - membership && membership.role in [:owner, :admin] - end + if Scope.superuser?(scope), do: true, else: do_admin?(scope) end def admin?(nil), do: false - # Private helper to extract membership from scope + # Pattern-matched private helpers for permission checks + + defp do_can?(%Scope{organization: nil}, _action, _resource), do: false + + defp do_can?(%Scope{} = scope, action, resource) do + scope |> get_membership() |> Policy.can?(action, resource) + end + + defp do_owner?(%Scope{organization: nil}), do: false + + defp do_owner?(%Scope{} = scope) do + membership = get_membership(scope) + membership && membership.role == :owner + end + + defp do_admin?(%Scope{organization: nil}), do: false + + defp do_admin?(%Scope{} = scope) do + membership = get_membership(scope) + membership && membership.role in [:owner, :admin] + end + defp get_membership(%Scope{organization: org, user: user}) when not is_nil(org) and not is_nil(user) do - # Organization should be preloaded with memberships if Ecto.assoc_loaded?(org.memberships) do Enum.find(org.memberships, &(&1.user_id == user.id)) - # If memberships not preloaded, return nil (permission check will fail safely) end end