Skip to content

Conversation

@nachikamod
Copy link

Description

The commit adds Keycloak JWT support. We now recognise JWTs issued by the configured Keycloak realm, fetch the signing key via JWKS (jwks-rsa), and verify RS256 tokens. For those tokens we temporarily use sub as id so existing APIs keep working; local RSA tokens continue to verify with the existing public key. The commit also adds a watch-targeted test script, pulls in the new dependencies (jwks-rsa, jose, TypeScript types), and extends JWTService.spec.js to exercise the Keycloak flow by obtaining a real token via the Keycloak token endpoint. Finally, verifyJWTHandler awaits the asynchronous verification.

Issue(s) addressed

What kind of change(s) does this PR introduce?

  • Enhancement
  • Bug fix
  • Refactor

Please check if the PR fulfils these requirements

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

Issue

What is the current behavior?
API only trusted JWTs it issued itself (signed with the local private key); external Keycloak access tokens were rejected.
JWTService.verify was synchronous and verification middleware assumed immediate return.
No focused test script for iterating on JWT logic.

What is the new behavior?
Incoming tokens are checked for the configured Keycloak issuer; if present we fetch the JWKS, verify RS256 signatures, and treat the Keycloak subject (sub) as the caller id for compatibility with existing routes. Local RSA tokens still work.
verifyJWTHandler now awaits the async verification.
A test-watch-jwt script plus an integration spec validate the Keycloak flow.

Other useful information

Newly discovered Issues

Issue 1: Keycloak login creates orphaned wallet trust rows

Summary
Keycloak tokens are accepted by JWTService.verify, but we set result.id = result.sub. When a Keycloak caller hits POST /wallets, walletService.createWallet uses that id as actor_wallet_id/originator_wallet_id. If no wallet exists with that UUID, we insert trust records pointing to a non-existent parent wallet (no FK constraint catches it). The API still returns 201, leaving orphaned trust rows.

Impact

  • Newly created wallets can be linked to phantom parents.
  • Permission checks that rely on wallet_trust may break or allow unexpected access.
  • It’s not obvious to the caller that their token isn’t mapped to a real wallet.

Steps

  1. Obtain a Keycloak access token that doesn’t correspond to an existing wallet.
  2. Call POST /wallets (include API key + new wallet data).
  3. DB shows wallet_trust.actor_wallet_id = Keycloak sub, but wallet table has no such id.

Proposed Fix

  • Introduce a proper mapping from Keycloak identities to wallet records, or reject Keycloak tokens that lack a wallet association.
  • Add foreign-key constraints on wallet_trust to prevent orphaned references.
  • Audit existing trust rows and clean up any invalid parents.

Issue 2: e2e tests expect 200 but API now returns 201 on create flows

Summary
Several end-to-end tests fail on master because they expect HTTP 200 for operations that now intentionally respond with 201 Created.

Failing specs

__tests__/e2e/__tests__/sendTokens/managingWalletMovesTokensBetweenWallets.test.js
__tests__/e2e/__tests__/sendTokens/sendTokensWithTrustRelationship.test.js
__tests__/e2e/__tests__/wallets.test.js

Each asserts response.status === 200 but the API returns 201 for successful trust relationship creation and managed wallet creation. The assertions in __tests__/e2e/libs/assertionLibrary.equals throw (expected 201 to equal 200).

Why it happens
Handlers set 201 after creating new resources:

  • trustPost → res.status(201).json(trustRelationship)
  • walletPost → res.status(201).json(returnedWallet)

Proposed fix

  • Update the e2e expectations to accept 201 (or a range 200–201).
  • Optional: log the response body when the status mismatches to aid diagnosis.

Notes
These failures occur on a clean checkout without my changes.
After adjusting the assertions, the tests should pass against current API behavior.

@Kpoke Kpoke marked this pull request as draft November 26, 2025 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

E2E tests (auth flow)

2 participants