Accounts: require proof of the second factor to finish a 2FA login

92ad654 · Gabriel Morell · 2026-10-18 18:49

28 files +741 -87
Message
{commit_body(@commit)}

Files changed

modified lib/git_gud/accounts.ex
+59 −1
@@ -265,7 +265,65 @@ defmodule GitGud.Accounts do
265 265
266 266 def get_active_invite(_), do: nil
267 267
268 − @doc "Mark an invite as consumed by the given user."
268 + @doc """
269 + Register a user on an invite, spending it.
270 +
271 + The invite is claimed first with a conditional update (`consumed_at
272 + IS NULL`, not expired), so of two registrations racing on — or
273 + replaying — the same invite only one gets past it; the other gets
274 + `{:error, :invite_used}` and no account. If the registration itself
275 + then fails the claim is released so the invite can be used again.
276 +
277 + Not one transaction: `register_user/1` retries handle collisions on
278 + the unique-constraint error, which would abort an enclosing one.
279 +
280 + With a `nil` invite this is plain `register_user/1`.
281 + """
282 + def register_user_with_invite(attrs, nil), do: register_user(attrs)
283 +
284 + def register_user_with_invite(attrs, %Invite{id: id}) do
285 + with {:ok, claimed_at} <- claim_invite(id) do
286 + case register_user(attrs) do
287 + {:ok, user} ->
288 + Repo.update_all(from(i in Invite, where: i.id == ^id), set: [consumed_by_id: user.id])
289 + {:ok, user}
290 +
291 + error ->
292 + Repo.update_all(
293 + from(i in Invite,
294 + where: i.id == ^id and i.consumed_at == ^claimed_at and is_nil(i.consumed_by_id)
295 + ),
296 + set: [consumed_at: nil]
297 + )
298 +
299 + error
300 + end
301 + end
302 + end
303 +
304 + defp claim_invite(id) do
305 + now = DateTime.utc_now(:second)
306 +
307 + {claimed, _} =
308 + Repo.update_all(
309 + from(i in Invite,
310 + where:
311 + i.id == ^id and is_nil(i.consumed_at) and
312 + (is_nil(i.expires_at) or i.expires_at > ^now)
313 + ),
314 + set: [consumed_at: now]
315 + )
316 +
317 + if claimed == 1, do: {:ok, now}, else: {:error, :invite_used}
318 + end
319 +
320 + @doc """
321 + Mark an invite as consumed by the given user.
322 +
323 + Not safe against reuse when `invite` was loaded earlier: it trusts the
324 + struct's `consumed_at`. Registration goes through
325 + `register_user_with_invite/2` instead.
326 + """
269 327 def consume_invite(%Invite{} = invite, %User{} = user) do
270 328 invite
271 329 |> Invite.consume_changeset(user)
modified lib/git_gud/accounts/two_factor.ex
+9 −2
@@ -224,8 +224,15 @@ defmodule GitGud.Accounts.TwoFactor do
224 224 {:error, :invalid_code}
225 225
226 226 row ->
227 − {:ok, _} = row |> RecoveryCode.mark_used() |> Repo.update()
228 − :ok
227 + # Claim the row atomically: two concurrent submissions of the
228 + # same code both match above, but only one may spend it.
229 + case Repo.update_all(
230 + from(r in RecoveryCode, where: r.id == ^row.id and is_nil(r.used_at)),
231 + set: [used_at: DateTime.utc_now(:second)]
232 + ) do
233 + {1, _} -> :ok
234 + {0, _} -> {:error, :invalid_code}
235 + end
229 236 end
230 237 end
231 238
added lib/git_gud/accounts/two_factor_attempts.ex
+98 −0
@@ -0,0 +1,98 @@
1 +defmodule GitGud.Accounts.TwoFactorAttempts do
2 + @moduledoc """
3 + Counts failed second-factor codes (TOTP and recovery codes) so they
4 + can't be guessed without bound.
5 +
6 + Two counters, both bumped on every failure:
7 +
8 + * per pending login (keyed by the nonce `UserAuth.log_in_user/3`
9 + puts in the session): five failures and that login is dead, the
10 + user has to sign in with their password again;
11 + * per user, across logins: twenty failures an hour, so re-entering a
12 + known password doesn't buy fresh guesses forever.
13 +
14 + Backed by a public ETS table owned by this process, swept once a
15 + minute. Per node: in a cluster the per-user cap is multiplied by the
16 + number of nodes, which still keeps a 6-digit code out of reach.
17 + """
18 +
19 + use GenServer
20 +
21 + @table :gitgud_two_factor_attempts
22 + @sweep_interval_ms 60_000
23 +
24 + @per_login 5
25 + @login_ttl_ms :timer.minutes(30)
26 + @per_user 20
27 + @user_ttl_ms :timer.hours(1)
28 +
29 + def start_link(opts), do: GenServer.start_link(__MODULE__, opts, name: __MODULE__)
30 +
31 + @impl true
32 + def init(_opts) do
33 + :ets.new(@table, [:public, :named_table, :set, write_concurrency: true])
34 + schedule_sweep()
35 + {:ok, %{}}
36 + end
37 +
38 + @impl true
39 + def handle_info(:sweep, state) do
40 + now = now()
41 + :ets.select_delete(@table, [{{:_, :_, :"$1"}, [{:<, :"$1", now}], [true]}])
42 + schedule_sweep()
43 + {:noreply, state}
44 + end
45 +
46 + defp schedule_sweep, do: Process.send_after(self(), :sweep, @sweep_interval_ms)
47 +
48 + @doc "Failures allowed per pending login before it's abandoned."
49 + def per_login_limit, do: @per_login
50 +
51 + @doc "Failures allowed per user per hour."
52 + def per_user_limit, do: @per_user
53 +
54 + @doc """
55 + True when no more codes may be tried for `user_id` — either this
56 + pending login (`login_nonce`, nil for none) or the user as a whole
57 + has used up its failures.
58 + """
59 + def locked?(user_id, login_nonce) do
60 + (login_nonce != nil and count({:login, login_nonce}) >= @per_login) or
61 + count({:user, user_id}) >= @per_user
62 + end
63 +
64 + @doc "Record a failed code. Returns `:locked` once the limit is reached."
65 + def record_failure(user_id, login_nonce) do
66 + if login_nonce != nil, do: bump({:login, login_nonce}, @login_ttl_ms)
67 + bump({:user, user_id}, @user_ttl_ms)
68 +
69 + if locked?(user_id, login_nonce), do: :locked, else: :ok
70 + end
71 +
72 + @doc "Forget the failures after a successful factor."
73 + def clear(user_id, login_nonce) do
74 + if login_nonce != nil, do: :ets.delete(@table, {:login, login_nonce})
75 + :ets.delete(@table, {:user, user_id})
76 + :ok
77 + end
78 +
79 + defp count(key) do
80 + now = now()
81 +
82 + case :ets.lookup(@table, key) do
83 + [{^key, n, expires}] when expires >= now -> n
84 + _ -> 0
85 + end
86 + end
87 +
88 + defp bump(key, ttl) do
89 + now = now()
90 +
91 + case :ets.lookup(@table, key) do
92 + [{^key, _, expires}] when expires < now -> :ets.insert(@table, {key, 1, now + ttl})
93 + _ -> :ets.update_counter(@table, key, {2, 1}, {key, 0, now + ttl})
94 + end
95 + end
96 +
97 + defp now, do: System.monotonic_time(:millisecond)
98 +end
modified lib/git_gud/application.ex
+2 −0
@@ -33,6 +33,8 @@ defmodule GitGud.Application do
33 33 GitGud.Cache,
34 34 {Oban, Application.fetch_env!(:git_gud, Oban)},
35 35 GitGud.Federation.RateLimiter,
36 + # Failed TOTP / recovery-code counters for the 2FA challenge.
37 + GitGud.Accounts.TwoFactorAttempts,
36 38 # Holds (and renews) the OpenBao token the F-Droid signer
37 39 # uses. Idle unless OpenBao is configured.
38 40 GitGud.Packages.Signing.OpenBao.Session,
modified lib/git_gud_web/controllers/user_session_controller.ex
+28 −10
@@ -3,6 +3,7 @@ defmodule GitGudWeb.UserSessionController do
3 3
4 4 alias GitGud.Accounts
5 5 alias GitGud.Accounts.TwoFactor
6 + alias GitGud.Accounts.TwoFactorAttempts
6 7 alias GitGudWeb.UserAuth
7 8
8 9 def create(conn, %{"_action" => "confirmed"} = params) do
@@ -74,16 +75,33 @@ defmodule GitGudWeb.UserSessionController do
74 75 user = conn.assigns.current_scope.user
75 76 code = String.replace(code, ~r/\s+/, "")
76 77
77 − if Accounts.second_factor_reauth_available?(user) and
78 − TwoFactor.verify_code(user, code) == :ok do
79 − conn
80 − |> put_flash(:info, "Welcome back!")
81 − |> UserAuth.reauthenticate(user)
82 − |> redirect(to: reauth_return_to(conn, params))
83 − else
84 − conn
85 − |> put_flash(:error, "That code didn't match.")
86 − |> redirect(to: ~p"/users/log-in")
78 + cond do
79 + not Accounts.second_factor_reauth_available?(user) ->
80 + conn
81 + |> put_flash(:error, "That code didn't match.")
82 + |> redirect(to: ~p"/users/log-in")
83 +
84 + # Shares the per-user failure budget with the login challenge, so
85 + # a stolen session can't guess its way into sudo.
86 + TwoFactorAttempts.locked?(user.id, nil) ->
87 + conn
88 + |> put_flash(:error, "Too many incorrect codes. Try again later.")
89 + |> redirect(to: ~p"/users/log-in")
90 +
91 + TwoFactor.verify_code(user, code) == :ok ->
92 + TwoFactorAttempts.clear(user.id, nil)
93 +
94 + conn
95 + |> put_flash(:info, "Welcome back!")
96 + |> UserAuth.reauthenticate(user)
97 + |> redirect(to: reauth_return_to(conn, params))
98 +
99 + true ->
100 + TwoFactorAttempts.record_failure(user.id, nil)
101 +
102 + conn
103 + |> put_flash(:error, "That code didn't match.")
104 + |> redirect(to: ~p"/users/log-in")
87 105 end
88 106 end
89 107
modified lib/git_gud_web/controllers/webauthn_controller.ex
+26 −6

Click to load diff…

modified lib/git_gud_web/live/personal_access_token_live/index.ex
+10 −2

Click to load diff…

modified lib/git_gud_web/live/user_live/registration.ex
+11 −7

Click to load diff…

modified lib/git_gud_web/live/user_live/two_factor_challenge.ex
+82 −49

Click to load diff…

modified lib/git_gud_web/live/user_live/two_factor_setup.ex
+17 −7

Click to load diff…

modified lib/git_gud_web/router.ex
+3 −2

Click to load diff…

modified lib/git_gud_web/user_auth.ex
+84 −1

Click to load diff…

modified test/git_gud_web/live/settings_sudo_test.exs
+57 −0

Click to load diff…

modified test/git_gud_web/live/user_live/registration_test.exs
+32 −0

Click to load diff…

modified test/git_gud_web/second_factor_reauth_test.exs
+17 −0

Click to load diff…

added test/git_gud_web/two_factor_login_test.exs
+206 −0

Click to load diff…

Parents: a6f4469