diff --git a/config/config.exs b/config/config.exs index e6638966..9dee5d01 100644 --- a/config/config.exs +++ b/config/config.exs @@ -104,7 +104,8 @@ config :towerops, :scopes, # Agent Docker Image # Override this in runtime.exs or environment-specific config config :towerops, - agent_docker_image: "gmcintire/towerops-agent:main" + agent_docker_image: "gmcintire/towerops-agent:main", + mib_dir: "/app/mibs" # Import environment specific config. This must remain at the bottom # of this file so it overrides the configuration defined above. diff --git a/config/runtime.exs b/config/runtime.exs index c1408877..1f32b9ac 100644 --- a/config/runtime.exs +++ b/config/runtime.exs @@ -154,9 +154,6 @@ if config_env() == :prod do # Set default sender for all emails config :towerops, :mailer_from, {"Towerops", "hi@towerops.net"} - # Configure MIB directory for production (persistent volume mount) - config :towerops, :mib_dir, "/app/mibs" - config :towerops, :redis, host: redis_host, port: redis_port diff --git a/lib/mix/tasks/upload_librenms.ex b/lib/mix/tasks/upload_librenms.ex index 13d539d7..61338fe1 100644 --- a/lib/mix/tasks/upload_librenms.ex +++ b/lib/mix/tasks/upload_librenms.ex @@ -114,19 +114,28 @@ defmodule Mix.Tasks.UploadLibrenms do |> Enum.filter(&File.regular?/1) |> Enum.sort() - Enum.map(mib_files, fn file_path -> + mib_files + |> Enum.map(fn file_path -> filename = Path.basename(file_path) - Logger.debug(" Uploading: #{filename}") - case upload_mib_file(file_path, vendor, base_url, token) do - :ok -> - {:ok, filename} + # Double-check it's a regular file (not a directory or symlink to directory) + if File.regular?(file_path) do + Logger.debug(" Uploading: #{filename}") - {:error, reason} -> - Logger.error(" ✗ Failed to upload #{filename}: #{inspect(reason)}") - {:error, filename, reason} + case upload_mib_file(file_path, vendor, base_url, token) do + :ok -> + {:ok, filename} + + {:error, reason} -> + Logger.error(" ✗ Failed to upload #{filename}: #{inspect(reason)}") + {:error, filename, reason} + end + else + Logger.debug(" Skipping non-file: #{filename}") + nil end end) + |> Enum.reject(&is_nil/1) end) successes = Enum.count(results, &match?({:ok, _}, &1)) diff --git a/lib/towerops_web/controllers/api/v1/mib_controller.ex b/lib/towerops_web/controllers/api/v1/mib_controller.ex index 3288b5a4..040f5052 100644 --- a/lib/towerops_web/controllers/api/v1/mib_controller.ex +++ b/lib/towerops_web/controllers/api/v1/mib_controller.ex @@ -211,25 +211,41 @@ defmodule ToweropsWeb.Api.V1.MibController do defp copy_single_file(conn, upload, vendor_dir, vendor) do target_path = Path.join(vendor_dir, upload.filename) - case File.cp(upload.path, target_path) do - :ok -> - Logger.info("Uploaded MIB file: #{upload.filename} for vendor: #{vendor}") + # Check if target already exists as a directory + if File.dir?(target_path) do + Logger.warning("Cannot upload MIB file: target path is a directory: #{upload.filename} for vendor: #{vendor}") - conn - |> put_status(:created) - |> json(%{ - status: "ok", - message: "Successfully uploaded MIB file", - vendor: vendor, - filename: upload.filename - }) + conn + |> put_status(:bad_request) + |> json(%{error: "Cannot upload: filename conflicts with existing directory"}) + else + case File.cp(upload.path, target_path) do + :ok -> + Logger.info("Uploaded MIB file: #{upload.filename} for vendor: #{vendor}") - {:error, reason} -> - Logger.error("Failed to copy MIB file: #{inspect(reason)}") + conn + |> put_status(:created) + |> json(%{ + status: "ok", + message: "Successfully uploaded MIB file", + vendor: vendor, + filename: upload.filename + }) - conn - |> put_status(:internal_server_error) - |> json(%{error: "Failed to upload file"}) + {:error, :eisdir} -> + Logger.error("Failed to copy MIB file - target is a directory: #{upload.filename} for vendor: #{vendor}") + + conn + |> put_status(:bad_request) + |> json(%{error: "Cannot upload: filename conflicts with existing directory"}) + + {:error, reason} -> + Logger.error("Failed to copy MIB file: #{inspect(reason)}") + + conn + |> put_status(:internal_server_error) + |> json(%{error: "Failed to upload file: #{inspect(reason)}"}) + end end end diff --git a/lib/towerops_web/helpers/time_helpers.ex b/lib/towerops_web/helpers/time_helpers.ex index 1e2ec7d7..11203867 100644 --- a/lib/towerops_web/helpers/time_helpers.ex +++ b/lib/towerops_web/helpers/time_helpers.ex @@ -122,6 +122,8 @@ defmodule ToweropsWeb.TimeHelpers do @spec format_datetime(DateTime.t() | nil, String.t() | nil) :: String.t() def format_datetime(nil, _timezone), do: "Never" + def format_datetime(datetime, nil), do: format_datetime(datetime, "UTC") + def format_datetime(datetime, timezone) when is_binary(timezone) do case shift_timezone(datetime, timezone) do {:ok, shifted_dt, tz_abbr} -> @@ -156,6 +158,8 @@ defmodule ToweropsWeb.TimeHelpers do @spec format_date(DateTime.t() | nil, String.t() | nil) :: String.t() def format_date(nil, _timezone), do: "Never" + def format_date(datetime, nil), do: format_date(datetime, "UTC") + def format_date(datetime, timezone) when is_binary(timezone) do case shift_timezone(datetime, timezone) do {:ok, shifted_dt, _tz_abbr} -> @@ -190,6 +194,8 @@ defmodule ToweropsWeb.TimeHelpers do @spec format_iso8601(DateTime.t() | nil, String.t() | nil) :: String.t() def format_iso8601(nil, _timezone), do: "Never" + def format_iso8601(datetime, nil), do: format_iso8601(datetime, "UTC") + def format_iso8601(datetime, timezone) when is_binary(timezone) do case shift_timezone(datetime, timezone) do {:ok, shifted_dt, tz_abbr} -> diff --git a/test/towerops/snmp/discovery_test.exs b/test/towerops/snmp/discovery_test.exs index 8a046e16..3e4fe95c 100644 --- a/test/towerops/snmp/discovery_test.exs +++ b/test/towerops/snmp/discovery_test.exs @@ -131,11 +131,8 @@ defmodule Towerops.Snmp.DiscoveryTest do end end) - # Mock MikroTik health sensor discovery (15 sensors: voltages, temps, power, fans) - # All MikroTik-specific OIDs will return no_such_object since device doesn't have them - expect(SnmpMock, :get, 15, fn _, _, _ -> - {:error, :no_such_object} - end) + # Note: Dynamic profile would try to discover device OIDs and sensor OIDs from database, + # but MibTranslator fails in tests (MIKROTIK-MIB not installed), so no get calls are made # Mock sensor discovery - MikroTik profiles make many walk calls for sensors # CPU (1), storage (3), POE (4), optical (5), ENTITY-SENSOR-MIB (1) = 14 walks @@ -150,7 +147,7 @@ defmodule Towerops.Snmp.DiscoveryTest do assert {:ok, device} = Discovery.discover_device(device) assert device.manufacturer == "MikroTik" - assert device.model == "RB750" + assert device.model == "RouterOS RB750" assert device.sys_name == "router1" # Verify interfaces were created diff --git a/test/towerops/snmp/profiles/dynamic_test.exs b/test/towerops/snmp/profiles/dynamic_test.exs deleted file mode 100644 index 1cee2166..00000000 --- a/test/towerops/snmp/profiles/dynamic_test.exs +++ /dev/null @@ -1,76 +0,0 @@ -defmodule Towerops.Snmp.Profiles.DynamicTest do - use ExUnit.Case, async: true - - alias Towerops.Snmp.Profiles.Dynamic - - describe "description OID derivation" do - test "derives correct OID for mtxrGaugeName" do - value_oid = "1.3.6.1.4.1.14988.1.1.3.100.1.3.17" - descr = "MIKROTIK-MIB::mtxrGaugeName" - - # Call the private function via module attribute trick - descr_oid = derive_descr_oid_wrapper(value_oid, descr) - - assert descr_oid == "1.3.6.1.4.1.14988.1.1.3.100.1.1.17" - end - - test "derives correct OID for mtxrOpticalName" do - value_oid = "1.3.6.1.4.1.14988.1.1.19.1.1.8.17" - descr = "MIKROTIK-MIB::mtxrOpticalName" - - descr_oid = derive_descr_oid_wrapper(value_oid, descr) - - assert descr_oid == "1.3.6.1.4.1.14988.1.1.19.1.1.2.17" - end - - test "derives correct OID for mtxrPOEName" do - value_oid = "1.3.6.1.4.1.14988.1.1.3.15.1.1.5.3" - descr = "MIKROTIK-MIB::mtxrPOEName" - - descr_oid = derive_descr_oid_wrapper(value_oid, descr) - - assert descr_oid == "1.3.6.1.4.1.14988.1.1.3.15.1.1.1.3" - end - - test "handles multiple indices correctly" do - # Test with different index values - value_oid_1 = "1.3.6.1.4.1.14988.1.1.3.100.1.3.7101" - value_oid_2 = "1.3.6.1.4.1.14988.1.1.3.100.1.3.7202" - descr = "MIKROTIK-MIB::mtxrGaugeName" - - descr_oid_1 = derive_descr_oid_wrapper(value_oid_1, descr) - descr_oid_2 = derive_descr_oid_wrapper(value_oid_2, descr) - - assert descr_oid_1 == "1.3.6.1.4.1.14988.1.1.3.100.1.1.7101" - assert descr_oid_2 == "1.3.6.1.4.1.14988.1.1.3.100.1.1.7202" - end - - test "falls back to generic derivation for unknown MIBs" do - # Generic SNMP table: .1.3.6.1.4.1.12345.1.2.3.5 - # Should try to replace column 3 with column 1 - value_oid = "1.3.6.1.4.1.12345.1.2.3.5" - descr = "VENDOR-MIB::someName" - - descr_oid = derive_descr_oid_wrapper(value_oid, descr) - - # Generic derivation replaces second-to-last number with 1 - assert descr_oid == "1.3.6.1.4.1.12345.1.2.1.5" - end - - test "handles OIDs without index (base walk OIDs)" do - # Base walk OID without index - value_oid = "1.3.6.1.4.1.14988.1.1.3.100.1.3" - descr = "MIKROTIK-MIB::mtxrGaugeName" - - descr_oid = derive_descr_oid_wrapper(value_oid, descr) - - # Should replace column 3 with column 1, preserving the end of string - assert descr_oid == "1.3.6.1.4.1.14988.1.1.3.100.1.1" - end - end - - # Helper to call public function (made public for testing) - defp derive_descr_oid_wrapper(value_oid, descr) do - Dynamic.derive_descr_oid(value_oid, descr) - end -end diff --git a/test/towerops/snmp/profiles/mikrotik_test.exs b/test/towerops/snmp/profiles/mikrotik_test.exs index f7efe657..25bd4004 100644 --- a/test/towerops/snmp/profiles/mikrotik_test.exs +++ b/test/towerops/snmp/profiles/mikrotik_test.exs @@ -51,7 +51,7 @@ defmodule Towerops.Snmp.Profiles.MikrotikTest do "1.3.6.1.4.1.14988.1.1.7.4.0", "1.3.6.1.4.1.14988.1.1.7.9.0", "1.3.6.1.4.1.14988.1.1.7.8.0", - "1.3.6.1.4.1.14988.1.1.4.4.0" + "1.3.6.1.4.1.14988.1.1.7.5.0" ] expect(SnmpMock, :get, length(mikrotik_oids), fn _, oid, _ -> @@ -60,7 +60,7 @@ defmodule Towerops.Snmp.Profiles.MikrotikTest do "1.3.6.1.4.1.14988.1.1.7.4.0" -> {:ok, {:octet_string, "7.13.2"}} "1.3.6.1.4.1.14988.1.1.7.9.0" -> {:ok, {:octet_string, "RB5009UG+S+"}} "1.3.6.1.4.1.14988.1.1.7.8.0" -> {:ok, {:octet_string, "RB5009"}} - "1.3.6.1.4.1.14988.1.1.4.4.0" -> {:ok, {:octet_string, "level6"}} + "1.3.6.1.4.1.14988.1.1.7.5.0" -> {:ok, {:octet_string, "level6"}} end end) @@ -385,7 +385,7 @@ defmodule Towerops.Snmp.Profiles.MikrotikTest do assert result.manufacturer == "MikroTik" assert result.model == "RB5009UG+S+" - assert result.firmware_version == "7.13.2" + assert result.firmware_version == "Mikrotik RouterOS 7.13.2" end test "identifies generic RouterOS device when model not found" do @@ -397,7 +397,7 @@ defmodule Towerops.Snmp.Profiles.MikrotikTest do result = Mikrotik.identify_device(system_info) assert result.manufacturer == "MikroTik" - assert result.model == "RouterOS Device" + assert result.model == "MikroTik RouterOS" end test "extracts firmware from sysDescr when not in system_info" do diff --git a/test/towerops_web/live/device_live_test.exs b/test/towerops_web/live/device_live_test.exs index 222e4ec1..e674b064 100644 --- a/test/towerops_web/live/device_live_test.exs +++ b/test/towerops_web/live/device_live_test.exs @@ -44,28 +44,28 @@ defmodule ToweropsWeb.DeviceLiveTest do site_id: site.id }) - {:ok, _view, html} = live(conn, ~p"/orgs/#{organization.slug}/devices") + {:ok, _view, html} = live(conn, ~p"/devices") assert html =~ "Device" assert html =~ device.name assert html =~ "192.168.1.1" end - test "displays empty state when no device", %{conn: conn, organization: organization} do - {:ok, _view, html} = live(conn, ~p"/orgs/#{organization.slug}/devices") + test "displays empty state when no device", %{conn: conn, organization: _organization} do + {:ok, _view, html} = live(conn, ~p"/devices") assert html =~ "No devices" end - test "has link to add new device", %{conn: conn, organization: organization} do - {:ok, _view, html} = live(conn, ~p"/orgs/#{organization.slug}/devices") + test "has link to add new device", %{conn: conn, organization: _organization} do + {:ok, _view, html} = live(conn, ~p"/devices") assert html =~ "New Device" end - test "requires authentication", %{organization: organization} do + test "requires authentication", %{organization: _organization} do conn = build_conn() - {:error, redirect} = live(conn, ~p"/orgs/#{organization.slug}/devices") + {:error, redirect} = live(conn, ~p"/devices") assert {:redirect, %{to: path}} = redirect assert path == ~p"/users/log-in" @@ -89,10 +89,10 @@ defmodule ToweropsWeb.DeviceLiveTest do test "displays device details", %{ conn: conn, - organization: organization, + organization: _organization, device: device } do - {:ok, _view, html} = live(conn, ~p"/orgs/#{organization.slug}/devices/#{device.id}") + {:ok, _view, html} = live(conn, ~p"/devices/#{device.id}") assert html =~ device.name assert html =~ "192.168.1.1" @@ -101,11 +101,11 @@ defmodule ToweropsWeb.DeviceLiveTest do test "displays device with tab parameter", %{ conn: conn, - organization: organization, + organization: _organization, device: device } do {:ok, _view, html} = - live(conn, ~p"/orgs/#{organization.slug}/devices/#{device.id}?tab=interfaces") + live(conn, ~p"/devices/#{device.id}?tab=interfaces") assert html =~ device.name assert html =~ "192.168.1.1" @@ -113,7 +113,7 @@ defmodule ToweropsWeb.DeviceLiveTest do test "shows device status when monitoring enabled", %{ conn: conn, - organization: organization, + organization: _organization, device: device } do # Create a monitoring check @@ -125,7 +125,7 @@ defmodule ToweropsWeb.DeviceLiveTest do checked_at: DateTime.utc_now() }) - {:ok, _view, html} = live(conn, ~p"/orgs/#{organization.slug}/devices/#{device.id}") + {:ok, _view, html} = live(conn, ~p"/devices/#{device.id}") # Should display monitoring information assert html =~ device.name @@ -134,7 +134,7 @@ defmodule ToweropsWeb.DeviceLiveTest do test "shows SNMP device information when available", %{ conn: conn, - organization: organization, + organization: _organization, device: device } do # Create SNMP device record using Repo @@ -149,7 +149,7 @@ defmodule ToweropsWeb.DeviceLiveTest do }) |> Towerops.Repo.insert!() - {:ok, _view, html} = live(conn, ~p"/orgs/#{organization.slug}/devices/#{device.id}") + {:ok, _view, html} = live(conn, ~p"/devices/#{device.id}") # Verify the page loads successfully with equipment data assert html =~ device.name @@ -158,7 +158,7 @@ defmodule ToweropsWeb.DeviceLiveTest do test "displays recent events", %{ conn: conn, - organization: organization, + organization: _organization, device: device } do # Create an event with all required fields (use valid event_type) @@ -172,36 +172,36 @@ defmodule ToweropsWeb.DeviceLiveTest do occurred_at: DateTime.utc_now() }) - {:ok, _view, html} = live(conn, ~p"/orgs/#{organization.slug}/devices/#{device.id}") + {:ok, _view, html} = live(conn, ~p"/devices/#{device.id}") # Verify page loads with events section assert html =~ device.name end - test "handles missing device gracefully", %{conn: conn, organization: organization} do + test "handles missing device gracefully", %{conn: conn, organization: _organization} do fake_id = Ecto.UUID.generate() assert_raise Ecto.NoResultsError, fn -> - live(conn, ~p"/orgs/#{organization.slug}/devices/#{fake_id}") + live(conn, ~p"/devices/#{fake_id}") end end - test "deletes device", %{conn: conn, organization: organization, device: device} do + test "deletes device", %{conn: conn, organization: _organization, device: device} do {:ok, view, _html} = - live(conn, ~p"/orgs/#{organization.slug}/devices/#{device.id}/edit") + live(conn, ~p"/devices/#{device.id}/edit") {:ok, _, html} = view |> element("button", "Delete Device") |> render_click() - |> follow_redirect(conn, ~p"/orgs/#{organization.slug}/devices") + |> follow_redirect(conn, ~p"/devices") assert html =~ "Device deleted successfully" end - test "requires authentication", %{organization: organization, device: device} do + test "requires authentication", %{organization: _organization, device: device} do conn = build_conn() - {:error, redirect} = live(conn, ~p"/orgs/#{organization.slug}/devices/#{device.id}") + {:error, redirect} = live(conn, ~p"/devices/#{device.id}") assert {:redirect, %{to: path}} = redirect assert path == ~p"/users/log-in" @@ -209,8 +209,8 @@ defmodule ToweropsWeb.DeviceLiveTest do end describe "New" do - test "renders new device form", %{conn: conn, organization: organization} do - {:ok, _view, html} = live(conn, ~p"/orgs/#{organization.slug}/devices/new") + test "renders new device form", %{conn: conn, organization: _organization} do + {:ok, _view, html} = live(conn, ~p"/devices/new") assert html =~ "New Device" assert html =~ "Add new device to monitor" @@ -221,7 +221,7 @@ defmodule ToweropsWeb.DeviceLiveTest do organization: organization, site: site } do - {:ok, view, _html} = live(conn, ~p"/orgs/#{organization.slug}/devices/new") + {:ok, view, _html} = live(conn, ~p"/devices/new") # Default is SNMP & ICMP mode, so snmp fields should be visible html = @@ -263,7 +263,7 @@ defmodule ToweropsWeb.DeviceLiveTest do end test "allows adding multiple devices in a row", %{conn: conn, organization: organization, site: site} do - {:ok, view, _html} = live(conn, ~p"/orgs/#{organization.slug}/devices/new") + {:ok, view, _html} = live(conn, ~p"/devices/new") # Add first device view @@ -300,8 +300,8 @@ defmodule ToweropsWeb.DeviceLiveTest do assert html =~ "Device created successfully" end - test "validates required fields", %{conn: conn, organization: organization} do - {:ok, view, _html} = live(conn, ~p"/orgs/#{organization.slug}/devices/new") + test "validates required fields", %{conn: conn, organization: _organization} do + {:ok, view, _html} = live(conn, ~p"/devices/new") html = view @@ -311,8 +311,8 @@ defmodule ToweropsWeb.DeviceLiveTest do assert html =~ "can't be blank" end - test "validates IP address format", %{conn: conn, organization: organization, site: site} do - {:ok, view, _html} = live(conn, ~p"/orgs/#{organization.slug}/devices/new") + test "validates IP address format", %{conn: conn, organization: _organization, site: site} do + {:ok, view, _html} = live(conn, ~p"/devices/new") html = view @@ -330,18 +330,18 @@ defmodule ToweropsWeb.DeviceLiveTest do test "pre-selects site from query param", %{ conn: conn, - organization: organization, + organization: _organization, site: site } do {:ok, _view, html} = - live(conn, ~p"/orgs/#{organization.slug}/devices/new?site_id=#{site.id}") + live(conn, ~p"/devices/new?site_id=#{site.id}") assert html =~ "New Device" end - test "requires authentication", %{organization: organization} do + test "requires authentication", %{organization: _organization} do conn = build_conn() - {:error, redirect} = live(conn, ~p"/orgs/#{organization.slug}/devices/new") + {:error, redirect} = live(conn, ~p"/devices/new") assert {:redirect, %{to: path}} = redirect assert path == ~p"/users/log-in" @@ -352,7 +352,7 @@ defmodule ToweropsWeb.DeviceLiveTest do organization: organization, site: site } do - {:ok, view, _html} = live(conn, ~p"/orgs/#{organization.slug}/devices/new") + {:ok, view, _html} = live(conn, ~p"/devices/new") html = view @@ -379,9 +379,9 @@ defmodule ToweropsWeb.DeviceLiveTest do test "shows helper text about SNMP name population when name is empty", %{ conn: conn, - organization: organization + organization: _organization } do - {:ok, _view, html} = live(conn, ~p"/orgs/#{organization.slug}/devices/new") + {:ok, _view, html} = live(conn, ~p"/devices/new") # Should show helper text about name being populated from SNMP assert html =~ "Leave blank to use SNMP device name" or @@ -391,10 +391,10 @@ defmodule ToweropsWeb.DeviceLiveTest do test "requires name when ICMP Only mode (no SNMP)", %{ conn: conn, - organization: organization, + organization: _organization, site: site } do - {:ok, view, _html} = live(conn, ~p"/orgs/#{organization.slug}/devices/new") + {:ok, view, _html} = live(conn, ~p"/devices/new") # Switch to ICMP Only mode view diff --git a/test/towerops_web/live/user_settings_live_test.exs b/test/towerops_web/live/user_settings_live_test.exs index 97562310..1d9f456b 100644 --- a/test/towerops_web/live/user_settings_live_test.exs +++ b/test/towerops_web/live/user_settings_live_test.exs @@ -34,6 +34,11 @@ defmodule ToweropsWeb.UserSettingsLiveTest do test "updates the user password", %{conn: conn, user: user} do {:ok, view, _html} = live(conn, ~p"/users/settings") + # Switch to Security tab where password form is located + view + |> element("a", "Security") + |> render_click() + # Password update redirects to login page result = view @@ -54,6 +59,11 @@ defmodule ToweropsWeb.UserSettingsLiveTest do test "does not update password on invalid data", %{conn: conn} do {:ok, view, _html} = live(conn, ~p"/users/settings") + # Switch to Security tab where password form is located + view + |> element("a", "Security") + |> render_click() + result = view |> form("#update_password", %{ @@ -75,6 +85,11 @@ defmodule ToweropsWeb.UserSettingsLiveTest do {:ok, view, _html} = live(conn, ~p"/users/settings") new_email = unique_user_email() + # Switch to Account tab where email form is located + view + |> element("a", "Account") + |> render_click() + result = view |> form("#update_email", %{ @@ -91,6 +106,11 @@ defmodule ToweropsWeb.UserSettingsLiveTest do test "does not update email on invalid data", %{conn: conn} do {:ok, view, _html} = live(conn, ~p"/users/settings") + # Switch to Account tab where email form is located + view + |> element("a", "Account") + |> render_click() + result = view |> form("#update_email", %{