Skip to content

Add ORCID OAuth2 flow (login + link)#40

Open
leotrs wants to merge 10 commits intomainfrom
prs-614.2
Open

Add ORCID OAuth2 flow (login + link)#40
leotrs wants to merge 10 commits intomainfrom
prs-614.2

Conversation

@leotrs
Copy link
Copy Markdown
Contributor

@leotrs leotrs commented Mar 28, 2026

Summary

  • Adds ORCID OAuth2 authentication with three routes: /auth/orcid (redirect), /auth/orcid/callback (token exchange + login/register/link), /auth/orcid/unlink (remove ORCID)
  • Unauthenticated users can sign in or create an account via ORCID; authenticated users can link/unlink ORCID to their account
  • CSRF protection via random state parameter stored in cookie + server-side dict

Test plan

  • 13 unit tests covering: redirect URL params, state validation, login existing user, create new user, link when logged in, duplicate ORCID rejection, unlink with/without password, token exchange failure
  • CI passes all unit tests
  • Manual test with ORCID sandbox after deploy

🤖 Generated with Claude Code

leotrs and others added 4 commits March 28, 2026 07:25
Adds nullable, unique, indexed orcid_id column (String(20)) to the users
table for ORCID iD storage. Includes format validation (XXXX-XXXX-XXXX-XXXX
with optional X checksum) via @validates decorator and standalone function.
14 tests covering validation, uniqueness, NULL handling, and backward compat.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The orcid_id migration shared revision ID a1b2c3d4e5f6 with the
slug/publication_year migration, causing 'Multiple head revisions'
error during CI. Assign unique revision ID 596bb368fc0d.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Implement full ORCID OAuth2 flow with three routes:
- GET /auth/orcid: redirects to ORCID authorize URL with CSRF state
- GET /auth/orcid/callback: exchanges code for token, handles login/register/link
- GET /auth/orcid/unlink: removes ORCID from account (blocked if no password)

Includes 13 unit tests covering all OAuth scenarios with mocked httpx.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

Deploy Preview

https://scroll-press-pr-40.fly.dev

This preview will be destroyed when the PR is closed.

@leotrs
Copy link
Copy Markdown
Contributor Author

leotrs commented Mar 28, 2026

Needs human review

What changed: Added ORCID OAuth2 authentication — users can now sign in/register via ORCID, and link/unlink ORCID to existing accounts. New /auth/orcid routes, orcid_id column on users table, and database migration.

Review checklist:

  1. Open the deploy preview: https://press-prs-eou-2.fly.dev (check the Fly deploy from CI)
  2. Navigate to /login — verify an ORCID sign-in option is visible (or check /auth/orcid redirects to ORCID sandbox)
  3. Test the ORCID redirect flow: click the ORCID button, confirm it redirects to sandbox.orcid.org/oauth/authorize with correct params
  4. Register a new account via ORCID sandbox credentials — verify you land on /dashboard
  5. Log in with an existing email/password account, then visit /auth/orcid to link ORCID — verify /settings?orcid=linked redirect
  6. Try unlinking ORCID from settings — verify it works when you have a password
  7. Create an ORCID-only account (no password), then try to unlink — verify it blocks with orcid_no_password error
  8. Try linking an ORCID already used by another account — verify orcid_taken error

What to look for:

  • Placeholder email ({orcid}@orcid.placeholder) for ORCID-only users — is this acceptable UX or should users be prompted to set a real email?
  • Password hash sentinel !orcid-only — confirm this cannot be used to log in via the normal password flow
  • In-memory _pending_states dict has the same horizontal scaling limitation as CSRF tokens (noted in code) — acceptable for single-machine deploy
  • State cookie max_age=600 (10 min) — reasonable timeout for OAuth flow?
  • No UI changes visible in the diff (no template updates for ORCID button) — is there a separate PR for the frontend, or is this backend-only for now?

@leotrs
Copy link
Copy Markdown
Contributor Author

leotrs commented Mar 28, 2026

Screenshot 2026-03-28 at 20 54 38

no ORCID sign in option

@leotrs
Copy link
Copy Markdown
Contributor Author

