Feat/email password auth#31
Merged
Merged
Conversation
Add bcrypt_elixir for password hashing. Create migration to add hashed_password and confirmed_at fields to users table, and create users_tokens table for session/email token management. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- UserToken: session and email token generation with SHA256 hashing - User: registration_changeset, password_changeset, valid_password? - UserNotifier: confirmation and password reset emails via Swoosh - Accounts: register_user, session tokens, confirmation, password reset, get_or_create_google_user (with email-linking) - UserAuth: log_in_user/log_out_user session helpers - extract_user_token test helper in DataCase Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Switch all authentication from raw user_id in session to token-based session management. This enables token revocation, expiry, and is more secure. - RequireAuth plug reads user_token, looks up via session token - LiveAuth reads user_token, adds redirect_if_authenticated and mount_current_user on_mount callbacks - AuthController uses get_or_create_google_user and UserAuth.log_in_user - CompanySwitchController uses conn.assigns.current_user - Layouts logout link points to /users/log-out - All test files migrated to log_in_user helper - ConnCase.log_in_user/3 helper with optional extra session params Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add registration, login, forgot password, reset password, confirmation, and confirmation instructions LiveViews. Add UserSessionController for email/password login/logout. Add simple_form and public error components to CoreComponents. Add password_user factory and tests (22 new tests). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove allowed_email?/1, allowed_emails/0, parse_and_cache_allowed_emails/0, clear_allowed_emails_cache/0, and find_or_create_user/1 from Accounts context. Remove ALLOWED_EMAILS config from runtime.exs and test.exs. Update all tests to use get_or_create_google_user/1. Update landing page with login/register links instead of Google-only sign-in. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add mass-assignment security test for registration_changeset (Rule 2) - Add data-testid attributes to all auth LiveView templates (Rule 5) - Update LiveView tests to use has_element? with data-testid (Rule 4) - Add @SPEC to simple_form/1 and error/1 in CoreComponents (Rule 7) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces the Google allowlist OAuth flow with full email/password auth and token-based sessions: adds bcrypt hashing, user confirmation and password-reset tokens, UserToken/UserNotifier/UserAuth modules, new LiveViews/controllers/routes, DB migration, tests, test helpers, and removes ALLOWED_EMAILS configuration. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser as Web Browser
participant LiveView as Phoenix LiveView
participant Accounts as Accounts Context
participant DB as Database
participant Mailer as Email Mailer
User->>Browser: Open /users/register
Browser->>LiveView: mount registration form
LiveView->>Browser: render form
User->>LiveView: submit email & password
LiveView->>Accounts: register_user(attrs)
Accounts->>Accounts: validate & bcrypt hash password
Accounts->>DB: insert user (hashed_password, confirmed_at nil)
Accounts->>Accounts: build confirmation token
Accounts->>Mailer: deliver_user_confirmation_instructions(user, url)
Mailer-->>User: email sent
User->>Browser: click email link
Browser->>LiveView: mount /users/confirm/:token
LiveView->>Accounts: confirm_user(token)
Accounts->>DB: verify token & set confirmed_at
DB-->>Accounts: confirmation saved
LiveView->>Browser: show success
sequenceDiagram
participant User
participant Browser as Web Browser
participant LiveView as Phoenix LiveView
participant Controller as UserSessionController
participant Accounts as Accounts Context
participant DB as Database
User->>Browser: Open /users/log-in
Browser->>LiveView: mount login form
User->>LiveView: submit credentials (phx-trigger-action)
Browser->>Controller: POST /users/log-in
Controller->>Accounts: get_user_by_email_and_password(email, pass)
Accounts->>DB: fetch user by email
Accounts->>Accounts: verify password (bcrypt)
alt valid
Accounts-->>Controller: user
Controller->>Accounts: generate_user_session_token(user)
Accounts->>DB: insert session token
Controller->>Browser: put session user_token & redirect /dashboard
else invalid
Controller->>Browser: flash error & redirect /users/log-in
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/ksef_hub_web/live/company_live_test.exs (1)
40-46:⚠️ Potential issue | 🟡 MinorAdd data-testid attributes to template and use
has_element?/2for assertions. The current raw HTML assertions should be replaced with proper element selectors. First, add adata-testid="company-name"attribute to the company name cell in the template (line 152 oflib/ksef_hub_web/live/company_live/index.ex), then update the test assertions:{:ok, view, _html} = conn |> log_in_user(user, %{current_company_id: company.id}) |> live("/companies") - assert html =~ "My Visible Co" - refute html =~ "Someone Else Co" + assert has_element?(view, "[data-testid='company-name']", "My Visible Co") + refute has_element?(view, "[data-testid='company-name']", "Someone Else Co")Template change needed in
lib/ksef_hub_web/live/company_live/index.exline 152:- <:col :let={company} label="Name">{company.name}</:col> + <:col :let={company} label="Name"><span data-testid="company-name">{company.name}</span></:col>
🤖 Fix all issues with AI agents
In `@lib/ksef_hub_web/controllers/company_switch_controller.ex`:
- Around line 14-18: The update action currently reads user =
conn.assigns.current_user and then uses user.id which can be nil and crash;
change the with in update to pattern-match the user (e.g., {:ok, %User{} = user}
or user when not is_nil(user)) as the first clause so missing/unauthenticated
current_user is routed to the with's else branch instead of raising; ensure
subsequent clauses still use that matched user
(Companies.get_membership(user.id, uuid)) and add an appropriate else clause
handling the unauthenticated case.
In `@lib/ksef_hub_web/live/user_registration_live.ex`:
- Around line 30-35: The direct pattern match {:ok, _} =
Accounts.deliver_user_confirmation_instructions(...) can crash on delivery
failure; replace it with a safe branch (case or with) around
Accounts.deliver_user_confirmation_instructions in user_registration_live (the
block that builds url(~p"/users/confirm/#{&1}")) and handle {:ok, _} and
{:error, reason} explicitly — on success keep current flow, on error log the
reason and surface a user-friendly message via put_flash (or similar) so the
LiveView does not crash and the user sees a graceful failure response.
In `@lib/ksef_hub_web/user_auth.ex`:
- Around line 47-56: The docstring for signed_in_path/1 claims it redirects to
"/companies/new" when the user has no companies but the implementation always
returns ~p"/dashboard"; update the function signed_in_path(user) to inspect the
User struct (e.g., check user.companies or call Accounts.list_user_companies/1)
and return ~p"/companies/new" when none exist, otherwise ~p"/dashboard", or
alternatively change the `@doc` to state it always returns "/dashboard" so docs
and implementation match—ensure you modify the `@doc` or the function
signed_in_path/1 accordingly and keep the `@spec` signature unchanged.
In `@lib/ksef_hub/accounts.ex`:
- Around line 63-73: Normalize the incoming email in
get_user_by_email_and_password the same way registration does: trim whitespace
and downcase before calling get_user_by_email so lookups match stored normalized
emails; update get_user_by_email_and_password to transform the email (e.g.,
String.trim/1 |> String.downcase/1) prior to fetching the user and then continue
to call User.valid_password?/2 as before (keep function name
get_user_by_email_and_password and the call to get_user_by_email so only the
input is normalized).
In `@lib/ksef_hub/accounts/user_token.ex`:
- Around line 95-113: verify_email_token_query/2 can crash because
days_for_context/1 has no catch-all; add a default clause in days_for_context/1
that returns :error for unknown contexts (e.g., days_for_context(_)-> :error)
and then update verify_email_token_query/2 to pattern-match the days result (or
case on days_for_context(context)) and return :error when it receives :error
instead of calling ago/2 with an invalid value; reference functions
days_for_context/1 and verify_email_token_query/2 when making these changes
(also apply same pattern to the similar code around the other occurrence).
In `@test/support/conn_case.ex`:
- Around line 39-60: Update the `@doc` for the public functions to include
explicit parameter and return types: for defdelegate extract_user_token(fun)
document that fun is a zero-arity function or a function expected to send an
email (e.g., (-> any) or (() -> any)) and state the return type, and for
log_in_user(conn, user, extra_session \\ %{}) update its doc to specify types
for conn (Plug.Conn.t()), user (KsefHub.Accounts.User.t() or the appropriate
struct/type), extra_session (map()), and the return type (Plug.Conn.t() with
session containing :user_token). Keep the prose brief and place the type
annotations inside the `@doc` so readers can see parameter and return types for
extract_user_token and log_in_user.
In `@test/support/data_case.ex`:
- Around line 45-56: Update the public function doc for extract_user_token/1 to
include explicit parameter and return types: document the single parameter as a
function of type (String.t() -> any()) (or more specific if known) that receives
the URL, and document the return as a tuple {String.t(), any()} where the first
element is the encoded token; add a brief "Parameters" and "Returns" line in the
`@doc` for extract_user_token/1 reflecting these types and the example usage.
🧹 Nitpick comments (9)
test/support/data_case.ex (1)
57-62: Return {:ok, result}/{:error, reason} instead of raising on failures.This helper assumes success and will crash if the email function fails or the token markers are missing.
Based on learnings “Use `{:ok, result}` and `{:error, reason}` tuples for operations that can fail”.♻️ Possible refactor
def extract_user_token(fun) do - {:ok, captured} = fun.(&"[TOKEN]#{&1}[/TOKEN]") - %{text_body: body} = captured - [_, token_and_rest | _] = String.split(body, "[TOKEN]") - [token | _] = String.split(token_and_rest, "[/TOKEN]") - {token, captured} + case fun.(&"[TOKEN]#{&1}[/TOKEN]") do + {:ok, %{text_body: body} = captured} -> + with [_, token_and_rest | _] <- String.split(body, "[TOKEN]"), + [token | _] <- String.split(token_and_rest, "[/TOKEN]") do + {:ok, {token, captured}} + else + _ -> {:error, :token_not_found} + end + + {:error, reason} -> + {:error, reason} + end endlib/ksef_hub_web/live/user_login_live.ex (1)
28-32: Minor: Redundant parameter access in "save" handler.The
user_paramsis already destructured fromparams["user"], but line 30 accessesparams["user"]again. This could be simplified.♻️ Suggested simplification
- def handle_event("save", %{"user" => _user_params} = params, socket) do - # The form will POST to the session controller via phx-trigger-action - form = to_form(params["user"], as: "user") + def handle_event("save", %{"user" => user_params}, socket) do + # The form will POST to the session controller via phx-trigger-action + form = to_form(user_params, as: "user") {:noreply, assign(socket, form: form, trigger_submit: true)} endtest/ksef_hub_web/live/user_reset_password_live_test.exs (1)
50-59: Consider using element-based assertion for validation errors.Line 58 asserts against raw HTML content (
result =~ "should be at least 12 character"). Per coding guidelines, prefer testing for element presence rather than raw HTML content which can change. Consider asserting on an error element with a specific selector.♻️ Alternative using element assertion
test "renders errors for invalid password", %{conn: conn, token: token} do {:ok, view, _html} = live(conn, ~p"/users/reset-password/#{token}") - result = - view - |> form("#reset_password_form", user: %{password: "short"}) - |> render_submit() - - assert result =~ "should be at least 12 character" + view + |> form("#reset_password_form", user: %{password: "short"}) + |> render_submit() + + assert has_element?(view, "[data-testid='password-error']") or + has_element?(view, ".field-error") endNote: The exact selector depends on how errors are rendered in your form component.
test/ksef_hub_web/live/user_registration_live_test.exs (1)
50-60: Consider element-based assertion for validation errors.Similar to other tests, line 59 asserts against raw HTML content. Consider using an element selector for the error message if one exists.
test/ksef_hub/accounts/user_test.exs (1)
6-78: Prefer factory-built attrs over inline maps. This keeps tests consistent with the rest of the suite and reduces drift if defaults change.♻️ Possible direction
- attrs = %{email: "user@example.com", password: "valid_password123"} + attrs = params_for(:user, email: "user@example.com", password: "valid_password123")(Apply similarly across other tests.)
As per coding guidelines "Use ExMachina factories for test data instead of inline
valid_attrsmaps".test/ksef_hub_web/live/user_login_live_test.exs (1)
27-44: Consider using factory defaults for password.The test manually hashes the password with
Bcrypt.hash_pwd_salt(password). If the:password_userfactory already handles password hashing, you could simplify this by passing the plaintext password to the factory and letting it hash internally.However, this approach is acceptable if you need explicit control over the password value for testing.
test/ksef_hub_web/controllers/auth_controller_test.exs (1)
135-162: Consider using factory for consistency.The test uses
Accounts.register_user/1directly instead of the:password_userfactory. While this works, using factories provides consistency across tests.♻️ Optional refactor using factory
test "links Google account to existing email-registered user", %{conn: conn} do - # Register a user via email/password first - {:ok, email_user} = - Accounts.register_user(%{email: "link@example.com", password: "valid_password123"}) + email_user = insert(:password_user, email: "link@example.com") auth = %Ueberauth.Auth{ uid: "google-link-uid", info: %Ueberauth.Auth.Info{ email: "link@example.com",lib/ksef_hub/accounts/user_notifier.ex (1)
68-68: Consider making the sender address configurable.The hardcoded
"noreply@ksef-hub.com"may need to vary across environments (development, staging, production). Consider pulling this from application configuration:♻️ Proposed refactor
+ `@from_email` Application.compile_env(:ksef_hub, :mailer_from, {"KSeF Hub", "noreply@ksef-hub.com"}) + `@spec` deliver(String.t(), String.t(), String.t()) :: {:ok, Swoosh.Email.t()} | {:error, term()} defp deliver(recipient, subject, body) do email = new() |> to(recipient) - |> from({"KSeF Hub", "noreply@ksef-hub.com"}) + |> from(`@from_email`) |> subject(subject) |> text_body(body)test/ksef_hub/accounts_test.exs (1)
219-225: Simplify the double-negative assertion.
refute updated.id != user.idis a double negative that can be confusing. Consider using a direct assertion for clarity.♻️ Proposed refactor
assert {:ok, updated} = Accounts.reset_user_password(user, %{password: "new_password12345"}) assert Accounts.get_user_by_email_and_password("reset@example.com", "new_password12345") - refute updated.id != user.id + assert updated.id == user.id
Comment on lines
+30
to
+35
| {:ok, user} -> | ||
| {:ok, _} = | ||
| Accounts.deliver_user_confirmation_instructions( | ||
| user, | ||
| &url(~p"/users/confirm/#{&1}") | ||
| ) |
There was a problem hiding this comment.
Handle email delivery failure gracefully.
The pattern match {:ok, _} = Accounts.deliver_user_confirmation_instructions(...) will crash if email delivery fails. Consider handling the error case to provide a better user experience.
🛡️ Proposed fix
{:ok, user} ->
- {:ok, _} =
- Accounts.deliver_user_confirmation_instructions(
- user,
- &url(~p"/users/confirm/#{&1}")
- )
+ case Accounts.deliver_user_confirmation_instructions(
+ user,
+ &url(~p"/users/confirm/#{&1}")
+ ) do
+ {:ok, _} -> :ok
+ {:error, _} -> :ok # User created; email failure logged separately
+ end
changeset = Accounts.change_registration(user)
{:noreply, socket |> assign(trigger_submit: true) |> assign_form(changeset)}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {:ok, user} -> | |
| {:ok, _} = | |
| Accounts.deliver_user_confirmation_instructions( | |
| user, | |
| &url(~p"/users/confirm/#{&1}") | |
| ) | |
| {:ok, user} -> | |
| case Accounts.deliver_user_confirmation_instructions( | |
| user, | |
| &url(~p"/users/confirm/#{&1}") | |
| ) do | |
| {:ok, _} -> :ok | |
| {:error, _} -> :ok # User created; email failure logged separately | |
| end | |
| changeset = Accounts.change_registration(user) | |
| {:noreply, socket |> assign(trigger_submit: true) |> assign_form(changeset)} |
🤖 Prompt for AI Agents
In `@lib/ksef_hub_web/live/user_registration_live.ex` around lines 30 - 35, The
direct pattern match {:ok, _} =
Accounts.deliver_user_confirmation_instructions(...) can crash on delivery
failure; replace it with a safe branch (case or with) around
Accounts.deliver_user_confirmation_instructions in user_registration_live (the
block that builds url(~p"/users/confirm/#{&1}")) and handle {:ok, _} and
{:error, reason} explicitly — on success keep current flow, on error log the
reason and surface a user-friendly message via put_flash (or similar) so the
LiveView does not crash and the user sees a graceful failure response.
- CompanySwitchController: guard against nil current_user with case instead of flat with, separate unauthenticated vs not-found paths - UserRegistrationLive: handle email delivery failure gracefully with case instead of crash-on-error pattern match - UserAuth: fix signed_in_path docs to match implementation (always returns /dashboard, company redirect handled by LiveAuth on_mount) - Accounts: normalize email (trim + downcase) in get_user_by_email_and_password to match registration behavior - UserToken: add catch-all to days_for_context returning :error, update verify_email_token_query to handle unknown contexts safely - UserNotifier: make sender email configurable via :mailer_from - UserLoginLive: fix redundant params access in save handler - CompanyLive: add data-testid="company-name" to template - Test improvements: fix double-negative assertion, use factories consistently, improve docs on extract_user_token and log_in_user, use has_element? for company list assertions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@lib/ksef_hub_web/live/user_registration_live.ex`:
- Around line 31-41: The Logger.error call in the
Accounts.deliver_user_confirmation_instructions error branch exposes raw error
details via inspect(reason); update the error handling in
user_registration_live.ex to avoid logging tokens/credentials by removing
inspect(reason) and logging a non-sensitive, generic message (e.g., "Failed to
deliver confirmation email") or a sanitized error code/string instead, keeping
the surrounding structure (the case on
Accounts.deliver_user_confirmation_instructions and the require Logger) intact.
In `@lib/ksef_hub_web/user_auth.ex`:
- Around line 32-45: When logging out in log_out_user, broadcast a "disconnect"
to the user's LiveView topic so mounted LiveViews close immediately: after
reading user_token via get_session(conn, :user_token) (and before relying on
stale assigns), compose the live_socket_id string (e.g.
"users_sessions:#{user_token}") and call your endpoint's broadcast with event
"disconnect" and an empty payload, then proceed to call
Accounts.delete_user_session_token(user_token) and continue with
configure_session/clear_session/redirect; update lib/ksef_hub_web/user_auth.ex
around the log_out_user function to perform the broadcast using your app
Endpoint and the user_token-derived live_socket_id.
In `@lib/ksef_hub/accounts.ex`:
- Around line 110-135: In get_or_create_google_user/1, normalize the Google
email by downcasing it once and reuse that normalized value for lookups and
inserts to avoid case-mismatch duplicates; specifically, compute a
normalized_email = String.downcase(email || "") at the top of the function, call
get_user_by_email(normalized_email) (instead of the raw email), and when
building the User.changeset for the insert/update include the normalized_email
as the :email field so the behaviour matches User.changeset/2 which downcases
emails.
- Around line 185-197: Update the `@specs` for
deliver_user_confirmation_instructions/2 and
deliver_user_reset_password_instructions/2 to include mailer error returns:
change the return type from {:ok, Swoosh.Email.t()} | {:error,
:already_confirmed} (or solely {:ok, Swoosh.Email.t()}) to include {:error,
term()} (and keep the existing {:error, :already_confirmed} case if you want it
explicit), so the specs reflect that UserNotifier.deliver_* can return {:ok,
Swoosh.Email.t()} | {:error, term()} (and optionally | {:error,
:already_confirmed}) for the functions deliver_user_confirmation_instructions
and deliver_user_reset_password_instructions.
In `@test/support/conn_case.ex`:
- Around line 39-76: Add `@spec` annotations for the public helpers: declare
extract_user_token/1 with the type (fun :: (String.t() -> {:ok,
Swoosh.Email.t()})) :: {String.t(), Swoosh.Email.t()} placed immediately above
the defdelegate extract_user_token(fun) line, and declare log_in_user/3 with the
type (conn :: Plug.Conn.t(), user :: KsefHub.Accounts.User.t(), extra_session ::
map()) :: Plug.Conn.t() placed immediately above the def log_in_user(conn, user,
extra_session \\ %{}) definition; ensure the specs match the exported function
names extract_user_token/1 and log_in_user/3 and follow project formatting
conventions.
- Remove inspect(reason) from Logger.error in UserRegistrationLive to avoid leaking sensitive data in logs - Add LiveView broadcast disconnect on logout so mounted sessions close immediately - Normalize Google email in get_or_create_google_user to prevent case-sensitive duplicates - Update @specs for deliver_user_confirmation_instructions and deliver_user_reset_password_instructions to include {:error, term()} - Add @SPEC annotations to ConnCase helpers Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@lib/ksef_hub/accounts.ex`:
- Around line 63-74: The current get_user_by_email_and_password/2 function has a
guard requiring binaries and will raise a FunctionClauseError for non-binary
inputs; add a fallback clause def get_user_by_email_and_password(_, _), do: nil
so calls with nil or non-binary email/password safely return nil, leaving the
existing guarded version (which normalizes the email, calls get_user_by_email/1
and uses User.valid_password?/2) intact.
- Around line 102-138: The current get_or_create_google_user performs separate
reads and writes which can race; convert it to an atomic Ecto transaction
(Ecto.Multi) inside get_or_create_google_user: start a Repo.transaction that (1)
attempts to fetch the user by google_uid (using get_user_by_google_uid or
Repo.get_by within the transaction) and returns it if found, (2) otherwise
attempts to fetch by normalized email and, if found, build a User.changeset to
set google_uid/name/avatar_url and call Repo.update as a Multi.run step, (3) if
no email user, build the new user changeset and call Repo.insert as a
Multi.insert step; ensure the User schema enforces unique_constraint on :email
and :google_uid and handle unique_constraint errors from the transaction result
by returning the existing user (pattern-match {:ok, result} vs {:error,
_changeset} and resolve conflicts by fetching the already-created record and
returning {:ok, user}). This replaces the plain read→update/insert flow with an
atomic Ecto.Multi/Repo.transaction using User.changeset, Repo.insert/update, and
unique_constraint handling.
In `@test/support/conn_case.ex`:
- Around line 39-55: Update the callback type for extract_user_token/1 to
reflect that the passed-in fun receives a URL-builder function and returns {:ok,
Swoosh.Email.t()}: change the `@doc` to state fun has the shape (String.t() ->
String.t()) -> {:ok, Swoosh.Email.t()} and update the `@spec` from (function() ->
{:ok, Swoosh.Email.t()}) to ((String.t() -> String.t()) -> {:ok,
Swoosh.Email.t()}) :: {String.t(), Swoosh.Email.t()}; reference the behavior in
KsefHub.DataCase.extract_user_token/1 (which calls
fun.(&"[TOKEN]#{&1}[/TOKEN]")) when making these edits so the doc and spec match
the actual callback signature.
🧹 Nitpick comments (1)
test/support/conn_case.ex (1)
72-79: Preventextra_sessionfrom overriding:user_token.
Map.merge(%{user_token: token}, extra_session)lets callers overwrite:user_token, which can mask auth issues in tests. Consider ensuring the generated token always wins.♻️ Suggested adjustment
- session = Map.merge(%{user_token: token}, extra_session) + session = extra_session |> Map.put(:user_token, token)
- Add fallback clause for get_user_by_email_and_password/2 to safely return nil for non-binary inputs instead of raising FunctionClauseError - Convert get_or_create_google_user/1 to atomic Ecto.Multi transaction with unique constraint conflict resolution, preventing race conditions on concurrent Google sign-ins - Fix extract_user_token @doc and @SPEC in ConnCase and DataCase to reflect actual callback signature: (String.t() -> String.t()) -> {:ok, Swoosh.Email.t()} - Ensure log_in_user helper always wins over extra_session for :user_token key to prevent masking auth issues in tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/ksef_hub/accounts.ex`:
- Around line 179-213: Add explicit guards to the session-token functions:
change get_user_by_session_token/1 to have a clause with when is_binary(token)
that runs the existing verification and Repo.one(query), and add a fallback
clause def get_user_by_session_token(_), do: nil; likewise add when
is_binary(token) to delete_user_session_token/1 for the existing code that
hashes and deletes, and add a fallback def delete_user_session_token(_), do:
:ok. Reference the existing functions get_user_by_session_token/1 and
delete_user_session_token/1 and keep generate_user_session_token/1 unchanged.
Add is_binary(token) guards to get_user_by_session_token/1 and delete_user_session_token/1 with fallback clauses that safely return nil/:ok for non-binary inputs instead of raising. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Merged
This was referenced Mar 8, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Tests
Chores