fix(accounts): always set session cookie on login regardless of OL account lookup#12876
Merged
Conversation
…count lookup `_set_login_cookies` was gated inside `if ol_account := OpenLibraryAccount.get_by_email(email)` so users whose OL account could not be resolved by email (e.g. IA/OL email mismatch) would authenticate successfully but never receive a session cookie, causing a silent redirect back to the login page. Move `_set_login_cookies` outside the guard, matching the OTP login flow which already calls it unconditionally. `_set_login_cookies` already handles `ol_account=None` gracefully (sets session + pd cookies, skips sfw/yrg_banner_pref extras). Fixes #12857
github-actions
Bot
added
the
Priority: 1
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a login regression in the upstream account plugin where users could authenticate via IA but receive no Open Library session cookie when OpenLibraryAccount.get_by_email(...) returns None, resulting in a silent login loop.
Changes:
- Always call
_set_login_cookies(...)after a successful audit, regardless of whether an OL account lookup succeeds. - Keep the PDA flash-message behavior gated on
ol_accountwhile ensuring the session cookie is still set. - Reorders one import near the top of the module (but currently in a way that likely conflicts with Ruff/isort expectations).
Comments suppressed due to low confidence (1)
openlibrary/plugins/upstream/account.py:14
- Import order likely violates the repo’s Ruff/isort import grouping: third-party imports (requests, web) should come before first-party imports (infogami/openlibrary). Having
import infogami.core.code as coreaboverequests/webmay trigger RuffI(isort) and is inconsistent with other upstream modules (e.g. openlibrary/plugins/upstream/code.py keepsimport webbeforefrom infogami ...).
import requests
import web
import infogami.core.code as core # noqa: F401 side effects may be needed
jimchamp
approved these changes
Jun 8, 2026
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
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
Fixes a regression introduced by #11052 where users with an IA/OL email mismatch (or no linked OL account) would authenticate successfully but silently receive no session cookie, causing an infinite login loop with no error message.
Root cause:
_set_login_cookieswas moved inside anif ol_account := OpenLibraryAccount.get_by_email(email):guard in #11052. If that lookup returnsNone, the session cookie is never set, the browser gets no session, and the next request is treated as unauthenticated.Fix: Move
_set_login_cookiesoutside the guard, matching the OTP login flow (account_login_otp_redeem) which already calls it unconditionally with a potentially-Noneol_account._set_login_cookiesalready handlesol_account=Nonecorrectly.Before (broken):
After (fixed):
Test plan
/account/booksremember mecheckbox — session cookie has year-long expiryol_accountis found)Fixes #12857