API Review Checklist — GIW POC Identity Platform
Status: POC · Date: 2026-09-21 For: Architecture · Backend · Frontend · Security · Integration
Answers are filled in as the POC actually behaves. ⚠️ marks something a reviewer should push back on. Bring objections to §10.
1. Architecture
| Question | Answer |
|---|---|
| Who owns each API? | HR verify → HR team (TBC, T-HR-SOR). Entitlement, Session, Portal → Galaxy platform. Keycloak → Galaxy CIAM. |
| What is the system of record for employment status? | The HR system. Never Keycloak. Status is re-fetched at every login and never cached. |
| What is the system of record for identity? | Keycloak (Galaxy ID). ADR-002. |
| What decides access? | The Entitlement Service, and nothing else. Keycloak issues a token and stops. |
| Can any service open the network directly? | No. grant requires an entitlementId or returns 422. |
Is Session separable from Entitlement? |
Yes — separate services, separate calls, ADR-006. |
| Does the design survive HR being down? | Yes: authentication fails closed. No partial access, no cached bypass. |
| ⚠️ Is a second OIDC client the right way to express the employee flow? | It works and keeps the portal a plain relying party. The cost is two audiences and two token shapes. Reviewer call. |
2. Security
| Question | Answer |
|---|---|
| Authorization Code + PKCE? | Yes, S256 enforced on both clients. |
| Are implicit/ROPC disabled? | Yes on both clients. |
Is state single-use? |
Yes, cleared on the first callback. |
Is the id_token nonce checked? |
Yes. |
| Is the token fully validated downstream? | Yes — alg allowlist, JWKS signature, iss, aud, exp, nbf. |
| Can the caller assert its own identity? | No. Body subjectId contradicting sub → 403. |
| Service-to-service auth? | X-API-Key, constant-time compared. ⚠️ Static shared secret — production needs mTLS. |
| Is any secret committed? | No. .env is git-ignored; the HR key reaches Keycloak by env, not by realm-export.json. |
| TLS? | ⚠️ Plain HTTP. Localhost only. |
| Brute force? | Realm protection on password login; 5 attempts per auth session on the employee form. ⚠️ Per-session, therefore bypassable. |
| Rate limiting? | HR only (30/60 s). ⚠️ None on Entitlement or Session. |
| Account enumeration? | Prevented: identical body, constant-time compare, uniform scan, identical UI message. Asserted by test. |
| Replay protection? | Code single-use, state burned, nonce checked, idempotency keys on write paths. |
⚠️ Is POST /admin/fault authenticated? |
No, and it must be deleted before any deployment. It exists so failure modes can be tested. |
3. Privacy
| Question | Answer |
|---|---|
| Is there PII? | Yes — employeeId, email, sub. |
| Is there sensitive personal data? | Yes — citizenId (CCCD), Law 91/2025/QH15. |
| Is the CCCD stored? | No. Not in Keycloak, not in any service, not in any cache. |
| Is the CCCD logged? | No. Enforced by an automated test that greps all five container logs. |
| Is the CCCD in a token? | No. Enforced by an automated test over every claim. |
| Is the CCCD in an error response? | No. The error model has no field for it. |
| Is the CCCD masked in the UI? | Yes, type=password, autocomplete="off", not echoed on a failed attempt. |
| Is test data synthetic? | Yes, and the file says so. |
| Is consent captured? | ⚠️ No. Required in production for the CCCD path. |
| Is there an erasure path? | ⚠️ No. Required in production. |
| Is Controller/Processor defined? | ⚠️ No — decision D7 is open. This POC must not touch real HR data until it is. |
| ⚠️ Should CCCD be used at all? | ADR-003 and D3 say remove it. This POC implements it because the task order asked. Production: TBC. |
4. Integration
| Question | Answer |
|---|---|
| Is the HR contract minimal? | Yes — verdict, employeeRef, status, company, eligibility. No name, department or contact detail. |
| Is retry safe? | HR verify: yes, read-only. Token exchange: no, code is single-use. Entitlement/Session: yes, idempotency keys. |
| Is idempotency implemented or only documented? | Implemented, and tested — duplicate create and duplicate revoke both return replayed: true. |
| What happens on duplicate identity? | IDENTITY_LINK_CONFLICT, flow stops. No guessing, no silent merge. |
| Are timeouts explicit? | Yes, on every outbound call. Values are POC assumptions. |
| What if the HR contract changes? | The blast radius is HrVerificationClient plus the four claim mappings. A field rename breaks authentication for every employee — versioning is mandatory before HR is real. |
| Is there an async callback? | No. When one is added it needs correlationId, subjectId, sessionId, an idempotency key and an HMAC over the raw body — see API-MATRIX.md §4 and ADR-009. |
5. Error handling
| Question | Answer |
|---|---|
| Is there one error model? | Yes, four fields, used by all custom APIs. |
| Are codes stable and machine-readable? | Yes, 14 codes in ERROR-CATALOG.md. |
| Do errors leak internals? | No stack traces, no hostnames, no raw upstream bodies. |
| Does a failure ever fall through to access? | No. Every failure path was tested and none grants access. |
| Are failure cases tested? | Yes — 11 of the 39 tests are failure paths. |
6. Observability
| Question | Answer |
|---|---|
| Correlation ID? | Yes, X-Correlation-ID on every request and response, generated if absent. |
| Does it span the whole journey? | Yes: Portal → Keycloak → Authenticator → HR → Entitlement → Session. |
| Is it derived from personal data? | No — corr-<uuid4>, deliberately. |
| Structured logs? | Yes, single-line JSON everywhere. |
| Is the audit trail sufficient to answer "why did this person get access"? | Yes: correlationId → employeeRef → entitlementId → sessionId. |
| Metrics / tracing? | ⚠️ None. No Prometheus, no OpenTelemetry export. |
| Alerting? | ⚠️ None. |
7. Performance
| Question | Answer |
|---|---|
| Stated latency budget? | ⚠️ None. Only timeouts are set. |
| HR timeout | 3000 ms, 2 attempts → worst case ~6 s before the user sees a failure. ⚠️ Long for a captive portal. |
| Token exchange | 5000 ms |
| Entitlement / Session | 5000 ms each |
| Load tested? | ⚠️ No. Not attempted. |
| Known bottleneck | Portal session map and both in-memory stores grow without eviction. |
8. Versioning and compatibility
| Question | Answer |
|---|---|
| Versioning scheme? | URI path, /api/v1/..., on every custom API. |
| Standard OIDC endpoints? | Versioned by Keycloak; not ours to version. |
| Breaking change rule | Removing a field, renaming a field, narrowing a type, adding a required request field, or changing an error code is breaking → needs /v2. |
| Non-breaking | Adding an optional request field, adding a response field, adding a new error code that existing clients treat as unknown. |
| Deprecation policy | ⚠️ Not defined. Proposal: announce, run v1 and v2 side by side for one release cycle, then remove. |
| Token claim compatibility | Adding a claim is safe. Removing or retyping user_type, employee_verified, employee_ref or company breaks entitlement evaluation. |
| If HR changes its contract | Keycloak is the only consumer, so the blast radius is small — but it is on the authentication path, so a silent change locks every employee out. Contract tests required before HR is real. |
9. Test coverage
| Area | Tests |
|---|---|
| Health | 2 |
| Customer flow | 7 |
| Employee flow | 5 |
| Failure cases | 11 |
| API contract | 11 |
| CCCD containment | 2 |
| Total | 39, all passing |
Run: node tests/e2e.mjs
10. Reviewer sign-off
| Role | Name | Date | Verdict | Objections |
|---|---|---|---|---|
| Architecture | ||||
| Backend | ||||
| Frontend | ||||
| Security | ||||
| Integration | ||||
| Legal / DPO |
The Legal/DPO row is not optional. The CCCD question (§3, last row) cannot be closed by any of the other five.