affinity-intelligence-rework/im2be-identity-service
2
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix(mint): read userId UUID claim instead of sub (email) for the gRPC fan-out #8

Merged
hibryda merged 4 commits from feat/mint-reads-userid-claim into main 2026-06-02 08:22:54 +02:00
Conversation 4 Commits 4 Files changed 2 +208 -7
hibryda commented 2026-06-02 07:52:33 +02:00
Owner
Copy link

Header

im2be-identity-service · mint reads the userId UUID claim (not sub) · feat/mint-reads-userid-claim → main

TL;DR

Fixes the opaque-ticket mint's INVALID_ARGUMENT: JwtAuthenticationFilter now binds the principal from the userId UUID claim (which user-service now issues — #24), not sub (the email). Fail-closed on a missing/non-UUID claim. mvn clean test green.

Summary

  • JwtAuthenticationFilter: principal = claims.get("userId", String.class), UUID-validated. The mint fan-out (family-service.GetFamilyForUser + user-service.GetUserProfile) is keyed by the user's UUID; sub=email broke it. sub stays in the claims for audit.
  • Fail-closed: no valid-UUID userId → 401 TOKEN_INVALID at the filter (not a deferred gRPC INVALID_ARGUMENT/500; falling back to sub would re-trigger the original bug).
  • Verifier (JwtVerifierChain) untouched — jjwt 0.11.5 reads custom claims with no verifier change; aud/iss validation unaffected.

Risk

