diff --git a/docs/plans/make-sites-optional.md b/docs/plans/make-sites-optional.md new file mode 100644 index 00000000..4578dff3 --- /dev/null +++ b/docs/plans/make-sites-optional.md @@ -0,0 +1,686 @@ +# Refactor Plan: Make Sites Optional in Towerops + +## Goal +Remove the "create site first" wall for new users. Allow devices to belong directly to organizations without requiring site creation, while preserving backward compatibility for existing multi-site users. + +## Architecture Decision +**Approach**: True optional sites with nullable `site_id` +- Add `organization_id` foreign key directly to devices table +- Make `site_id` nullable +- Support 2-tier credential cascade (org → device) for site-less devices +- Preserve 3-tier cascade (org → site → device) when site is present + +## Current State Analysis + +### Database Schema +``` +Organization (id) + └── Site (organization_id, NOT NULL) + └── Device (site_id, NOT NULL) ← Devices MUST have site +``` + +**Problems**: +- Devices have NO direct `organization_id` - only indirect via `site.organization_id` +- Authorization checks crash if site is nil: `device.site.organization_id` +- Credential resolution crashes: `device.site.snmp_community` + +### Code Impact +- **42 functions** access `device.site.*` pattern and will crash if site is nil +- **17 credential resolution functions** in `lib/towerops/devices.ex` need refactoring +- **5 API controller functions** use `device.site.organization_id` for auth +- **3-tier credential cascade** for SNMP v2, SNMP v3, and MikroTik credentials + +## MVP Scope (Minimal Viable Onboarding Fix) + +### In Scope ✅ +1. Database migrations (add organization_id, make site_id nullable) +2. Core credential resolution refactor (CredentialResolver module) +3. Device form - remove site requirement, make site optional +4. Basic authorization fixes (API controllers, agent channel) +5. Core query functions (list_organization_devices, etc.) + +### Out of Scope (Future Enhancements) 🚫 +- Progressive disclosure UI (`use_sites` toggle in org settings) +- Conditional navigation (hiding "Sites" link) +- Device grouping UI changes (flat vs grouped view) +- Automatic default site creation +- Settings page site toggle +- Help text and education materials +- Mobile app changes + +**Rationale**: Focus on core backend functionality first. UI polish can come in follow-up iterations. + +--- + +## Implementation Plan + +### Phase 1: Database Schema Changes + +#### Migration 1: Add organization_id (with backfill) +**File**: `priv/repo/migrations/XXXXXX_add_organization_id_to_devices.exs` + +```elixir +def up do + # 1. Add nullable organization_id column + alter table(:devices) do + add :organization_id, references(:organizations, type: :binary_id, on_delete: :delete_all) + end + + # 2. Backfill from existing site relationships + execute """ + UPDATE devices d + SET organization_id = s.organization_id + FROM sites s + WHERE d.site_id = s.id + """ + + # 3. Make organization_id NOT NULL (data is backfilled) + alter table(:devices) do + modify :organization_id, :binary_id, null: false + end + + # 4. Add indexes + create index(:devices, [:organization_id]) + create index(:devices, [:organization_id, :display_order, :name]) + + # 5. Add check constraint (site must belong to same org) + create constraint(:devices, :devices_site_organization_match, + check: """ + site_id IS NULL OR + organization_id = (SELECT organization_id FROM sites WHERE id = site_id) + """ + ) +end + +def down do + drop constraint(:devices, :devices_site_organization_match) + drop index(:devices, [:organization_id, :display_order, :name]) + drop index(:devices, [:organization_id]) + + alter table(:devices) do + remove :organization_id + end +end +``` + +**Safety**: +- No downtime (backfill happens before NOT NULL) +- Rollbackable (can drop organization_id column) +- Check constraint prevents invalid data + +#### Migration 2: Make site_id nullable +**File**: `priv/repo/migrations/XXXXXX_make_site_id_nullable_on_devices.exs` + +```elixir +def up do + alter table(:devices) do + modify :site_id, :binary_id, null: true, from: {:binary_id, null: false} + end +end + +def down do + # Cannot rollback if NULL site_id values exist + execute """ + DO $$ + BEGIN + IF EXISTS (SELECT 1 FROM devices WHERE site_id IS NULL) THEN + RAISE EXCEPTION 'Cannot revert: devices with NULL site_id exist'; + END IF; + END $$; + """ + + alter table(:devices) do + modify :site_id, :binary_id, null: false + end +end +``` + +**Safety**: One-way migration once devices without sites exist + +--- + +### Phase 2: Core Schema Updates + +#### Update Device Schema +**File**: `lib/towerops/devices/device.ex` + +**Changes**: +1. Add `belongs_to :organization, Organization` (new field) +2. Change `validate_required` to include `:organization_id`, remove `:site_id` +3. Add `validate_site_belongs_to_organization/1` function +4. Update typespec to show `site_id: UUID.t() | nil` + +**Key validation**: +```elixir +defp validate_site_belongs_to_organization(changeset) do + site_id = get_field(changeset, :site_id) + org_id = get_field(changeset, :organization_id) + + if site_id && org_id do + site = Repo.get!(Site, site_id) + if site.organization_id != org_id do + add_error(changeset, :site_id, "must belong to the same organization") + else + changeset + end + else + changeset + end +end +``` + +--- + +### Phase 3: Credential Resolution Refactor + +#### Create CredentialResolver Module +**File**: `lib/towerops/devices/credential_resolver.ex` (new file) + +Centralizes safe credential resolution with 2-tier and 3-tier support: + +```elixir +defmodule Towerops.Devices.CredentialResolver do + @moduledoc """ + Safe credential resolution with optional sites. + + Supports: + - 2-tier: device → organization (when site_id is nil) + - 3-tier: device → site → organization (when site_id present) + """ + + def ensure_credentials_loaded(%Device{} = device) do + cond do + !Ecto.assoc_loaded?(device.organization) -> + preload_all(device) + device.site_id && !Ecto.assoc_loaded?(device.site) -> + Repo.preload(device, [:site]) + true -> + device + end + end + + defp preload_all(device) do + if device.site_id do + Repo.preload(device, [:organization, :site]) + else + Repo.preload(device, [:organization]) + end + end + + def resolve_field(device, device_field, site_field, org_field, opts \\ []) do + default = Keyword.get(opts, :default) + device = ensure_credentials_loaded(device) + + device_value = Map.get(device, device_field) + + site_value = + if device.site_id && device.site do + Map.get(device.site, site_field) + else + nil + end + + org_value = Map.get(device.organization, org_field) + + device_value || site_value || org_value || default + end + + def determine_source(device, device_field, site_field, org_field) do + device = ensure_credentials_loaded(device) + + cond do + Map.get(device, device_field) != nil -> :device + device.site_id && device.site && Map.get(device.site, site_field) != nil -> :site + Map.get(device.organization, org_field) != nil -> :organization + true -> :default + end + end +end +``` + +#### Update Credential Resolution Functions +**File**: `lib/towerops/devices.ex` + +**Refactor these functions** (lines 299-628): +- `resolve_snmp_config/1` - SNMP v2c credentials +- `resolve_snmpv3_config/1` - SNMP v3 credentials (6 fields) +- `resolve_mikrotik_config/1` - MikroTik API credentials (5 fields) + +**Pattern**: +```elixir +# BEFORE (crashes if site is nil): +defp resolve_snmp_community(device) do + device.snmp_community || + device.site.snmp_community || # ⚠️ Crashes + device.site.organization.snmp_community +end + +# AFTER (safe with CredentialResolver): +alias Towerops.Devices.CredentialResolver + +defp resolve_snmp_community(device) do + CredentialResolver.resolve_field( + device, + :snmp_community, + :snmp_community, + :snmp_community + ) +end +``` + +--- + +### Phase 4: Query Function Updates + +#### Update Organization Device Queries +**File**: `lib/towerops/devices.ex` + +**Functions to update**: +1. `list_organization_devices/2` (lines 39-63) +2. `count_organization_devices/1` (lines 83-91) +3. `list_devices_for_organizations/1` (lines 68-78) +4. `list_mikrotik_devices_with_api/0` (lines 141-156) + +**Pattern**: +```elixir +# BEFORE (requires site join): +def list_organization_devices(organization_id, filters) do + from(d in Device, + join: s in assoc(d, :site), # ⚠️ Inner join excludes site-less devices + where: s.organization_id == ^organization_id, + preload: [site: s] + ) +end + +# AFTER (direct organization_id): +def list_organization_devices(organization_id, filters) do + from(d in Device, + where: d.organization_id == ^organization_id, + order_by: [asc: d.display_order, asc: d.name], + preload: [:site, :organization] # Left join - includes site-less devices + ) +end +``` + +#### Update Preload Functions +Add safe preload helper: + +```elixir +def preload_device_with_associations(device) do + if device.site_id do + Repo.preload(device, [:organization, :site]) + else + Repo.preload(device, [:organization]) + end +end +``` + +Update `get_device/1`, `get_device!/1`, `get_device_with_details/1` to use it. + +--- + +### Phase 5: Authorization Fixes + +#### API Controllers +**File**: `lib/towerops_web/controllers/api/v1/devices_controller.ex` + +**Update authorization checks** (lines 147, 190, 236): +```elixir +# BEFORE: +if device.site.organization_id == organization_id do # ⚠️ Crashes + +# AFTER: +if device.organization_id == organization_id do # ✅ Safe +``` + +**Update site verification** (line 260): +```elixir +# BEFORE: +defp verify_site_access(nil, _organization_id), + do: {:error, "site_id is required"} + +# AFTER (site is optional): +defp verify_site_access(nil, _organization_id), + do: :ok +``` + +#### Agent Channel +**File**: `lib/towerops_web/channels/agent_channel.ex` + +**Update device verification** (line 639): +```elixir +# BEFORE: +if is_nil(organization_id) or device.site.organization_id == organization_id do + +# AFTER: +if is_nil(organization_id) or device.organization_id == organization_id do +``` + +--- + +### Phase 6: Device Form Updates + +#### Remove Site Requirement +**File**: `lib/towerops_web/live/device_live/form.ex` + +**Remove blocking check** (lines 34-38): +```elixir +# DELETE THIS: +if Enum.empty?(sites) do + {:ok, + socket + |> put_flash(:info, "Please create a site before adding a device.") + |> push_navigate(to: ~p"/sites/new")} +``` + +**Update changeset to auto-inject organization_id**: +```elixir +def handle_event("save", %{"device" => device_params}, socket) do + organization = socket.assigns.current_scope.organization + + # Inject organization_id from current context + device_params = Map.put(device_params, "organization_id", organization.id) + + save_device(socket, socket.assigns.live_action, device_params) +end +``` + +#### Make Site Selector Optional in Form +**File**: `lib/towerops_web/live/device_live/form.html.heex` + +**Update site input** (lines 168-177): +```heex + +
+ <.input + field={@form[:site_id]} + type="select" + label="Site (Optional)" + prompt="No site (ungrouped)" + options={Enum.map(@available_sites, &{&1.name, &1.id})} + /> +