leotrs commented Mar 28, 2026

Needs human review

What changed: Added ORCID OAuth2 authentication — users can now sign in/register via ORCID, and existing users can link/unlink their ORCID iD from account settings.

Review checklist:

  1. Open the deploy preview from the CI checks
  2. Navigate to /login — verify the ORCID sign-in option appears (if UI was added)
  3. Navigate to /settings while logged in — verify ORCID link/unlink controls appear
  4. Test the ORCID redirect flow: click "Sign in with ORCID" and verify it redirects to sandbox.orcid.org/oauth/authorize with correct params (response_type=code, scope=/authenticate)
  5. Verify that unauthenticated ORCID callback creates a new account and redirects to /dashboard
  6. Verify that authenticated ORCID callback links the ORCID to the current account and redirects to /settings?orcid=linked
  7. Verify unlinking ORCID when user has a password works (redirects to /settings?orcid=unlinked)
  8. Verify unlinking is blocked when user has no password (prevents lockout, redirects with error=orcid_no_password)
  9. Verify duplicate ORCID linking is rejected (error=orcid_taken)

What to look for:

  • ORCID state parameter uses secrets.token_urlsafe and is validated on callback (CSRF protection)
  • Placeholder email {orcid_id}@orcid.placeholder for ORCID-only registrations — confirm this is acceptable UX or if users should be prompted for email
  • password_hash="!orcid-only" sentinel — verify this doesn't conflict with password validation elsewhere
  • In-memory _pending_states dict has same horizontal scaling limitation as CSRF tokens (documented in CLAUDE.md)
  • New orcid_id column migration has unique index — verify it applies cleanly to production schema
  • ORCID_BASE_URL defaults to sandbox — confirm production deployment sets this to https://orcid.org

@leotrs
Copy link
Copy Markdown
Contributor Author

leotrs commented Mar 28, 2026

Needs human review

What changed: Added ORCID OAuth2 authentication — users can now sign in, create accounts, or link/unlink their ORCID iD. Includes a new DB migration adding orcid_id column, three new routes (/auth/orcid, /auth/orcid/callback, /auth/orcid/unlink), and ORCID-only account creation with placeholder emails.

