From a278d523075ac2850f1b0e7c811a1a8a748d794b Mon Sep 17 00:00:00 2001 From: Rory McKinley Date: Wed, 8 Jan 2025 12:18:10 +0200 Subject: [PATCH] 2668 GitHub connection auditing (#2742) * Trivial doc changes * Audit repo creation * Audit repo removals * tidy up * Update CHANGELOG * Fix typo * Update CHANGELOG * Move auditing to VersionControl.Audit * Fix --- CHANGELOG.md | 2 + DEPLOYMENT.md | 6 +- lib/lightning/version_control/audit.ex | 57 +++++++ .../version_control/version_control.ex | 17 +- .../usage_tracking/day_worker_test.exs | 2 +- test/lightning/version_control/audit_test.exs | 140 +++++++++++++++ test/lightning/version_control_test.exs | 160 +++++++++++++++++- 7 files changed, 376 insertions(+), 8 deletions(-) create mode 100644 lib/lightning/version_control/audit.ex create mode 100644 test/lightning/version_control/audit_test.exs diff --git a/CHANGELOG.md b/CHANGELOG.md index 5b58256e30..42a57d6ca3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -66,6 +66,8 @@ and this project adheres to - Handle errors from the AI Assistant more gracefully [#2741](https://github.com/OpenFn/lightning/issues/2741) +- Audit the creation and removal of Github repo connections. + [#2668](https://github.com/OpenFn/lightning/issues/2668) ### Changed diff --git a/DEPLOYMENT.md b/DEPLOYMENT.md index 2c56925c88..f4e3fe366b 100644 --- a/DEPLOYMENT.md +++ b/DEPLOYMENT.md @@ -59,10 +59,10 @@ Note that for secure deployments, it's recommended to use a combination of JSON plug and may (in future) limit the size of dataclips that can be stored as run_results via the websocket connection from a worker. -### Github +### GitHub -Lightning enables connection to github via Github Apps. The following github -permissions are needed for the github app: +Lightning enables connection to GitHub via GitHub Apps. The following GitHub +repository permissions are needed for the GitHub app: | **Resource** | **Access** | | ------------ | -------------- | diff --git a/lib/lightning/version_control/audit.ex b/lib/lightning/version_control/audit.ex new file mode 100644 index 0000000000..d84cdfd9ca --- /dev/null +++ b/lib/lightning/version_control/audit.ex @@ -0,0 +1,57 @@ +defmodule Lightning.VersionControl.Audit do + @moduledoc """ + Generate Audit changesets for changes related to VersionControl. + """ + use Lightning.Auditing.Audit, + repo: Lightning.Repo, + item: "project", + events: [ + "repo_connection_created", + "repo_connection_removed" + ] + + @spec repo_connection( + Lightning.VersionControl.ProjectRepoConnection.t(), + :created | :removed, + Lightning.Accounts.User.t() + | Lightning.VersionControl.ProjectRepoConnection.t() + | Lightning.Workflows.Trigger.t() + ) :: Ecto.Changeset.t() + def repo_connection(repo_connection, action, actor) do + %{ + branch: branch, + config_path: config_path, + project_id: project_id, + repo: repo + } = repo_connection + + changes = + %{ + branch: branch, + repo: repo + } + |> then(fn connection_properties -> + if config_path do + Map.put(connection_properties, :config_path, config_path) + else + connection_properties + end + end) + |> then(fn connection_properties -> + if action == :created do + %{ + after: + Map.put( + connection_properties, + :sync_direction, + repo_connection.sync_direction + ) + } + else + %{before: connection_properties} + end + end) + + event("repo_connection_#{action}", project_id, actor, changes) + end +end diff --git a/lib/lightning/version_control/version_control.ex b/lib/lightning/version_control/version_control.ex index 803de7ceb2..2630ce6cc1 100644 --- a/lib/lightning/version_control/version_control.ex +++ b/lib/lightning/version_control/version_control.ex @@ -8,9 +8,11 @@ defmodule Lightning.VersionControl do import Ecto.Query, warn: false + alias Ecto.Multi alias Lightning.Accounts.User alias Lightning.Extensions.UsageLimiting alias Lightning.Repo + alias Lightning.VersionControl.Audit alias Lightning.VersionControl.Events alias Lightning.VersionControl.GithubClient alias Lightning.VersionControl.GithubError @@ -35,6 +37,10 @@ defmodule Lightning.VersionControl do Repo.transact(fn -> with {:ok, repo_connection} <- Repo.insert(changeset), + {:ok, _audit} <- + repo_connection + |> Audit.repo_connection(:created, user) + |> Repo.insert(), :ok <- VersionControlUsageLimiter.limit_github_sync( repo_connection.project_id @@ -103,10 +109,15 @@ defmodule Lightning.VersionControl do Deletes a github connection """ def remove_github_connection(repo_connection, user) do - repo_connection - |> Repo.delete() + Multi.new() + |> Multi.delete(:delete_repo_connection, repo_connection) + |> Multi.insert( + :audit, + Audit.repo_connection(repo_connection, :removed, user) + ) + |> Repo.transaction() |> tap(fn - {:ok, repo_connection} -> + {:ok, %{delete_repo_connection: repo_connection}} -> undo_repo_actions( repo_connection, user diff --git a/test/lightning/usage_tracking/day_worker_test.exs b/test/lightning/usage_tracking/day_worker_test.exs index 07c6bc86fa..18f0f374c3 100644 --- a/test/lightning/usage_tracking/day_worker_test.exs +++ b/test/lightning/usage_tracking/day_worker_test.exs @@ -29,7 +29,7 @@ defmodule Lightning.UsageTracking.DayWorkerTest do %{tracking_enabled_at: enabled_at} = Repo.one(DailyReportConfiguration) - assert DateTime.diff(DateTime.utc_now(), enabled_at, :second) < 2 + assert DateTime.diff(DateTime.utc_now(), enabled_at, :second) < 3 end test "does not enqueue more jobs than the batch size" do diff --git a/test/lightning/version_control/audit_test.exs b/test/lightning/version_control/audit_test.exs new file mode 100644 index 0000000000..bc49494be7 --- /dev/null +++ b/test/lightning/version_control/audit_test.exs @@ -0,0 +1,140 @@ +defmodule Lightning.VersionControl.AuditTest do + use Lightning.DataCase, async: true + + alias Lightning.VersionControl.Audit + + describe "repo_connection/3 - created" do + test "returns a changeset including the config_path" do + %{ + branch: branch, + config_path: config_path, + project_id: project_id, + repo: repo, + sync_direction: sync_direction + } = + repo_connection = + insert(:project_repo_connection, config_path: "config_path") + + %{id: user_id} = user = insert(:user) + + changeset = + Audit.repo_connection(repo_connection, :created, user) + + assert %{ + changes: %{ + event: "repo_connection_created", + item_type: "project", + item_id: ^project_id, + actor_id: ^user_id, + changes: %{ + changes: audit_changes + } + }, + valid?: true + } = changeset + + assert %{ + after: %{ + branch: branch, + config_path: config_path, + repo: repo, + sync_direction: sync_direction + } + } == audit_changes + end + + test "excludes the config_path from the changeset if it is nil" do + %{ + branch: branch, + repo: repo, + sync_direction: sync_direction + } = + repo_connection = + insert(:project_repo_connection, config_path: nil) + + user = insert(:user) + + changeset = + Audit.repo_connection(repo_connection, :created, user) + + %{changes: %{changes: %{changes: audit_changes}}} = changeset + + assert %{ + after: %{ + branch: branch, + repo: repo, + sync_direction: sync_direction + } + } == audit_changes + end + end + + describe "repo_connection/3 - :removed" do + test "returns a changeset including the config_path" do + %{ + branch: branch, + config_path: config_path, + project_id: project_id, + repo: repo + } = + repo_connection = + insert(:project_repo_connection, config_path: "config_path") + + %{id: user_id} = user = insert(:user) + + changeset = + Audit.repo_connection(repo_connection, :removed, user) + + assert %{ + changes: %{ + event: "repo_connection_removed", + item_type: "project", + item_id: ^project_id, + actor_id: ^user_id, + changes: %{ + changes: audit_changes + } + }, + valid?: true + } = changeset + + assert %{ + before: %{ + branch: branch, + config_path: config_path, + repo: repo + } + } == audit_changes + end + + test "excludes the config_path if it is nil" do + %{ + branch: branch, + repo: repo + } = + repo_connection = + insert(:project_repo_connection, config_path: nil) + + user = insert(:user) + + changeset = + Audit.repo_connection(repo_connection, :removed, user) + + assert %{ + changes: %{ + changes: %{ + changes: audit_changes + } + }, + valid?: true + } = changeset + + assert %{ + before: %{ + branch: branch, + repo: repo + } + } == audit_changes + end + end +end diff --git a/test/lightning/version_control_test.exs b/test/lightning/version_control_test.exs index 05b478550f..0223014980 100644 --- a/test/lightning/version_control_test.exs +++ b/test/lightning/version_control_test.exs @@ -2,9 +2,9 @@ defmodule Lightning.VersionControlTest do use Lightning.DataCase, async: true alias Lightning.Auditing.Audit + alias Lightning.Repo alias Lightning.VersionControl alias Lightning.VersionControl.ProjectRepoConnection - alias Lightning.Repo alias Lightning.Workflows.Snapshot import Lightning.Factories @@ -124,6 +124,99 @@ defmodule Lightning.VersionControlTest do params["github_installation_id"] end + test "creating the repo connection creates an audit entry" do + %{id: project_id} = insert(:project) + %{id: user_id} = user = user_with_valid_github_oauth() + + repo = "someaccount/somerepo" + branch = "somebranch" + github_installation_id = "1234" + + expected_installation = %{ + "id" => github_installation_id, + "account" => %{ + "type" => "User", + "login" => "username" + } + } + + expected_repo = %{ + "full_name" => repo, + "default_branch" => "main" + } + + expected_branch = %{"name" => branch} + + # push pull.yml + expect_get_repo(expected_repo["full_name"], 200, expected_repo) + expect_create_blob(expected_repo["full_name"]) + + expect_get_commit( + expected_repo["full_name"], + expected_repo["default_branch"] + ) + + expect_create_tree(expected_repo["full_name"]) + expect_create_commit(expected_repo["full_name"]) + + expect_update_ref( + expected_repo["full_name"], + expected_repo["default_branch"] + ) + + # push deploy.yml + config.json + # deploy.yml blob + expect_create_blob(expected_repo["full_name"]) + # config.json blob + expect_create_blob(expected_repo["full_name"]) + expect_get_commit(expected_repo["full_name"], expected_branch["name"]) + expect_create_tree(expected_repo["full_name"]) + expect_create_commit(expected_repo["full_name"]) + expect_update_ref(expected_repo["full_name"], expected_branch["name"]) + + # write secret + expect_get_public_key(expected_repo["full_name"]) + secret_name = "OPENFN_#{String.replace(project_id, "-", "_")}_API_KEY" + expect_create_repo_secret(expected_repo["full_name"], secret_name) + + # initialize sync + expect_create_installation_token(expected_installation["id"]) + expect_get_repo(expected_repo["full_name"], 200, expected_repo) + + expect_create_workflow_dispatch( + expected_repo["full_name"], + "openfn-pull.yml" + ) + + params = %{ + "project_id" => project_id, + "repo" => repo, + "branch" => branch, + "github_installation_id" => github_installation_id, + "sync_direction" => "pull", + "accept" => "true" + } + + {:ok, %{id: _repo_connection_id}} = + VersionControl.create_github_connection(params, user) + + audit = Repo.one!(Audit) + + assert %{ + event: "repo_connection_created", + item_id: ^project_id, + item_type: "project", + actor_id: ^user_id, + changes: %{ + after: %{ + "repo" => ^repo, + "branch" => ^branch, + "sync_direction" => "pull" + } + } + } = audit + end + test "user without an oauth token cannot create a repo connection" do project = insert(:project) user = insert(:user) @@ -204,6 +297,71 @@ defmodule Lightning.VersionControlTest do assert Repo.aggregate(ProjectRepoConnection, :count) == 0 end + test "audits the removal of the repo connection" do + %{id: project_id} = project = insert(:project) + %{id: user_id} = user = user_with_valid_github_oauth() + + repo = "someaccount/somerepo" + branch = "somebranch" + + repo_connection = + insert(:project_repo_connection, + project: project, + repo: repo, + branch: branch, + github_installation_id: "1234", + access_token: "someaccesstoken" + ) + + assert is_map(user.github_oauth_token) + + # check if deploy yml exists for deletion + expected_deploy_yml_path = + ".github/workflows/openfn-#{project.id}-deploy.yml" + + expect_get_repo_content(repo_connection.repo, expected_deploy_yml_path) + + # deletes successfully + expect_delete_repo_content( + repo_connection.repo, + expected_deploy_yml_path + ) + + # check if deploy yml exists for deletion + expected_config_json_path = "openfn-#{project.id}-config.json" + expect_get_repo_content(repo_connection.repo, expected_config_json_path) + # fails to delete + expect_delete_repo_content( + repo_connection.repo, + expected_config_json_path, + 400, + %{"something" => "happened"} + ) + + # delete secret + expect_delete_repo_secret( + repo_connection.repo, + "OPENFN_#{String.replace(project.id, "-", "_")}_API_KEY" + ) + + VersionControl.remove_github_connection(repo_connection, user) + + audit = Repo.one!(Audit) + + assert %{ + event: "repo_connection_removed", + item_id: ^project_id, + item_type: "project", + actor_id: ^user_id, + changes: %{ + before: %{ + "repo" => ^repo, + "branch" => ^branch + } + } + } = audit + end + test "user without an oauth token can successfully remove a connection" do project = insert(:project) user = insert(:user)