diff --git a/CHANGELOG.md b/CHANGELOG.md index 1efc5e656b..fb61cf885f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,9 @@ and this project adheres to - Creating a new user without a password fails and there is no user feedback [#731](https://github.com/OpenFn/Lightning/issues/731) +- Crash when setting up version control + [#1112](https://github.com/OpenFn/Lightning/issues/1112) + ## [v0.9.2] - 2023-09-20 ### Added diff --git a/lib/lightning/version_control/github_client.ex b/lib/lightning/version_control/github_client.ex index e808449d3a..a6d65cdb76 100644 --- a/lib/lightning/version_control/github_client.ex +++ b/lib/lightning/version_control/github_client.ex @@ -5,132 +5,97 @@ defmodule Lightning.VersionControl.GithubClient do """ use Tesla require Logger + alias Lightning.VersionControl.GithubError alias Lightning.VersionControl.GithubToken plug(Tesla.Middleware.BaseUrl, "https://api.github.com") plug(Tesla.Middleware.JSON) def installation_repos(installation_id) do - with {:ok, installation_client} <- build_client(installation_id), - {:ok, %{status: 200} = repos_resp} <- - installation_client - |> get("/installation/repositories") do - {:ok, - repos_resp.body["repositories"] - |> Enum.map(fn g_repo -> g_repo["full_name"] end)} - else - {:error, :installation_not_found, meta} -> - installation_id_error(meta) - - {:error, :invalid_pem} -> - invalid_pem_error() + with {:ok, client} <- build_client(installation_id), + {:ok, %Tesla.Env{status: 200, body: body}} <- + get(client, "/installation/repositories") do + {:ok, Enum.map(body["repositories"], fn g_repo -> g_repo["full_name"] end)} end end def get_repo_branches(installation_id, repo_name) do - with {:ok, installation_client} <- build_client(installation_id), - {:ok, %{status: 200} = branches} <- - installation_client - |> get("/repos/#{repo_name}/branches") do - branch_names = - branches.body - |> Enum.map(fn b -> b["name"] end) - - {:ok, branch_names} - else - {:error, :installation_not_found, meta} -> - installation_id_error(meta) - - {:error, :invalid_pem} -> - invalid_pem_error() + with {:ok, client} <- build_client(installation_id), + {:ok, %Tesla.Env{status: 200, body: body}} <- + get(client, "/repos/#{repo_name}/branches") do + {:ok, Enum.map(body, fn b -> b["name"] end)} end end def fire_repository_dispatch(installation_id, repo_name, user_email) do - with {:ok, installation_client} <- build_client(installation_id), - {:ok, %{status: 204}} <- - installation_client - |> post("/repos/#{repo_name}/dispatches", %{ + with {:ok, client} <- build_client(installation_id), + {:ok, %Tesla.Env{status: 204}} <- + post(client, "/repos/#{repo_name}/dispatches", %{ event_type: "sync_project", client_payload: %{ message: "#{user_email} initiated a sync from Lightning" } }) do {:ok, :fired} - else - {:error, :installation_not_found, meta} -> - installation_id_error(meta) - - {:error, :invalid_pem} -> - invalid_pem_error() - - err -> - Logger.error(inspect(err)) - {:error, "Error Initiating sync"} end end - def send_sentry_error(msg, meta \\ %{}) do - Sentry.capture_message("Github configuration error", - level: "warning", - extra: meta, - message: msg, - tags: %{type: "github"} - ) - end - - defp installation_id_error(meta) do - send_sentry_error("Github Installation APP ID is misconfigured", meta) - - {:error, - %{ - message: - "Sorry, it seems that the GitHub App ID has not been properly configured for this instance of Lightning. Please contact the instance administrator" - }} - end - - defp invalid_pem_error do - send_sentry_error("Github Cert is misconfigured") - - {:error, - %{ - message: - "Sorry, it seems that the GitHub cert has not been properly configured for this instance of Lightning. Please contact the instance administrator" - }} - end - defp build_client(installation_id) do %{cert: cert, app_id: app_id} = Application.get_env(:lightning, :github_app) |> Map.new() - with {:ok, auth_token, _} <- GithubToken.build(cert, app_id), - client <- + with {:ok, auth_token, _} <- GithubToken.build(cert, app_id) do + client = + Tesla.client([ + {Tesla.Middleware.Headers, + [ + {"Authorization", "Bearer #{auth_token}"} + ]} + ]) + + case post( + client, + "/app/installations/#{installation_id}/access_tokens", + "" + ) do + {:ok, %{status: 201} = installation_token_resp} -> + installation_token = installation_token_resp.body["token"] + + {:ok, Tesla.client([ {Tesla.Middleware.Headers, [ - {"Authorization", "Bearer #{auth_token}"} + {"Authorization", "Bearer " <> installation_token} ]} - ]), - {:ok, installation_token_resp} <- - client - |> post("/app/installations/#{installation_id}/access_tokens", ""), - %{status: 201} <- installation_token_resp do - installation_token = installation_token_resp.body["token"] - - {:ok, - Tesla.client([ - {Tesla.Middleware.Headers, - [ - {"Authorization", "Bearer " <> installation_token} - ]} - ])} - else - %{status: 404} = err -> - {:error, :installation_not_found, err} - - _unused_status -> - {:error, :invalid_pem} + ])} + + {:ok, %{status: 404, body: body}} -> + Logger.error("Unexpected Github Response: #{inspect(body)}") + + error = + GithubError.installation_not_found( + "Github Installation APP ID is misconfigured", + body + ) + + Sentry.capture_exception(error) + + {:error, error} + + {:ok, %{status: 401, body: body}} -> + Logger.error("Unexpected Github Response: #{inspect(body)}") + + error = + GithubError.invalid_certificate( + "Github Certificate is misconfigured", + body + ) + + Sentry.capture_exception(error) + + {:error, error} + end end end end diff --git a/lib/lightning/version_control/github_error.ex b/lib/lightning/version_control/github_error.ex new file mode 100644 index 0000000000..5679645537 --- /dev/null +++ b/lib/lightning/version_control/github_error.ex @@ -0,0 +1,22 @@ +defmodule Lightning.VersionControl.GithubError do + @moduledoc """ + Github Error exception + """ + defexception [:code, :message, :meta] + + def new(code, msg, meta) when is_binary(msg) do + %__MODULE__{code: code, message: msg, meta: Map.new(meta)} + end + + def installation_not_found(msg, meta \\ %{}) do + new(:installation_not_found, msg, meta) + end + + def misconfigured(msg, meta \\ %{}) do + new(:misconfigured, msg, meta) + end + + def invalid_certificate(msg, meta \\ %{}) do + new(:invalid_certificate, msg, meta) + end +end diff --git a/lib/lightning/version_control/project_repo_connection.ex b/lib/lightning/version_control/project_repo_connection.ex index b475d8ae35..4da480eb19 100644 --- a/lib/lightning/version_control/project_repo_connection.ex +++ b/lib/lightning/version_control/project_repo_connection.ex @@ -37,5 +37,8 @@ defmodule Lightning.VersionControl.ProjectRepoConnection do project_repo_connection |> cast(attrs, @fields ++ @required_fields) |> validate_required(@required_fields) + |> unique_constraint(:project_id, + message: "project already has a repo connection" + ) end end diff --git a/lib/lightning/version_control/version_control.ex b/lib/lightning/version_control/version_control.ex index 8dc7c5e770..d523c00205 100644 --- a/lib/lightning/version_control/version_control.ex +++ b/lib/lightning/version_control/version_control.ex @@ -36,9 +36,20 @@ defmodule Lightning.VersionControl do ) end + @spec get_pending_user_installation(Ecto.UUID.t()) :: + ProjectRepoConnection.t() | nil + def get_pending_user_installation(user_id) do + query = + from(prc in ProjectRepoConnection, + where: prc.user_id == ^user_id and is_nil(prc.github_installation_id) + ) + + Repo.one(query) + end + def add_github_installation_id(user_id, installation_id) do pending_installation = - Repo.one( + Repo.one!( from(prc in ProjectRepoConnection, where: prc.user_id == ^user_id and is_nil(prc.github_installation_id) ) @@ -51,7 +62,7 @@ defmodule Lightning.VersionControl do def add_github_repo_and_branch(project_id, repo, branch) do pending_installation = - Repo.one( + Repo.one!( from(prc in ProjectRepoConnection, where: prc.project_id == ^project_id ) diff --git a/lib/lightning_web/controllers/version_control_controller.ex b/lib/lightning_web/controllers/version_control_controller.ex index ad42ef160c..dc3c879f8b 100644 --- a/lib/lightning_web/controllers/version_control_controller.ex +++ b/lib/lightning_web/controllers/version_control_controller.ex @@ -6,16 +6,27 @@ defmodule LightningWeb.VersionControlController do def index(conn, params) do # add installation id to project repo # {:error, %{reason: "Can't find a pending connection."}} + user_id = conn.assigns.current_user.id + pending_connection = VersionControl.get_pending_user_installation(user_id) - {:ok, project_repo_connection} = - VersionControl.add_github_installation_id( - conn.assigns.current_user.id, - params["installation_id"] + if params["setup_action"] == "update" and is_nil(pending_connection) do + conn + |> put_resp_content_type("text/plain") + |> send_resp( + 200, + "Github installation updated successfully; you may close this page or navigate to any OpenFn project which uses this installation: #{params["installation_id"]}" ) + else + {:ok, project_repo_connection} = + VersionControl.add_github_installation_id( + user_id, + params["installation_id"] + ) - # get project repo connection and forward to project settings - redirect(conn, - to: ~p"/projects/#{project_repo_connection.project_id}/settings#vcs" - ) + # get project repo connection and forward to project settings + redirect(conn, + to: ~p"/projects/#{project_repo_connection.project_id}/settings#vcs" + ) + end end end diff --git a/lib/lightning_web/live/project_live/settings.ex b/lib/lightning_web/live/project_live/settings.ex index c1e30c2cda..537bdf42a6 100644 --- a/lib/lightning_web/live/project_live/settings.ex +++ b/lib/lightning_web/live/project_live/settings.ex @@ -2,8 +2,9 @@ defmodule LightningWeb.ProjectLive.Settings do @moduledoc """ Index Liveview for Runs """ - alias Lightning.VersionControl.GithubClient + use LightningWeb, :live_view + require Logger alias Lightning.VersionControl alias Lightning.Policies.ProjectUsers @@ -14,6 +15,7 @@ defmodule LightningWeb.ProjectLive.Settings do alias LightningWeb.Components.Form alias LightningWeb.ProjectLive.DeleteConnectionModal + alias Lightning.VersionControl.GithubError on_mount {LightningWeb.Hooks, :project_scope} @@ -52,7 +54,7 @@ defmodule LightningWeb.ProjectLive.Settings do project_repo_connection} = repo_settings(socket) - if show_repo_setup do + if show_repo_setup and connected?(socket) do collect_project_repo_connections(socket.assigns.project.id) end @@ -251,9 +253,14 @@ defmodule LightningWeb.ProjectLive.Settings do case Application.get_env(:lightning, :github_app) |> Map.new() do %{app_name: nil} -> + Logger.error("Github App Name not configured") # Send to sentry and show cozy error + error = + GithubError.misconfigured("Github App Name Misconfigured", %{ + app_name: nil + }) - GithubClient.send_sentry_error("Github App Name Misconfigured") + Sentry.capture_exception(error) {:noreply, socket @@ -270,7 +277,9 @@ defmodule LightningWeb.ProjectLive.Settings do }) {:noreply, - redirect(socket, external: "https://github.com/apps/#{app_name}")} + redirect(socket, + external: "https://github.com/apps/#{app_name}" + )} end end @@ -280,7 +289,12 @@ defmodule LightningWeb.ProjectLive.Settings do case Application.get_env(:lightning, :github_app) |> Map.new() do %{app_name: nil} -> - GithubClient.send_sentry_error("Github App Name Misconfigured") + error = + GithubError.misconfigured("Github App Name Misconfigured", %{ + app_name: nil + }) + + Sentry.capture_exception(error) {:noreply, socket @@ -383,8 +397,8 @@ defmodule LightningWeb.ProjectLive.Settings do @impl true def handle_info({:branches_fetched, branches_result}, socket) do case branches_result do - {:error, %{message: message}} -> - {:noreply, socket |> put_flash(:error, message)} + {:error, error} -> + {:noreply, socket |> put_flash(:error, error_message(error))} branches -> {:noreply, socket |> assign(loading_branches: false, branches: branches)} @@ -393,8 +407,8 @@ defmodule LightningWeb.ProjectLive.Settings do def handle_info({:repos_fetched, result}, socket) do case result do - {:error, %{message: message}} -> - {:noreply, socket |> put_flash(:error, message)} + {:error, error} -> + {:noreply, socket |> put_flash(:error, error_message(error))} {:ok, [_ | _] = repos} -> {:noreply, socket |> assign(repos: repos)} @@ -406,6 +420,16 @@ defmodule LightningWeb.ProjectLive.Settings do end end + defp error_message(error) do + case error do + %{code: :installation_not_found} -> + "Sorry, it seems that the GitHub App ID has not been properly configured for this instance of Lightning. Please contact the instance administrator" + + %{code: :invalid_certificate} -> + "Sorry, it seems that the GitHub cert has not been properly configured for this instance of Lightning. Please contact the instance administrator" + end + end + defp dispatch_flash(change_result, socket) do case change_result do {:ok, %ProjectUser{}} -> diff --git a/lib/lightning_web/live/project_live/settings.html.heex b/lib/lightning_web/live/project_live/settings.html.heex index 9c81b437aa..f2ddb2c139 100644 --- a/lib/lightning_web/live/project_live/settings.html.heex +++ b/lib/lightning_web/live/project_live/settings.html.heex @@ -347,7 +347,7 @@ <%= if @github_enabled do %> -
+
Install Github App to get started diff --git a/priv/repo/migrations/20230926034840_add_unique_constraint_to_project_repo_connections.exs b/priv/repo/migrations/20230926034840_add_unique_constraint_to_project_repo_connections.exs new file mode 100644 index 0000000000..212d8daf9c --- /dev/null +++ b/priv/repo/migrations/20230926034840_add_unique_constraint_to_project_repo_connections.exs @@ -0,0 +1,7 @@ +defmodule Lightning.Repo.Migrations.AddUniqueConstraintToProjectRepoConnections do + use Ecto.Migration + + def change do + create unique_index("project_repo_connections", [:project_id]) + end +end diff --git a/test/lightning/version_control/github_client_test.exs b/test/lightning/version_control/github_client_test.exs index 4814bf0d4d..3659a72206 100644 --- a/test/lightning/version_control/github_client_test.exs +++ b/test/lightning/version_control/github_client_test.exs @@ -21,16 +21,16 @@ defmodule Lightning.VersionControl.GithubClientTest do Tesla.Mock.mock(fn env -> case env.url do "https://api.github.com/app/installations/some-id/access_tokens" -> - %Tesla.Env{status: 400} + %Tesla.Env{status: 401, body: %{}} "https://api.github.com/app/installations/fail-id/access_tokens" -> - %Tesla.Env{status: 404} + %Tesla.Env{status: 404, body: %{}} "https://api.github.com/installation/repositories" -> - %Tesla.Env{status: 404} + %Tesla.Env{status: 404, body: %{}} "https://api.github.com/repos/some/repo/branches" -> - %Tesla.Env{status: 400} + %Tesla.Env{status: 401, body: %{}} end end) end @@ -40,8 +40,8 @@ defmodule Lightning.VersionControl.GithubClientTest do assert {:error, %{ - message: - "Sorry, it seems that the GitHub cert has not been properly configured for this instance of Lightning. Please contact the instance administrator" + code: :invalid_certificate, + message: "Github Certificate is misconfigured" }} = VersionControl.fetch_installation_repos(p_repo.project_id) end @@ -52,8 +52,8 @@ defmodule Lightning.VersionControl.GithubClientTest do assert {:error, %{ - message: - "Sorry, it seems that the GitHub App ID has not been properly configured for this instance of Lightning. Please contact the instance administrator" + code: :installation_not_found, + message: "Github Installation APP ID is misconfigured" }} = VersionControl.run_sync(p_repo.project_id, "some-user-name") end @@ -63,8 +63,8 @@ defmodule Lightning.VersionControl.GithubClientTest do assert {:error, %{ - message: - "Sorry, it seems that the GitHub cert has not been properly configured for this instance of Lightning. Please contact the instance administrator" + code: :invalid_certificate, + message: "Github Certificate is misconfigured" }} = VersionControl.fetch_repo_branches(p_repo.project_id, p_repo.repo) end @@ -74,8 +74,8 @@ defmodule Lightning.VersionControl.GithubClientTest do assert {:error, %{ - message: - "Sorry, it seems that the GitHub cert has not been properly configured for this instance of Lightning. Please contact the instance administrator" + code: :invalid_certificate, + message: "Github Certificate is misconfigured" }} = VersionControl.fetch_installation_repos(p_repo.project_id) end @@ -104,7 +104,7 @@ defmodule Lightning.VersionControl.GithubClientTest do %Tesla.Env{status: 200, body: [%{"name" => "master"}]} "https://api.github.com/repos/some/repo/dispatches" -> - %Tesla.Env{status: 204} + %Tesla.Env{status: 204, body: %{}} end end) end diff --git a/test/lightning/version_control_test.exs b/test/lightning/version_control_test.exs index ec484b6050..4ee4c95c86 100644 --- a/test/lightning/version_control_test.exs +++ b/test/lightning/version_control_test.exs @@ -67,32 +67,50 @@ defmodule Lightning.VersionControlTest do assert updated_connection.repo == "some_repo" end - test "given a user_id and a installation_id it should update the correct record" do - project = insert(:project) - [u1, u2] = insert_pair(:user) - - attrs1 = %{project_id: project.id, user_id: u1.id} - - attrs2 = %{project_id: project.id, user_id: u2.id} - - assert Repo.aggregate(ProjectRepoConnection, :count) == 0 - - Enum.each([attrs1, attrs2], &VersionControl.create_github_connection/1) - - assert Repo.aggregate(ProjectRepoConnection, :count) == 2 - - assert {:ok, updated_connection} = - VersionControl.add_github_installation_id( - u1.id, - "some_installation" - ) + test "add_github_installation_id/2 updates the installation_id for the correct project for the given user" do + project1 = insert(:project) + project2 = insert(:project) + user = insert(:user) - assert updated_connection.user_id == u1.id + {:ok, _connection1} = + VersionControl.create_github_connection(%{ + project_id: project1.id, + user_id: user.id, + github_installation_id: "some-id" + }) + + {:ok, connection2} = + VersionControl.create_github_connection(%{ + project_id: project2.id, + user_id: user.id + }) + + {:ok, updated_connection} = + VersionControl.add_github_installation_id( + user.id, + "some_installation" + ) + + assert updated_connection.id == connection2.id + end - not_updated = Repo.get_by(ProjectRepoConnection, user_id: u2.id) + test "add_github_installation_id/2 raises when you there's no pending installation" do + project1 = insert(:project) + user = insert(:user) - refute not_updated.id == updated_connection.id - refute not_updated.github_installation_id + {:ok, _connection1} = + VersionControl.create_github_connection(%{ + project_id: project1.id, + user_id: user.id, + github_installation_id: "some-id" + }) + + assert_raise Ecto.NoResultsError, fn -> + VersionControl.add_github_installation_id( + user.id, + "some_installation" + ) + end end end end diff --git a/test/lightning_web/controllers/version_control_controller_test.exs b/test/lightning_web/controllers/version_control_controller_test.exs index 8ca943e6a8..03215eccc0 100644 --- a/test/lightning_web/controllers/version_control_controller_test.exs +++ b/test/lightning_web/controllers/version_control_controller_test.exs @@ -32,6 +32,32 @@ defmodule LightningWeb.VersionControlControllerTest do assert redirected_to(response) == ~p"/projects/#{p_repo.project_id}/settings#vcs" end + + test "responds with a text when the setup_action is set to update and there's no pending installation", + %{ + conn: conn, + project: project, + user: user + } do + installation_id = "my_id" + + insert(:project_repo_connection, %{ + github_installation_id: installation_id, + branch: nil, + repo: nil, + user: user, + project: project + }) + + conn = + get( + conn, + ~p"/setup_vcs?installation_id=#{installation_id}&setup_action=update" + ) + + assert text_response(conn, 200) == + "Github installation updated successfully; you may close this page or navigate to any OpenFn project which uses this installation: #{installation_id}" + end end describe "when not logged in" do diff --git a/test/lightning_web/live/project_live_test.exs b/test/lightning_web/live/project_live_test.exs index a9a9a273a6..3075a5eaa4 100644 --- a/test/lightning_web/live/project_live_test.exs +++ b/test/lightning_web/live/project_live_test.exs @@ -399,10 +399,10 @@ defmodule LightningWeb.ProjectLiveTest do Tesla.Mock.mock_global(fn env -> case env.url do "https://api.github.com/app/installations/bad-id/access_tokens" -> - %Tesla.Env{status: 404} + %Tesla.Env{status: 404, body: %{}} "https://api.github.com/app/installations/wrong-cert/access_tokens" -> - %Tesla.Env{status: 200} + %Tesla.Env{status: 401, body: %{}} "https://api.github.com/app/installations/some-id/access_tokens" -> %Tesla.Env{status: 201, body: %{"token" => "some-token"}} @@ -483,7 +483,6 @@ defmodule LightningWeb.ProjectLiveTest do end @tag role: :admin - @tag :skip test "Flashes an error when APP ID is wrong", %{ conn: conn, project: project, @@ -509,12 +508,13 @@ defmodule LightningWeb.ProjectLiveTest do ~p"/projects/#{project.id}/settings#vcs" ) + Process.sleep(15) + assert render(view) =~ "Sorry, it seems that the GitHub App ID has not been properly configured for this instance of Lightning. Please contact the instance administrator" end @tag role: :admin - @tag :skip test "Flashes an error when PEM CERT is corrupt", %{ conn: conn, project: project, @@ -540,6 +540,8 @@ defmodule LightningWeb.ProjectLiveTest do ~p"/projects/#{project.id}/settings#vcs" ) + Process.sleep(10) + assert render(view) =~ "Sorry, it seems that the GitHub cert has not been properly configured for this instance of Lightning. Please contact the instance administrator" end @@ -594,6 +596,27 @@ defmodule LightningWeb.ProjectLiveTest do assert view |> render_click("install_app", %{}) end + @tag role: :admin + test "Flashes an error when APP Name is missing during installation", %{ + conn: conn, + project: project + } do + put_temporary_env(:lightning, :github_app, + cert: @cert, + app_id: nil, + app_name: nil + ) + + {:ok, view, _html} = + live( + conn, + ~p"/projects/#{project.id}/settings#vcs" + ) + + assert view |> render_click("install_app", %{}) =~ + "Sorry, it seems that the GitHub App Name has not been properly configured for this instance of Lighting. Please contact the instance administrator" + end + @tag role: :admin test "can reinstall github app", %{ conn: conn, @@ -616,6 +639,29 @@ defmodule LightningWeb.ProjectLiveTest do assert view |> render_click("reinstall_app", %{}) end + @tag role: :admin + test "Flashes an error when APP Name is missing during reinstallation", %{ + conn: conn, + project: project + } do + put_temporary_env(:lightning, :github_app, + cert: @cert, + app_id: nil, + app_name: nil + ) + + insert(:project_repo_connection, %{project_id: project.id, project: nil}) + + {:ok, view, _html} = + live( + conn, + ~p"/projects/#{project.id}/settings#vcs" + ) + + assert view |> render_click("reinstall_app", %{}) =~ + "Sorry, it seems that the GitHub App Name has not been properly configured for this instance of Lighting. Please contact the instance administrator" + end + @tag role: :admin test "can delete github repo connection", %{ conn: conn,