+ Optionally group this device into a site (office, datacenter, etc.) +

+
+``` + +--- + +### Phase 7: API Updates + +#### Device Create Endpoint +**File**: `lib/towerops_web/controllers/api/v1/devices_controller.ex` + +**Update to auto-inject organization_id**: +```elixir +def create(conn, %{"device" => device_params}) do + organization_id = conn.assigns.current_organization_id + + # Inject organization_id from authenticated context + device_params = Map.put(device_params, "organization_id", organization_id) + + # site_id is now optional + with :ok <- verify_site_access(device_params["site_id"], organization_id), + {:ok, device} <- Devices.create_device(device_params) do + # ... + end +end +``` + +--- + +### Phase 8: Agent Assignment Updates + +#### Safe Agent Resolution +**File**: `lib/towerops/devices.ex` (lines 167-189) + +**Update agent fallback chain**: +```elixir +def resolve_agent_token_id(device) do + device = + if device.site_id do + Repo.preload(device, [:organization, :site, agent_assignments: []]) + else + Repo.preload(device, [:organization, agent_assignments: []]) + end + + get_direct_agent(device) || + get_site_agent(device) || + get_org_default_agent(device) || + get_global_default_agent() +end + +# Safe: only access site if site_id present +defp get_site_agent(device) do + if device.site_id && device.site do + device.site.agent_token_id + else + nil + end +end + +# Safe: use direct organization relationship +defp get_org_default_agent(device) do + device.organization.default_agent_token_id +end +``` + +--- + +### Phase 9: Propagation Functions + +#### Update Credential Propagation +**File**: `lib/towerops/devices.ex` + +**Functions to update**: +- `propagate_organization_community_change/2` (lines 905-934) +- `propagate_organization_mikrotik_change/2` (lines 490-521) +- `propagate_organization_snmpv3_change/2` (lines 662-688) + +**Pattern**: +```elixir +# BEFORE (inner join excludes site-less devices): +from(d in Device, + join: s in assoc(d, :site), + where: s.organization_id == ^organization_id, + ... +) + +# AFTER (includes site-less devices): +from(d in Device, + left_join: s in assoc(d, :site), + where: d.organization_id == ^organization_id, + where: (is_nil(d.site_id)) or (is_nil(s.snmp_community) or s.snmp_community == ""), + ... +) +``` + +--- + +### Phase 10: Device Quota Validation + +#### Update Quota Checks +**File**: `lib/towerops/devices.ex` (lines 727-756) + +**Change to use organization_id**: +```elixir +# BEFORE (derives org from site): +defp do_check_quota(changeset) do + case Ecto.Changeset.fetch_change(changeset, :site_id) do + {:ok, site_id} -> + validate_device_quota(changeset, site_id) + :error -> + {:ok, changeset} + end +end + +defp validate_device_quota(changeset, site_id) do + site = site_id |> Sites.get_site!() |> Repo.preload(:organization) + organization = site.organization + # ... +end + +# AFTER (uses organization_id directly): +defp do_check_quota(changeset) do + case Ecto.Changeset.fetch_change(changeset, :organization_id) do + {:ok, organization_id} -> + validate_device_quota(changeset, organization_id) + :error -> + {:ok, changeset} + end +end + +defp validate_device_quota(changeset, organization_id) do + organization = Repo.get!(Organization, organization_id) + # ... +end +``` + +--- + +## Testing Strategy + +### Unit Tests + +**File**: `test/towerops/devices/credential_resolver_test.exs` (new) + +Test scenarios: +- 3-tier resolution (device with site): device → site → org → default +- 2-tier resolution (device without site): device → org → default +- All credential types (SNMP v2, SNMP v3, MikroTik) + +**File**: `test/towerops/devices_test.exs` + +Add tests for: +- `get_snmp_config/1` with and without site +- `resolve_agent_token_id/1` with and without site +- Query functions return site-less devices +- Propagation functions handle site-less devices + +### Integration Tests + +**File**: `test/towerops_web/controllers/api/v1/devices_controller_test.exs` + +Test scenarios: +- Create device without site_id → succeeds +- Create device with site_id from different org → fails +- Show/update/delete device without site → succeeds + +**File**: `test/towerops_web/live/device_live_test.exs` + +Test scenarios: +- Navigate to /devices/new without sites → no redirect +- Submit device form without site_id → succeeds +- Device appears in list view + +### Migration Tests + +Verify: +- Migration 1 backfills all organization_id values correctly +- Check constraint prevents mismatched site/org +- Migration 2 rollback blocked when NULL site_id exists + +--- + +## Implementation Sequence + +### Day 1-2: Schema Foundation +1. Write and test migration 1 (add organization_id) +2. Update Device schema module +3. Write CredentialResolver module +4. Add unit tests for CredentialResolver + +### Day 3-4: Credential Resolution +1. Refactor SNMP credential resolution +2. Refactor MikroTik credential resolution +3. Refactor SNMPv3 credential resolution +4. Update agent assignment resolution +5. Add integration tests + +### Day 5-6: Query Updates +1. Update list_organization_devices and related functions +2. Update preload functions +3. Update propagation functions +4. Update quota validation +5. Add query tests + +### Day 7-8: Authorization & Forms +1. Update API controller authorization +2. Update agent channel authorization +3. Update device form (remove site wall, make optional) +4. Update API create endpoint +5. Add E2E tests + +### Day 9: Migration & Deployment +1. Write and test migration 2 (make site_id nullable) +2. Run migrations in staging +3. Manual testing with real devices +4. Deploy to production + +### Day 10: Validation +1. Monitor error rates +2. Test creating devices without sites +3. Verify existing devices still work +4. Performance testing + +--- + +## Critical Files Summary + +### Migrations (New) +- `priv/repo/migrations/XXXXXX_add_organization_id_to_devices.exs` +- `priv/repo/migrations/XXXXXX_make_site_id_nullable_on_devices.exs` + +### Schema & Context (Modified) +- `lib/towerops/devices/device.ex` - Add organization_id field, validation +- `lib/towerops/devices/credential_resolver.ex` - New module for safe resolution +- `lib/towerops/devices.ex` - 30+ functions need updates + +### Controllers & LiveViews (Modified) +- `lib/towerops_web/controllers/api/v1/devices_controller.ex` - Auth fixes +- `lib/towerops_web/channels/agent_channel.ex` - Auth fixes +- `lib/towerops_web/live/device_live/form.ex` - Remove site wall +- `lib/towerops_web/live/device_live/form.html.heex` - Optional site selector + +### Tests (New/Modified) +- `test/towerops/devices/credential_resolver_test.exs` - New unit tests +- `test/towerops/devices_test.exs` - Updated integration tests +- `test/towerops_web/controllers/api/v1/devices_controller_test.exs` - API tests +- `test/towerops_web/live/device_live_test.exs` - LiveView tests + +--- + +## Rollback Plan + +If issues arise: +1. **Revert application code** (Git revert) +2. **Keep migration 1** (organization_id column is additive, doesn't break anything) +3. **Revert migration 2** (only if no NULL site_id values exist) +4. System continues working with 3-tier credential resolution only + +--- + +## Success Criteria + +✅ New user registers → can add device immediately (no site required) +✅ Existing multi-site users see no change in behavior +✅ All credential resolution works (SNMP, SNMPv3, MikroTik) +✅ Agent polling continues for all devices +✅ API endpoints work for site-less devices +✅ No N+1 query regressions +✅ All tests passing (unit, integration, E2E) +✅ Zero downtime deployment + +--- + +## Future Enhancements (Post-MVP) + +After core functionality is stable, consider: +- Progressive disclosure UI (`use_sites` toggle in org settings) +- Conditional navigation (hide "Sites" link when unused) +- Device grouping UI (flat vs grouped view) +- Auto-create default site on org creation +- Help text and onboarding education +- Mobile app updates +- Advanced site features (hierarchical sites, site tags, etc.) diff --git a/lib/towerops/devices.ex b/lib/towerops/devices.ex index 1044343c..a4654a35 100644 --- a/lib/towerops/devices.ex +++ b/lib/towerops/devices.ex @@ -6,8 +6,10 @@ defmodule Towerops.Devices do import Ecto.Query alias Towerops.Agents + alias Towerops.Devices.CredentialResolver alias Towerops.Devices.Device, as: DeviceSchema alias Towerops.Devices.Event + alias Towerops.Organizations alias Towerops.Organizations.SubscriptionLimits alias Towerops.Repo alias Towerops.Sites @@ -29,7 +31,7 @@ defmodule Towerops.Devices do end @doc """ - Returns the list of all devices for an organization (via sites). + Returns the list of all devices for an organization. Ordered by custom display_order (if set), then alphabetically by name. Supports filtering by: @@ -39,8 +41,8 @@ defmodule Towerops.Devices do def list_organization_devices(organization_id, filters \\ %{}) do query = from(e in DeviceSchema, - join: s in assoc(e, :site), - where: s.organization_id == ^organization_id, + left_join: s in assoc(e, :site), + where: e.organization_id == ^organization_id, order_by: [asc: e.display_order, asc: e.name], preload: [site: s] ) @@ -68,11 +70,11 @@ defmodule Towerops.Devices do def list_devices_for_organizations(organization_ids) when is_list(organization_ids) do Repo.all( from(e in DeviceSchema, - join: s in assoc(e, :site), - join: o in assoc(s, :organization), - where: s.organization_id in ^organization_ids, - order_by: [asc: s.organization_id, asc: e.name], - preload: [site: {s, organization: o}] + left_join: s in assoc(e, :site), + join: o in assoc(e, :organization), + where: e.organization_id in ^organization_ids, + order_by: [asc: e.organization_id, asc: e.name], + preload: [site: s, organization: o] ) ) end @@ -83,8 +85,7 @@ defmodule Towerops.Devices do def count_organization_devices(organization_id) do Repo.aggregate( from(e in DeviceSchema, - join: s in assoc(e, :site), - where: s.organization_id == ^organization_id + where: e.organization_id == ^organization_id ), :count ) @@ -140,16 +141,17 @@ defmodule Towerops.Devices do """ def list_mikrotik_devices_with_api do # Get all MikroTik devices with API enabled and credentials configured + # Credentials are denormalized - already resolved on device during create/update # Agent assignment is resolved hierarchically in backup_device/1 (includes global default) Repo.all( from(d in DeviceSchema, left_join: s in assoc(d, :site), - left_join: o in assoc(s, :organization), + left_join: o in assoc(d, :organization), left_join: aa in assoc(d, :agent_assignments), where: d.mikrotik_enabled == true, - where: not is_nil(d.mikrotik_username) or not is_nil(s.mikrotik_username) or not is_nil(o.mikrotik_username), + where: not is_nil(d.mikrotik_username), order_by: [asc: d.name], - preload: [site: {s, organization: o}, agent_assignments: aa], + preload: [site: s, organization: o, agent_assignments: aa], distinct: d.id ) ) @@ -165,8 +167,13 @@ defmodule Towerops.Devices do Returns nil if no agent is assigned. """ def resolve_agent_token_id(device) do - # Ensure site and organization are preloaded - device = Repo.preload(device, site: :organization, agent_assignments: []) + # Ensure organization, site (if present), and agent_assignments are preloaded + device = + if device.site_id do + Repo.preload(device, [:organization, :site, agent_assignments: []]) + else + Repo.preload(device, [:organization, agent_assignments: []]) + end # Try each level in order get_direct_agent(device) || @@ -182,9 +189,17 @@ defmodule Towerops.Devices do end end - defp get_site_agent(device), do: device.site.agent_token_id + defp get_site_agent(device) do + # Safe: only access site if it exists + if device.site_id && device.site do + device.site.agent_token_id + end + end - defp get_org_default_agent(device), do: device.site.organization.default_agent_token_id + defp get_org_default_agent(device) do + # Use direct organization relationship instead of site.organization + device.organization.default_agent_token_id + end defp get_global_default_agent, do: Towerops.Settings.get_global_default_cloud_poller() @@ -196,7 +211,7 @@ defmodule Towerops.Devices do |> Repo.get(id) |> case do nil -> nil - device -> Repo.preload(device, [:site]) + device -> Repo.preload(device, [:organization, :site]) end end @@ -206,7 +221,7 @@ defmodule Towerops.Devices do def get_device!(id) do DeviceSchema |> Repo.get!(id) - |> Repo.preload([:site]) + |> Repo.preload([:organization, :site]) end @doc """ @@ -270,73 +285,20 @@ defmodule Towerops.Devices do - source: Where the config came from (:device, :site, or :organization) """ def get_snmp_config(device_id) when is_binary(device_id) do - device = - DeviceSchema - |> Repo.get!(device_id) - |> Repo.preload(site: :organization) - + device = Repo.get!(DeviceSchema, device_id) get_snmp_config(device) end def get_snmp_config(%DeviceSchema{} = device) do - device = ensure_snmp_config_loaded(device) - resolve_snmp_config(device) - end - - defp ensure_snmp_config_loaded(device) do - # Ensure site and organization associations are loaded - needs_preload = - !Ecto.assoc_loaded?(device.site) || - !Ecto.assoc_loaded?(device.site.organization) - - if needs_preload do - Repo.preload(device, site: :organization) - else - device - end - end - - defp resolve_snmp_config(device) do - # Resolve community, version, and port independently with hierarchical fallback - # This allows setting version at device level while inheriting community from org - community = - device.snmp_community || - device.site.snmp_community || - device.site.organization.snmp_community - - version = - device.snmp_version || - device.site.snmp_version || - device.site.organization.snmp_version || - "2c" - - port = - device.snmp_port || - device.site.snmp_port || - device.site.organization.snmp_port || - 161 - - source = determine_snmp_source(device) - + # Credentials are already denormalized on the device - just read them! %{ - version: version, - community: community, - port: port, - source: source + version: device.snmp_version || "2c", + community: device.snmp_community, + port: device.snmp_port || 161, + source: String.to_atom(device.snmp_community_source || "organization") } end - defp determine_snmp_source(device) do - # Determine source based on where community string came from (primary credential) - # If no community anywhere, source is :default - cond do - device.snmp_community != nil -> :device - device.site.snmp_community != nil -> :site - device.site.organization.snmp_community != nil -> :organization - true -> :default - end - end - @doc """ Gets MikroTik API configuration for a device with hierarchical fallback. @@ -355,98 +317,27 @@ defmodule Towerops.Devices do } """ def get_mikrotik_config(device_id) when is_binary(device_id) do - device = - DeviceSchema - |> Repo.get!(device_id) - |> Repo.preload(site: :organization) - + device = Repo.get!(DeviceSchema, device_id) get_mikrotik_config(device) end def get_mikrotik_config(%DeviceSchema{} = device) do - device = ensure_mikrotik_config_loaded(device) resolve_mikrotik_config(device) end - defp ensure_mikrotik_config_loaded(device) do - # Ensure site and organization associations are loaded - needs_preload = - !Ecto.assoc_loaded?(device.site) || - !Ecto.assoc_loaded?(device.site.organization) - - if needs_preload do - Repo.preload(device, site: :organization) - else - device - end - end - defp resolve_mikrotik_config(device) do - # Resolve each field independently with hierarchical fallback + # Credentials are already denormalized on the device - just read them! %{ - username: resolve_mikrotik_username(device), - password: resolve_mikrotik_password(device), - port: resolve_mikrotik_port(device), - ssh_port: resolve_mikrotik_ssh_port(device), - use_ssl: resolve_mikrotik_use_ssl(device), - enabled: resolve_mikrotik_enabled(device), - source: determine_mikrotik_source(device) + username: device.mikrotik_username, + password: device.mikrotik_password, + port: device.mikrotik_port || 8729, + ssh_port: device.mikrotik_ssh_port || 22, + use_ssl: if(device.mikrotik_use_ssl == nil, do: true, else: device.mikrotik_use_ssl), + enabled: device.mikrotik_enabled || false, + source: String.to_atom(device.mikrotik_credential_source || "organization") } end - defp resolve_mikrotik_username(device) do - device.mikrotik_username || - device.site.mikrotik_username || - device.site.organization.mikrotik_username - end - - defp resolve_mikrotik_password(device) do - device.mikrotik_password || - device.site.mikrotik_password || - device.site.organization.mikrotik_password - end - - defp resolve_mikrotik_port(device) do - device.mikrotik_port || - device.site.mikrotik_port || - device.site.organization.mikrotik_port || - 8729 - end - - defp resolve_mikrotik_ssh_port(device) do - device.mikrotik_ssh_port || - device.site.mikrotik_ssh_port || - device.site.organization.mikrotik_ssh_port || - 22 - end - - defp resolve_mikrotik_use_ssl(device) do - if device.mikrotik_use_ssl == nil do - device.site.mikrotik_use_ssl || - device.site.organization.mikrotik_use_ssl || - true - else - device.mikrotik_use_ssl - end - end - - defp resolve_mikrotik_enabled(device) do - device.mikrotik_enabled || - device.site.mikrotik_enabled || - device.site.organization.mikrotik_enabled || - false - end - - defp determine_mikrotik_source(device) do - # Determine source based on where username came from (primary credential) - cond do - device.mikrotik_username != nil -> :device - device.site.mikrotik_username != nil -> :site - device.site.organization.mikrotik_username != nil -> :organization - true -> :default - end - end - @doc """ Propagates MikroTik credential changes from a site to all devices in that site that inherit credentials from the site (credential_source = "site"). @@ -488,18 +379,13 @@ defmodule Towerops.Devices do Updates username, password, port, use_ssl, and enabled fields. """ def propagate_organization_mikrotik_change(organization_id, attrs) do - # Find all devices in this org's sites that: - # 1. Inherit from site (credential_source = "site") - # 2. Their site has no MikroTik username (so they fall back to org) - empty_string = "" - + # With denormalized credentials, update all devices that inherit from the organization + # (devices with mikrotik_credential_source = "organization") devices_to_update = Repo.all( from(d in DeviceSchema, - join: s in assoc(d, :site), - where: s.organization_id == ^organization_id, - where: d.mikrotik_credential_source == "site", - where: is_nil(s.mikrotik_username) or s.mikrotik_username == ^empty_string + where: d.organization_id == ^organization_id, + where: d.mikrotik_credential_source == "organization" ) ) @@ -539,11 +425,7 @@ defmodule Towerops.Devices do } """ def get_snmpv3_config(device_id) when is_binary(device_id) do - device = - DeviceSchema - |> Repo.get!(device_id) - |> Repo.preload(site: :organization) - + device = Repo.get!(DeviceSchema, device_id) get_snmpv3_config(device) end @@ -552,81 +434,18 @@ defmodule Towerops.Devices do end def get_snmpv3_config(%DeviceSchema{} = device) do - device = ensure_snmpv3_config_loaded(device) - resolve_snmpv3_config(device) - end - - defp ensure_snmpv3_config_loaded(device) do - needs_preload = - !Ecto.assoc_loaded?(device.site) || - !Ecto.assoc_loaded?(device.site.organization) - - if needs_preload do - Repo.preload(device, site: :organization) - else - device - end - end - - defp resolve_snmpv3_config(device) do + # Credentials are already denormalized on the device - just read them! %{ - security_level: resolve_snmpv3_security_level(device), - username: resolve_snmpv3_username(device), - auth_protocol: resolve_snmpv3_auth_protocol(device), - auth_password: resolve_snmpv3_auth_password(device), - priv_protocol: resolve_snmpv3_priv_protocol(device), - priv_password: resolve_snmpv3_priv_password(device), - source: determine_snmpv3_source(device) + security_level: device.snmpv3_security_level, + username: device.snmpv3_username, + auth_protocol: device.snmpv3_auth_protocol || "SHA-256", + auth_password: device.snmpv3_auth_password, + priv_protocol: device.snmpv3_priv_protocol || "AES", + priv_password: device.snmpv3_priv_password, + source: String.to_atom(device.snmpv3_credential_source || "organization") } end - defp resolve_snmpv3_security_level(device) do - device.snmpv3_security_level || - device.site.snmpv3_security_level || - device.site.organization.snmpv3_security_level - end - - defp resolve_snmpv3_username(device) do - device.snmpv3_username || - device.site.snmpv3_username || - device.site.organization.snmpv3_username - end - - defp resolve_snmpv3_auth_protocol(device) do - device.snmpv3_auth_protocol || - device.site.snmpv3_auth_protocol || - device.site.organization.snmpv3_auth_protocol || - "SHA-256" - end - - defp resolve_snmpv3_auth_password(device) do - device.snmpv3_auth_password || - device.site.snmpv3_auth_password || - device.site.organization.snmpv3_auth_password - end - - defp resolve_snmpv3_priv_protocol(device) do - device.snmpv3_priv_protocol || - device.site.snmpv3_priv_protocol || - device.site.organization.snmpv3_priv_protocol || - "AES" - end - - defp resolve_snmpv3_priv_password(device) do - device.snmpv3_priv_password || - device.site.snmpv3_priv_password || - device.site.organization.snmpv3_priv_password - end - - defp determine_snmpv3_source(device) do - cond do - device.snmpv3_username != nil -> :device - device.site.snmpv3_username != nil -> :site - device.site.organization.snmpv3_username != nil -> :organization - true -> :default - end - end - @doc """ Propagates SNMPv3 credential changes from a site to all devices in that site that inherit credentials from the site (credential_source = "site"). @@ -660,15 +479,13 @@ defmodule Towerops.Devices do in sites that have no site-level credentials. """ def propagate_organization_snmpv3_change(organization_id, attrs) do - empty_string = "" - + # With denormalized credentials, update all devices that inherit from the organization + # (devices with snmpv3_credential_source = "organization") devices_to_update = Repo.all( from(d in DeviceSchema, - join: s in assoc(d, :site), - where: s.organization_id == ^organization_id, - where: d.snmpv3_credential_source == "site", - where: is_nil(s.snmpv3_username) or s.snmpv3_username == ^empty_string + where: d.organization_id == ^organization_id, + where: d.snmpv3_credential_source == "organization" ) ) @@ -717,6 +534,9 @@ defmodule Towerops.Devices do defp check_device_quota(attrs, bypass_limits) do changeset = DeviceSchema.changeset(%DeviceSchema{}, attrs) + # Apply credential resolution + changeset = apply_credential_resolution(changeset) + if bypass_limits do {:ok, changeset} else @@ -724,20 +544,40 @@ defmodule Towerops.Devices do end end + defp apply_credential_resolution(changeset) do + # Extract organization_id and site_id from changeset + organization_id = Ecto.Changeset.get_field(changeset, :organization_id) + site_id = Ecto.Changeset.get_field(changeset, :site_id) + + if organization_id do + # Load organization and site (if present) + organization = Organizations.get_organization!(organization_id) + site = if site_id, do: Sites.get_site!(site_id) + + # Apply credential resolution + changeset + |> CredentialResolver.resolve_snmp_credentials(organization, site) + |> CredentialResolver.resolve_snmpv3_credentials(organization, site) + |> CredentialResolver.resolve_mikrotik_credentials(organization, site) + else + # No organization_id yet, skip credential resolution + changeset + end + end + defp do_check_quota(changeset) do - case Ecto.Changeset.fetch_change(changeset, :site_id) do - {:ok, site_id} -> - validate_device_quota(changeset, site_id) + case Ecto.Changeset.fetch_change(changeset, :organization_id) do + {:ok, organization_id} -> + validate_device_quota(changeset, organization_id) :error -> - # No site_id in changeset, let normal validation handle it + # No organization_id in changeset, let normal validation handle it {:ok, changeset} end end - defp validate_device_quota(changeset, site_id) do - site = site_id |> Sites.get_site!() |> Repo.preload(:organization) - organization = site.organization + defp validate_device_quota(changeset, organization_id) do + organization = Organizations.get_organization!(organization_id) case SubscriptionLimits.check_device_limit(organization) do {:ok, :within_limit} -> @@ -766,6 +606,9 @@ defmodule Towerops.Devices do changeset = DeviceSchema.changeset(device, attrs) + # Apply credential resolution + changeset = apply_credential_resolution(changeset) + # Validate MikroTik SSL requirement for cloud pollers changeset = validate_mikrotik_ssl_with_cloud_poller(changeset, device) @@ -903,16 +746,13 @@ defmodule Towerops.Devices do and site.snmp_community is nil). """ def propagate_organization_community_change(organization_id, new_community) do - # Find all devices in this org's sites that: - # 1. Inherit from site (source = "site") - # 2. Their site has no community string (so they fall back to org) + # With denormalized credentials, update all devices that inherit from the organization + # (devices with snmp_community_source = "organization") devices_to_update = Repo.all( from(d in DeviceSchema, - join: s in assoc(d, :site), - where: s.organization_id == ^organization_id, - where: d.snmp_community_source == "site", - where: is_nil(s.snmp_community) or s.snmp_community == "" + where: d.organization_id == ^organization_id, + where: d.snmp_community_source == "organization" ) ) diff --git a/lib/towerops/devices/credential_resolver.ex b/lib/towerops/devices/credential_resolver.ex new file mode 100644 index 00000000..7bc3bd20 --- /dev/null +++ b/lib/towerops/devices/credential_resolver.ex @@ -0,0 +1,219 @@ +defmodule Towerops.Devices.CredentialResolver do + @moduledoc """ + Resolves and denormalizes credentials for devices from organization/site hierarchy. + + This module populates device credential fields during create/update by resolving from + the organization/site hierarchy and storing the resolved values directly on the device. + + ## Design Philosophy + + Instead of runtime cascading (device → site → org), we: + 1. Resolve credentials ONCE during device create/update + 2. Store resolved values directly on device fields + 3. Track source in `*_source` fields ("organization", "site", "device") + 4. Propagate changes when org/site credentials change + + ## Benefits + + - **Performance**: No need to preload site/org on every access + - **Simplicity**: Just read `device.snmp_community` directly at runtime + - **Clear audit**: Source fields show where credentials came from + - **Easy propagation**: Find devices by source and update them + + ## Usage + + ### During device create/update: + + # In changeset pipeline + changeset + |> resolve_snmp_credentials(organization, site) + |> resolve_mikrotik_credentials(organization, site) + |> resolve_snmpv3_credentials(organization, site) + + ### At runtime (SNMP polling, etc): + + # Just read the fields directly - already resolved! + device.snmp_community # "public" (from org) + device.snmp_community_source # "organization" + + ### When org/site credentials change: + + # Propagate to devices that inherit from that source + Devices.propagate_organization_community_change(org_id, new_community) + """ + + import Ecto.Changeset + + alias Towerops.Organizations.Organization + alias Towerops.Sites.Site + + @doc """ + Resolves SNMP v2c credentials for a device changeset. + + Populates snmp_community, snmp_version, snmp_port on the device if they're not + explicitly set (or empty), using site → organization fallback. + + Updates snmp_community_source to track where the community came from. + """ + @spec resolve_snmp_credentials(Ecto.Changeset.t(), Organization.t(), Site.t() | nil) :: + Ecto.Changeset.t() + 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 + |> resolve_snmp_version(organization, site) + |> resolve_snmp_port(organization, site) + end + + defp resolve_snmp_version(changeset, organization, site) do + case get_field(changeset, :snmp_version) do + nil -> + version = resolve_value(site, organization, :snmp_version) || "2c" + put_change(changeset, :snmp_version, version) + + _value -> + changeset + end + end + + defp resolve_snmp_port(changeset, organization, site) do + case get_field(changeset, :snmp_port) do + nil -> + port = resolve_value(site, organization, :snmp_port) || 161 + put_change(changeset, :snmp_port, port) + + _value -> + changeset + end + end + + @doc """ + Resolves SNMPv3 credentials for a device changeset. + + Populates SNMPv3 fields (username, security_level, auth_protocol, etc.) if not set, + using site → organization fallback. + + Updates snmpv3_credential_source to track where credentials came from. + """ + @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 + end + + @doc """ + Resolves MikroTik API credentials for a device changeset. + + Populates MikroTik fields (username, password, port, use_ssl) if not set, + using site → organization fallback. + + Updates mikrotik_credential_source to track where credentials came from. + """ + @spec resolve_mikrotik_credentials(Ecto.Changeset.t(), Organization.t(), Site.t() | nil) :: + Ecto.Changeset.t() + def resolve_mikrotik_credentials(changeset, organization, site) do + # If user explicitly set username, keep it and mark as "device" + # Otherwise resolve from site → org and track source + case get_field(changeset, :mikrotik_username) do + nil -> + # Resolve from hierarchy + {username, source} = resolve_with_source(site, organization, :mikrotik_username) + password = resolve_value(site, organization, :mikrotik_password) + port = resolve_value(site, organization, :mikrotik_port) || 8729 + ssh_port = resolve_value(site, organization, :mikrotik_ssh_port) || 22 + use_ssl = resolve_value(site, organization, :mikrotik_use_ssl) + use_ssl = if use_ssl == nil, do: true, else: use_ssl + enabled = resolve_value(site, organization, :mikrotik_enabled) || false + + changeset + |> put_change(:mikrotik_username, username) + |> put_change(:mikrotik_password, password) + |> put_change(:mikrotik_port, port) + |> put_change(:mikrotik_ssh_port, ssh_port) + |> put_change(:mikrotik_use_ssl, use_ssl) + |> put_change(:mikrotik_enabled, enabled) + |> put_change(:mikrotik_credential_source, source) + + _value -> + # User set a value, mark as device-specific + put_change(changeset, :mikrotik_credential_source, "device") + end + end + + # 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"} + + Map.get(organization, field) != nil -> + {Map.get(organization, field), "organization"} + + true -> + {nil, "organization"} + end + end + + # 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) + end + end +end diff --git a/lib/towerops/devices/device.ex b/lib/towerops/devices/device.ex index 152fd28d..15ae5f28 100644 --- a/lib/towerops/devices/device.ex +++ b/lib/towerops/devices/device.ex @@ -15,6 +15,7 @@ defmodule Towerops.Devices.Device do alias Ecto.Association.NotLoaded alias Towerops.Agents.AgentAssignment alias Towerops.Encrypted.Binary + alias Towerops.Organizations.Organization alias Towerops.Sites.Site alias Towerops.Snmp.Device, as: SnmpDevice @@ -59,6 +60,7 @@ defmodule Towerops.Devices.Device do field :mikrotik_credential_source, :string, default: "site" belongs_to :site, Site + belongs_to :organization, Organization has_one :snmp_device, SnmpDevice has_many :agent_assignments, AgentAssignment @@ -97,8 +99,10 @@ defmodule Towerops.Devices.Device do mikrotik_use_ssl: boolean() | nil, mikrotik_enabled: boolean() | nil, mikrotik_credential_source: String.t() | nil, - site_id: Ecto.UUID.t(), - site: NotLoaded.t() | Site.t(), + site_id: Ecto.UUID.t() | nil, + site: NotLoaded.t() | Site.t() | nil, + organization_id: Ecto.UUID.t(), + organization: NotLoaded.t() | Organization.t() | nil, snmp_device: NotLoaded.t() | SnmpDevice.t() | nil, agent_assignments: NotLoaded.t() | [AgentAssignment.t()], inserted_at: DateTime.t(), @@ -114,6 +118,7 @@ defmodule Towerops.Devices.Device do :description, :display_order, :site_id, + :organization_id, :monitoring_enabled, :check_interval_seconds, :snmp_enabled, @@ -141,7 +146,7 @@ defmodule Towerops.Devices.Device do :mikrotik_enabled, :mikrotik_credential_source ]) - |> validate_required([:ip_address, :site_id]) + |> validate_required([:ip_address, :organization_id]) |> validate_name() |> validate_length(:name, min: 2, max: 200) |> validate_length(:description, max: 1000) @@ -150,10 +155,12 @@ defmodule Towerops.Devices.Device do |> validate_snmp() |> validate_snmpv3_fields() |> validate_mikrotik() + |> validate_site_belongs_to_organization() |> update_community_source() |> update_snmpv3_credential_source() |> update_mikrotik_credential_source() |> foreign_key_constraint(:site_id) + |> foreign_key_constraint(:organization_id) end defp validate_name(changeset) do @@ -191,6 +198,23 @@ defmodule Towerops.Devices.Device do |> :inet.parse_address() end + defp validate_site_belongs_to_organization(changeset) do + site_id = get_field(changeset, :site_id) + org_id = get_field(changeset, :organization_id) + + # If no site_id or org_id, no validation needed + if !site_id || !org_id do + changeset + else + # Verify that the site belongs to the same organization + case Towerops.Repo.get(Site, site_id) do + nil -> add_error(changeset, :site_id, "does not exist") + %{organization_id: ^org_id} -> changeset + _site -> add_error(changeset, :site_id, "must belong to the same organization") + end + end + end + defp validate_snmp(changeset) do snmp_enabled = get_field(changeset, :snmp_enabled) diff --git a/lib/towerops/organizations.ex b/lib/towerops/organizations.ex index 0fcf4c6a..0a1f68cf 100644 --- a/lib/towerops/organizations.ex +++ b/lib/towerops/organizations.ex @@ -114,52 +114,64 @@ defmodule Towerops.Organizations do Updates an organization. """ def update_organization(%Organization{} = organization, attrs) do - old_community = organization.snmp_community - old_mikrotik_username = organization.mikrotik_username - old_mikrotik_password = organization.mikrotik_password - old_mikrotik_port = organization.mikrotik_port - old_mikrotik_use_ssl = organization.mikrotik_use_ssl - old_mikrotik_enabled = organization.mikrotik_enabled + old_values = %{ + snmp_community: organization.snmp_community, + mikrotik_username: organization.mikrotik_username, + mikrotik_password: organization.mikrotik_password, + mikrotik_port: organization.mikrotik_port, + mikrotik_use_ssl: organization.mikrotik_use_ssl, + mikrotik_enabled: organization.mikrotik_enabled, + use_sites: organization.use_sites + } - case organization - |> Organization.changeset(attrs) - |> Repo.update() do - {:ok, updated_organization} = result -> - # If community string changed, propagate to inheriting devices - if updated_organization.snmp_community != old_community do - Towerops.Devices.propagate_organization_community_change( - updated_organization.id, - updated_organization.snmp_community - ) - end + with {:ok, updated_organization} <- organization |> Organization.changeset(attrs) |> Repo.update() do + propagate_organization_changes(updated_organization, old_values) + {:ok, updated_organization} + end + end - # If any MikroTik settings changed, propagate to inheriting devices - mikrotik_changed = - updated_organization.mikrotik_username != old_mikrotik_username || - updated_organization.mikrotik_password != old_mikrotik_password || - updated_organization.mikrotik_port != old_mikrotik_port || - updated_organization.mikrotik_use_ssl != old_mikrotik_use_ssl || - updated_organization.mikrotik_enabled != old_mikrotik_enabled + defp propagate_organization_changes(updated_organization, old_values) do + propagate_snmp_changes(updated_organization, old_values) + propagate_mikrotik_changes(updated_organization, old_values) + handle_sites_disabled(updated_organization, old_values) + end - if mikrotik_changed do - mikrotik_attrs = %{ - mikrotik_username: updated_organization.mikrotik_username, - mikrotik_password: updated_organization.mikrotik_password, - mikrotik_port: updated_organization.mikrotik_port, - mikrotik_use_ssl: updated_organization.mikrotik_use_ssl, - mikrotik_enabled: updated_organization.mikrotik_enabled - } + defp propagate_snmp_changes(updated_organization, %{snmp_community: old_community}) do + if updated_organization.snmp_community != old_community do + Towerops.Devices.propagate_organization_community_change( + updated_organization.id, + updated_organization.snmp_community + ) + end + end - Towerops.Devices.propagate_organization_mikrotik_change( - updated_organization.id, - mikrotik_attrs - ) - end + defp propagate_mikrotik_changes(updated_organization, old_values) do + mikrotik_changed = + updated_organization.mikrotik_username != old_values.mikrotik_username || + updated_organization.mikrotik_password != old_values.mikrotik_password || + updated_organization.mikrotik_port != old_values.mikrotik_port || + updated_organization.mikrotik_use_ssl != old_values.mikrotik_use_ssl || + updated_organization.mikrotik_enabled != old_values.mikrotik_enabled - result + if mikrotik_changed do + mikrotik_attrs = %{ + mikrotik_username: updated_organization.mikrotik_username, + mikrotik_password: updated_organization.mikrotik_password, + mikrotik_port: updated_organization.mikrotik_port, + mikrotik_use_ssl: updated_organization.mikrotik_use_ssl, + mikrotik_enabled: updated_organization.mikrotik_enabled + } - error -> - error + Towerops.Devices.propagate_organization_mikrotik_change( + updated_organization.id, + mikrotik_attrs + ) + end + end + + defp handle_sites_disabled(updated_organization, %{use_sites: old_use_sites}) do + if old_use_sites && !updated_organization.use_sites do + clear_all_site_assignments(updated_organization.id) end end @@ -397,6 +409,21 @@ defmodule Towerops.Organizations do end end + @doc """ + Clears all site_id assignments for devices in an organization when sites are disabled. + + Returns the number of device records updated. + """ + def clear_all_site_assignments(organization_id) do + Repo.update_all( + from(d in Device, where: d.organization_id == ^organization_id), + set: [ + site_id: nil, + updated_at: DateTime.truncate(DateTime.utc_now(), :second) + ] + ) + end + ## Authorization @doc """ diff --git a/lib/towerops/organizations/organization.ex b/lib/towerops/organizations/organization.ex index 074afc0e..ab01b139 100644 --- a/lib/towerops/organizations/organization.ex +++ b/lib/towerops/organizations/organization.ex @@ -25,6 +25,7 @@ defmodule Towerops.Organizations.Organization do field :name, :string field :slug, :string field :subscription_plan, :string, default: "free" + field :use_sites, :boolean, default: false # device) field :snmp_version, :string, default: "2c" @@ -61,6 +62,7 @@ defmodule Towerops.Organizations.Organization do name: String.t(), slug: String.t(), subscription_plan: String.t(), + use_sites: boolean(), snmp_version: String.t() | nil, snmp_community: String.t() | nil, snmp_port: integer(), @@ -91,6 +93,7 @@ defmodule Towerops.Organizations.Organization do |> cast(attrs, [ :name, :subscription_plan, + :use_sites, :default_agent_token_id, :snmp_version, :snmp_community, diff --git a/lib/towerops_web/channels/agent_channel.ex b/lib/towerops_web/channels/agent_channel.ex index 87c8432b..da38f203 100644 --- a/lib/towerops_web/channels/agent_channel.ex +++ b/lib/towerops_web/channels/agent_channel.ex @@ -629,7 +629,7 @@ defmodule ToweropsWeb.AgentChannel do defp verify_device_organization(device, organization_id) do # Cloud pollers (organization_id = nil) can access any device - if is_nil(organization_id) or device.site.organization_id == organization_id do + if is_nil(organization_id) or device.organization_id == organization_id do :ok else {:error, :wrong_organization} diff --git a/lib/towerops_web/components/layouts.ex b/lib/towerops_web/components/layouts.ex index 65034291..df86a13d 100644 --- a/lib/towerops_web/components/layouts.ex +++ b/lib/towerops_web/components/layouts.ex @@ -211,6 +211,7 @@ defmodule ToweropsWeb.Layouts do Dashboard <.nav_link + :if={@current_organization && @current_organization.use_sites} navigate={~p"/sites"} active={@active_page == "sites"} > @@ -374,6 +375,7 @@ defmodule ToweropsWeb.Layouts do <.icon name="hero-home" class="size-5" /> Dashboard <.mobile_nav_link + :if={@current_organization && @current_organization.use_sites} navigate={~p"/sites"} active={@active_page == "sites"} > diff --git a/lib/towerops_web/controllers/api/v1/devices_controller.ex b/lib/towerops_web/controllers/api/v1/devices_controller.ex index c6619bd7..dd85a1dc 100644 --- a/lib/towerops_web/controllers/api/v1/devices_controller.ex +++ b/lib/towerops_web/controllers/api/v1/devices_controller.ex @@ -139,12 +139,9 @@ defmodule ToweropsWeb.Api.V1.DevicesController do def show(conn, %{"id" => id}) do organization_id = conn.assigns.current_organization_id - device = - id - |> Devices.get_device!() - |> Towerops.Repo.preload(:site) + device = Devices.get_device!(id) - if device.site.organization_id == organization_id do + if device.organization_id == organization_id do json(conn, format_device_details(device)) else conn @@ -182,12 +179,9 @@ defmodule ToweropsWeb.Api.V1.DevicesController do def update(conn, %{"id" => id, "device" => device_params}) do organization_id = conn.assigns.current_organization_id - device = - id - |> Devices.get_device!() - |> Towerops.Repo.preload(:site) + device = Devices.get_device!(id) - if device.site.organization_id == organization_id do + if device.organization_id == organization_id do case Devices.update_device(device, device_params) do {:ok, updated_device} -> json(conn, format_device_details(updated_device)) @@ -228,12 +222,9 @@ defmodule ToweropsWeb.Api.V1.DevicesController do def delete(conn, %{"id" => id}) do organization_id = conn.assigns.current_organization_id - device = - id - |> Devices.get_device!() - |> Towerops.Repo.preload(:site) + device = Devices.get_device!(id) - if device.site.organization_id == organization_id do + if device.organization_id == organization_id do case Devices.delete_device(device) do {:ok, _device} -> json(conn, %{success: true}) diff --git a/lib/towerops_web/live/alert_live/index.html.heex b/lib/towerops_web/live/alert_live/index.html.heex index 88f72a89..c75f06af 100644 --- a/lib/towerops_web/live/alert_live/index.html.heex +++ b/lib/towerops_web/live/alert_live/index.html.heex @@ -130,7 +130,7 @@ <% end %> -
+
Site: <.link navigate={~p"/sites/#{alert.device.site.id}"} diff --git a/lib/towerops_web/live/device_live/form.ex b/lib/towerops_web/live/device_live/form.ex index e90dec05..5838a888 100644 --- a/lib/towerops_web/live/device_live/form.ex +++ b/lib/towerops_web/live/device_live/form.ex @@ -30,28 +30,21 @@ defmodule ToweropsWeb.DeviceLive.Form do limit -> current_devices >= limit end - # Redirect to sites page if no sites exist - if Enum.empty?(sites) do - {:ok, - socket - |> put_flash(:info, t_equipment("Please create a site before adding a device.")) - |> push_navigate(to: ~p"/sites/new")} - else - {:ok, - socket - |> assign(:organization, organization) - |> assign(:available_sites, sites) - |> assign(:available_agents, all_agents) - |> assign(:preselected_site_id, params["site_id"]) - |> assign(:snmp_test_result, nil) - |> assign(:duplicate_device, nil) - |> assign(:non_routable_ip_error, false) - |> assign(:device_quota, %{ - current: current_devices, - limit: device_limit, - at_limit: is_at_limit - })} - end + # Sites are now optional - devices can belong directly to organization + {:ok, + socket + |> assign(:organization, organization) + |> assign(:available_sites, sites) + |> assign(:available_agents, all_agents) + |> assign(:preselected_site_id, params["site_id"]) + |> assign(:snmp_test_result, nil) + |> assign(:duplicate_device, nil) + |> assign(:non_routable_ip_error, false) + |> assign(:device_quota, %{ + current: current_devices, + limit: device_limit, + at_limit: is_at_limit + })} end @impl true @@ -247,6 +240,11 @@ defmodule ToweropsWeb.DeviceLive.Form do @impl true def handle_event("save", %{"device" => device_params}, socket) do device_params = sanitize_device_params(device_params) + + # Inject organization_id from current context + organization_id = socket.assigns.organization.id + device_params = Map.put(device_params, "organization_id", organization_id) + save_device(socket, socket.assigns.live_action, device_params) end @@ -272,38 +270,57 @@ defmodule ToweropsWeb.DeviceLive.Form do @impl true def handle_event("test_snmp", _params, socket) do - # Get current form data from the changeset changeset = socket.assigns.form.source form_data = Ecto.Changeset.apply_changes(changeset) + {auth_password, priv_password} = resolve_snmpv3_passwords(changeset, form_data) + + device_map = build_device_map(form_data, auth_password, priv_password) + effective_agent_id = determine_effective_agent_id(device_map, socket.assigns) + + handle_snmp_test(socket, device_map, effective_agent_id) + end + + @impl true + def handle_event("trigger_discovery", _params, socket) do + device = socket.assigns.device + + if device.snmp_enabled do + _ = enqueue_discovery(device.id) + + {:noreply, + socket + |> put_flash(:info, t_equipment("Discovery started...")) + |> push_navigate(to: ~p"/devices/#{device.id}")} + else + {:noreply, put_flash(socket, :error, t_equipment("SNMP is not enabled for this device"))} + end + end + + defp resolve_snmpv3_passwords(changeset, form_data) do changes = changeset.changes + passwords_changed? = Map.has_key?(changes, :snmpv3_auth_password) or Map.has_key?(changes, :snmpv3_priv_password) - # Determine which credentials to use: - # 1. If password fields were changed in form, use new plain text values - # 2. If existing device and no changes, use decrypted database values - # 3. If new device, use form values - {auth_password, priv_password} = - cond do - # User changed passwords in form - use new plain text values - Map.has_key?(changes, :snmpv3_auth_password) or - Map.has_key?(changes, :snmpv3_priv_password) -> - {Map.get(changes, :snmpv3_auth_password), Map.get(changes, :snmpv3_priv_password)} + cond do + passwords_changed? -> + {Map.get(changes, :snmpv3_auth_password), Map.get(changes, :snmpv3_priv_password)} - # Existing device with no password changes - use database values - form_data.id -> - config = Devices.get_snmpv3_config(form_data.id) + form_data.id -> + get_existing_device_passwords(form_data.id) - case config do - nil -> {nil, nil} - config -> {config.auth_password, config.priv_password} - end + true -> + {Map.get(changes, :snmpv3_auth_password), Map.get(changes, :snmpv3_priv_password)} + end + end - # New device - use form values - true -> - {Map.get(changes, :snmpv3_auth_password), Map.get(changes, :snmpv3_priv_password)} - end + defp get_existing_device_passwords(device_id) do + case Devices.get_snmpv3_config(device_id) do + nil -> {nil, nil} + config -> {config.auth_password, config.priv_password} + end + end - # Convert to map for easier access - device_map = %{ + defp build_device_map(form_data, auth_password, priv_password) do + %{ "agent_token_id" => Map.get(form_data, :agent_token_id), "site_id" => form_data.site_id, "ip_address" => form_data.ip_address, @@ -317,55 +334,34 @@ defmodule ToweropsWeb.DeviceLive.Form do "snmpv3_priv_protocol" => form_data.snmpv3_priv_protocol, "snmpv3_priv_password" => priv_password } - - # Determine effective agent (device -> site -> organization -> cloud) - effective_agent_id = determine_effective_agent_id(device_map, socket.assigns) - - if effective_agent_id do - # Send test job to agent - test_id = "test:#{Ecto.UUID.generate()}" - - case send_credential_test_to_agent(effective_agent_id, device_map, test_id) do - :ok -> - # Subscribe to test result - Phoenix.PubSub.subscribe(Towerops.PubSub, "credential_test:#{test_id}") - - {:noreply, - socket - |> assign(:snmp_test_result, %{testing: true}) - |> assign(:test_id, test_id)} - - {:error, reason} -> - {:noreply, - assign(socket, :snmp_test_result, %{ - success: false, - message: "Failed to send test: #{reason}" - })} - end - else - {:noreply, - assign(socket, :snmp_test_result, %{ - success: false, - message: "No agent available for testing. Please assign an agent or configure organization defaults." - })} - end end - @impl true - def handle_event("trigger_discovery", _params, socket) do - device = socket.assigns.device + defp handle_snmp_test(socket, _device_map, nil) do + {:noreply, + assign(socket, :snmp_test_result, %{ + success: false, + message: "No agent available for testing. Please assign an agent or configure organization defaults." + })} + end - if device.snmp_enabled do - # Enqueue discovery job - _ = enqueue_discovery(device.id) + defp handle_snmp_test(socket, device_map, effective_agent_id) do + test_id = "test:#{Ecto.UUID.generate()}" - # Navigate to device show page where live updates will appear - {:noreply, - socket - |> put_flash(:info, t_equipment("Discovery started...")) - |> push_navigate(to: ~p"/devices/#{device.id}")} - else - {:noreply, put_flash(socket, :error, t_equipment("SNMP is not enabled for this device"))} + case send_credential_test_to_agent(effective_agent_id, device_map, test_id) do + :ok -> + Phoenix.PubSub.subscribe(Towerops.PubSub, "credential_test:#{test_id}") + + {:noreply, + socket + |> assign(:snmp_test_result, %{testing: true}) + |> assign(:test_id, test_id)} + + {:error, reason} -> + {:noreply, + assign(socket, :snmp_test_result, %{ + success: false, + message: "Failed to send test: #{reason}" + })} end end @@ -588,32 +584,8 @@ defmodule ToweropsWeb.DeviceLive.Form do end defp send_credential_test_to_agent(agent_token_id, device_map, test_id) do - # Build SNMP device config from device map (string keys) - # Convert port to integer (form params are strings) - require Logger + snmp_config = build_snmp_test_config(device_map) - port = - case device_map["snmp_port"] do - port when is_binary(port) -> String.to_integer(port) - port when is_integer(port) -> port - _ -> 161 - end - - snmp_config = %{ - ip: device_map["ip_address"] || "", - port: port, - version: device_map["snmp_version"] || "2c", - community: device_map["snmp_community"] || "", - v3_security_level: device_map["snmpv3_security_level"] || "", - v3_username: device_map["snmpv3_username"] || "", - v3_auth_protocol: device_map["snmpv3_auth_protocol"] || "", - v3_auth_password: device_map["snmpv3_auth_password"] || "", - v3_priv_protocol: device_map["snmpv3_priv_protocol"] || "", - v3_priv_password: device_map["snmpv3_priv_password"] || "" - } - - # Broadcast credential test request to agent via PubSub - # The agent channel will pick this up and send the test job Phoenix.PubSub.broadcast( Towerops.PubSub, "agent:#{agent_token_id}:credential_test", @@ -621,6 +593,27 @@ defmodule ToweropsWeb.DeviceLive.Form do ) end + defp build_snmp_test_config(device_map) do + %{ + ip: get_value(device_map, "ip_address", ""), + port: normalize_port(device_map["snmp_port"]), + version: get_value(device_map, "snmp_version", "2c"), + community: get_value(device_map, "snmp_community", ""), + v3_security_level: get_value(device_map, "snmpv3_security_level", ""), + v3_username: get_value(device_map, "snmpv3_username", ""), + v3_auth_protocol: get_value(device_map, "snmpv3_auth_protocol", ""), + v3_auth_password: get_value(device_map, "snmpv3_auth_password", ""), + v3_priv_protocol: get_value(device_map, "snmpv3_priv_protocol", ""), + v3_priv_password: get_value(device_map, "snmpv3_priv_password", "") + } + end + + defp get_value(map, key, default), do: map[key] || default + + defp normalize_port(port) when is_binary(port), do: String.to_integer(port) + defp normalize_port(port) when is_integer(port), do: port + defp normalize_port(_), do: 161 + # Check if a non-routable IP is being used with cloud poller # Skip this check in dev mode or for specific users defp check_non_routable_ip_cloud_error(device_params, assigns) do diff --git a/lib/towerops_web/live/device_live/form.html.heex b/lib/towerops_web/live/device_live/form.html.heex index beab19f9..2606d967 100644 --- a/lib/towerops_web/live/device_live/form.html.heex +++ b/lib/towerops_web/live/device_live/form.html.heex @@ -165,15 +165,17 @@
-
+
<.input field={@form[:site_id]} type="select" - label="Site" - required - prompt="Select a site" + label="Site (Optional)" + prompt="No site (ungrouped)" options={Enum.map(@available_sites, &{&1.name, &1.id})} /> +

