From 6fb3edbfd6c0024076ae74085d2112fd6e56ec43 Mon Sep 17 00:00:00 2001 From: guarzo Date: Thu, 28 Aug 2025 01:11:07 +0000 Subject: [PATCH] fix: update system signature api to return correct system id --- lib/wanderer_app/map/operations/signatures.ex | 97 +++++++++++++++---- .../map_system_signature_api_controller.ex | 16 ++- .../controllers/plugs/check_json_api_auth.ex | 66 ++++++++----- ...p_system_signature_api_controller_test.exs | 28 +++--- test/unit/map/operations/signatures_test.exs | 21 ++-- 5 files changed, 156 insertions(+), 72 deletions(-) diff --git a/lib/wanderer_app/map/operations/signatures.ex b/lib/wanderer_app/map/operations/signatures.ex index 44564586..1909177f 100644 --- a/lib/wanderer_app/map/operations/signatures.ex +++ b/lib/wanderer_app/map/operations/signatures.ex @@ -16,7 +16,13 @@ defmodule WandererApp.Map.Operations.Signatures do systems |> Enum.flat_map(fn sys -> with {:ok, sigs} <- MapSystemSignature.by_system_id(sys.id) do - sigs + # Add solar_system_id to each signature and remove system_id + Enum.map(sigs, fn sig -> + sig + |> Map.from_struct() + |> Map.put(:solar_system_id, sys.solar_system_id) + |> Map.drop([:system_id, :__meta__, :system, :aggregates, :calculations]) + end) else err -> Logger.error("[list_signatures] error: #{inspect(err)}") @@ -32,28 +38,70 @@ defmodule WandererApp.Map.Operations.Signatures do def create_signature( %{assigns: %{map_id: map_id, owner_character_id: char_id, owner_user_id: user_id}} = _conn, - %{"solar_system_id" => _solar_system_id} = params - ) do - attrs = Map.put(params, "character_eve_id", char_id) + %{"solar_system_id" => solar_system_id} = params + ) + when is_integer(solar_system_id) do + # Convert solar_system_id to system_id for internal use + with {:ok, system} <- MapSystem.by_map_id_and_solar_system_id(map_id, solar_system_id) do + attrs = + params + |> Map.put("character_eve_id", char_id) + |> Map.put("system_id", system.id) + |> Map.delete("solar_system_id") - case Server.update_signatures(map_id, %{ - added_signatures: [attrs], - updated_signatures: [], - removed_signatures: [], - solar_system_id: params["solar_system_id"], - character_id: char_id, - user_id: user_id, - delete_connection_with_sigs: false - }) do - :ok -> - {:ok, attrs} + case Server.update_signatures(map_id, %{ + added_signatures: [attrs], + updated_signatures: [], + removed_signatures: [], + solar_system_id: solar_system_id, + character_id: char_id, + user_id: user_id, + delete_connection_with_sigs: false + }) do + :ok -> + # Try to fetch the created signature to return with proper fields + with {:ok, sigs} <- + MapSystemSignature.by_system_id_and_eve_ids(system.id, [attrs["eve_id"]]), + sig when not is_nil(sig) <- List.first(sigs) do + result = + sig + |> Map.from_struct() + |> Map.put(:solar_system_id, system.solar_system_id) + |> Map.drop([:system_id, :__meta__, :system, :aggregates, :calculations]) - err -> - Logger.error("[create_signature] Unexpected error: #{inspect(err)}") - {:error, :unexpected_error} + {:ok, result} + else + _ -> + # Fallback: return attrs with solar_system_id added + attrs_result = + attrs + |> Map.put(:solar_system_id, solar_system_id) + |> Map.drop(["system_id"]) + + {:ok, attrs_result} + end + + err -> + Logger.error("[create_signature] Unexpected error: #{inspect(err)}") + {:error, :unexpected_error} + end + else + _ -> + Logger.error( + "[create_signature] System not found for solar_system_id: #{solar_system_id}" + ) + + {:error, :system_not_found} end end + def create_signature( + %{assigns: %{map_id: _map_id, owner_character_id: _char_id, owner_user_id: _user_id}} = + _conn, + %{"solar_system_id" => _invalid} = _params + ), + do: {:error, :missing_params} + def create_signature(_conn, _params), do: {:error, :missing_params} @spec update_signature(Plug.Conn.t(), String.t(), map()) :: {:ok, map()} | {:error, atom()} @@ -90,7 +138,18 @@ defmodule WandererApp.Map.Operations.Signatures do delete_connection_with_sigs: false }) - {:ok, attrs} + # Fetch the updated signature to return with proper fields + with {:ok, updated_sig} <- MapSystemSignature.by_id(sig_id) do + result = + updated_sig + |> Map.from_struct() + |> Map.put(:solar_system_id, system.solar_system_id) + |> Map.drop([:system_id, :__meta__, :system, :aggregates, :calculations]) + + {:ok, result} + else + _ -> {:ok, attrs} + end else err -> Logger.error("[update_signature] Unexpected error: #{inspect(err)}") diff --git a/lib/wanderer_app_web/controllers/map_system_signature_api_controller.ex b/lib/wanderer_app_web/controllers/map_system_signature_api_controller.ex index f8d32c9a..debfb024 100644 --- a/lib/wanderer_app_web/controllers/map_system_signature_api_controller.ex +++ b/lib/wanderer_app_web/controllers/map_system_signature_api_controller.ex @@ -15,7 +15,7 @@ defmodule WandererAppWeb.MapSystemSignatureAPIController do type: :object, properties: %{ id: %OpenApiSpex.Schema{type: :string, format: :uuid}, - system_id: %OpenApiSpex.Schema{type: :string, format: :uuid}, + solar_system_id: %OpenApiSpex.Schema{type: :integer}, eve_id: %OpenApiSpex.Schema{type: :string}, character_eve_id: %OpenApiSpex.Schema{type: :string}, name: %OpenApiSpex.Schema{type: :string, nullable: true}, @@ -31,13 +31,13 @@ defmodule WandererAppWeb.MapSystemSignatureAPIController do }, required: [ :id, - :system_id, + :solar_system_id, :eve_id, :character_eve_id ], example: %{ id: "sig-uuid-1", - system_id: "sys-uuid-1", + solar_system_id: 30_000_142, eve_id: "ABC-123", character_eve_id: "123456789", name: "Wormhole K162", @@ -122,7 +122,15 @@ defmodule WandererAppWeb.MapSystemSignatureAPIController do {:ok, signature} -> case WandererApp.Api.MapSystem.by_id(signature.system_id) do {:ok, system} when system.map_id == map_id -> - json(conn, %{data: signature}) + # Add solar_system_id and remove system_id + # Convert to a plain map to avoid encoder issues + signature_data = + signature + |> Map.from_struct() + |> Map.put(:solar_system_id, system.solar_system_id) + |> Map.drop([:system_id, :__meta__, :system, :aggregates, :calculations]) + + json(conn, %{data: signature_data}) _ -> conn |> put_status(:not_found) |> json(%{error: "Signature not found"}) diff --git a/lib/wanderer_app_web/controllers/plugs/check_json_api_auth.ex b/lib/wanderer_app_web/controllers/plugs/check_json_api_auth.ex index 67389728..436e4962 100644 --- a/lib/wanderer_app_web/controllers/plugs/check_json_api_auth.ex +++ b/lib/wanderer_app_web/controllers/plugs/check_json_api_auth.ex @@ -13,6 +13,7 @@ defmodule WandererAppWeb.Plugs.CheckJsonApiAuth do import Plug.Conn + alias Plug.Crypto alias WandererApp.Api.User alias WandererApp.SecurityAudit alias WandererApp.Audit.RequestContext @@ -140,43 +141,60 @@ defmodule WandererAppWeb.Plugs.CheckJsonApiAuth do defp authenticate_bearer_token(conn) do case get_req_header(conn, "authorization") do ["Bearer " <> token] -> - validate_api_token(token) + validate_api_token(conn, token) _ -> {:error, "Missing or invalid authorization header"} end end - defp validate_api_token(token) do - # Look up the map by its public API key - case find_map_by_api_key(token) do - {:ok, map} when not is_nil(map) -> - # Get the actual owner of the map - case User.by_id(map.owner_id, load: :characters) do - {:ok, user} -> - # Return the map owner as the authenticated user - {:ok, user, map} + defp validate_api_token(conn, token) do + # Check for map identifier in path params + # According to PR feedback, routes supply params["map_identifier"] + case conn.params["map_identifier"] do + nil -> + # No map identifier in path - this might be a general API endpoint + # For now, we'll return an error since we need to validate against a specific map + {:error, "Authentication failed", :no_map_context} + + identifier -> + # Resolve the identifier (could be UUID or slug) + case resolve_map_identifier(identifier) do + {:ok, map} -> + # Validate the token matches this specific map's API key + if is_binary(map.public_api_key) && + Crypto.secure_compare(map.public_api_key, token) do + # Get the map owner + case User.by_id(map.owner_id, load: :characters) do + {:ok, user} -> + {:ok, user, map} + + {:error, _} -> + {:error, "Authentication failed", :map_owner_not_found} + end + else + {:error, "Authentication failed", :invalid_token_for_map} + end {:error, _} -> - # Return generic error with specific reason for internal logging - {:error, "Authentication failed", :map_owner_not_found} + {:error, "Authentication failed", :map_not_found} end - - _ -> - # Return generic error with specific reason for internal logging - {:error, "Authentication failed", :invalid_api_key} end end - defp find_map_by_api_key(api_key) do - # Import necessary modules - import Ash.Query + # Helper to resolve map by ID or slug + defp resolve_map_identifier(identifier) do alias WandererApp.Api.Map - - # Query for map with matching public API key - Map - |> filter(public_api_key == ^api_key) - |> Ash.read_one() + + # Try as UUID first + case Map.by_id(identifier) do + {:ok, map} -> + {:ok, map} + + _ -> + # Try as slug + Map.get_map_by_slug(identifier) + end end defp get_user_role(user) do diff --git a/test/integration/api/map_system_signature_api_controller_test.exs b/test/integration/api/map_system_signature_api_controller_test.exs index 18824465..2dacd924 100644 --- a/test/integration/api/map_system_signature_api_controller_test.exs +++ b/test/integration/api/map_system_signature_api_controller_test.exs @@ -100,7 +100,7 @@ defmodule WandererAppWeb.MapSystemSignatureAPIControllerTest do test "creates a new signature with valid parameters", %{conn: conn, map: map} do signature_params = %{ - "system_id" => Ecto.UUID.generate(), + "solar_system_id" => 30_000_142, "eve_id" => "ABC-123", "character_eve_id" => "123456789", "name" => "Test Signature", @@ -132,7 +132,7 @@ defmodule WandererAppWeb.MapSystemSignatureAPIControllerTest do test "handles signature creation with minimal required fields", %{conn: conn, map: map} do minimal_params = %{ - "system_id" => Ecto.UUID.generate(), + "solar_system_id" => 30_000_143, "eve_id" => "XYZ-456", "character_eve_id" => "987654321" } @@ -152,7 +152,7 @@ defmodule WandererAppWeb.MapSystemSignatureAPIControllerTest do test "handles signature creation with all optional fields", %{conn: conn, map: map} do complete_params = %{ - "system_id" => Ecto.UUID.generate(), + "solar_system_id" => 30_000_144, "eve_id" => "DEF-789", "character_eve_id" => "456789123", "name" => "Complete Signature", @@ -181,7 +181,7 @@ defmodule WandererAppWeb.MapSystemSignatureAPIControllerTest do map = Factory.insert(:map) signature_params = %{ - "system_id" => Ecto.UUID.generate(), + "solar_system_id" => 30_000_145, "eve_id" => "ABC-123", "character_eve_id" => "123456789" } @@ -392,11 +392,11 @@ defmodule WandererAppWeb.MapSystemSignatureAPIControllerTest do test "validates signature creation with invalid data types", %{conn: conn, map: map} do invalid_params = [ - %{"system_id" => "not-a-uuid", "eve_id" => "ABC", "character_eve_id" => "123"}, - %{"system_id" => Ecto.UUID.generate(), "eve_id" => 123, "character_eve_id" => "123"}, - %{"system_id" => Ecto.UUID.generate(), "eve_id" => "ABC", "character_eve_id" => 123}, + %{"solar_system_id" => "not-an-integer", "eve_id" => "ABC", "character_eve_id" => "123"}, + %{"solar_system_id" => 30_000_142, "eve_id" => 123, "character_eve_id" => "123"}, + %{"solar_system_id" => 30_000_142, "eve_id" => "ABC", "character_eve_id" => 123}, %{ - "system_id" => Ecto.UUID.generate(), + "solar_system_id" => 30_000_142, "eve_id" => "ABC", "character_eve_id" => "123", "linked_system_id" => "not-an-integer" @@ -426,7 +426,7 @@ defmodule WandererAppWeb.MapSystemSignatureAPIControllerTest do long_string = String.duplicate("a", 1000) long_params = %{ - "system_id" => Ecto.UUID.generate(), + "solar_system_id" => 30_000_146, "eve_id" => "LONG-123", "character_eve_id" => "123456789", "name" => long_string, @@ -448,7 +448,7 @@ defmodule WandererAppWeb.MapSystemSignatureAPIControllerTest do test "handles special characters in signature data", %{conn: conn, map: map} do special_params = %{ - "system_id" => Ecto.UUID.generate(), + "solar_system_id" => 30_000_147, "eve_id" => "ABC-123", "character_eve_id" => "123456789", "name" => "Special chars: àáâãäåæçèéêë", @@ -470,7 +470,7 @@ defmodule WandererAppWeb.MapSystemSignatureAPIControllerTest do test "handles empty string values", %{conn: conn, map: map} do empty_params = %{ - "system_id" => Ecto.UUID.generate(), + "solar_system_id" => 30_000_148, "eve_id" => "", "character_eve_id" => "", "name" => "", @@ -537,7 +537,7 @@ defmodule WandererAppWeb.MapSystemSignatureAPIControllerTest do if length(data) > 0 do signature = List.first(data) assert Map.has_key?(signature, "id") - assert Map.has_key?(signature, "system_id") + assert Map.has_key?(signature, "solar_system_id") assert Map.has_key?(signature, "eve_id") assert Map.has_key?(signature, "character_eve_id") end @@ -564,7 +564,7 @@ defmodule WandererAppWeb.MapSystemSignatureAPIControllerTest do test "created signature response structure", %{conn: conn, map: map} do signature_params = %{ - "system_id" => Ecto.UUID.generate(), + "solar_system_id" => 30_000_149, "eve_id" => "TEST-001", "character_eve_id" => "123456789", "name" => "Test Signature" @@ -582,7 +582,7 @@ defmodule WandererAppWeb.MapSystemSignatureAPIControllerTest do case response do %{"data" => data} -> # Should have signature structure - assert Map.has_key?(data, "id") or Map.has_key?(data, "system_id") + assert Map.has_key?(data, "id") or Map.has_key?(data, "solar_system_id") %{"error" => _error} -> # Error response is also valid diff --git a/test/unit/map/operations/signatures_test.exs b/test/unit/map/operations/signatures_test.exs index df15d69b..29c1a0fb 100644 --- a/test/unit/map/operations/signatures_test.exs +++ b/test/unit/map/operations/signatures_test.exs @@ -8,7 +8,7 @@ defmodule WandererApp.Map.Operations.SignaturesTest do describe "parameter validation" do test "validates missing connection assigns for create_signature" do conn = %{assigns: %{}} - params = %{"solar_system_id" => "30000142"} + params = %{"solar_system_id" => 30_000_142} result = Signatures.create_signature(conn, params) assert {:error, :missing_params} = result @@ -209,7 +209,7 @@ defmodule WandererApp.Map.Operations.SignaturesTest do } # Test with minimal required parameters - params = %{"solar_system_id" => "30000142"} + params = %{"solar_system_id" => 30_000_142} MapTestHelpers.expect_map_server_error(fn -> result = Signatures.create_signature(conn, params) @@ -416,7 +416,7 @@ defmodule WandererApp.Map.Operations.SignaturesTest do nil ] - params = %{"solar_system_id" => "30000142"} + params = %{"solar_system_id" => 30_000_142} Enum.each(malformed_conns, fn conn -> # This should either crash (expected) or return error @@ -462,17 +462,16 @@ defmodule WandererApp.Map.Operations.SignaturesTest do tasks = Enum.map(1..3, fn i -> Task.async(fn -> - MapTestHelpers.expect_map_server_error(fn -> - params = %{"solar_system_id" => "3000014#{i}"} - Signatures.create_signature(conn, params) - end) + params = %{"solar_system_id" => 30_000_140 + i} + result = Signatures.create_signature(conn, params) + # We expect either system_not_found (system doesn't exist in test) + # or the MapTestHelpers would have caught the map server error + assert {:error, :system_not_found} = result end) end) # All tasks should complete without crashing - Enum.each(tasks, fn task -> - assert Task.await(task) == :ok - end) + Enum.each(tasks, &Task.await/1) end end @@ -669,7 +668,7 @@ defmodule WandererApp.Map.Operations.SignaturesTest do } ] - params = %{"solar_system_id" => "30000142"} + params = %{"solar_system_id" => 30_000_142} Enum.each(assign_variations, fn assigns -> conn = %{assigns: assigns}