feat: add Keycloak cleanup script #2426
Conversation
wilsonrivera
requested review from
JivusAyrus,
StarpTech and
thisisnithin
as code owners
WalkthroughAdds a new TypeScript CLI that reconciles Keycloak users with the local Postgres DB and removes Keycloak-only accounts; also adds a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (2)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
controlplane/src/bin/cleanup-keycloak-users.ts(1 hunks)controlplane/src/bin/migrate-groups.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build_push_image
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build_test
- GitHub Check: Analyze (go)
🔇 Additional comments (8)
controlplane/src/bin/migrate-groups.ts (2)
1-1: LGTM!Adding the
dotenv/configimport ensures environment variables are loaded before configuration is accessed, aligning with the pattern used in the new cleanup script.
55-55: LGTM!Grammar fix looks good.
controlplane/src/bin/cleanup-keycloak-users.ts (6)
1-19: LGTM!The imports and constants are well-structured. The date-based cutoff logic appropriately targets users created before a stable monthly boundary, ensuring consistent cleanup behavior.
21-46: LGTM!Configuration and Keycloak client initialization follow established patterns from the existing migration script.
101-136: LGTM!The pagination logic with proper break conditions and filtering criteria is well-implemented. The filter correctly targets users who are old, unverified, and haven't accepted terms.
138-152: LGTM!The database query is straightforward. The call site guarantees a non-empty emails array due to the early exit check.
154-177: LGTM!Good use of case-insensitive email comparison. Per-user error handling with
console.warnensures one failed deletion doesn't halt the entire cleanup process.
179-199: LGTM!Type definitions are clear and appropriate for this script's scope.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
controlplane/src/bin/cleanup-keycloak-users.ts (1)
48-99: Database connection not closed on early exit or error paths.The
queryConnectionis not closed when the script exits early at line 72 or when an error occurs after the connection is created at line 59. This issue was previously identified and remains unresolved.
🧹 Nitpick comments (1)
controlplane/src/bin/cleanup-keycloak-users.ts (1)
157-180: Consider adding a dry-run mode for safety.This function performs destructive operations (deleting Keycloak users) without a dry-run option. Adding a command-line flag or environment variable to enable a dry-run mode that logs what would be deleted without actually deleting would provide an extra safety layer for this one-time cleanup script.
Example implementation
Add a configuration flag:
const DRY_RUN = process.env.DRY_RUN === 'true';Then modify the deletion logic:
async function removeRelevantKeycloakUsers(keycloakUsers: UserRepresentation[], dbUsers: DbUserRepresentation[]) { let numberOfRemovedUsers = 0; for (const user of keycloakUsers) { const dbUser = dbUsers.find((u) => u.email.toLowerCase() === user.email.toLowerCase()); if (dbUser) { // The user exists in the database, we don't need to delete the Keycloak user continue; } try { + if (DRY_RUN) { + console.log(`\t- [DRY RUN] Would remove user "${user.email}" from Keycloak`); + numberOfRemovedUsers++; + continue; + } + await keycloakClient.client.users.del({ id: user.id, realm, }); numberOfRemovedUsers++; console.log(`\t- User "${user.email}" removed from Keycloak successfully`); } catch (err) { console.warn(`\t- Failed to remove user "${user.email}" from Keycloak: ${err}`); } } return numberOfRemovedUsers; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
controlplane/src/bin/cleanup-keycloak-users.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
controlplane/src/bin/cleanup-keycloak-users.ts (3)
controlplane/src/bin/get-config.ts (1)
getConfig(55-55)controlplane/src/core/services/Keycloak.ts (1)
Keycloak(10-451)controlplane/src/db/schema.ts (1)
users(1139-1145)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
controlplane/src/bin/cleanup-keycloak-users.ts (1)
19-19: Verify the intended date cutoff logic.The use of
startOfMonth(subDays(new Date(), 60))creates a variable cutoff ranging from approximately 60 to 90 days ago, depending on the current day of the month. If the intent is to filter users created more than 60 days ago, consider using justsubDays(new Date(), 60)withoutstartOfMonth. If the start-of-month behavior is intentional, adding a clarifying comment would be helpful.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2426 +/- ##
===========================================
+ Coverage 30.23% 62.75% +32.51%
===========================================
Files 587 296 -291
Lines 88141 41353 -46788
Branches 4674 4244 -430
===========================================
- Hits 26652 25951 -701
+ Misses 61186 15381 -45805
+ Partials 303 21 -282
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
controlplane/src/bin/cleanup-keycloak-users.ts (1)
73-86: Database connection still not closed on error paths.The early-exit issue from the previous review has been resolved by moving the connection creation after Keycloak retrieval. However, if an error occurs between creating the connection (line 73) and closing it (line 84-86), the connection will leak because the catch block at line 96 doesn't close it.
Wrap the connection usage in try/finally to guarantee cleanup:
🔎 Proposed fix using try/finally
const queryConnection = postgres(databaseConnectionUrl, { ...connectionConfig }); + try { // Retrieve the existing users from the database console.log('Retrieving users from database...'); const db = drizzle(queryConnection, { schema: { ...schema } }); const dbUsers = await getExistingDatabaseUsers( db, keycloakUsers.map((user) => user.email), ); - - await queryConnection.end({ - timeout: 1, - }); // Remove any user that exists in Keycloak but doesn't exist in the database console.log(); console.log('Removing Keycloak users not found in the database...'); const numberOfRemovedUsers = await removeRelevantKeycloakUsers(keycloakUsers, dbUsers); // Cleanup completed const duration = ((performance.now() - start) / 1000).toFixed(3); console.log(`Cleanup completed after ${duration} seconds. A total of ${numberOfRemovedUsers} user(s) were removed.`); + } finally { + await queryConnection.end({ timeout: 1 }); + } } catch (err) { console.error(err); // eslint-disable-next-line unicorn/no-process-exit process.exit(1); }Also applies to: 96-100
🧹 Nitpick comments (1)
controlplane/src/bin/cleanup-keycloak-users.ts (1)
108-108: Missing semicolon.Add a semicolon at the end of line 108 for consistency with the project's style.
- console.log(`\tFetching range ${(startIndex + 1)}-${startIndex + NUMBER_OF_USERS_PER_PAGE}`) + console.log(`\tFetching range ${(startIndex + 1)}-${startIndex + NUMBER_OF_USERS_PER_PAGE}`);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
controlplane/src/bin/cleanup-keycloak-users.ts
🧰 Additional context used
🧬 Code graph analysis (1)
controlplane/src/bin/cleanup-keycloak-users.ts (2)
controlplane/src/core/services/Keycloak.ts (1)
Keycloak(10-451)controlplane/src/db/schema.ts (1)
users(1139-1145)
🪛 GitHub Check: build_test
controlplane/src/bin/cleanup-keycloak-users.ts
[failure] 159-159:
Async function 'removeRelevantKeycloakUsers' has no 'await' expression
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build_push_image
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
controlplane/src/bin/cleanup-keycloak-users.ts (2)
19-19: Verify the date calculation logic aligns with requirements.The date calculation
startOfMonth(subDays(new Date(), 60))computes the start of the month that was 60 days ago, not simply 60 days ago. For example, if today is December 22, this evaluates to October 1 rather than October 23. This means users created between October 1-23 will be included in the cleanup even though they're not quite 60 days old.If the intent is to clean up users older than exactly 60 days, use
subDays(new Date(), 60)directly. If month-boundary alignment is required for data retention policies, the current logic is correct but deserves a clarifying comment.
162-162: LGTM: Proper case-insensitive email comparison.The case-insensitive email comparison is correct and handles email matching appropriately.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
controlplane/src/bin/cleanup-keycloak-users.ts (1)
73-86: Database connection not closed on error paths.If an error occurs after the connection is created (line 73) but before
queryConnection.end()(line 84), the connection will leak. Wrap the database operations in try/finally to ensure cleanup.🔎 Proposed fix using try/finally
const queryConnection = postgres(databaseConnectionUrl, { ...connectionConfig }); + try { // Retrieve the existing users from the database console.log('Retrieving users from database...'); const db = drizzle(queryConnection, { schema: { ...schema } }); const dbUsers = await getExistingDatabaseUsers( db, keycloakUsers.map((user) => user.email), ); - - await queryConnection.end({ - timeout: 1, - }); + } finally { + await queryConnection.end({ + timeout: 1, + }); + }
🧹 Nitpick comments (1)
controlplane/src/bin/cleanup-keycloak-users.ts (1)
159-182: Consider using a Set for O(1) lookups.The current implementation uses
find()in a loop, resulting in O(n*m) complexity. For large user sets, consider using a Set for O(n) overall.🔎 Proposed optimization
async function removeRelevantKeycloakUsers(keycloakUsers: UserRepresentation[], dbUsers: DbUserRepresentation[]) { + const dbEmailSet = new Set(dbUsers.map((u) => u.email.toLowerCase())); let numberOfRemovedUsers = 0; + for (const user of keycloakUsers) { - const dbUser = dbUsers.find((u) => u.email.toLowerCase() === user.email.toLowerCase()); - if (dbUser) { + if (dbEmailSet.has(user.email.toLowerCase())) { // The user exists in the database, we don't need to delete the Keycloak user continue; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
controlplane/src/bin/cleanup-keycloak-users.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
controlplane/src/bin/cleanup-keycloak-users.ts (5)
1-11: LGTM!Imports are well-organized with external dependencies first, followed by local imports.
18-19: Verify intent of date calculation.The expression
startOfMonth(subDays(new Date(), 60))first subtracts 60 days, then gets the start of that month. This means the cutoff date jumps to month boundaries rather than being exactly 60 days ago.For example, if run on Dec 22, 2025: 60 days ago is Oct 23 →
startOfMonthreturns Oct 1, 2025 (82 days ago).Is this the intended behavior, or should it simply be
subDays(new Date(), 60)for a rolling 60-day window?
21-46: LGTM!Configuration extraction and Keycloak client initialization are properly structured.
102-138: LGTM!The paginated retrieval logic is sound. The filtering correctly identifies users who: were created before the cutoff date, haven't verified their email, and still need to accept terms and conditions.
184-205: LGTM!Type definitions are well-structured and appropriate for this script's needs.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.
Checklist