docs(07): revise plans after checker review

This commit is contained in:
Jakub Zych
2026-09-22 12:37:08 +02:00
parent 57745e32a2
commit 3aed5c88c3
6 changed files with 65 additions and 22 deletions

View File

@@ -501,19 +501,19 @@ func CheckAndRecordLogin(ctx context.Context, db *gorm.DB, user *models.User, ip
| A3 | `getAvatarThumb()`'s underlying `File::getThumb($size, $size, [])` uses Winter's default resize mode (`'auto'`), matching the `mode` value already implemented in `lagoon/attach.File.Thumb` | Architecture Patterns (avatar) | If PHP's actual default mode differs (e.g. `'crop'`), the avatar thumbnail's aspect-ratio/cropping behavior would visually differ from PHP even though the endpoint and payload shape are correct — a UI regression, not a wire-contract break (the field names/URLs are unaffected). Low risk; verify against one recorded avatar-upload fixture's actual thumbnail file if pixel-parity matters, otherwise not blocking. |
| A4 | `permissions: []`, `groups: {}`, `role: null` are the only values a real Płytarium user's `getApiArray()` payload ever produces (no roles/groups are configured in this app), so no port of Winter Storm's RBAC (`Role`, `UserGroup`) is needed | Anti-Patterns / Don't Hand-Roll | If a real user does have a group or role assigned in the live PHP data, a recorded fixture (D-11's `/_user/api/v1` capture pass) will show non-empty values and the "stub as empty" plan collapses — but this will be caught automatically the moment fixtures are recorded (D-11), before any code is written against a wrong assumption, so risk is self-correcting and low. |
## Open Questions
## Open Questions (RESOLVED)
1. **Does PHP's `change-password` endpoint return a fresh JWT alongside the "Password changed" message?**
1. **RESOLVED: Does PHP's `change-password` endpoint return a fresh JWT alongside the "Password changed" message?**
- What we know: The handler body (read in full, `ApiController.php:412-515`) returns `{'message': 'Password changed', 'user': $user->getApiArray()}` — no `token` key anywhere in that response.
- What's unclear: D-20 asks the researcher to confirm this so the "invalidate all older tokens on password change" mechanism (a per-user "valid after" timestamp) doesn't lock out the very request that just changed the password.
- Recommendation: **Confirmed — PHP does NOT return a fresh token from change-password or reset-password.** The plan must therefore exempt the *presenting* token from the new "valid after" cutoff (set the per-user timestamp to one second before the presenting token's own `iat`, or equivalently special-case "the token used to authenticate this exact change-password call is allowed through even if it predates the new cutoff") so the calling device isn't logged out mid-session, exactly as D-20 anticipates. `reset-password` is unauthenticated (no presenting token to exempt), so its "valid after" write has no such carve-out to make.
2. **What is the correct `mimes` list interaction with `lagoon.Validate`'s planned `file`/`mimes` extension for the avatar endpoint specifically (vs. Phase 12's album photo needs)?**
2. **RESOLVED: What is the correct `mimes` list interaction with `lagoon.Validate`'s planned `file`/`mimes` extension for the avatar endpoint specifically (vs. Phase 12's album photo needs)?**
- What we know: PHP's rule is `avatar => required|file|mimes:jpeg,jpg,png,webp,gif|max:4000` (4000 KB, i.e. ~4MB) — confirmed in `ApiController.php:534-536`.
- What's unclear: Whether the `max:4000` unit convention (KB, Laravel's default for `file`) should be reproduced as a `lagoon.Validate` token or left as a simple, separate `http.MaxBytesReader` cap at the handler/group level (which is also required regardless, per P6 D-18, to bound the multipart body before any parsing happens at all).
- Recommendation: Use `http.MaxBytesReader` (already an established Phase 6 pattern) as the hard byte cap at the group/handler level for DoS protection, and treat `mimes:jpeg,jpg,png,webp,gif` as the only new `lagoon.Validate` token actually needed this phase — don't invent a `max` unit-conversion rule inside `lagoon.Validate` for file sizes when the existing multipart cap mechanism already covers it more cheaply.
3. **Exact PHP behavior for a migrated user row with `has_self_set_password = false` calling `change-password` in Go, where the OTP-bootstrap branch (428/503) is deferred (D-03).**
3. **RESOLVED: Exact PHP behavior for a migrated user row with `has_self_set_password = false` calling `change-password` in Go, where the OTP-bootstrap branch (428/503) is deferred (D-03).**
- What we know: D-03 states this branch is deferred with the social-login flow.
- What's unclear: Whether such a row can exist in the *current* Płytarium production data at all (the column defaults true per the PHP model's `has_self_set_password ?? true` fallback, and CONTEXT.md states "no Go path can create a user with `has_self_set_password = false`").
- Recommendation: Confirmed low-risk — since no Go-created row can have this flag false, and cutover-migrated PHP rows are out of this phase's scope (data migration happens at cutover, Phase 15), Phase 7's Go `change-password` handler can safely assume `has_self_set_password` is always true for any row it creates or touches, and simply 500-guard (fail loud, don't silently treat as self-set) if it ever encounters `false` — matching D-03's explicit "documented as deferred, not silently treated as self-set" instruction.