scaffolding for #10262 ol registration by mekarpeles · Pull Request #11052 · internetarchive/openlibrary

mekarpeles · 2025-07-21T22:03:10Z

Closes #10262

Technical

adds xauthn verify / activate method
refactors account plugin to shift logic to audit
reclaims previous activate class for new style activation/verification
DRY login cookies

Must be coordinated with https://git.archive.org/ia/petabox/-/merge_requests/5406/diffs

Stakeholders

@jimchamp

Copilot

Pull Request Overview

This PR implements scaffolding for OpenLibrary registration verification using Internet Archive's xauthn service, refactoring account authentication logic and improving code organization.

Adds new xauthn verify/activate method for account verification
Refactors account plugin to move Print Disability (PD) logic to the audit layer
Consolidates login cookie management into reusable methods

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.

File	Description
openlibrary/plugins/upstream/account.py	Refactors login logic, removes PD helper functions, adds new verification endpoint, and consolidates cookie handling
openlibrary/accounts/model.py	Moves PD-related functionality to OpenLibraryAccount class, adds InternetArchiveAccount.verify method, and integrates PD processing into audit_accounts

Comments suppressed due to low confidence (1)

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

mekarpeles · 2026-04-28T04:37:10Z

Re: Copilot round-2 review — Addressed all feedback:

Real bugs fixed:

elif not pda: unreachable — changed to else: so REQUESTED status is correctly set when user has PD cookie but no special access yet.
Enum vs int comparison — pd_status == PDRequestStatus.REQUESTED.value changed to pd_status == PDRequestStatus.REQUESTED (compare enum to enum, not to its int value).
pd_status treats 0 as falsey — if rpd changed to if rpd is not None; also added int(rpd) normalization before constructing the enum.
update_pd bugs — fixed if rpd → if rpd is not None; fixed save_preferences(**prefs) → save_preferences(prefs) (takes a dict, not unpacked kwargs).
Double EMAILED update — removed the redundant update_pd(pda, EMAILED) call after send_pd_email() since send_pd_email already calls update_pd(rpd=EMAILED) internally.
action.split(':') ValueError — changed to (action.split(':', 1) + [''])[:2] to safely handle actions with no colon or extra colons.
send_verification_email removed but still called — restored both the module-level function and the Account.send_verification_email() instance method so the admin resend flow (admin/code.py:404) continues to work.

False positives (no changes needed):

v not defined — v is the loop variable in for k, v in kwargs.items(), defined on the same line as the web.setcookie call.
ol_user not defined — ol_user = ol_account.get_user() is assigned immediately before its first use, inside the same if ol_account := ... block.
Incomplete conditional for yrg_banner_pref — if pref_key := ... is a walrus-operator assignment; the cookie is set only if the preference key exists. Correct as-is.
Missing comma / syntax error in account_verify.GET — the code account_login().login(access=r['s3']['access'], secret=r['s3']['secret']) has no email argument and no syntax error.
Login called as class method — account_login().login(...) correctly instantiates the handler then calls the method. Not a class method call.
Bare raise / XXX comment — no bare raise or XXX exists in the current code. Stale comment from an earlier diff state.
prd typo / ol_account undefined in send_pd_email — both were fixed in the previous round of Copilot review. Already resolved.

mekarpeles · 2026-05-04T00:16:59Z

Tested and works; should show some sort of toast message that verification succeeded and patron is logged in if the /account/verify?t= endpoint succeeds.

Tested subsequent activation attempts fail (as desired); token can only be used to activate + login once.

mekarpeles · 2026-05-04T00:17:44Z

Since email is sent from achive.org system, can this block of code be removed? Is it used anywhere?

https://github.com/internetarchive/openlibrary/pull/11052/changes#diff-884f3a712a7e51cce625ca19060335684272f33a967702b19e66e48dbdd1be69R223-R232

mekarpeles · 2026-05-04T00:23:31Z

TL;DR

Adds support for /account/verify?t=

Archive.org sendmail can be updated to email link for openlibrary.org
https://git.archive.org/ia/petabox/-/merge_requests/5406/diffs

Until this happens, the existing code should still work (for now it expects IA to send archive.org link). When we switch over, it should work automatically.

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

jimchamp · 2026-05-14T22:37:05Z

Since email is sent from achive.org system, can this block of code be removed? Is it used anywhere?

https://github.com/internetarchive/openlibrary/pull/11052/changes#diff-884f3a712a7e51cce625ca19060335684272f33a967702b19e66e48dbdd1be69R223-R232

I think you're talking about clear_cookies. Isn't this used when we login from /admin/people?

jimchamp · 2026-05-14T22:43:13Z

This all seems fine to me.

I fixed a merge conflict related to our recent open redirect fixes. The commit is here. Please check it out before merging.