Review checklist:

  1. Open the deploy preview: https://github.com/aris-pub/press/actions/runs/23692444029/job/69022157435 (check the deploy job for the preview URL)
  2. Navigate to /login and verify the ORCID sign-in option appears (or confirm it's backend-only with no UI yet)
  3. If UI exists: click "Sign in with ORCID", verify redirect goes to sandbox.orcid.org with correct params
  4. Test the link flow: log in with email/password, then visit /auth/orcid — verify it redirects to ORCID authorize
  5. Test the unlink flow: visit /auth/orcid/unlink while logged in with a password-based account
  6. Verify /auth/orcid/unlink is blocked for ORCID-only accounts (no password set)
  7. Check that the migration applies cleanly: orcid_id column is nullable, unique-indexed

What to look for:

  • Security: CSRF state validation — cookie + server-side dict match. Confirm state is consumed after use (no replay)
  • Account lockout: ORCID-only users (password_hash = !orcid-only) cannot unlink, which would lock them out
  • Placeholder emails: New ORCID accounts get {orcid_id}@orcid.placeholder — verify this doesn't conflict with email validation elsewhere (login, password reset, email verification flows)
  • Scaling concern: _pending_states is in-memory dict (same pattern as CSRF tokens) — already documented as pre-horizontal-scaling limitation
  • Edge case: What happens if a user tries to link an ORCID already taken by another account? Should show clear error, not silently fail
  • DB migration: Confirm down_revision chain is correct (d3a7b8c1e2f4 exists)

Addresses reviewer feedback: the ORCID OAuth2 backend routes existed but
had no UI entry points. Adds ORCID buttons on login/register pages with
an "or" divider, and link/unlink controls in the dashboard's account
management section. Fixes redirect targets from nonexistent /settings to
/dashboard.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@leotrs
Copy link
Copy Markdown
Contributor Author

leotrs commented Mar 28, 2026

Needs human review

What changed: Added ORCID OAuth2 authentication — "Sign in/up with ORCID" buttons on login and register pages, plus ORCID link/unlink controls on the dashboard. New DB migration adds orcid_id column to users.

Review checklist:

  1. Open the deploy preview from the CI checks (deploy job passed)
  2. Navigate to /login — verify the "or" divider and "Sign in with ORCID" button appear below the email/password form
  3. Navigate to /register — verify the "Sign up with ORCID" button appears similarly
  4. Log in with a test account and go to /dashboard — verify the ORCID section shows "No ORCID linked" with a "Link ORCID" button
  5. Click "Link ORCID" and confirm it redirects to the ORCID sandbox authorize page with correct query params (client_id, scope=/authenticate, state)
  6. If you have an ORCID sandbox account, complete the flow and verify the dashboard updates to show the linked ORCID iD with an "Unlink ORCID" button
  7. Verify that a user created via ORCID-only login (no password) sees a disabled "Unlink ORCID" button with a tooltip explaining they need to set a password first

What to look for:

  • ORCID button styling: green icon, white background, proper alignment with the form above
  • "or" divider renders cleanly between the form and ORCID button
  • Mobile responsive: button should have min-height 44px touch target, no overflow
  • Dashboard ORCID section spacing is consistent with the existing "Account Management" section
  • ORCID iD link on dashboard opens https://orcid.org/{id} in a new tab
  • Error states: try /login?error=orcid_state and /dashboard?error=orcid_taken — verify no broken UI (even if no user-visible error message is shown yet)
  • The !orcid-only password sentinel is not exposed anywhere in the UI

@leotrs
Copy link
Copy Markdown
Contributor Author

leotrs commented Mar 29, 2026

Screenshot 2026-03-29 at 08 04 35

not there. see screenshot. console contains this error:
Uncaught (in promise) TypeError: Cannot read properties of null (reading 'includes')
at AutofillOverlayContentService. (bootstrap-autofill-overlay.js:9562:81)
at Generator.next ()
at bootstrap-autofill-overlay.js:8522:71
at new Promise ()
at autofill_overlay_content_service_awaiter (bootstrap-autofill-overlay.js:8518:12)
at AutofillOverlayContentService.setQualifiedLoginFillType (bootstrap-autofill-overlay.js:9555:16)
at AutofillOverlayContentService.isIgnoredField (bootstrap-autofill-overlay.js:9530:23)
at AutofillOverlayContentService. (bootstrap-autofill-overlay.js:8981:22)
at Generator.next ()
at bootstrap-autofill-overlay.js:8522:71

leotrs and others added 2 commits March 29, 2026 08:09
…view)

The buttons were wrapped in {% if orcid_client_id %} which hid them when
ORCID_CLIENT_ID env var wasn't set (e.g. on PR preview deploys). Now buttons
always render; the route handler guards against unconfigured ORCID credentials.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@leotrs
Copy link
Copy Markdown
Contributor Author

leotrs commented Mar 29, 2026

Needs human review

What changed: Added ORCID OAuth2 authentication — new "Sign in/up with ORCID" buttons on login and register pages, and an ORCID link/unlink section on the dashboard.

