From cf2f42975ef8438f3e1a34a600807e73d4c1a6b4 Mon Sep 17 00:00:00 2001 From: Daniel Date: Sun, 13 Sep 2026 13:56:48 +0200 Subject: [PATCH] =?UTF-8?q?feat:=20no=20invite=20codes=20and=20no=20email?= =?UTF-8?q?=20sign-in=20codes=20=E2=80=94=20that=20is=20the=20provider's?= =?UTF-8?q?=20job?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both were this app doing an identity provider's work. Sign-in and sign-up happen at sso.pedshub.com now: it takes the address, sends the code, checks it, and knows about second factors — none of which belongs here, and two of which were never done here at all. Gone: services/invites.py, services/login_codes.py, routers/login_code.py, the two models, the three admin invite routes, the invite_only flag and its switch, the invite field on both sign-up forms, and the code half of the sign-in page — which was the primary way in and is now a button that says "Sign in with PedsHub SSO". The password form stays for a site with no provider configured. Migration r7b8c9d0e1f2 drops invite_codes (three spent rows) and login_codes (empty). The dump beside it has both. 585 tests, and the contract snapshot is 320 routes — five fewer, all five named in the diff so the removal is reviewable rather than discovered later by a client. Also: "Make a deck" in the signed-in menu and on the landing page, going to the scribe's My Resources at app.pedshub.com/#resources. Same sign-in on both sides; the arrow says it leaves. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TqXevQJhxFrM7jJg82cgZN --- .../versions/r7b8c9d0e1f2_drop_code_tables.py | 31 ++ backend/app/main.py | 2 - backend/app/models/__init__.py | 2 - backend/app/models/invite.py | 28 -- backend/app/models/login_code.py | 37 --- backend/app/routers/admin.py | 47 +-- backend/app/routers/auth.py | 21 +- backend/app/routers/login_code.py | 168 ---------- backend/app/schemas/auth.py | 11 - backend/app/services/invites.py | 67 ---- backend/app/services/login_codes.py | 159 ---------- backend/app/services/site_settings.py | 2 - backend/tests/api-contract.json | 41 --- backend/tests/test_invites_and_flags.py | 125 -------- backend/tests/test_login_codes.py | 290 ------------------ frontend/src/components/Navbar.jsx | 25 +- frontend/src/components/SitePolicy.jsx | 88 +----- frontend/src/pages/LandingPage.jsx | 27 +- frontend/src/pages/LoginCaptcha.test.jsx | 9 +- frontend/src/pages/LoginCode.test.jsx | 128 -------- frontend/src/pages/LoginPage.jsx | 143 +++------ frontend/src/pages/RegisterInvite.test.jsx | 68 ---- frontend/src/pages/RegisterPage.jsx | 25 +- 23 files changed, 107 insertions(+), 1437 deletions(-) create mode 100644 backend/alembic/versions/r7b8c9d0e1f2_drop_code_tables.py delete mode 100644 backend/app/models/invite.py delete mode 100644 backend/app/models/login_code.py delete mode 100644 backend/app/routers/login_code.py delete mode 100644 backend/app/services/invites.py delete mode 100644 backend/app/services/login_codes.py delete mode 100644 backend/tests/test_invites_and_flags.py delete mode 100644 backend/tests/test_login_codes.py delete mode 100644 frontend/src/pages/LoginCode.test.jsx delete mode 100644 frontend/src/pages/RegisterInvite.test.jsx diff --git a/backend/alembic/versions/r7b8c9d0e1f2_drop_code_tables.py b/backend/alembic/versions/r7b8c9d0e1f2_drop_code_tables.py new file mode 100644 index 0000000..a6e9902 --- /dev/null +++ b/backend/alembic/versions/r7b8c9d0e1f2_drop_code_tables.py @@ -0,0 +1,31 @@ +"""Sign-up codes and email sign-in codes, gone. + +Both were the app doing an identity provider's job. Sign-in is through +sso.pedshub.com now: it takes the email, sends the code, checks it, and knows +about second factors — none of which this app should be reimplementing, and +two of which it never did. + +invite_codes held three rows, all of them spent or stale; login_codes held +none. The dump taken beside this change has both if anybody ever wants to know +who was invited. + +Revision ID: r7b8c9d0e1f2 +Revises: q6a7b8c9d0e1 +""" +from alembic import op + +revision = "r7b8c9d0e1f2" +down_revision = "q6a7b8c9d0e1" +branch_labels = None +depends_on = None + + +def upgrade(): + op.execute("DROP TABLE IF EXISTS login_codes") + op.execute("DROP TABLE IF EXISTS invite_codes") + + +def downgrade(): + # The tables can be recreated from the models in git history; their + # contents cannot, and inventing an invite is worse than not having one. + pass diff --git a/backend/app/main.py b/backend/app/main.py index 4435697..4bdc985 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -17,7 +17,6 @@ from app.routers import access from app.routers import feedback from app.routers import folders from app.routers import study_tools, uploads, articles, share, collections, study_plans, media, search, ai_mode, drafts, public -from app.routers import login_code from app.utils.auth import get_password_hash @@ -610,7 +609,6 @@ app.add_middleware(VersionAlias) app.include_router(uploads.router) app.include_router(auth.router, prefix=f"{VERSIONED_ROOT}/auth", tags=["auth"]) -app.include_router(login_code.router, prefix=f"{VERSIONED_ROOT}/auth", tags=["auth"]) # Counts the landing page states about itself. No auth: a stranger reads it. app.include_router(public.router, prefix=f"{VERSIONED_ROOT}/public", tags=["public"]) app.include_router(articles.router, prefix=f"{VERSIONED_ROOT}/articles", tags=["articles"]) diff --git a/backend/app/models/__init__.py b/backend/app/models/__init__.py index e7b5d7d..e242dfc 100644 --- a/backend/app/models/__init__.py +++ b/backend/app/models/__init__.py @@ -46,6 +46,4 @@ __all__ = [ from app.models.feedback import ArticleFeedback, QuestionFeedback # noqa: F401 from app.models.folder import QuestionFolder, QuestionFolderQuestion # noqa: F401 from app.models.user_note import ArticleSectionNote, QuestionNote # noqa: F401 -from app.models.invite import InviteCode # noqa: F401 from app.models.draft_question import DraftBatch, DraftQuestion # noqa: F401 -from app.models.login_code import LoginCode # noqa: F401 diff --git a/backend/app/models/invite.py b/backend/app/models/invite.py deleted file mode 100644 index f79c55c..0000000 --- a/backend/app/models/invite.py +++ /dev/null @@ -1,28 +0,0 @@ -from datetime import datetime - -from sqlalchemy import Column, DateTime, ForeignKey, Integer, String - -from app.database import Base - - -class InviteCode(Base): - """A code an administrator issues so one person can register. - - Single use by default: the point of invite-only is knowing who came in, and - a code that works forever is a password shared by everyone who has seen it. - A code is never deleted once used — who it let in is the record worth - keeping. - """ - - __tablename__ = "invite_codes" - - id = Column(Integer, primary_key=True, index=True) - code = Column(String(32), unique=True, nullable=False, index=True) - note = Column(String(200), nullable=True) # who it was meant for - created_by = Column(Integer, ForeignKey("users.id", ondelete="SET NULL"), nullable=True) - created_at = Column(DateTime, default=datetime.utcnow) - #: Set when someone registers with it. Present means spent. - used_by = Column(Integer, ForeignKey("users.id", ondelete="SET NULL"), nullable=True) - used_at = Column(DateTime, nullable=True) - #: An administrator can withdraw a code that has not been used. - revoked_at = Column(DateTime, nullable=True) diff --git a/backend/app/models/login_code.py b/backend/app/models/login_code.py deleted file mode 100644 index 5cd4672..0000000 --- a/backend/app/models/login_code.py +++ /dev/null @@ -1,37 +0,0 @@ -from datetime import datetime - -from sqlalchemy import Column, DateTime, ForeignKey, Integer, String - -from app.database import Base - - -class LoginCode(Base): - """One issued sign-in code. - - Only the fingerprint of the code is kept. A row here is a session waiting - to happen — anybody who could read the table could sign in as its owner — - so it holds something to compare against rather than something to type in. - - The failed guesses are counted in this row and not in Redis. The rate - limiter fails open when the cache is down, which is the right call for - anything it normally protects; it is the wrong call for the only thing - standing between a patient stranger and six characters. - """ - - __tablename__ = "login_codes" - - id = Column(Integer, primary_key=True, index=True) - user_id = Column(Integer, ForeignKey("users.id", ondelete="CASCADE"), nullable=False, index=True) - #: SHA-256 of the normalised code, hex. Not bcrypt: the guess budget is - #: what makes a short code safe, not the cost of testing one, and this is - #: compared on a path a person is waiting on. Not indexed and not unique - #: either — a code is only ever looked up against the one account it was - #: issued for, and six characters do collide. - code_hash = Column(String(64), nullable=False) - attempts = Column(Integer, nullable=False, default=0) - expires_at = Column(DateTime, nullable=False) - #: Set when the code is spent, when its guesses run out, and when a later - #: request retires it. All three mean the same thing to every reader here: - #: no longer usable. - consumed_at = Column(DateTime, nullable=True) - created_at = Column(DateTime, default=datetime.utcnow) diff --git a/backend/app/routers/admin.py b/backend/app/routers/admin.py index 7992e38..4bc2e79 100644 --- a/backend/app/routers/admin.py +++ b/backend/app/routers/admin.py @@ -13,8 +13,7 @@ from app.config import settings from app.database import get_db from app.models.user import User from app.models.ai_model_config import AIModelConfig -from app.models.invite import InviteCode -from app.services import ai_service, invites, site_settings, tts_voices +from app.services import ai_service, site_settings, tts_voices from app.schemas.auth import UserResponse, UserUpdateRole, UserCreate from app.schemas.admin import AIModelConfigCreate, AIModelConfigResponse, AIModelConfigUpdate from app.utils.auth import require_admin, get_current_user, get_password_hash @@ -560,50 +559,6 @@ def search_tts_voices( raise HTTPException(status_code=400, detail=f"Unknown provider '{provider}'. Valid: litellm") -# --- Invite codes --- - -class InviteIn(BaseModel): - note: str | None = Field(default=None, max_length=200) - - -@router.get("/invites") -def list_invites(db: Session = Depends(get_db), admin: User = Depends(require_admin)): - """Every code, newest first, with who it let in.""" - rows = db.query(InviteCode).order_by(InviteCode.created_at.desc()).limit(200).all() - users = {u.id: u for u in db.query(User).filter( - User.id.in_({r.used_by for r in rows if r.used_by}))} if rows else {} - return [invites.as_json(row, users) for row in rows] - - -@router.post("/invites", status_code=201) -def create_invite(data: InviteIn, db: Session = Depends(get_db), - admin: User = Depends(require_admin)): - row = invites.create(db, created_by=admin.id, note=data.note) - return invites.as_json(row, {}) - - -@router.delete("/invites/{invite_id}", status_code=204) -def revoke_invite(invite_id: int, db: Session = Depends(get_db), - admin: User = Depends(require_admin)): - """Withdraw an unused code, or clear away a spent one. - - An unused code is withdrawn — it stays listed, so it is clear that it was - issued and then stopped. A spent or already-withdrawn code has nothing left - to stop, and a list that only grows is a list nobody reads; removing it - loses who it let in, but that person has an account, which is the record - that matters. - """ - row = db.get(InviteCode, invite_id) - if not row: - raise HTTPException(404, "Invite not found") - if row.used_by is not None or row.revoked_at is not None: - db.delete(row) - db.commit() - return - row.revoked_at = datetime.utcnow() - db.commit() - - # --- System Settings --- @router.get("/settings") diff --git a/backend/app/routers/auth.py b/backend/app/routers/auth.py index 74f8692..70690d6 100644 --- a/backend/app/routers/auth.py +++ b/backend/app/routers/auth.py @@ -5,7 +5,7 @@ from datetime import datetime, timedelta from fastapi import APIRouter, Depends, HTTPException, BackgroundTasks, status, Request from sqlalchemy.orm import Session -from app.services import captcha, invites, refresh_tokens, site_settings +from app.services import captcha, refresh_tokens, site_settings from app.database import get_db from app.models.user import User from app.models.email_verification import EmailVerification @@ -116,19 +116,18 @@ def signup_policy(db: Session = Depends(get_db)): # The very first account is always allowed, or a new install could lock # itself out before an administrator exists to issue a code. if db.query(User).count() == 0: - return {"invite_required": False, "first_user": True, "registration_open": True} + return {"first_user": True, "registration_open": True} # Whether anyone may join at all, so the sign-in page can stop offering a # door that is locked. It was offering one: registration can be turned off # site-wide, and the only way to find out was to fill the form in and be # refused. # Single sign-on closes the password door, and this is the one question # the sign-up form already asks. Saying so here means the form can decline - # to draw itself rather than collect a name, an email, a password twice and - # an invite code, and then be refused by the POST. + # to draw itself rather than collect a name, an email and a password + # twice, and then be refused by the POST. from app.config import settings as cfg sso_only = _get_sso_settings()["sso_only"] - return {"invite_required": site_settings.get_flag("invite_only"), - "first_user": False, + return {"first_user": False, "sso_only": sso_only, # So the page can name the provider rather than say "single # sign-on" at somebody who only knows it as PedsHub SSO. @@ -161,14 +160,6 @@ async def register(user_data: UserCreate, background_tasks: BackgroundTasks, db: except Exception as e: import logging; logging.getLogger(__name__).warning(f"Redis registration check failed (failing open): {e}") - # Invite-only: a code is checked before anything is created, and spent only - # once the account exists, so a failure part-way through does not burn it. - invite = None - if not is_first_user and site_settings.get_flag("invite_only"): - invite = invites.usable(db, user_data.invite_code) - if invite is None: - raise HTTPException(403, "This site is invite-only. A valid invite code is required.") - if len(user_data.password) < 8: raise HTTPException(status_code=400, detail="Password must be at least 8 characters") @@ -195,8 +186,6 @@ async def register(user_data: UserCreate, background_tasks: BackgroundTasks, db: ) db.add(verification) db.commit() - if invite is not None: - invites.spend(db, invite, user) db.refresh(user) if is_first_user: diff --git a/backend/app/routers/login_code.py b/backend/app/routers/login_code.py deleted file mode 100644 index 7aa8143..0000000 --- a/backend/app/routers/login_code.py +++ /dev/null @@ -1,168 +0,0 @@ -"""Ask for a sign-in code, and type one in. - -Kept out of the auth router because these two routes answer to a rule the rest -of that file does not: whatever the state of the address given, the reply is -the same reply, and takes the same time to arrive. That is easy to break by -adding an early return next to routes that quite reasonably have several. -""" -import asyncio -import logging -import time - -from fastapi import APIRouter, BackgroundTasks, Depends, HTTPException, Request -from sqlalchemy.orm import Session - -from app.database import get_db -from app.models.email_verification import EmailVerification -from app.models.user import User -from app.schemas.auth import LoginCodeRequest, LoginCodeVerify, Token -from app.services import email_service, login_codes, site_settings -from app.utils.auth import check_rate_limit, create_access_token - -logger = logging.getLogger(__name__) - -router = APIRouter() - -#: The only thing the request endpoint ever says. Not "we sent you a code" and -#: not "no such account": either would turn the form into a way to ask the site -#: whether a given person is a member of it. -SENT = "If that address has an account, a sign-in code is on its way." - -#: And the only thing the verify endpoint says when it will not let somebody -#: in. Wrong code, expired code, code out of guesses, address with no account — -#: one answer, because the alternative is a way to sort guesses into warm and -#: cold. -REFUSED = "That code is not right, or it has expired. Request a new one." - -#: Wording and status are identical whichever way it went — but the work is -#: not, and a stopwatch reads the difference. Every reply from both routes is -#: held to this floor, comfortably above what either costs. Sending the mail is -#: a background task and runs after the response, so it never shows up in the -#: timing at all. -MIN_SECONDS = 0.4 - -#: Three per quarter hour, per address. This is the limit that matters when -#: issuing: the address is who the mail lands on, and the one thing an attacker -#: cannot vary while still reaching the person they mean to reach. -PER_ADDRESS_CALLS, PER_ADDRESS_WINDOW = 3, 15 * 60 - -#: Twenty per hour, per source address. Deliberately the looser of the two: a -#: hospital or university puts a whole department behind one address, so a -#: tight limit here locks out real people and barely inconveniences anyone -#: renting a hundred addresses. It protects the site's sending reputation from -#: one machine working through a word list; the per-address limit is what -#: protects any individual mailbox. -PER_IP_CALLS, PER_IP_WINDOW = 20, 60 * 60 - -#: Ten guesses a quarter hour against one address, on top of the five any one -#: code will answer. The code's own budget is the harder bound and survives the -#: cache being down; this one bounds what somebody gets by burning through -#: codes — three issued in a window, five guesses each, would otherwise be -#: fifteen. It is keyed by address rather than by network because a code only -#: means anything against the address it was issued for: limiting the network -#: would lock out a shared one while an attacker simply moved to the next -#: address. -PER_ADDRESS_TRIES, PER_ADDRESS_TRIES_WINDOW = 10, 15 * 60 - - -def _sso_only() -> bool: - return site_settings.get_flag("sso_only") - - -async def _hold(started: float): - elapsed = time.perf_counter() - started - if elapsed < MIN_SECONDS: - await asyncio.sleep(MIN_SECONDS - elapsed) - - -@router.post("/login-code") -async def request_login_code( - data: LoginCodeRequest, - background_tasks: BackgroundTasks, - request: Request = None, - db: Session = Depends(get_db), -): - """Send a sign-in code to an address, if that address has an account.""" - started = time.perf_counter() - - if _sso_only(): - raise HTTPException(status_code=403, detail="Email sign-in is disabled. Please use SSO.") - - address = data.email.lower().strip() - # Both limits are checked before the account is looked up, so that being - # throttled cannot itself become the tell about whether an account exists. - # The address is keyed by its fingerprint: Redis is a cache with a rather - # more relaxed life than the database, and it has no business holding a - # readable list of who has an account here. - check_rate_limit( - key=f"login_code:addr:{login_codes.fingerprint(address)}", - max_calls=PER_ADDRESS_CALLS, - window_seconds=PER_ADDRESS_WINDOW, - detail="Too many sign-in codes requested for that address. Please wait a few minutes.", - ) - client_ip = (request.client.host if request and request.client else "unknown") - check_rate_limit( - key=f"login_code:ip:{client_ip}", - max_calls=PER_IP_CALLS, - window_seconds=PER_IP_WINDOW, - detail="Too many sign-in codes requested from this network. Please wait a while.", - ) - - user = db.query(User).filter(User.email == address).first() - if user is not None: - code = login_codes.issue(db, user) - background_tasks.add_task(email_service.send_login_code_email, - user.email, user.name, login_codes.for_display(code)) - - await _hold(started) - return {"message": SENT} - - -@router.post("/login-code/verify", response_model=Token) -async def verify_login_code(data: LoginCodeVerify, db: Session = Depends(get_db)): - """Spend a code and hand back a session. - - The code is typed into the page that asked for it, so nothing has to travel - between devices, no credential ends up in a URL, and there is no history - entry to clear afterwards. - """ - started = time.perf_counter() - - if _sso_only(): - raise HTTPException(status_code=403, detail="Email sign-in is disabled. Please use SSO.") - - address = data.email.lower().strip() - # Before the lookup, and counted for an address with no account exactly as - # for one with — otherwise the 429 says which is which. - check_rate_limit( - key=f"login_code:try:{login_codes.fingerprint(address)}", - max_calls=PER_ADDRESS_TRIES, - window_seconds=PER_ADDRESS_TRIES_WINDOW, - detail="Too many attempts for that address. Please wait a few minutes and request a new code.", - ) - - user = login_codes.verify(db, address, data.code) - if user is None: - await _hold(started) - raise HTTPException(status_code=400, detail=REFUSED) - - # Checked after the code is spent, and deliberately: a refusal must not - # leave a usable credential behind. - # - # Reading a code out of the mailbox proves control of it, which is exactly - # what verification checks — and it is still not allowed to stand in for - # it. Somebody can register an address that is not theirs, choose a - # password and never verify; if the real owner's sign-in code marked that - # account verified, they would be signing in to an account the squatter - # already has the password to. The verification mail is in the same inbox. - verification = db.query(EmailVerification).filter(EmailVerification.user_id == user.id).first() - if verification and verification.verified_at is None: - await _hold(started) - raise HTTPException( - status_code=403, - detail="Email not verified. Please check your inbox and verify your email before signing in.", - ) - - logger.info("Sign-in code spent by user %s", user.id) - await _hold(started) - return Token(access_token=create_access_token(data={"sub": user.email})) diff --git a/backend/app/schemas/auth.py b/backend/app/schemas/auth.py index ce5cef9..eefa40f 100644 --- a/backend/app/schemas/auth.py +++ b/backend/app/schemas/auth.py @@ -9,8 +9,6 @@ class UserCreate(BaseModel): name: str #: Solved hCaptcha challenge. Absent when the site has no secret configured. captcha_token: str | None = None - #: Required only while the site is invite-only. - invite_code: str | None = None class UserResponse(BaseModel): @@ -101,12 +99,3 @@ class SsoExchangeRequest(BaseModel): code: str = Field(min_length=8, max_length=200) -class LoginCodeRequest(BaseModel): - email: EmailStr - - -class LoginCodeVerify(BaseModel): - email: EmailStr - #: Typed by a person, so it arrives in whatever shape they typed it and is - #: normalised before it is compared. - code: str diff --git a/backend/app/services/invites.py b/backend/app/services/invites.py deleted file mode 100644 index f68b64f..0000000 --- a/backend/app/services/invites.py +++ /dev/null @@ -1,67 +0,0 @@ -"""Invite codes: issuing them, checking one, and spending it. - -Kept out of the routers because two of them need it — registration spends a -code, administration issues them — and the rule for "usable" is the kind of -thing that must have exactly one definition. -""" -import secrets -from datetime import datetime - -from sqlalchemy.orm import Session - -from app.models.invite import InviteCode -from app.models.user import User - -#: Unambiguous when read aloud or copied: no O/0, no I/1/l. -ALPHABET = "ABCDEFGHJKLMNPQRSTUVWXYZ23456789" -LENGTH = 10 - - -def generate_code() -> str: - return "".join(secrets.choice(ALPHABET) for _ in range(LENGTH)) - - -def create(db: Session, created_by: int | None, note: str | None = None) -> InviteCode: - # Retried rather than trusted: the column is unique, and a collision is - # cheaper to avoid than to explain. - for _ in range(5): - code = generate_code() - if not db.query(InviteCode.id).filter(InviteCode.code == code).first(): - row = InviteCode(code=code, note=(note or None), created_by=created_by) - db.add(row) - db.commit() - db.refresh(row) - return row - raise RuntimeError("Could not allocate an unused invite code") - - -def usable(db: Session, code: str | None) -> InviteCode | None: - """The code, if it exists and has neither been spent nor withdrawn.""" - cleaned = (code or "").strip().upper() - if not cleaned: - return None - row = db.query(InviteCode).filter(InviteCode.code == cleaned).first() - if row is None or row.used_by is not None or row.revoked_at is not None: - return None - return row - - -def spend(db: Session, invite: InviteCode, user: User) -> None: - invite.used_by = user.id - invite.used_at = datetime.utcnow() - db.commit() - - -def as_json(row: InviteCode, users: dict[int, User]) -> dict: - used_by = users.get(row.used_by) - return { - "id": row.id, - "code": row.code, - "note": row.note, - "created_at": row.created_at, - "used_at": row.used_at, - "used_by_name": getattr(used_by, "name", None), - "used_by_email": getattr(used_by, "email", None), - "revoked_at": row.revoked_at, - "status": "used" if row.used_by else "revoked" if row.revoked_at else "open", - } diff --git a/backend/app/services/login_codes.py b/backend/app/services/login_codes.py deleted file mode 100644 index a2b3441..0000000 --- a/backend/app/services/login_codes.py +++ /dev/null @@ -1,159 +0,0 @@ -"""Signing in with a code sent to your own mailbox. - -Six characters a person reads off an email and types into the page they asked -from. Everything here follows from the code being short: it can be guessed, so -what makes it safe is not its length on its own but the arithmetic between its -length, how many guesses it will ever answer, and how long it lives. -""" -import hashlib -import hmac -import re -import secrets -from datetime import datetime, timedelta - -from sqlalchemy.orm import Session - -from app.models.login_code import LoginCode -from app.models.user import User -#: The house alphabet, and the reason it is the house alphabet: no O/0 and no -#: I/1, because these are read off a screen and typed back. Borrowed rather -#: than copied — there should be one answer to "which characters may a person -#: be asked to retype", not one per feature. -from app.services.invites import ALPHABET - -#: Six characters of a 32-letter alphabet: 2^30 codes, where the six digits -#: everyone is used to would be 2^20. The length is only half the argument -#: though, and the smaller half. A code answers at most MAX_ATTEMPTS guesses -#: before it is retired, only one is ever live per person, and the address may -#: only be tried so often — so an attacker gets tens of guesses an hour against -#: a billion, while sending the victim a mail for every code they burn through. -#: Eight characters would buy a thousandfold against an attack the guess budget -#: has already ended, and cost every user two more characters to type. -LENGTH = 6 - -#: Five. Enough that a mistyped character, a stale code from a first attempt -#: and one more fumble do not lock somebody out of their own account; few -#: enough that the whole budget is nothing against 2^30. Counted in the row -#: rather than the cache — see the model. -MAX_ATTEMPTS = 5 - -#: Fifteen minutes, unchanged from when this was a link, and for a reason that -#: survived the change: a first mail between a sender and a recipient who have -#: never corresponded is routinely greylisted five to ten minutes, and a code -#: that has expired before it arrives is not a sign-in method. Shortening it -#: would not slow an attacker down — only one code is ever live per person and -#: it answers five guesses however long it sits there — so the clock is a -#: deliverability question, and the one thing it does buy is a shorter window -#: for a mailbox somebody else is reading. Fifteen is where those meet. -CODE_TTL = timedelta(minutes=15) - -#: Rows are dead weight once well past expiry and nothing reads them. Kept a -#: day rather than deleted on the spot, so "it did not work this morning" can -#: still be answered from the table. -KEEP_SPENT_FOR = timedelta(days=1) - - -def normalise(code: str) -> str: - """What the person meant, from what they typed. - - Case, spaces and dashes are not part of the code — the mail shows it in two - groups of three, and somebody will type the space. Anything that is not a - letter or a digit goes; the rest is upper-cased. There is no look-alike - folding to do, which is the point of that alphabet: O and 0 are not two - characters one might mistake for each other here, they are both simply not - characters a code can contain. - """ - return re.sub(r"[^A-Z0-9]", "", (code or "").upper()) - - -def fingerprint(value: str) -> str: - return hashlib.sha256(value.strip().encode("utf-8")).hexdigest() - - -def generate_code() -> str: - return "".join(secrets.choice(ALPHABET) for _ in range(LENGTH)) - - -def for_display(code: str) -> str: - """Grouped for reading aloud in one's head. Typed back in any shape.""" - return f"{code[:3]} {code[3:]}" if len(code) == 6 else code - - -def issue(db: Session, user: User) -> str: - """Retire this user's outstanding codes, mint a new one, return it. - - Superseding rather than accumulating, and it matters more here than it did - for a link: with several codes live at once, a guess is tested against all - of them, and a six-character code gets easier to hit with every one left - lying around. Exactly one is live per person at any moment. - """ - now = datetime.utcnow() - db.query(LoginCode).filter( - LoginCode.user_id == user.id, - LoginCode.consumed_at.is_(None), - ).update({"consumed_at": now}, synchronize_session=False) - db.query(LoginCode).filter( - LoginCode.user_id == user.id, - LoginCode.expires_at < now - KEEP_SPENT_FOR, - ).delete(synchronize_session=False) - - code = generate_code() - db.add(LoginCode(user_id=user.id, code_hash=fingerprint(code), expires_at=now + CODE_TTL)) - db.commit() - return code - - -def verify(db: Session, email: str, code: str) -> User | None: - """Spend a code and return whose it is, or None if it is no good. - - Scoped to the address, and that is not merely convenient. A code looked up - on its own would be tested against every code live on the site at once, so - each guess would be as many guesses as there are people signing in — the - short code's one real weakness, closed by knowing whose code it is meant to - be before comparing. - - One answer for every failure: wrong code, expired code, code whose guesses - have run out, address with no account. Telling them apart would say which - of those was true, and none of them leaves the person anything to do but - ask for a new code. - """ - user = db.query(User).filter(User.email == email.lower().strip()).first() - if user is None: - return None - - row = db.query(LoginCode).filter( - LoginCode.user_id == user.id, - LoginCode.consumed_at.is_(None), - ).order_by(LoginCode.id.desc()).first() - if row is None or row.expires_at <= datetime.utcnow(): - return None - - if not hmac.compare_digest(row.code_hash, fingerprint(normalise(code))): - # A wrong guess costs one of five, and the fifth ends the code rather - # than leaving it to be guessed at a slower rate: whoever is typing has - # either lost the mail or is not its owner, and both are one click from - # a new code. The count is a SQL expression so that two guesses racing - # each other cost two. - exhausted = row.attempts + 1 >= MAX_ATTEMPTS - spend = {"attempts": LoginCode.attempts + 1} - if exhausted: - spend["consumed_at"] = datetime.utcnow() - db.query(LoginCode).filter( - LoginCode.id == row.id, - LoginCode.consumed_at.is_(None), - ).update(spend, synchronize_session=False) - db.commit() - return None - - # The claim is the UPDATE, not the read above: two requests carrying the - # same code both pass the comparison, and only the one whose UPDATE matched - # a row still unspent gets to sign in. - claimed = db.query(LoginCode).filter( - LoginCode.id == row.id, - LoginCode.consumed_at.is_(None), - ).update({"consumed_at": datetime.utcnow()}, synchronize_session=False) - db.commit() - if not claimed: - return None - - return user diff --git a/backend/app/services/site_settings.py b/backend/app/services/site_settings.py index d813018..a43b254 100644 --- a/backend/app/services/site_settings.py +++ b/backend/app/services/site_settings.py @@ -19,8 +19,6 @@ FLAGS: dict[str, bool] = { "registration_enabled": True, #: Whether a learner may create a public share link for a session. "sharing_enabled": True, - #: Whether registering requires an invite code issued by an administrator. - "invite_only": False, #: Whether the only way in is the identity provider. Read here as well as #: in the SSO routes because it governs every email-based way of signing #: in, link included, and each of those had been reading the key itself. diff --git a/backend/tests/api-contract.json b/backend/tests/api-contract.json index f9f9daa..87ecf7f 100644 --- a/backend/tests/api-contract.json +++ b/backend/tests/api-contract.json @@ -12,16 +12,6 @@ "422" ] }, - "DELETE /api/v1/admin/invites/{invite_id}": { - "body": false, - "params": [ - "path:invite_id" - ], - "responses": [ - "204", - "422" - ] - }, "DELETE /api/v1/admin/models/{model_id}": { "body": false, "params": [ @@ -550,13 +540,6 @@ "200" ] }, - "GET /api/v1/admin/invites": { - "body": false, - "params": [], - "responses": [ - "200" - ] - }, "GET /api/v1/admin/models": { "body": false, "params": [], @@ -2003,14 +1986,6 @@ "200" ] }, - "POST /api/v1/admin/invites": { - "body": true, - "params": [], - "responses": [ - "201", - "422" - ] - }, "POST /api/v1/admin/litellm/models": { "body": true, "params": [], @@ -2228,22 +2203,6 @@ "422" ] }, - "POST /api/v1/auth/login-code": { - "body": true, - "params": [], - "responses": [ - "200", - "422" - ] - }, - "POST /api/v1/auth/login-code/verify": { - "body": true, - "params": [], - "responses": [ - "200", - "422" - ] - }, "POST /api/v1/auth/logout": { "body": true, "params": [], diff --git a/backend/tests/test_invites_and_flags.py b/backend/tests/test_invites_and_flags.py deleted file mode 100644 index ac49bfe..0000000 --- a/backend/tests/test_invites_and_flags.py +++ /dev/null @@ -1,125 +0,0 @@ -"""Invite-only sign-up, and the switches an administrator sets once. - -Disposable SQLite; Redis is a Mock. The rules worth pinning: a code works once, -a spent code is kept rather than deleted, the flag falls back to its default -when Redis is down — and, for the gate that protects sign-up, the default is -the safe direction. -""" -import sys -import unittest -from types import ModuleType -from unittest.mock import Mock, patch - -import test_quiz_builder as fixtures -from app.models.invite import InviteCode -from app.models.user import User -from app.services import invites, site_settings - - -def fake_redis(store, broken=False): - client = Mock() - if broken: - client.get.side_effect = ConnectionError("redis down") - client.set.side_effect = ConnectionError("redis down") - else: - client.get.side_effect = lambda k: store.get(k) - client.set.side_effect = lambda k, v: store.__setitem__(k, v) - module = Mock() - module.from_url.return_value = client - return module - - -class FlagTests(unittest.TestCase): - def test_unset_flags_take_their_default(self): - with patch.dict(sys.modules, {"redis": fake_redis({})}): - self.assertTrue(site_settings.get_flag("sharing_enabled")) - self.assertFalse(site_settings.get_flag("invite_only")) - - def test_a_flag_reads_back_what_was_set(self): - store = {} - with patch.dict(sys.modules, {"redis": fake_redis(store)}): - site_settings.set_flag("invite_only", True) - self.assertEqual(store["settings:invite_only"], "true") - self.assertTrue(site_settings.get_flag("invite_only")) - - def test_losing_redis_falls_back_rather_than_failing(self): - with patch.dict(sys.modules, {"redis": fake_redis({}, broken=True)}): - # Sharing keeps working; sign-up does not silently open. - self.assertTrue(site_settings.get_flag("sharing_enabled")) - self.assertFalse(site_settings.get_flag("invite_only")) - - def test_an_unknown_flag_is_refused_rather_than_invented(self): - with self.assertRaises(KeyError): - site_settings.get_flag("nonsense") - with self.assertRaises(KeyError): - site_settings.set_flag("nonsense", True) - - -class InviteTests(unittest.TestCase): - def setUp(self): - self.bank = fixtures.BuilderTests() - self.bank.setUp() - self.db = self.bank.db - - def tearDown(self): - self.bank.tearDown() - - def test_a_code_is_unambiguous_to_read_aloud(self): - code = invites.generate_code() - self.assertEqual(len(code), invites.LENGTH) - # No O/0 or I/1/l to mistype. - self.assertFalse(set(code) & set("O0I1l")) - - def test_a_code_works_once(self): - row = invites.create(self.db, created_by=None, note="For a new tutor") - self.assertIsNotNone(invites.usable(self.db, row.code)) - # Case and surrounding space are forgiven; a typed code is typed. - self.assertIsNotNone(invites.usable(self.db, f" {row.code.lower()} ")) - - invites.spend(self.db, row, self.bank.peer) - self.assertIsNone(invites.usable(self.db, row.code)) - # Kept, not deleted: who it let in is the record worth having. - self.assertEqual(self.db.query(InviteCode).count(), 1) - self.assertEqual(self.db.get(InviteCode, row.id).used_by, self.bank.peer.id) - - def test_nothing_and_nonsense_are_not_codes(self): - self.assertIsNone(invites.usable(self.db, None)) - self.assertIsNone(invites.usable(self.db, "")) - self.assertIsNone(invites.usable(self.db, "NOTACODE12")) - - def test_a_withdrawn_code_stops_working(self): - from datetime import datetime - row = invites.create(self.db, created_by=None) - row.revoked_at = datetime.utcnow() - self.db.commit() - self.assertIsNone(invites.usable(self.db, row.code)) - - -if __name__ == "__main__": - unittest.main() - - -class TutorFlagTests(unittest.TestCase): - """Whether the tutor may be opened during a session. - - The rule that is not a setting: the tutor is handed the correct answer and - told it may explain it, so an exam-mode tutor is an answer key. That is - refused whatever the flag says. The flag only decides study mode. - """ - - def test_the_tutor_is_allowed_by_default(self): - with patch.dict(sys.modules, {"redis": fake_redis({})}): - self.assertTrue(site_settings.get_flag("tutor_in_quiz")) - - def test_turning_it_off_reads_back(self): - store = {} - with patch.dict(sys.modules, {"redis": fake_redis(store)}): - site_settings.set_flag("tutor_in_quiz", False) - self.assertFalse(site_settings.get_flag("tutor_in_quiz")) - - def test_redis_being_down_leaves_the_tutor_on(self): - # The safe direction here is the permissive one: losing Redis should - # not silently remove a study aid. Nothing is revealed that study mode - # does not already show. - with patch.dict(sys.modules, {"redis": fake_redis({}, broken=True)}): - self.assertTrue(site_settings.get_flag("tutor_in_quiz")) diff --git a/backend/tests/test_login_codes.py b/backend/tests/test_login_codes.py deleted file mode 100644 index 07b6ceb..0000000 --- a/backend/tests/test_login_codes.py +++ /dev/null @@ -1,290 +0,0 @@ -"""Signing in with a code sent by email. - -The properties worth pinning are the ones that are invisible when they break: a -stranger's address gets the same answer as a member's, in the same time; a code -works once and not after that; a wrong guess costs one of five and the fifth -ends the code; and the limiter counts requests for an address that has no -account exactly as it counts the others, or the 429 becomes the answer the -identical message was there to withhold. - -Disposable SQLite, a Mock for Redis, and nothing leaves the process. -""" -import hashlib -import sys -import time -import unittest -from datetime import datetime, timedelta -from types import ModuleType -from unittest.mock import AsyncMock, patch - -import test_quiz_builder as fixtures -from app.models.email_verification import EmailVerification -from app.models.login_code import LoginCode -from app.routers import login_code -from app.services import email_service, invites, login_codes - - -class MemoryRedis: - def __init__(self): self.values = {} - def get(self, key): return self.values.get(key) - def set(self, key, value): self.values[key] = value - def incr(self, key): - self.values[key] = int(self.values.get(key, 0)) + 1 - return self.values[key] - def expire(self, *args): return True - - -class LoginCodeTests(unittest.TestCase): - def setUp(self): - self.bank = fixtures.BuilderTests() - self.bank.setUp() - self.bank.owner.email = "owner@example.com" - # The peer too: the address goes through EmailStr on the way in, which - # refuses the reserved .test domain the fixtures otherwise use. - self.bank.peer.email = "peer@example.com" - self.bank.db.commit() - self.client = self.bank.client - self.client.app.include_router(login_code.router, prefix="/auth") - - self.redis = MemoryRedis() - module = ModuleType("redis") - module.from_url = lambda *args, **kwargs: self.redis - modules = patch.dict(sys.modules, {"redis": module}) - modules.start() - self.addCleanup(modules.stop) - - # Every reply is paced to a floor; the pacing itself has its own test, - # and the rest of the suite has no reason to wait for it. - floor = patch.object(login_code, "MIN_SECONDS", 0.01) - floor.start() - self.addCleanup(floor.stop) - - sent = patch.object(email_service, "send_login_code_email", new_callable=AsyncMock) - self.sent = sent.start() - self.addCleanup(sent.stop) - - def tearDown(self): - self.bank.tearDown() - - def ask(self, email="owner@example.com"): - return self.client.post("/auth/login-code", json={"email": email}) - - def code_for(self, email="owner@example.com"): - """The code as the mail shows it — spaces and all.""" - self.sent.reset_mock() - self.assertEqual(self.ask(email).status_code, 200) - self.sent.assert_awaited_once() - return self.sent.await_args.args[2] - - def enter(self, code, email="owner@example.com"): - return self.client.post("/auth/login-code/verify", json={"email": email, "code": code}) - - def wrong(self, email="owner@example.com"): - return self.enter("ZZZZZZ", email) - - # ── the answer gives nothing away ────────────────────────────────── - - def test_a_stranger_gets_the_member_s_answer_word_for_word(self): - known = self.ask() - unknown = self.ask("nobody@example.com") - self.assertEqual(known.status_code, unknown.status_code) - self.assertEqual(known.json(), unknown.json()) - self.assertEqual(known.json(), {"message": login_code.SENT}) - # And the only difference behind it is one that never reaches the caller. - self.assertEqual(self.bank.db.query(LoginCode).count(), 1) - - def test_both_answers_are_held_to_the_same_floor(self): - # The real constant, because the point of the test is the value of it. - with patch.object(login_code, "MIN_SECONDS", 0.3): - times = [] - for address in ("owner@example.com", "nobody@example.com"): - started = time.perf_counter() - self.ask(address) - times.append(time.perf_counter() - started) - for elapsed in times: - self.assertGreaterEqual(elapsed, 0.3) - self.assertLess(abs(times[0] - times[1]), 0.15) - - def test_a_wrong_code_reads_the_same_as_an_address_with_no_account(self): - self.code_for() - mistyped = self.wrong() - stranger = self.enter("ZZZZZZ", "nobody@example.com") - self.assertEqual(mistyped.status_code, stranger.status_code) - self.assertEqual(mistyped.json(), stranger.json()) - self.assertEqual(mistyped.json()["detail"], login_code.REFUSED) - - def test_the_limiter_counts_a_stranger_s_address_too(self): - # Otherwise the 429 arrives only for addresses that exist, and the - # identical message above has been undone by the thing protecting it. - for _ in range(login_code.PER_ADDRESS_CALLS): - self.assertEqual(self.ask("nobody@example.com").status_code, 200) - self.assertEqual(self.ask("nobody@example.com").status_code, 429) - self.assertEqual(self.ask().status_code, 200) - - def test_the_address_limit_bites_on_the_fourth_request(self): - for _ in range(login_code.PER_ADDRESS_CALLS): - self.assertEqual(self.ask().status_code, 200) - refused = self.ask() - self.assertEqual(refused.status_code, 429) - self.assertIn("Too many sign-in codes", refused.json()["detail"]) - # Nothing was issued on the refused attempt. - self.assertEqual(self.bank.db.query(LoginCode).count(), login_code.PER_ADDRESS_CALLS) - - def test_the_network_limit_is_the_looser_of_the_two(self): - self.assertGreater(login_code.PER_IP_CALLS, login_code.PER_ADDRESS_CALLS) - # A different address each time, so the tighter limit cannot be what - # stops it. - for n in range(login_code.PER_IP_CALLS): - self.assertEqual(self.ask(f"person{n}@example.com").status_code, 200) - refused = self.ask(f"person{login_code.PER_IP_CALLS}@example.com") - self.assertEqual(refused.status_code, 429) - self.assertIn("from this network", refused.json()["detail"]) - - # ── the code itself ──────────────────────────────────────────────── - - def test_a_code_signs_you_in_once_and_never_again(self): - code = self.code_for() - first = self.enter(code) - self.assertEqual(first.status_code, 200, first.text) - self.assertTrue(first.json()["access_token"]) - second = self.enter(code) - self.assertEqual(second.status_code, 400) - self.assertEqual(second.json()["detail"], login_code.REFUSED) - - def test_six_characters_of_the_house_alphabet(self): - code = login_codes.normalise(self.code_for()) - self.assertEqual(len(code), login_codes.LENGTH) - self.assertEqual(login_codes.LENGTH, 6) - self.assertTrue(set(code) <= set(invites.ALPHABET)) - # The characters that get misread are not in it to be misread. - self.assertFalse(set("O0I1") & set(invites.ALPHABET)) - - def test_case_spaces_and_dashes_do_not_decide_who_gets_in(self): - code = login_codes.normalise(self.code_for()) - typed = f" {code[:3].lower()}-{code[3:]} " - self.assertEqual(self.enter(typed).status_code, 200) - - def test_the_code_as_the_mail_shows_it_types_straight_back(self): - shown = self.code_for() - self.assertIn(" ", shown) - self.assertEqual(self.enter(shown).status_code, 200) - - def test_an_expired_code_is_refused(self): - code = self.code_for() - row = self.bank.db.query(LoginCode).one() - row.expires_at = datetime.utcnow() - timedelta(seconds=1) - self.bank.db.commit() - self.assertEqual(self.enter(code).status_code, 400) - # Refused without being marked spent: it was never used, and the row - # says so. - self.bank.db.refresh(row) - self.assertIsNone(row.consumed_at) - - def test_fifteen_minutes_and_not_an_hour(self): - self.code_for() - row = self.bank.db.query(LoginCode).one() - self.assertAlmostEqual((row.expires_at - row.created_at).total_seconds(), 15 * 60, delta=5) - - def test_a_second_request_retires_the_first_code(self): - stale = self.code_for() - fresh = self.code_for() - self.assertEqual(self.enter(stale).status_code, 400) - self.assertEqual(self.enter(fresh).status_code, 200) - - def test_the_table_holds_a_fingerprint_rather_than_the_code(self): - code = login_codes.normalise(self.code_for()) - row = self.bank.db.query(LoginCode).one() - self.assertNotIn(code, str(row.code_hash)) - self.assertEqual(row.code_hash, hashlib.sha256(code.encode()).hexdigest()) - - def test_a_code_is_no_good_against_somebody_else_s_address(self): - # A short code looked up on its own would be tried against every code - # live on the site at once. It is only ever tried against one account. - code = self.code_for() - self.assertEqual(self.enter(code, "peer@example.com").status_code, 400) - self.assertEqual(self.enter(code).status_code, 200) - - # ── guessing costs something ─────────────────────────────────────── - - def test_a_wrong_code_counts_against_the_limit(self): - self.code_for() - for expected in range(1, 3): - self.assertEqual(self.wrong().status_code, 400) - self.assertEqual(self.bank.db.query(LoginCode).one().attempts, expected) - - def test_a_code_out_of_guesses_is_retired_rather_than_left_guessable(self): - code = self.code_for() - for _ in range(login_codes.MAX_ATTEMPTS): - self.assertEqual(self.wrong().status_code, 400) - row = self.bank.db.query(LoginCode).one() - self.assertIsNotNone(row.consumed_at) - # And now not even the right code opens it — the way back is a new one. - self.assertEqual(self.enter(code).status_code, 400) - self.assertEqual(self.enter(self.code_for()).status_code, 200) - - def test_the_attempts_limit_keeps_counting_after_the_code_is_dead(self): - # The code's own five run out first. The address limit is what stops - # somebody working through codes to buy five more guesses at a time. - self.code_for() - for _ in range(login_code.PER_ADDRESS_TRIES): - self.assertEqual(self.wrong().status_code, 400) - refused = self.wrong() - self.assertEqual(refused.status_code, 429) - self.assertIn("Too many attempts", refused.json()["detail"]) - - def test_guessing_a_stranger_s_address_is_counted_the_same_way(self): - for _ in range(login_code.PER_ADDRESS_TRIES): - self.assertEqual(self.wrong("nobody@example.com").status_code, 400) - self.assertEqual(self.wrong("nobody@example.com").status_code, 429) - - # ── the rules the password form already keeps ────────────────────── - - def test_an_unverified_account_is_turned_away_as_it_is_at_the_password_form(self): - self.bank.db.add(EmailVerification(user_id=self.bank.owner.id, token="verification-token", - expires_at=datetime.utcnow() + timedelta(hours=1), verified_at=None)) - self.bank.db.commit() - code = self.code_for() - refused = self.enter(code) - self.assertEqual(refused.status_code, 403) - self.assertIn("Email not verified", refused.json()["detail"]) - # Refused, and the code spent all the same: turning somebody away must - # not leave a working credential behind them. - self.assertIsNotNone(self.bank.db.query(LoginCode).one().consumed_at) - - def test_sso_only_closes_this_door_as_well_as_the_password_one(self): - code = self.code_for() - self.redis.values["settings:sso_only"] = "true" - refused = self.ask() - self.assertEqual(refused.status_code, 403) - self.assertIn("Please use SSO", refused.json()["detail"]) - self.assertEqual(self.enter(code).status_code, 403) - self.assertEqual(self.bank.db.query(LoginCode).count(), 1) - - -class ServiceTests(unittest.TestCase): - def setUp(self): - self.bank = fixtures.BuilderTests() - self.bank.setUp() - self.addCleanup(self.bank.tearDown) - - def test_only_one_of_two_claims_on_one_code_succeeds(self): - code = login_codes.issue(self.bank.db, self.bank.owner) - self.assertIsNotNone(login_codes.verify(self.bank.db, self.bank.owner.email, code)) - self.assertIsNone(login_codes.verify(self.bank.db, self.bank.owner.email, code)) - - def test_rows_long_past_their_expiry_are_swept_on_the_next_request(self): - login_codes.issue(self.bank.db, self.bank.owner) - stale = self.bank.db.query(LoginCode).one() - stale.expires_at = datetime.utcnow() - timedelta(days=3) - self.bank.db.commit() - login_codes.issue(self.bank.db, self.bank.owner) - self.assertEqual(self.bank.db.query(LoginCode).count(), 1) - - def test_normalising_keeps_only_what_the_code_is_made_of(self): - self.assertEqual(login_codes.normalise(" ab3-c d9 "), "AB3CD9") - self.assertEqual(login_codes.normalise("abc\u2013def"), "ABCDEF") - self.assertEqual(login_codes.normalise(""), "") - - -if __name__ == "__main__": - unittest.main() diff --git a/frontend/src/components/Navbar.jsx b/frontend/src/components/Navbar.jsx index 6d6086c..b07d221 100644 --- a/frontend/src/components/Navbar.jsx +++ b/frontend/src/components/Navbar.jsx @@ -223,6 +223,10 @@ export default function Navbar({ onSignIn, onRegister, onSearch }) { { to: '/study-plans', label: 'Study plans' }, { to: '/articles', label: 'Reading' }, { to: '/flashcards', label: 'Cards' }, + // The scribe, where a deck is written from your own material. A different + // application behind the same sign-in, so it is a real link out rather + // than a route — and it says so with the arrow. + { to: 'https://app.pedshub.com/#resources', label: 'Make a deck', external: true }, ] : [] return ( @@ -310,10 +314,16 @@ export default function Navbar({ onSignIn, onRegister, onSearch }) { a mouse the last links were unreachable. */} {navLinks.map(l => ( + l.external ? ( + + {l.label} ↗ + + ) : ( {l.label} + ) ))} @@ -327,17 +337,24 @@ export default function Navbar({ onSignIn, onRegister, onSearch }) { padding: '8px 0 12px', }}>
- {navLinks.map(l => ( - { + const style = { color: 'var(--navbar-fg)', textDecoration: 'none', padding: '10px 4px', fontSize: '0.95rem', fontWeight: location.pathname === l.to ? 600 : 400, opacity: location.pathname === l.to ? 1 : 0.75, borderBottom: '1px solid rgba(255,255,255,0.05)', - }}> + } + return l.external ? ( + + {l.label} ↗ + + ) : ( + {l.label} - ))} + ) + })} -
- - {codes.length === 0 ? ( -

No codes yet. Create one to let somebody in.

- ) : ( - - )} - - )} ) } diff --git a/frontend/src/pages/LandingPage.jsx b/frontend/src/pages/LandingPage.jsx index a104166..4d35444 100644 --- a/frontend/src/pages/LandingPage.jsx +++ b/frontend/src/pages/LandingPage.jsx @@ -247,12 +247,6 @@ function AuthModal({ mode, onClose, onSwitch }) { const [registered, setRegistered] = useState(false) // Typed twice. The same field exists on /register; this is the other door. const [confirm, setConfirm] = useState('') - // Whether this site is invite only. The register page asks the same - // question; this form is the other door and used not to ask at all, so - // turning the gate on left everyone here failing with "an invite code is - // required" and nowhere to put one. - const [inviteRequired, setInviteRequired] = useState(false) - const [inviteCode, setInviteCode] = useState('') const [signupOpen, setSignupOpen] = useState(true) //: The public page's own sign-in box. It is a different component from //: /login and knew nothing about the provider, so on a site where the only @@ -267,7 +261,6 @@ function AuthModal({ mode, onClose, onSwitch }) { api.get('/auth/signup-policy') .then(res => { if (!live) return - setInviteRequired(!!res.data?.invite_required) // Registration closed — by the switch, or because the site signs in // through a provider. Either way there is no password sign-up to // offer, and a Register tab that leads to a refusal is worse than no @@ -315,7 +308,6 @@ function AuthModal({ mode, onClose, onSwitch }) { const res = await api.post('/auth/register', { email, password, name, captcha_token: captchaToken || null, - invite_code: inviteCode.trim() || null, }) if (res.data.requires_verification) { setRegistered(true) @@ -447,23 +439,10 @@ function AuthModal({ mode, onClose, onSwitch }) { These do not match yet. )} - {inviteRequired && ( -
- - {/* Asked for only where it is needed. The form never says - whether a code is valid before the account is made — - that would be a place to guess them. */} - setInviteCode(e.target.value.toUpperCase())} /> - This site is invite only. -
- )} @@ -778,7 +757,9 @@ export default function LandingPage() {
diff --git a/frontend/src/pages/LoginCaptcha.test.jsx b/frontend/src/pages/LoginCaptcha.test.jsx index 76a2bbe..a66381a 100644 --- a/frontend/src/pages/LoginCaptcha.test.jsx +++ b/frontend/src/pages/LoginCaptcha.test.jsx @@ -23,10 +23,9 @@ afterEach(() => { document.getElementById('cap-widget-script')?.remove() }) function mount(Component) { render(} />Signed in successfully} />) } -async function submitLogin({ revealPassword = false } = {}) { - // The standalone page leads with a code by email; the password is behind a - // second option, and the landing modal still asks for it outright. - if (revealPassword) await userEvent.click(screen.getByRole('button', { name: 'Sign in with a password instead' })) +async function submitLogin() { + // Both pages ask for the password outright now. Sign-in codes were the + // standalone page's first offer; they are the identity provider's job. const form = screen.getByRole('form', { name: 'Sign in' }) expect(within(form).getByRole('button', { name: 'Sign In', exact: true })).toBeEnabled() await userEvent.type(within(form).getByLabelText('Email'), 'owner@example.test') @@ -38,7 +37,7 @@ async function submitLogin({ revealPassword = false } = {}) { it('logs in on the standalone page without rendering, loading or submitting a captcha', async () => { mount(LoginPage) - await submitLogin({ revealPassword: true }) + await submitLogin() // Signing in is not a form a stranger can spam into existence — there is an // account behind it and a rate limit in front of it. expect(document.querySelector('cap-widget')).toBeNull() diff --git a/frontend/src/pages/LoginCode.test.jsx b/frontend/src/pages/LoginCode.test.jsx deleted file mode 100644 index 1c74b50..0000000 --- a/frontend/src/pages/LoginCode.test.jsx +++ /dev/null @@ -1,128 +0,0 @@ -import { render, screen, waitFor } from '@testing-library/react' -import userEvent from '@testing-library/user-event' -import { MemoryRouter, Route, Routes } from 'react-router-dom' -import { beforeEach, expect, it, vi } from 'vitest' - -vi.mock('../api/client', () => ({ default: { get: vi.fn(), post: vi.fn() } })) - -import LoginPage from './LoginPage' -import { AuthProvider } from '../context/AuthContext' -import api from '../api/client' - -const SENT = 'If that address has an account, a sign-in code is on its way.' -const REFUSED = 'That code is not right, or it has expired. Request a new one.' - -function answers({ sso = { sso_enabled: false }, policy = { registration_open: true } } = {}) { - api.get.mockImplementation(url => Promise.resolve({ - data: url === '/auth/sso/config' ? sso - : url === '/auth/signup-policy' ? policy - : { id: 1, name: 'Test', role: 'user' }, - })) -} - -beforeEach(() => { - vi.resetAllMocks() - localStorage.clear() - answers() - api.post.mockResolvedValue({ data: { message: SENT } }) -}) - -function mount() { - render( - - } /> - Signed in successfully} /> - - ) -} - -async function askForCode(address = 'owner@example.test') { - await userEvent.type(screen.getByLabelText('Email'), address) - await userEvent.click(screen.getByRole('button', { name: 'Continue with email' })) - expect(api.post).toHaveBeenCalledWith('/auth/login-code', { email: address }) - return screen.findByLabelText('Sign-in code') -} - -it('sends a code and takes it on the same page', async () => { - mount() - expect(screen.queryByLabelText('Password')).not.toBeInTheDocument() - await askForCode() - expect(screen.getByText(SENT)).toBeInTheDocument() - // The address field is gone, so there is nothing left to fill in but the - // code — and nowhere for the sign-in to travel to. - expect(screen.queryByLabelText('Email')).not.toBeInTheDocument() - - api.post.mockResolvedValue({ data: { access_token: 'synthetic-code-session' } }) - await userEvent.type(screen.getByLabelText('Sign-in code'), 'abc def') - await userEvent.click(screen.getByRole('button', { name: 'Sign In', exact: true })) - expect(await screen.findByText('Signed in successfully')).toBeInTheDocument() - // Typed in whatever shape it was read in; the server does the normalising, - // and is sent the address so a code is only ever tried against one account. - expect(api.post).toHaveBeenCalledWith('/auth/login-code/verify', - { email: 'owner@example.test', code: 'ABC DEF' }) - expect(localStorage.getItem('token')).toBe('synthetic-code-session') -}) - -it('repeats what the server said rather than claiming the mail was sent', async () => { - // The message is the one thing that must read the same for an address with - // an account and one without, so the page is not allowed a copy of its own. - api.post.mockResolvedValue({ data: { message: 'A differently worded but equally uncommitted answer.' } }) - mount() - await askForCode('nobody@example.test') - expect(screen.getByText('A differently worded but equally uncommitted answer.')).toBeInTheDocument() - expect(screen.queryByText(/we sent|no account|not registered/i)).not.toBeInTheDocument() -}) - -it('keeps the code panel up when a code is refused', async () => { - mount() - await askForCode() - api.post.mockRejectedValue({ response: { status: 400, data: { detail: REFUSED } } }) - await userEvent.type(screen.getByLabelText('Sign-in code'), 'ZZZZZZ') - await userEvent.click(screen.getByRole('button', { name: 'Sign In', exact: true })) - expect(await screen.findByText(REFUSED)).toBeInTheDocument() - // Still here to try again, and still on the address it was sent to. - expect(screen.getByLabelText('Sign-in code')).toBeInTheDocument() -}) - -it('shows the reason when the site refuses to send another', async () => { - mount() - await askForCode() - api.post.mockRejectedValue({ response: { status: 429, data: { detail: 'Too many sign-in codes requested for that address. Please wait a few minutes.' } } }) - await userEvent.click(screen.getByRole('button', { name: 'Send a new code' })) - expect(await screen.findByText(/Too many sign-in codes/)).toBeInTheDocument() -}) - -it('lets somebody go back and use a different address', async () => { - mount() - await askForCode() - await userEvent.click(screen.getByRole('button', { name: 'Use a different address' })) - expect(await screen.findByLabelText('Email')).toHaveValue('owner@example.test') - expect(screen.queryByLabelText('Sign-in code')).not.toBeInTheDocument() -}) - -it('keeps a password one click away for whoever has one', async () => { - api.post.mockResolvedValue({ data: { access_token: 'synthetic-login-token' } }) - mount() - await userEvent.click(screen.getByRole('button', { name: 'Sign in with a password instead' })) - await userEvent.type(screen.getByLabelText('Email'), 'owner@example.test') - await userEvent.type(screen.getByLabelText('Password'), 'synthetic-password') - await userEvent.click(screen.getByRole('button', { name: 'Sign In', exact: true })) - expect(await screen.findByText('Signed in successfully')).toBeInTheDocument() - expect(api.post).toHaveBeenCalledWith('/auth/login', { email: 'owner@example.test', password: 'synthetic-password' }) - expect(api.post).not.toHaveBeenCalledWith('/auth/login-code', expect.anything()) -}) - -it('leaves single sign-on the first thing on the page', async () => { - answers({ sso: { sso_enabled: true, sso_only: false, provider_name: 'Test SSO' } }) - mount() - const sso = await screen.findByRole('link', { name: 'Sign in with Test SSO' }) - const byEmail = screen.getByRole('button', { name: 'Continue with email' }) - expect(sso.compareDocumentPosition(byEmail) & Node.DOCUMENT_POSITION_FOLLOWING).toBeTruthy() -}) - -it('offers registration only while the site is open to it', async () => { - answers({ policy: { registration_open: false } }) - mount() - expect(await screen.findByRole('button', { name: 'Continue with email' })).toBeInTheDocument() - await waitFor(() => expect(screen.queryByRole('link', { name: 'Sign up' })).not.toBeInTheDocument()) -}) diff --git a/frontend/src/pages/LoginPage.jsx b/frontend/src/pages/LoginPage.jsx index 6240fea..8777c2c 100644 --- a/frontend/src/pages/LoginPage.jsx +++ b/frontend/src/pages/LoginPage.jsx @@ -3,22 +3,21 @@ import { useNavigate, Link } from 'react-router-dom' import { useAuth } from '../context/AuthContext' import api from '../api/client' -// Said by the server, and repeated here only if it says nothing. The wording -// is load-bearing — it must not admit whether the address has an account — so -// it lives in one place and this is not that place. -const SENT = 'If that address has an account, a sign-in code is on its way.' - +/** + * Signing in. + * + * The provider first, and on a site that signs in only through one, the + * provider and nothing else. Email sign-in codes used to be the primary way in + * and lived here; codes are the identity provider's job now — it sends them, + * checks them, and knows about second factors — so the page that used to + * reimplement all of that offers a button instead. + * + * The password form survives for a site with no provider configured, or one + * that keeps both doors open. + */ export default function LoginPage() { const [email, setEmail] = useState('') const [password, setPassword] = useState('') - const [code, setCode] = useState('') - // A password is the second way in, not the first: most people do not have - // one worth remembering, and the ones who do are one click from it. - const [withPassword, setWithPassword] = useState(false) - // The code is typed on this page rather than followed from the mail, so the - // whole sign-in happens where it started — nothing to hand between devices, - // and no credential in anybody's address bar or history. - const [sent, setSent] = useState('') const [error, setError] = useState('') const [loading, setLoading] = useState(false) const [unverified, setUnverified] = useState(false) @@ -28,7 +27,7 @@ export default function LoginPage() { // Null until asked, and treated as open while unknown: a slow answer must // not hide a way in that exists. const [policy, setPolicy] = useState(null) - const { login, loginWithToken } = useAuth() + const { login } = useAuth() const navigate = useNavigate() const searchParams = new URLSearchParams(window.location.search) const ssoError = searchParams.get('error') @@ -38,74 +37,38 @@ export default function LoginPage() { api.get('/auth/signup-policy').then(r => setPolicy(r.data)).catch(() => {}) }, []) - const failed = (err, fallback) => { - const detail = err.response?.data?.detail - if (typeof detail === 'string') setError(detail) - else if (Array.isArray(detail)) setError(detail.some(e => e.loc?.includes('email')) ? 'Invalid email address.' : 'Please check your input and try again.') - else setError(fallback) - } - - const requestCode = async () => { - const res = await api.post('/auth/login-code', { email }) - setCode('') - setSent(res.data?.message || SENT) - } - const handleSubmit = async (e) => { e.preventDefault() setError('') setUnverified(false) setLoading(true) try { - if (withPassword) { - await login(email, password) - navigate('/') - } else if (sent) { - const res = await api.post('/auth/login-code/verify', { email, code }) - await loginWithToken(res.data.access_token) - navigate('/') - } else { - await requestCode() - } + await login(email, password) + navigate('/') } catch (err) { - if (withPassword && err.response?.status === 403) { + if (err.response?.status === 403) { setUnverified(true) } else { - failed(err, withPassword ? 'Login failed' : 'Something went wrong. Please try again.') + const detail = err.response?.data?.detail + setError(typeof detail === 'string' ? detail : 'Login failed') } } finally { setLoading(false) } } - const resend = async () => { - setError('') - setResending(true) - try { await requestCode() } - catch (err) { failed(err, 'Could not send another code. Please try again.') } - finally { setResending(false) } - } - const resendVerification = async () => { setResending(true) try { await api.post('/auth/resend-verification', { email }) - setResendSent(true) } catch { - setResendSent(true) // always show success to avoid enumeration + // Always shown as sent, so this cannot be used to learn who has an account. } finally { + setResendSent(true) setResending(false) } } - const startOver = () => { setSent(''); setCode(''); setError('') } - const usePassword = () => { startOver(); setWithPassword(true) } - - const linkButton = { - background: 'none', border: 'none', color: 'var(--primary)', - cursor: 'pointer', padding: 0, fontSize: '0.85rem', - } - return (
@@ -133,7 +96,7 @@ export default function LoginPage() { {ssoConfig?.sso_enabled && ( <> - + Sign in with {ssoConfig.provider_name} {!ssoConfig.sso_only &&
or sign in with email
} @@ -143,60 +106,24 @@ export default function LoginPage() { {!ssoConfig?.sso_only && ( <>
- {sent && !withPassword ? ( - <> -

{sent}

-

- It expires in 15 minutes. Check your spam folder if it isn't there. -

-
- - {/* Upper-cased as it is typed, as the invite field is. What - the server compares is normalised anyway: case, spaces - and dashes are not part of the code. */} - setCode(e.target.value.toUpperCase())} /> -
- - ) : ( -
- - setEmail(e.target.value)} required /> -
- )} - {withPassword && ( -
- - setPassword(e.target.value)} required /> -
- )} +
+ + setEmail(e.target.value)} required /> +
+
+ + setPassword(e.target.value)} required /> +
- {withPassword ? ( -
- - Forgot password? -
- ) : sent ? ( -
- - -
- ) : ( -
- {/* No mail to wait for if you already have a password. */} - -
- )} +
+ Forgot password? +
{/* Only where there is a door. Registration can be turned off site-wide, and this offered a locked one — the only way to find diff --git a/frontend/src/pages/RegisterInvite.test.jsx b/frontend/src/pages/RegisterInvite.test.jsx deleted file mode 100644 index cf8f731..0000000 --- a/frontend/src/pages/RegisterInvite.test.jsx +++ /dev/null @@ -1,68 +0,0 @@ -import { beforeEach, describe, expect, it, vi } from 'vitest' -import { render, screen, waitFor } from '@testing-library/react' -import userEvent from '@testing-library/user-event' -import { MemoryRouter } from 'react-router-dom' -import RegisterPage from './RegisterPage' -import api from '../api/client' - -vi.mock('../api/client', () => ({ default: { get: vi.fn(), post: vi.fn() } })) -vi.mock('../context/AuthContext', () => ({ useAuth: () => ({ loginWithToken: vi.fn() }) })) - -const mount = (inviteRequired) => { - api.get.mockResolvedValue({ data: { invite_required: inviteRequired, first_user: false } }) - return render() -} - -const fill = async () => { - await userEvent.type(screen.getByLabelText('Name'), 'Ada') - await userEvent.type(screen.getByLabelText('Email'), 'ada@example.test') - await userEvent.type(screen.getByLabelText('Password'), 'longenough1') - // Typed twice, as a real person would. - await userEvent.type(screen.getByLabelText('Confirm password'), 'longenough1') -} - -describe('registering when the site is invite only', () => { - beforeEach(() => { vi.clearAllMocks() }) - - it('asks for nothing extra on an open site', async () => { - mount(false) - await waitFor(() => expect(api.get).toHaveBeenCalledWith('/auth/signup-policy')) - expect(screen.queryByLabelText('Invite code')).not.toBeInTheDocument() - }) - - it('asks for a code, and will not submit without one', async () => { - mount(true) - const field = await screen.findByLabelText('Invite code') - await fill() - expect(screen.getByRole('button', { name: /Sign Up/ })).toBeDisabled() - - await userEvent.type(field, 'abcd234xyz') - // Typed in whatever case, sent in the one the codes are issued in. - expect(field).toHaveValue('ABCD234XYZ') - expect(screen.getByRole('button', { name: /Sign Up/ })).toBeEnabled() - - api.post.mockResolvedValue({ data: { requires_verification: true } }) - await userEvent.click(screen.getByRole('button', { name: /Sign Up/ })) - await waitFor(() => expect(api.post).toHaveBeenCalledWith('/auth/register', - expect.objectContaining({ invite_code: 'ABCD234XYZ' }))) - }) - - it('shows the refusal rather than a blank failure', async () => { - mount(true) - await screen.findByLabelText('Invite code') - await fill() - await userEvent.type(screen.getByLabelText('Invite code'), 'WRONGCODE1') - api.post.mockRejectedValue({ response: { data: { detail: 'This site is invite-only. A valid invite code is required.' } } }) - await userEvent.click(screen.getByRole('button', { name: /Sign Up/ })) - expect(await screen.findByText(/invite-only/)).toBeInTheDocument() - }) - - it('stays usable if the policy cannot be fetched', async () => { - api.get.mockRejectedValue(new Error('down')) - render() - // No code asked for, rather than a form nobody can complete. - await waitFor(() => expect(screen.queryByLabelText('Invite code')).not.toBeInTheDocument()) - await fill() - expect(screen.getByRole('button', { name: /Sign Up/ })).toBeEnabled() - }) -}) diff --git a/frontend/src/pages/RegisterPage.jsx b/frontend/src/pages/RegisterPage.jsx index 355d81b..3b874c3 100644 --- a/frontend/src/pages/RegisterPage.jsx +++ b/frontend/src/pages/RegisterPage.jsx @@ -15,13 +15,9 @@ export default function RegisterPage() { const [loading, setLoading] = useState(false) const [done, setDone] = useState(false) const [captchaToken, setCaptchaToken] = useState('') - // Whether this site is invite-only. Asked before there is an account to ask - // with, so the form knows whether to want a code. - const [inviteRequired, setInviteRequired] = useState(false) - const [inviteCode, setInviteCode] = useState('') //: Null until the policy is known. A site on single sign-on has no password //: sign-up, and drawing the form and refusing the POST is asking somebody - //: for a name, an email, a password twice and an invite code before telling + //: for a name, an email and a password twice before telling //: them the door does not exist. const [policy, setPolicy] = useState(null) const { loginWithToken } = useAuth() @@ -31,7 +27,6 @@ export default function RegisterPage() { api.get('/auth/signup-policy') .then(res => { if (!live) return - setInviteRequired(!!res.data?.invite_required) setPolicy(res.data || {}) }) .catch(() => { if (live) setPolicy({}) }) @@ -47,7 +42,6 @@ export default function RegisterPage() { const res = await api.post('/auth/register', { email, password, name, captcha_token: captchaToken || null, - invite_code: inviteCode.trim() || null, }) if (res.data.requires_verification) { setDone(true) @@ -140,25 +134,10 @@ export default function RegisterPage() { )}
- {inviteRequired && ( -
- - {/* Asked for only where it is needed. The form never says whether - a code is valid before the account is made — that would be a - place to guess them. */} - setInviteCode(e.target.value.toUpperCase())} /> - - This site is invite only. - -
- )}