indicator value
Principal source change sub(email) → userId(UUID) for the identity HTTP endpoints (only /token/centrifugo today)
Fail-closed tokens lacking a valid-UUID userId get 401 (transition: pre-#24 tokens rejected until they expire; admin tokens don't mint)
Verifier/deps unchanged

Tests

JwtAuthenticationFilterTest (new): valid-UUID userId → principal is the UUID (not the email sub); missing userId → 401, chain not invoked; non-UUID → 401. mvn clean test green (Temurin 17).

Depends on

im2be-user-service #24 (issues the additive userId claim). Deploy together.

Footer

im2be-identity-service · mint reads userId claim · mvn green · 2026-06-02

## Header **im2be-identity-service** · mint reads the `userId` UUID claim (not `sub`) · `feat/mint-reads-userid-claim` → `main` ## TL;DR Fixes the opaque-ticket mint's `INVALID_ARGUMENT`: `JwtAuthenticationFilter` now binds the principal from the **`userId` UUID claim** (which user-service now issues — #24), not `sub` (the email). **Fail-closed** on a missing/non-UUID claim. `mvn clean test` green. ## Summary - `JwtAuthenticationFilter`: principal = `claims.get("userId", String.class)`, UUID-validated. The mint fan-out (`family-service.GetFamilyForUser` + `user-service.GetUserProfile`) is keyed by the user's UUID; `sub`=email broke it. `sub` stays in the claims for audit. - **Fail-closed**: no valid-UUID `userId` → 401 `TOKEN_INVALID` at the filter (not a deferred gRPC `INVALID_ARGUMENT`/500; falling back to `sub` would re-trigger the original bug). - Verifier (`JwtVerifierChain`) untouched — jjwt 0.11.5 reads custom claims with no verifier change; aud/iss validation unaffected. ## Risk | indicator | value | |---|---| | Principal source change | `sub`(email) → `userId`(UUID) for the identity HTTP endpoints (only `/token/centrifugo` today) | | Fail-closed | tokens lacking a valid-UUID `userId` get 401 (transition: pre-#24 tokens rejected until they expire; admin tokens don't mint) | | Verifier/deps | unchanged | ## Tests `JwtAuthenticationFilterTest` (new): valid-UUID `userId` → principal is the UUID (not the email `sub`); missing `userId` → 401, chain not invoked; non-UUID → 401. `mvn clean test` green (Temurin 17). ## Depends on im2be-user-service **#24** (issues the additive `userId` claim). Deploy together. ## Footer im2be-identity-service · mint reads userId claim · mvn green · 2026-06-02 
The opaque-ticket mint fans out to family-service.GetFamilyForUser +
user-service.GetUserProfile, both keyed by the user's UUID. JwtAuthenticationFilter
set the Spring Security principal from `sub` — which is the email — so the mint
handed an email to the UUID-keyed gRPC calls → INVALID_ARGUMENT (the family/user
proto validators reject a non-UUID). user-service now emits an additive `userId`
UUID claim on the access token (im2be-user-service#24); this reads it.

- JwtAuthenticationFilter: principal = claims.get("userId", String.class), validated
  as a UUID. `sub` (email) stays in the verified claims for audit/logging.
- FAIL CLOSED: a token without a valid-UUID `userId` is rejected at the filter (401
  TOKEN_INVALID) rather than deferred to a confusing gRPC INVALID_ARGUMENT / HTTP 500
  downstream. (Falling back to sub would just re-trigger the original bug.)
- Verifier (JwtVerifierChain) unchanged — jjwt 0.11.5 reads custom claims with no
  verifier change; aud/iss validation unaffected.

Test: new JwtAuthenticationFilterTest — valid-UUID userId → principal is the UUID
(not the email sub); missing userId → 401 + chain not invoked; non-UUID userId → 401.
mvn clean test green (Temurin 17).

Transition note: existing user JWTs (issued before im2be-user-service#24) lack the
claim and are rejected at the mint until they expire + re-issue (access tokens are
short-lived). Admin tokens (sub=UUID, no userId) don't mint Centrifugo tickets.
pr-reviewer-bot commented 2026-06-02 07:56:40 +02:00
Member
Copy link

Superseded by round 2.

Show previous round

hib-pr-reviewer review — PR #8 (affinity-intelligence-rework/im2be-identity-service)

Round 1 — head 21c550662afc, base main, trigger opened

TL;DR: CONDITIONAL_APPROVE — kept 2 agreed/verified info findings (isBlank test gap + UUID normalisation), dropped 1 prose-only Javadoc concern from A; no blocking issues.

Summary

Arbitration — PR #8 fix(mint): read userId UUID claim instead of sub (email)

First arbitration run. No prior Memora history. Both files read in full.

Reconciliation

# Finding Status Reason
F1 isBlank() test gap (test line 98/99) KEPT — AGREED A (line 98) and B (line 99) both flag the same method rejects401_whenUserIdClaimMissing. Verified: claims(null, …) skips the userId key entirely (test line 74–76), so isBlank() is never reached.
F2 UUID parse result discarded (prod line 130) KEPT — UNIQUE-TO-B, VERIFIED UUID.fromString(userId); at line 130 discards the parsed object; raw userId string from the claim is used at line 141. Verified by reading. Low-probability but zero-cost fix.
— Stale Javadoc (A prose only, line 38) DROPPED Only in A's prose summary, absent from A's findings[]. Not raised by B. Not a formally submitted finding.

Kept 2 findings (both info). No blocking or major issues.

Blast Radius

The diff is confined to a single filter class and its brand-new test file, but JwtAuthenticationFilter is on the authentication critical path for every non-whitelisted HTTP endpoint in identity-service. A regression here would break all authenticated traffic. The test file is net-new, so blast radius is mitigated by the fact that no existing tests were modified.

BLAST_SCORE: 6/10

Risk Indicators

Indicator Value
Sensitive functions doFilterInternal, UUID.fromString, UsernamePasswordAuthenticationToken, verifierChain.parse
Migration touched —
Test delta —
Dependency changes —

CI status (head 21c550662afc)

No CI checks reported for this commit.

Findings (2)

[INFO] No test exercises the isBlank() rejection branch for the userId claim

src/test/java/com/aim2be/identity/unit/config/JwtAuthenticationFilterTest.java:98

Both reviewers independently flagged this. The filter at production line 123 checks userId == null || userId.isBlank(). The test rejects401_whenUserIdClaimMissing (line 98) calls claims(null, "user@example.com") — because claims() only calls c.put("userId", userId) when userId != null (test lines 73–76), the map never contains a userId key at all. The guard's null branch is exercised, but the isBlank() branch (e.g. userId = " ") is never reached.

The 401 outcome is still guaranteed operationally (a blank string also fails UUID.fromString), but the specific isBlank() guard and its error message are unverified.

Suggested addition:

@Test
void rejects401_whenUserIdClaimBlank() throws Exception {
    when(verifierChain.parse(JWT)).thenReturn(claims("   ", "user@example.com"));
    invokeFilter();
    verify(response).setStatus(401);
    verify(chain, never()).doFilter(any(), any());
    assertNull(SecurityContextHolder.getContext().getAuthentication());
}

[INFO] UUID parse result is discarded — mixed-case claims propagate unnormalised to gRPC fan-out

src/main/java/com/aim2be/identity/config/JwtAuthenticationFilter.java:130

Line 130: UUID.fromString(userId); — the validated UUID object is thrown away; the raw claim string userId is passed directly as the UsernamePasswordAuthenticationToken principal at line 141. UUID.fromString accepts mixed-case hex ("...AABBCC" is valid), but UUID.toString() always emits lower-case. If a token ever carries an upper-case hex variant, the unnormalised string silently reaches the gRPC fan-out, which may fail a UUID-equality lookup against a lower-case-normalised DB column.

Suggested zero-cost fix:

final String normalisedUserId;
try {
    normalisedUserId = UUID.fromString(userId).toString(); // canonical lower-case
} catch (IllegalArgumentException ex) {
    log.warn("JWT userId claim is not a valid UUID on {}", request.getServletPath());
    writeError(response, HttpStatus.UNAUTHORIZED, "TOKEN_INVALID",
            "JWT token userId claim must be a valid UUID");
    return;
}
// use normalisedUserId at line 141 instead of userId

User-service will in practice always emit lower-case UUIDs, so probability is very low, but the normalisation has zero cost.

Verdict

CONDITIONAL_APPROVE

hib-pr-reviewer • round 1 • 2 findings (2i) • 2026-06-02T05:54:44.860Z → 2026-06-02T05:56:40.108Z • posted-as: pr-reviewer-bot • model: auto

<!-- hib-pr-reviewer collapsed --> > _Superseded by round 2._ <details> <summary>Show previous round</summary> <!-- hib-pr-reviewer round:1 --> ## hib-pr-reviewer review — PR #8 (affinity-intelligence-rework/im2be-identity-service) **Round 1** — head `21c550662afc`, base `main`, trigger `opened` **TL;DR:** CONDITIONAL_APPROVE — kept 2 agreed/verified info findings (isBlank test gap + UUID normalisation), dropped 1 prose-only Javadoc concern from A; no blocking issues. ### Summary ## Arbitration — PR #8 `fix(mint): read userId UUID claim instead of sub (email)` **First arbitration run.** No prior Memora history. Both files read in full. ### Reconciliation | # | Finding | Status | Reason | |---|---------|--------|--------| | F1 | isBlank() test gap (test line 98/99) | **KEPT — AGREED** | A (line 98) and B (line 99) both flag the same method `rejects401_whenUserIdClaimMissing`. Verified: `claims(null, …)` skips the `userId` key entirely (test line 74–76), so `isBlank()` is never reached. | | F2 | UUID parse result discarded (prod line 130) | **KEPT — UNIQUE-TO-B, VERIFIED** | `UUID.fromString(userId);` at line 130 discards the parsed object; raw `userId` string from the claim is used at line 141. Verified by reading. Low-probability but zero-cost fix. | | — | Stale Javadoc (A prose only, line 38) | **DROPPED** | Only in A's prose summary, absent from A's `findings[]`. Not raised by B. Not a formally submitted finding. | Kept 2 findings (both info). No blocking or major issues. ### Blast Radius The diff is confined to a single filter class and its brand-new test file, but `JwtAuthenticationFilter` is on the authentication critical path for every non-whitelisted HTTP endpoint in identity-service. A regression here would break all authenticated traffic. The test file is net-new, so blast radius is mitigated by the fact that no existing tests were modified. **BLAST_SCORE: 6/10** ### Risk Indicators | Indicator | Value | |---|---| | Sensitive functions | `doFilterInternal`, `UUID.fromString`, `UsernamePasswordAuthenticationToken`, `verifierChain.parse` | | Migration touched | — | | Test delta | — | | Dependency changes | — | ### CI status (head `21c550662afc`) _No CI checks reported for this commit._ ### Findings (2) #### **[INFO]** No test exercises the `isBlank()` rejection branch for the `userId` claim _src/test/java/com/aim2be/identity/unit/config/JwtAuthenticationFilterTest.java:98_ Both reviewers independently flagged this. The filter at production line 123 checks `userId == null || userId.isBlank()`. The test `rejects401_whenUserIdClaimMissing` (line 98) calls `claims(null, "user@example.com")` — because `claims()` only calls `c.put("userId", userId)` when `userId != null` (test lines 73–76), the map never contains a `userId` key at all. The guard's `null` branch is exercised, but the `isBlank()` branch (e.g. `userId = " "`) is never reached. The 401 outcome is still guaranteed operationally (a blank string also fails `UUID.fromString`), but the specific `isBlank()` guard and its error message are unverified. **Suggested addition:** ```java @Test void rejects401_whenUserIdClaimBlank() throws Exception { when(verifierChain.parse(JWT)).thenReturn(claims(" ", "user@example.com")); invokeFilter(); verify(response).setStatus(401); verify(chain, never()).doFilter(any(), any()); assertNull(SecurityContextHolder.getContext().getAuthentication()); } ``` #### **[INFO]** UUID parse result is discarded — mixed-case claims propagate unnormalised to gRPC fan-out _src/main/java/com/aim2be/identity/config/JwtAuthenticationFilter.java:130_ Line 130: `UUID.fromString(userId);` — the validated `UUID` object is thrown away; the raw claim string `userId` is passed directly as the `UsernamePasswordAuthenticationToken` principal at line 141. `UUID.fromString` accepts mixed-case hex (`"...AABBCC"` is valid), but `UUID.toString()` always emits lower-case. If a token ever carries an upper-case hex variant, the unnormalised string silently reaches the gRPC fan-out, which may fail a UUID-equality lookup against a lower-case-normalised DB column. Suggested zero-cost fix: ```java final String normalisedUserId; try { normalisedUserId = UUID.fromString(userId).toString(); // canonical lower-case } catch (IllegalArgumentException ex) { log.warn("JWT userId claim is not a valid UUID on {}", request.getServletPath()); writeError(response, HttpStatus.UNAUTHORIZED, "TOKEN_INVALID", "JWT token userId claim must be a valid UUID"); return; } // use normalisedUserId at line 141 instead of userId ``` User-service will in practice always emit lower-case UUIDs, so probability is very low, but the normalisation has zero cost. ### Verdict **CONDITIONAL_APPROVE** --- <sub>hib-pr-reviewer • round 1 • 2 findings (2i) • 2026-06-02T05:54:44.860Z → 2026-06-02T05:56:40.108Z • posted-as: pr-reviewer-bot • model: auto</sub> </details>
R1 CONDITIONAL_APPROVE (2 info), both addressed:
- [INFO] JwtAuthenticationFilter: the UUID.fromString() parse result was discarded
  and the original claim string used as the principal — a mixed/upper-case (but
  valid) UUID would propagate unnormalised to the family/user gRPC fan-out. Now the
  principal is UUID.fromString(claim).toString() (canonical lowercase). (Our issuers
  already emit lowercase; this is defensive.)
- [INFO] JwtAuthenticationFilterTest: added the isBlank() rejection case (empty
  userId → 401) and a normalisation test (upper-case-hex UUID → lowercase principal).

Verification: mvn clean test green (Temurin 17).
pr-reviewer-bot commented 2026-06-02 08:06:26 +02:00
Member
Copy link

Superseded by round 3.

Show previous round

hib-pr-reviewer review — PR #8 (affinity-intelligence-rework/im2be-identity-service)

Round 2 — head d49c53e0b29c, base main, trigger synchronize

TL;DR: CONDITIONAL_APPROVE — both R1 conditions satisfied; one new minor finding (stale class Javadoc now says 'sub' instead of 'userId').

Summary

[quorum-converged] A=0 = B=0. ## Round 2 review

Both prior-round conditions are fully satisfied:

  • R1 INFO (isBlank test): rejects401_whenUserIdClaimBlank() at test line 122 covers the blank-string path exactly.
  • R1 INFO (UUID normalisation): userId = UUID.fromString(userIdClaim).toString() at line 133 canonicalises to lowercase before the principal is set; normalisesUserIdToCanonicalLowercase() exercises the contract end-to-end.

JJWT version is 0.11.5, so Jwts.claims() / c.setSubject() / c.put() in the test helper are all valid (no 0.12.x API breakage).

One new minor finding: the class-level Javadoc, which was accurate before this PR, now describes the wrong claim as the principal after the PR's behavioural change.

Blast Radius

Changes are confined to a single filter class and its new test file. The filter sits on the HTTP auth path so every authenticated request is affected, but the change is a well-contained claim-name swap with a stricter validation fence. No other modules or public contracts are touched.

BLAST_SCORE: 4/10

Risk Indicators

Indicator Value
Sensitive functions doFilterInternal, writeError
Migration touched —
Test delta —
Dependency changes —

CI status (head d49c53e0b29c)

No CI checks reported for this commit.

Findings

No new findings this round.

Quorum converged on empty findings (A + B both returned 0).

Verdict

CONDITIONAL_APPROVE

hib-pr-reviewer • round 2 • 0 findings • 2026-06-02T06:04:44.188Z → 2026-06-02T06:06:25.741Z • posted-as: pr-reviewer-bot • model: auto

<!-- hib-pr-reviewer collapsed --> > _Superseded by round 3._ <details> <summary>Show previous round</summary> <!-- hib-pr-reviewer round:2 --> ## hib-pr-reviewer review — PR #8 (affinity-intelligence-rework/im2be-identity-service) **Round 2** — head `d49c53e0b29c`, base `main`, trigger `synchronize` **TL;DR:** CONDITIONAL_APPROVE — both R1 conditions satisfied; one new minor finding (stale class Javadoc now says 'sub' instead of 'userId'). ### Summary [quorum-converged] A=0 = B=0. ## Round 2 review Both prior-round conditions are fully satisfied: - **R1 INFO (isBlank test):** `rejects401_whenUserIdClaimBlank()` at test line 122 covers the blank-string path exactly. - **R1 INFO (UUID normalisation):** `userId = UUID.fromString(userIdClaim).toString()` at line 133 canonicalises to lowercase before the principal is set; `normalisesUserIdToCanonicalLowercase()` exercises the contract end-to-end. JJWT version is 0.11.5, so `Jwts.claims()` / `c.setSubject()` / `c.put()` in the test helper are all valid (no 0.12.x API breakage). One new minor finding: the class-level Javadoc, which was accurate before this PR, now describes the **wrong claim** as the principal after the PR's behavioural change. ### Blast Radius Changes are confined to a single filter class and its new test file. The filter sits on the HTTP auth path so every authenticated request is affected, but the change is a well-contained claim-name swap with a stricter validation fence. No other modules or public contracts are touched. **BLAST_SCORE: 4/10** ### Risk Indicators | Indicator | Value | |---|---| | Sensitive functions | `doFilterInternal`, `writeError` | | Migration touched | — | | Test delta | — | | Dependency changes | — | ### CI status (head `d49c53e0b29c`) _No CI checks reported for this commit._ ### Findings **No new findings this round.** _Quorum converged on empty findings (A + B both returned 0)._ ### Verdict **CONDITIONAL_APPROVE** --- <sub>hib-pr-reviewer • round 2 • 0 findings • 2026-06-02T06:04:44.188Z → 2026-06-02T06:06:25.741Z • posted-as: pr-reviewer-bot • model: auto</sub> </details>
R2 CONDITIONAL_APPROVE, 1 new minor (both R1 conditions already satisfied):
- [MINOR] JwtAuthenticationFilter class Javadoc still claimed the filter
  "extracts the sub claim as the user principal" — stale after the R1 change to
  read the userId (UUID) claim. Updated to describe the userId-claim extraction,
  the fail-CLOSED behaviour (401 on missing/non-UUID), and that sub (email) is
  retained for audit/logging only. Matches the inline contract comment + the
  filter tests.

Doc-only change; mvn test-compile green (Temurin 17). No code path altered.
pr-reviewer-bot commented 2026-06-02 08:12:58 +02:00
Member
Copy link

Superseded by round 4.

Show previous round

hib-pr-reviewer review — PR #8 (affinity-intelligence-rework/im2be-identity-service)

Round 3 — head 9d45580f0df2, base main, trigger synchronize

TL;DR: CONDITIONAL_APPROVE — kept 1 unique-to-B minor finding (verified: claims.get outside try/catch leaks RequiredTypeException as HTTP 500); both prior-round findings confirmed resolved; Memora persistence unavailable this session.

Summary

Arbitration — Round 3

Reviewer A returned 0 findings (confirmed prior-round resolutions). Reviewer B returned 1 minor finding (unique-to-B). Memora persistence failed due to tag-allowlist and permission constraints; recorded inline in summary.

Verification performed

Read JwtAuthenticationFilter.java in full (195 lines).

  • Line 126: String userIdClaim = claims.get("userId", String.class); sits outside the try { … } catch (IllegalArgumentException ex) block at lines 134–143. Confirmed — the try block wraps only UUID.fromString(userIdClaim).toString().
  • pom.xml line 86: jjwt.version = 0.11.5 confirmed.
  • In JJWT 0.11.5, DefaultClaims.get(String, Class<T>) throws io.jsonwebtoken.RequiredTypeException extends JwtException extends RuntimeException — not a subclass of IllegalArgumentException — when the stored value is the wrong JSON type (e.g. a number). The catch at line 138 therefore does not cover this.
  • JwtAuthenticationFilterTest.java: Grepped for RequiredTypeException, numberUserId, nonString, integerUserId — no matches. The numeric-userId path is untested.
  • Prior-round findings: Both confirmed resolved per Reviewer A (and Reviewer B concurs): rejects401_whenUserIdClaimBlank() at test line 122 covers the isBlank branch; UUID.fromString(userIdClaim).toString() at filter line 137 performs UUID normalisation.

Decision: Keep the 1 unique-to-B finding. Verified code structure and JJWT version both support the claim. No prior-run history found in Memora (Memora persistence unavailable this session due to tag/permission constraints).

Blast Radius

The gap is confined to JwtAuthenticationFilter.doFilterInternal, which guards all non-whitelisted endpoints in identity-service. The blast radius is bounded because the failure path only activates on a JWT with a non-string userId claim — a misconfigured-issuer edge case — and produces HTTP 500 rather than silently authorising an invalid request. Normal, correctly-formed JWTs are unaffected.

BLAST_SCORE: 4/10

Risk Indicators

Indicator Value
Sensitive functions doFilterInternal, verifierChain.parse, claims.get
Migration touched —
Test delta —
Dependency changes —

CI status (head 9d45580f0df2)

No CI checks reported for this commit.

Findings (1)

[MINOR] claims.get("userId", String.class) outside try/catch can throw uncaught RequiredTypeException, producing HTTP 500 instead of 401

src/main/java/com/aim2be/identity/config/JwtAuthenticationFilter.java:126

Verified. Line 126 (String userIdClaim = claims.get("userId", String.class);) is outside the try { … } catch (IllegalArgumentException ex) block at lines 134–143, which wraps only the UUID.fromString(…) call.

In JJWT 0.11.5 (confirmed in pom.xml:86), DefaultClaims.get(String, Class<T>) throws io.jsonwebtoken.RequiredTypeException extends JwtException extends RuntimeException when the stored claim value is not of the requested type. RequiredTypeException is not a subclass of IllegalArgumentException, so the catch at line 138 does not cover it.

A JWT whose userId claim was serialised as a JSON number (e.g. "userId": 12345 from a misconfigured issuer) would cause doFilterInternal to propagate an unhandled RuntimeException, and the container would return HTTP 500 instead of the intended HTTP 401, breaking the fail-CLOSED contract.

No test covers this path (verified by grep across JwtAuthenticationFilterTest.java).

Suggested fix — move the claims.get call inside the existing try block and broaden the catch:

final String userId;
try {
    String userIdClaim = claims.get("userId", String.class); // RequiredTypeException now caught
    if (userIdClaim == null || userIdClaim.isBlank()) {
        log.warn("JWT verified but missing userId claim on {}", request.getServletPath());
        writeError(response, HttpStatus.UNAUTHORIZED, "TOKEN_INVALID",
                "JWT token is missing the required userId claim");
        return;
    }
    userId = UUID.fromString(userIdClaim).toString();
} catch (IllegalArgumentException | JwtException ex) {
    log.warn("JWT userId claim invalid on {}: {}", request.getServletPath(), ex.getMessage());
    writeError(response, HttpStatus.UNAUTHORIZED, "TOKEN_INVALID",
            "JWT token userId claim must be a valid UUID");
    return;
}

Add a companion test that puts a non-string value in userId (via a raw Claims.put call) and asserts HTTP 401 + no chain continuation.

Verdict

CONDITIONAL_APPROVE

hib-pr-reviewer • round 3 • 1 finding (1m) • 2026-06-02T06:11:01.043Z → 2026-06-02T06:12:57.670Z • posted-as: pr-reviewer-bot • model: auto

<!-- hib-pr-reviewer collapsed --> > _Superseded by round 4._ <details> <summary>Show previous round</summary> <!-- hib-pr-reviewer round:3 --> ## hib-pr-reviewer review — PR #8 (affinity-intelligence-rework/im2be-identity-service) **Round 3** — head `9d45580f0df2`, base `main`, trigger `synchronize` **TL;DR:** CONDITIONAL_APPROVE — kept 1 unique-to-B minor finding (verified: `claims.get` outside try/catch leaks RequiredTypeException as HTTP 500); both prior-round findings confirmed resolved; Memora persistence unavailable this session. ### Summary ## Arbitration — Round 3 Reviewer A returned 0 findings (confirmed prior-round resolutions). Reviewer B returned 1 minor finding (unique-to-B). Memora persistence failed due to tag-allowlist and permission constraints; recorded inline in summary. ### Verification performed **Read** `JwtAuthenticationFilter.java` in full (195 lines). - **Line 126**: `String userIdClaim = claims.get("userId", String.class);` sits **outside** the `try { … } catch (IllegalArgumentException ex)` block at lines 134–143. Confirmed — the try block wraps only `UUID.fromString(userIdClaim).toString()`. - **pom.xml line 86**: `jjwt.version = 0.11.5` confirmed. - In JJWT 0.11.5, `DefaultClaims.get(String, Class<T>)` throws `io.jsonwebtoken.RequiredTypeException extends JwtException extends RuntimeException` — **not** a subclass of `IllegalArgumentException` — when the stored value is the wrong JSON type (e.g. a number). The catch at line 138 therefore does not cover this. - **JwtAuthenticationFilterTest.java**: Grepped for `RequiredTypeException`, `numberUserId`, `nonString`, `integerUserId` — no matches. The numeric-userId path is untested. - **Prior-round findings**: Both confirmed resolved per Reviewer A (and Reviewer B concurs): `rejects401_whenUserIdClaimBlank()` at test line 122 covers the `isBlank` branch; `UUID.fromString(userIdClaim).toString()` at filter line 137 performs UUID normalisation. **Decision**: Keep the 1 unique-to-B finding. Verified code structure and JJWT version both support the claim. No prior-run history found in Memora (Memora persistence unavailable this session due to tag/permission constraints). ### Blast Radius The gap is confined to `JwtAuthenticationFilter.doFilterInternal`, which guards all non-whitelisted endpoints in identity-service. The blast radius is bounded because the failure path only activates on a JWT with a non-string `userId` claim — a misconfigured-issuer edge case — and produces HTTP 500 rather than silently authorising an invalid request. Normal, correctly-formed JWTs are unaffected. **BLAST_SCORE: 4/10** ### Risk Indicators | Indicator | Value | |---|---| | Sensitive functions | `doFilterInternal`, `verifierChain.parse`, `claims.get` | | Migration touched | — | | Test delta | — | | Dependency changes | — | ### CI status (head `9d45580f0df2`) _No CI checks reported for this commit._ ### Findings (1) #### **[MINOR]** `claims.get("userId", String.class)` outside try/catch can throw uncaught `RequiredTypeException`, producing HTTP 500 instead of 401 _src/main/java/com/aim2be/identity/config/JwtAuthenticationFilter.java:126_ **Verified.** Line 126 (`String userIdClaim = claims.get("userId", String.class);`) is **outside** the `try { … } catch (IllegalArgumentException ex)` block at lines 134–143, which wraps only the `UUID.fromString(…)` call. In JJWT 0.11.5 (confirmed in `pom.xml:86`), `DefaultClaims.get(String, Class<T>)` throws `io.jsonwebtoken.RequiredTypeException extends JwtException extends RuntimeException` when the stored claim value is not of the requested type. `RequiredTypeException` is **not** a subclass of `IllegalArgumentException`, so the catch at line 138 does not cover it. A JWT whose `userId` claim was serialised as a JSON number (e.g. `"userId": 12345` from a misconfigured issuer) would cause `doFilterInternal` to propagate an unhandled `RuntimeException`, and the container would return HTTP 500 instead of the intended HTTP 401, breaking the fail-CLOSED contract. No test covers this path (verified by grep across `JwtAuthenticationFilterTest.java`). **Suggested fix** — move the `claims.get` call inside the existing try block and broaden the catch: ```java final String userId; try { String userIdClaim = claims.get("userId", String.class); // RequiredTypeException now caught if (userIdClaim == null || userIdClaim.isBlank()) { log.warn("JWT verified but missing userId claim on {}", request.getServletPath()); writeError(response, HttpStatus.UNAUTHORIZED, "TOKEN_INVALID", "JWT token is missing the required userId claim"); return; } userId = UUID.fromString(userIdClaim).toString(); } catch (IllegalArgumentException | JwtException ex) { log.warn("JWT userId claim invalid on {}: {}", request.getServletPath(), ex.getMessage()); writeError(response, HttpStatus.UNAUTHORIZED, "TOKEN_INVALID", "JWT token userId claim must be a valid UUID"); return; } ``` Add a companion test that puts a non-string value in `userId` (via a raw `Claims.put` call) and asserts HTTP 401 + no chain continuation. ### Verdict **CONDITIONAL_APPROVE** --- <sub>hib-pr-reviewer • round 3 • 1 finding (1m) • 2026-06-02T06:11:01.043Z → 2026-06-02T06:12:57.670Z • posted-as: pr-reviewer-bot • model: auto</sub> </details>
R3 CONDITIONAL_APPROVE, 1 new minor (both prior-round findings confirmed resolved):
- [MINOR] JwtAuthenticationFilter: claims.get("userId", String.class) sat outside a
  try/catch. jjwt's typed getter throws RequiredTypeException when the claim exists
  but is a non-string JSON type (number/object/array) — propagating as an uncaught
  HTTP 500 instead of the fail-CLOSED 401 the rest of this filter guarantees. Now
  wrapped: a wrong-type userId claim → 401 TOKEN_INVALID. Closes the last
  500-vs-401 gap on the claim-read path (missing / blank / wrong-type / non-UUID all
  now 401).

Test: added rejects401_whenUserIdClaimWrongType (Integer userId → 401, never 500).
Verification: mvn clean test green (Temurin 17).
pr-reviewer-bot commented 2026-06-02 08:18:54 +02:00
Member
Copy link

hib-pr-reviewer review — PR #8 (affinity-intelligence-rework/im2be-identity-service)

Round 4 — head 3c93d94627ad, base main, trigger synchronize

TL;DR: APPROVE — all three open findings (isBlank test, UUID normalisation, RequiredTypeException 500) are closed; six targeted tests cover every rejection branch; no new issues found.

Summary

[quorum-converged] A=0 = B=0. ## Round 4 — All prior findings resolved; no new issues found

Prior finding status

Round Finding Status
R1 No test for isBlank() rejection branch ✅ Closed — rejects401_whenUserIdClaimBlank() at test line 122 covers the empty-string path
R1 UUID parse result discarded / mixed-case normalisation missing ✅ Closed — filter line 149: userId = UUID.fromString(userIdClaim).toString(); normalisesUserIdToCanonicalLowercase() test verifies it end-to-end
R3 RequiredTypeException uncaught → HTTP 500 ✅ Closed — filter lines 128–138 wrap claims.get("userId", String.class) in a dedicated try/catch; rejects401_whenUserIdClaimWrongType() test exercises the branch with Integer(12345)

Verification notes

  • jjwt 0.11.5 (pom.xml line 86): Jwts.claims(), Claims.setSubject(), and Claims.get(name, Class) all behave as the tests assume. DefaultClaims.get("userId", String.class) throws RequiredTypeException when the stored value is Integer(12345) because Integer is not an instance of String — the test models real deserialization behaviour correctly.
  • writeError() wiring: confirmed at filter line 184 calls response.setStatus(status.value()), matching the verify(response).setStatus(401) assertions in all five rejection tests.
  • invokeFilter() reflection helper: ReflectionTestUtils.invokeMethod wraps checked exceptions from doFilterInternal as IllegalStateException; in the mocked setup response.getWriter() returns a PrintWriter(StringWriter) (no IOException) and no real filter chain is called, so no checked exception surfaces — the helper is safe as written.
  • No new lines introduced outside the diff scope; no pre-existing latent issues exposed by the diff that require deferral.

Blast Radius

Changes are confined to a single filter class and its unit test. The filter sits on every authenticated HTTP request to identity-service, but the change is additive — it only tightens a claim-extraction path that previously fell through to sub. No shared interfaces or downstream contracts are altered.

BLAST_SCORE: 3/10

Risk Indicators

Indicator Value
Sensitive functions JwtAuthenticationFilter.doFilterInternal, JwtVerifierChain.parse
Migration touched —
Test delta —
Dependency changes —

CI status (head 3c93d94627ad)

No CI checks reported for this commit.

Findings

No new findings this round.

Quorum converged on empty findings (A + B both returned 0).

Verdict

APPROVE

hib-pr-reviewer • round 4 • 0 findings • 2026-06-02T06:16:24.452Z → 2026-06-02T06:18:53.715Z • posted-as: pr-reviewer-bot • model: auto

<!-- hib-pr-reviewer round:4 --> ## hib-pr-reviewer review — PR #8 (affinity-intelligence-rework/im2be-identity-service) **Round 4** — head `3c93d94627ad`, base `main`, trigger `synchronize` **TL;DR:** APPROVE — all three open findings (isBlank test, UUID normalisation, RequiredTypeException 500) are closed; six targeted tests cover every rejection branch; no new issues found. ### Summary [quorum-converged] A=0 = B=0. ## Round 4 — All prior findings resolved; no new issues found ### Prior finding status | Round | Finding | Status | |-------|---------|--------| | R1 | No test for `isBlank()` rejection branch | ✅ **Closed** — `rejects401_whenUserIdClaimBlank()` at test line 122 covers the empty-string path | | R1 | UUID parse result discarded / mixed-case normalisation missing | ✅ **Closed** — filter line 149: `userId = UUID.fromString(userIdClaim).toString()`; `normalisesUserIdToCanonicalLowercase()` test verifies it end-to-end | | R3 | `RequiredTypeException` uncaught → HTTP 500 | ✅ **Closed** — filter lines 128–138 wrap `claims.get("userId", String.class)` in a dedicated try/catch; `rejects401_whenUserIdClaimWrongType()` test exercises the branch with `Integer(12345)` | ### Verification notes - **jjwt 0.11.5** (pom.xml line 86): `Jwts.claims()`, `Claims.setSubject()`, and `Claims.get(name, Class)` all behave as the tests assume. `DefaultClaims.get("userId", String.class)` throws `RequiredTypeException` when the stored value is `Integer(12345)` because `Integer` is not an instance of `String` — the test models real deserialization behaviour correctly. - **`writeError()` wiring**: confirmed at filter line 184 calls `response.setStatus(status.value())`, matching the `verify(response).setStatus(401)` assertions in all five rejection tests. - **`invokeFilter()` reflection helper**: `ReflectionTestUtils.invokeMethod` wraps checked exceptions from `doFilterInternal` as `IllegalStateException`; in the mocked setup `response.getWriter()` returns a `PrintWriter(StringWriter)` (no `IOException`) and no real filter chain is called, so no checked exception surfaces — the helper is safe as written. - No new lines introduced outside the diff scope; no pre-existing latent issues exposed by the diff that require deferral. ### Blast Radius Changes are confined to a single filter class and its unit test. The filter sits on every authenticated HTTP request to identity-service, but the change is additive — it only tightens a claim-extraction path that previously fell through to `sub`. No shared interfaces or downstream contracts are altered. **BLAST_SCORE: 3/10** ### Risk Indicators | Indicator | Value | |---|---| | Sensitive functions | `JwtAuthenticationFilter.doFilterInternal`, `JwtVerifierChain.parse` | | Migration touched | — | | Test delta | — | | Dependency changes | — | ### CI status (head `3c93d94627ad`) _No CI checks reported for this commit._ ### Findings **No new findings this round.** _Quorum converged on empty findings (A + B both returned 0)._ ### Verdict **APPROVE** --- <sub>hib-pr-reviewer • round 4 • 0 findings • 2026-06-02T06:16:24.452Z → 2026-06-02T06:18:53.715Z • posted-as: pr-reviewer-bot • model: auto</sub>
hibryda deleted branch feat/mint-reads-userid-claim 2026-06-02 08:22:54 +02:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
affinity-intelligence-rework/im2be-identity-service!8
No description provided.