From 4f0fa59be8f50304ff279f87bf2f4ea60ec69ab5 Mon Sep 17 00:00:00 2001
From: Graham McIntire <graham@mcintire.me>
Date: Thu, 10 Jul 2025 16:45:21 -0500
Subject: [PATCH] ip lookup on page load

---
 lib/aprsme_web/live/map_live/index.ex         | 153 +++---------------
 lib/aprsme_web/plugs/ip_geolocation.ex        | 133 +++++++++++++++
 lib/aprsme_web/router.ex                      |   2 +
 .../live/map_live/geolocation_test.exs        |  85 ++++++++++
 test/aprsme_web/plugs/ip_geolocation_test.exs | 132 +++++++++++++++
 5 files changed, 373 insertions(+), 132 deletions(-)
 create mode 100644 lib/aprsme_web/plugs/ip_geolocation.ex
 create mode 100644 test/aprsme_web/live/map_live/geolocation_test.exs
 create mode 100644 test/aprsme_web/plugs/ip_geolocation_test.exs

diff --git a/lib/aprsme_web/live/map_live/index.ex b/lib/aprsme_web/live/map_live/index.ex
index a1c459d..6bbea32 100644
--- a/lib/aprsme_web/live/map_live/index.ex
+++ b/lib/aprsme_web/live/map_live/index.ex
@@ -67,7 +67,7 @@ defmodule AprsmeWeb.MapLive.Index do
   end
 
   @impl true
-  def mount(params, _session, socket) do
+  def mount(params, session, socket) do
     require Logger
 
     if connected?(socket) do
@@ -86,10 +86,27 @@ defmodule AprsmeWeb.MapLive.Index do
     one_hour_ago = TimeUtils.one_day_ago()
 
     # Parse map state from URL parameters
-    {map_center, map_zoom} = parse_map_params(params)
+    {url_center, url_zoom} = parse_map_params(params)
 
-    Logger.debug("Parsed map params from URL: center=#{inspect(map_center)}, zoom=#{map_zoom}")
-    Logger.debug("Raw params: #{inspect(params)}")
+    # Check for IP geolocation in session
+    {map_center, map_zoom} = 
+      case session["ip_geolocation"] do
+        %{"lat" => lat, "lng" => lng} when is_number(lat) and is_number(lng) ->
+          # Use IP geolocation if available and no URL params specified
+          if params["lat"] || params["lng"] do
+            # URL params take precedence
+            {url_center, url_zoom}
+          else
+            # Use IP geolocation with closer zoom
+            {%{lat: lat, lng: lng}, 12}
+          end
+        _ ->
+          # No geolocation available, use URL params or defaults
+          {url_center, url_zoom}
+      end
+
+    Logger.debug("Final map params: center=#{inspect(map_center)}, zoom=#{map_zoom}")
+    Logger.debug("Raw params: #{inspect(params)}, session geo: #{inspect(session["ip_geolocation"])}")
 
     socket = assign_defaults(socket, one_hour_ago)
 
@@ -102,8 +119,6 @@ defmodule AprsmeWeb.MapLive.Index do
     if connected?(socket) do
       Endpoint.subscribe("aprs_messages")
       Phoenix.PubSub.subscribe(Aprsme.PubSub, "postgres:aprsme_packets")