+ Optionally group this device into a site (office, datacenter, etc.) +

diff --git a/lib/towerops_web/live/device_live/index.ex b/lib/towerops_web/live/device_live/index.ex index dbeefe93..32682b3a 100644 --- a/lib/towerops_web/live/device_live/index.ex +++ b/lib/towerops_web/live/device_live/index.ex @@ -230,7 +230,12 @@ defmodule ToweropsWeb.DeviceLive.Index do end) |> Enum.sort_by( fn {site, _devices, _stats} -> - {site.display_order || 999_999, site.name} + if site do + {site.display_order || 999_999, site.name} + else + # Put devices without sites first (display_order 0) + {0, ""} + end end, :asc ) diff --git a/lib/towerops_web/live/device_live/index.html.heex b/lib/towerops_web/live/device_live/index.html.heex index 6fbad8b4..2657b652 100644 --- a/lib/towerops_web/live/device_live/index.html.heex +++ b/lib/towerops_web/live/device_live/index.html.heex @@ -224,12 +224,12 @@ if(index == 0, do: "border-gray-200", else: "border-gray-200"), "dark:border-white/10" ]} - data-site-id={site.id} + data-site-id={if site, do: site.id, else: "no-site"} data-site-position={index + 1} - draggable={if @reorder_mode, do: "true", else: "false"} + draggable={if @reorder_mode && site, do: "true", else: "false"} >
+ <% "sites" -> %> +
+

+ Sites +

+ +
+

+ Sites are an optional organizational feature that lets you group devices by physical or logical + locations such as offices, datacenters, customer locations, or network zones. Sites are disabled + by default to keep things simple for single-location deployments. +

+ +

+ When to Enable Sites +

+ +

+ Consider enabling sites if you: +

+ +
    +
  • + + Manage multiple physical locations + + - Offices, datacenters, retail stores, etc. +
    • +
  • + + Want to group credentials by location + + - Different SNMP communities or credentials per site +
    • +
  • + + Need location-based monitoring + + - Assign different remote pollers to different sites +
    • +
  • + + Organize by customer or department + + - Logical groupings for multi-tenant or large networks +
    • +
+ +

+ When to Skip Sites +

+ +

+ You can skip enabling sites if you: +

+ +
    +
  • + Have a single location + - All devices in one office or datacenter +
    • +
  • + Want a simpler setup + - Devices belong directly to your organization without extra grouping +
    • +
  • + + Don't need location-based credentials + + - Organization-level credentials work for all devices +
    • +
+ +
+
+ <.icon + name="hero-information-circle" + class="h-5 w-5 text-blue-600 dark:text-blue-400 flex-shrink-0 mt-0.5" + /> +
+

+ Note +

+

+ Sites are optional and disabled by default. You can start without sites and enable them + later if your needs change. When sites are disabled, all devices belong directly to + your organization. +

+
+
+
+ +

+ How Sites Work +

+ +

+ When sites are enabled, devices can be organized into sites, and credentials (SNMP communities, + usernames, passwords) can be set at three levels: +

+ +
+
+

+ 1. Organization Level +

+

+ Default credentials that apply to all devices across all sites in your organization. +

+
+ +
+

+ 2. Site Level (when sites enabled) +

+

+ Override organization defaults for all devices at a specific site. Useful when different + locations have different network configurations. +

+
+ +
+

+ 3. Device Level +

+

+ Override organization or site credentials for a specific device. Useful for devices with + unique configurations. +

+
+
+ +

+ Enabling Sites +

+ +
+ +
+
+
+ 1 +
+
+
+

+ Navigate to Organization Settings +

+

+ Go to + <.code>Settings + → + <.code>Organization + and scroll to the "Site Organization" section. +

+
+
+ + +
+
+
+ 2 +
+
+
+

+ Enable the "Use Sites" Toggle +

+

+ Check the box for "Use sites to organize devices" and save your settings. +

+
+
+ + +
+
+
+ 3 +
+
+
+

+ Create Your First Site +

+

+ Navigate to + <.code>Sites + → + <.code>Add Site + to create your first site. Give it a descriptive name like "Main Office" or "Dallas Datacenter". +

+
+
+ + +
+
+
+ 4 +
+
+
+

+ Assign Devices to Sites +

+

+ When creating or editing devices, you can now assign them to specific sites. The site + selector will appear in the device form. +

+
+
+
+ +
+
+ <.icon + name="hero-exclamation-triangle" + class="h-5 w-5 text-yellow-600 dark:text-yellow-400 flex-shrink-0 mt-0.5" + /> +
+

+ Disabling Sites +

+

+ If you disable sites after enabling them, all devices will be removed from their sites + and assigned directly to the organization. Site assignments will be lost, but devices + will remain in your organization and continue to be monitored. +

+
+
+
+
+
<% "cloud-pollers" -> %>

diff --git a/lib/towerops_web/live/org/settings_live.html.heex b/lib/towerops_web/live/org/settings_live.html.heex index f1b4940c..8b7e60c4 100644 --- a/lib/towerops_web/live/org/settings_live.html.heex +++ b/lib/towerops_web/live/org/settings_live.html.heex @@ -49,6 +49,50 @@

+ +
+
+

+ Site Organization +

+

+ Enable site organization to group devices into physical locations (offices, datacenters, etc.). +

+
+ +
+ <.input + field={@form[:use_sites]} + type="checkbox" + label="Use sites to organize devices" + /> +

+ When enabled, you can organize devices into sites. When disabled, all devices belong directly to the organization. +

+ + <%= if @organization.use_sites && @form[:use_sites].value == false do %> +
+
+
+ <.icon name="hero-exclamation-triangle" class="h-5 w-5 text-amber-400" /> +
+
+

+ Warning: Disabling Sites +

+
+

+ All devices will be removed from their current sites and assigned directly to the organization. + Site assignments will be lost, but devices will remain in the organization. +

+
+
+
+
+ <% end %> +
+
+
diff --git a/priv/repo/migrations/20260204182005_add_organization_id_to_devices.exs b/priv/repo/migrations/20260204182005_add_organization_id_to_devices.exs new file mode 100644 index 00000000..aadbe03a --- /dev/null +++ b/priv/repo/migrations/20260204182005_add_organization_id_to_devices.exs @@ -0,0 +1,39 @@ +defmodule Towerops.Repo.Migrations.AddOrganizationIdToDevices do + use Ecto.Migration + + def up do + # 1. Add nullable organization_id column + alter table(:devices) do + add :organization_id, references(:organizations, type: :binary_id, on_delete: :delete_all) + end + + # 2. Backfill from existing site relationships + execute """ + UPDATE devices d + SET organization_id = s.organization_id + FROM sites s + WHERE d.site_id = s.id + """ + + # 3. Make organization_id NOT NULL (data is backfilled) + alter table(:devices) do + modify :organization_id, :binary_id, null: false + end + + # 4. Add indexes + create index(:devices, [:organization_id]) + create index(:devices, [:organization_id, :display_order, :name]) + + # Note: We validate site belongs to same org in application code (changeset) + # PostgreSQL doesn't support subqueries in CHECK constraints + end + + def down do + drop index(:devices, [:organization_id, :display_order, :name]) + drop index(:devices, [:organization_id]) + + alter table(:devices) do + remove :organization_id + end + end +end diff --git a/priv/repo/migrations/20260204182038_add_use_sites_to_organizations.exs b/priv/repo/migrations/20260204182038_add_use_sites_to_organizations.exs new file mode 100644 index 00000000..5eadd29e --- /dev/null +++ b/priv/repo/migrations/20260204182038_add_use_sites_to_organizations.exs @@ -0,0 +1,29 @@ +defmodule Towerops.Repo.Migrations.AddUseSitesToOrganizations do + use Ecto.Migration + + def up do + # Add use_sites boolean field (default false) + alter table(:organizations) do + add :use_sites, :boolean, default: false, null: false + end + + # Backfill: Orgs with 2+ sites OR 1 named site (not "Default") → use_sites = true + # This detects existing multi-site setups + execute """ + UPDATE organizations + SET use_sites = true + WHERE id IN ( + SELECT organization_id + FROM sites + GROUP BY organization_id + HAVING COUNT(*) > 1 OR (COUNT(*) = 1 AND MAX(name) != 'Default') + ) + """ + end + + def down do + alter table(:organizations) do + remove :use_sites + end + end +end diff --git a/priv/repo/migrations/20260204182109_make_site_id_nullable_on_devices.exs b/priv/repo/migrations/20260204182109_make_site_id_nullable_on_devices.exs new file mode 100644 index 00000000..5aa25d9c --- /dev/null +++ b/priv/repo/migrations/20260204182109_make_site_id_nullable_on_devices.exs @@ -0,0 +1,25 @@ +defmodule Towerops.Repo.Migrations.MakeSiteIdNullableOnDevices do + use Ecto.Migration + + def up do + alter table(:devices) do + modify :site_id, :binary_id, null: true, from: {:binary_id, null: false} + end + end + + def down do + # Cannot rollback if NULL site_id values exist + execute """ + DO $$ + BEGIN + IF EXISTS (SELECT 1 FROM devices WHERE site_id IS NULL) THEN + RAISE EXCEPTION 'Cannot revert: devices with NULL site_id exist'; + END IF; + END $$; + """ + + alter table(:devices) do + modify :site_id, :binary_id, null: false + end + end +end diff --git a/priv/repo/migrations/20260204183342_backfill_device_credentials.exs b/priv/repo/migrations/20260204183342_backfill_device_credentials.exs new file mode 100644 index 00000000..4b862bee --- /dev/null +++ b/priv/repo/migrations/20260204183342_backfill_device_credentials.exs @@ -0,0 +1,137 @@ +defmodule Towerops.Repo.Migrations.BackfillDeviceCredentials do + use Ecto.Migration + + def up do + # Backfill SNMP v2c credentials from site → organization hierarchy + execute """ + UPDATE devices d + SET + snmp_community = COALESCE( + NULLIF(d.snmp_community, ''), + s.snmp_community, + o.snmp_community + ), + snmp_community_source = CASE + WHEN d.snmp_community IS NOT NULL AND d.snmp_community != '' THEN 'device' + WHEN s.snmp_community IS NOT NULL THEN 'site' + ELSE 'organization' + END, + snmp_version = COALESCE( + NULLIF(d.snmp_version, ''), + s.snmp_version, + o.snmp_version, + '2c' + ), + snmp_port = COALESCE( + d.snmp_port, + s.snmp_port, + o.snmp_port, + 161 + ) + FROM sites s + INNER JOIN organizations o ON s.organization_id = o.id + WHERE d.site_id = s.id + """ + + # Backfill SNMPv3 credentials from site → organization hierarchy + execute """ + UPDATE devices d + SET + snmpv3_username = COALESCE( + NULLIF(d.snmpv3_username, ''), + s.snmpv3_username, + o.snmpv3_username + ), + snmpv3_security_level = COALESCE( + NULLIF(d.snmpv3_security_level, ''), + s.snmpv3_security_level, + o.snmpv3_security_level + ), + snmpv3_auth_protocol = COALESCE( + NULLIF(d.snmpv3_auth_protocol, ''), + s.snmpv3_auth_protocol, + o.snmpv3_auth_protocol, + 'SHA-256' + ), + snmpv3_auth_password = COALESCE( + d.snmpv3_auth_password, + s.snmpv3_auth_password, + o.snmpv3_auth_password + ), + snmpv3_priv_protocol = COALESCE( + NULLIF(d.snmpv3_priv_protocol, ''), + s.snmpv3_priv_protocol, + o.snmpv3_priv_protocol, + 'AES' + ), + snmpv3_priv_password = COALESCE( + d.snmpv3_priv_password, + s.snmpv3_priv_password, + o.snmpv3_priv_password + ), + snmpv3_credential_source = CASE + WHEN d.snmpv3_username IS NOT NULL AND d.snmpv3_username != '' THEN 'device' + WHEN s.snmpv3_username IS NOT NULL THEN 'site' + ELSE 'organization' + END + FROM sites s + INNER JOIN organizations o ON s.organization_id = o.id + WHERE d.site_id = s.id + AND d.snmp_version = '3' + """ + + # Backfill MikroTik credentials from site → organization hierarchy + execute """ + UPDATE devices d + SET + mikrotik_username = COALESCE( + d.mikrotik_username, + s.mikrotik_username, + o.mikrotik_username + ), + mikrotik_password = COALESCE( + d.mikrotik_password, + s.mikrotik_password, + o.mikrotik_password + ), + mikrotik_port = COALESCE( + d.mikrotik_port, + s.mikrotik_port, + o.mikrotik_port, + 8729 + ), + mikrotik_ssh_port = COALESCE( + d.mikrotik_ssh_port, + s.mikrotik_ssh_port, + o.mikrotik_ssh_port, + 22 + ), + mikrotik_use_ssl = COALESCE( + d.mikrotik_use_ssl, + s.mikrotik_use_ssl, + o.mikrotik_use_ssl, + true + ), + mikrotik_enabled = COALESCE( + d.mikrotik_enabled, + s.mikrotik_enabled, + o.mikrotik_enabled, + false + ), + mikrotik_credential_source = CASE + WHEN d.mikrotik_username IS NOT NULL THEN 'device' + WHEN s.mikrotik_username IS NOT NULL THEN 'site' + ELSE 'organization' + END + FROM sites s + INNER JOIN organizations o ON s.organization_id = o.id + WHERE d.site_id = s.id + """ + end + + def down do + # This migration is not reversible - we can't "un-denormalize" the data + # The credentials are now explicitly stored on devices + :ok + end +end diff --git a/test/support/fixtures/devices_fixtures.ex b/test/support/fixtures/devices_fixtures.ex new file mode 100644 index 00000000..1134fa26 --- /dev/null +++ b/test/support/fixtures/devices_fixtures.ex @@ -0,0 +1,46 @@ +defmodule Towerops.DevicesFixtures do + @moduledoc """ + This module defines test helpers for creating devices with all required fields. + """ + + alias Towerops.Devices + + @doc """ + Generate a device with all required fields. + """ + def device_fixture(attrs \\ %{}) do + # Ensure organization and site exist + organization = + attrs[:organization] || Map.get(attrs, "organization") || + Towerops.OrganizationsFixtures.organization_fixture() + + site = + attrs[:site] || Map.get(attrs, "site") || + Towerops.OrganizationsFixtures.site_fixture(%{organization_id: organization.id}) + + # Default device attributes + default_attrs = %{ + name: "Test Device #{System.unique_integer([:positive])}", + ip_address: "192.168.1.#{:rand.uniform(254)}", + snmp_enabled: true, + snmp_version: "2c", + snmp_community: "public", + snmp_port: 161, + site_id: site.id, + organization_id: organization.id + } + + # Merge with provided attrs, converting string keys if needed + merged_attrs = + Map.merge( + default_attrs, + attrs |> Map.drop([:organization, :site, "organization", "site"]) |> Map.new(fn {k, v} -> {to_atom_key(k), v} end) + ) + + {:ok, device} = Devices.create_device(merged_attrs, bypass_limits: true) + device + end + + defp to_atom_key(key) when is_atom(key), do: key + defp to_atom_key(key) when is_binary(key), do: String.to_existing_atom(key) +end diff --git a/test/towerops/snmp/state_sensor_test.exs b/test/towerops/snmp/state_sensor_test.exs index 3d94ceec..052a7895 100644 --- a/test/towerops/snmp/state_sensor_test.exs +++ b/test/towerops/snmp/state_sensor_test.exs @@ -26,7 +26,8 @@ defmodule Towerops.Snmp.StateSensorTest do snmp_version: "2c", snmp_community: "public", snmp_port: 161, - site_id: site.id + site_id: site.id, + organization_id: organization.id }) snmp_device = diff --git a/test/towerops/workers/discovery_worker_test.exs b/test/towerops/workers/discovery_worker_test.exs index 494b86a0..e8b26bf7 100644 --- a/test/towerops/workers/discovery_worker_test.exs +++ b/test/towerops/workers/discovery_worker_test.exs @@ -29,7 +29,8 @@ defmodule Towerops.Workers.DiscoveryWorkerTest do snmp_version: "2c", snmp_community: "public", snmp_port: 161, - site_id: site.id + site_id: site.id, + organization_id: organization.id }) {:ok, device: device, org: organization}