refactor: centralize LiveView access control checks

Extract duplicate access control logic from multiple LiveView files into
a reusable AccessControl helper module.

**Changes:**
- NEW: lib/towerops_web/live/helpers/access_control.ex
- NEW: test/towerops_web/live/helpers/access_control_test.exs
- Refactor device_live/index.ex to use AccessControl.verify_site_access/2
  and verify_device_access/2
- Refactor device_live/form.ex to use AccessControl.verify_device_access/2
- Refactor alert_live/index.ex to use AccessControl.verify_alert_access/2
- Refactor device_live/show.ex to use AccessControl.verify_device_access/2
- Remove unused Repo aliases from refactored files

**Benefits:**
- Reduces code duplication across 7+ locations
- Centralizes security-critical access checks
- Improves testability (helper module has >90% coverage)
- Consistent error handling across all LiveViews

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This commit is contained in:
Graham McIntire 2026-02-04 17:51:05 -06:00
parent d16f09666f
commit 6aab59dcf5
No known key found for this signature in database
6 changed files with 368 additions and 93 deletions

View file

@ -3,7 +3,7 @@ defmodule ToweropsWeb.AlertLive.Index do
use ToweropsWeb, :live_view
alias Towerops.Alerts
alias Towerops.Repo
alias ToweropsWeb.Live.Helpers.AccessControl
@impl true
def mount(_params, _session, socket) do
@ -38,19 +38,15 @@ defmodule ToweropsWeb.AlertLive.Index do
organization = socket.assigns.current_scope.organization
user_id = socket.assigns.current_scope.user.id
case Alerts.get_alert(id) do
nil ->
case AccessControl.verify_alert_access(id, organization.id) do
{:ok, alert} ->
perform_acknowledge_alert(socket, alert, user_id, organization.id)
{:error, :not_found} ->
{:noreply, put_flash(socket, :error, t_equipment("Alert not found"))}
alert ->
# Preload device and site to check organization access
alert = Repo.preload(alert, device: [site: :organization])
if alert.device.site.organization_id == organization.id do
perform_acknowledge_alert(socket, alert, user_id, organization.id)
else
{:noreply, put_flash(socket, :error, t_equipment("You don't have access to this alert"))}
end
{:error, :unauthorized} ->
{:noreply, put_flash(socket, :error, t_equipment("You don't have access to this alert"))}
end
end

View file

@ -7,8 +7,10 @@ defmodule ToweropsWeb.DeviceLive.Form do
alias Towerops.Devices
alias Towerops.Devices.Device, as: DeviceSchema
alias Towerops.Organizations.SubscriptionLimits
alias Towerops.Repo
alias Towerops.Sites
alias Towerops.Workers.DiscoveryWorker
alias ToweropsWeb.Live.Helpers.AccessControl
@impl true
def mount(params, _session, socket) do
@ -96,24 +98,21 @@ defmodule ToweropsWeb.DeviceLive.Form do
defp apply_action(socket, :edit, %{"id" => id}) do
organization = socket.assigns.current_scope.organization
case Devices.get_device(id) do
nil ->
case AccessControl.verify_device_access(id, organization.id) do
{:ok, device} ->
# Preload necessary associations for agent resolution
device = Repo.preload(device, site: [organization: :default_agent_token])
apply_edit_action(socket, device)
{:error, :not_found} ->
socket
|> put_flash(:error, t_equipment("Device not found"))
|> push_navigate(to: ~p"/devices")
device ->
# Check organization access using denormalized organization_id field
# Works for both devices with sites and devices without sites
if device.organization_id == organization.id do
# Preload necessary associations for agent resolution
device = Towerops.Repo.preload(device, site: [organization: :default_agent_token])
apply_edit_action(socket, device)
else
socket
|> put_flash(:error, t_equipment("You don't have access to this device"))
|> push_navigate(to: ~p"/devices")
end
{:error, :unauthorized} ->
socket
|> put_flash(:error, t_equipment("You don't have access to this device"))
|> push_navigate(to: ~p"/devices")
end
end
@ -153,7 +152,7 @@ defmodule ToweropsWeb.DeviceLive.Form do
mikrotik_config = Devices.get_mikrotik_config(device)
# Check if device is MikroTik (based on SNMP discovery)
device_with_snmp = Towerops.Repo.preload(device, :snmp_device, force: true)
device_with_snmp = Repo.preload(device, :snmp_device, force: true)
is_mikrotik = mikrotik_device?(device_with_snmp)
# Determine monitoring mode based on current snmp_enabled value

View file