Review checklist:

  1. Open the deploy preview from the CI checks
  2. Navigate to /login — verify the "Sign in with ORCID" button appears below the login form with an "or" divider
  3. Navigate to /register — verify the "Sign up with ORCID" button appears below the register form with an "or" divider
  4. Log in with a test account and navigate to /dashboard — verify the "ORCID" section appears with a "Link ORCID" button
  5. Resize browser to mobile width on login and register pages — verify the ORCID button is properly sized (min 44px tap target) and layout doesn't break
  6. Click "Sign in with ORCID" — verify it redirects to sandbox.orcid.org with correct OAuth parameters (or shows an error if ORCID credentials aren't configured in the preview environment)

What to look for:

  • ORCID button styling: white background, green ORCID logo, consistent with existing button styles
  • "or" divider alignment and spacing between the form and ORCID button
  • Dashboard ORCID section placement relative to the existing "Account Management" section
  • Mobile responsiveness of the new elements
  • The disabled "Unlink ORCID" button state for ORCID-only users (tooltip should explain why)

@leotrs
Copy link
Copy Markdown
Contributor Author

leotrs commented Mar 29, 2026

Screenshot 2026-03-29 at 18 13 33

steps 1-5 succeeded. step 6 failed. see screenshot.

…ages

When ORCID OAuth fails (e.g. not configured, state mismatch, token error),
the redirect includes an error query param that was silently ignored. Now
login, register, and dashboard pages display user-friendly error banners.
Dashboard also shows success messages for link/unlink operations.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@leotrs
Copy link
Copy Markdown
Contributor Author

leotrs commented Mar 29, 2026

Needs human review

What changed: Added ORCID OAuth2 authentication — users can now sign in/up via ORCID, and link/unlink ORCID from their dashboard.

Review checklist:

  1. Open the deploy preview from the CI checks
  2. Navigate to /login — verify the "Sign in with ORCID" button appears below the login form with an "or" divider
  3. Navigate to /register — verify the "Sign up with ORCID" button appears with the same pattern
  4. Log in with a test account and go to /dashboard — verify the new "ORCID" section appears with a "Link ORCID" button
  5. Click "Link ORCID" and verify it redirects to the ORCID sandbox authorize page with correct params (client_id, scope=/authenticate, state)
  6. Test error states by visiting /login?error=orcid_not_configured — verify the error banner renders
  7. Test /dashboard?error=orcid_taken and /dashboard?orcid=linked — verify error/success alerts render correctly
  8. Verify the ORCID button styling works in dark mode (check alert-error color override)
  9. Resize browser to mobile width — verify the ORCID button has min-height 44px and proper spacing

What to look for:

  • ORCID button alignment and spacing relative to the form and "or" divider
  • ORCID green logo renders correctly in the button SVG
  • Dark mode: error alert text should be readable (light red on dark bg)
  • Dashboard "ORCID" section placement relative to "Account Management" section — no visual collision
  • Unlink button is disabled (greyed out) for ORCID-only users with no password
  • Mobile: touch target size is adequate (44px min-height), no overflow or clipping

@leotrs
Copy link
Copy Markdown
Contributor Author

leotrs commented Mar 29, 2026

Steps 1-4 worked. Clicking on "Link ORCID" redirected me to the homepage

leotrs and others added 2 commits March 29, 2026 18:49
When a logged-in user triggers an ORCID error (e.g. not configured, token
failure), redirect to /dashboard with the error instead of /login. Previously
/login detected the active session and bounced to /, losing the error message.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@leotrs
Copy link
Copy Markdown
Contributor Author

leotrs commented Mar 29, 2026

Needs human review

What changed: Added ORCID OAuth2 authentication — "Sign in/up with ORCID" buttons on login and register pages, plus an ORCID link/unlink section on the dashboard.

Review checklist:

  1. Open the deploy preview (check the deploy CI job for the Fly preview URL)
  2. Navigate to /login — verify the "Sign in with ORCID" button appears below the form with an "or" divider
  3. Navigate to /register — verify the "Sign up with ORCID" button appears with matching style
  4. Log in with a test account and go to /dashboard — verify the new "ORCID" section appears with a "Link ORCID" button
  5. Test error states by visiting /login?error=orcid_not_configured and /dashboard?error=orcid_taken — verify error alerts render correctly
  6. Test success states by visiting /dashboard?orcid=linked — verify the success alert renders
  7. If ORCID sandbox credentials are configured, try the full flow: click "Sign in with ORCID" on login page, authorize with sandbox account, verify redirect back to dashboard

What to look for:

  • ORCID button styling: green ORCID logo icon, consistent with the rest of the auth form design
  • "or" divider alignment and spacing between the form and ORCID button
  • Error/success alert styling (red for errors, green for success) in both light and dark themes
  • Dashboard ORCID section: linked state shows ORCID iD as a clickable link to orcid.org, unlinked state shows "Link ORCID" button
  • Mobile responsiveness: buttons should have 44px min touch target, no overflow
  • Security: ORCID-only users (no password) should see a disabled "Unlink ORCID" button with explanatory tooltip

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant