diff --git a/bugs.md b/bugs.md index 58bf62ea..94644a25 100644 --- a/bugs.md +++ b/bugs.md @@ -230,18 +230,6 @@ --- -### M17. SNMPv3 Fallback to MD5 Auth Protocol - -**Files:** Multiple executor files - -**Severity:** MEDIUM — Cryptographically broken auth protocol - -**Description:** Default SNMPv3 auth protocol is `"MD5"`. MD5 is cryptographically broken for authentication. - -**Fix:** Change default to `"SHA-256"` for new devices. - ---- - ### M18. No String Length Validation for SNMP Credentials in Protobuf **File:** `lib/towerops/proto/decode.ex:1509-1593` @@ -302,18 +290,6 @@ --- -### M23. Permission Checks Silent Failure on Unpreloaded Associations - -**File:** `lib/towerops_web/permissions.ex:140-146` - -**Severity:** MEDIUM — Hard-to-debug silent denial of access - -**Description:** When `org.memberships` is not preloaded, `Ecto.assoc_loaded?` returns false, the `if` block evaluates to `nil`, and `do_can?` returns `false` silently. - -**Fix:** Log a warning when permissions are checked without preloaded data. - ---- - ## LOW ### L1. Cloudflare API Token Potentially Leaked in Logs @@ -364,18 +340,6 @@ --- -### L9. Duplicate Pattern in Sync Workers - -**Files:** `gaiia_sync_worker.ex`, `sonar_sync_worker.ex`, `splynx_sync_worker.ex`, `visp_sync_worker.ex`, `netbox_sync_worker.ex` - -**Severity:** LOW — Code duplication invites inconsistency - -**Description:** Five workers have identical `should_sync?/1` logic duplicated verbatim. - -**Fix:** Extract to `Integrations` context module. - ---- - ### L12. `window.liveSocket` Exposed Globally **File:** `assets/js/app.ts:423` diff --git a/lib/towerops/integrations.ex b/lib/towerops/integrations.ex index 50f80b70..e4f0059f 100644 --- a/lib/towerops/integrations.ex +++ b/lib/towerops/integrations.ex @@ -7,6 +7,27 @@ defmodule Towerops.Integrations do alias Towerops.Integrations.Integration alias Towerops.Repo + @doc """ + Returns true if the integration is due for another sync. Used by the + per-vendor cron dispatcher workers so the same interval check doesn't + get rewritten in each one. + + `default_interval_minutes` is used when the integration has no explicit + `sync_interval_minutes` value. + """ + @spec due_for_sync?(map(), pos_integer()) :: boolean() + def due_for_sync?(integration, default_interval_minutes \\ 10) do + case integration.last_synced_at do + nil -> + true + + last_synced_at -> + interval_seconds = (integration.sync_interval_minutes || default_interval_minutes) * 60 + elapsed = DateTime.diff(DateTime.utc_now(), last_synced_at, :second) + elapsed >= interval_seconds + end + end + def list_integrations(organization_id) do Integration |> where(organization_id: ^organization_id) diff --git a/lib/towerops/monitoring/executors/snmp_interface_executor.ex b/lib/towerops/monitoring/executors/snmp_interface_executor.ex index 8dc51ca7..dd1d3860 100644 --- a/lib/towerops/monitoring/executors/snmp_interface_executor.ex +++ b/lib/towerops/monitoring/executors/snmp_interface_executor.ex @@ -95,7 +95,9 @@ defmodule Towerops.Monitoring.Executors.SnmpInterfaceExecutor do opts |> Keyword.put(:security_name, device.snmpv3_username || "") |> Keyword.put(:security_level, device.snmpv3_security_level || "noAuthNoPriv") - |> Keyword.put(:auth_protocol, device.snmpv3_auth_protocol || "MD5") + # SHA-256 default — MD5 is cryptographically broken for authentication. + # Devices that explicitly require MD5 still get it via the schema field. + |> Keyword.put(:auth_protocol, device.snmpv3_auth_protocol || "SHA-256") |> Keyword.put(:auth_password, device.snmpv3_auth_password || "") |> Keyword.put(:priv_protocol, device.snmpv3_priv_protocol || "DES") |> Keyword.put(:priv_password, device.snmpv3_priv_password || "") diff --git a/lib/towerops/monitoring/executors/snmp_processor_executor.ex b/lib/towerops/monitoring/executors/snmp_processor_executor.ex index 258e8a25..ced16c2c 100644 --- a/lib/towerops/monitoring/executors/snmp_processor_executor.ex +++ b/lib/towerops/monitoring/executors/snmp_processor_executor.ex @@ -98,7 +98,7 @@ defmodule Towerops.Monitoring.Executors.SnmpProcessorExecutor do opts |> Keyword.put(:security_name, device.snmpv3_username || "") |> Keyword.put(:security_level, device.snmpv3_security_level || "noAuthNoPriv") - |> Keyword.put(:auth_protocol, device.snmpv3_auth_protocol || "MD5") + |> Keyword.put(:auth_protocol, device.snmpv3_auth_protocol || "SHA-256") |> Keyword.put(:auth_password, device.snmpv3_auth_password || "") |> Keyword.put(:priv_protocol, device.snmpv3_priv_protocol || "DES") |> Keyword.put(:priv_password, device.snmpv3_priv_password || "") diff --git a/lib/towerops/monitoring/executors/snmp_sensor_executor.ex b/lib/towerops/monitoring/executors/snmp_sensor_executor.ex index 0e026444..4726e97b 100644 --- a/lib/towerops/monitoring/executors/snmp_sensor_executor.ex +++ b/lib/towerops/monitoring/executors/snmp_sensor_executor.ex @@ -93,7 +93,7 @@ defmodule Towerops.Monitoring.Executors.SnmpSensorExecutor do opts |> Keyword.put(:security_name, device.snmpv3_username || "") |> Keyword.put(:security_level, device.snmpv3_security_level || "noAuthNoPriv") - |> Keyword.put(:auth_protocol, device.snmpv3_auth_protocol || "MD5") + |> Keyword.put(:auth_protocol, device.snmpv3_auth_protocol || "SHA-256") |> Keyword.put(:auth_password, device.snmpv3_auth_password || "") |> Keyword.put(:priv_protocol, device.snmpv3_priv_protocol || "DES") |> Keyword.put(:priv_password, device.snmpv3_priv_password || "") diff --git a/lib/towerops/monitoring/executors/snmp_storage_executor.ex b/lib/towerops/monitoring/executors/snmp_storage_executor.ex index 271e4bb9..95129215 100644 --- a/lib/towerops/monitoring/executors/snmp_storage_executor.ex +++ b/lib/towerops/monitoring/executors/snmp_storage_executor.ex @@ -102,7 +102,7 @@ defmodule Towerops.Monitoring.Executors.SnmpStorageExecutor do opts |> Keyword.put(:security_name, device.snmpv3_username || "") |> Keyword.put(:security_level, device.snmpv3_security_level || "noAuthNoPriv") - |> Keyword.put(:auth_protocol, device.snmpv3_auth_protocol || "MD5") + |> Keyword.put(:auth_protocol, device.snmpv3_auth_protocol || "SHA-256") |> Keyword.put(:auth_password, device.snmpv3_auth_password || "") |> Keyword.put(:priv_protocol, device.snmpv3_priv_protocol || "DES") |> Keyword.put(:priv_password, device.snmpv3_priv_password || "") diff --git a/lib/towerops/workers/gaiia_sync_worker.ex b/lib/towerops/workers/gaiia_sync_worker.ex index d7a41af7..02c6fcac 100644 --- a/lib/towerops/workers/gaiia_sync_worker.ex +++ b/lib/towerops/workers/gaiia_sync_worker.ex @@ -31,7 +31,7 @@ defmodule Towerops.Workers.GaiiaSyncWorker do end defp sync_integration(integration) do - if should_sync?(integration) do + if Integrations.due_for_sync?(integration, 15) do case Sync.sync_organization(integration) do {:ok, result} -> Logger.info("Gaiia sync completed for org #{integration.organization_id}: #{inspect(result)}") @@ -47,16 +47,4 @@ defmodule Towerops.Workers.GaiiaSyncWorker do :skipped end end - - defp should_sync?(integration) do - case integration.last_synced_at do - nil -> - true - - last_synced_at -> - interval_seconds = (integration.sync_interval_minutes || 15) * 60 - elapsed = DateTime.diff(DateTime.utc_now(), last_synced_at, :second) - elapsed >= interval_seconds - end - end end diff --git a/lib/towerops/workers/netbox_sync_worker.ex b/lib/towerops/workers/netbox_sync_worker.ex index 4dff455f..df029ed2 100644 --- a/lib/towerops/workers/netbox_sync_worker.ex +++ b/lib/towerops/workers/netbox_sync_worker.ex @@ -31,7 +31,7 @@ defmodule Towerops.Workers.NetBoxSyncWorker do end defp sync_integration(integration) do - if should_sync?(integration) do + if Integrations.due_for_sync?(integration, 30) do case Sync.sync_organization(integration) do {:ok, result} -> Logger.info("NetBox sync completed for org #{integration.organization_id}: #{inspect(result)}") @@ -47,17 +47,4 @@ defmodule Towerops.Workers.NetBoxSyncWorker do :skipped end end - - @doc false - def should_sync?(integration) do - case integration.last_synced_at do - nil -> - true - - last_synced_at -> - interval_seconds = (integration.sync_interval_minutes || 30) * 60 - elapsed = DateTime.diff(DateTime.utc_now(), last_synced_at, :second) - elapsed >= interval_seconds - end - end end diff --git a/lib/towerops/workers/sonar_sync_worker.ex b/lib/towerops/workers/sonar_sync_worker.ex index 346bbc7d..45f8d03b 100644 --- a/lib/towerops/workers/sonar_sync_worker.ex +++ b/lib/towerops/workers/sonar_sync_worker.ex @@ -27,7 +27,7 @@ defmodule Towerops.Workers.SonarSyncWorker do end defp sync_integration(integration) do - if should_sync?(integration) do + if Integrations.due_for_sync?(integration, 10) do case Sync.sync_organization(integration) do {:ok, result} -> Logger.info("Sonar sync completed for org #{integration.organization_id}: #{inspect(result)}") @@ -41,17 +41,4 @@ defmodule Towerops.Workers.SonarSyncWorker do :skipped end end - - @doc false - def should_sync?(integration) do - case integration.last_synced_at do - nil -> - true - - last_synced_at -> - interval_seconds = (integration.sync_interval_minutes || 10) * 60 - elapsed = DateTime.diff(DateTime.utc_now(), last_synced_at, :second) - elapsed >= interval_seconds - end - end end diff --git a/lib/towerops/workers/splynx_sync_worker.ex b/lib/towerops/workers/splynx_sync_worker.ex index de90b75a..c75b9d31 100644 --- a/lib/towerops/workers/splynx_sync_worker.ex +++ b/lib/towerops/workers/splynx_sync_worker.ex @@ -27,7 +27,7 @@ defmodule Towerops.Workers.SplynxSyncWorker do end defp sync_integration(integration) do - if should_sync?(integration) do + if Integrations.due_for_sync?(integration, 10) do case Sync.sync_organization(integration) do {:ok, result} -> Logger.info("Splynx sync completed for org #{integration.organization_id}: #{inspect(result)}") @@ -41,17 +41,4 @@ defmodule Towerops.Workers.SplynxSyncWorker do :skipped end end - - @doc false - def should_sync?(integration) do - case integration.last_synced_at do - nil -> - true - - last_synced_at -> - interval_seconds = (integration.sync_interval_minutes || 10) * 60 - elapsed = DateTime.diff(DateTime.utc_now(), last_synced_at, :second) - elapsed >= interval_seconds - end - end end diff --git a/lib/towerops/workers/visp_sync_worker.ex b/lib/towerops/workers/visp_sync_worker.ex index b102c904..3e7588c4 100644 --- a/lib/towerops/workers/visp_sync_worker.ex +++ b/lib/towerops/workers/visp_sync_worker.ex @@ -27,7 +27,7 @@ defmodule Towerops.Workers.VispSyncWorker do end defp sync_integration(integration) do - if should_sync?(integration) do + if Integrations.due_for_sync?(integration, 10) do case Sync.sync_organization(integration) do {:ok, result} -> Logger.info("VISP sync completed for org #{integration.organization_id}: #{inspect(result)}") @@ -41,17 +41,4 @@ defmodule Towerops.Workers.VispSyncWorker do :skipped end end - - @doc false - def should_sync?(integration) do - case integration.last_synced_at do - nil -> - true - - last_synced_at -> - interval_seconds = (integration.sync_interval_minutes || 10) * 60 - elapsed = DateTime.diff(DateTime.utc_now(), last_synced_at, :second) - elapsed >= interval_seconds - end - end end diff --git a/lib/towerops_web/permissions.ex b/lib/towerops_web/permissions.ex index 38e16eda..8b48d841 100644 --- a/lib/towerops_web/permissions.ex +++ b/lib/towerops_web/permissions.ex @@ -140,6 +140,18 @@ defmodule ToweropsWeb.Permissions do defp get_membership(%Scope{organization: org, user: user}) when not is_nil(org) and not is_nil(user) do if Ecto.assoc_loaded?(org.memberships) do Enum.find(org.memberships, &(&1.user_id == user.id)) + else + # Memberships aren't preloaded — the permission check silently + # treats the user as having no role and denies access. That used + # to look indistinguishable from a real denial; surface it so + # missing preloads are noticed and fixed at the call site. + require Logger + + Logger.warning( + "Permission check on user=#{user.id} org=#{org.id} without preloaded memberships — denying access by default" + ) + + nil end end diff --git a/test/towerops/workers/sync_worker_should_sync_test.exs b/test/towerops/workers/sync_worker_should_sync_test.exs index e112d2f4..b924253c 100644 --- a/test/towerops/workers/sync_worker_should_sync_test.exs +++ b/test/towerops/workers/sync_worker_should_sync_test.exs @@ -1,63 +1,67 @@ defmodule Towerops.Workers.SyncWorkerShouldSyncTest do @moduledoc """ - Tests the `should_sync?/1` helper across provider sync workers. - Each worker has identical logic with different default intervals. + Tests the shared `Towerops.Integrations.due_for_sync?/2` predicate that + the per-vendor sync workers delegate to. UispSyncWorker still has its + own `should_sync?/1` for backwards compat with the dispatcher; the + others all go through the shared helper. """ use ExUnit.Case, async: true - alias Towerops.Workers.NetBoxSyncWorker - alias Towerops.Workers.SonarSyncWorker - alias Towerops.Workers.SplynxSyncWorker + alias Towerops.Integrations alias Towerops.Workers.UispSyncWorker - alias Towerops.Workers.VispSyncWorker - @workers [NetBoxSyncWorker, SonarSyncWorker, SplynxSyncWorker, UispSyncWorker, VispSyncWorker] + # Default-interval-minutes per vendor — kept in sync with the workers. + @defaults [ + {10, [:netbox_default_handled_by_30, :sonar, :splynx, :visp]}, + {30, [:netbox]} + ] - describe "should_sync?/1 — nil last_synced_at" do - test "returns true (never synced)" do - for worker <- @workers do - assert worker.should_sync?(%{last_synced_at: nil, sync_interval_minutes: nil}), - "expected #{inspect(worker)} to sync when last_synced_at is nil" + describe "due_for_sync?/2 — nil last_synced_at" do + test "returns true (never synced) regardless of default interval" do + for {default, _} <- @defaults do + assert Integrations.due_for_sync?(%{last_synced_at: nil, sync_interval_minutes: nil}, default) end + + assert UispSyncWorker.should_sync?(%{last_synced_at: nil, sync_interval_minutes: nil}) end end - describe "should_sync?/1 — recent last_synced_at" do + describe "due_for_sync?/2 — recent last_synced_at" do test "returns false when just synced" do now = DateTime.utc_now() + integration = %{last_synced_at: now, sync_interval_minutes: 10} - for worker <- @workers do - integration = %{last_synced_at: now, sync_interval_minutes: 10} - - refute worker.should_sync?(integration), - "expected #{inspect(worker)} NOT to sync when just synced" + for {default, _} <- @defaults do + refute Integrations.due_for_sync?(integration, default) end + + refute UispSyncWorker.should_sync?(integration) end end - describe "should_sync?/1 — sufficient elapsed time" do + describe "due_for_sync?/2 — sufficient elapsed time" do test "returns true after interval elapses" do long_ago = DateTime.add(DateTime.utc_now(), -3600, :second) + integration = %{last_synced_at: long_ago, sync_interval_minutes: 10} - for worker <- @workers do - integration = %{last_synced_at: long_ago, sync_interval_minutes: 10} - - assert worker.should_sync?(integration), - "expected #{inspect(worker)} to sync after interval" + for {default, _} <- @defaults do + assert Integrations.due_for_sync?(integration, default) end + + assert UispSyncWorker.should_sync?(integration) end end - describe "should_sync?/1 — respects custom interval" do + describe "due_for_sync?/2 — respects custom interval" do test "90-minute interval still blocks when only 30 min elapsed" do half_hour_ago = DateTime.add(DateTime.utc_now(), -30 * 60, :second) + integration = %{last_synced_at: half_hour_ago, sync_interval_minutes: 90} - for worker <- @workers do - integration = %{last_synced_at: half_hour_ago, sync_interval_minutes: 90} - - refute worker.should_sync?(integration), - "expected #{inspect(worker)} NOT to sync within custom 90min interval" + for {default, _} <- @defaults do + refute Integrations.due_for_sync?(integration, default) end + + refute UispSyncWorker.should_sync?(integration) end end end diff --git a/test/towerops/workers/sync_workers_should_sync_test.exs b/test/towerops/workers/sync_workers_should_sync_test.exs index c1c83b33..a26cc103 100644 --- a/test/towerops/workers/sync_workers_should_sync_test.exs +++ b/test/towerops/workers/sync_workers_should_sync_test.exs @@ -1,13 +1,14 @@ defmodule Towerops.Workers.SyncWorkersShouldSyncTest do @moduledoc """ - Tests the `should_sync?/1` predicate on each provider's sync worker — a - pure helper unaffected by the underlying API client. Exercising it - separately covers the empty-`:available`-integrations branch in `perform/1` - via the worker invocation as well. + Tests the shared `Towerops.Integrations.due_for_sync?/2` predicate that + every provider's sync worker now delegates to. Each worker still wires + its own default interval (NetBox 30 min, Sonar/Splynx/VISP 10 min) so + the dispatch shape is preserved end-to-end through perform/1. """ use Towerops.DataCase, async: false use Oban.Testing, repo: Towerops.Repo + alias Towerops.Integrations alias Towerops.Workers.NetBoxSyncWorker alias Towerops.Workers.SonarSyncWorker alias Towerops.Workers.SplynxSyncWorker @@ -20,41 +21,34 @@ defmodule Towerops.Workers.SyncWorkersShouldSyncTest do } end - describe "should_sync?/1 — nil last_synced_at" do - test "returns true for never-synced integration on every worker" do - assert NetBoxSyncWorker.should_sync?(integration(nil)) - assert SonarSyncWorker.should_sync?(integration(nil)) - assert SplynxSyncWorker.should_sync?(integration(nil)) - assert VispSyncWorker.should_sync?(integration(nil)) + describe "due_for_sync?/2 — nil last_synced_at" do + test "returns true for never-synced integration regardless of default" do + assert Integrations.due_for_sync?(integration(nil), 30) + assert Integrations.due_for_sync?(integration(nil), 10) end end - describe "should_sync?/1 — interval elapsed" do + describe "due_for_sync?/2 — interval elapsed" do test "returns true when more than the configured interval has elapsed" do hour_ago = DateTime.add(DateTime.utc_now(), -3600, :second) - assert NetBoxSyncWorker.should_sync?(integration(hour_ago, 10)) - assert SonarSyncWorker.should_sync?(integration(hour_ago, 5)) - assert SplynxSyncWorker.should_sync?(integration(hour_ago, 5)) - assert VispSyncWorker.should_sync?(integration(hour_ago, 5)) + assert Integrations.due_for_sync?(integration(hour_ago, 10), 30) + assert Integrations.due_for_sync?(integration(hour_ago, 5), 10) end - test "uses default interval when sync_interval_minutes is nil" do + test "falls back to the worker's default interval when the field is nil" do hour_ago = DateTime.add(DateTime.utc_now(), -3600, :second) - # All four workers default-trigger after some minutes when the field is nil - assert NetBoxSyncWorker.should_sync?(integration(hour_ago)) - assert SonarSyncWorker.should_sync?(integration(hour_ago)) - assert SplynxSyncWorker.should_sync?(integration(hour_ago)) - assert VispSyncWorker.should_sync?(integration(hour_ago)) + # All four worker defaults are < 60 minutes so the hour-old timestamp + # should trigger a sync. + assert Integrations.due_for_sync?(integration(hour_ago), 30) + assert Integrations.due_for_sync?(integration(hour_ago), 10) end end - describe "should_sync?/1 — interval NOT elapsed" do + describe "due_for_sync?/2 — interval NOT elapsed" do test "returns false when synced too recently" do now = DateTime.utc_now() - refute NetBoxSyncWorker.should_sync?(integration(now, 60)) - refute SonarSyncWorker.should_sync?(integration(now, 60)) - refute SplynxSyncWorker.should_sync?(integration(now, 60)) - refute VispSyncWorker.should_sync?(integration(now, 60)) + refute Integrations.due_for_sync?(integration(now, 60), 30) + refute Integrations.due_for_sync?(integration(now, 60), 10) end end