@ -4,10 +4,10 @@ defmodule ToweropsWeb.DeviceLive.Index do
alias Towerops.Devices
alias Towerops.Organizations.SubscriptionLimits
alias Towerops.Repo
alias Towerops.Sites
alias Towerops.Snmp
alias Towerops.Workers.DiscoveryWorker
alias ToweropsWeb.Live.Helpers.AccessControl
@impl true
def mount(_params, _session, socket) do
@ -101,16 +101,15 @@ defmodule ToweropsWeb.DeviceLive.Index do
organization = socket.assigns.current_scope.organization
# Verify site belongs to current organization
case Sites.get_site(site_id) do
nil ->
case AccessControl.verify_site_access(site_id, organization.id) do
{:ok, _site} ->
perform_site_reorder(socket, site_id, new_position, organization.id)
{:error, :not_found} ->
{:noreply, put_flash(socket, :error, t_equipment("Site not found"))}
site ->
if site.organization_id == organization.id do
perform_site_reorder(socket, site_id, new_position, organization.id)
else
{:noreply, put_flash(socket, :error, t_equipment("You don't have access to this site"))}
end
{:error, :unauthorized} ->
{:noreply, put_flash(socket, :error, t_equipment("You don't have access to this site"))}
end
end
@ -119,19 +118,15 @@ defmodule ToweropsWeb.DeviceLive.Index do
organization = socket.assigns.current_scope.organization
# Verify device belongs to current organization
case Devices.get_device(device_id) do
nil ->
case AccessControl.verify_device_access(device_id, organization.id) do
{:ok, _device} ->
perform_device_reorder(socket, device_id, new_position, organization.id)
{:error, :not_found} ->
{:noreply, put_flash(socket, :error, t_equipment("Device not found"))}
device ->
# Preload site to check organization access
device = Repo.preload(device, site: :organization)
if device.site.organization_id == organization.id do
perform_device_reorder(socket, device_id, new_position, organization.id)
else
{:noreply, put_flash(socket, :error, t_equipment("You don't have access to this device"))}
end
{:error, :unauthorized} ->
{:noreply, put_flash(socket, :error, t_equipment("You don't have access to this device"))}
end
end

View file

