diff --git a/docs/plans/2026-02-20-profile-validation-plan.md b/docs/plans/2026-02-20-profile-validation-plan.md new file mode 100644 index 0000000..7d03b33 --- /dev/null +++ b/docs/plans/2026-02-20-profile-validation-plan.md @@ -0,0 +1,586 @@ +# Profile Form Validation Implementation Plan + +> **For Claude:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task. + +**Goal:** Replace inline profile validation with a shared Pydantic model using `EmailStr` and `pydantic-extra-types` `PhoneNumberValidator` for E.164 phone validation. + +**Architecture:** New `src/porchlight/validation.py` module with a `ProfileUpdate` Pydantic model. Both manage and admin route handlers parse form data through this model, catch `ValidationError`, and return user-friendly errors via the existing htmx pattern. + +**Tech Stack:** Pydantic v2 (`EmailStr`), `pydantic-extra-types[phonenumbers]` (Google's libphonenumber), FastAPI form handling. + +--- + +### Task 1: Add `pydantic-extra-types[phonenumbers]` dependency + +**Files:** +- Modify: `pyproject.toml:7-22` + +**Step 1: Add the dependency** + +In `pyproject.toml`, add `"pydantic-extra-types[phonenumbers]>=2.0"` to the `dependencies` list (after `pydantic-settings`). + +```toml + "pydantic-settings>=2.7", + "pydantic-extra-types[phonenumbers]>=2.0", +``` + +**Step 2: Install** + +Run: `uv sync` + +**Step 3: Verify import works** + +Run: `uv run python -c "from pydantic_extra_types.phone_numbers import PhoneNumberValidator; print('OK')"` +Expected: `OK` + +**Step 4: Commit** + +```bash +git add pyproject.toml uv.lock +git commit -m "build: add pydantic-extra-types[phonenumbers] dependency" +``` + +--- + +### Task 2: Create `ProfileUpdate` validation model with tests + +**Files:** +- Create: `src/porchlight/validation.py` +- Create: `tests/test_validation.py` + +**Step 1: Write the failing tests** + +Create `tests/test_validation.py`: + +```python +import pytest +from pydantic import ValidationError + +from porchlight.validation import ProfileUpdate + + +class TestProfileUpdateEmail: + def test_valid_email(self) -> None: + p = ProfileUpdate(email="user@example.com") + assert p.email == "user@example.com" + + def test_empty_email_becomes_none(self) -> None: + p = ProfileUpdate(email="") + assert p.email is None + + def test_invalid_email_no_at(self) -> None: + with pytest.raises(ValidationError, match="email"): + ProfileUpdate(email="not-an-email") + + def test_invalid_email_no_domain(self) -> None: + with pytest.raises(ValidationError, match="email"): + ProfileUpdate(email="user@") + + +class TestProfileUpdatePhone: + def test_valid_e164(self) -> None: + p = ProfileUpdate(phone_number="+46701234567") + assert p.phone_number == "+46701234567" + + def test_valid_e164_with_spaces_normalized(self) -> None: + p = ProfileUpdate(phone_number="+46 70 123 45 67") + assert p.phone_number == "+46701234567" + + def test_empty_phone_becomes_none(self) -> None: + p = ProfileUpdate(phone_number="") + assert p.phone_number is None + + def test_invalid_phone(self) -> None: + with pytest.raises(ValidationError, match="phone"): + ProfileUpdate(phone_number="not-a-phone") + + def test_invalid_phone_no_plus(self) -> None: + with pytest.raises(ValidationError, match="phone"): + ProfileUpdate(phone_number="46701234567") + + +class TestProfileUpdatePicture: + def test_valid_https_url(self) -> None: + p = ProfileUpdate(picture="https://example.com/pic.jpg") + assert p.picture == "https://example.com/pic.jpg" + + def test_valid_http_url(self) -> None: + p = ProfileUpdate(picture="http://example.com/pic.jpg") + assert p.picture == "http://example.com/pic.jpg" + + def test_empty_picture_becomes_none(self) -> None: + p = ProfileUpdate(picture="") + assert p.picture is None + + def test_invalid_picture_not_url(self) -> None: + with pytest.raises(ValidationError, match="picture"): + ProfileUpdate(picture="not-a-url") + + def test_invalid_picture_ftp_scheme(self) -> None: + with pytest.raises(ValidationError, match="picture"): + ProfileUpdate(picture="ftp://example.com/pic.jpg") + + +class TestProfileUpdateFieldLengths: + def test_given_name_too_long(self) -> None: + with pytest.raises(ValidationError, match="given_name"): + ProfileUpdate(given_name="x" * 256) + + def test_phone_number_max_length(self) -> None: + """E.164 max is 15 digits + plus sign = 16 chars.""" + p = ProfileUpdate(phone_number="+123456789012345") + assert p.phone_number == "+123456789012345" + + def test_locale_too_long(self) -> None: + with pytest.raises(ValidationError, match="locale"): + ProfileUpdate(locale="x" * 21) + + +class TestProfileUpdateDefaults: + def test_all_defaults(self) -> None: + p = ProfileUpdate() + assert p.given_name == "" + assert p.family_name == "" + assert p.preferred_username == "" + assert p.email is None + assert p.phone_number is None + assert p.picture is None + assert p.locale == "" +``` + +**Step 2: Run tests to verify they fail** + +Run: `uv run python -m pytest tests/test_validation.py -v` +Expected: FAIL — `ModuleNotFoundError: No module named 'porchlight.validation'` + +**Step 3: Write the implementation** + +Create `src/porchlight/validation.py`: + +```python +from typing import Annotated +from urllib.parse import urlparse + +from pydantic import BaseModel, EmailStr, Field, field_validator +from pydantic_extra_types.phone_numbers import PhoneNumberValidator + +E164Phone = Annotated[str, PhoneNumberValidator(number_format="E164")] + + +class ProfileUpdate(BaseModel): + given_name: str = Field(default="", max_length=255) + family_name: str = Field(default="", max_length=255) + preferred_username: str = Field(default="", max_length=255) + email: EmailStr | None = None + phone_number: E164Phone | None = None + picture: str | None = Field(default=None, max_length=2048) + locale: str = Field(default="", max_length=20) + + @field_validator("email", mode="before") + @classmethod + def empty_email_to_none(cls, v: str) -> str | None: + if isinstance(v, str) and v.strip() == "": + return None + return v + + @field_validator("phone_number", mode="before") + @classmethod + def empty_phone_to_none(cls, v: str) -> str | None: + if isinstance(v, str) and v.strip() == "": + return None + return v + + @field_validator("picture", mode="before") + @classmethod + def validate_picture_url(cls, v: str) -> str | None: + if isinstance(v, str) and v.strip() == "": + return None + if isinstance(v, str): + parsed = urlparse(v) + if parsed.scheme not in ("http", "https") or not parsed.netloc: + raise ValueError("Picture URL must be a valid HTTP or HTTPS URL") + return v +``` + +**Step 4: Run tests to verify they pass** + +Run: `uv run python -m pytest tests/test_validation.py -v` +Expected: All PASS + +**Step 5: Commit** + +```bash +git add src/porchlight/validation.py tests/test_validation.py +git commit -m "feat: add ProfileUpdate pydantic model with email and phone validation" +``` + +--- + +### Task 3: Wire `ProfileUpdate` into manage routes + +**Files:** +- Modify: `src/porchlight/manage/routes.py:199-255` +- Create: `tests/test_manage_profile.py` + +**Step 1: Write the failing integration tests** + +Create `tests/test_manage_profile.py`: + +```python +from datetime import UTC, datetime + +from argon2 import PasswordHasher +from httpx import AsyncClient + +from porchlight.authn.password import PasswordService +from porchlight.models import PasswordCredential, User +from tests.conftest import get_csrf_token + + +async def _login(client: AsyncClient, username: str = "alice", userid: str = "user-01") -> None: + app = client._transport.app # type: ignore[union-attr] + user_repo = app.state.user_repo + cred_repo = app.state.credential_repo + + user = await user_repo.get_by_username(username) + if user is None: + user = User( + userid=userid, + username=username, + created_at=datetime.now(UTC), + updated_at=datetime.now(UTC), + ) + await user_repo.create(user) + + svc = PasswordService(hasher=PasswordHasher(time_cost=1, memory_cost=8192)) + existing = await cred_repo.get_password_by_user(user.userid) + if existing is None: + await cred_repo.create_password(PasswordCredential(user_id=user.userid, password_hash=svc.hash("password"))) + + token = await get_csrf_token(client) + await client.post( + "/login/password", + data={"username": username, "password": "password"}, + headers={"HX-Request": "true", "X-CSRF-Token": token}, + ) + + +async def test_update_profile_success(client: AsyncClient) -> None: + await _login(client) + token = await get_csrf_token(client) + response = await client.post( + "/manage/profile", + data={ + "given_name": "Alice", + "family_name": "Smith", + "preferred_username": "ally", + "email": "alice@example.com", + "phone_number": "+46701234567", + "picture": "https://example.com/alice.jpg", + "locale": "en", + }, + headers={"X-CSRF-Token": token}, + ) + assert response.status_code == 200 + assert "Profile updated" in response.text + + app = client._transport.app # type: ignore[union-attr] + user = await app.state.user_repo.get_by_userid("user-01") + assert user is not None + assert user.given_name == "Alice" + assert user.email == "alice@example.com" + assert user.phone_number == "+46701234567" + + +async def test_update_profile_invalid_email(client: AsyncClient) -> None: + await _login(client) + token = await get_csrf_token(client) + response = await client.post( + "/manage/profile", + data={"email": "not-an-email"}, + headers={"X-CSRF-Token": token}, + ) + assert response.status_code == 200 + assert "alert" in response.text + assert "email" in response.text.lower() + + +async def test_update_profile_invalid_phone(client: AsyncClient) -> None: + await _login(client) + token = await get_csrf_token(client) + response = await client.post( + "/manage/profile", + data={"phone_number": "not-a-phone"}, + headers={"X-CSRF-Token": token}, + ) + assert response.status_code == 200 + assert "alert" in response.text + assert "phone" in response.text.lower() + + +async def test_update_profile_phone_normalized(client: AsyncClient) -> None: + await _login(client) + token = await get_csrf_token(client) + response = await client.post( + "/manage/profile", + data={"phone_number": "+46 70 123 45 67"}, + headers={"X-CSRF-Token": token}, + ) + assert response.status_code == 200 + assert "Profile updated" in response.text + + app = client._transport.app # type: ignore[union-attr] + user = await app.state.user_repo.get_by_userid("user-01") + assert user is not None + assert user.phone_number == "+46701234567" + + +async def test_update_profile_invalid_picture(client: AsyncClient) -> None: + await _login(client) + token = await get_csrf_token(client) + response = await client.post( + "/manage/profile", + data={"picture": "not-a-url"}, + headers={"X-CSRF-Token": token}, + ) + assert response.status_code == 200 + assert "alert" in response.text + assert "Picture URL" in response.text + + +async def test_update_profile_field_too_long(client: AsyncClient) -> None: + await _login(client) + token = await get_csrf_token(client) + response = await client.post( + "/manage/profile", + data={"given_name": "x" * 256}, + headers={"X-CSRF-Token": token}, + ) + assert response.status_code == 200 + assert "alert" in response.text +``` + +**Step 2: Run tests to verify they fail** + +Run: `uv run python -m pytest tests/test_manage_profile.py -v` +Expected: Some tests fail (e.g. phone validation won't reject invalid, no normalization) + +**Step 3: Update the manage route handler** + +Replace `src/porchlight/manage/routes.py` lines 199-255 with: + +```python +@router.post("/profile", response_class=HTMLResponse) +async def update_profile( + request: Request, + given_name: str = Form(""), + family_name: str = Form(""), + preferred_username: str = Form(""), + email: str = Form(""), + phone_number: str = Form(""), + picture: str = Form(""), + locale: str = Form(""), +) -> Response: + session_user = get_session_user(request) + if session_user is None: + return RedirectResponse("/login", status_code=303) + + userid, _username = session_user + + try: + profile = ProfileUpdate( + given_name=given_name, + family_name=family_name, + preferred_username=preferred_username, + email=email, + phone_number=phone_number, + picture=picture, + locale=locale, + ) + except ValidationError as exc: + error = exc.errors()[0] + field = error["loc"][-1] if error["loc"] else "input" + msg = error["msg"] + # Produce user-friendly field labels + labels = { + "given_name": "Given name", + "family_name": "Family name", + "preferred_username": "Display name", + "email": "Email", + "phone_number": "Phone number", + "picture": "Picture URL", + "locale": "Locale", + } + label = labels.get(str(field), str(field)) + # Use custom message for picture URL, generic pydantic message otherwise + if "Picture URL" in msg: + display_msg = msg + else: + display_msg = f"{label}: {msg}" + return HTMLResponse(f'