-      # Defer IP geolocation to avoid blocking initial page load
-      Process.send_after(self(), :start_geolocation, 2000)
     end
 
     {:ok,
@@ -180,7 +195,6 @@ defmodule AprsmeWeb.MapLive.Index do
       packet_age_threshold: one_hour_ago,
       map_ready: false,
       historical_loaded: false,
-      pending_geolocation: nil,
       bounds_update_timer: nil,
       pending_bounds: nil,
       initial_bounds_loaded: false,
@@ -290,15 +304,6 @@ defmodule AprsmeWeb.MapLive.Index do
     # The calculated bounds might be too small/inaccurate
     Logger.debug("Map ready - waiting for JavaScript to send actual bounds before loading historical packets")
 
-    # If we have pending geolocation, zoom to it now
-    socket =
-      if socket.assigns.pending_geolocation do
-        %{lat: lat, lng: lng} = socket.assigns.pending_geolocation
-        push_event(socket, "zoom_to_location", %{lat: lat, lng: lng, zoom: 12})
-      else
-        socket
-      end
-
     {:noreply, socket}
   end
 
@@ -514,10 +519,6 @@ defmodule AprsmeWeb.MapLive.Index do
   @impl true
   def handle_info({:process_bounds_update, map_bounds}, socket), do: handle_info_process_bounds_update(map_bounds, socket)
 
-  def handle_info({:delayed_zoom, %{lat: lat, lng: lng}}, socket), do: handle_info_delayed_zoom(lat, lng, socket)
-
-  def handle_info({:ip_location, %{lat: lat, lng: lng}}, socket), do: handle_info_ip_location(lat, lng, socket)
-
   def handle_info(:initialize_replay, socket), do: handle_info_initialize_replay(socket)
 
   def handle_info(:cleanup_old_packets, socket), do: handle_cleanup_old_packets(socket)
@@ -526,8 +527,6 @@ defmodule AprsmeWeb.MapLive.Index do
 
   def handle_info({:postgres_packet, packet}, socket), do: handle_info_postgres_packet(packet, socket)
 
-  def handle_info(:start_geolocation, socket), do: handle_info_start_geolocation(socket)
-
   def handle_info({:load_historical_batch, batch_offset}, socket) do
     socket = load_historical_batch(socket, batch_offset)
     {:noreply, socket}
@@ -562,34 +561,6 @@ defmodule AprsmeWeb.MapLive.Index do
     end
   end
 
-  defp handle_info_delayed_zoom(lat, lng, socket) do
-    socket = push_event(socket, "zoom_to_location", %{lat: lat, lng: lng, zoom: 12})
-    {:noreply, socket}
-  end
-
-  defp handle_info_ip_location(lat, lng, socket) do
-    lat_float =
-      cond do
-        is_binary(lat) -> String.to_float(lat)
-        is_integer(lat) -> lat / 1.0
-        true -> lat
-      end
-
-    lng_float =
-      cond do
-        is_binary(lng) -> String.to_float(lng)
-        is_integer(lng) -> lng / 1.0
-        true -> lng
-      end
-
-    # Schedule a delayed zoom to give the user a moment to see the map
-    Process.send_after(self(), {:delayed_zoom, %{lat: lat_float, lng: lng_float}}, 500)
-
-    # We can still optimistically set the center and zoom
-    socket = assign(socket, map_center: %{lat: lat_float, lng: lng_float}, map_zoom: 12)
-    {:noreply, socket}
-  end
-
   defp handle_info_initialize_replay(socket) do
     if not socket.assigns.historical_loaded and socket.assigns.map_ready do
       # Only proceed if we have actual map bounds - don't use world bounds
@@ -694,52 +665,6 @@ defmodule AprsmeWeb.MapLive.Index do
     {:noreply, socket}
   end
 
-  defp handle_info_start_geolocation(socket) do
-    if geolocation_enabled?() do
-      ip_for_geolocation =
-        if Application.get_env(:aprsme, AprsmeWeb.Endpoint)[:code_reloader] do
-          # For testing geolocation in dev environment, use a public IP address.
-          # This will be geolocated to Mountain View, CA.
-          "8.8.8.8"
-        else
-          extract_ip(socket)
-        end
-
-      if valid_ip_for_geolocation?(ip_for_geolocation) do
-        start_geolocation_task(ip_for_geolocation)
-      end
-    end
-
-    {:noreply, socket}
-  end
-
-  defp geolocation_enabled? do
-    Application.get_env(:aprsme, :disable_aprs_connection, false) != true
-  end
-
-  defp extract_ip(socket) do
-    case socket.private[:connect_info][:peer_data][:address] do
-      {a, b, c, d} -> "#{a}.#{b}.#{c}.#{d}"
-      {a, b, c, d, e, f, g, h} -> "#{a}:#{b}:#{c}:#{d}:#{e}:#{f}:#{g}:#{h}"
-      _ -> nil
-    end
-  end
-
-  defp valid_ip_for_geolocation?(ip) do
-    ip && !String.starts_with?(ip, "127.") && !String.starts_with?(ip, "::1")
-  end
-
-  defp start_geolocation_task(ip) do
-    Task.start(fn ->
-      try do
-        get_ip_location(ip)
-      rescue
-        _error ->
-          send(self(), {:ip_location, @default_center})
-      end
-    end)
-  end
-
   # Handle replaying the next historical packet
 
   @impl true
@@ -1715,42 +1640,6 @@ defmodule AprsmeWeb.MapLive.Index do
     end
   end
 
-  # Get location from IP using ip-api.com
-  @spec get_ip_location(String.t() | nil) :: {float(), float()} | nil
-  defp get_ip_location(nil), do: nil
-
-  defp get_ip_location(ip) do
-    # Asynchronously fetch IP location
-    case Req.get("https://ip-api.com/json/#{ip}") do
-      {:ok, %Req.Response{status: 200, body: body}} -> handle_ip_api_response(body)
-      {:ok, _response} -> send_default_ip_location()
-      {:error, %{reason: :timeout}} -> send_default_ip_location()
-      {:error, _error} -> send_default_ip_location()
-    end
-  end
-
-  defp handle_ip_api_response(body) do
-    case Jason.decode(body) do
-      {:ok, %{"status" => "success", "lat" => lat, "lon" => lng}}
-      when is_number(lat) and is_number(lng) ->
-        if lat >= -90 and lat <= 90 and lng >= -180 and lng <= 180 do
-          lat_float = lat / 1.0
-          lng_float = lng / 1.0
-          send(self(), {:ip_location, %{lat: lat_float, lng: lng_float}})
-        else
-          send_default_ip_location()
-        end
-
-      {:ok, _} ->
-        send_default_ip_location()
-
-      {:error, _decode_error} ->
-        send_default_ip_location()
-    end
-  end
-
-  defp send_default_ip_location, do: send(self(), {:ip_location, @default_center})
-
   @impl true
   def terminate(_reason, socket) do
     if socket.assigns.buffer_timer, do: Process.cancel_timer(socket.assigns.buffer_timer)
diff --git a/lib/aprsme_web/plugs/ip_geolocation.ex b/lib/aprsme_web/plugs/ip_geolocation.ex
new file mode 100644
index 0000000..9d22da4
--- /dev/null
+++ b/lib/aprsme_web/plugs/ip_geolocation.ex
@@ -0,0 +1,133 @@
+defmodule AprsmeWeb.Plugs.IPGeolocation do
+  @moduledoc """
+  Plug that performs IP-based geolocation on initial page load.
+  Results are stored in the session to avoid repeated API calls.
+  """
+  import Plug.Conn
+  require Logger
+
+  @behaviour Plug
+
+  @impl true
+  def init(opts), do: opts
+
+  @impl true
+  def call(conn, _opts) do
+    # Only run for the main map page
+    if conn.request_path == "/" && conn.method == "GET" do
+      case get_session(conn, :ip_geolocation) do
+        nil ->
+          # No cached geolocation, fetch it
+          perform_geolocation(conn)
+
+        _cached ->
+          # Already have geolocation in session
+          conn
+      end
+    else
+      conn
+    end
+  end
+
+  defp perform_geolocation(conn) do
+    ip = get_client_ip(conn)
+
+    if valid_ip_for_geolocation?(ip) do
+      case fetch_ip_location(ip) do
+        {:ok, location} ->
+          put_session(conn, :ip_geolocation, location)
+
+        {:error, reason} ->
+          Logger.debug("IP geolocation failed for #{ip}: #{inspect(reason)}")
+          conn
+      end
+    else
+      conn
+    end
+  end
+
+  defp get_client_ip(conn) do
+    # Check for forwarded IP first (when behind proxy/load balancer)
+    forwarded_for = get_req_header(conn, "x-forwarded-for")
+
+    case forwarded_for do
+      [forwarded | _] ->
+        # Take the first IP from the X-Forwarded-For header
+        forwarded
+        |> String.split(",")
+        |> List.first()
+        |> String.trim()
+
+      [] ->
+        # Fall back to remote_ip
+        case conn.remote_ip do
+          {a, b, c, d} -> "#{a}.#{b}.#{c}.#{d}"
+          {a, b, c, d, e, f, g, h} -> "#{a}:#{b}:#{c}:#{d}:#{e}:#{f}:#{g}:#{h}"
+          _ -> nil
+        end
+    end
+  end
+
+  defp valid_ip_for_geolocation?(ip) when is_binary(ip) do
+    # Skip local/private IPs
+    not (String.starts_with?(ip, "127.") or
+           String.starts_with?(ip, "::1") or
+           String.starts_with?(ip, "10.") or
+           String.starts_with?(ip, "172.16.") or
+           String.starts_with?(ip, "172.17.") or
+           String.starts_with?(ip, "172.18.") or
+           String.starts_with?(ip, "172.19.") or
+           String.starts_with?(ip, "172.20.") or
+           String.starts_with?(ip, "172.21.") or
+           String.starts_with?(ip, "172.22.") or
+           String.starts_with?(ip, "172.23.") or
+           String.starts_with?(ip, "172.24.") or
+           String.starts_with?(ip, "172.25.") or
+           String.starts_with?(ip, "172.26.") or
+           String.starts_with?(ip, "172.27.") or
+           String.starts_with?(ip, "172.28.") or
+           String.starts_with?(ip, "172.29.") or
+           String.starts_with?(ip, "172.30.") or
+           String.starts_with?(ip, "172.31.") or
+           String.starts_with?(ip, "192.168."))
+  end
+
+  defp valid_ip_for_geolocation?(_), do: false
+
+  defp fetch_ip_location(ip) do
+    case Req.get("https://ip-api.com/json/#{ip}") do
+      {:ok, %Req.Response{status: 200, body: body}} ->
+        parse_ip_api_response(body)
+
+      {:ok, _response} ->
+        {:error, :invalid_response}
+
+      {:error, reason} ->
+        {:error, reason}
+    end
+  end
+
+  defp parse_ip_api_response(body) when is_map(body) do
+    case body do
+      %{"status" => "success", "lat" => lat, "lon" => lon}
+      when is_number(lat) and is_number(lon) ->
+        if lat >= -90 and lat <= 90 and lon >= -180 and lon <= 180 do
+          {:ok, %{"lat" => lat / 1.0, "lng" => lon / 1.0}}
+        else
+          {:error, :invalid_coordinates}
+        end
+
+      _data ->
+        {:error, :api_failure}
+    end
+  end
+
+  defp parse_ip_api_response(body) when is_binary(body) do
+    case Jason.decode(body) do
+      {:ok, decoded} -> parse_ip_api_response(decoded)
+      {:error, _} -> {:error, :json_decode_error}
+    end
+  end
+
+  defp parse_ip_api_response(_), do: {:error, :invalid_response}
+end
\ No newline at end of file
diff --git a/lib/aprsme_web/router.ex b/lib/aprsme_web/router.ex
index 4590d78..0e004a4 100644
--- a/lib/aprsme_web/router.ex
+++ b/lib/aprsme_web/router.ex
@@ -6,6 +6,7 @@ defmodule AprsmeWeb.Router do
   import Phoenix.LiveDashboard.Router
 
   alias AprsmeWeb.Plugs.RateLimiter
+  alias AprsmeWeb.Plugs.IPGeolocation
 
   pipeline :browser do
     plug :accepts, ["html"]
@@ -16,6 +17,7 @@ defmodule AprsmeWeb.Router do
     plug :put_secure_browser_headers
     plug :fetch_current_user
     plug AprsmeWeb.Plugs.SetLocale
+    plug IPGeolocation
     plug RateLimiter, scale: 60_000, limit: 200
   end
 
diff --git a/test/aprsme_web/live/map_live/geolocation_test.exs b/test/aprsme_web/live/map_live/geolocation_test.exs
new file mode 100644
index 0000000..a63bcef
--- /dev/null
+++ b/test/aprsme_web/live/map_live/geolocation_test.exs
@@ -0,0 +1,85 @@
+defmodule AprsmeWeb.MapLive.GeolocationTest do
+  use AprsmeWeb.ConnCase
+  import Phoenix.LiveViewTest
+  import Mox
+
+  setup :verify_on_exit!
+
+  describe "mount with IP geolocation" do
+    test "uses IP geolocation when available in session", %{conn: conn} do
+      # Add geolocation data to session
+      conn =
+        conn
+        |> init_test_session(%{
+          "ip_geolocation" => %{"lat" => 37.4056, "lng" => -122.0775}
+        })
+
+      {:ok, _view, html} = live(conn, "/")
+
+      # Verify the map is initialized with the geolocation data
+      assert html =~ "data-center"
+      assert html =~ "37.4056"
+      assert html =~ "-122.0775"
+      assert html =~ "data-zoom=\"12\""
+    end
+
+    test "uses default center when no geolocation in session", %{conn: conn} do
+      {:ok, _view, html} = live(conn, "/")
+
+      # Verify default center is used
+      assert html =~ "data-center"
+      assert html =~ "39.8283"
+      assert html =~ "-98.5795"
+      assert html =~ "data-zoom=\"5\""
+    end
+
+    test "URL parameters override IP geolocation", %{conn: conn} do
+      # Add geolocation data to session
+      conn =
+        conn
+        |> init_test_session(%{
+          "ip_geolocation" => %{"lat" => 37.4056, "lng" => -122.0775}
+        })
+
+      # Visit with URL parameters
+      {:ok, _view, html} = live(conn, "/?lat=40.7128&lng=-74.0060&z=10")
+
+      # Verify URL params take precedence
+      assert html =~ "40.7128"
+      assert html =~ "-74.0060"
+      assert html =~ "data-zoom=\"10\""
+    end
+
+    test "handles invalid geolocation data gracefully", %{conn: conn} do
+      # Add invalid geolocation data to session
+      conn =
+        conn
+        |> init_test_session(%{
+          "ip_geolocation" => %{"lat" => "invalid", "lng" => "invalid"}
+        })
+
+      {:ok, _view, html} = live(conn, "/")
+
+      # Should fall back to defaults
+      assert html =~ "39.8283"
+      assert html =~ "-98.5795"
+      assert html =~ "data-zoom=\"5\""
+    end
+
+    test "handles missing lat/lng in geolocation data", %{conn: conn} do
+      # Add incomplete geolocation data
+      conn =
+        conn
+        |> init_test_session(%{
+          "ip_geolocation" => %{"city" => "Mountain View"}
+        })
+
+      {:ok, _view, html} = live(conn, "/")
+
+      # Should fall back to defaults
+      assert html =~ "39.8283"
+      assert html =~ "-98.5795"
+      assert html =~ "data-zoom=\"5\""
+    end
+  end
+end
\ No newline at end of file
diff --git a/test/aprsme_web/plugs/ip_geolocation_test.exs b/test/aprsme_web/plugs/ip_geolocation_test.exs
new file mode 100644
index 0000000..c8bf21f
--- /dev/null
+++ b/test/aprsme_web/plugs/ip_geolocation_test.exs
@@ -0,0 +1,132 @@
+defmodule AprsmeWeb.Plugs.IPGeolocationTest do
+  use AprsmeWeb.ConnCase
+  use ExVCR.Mock, adapter: ExVCR.Adapter.Hackney
+
+  alias AprsmeWeb.Plugs.IPGeolocation
+
+  @test_ip {204, 110, 191, 254}  # Test IP address
+
+  describe "call/2" do
+    test "adds geolocation data to session when successful", %{conn: conn} do
+      use_cassette "ip_geolocation_success" do
+        conn =
+          conn
+          |> Map.put(:remote_ip, @test_ip)
+          |> IPGeolocation.call([])
+
+        geo = get_session(conn, :ip_geolocation)
+        assert is_map(geo)
+        assert is_number(geo["lat"])
+        assert is_number(geo["lng"])
+        # Test IP should geolocate to somewhere in the US
+        assert geo["lat"] >= 24 and geo["lat"] <= 49
+        assert geo["lng"] >= -125 and geo["lng"] <= -66
+      end
+    end
+
+    test "skips geolocation for local IP addresses", %{conn: conn} do
+      conn =
+        conn
+        |> Map.put(:remote_ip, {127, 0, 0, 1})
+        |> IPGeolocation.call([])
+
+      assert get_session(conn, :ip_geolocation) == nil
+    end
+
+    test "handles API failure gracefully", %{conn: conn} do
+      use_cassette "ip_geolocation_failure" do
+        conn =
+          conn
+          |> Map.put(:remote_ip, {192, 0, 2, 1})
+          |> IPGeolocation.call([])
+
+        assert get_session(conn, :ip_geolocation) == nil
+      end
+    end
+
+    test "validates latitude and longitude bounds", %{conn: conn} do
+      use_cassette "ip_geolocation_invalid_coords" do
+        conn =
+          conn
+          |> Map.put(:remote_ip, {192, 0, 2, 2})
+          |> IPGeolocation.call([])
+
+        assert get_session(conn, :ip_geolocation) == nil
+      end
+    end
+
+    test "handles IPv6 addresses", %{conn: conn} do
+      use_cassette "ip_geolocation_ipv6" do
+        conn =
+          conn
+          |> Map.put(:remote_ip, {0x2001, 0x4860, 0x4860, 0, 0, 0, 0, 0x8888})
+          |> IPGeolocation.call([])
+
+        geo = get_session(conn, :ip_geolocation)
+        assert is_map(geo)
+        assert is_number(geo["lat"])
+        assert is_number(geo["lng"])
+      end
+    end
+
+    test "uses forwarded IP when behind proxy", %{conn: conn} do
+      use_cassette "ip_geolocation_forwarded" do
+        conn =
+          conn
+          |> Map.put(:remote_ip, {10, 0, 0, 1})
+          |> put_req_header("x-forwarded-for", "204.110.191.254, 10.0.0.1")
+          |> IPGeolocation.call([])
+
+        geo = get_session(conn, :ip_geolocation)
+        assert is_map(geo)
+        assert is_number(geo["lat"])
+        assert is_number(geo["lng"])
+      end
+    end
+
+    test "caches geolocation in session", %{conn: conn} do
+      use_cassette "ip_geolocation_cached" do
+        # First request should call API
+        conn =
+          conn
+          |> Map.put(:remote_ip, @test_ip)
+          |> IPGeolocation.call([])
+          |> fetch_session()
+
+        geo1 = get_session(conn, :ip_geolocation)
+
+        # Second request with same session should not call API
+        conn2 =
+          conn
+          |> recycle()
+          |> Map.put(:remote_ip, @test_ip)
+          |> IPGeolocation.call([])
+
+        geo2 = get_session(conn2, :ip_geolocation)
+        
+        # Should return the same cached data
+        assert geo2 == geo1
+      end
+    end
+
+    test "skips non-root paths", %{conn: conn} do
+      conn =
+        conn
+        |> Map.put(:remote_ip, @test_ip)
+        |> Map.put(:request_path, "/about")
+        |> IPGeolocation.call([])
+
+      assert get_session(conn, :ip_geolocation) == nil
+    end
+
+    test "skips non-GET requests", %{conn: conn} do
+      conn =
+        conn
+        |> Map.put(:remote_ip, @test_ip)
+        |> Map.put(:method, "POST")
+        |> IPGeolocation.call([])
+
+      assert get_session(conn, :ip_geolocation) == nil
+    end
+  end
+end
\ No newline at end of file