@ -10,6 +10,7 @@ defmodule ToweropsWeb.DeviceLive.Show do
alias Towerops.Monitoring
alias Towerops.Repo
alias Towerops.Snmp
alias ToweropsWeb.Live.Helpers.AccessControl
# SNMP interface type to category mappings
@interface_type_categories %{
@ -47,35 +48,32 @@ defmodule ToweropsWeb.DeviceLive.Show do
organization = socket.assigns.current_scope.organization
# Check if device exists and verify organization access
case Devices.get_device(id) do
nil ->
case AccessControl.verify_device_access(id, organization.id) do
{:ok, _device} ->
maybe_subscribe_and_schedule_refresh(socket, id)
tab = Map.get(params, "tab", "overview")
page = params |> Map.get("page", "1") |> String.to_integer()
socket =
socket
|> load_equipment_data(id)
|> assign(:active_tab, tab)
|> apply_backups_pagination(tab, page)
{:noreply, socket}
{:error, :not_found} ->
{:noreply,
socket
|> put_flash(:error, t_equipment("Device not found"))
|> push_navigate(to: ~p"/devices")}
device ->
# Check organization access using denormalized organization_id field
# Works for both devices with sites and devices without sites
if device.organization_id == organization.id do
maybe_subscribe_and_schedule_refresh(socket, id)
tab = Map.get(params, "tab", "overview")
page = params |> Map.get("page", "1") |> String.to_integer()
socket =
socket
|> load_equipment_data(id)
|> assign(:active_tab, tab)
|> apply_backups_pagination(tab, page)
{:noreply, socket}
else
{:noreply,
socket
|> put_flash(:error, t_equipment("You don't have access to this device"))
|> push_navigate(to: ~p"/devices")}
end
{:error, :unauthorized} ->
{:noreply,
socket
|> put_flash(:error, t_equipment("You don't have access to this device"))
|> push_navigate(to: ~p"/devices")}
end
end
@ -962,18 +960,18 @@ defmodule ToweropsWeb.DeviceLive.Show do
@impl true
def handle_event("download_backup", %{"id" => backup_id}, socket) do
backup = MikrotikBackups.get_backup!(backup_id)
organization = socket.assigns.current_scope.organization
# Verify the backup belongs to a device in the user's organization
device = Devices.get_device!(backup.device_id)
device_with_site = Repo.preload(device, site: :organization)
case AccessControl.verify_device_access(backup.device_id, organization.id) do
{:ok, device} ->
config_text = MikrotikBackups.decompress_config(backup.config_compressed)
filename = "#{device.name}_backup_#{Calendar.strftime(backup.backed_up_at, "%Y%m%d_%H%M%S")}.rsc"
if device_with_site.site.organization_id == socket.assigns.current_scope.organization.id do
config_text = MikrotikBackups.decompress_config(backup.config_compressed)
filename = "#{device.name}_backup_#{Calendar.strftime(backup.backed_up_at, "%Y%m%d_%H%M%S")}.rsc"
{:noreply, push_event(socket, "download", %{content: config_text, filename: filename, mime_type: "text/plain"})}
{:noreply, push_event(socket, "download", %{content: config_text, filename: filename, mime_type: "text/plain"})}
else
{:noreply, put_flash(socket, :error, t_equipment("You don't have permission to access this backup"))}
{:error, _} ->
{:noreply, put_flash(socket, :error, t_equipment("You don't have permission to access this backup"))}
end
end
@ -1127,24 +1125,24 @@ defmodule ToweropsWeb.DeviceLive.Show do
# Private helper for deleting backups
defp do_delete_backup(backup_id, socket) do
backup = MikrotikBackups.get_backup!(backup_id)
organization = socket.assigns.current_scope.organization
# Verify the backup belongs to a device in the user's organization
device = Devices.get_device!(backup.device_id)
device_with_site = Repo.preload(device, site: :organization)
case AccessControl.verify_device_access(backup.device_id, organization.id) do
{:ok, _device} ->
case MikrotikBackups.delete_backup(backup) do
{:ok, _} ->
{:noreply,
socket
|> put_flash(:info, t_equipment("Backup deleted successfully"))
|> load_equipment_data(socket.assigns.device.id)}
if device_with_site.site.organization_id == socket.assigns.current_scope.organization.id do
case MikrotikBackups.delete_backup(backup) do
{:ok, _} ->
{:noreply,
socket
|> put_flash(:info, t_equipment("Backup deleted successfully"))
|> load_equipment_data(socket.assigns.device.id)}
{:error, _changeset} ->
{:noreply, put_flash(socket, :error, t_equipment("Failed to delete backup"))}
end
{:error, _changeset} ->
{:noreply, put_flash(socket, :error, t_equipment("Failed to delete backup"))}
end
else
{:noreply, put_flash(socket, :error, t_equipment("You don't have permission to access this backup"))}
{:error, _} ->
{:noreply, put_flash(socket, :error, t_equipment("You don't have permission to access this backup"))}
end
end

View file

@ -0,0 +1,112 @@
defmodule ToweropsWeb.Live.Helpers.AccessControl do
@moduledoc """
Centralized organization-based access control for LiveViews.
Provides functions to verify that users have access to resources
(devices, sites, alerts) within their organization scope.
"""
alias Towerops.Alerts
alias Towerops.Devices
alias Towerops.Repo
alias Towerops.Sites
@doc """
Verifies that a device belongs to the specified organization.
Returns `{:ok, device}` if access is granted, or an error tuple.
## Examples
iex> verify_device_access(device_id, org_id)
{:ok, %Device{}}
iex> verify_device_access(device_id, wrong_org_id)
{:error, :unauthorized}
iex> verify_device_access("nonexistent", org_id)
{:error, :not_found}
"""
@spec verify_device_access(binary(), binary()) ::
{:ok, Devices.Device.t()} | {:error, :not_found | :unauthorized}
def verify_device_access(device_id, organization_id) do
case Devices.get_device(device_id) do
nil ->
{:error, :not_found}
device ->
if device.organization_id == organization_id do
{:ok, device}
else
{:error, :unauthorized}
end
end
end
@doc """
Verifies that a site belongs to the specified organization.
Returns `{:ok, site}` if access is granted, or an error tuple.
## Examples
iex> verify_site_access(site_id, org_id)
{:ok, %Site{}}
iex> verify_site_access(site_id, wrong_org_id)
{:error, :unauthorized}
iex> verify_site_access("nonexistent", org_id)
{:error, :not_found}
"""
@spec verify_site_access(binary(), binary()) ::
{:ok, Sites.Site.t()} | {:error, :not_found | :unauthorized}
def verify_site_access(site_id, organization_id) do
case Sites.get_site(site_id) do
nil ->
{:error, :not_found}
site ->
if site.organization_id == organization_id do
{:ok, site}
else
{:error, :unauthorized}
end
end
end
@doc """
Verifies that an alert's device belongs to the specified organization.
Returns `{:ok, alert}` if access is granted, or an error tuple.
The alert is preloaded with `device: [site: :organization]` for access checking.
## Examples
iex> verify_alert_access(alert_id, org_id)
{:ok, %Alert{device: %Device{site: %Site{organization: %Organization{}}}}}
iex> verify_alert_access(alert_id, wrong_org_id)
{:error, :unauthorized}
iex> verify_alert_access("nonexistent", org_id)
{:error, :not_found}
"""
@spec verify_alert_access(binary(), binary()) ::
{:ok, Alerts.Alert.t()} | {:error, :not_found | :unauthorized}
def verify_alert_access(alert_id, organization_id) do
case Alerts.get_alert(alert_id) do
nil ->
{:error, :not_found}
alert ->
alert = Repo.preload(alert, device: [site: :organization])
if alert.device.site.organization_id == organization_id do
{:ok, alert}
else
{:error, :unauthorized}
end
end
end
end

View file

@ -0,0 +1,175 @@
defmodule ToweropsWeb.Live.Helpers.AccessControlTest do
use Towerops.DataCase, async: true
import Towerops.AccountsFixtures
import Towerops.DevicesFixtures
import Towerops.OrganizationsFixtures
alias Towerops.Alerts
alias Towerops.Sites
alias ToweropsWeb.Live.Helpers.AccessControl
describe "verify_device_access/2" do
setup do
user1 = user_fixture()
user2 = user_fixture()
org1 = organization_fixture(user1.id)
org2 = organization_fixture(user2.id)
site1 = site_fixture(organization_id: org1.id)
site2 = site_fixture(organization_id: org2.id)
device1 = device_fixture(%{site: site1, organization: org1})
device2 = device_fixture(%{site: site2, organization: org2})
%{
org1: org1,
org2: org2,
site1: site1,
site2: site2,
device1: device1,
device2: device2
}
end
test "returns {:ok, device} when device belongs to organization", %{
device1: device,
org1: org
} do
assert {:ok, returned_device} = AccessControl.verify_device_access(device.id, org.id)
assert returned_device.id == device.id
end
test "returns {:error, :unauthorized} when device belongs to different organization", %{
device1: device,
org2: wrong_org
} do
assert {:error, :unauthorized} = AccessControl.verify_device_access(device.id, wrong_org.id)
end
test "returns {:error, :not_found} when device does not exist", %{org1: org} do
fake_id = Ecto.UUID.generate()
assert {:error, :not_found} = AccessControl.verify_device_access(fake_id, org.id)
end
end
describe "verify_site_access/2" do
setup do
user1 = user_fixture()
user2 = user_fixture()
org1 = organization_fixture(user1.id)
org2 = organization_fixture(user2.id)
site1 = site_fixture(organization_id: org1.id)
site2 = site_fixture(organization_id: org2.id)
%{
org1: org1,
org2: org2,
site1: site1,
site2: site2
}
end
test "returns {:ok, site} when site belongs to organization", %{
site1: site,
org1: org
} do
assert {:ok, returned_site} = AccessControl.verify_site_access(site.id, org.id)
assert returned_site.id == site.id
end
test "returns {:error, :unauthorized} when site belongs to different organization", %{
site1: site,
org2: wrong_org
} do
assert {:error, :unauthorized} = AccessControl.verify_site_access(site.id, wrong_org.id)
end
test "returns {:error, :not_found} when site does not exist", %{org1: org} do
fake_id = Ecto.UUID.generate()
assert {:error, :not_found} = AccessControl.verify_site_access(fake_id, org.id)
end
end
describe "verify_alert_access/2" do
setup do
user1 = user_fixture()
user2 = user_fixture()
org1 = organization_fixture(user1.id)
org2 = organization_fixture(user2.id)
site1 = site_fixture(organization_id: org1.id)
site2 = site_fixture(organization_id: org2.id)
device1 = device_fixture(%{site: site1, organization: org1})
device2 = device_fixture(%{site: site2, organization: org2})
alert1 = alert_fixture(device_id: device1.id)
alert2 = alert_fixture(device_id: device2.id)
%{
org1: org1,
org2: org2,
device1: device1,
device2: device2,
alert1: alert1,
alert2: alert2
}
end
test "returns {:ok, alert} when alert's device belongs to organization", %{
alert1: alert,
org1: org
} do
assert {:ok, returned_alert} = AccessControl.verify_alert_access(alert.id, org.id)
assert returned_alert.id == alert.id
# Verify preloading worked
assert returned_alert.device
assert returned_alert.device.site
assert returned_alert.device.site.organization
end
test "returns {:error, :unauthorized} when alert's device belongs to different organization",
%{
alert1: alert,
org2: wrong_org
} do
assert {:error, :unauthorized} = AccessControl.verify_alert_access(alert.id, wrong_org.id)
end
test "returns {:error, :not_found} when alert does not exist", %{org1: org} do
fake_id = Ecto.UUID.generate()
assert {:error, :not_found} = AccessControl.verify_alert_access(fake_id, org.id)
end
end
# Helper functions to create test data
defp site_fixture(attrs) do
{:ok, site} =
attrs
|> Enum.into(%{
name: "Test Site #{System.unique_integer([:positive])}"
})
|> Sites.create_site()
site
end
defp alert_fixture(attrs) do
{:ok, alert} =
attrs
|> Enum.into(%{
alert_type: :device_down,
message: "Test alert",
triggered_at: DateTime.utc_now()
})
|> Alerts.create_alert()
alert
end
end