From ec4b09631cea4bf82a8039423813379ade94db91 Mon Sep 17 00:00:00 2001 From: Leo Torres Date: Sat, 28 Mar 2026 07:25:45 +0100 Subject: [PATCH 1/7] Add orcid_id column to User model with validation and migration 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) --- .../a1b2c3d4e5f6_add_orcid_id_to_users.py | 31 ++++ app/models/user.py | 22 ++- tests/test_orcid.py | 154 ++++++++++++++++++ 3 files changed, 206 insertions(+), 1 deletion(-) create mode 100644 alembic/versions/a1b2c3d4e5f6_add_orcid_id_to_users.py create mode 100644 tests/test_orcid.py diff --git a/alembic/versions/a1b2c3d4e5f6_add_orcid_id_to_users.py b/alembic/versions/a1b2c3d4e5f6_add_orcid_id_to_users.py new file mode 100644 index 0000000..421197a --- /dev/null +++ b/alembic/versions/a1b2c3d4e5f6_add_orcid_id_to_users.py @@ -0,0 +1,31 @@ +"""Add orcid_id column to users + +Revision ID: a1b2c3d4e5f6 +Revises: d3a7b8c1e2f4 +Create Date: 2026-03-28 + +""" + +from typing import Sequence, Union + +import sqlalchemy as sa + +from alembic import op + +revision: str = "a1b2c3d4e5f6" +down_revision: Union[str, Sequence[str], None] = "d3a7b8c1e2f4" +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def upgrade() -> None: + op.add_column( + "users", + sa.Column("orcid_id", sa.String(20), nullable=True), + ) + op.create_index("ix_users_orcid_id", "users", ["orcid_id"], unique=True) + + +def downgrade() -> None: + op.drop_index("ix_users_orcid_id", table_name="users") + op.drop_column("users", "orcid_id") diff --git a/app/models/user.py b/app/models/user.py index 8dad6f0..5313f91 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -1,12 +1,27 @@ from datetime import datetime, timezone +import re import uuid from sqlalchemy import Boolean, Column, DateTime, String, TypeDecorator from sqlalchemy.dialects.postgresql import UUID -from sqlalchemy.orm import relationship +from sqlalchemy.orm import relationship, validates from app.database import Base +ORCID_PATTERN = re.compile(r"^\d{4}-\d{4}-\d{4}-\d{3}[\dX]$") + + +def validate_orcid_id(value: str | None) -> str | None: + """Validate ORCID iD format: XXXX-XXXX-XXXX-XXXX (last char may be X checksum).""" + if value is None: + return None + if not ORCID_PATTERN.match(value): + raise ValueError( + f"Invalid ORCID iD format: '{value}'. " + "Expected format: 0000-0002-1234-5678" + ) + return value + # Cross-platform UUID type that works with SQLite class GUID(TypeDecorator): @@ -58,11 +73,16 @@ class User(Base): onupdate=lambda: datetime.now(timezone.utc), nullable=False, ) + orcid_id = Column(String(20), nullable=True, unique=True, index=True) # Relationships scrolls = relationship("Scroll", back_populates="user") sessions = relationship("Session", back_populates="user") tokens = relationship("Token", back_populates="user") + @validates("orcid_id") + def _validate_orcid_id(self, _key: str, value: str | None) -> str | None: + return validate_orcid_id(value) + def __repr__(self): return f"" diff --git a/tests/test_orcid.py b/tests/test_orcid.py new file mode 100644 index 0000000..f40283d --- /dev/null +++ b/tests/test_orcid.py @@ -0,0 +1,154 @@ +"""Tests for ORCID iD storage on the User model.""" + +import pytest +import pytest_asyncio +from sqlalchemy import select +from sqlalchemy.exc import IntegrityError + +from app.models.user import User, validate_orcid_id + + +class TestValidateOrcidId: + """Tests for the orcid_id format validation function.""" + + def test_valid_orcid(self): + assert validate_orcid_id("0000-0002-1234-5678") == "0000-0002-1234-5678" + + def test_valid_orcid_with_checksum_x(self): + assert validate_orcid_id("0000-0002-1234-567X") == "0000-0002-1234-567X" + + def test_none_passes_through(self): + assert validate_orcid_id(None) is None + + def test_rejects_missing_hyphens(self): + with pytest.raises(ValueError, match="ORCID"): + validate_orcid_id("0000000212345678") + + def test_rejects_too_short(self): + with pytest.raises(ValueError, match="ORCID"): + validate_orcid_id("0000-0002-1234") + + def test_rejects_letters_in_body(self): + with pytest.raises(ValueError, match="ORCID"): + validate_orcid_id("000A-0002-1234-5678") + + def test_rejects_empty_string(self): + with pytest.raises(ValueError, match="ORCID"): + validate_orcid_id("") + + def test_rejects_lowercase_x(self): + with pytest.raises(ValueError, match="ORCID"): + validate_orcid_id("0000-0002-1234-567x") + + +@pytest.mark.asyncio +class TestUserOrcidColumn: + """Tests for the orcid_id column on the User model.""" + + async def test_user_created_without_orcid(self, test_db): + """Backward compat: users can be created with no orcid_id.""" + from app.auth.utils import get_password_hash + + user = User( + email="noorcid@example.com", + password_hash=get_password_hash("password123"), + display_name="No ORCID User", + email_verified=True, + ) + test_db.add(user) + await test_db.commit() + await test_db.refresh(user) + + assert user.orcid_id is None + + async def test_user_created_with_valid_orcid(self, test_db): + from app.auth.utils import get_password_hash + + user = User( + email="orcid@example.com", + password_hash=get_password_hash("password123"), + display_name="ORCID User", + email_verified=True, + orcid_id="0000-0002-1234-5678", + ) + test_db.add(user) + await test_db.commit() + await test_db.refresh(user) + + assert user.orcid_id == "0000-0002-1234-5678" + + async def test_orcid_with_checksum_x(self, test_db): + from app.auth.utils import get_password_hash + + user = User( + email="orcidx@example.com", + password_hash=get_password_hash("password123"), + display_name="ORCID X User", + email_verified=True, + orcid_id="0000-0002-1234-567X", + ) + test_db.add(user) + await test_db.commit() + await test_db.refresh(user) + + assert user.orcid_id == "0000-0002-1234-567X" + + async def test_orcid_uniqueness_constraint(self, test_db): + """Two users cannot share the same ORCID iD.""" + from app.auth.utils import get_password_hash + + user1 = User( + email="user1@example.com", + password_hash=get_password_hash("password123"), + display_name="User 1", + email_verified=True, + orcid_id="0000-0002-1234-5678", + ) + test_db.add(user1) + await test_db.commit() + + user2 = User( + email="user2@example.com", + password_hash=get_password_hash("password123"), + display_name="User 2", + email_verified=True, + orcid_id="0000-0002-1234-5678", + ) + test_db.add(user2) + + with pytest.raises(IntegrityError): + await test_db.commit() + + async def test_multiple_users_with_null_orcid(self, test_db): + """Multiple users can have NULL orcid_id (no false unique violation).""" + from app.auth.utils import get_password_hash + + for i in range(3): + user = User( + email=f"null_orcid_{i}@example.com", + password_hash=get_password_hash("password123"), + display_name=f"Null ORCID {i}", + email_verified=True, + ) + test_db.add(user) + + await test_db.commit() + + result = await test_db.execute( + select(User).where(User.orcid_id.is_(None)) + ) + users = result.scalars().all() + assert len(users) == 3 + + async def test_invalid_orcid_rejected_by_validator(self, test_db): + """The @validates decorator rejects bad formats before hitting the DB.""" + from app.auth.utils import get_password_hash + + with pytest.raises(ValueError, match="ORCID"): + User( + email="bad@example.com", + password_hash=get_password_hash("password123"), + display_name="Bad ORCID", + email_verified=True, + orcid_id="not-an-orcid", + ) From 5bdd80257944b49582493078a3fbfb11f06f8228 Mon Sep 17 00:00:00 2001 From: Leo Torres Date: Sat, 28 Mar 2026 07:39:17 +0100 Subject: [PATCH 2/7] Fix duplicate Alembic revision ID for orcid_id migration 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) --- ...d_id_to_users.py => 596bb368fc0d_add_orcid_id_to_users.py} | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) rename alembic/versions/{a1b2c3d4e5f6_add_orcid_id_to_users.py => 596bb368fc0d_add_orcid_id_to_users.py} (92%) diff --git a/alembic/versions/a1b2c3d4e5f6_add_orcid_id_to_users.py b/alembic/versions/596bb368fc0d_add_orcid_id_to_users.py similarity index 92% rename from alembic/versions/a1b2c3d4e5f6_add_orcid_id_to_users.py rename to alembic/versions/596bb368fc0d_add_orcid_id_to_users.py index 421197a..6837c41 100644 --- a/alembic/versions/a1b2c3d4e5f6_add_orcid_id_to_users.py +++ b/alembic/versions/596bb368fc0d_add_orcid_id_to_users.py @@ -1,6 +1,6 @@ """Add orcid_id column to users -Revision ID: a1b2c3d4e5f6 +Revision ID: 596bb368fc0d Revises: d3a7b8c1e2f4 Create Date: 2026-03-28 @@ -12,7 +12,7 @@ from alembic import op -revision: str = "a1b2c3d4e5f6" +revision: str = "596bb368fc0d" down_revision: Union[str, Sequence[str], None] = "d3a7b8c1e2f4" branch_labels: Union[str, Sequence[str], None] = None depends_on: Union[str, Sequence[str], None] = None From 7063f1c346587e52e8738db44fdd5875a6926d72 Mon Sep 17 00:00:00 2001 From: Leo Torres Date: Sat, 28 Mar 2026 20:17:41 +0100 Subject: [PATCH 3/7] Add ORCID OAuth2 authentication flow (login, register, link, unlink) 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) --- app/routes/orcid.py | 201 +++++++++++++++++++++++++ main.py | 5 +- tests/test_orcid_oauth.py | 308 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 512 insertions(+), 2 deletions(-) create mode 100644 app/routes/orcid.py create mode 100644 tests/test_orcid_oauth.py diff --git a/app/routes/orcid.py b/app/routes/orcid.py new file mode 100644 index 0000000..431ec04 --- /dev/null +++ b/app/routes/orcid.py @@ -0,0 +1,201 @@ +"""ORCID OAuth2 authentication routes.""" + +import os +import secrets + +from fastapi import APIRouter, Depends, Request +from fastapi.responses import RedirectResponse +import httpx +from sqlalchemy import select +from sqlalchemy.ext.asyncio import AsyncSession + +from app.auth.session import create_session, get_current_user_from_session +from app.database import get_db +from app.logging_config import get_logger +from app.models.user import User + +router = APIRouter(prefix="/auth/orcid") + +IS_PRODUCTION = os.getenv("ENVIRONMENT") == "production" + +ORCID_CLIENT_ID = os.getenv("ORCID_CLIENT_ID", "") +ORCID_CLIENT_SECRET = os.getenv("ORCID_CLIENT_SECRET", "") +ORCID_BASE_URL = os.getenv("ORCID_BASE_URL", "https://sandbox.orcid.org") + +# Pending OAuth states: state_token -> True +# Short-lived, cleared on use. In production, use Redis/DB before horizontal scaling. +_pending_states: dict[str, bool] = {} + + +def _get_redirect_uri(request: Request) -> str: + """Build the ORCID callback URI from the current request.""" + return str(request.url_for("orcid_callback")) + + +@router.get("", name="orcid_redirect") +async def orcid_redirect(request: Request): + """Redirect to ORCID authorize URL with CSRF state.""" + state = secrets.token_urlsafe(32) + _pending_states[state] = True + + redirect_uri = _get_redirect_uri(request) + authorize_url = ( + f"{ORCID_BASE_URL}/oauth/authorize" + f"?client_id={ORCID_CLIENT_ID}" + f"&response_type=code" + f"&scope=/authenticate" + f"&redirect_uri={redirect_uri}" + f"&state={state}" + ) + + response = RedirectResponse(url=authorize_url, status_code=302) + response.set_cookie( + "orcid_state", state, httponly=True, secure=IS_PRODUCTION, + samesite="lax", max_age=600, + ) + return response + + +@router.get("/callback", name="orcid_callback") +async def orcid_callback( + request: Request, + code: str | None = None, + state: str | None = None, + db: AsyncSession = Depends(get_db), +): + """Handle ORCID OAuth2 callback.""" + logger = get_logger() + + # Validate state + cookie_state = request.cookies.get("orcid_state") + if not state or not cookie_state or state != cookie_state or state not in _pending_states: + logger.warning("ORCID callback: invalid or missing state") + return RedirectResponse(url="/login?error=orcid_state", status_code=302) + + _pending_states.pop(state, None) + + if not code: + logger.warning("ORCID callback: missing code") + return RedirectResponse(url="/login?error=orcid_missing_code", status_code=302) + + # Exchange code for token + redirect_uri = _get_redirect_uri(request) + try: + async with httpx.AsyncClient() as client: + token_resp = await client.post( + f"{ORCID_BASE_URL}/oauth/token", + data={ + "client_id": ORCID_CLIENT_ID, + "client_secret": ORCID_CLIENT_SECRET, + "grant_type": "authorization_code", + "code": code, + "redirect_uri": redirect_uri, + }, + headers={"Accept": "application/json"}, + ) + + if token_resp.status_code != 200: + logger.error(f"ORCID token exchange failed: {token_resp.status_code}") + return RedirectResponse(url="/login?error=orcid_token", status_code=302) + + token_data = token_resp.json() + except Exception as e: + logger.error(f"ORCID token exchange error: {e}") + return RedirectResponse(url="/login?error=orcid_token", status_code=302) + + orcid_id = token_data.get("orcid") + orcid_name = token_data.get("name", "") + + if not orcid_id: + logger.error("ORCID token response missing orcid field") + return RedirectResponse(url="/login?error=orcid_token", status_code=302) + + current_user = await get_current_user_from_session(request, db) + + if current_user: + return await _link_orcid(db, current_user, orcid_id) + + return await _login_or_register(db, orcid_id, orcid_name) + + +async def _link_orcid(db: AsyncSession, user: User, orcid_id: str) -> RedirectResponse: + """Link ORCID to an existing logged-in user.""" + logger = get_logger() + + # Check if ORCID is already taken by another user + result = await db.execute(select(User).where(User.orcid_id == orcid_id)) + existing = result.scalar_one_or_none() + if existing and existing.id != user.id: + logger.warning(f"ORCID {orcid_id} already linked to user {existing.id}") + return RedirectResponse(url="/settings?error=orcid_taken", status_code=302) + + user.orcid_id = orcid_id + await db.commit() + logger.info(f"Linked ORCID {orcid_id} to user {user.id}") + return RedirectResponse(url="/settings?orcid=linked", status_code=302) + + +async def _login_or_register( + db: AsyncSession, orcid_id: str, orcid_name: str, +) -> RedirectResponse: + """Log in existing ORCID user or create a new account.""" + logger = get_logger() + + result = await db.execute(select(User).where(User.orcid_id == orcid_id)) + user = result.scalar_one_or_none() + + if user: + session_id = await create_session(db, user.id) + logger.info(f"ORCID login for user {user.id}") + response = RedirectResponse(url="/dashboard", status_code=302) + response.set_cookie( + "session_id", session_id, httponly=True, + secure=IS_PRODUCTION, samesite="lax", + ) + return response + + # Create new user + # Generate a placeholder email using ORCID (users can update it later) + placeholder_email = f"{orcid_id}@orcid.placeholder" + display_name = orcid_name.strip() if orcid_name else f"ORCID User {orcid_id[-4:]}" + + new_user = User( + email=placeholder_email, + password_hash="!orcid-only", + display_name=display_name, + email_verified=True, + orcid_id=orcid_id, + ) + db.add(new_user) + await db.commit() + await db.refresh(new_user) + + session_id = await create_session(db, new_user.id) + logger.info(f"Created new user {new_user.id} via ORCID {orcid_id}") + + response = RedirectResponse(url="/dashboard", status_code=302) + response.set_cookie( + "session_id", session_id, httponly=True, + secure=IS_PRODUCTION, samesite="lax", + ) + return response + + +@router.get("/unlink", name="orcid_unlink") +async def orcid_unlink(request: Request, db: AsyncSession = Depends(get_db)): + """Remove ORCID from the current user's account.""" + current_user = await get_current_user_from_session(request, db) + if not current_user: + return RedirectResponse(url="/login", status_code=302) + + logger = get_logger() + + # Block unlink if user has no password (would lock them out) + if not current_user.password_hash or current_user.password_hash == "!orcid-only": + logger.warning(f"User {current_user.id} tried to unlink ORCID without password") + return RedirectResponse(url="/settings?error=orcid_no_password", status_code=302) + + current_user.orcid_id = None + await db.commit() + logger.info(f"Unlinked ORCID from user {current_user.id}") + return RedirectResponse(url="/settings?orcid=unlinked", status_code=302) diff --git a/main.py b/main.py index 8092f81..1b6b306 100644 --- a/main.py +++ b/main.py @@ -14,12 +14,12 @@ from sentry_sdk.integrations.asyncio import AsyncioIntegration from sentry_sdk.integrations.fastapi import FastApiIntegration from sentry_sdk.integrations.sqlalchemy import SqlalchemyIntegration +from starlette.exceptions import HTTPException as StarletteHTTPException from app.exception_handlers import ( http_exception_handler, internal_server_error_handler, ) -from starlette.exceptions import HTTPException as StarletteHTTPException from app.logging_config import get_logger from app.memory_profiling_middleware import MemoryProfilingMiddleware from app.middleware import ( @@ -31,7 +31,7 @@ SecurityHeadersMiddleware, StaticFilesCacheMiddleware, ) -from app.routes import api, auth, main, scrolls +from app.routes import api, auth, main, orcid, scrolls from app.security.nonce_middleware import NonceMiddleware from app.sentry_config import before_send @@ -226,3 +226,4 @@ async def health_check(): app.include_router(auth.router) app.include_router(scrolls.router) app.include_router(api.router) +app.include_router(orcid.router) diff --git a/tests/test_orcid_oauth.py b/tests/test_orcid_oauth.py new file mode 100644 index 0000000..c0ea057 --- /dev/null +++ b/tests/test_orcid_oauth.py @@ -0,0 +1,308 @@ +"""Tests for ORCID OAuth2 authentication flow.""" + +from unittest.mock import AsyncMock, MagicMock, patch +from urllib.parse import parse_qs, urlparse + +import pytest +import pytest_asyncio +from sqlalchemy import select + +from app.auth.utils import get_password_hash +from app.models.user import User + +FAKE_ORCID = "0000-0002-1234-5678" +FAKE_ORCID_2 = "0000-0002-9999-0001" + + +@pytest.fixture(autouse=True) +def orcid_env(monkeypatch): + """Set ORCID env vars and reset module-level state for each test.""" + monkeypatch.setenv("ORCID_CLIENT_ID", "APP-TESTCLIENTID") + monkeypatch.setenv("ORCID_CLIENT_SECRET", "test-secret") + monkeypatch.setenv("ORCID_BASE_URL", "https://sandbox.orcid.org") + + import app.routes.orcid as orcid_mod + + monkeypatch.setattr(orcid_mod, "ORCID_CLIENT_ID", "APP-TESTCLIENTID") + monkeypatch.setattr(orcid_mod, "ORCID_CLIENT_SECRET", "test-secret") + monkeypatch.setattr(orcid_mod, "ORCID_BASE_URL", "https://sandbox.orcid.org") + orcid_mod._pending_states.clear() + yield + orcid_mod._pending_states.clear() + + +@pytest_asyncio.fixture +async def orcid_user(test_db): + """A user who already has an ORCID linked.""" + user = User( + email="orcid-linked@example.com", + password_hash=get_password_hash("password123"), + display_name="ORCID User", + email_verified=True, + orcid_id=FAKE_ORCID, + ) + test_db.add(user) + await test_db.commit() + await test_db.refresh(user) + return user + + +@pytest_asyncio.fixture +async def passwordless_orcid_user(test_db): + """A user created via ORCID login (no real password).""" + user = User( + email="orcid-only@example.com", + password_hash="!orcid-only", + display_name="ORCID Only User", + email_verified=True, + orcid_id=FAKE_ORCID_2, + ) + test_db.add(user) + await test_db.commit() + await test_db.refresh(user) + return user + + +def _mock_orcid_token_response(orcid_id=FAKE_ORCID, name="Jane Doe"): + """Build a mock httpx response for ORCID token exchange.""" + mock_resp = MagicMock() + mock_resp.status_code = 200 + mock_resp.json.return_value = { + "access_token": "fake-access-token", + "token_type": "bearer", + "orcid": orcid_id, + "name": name, + } + return mock_resp + + +def _mock_orcid_token_error(): + mock_resp = MagicMock() + mock_resp.status_code = 401 + mock_resp.json.return_value = {"error": "invalid_grant"} + return mock_resp + + +@pytest.mark.asyncio +class TestOrcidRedirect: + """GET /auth/orcid should redirect to ORCID authorize URL.""" + + async def test_redirects_to_orcid(self, client): + resp = await client.get("/auth/orcid", follow_redirects=False) + assert resp.status_code == 302 + + location = resp.headers["location"] + parsed = urlparse(location) + assert "orcid.org" in parsed.netloc + assert parsed.path == "/oauth/authorize" + + params = parse_qs(parsed.query) + assert params["response_type"] == ["code"] + assert params["scope"] == ["/authenticate"] + assert "redirect_uri" in params + assert "state" in params + assert "client_id" in params + + async def test_state_param_is_random(self, client): + """Each redirect should generate a unique state.""" + resp1 = await client.get("/auth/orcid", follow_redirects=False) + resp2 = await client.get("/auth/orcid", follow_redirects=False) + + state1 = parse_qs(urlparse(resp1.headers["location"]).query)["state"][0] + state2 = parse_qs(urlparse(resp2.headers["location"]).query)["state"][0] + assert state1 != state2 + + +@pytest.mark.asyncio +class TestOrcidCallback: + """GET /auth/orcid/callback tests.""" + + async def test_rejects_missing_state(self, client): + resp = await client.get("/auth/orcid/callback?code=abc", follow_redirects=False) + assert resp.status_code == 302 + assert "/login" in resp.headers["location"] + + async def test_rejects_invalid_state(self, client): + # First, hit /auth/orcid to set a state in session + await client.get("/auth/orcid", follow_redirects=False) + + resp = await client.get( + "/auth/orcid/callback?code=abc&state=wrong-state", follow_redirects=False + ) + assert resp.status_code == 302 + assert "/login" in resp.headers["location"] + + async def test_rejects_missing_code(self, client): + # Get valid state + redir = await client.get("/auth/orcid", follow_redirects=False) + state = parse_qs(urlparse(redir.headers["location"]).query)["state"][0] + + resp = await client.get( + f"/auth/orcid/callback?state={state}", follow_redirects=False + ) + assert resp.status_code == 302 + assert "/login" in resp.headers["location"] + + async def test_login_existing_orcid_user(self, client, orcid_user, test_db): + """Callback with known ORCID logs in existing user.""" + redir = await client.get("/auth/orcid", follow_redirects=False) + state = parse_qs(urlparse(redir.headers["location"]).query)["state"][0] + + mock_resp = _mock_orcid_token_response(orcid_id=FAKE_ORCID) + with patch("app.routes.orcid.httpx.AsyncClient") as MockClient: + MockClient.return_value.__aenter__ = AsyncMock(return_value=MockClient.return_value) + MockClient.return_value.__aexit__ = AsyncMock(return_value=False) + MockClient.return_value.post = AsyncMock(return_value=mock_resp) + + resp = await client.get( + f"/auth/orcid/callback?code=valid-code&state={state}", + follow_redirects=False, + ) + + assert resp.status_code == 302 + assert "/dashboard" in resp.headers["location"] + assert "session_id" in resp.cookies + + async def test_creates_new_user_for_unknown_orcid(self, client, test_db): + """Callback with unknown ORCID creates a new account.""" + redir = await client.get("/auth/orcid", follow_redirects=False) + state = parse_qs(urlparse(redir.headers["location"]).query)["state"][0] + + mock_resp = _mock_orcid_token_response(orcid_id="0000-0003-0000-0001", name="New Researcher") + with patch("app.routes.orcid.httpx.AsyncClient") as MockClient: + MockClient.return_value.__aenter__ = AsyncMock(return_value=MockClient.return_value) + MockClient.return_value.__aexit__ = AsyncMock(return_value=False) + MockClient.return_value.post = AsyncMock(return_value=mock_resp) + + resp = await client.get( + f"/auth/orcid/callback?code=valid-code&state={state}", + follow_redirects=False, + ) + + assert resp.status_code == 302 + assert "/dashboard" in resp.headers["location"] + + # Verify user was created + result = await test_db.execute( + select(User).where(User.orcid_id == "0000-0003-0000-0001") + ) + user = result.scalar_one_or_none() + assert user is not None + assert user.display_name == "New Researcher" + assert user.email_verified is True + + async def test_links_orcid_when_logged_in(self, authenticated_client, test_user, test_db): + """Callback when logged in links ORCID to current account.""" + redir = await authenticated_client.get("/auth/orcid", follow_redirects=False) + state = parse_qs(urlparse(redir.headers["location"]).query)["state"][0] + + mock_resp = _mock_orcid_token_response(orcid_id="0000-0003-5555-6666", name="Ignored Name") + with patch("app.routes.orcid.httpx.AsyncClient") as MockClient: + MockClient.return_value.__aenter__ = AsyncMock(return_value=MockClient.return_value) + MockClient.return_value.__aexit__ = AsyncMock(return_value=False) + MockClient.return_value.post = AsyncMock(return_value=mock_resp) + + resp = await authenticated_client.get( + f"/auth/orcid/callback?code=valid-code&state={state}", + follow_redirects=False, + ) + + assert resp.status_code == 302 + + # Verify ORCID was linked + await test_db.refresh(test_user) + assert test_user.orcid_id == "0000-0003-5555-6666" + + async def test_token_exchange_failure(self, client): + """Callback handles ORCID token exchange failure.""" + redir = await client.get("/auth/orcid", follow_redirects=False) + state = parse_qs(urlparse(redir.headers["location"]).query)["state"][0] + + mock_resp = _mock_orcid_token_error() + with patch("app.routes.orcid.httpx.AsyncClient") as MockClient: + MockClient.return_value.__aenter__ = AsyncMock(return_value=MockClient.return_value) + MockClient.return_value.__aexit__ = AsyncMock(return_value=False) + MockClient.return_value.post = AsyncMock(return_value=mock_resp) + + resp = await client.get( + f"/auth/orcid/callback?code=bad-code&state={state}", + follow_redirects=False, + ) + + assert resp.status_code == 302 + assert "/login" in resp.headers["location"] + + async def test_duplicate_orcid_link_rejected(self, client, orcid_user, test_db): + """Cannot link an ORCID that already belongs to another user.""" + # Create and authenticate a second user + from app.auth.session import create_session + + user2 = User( + email="user2@example.com", + password_hash=get_password_hash("password123"), + display_name="User Two", + email_verified=True, + ) + test_db.add(user2) + await test_db.commit() + await test_db.refresh(user2) + + session_id = await create_session(test_db, user2.id) + client.cookies.set("session_id", session_id) + + redir = await client.get("/auth/orcid", follow_redirects=False) + state = parse_qs(urlparse(redir.headers["location"]).query)["state"][0] + + # Try to link orcid_user's ORCID + mock_resp = _mock_orcid_token_response(orcid_id=FAKE_ORCID) + with patch("app.routes.orcid.httpx.AsyncClient") as MockClient: + MockClient.return_value.__aenter__ = AsyncMock(return_value=MockClient.return_value) + MockClient.return_value.__aexit__ = AsyncMock(return_value=False) + MockClient.return_value.post = AsyncMock(return_value=mock_resp) + + resp = await client.get( + f"/auth/orcid/callback?code=valid-code&state={state}", + follow_redirects=False, + ) + + assert resp.status_code == 302 + # Should redirect with an error, not link + assert "/settings" in resp.headers["location"] or "/login" in resp.headers["location"] + + # ORCID should NOT be linked to user2 + await test_db.refresh(user2) + assert user2.orcid_id is None + + +@pytest.mark.asyncio +class TestOrcidUnlink: + """GET /auth/orcid/unlink tests.""" + + async def test_unlink_requires_auth(self, client): + resp = await client.get("/auth/orcid/unlink", follow_redirects=False) + assert resp.status_code == 302 + assert "/login" in resp.headers["location"] + + async def test_unlink_removes_orcid(self, authenticated_client, test_user, test_db): + """Unlink removes orcid_id when user has a password.""" + test_user.orcid_id = FAKE_ORCID + await test_db.commit() + + resp = await authenticated_client.get("/auth/orcid/unlink", follow_redirects=False) + assert resp.status_code == 302 + + await test_db.refresh(test_user) + assert test_user.orcid_id is None + + async def test_unlink_blocked_without_password(self, client, passwordless_orcid_user, test_db): + """Cannot unlink ORCID if user has no password (would be locked out).""" + from app.auth.session import create_session + + session_id = await create_session(test_db, passwordless_orcid_user.id) + client.cookies.set("session_id", session_id) + + resp = await client.get("/auth/orcid/unlink", follow_redirects=False) + assert resp.status_code == 302 + # ORCID should still be linked + await test_db.refresh(passwordless_orcid_user) + assert passwordless_orcid_user.orcid_id == FAKE_ORCID_2 From bdb8850c9d99262c64ecdbdeea17bab84a46a8ac Mon Sep 17 00:00:00 2001 From: Leo Torres Date: Sat, 28 Mar 2026 21:09:27 +0100 Subject: [PATCH 4/7] Add ORCID sign-in buttons to login, register, and dashboard pages 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) --- app/routes/orcid.py | 8 ++--- app/templates/auth/login.html | 11 ++++++ app/templates/auth/register.html | 11 ++++++ app/templates/dashboard.html | 24 +++++++++++++ app/templates_config.py | 2 ++ static/css/main.css | 58 ++++++++++++++++++++++++++++++-- tests/test_orcid_oauth.py | 41 +++++++++++++++++++++- 7 files changed, 148 insertions(+), 7 deletions(-) diff --git a/app/routes/orcid.py b/app/routes/orcid.py index 431ec04..44ba3e7 100644 --- a/app/routes/orcid.py +++ b/app/routes/orcid.py @@ -127,12 +127,12 @@ async def _link_orcid(db: AsyncSession, user: User, orcid_id: str) -> RedirectRe existing = result.scalar_one_or_none() if existing and existing.id != user.id: logger.warning(f"ORCID {orcid_id} already linked to user {existing.id}") - return RedirectResponse(url="/settings?error=orcid_taken", status_code=302) + return RedirectResponse(url="/dashboard?error=orcid_taken", status_code=302) user.orcid_id = orcid_id await db.commit() logger.info(f"Linked ORCID {orcid_id} to user {user.id}") - return RedirectResponse(url="/settings?orcid=linked", status_code=302) + return RedirectResponse(url="/dashboard?orcid=linked", status_code=302) async def _login_or_register( @@ -193,9 +193,9 @@ async def orcid_unlink(request: Request, db: AsyncSession = Depends(get_db)): # Block unlink if user has no password (would lock them out) if not current_user.password_hash or current_user.password_hash == "!orcid-only": logger.warning(f"User {current_user.id} tried to unlink ORCID without password") - return RedirectResponse(url="/settings?error=orcid_no_password", status_code=302) + return RedirectResponse(url="/dashboard?error=orcid_no_password", status_code=302) current_user.orcid_id = None await db.commit() logger.info(f"Unlinked ORCID from user {current_user.id}") - return RedirectResponse(url="/settings?orcid=unlinked", status_code=302) + return RedirectResponse(url="/dashboard?orcid=unlinked", status_code=302) diff --git a/app/templates/auth/login.html b/app/templates/auth/login.html index 5ec2cb8..171f953 100644 --- a/app/templates/auth/login.html +++ b/app/templates/auth/login.html @@ -11,6 +11,17 @@

