From 727e866f98135d58dff7a2293a0a454650b0cafa Mon Sep 17 00:00:00 2001 From: Nelson Kopliku Date: Wed, 10 Jul 2024 11:49:07 +0200 Subject: [PATCH] Add Persistent Activity Logger (#2730) * Adjust ActivityLogger.log_activity signature * Add an ActivityLog catalog determining what is of interest to be logged * Add an ActivityParser behaviour that extracts relevant information from an activity context * Add an ActivityLogWriter behaviour that is responsible to write activity logs to a destination * Add a PhoenixConnParser able to parse/extract activity from phoenix http connections * Add a DatabaseWriter that writes activity logs to the database * Add a Persistent activity logger * Adjust configuration * Define custom activity_type type * Alias Trento Repo in Database log writer * Refactor activity logging * Defer activity log writer abstraction --- config/config.exs | 3 - .../activity_logging/activity_catalog.ex | 82 ++++++++ .../activity_logging/activity_logger.ex | 58 ++++- .../parser/activity_parser.ex | 20 ++ .../parser/phoenix_conn_parser.ex | 140 ++++++++++++ .../logger/adapter/noop_logger.ex | 10 - test/support/conn_case.ex | 16 +- test/test_helper.exs | 8 - .../activity_catalog_test.exs | 120 +++++++++++ .../activity_logging/activity_logger_test.exs | 199 ++++++++++++++++++ .../phoenix_conn_parser_test.exs | 84 ++++++++ .../plugs/activity_logging_plug_test.exs | 113 +--------- 12 files changed, 718 insertions(+), 135 deletions(-) create mode 100644 lib/trento/activity_logging/activity_catalog.ex create mode 100644 lib/trento/activity_logging/parser/activity_parser.ex create mode 100644 lib/trento/activity_logging/parser/phoenix_conn_parser.ex delete mode 100644 lib/trento/infrastructure/activity_log/logger/adapter/noop_logger.ex create mode 100644 test/trento/activity_logging/activity_catalog_test.exs create mode 100644 test/trento/activity_logging/activity_logger_test.exs create mode 100644 test/trento/activity_logging/phoenix_conn_parser_test.exs diff --git a/config/config.exs b/config/config.exs index 0cd262ec67..9b707213b8 100644 --- a/config/config.exs +++ b/config/config.exs @@ -203,9 +203,6 @@ config :trento, Trento.Infrastructure.SoftwareUpdates.MockSuma, relevant_patches config :trento, Trento.Infrastructure.SoftwareUpdates.SumaApi, executor: Trento.Infrastructure.SoftwareUpdates.Suma.HttpExecutor -config :trento, Trento.ActivityLog.ActivityLogger, - adapter: Trento.Infrastructure.ActivityLog.Logger.NoopLogger - config :bodyguard, # The second element of the {:error, reason} tuple returned on auth failure default_error: :forbidden diff --git a/lib/trento/activity_logging/activity_catalog.ex b/lib/trento/activity_logging/activity_catalog.ex new file mode 100644 index 0000000000..1a1738ad17 --- /dev/null +++ b/lib/trento/activity_logging/activity_catalog.ex @@ -0,0 +1,82 @@ +defmodule Trento.ActivityLog.ActivityCatalog do + @moduledoc """ + Activity logging catalog + """ + + @type logged_activity :: {controller :: module(), activity :: atom()} + + @type activity_type :: + :login_attempt + | :resource_tagging + | :resource_untagging + | :api_key_generation + | :saving_suma_settings + | :changing_suma_settings + | :clearing_suma_settings + | :user_creation + | :user_modification + | :user_deletion + | :profile_update + | :cluster_checks_execution_request + + @login_attempt {TrentoWeb.SessionController, :create} + @api_key_generation {TrentoWeb.V1.SettingsController, :update_api_key_settings} + @saving_suma_settings {TrentoWeb.V1.SUMACredentialsController, :create} + @changing_suma_settings {TrentoWeb.V1.SUMACredentialsController, :update} + @clearing_suma_settings {TrentoWeb.V1.SUMACredentialsController, :delete} + @tagging {TrentoWeb.V1.TagsController, :add_tag} + @untagging {TrentoWeb.V1.TagsController, :remove_tag} + @user_creation {TrentoWeb.V1.UsersController, :create} + @user_modification {TrentoWeb.V1.UsersController, :update} + @user_deletion {TrentoWeb.V1.UsersController, :delete} + @profile_update {TrentoWeb.V1.ProfileController, :update} + # @cluster_checks_selection {TrentoWeb.V1.ClusterController, :select_checks} # <-- this is **also** event sourced + @cluster_checks_execution_request {TrentoWeb.V1.ClusterController, :request_checks_execution} + + @activity_catalog %{ + @login_attempt => {:login_attempt, :any}, + @tagging => {:resource_tagging, 201}, + @untagging => {:resource_untagging, 204}, + @api_key_generation => {:api_key_generation, 200}, + @saving_suma_settings => {:saving_suma_settings, 201}, + @changing_suma_settings => {:changing_suma_settings, 200}, + @clearing_suma_settings => {:clearing_suma_settings, 204}, + @user_creation => {:user_creation, 201}, + @user_modification => {:user_modification, 200}, + @user_deletion => {:user_deletion, 204}, + @profile_update => {:profile_update, 200}, + @cluster_checks_execution_request => {:cluster_checks_execution_request, 202} + } + + Enum.each(@activity_catalog, fn {activity, {activity_type, _}} -> + defmacro unquote(activity_type)(), do: unquote(activity) + end) + + def activity_catalog, do: Enum.map(@activity_catalog, fn {activity, _} -> activity end) + + def interested?(activity, %Plug.Conn{status: status}), + do: + Map.has_key?(@activity_catalog, activity) && + @activity_catalog + |> Map.fetch!(activity) + |> interesting_occurrence?(status) + + def interested?(_, _), do: false + + @spec get_activity_type(logged_activity()) :: atom() | nil + def get_activity_type(activity) do + case Map.fetch(@activity_catalog, activity) do + {:ok, {activity_type, _}} -> activity_type + :error -> nil + end + end + + @spec interesting_occurrence?( + conn_activity_occurrence :: {activity_type(), relevant_status :: integer() | :any}, + detected_status :: integer() + ) :: + boolean() + defp interesting_occurrence?({_, :any}, _), do: true + defp interesting_occurrence?({_, status}, status), do: true + defp interesting_occurrence?(_, _), do: false +end diff --git a/lib/trento/activity_logging/activity_logger.ex b/lib/trento/activity_logging/activity_logger.ex index be2099c8f7..df3cb19ae3 100644 --- a/lib/trento/activity_logging/activity_logger.ex +++ b/lib/trento/activity_logging/activity_logger.ex @@ -3,9 +3,61 @@ defmodule Trento.ActivityLog.ActivityLogger do ActivityLogger entry point """ - @callback log_activity(context :: any()) :: :ok + alias Trento.Repo - def log_activity(context), do: adapter().log_activity(context) + alias Trento.ActivityLog.ActivityCatalog + alias Trento.ActivityLog.ActivityLog - defp adapter, do: Application.fetch_env!(:trento, __MODULE__)[:adapter] + require Logger + + def log_activity(activity_context) do + with {:ok, activity_parser} <- get_activity_parser(activity_context), + detected_activity <- detect_activity(activity_parser, activity_context), + true <- ActivityCatalog.interested?(detected_activity, activity_context) do + write_log(%{ + type: get_activity_type(detected_activity), + actor: get_actor(activity_parser, detected_activity, activity_context), + metadata: get_metadata(activity_parser, detected_activity, activity_context) + }) + end + + :ok + end + + defp get_activity_parser(%Plug.Conn{}), + do: {:ok, Trento.ActivityLog.Logger.Parser.PhoenixConnParser} + + defp get_activity_parser(_), do: {:error, :unsupported_activity} + + # defp get_activity_parser(%Commanded.Middleware.Pipeline{}), + # do: {:ok, Trento.ActivityLog.Logger.Parser.CommandedParser} + + defp detect_activity(activity_parser, activity_context), + do: activity_parser.detect_activity(activity_context) + + defp get_activity_type(detected_activity), + do: + detected_activity + |> ActivityCatalog.get_activity_type() + |> Atom.to_string() + + defp get_actor(activity_parser, detected_activity, activity_context), + do: activity_parser.get_activity_actor(detected_activity, activity_context) + + defp get_metadata(activity_parser, detected_activity, activity_context), + do: activity_parser.get_activity_metadata(detected_activity, activity_context) + + defp write_log(%{type: activity_type} = entry) do + case %ActivityLog{} + |> ActivityLog.changeset(entry) + |> Repo.insert() do + {:ok, _} -> + Logger.info("Logged activity: #{activity_type}") + + {:error, reason} -> + Logger.error( + "An error occurred while logging activity: #{activity_type}. Reason: #{inspect(reason)}" + ) + end + end end diff --git a/lib/trento/activity_logging/parser/activity_parser.ex b/lib/trento/activity_logging/parser/activity_parser.ex new file mode 100644 index 0000000000..4c81671429 --- /dev/null +++ b/lib/trento/activity_logging/parser/activity_parser.ex @@ -0,0 +1,20 @@ +defmodule Trento.ActivityLog.Parser.ActivityParser do + @moduledoc """ + Behavior for activity parsers. + It extracts the activity relevant information from the context. + """ + + alias Trento.ActivityLog.ActivityCatalog + + @callback detect_activity(activity_context :: any()) :: ActivityCatalog.logged_activity() | nil + + @callback get_activity_actor( + activity :: ActivityCatalog.logged_activity(), + activity_context :: any() + ) :: any() + + @callback get_activity_metadata( + activity :: ActivityCatalog.logged_activity(), + activity_context :: any() + ) :: map() +end diff --git a/lib/trento/activity_logging/parser/phoenix_conn_parser.ex b/lib/trento/activity_logging/parser/phoenix_conn_parser.ex new file mode 100644 index 0000000000..273bb298f7 --- /dev/null +++ b/lib/trento/activity_logging/parser/phoenix_conn_parser.ex @@ -0,0 +1,140 @@ +defmodule Trento.ActivityLog.Logger.Parser.PhoenixConnParser do + @moduledoc """ + Phoenix connection activity parser + """ + + alias Phoenix.Controller + + alias Trento.Users.User + + require Trento.ActivityLog.ActivityCatalog, as: ActivityCatalog + + @behaviour Trento.ActivityLog.Parser.ActivityParser + + @impl true + def detect_activity(%Plug.Conn{} = conn) do + {Controller.controller_module(conn), Controller.action_name(conn)} + rescue + _ -> nil + end + + @impl true + def get_activity_actor(ActivityCatalog.login_attempt(), %Plug.Conn{body_params: request_payload}), + do: Map.get(request_payload, "username", "no_username") + + @impl true + def get_activity_actor(_, %Plug.Conn{} = conn) do + case Pow.Plug.current_user(conn) do + %User{username: username} -> username + _ -> "system" + end + end + + @impl true + def get_activity_metadata( + ActivityCatalog.login_attempt() = action, + %Plug.Conn{ + assigns: %{ + reason: reason + }, + status: 401 + } = conn + ) do + %{ + username: get_activity_actor(action, conn), + reason: reason + } + end + + @impl true + def get_activity_metadata( + ActivityCatalog.resource_tagging(), + %Plug.Conn{ + params: %{id: resource_id}, + assigns: %{ + resource_type: resource_type + }, + body_params: %{value: added_tag} + } + ) do + %{ + resource_id: resource_id, + resource_type: resource_type, + added_tag: added_tag + } + end + + @impl true + def get_activity_metadata( + ActivityCatalog.resource_untagging(), + %Plug.Conn{ + params: %{ + id: resource_id, + value: removed_tag + }, + assigns: %{ + resource_type: resource_type + } + } + ) do + %{ + resource_id: resource_id, + resource_type: resource_type, + removed_tag: removed_tag + } + end + + @impl true + def get_activity_metadata( + ActivityCatalog.api_key_generation(), + %Plug.Conn{ + body_params: request_body + } + ) do + request_body + end + + @impl true + def get_activity_metadata( + action, + %Plug.Conn{ + body_params: request_body + } + ) + when action in [ + ActivityCatalog.saving_suma_settings(), + ActivityCatalog.changing_suma_settings() + ] do + request_body + |> redact(:password) + |> redact(:ca_cert) + end + + @impl true + def get_activity_metadata( + action, + %Plug.Conn{ + body_params: request_body + } + ) + when action in [ + ActivityCatalog.user_creation(), + ActivityCatalog.user_modification(), + ActivityCatalog.profile_update() + ] do + request_body + |> redact(:password) + |> redact(:current_password) + |> redact(:password_confirmation) + end + + @impl true + def get_activity_metadata(_, _), do: %{} + + defp redact(request_body, key) do + case Map.has_key?(request_body, key) && Map.fetch!(request_body, key) != nil do + true -> Map.update!(request_body, key, fn _ -> "REDACTED" end) + false -> request_body + end + end +end diff --git a/lib/trento/infrastructure/activity_log/logger/adapter/noop_logger.ex b/lib/trento/infrastructure/activity_log/logger/adapter/noop_logger.ex deleted file mode 100644 index 906e0c5e3e..0000000000 --- a/lib/trento/infrastructure/activity_log/logger/adapter/noop_logger.ex +++ /dev/null @@ -1,10 +0,0 @@ -defmodule Trento.Infrastructure.ActivityLog.Logger.NoopLogger do - @moduledoc """ - Noop Activity Logger Adapter - """ - - @behaviour Trento.ActivityLog.ActivityLogger - - @impl true - def log_activity(_), do: :ok -end diff --git a/test/support/conn_case.ex b/test/support/conn_case.ex index d15b7fd11a..43448e5887 100644 --- a/test/support/conn_case.ex +++ b/test/support/conn_case.ex @@ -17,6 +17,8 @@ defmodule TrentoWeb.ConnCase do use ExUnit.CaseTemplate + use Trento.TaskCase + alias Ecto.Adapters.SQL.Sandbox using do @@ -35,17 +37,13 @@ defmodule TrentoWeb.ConnCase do setup tags do pid = Sandbox.start_owner!(Trento.Repo, shared: not tags[:async]) - on_exit(fn -> Sandbox.stop_owner(pid) end) - stub_activity_logger() + on_exit(fn -> + wait_for_tasks_completion() + + Sandbox.stop_owner(pid) + end) {:ok, conn: Phoenix.ConnTest.build_conn()} end - - defp stub_activity_logger, - do: - Mox.stub_with( - Trento.ActivityLog.ActivityLogger.Mock, - Trento.Infrastructure.ActivityLog.Logger.NoopLogger - ) end diff --git a/test/test_helper.exs b/test/test_helper.exs index 5bc8c0f5a6..791f09d3a3 100644 --- a/test/test_helper.exs +++ b/test/test_helper.exs @@ -30,14 +30,6 @@ Application.put_env(:trento, Trento.Infrastructure.SoftwareUpdates.Suma, auth: Trento.Infrastructure.SoftwareUpdates.Auth.Mock ) -Mox.defmock(Trento.ActivityLog.ActivityLogger.Mock, - for: Trento.ActivityLog.ActivityLogger -) - -Application.put_env(:trento, Trento.ActivityLog.ActivityLogger, - adapter: Trento.ActivityLog.ActivityLogger.Mock -) - Mox.defmock(Trento.Infrastructure.Messaging.Adapter.Mock, for: Trento.Infrastructure.Messaging.Adapter.Gen ) diff --git a/test/trento/activity_logging/activity_catalog_test.exs b/test/trento/activity_logging/activity_catalog_test.exs new file mode 100644 index 0000000000..201ddecf69 --- /dev/null +++ b/test/trento/activity_logging/activity_catalog_test.exs @@ -0,0 +1,120 @@ +defmodule Trento.ActivityLog.ActivityCatalogTest do + @moduledoc false + + use TrentoWeb.ConnCase, async: false + + alias Trento.ActivityLog.ActivityCatalog + + require Trento.ActivityLog.ActivityCatalog, as: ActivityCatalog + + test "should ignore unknown activities", %{ + conn: conn + } do + Enum.each([:foo_bar, %{bar: "baz"}, "not-interesting"], fn activity -> + refute ActivityCatalog.interested?(activity, conn) + assert nil == ActivityCatalog.get_activity_type(activity) + end) + end + + scenarios = [ + %{ + activity: ActivityCatalog.login_attempt(), + interesting_statuses: [200, 401, 404, 500], + expected_activity_type: :login_attempt + }, + %{ + activity: ActivityCatalog.resource_tagging(), + interesting_statuses: 201, + not_interesting_statuses: [400, 401, 403, 404, 500], + expected_activity_type: :resource_tagging + }, + %{ + activity: ActivityCatalog.resource_untagging(), + interesting_statuses: 204, + not_interesting_statuses: [400, 401, 403, 404, 500], + expected_activity_type: :resource_untagging + }, + %{ + activity: ActivityCatalog.api_key_generation(), + interesting_statuses: 200, + not_interesting_statuses: [400, 401, 403, 404, 500], + expected_activity_type: :api_key_generation + }, + %{ + activity: ActivityCatalog.saving_suma_settings(), + interesting_statuses: 201, + not_interesting_statuses: [400, 401, 403, 404, 500], + expected_activity_type: :saving_suma_settings + }, + %{ + activity: ActivityCatalog.changing_suma_settings(), + interesting_statuses: 200, + not_interesting_statuses: [400, 401, 403, 404, 500], + expected_activity_type: :changing_suma_settings + }, + %{ + activity: ActivityCatalog.clearing_suma_settings(), + interesting_statuses: 204, + not_interesting_statuses: [400, 401, 403, 404, 500], + expected_activity_type: :clearing_suma_settings + }, + %{ + activity: ActivityCatalog.user_creation(), + interesting_statuses: 201, + not_interesting_statuses: [400, 401, 403, 404, 500], + expected_activity_type: :user_creation + }, + %{ + activity: ActivityCatalog.user_modification(), + interesting_statuses: 200, + not_interesting_statuses: [400, 401, 403, 404, 500], + expected_activity_type: :user_modification + }, + %{ + activity: ActivityCatalog.user_deletion(), + interesting_statuses: 204, + not_interesting_statuses: [400, 401, 403, 404, 500], + expected_activity_type: :user_deletion + }, + %{ + activity: ActivityCatalog.profile_update(), + interesting_statuses: 200, + not_interesting_statuses: [400, 401, 403, 404, 500], + expected_activity_type: :profile_update + }, + %{ + activity: ActivityCatalog.cluster_checks_execution_request(), + interesting_statuses: 202, + not_interesting_statuses: [400, 401, 403, 404, 500], + expected_activity_type: :cluster_checks_execution_request + } + ] + + for %{expected_activity_type: expected_activity_type} = scenario <- scenarios do + @scenario scenario + + test "should detect interesting connection for activity #{expected_activity_type}", %{ + conn: conn + } do + %{ + activity: activity, + interesting_statuses: interesting_statuses, + expected_activity_type: expected_activity_type + } = @scenario + + interesting_statuses + |> List.wrap() + |> Enum.each(fn status -> + assert ActivityCatalog.interested?(activity, put_status(conn, status)) + end) + + Map.get(@scenario, :not_interesting_statuses, []) + |> List.wrap() + |> Enum.each(fn status -> + refute ActivityCatalog.interested?(activity, put_status(conn, status)) + end) + + assert ActivityCatalog.get_activity_type(activity) == expected_activity_type + end + end +end diff --git a/test/trento/activity_logging/activity_logger_test.exs b/test/trento/activity_logging/activity_logger_test.exs new file mode 100644 index 0000000000..639880a542 --- /dev/null +++ b/test/trento/activity_logging/activity_logger_test.exs @@ -0,0 +1,199 @@ +defmodule Trento.ActivityLog.ActivityLoggerTest do + @moduledoc false + + use TrentoWeb.ConnCase, async: true + use Plug.Test + + import Trento.Factory + + import Mox + + alias Trento.Abilities.Ability + + alias Trento.ActivityLog.ActivityLog + alias Trento.Tags.Tag + + alias TrentoWeb.Auth.AccessToken + + setup [:set_mox_from_context, :verify_on_exit!] + + setup do + {:ok, user} = + Trento.Users.create_user(%{ + username: "admin", + password: "testpassword", + confirm_password: "testpassword", + email: "test@trento.com", + fullname: "Full Name", + abilities: [%Ability{id: 1}] + }) + + stub( + Joken.CurrentTime.Mock, + :current_time, + fn -> + 1_671_715_992 + end + ) + + {:ok, user: user} + end + + defp with_token(conn, user_id) do + jwt = AccessToken.generate_access_token!(%{"sub" => user_id}) + + Plug.Conn.put_req_header(conn, "authorization", "Bearer " <> jwt) + end + + describe "login activity detection" do + defp login(conn, credentials) do + post(conn, "/api/session", credentials) + end + + login_scenarios = [ + %{ + name: "with invalid credentials", + credentials: %{ + username: "foo", + password: "bar" + }, + expected_metadata: %{ + "username" => "foo", + "reason" => "Invalid credentials." + } + }, + %{ + name: "with valid credentials", + credentials: %{ + username: "admin", + password: "testpassword" + }, + expected_metadata: %{} + } + ] + + for %{name: name} = scenario <- login_scenarios do + @scenario scenario + + test "should get the metadata for a login attempt: #{name}", %{conn: conn} do + %{credentials: %{username: username} = credentials, expected_metadata: expected_metadata} = + @scenario + + login(conn, credentials) + + wait_for_tasks_completion() + + assert [ + %ActivityLog{ + type: "login_attempt", + actor: ^username, + metadata: ^expected_metadata + } + ] = Trento.Repo.all(ActivityLog) + end + end + end + + describe "tagging/untagging activity detection" do + defp tag_resource(conn, resource_id, resource_type, tag) do + conn + |> put_req_header("content-type", "application/json") + |> post("/api/v1/#{resource_type}/#{resource_id}/tags", %{ + "value" => tag + }) + end + + defp untag_resource(conn, resource_id, resource_type, tag) do + conn + |> put_req_header("content-type", "application/json") + |> delete("/api/v1/#{resource_type}/#{resource_id}/tags/#{tag}") + end + + tagging_extraction_scenarios = [ + %{ + path_resource: "hosts", + expected_resource_type: "host" + }, + %{ + path_resource: "clusters", + expected_resource_type: "cluster" + }, + %{ + path_resource: "databases", + expected_resource_type: "database" + }, + %{ + path_resource: "sap_systems", + expected_resource_type: "sap_system" + } + ] + + for %{path_resource: path_resource} = scenario <- tagging_extraction_scenarios do + @scenario scenario + + test "should get the metadata for resource tagging activity: #{path_resource}", %{ + conn: conn, + user: %{id: user_id, username: username} + } do + %{path_resource: path_resource, expected_resource_type: expected_resource_type} = + @scenario + + resource_id = Faker.UUID.v4() + tag = Faker.Lorem.word() + + conn + |> with_token(user_id) + |> tag_resource(resource_id, path_resource, tag) + + wait_for_tasks_completion() + + assert [ + %ActivityLog{ + type: "resource_tagging", + actor: ^username, + metadata: %{ + "resource_id" => ^resource_id, + "resource_type" => ^expected_resource_type, + "added_tag" => ^tag + } + } + ] = Trento.Repo.all(ActivityLog) + end + end + + for %{path_resource: path_resource} = scenario <- tagging_extraction_scenarios do + @scenario scenario + + test "should get the metadata for resource untagging activity: #{path_resource}", %{ + conn: conn, + user: %{id: user_id, username: username} + } do + %{path_resource: path_resource, expected_resource_type: expected_resource_type} = + @scenario + + %Tag{ + value: tag, + resource_id: resource_id + } = insert(:tag, resource_type: expected_resource_type) + + conn + |> with_token(user_id) + |> untag_resource(resource_id, path_resource, tag) + + wait_for_tasks_completion() + + assert [ + %ActivityLog{ + type: "resource_untagging", + actor: ^username, + metadata: %{ + "resource_id" => ^resource_id, + "resource_type" => ^expected_resource_type, + "removed_tag" => ^tag + } + } + ] = Trento.Repo.all(ActivityLog) + end + end + end +end diff --git a/test/trento/activity_logging/phoenix_conn_parser_test.exs b/test/trento/activity_logging/phoenix_conn_parser_test.exs new file mode 100644 index 0000000000..b4400f8494 --- /dev/null +++ b/test/trento/activity_logging/phoenix_conn_parser_test.exs @@ -0,0 +1,84 @@ +defmodule Trento.ActivityLog.PhoenixConnParserTest do + @moduledoc false + + use TrentoWeb.ConnCase, async: false + use Plug.Test + + import Trento.Factory + + alias Trento.ActivityLog.Logger.Parser.PhoenixConnParser + + require Trento.ActivityLog.ActivityCatalog, as: ActivityCatalog + + describe "activity detection" do + test "should not be able to detect an activity from a connection without controller/action info", + %{conn: conn} do + assert nil == PhoenixConnParser.detect_activity(conn) + end + + test "should detect activity from a connection with controller/action info", %{conn: conn} do + assert {Foo.Bar.AcmeController, :foo_action} == + conn + |> Plug.Conn.put_private(:phoenix_controller, Foo.Bar.AcmeController) + |> Plug.Conn.put_private(:phoenix_action, :foo_action) + |> PhoenixConnParser.detect_activity() + end + end + + describe "actor detection" do + test "should get the actor from the request payload for login attempts", %{conn: conn} do + scenarios = [ + %{ + body_params: %{"username" => "foo"}, + expected_username: "foo" + }, + %{ + body_params: %{"not-username" => "bar"}, + expected_username: "no_username" + }, + %{ + body_params: %{}, + expected_username: "no_username" + } + ] + + for %{body_params: body_params, expected_username: expected_username} <- scenarios do + assert PhoenixConnParser.get_activity_actor(ActivityCatalog.login_attempt(), %{ + conn + | body_params: body_params + }) == expected_username + end + end + + test "should get the actor from the current user", %{conn: conn} do + %{username: expected_username} = user = build(:user) + + pow_config = [otp_app: :trento] + + conn = + conn + |> Pow.Plug.put_config(pow_config) + |> Pow.Plug.assign_current_user(user, pow_config) + + assert_for_relevant_activity(fn activity -> + assert PhoenixConnParser.get_activity_actor(activity, conn) == expected_username + end) + end + + test "should fallback to system actor", %{conn: conn} do + pow_config = [otp_app: :trento] + + conn = Pow.Plug.put_config(conn, pow_config) + + assert_for_relevant_activity(fn activity -> + assert PhoenixConnParser.get_activity_actor(activity, conn) == "system" + end) + end + end + + defp assert_for_relevant_activity(assertion_function) do + ActivityCatalog.activity_catalog() + |> Enum.filter(&(&1 != ActivityCatalog.login_attempt())) + |> Enum.each(fn activity -> assertion_function.(activity) end) + end +end diff --git a/test/trento_web/plugs/activity_logging_plug_test.exs b/test/trento_web/plugs/activity_logging_plug_test.exs index 5eefa1e6d0..32fb99470b 100644 --- a/test/trento_web/plugs/activity_logging_plug_test.exs +++ b/test/trento_web/plugs/activity_logging_plug_test.exs @@ -2,115 +2,24 @@ defmodule TrentoWeb.Plugs.ActivityLoggingPlugTest do @moduledoc false use Plug.Test use TrentoWeb.ConnCase, async: true - use Trento.TaskCase - import Mox - - import Trento.Factory - - alias Trento.Users.User alias TrentoWeb.Plugs.ActivityLoggingPlug - require Logger - - setup :verify_on_exit! - - describe "logging activity on connections" do - for method <- [:get, :post, :put, :patch, :delete] do - @method method - test "should log activity on requests without user information - method: #{method}" do - %{private: private} = conn = build_conn(@method, "/foo/bar", nil) - refute Map.has_key?(private, :before_send) - - expect(Trento.ActivityLog.ActivityLogger.Mock, :log_activity, 1, fn %Plug.Conn{ - assigns: assigns - } -> - refute Map.has_key?(assigns, :current_user) - :ok - end) - - %{private: %{before_send: [logging_function | _]}} = - conn_with_registered_logger = ActivityLoggingPlug.call(conn) - - logging_function.(conn_with_registered_logger) - - wait_for_tasks_completion() - end - - test "should log activity on requests with stateless user information - method #{method}" do - %{id: user_id} = insert(:user) - - pow_config = [otp_app: :trento] - conn = Pow.Plug.put_config(build_conn(), pow_config) - - conn = Pow.Plug.assign_current_user(conn, %{"user_id" => user_id}, pow_config) + for method <- [:get, :post, :put, :patch, :delete] do + @method method + test "should log activity on requests without user information - method: #{method}" do + %{private: private} = conn = build_conn(@method, "/foo/bar", nil) + refute Map.has_key?(private, :before_send) - expect(Trento.ActivityLog.ActivityLogger.Mock, :log_activity, 1, fn %Plug.Conn{ - assigns: %{ - current_user: - %User{ - id: ^user_id - } - } - } -> - :ok - end) + %{private: modified_private} = ActivityLoggingPlug.call(conn) - %{private: %{before_send: [logging_function | _]}} = - conn_with_registered_logger = ActivityLoggingPlug.call(conn) - - logging_function.(conn_with_registered_logger) - - wait_for_tasks_completion() - end - - test "should log activity on requests with already loaded user information - method #{method}" do - user = insert(:user) - - pow_config = [otp_app: :trento] - conn = Pow.Plug.put_config(build_conn(), pow_config) - - conn = Pow.Plug.assign_current_user(conn, user, pow_config) - - expect(Trento.ActivityLog.ActivityLogger.Mock, :log_activity, 1, fn %Plug.Conn{ - assigns: %{ - current_user: - ^user - } - } -> - :ok - end) - - %{private: %{before_send: [logging_function | _]}} = - conn_with_registered_logger = ActivityLoggingPlug.call(conn) - - logging_function.(conn_with_registered_logger) - - wait_for_tasks_completion() - end - end - end + assert Map.has_key?(modified_private, :before_send) + assert %{before_send: [logging_function]} = modified_private - describe "logging activity on requests" do - test "should log tagging activity", %{conn: conn} do - expect(Trento.ActivityLog.ActivityLogger.Mock, :log_activity, 1, fn %Plug.Conn{ - body_params: %{ - value: "some-tag" - }, - assigns: %{ - current_user: _, - resource_type: :host - }, - status: 201 - } -> - :ok - end) + function_info = Function.info(logging_function) - conn - |> put_req_header("content-type", "application/json") - |> post("/api/v1/hosts/#{Faker.UUID.v4()}/tags", %{ - "value" => "some-tag" - }) + assert Keyword.get(function_info, :module) == TrentoWeb.Plugs.ActivityLoggingPlug + assert Keyword.get(function_info, :name) == :log_activity end end end