From 56ef119e570f81cf11b44b96762034a935a38121 Mon Sep 17 00:00:00 2001 From: Brian Peiris Date: Mon, 30 May 2022 15:21:42 -0400 Subject: [PATCH] Fix hub invites. Add hub invite tests --- config/test.exs | 2 + lib/ret/hub_invite.ex | 10 +- lib/ret_web/channels/hub_channel.ex | 17 +- test/ret_web/channels/hub_channel_test.exs | 198 +++++++++++++++++---- 4 files changed, 182 insertions(+), 45 deletions(-) diff --git a/config/test.exs b/config/test.exs index 401a78390..4a250fb0e 100644 --- a/config/test.exs +++ b/config/test.exs @@ -66,6 +66,8 @@ config :ret, Ret.Repo.Migrations.AdminSchemaInit, postgrest_password: "password" config :ret, Ret.Locking, lock_timeout_ms: 1000 * 60 * 15 config :ret, Ret.Account, admin_email: "admin@mozilla.com" +config :ret, RetWeb.HubChannel, enable_terminate_actions: false + config :ret, Ret.PermsToken, # This config value is for local development only. perms_key: diff --git a/lib/ret/hub_invite.ex b/lib/ret/hub_invite.ex index 0b13be19a..518f34c92 100644 --- a/lib/ret/hub_invite.ex +++ b/lib/ret/hub_invite.ex @@ -28,15 +28,15 @@ defmodule Ret.HubInvite do end end - def revoke_invite(hub_invite_sid) when is_binary(hub_invite_sid) do - hub_invite = HubInvite |> Repo.get_by(hub_invite_sid: hub_invite_sid) + def revoke_invite(%Hub{} = hub, hub_invite_sid) when is_binary(hub_invite_sid) do + hub_invite = HubInvite |> Repo.get_by(hub_id: hub.hub_id, hub_invite_sid: hub_invite_sid) change(hub_invite, state: :revoked) |> Repo.update!() end - def active?(nil = _hub_invite_sid), do: false + def active?(_hub, nil = _hub_invite_sid), do: false - def active?(hub_invite_sid) do - case Ret.HubInvite |> Ret.Repo.get_by(hub_invite_sid: hub_invite_sid) do + def active?(hub, hub_invite_sid) do + case Ret.HubInvite |> Ret.Repo.get_by(hub_id: hub.hub_id, hub_invite_sid: hub_invite_sid) do nil -> false %Ret.HubInvite{state: :revoked} -> false %Ret.HubInvite{state: :active} -> true diff --git a/lib/ret_web/channels/hub_channel.ex b/lib/ret_web/channels/hub_channel.ex index af616bbdb..f8ac70ba1 100644 --- a/lib/ret_web/channels/hub_channel.ex +++ b/lib/ret_web/channels/hub_channel.ex @@ -102,7 +102,7 @@ defmodule RetWeb.HubChannel do {nil, nil} end - has_active_invite = Ret.HubInvite.active?(params["hub_invite_id"]) + has_active_invite = Ret.HubInvite.active?(hub, params["hub_invite_id"]) params = params @@ -458,8 +458,8 @@ defmodule RetWeb.HubChannel do hub = hub_for_socket(socket) account = Guardian.Phoenix.Socket.current_resource(socket) - if account |> can?(update_hub(hub)) do - HubInvite.revoke_invite(payload["hub_invite_id"]) + if account |> can?(update_hub(hub)) and HubInvite.active?(hub, payload["hub_invite_id"]) do + HubInvite.revoke_invite(hub, payload["hub_invite_id"]) # Hubs can only have one invite for now, so we create a new one when the old one was revoked. hub_invite = hub |> HubInvite.find_or_create_invite_for_hub() {:reply, {:ok, %{hub_invite_id: hub_invite.hub_invite_sid}}, socket} @@ -771,9 +771,14 @@ defmodule RetWeb.HubChannel do end def terminate(_reason, socket) do - socket - |> SessionStat.stat_query_for_socket() - |> Repo.update_all(set: [ended_at: NaiveDateTime.utc_now()]) + # enable_terminate_actions is set to false during tests. Since the GenServer is forcefully + # terminated when a test ends, we want to avoid running into an error that would happen if we + # invoked a DB mutation during termination. + if Application.get_env(:ret, __MODULE__)[:enable_terminate_actions] !== false do + socket + |> SessionStat.stat_query_for_socket() + |> Repo.update_all(set: [ended_at: NaiveDateTime.utc_now()]) + end :ok end diff --git a/test/ret_web/channels/hub_channel_test.exs b/test/ret_web/channels/hub_channel_test.exs index 0d504f184..3151c79fa 100644 --- a/test/ret_web/channels/hub_channel_test.exs +++ b/test/ret_web/channels/hub_channel_test.exs @@ -3,7 +3,9 @@ defmodule RetWeb.HubChannelTest do import Ret.TestHelpers alias RetWeb.{Presence, SessionSocket} - alias Ret.{AppConfig, Account, Repo, Hub} + alias Ret.{AppConfig, Account, Repo, Hub, HubInvite} + + @default_join_params %{"profile" => %{}, "context" => %{}} setup [:create_account, :create_owned_file, :create_scene, :create_hub, :create_account] @@ -12,52 +14,180 @@ defmodule RetWeb.HubChannelTest do {:ok, socket: socket} end - test "joining hub works", %{socket: socket, hub: hub} do - {:ok, %{session_id: _session_id}, _socket} = subscribe_and_join(socket, "hub:#{hub.hub_sid}", join_params()) - end + describe "authorization" do + test "joining hub works", %{socket: socket, hub: hub} do + {:ok, %{session_id: _session_id}, _socket} = + subscribe_and_join(socket, "hub:#{hub.hub_sid}", @default_join_params) + end - test "joining hub does not work if sign in required", %{socket: socket, scene: scene} do - AppConfig.set_config_value("features|require_account_for_join", true) - {:ok, hub} = %Hub{} |> Hub.changeset(scene, %{name: "Test Hub"}) |> Repo.insert() - {:error, _reason} = subscribe_and_join(socket, "hub:#{hub.hub_sid}", join_params()) - AppConfig.set_config_value("features|require_account_for_join", false) - end + test "joining hub does not work if sign in required", %{socket: socket, scene: scene} do + AppConfig.set_config_value("features|require_account_for_join", true) + {:ok, hub} = %Hub{} |> Hub.changeset(scene, %{name: "Test Hub"}) |> Repo.insert() + {:error, _reason} = subscribe_and_join(socket, "hub:#{hub.hub_sid}", @default_join_params) + AppConfig.set_config_value("features|require_account_for_join", false) + end - test "joining hub does not work if account is disabled", %{socket: socket, hub: hub} do - disabled_account = create_account("disabled_account") - disabled_account |> Ecto.Changeset.change(state: :disabled) |> Ret.Repo.update!() + test "joining hub does not work if account is disabled", %{socket: socket, hub: hub} do + disabled_account = create_account("disabled_account") + disabled_account |> Ecto.Changeset.change(state: :disabled) |> Ret.Repo.update!() - {:error, %{reason: "join_denied"}} = - subscribe_and_join(socket, "hub:#{hub.hub_sid}", join_params_for_account(disabled_account)) + {:error, %{reason: "join_denied"}} = + subscribe_and_join(socket, "hub:#{hub.hub_sid}", join_params_for_account(disabled_account)) + end end - test "joining hub registers in presence", %{socket: socket, hub: hub} do - {:ok, %{session_id: session_id}, socket} = subscribe_and_join(socket, "hub:#{hub.hub_sid}", join_params()) - :timer.sleep(100) - presence = socket |> Presence.list() - assert presence[session_id] + describe "presence" do + test "joining hub registers in presence", %{socket: socket, hub: hub} do + {:ok, %{session_id: session_id}, socket} = subscribe_and_join(socket, "hub:#{hub.hub_sid}", @default_join_params) + :timer.sleep(100) + presence = socket |> Presence.list() + assert presence[session_id] + end + + test "joining hub with an account with an identity registers identity in presence", %{ + socket: socket, + hub: hub, + account: account + } do + account |> Account.set_identity!("Test User") + + {:ok, %{session_id: session_id}, socket} = + subscribe_and_join(socket, "hub:#{hub.hub_sid}", join_params_for_account(account)) + + :timer.sleep(100) + presence = socket |> Presence.list() + meta = presence[session_id][:metas] |> Enum.at(0) + assert meta[:profile]["identityName"] === "Test User" + end end - test "joining hub with an account with an identity registers identity in presence", %{ - socket: socket, - hub: hub, - account: account - } do - account |> Account.set_identity!("Test User") + describe "hub invites" do + test "join is denied when joining without an invite", %{socket: socket} do + %{hub: hub} = create_invite_only_hub() + + {:error, %{reason: "join_denied"}} = join_hub(socket, hub, @default_join_params) + end + + test "join is denied when joining with an invalid invite", %{socket: socket} do + %{hub: hub} = create_invite_only_hub() + + {:error, %{reason: "join_denied"}} = join_hub(socket, hub, join_params_for_hub_invite_id("invalid_invite_id")) + end + + test "join is denied when joining with a revoked invite", %{socket: socket} do + %{hub: hub, hub_invite: hub_invite} = create_invite_only_hub() + + Ret.HubInvite.revoke_invite(hub, hub_invite.hub_invite_sid) + + {:error, %{reason: "join_denied"}} = join_hub(socket, hub, join_params_for_hub_invite(hub_invite)) + end + + test "join is denied when joining with a mismatched invite", %{socket: socket} do + %{hub: hub_one} = create_invite_only_hub() + %{hub: _hub_two, hub_invite: hub_two_invite} = create_invite_only_hub() + + # Attempt to join hub_one, using invite associated with hub_two + {:error, %{reason: "join_denied"}} = + join_hub( + socket, + hub_one, + join_params_for_hub_invite(hub_two_invite) + ) + end + + test "revoke cannot be performed by an anonymous user", %{socket: socket} do + %{hub: hub, hub_invite: hub_invite} = create_invite_only_hub() + + # Join hub as anonymous user + {:ok, _context, socket} = join_hub(socket, hub, join_params_for_hub_invite(hub_invite)) + + # Attempt to revoke invite + push(socket, "revoke_invite", %{hub_invite_id: hub_invite.hub_invite_sid}) + |> assert_reply(:ok, %{}) + + # Join is still denied with invalid invite + {:error, %{reason: "join_denied"}} = join_hub(socket, hub, join_params_for_hub_invite_id("invalid_invite_id")) + + # Join is still allowed with original invite + {:ok, _context, _socket} = join_hub(socket, hub, join_params_for_hub_invite(hub_invite)) + end + + test "revoke is keyed to hub", %{socket: socket} do + %{hub: hub_one} = create_invite_only_hub() + %{hub: hub_two, hub_invite: hub_two_invite} = create_invite_only_hub() + + account_one = create_account("account_one") + assign_creator(hub_one, account_one) + + # Join hub_one + {:ok, _context, socket} = join_hub(socket, hub_one, join_params_for_account(account_one)) - {:ok, %{session_id: session_id}, socket} = - subscribe_and_join(socket, "hub:#{hub.hub_sid}", join_params_for_account(account)) + # Attempt to revoke invite associated with hub_two, using channel associated with hub_one + %{payload: response_payload} = + push(socket, "revoke_invite", %{hub_invite_id: hub_two_invite.hub_invite_sid}) + |> assert_reply(:ok, %{}) - :timer.sleep(100) - presence = socket |> Presence.list() - meta = presence[session_id][:metas] |> Enum.at(0) - assert meta[:profile]["identityName"] === "Test User" + # Response payload should be empty when a revoke is ignored + assert response_payload |> Map.keys() |> length === 0 + + # Joining hub_two is still denied with invalid invite + {:error, %{reason: "join_denied"}} = join_hub(socket, hub_two, join_params_for_hub_invite_id("invalid_invite_id")) + + # Joining hub_two still allowed with original invite + {:ok, _context, _socket} = join_hub(socket, hub_two, join_params_for_hub_invite(hub_two_invite)) + end + + test "revoke can be performed by hub creator", %{socket: socket} do + %{hub: hub, hub_invite: hub_invite} = create_invite_only_hub() + + account = create_account("test_account") + assign_creator(hub, account) + + {:ok, _context, socket} = join_hub(socket, hub, join_params_for_account(account)) + + push(socket, "revoke_invite", %{hub_invite_id: hub_invite.hub_invite_sid}) + |> assert_reply(:ok, %{hub_invite_id: new_hub_invite_id}) + + assert new_hub_invite_id !== hub_invite.hub_invite_sid + end + + test "join is allowed on an invite-only hub with a correct invite id", %{socket: socket} do + %{hub: hub, hub_invite: hub_invite} = create_invite_only_hub() + + {:ok, _context, _socket} = join_hub(socket, hub, join_params_for_hub_invite(hub_invite)) + end end - defp join_params, do: %{"profile" => %{}, "context" => %{}} + defp join_params_for_hub_invite(%HubInvite{} = hub_invite) do + join_params_for_hub_invite_id(hub_invite.hub_invite_sid) + end + + defp join_params_for_hub_invite_id(hub_invite_id) do + join_params(%{"hub_invite_id" => hub_invite_id}) + end defp join_params_for_account(account) do {:ok, token, _params} = account |> Ret.Guardian.encode_and_sign() - %{"profile" => %{}, "context" => %{}, "auth_token" => token} + join_params(%{"auth_token" => token}) + end + + defp join_params(%{} = params) do + Map.merge(@default_join_params, params) + end + + defp join_hub(socket, %Hub{} = hub, params) do + subscribe_and_join(socket, "hub:#{hub.hub_sid}", params) + end + + defp create_invite_only_hub() do + {:ok, hub: hub} = create_hub(%{scene: nil}) + + hub + |> Ret.Hub.changeset_for_entry_mode(:invite) + |> Ret.Repo.update!() + + hub_invite = Ret.HubInvite.find_or_create_invite_for_hub(hub) + + %{hub: hub, hub_invite: hub_invite} end end