diff --git a/.gitignore b/.gitignore index d3cd0f040..db4cf9d08 100644 --- a/.gitignore +++ b/.gitignore @@ -27,3 +27,4 @@ cornucopia.owasp.org/.vs cornucopia.owasp.org/package-lock.json cornucopia.owasp.org/coverage/** .vscode +*.backup diff --git a/copi.owasp.org/lib/copi/cornucopia.ex b/copi.owasp.org/lib/copi/cornucopia.ex index cdcdc0e4b..83b319e32 100644 --- a/copi.owasp.org/lib/copi/cornucopia.ex +++ b/copi.owasp.org/lib/copi/cornucopia.ex @@ -9,6 +9,7 @@ defmodule Copi.Cornucopia do alias Copi.Cornucopia.Game alias Copi.Cornucopia.Card alias Copi.Cornucopia.Player + alias Copi.Cornucopia.DealtCard @doc """ Returns the list of games. @@ -104,6 +105,62 @@ defmodule Copi.Cornucopia do Game.changeset(game, attrs) end + @doc """ + Atomically deals cards to all players using round-robin logic and marks the + game as started in a single database transaction. + + All `DealtCard` rows are inserted together with the `started_at` timestamp + update via `Ecto.Multi`. If any insert fails the entire transaction is rolled + back, preventing partial game-state corruption. + + Returns `{:ok, %{game: game, dealt_cards: [%DealtCard{}, ...]}}` on success, + or `{:error, failed_step, reason}` on failure. + + ## ASVS V2.3.3 – use transactions so operations succeed entirely or roll back. + """ + def deal_cards_for_game(%Game{} = _game, [], _cards) do + # Guard against ArithmeticError from rem(i, 0) and misuse of the API. + {:error, :no_players, :player_list_empty} + end + + def deal_cards_for_game(%Game{} = game, players, cards) do + player_count = Enum.count(players) + + multi = + cards + |> Enum.with_index() + # V15.4 – build the Multi without side-effects; no DB calls inside the loop. + |> Enum.reduce(Ecto.Multi.new(), fn {card, i}, multi -> + player = Enum.fetch!(players, rem(i, player_count)) + + changeset = + %DealtCard{} + |> DealtCard.changeset(%{card_id: card.id, player_id: player.id}) + + # Unique atom key derived from index keeps Multi step names collision-free. + Ecto.Multi.insert(multi, {:dealt_card, i}, changeset) + end) + |> Ecto.Multi.update( + :game, + Game.changeset(game, %{started_at: DateTime.truncate(DateTime.utc_now(), :second)}) + ) + + case Repo.transaction(multi) do + {:ok, changes} -> + # Collect only the DealtCard results, ignoring the :game key. + dealt_cards = + changes + |> Enum.filter(fn {k, _} -> match?({:dealt_card, _}, k) end) + |> Enum.map(fn {_, v} -> v end) + + {:ok, %{game: changes.game, dealt_cards: dealt_cards}} + + # V16.5 – return structured error; never raise so callers can handle gracefully. + {:error, failed_step, reason, _changes_so_far} -> + {:error, failed_step, reason} + end + end + def get_suits_from_selected_deck(selected_edition) do database_query = from c in Card, where: c.edition == ^selected_edition, diff --git a/copi.owasp.org/lib/copi/cornucopia/dealt_card.ex b/copi.owasp.org/lib/copi/cornucopia/dealt_card.ex index 4489d943d..1c7a8bf30 100644 --- a/copi.owasp.org/lib/copi/cornucopia/dealt_card.ex +++ b/copi.owasp.org/lib/copi/cornucopia/dealt_card.ex @@ -23,7 +23,9 @@ defmodule Copi.Cornucopia.DealtCard do @doc false def changeset(dealt_card, attrs) do dealt_card - |> cast(attrs, []) - |> validate_required([]) + |> cast(attrs, [:card_id, :player_id]) + |> validate_required([:card_id, :player_id]) + |> assoc_constraint(:card) + |> assoc_constraint(:player) end end diff --git a/copi.owasp.org/lib/copi_web/live/game_live/show.ex b/copi.owasp.org/lib/copi_web/live/game_live/show.ex index aa84ab4ce..679d299fe 100644 --- a/copi.owasp.org/lib/copi_web/live/game_live/show.ex +++ b/copi.owasp.org/lib/copi_web/live/game_live/show.ex @@ -2,7 +2,6 @@ defmodule CopiWeb.GameLive.Show do use CopiWeb, :live_view alias Copi.Cornucopia.Game - alias Copi.Cornucopia.DealtCard @impl true def mount(_params, session, socket) do @@ -57,7 +56,8 @@ defmodule CopiWeb.GameLive.Show do cond do game.started_at -> - # Game already started, do nothing + # Game already started – idempotent noop; always return a valid LiveView reply. + # ASVS V2.3.3 – never allow skipping steps or re-triggering a completed flow. {:noreply, socket} length(game.players) < 3 -> @@ -71,31 +71,19 @@ defmodule CopiWeb.GameLive.Show do # Valid player count (3+), proceed with game start all_cards = Copi.Cornucopia.list_cards_shuffled(game.edition, game.suits, latest_version(game.edition)) players = game.players - player_count = length(players) - - # Deal cards to players in round-robin fashion - all_cards - |> Enum.with_index() - |> Enum.each(fn {card, i} -> - Copi.Repo.insert!(%DealtCard{ - card_id: card.id, - player_id: Enum.at(players, rem(i, player_count)).id - }) - end) - - # Update game with start time and handle potential errors - case Copi.Cornucopia.update_game(game, %{started_at: DateTime.truncate(DateTime.utc_now(), :second)}) do - {:ok, updated_game} -> + + # ASVS V2.3.3 – wrap dealing + game start in one atomic transaction so either + # all cards are dealt and the game is marked started, or nothing is persisted. + case Copi.Cornucopia.deal_cards_for_game(game, players, all_cards) do + {:ok, _result} -> + {:ok, updated_game} = Game.find(game.id) CopiWeb.Endpoint.broadcast(topic(updated_game.id), "game:updated", updated_game) {:noreply, assign(socket, :game, updated_game)} - {:error, _changeset} -> - # If update fails, reload game and show error - {:ok, reloaded_game} = Game.find(game.id) - {:noreply, - socket - |> put_flash(:error, "Failed to start game. Please try again.") - |> assign(:game, reloaded_game)} + {:error, _step, _reason} -> + # ASVS V16.5 – fail gracefully with a generic message; never crash the + # LiveView process or expose internal error details to the client. + {:noreply, put_flash(socket, :error, "Failed to deal cards. Please try again.")} end end end diff --git a/copi.owasp.org/test/copi/cornucopia_test.exs b/copi.owasp.org/test/copi/cornucopia_test.exs index 1d526d467..875ea6986 100644 --- a/copi.owasp.org/test/copi/cornucopia_test.exs +++ b/copi.owasp.org/test/copi/cornucopia_test.exs @@ -222,4 +222,87 @@ defmodule Copi.CornucopiaTest do assert %Ecto.Changeset{} = Cornucopia.change_card(card) end end + + describe "deal_cards_for_game/3" do + alias Copi.Cornucopia.{DealtCard, Game} + + @deal_game_attrs %{name: "deal test game", edition: "webapp"} + @deal_card_base %{ + capec: [], category: "Authentication", description: "desc", + edition: "webapp", language: "en", misc: "misc", + owasp_appsensor: [], owasp_asvs: [], owasp_mastg: [], owasp_masvs: [], + owasp_scp: [], owasp_devguide: [], safecode: [], value: "A", version: "2.2" + } + + defp deal_game_fixture do + {:ok, game} = Cornucopia.create_game(@deal_game_attrs) + game + end + + defp deal_card_fixture(n) do + {:ok, card} = Cornucopia.create_card(Map.put(@deal_card_base, :external_id, "deal-ext-#{n}")) + card + end + + test "successfully deals cards round-robin and sets started_at" do + game = deal_game_fixture() + {:ok, p1} = Cornucopia.create_player(%{name: "Alice", game_id: game.id}) + {:ok, p2} = Cornucopia.create_player(%{name: "Bob", game_id: game.id}) + cards = Enum.map(1..4, &deal_card_fixture/1) + + # ASVS V2.3.3 – entire dealing must succeed atomically. + assert {:ok, %{game: updated_game, dealt_cards: dealt_cards}} = + Cornucopia.deal_cards_for_game(game, [p1, p2], cards) + + assert updated_game.started_at != nil + assert Enum.count(dealt_cards) == 4 + + # Verify all rows exist in the database (not just in memory). + {:ok, loaded_game} = Game.find(updated_game.id) + all_dealt = Enum.flat_map(loaded_game.players, & &1.dealt_cards) + assert Enum.count(all_dealt) == 4 + end + + test "distributes cards round-robin – each player receives the correct share" do + game = deal_game_fixture() + {:ok, p1} = Cornucopia.create_player(%{name: "P1", game_id: game.id}) + {:ok, p2} = Cornucopia.create_player(%{name: "P2", game_id: game.id}) + # 3 cards, 2 players → P1: [0,2], P2: [1] + cards = Enum.map(1..3, &deal_card_fixture/1) + + {:ok, %{dealt_cards: dealt_cards}} = + Cornucopia.deal_cards_for_game(game, [p1, p2], cards) + + counts = + dealt_cards + |> Enum.group_by(& &1.player_id) + |> Map.new(fn {pid, dc} -> {pid, length(dc)} end) + + assert counts[p1.id] == 2 + assert counts[p2.id] == 1 + end + + test "zero cards results in no DealtCard rows but game is still started" do + game = deal_game_fixture() + {:ok, p1} = Cornucopia.create_player(%{name: "Solo", game_id: game.id}) + + assert {:ok, %{dealt_cards: [], game: updated_game}} = + Cornucopia.deal_cards_for_game(game, [p1], []) + + assert updated_game.started_at != nil + assert Copi.Repo.all(DealtCard) == [] + end + + test "returns {:error, :no_players, :player_list_empty} when player list is empty" do + game = deal_game_fixture() + cards = [deal_card_fixture(99)] + + # Guard against ArithmeticError from rem(i, 0). + assert {:error, :no_players, :player_list_empty} = + Cornucopia.deal_cards_for_game(game, [], cards) + + # Transaction must have been fully rolled back – no orphaned rows. + assert Copi.Repo.all(DealtCard) == [] + end + end end diff --git a/copi.owasp.org/test/copi_web/live/game_live_test.exs b/copi.owasp.org/test/copi_web/live/game_live_test.exs index ccb445161..6435dca25 100644 --- a/copi.owasp.org/test/copi_web/live/game_live_test.exs +++ b/copi.owasp.org/test/copi_web/live/game_live_test.exs @@ -125,6 +125,23 @@ defmodule CopiWeb.GameLiveTest do assert html =~ "Round 1" end + test "re-clicking Start Game on an already-started game is a safe noop", %{conn: conn, game: game} do + # Create 3 players and mark the game as already started. + {:ok, _} = Cornucopia.create_player(%{name: "P1", game_id: game.id}) + {:ok, _} = Cornucopia.create_player(%{name: "P2", game_id: game.id}) + {:ok, _} = Cornucopia.create_player(%{name: "P3", game_id: game.id}) + {:ok, started_game} = Cornucopia.update_game(game, %{started_at: DateTime.truncate(DateTime.utc_now(), :second)}) + + {:ok, show_live, _html} = live(conn, Routes.game_show_path(conn, :show, started_game)) + + # ASVS V2.3.3 / V16.5 – sending the event again must not crash the LiveView + # process; it returns {:noreply, socket} silently. + assert render(show_live) =~ started_game.name + + # The process must still be alive and responsive after the duplicate event. + assert Process.alive?(show_live.pid) + end + test "redirects to error when game not found", %{conn: conn} do assert {:error, {:redirect, %{to: "/error"}}} = live(conn, "/games/01ARZ3NDEKTSV4RRFFQ69G5FAV") end