[Backend][Bug] UpdateUserCommandHandler silently swallows all Keycloak failures — failed user updates report success #656

New Issue

2026-07-17T14:33:03Z

spikerj commented

2026-07-17 14:33:03 +00:00

Found via SonarQube S2486 triage (2026-07-17), verified end-to-end.

UpdateUserCommandHandler.Handle wrapped its Keycloak PutUsersByUserIdAsync call in two empty catches — catch (ApiException) { } and catch (Exception) { } — with no log and no rethrow. So ANY failure (Keycloak down, auth failure, validation rejection, network error) was swallowed and the command completed as if it succeeded.

Impact: UserController.UpdateUser is explicitly built to handle a throwing handler — it try/catches, logs, and returns BadRequest+ProblemDetails on exception, and there's even a test UpdateUser_ReturnsBadRequest_OnException for that path. But because the handler never threw, that whole error path was DEAD CODE, and the endpoint always returned Ok("Successfully updated user") — even when the update silently failed. A user's profile change reports success and doesn't persist.

Worse, two handler tests (Update_NullUserId_DoesNotThrow, Update_InvalidKeycloakUrl_DoesNotThrow) actively PINNED the swallow behavior by asserting NotThrow — encoding the bug as the contract.

Fix (PR incoming): remove the swallowing so failures propagate to the controller (restoring the designed, already-tested behavior). Corrected the handler tests to assert failures propagate. Handler + controller suites green; slnf clean. No new risk — the controller was already written and tested for a throwing handler.

**Found via SonarQube S2486 triage (2026-07-17), verified end-to-end.** `UpdateUserCommandHandler.Handle` wrapped its Keycloak `PutUsersByUserIdAsync` call in two empty catches — `catch (ApiException) { }` and `catch (Exception) { }` — with no log and no rethrow. So ANY failure (Keycloak down, auth failure, validation rejection, network error) was swallowed and the command completed as if it succeeded. **Impact:** `UserController.UpdateUser` is explicitly built to handle a throwing handler — it try/catches, logs, and returns BadRequest+ProblemDetails on exception, and there's even a test `UpdateUser_ReturnsBadRequest_OnException` for that path. But because the handler never threw, that whole error path was DEAD CODE, and the endpoint always returned `Ok("Successfully updated user")` — even when the update silently failed. A user's profile change reports success and doesn't persist. Worse, two handler tests (`Update_NullUserId_DoesNotThrow`, `Update_InvalidKeycloakUrl_DoesNotThrow`) actively PINNED the swallow behavior by asserting NotThrow — encoding the bug as the contract. **Fix (PR incoming):** remove the swallowing so failures propagate to the controller (restoring the designed, already-tested behavior). Corrected the handler tests to assert failures propagate. Handler + controller suites green; slnf clean. No new risk — the controller was already written and tested for a throwing handler.

spikerj commented

2026-07-18 17:57:52 +00:00

Verified fixed on master — closing. (Skeptical re-check 2026-07-18, ticket-triage loop.)

The fix shipped in commit 29e0a6c3 fix(users): stop UpdateUserCommandHandler swallowing Keycloak failures — but its message cited #650 (a closed Reader/Perf ticket, unrelated), so this ticket stayed open. Same wrong-ticket-ref pattern as #657.

Re-verified on current master:

UpdateUserCommandHandler has no catch blocks — PutUsersByUserIdAsync failures propagate, with an in-code comment documenting why swallowing is forbidden.
The pinning tests are gone; UserHandlerTests.Update_KeycloakUnreachable_Propagates now asserts ThrowAsync (3/3 green).
UserController.UpdateUser's BadRequest-on-exception path is live again (2/2 green).

No code change required; closing as completed (fix merged, wrong ticket ref in the commit message).

**Verified fixed on master — closing.** (Skeptical re-check 2026-07-18, ticket-triage loop.) The fix shipped in commit `29e0a6c3` *fix(users): stop UpdateUserCommandHandler swallowing Keycloak failures* — but its message cited **#650** (a closed Reader/Perf ticket, unrelated), so this ticket stayed open. Same wrong-ticket-ref pattern as #657. Re-verified on current master: - `UpdateUserCommandHandler` has **no catch blocks** — `PutUsersByUserIdAsync` failures propagate, with an in-code comment documenting why swallowing is forbidden. - The pinning tests are gone; `UserHandlerTests.Update_KeycloakUnreachable_Propagates` now asserts `ThrowAsync` (3/3 green). - `UserController.UpdateUser`'s BadRequest-on-exception path is live again (2/2 green). No code change required; closing as completed (fix merged, wrong ticket ref in the commit message).

spikerj closed this issue

2026-07-18 17:57:52 +00:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: spikerj/spikersoft-issues#656