From ac77ced311e310f71bc0b95b09f75aa7a10706eb Mon Sep 17 00:00:00 2001 From: "Adam C. Stephens" Date: Mon, 31 Aug 2026 14:44:30 -0400 Subject: [PATCH] fix: only start the oidcc worker for an issuer that could answer Refs: GS-36 Assisted-By: Claude Opus 5 --- lib/git_sync/forgejo/provider.ex | 18 ++++++++- .../controllers/session_controller.ex | 2 +- .../controllers/session_html/new.html.heex | 4 +- test/git_sync/forgejo/provider_test.exs | 38 +++++++++++++++++++ .../controllers/session_controller_test.exs | 13 +++++++ 5 files changed, 71 insertions(+), 4 deletions(-) create mode 100644 test/git_sync/forgejo/provider_test.exs diff --git a/lib/git_sync/forgejo/provider.ex b/lib/git_sync/forgejo/provider.ex index d5dd8c7..d474a9e 100644 --- a/lib/git_sync/forgejo/provider.ex +++ b/lib/git_sync/forgejo/provider.ex @@ -6,6 +6,11 @@ defmodule GitSync.Forgejo.Provider do started dynamically rather than listed in the application's children. It backs off and retries rather than terminating, so a Forgejo instance that is briefly unreachable does not leave the app with no way to log in. + + That patience is only worth spending on an issuer that could answer. A + development instance points `base_url` at the bare repositories it clones + from, and nothing is discoverable over `file://`, so a worker started on one + would retry until the instance is shut down. """ alias GitSync.Connection @@ -19,6 +24,14 @@ defmodule GitSync.Forgejo.Provider do def name, do: @worker + @doc """ + Whether a connection's Forgejo could be an OIDC issuer. + """ + def issuer?(%Connection{base_url: base_url}), + do: URI.parse(base_url).scheme in ["http", "https"] + + def issuer?(nil), do: false + @doc """ Starts the worker for `issuer`, replacing one pointing at a different issuer. """ @@ -42,8 +55,9 @@ defmodule GitSync.Forgejo.Provider do """ def start_configured do case Connections.forgejo() do - %Connection{client_id: client_id, base_url: base_url} when is_binary(client_id) -> - ensure_started(base_url) + %Connection{client_id: client_id, base_url: base_url} = connection + when is_binary(client_id) -> + if issuer?(connection), do: ensure_started(base_url), else: :ignore _ -> :ignore diff --git a/lib/git_sync_web/controllers/session_controller.ex b/lib/git_sync_web/controllers/session_controller.ex index 561eac9..55cc853 100644 --- a/lib/git_sync_web/controllers/session_controller.ex +++ b/lib/git_sync_web/controllers/session_controller.ex @@ -20,7 +20,7 @@ defmodule GitSyncWeb.SessionController do plug Oidcc.Plug.AuthorizationCallback, @client when action == :callback def new(conn, _params) do - render(conn, :new) + render(conn, :new, forgejo_issuer?: Provider.issuer?(Connections.forgejo())) end def create(conn, _params) do diff --git a/lib/git_sync_web/controllers/session_html/new.html.heex b/lib/git_sync_web/controllers/session_html/new.html.heex index 211b740..d993a38 100644 --- a/lib/git_sync_web/controllers/session_html/new.html.heex +++ b/lib/git_sync_web/controllers/session_html/new.html.heex @@ -4,7 +4,9 @@ <:subtitle>git-sync is gated by your Forgejo instance. - <.button id="sign-in" href={~p"/auth/forgejo"} method="post">Sign in with Forgejo + <.button :if={@forgejo_issuer?} id="sign-in" href={~p"/auth/forgejo"} method="post"> + Sign in with Forgejo + <.dev_sign_in /> diff --git a/test/git_sync/forgejo/provider_test.exs b/test/git_sync/forgejo/provider_test.exs new file mode 100644 index 0000000..a28b160 --- /dev/null +++ b/test/git_sync/forgejo/provider_test.exs @@ -0,0 +1,38 @@ +defmodule GitSync.Forgejo.ProviderTest do + use GitSync.DataCase + + alias GitSync.Connection + alias GitSync.Forgejo.Provider + + describe "issuer?/1" do + test "an instance reached over HTTP can be one" do + assert Provider.issuer?(%Connection{base_url: "https://forge.test"}) + end + + test "the repositories a development instance clones from cannot" do + refute Provider.issuer?(%Connection{base_url: "file:///srv/dev_repos/forgejo"}) + end + + test "an instance nobody has configured cannot" do + refute Provider.issuer?(nil) + end + end + + describe "start_configured/0" do + test "leaves the worker down for an instance that cannot be an issuer" do + Repo.insert!(%Connection{ + kind: :forgejo, + base_url: "file:///srv/dev_repos/forgejo", + client_id: "dev", + client_secret: "dev" + }) + + assert Provider.start_configured() == :ignore + refute Process.whereis(Provider.name()) + end + + test "leaves it down until the wizard has run" do + assert Provider.start_configured() == :ignore + end + end +end diff --git a/test/git_sync_web/controllers/session_controller_test.exs b/test/git_sync_web/controllers/session_controller_test.exs index 393ac32..c416db9 100644 --- a/test/git_sync_web/controllers/session_controller_test.exs +++ b/test/git_sync_web/controllers/session_controller_test.exs @@ -13,6 +13,19 @@ defmodule GitSyncWeb.SessionControllerTest do assert html_response(conn, 200) =~ "Sign in with Forgejo" end + test "the page does not offer a Forgejo that cannot be an issuer", %{ + conn: conn, + connection: connection + } do + connection + |> Ecto.Changeset.change(base_url: "file:///srv/dev_repos/forgejo") + |> GitSync.Repo.update!() + + conn = get(conn, ~p"/login") + + refute html_response(conn, 200) =~ "Sign in with Forgejo" + end + describe "with a live provider" do setup %{connection: connection} do issuer = OidcProvider.start(subject: "alice", client_id: connection.client_id) -- 2.51.2