feat: trust-service support and ecosystem authentication#1565
Merged
tipusinghaw merged 9 commits intomainfrom Mar 4, 2026
Merged
feat: trust-service support and ecosystem authentication#1565tipusinghaw merged 9 commits intomainfrom
tipusinghaw merged 9 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRegisters ClientRegistrationModule and new KeycloakConfigModule; extends ClientRegistrationService with management-token and Keycloak ecosystem-access APIs; KeycloakConfigService runs on init to ensure ecosystem protocol mappers and verifies users; EcosystemService now syncs leads/members and service-account access to Keycloak; tsconfig, API gateway wiring, seed, and helper scripts updated. Changes
Sequence Diagram(s)sequenceDiagram
participant Ecosystem as EcosystemService
participant CRS as ClientRegistrationService
participant KCSvc as KeycloakConfigService
participant KC as Keycloak (REST)
rect rgba(0,150,200,0.5)
Note over Ecosystem,KC: Ecosystem create/invite/accept flows sync users
Ecosystem->>CRS: getPlatformManagementToken()
CRS-->>Ecosystem: access_token
Ecosystem->>CRS: updateUserEcosystemLeads(userId, orgId, ecosystemId, token)
CRS->>KC: GET user -> modify ecosystem_access -> PUT user
KC-->>CRS: 200/204
end
rect rgba(200,100,100,0.5)
Note over KCSvc,KC: Module init ensures mappers and verifies users
KCSvc->>CRS: getPlatformManagementToken()
CRS-->>KCSvc: access_token
KCSvc->>KC: GET client-scopes/profile -> GET mappers -> POST mapper (if missing)
KC-->>KCSvc: 200/201/409
KCSvc->>KC: GET clients -> for each client GET mappers -> POST mapper (if missing)
KC-->>KCSvc: 200/201/409
KCSvc->>KC: GET users -> examine ecosystem_access attributes
KC-->>KCSvc: 200
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/prisma-service/prisma/seed.ts (1)
665-670:⚠️ Potential issue | 🟡 MinorDead check:
platformAdminData.passwordis validated but never used.Line 668 throws if
platformAdminData.passwordis missing from the JSON config, but Line 700 ignores it entirely and uses the hardcoded value. This validation is misleading — either use the config value or remove the check.
🤖 Fix all issues with AI agents
In `@apps/ecosystem/src/ecosystem.service.ts`:
- Around line 200-202: There is a duplicate null-check on the local variable
invitation that re-throws
ForbiddenException(ResponseMessages.ecosystem.error.invitationRequired); remove
the redundant guard block that checks `if (!invitation) { throw new
ForbiddenException(ResponseMessages.ecosystem.error.invitationRequired); }`
(leave the original check earlier in the same method intact) so the method only
performs the invitation presence validation once.
In `@libs/client-registration/src/client-registration.service.ts`:
- Line 804: The current log statement in updateUserEcosystemAccess (and similar
ones in removeUserEcosystemAccess) prints full user attributes via
JSON.stringify(attributes), risking PII/security leaks; change these logs to
avoid dumping full attribute objects by either logging a whitelist of
non-sensitive keys or a sanitized summary (e.g., attribute names and counts, or
explicit safe fields only), or redact sensitive values before logging; update
the logger calls in the updateUserEcosystemAccess and removeUserEcosystemAccess
methods to use the sanitized summary instead of JSON.stringify(attributes).
- Around line 854-861: The code is currently spreading the entire Keycloak GET
response into the PUT payload (updatePayload = { ...currentUser, attributes })
in updateUserEcosystemAccess (and the same pattern in
removeUserEcosystemAccess), which can send read-only/sensitive fields back to
Keycloak; instead, build a minimal, explicit payload containing only
allowed/updatable fields (for example: attributes plus only username, email,
firstName, lastName or other documented updatable fields) and pass that to
this.commonService.httpPut(userUrl, payload, this.getAuthHeader(token)); remove
the spread of currentUser and apply the same change to removeUserEcosystemAccess
so only intended fields are sent.
In `@libs/prisma-service/prisma/seed.ts`:
- Line 700: Replace the hardcoded weak password 'Password@1' used when seeding
the platform admin Keycloak user with a runtime value sourced from
process.env.PLATFORM_ADMIN_USER_PASSWORD; update the seeding logic that sets the
password field (the object creating the platform admin user) to read from that
env var and throw/exit if it is missing or empty, and add a validation check for
process.env.PLATFORM_ADMIN_USER_PASSWORD alongside the other env-var validations
in the same seed file so deployments fail fast when the secret is not provided.
🧹 Nitpick comments (2)
libs/client-registration/src/client-registration.service.ts (1)
784-870: Read-modify-write on Keycloak attributes is not atomic — potential for lost updates under concurrency.If two ecosystem operations run concurrently for the same user (e.g., invited to two ecosystems simultaneously), one update can overwrite the other's attribute changes. For now this may be acceptable if ecosystem operations are infrequent, but consider documenting this limitation or implementing optimistic locking if usage scales.
apps/ecosystem/src/ecosystem.service.ts (1)
230-250: Keycloak update failures are silently swallowed — ecosystem state will diverge without any recovery path.Both
updateKeycloakEcosystemLeadsandupdateKeycloakEcosystemMembercatch all errors and only log them. If Keycloak is down or the update fails, the ecosystem is created/accepted in the database but Keycloak attributes are stale. There's no retry, no queue, and no reconciliation mechanism.This is acceptable for a first iteration, but consider at least:
- Emitting a warning-level event or metric so ops can detect divergence
- Adding a background reconciliation job or retry queue in the future
tipusinghaw
force-pushed
the
feat/ecosystem-auth-main
branch
from
3d9f7bd to
728273d
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ecosystem/src/ecosystem.service.ts (1)
338-342:⚠️ Potential issue | 🟡 MinorDuplicate condition:
checkUser.status === Invitation.PENDINGappears twice.Line 341 and line 342 check the same condition. One of them likely should check a different field (e.g.,
checkUser.typeor something else), or the duplicate should be removed.Proposed fix (assuming true duplicate)
if ( checkUser?.ecosystemId === ecosystemId && checkUser.invitedOrg === orgId && checkUser.status === Invitation.PENDING && - checkUser.status === Invitation.PENDING && checkUser.type === InviteType.MEMBER ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ecosystem/src/ecosystem.service.ts` around lines 338 - 342, The condition block in ecosystem.service.ts contains a duplicated check of checkUser.status === Invitation.PENDING; update the conditional to remove the duplicate or replace the second occurrence with the intended field check (e.g., checkUser.type === <expectedValue> or another property), ensuring the full if uses checkUser?.ecosystemId, checkUser.invitedOrg, checkUser.status === Invitation.PENDING and the correct additional condition; refer to the checkUser variable and Invitation.PENDING symbol to locate and fix the duplicate.
🧹 Nitpick comments (6)
apps/api-gateway/src/app.module.ts (1)
78-78: Consider makingKeycloakConfigModuleconditional on Keycloak being configured.Unlike the OIDC modules (wrapped in
ConditionalModule.registerWhen), this module loads unconditionally and runs Keycloak HTTP calls on every startup. IfKEYCLOAK_DOMAINorKEYCLOAK_REALMis unset, the init will fail (gracefully, since it's caught). If this is intentional and Keycloak is always expected, this is fine — but if the platform supports non-Keycloak deployments, consider guarding it similarly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api-gateway/src/app.module.ts` at line 78, KeycloakConfigModule is loaded unconditionally and triggers Keycloak HTTP calls on startup; make its import conditional by wrapping KeycloakConfigModule with the same ConditionalModule.registerWhen pattern used for OIDC modules (e.g., create or reuse an isKeycloakConfigured predicate that checks process.env.KEYCLOAK_DOMAIN and process.env.KEYCLOAK_REALM) so KeycloakConfigModule only registers when both env vars are set, preventing startup calls when Keycloak is not configured.libs/keycloak-config/src/keycloak-config.service.ts (2)
112-121: Sequential processing of all clients could be slow at scale.Each client is processed one-by-one with two HTTP calls (GET mappers + conditional POST). With hundreds of service-account-enabled clients, this init step could take minutes. This runs in
onModuleInit, so it blocks the module from being fully ready.Consider batching with
Promise.allSettled(with a concurrency limit) if startup time becomes a concern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/keycloak-config/src/keycloak-config.service.ts` around lines 112 - 121, The loop over orgClients in onModuleInit that calls ensureClientProtocolMapper sequentially is slow; change it to run clients in parallel with bounded concurrency (e.g., use Promise.allSettled with a concurrency limiter like p-limit or a simple worker queue) so multiple ensureClientProtocolMapper(realm, client.id, client.clientId, token) calls execute concurrently but limited to a safe number; collect results from Promise.allSettled, then increment created/skipped/failed based on each settled result value (handle rejections as failures) to preserve existing counters and avoid blocking module startup.
1-3: Blanket ESLint disables weaken type safety across the entire file.Disabling
explicit-function-return-typeandexplicit-module-boundary-typesfor the whole file is overly broad. Consider adding return types to the public/private methods instead, and using inline disables only where truly needed (e.g.,buildProtocolMapperPayload).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/keycloak-config/src/keycloak-config.service.ts` around lines 1 - 3, Remove the file-level ESLint disables for explicit-function-return-type and explicit-module-boundary-types; instead add explicit return types to the public and private methods on the KeycloakConfigService class and any exported functions, and only use inline eslint-disable comments directly above a specific function that truly requires it (e.g., keep a narrow disable for buildProtocolMapperPayload if it cannot have a typed signature). Ensure exported methods and class members declare their return types to satisfy TypeScript and move any remaining eslint-disable comments from the top of the file to the smallest possible scope.apps/ecosystem/src/ecosystem.service.ts (1)
219-223: Keycloak sync aftercreateEcosystemsilently swallows failures — ecosystem and Keycloak can diverge.
updateKeycloakEcosystemLeadscatches all errors and only logs them (line 248). This means the DB transaction succeeds but Keycloak may not be updated, leaving the system in an inconsistent state with no way to recover automatically. The same pattern applies toupdateKeycloakEcosystemMemberat line 510.Consider at minimum:
- Emitting a structured event/metric on failure so monitoring can catch divergence.
- Implementing a reconciliation mechanism or retry queue for failed Keycloak updates.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ecosystem/src/ecosystem.service.ts` around lines 219 - 223, The updateKeycloakEcosystemLeads call currently swallows errors (and updateKeycloakEcosystemMember does the same), which can leave DB state and Keycloak out of sync; change these routines so failures are not silently ignored: in updateKeycloakEcosystemLeads and updateKeycloakEcosystemMember, stop catching-and-only-logging errors—either rethrow after logging so the caller can handle/rollback, or emit a structured failure event/metric and enqueue the failed update to a retry/reconciliation queue (or both). Ensure the caller of createEcosystem (and any callers of the member update) reacts to a thrown error or subscribes to the emitted failure event so monitoring and automated retries can reconcile Keycloak divergence.libs/client-registration/src/client-registration.service.ts (2)
908-912: Silently returning when no service account is found may mask errors.If the service account doesn't exist, this method silently returns without updating anything, and the caller has no indication that the operation was a no-op. Consider throwing or at least returning a flag so the caller can decide how to handle the missing service account.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/client-registration/src/client-registration.service.ts` around lines 908 - 912, The method updateClientServiceAccountEcosystemAccess currently logs and returns silently when serviceAccountUser is missing; change this to surface the condition to callers by either throwing a descriptive error (e.g., ServiceAccountNotFoundError including clientIdpId) or by returning an explicit boolean/enum (e.g., false or "not_found") instead of undefined; update callers or function signature accordingly and replace the logger.warn only with the new error/return path while keeping the log for observability (use this.logger.warn + throw or this.logger.warn + return false) so callers can detect the no-op.
794-868: Excessivethis.logger.log()calls — consider reducing verbosity or usingdebuglevel.Both
updateUserEcosystemAccessandremoveUserEcosystemAccesscontain 10+ log statements each at the defaultloglevel (which maps to INFO in most NestJS transports). This will be very noisy in production. Reservelogfor significant events (start/success/failure) and move the intermediate tracing todebug.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/client-registration/src/client-registration.service.ts` around lines 794 - 868, The updateUserEcosystemAccess and removeUserEcosystemAccess methods are too chatty at INFO level; keep only high-level events (start, success, failure) using this.logger.log and demote intermediate trace lines to this.logger.debug. Specifically, in updateUserEcosystemAccess change the following this.logger.log calls to this.logger.debug: the "User URL", "Current user fetched", "Current attributes", "Parsed existing ecosystem_access" (and the warning stays as warn), "Initialized ecosystem_access for org", checks that create empty arrays, "Existing X ecosystems", "Added ecosystem ..."/"Ecosystem ... already exists", and "Updated ecosystem_access" (keep "Sending update to Keycloak..." and the final success log at log level). Apply the same pattern in removeUserEcosystemAccess: preserve only start/success/error as log and move intermediate state/parse/update messages to debug so production logs are not noisy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/client-registration/src/client-registration.service.ts`:
- Around line 1004-1009: The code reads orgRoles.lead.length and
orgRoles.member.length which can throw if either key is missing; before the
length checks in the block that updates ecosystemAccess[orgId].ecosystem_role
(in the removeUserEcosystemAccess flow), ensure both arrays exist by normalizing
orgRoles: e.g., retrieve const orgRoles = ecosystemAccess[orgId].ecosystem_role
and set orgRoles.lead = orgRoles.lead ?? [] and orgRoles.member =
orgRoles.member ?? [] (or equivalent defensive checks) so the subsequent if
(orgRoles.lead.length === 0 && orgRoles.member.length === 0) is safe. Ensure you
mutate the same orgRoles object stored in ecosystemAccess so delete
ecosystemAccess[orgId] still works as intended.
In `@libs/keycloak-config/src/keycloak-config.service.ts`:
- Around line 36-37: The code reads process.env.KEYCLOAK_REALM into realmName
and logs it without validating, which can lead to API calls using "undefined";
add an early guard in KeycloakConfigService (where realmName is assigned) to
validate process.env.KEYCLOAK_REALM, and if missing throw or log an explicit
error and stop initialization (e.g., throw new Error or return/fail fast) so
subsequent methods that build Keycloak URLs (using realmName) never run with an
invalid value; update the logger call to only run after validation and include
the validated realmName.
- Around line 101-104: The filter callback for orgClients violates
implicit-arrow-linebreak; fix by placing the arrow and its expression on the
same line or using a block return. For example, change the clients.filter
callback to a single-line arrow: clients.filter((client: {
serviceAccountsEnabled: boolean; clientId: string }) =>
client.serviceAccountsEnabled && !client.clientId.startsWith('realm-')) or use a
block form with braces and an explicit return inside the callback for the
orgClients assignment.
In `@libs/keycloak-url/src/keycloak-url.service.ts`:
- Around line 94-96: The GetClientsURL function currently hardcodes ?max=1000
which can truncate results; change GetClientsURL(realm: string) to either accept
an optional pageSize and page params or read a configurable page size from env
(e.g., process.env.KEYCLOAK_CLIENTS_PAGE_SIZE) and build the URL without a fixed
1000, and update the calling code to iterate pages (pagination) using the
clients endpoint until no more results; reference the GetClientsURL function and
ensure the new signature or behavior is used where clients are fetched during
initialization so all clients are processed rather than silently truncated.
- Around line 78-92: GetClientProtocolMappersURL and
GetClientProtocolMappersByIdURL currently return the same path due to a
copy/paste; decide whether you need a single method or a distinct "by id" path
and fix accordingly: either remove GetClientProtocolMappersByIdURL and
consolidate calls to GetClientProtocolMappersURL, or change
GetClientProtocolMappersByIdURL to accept a mapperId (e.g., mapperId: string)
and return
`${process.env.KEYCLOAK_DOMAIN}admin/realms/${realm}/clients/${clientIdpId}/protocol-mappers/models/${mapperId}`;
update any callers of GetClientProtocolMappersByIdURL to pass the mapperId if
you choose the latter.
---
Outside diff comments:
In `@apps/ecosystem/src/ecosystem.service.ts`:
- Around line 338-342: The condition block in ecosystem.service.ts contains a
duplicated check of checkUser.status === Invitation.PENDING; update the
conditional to remove the duplicate or replace the second occurrence with the
intended field check (e.g., checkUser.type === <expectedValue> or another
property), ensuring the full if uses checkUser?.ecosystemId,
checkUser.invitedOrg, checkUser.status === Invitation.PENDING and the correct
additional condition; refer to the checkUser variable and Invitation.PENDING
symbol to locate and fix the duplicate.
---
Duplicate comments:
In `@apps/ecosystem/src/ecosystem.service.ts`:
- Around line 200-202: Remove the dead commented-out guard in EcosystemService
(the commented lines checking "if (!invitation) { throw new
ForbiddenException(ResponseMessages.ecosystem.error.invitationRequired); }") so
the duplicate, commented code is deleted from the method where it appears;
ensure no other references rely on that comment and run tests/lint to confirm
nothing else needs updating.
In `@libs/client-registration/src/client-registration.service.ts`:
- Around line 854-858: The PUT payload currently spreads the entire Keycloak
user object (const updatePayload = { ...currentUser, attributes }) which can
leak read-only or sensitive fields; change this to build a minimal explicit
payload containing only attributes and any other explicitly updatable fields
(e.g., username, firstName, lastName if allowed) instead of spreading
currentUser. Do the same in the removeUserEcosystemAccess flow (the block around
removeUserEcosystemAccess) so both updateUser/updateKeycloak calls only send the
minimal allowed properties rather than the whole currentUser object.
- Line 804: The log currently emits full user attributes
(this.logger.log(`[updateUserEcosystemAccess] Current attributes:
${JSON.stringify(attributes)}`)) which can leak PII; update both
updateUserEcosystemAccess and removeUserEcosystemAccess to stop using
JSON.stringify(attributes) and JSON.stringify(ecosystemAccess) and instead log
only non-sensitive fields or a summary (e.g., allowed keys like id, role, status
or a count of keys/entries) by building a filtered summary object or count prior
to logging and pass that to this.logger.log so sensitive data is never emitted.
---
Nitpick comments:
In `@apps/api-gateway/src/app.module.ts`:
- Line 78: KeycloakConfigModule is loaded unconditionally and triggers Keycloak
HTTP calls on startup; make its import conditional by wrapping
KeycloakConfigModule with the same ConditionalModule.registerWhen pattern used
for OIDC modules (e.g., create or reuse an isKeycloakConfigured predicate that
checks process.env.KEYCLOAK_DOMAIN and process.env.KEYCLOAK_REALM) so
KeycloakConfigModule only registers when both env vars are set, preventing
startup calls when Keycloak is not configured.
In `@apps/ecosystem/src/ecosystem.service.ts`:
- Around line 219-223: The updateKeycloakEcosystemLeads call currently swallows
errors (and updateKeycloakEcosystemMember does the same), which can leave DB
state and Keycloak out of sync; change these routines so failures are not
silently ignored: in updateKeycloakEcosystemLeads and
updateKeycloakEcosystemMember, stop catching-and-only-logging errors—either
rethrow after logging so the caller can handle/rollback, or emit a structured
failure event/metric and enqueue the failed update to a retry/reconciliation
queue (or both). Ensure the caller of createEcosystem (and any callers of the
member update) reacts to a thrown error or subscribes to the emitted failure
event so monitoring and automated retries can reconcile Keycloak divergence.
In `@libs/client-registration/src/client-registration.service.ts`:
- Around line 908-912: The method updateClientServiceAccountEcosystemAccess
currently logs and returns silently when serviceAccountUser is missing; change
this to surface the condition to callers by either throwing a descriptive error
(e.g., ServiceAccountNotFoundError including clientIdpId) or by returning an
explicit boolean/enum (e.g., false or "not_found") instead of undefined; update
callers or function signature accordingly and replace the logger.warn only with
the new error/return path while keeping the log for observability (use
this.logger.warn + throw or this.logger.warn + return false) so callers can
detect the no-op.
- Around line 794-868: The updateUserEcosystemAccess and
removeUserEcosystemAccess methods are too chatty at INFO level; keep only
high-level events (start, success, failure) using this.logger.log and demote
intermediate trace lines to this.logger.debug. Specifically, in
updateUserEcosystemAccess change the following this.logger.log calls to
this.logger.debug: the "User URL", "Current user fetched", "Current attributes",
"Parsed existing ecosystem_access" (and the warning stays as warn), "Initialized
ecosystem_access for org", checks that create empty arrays, "Existing X
ecosystems", "Added ecosystem ..."/"Ecosystem ... already exists", and "Updated
ecosystem_access" (keep "Sending update to Keycloak..." and the final success
log at log level). Apply the same pattern in removeUserEcosystemAccess: preserve
only start/success/error as log and move intermediate state/parse/update
messages to debug so production logs are not noisy.
In `@libs/keycloak-config/src/keycloak-config.service.ts`:
- Around line 112-121: The loop over orgClients in onModuleInit that calls
ensureClientProtocolMapper sequentially is slow; change it to run clients in
parallel with bounded concurrency (e.g., use Promise.allSettled with a
concurrency limiter like p-limit or a simple worker queue) so multiple
ensureClientProtocolMapper(realm, client.id, client.clientId, token) calls
execute concurrently but limited to a safe number; collect results from
Promise.allSettled, then increment created/skipped/failed based on each settled
result value (handle rejections as failures) to preserve existing counters and
avoid blocking module startup.
- Around line 1-3: Remove the file-level ESLint disables for
explicit-function-return-type and explicit-module-boundary-types; instead add
explicit return types to the public and private methods on the
KeycloakConfigService class and any exported functions, and only use inline
eslint-disable comments directly above a specific function that truly requires
it (e.g., keep a narrow disable for buildProtocolMapperPayload if it cannot have
a typed signature). Ensure exported methods and class members declare their
return types to satisfy TypeScript and move any remaining eslint-disable
comments from the top of the file to the smallest possible scope.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
libs/keycloak-config/src/keycloak-config.service.ts (3)
25-28: Accessingerror.messagewithout type-narrowing the caught value.In TypeScript,
catchvariables areunknownby default (withuseUnknownInCatchVariablesor strict mode). Accessing.messagedirectly will fail compilation in stricter configs. This also applies to line 154.Proposed fix
} catch (error) { this.logger.error(`=== Keycloak Ecosystem Configuration FAILED ===`); - this.logger.error(`Error: ${error.message || error}`); + this.logger.error(`Error: ${error instanceof Error ? error.message : error}`); }And similarly at line 154:
} catch (error) { - this.logger.warn(` [${clientId}] FAILED: ${error.message || error}`); + this.logger.warn(` [${clientId}] FAILED: ${error instanceof Error ? error.message : error}`); return 'failed'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/keycloak-config/src/keycloak-config.service.ts` around lines 25 - 28, The catch blocks in KeycloakConfigService are accessing error.message directly which fails under strict/unknown catch types; update both catch sites (the try/catch around the Keycloak ecosystem initialization and the other catch at the later block) to narrow the caught value first: check if (error instanceof Error) and log error.message, otherwise log String(error) or JSON.stringify(error) to ensure compilation and meaningful output, e.g. replace direct error.message access with a guarded branch that calls this.logger.error(error.message) when instanceof Error and this.logger.error(String(error)) otherwise.
1-3: Consider narrowing the ESLint disable directives.The
explicit-function-return-typeandexplicit-module-boundary-typesdisables are file-wide but only needed forbuildProtocolMapperPayloadandgetAuthHeader. Adding return types to the other methods and moving the disables to inline comments on those two methods would keep the rest of the file under lint coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/keycloak-config/src/keycloak-config.service.ts` around lines 1 - 3, Remove the file-wide ESLint disables and instead annotate only the two methods that need them: add explicit return types to all other functions in this file, and move the existing disables to inline comments on the buildProtocolMapperPayload and getAuthHeader declarations (e.g., // eslint-disable-next-line `@typescript-eslint/explicit-function-return-type`, `@typescript-eslint/explicit-module-boundary-types`) so linting remains enabled for the rest of the file while preserving exemptions only for those two functions.
101-101: Consider extracting the system-client list as a named constant.A
private static readonlyat the class level (or a module-level constant) makes it easier to discover and update when Keycloak versions add new built-in clients.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/keycloak-config/src/keycloak-config.service.ts` at line 101, Extract the inline array const systemClients into a named constant (either a module-level export or a private static readonly on the KeycloakConfigService class, e.g. SYSTEM_CLIENTS or KeycloakConfigService.SYSTEM_CLIENTS) and replace the inline declaration with a reference to that constant; this makes the list discoverable and maintainable across the codebase and should be used wherever systemClients is referenced in keycloak-config.service.ts (replace the local variable with the new constant in any method that currently uses systemClients).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/keycloak-config/src/keycloak-config.service.ts`:
- Around line 31-46: In configureEcosystemAccess validate the management token
returned by this.clientRegistrationService.getPlatformManagementToken() (in case
it's null, undefined, or empty) before calling ensureRealmProtocolMapper or
ensureAllClientsProtocolMapper; if the token is invalid, log a clear error via
this.logger.error including context like 'Failed to obtain management token' and
throw or return early to prevent subsequent 401 failures when
ensureRealmProtocolMapper/retrieve clients run. Ensure the check references
configureEcosystemAccess and getPlatformManagementToken so reviewers can find
and update the flow.
- Around line 69-72: The realm-level mapper POST in configureEcosystemAccess
(calling this.commonService.httpPost with the payload from
this.buildProtocolMapperPayload('ecosystem_access') and headers from
this.getAuthHeader) should be wrapped in a local try/catch so failures don't
abort the rest of configureEcosystemAccess; catch the error, call
this.logger.error with a descriptive message including the error, and proceed
(do not rethrow) so the per-client mapper setup (which already has its own
try/catch) still runs.
---
Duplicate comments:
In `@libs/keycloak-config/src/keycloak-config.service.ts`:
- Around line 36-37: The code reads process.env.KEYCLOAK_REALM into realmName
(const realmName) and logs it without validating; if undefined it creates
invalid API URLs — update the KeycloakConfigService initialization (where
realmName is defined) to validate that realmName is a non-empty string, log a
clear error including the variable name when missing, and throw an exception (or
fail-fast) so callers cannot proceed with an "undefined" realm; alternatively,
allow a documented default only if acceptable. Ensure you reference and update
the place that defines const realmName and the logger.log call so the service
never constructs API URLs using an undefined realm.
- Around line 103-106: The implicit-arrow-linebreak ESLint error comes from the
multiline arrow callback passed to clients.filter when computing targetClients;
fix it by making the arrow function body start on the same line as the arrow (or
convert the arrow to a block with curly braces and an explicit return on the
same line as the arrow). Update the clients.filter callback (the anonymous arrow
in the targetClients assignment) so the arrow and its body are not split by a
line break, e.g. place "=> !client.clientId.startsWith(...)" on the same line as
the parameter list or use "{ return ... }" immediately after the "=>".
---
Nitpick comments:
In `@libs/keycloak-config/src/keycloak-config.service.ts`:
- Around line 25-28: The catch blocks in KeycloakConfigService are accessing
error.message directly which fails under strict/unknown catch types; update both
catch sites (the try/catch around the Keycloak ecosystem initialization and the
other catch at the later block) to narrow the caught value first: check if
(error instanceof Error) and log error.message, otherwise log String(error) or
JSON.stringify(error) to ensure compilation and meaningful output, e.g. replace
direct error.message access with a guarded branch that calls
this.logger.error(error.message) when instanceof Error and
this.logger.error(String(error)) otherwise.
- Around line 1-3: Remove the file-wide ESLint disables and instead annotate
only the two methods that need them: add explicit return types to all other
functions in this file, and move the existing disables to inline comments on the
buildProtocolMapperPayload and getAuthHeader declarations (e.g., //
eslint-disable-next-line `@typescript-eslint/explicit-function-return-type`,
`@typescript-eslint/explicit-module-boundary-types`) so linting remains enabled
for the rest of the file while preserving exemptions only for those two
functions.
- Line 101: Extract the inline array const systemClients into a named constant
(either a module-level export or a private static readonly on the
KeycloakConfigService class, e.g. SYSTEM_CLIENTS or
KeycloakConfigService.SYSTEM_CLIENTS) and replace the inline declaration with a
reference to that constant; this makes the list discoverable and maintainable
across the codebase and should be used wherever systemClients is referenced in
keycloak-config.service.ts (replace the local variable with the new constant in
any method that currently uses systemClients).
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
libs/keycloak-config/src/keycloak-config.service.ts (3)
1-3: Consider narrowing ESLint disables to specific lines.Blanket file-level disables for
@typescript-eslint/explicit-function-return-typeand@typescript-eslint/explicit-module-boundary-typessuppress useful checks across the whole file. OnlybuildProtocolMapperPayload(line 275) andgetAuthHeader(line 291) actually lack explicit return types — adding return types to those two methods would let you remove the file-level disables.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/keycloak-config/src/keycloak-config.service.ts` around lines 1 - 3, Remove the broad file-level ESLint disables and instead add explicit return types to the two functions that need them: declare a proper return type for buildProtocolMapperPayload (the function around line 275) and for getAuthHeader (around line 291), then delete the `@typescript-eslint/explicit-function-return-type` and `@typescript-eslint/explicit-module-boundary-types` disables at the top of keycloak-config.service.ts so only those specific fixes satisfy the linter without suppressing checks for the whole file.
20-35: Configuration runs on every application startup — consider a feature flag.
onModuleInittriggers the full Keycloak configuration workflow (token fetch, scope lookup, client enumeration, user verification) on every process restart. In production environments with many clients/users, this adds HTTP round-trips and log volume to each startup cycle. Since the mappers are persistent in Keycloak, consider gating this behind an environment variable (e.g.,KEYCLOAK_ECOSYSTEM_CONFIG_ENABLED=true) so it can be disabled once the initial setup is complete.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/keycloak-config/src/keycloak-config.service.ts` around lines 20 - 35, The onModuleInit method currently runs the full Keycloak configuration on every startup; gate that behavior behind an opt-in environment flag (e.g., KEYCLOAK_ECOSYSTEM_CONFIG_ENABLED) so the workflow only runs when explicitly enabled. Modify onModuleInit to read the flag (via process.env or your config provider) at the top, log a concise message and return early when the flag is false/unset, and only call configureEcosystemAccess() when the flag is truthy; keep existing try/catch and logging around configureEcosystemAccess() unchanged. Ensure the flag name and onModuleInit and configureEcosystemAccess identifiers are used so reviewers can find the change.
130-158: Excessive INFO-level logging of all client metadata during initialization.Lines 131–136 log every client's
clientId, internalid,serviceAccountsEnabled,bearerOnly, andenabledstatus at INFO level. Lines 152–158 repeat similar detail for target/excluded lists. This produces substantial log volume on every application startup and may expose internal Keycloak resource IDs in production logs.Consider gating this behind a debug/verbose flag or lowering to
this.logger.debug(...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/keycloak-config/src/keycloak-config.service.ts` around lines 130 - 158, The code is logging full client metadata at info level; change detailed per-client outputs to debug (replace this.logger.log(...) with this.logger.debug(...)) or wrap them behind a verbose/debug flag check so only summary lines stay at info; keep the concise summary (e.g., this.logger.log(`Found ${clients.length}...`) and counts for targetClients/excludedClients) at info but remove or redact internal IDs from any remaining info-level messages (avoid logging c.id), and use the existing variables/arrays (clients, systemClients, targetClients, excludedClients) and this.logger methods to implement this.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/keycloak-config/src/keycloak-config.service.ts`:
- Around line 235-257: The per-user logging in the for loop that calls
this.logger.log with user.username and user.id (and attributes) should be
removed or made non-PII: update the loop in keycloak-config.service.ts (the
section that increments serviceAccountUsers, usersWithAttribute,
usersWithoutAttribute) to stop emitting raw identifiers at INFO level—either
gate those logs behind a debug flag (use this.logger.debug) or redact/hash
user.username and user.id before logging, and ensure any remaining messages
contain only aggregate counts or non-identifying summaries rather than raw PII.
- Line 243: The log construction in keycloak-config.service.ts uses
user.attributes.ecosystem_access[0].substring(0, 100) which can throw if that
value isn't a string; wrap the value with String(...) before calling substring
(same pattern used on the other occurrence around line with .ecosystem_access)
so the template uses
String(user.attributes.ecosystem_access[0]).substring(0,100) (or equivalently
convert to string then substring) to safely guard ServiceAccount logging in the
verify/verification loop.
- Around line 218-222: In verifyUsersEcosystemAccess, stop building the users
URL via process.env and string concatenation; instead call
KeycloakUrlService.createUserURL(realm) to get the base users endpoint and
append the query param(s) to that result. Replace the hard-coded "?max=100"
behavior with proper pagination (e.g., loop requests using start/max or
next-page parameters) so you fetch all users rather than capping at 100; keep
using this.getAuthHeader(token) and commonService.httpGet for each page. Ensure
error handling/logging remains intact and reference verifyUsersEcosystemAccess
and KeycloakUrlService.createUserURL when locating the change.
---
Duplicate comments:
In `@libs/keycloak-config/src/keycloak-config.service.ts`:
- Around line 37-59: In configureEcosystemAccess(), add null/undefined guards
immediately after obtaining token and reading environment variables: verify the
result of this.clientRegistrationService.getPlatformManagementToken() (token)
and process.env.KEYCLOAK_REALM / process.env.KEYCLOAK_DOMAIN (realmName,
keycloakDomain); if any are missing/invalid, log a clear error with context and
stop further execution (throw an Error or return) before calling
ensureRealmProtocolMapper, ensureAllClientsProtocolMapper, or
verifyUsersEcosystemAccess to avoid downstream 401s or malformed URLs.
- Around line 91-98: The realm-level POST call using this.commonService.httpPost
in configureEcosystemAccess can throw and currently aborts subsequent steps;
wrap the createResponse call in a try/catch similar to the per-client flow so
failures are logged but do not stop ensureAllClientsProtocolMapper and
verifyUsersEcosystemAccess from running. Specifically, enclose the
httpPost(mappersUrl, mapperPayload, this.getAuthHeader(token)) invocation (after
building mapperPayload via buildProtocolMapperPayload('ecosystem_access')) in a
try block, on catch log the error via this.logger.error with context (include
the error and mappersUrl/mapperPayload identifiers) and continue execution
rather than rethrowing. Ensure behavior mirrors the per-client method's error
handling to maintain resilience while still recording the failure.
- Around line 140-148: ESLint flags implicit-arrow-linebreak for the multi-line
arrow predicates in the clients.filter calls; fix by putting the arrow body
expression on the same line as the => (or convert the arrow to a block with
explicit return) for both targetClients and excludedClients so the predicate
starts immediately after the => in the clients.filter(...) calls referencing
client.clientId, client.bearerOnly and systemClients.includes.
---
Nitpick comments:
In `@libs/keycloak-config/src/keycloak-config.service.ts`:
- Around line 1-3: Remove the broad file-level ESLint disables and instead add
explicit return types to the two functions that need them: declare a proper
return type for buildProtocolMapperPayload (the function around line 275) and
for getAuthHeader (around line 291), then delete the
`@typescript-eslint/explicit-function-return-type` and
`@typescript-eslint/explicit-module-boundary-types` disables at the top of
keycloak-config.service.ts so only those specific fixes satisfy the linter
without suppressing checks for the whole file.
- Around line 20-35: The onModuleInit method currently runs the full Keycloak
configuration on every startup; gate that behavior behind an opt-in environment
flag (e.g., KEYCLOAK_ECOSYSTEM_CONFIG_ENABLED) so the workflow only runs when
explicitly enabled. Modify onModuleInit to read the flag (via process.env or
your config provider) at the top, log a concise message and return early when
the flag is false/unset, and only call configureEcosystemAccess() when the flag
is truthy; keep existing try/catch and logging around configureEcosystemAccess()
unchanged. Ensure the flag name and onModuleInit and configureEcosystemAccess
identifiers are used so reviewers can find the change.
- Around line 130-158: The code is logging full client metadata at info level;
change detailed per-client outputs to debug (replace this.logger.log(...) with
this.logger.debug(...)) or wrap them behind a verbose/debug flag check so only
summary lines stay at info; keep the concise summary (e.g.,
this.logger.log(`Found ${clients.length}...`) and counts for
targetClients/excludedClients) at info but remove or redact internal IDs from
any remaining info-level messages (avoid logging c.id), and use the existing
variables/arrays (clients, systemClients, targetClients, excludedClients) and
this.logger methods to implement this.
| serviceAccountUsers++; | ||
| if (hasEcosystemAccess) { | ||
| this.logger.log( | ||
| ` [SERVICE-ACCOUNT] ${user.username} - HAS ecosystem_access: ${user.attributes.ecosystem_access[0].substring(0, 100)}...` |
There was a problem hiding this comment.
substring call can throw if ecosystem_access[0] is not a string.
Keycloak user attributes are typically string[], but if a value is unexpectedly a number or object, calling .substring(0, 100) will throw a TypeError at runtime, crashing the verification loop.
Proposed fix
- ` [SERVICE-ACCOUNT] ${user.username} - HAS ecosystem_access: ${user.attributes.ecosystem_access[0].substring(0, 100)}...`
+ ` [SERVICE-ACCOUNT] ${user.username} - HAS ecosystem_access: ${String(user.attributes.ecosystem_access[0]).substring(0, 100)}...`Apply the same String(...) wrapping on line 250.
Also applies to: 250-250
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/keycloak-config/src/keycloak-config.service.ts` at line 243, The log
construction in keycloak-config.service.ts uses
user.attributes.ecosystem_access[0].substring(0, 100) which can throw if that
value isn't a string; wrap the value with String(...) before calling substring
(same pattern used on the other occurrence around line with .ecosystem_access)
so the template uses
String(user.attributes.ecosystem_access[0]).substring(0,100) (or equivalently
convert to string then substring) to safely guard ServiceAccount logging in the
verify/verification loop.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
scripts/cleanup-ecosystem-mappers.ts (3)
29-31: Fragile Content-Type heuristic.The check
url.includes('token')to decide betweenapplication/x-www-form-urlencodedandapplication/jsonis brittle — any URL containing "token" in a path segment (e.g., a client named "token-service") would get the wrong Content-Type. SincegetTokenis the only caller that needs form encoding, consider passing the content type explicitly instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/cleanup-ecosystem-mappers.ts` around lines 29 - 31, The Content-Type heuristic using url.includes('token') is brittle; modify the request helper (the code that sets headers['Content-Type'] where currently it uses 'POST' === method && url.includes('token')) to accept an explicit contentType parameter (or options.contentType) and use that if provided, falling back to 'application/json' for JSON requests and 'application/x-www-form-urlencoded' only when explicitly requested; update the getToken caller to pass contentType = 'application/x-www-form-urlencoded' so only getToken uses form encoding and remove the url.includes('token') check from the header assignment.
138-167: User cleanup silently caps at 500 users; clients at 1000.
removeUserAttributesfetchesmax=500(Line 142) andremoveClientMappersfetchesmax=1000(Line 107). If the realm exceeds these limits, the cleanup will be incomplete without any warning. Consider logging a warning when the result count equals the max, or implementing pagination.
⚠️ Example warningconst users = await request('GET', usersUrl, token); + if (users.length >= 500) { + log('WARNING: Fetched 500 users (the max). There may be more users that need cleanup. Consider re-running or increasing the limit.'); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/cleanup-ecosystem-mappers.ts` around lines 138 - 167, removeUserAttributes and removeClientMappers silently cap results because they request fixed max limits (usersUrl uses "?max=500" and clientsUrl uses "?max=1000"), so when the API returns exactly that max you should either implement pagination or at minimum log a warning so cleanup isn't incomplete. Update removeUserAttributes and removeClientMappers to detect when the returned array length equals the requested max (users/clients response length) and then either loop with pagination (fetch next page using start/first/offset params) until fewer than max are returned, or emit a clear warning log mentioning usersUrl/clientsUrl and the max value so operators know the run was partial.
35-64: No request timeout — script can hang indefinitely if Keycloak is unresponsive.The
http/https.requestcalls have no timeout configured. For an operational cleanup script, this can leave processes hanging. Consider adding a timeout option.⏱️ Proposed fix
const req = lib.request( { hostname: parsed.hostname, port: parsed.port, path: parsed.pathname + parsed.search, method, - headers + headers, + timeout: 30000 }, (res) => {Also add a timeout handler:
req.on('error', reject); + req.on('timeout', () => { + req.destroy(); + reject(new Error(`Request timed out: ${method} ${url}`)); + }); if (body) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/cleanup-ecosystem-mappers.ts` around lines 35 - 64, The request currently uses lib.request without any timeout, which can hang; add a bounded timeout (e.g., const REQUEST_TIMEOUT_MS = 30000) and attach a handler to the created req: call req.setTimeout(REQUEST_TIMEOUT_MS) and on timeout destroy/abort the request and call reject(new Error(`request timeout ${REQUEST_TIMEOUT_MS}ms for ${method} ${url}`)); also ensure you remove/avoid double rejecting by only rejecting once and that the 'error' listener on req still exists; update the block around lib.request/req to include this timeout handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/keycloak-config/src/keycloak-config.service.ts`:
- Around line 168-195: The startup logs currently emit per-client details using
this.logger.log inside the onModuleInit flow (iterating over clients,
targetClients, and excludedClients), causing verbose INFO-level output; change
the per-client lines that call this.logger.log for each client to use
this.logger.debug instead (keeping the existing aggregate counts and the "Target
clients (N)" / "Excluded clients (N)" summary at INFO), and avoid printing
sensitive fields (IDs/serviceAccount/bearerOnly) at INFO—only include them in
the debug-level messages for targetClients/excludedClients so detailed client
metadata is only visible when debug logging is enabled.
In `@scripts/cleanup-ecosystem-mappers.ts`:
- Around line 12-15: Add an early guard that validates KEYCLOAK_DOMAIN,
KEYCLOAK_REALM, KEYCLOAK_MANAGEMENT_CLIENT_ID, and
KEYCLOAK_MANAGEMENT_CLIENT_SECRET are defined and non-empty before any URL
construction or Keycloak calls; if any are missing, log a clear error including
which env var(s) are missing (use the constant names KEYCLOAK_DOMAIN,
KEYCLOAK_REALM, KEYCLOAK_MANAGEMENT_CLIENT_ID,
KEYCLOAK_MANAGEMENT_CLIENT_SECRET) and exit the process with a non-zero code so
the script fails fast rather than producing malformed URLs.
---
Duplicate comments:
In `@libs/keycloak-config/src/keycloak-config.service.ts`:
- Around line 177-185: The two filter callbacks used to compute targetClients
and excludedClients violate implicit-arrow-linebreak; update the arrow functions
in the clients.filter calls (the callbacks that reference client.clientId,
client.bearerOnly and systemClients) so the predicate is either placed on the
same line as the => (single-line concise body) or converted to a block body with
explicit return (e.g., { return ...; }), ensuring no line break directly before
the arrow body.
- Around line 37-46: In configureEcosystemAccess(), validate the management
token returned by clientRegistrationService.getPlatformManagementToken() and the
environment values before proceeding: check that token is non-null/non-empty and
that process.env.KEYCLOAK_REALM (and preferably process.env.KEYCLOAK_DOMAIN) are
defined; if any are missing or invalid, log a clear error via this.logger.error
including which value failed and return/throw to avoid issuing downstream
requests; ensure subsequent steps that construct URLs or call Keycloak APIs (the
existing configuration steps in configureEcosystemAccess) only run when these
validations pass.
- Around line 128-135: In ensureRealmProtocolMapper, the unguarded await
this.commonService.httpPost(...) can throw and abort further steps in
configureEcosystemAccess; wrap the POST call in a try/catch (like the per-client
method already does) around the createResponse call so failures (HTTP 409,
network errors, etc.) are caught, log a descriptive message including the error
and mapperPayload (use this.logger.error and JSON.stringify where appropriate),
and allow the method to continue to subsequent steps; reference the
ensureRealmProtocolMapper function, commonService.httpPost,
buildProtocolMapperPayload('ecosystem_access'), and getAuthHeader(token) when
making the change.
- Around line 255-294: In verifyUsersEcosystemAccess replace the manual usersUrl
construction with this.keycloakUrlService.createUserURL(realm) to centralize URL
generation; avoid logging PII at INFO by switching the per-user this.logger.log
calls to this.logger.debug (or remove them and only log aggregated counts
usersWithAttribute/usersWithoutAttribute/serviceAccountUsers) and keep the
summary at info; and guard the ecosystem_access substring calls by ensuring the
value is a string before calling substring (e.g., coerce with
String(user.attributes.ecosystem_access[0]) or check typeof before using
substring) to prevent TypeError when attribute values are not strings.
In `@libs/keycloak-url/src/keycloak-url.service.ts`:
- Around line 74-100: GetClientProtocolMappersByIdURL currently returns the
exact same path as GetClientProtocolMappersURL; change
GetClientProtocolMappersByIdURL to accept a mapperId (e.g., signature
GetClientProtocolMappersByIdURL(realm: string, clientIdpId: string, mapperId:
string)) and include the mapperId in the path segment after
/protocol-mappers/models so it points to a single mapper resource. For
GetClientsURL, remove the hardcoded ?max=1000 and instead accept an optional
max/limit parameter (e.g., max?: number) or build the query string only when a
max is provided so large realms aren’t silently truncated; ensure the method
still URL-encodes and appends the query correctly when present.
---
Nitpick comments:
In `@scripts/cleanup-ecosystem-mappers.ts`:
- Around line 29-31: The Content-Type heuristic using url.includes('token') is
brittle; modify the request helper (the code that sets headers['Content-Type']
where currently it uses 'POST' === method && url.includes('token')) to accept an
explicit contentType parameter (or options.contentType) and use that if
provided, falling back to 'application/json' for JSON requests and
'application/x-www-form-urlencoded' only when explicitly requested; update the
getToken caller to pass contentType = 'application/x-www-form-urlencoded' so
only getToken uses form encoding and remove the url.includes('token') check from
the header assignment.
- Around line 138-167: removeUserAttributes and removeClientMappers silently cap
results because they request fixed max limits (usersUrl uses "?max=500" and
clientsUrl uses "?max=1000"), so when the API returns exactly that max you
should either implement pagination or at minimum log a warning so cleanup isn't
incomplete. Update removeUserAttributes and removeClientMappers to detect when
the returned array length equals the requested max (users/clients response
length) and then either loop with pagination (fetch next page using
start/first/offset params) until fewer than max are returned, or emit a clear
warning log mentioning usersUrl/clientsUrl and the max value so operators know
the run was partial.
- Around line 35-64: The request currently uses lib.request without any timeout,
which can hang; add a bounded timeout (e.g., const REQUEST_TIMEOUT_MS = 30000)
and attach a handler to the created req: call req.setTimeout(REQUEST_TIMEOUT_MS)
and on timeout destroy/abort the request and call reject(new Error(`request
timeout ${REQUEST_TIMEOUT_MS}ms for ${method} ${url}`)); also ensure you
remove/avoid double rejecting by only rejecting once and that the 'error'
listener on req still exists; update the block around lib.request/req to include
this timeout handling.
Comment on lines
+168
to
+195
| this.logger.log('All clients:'); | ||
| for (const c of clients) { | ||
| this.logger.log( | ||
| ` - clientId: "${c.clientId}", id: "${c.id}", serviceAccount: ${c.serviceAccountsEnabled}, bearerOnly: ${c.bearerOnly}, enabled: ${c.enabled}` | ||
| ); | ||
| } | ||
|
|
||
| const systemClients = ['admin-cli', 'account', 'account-console', 'broker', 'security-admin-console']; | ||
|
|
||
| const targetClients = clients.filter( | ||
| (client: { clientId: string; bearerOnly: boolean }) => | ||
| !client.clientId.startsWith('realm-') && !client.bearerOnly && !systemClients.includes(client.clientId) | ||
| ); | ||
|
|
||
| const excludedClients = clients.filter( | ||
| (client: { clientId: string; bearerOnly: boolean }) => | ||
| client.clientId.startsWith('realm-') || client.bearerOnly || systemClients.includes(client.clientId) | ||
| ); | ||
|
|
||
| this.logger.log(''); | ||
| this.logger.log(`Target clients (${targetClients.length}):`); | ||
| for (const c of targetClients) { | ||
| this.logger.log(` + "${c.clientId}" (id: ${c.id})`); | ||
| } | ||
| this.logger.log(`Excluded clients (${excludedClients.length}):`); | ||
| for (const c of excludedClients) { | ||
| this.logger.log(` - "${c.clientId}" (reason: system/bearer-only/realm)`); | ||
| } |
There was a problem hiding this comment.
Excessive verbose logging of all clients on every startup.
Lines 168-195 enumerate every client in the realm (IDs, service-account flags, bearer-only status) at INFO level during onModuleInit. This runs on every API gateway restart, producing significant log noise in production and potentially leaking client metadata to log aggregation systems.
Consider downgrading per-client detail to debug level and keeping only the aggregate count at info.
♻️ Suggested approach
this.logger.log(`Found ${clients.length} total clients in realm`);
- this.logger.log('All clients:');
for (const c of clients) {
- this.logger.log(
+ this.logger.debug(
` - clientId: "${c.clientId}", id: "${c.id}", serviceAccount: ${c.serviceAccountsEnabled}, bearerOnly: ${c.bearerOnly}, enabled: ${c.enabled}`
);
}
// ... same for targetClients and excludedClients loops🧰 Tools
🪛 ESLint
[error] 179-179: Expected no linebreak before this expression.
(implicit-arrow-linebreak)
[error] 184-184: Expected no linebreak before this expression.
(implicit-arrow-linebreak)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/keycloak-config/src/keycloak-config.service.ts` around lines 168 - 195,
The startup logs currently emit per-client details using this.logger.log inside
the onModuleInit flow (iterating over clients, targetClients, and
excludedClients), causing verbose INFO-level output; change the per-client lines
that call this.logger.log for each client to use this.logger.debug instead
(keeping the existing aggregate counts and the "Target clients (N)" / "Excluded
clients (N)" summary at INFO), and avoid printing sensitive fields
(IDs/serviceAccount/bearerOnly) at INFO—only include them in the debug-level
messages for targetClients/excludedClients so detailed client metadata is only
visible when debug logging is enabled.
Signed-off-by: Tipu_Singh <tipu.singh@ayanworks.com>
Signed-off-by: Tipu_Singh <tipu.singh@ayanworks.com>
Signed-off-by: Tipu_Singh <tipu.singh@ayanworks.com>
Signed-off-by: Tipu_Singh <tipu.singh@ayanworks.com>
Signed-off-by: Tipu_Singh <tipu.singh@ayanworks.com>
tipusinghaw
force-pushed
the
feat/ecosystem-auth-main
branch
from
aea5ff1 to
d71fe38
Compare
Signed-off-by: Tipu_Singh <tipu.singh@ayanworks.com>
Signed-off-by: Tipu_Singh <tipu.singh@ayanworks.com>
tipusinghaw
changed the title
GHkrishna
requested changes
Mar 3, 2026
Comment on lines
+678
to
+680
| if (!uuidRegex.test(clientId)) { | ||
| return this.authenticateClientKeycloak(clientId, clientSecret); | ||
| } |
Contributor
There was a problem hiding this comment.
This check seems unwanted here. We can instead check this after the authenticationResult line and return value for it
Contributor
Author
There was a problem hiding this comment.
Currently, we only support UUID-based clients. However, for the trust-service, we need a dedicated custom client (trust-service). We do not require additional platform features such as session management. We only need a raw access token from Keycloak.
Contributor
Author
There was a problem hiding this comment.
Currently, we only support UUID-based clients. However, for the trust-service, we need a dedicated custom client (trust-service). We do not require additional platform features such as session management — we only need a raw access token from Keycloak.
Comment on lines
+237
to
+238
| this.logger.log(` [${clientId}] Has ${mapperNames.length} mappers: [${mapperNames.join(', ')}]`); | ||
| this.logger.log(` [${clientId}] "ecosystem_access_mapper" NOT FOUND - CREATING...`); |
Contributor
There was a problem hiding this comment.
Suggested change
| this.logger.log(` [${clientId}] Has ${mapperNames.length} mappers: [${mapperNames.join(', ')}]`); | |
| this.logger.log(` [${clientId}] "ecosystem_access_mapper" NOT FOUND - CREATING...`); | |
| this.logger.debug(` [${clientId}] Has ${mapperNames.length} mappers: [${mapperNames.join(', ')}]`); | |
| this.logger.debug(` [${clientId}] "ecosystem_access_mapper" NOT FOUND - CREATING...`); |
Contributor
Author
There was a problem hiding this comment.
We will remove all these logs (these many logs are not needed) after testing on the instance.
Signed-off-by: Tipu_Singh <tipu.singh@ayanworks.com>
Signed-off-by: Tipu_Singh <tipu.singh@ayanworks.com>
|
GHkrishna
approved these changes
Mar 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Chores