diff --git a/lib/query_canary/checks.ex b/lib/query_canary/checks.ex index a7f8476..c62f542 100644 --- a/lib/query_canary/checks.ex +++ b/lib/query_canary/checks.ex @@ -97,13 +97,19 @@ defmodule QueryCanary.Checks do end def get_check!(%Scope{} = scope, id) do - Repo.get_by!(Check, id: id, user_id: scope.user.id) - |> Repo.preload(:server) + Check + |> where([c], c.id == ^id) + |> accessible_by_user(scope.user.id) + |> Repo.one!() + |> Repo.preload(server: [:team]) end def get_check(%Scope{} = scope, id) do - Repo.get_by(Check, id: id, user_id: scope.user.id) - |> Repo.preload(:server) + Check + |> where([c], c.id == ^id) + |> accessible_by_user(scope.user.id) + |> Repo.one() + |> Repo.preload(server: [:team]) end def get_check_for_system!(id) do @@ -151,7 +157,7 @@ defmodule QueryCanary.Checks do """ def update_check(%Scope{} = scope, %Check{} = check, attrs) do - true = check.user_id == scope.user.id + true = can_perform?(:edit, scope, check) with {:ok, check = %Check{}} <- check @@ -175,7 +181,7 @@ defmodule QueryCanary.Checks do """ def delete_check(%Scope{} = scope, %Check{} = check) do - true = check.user_id == scope.user.id + true = can_perform?(:edit, scope, check) with {:ok, check = %Check{}} <- Repo.delete(check) do @@ -194,7 +200,7 @@ defmodule QueryCanary.Checks do """ def change_check(%Scope{} = scope, %Check{} = check, attrs \\ %{}) do - true = check.user_id == scope.user.id + true = can_perform?(:edit, scope, check) Check.changeset(check, attrs, scope) end diff --git a/lib/query_canary/checks/check.ex b/lib/query_canary/checks/check.ex index 0e2b294..52a8759 100644 --- a/lib/query_canary/checks/check.ex +++ b/lib/query_canary/checks/check.ex @@ -1,6 +1,10 @@ defmodule QueryCanary.Checks.Check do use Ecto.Schema import Ecto.Changeset + import Ecto.Query + + alias QueryCanary.Accounts.TeamUser + alias QueryCanary.Servers.Server schema "checks" do field :name, :string @@ -25,7 +29,9 @@ defmodule QueryCanary.Checks.Check do |> validate_required([:name, :schedule, :enabled, :query, :server_id]) |> validate_cron_expression(:schedule) |> foreign_key_constraint(:server_id) - |> put_change(:user_id, user_scope.user.id) + |> validate_server_id_unchanged() + |> validate_server_access(user_scope) + |> put_user_id_on_insert(user_scope) end defp validate_cron_expression(changeset, field) do @@ -40,4 +46,49 @@ defmodule QueryCanary.Checks.Check do end end) end + + defp validate_server_access(changeset, user_scope) do + server_id = get_field(changeset, :server_id) + + cond do + Keyword.has_key?(changeset.errors, :server_id) -> + changeset + + is_nil(server_id) -> + changeset + + true -> + user_id = user_scope.user.id + + query = + from s in Server, + left_join: tu in TeamUser, + on: tu.team_id == s.team_id, + where: s.id == ^server_id and (s.user_id == ^user_id or tu.user_id == ^user_id) + + if QueryCanary.Repo.exists?(query) do + changeset + else + add_error(changeset, :server_id, "is not accessible to the current user") + end + end + end + + defp validate_server_id_unchanged(%Ecto.Changeset{data: %__MODULE__{id: nil}} = changeset) do + changeset + end + + defp validate_server_id_unchanged(changeset) do + case get_change(changeset, :server_id) do + nil -> changeset + server_id when server_id == changeset.data.server_id -> changeset + _server_id -> add_error(changeset, :server_id, "cannot be changed after creation") + end + end + + defp put_user_id_on_insert(%Ecto.Changeset{data: %__MODULE__{id: nil}} = changeset, user_scope) do + put_change(changeset, :user_id, user_scope.user.id) + end + + defp put_user_id_on_insert(changeset, _user_scope), do: changeset end diff --git a/test/query_canary/checks_test.exs b/test/query_canary/checks_test.exs index 8c69f24..6ed48d5 100644 --- a/test/query_canary/checks_test.exs +++ b/test/query_canary/checks_test.exs @@ -30,8 +30,9 @@ defmodule QueryCanary.ChecksTest do alias QueryCanary.Checks alias QueryCanary.Checks.Check alias QueryCanary.Checks.CheckResult + alias QueryCanary.Accounts - import QueryCanary.AccountsFixtures, only: [user_scope_fixture: 0] + import QueryCanary.AccountsFixtures, only: [team_fixture: 1, user_scope_fixture: 0] import QueryCanary.ChecksFixtures import QueryCanary.ServersFixtures @@ -55,6 +56,16 @@ defmodule QueryCanary.ChecksTest do assert_raise Ecto.NoResultsError, fn -> Checks.get_check!(other_scope, check.id) end end + test "get_check!/2 returns a team check for a team member" do + owner_scope = user_scope_fixture() + team = team_fixture(owner_scope) + member_scope = add_team_member(owner_scope, team) + server = server_fixture(owner_scope, %{team_id: team.id}) + check = check_fixture(owner_scope, %{server_id: server.id}) + + assert Checks.get_check!(member_scope, check.id).id == check.id + end + test "create_check/2 with valid data creates a check" do scope = user_scope_fixture() server = server_fixture(scope) @@ -87,6 +98,32 @@ defmodule QueryCanary.ChecksTest do assert check.query == "some updated query" end + test "update_check/3 allows a team member to update a check they did not create" do + owner_scope = user_scope_fixture() + team = team_fixture(owner_scope) + member_scope = add_team_member(owner_scope, team) + server = server_fixture(owner_scope, %{team_id: team.id}) + check = check_fixture(owner_scope, %{server_id: server.id}) + + assert {:ok, %Check{} = check} = + Checks.update_check(member_scope, check, %{query: "some updated query"}) + + assert check.query == "some updated query" + assert check.user_id == owner_scope.user.id + end + + test "update_check/3 rejects changing a check's server" do + scope = user_scope_fixture() + other_scope = user_scope_fixture() + check = check_fixture(scope) + other_server = server_fixture(other_scope) + + assert {:error, %Ecto.Changeset{} = changeset} = + Checks.update_check(scope, check, %{server_id: other_server.id}) + + assert {"cannot be changed after creation", _} = changeset.errors[:server_id] + end + test "update_check/3 with invalid scope raises" do scope = user_scope_fixture() other_scope = user_scope_fixture() @@ -111,6 +148,17 @@ defmodule QueryCanary.ChecksTest do assert_raise Ecto.NoResultsError, fn -> Checks.get_check!(scope, check.id) end end + test "delete_check/2 allows a team member to delete a check they did not create" do + owner_scope = user_scope_fixture() + team = team_fixture(owner_scope) + member_scope = add_team_member(owner_scope, team) + server = server_fixture(owner_scope, %{team_id: team.id}) + check = check_fixture(owner_scope, %{server_id: server.id}) + + assert {:ok, %Check{}} = Checks.delete_check(member_scope, check) + assert_raise Ecto.NoResultsError, fn -> Checks.get_check!(owner_scope, check.id) end + end + test "delete_check/2 with invalid scope raises" do scope = user_scope_fixture() other_scope = user_scope_fixture() @@ -124,6 +172,16 @@ defmodule QueryCanary.ChecksTest do assert %Ecto.Changeset{} = Checks.change_check(scope, check) end + test "change_check/2 allows a team member to edit a check they did not create" do + owner_scope = user_scope_fixture() + team = team_fixture(owner_scope) + member_scope = add_team_member(owner_scope, team) + server = server_fixture(owner_scope, %{team_id: team.id}) + check = check_fixture(owner_scope, %{server_id: server.id}) + + assert %Ecto.Changeset{} = Checks.change_check(member_scope, check) + end + test "run_check/1 gives checks a 30 second query timeout" do scope = user_scope_fixture() server = server_fixture(scope) @@ -176,4 +234,13 @@ defmodule QueryCanary.ChecksTest do # assert {:ok, :notification_sent} = Checks.maybe_send_check_notification(check, result) # end end + + defp add_team_member(owner_scope, team) do + member_scope = user_scope_fixture() + + {:ok, _user} = Accounts.invite_user_to_team(owner_scope, team, member_scope.user.email) + {:ok, _team_user} = Accounts.accept_team_invite(member_scope, team) + + member_scope + end end diff --git a/test/query_canary_web/live/check_live_test.exs b/test/query_canary_web/live/check_live_test.exs index b4cfd6c..b4f0ba9 100644 --- a/test/query_canary_web/live/check_live_test.exs +++ b/test/query_canary_web/live/check_live_test.exs @@ -4,6 +4,9 @@ defmodule QueryCanaryWeb.CheckLiveTest do import Phoenix.LiveViewTest import QueryCanary.ChecksFixtures import QueryCanary.AccountsFixtures + import QueryCanary.ServersFixtures + + alias QueryCanary.Accounts @update_attrs %{ name: "Updated Check", @@ -123,5 +126,47 @@ defmodule QueryCanaryWeb.CheckLiveTest do assert redirected_to =~ "/" end + + test "can edit a team check created by another user", %{conn: conn, scope: member_scope} do + check = team_check_for_member(member_scope) + + assert {:ok, form_live, html} = live(conn, ~p"/checks/#{check}/edit") + + assert html =~ "Edit Check" + + assert {:ok, index_live, _html} = + form_live + |> form("#check-form", check: @update_attrs) + |> render_submit() + |> follow_redirect(conn, ~p"/checks") + + html = render(index_live) + assert html =~ "Check updated successfully" + assert html =~ "Updated Check" + end + + test "can delete a team check created by another user from the listing", %{ + conn: conn, + scope: member_scope + } do + check = team_check_for_member(member_scope) + + {:ok, index_live, html} = live(conn, ~p"/checks") + + assert html =~ check.name + assert index_live |> element("#checks-#{check.id} a", "Delete") |> render_click() + refute has_element?(index_live, "#checks-#{check.id}") + end + end + + defp team_check_for_member(member_scope) do + owner_scope = user_scope_fixture() + team = team_fixture(owner_scope) + + {:ok, _user} = Accounts.invite_user_to_team(owner_scope, team, member_scope.user.email) + {:ok, _team_user} = Accounts.accept_team_invite(member_scope, team) + + server = server_fixture(owner_scope, %{team_id: team.id}) + check_fixture(owner_scope, %{server_id: server.id}) end end