fix: 3 more bugs (M17, M23, L9)

- M17: default SNMPv3 auth protocol on the four SNMP executors
  (interface/storage/processor/sensor) is now SHA-256 instead of MD5.
  Devices that explicitly set MD5 still use it; only the fallback for
  unconfigured devices changes.
- M23: permissions check now logs a warning when an org's memberships
  aren't preloaded so a silent denial is visible at the call site
  instead of looking identical to a real authz failure.
- L9: extracted the duplicated should_sync?/1 logic from gaiia, sonar,
  splynx, visp, and netbox sync workers into
  Towerops.Integrations.due_for_sync?/2. Each worker now passes its
  own default interval (10 or 30 minutes) to a single shared helper.
  Tests updated to cover the helper directly.

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`

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)

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 || "")

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 || "")

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 || "")

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 || "")

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

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

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

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

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

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
 

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

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