Mobile API: public GET /auth/registration-fields for configurable/custom signup fields (#255)#277
Conversation
…e app
The Mobile API validated custom registration fields (POST /auth/register) and
returned them for logged-in users (GET /me), but had no public discovery
endpoint — a client had no way to learn, before signup, which built-in fields
are required on this instance or which admin-defined custom fields to render.
Add public GET /api/v1/auth/registration-fields (no token, gated on the same
app-access flag as the other auth endpoints). Returns:
- registration_enabled (client tells "app disabled" from "signup closed")
- builtin_fields: the three config-driven toggles (cognome/telefono/indirizzo)
each { required: isRequired(key), configurable: true }
- custom_fields: RegistrationFields::apiDefinitions() — active only,
{ id, label, type, required }, ordered, leaking no attivo/ordine.
data_nascita/cod_fiscale/sesso are intentionally NOT in the signup contract:
register() never reads them; they are profile-only, editable via GET/PATCH /me.
Reuses existing RegistrationFields::{BUILTIN_TOGGLES, isRequired, apiDefinitions}
— no new src file. OpenAPI + manifest test updated; mobile-api 1.3.0 -> 1.4.0.
Verified live: GET returns 200 with the correct envelope; builtins reflect the
real isRequired() config, custom_fields dynamically reflect apiDefinitions()
(seeded a field -> appeared, deleted -> gone). php -l + PHPStan level 5 clean.
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAggiunge un endpoint pubblico Mobile API per scoprire i campi di registrazione, con controllo dell’accesso, rate limiting, payload documentato in OpenAPI, aggiornamento della versione del plugin e verifica nei test di idempotenza. ChangesDiscovery registrazione mobile
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🔎 Adams Review — PR #277 (mobile registration-fields)Adversarial multi-lens pass (correctness / structural blast-radius / security). Endpoint is correct, safe, and well-scoped — but the structural lens surfaced a real redundancy worth deciding on before merge.
|
/health (existing) |
/auth/registration-fields (this PR) |
|
|---|---|---|
| enabled flag | registration_enabled |
registration_enabled |
| built-in required | registration.require_cognome / require_telefono / require_indirizzo (flat booleans) |
builtin_fields.<field>.{required, configurable} (nested) |
| custom fields | registration.custom_fields = apiDefinitions() |
custom_fields = apiDefinitions() |
So a client now has two public ways to learn the same signup contract, in two shapes it must reconcile. HealthController.php:93-97.
Decide one of:
- Drop the new endpoint, have the Android register screen read the
registrationblock from/health(it already calls/healthon startup) — smallest surface. - Keep it (a dedicated, purpose-named endpoint is arguably cleaner for the register screen) but consider deprecating the
registrationblock in/health, or at least documenting that the two are intentional and which is canonical.
Not a bug — but two public contracts for one thing tends to drift. Your call.
ℹ️ Minor — discovery payload omits the always-required core fields
register() unconditionally requires nome/email/password, but the discovery payload advertises cognome (in builtin_fields) and the custom fields, not nome/email/password. A client rendering the form purely from this contract could omit nome. In practice every client hardcodes name/email/password, so this is low — but a one-line note in the payload or docs ("core fields nome/email/password are always required and not listed here") would close the gap. AuthController.php:355.
✅ Clean
- Security (L6): public + no-token is intentional and safe — schema-only disclosure equivalent to the public
/registratiweb page; no PII/user values,apiDefinitions()correctly hidesattivo/ordine, same app-access 403 gate as the other public auth endpoints, rate-limited 30/300s. - Correctness (L1): the one flagged item — manifest row
kind:'doc'asserting HTTP 200 while the handler can 403 — is a false positive: the test harness seedsmobile_api.enabled=1(mobile-api-idempotency.spec.js:86), so the endpoint always returns 200 under test. - Route correctly public inside
/api/v1, envelope matchesResponseEnvelope::success, required flags can't drift fromregister()(both callisRequired()), OpenAPI ↔ route ↔ manifest consistent.
Lenses: L1 diff-local, L2 structural, L6 security. Findings verified against the code, not just flagged.
…health
Adams-review follow-ups on the registration-fields endpoint:
- Minor finding: the discovery payload advertised the configurable toggles
(cognome/telefono/indirizzo) and custom fields but NOT nome/email/password,
which register() requires unconditionally — a client rendering purely from
the contract could omit `nome`. Now builtin_fields carries the three core
fields with {required:true, configurable:false}, so the contract is
self-complete and a client can tell mandatory-fixed from configurable.
(password_confirm stays a client-side echo, not advertised.)
- Structural finding (two public sources of truth): /health also carries a
registration summary. It shipped in 0.7.37 and has an existing client field
+ test, so removing it would be a breaking change to a public API — worse
than the redundancy. Both derive from the same RegistrationFields source, so
values cannot drift. Documented the relationship in both controllers + the
OpenAPI description: /auth/registration-fields is the canonical form-render
contract, /health carries a lightweight mirror.
Verified live: builtin_fields now returns nome/email/password (configurable
false) + the three toggles; PHPStan level 5 clean.
…tests: envelope, core fields, toggles, no profile-leak, custom-field shape, public)
1 participant
Adds the missing discovery endpoint so the Android app (and any client) can render the registration form for #255's configurable + custom fields.
Why
POST /auth/registeralready validates custom fields andGET /mereturns them for logged-in users, but there was no public endpoint exposing the field schema before signup — so a client couldn't know which built-in fields are required on this instance or which admin-defined custom fields to show.Endpoint
GET /api/v1/auth/registration-fields— public (no token), gated on the same app-access flag as the other auth endpoints. Returns:registration_enabled— lets the client distinguish "app disabled" from "signup closed"builtin_fields— the three config-driven toggles (cognome/telefono/indirizzo), each{ required: isRequired(key), configurable: true }custom_fields—RegistrationFields::apiDefinitions(): active fields only,{ id, label, type, required }, ordered, leaking noattivo/ordinedata_nascita/cod_fiscale/sessoare intentionally not in the signup contract (register() never reads them; profile-only, viaGET/PATCH /me).Implementation
Reuses existing
RegistrationFields::{BUILTIN_TOGGLES, isRequired, apiDefinitions}— no new controller file. OpenAPI spec +mobile-api-idempotencymanifest test updated; mobile-api plugin1.3.0 → 1.4.0.Verified (live, on the dev install)
GETreturns 200 with the correct{data, meta, error}envelope.builtin_fieldsreflects the realisRequired()config.custom_fieldsreflectsapiDefinitions()dynamically — seeded a field → it appeared, deleted it → gone.php -l+ PHPStan level 5 clean; route ↔ OpenAPI ↔ manifest consistent.The matching Android client work (models,
PinakesApi, dynamic register/profile fields) is in a separate PR on thePinakes-Androidrepo.Summary by CodeRabbit
Nuove funzionalità
Correzioni
Manutenzione