fix: lifecycle guard for play_card API endpoint (#2631)

Reject card plays with 422 Unprocessable Entity when:
- game.started_at == nil (game has not started yet)
- game.finished_at != nil (game has already ended)

Add tests covering both lifecycle guard cases, and update existing
tests that exercise the success and card/player validation paths to
first start the game, since those paths now require an active game.

Author: immortal71

copi.owasp.org/lib/copi_web/controllers/api_controller.ex

@@ -3,36 +3,43 @@ defmodule CopiWeb.ApiController do
   alias Copi.Cornucopia.Game
   def play_card(conn, %{"game_id" => game_id, "player_id" => player_id, "dealt_card_id" => dealt_card_id}) do
     with {:ok, game} <- Game.find(game_id) do
-      player = Enum.find(game.players, fn player -> player.id == player_id end)
-      if player do
-        dealt_card = Enum.find(player.dealt_cards, fn dealt_card -> Integer.to_string(dealt_card.id) == dealt_card_id end)
-        if dealt_card do
-          current_round = game.rounds_played + 1
-          cond do
-            dealt_card.played_in_round ->
-              conn |> put_status(:not_acceptable) |> json(%{"error" => "Card already played"})
-            Enum.find(player.dealt_cards, fn dealt_card -> dealt_card.played_in_round == current_round end) ->
-              conn |> put_status(:forbidden) |> json(%{"error" => "Player already played a card in this round"})
-            true ->
-              dealt_card = Ecto.Changeset.change dealt_card, played_in_round: current_round
-              case Copi.Repo.update dealt_card do
-                {:ok, dealt_card} ->
-                  with {:ok, updated_game} <- Game.find(game.id) do
-                    CopiWeb.Endpoint.broadcast(topic(game.id), "game:updated", updated_game)
-                  else
-                    {:error, _reason} ->
-                      conn |> put_status(:internal_server_error) |> json(%{"error" => "Could not find updated game"})
+      cond do
+        is_nil(game.started_at) ->
+          conn |> put_status(:unprocessable_entity) |> json(%{"error" => "Game has not started yet"})
+        not is_nil(game.finished_at) ->
+          conn |> put_status(:unprocessable_entity) |> json(%{"error" => "Game has already ended"})
+        true ->
+          player = Enum.find(game.players, fn player -> player.id == player_id end)
+          if player do
+            dealt_card = Enum.find(player.dealt_cards, fn dealt_card -> Integer.to_string(dealt_card.id) == dealt_card_id end)
+            if dealt_card do
+              current_round = game.rounds_played + 1
+              cond do
+                dealt_card.played_in_round ->
+                  conn |> put_status(:not_acceptable) |> json(%{"error" => "Card already played"})
+                Enum.find(player.dealt_cards, fn dealt_card -> dealt_card.played_in_round == current_round end) ->
+                  conn |> put_status(:forbidden) |> json(%{"error" => "Player already played a card in this round"})
+                true ->
+                  dealt_card = Ecto.Changeset.change dealt_card, played_in_round: current_round
+                  case Copi.Repo.update dealt_card do
+                    {:ok, dealt_card} ->
+                      with {:ok, updated_game} <- Game.find(game.id) do
+                        CopiWeb.Endpoint.broadcast(topic(game.id), "game:updated", updated_game)
+                      else
+                        {:error, _reason} ->
+                          conn |> put_status(:internal_server_error) |> json(%{"error" => "Could not find updated game"})
+                      end
+                      conn |> json(%{"id" => dealt_card.id})
+                    {:error, _changeset} ->
+                      conn |> put_status(:internal_server_error) |> json(%{"error" => "Could not update dealt card"})
                   end
-                  conn |> json(%{"id" => dealt_card.id})
-                {:error, _changeset} ->
-                  conn |> put_status(:internal_server_error) |> json(%{"error" => "Could not update dealt card"})
               end
+            else
+              conn |> put_status(:not_found) |> json(%{"error" => "Could not find player and dealt card"})
+            end
+          else
+            conn |> put_status(:not_found) |> json(%{"error" => "Player not found in this game"})
           end
-        else
-          conn |> put_status(:not_found) |> json(%{"error" => "Could not find player and dealt card"})
-        end
-      else
-        conn |> put_status(:not_found) |> json(%{"error" => "Player not found in this game"})
       end
     else
       {:error, _reason} -> conn |> put_status(:not_found) |> json(%{"error" => "Could not find game"})

copi.owasp.org/test/copi_web/controllers/api_controller_test.exs

@@ -22,6 +22,8 @@ defmodule CopiWeb.ApiControllerTest do
   end
 
   test "play_card success", %{conn: conn, game: game, player: player, dealt_card: dealt_card} do
+    {:ok, _} = Cornucopia.update_game(game, %{started_at: DateTime.truncate(DateTime.utc_now(), :second)})
+
     conn = put(conn, "/api/games/#{game.id}/players/#{player.id}/card", %{
       "game_id" => game.id,
       "player_id" => player.id,
@@ -35,6 +37,7 @@ defmodule CopiWeb.ApiControllerTest do
   end
 
   test "play_card fails if card already played", %{conn: conn, game: game, player: player, dealt_card: dealt_card} do
+    {:ok, _} = Cornucopia.update_game(game, %{started_at: DateTime.truncate(DateTime.utc_now(), :second)})
     {:ok, _} = Repo.update(Ecto.Changeset.change(dealt_card, played_in_round: 1))
 
     conn = put(conn, "/api/games/#{game.id}/players/#{player.id}/card", %{
@@ -75,7 +78,33 @@ defmodule CopiWeb.ApiControllerTest do
     assert json_response(conn, 404)["error"] == "Player not found in this game"
   end
 
+  test "play_card returns 422 when game has not started", %{conn: conn, game: game, player: player, dealt_card: dealt_card} do
+    conn = put(conn, "/api/games/#{game.id}/players/#{player.id}/card", %{
+      "game_id" => game.id,
+      "player_id" => player.id,
+      "dealt_card_id" => to_string(dealt_card.id)
+    })
+
+    assert json_response(conn, 422)["error"] == "Game has not started yet"
+  end
+
+  test "play_card returns 422 when game has already ended", %{conn: conn, game: game, player: player, dealt_card: dealt_card} do
+    {:ok, _} = Cornucopia.update_game(game, %{
+      started_at: DateTime.truncate(DateTime.utc_now(), :second),
+      finished_at: DateTime.truncate(DateTime.utc_now(), :second)
+    })
+
+    conn = put(conn, "/api/games/#{game.id}/players/#{player.id}/card", %{
+      "game_id" => game.id,
+      "player_id" => player.id,
+      "dealt_card_id" => to_string(dealt_card.id)
+    })
+
+    assert json_response(conn, 422)["error"] == "Game has already ended"
+  end
+
   test "play_card fails if player already played in round", %{conn: conn, game: game, player: player, dealt_card: dealt_card} do
+    {:ok, _} = Cornucopia.update_game(game, %{started_at: DateTime.truncate(DateTime.utc_now(), :second)})
     {:ok, card2} = Cornucopia.create_card(%{
       category: "Cornucopia", value: "K", description: "desc", misc: "misc",
       edition: "webapp", external_id: "2", language: "en", version: "1",