feat: add Keycloak cleanup script #2426
Conversation
wilsonrivera
requested review from
JivusAyrus,
StarpTech and
thisisnithin
as code owners
WalkthroughAdds a new TypeScript CLI script that cleans up Keycloak users not present in the local Postgres DB and a tiny edit to an existing migration script (dotenv import and a comment fix). 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)
🚧 Files skipped from review as they are similar to previous changes (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)
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.
| try { | ||
| const start = performance.now(); | ||
|
|
||
| // Create the database connection. TLS is optional. | ||
| const connectionConfig = await buildDatabaseConnectionConfig({ | ||
| tls: | ||
| databaseTlsCa || databaseTlsCert || databaseTlsKey | ||
| ? { ca: databaseTlsCa, cert: databaseTlsCert, key: databaseTlsKey } | ||
| : undefined, | ||
| }); | ||
|
|
||
| const queryConnection = postgres(databaseConnectionUrl, { ...connectionConfig }); | ||
|
|
||
| // Ensure Keycloak is up and running | ||
| console.log('Retrieving users from Keycloak...'); | ||
| await keycloakClient.authenticateClient(); | ||
|
|
||
| // Retrieve all the users paged | ||
| const keycloakUsers = await getKeycloakUsers(); | ||
| if (keycloakUsers.length === 0) { | ||
| console.log(); | ||
| console.log('All Keycloak users have accepted the terms and conditions!'); | ||
|
|
||
| // eslint-disable-next-line unicorn/no-process-exit | ||
| process.exit(0); | ||
| } | ||
|
|
||
| // 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.`); | ||
| } catch (err) { | ||
| console.error(err); | ||
| // eslint-disable-next-line unicorn/no-process-exit | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
Database connection not closed on early exit or error paths.
The queryConnection created at line 59 is not closed when:
- Early exit at line 72 (no users to cleanup)
- Any error occurs after connection creation
This can leak database connections.
🔎 Proposed fix using try/finally pattern
try {
const start = performance.now();
// Create the database connection. TLS is optional.
const connectionConfig = await buildDatabaseConnectionConfig({
tls:
databaseTlsCa || databaseTlsCert || databaseTlsKey
? { ca: databaseTlsCa, cert: databaseTlsCert, key: databaseTlsKey }
: undefined,
});
const queryConnection = postgres(databaseConnectionUrl, { ...connectionConfig });
+ try {
// Ensure Keycloak is up and running
console.log('Retrieving users from Keycloak...');
await keycloakClient.authenticateClient();
// Retrieve all the users paged
const keycloakUsers = await getKeycloakUsers();
if (keycloakUsers.length === 0) {
console.log();
console.log('All Keycloak users have accepted the terms and conditions!');
// eslint-disable-next-line unicorn/no-process-exit
process.exit(0);
}
// 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);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| const start = performance.now(); | |
| // Create the database connection. TLS is optional. | |
| const connectionConfig = await buildDatabaseConnectionConfig({ | |
| tls: | |
| databaseTlsCa || databaseTlsCert || databaseTlsKey | |
| ? { ca: databaseTlsCa, cert: databaseTlsCert, key: databaseTlsKey } | |
| : undefined, | |
| }); | |
| const queryConnection = postgres(databaseConnectionUrl, { ...connectionConfig }); | |
| // Ensure Keycloak is up and running | |
| console.log('Retrieving users from Keycloak...'); | |
| await keycloakClient.authenticateClient(); | |
| // Retrieve all the users paged | |
| const keycloakUsers = await getKeycloakUsers(); | |
| if (keycloakUsers.length === 0) { | |
| console.log(); | |
| console.log('All Keycloak users have accepted the terms and conditions!'); | |
| // eslint-disable-next-line unicorn/no-process-exit | |
| process.exit(0); | |
| } | |
| // 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.`); | |
| } catch (err) { | |
| console.error(err); | |
| // eslint-disable-next-line unicorn/no-process-exit | |
| process.exit(1); | |
| } | |
| try { | |
| const start = performance.now(); | |
| // Create the database connection. TLS is optional. | |
| const connectionConfig = await buildDatabaseConnectionConfig({ | |
| tls: | |
| databaseTlsCa || databaseTlsCert || databaseTlsKey | |
| ? { ca: databaseTlsCa, cert: databaseTlsCert, key: databaseTlsKey } | |
| : undefined, | |
| }); | |
| const queryConnection = postgres(databaseConnectionUrl, { ...connectionConfig }); | |
| try { | |
| // Ensure Keycloak is up and running | |
| console.log('Retrieving users from Keycloak...'); | |
| await keycloakClient.authenticateClient(); | |
| // Retrieve all the users paged | |
| const keycloakUsers = await getKeycloakUsers(); | |
| if (keycloakUsers.length === 0) { | |
| console.log(); | |
| console.log('All Keycloak users have accepted the terms and conditions!'); | |
| // eslint-disable-next-line unicorn/no-process-exit | |
| process.exit(0); | |
| } | |
| // 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), | |
| ); | |
| // 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); | |
| } |
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
❌ Your patch check has failed because the patch coverage (0.00%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2426 +/- ##
==========================================
+ Coverage 61.35% 62.07% +0.71%
==========================================
Files 229 296 +67
Lines 23814 41292 +17478
Branches 0 4195 +4195
==========================================
+ Hits 14612 25632 +11020
- Misses 7970 15639 +7669
+ Partials 1232 21 -1211
🚀 New features to boost your workflow:
|
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.
Checklist