Sign In

{% include "auth/partials/login_form.html" %} + {% if orcid_client_id %} +
or
+ + + Sign in with ORCID + + {% endif %} +

Forgot your password?

Don't have an account? Create one here

diff --git a/app/templates/auth/register.html b/app/templates/auth/register.html index 02e0f02..2d9a3bc 100644 --- a/app/templates/auth/register.html +++ b/app/templates/auth/register.html @@ -17,6 +17,17 @@

Create Your Account

{% include "auth/partials/register_form.html" %} + {% if orcid_client_id %} +
or
+ + + Sign up with ORCID + + {% endif %} +

Already have an account? Sign in here

diff --git a/app/templates/dashboard.html b/app/templates/dashboard.html index 034a87e..a7f926d 100644 --- a/app/templates/dashboard.html +++ b/app/templates/dashboard.html @@ -90,6 +90,30 @@

No published papers yet

{% endif %}
+ {% if orcid_client_id %} + + + {% endif %} + + {% if error %} + + {% endif %} + {% include "auth/partials/register_form.html" %}
or
diff --git a/app/templates/dashboard.html b/app/templates/dashboard.html index d768cf4..a41799e 100644 --- a/app/templates/dashboard.html +++ b/app/templates/dashboard.html @@ -100,6 +100,16 @@

No published papers yet