feat: implement endpoint to ensure user is initialized in Cosmo#2497
Conversation
WalkthroughAdds a new PlatformService RPC Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2497 +/- ##
==========================================
- Coverage 41.99% 36.68% -5.31%
==========================================
Files 1000 947 -53
Lines 138762 124031 -14731
Branches 8008 5102 -2906
==========================================
- Hits 58270 45506 -12764
+ Misses 78867 76956 -1911
+ Partials 1625 1569 -56
🚀 New features to boost your workflow:
|
Router image scan passed✅ No security vulnerabilities found in image: |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@controlplane/src/core/auth-utils.ts`:
- Around line 431-436: The advisory lock is acquired using db.execute which can
use a different connection and won't scope the lock to the transaction; inside
the db.transaction callback replace the db.execute call with the transaction
handle (tx.execute) so the pg_try_advisory_xact_lock(hashtext(${userId})) runs
on the same connection as the rest of the transaction (the code around
db.transaction, advisoryLockRows, userId and the returned [undefined, -1] should
remain unchanged aside from swapping db.execute → tx.execute).
- Around line 459-474: The webhook call platformWebhooks.send is async but not
awaited, so rejections escape the surrounding try/catch; update the call in the
block that checks numberOfOrganizations === 0 to await
platformWebhooks.send(...) (i.e., add the await keyword before
platformWebhooks.send and keep it inside the existing try/catch), and confirm
the enclosing function (in auth-utils.ts) is async so the await is valid—if the
enclosing function cannot be made async, instead return or properly chain the
Promise so failures are still caught.
In `@controlplane/src/core/bufservices/user/initializeCosmoUser.ts`:
- Around line 52-67: AuthUtils.handleAuthCallback is currently awaited but its
return value (which can be [undefined, -1] on advisory lock acquisition failure)
is ignored; update initializeCosmoUser to capture the result of
AuthUtils.handleAuthCallback, inspect the returned numberOfOrganizations (or
second tuple element) for -1, and if -1 return an error response instead of
EnumStatusCode.OK (or propagate/throw an error) so concurrent lock failures are
surfaced; reference the call site AuthUtils.handleAuthCallback and the
EnumStatusCode.OK response to implement the conditional handling.
In `@controlplane/src/core/services/Keycloak.ts`:
- Around line 108-123: The catch block inside the token verification loop in
Keycloak.ts never records errors, so AggregateError is never reached; update the
catch to capture the thrown value (e) and push a normalized Error into the
errors array (e.g., if e is already an Error push it, otherwise create new
Error(String(e))) while continuing the loop over signingKeys; keep the existing
post-loop logic that throws 'None of the signing keys...' when errors is empty
and otherwise throws new AggregateError(errors, 'Token verification failed') so
diagnostic info is preserved for jwtVerify and signingKeys failures.
🧹 Nitpick comments (3)
controlplane/src/core/services/Keycloak.ts (1)
125-149: Add a timeout to JWKS fetch.
Axios defaults to no timeout, so token verification can hang on network stalls.⏱️ Suggested tweak
- const response = await axios.get<JSONWebKeySet>(jwkEndpoint); + const response = await axios.get<JSONWebKeySet>(jwkEndpoint, { timeout: 5_000 });controlplane/src/core/bufservices/PlatformService.ts (1)
177-177: Consider groupinginitializeCosmoUserwith mutations for readability.This RPC initializes/creates users (side effects), so placing it under the Mutations section would keep the file’s organization consistent.
Also applies to: 480-482
controlplane/test/initialize-cosmo-user.test.ts (1)
24-128: Ensure server cleanup runs even on assertion failures.
server.close()only runs on the happy path; a thrown assertion can leak handles and hang the suite. Wrap each test in atry/finally(or use a shared helper).♻️ Suggested pattern (apply to each test)
- test.each(['', ' '])('should return `Bad Request` when token is empty or whitespace', async (token: string) => { - const { client, server } = await SetupTest({ dbname }); - - const response = await client.initializeCosmoUser({ token }); - expect(response.response?.code).toBe(EnumStatusCode.ERR_BAD_REQUEST); - - await server.close(); - }); + test.each(['', ' '])('should return `Bad Request` when token is empty or whitespace', async (token: string) => { + const { client, server } = await SetupTest({ dbname }); + try { + const response = await client.initializeCosmoUser({ token }); + expect(response.response?.code).toBe(EnumStatusCode.ERR_BAD_REQUEST); + } finally { + await server.close(); + } + });
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@controlplane/test/initialize-cosmo-user.test.ts`:
- Around line 24-65: Each test should guarantee server shutdown even on
assertion failures: for every test that calls SetupTest(...) and obtains {
client, server, ... }, wrap the test body in try/finally and move await
server.close() into the finally block so server.close() always runs; do this for
all tests that call SetupTest and use client.initializeCosmoUser, including the
parameterized test, the invalid token test, and the existing-user test that uses
createAuthenticator, UserRepository and OrganizationRepository.
…ize-user-in-cosmo-using-a-bearer
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@controlplane/test/initialize-cosmo-user.test.ts`:
- Around line 87-88: Update the mistaken comment text "that the was just
created" to "that the user was just created" in the test comment that begins "//
To simulate that the was just created in Keycloak..." to fix the typo and
improve clarity.
🧹 Nitpick comments (1)
controlplane/test/initialize-cosmo-user.test.ts (1)
69-70: Terminology mismatch: comments say "ID token" but code usesaccess_token.The comments on lines 69 and 107 refer to "ID token," but the code extracts and uses
access_token. Similarly, the error message on line 157 says "No ID token returned" while checking foraccess_token. Consider aligning the terminology for clarity.✏️ Suggested terminology fixes
- // Authenticate the user to get an ID token + // Authenticate the user to get an access token const { access_token: token } = await signIn(users.adminAliceCompanyA.email, 'wunder@123');- // Authenticate the new user to get an ID token + // Authenticate the new user to get an access token const { access_token: token } = await signIn(userEmail, 'wunder@123');if (!respObj.access_token) { - throw new Error('No ID token returned from Keycloak'); + throw new Error('No access token returned from Keycloak'); }Also applies to: 107-108, 155-158
Summary by CodeRabbit
New Features
Tests
Checklist