- Adds /account/verify?t= endpoint: validates IA xauthn token, logs user in with S3 keys, flashes success message - Refactors account plugin: moves PD logic to audit layer, DRYs login cookie handling into set_cookies() - Moves create_link_doc to accounts/model.py; removes OL-side verification email (IA/xauthn sends it now) - Adds tests for account_verify.GET, set_cookies, InternetArchiveAccount.verify Closes #10262

for more information, see https://pre-commit.ci

Replaces inline cookie-setting in account_login.login() and the OTP redeem handler with a single module-level _set_login_cookies(audit, ol_account, remember) that owns all session cookies (login token, pd, sfw, yrg_banner_pref). Also fixes the CI ruff F821 error caused by a call to _set_account_cookies which was removed when #11052 landed.

mekarpeles assigned jimchamp Jul 21, 2025

Copilot AI review requested due to automatic review settings July 21, 2025 22:03

mekarpeles added the Needs: Special Deploy This PR will need a non-standard deploy to production label Jul 21, 2025

mekarpeles marked this pull request as draft July 21, 2025 22:03

github-actions Bot added the Priority: 2 Important, as time permits. [managed] label Jul 21, 2025

Copilot AI reviewed Jul 21, 2025

View reviewed changes

mekarpeles commented Jul 23, 2025

View reviewed changes

Comment thread openlibrary/accounts/model.py Outdated

mekarpeles mentioned this pull request Aug 15, 2025

Implement Bookshare OAuth Integration for Print Disability Qualification Flow #11155

Open

8 tasks

Copilot AI mentioned this pull request Aug 15, 2025

Implement Bookshare OAuth Integration for Print Disability Qualification Flow #11159

Closed

mekarpeles mentioned this pull request Aug 15, 2025

Enable Open Library to perform account activation + sign-in #10262

Closed

3 tasks

jimchamp removed their assignment Mar 11, 2026

mekarpeles force-pushed the 10262/registration branch from 514d843 to 75cd969 Compare April 28, 2026 03:46

mekarpeles requested a review from Copilot April 28, 2026 04:19

Copilot started reviewing on behalf of mekarpeles April 28, 2026 04:19 View session

Copilot AI reviewed Apr 28, 2026

View reviewed changes

mekarpeles force-pushed the 10262/registration branch from 3b3099b to d742659 Compare April 28, 2026 04:36

mekarpeles marked this pull request as ready for review May 1, 2026 18:32

mekarpeles force-pushed the 10262/registration branch from 3d5dc0e to 160ceb3 Compare May 3, 2026 23:11

mekarpeles assigned jimchamp May 4, 2026

mekarpeles commented May 5, 2026

View reviewed changes

Comment thread openlibrary/plugins/upstream/account.py Outdated

mekarpeles requested a review from Copilot May 6, 2026 04:26

Copilot started reviewing on behalf of mekarpeles May 6, 2026 04:27 View session

Copilot AI reviewed May 6, 2026

View reviewed changes

Comment thread openlibrary/plugins/upstream/account.py

Comment thread openlibrary/plugins/upstream/account.py

Comment thread openlibrary/plugins/upstream/account.py Outdated

Comment thread openlibrary/accounts/model.py Outdated

Comment thread openlibrary/templates/admin/people/view.html

mekarpeles force-pushed the 10262/registration branch 3 times, most recently from 22f723d to fd761c9 Compare May 6, 2026 04:41

mekarpeles force-pushed the 10262/registration branch 2 times, most recently from 246efff to a993d00 Compare May 7, 2026 23:55

This was referenced May 8, 2026

Support OTP (passwordless) login on Open Library login page #12664

Closed

feat(accounts): add OTP (passwordless) login support via xauthn #12681

Merged

mekarpeles force-pushed the 10262/registration branch from 8c79344 to 6095dab Compare May 20, 2026 23:09

[pre-commit.ci] auto fixes from pre-commit.com hooks

2d9c783

for more information, see https://pre-commit.ci

mekarpeles merged commit dc6cac6 into master May 31, 2026
7 of 8 checks passed

mekarpeles deleted the 10262/registration branch May 31, 2026 17:13

This was referenced Jun 8, 2026

openlibrary.org login fails; archive.org succeeds but doesn't work for openlibrary.org #12857

Closed

fix(accounts): always set session cookie on login regardless of OL account lookup #12876

Merged

Uh oh!

Conversation

mekarpeles commented Jul 21, 2025

Technical

Stakeholders

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mekarpeles commented Apr 28, 2026

Uh oh!

mekarpeles commented May 4, 2026

Uh oh!

mekarpeles commented May 4, 2026

Uh oh!

mekarpeles commented May 4, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jimchamp commented May 14, 2026

Uh oh!

jimchamp commented May 14, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants