Skip to content

Conversation

@shitrerohit
Copy link
Contributor

@shitrerohit shitrerohit commented Sep 26, 2025

What?

API for generate client token for end user using orgId and admin client details.
Add new column for storing the app launcher details.

How?

Wrote the API to get admin client details and org id using this details generate the institute token and return it
Added new column in organization table to store the app launcher details.

Summary by CodeRabbit

  • New Features

    • New endpoint to generate client API tokens for organizations.
  • Improvements

    • Standardized client-token request format across services.
    • Organization responses now include app launch details.
    • More reliable token-generation flow with improved admin/client token handling, external lookups, and clearer error messages for token/client failures.
  • Chores

    • Database migration added to support app launch details.

@shitrerohit shitrerohit self-assigned this Sep 26, 2025
@coderabbitai
Copy link

coderabbitai bot commented Sep 26, 2025

Walkthrough

Adds client-credentials token generation: new ClientTokenDto across API Gateway, Organization app, and libraries; API Gateway endpoint forwards DTO over NATS; Organization service handles token flow via client-registration and Keycloak URL helpers; adds DB migration for organisation.appLaunchDetails and related response messages.

Changes

Cohort / File(s) Summary
API Gateway: DTO, Controller, Service
apps/api-gateway/src/organization/dtos/client-token.dto.ts, apps/api-gateway/src/organization/organization.controller.ts, apps/api-gateway/src/organization/organization.service.ts
Add ClientTokenDto; add POST /generateIssuerApiToken endpoint accepting DTO; service forwards DTO via NATS subject generate-client-api-token.
Organization App: DTO, Controller, Service, Repository
apps/organization/dtos/client-token.dto.ts, apps/organization/src/organization.controller.ts, apps/organization/src/organization.service.ts, apps/organization/repositories/organization.repository.ts
Add ClientTokenDto; add MessagePattern handler generate-client-api-token and controller/service methods implementing token-generation flow (org validation, admin SAT via client-registration, fetch client details, exchange client creds for access token); repository selects appLaunchDetails.
Client Registration Library
libs/client-registration/src/client-registration.service.ts, libs/client-registration/src/dtos/client-token.dto.ts
Add ClientTokenDto; add generateTokenUsingAdminCredentials (form-encoded POST to SAT) and fetchClientDetails (GET client/secret) methods; introduce qs usage and new imports.
Keycloak URL Service
libs/keycloak-url/src/keycloak-url.service.ts
Add URL builders GetUserByUsernameURL, GetSATURL, ResetPasswordURL; standardize client-related parameter names to clientId.
Common Response Messages
libs/common/src/response-messages/index.ts
Add adminTokenDetails, clientDetails, and invalidClientCredentials keys under ResponseMessages.organisation.error.
Database Schema & Migration
libs/prisma-service/prisma/schema.prisma, libs/prisma-service/prisma/migrations/.../migration.sql
Add optional appLaunchDetails (Json / JSONB) field to organisation model and provide SQL migration.
Repo tooling
.husky/pre-push
Adjust unsigned-commit detection messaging and related formatting/text; remove head_commit block.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Client
  participant APIGW as API Gateway\nOrganizationController
  participant AGS as API Gateway\nOrganizationService
  participant NATS as NATS
  participant ORG as Organization Service
  participant CR as ClientRegistrationService
  participant KURL as KeycloakUrlService
  participant KC as Keycloak

  Client->>APIGW: POST /generateIssuerApiToken\n{ orgId, clientAlias, clientId, clientSecret, grantType? }
  APIGW->>AGS: generateClientApiToken(dto)
  AGS->>NATS: send('generate-client-api-token', dto)
  NATS->>ORG: RPC generate-client-api-token(dto)

  ORG->>ORG: validate organization exists
  ORG->>CR: generateTokenUsingAdminCredentials({ orgId, clientAlias })
  CR->>KURL: build SAT URL
  CR->>KC: POST admin credentials (form-encoded)
  KC-->>CR: { access_token }
  CR-->>ORG: admin token

  ORG->>CR: fetchClientDetails(clientId, admin token)
  CR->>KURL: build client & secret URLs
  CR->>KC: GET client & secret
  KC-->>CR: client details + secret
  CR-->>ORG: clientSecret

  ORG->>KC: POST token (clientId, clientSecret, grant_type)
  KC-->>ORG: { access_token }
  ORG-->>NATS: { token }
  NATS-->>AGS: { token }
  AGS-->>APIGW: { token }
  APIGW-->>Client: 200 application/json { token }
Loading
sequenceDiagram
  participant ORG as Organization Service
  participant LOG as Logger
  participant Caller as Caller

  rect rgba(255,235,205,0.18)
  note right of ORG: Error handling summary
  ORG-->>Caller: NotFound if org missing
  ORG-->>Caller: NotFound if client alias/credentials mismatch
  ORG-->>Caller: InternalServerError if admin token absent
  ORG->>LOG: log error
  ORG->>Caller: throw RpcException
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Suggested labels

feature

Suggested reviewers

  • RinkalBhojani
  • GHkrishna

Poem

"I hop through code with carrot bright,
DTOs sprout tokens in the night.
NATS hums softly, Keycloak turns the key,
appLaunchDetails nests where features be.
A rabbit cheers — deploy with glee! 🐇"

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title “Feat/generate token for client” accurately captures the primary purpose of the pull request—adding functionality to generate a client token—and is concise and directly related to the main change set.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/generate-token-for-client

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 598d182 and 42eca11.

📒 Files selected for processing (1)
  • .husky/pre-push (2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-30T06:35:15.457Z
Learnt from: GHkrishna
PR: credebl/platform#1469
File: .husky/pre-push:34-35
Timestamp: 2025-09-30T06:35:15.457Z
Learning: In the credebl/platform repository's pre-push hook (.husky/pre-push), when checking commit signatures, user GHkrishna encountered an edge case where pulling squashed commits from other contributors causes signature validation issues because those commits are signed with others' GPG keys. The user prefers a solution that distinguishes local commits from upstream commits to apply signature validation only to the current user's own commits.

Applied to files:

  • .husky/pre-push
🔇 Additional comments (2)
.husky/pre-push (2)

31-31: Cosmetic whitespace change with no functional impact.

The added trailing whitespace after the assignment doesn't affect the script's behavior.


1-91: Consider addressing remaining feedback from previous reviews.

While this change addresses the success message wording, previous reviews identified other concerns that remain unaddressed:

  1. Missing GPG availability check at the start of the script
  2. Rebase logic issues (lines 50-60) regarding root commit handling, misleading messaging, and missing failure checks

These issues are outside the scope of the current changes but should be addressed in a follow-up to ensure the hook is robust.

Based on past review comments.


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.

❤️ Share
🧪 Early access (Sonnet 4.5): enabled

We are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience.

Note:

  • Public repositories are always opted into early access features.
  • You can enable or disable early access features from the CodeRabbit UI or by updating the CodeRabbit configuration file.

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 14

🧹 Nitpick comments (6)
apps/organization/src/organization.service.ts (1)

2054-2057: Use the resolved client ID instead of the org ID

After fetching the Keycloak client information, we still hand orgId to authenticateClientKeycloak. That only works because we currently choose clientId === orgId; if we ever register clients differently (or migrate existing data), token generation will silently break. Please pass the actual client identifier returned by Keycloak/our repository instead of assuming they will always stay in lockstep.

Apply this diff to rely on the client identifier we just fetched:

-      const { secret } = clientDetails[0];
+      const { clientId: resolvedClientId, secret } = clientDetails[0];
       //step3: generate the token using client id and secret
-      const authenticationResult = await this.authenticateClientKeycloak(generateTokenDetails.orgId, secret);
+      const authenticationResult = await this.authenticateClientKeycloak(resolvedClientId, secret);
apps/organization/dtos/client-token.dto.ts (1)

1-6: Avoid DTO duplication; re-export the shared type from libs.

Consumers in this app can import the same DTO from libs, keeping a single source of truth.

Apply this diff:

-export class ClientTokenDto {
-  orgId: string;
-  clientId: string;
-  clientSecret: string;
-  grantType?: string = 'client_credentials';
-}
+export { ClientTokenDto } from '@credebl/client-registration/dtos/client-token.dto';
libs/client-registration/src/dtos/client-token.dto.ts (1)

1-6: Constrain grantType and document secret handling.

Narrowing grantType improves type-safety; also ensure secrets aren’t logged anywhere downstream.

Apply this diff:

-export class ClientTokenDto {
-  orgId: string;
-  clientId: string;
-  clientSecret: string;
-  grantType?: string = 'client_credentials';
-}
+export type GrantType = 'client_credentials';
+export class ClientTokenDto {
+  orgId: string;
+  clientId: string;
+  clientSecret: string; // Do not log this value.
+  grantType?: GrantType = 'client_credentials';
+}
apps/organization/src/organization.controller.ts (2)

7-15: Prevent confusion with Nest’s Payload decorator by aliasing the type.

Alias the imported Payload type to avoid colliding with Nest’s decorator name and improve readability.

-import {
+import {
   IDidList,
   IGetOrgById,
   IGetOrganization,
   IOrgDetails,
   IUpdateOrganization,
-  Payload
+  Payload as OrgPayload
 } from '../interfaces/organization.interface';

Update usages below accordingly (see related suggestions).


268-270: Fix method name typo (credentails → credentials).

Purely cosmetic but improves readability and searchability.

-  async clientLoginCredentails(payload: { clientId: string; clientSecret: string }): Promise<IAccessTokenData> {
-    return this.organizationService.clientLoginCredentails(payload);
+  async clientLoginCredentials(payload: { clientId: string; clientSecret: string }): Promise<IAccessTokenData> {
+    return this.organizationService.clientLoginCredentials(payload);
apps/api-gateway/src/organization/dtos/client-token.dto.ts (1)

1-20: Harden validation and hide secrets in Swagger.

Constrain grantType and make clientSecret writeOnly in API docs.

-import { IsString } from 'class-validator';
+import { IsString, IsIn } from 'class-validator';
@@
   @ApiProperty()
   @IsString({ message: 'clientSecret must be in string format.' })
   clientSecret: string;
 
-  @ApiProperty()
-  @IsString({ message: 'grantType must be in string format.' })
-  grantType?: string = 'client_credentials';
+  @ApiProperty({ default: 'client_credentials' })
+  @IsString({ message: 'grantType must be in string format.' })
+  @IsIn(['client_credentials'], { message: 'grantType must be client_credentials.' })
+  grantType?: string = 'client_credentials';
}

Additionally, consider:

-  @ApiProperty()
+  @ApiProperty({ writeOnly: true })
   @IsString({ message: 'clientSecret must be in string format.' })
   clientSecret: string;
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 72bfb3e and ef12775.

📒 Files selected for processing (13)
  • apps/api-gateway/src/organization/dtos/client-token.dto.ts (1 hunks)
  • apps/api-gateway/src/organization/organization.controller.ts (2 hunks)
  • apps/api-gateway/src/organization/organization.service.ts (2 hunks)
  • apps/organization/dtos/client-token.dto.ts (1 hunks)
  • apps/organization/repositories/organization.repository.ts (1 hunks)
  • apps/organization/src/organization.controller.ts (16 hunks)
  • apps/organization/src/organization.service.ts (2 hunks)
  • libs/client-registration/src/client-registration.service.ts (2 hunks)
  • libs/client-registration/src/dtos/client-token.dto.ts (1 hunks)
  • libs/common/src/response-messages/index.ts (1 hunks)
  • libs/keycloak-url/src/keycloak-url.service.ts (1 hunks)
  • libs/prisma-service/prisma/migrations/20250925060521_added_app_launch_details_column_in_organization/migration.sql (1 hunks)
  • libs/prisma-service/prisma/schema.prisma (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
apps/api-gateway/src/organization/organization.controller.ts (3)
apps/api-gateway/src/organization/dtos/client-token.dto.ts (1)
  • ClientTokenDto (4-20)
apps/organization/dtos/client-token.dto.ts (1)
  • ClientTokenDto (1-6)
libs/client-registration/src/dtos/client-token.dto.ts (1)
  • ClientTokenDto (1-6)
apps/organization/dtos/client-token.dto.ts (2)
apps/api-gateway/src/organization/dtos/client-token.dto.ts (1)
  • ClientTokenDto (4-20)
libs/client-registration/src/dtos/client-token.dto.ts (1)
  • ClientTokenDto (1-6)
apps/api-gateway/src/organization/dtos/client-token.dto.ts (2)
apps/organization/dtos/client-token.dto.ts (1)
  • ClientTokenDto (1-6)
libs/client-registration/src/dtos/client-token.dto.ts (1)
  • ClientTokenDto (1-6)
libs/client-registration/src/dtos/client-token.dto.ts (1)
apps/api-gateway/src/organization/dtos/client-token.dto.ts (1)
  • ClientTokenDto (4-20)
libs/client-registration/src/client-registration.service.ts (1)
libs/client-registration/src/dtos/client-token.dto.ts (1)
  • ClientTokenDto (1-6)
apps/organization/src/organization.service.ts (2)
apps/organization/dtos/client-token.dto.ts (1)
  • ClientTokenDto (1-6)
libs/common/src/response-messages/index.ts (1)
  • ResponseMessages (1-520)
apps/organization/src/organization.controller.ts (1)
apps/organization/dtos/client-token.dto.ts (1)
  • ClientTokenDto (1-6)
apps/api-gateway/src/organization/organization.service.ts (3)
apps/api-gateway/src/organization/dtos/client-token.dto.ts (1)
  • ClientTokenDto (4-20)
apps/organization/dtos/client-token.dto.ts (1)
  • ClientTokenDto (1-6)
libs/client-registration/src/dtos/client-token.dto.ts (1)
  • ClientTokenDto (1-6)
🔇 Additional comments (20)
libs/prisma-service/prisma/schema.prisma (1)

136-136: Schema addition looks good.

Optional JSON column aligns with the repository changes and won’t disrupt existing rows.

libs/prisma-service/prisma/migrations/20250925060521_added_app_launch_details_column_in_organization/migration.sql (1)

1-2: Migration addition looks good.

The optional JSONB column is safe to add and matches the Prisma schema change.

apps/api-gateway/src/organization/organization.service.ts (2)

27-27: LGTM on DTO import.

Importing ClientTokenDto locally keeps gateway validation concerns isolated from shared libs.


242-244: Approve generateClientApiToken NATS subject and response shape Verified the NATS subject aligns with the org controller handler and the organization service returns { token: string } as expected.

apps/organization/src/organization.controller.ts (16)

27-27: LGTM on DTO import.


59-61: Doc-only change.


77-81: LGTM on signature update.

Type alignment to IUpdateOrganization is fine.


91-92: LGTM.

Param shape is clear and matches usage.


135-139: Doc-only change.


159-161: Doc-only change.


164-165: LGTM.

Signature and delegation look fine.


215-216: LGTM.

Clear param shape without HTTP-only decorators.


248-250: LGTM.


253-254: LGTM.


258-260: LGTM.


263-265: LGTM.


278-280: LGTM.


283-285: LGTM.


303-305: LGTM.


101-101: Remove @Body and (optionally) use aliased OrgPayload.

-  async getOrganizations(@Body() payload: { userId: string } & Payload): Promise<IGetOrganization> {
+  async getOrganizations(payload: { userId: string } & OrgPayload): Promise<IGetOrganization> {

Likely an incorrect or invalid review comment.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ef12775 and 2543ea0.

📒 Files selected for processing (2)
  • libs/client-registration/src/client-registration.service.ts (2 hunks)
  • libs/common/src/response-messages/index.ts (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
libs/client-registration/src/client-registration.service.ts (1)
libs/client-registration/src/dtos/client-token.dto.ts (1)
  • ClientTokenDto (1-6)
🔇 Additional comments (1)
libs/common/src/response-messages/index.ts (1)

133-135: New error messages align with token flows.

The added responses cover the admin/client token failure cases and follow existing naming conventions. Looks good.

@shitrerohit shitrerohit force-pushed the feat/generate-token-for-client branch from dc1c218 to 543f1d6 Compare September 29, 2025 06:27
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (1)
apps/organization/dtos/client-token.dto.ts (1)

1-6: Deduplicate ClientTokenDto and narrow types.

This DTO exists in multiple places (gateway and libs). Centralize it (e.g., export from libs/client-registration/src/dtos) to avoid drift. Also constrain grantType to the only supported literal and make it non‑optional.

 export class ClientTokenDto {
   orgId: string;
   clientId: string;
   clientSecret: string;
-  grantType?: string = 'client_credentials';
+  // Only client_credentials is supported; keep the type narrow and non-optional
+  grantType: 'client_credentials' = 'client_credentials';
 }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between dc1c218 and 543f1d6.

📒 Files selected for processing (13)
  • apps/api-gateway/src/organization/dtos/client-token.dto.ts (1 hunks)
  • apps/api-gateway/src/organization/organization.controller.ts (2 hunks)
  • apps/api-gateway/src/organization/organization.service.ts (2 hunks)
  • apps/organization/dtos/client-token.dto.ts (1 hunks)
  • apps/organization/repositories/organization.repository.ts (1 hunks)
  • apps/organization/src/organization.controller.ts (16 hunks)
  • apps/organization/src/organization.service.ts (2 hunks)
  • libs/client-registration/src/client-registration.service.ts (2 hunks)
  • libs/client-registration/src/dtos/client-token.dto.ts (1 hunks)
  • libs/common/src/response-messages/index.ts (1 hunks)
  • libs/keycloak-url/src/keycloak-url.service.ts (1 hunks)
  • libs/prisma-service/prisma/migrations/20250925060521_added_app_launch_details_column_in_organization/migration.sql (1 hunks)
  • libs/prisma-service/prisma/schema.prisma (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
  • libs/client-registration/src/client-registration.service.ts
  • apps/api-gateway/src/organization/organization.controller.ts
  • libs/prisma-service/prisma/schema.prisma
  • libs/client-registration/src/dtos/client-token.dto.ts
  • libs/prisma-service/prisma/migrations/20250925060521_added_app_launch_details_column_in_organization/migration.sql
  • apps/api-gateway/src/organization/organization.service.ts
  • libs/common/src/response-messages/index.ts
  • apps/organization/repositories/organization.repository.ts
  • libs/keycloak-url/src/keycloak-url.service.ts
  • apps/api-gateway/src/organization/dtos/client-token.dto.ts
  • apps/organization/src/organization.controller.ts
🧰 Additional context used
🧬 Code graph analysis (2)
apps/organization/dtos/client-token.dto.ts (2)
apps/api-gateway/src/organization/dtos/client-token.dto.ts (1)
  • ClientTokenDto (4-20)
libs/client-registration/src/dtos/client-token.dto.ts (1)
  • ClientTokenDto (1-6)
apps/organization/src/organization.service.ts (3)
apps/organization/dtos/client-token.dto.ts (1)
  • ClientTokenDto (1-6)
libs/common/src/response-messages/index.ts (1)
  • ResponseMessages (1-520)
libs/logger/src/logger.service.ts (1)
  • error (51-54)
🔇 Additional comments (1)
apps/organization/src/organization.service.ts (1)

68-68: Import looks correct. Used by the new method below.

Comment on lines 1 to 7
export class ClientTokenDto {
orgId: string;
clientId: string;
clientSecret: string;
grantType?: string = 'client_credentials';
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Do not accept admin clientSecret from external callers; fetch server‑side instead.

Having clientSecret in a request DTO is a major secret‑handling risk (leak via transit, logs, traces, and NATS payloads). Prefer reading admin credentials from secure config/secret store on the service side and remove clientSecret (and clientId if it refers to an admin SA) from the API surface. Return 400 if callers send them.

🤖 Prompt for AI Agents
In apps/organization/dtos/client-token.dto.ts around lines 1-6, the DTO
currently accepts clientSecret (and potentially admin clientId) from external
callers which is a secret-handling risk; remove clientSecret (and clientId if
it's an admin service account) from the DTO, ensure the service reads admin
credentials from secure config/secret store/server-side env, and update any
callers/validators to reject requests that include those fields with a 400
response; adjust related handlers to fetch the secret server-side and only
accept non-sensitive fields (e.g., orgId, grantType) in the API payload.

Comment on lines 2034 to 2072
async generateClientApiToken(generateTokenDetails: ClientTokenDto): Promise<{ token: string }> {
try {
const orgDetails = await this.organizationRepository.getOrganizationDetails(generateTokenDetails.orgId);
if (!orgDetails) {
throw new NotFoundException(ResponseMessages.organisation.error.orgNotFound);
}
// Step:1 generate the token using admin credentials
const adminTokenDetails =
await this.clientRegistrationService.generateTokenUsingAdminCredentials(generateTokenDetails);
if (!adminTokenDetails?.access_token) {
throw new InternalServerErrorException(ResponseMessages.organisation.error.adminTokenDetails);
}
// Step:2 Call API to fetch client id and secret
const clientDetails = await this.clientRegistrationService.fetchClientDetails(
generateTokenDetails.orgId,
adminTokenDetails.access_token
);
if (!clientDetails || 0 === clientDetails.length) {
throw new NotFoundException(ResponseMessages.organisation.error.clientDetails);
}
const { secret } = clientDetails[0];
//step3: generate the token using client id and secret
const authenticationResult = await this.authenticateClientKeycloak(generateTokenDetails.orgId, secret);
return { token: authenticationResult.access_token };
} catch (error) {
this.logger.error(`in generating issuer api token : ${JSON.stringify(error)}`);
throw new RpcException(error.response ? error.response : error);
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

🧩 Analysis chain

Harden secret handling and logs.

This path touches admin tokens and client secrets. Ensure downstream services never echo tokens/secrets into error objects, and avoid JSON.stringify(error) which can include sensitive fields. The minimal change above logs a safe summary. Consider adding a masking helper for consistency.

Run to spot other risky logs:


🏁 Script executed:

#!/bin/bash
# Find error logs that serialize entire error objects (risk of leaking tokens/secrets)
rg -nP -C1 "logger\.error\(\s*`|\blogger\.error\([^,]+,\s*JSON\.stringify" --type ts

Length of output: 68


🏁 Script executed:

#!/bin/bash
# Search for logger.error using template literals (risk of logging full error)
rg -nP -C1 'logger\.error\(\s*`' --type ts

# Search for logger.error calls that include JSON.stringify (risk of logging sensitive fields)
rg -nP -C1 'logger\.error\([^,]+,\s*JSON\.stringify' --type ts

Length of output: 167430


Centralize and sanitize error logging
Replace all direct JSON.stringify(error) and error interpolation in logger calls—including in apps/organization/src/organization.service.ts (line 2059) and the 300+ similar occurrences across the repo—with a shared helper (e.g. maskSensitiveError) that strips tokens/secrets before logging.

🤖 Prompt for AI Agents
In apps/organization/src/organization.service.ts around lines 2034 to 2062, the
catch block logs the raw error via JSON.stringify(error); replace this with a
call to a shared sanitizer (e.g. maskSensitiveError) that strips tokens, secrets
and other PII before logging. Import the shared helper from the centralized
utilities, call it to produce a sanitized string/obj (handle error as
string/object and preserve non-sensitive message/stack as needed), then pass
that sanitized output to this.logger.error and continue throwing the
RpcException as before. Ensure the helper is used throughout similar catch/log
sites so sensitive fields are consistently removed before any logging.

⚠️ Potential issue | 🔴 Critical

Bug: Using orgId as client_id when exchanging for token will fail for non‑identical IDs.

authenticateClientKeycloak(clientId, clientSecret) expects the Keycloak client_id, but the code passes generateTokenDetails.orgId instead of the org’s clientId. This will break token issuance whenever clientId !== orgId. Use the org’s clientId (from DB) or the clientDetails payload.

Apply:

   async generateClientApiToken(generateTokenDetails: ClientTokenDto): Promise<{ token: string }> {
     try {
       const orgDetails = await this.organizationRepository.getOrganizationDetails(generateTokenDetails.orgId);
       if (!orgDetails) {
         throw new NotFoundException(ResponseMessages.organisation.error.orgNotFound);
       }
       // Step:1 generate the token using admin credentials
       const adminTokenDetails =
         await this.clientRegistrationService.generateTokenUsingAdminCredentials(generateTokenDetails);
       if (!adminTokenDetails?.access_token) {
         throw new InternalServerErrorException(ResponseMessages.organisation.error.adminTokenDetails);
       }
       // Step:2 Call API to fetch client id and secret
       const clientDetails = await this.clientRegistrationService.fetchClientDetails(
         generateTokenDetails.orgId,
         adminTokenDetails.access_token
       );
       if (!clientDetails || 0 === clientDetails.length) {
         throw new NotFoundException(ResponseMessages.organisation.error.clientDetails);
       }
-      const { secret } = clientDetails[0];
-      //step3: generate the token using client id and secret
-      const authenticationResult = await this.authenticateClientKeycloak(generateTokenDetails.orgId, secret);
+      const { secret } = clientDetails[0];
+      // Prefer the persisted clientId, else fall back to returned details
+      const clientId = orgDetails.clientId ?? clientDetails[0]?.clientId;
+      if (!clientId) {
+        throw new NotFoundException('Client ID not found for organization');
+      }
+      // step3: generate the token using client id and secret
+      const authenticationResult = await this.authenticateClientKeycloak(clientId, secret);
       return { token: authenticationResult.access_token };
     } catch (error) {
-      this.logger.error(`in generating issuer api token : ${JSON.stringify(error)}`);
+      // Avoid logging sensitive token/secret data
+      this.logger.error('in generating issuer api token', { message: error?.message, status: error?.status });
       throw new RpcException(error.response ? error.response : error);
     }
   }
📝 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.

Suggested change
async generateClientApiToken(generateTokenDetails: ClientTokenDto): Promise<{ token: string }> {
try {
const orgDetails = await this.organizationRepository.getOrganizationDetails(generateTokenDetails.orgId);
if (!orgDetails) {
throw new NotFoundException(ResponseMessages.organisation.error.orgNotFound);
}
// Step:1 generate the token using admin credentials
const adminTokenDetails =
await this.clientRegistrationService.generateTokenUsingAdminCredentials(generateTokenDetails);
if (!adminTokenDetails?.access_token) {
throw new InternalServerErrorException(ResponseMessages.organisation.error.adminTokenDetails);
}
// Step:2 Call API to fetch client id and secret
const clientDetails = await this.clientRegistrationService.fetchClientDetails(
generateTokenDetails.orgId,
adminTokenDetails.access_token
);
if (!clientDetails || 0 === clientDetails.length) {
throw new NotFoundException(ResponseMessages.organisation.error.clientDetails);
}
const { secret } = clientDetails[0];
//step3: generate the token using client id and secret
const authenticationResult = await this.authenticateClientKeycloak(generateTokenDetails.orgId, secret);
return { token: authenticationResult.access_token };
} catch (error) {
this.logger.error(`in generating issuer api token : ${JSON.stringify(error)}`);
throw new RpcException(error.response ? error.response : error);
}
}
async generateClientApiToken(generateTokenDetails: ClientTokenDto): Promise<{ token: string }> {
try {
const orgDetails = await this.organizationRepository.getOrganizationDetails(generateTokenDetails.orgId);
if (!orgDetails) {
throw new NotFoundException(ResponseMessages.organisation.error.orgNotFound);
}
// Step:1 generate the token using admin credentials
const adminTokenDetails =
await this.clientRegistrationService.generateTokenUsingAdminCredentials(generateTokenDetails);
if (!adminTokenDetails?.access_token) {
throw new InternalServerErrorException(ResponseMessages.organisation.error.adminTokenDetails);
}
// Step:2 Call API to fetch client id and secret
const clientDetails = await this.clientRegistrationService.fetchClientDetails(
generateTokenDetails.orgId,
adminTokenDetails.access_token
);
if (!clientDetails || 0 === clientDetails.length) {
throw new NotFoundException(ResponseMessages.organisation.error.clientDetails);
}
const { secret } = clientDetails[0];
// Prefer the persisted clientId, else fall back to returned details
const clientId = orgDetails.clientId ?? clientDetails[0]?.clientId;
if (!clientId) {
throw new NotFoundException('Client ID not found for organization');
}
// step3: generate the token using client id and secret
const authenticationResult = await this.authenticateClientKeycloak(clientId, secret);
return { token: authenticationResult.access_token };
} catch (error) {
// Avoid logging sensitive token/secret data
this.logger.error('in generating issuer api token', { message: error?.message, status: error?.status });
throw new RpcException(error.response ? error.response : error);
}
}
🤖 Prompt for AI Agents
In apps/organization/src/organization.service.ts around lines 2034 to 2062, the
code passes generateTokenDetails.orgId into authenticateClientKeycloak but that
function expects the Keycloak client_id; replace the orgId with the actual
clientId obtained from the clientDetails (or orgDetails if stored there).
Concretely, after fetching clientDetails use the client id field (e.g.,
clientDetails[0].clientId or the exact property name returned by the API) as the
first argument to authenticateClientKeycloak and keep using the secret for the
second argument, then return authenticationResult.access_token as before.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 543f1d6 and 24e9c34.

📒 Files selected for processing (4)
  • apps/api-gateway/src/organization/dtos/client-token.dto.ts (1 hunks)
  • apps/organization/dtos/client-token.dto.ts (1 hunks)
  • apps/organization/src/organization.service.ts (2 hunks)
  • libs/common/src/response-messages/index.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • libs/common/src/response-messages/index.ts
🧰 Additional context used
🧬 Code graph analysis (3)
apps/organization/src/organization.service.ts (2)
apps/organization/dtos/client-token.dto.ts (1)
  • ClientTokenDto (1-7)
libs/common/src/response-messages/index.ts (1)
  • ResponseMessages (1-521)
apps/organization/dtos/client-token.dto.ts (2)
apps/api-gateway/src/organization/dtos/client-token.dto.ts (1)
  • ClientTokenDto (4-24)
libs/client-registration/src/dtos/client-token.dto.ts (1)
  • ClientTokenDto (1-6)
apps/api-gateway/src/organization/dtos/client-token.dto.ts (2)
apps/organization/dtos/client-token.dto.ts (1)
  • ClientTokenDto (1-7)
libs/client-registration/src/dtos/client-token.dto.ts (1)
  • ClientTokenDto (1-6)
🔇 Additional comments (2)
apps/organization/src/organization.service.ts (1)

2064-2067: Use the actual Keycloak client ID when exchanging for a token.
authenticateClientKeycloak expects the Keycloak client_id, but we pass generateTokenDetails.orgId. This only succeeds if orgId === clientId; otherwise token issuance breaks. Use the client ID returned from fetchClientDetails (or orgDetails.clientId) instead.

Apply:

-      const { secret } = clientDetails[0];
-      //step3: generate the token using client id and secret
-      const authenticationResult = await this.authenticateClientKeycloak(generateTokenDetails.orgId, secret);
+      const { clientId: fetchedClientId, secret } = clientDetails[0];
+      const clientId = orgDetails.clientId ?? fetchedClientId;
+      if (!clientId) {
+        throw new NotFoundException(ResponseMessages.organisation.error.clientDetails);
+      }
+      const authenticationResult = await this.authenticateClientKeycloak(clientId, secret);
apps/organization/dtos/client-token.dto.ts (1)

1-6: Eliminate clientSecret/clientId from the organization DTO.
Keeping these fields means service-to-service calls still move admin credentials through untrusted payloads, violating secret-handling policy. Fetch the admin client config internally and reject requests carrying these properties.

Comment on lines +9 to +19
@ApiProperty()
@IsString({ message: 'clientAlias must be in string format.' })
clientAlias: string;

@ApiProperty()
@IsString({ message: 'clientId must be in string format.' })
clientId: string;

@ApiProperty()
@IsString({ message: 'clientSecret must be in string format.' })
clientSecret: string;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Remove secret-bearing fields from the gateway DTO.
This DTO exposes clientId and clientSecret to external callers, meaning admin credentials now traverse user payloads, logs, traces, and NATS messages. That is a critical secret-handling flaw—attackers can supply arbitrary secrets or exfiltrate the real ones. Fetch the admin client credentials server-side (e.g., by clientAlias from a secure store) and drop these fields from the public contract. Return a 400 if clients include them.

🤖 Prompt for AI Agents
In apps/api-gateway/src/organization/dtos/client-token.dto.ts around lines 9 to
19, the DTO currently exposes clientId and clientSecret which leaks admin
credentials; remove clientId and clientSecret properties from this public DTO
and leave only clientAlias, then update validation to reject requests that
include clientId or clientSecret (respond with 400) so callers cannot supply
secrets; ensure server-side logic fetches the admin client credentials from the
secure store using clientAlias instead of relying on values from the incoming
payload and update any downstream handlers to use the fetched credentials.

Comment on lines 2051 to 2058
const adminTokenDetails =
await this.clientRegistrationService.generateTokenUsingAdminCredentials(generateTokenDetails);
if (!adminTokenDetails?.access_token) {
throw new InternalServerErrorException(ResponseMessages.organisation.error.adminTokenDetails);
}
// Step:2 Call API to fetch client id and secret
const clientDetails = await this.clientRegistrationService.fetchClientDetails(
generateTokenDetails.orgId,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Do not trust caller-supplied admin credentials.
generateTokenUsingAdminCredentials consumes the DTO’s clientId/clientSecret, so the admin secret now comes from the HTTP request. Any caller can inject bogus secrets or harvest real ones, defeating privileged separation. Resolve the admin credentials server-side (based on clientAlias or org metadata) and stop accepting/forwarding them from user input; reject requests that try.

🤖 Prompt for AI Agents
In apps/organization/src/organization.service.ts around lines 2051-2058, the
code currently uses admin credentials supplied in the incoming DTO to call
generateTokenUsingAdminCredentials which trusts caller-provided
clientId/clientSecret; instead, reject any incoming admin credential fields and
resolve admin credentials server-side (look up the org by
generateTokenDetails.clientAlias or orgId, retrieve the authoritative
clientId/clientSecret from secure storage/config or a server-only mapping), then
call generateTokenUsingAdminCredentials with those server-sourced credentials;
also update DTO validation to forbid clientId/clientSecret from requests and
throw a BadRequestException if they are present so callers cannot inject or
harvest admin secrets.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
libs/client-registration/src/client-registration.service.ts (2)

67-74: createdTimestamp is NaN due to Date.parse(Date.now.toString())

Date.now is a function; parsing its function-string returns NaN. Use Date.now().

-      createdTimestamp: Date.parse(Date.now.toString()),
+      createdTimestamp: Date.now(),

39-47: Unsafe array indexing; wrong ID returned when second element matches

Accessing [1] without length check can throw, and returning [0].id even if [1] matches is incorrect. Find the matching user safely.

-      const getUserResponse = await this.commonService.httpGet(
+      const getUserResponse = await this.commonService.httpGet(
         await this.keycloakUrlService.getUserByUsernameURL(realm, userDetails.email),
         this.getAuthHeader(token)
       );
-      if (getUserResponse[0].username === userDetails.email || getUserResponse[1].username === userDetails.email) {
-        return { keycloakUserId: getUserResponse[0].id };
-      } else {
-        throw new NotFoundException(ResponseMessages.user.error.invalidKeycloakId);
-      }
+      const match = Array.isArray(getUserResponse)
+        ? getUserResponse.find((u) => u?.username === userDetails.email)
+        : null;
+      if (match?.id) {
+        return { keycloakUserId: match.id };
+      }
+      throw new NotFoundException(ResponseMessages.user.error.invalidKeycloakId);
🧹 Nitpick comments (2)
libs/client-registration/src/client-registration.service.ts (2)

739-745: Avoid duplicating token-posting logic; reuse getToken()

Reuse the existing getToken() to reduce duplication and keep SAT call behavior consistent.

-    const generatedTokenDetails = await this.commonService.httpPost(
-      await this.keycloakUrlService.GetSATURL(realmName),
-      qs.stringify(payload),
-      config
-    );
-    return generatedTokenDetails;
+    const payloadDto = Object.assign(new ClientCredentialTokenPayloadDto(), payload);
+    return this.getToken(payloadDto);

7-8: Pin qs to a secure recent 6.x in package.json

You’re using qs.stringify here; ensure dependency is pinned to a current 6.x (e.g., ^6.14.0) to pick up security hardening.

Based on learnings

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 24e9c34 and 1771d41.

📒 Files selected for processing (1)
  • libs/client-registration/src/client-registration.service.ts (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
libs/client-registration/src/client-registration.service.ts (1)
libs/client-registration/src/dtos/client-token.dto.ts (1)
  • ClientTokenDto (1-6)
🔇 Additional comments (3)
libs/client-registration/src/client-registration.service.ts (3)

9-10: LGTM on import additions.

Imports look correct and used.

Also applies to: 12-12, 16-17, 22-23


179-183: Stop logging secrets, tokens, and PII (several places)

Multiple logs output client_secret, token responses, or user data. Remove or mask. This is a compliance and security risk.

Apply:

@@
-    this.logger.log(`management Payload: ${JSON.stringify(payload)}`);
@@
-    this.logger.debug(`ClientRegistrationService management token ${JSON.stringify(mgmtTokenResponse)}`);
+    this.logger.log('Management token retrieved.');
@@
-    this.logger.debug(
-      `ClientRegistrationService create realm client secret ${JSON.stringify(createClientSercretResponse)}`
-    );
@@
-    this.logger.debug(`ClientRegistrationService get client secret ${JSON.stringify(getClientSercretResponse)}`);
-    this.logger.log(`${getClientSercretResponse.value}`);
+    this.logger.log('Client secret generated.');
@@
-    this.logger.debug(
-      `ClientRegistrationService get realm admin client secret ${JSON.stringify(getClientSercretResponse)}`
-    );
-    this.logger.log(`${getClientSercretResponse.value}`);
+    this.logger.log('Admin client secret fetched.');
@@
-    this.logger.debug(`ClientRegistrationService create connection app ${JSON.stringify(clientConnResponse)}`);
+    this.logger.log('Connection created.');
@@
-    this.logger.log(`access Token for holderPayload: ${JSON.stringify(payload)}`);
+    // payload contains client_secret; do not log
@@
-    this.logger.debug(`ClientRegistrationService token ${JSON.stringify(tokenResponse)}`);
+    // tokenResponse contains tokens; do not log
@@
-    this.logger.debug(`redirectUrls ${JSON.stringify(redirectUrls)}`);
+    // avoid logging redirect URLs verbatim (may contain tenant data)
@@
-    this.logger.debug(`userInfo ${JSON.stringify(userInfo)}`);
+    // avoid logging full user info (PII)
#!/bin/bash
# Sanity-check for any remaining sensitive logs in this file.
rg -n -C1 --type=ts "logger\.(debug|log|error)\(" libs/client-registration/src/client-registration.service.ts \
  | rg -n "(secret|token|password|userInfo|payload)"

Also applies to: 345-356, 421-426, 569-575, 671-671, 696-697, 709-709, 722-722


747-755: No sensitive client details are returned: The only call to fetchClientDetails in organization.service.ts extracts just the secret for internal token generation and returns { token }. Raw client metadata is never forwarded to end users.

Comment on lines +727 to +736
async generateTokenUsingAdminCredentials(clientDetails: ClientTokenDto) {
const realmName = process.env.KEYCLOAK_REALM;
const payload = {
client_id: clientDetails.clientId,
client_secret: clientDetails.clientSecret,
grant_type: clientDetails.grantType ?? 'client_credentials'
};
const config = {
headers: {
'Content-Type': 'application/x-www-form-urlencoded'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

🧩 Analysis chain

Validate required inputs in generateTokenUsingAdminCredentials

Guard against missing clientId/clientSecret to avoid 400s from Keycloak and confusing errors.

Apply:

   async generateTokenUsingAdminCredentials(clientDetails: ClientTokenDto) {
     const realmName = process.env.KEYCLOAK_REALM;
+    if (!clientDetails?.clientId || !clientDetails?.clientSecret) {
+      throw new BadRequestException('clientId and clientSecret are required');
+    }
     const payload = {
       client_id: clientDetails.clientId,
       client_secret: clientDetails.clientSecret,
       grant_type: clientDetails.grantType ?? 'client_credentials'
     };

If admin creds can arrive encrypted (as elsewhere in this service), confirm whether decryption is required here.


🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

# Locate the target method with surrounding context
rg -n 'generateTokenUsingAdminCredentials' -C5 libs/client-registration/src/client-registration.service.ts

# Search for decryption calls within that file
rg -n 'decrypt' -C3 libs/client-registration/src/client-registration.service.ts

# Search for any CryptoService usage across the repo
rg -n 'CryptoService' -C3 .

# Confirm where the method is invoked and if arguments are pre-processed
rg -n 'generateTokenUsingAdminCredentials' -C2 .

Length of output: 2330


🏁 Script executed:

rg -n 'generateTokenUsingAdminCredentials' -C1 .

Length of output: 667


🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

# Show context where generateTokenDetails is defined and passed
rg -n 'generateTokenDetails' -C10 apps/organization/src/organization.service.ts

Length of output: 2656


🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

# Find definition of ClientTokenDto
rg -n "export.*ClientTokenDto" -C3 .

Length of output: 1263


Validate and decrypt admin credentials in generateTokenUsingAdminCredentials

Add input checks and decrypt both clientId and clientSecret before building the payload:

 libs/client-registration/src/client-registration.service.ts
 async generateTokenUsingAdminCredentials(clientDetails: ClientTokenDto) {
   const realmName = process.env.KEYCLOAK_REALM;
+  if (!clientDetails?.clientId || !clientDetails?.clientSecret) {
+    throw new BadRequestException('clientId and clientSecret are required');
+  }
+  const decryptClientId = await this.commonService.decryptPassword(clientDetails.clientId);
+  const decryptClientSecret = await this.commonService.decryptPassword(clientDetails.clientSecret);
   const payload = {
-    client_id: clientDetails.clientId,
-    client_secret: clientDetails.clientSecret,
+    client_id: decryptClientId,
+    client_secret: decryptClientSecret,
     grant_type: clientDetails.grantType ?? 'client_credentials'
   };
   const config = {
     headers: {
       'Content-Type': 'application/x-www-form-urlencoded'

This mirrors other token methods’ use of decryptPassword and guards against missing inputs.

📝 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.

Suggested change
async generateTokenUsingAdminCredentials(clientDetails: ClientTokenDto) {
const realmName = process.env.KEYCLOAK_REALM;
const payload = {
client_id: clientDetails.clientId,
client_secret: clientDetails.clientSecret,
grant_type: clientDetails.grantType ?? 'client_credentials'
};
const config = {
headers: {
'Content-Type': 'application/x-www-form-urlencoded'
async generateTokenUsingAdminCredentials(clientDetails: ClientTokenDto) {
const realmName = process.env.KEYCLOAK_REALM;
if (!clientDetails?.clientId || !clientDetails?.clientSecret) {
throw new BadRequestException('clientId and clientSecret are required');
}
const decryptClientId = await this.commonService.decryptPassword(clientDetails.clientId);
const decryptClientSecret = await this.commonService.decryptPassword(clientDetails.clientSecret);
const payload = {
client_id: decryptClientId,
client_secret: decryptClientSecret,
grant_type: clientDetails.grantType ?? 'client_credentials'
};
const config = {
headers: {
'Content-Type': 'application/x-www-form-urlencoded'

@shitrerohit shitrerohit force-pushed the feat/generate-token-for-client branch from 1771d41 to b1ef5f6 Compare September 29, 2025 14:34
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
apps/organization/src/organization.controller.ts (2)

187-195: Remove @Body() in message handler for fetchUserInvitation.

Switch to plain param or @payload() to receive the broker payload.

-async fetchUserInvitation(
-  @Body() payload: { email: string; status: string } & Payload
-): Promise<IOrganizationInvitations> {
+async fetchUserInvitation(payload: { email: string; status: string } & Payload): Promise<IOrganizationInvitations> {

203-206: Remove @Body() in update-invitation-status handler.

Use plain param or @payload() for microservices handlers.

-async updateOrgInvitation(@Body() payload: UpdateInvitationDto): Promise<string> {
+async updateOrgInvitation(payload: UpdateInvitationDto): Promise<string> {
🧹 Nitpick comments (5)
.husky/pre-push (2)

13-15: Prep for author-scoped signing: surface CURRENT_EMAIL via env with sensible default.

To support “sign only my commits” flows, make CURRENT_EMAIL available (env-overridable):

-# CURRENT_EMAIL=$(git config user.email)
+CURRENT_EMAIL=${CURRENT_EMAIL:-$(git config user.email)}

This enables optional author filtering (see next comment) while preserving today’s behavior. Based on learnings.


35-41: Optional: gate auto‑sign to your own commits (avoid rewriting co‑authored ranges).

Enable the commented block behind an env toggle so teams can opt in without changing defaults:

-# author_email=$(git log -1 --format='%ae' $commit)
-# if [ "$author_email" != "$CURRENT_EMAIL" ]; then
-#   echo "${CYAN}ℹ️  Skipping commit $commit authored by $author_email${RESET}"
-#   continue
-# fi
+if [ "${SIGN_SCOPE:-all}" = "mine" ]; then
+  author_email=$(git log -1 --format='%ae' "$commit")
+  if [ "$author_email" != "$CURRENT_EMAIL" ]; then
+    echo "${CYAN}ℹ️  Skipping commit $commit authored by $author_email${RESET}"
+    continue
+  fi
+fi

Usage:

  • Default: SIGN_SCOPE=all (current behavior).
  • Opt-in: SIGN_SCOPE=mine to only auto‑sign your commits.

This aligns with the team preference to allow auto‑rebase for signing personal local commits. Based on learnings.

libs/prisma-service/prisma/schema.prisma (1)

136-136: Add JSONB index if you’ll filter by keys; also define a TS shape at the boundary.

  • If you query inside appLaunchDetails (e.g., details->>'foo'), add a GIN index to avoid seq scans.
  • Expose a typed shape at service/DTO boundary (e.g., zod/TS interface) to prevent arbitrary JSON drift.
apps/organization/src/organization.controller.ts (1)

268-270: Typo: rename method to clientLoginCredentials.

Fix spelling to avoid confusion in references and logs.

-async clientLoginCredentails(payload: { clientId: string; clientSecret: string }): Promise<IAccessTokenData> {
+async clientLoginCredentials(payload: { clientId: string; clientSecret: string }): Promise<IAccessTokenData> {
apps/api-gateway/src/organization/dtos/client-token.dto.ts (1)

21-23: Narrow grantType to literal and validate.

Only 'client_credentials' is supported; make it a literal and enforce via validation.

-  @ApiProperty()
-  @IsString({ message: 'grantType must be in string format.' })
-  grantType?: string = 'client_credentials';
+  @ApiProperty({ enum: ['client_credentials'], default: 'client_credentials' })
+  @IsIn(['client_credentials'])
+  grantType?: 'client_credentials' = 'client_credentials';
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1771d41 and b1ef5f6.

📒 Files selected for processing (14)
  • .husky/pre-push (4 hunks)
  • apps/api-gateway/src/organization/dtos/client-token.dto.ts (1 hunks)
  • apps/api-gateway/src/organization/organization.controller.ts (2 hunks)
  • apps/api-gateway/src/organization/organization.service.ts (2 hunks)
  • apps/organization/dtos/client-token.dto.ts (1 hunks)
  • apps/organization/repositories/organization.repository.ts (1 hunks)
  • apps/organization/src/organization.controller.ts (16 hunks)
  • apps/organization/src/organization.service.ts (2 hunks)
  • libs/client-registration/src/client-registration.service.ts (2 hunks)
  • libs/client-registration/src/dtos/client-token.dto.ts (1 hunks)
  • libs/common/src/response-messages/index.ts (1 hunks)
  • libs/keycloak-url/src/keycloak-url.service.ts (1 hunks)
  • libs/prisma-service/prisma/migrations/20250925060521_added_app_launch_details_column_in_organization/migration.sql (1 hunks)
  • libs/prisma-service/prisma/schema.prisma (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
  • libs/prisma-service/prisma/migrations/20250925060521_added_app_launch_details_column_in_organization/migration.sql
  • libs/client-registration/src/client-registration.service.ts
  • apps/api-gateway/src/organization/organization.service.ts
  • apps/organization/dtos/client-token.dto.ts
  • apps/api-gateway/src/organization/organization.controller.ts
  • libs/common/src/response-messages/index.ts
  • apps/organization/repositories/organization.repository.ts
  • apps/organization/src/organization.service.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-29T12:14:52.466Z
Learnt from: GHkrishna
PR: credebl/platform#1468
File: .husky/pre-push:47-56
Timestamp: 2025-09-29T12:14:52.466Z
Learning: User GHkrishna prefers allowing automatic rebase in pre-push hooks for signing commits, considering it acceptable since they're rebasing their own local commits before sharing.

Applied to files:

  • .husky/pre-push
🧬 Code graph analysis (3)
libs/client-registration/src/dtos/client-token.dto.ts (2)
apps/api-gateway/src/organization/dtos/client-token.dto.ts (1)
  • ClientTokenDto (4-24)
apps/organization/dtos/client-token.dto.ts (1)
  • ClientTokenDto (1-7)
apps/organization/src/organization.controller.ts (1)
apps/organization/dtos/client-token.dto.ts (1)
  • ClientTokenDto (1-7)
apps/api-gateway/src/organization/dtos/client-token.dto.ts (2)
apps/organization/dtos/client-token.dto.ts (1)
  • ClientTokenDto (1-7)
libs/client-registration/src/dtos/client-token.dto.ts (1)
  • ClientTokenDto (1-6)
🔇 Additional comments (10)
libs/keycloak-url/src/keycloak-url.service.ts (1)

7-74: Keep URL helpers synchronous to avoid [object Promise] URLs.

Every helper is marked async, so each call now returns Promise<string>. Most callers still treat these as plain strings, and at runtime we concatenate [object Promise] into the request URL—this breaks the token generation flow exactly as flagged earlier. Please drop the async keyword (and the implicit promise) across these helpers so they once again return string. (keycloak.org)

Apply the following pattern to each helper:

-  async createUserURL(realm: string): Promise<string> {
-    return `${process.env.KEYCLOAK_DOMAIN}admin/realms/${realm}/users`;
-  }
+  createUserURL(realm: string): string {
+    return `${process.env.KEYCLOAK_DOMAIN}admin/realms/${realm}/users`;
+  }

Do the same for every other URL builder in this service.

apps/organization/src/organization.controller.ts (8)

77-81: LGTM: typed payload for update-organization.

Clear shape and consistent delegation.


91-93: LGTM: microservice-friendly payload.

No HTTP decorators; returns DID list from service.


164-166: LGTM: get-org-roles signature.

Simple pass-through with explicit types.


215-217: LGTM: update-user-roles.

Typed args; minimal controller logic.


248-250: LGTM: get-organization-details.


253-255: LGTM: delete-organization.


258-260: LGTM: delete-org-client-credentials.


263-265: LGTM: delete-organization-invitation.

apps/api-gateway/src/organization/dtos/client-token.dto.ts (1)

13-20: Remove secret-bearing fields from public DTO; fetch credentials server-side.

Do not accept clientId/clientSecret from external callers. Keep only orgId and clientAlias; resolve secrets internally.

 export class ClientTokenDto {
   @ApiProperty()
   @IsString({ message: 'orgId must be in string format.' })
   orgId: string;

   @ApiProperty()
   @IsString({ message: 'clientAlias must be in string format.' })
   clientAlias: string;

-  @ApiProperty()
-  @IsString({ message: 'clientId must be in string format.' })
-  clientId: string;
-
-  @ApiProperty()
-  @IsString({ message: 'clientSecret must be in string format.' })
-  clientSecret: string;

.husky/pre-push Outdated
Comment on lines 42 to 45
# Check signature
sig_status=$(git log --format='%G?' -n 1 $commit)
if [ "$sig_status" = "N" ]; then
unsigned_commits="$unsigned_commits $commit"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

🧩 Analysis chain

Only flagging 'N' (no signature) now; adjust end message and optionally warn on unverified signatures.

Checking for %G? == "N" correctly targets truly unsigned commits. Note that statuses like E (cannot be checked), B/U/X/Y/R mean the commit is signed but not necessarily verified; these will now pass your gate. Consider:

  • Update the final success message to avoid “verified” wording.
  • Optionally, collect and warn (don’t block) when encountering E/B/U/X/Y/R so developers know to fix key/allowedSigners config.

Quick messaging fix:

-echo "${GREEN}✅ All commits are signed and verified${RESET}"
+echo "${GREEN}✅ No unsigned commits found${RESET}"

Optional warning pattern:

# near line 24
unverified_commits=""

# replace lines 42-45 with a case
sig_status=$(git log --format='%G?' -n 1 "$commit")
case "$sig_status" in
  N) unsigned_commits="$unsigned_commits $commit" ;;
  E|B|U|X|Y|R) unverified_commits="$unverified_commits $commit" ;;
esac

…and after the main unsigned flow, add:

[ -n "$unverified_commits" ] && echo "${YELLOW}⚠️  Signed but not verified commits:${RESET} $unverified_commits"

Reference for %G? meanings: Git pretty-formats docs.

Run to see statuses in your current push range:


🏁 Script executed:

#!/bin/bash
set -euo pipefail
branch=$(git rev-parse --abbrev-ref HEAD)
if upstream=$(git rev-parse --abbrev-ref --symbolic-full-name @{u} 2>/dev/null); then
  range="$upstream..HEAD"
else
  range="HEAD"
fi
echo "Range: $range"
git log --no-merges --pretty='%h %G? %s' $range | awk '
BEGIN{ok=0;uns=0;unv=0}
{ if ($2=="G") ok++; else if ($2=="N") uns++; else unv++; print }
END{ printf("\nG(good)=%d  N(unsigned)=%d  other(signed-but-not-verified)=%d\n", ok, uns, unv) }'

Length of output: 146603


Add a preliminary gpg availability check and update the success message

  • At the top of the hook, bail out with a clear warning if gpg isn’t installed:
    + if ! command -v gpg >/dev/null; then
    +   echo "${YELLOW}⚠️  gpg not found; skipping commit signature checks${RESET}"
    +   exit 0
    + fi
  • Replace the final “signed and verified” echo to avoid misleading wording:
    - echo "${GREEN}✅ All commits are signed and verified${RESET}"
    + echo "${GREEN}✅ No unsigned commits found${RESET}"
🤖 Prompt for AI Agents
In .husky/pre-push around lines 42-45, the hook assumes gpg is present and
prints a misleading "signed and verified" success message; add an early check
that aborts with a clear warning if the gpg binary is missing (e.g., test for
command -v gpg) and exit non-zero so the hook doesn't continue, and update the
final success echo to avoid "signed and verified" (use a phrasing like "All
commits have valid signatures" or "No unsigned commits found") to accurately
reflect the verification result.

Comment on lines +101 to 104
async getOrganizations(@Body() payload: { userId: string } & Payload): Promise<IGetOrganization> {
const { userId, pageNumber, pageSize, search, role } = payload;
return this.organizationService.getOrganizations(userId, pageNumber, pageSize, search, role);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Remove @Body() from @MessagePattern handler.

@Body is HTTP-only; with NATS it yields undefined. Use plain param or @payload() from microservices.

-async getOrganizations(@Body() payload: { userId: string } & Payload): Promise<IGetOrganization> {
+async getOrganizations(payload: { userId: string } & Payload): Promise<IGetOrganization> {
📝 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.

Suggested change
async getOrganizations(@Body() payload: { userId: string } & Payload): Promise<IGetOrganization> {
const { userId, pageNumber, pageSize, search, role } = payload;
return this.organizationService.getOrganizations(userId, pageNumber, pageSize, search, role);
}
async getOrganizations(payload: { userId: string } & Payload): Promise<IGetOrganization> {
const { userId, pageNumber, pageSize, search, role } = payload;
return this.organizationService.getOrganizations(userId, pageNumber, pageSize, search, role);
}
🤖 Prompt for AI Agents
In apps/organization/src/organization.controller.ts around lines 101 to 104, the
MessagePattern handler currently uses the HTTP-only @Body() decorator which
yields undefined for NATS; remove @Body() and accept the payload as a plain
parameter or annotate it with @Payload() from @nestjs/microservices, keep the
existing type ({ userId: string } & Payload | IGetOrganization as appropriate),
and update imports to include @Payload if used; ensure the method signature and
service call stay the same but without @Body().

Comment on lines +307 to +310
@MessagePattern({ cmd: 'generate-client-api-token' })
async generateClientApiToken(payload: ClientTokenDto): Promise<{ token: string }> {
return this.organizationService.generateClientApiToken(payload);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Do not accept or forward admin secrets; use alias-only request and fetch creds server-side.

The DTO currently includes clientId/clientSecret (via import). Secrets should not traverse the message bus. Accept orgId + clientAlias only; resolve clientId/secret inside the organization service.

-@MessagePattern({ cmd: 'generate-client-api-token' })
-async generateClientApiToken(payload: ClientTokenDto): Promise<{ token: string }> {
-  return this.organizationService.generateClientApiToken(payload);
-}
+@MessagePattern({ cmd: 'generate-client-api-token' })
+async generateClientApiToken(payload: { orgId: string; clientAlias: string; grantType?: 'client_credentials' }): Promise<{ token: string }> {
+  return this.organizationService.generateClientApiToken(payload);
+}

🏁 Script executed:

#!/bin/bash
# Ensure no @Body() remains in @MessagePattern handlers
rg -n "@MessagePattern" -n -C1 apps/organization/src/organization.controller.ts | rg -n "@Body\\("

Length of output: 712


🏁 Script executed:

#!/bin/bash
# Display context around generateClientApiToken in controller
sed -n '250,330p' apps/organization/src/organization.controller.ts
# Locate ClientTokenDto definition and inspect fields
rg -n "export class ClientTokenDto" -g "apps/organization/src/**/*.dto.ts" -C3

Length of output: 2865


🏁 Script executed:

#!/bin/bash
# Locate ClientTokenDto definition in repository
rg -n "class ClientTokenDto" -C5 .
rg -n "interface ClientTokenDto" -C5 .

Length of output: 1733


Remove client secrets from the message bus and strip @Body() decorators

  • The ClientTokenDto (apps/organization/dtos/client-token.dto.ts) still includes clientId and clientSecret, which exposes admin credentials over the message bus. Update the handler signature to accept only { orgId: string; clientAlias: string; grantType?: 'client_credentials' } and resolve the actual client ID/secret inside organizationService.generateClientApiToken.
@MessagePattern({ cmd: 'generate-client-api-token' })
-async generateClientApiToken(payload: ClientTokenDto): Promise<{ token: string }> {
+async generateClientApiToken(payload: { orgId: string; clientAlias: string; grantType?: 'client_credentials' }): Promise<{ token: string }> {
   return this.organizationService.generateClientApiToken(payload);
}
  • The verification script shows many other @MessagePattern handlers still using @Body() (e.g., setPrimaryDid, getOrganizations, etc.). Message handlers should accept raw payload parameters; remove all @Body() decorators.
📝 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.

Suggested change
@MessagePattern({ cmd: 'generate-client-api-token' })
async generateClientApiToken(payload: ClientTokenDto): Promise<{ token: string }> {
return this.organizationService.generateClientApiToken(payload);
}
@MessagePattern({ cmd: 'generate-client-api-token' })
async generateClientApiToken(
payload: { orgId: string; clientAlias: string; grantType?: 'client_credentials' }
): Promise<{ token: string }> {
return this.organizationService.generateClientApiToken(payload);
}

Comment on lines +1 to +6
export class ClientTokenDto {
orgId: string;
clientId: string;
clientSecret: string;
grantType?: string = 'client_credentials';
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

🧩 Analysis chain

Unify DTO and drop secrets from the request contract.

Adopt a single shared DTO in libs that only carries orgId, clientAlias, grantType. Fetch clientId/clientSecret server-side from a secure store. Avoid three divergent ClientTokenDto shapes across modules.

Apply in libs (then update callers):

-export class ClientTokenDto {
-  orgId: string;
-  clientId: string;
-  clientSecret: string;
-  grantType?: string = 'client_credentials';
-}
+export class ClientTokenDto {
+  orgId: string;
+  clientAlias: string;
+  grantType?: 'client_credentials' = 'client_credentials';
+}
+
+// If needed internally only (never exposed externally):
+export class InternalClientCredentialsDto {
+  orgId: string;
+  clientId: string;
+  clientSecret: string;
+  grantType?: 'client_credentials' = 'client_credentials';
+}

Run to find all usages to update:


🏁 Script executed:

#!/bin/bash
rg -n --pretty "\bClientTokenDto\b|clientAlias|clientSecret|clientId" -g '!**/dist/**'

Length of output: 148


Re-run the search with a positive scope:


🏁 Script executed:

#!/bin/bash
rg -n --pretty '\b(ClientTokenDto|clientAlias|clientSecret|clientId)\b' libs

Length of output: 5811


Consolidate ClientTokenDto and restrict external contract

  • Public ClientTokenDto should only expose { orgId, clientAlias, grantType? }
  • Introduce InternalClientCredentialsDto for { orgId, clientId, clientSecret, grantType? } used server-side
  • Update all callsites in libs/client-registration, libs/common, libs/keycloak-url and libs/prisma-service to fetch secrets from a secure store instead of passing them in requests
🤖 Prompt for AI Agents
In libs/client-registration/src/dtos/client-token.dto.ts lines 1-6, the public
ClientTokenDto currently exposes orgId, clientId and clientSecret which must be
restricted; replace it with a public DTO exposing only { orgId, clientAlias,
grantType? } and create a new InternalClientCredentialsDto for server-side use
containing { orgId, clientId, clientSecret, grantType? }. Update all local
imports/usages in libs/client-registration to consume the public DTO for
incoming requests and to map clientAlias -> clientId/clientSecret by retrieving
secrets from the secure store, then use InternalClientCredentialsDto internally
when calling Keycloak or Prisma; likewise update callsites in libs/common,
libs/keycloak-url and libs/prisma-service to stop accepting client credentials
in request objects and instead fetch secrets from the secure store before
performing operations, updating types and function signatures accordingly.

Comment on lines +11 to +13
async getUserByUsernameURL(realm: string, username: string): Promise<string> {
return `${process.env.KEYCLOAK_DOMAIN}admin/realms/${realm}/users?username=${username}`;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Encode the username query parameter.

The new getUserByUsernameURL places the raw username into the query string. Usernames frequently contain characters such as +, @, or spaces; without encodeURIComponent the generated URL becomes invalid and lookups will fail for those users. Please URL-encode the value before interpolating it. (keycloak.org)

-    return `${process.env.KEYCLOAK_DOMAIN}admin/realms/${realm}/users?username=${username}`;
+    const encodedUsername = encodeURIComponent(username);
+    return `${process.env.KEYCLOAK_DOMAIN}admin/realms/${realm}/users?username=${encodedUsername}`;
📝 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.

Suggested change
async getUserByUsernameURL(realm: string, username: string): Promise<string> {
return `${process.env.KEYCLOAK_DOMAIN}admin/realms/${realm}/users?username=${username}`;
}
async getUserByUsernameURL(realm: string, username: string): Promise<string> {
const encodedUsername = encodeURIComponent(username);
return `${process.env.KEYCLOAK_DOMAIN}admin/realms/${realm}/users?username=${encodedUsername}`;
}
🤖 Prompt for AI Agents
In libs/keycloak-url/src/keycloak-url.service.ts around lines 11 to 13, the
username is placed raw into the query string which breaks URLs for characters
like spaces, +, @; wrap the username with encodeURIComponent before
interpolation (e.g. ?username=${encodeURIComponent(username)}) so the generated
URL is valid for any username.

Signed-off-by: shitrerohit <rohit.shitre@ayanworks.com>
Signed-off-by: shitrerohit <rohit.shitre@ayanworks.com>
Signed-off-by: shitrerohit <rohit.shitre@ayanworks.com>
Signed-off-by: shitrerohit <rohit.shitre@ayanworks.com>
Signed-off-by: shitrerohit <rohit.shitre@ayanworks.com>
Signed-off-by: shitrerohit <rohit.shitre@ayanworks.com>
Signed-off-by: shitrerohit <rohit.shitre@ayanworks.com>
Signed-off-by: shitrerohit <rohit.shitre@ayanworks.com>
Signed-off-by: shitrerohit <rohit.shitre@ayanworks.com>
@shitrerohit shitrerohit force-pushed the feat/generate-token-for-client branch from b1ef5f6 to 9ce164f Compare September 30, 2025 06:40
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (9)
apps/organization/repositories/organization.repository.ts (1)

665-665: Update return type interfaces to include appLaunchDetails.

The query now selects appLaunchDetails, but the return type (IGetOrganization and related interfaces) doesn't include this property. Downstream code cannot access the field without type assertions, defeating TypeScript's type safety.

Update the IGetOrganization and IOrganization interface definitions to include:

appLaunchDetails?: <appropriate_type>;  // use Json, JsonValue, or specific shape

Also update any DTOs or mappers that construct/consume organization objects to reflect this new property.

apps/organization/dtos/client-token.dto.ts (1)

1-7: Do not accept admin clientSecret from external callers; fetch server-side instead.

Accepting clientSecret in a request DTO creates a major secret-handling risk (exposure via transit, logs, traces, and NATS payloads). Admin credentials should be retrieved from a secure config/secret store on the service side, not sent by external callers.

Additional concern: The DTO shape is inconsistent across layers. This file includes clientAlias (line 3), but libs/client-registration/src/dtos/client-token.dto.ts omits it, which may cause downstream mapping issues or confusion about which fields are actually required.

libs/client-registration/src/client-registration.service.ts (1)

727-745: Validate and decrypt admin credentials before building the payload.

This method sends clientId and clientSecret directly to Keycloak without validation or decryption. Other token methods in this service (e.g., getManagementToken at lines 151-171, getUserToken at lines 577-617) decrypt credentials using this.commonService.decryptPassword() before use, suggesting credentials are stored encrypted.

Apply this diff to add validation and decryption:

 async generateTokenUsingAdminCredentials(clientDetails: ClientTokenDto) {
   const realmName = process.env.KEYCLOAK_REALM;
+  if (!clientDetails?.clientId || !clientDetails?.clientSecret) {
+    throw new BadRequestException('clientId and clientSecret are required');
+  }
+  const decryptClientId = await this.commonService.decryptPassword(clientDetails.clientId);
+  const decryptClientSecret = await this.commonService.decryptPassword(clientDetails.clientSecret);
   const payload = {
-    client_id: clientDetails.clientId,
-    client_secret: clientDetails.clientSecret,
+    client_id: decryptClientId,
+    client_secret: decryptClientSecret,
     grant_type: clientDetails.grantType ?? 'client_credentials'
   };
libs/keycloak-url/src/keycloak-url.service.ts (2)

7-74: Remove async from pure URL builder methods.

Every URL helper now returns Promise<string> but only synchronously builds template strings. Callers expecting plain strings will receive Promise objects, which get stringified to [object Promise] when passed to HTTP libraries, breaking all Keycloak requests in this PR's token flow.

Remove the async keyword and change return types from Promise<string> to string:

-async createUserURL(realm: string): Promise<string> {
+createUserURL(realm: string): string {
   return `${process.env.KEYCLOAK_DOMAIN}admin/realms/${realm}/users`;
 }

Apply the same change to all other URL builder methods in this file.


11-13: URL-encode the username query parameter.

The username parameter is inserted raw into the query string. Usernames containing @, +, spaces, or other special characters will produce invalid URLs and cause lookup failures.

Apply this diff:

 async getUserByUsernameURL(realm: string, username: string): Promise<string> {
-  return `${process.env.KEYCLOAK_DOMAIN}admin/realms/${realm}/users?username=${username}`;
+  return `${process.env.KEYCLOAK_DOMAIN}admin/realms/${realm}/users?username=${encodeURIComponent(username)}`;
 }
apps/organization/src/organization.service.ts (4)

2050-2055: CRITICAL: Do not trust caller-supplied admin credentials.

The generateTokenUsingAdminCredentials method receives the entire DTO, which includes clientId and clientSecret fields from the HTTP request (as seen in the DTO definition). Accepting admin credentials from user input creates a severe security vulnerability:

  • Any caller can inject arbitrary admin credentials
  • Attackers could harvest real admin secrets by trial and error
  • Privilege separation is defeated

Resolution: After validating the clientAlias, resolve the actual admin credentials server-side from secure storage or environment variables (using the validated alias as a key), and pass only those server-sourced credentials to generateTokenUsingAdminCredentials. Reject any incoming requests that include admin credential fields in the DTO.

Based on past review comments.


2065-2067: CRITICAL BUG: Using orgId instead of clientId for authentication.

Line 2066 passes generateTokenDetails.orgId to authenticateClientKeycloak, but this function expects the Keycloak client_id. The orgId and clientId are typically different values, so this will cause token generation to fail.

Fix: Use the actual clientId from either:

  1. orgDetails.clientId (if persisted in the database), or
  2. clientDetails[0].clientId (just fetched from Keycloak)

Apply this diff:

  const { secret } = clientDetails[0];
+ const clientId = clientDetails[0]?.clientId ?? orgDetails.clientId;
+ if (!clientId) {
+   throw new NotFoundException('Client ID not found for organization');
+ }
  //step3: generate the token using client id and secret
- const authenticationResult = await this.authenticateClientKeycloak(generateTokenDetails.orgId, secret);
+ const authenticationResult = await this.authenticateClientKeycloak(clientId, secret);
  return { token: authenticationResult.access_token };

Based on past review comments.


2068-2071: Avoid logging sensitive data in errors.

Line 2069 logs the entire error object using JSON.stringify(error), which can inadvertently expose tokens, secrets, and other sensitive data. This is particularly risky in a token generation flow where errors may contain credential-related information.

Apply this diff to log only safe error properties:

  } catch (error) {
-   this.logger.error(`in generating issuer api token : ${JSON.stringify(error)}`);
+   this.logger.error('in generating issuer api token', { 
+     message: error?.message, 
+     status: error?.status,
+     name: error?.name 
+   });
    throw new RpcException(error.response ? error.response : error);
  }

Based on past review comments.


2056-2064: Extract clientId from client details.

While the code correctly fetches and extracts the secret, it should also extract the clientId from clientDetails[0] for use in the authentication step. Currently, only the secret is extracted on line 2064.

Apply this diff to extract both clientId and secret:

- const { secret } = clientDetails[0];
+ const { clientId, secret } = clientDetails[0];

This clientId should then be used in line 2066 instead of generateTokenDetails.orgId.

🧹 Nitpick comments (1)
apps/organization/src/organization.service.ts (1)

2040-2049: Improve error message clarity.

The client alias validation logic is sound, but line 2044 uses ResponseMessages.organisation.error.clientDetails which is misleading. When the env var for a client alias doesn't exist, the error should clearly indicate that the alias is not configured rather than suggesting issues with client details.

Consider using a more specific error message:

  if (!process.env[clientIdKey]) {
-   throw new NotFoundException(ResponseMessages.organisation.error.clientDetails);
+   throw new NotFoundException(`Client alias "${generateTokenDetails.clientAlias}" is not configured`);
  }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b1ef5f6 and 9ce164f.

📒 Files selected for processing (14)
  • .husky/pre-push (4 hunks)
  • apps/api-gateway/src/organization/dtos/client-token.dto.ts (1 hunks)
  • apps/api-gateway/src/organization/organization.controller.ts (2 hunks)
  • apps/api-gateway/src/organization/organization.service.ts (2 hunks)
  • apps/organization/dtos/client-token.dto.ts (1 hunks)
  • apps/organization/repositories/organization.repository.ts (1 hunks)
  • apps/organization/src/organization.controller.ts (16 hunks)
  • apps/organization/src/organization.service.ts (2 hunks)
  • libs/client-registration/src/client-registration.service.ts (2 hunks)
  • libs/client-registration/src/dtos/client-token.dto.ts (1 hunks)
  • libs/common/src/response-messages/index.ts (1 hunks)
  • libs/keycloak-url/src/keycloak-url.service.ts (1 hunks)
  • libs/prisma-service/prisma/migrations/20250925060521_added_app_launch_details_column_in_organization/migration.sql (1 hunks)
  • libs/prisma-service/prisma/schema.prisma (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
  • libs/client-registration/src/dtos/client-token.dto.ts
  • apps/api-gateway/src/organization/dtos/client-token.dto.ts
  • .husky/pre-push
  • apps/api-gateway/src/organization/organization.controller.ts
  • libs/prisma-service/prisma/migrations/20250925060521_added_app_launch_details_column_in_organization/migration.sql
  • libs/prisma-service/prisma/schema.prisma
  • apps/organization/src/organization.controller.ts
🧰 Additional context used
🧬 Code graph analysis (4)
apps/organization/dtos/client-token.dto.ts (2)
apps/api-gateway/src/organization/dtos/client-token.dto.ts (1)
  • ClientTokenDto (4-24)
libs/client-registration/src/dtos/client-token.dto.ts (1)
  • ClientTokenDto (1-6)
apps/api-gateway/src/organization/organization.service.ts (3)
apps/api-gateway/src/organization/dtos/client-token.dto.ts (1)
  • ClientTokenDto (4-24)
apps/organization/dtos/client-token.dto.ts (1)
  • ClientTokenDto (1-7)
libs/client-registration/src/dtos/client-token.dto.ts (1)
  • ClientTokenDto (1-6)
libs/client-registration/src/client-registration.service.ts (1)
libs/client-registration/src/dtos/client-token.dto.ts (1)
  • ClientTokenDto (1-6)
apps/organization/src/organization.service.ts (2)
apps/organization/dtos/client-token.dto.ts (1)
  • ClientTokenDto (1-7)
libs/common/src/response-messages/index.ts (1)
  • ResponseMessages (1-521)
🔇 Additional comments (3)
libs/client-registration/src/client-registration.service.ts (1)

747-755: LGTM — straightforward client details retrieval.

The method correctly retrieves client details using the Keycloak admin API with proper authorization headers.

apps/api-gateway/src/organization/organization.service.ts (1)

242-244: LGTM — clean NATS delegation.

The method correctly delegates token generation to the organization service via NATS messaging. The implementation follows the established pattern used throughout this service.

Note: Security concerns with ClientTokenDto accepting clientSecret from external callers have been flagged in the DTO file review.

apps/organization/src/organization.service.ts (1)

2034-2039: LGTM: Organization validation is correct.

The method properly validates that the organization exists before proceeding with token generation.

MaximumOrgsLimit: 'Limit reached: You can be associated with or create maximum 10 organizations.',
adminTokenDetails: 'Error in generating admin token details',
clientDetails: 'Error in fetching client details',
missMachingAlias: 'Client alias does not match the registered alias'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Fix typo in property name.

The key missMachingAlias contains a typo. It should be mismatchingAlias (or misMatchingAlias if you prefer capital 'M').

Apply this diff to fix the typo:

-      missMachingAlias: 'Client alias does not match the registered alias'
+      mismatchingAlias: 'Client alias does not match the registered alias'
📝 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.

Suggested change
missMachingAlias: 'Client alias does not match the registered alias'
mismatchingAlias: 'Client alias does not match the registered alias'
🤖 Prompt for AI Agents
In libs/common/src/response-messages/index.ts around line 136, the property key
missMachingAlias contains a typo; rename the key to mismatchingAlias (or
misMatchingAlias if you prefer camelCase with capital M) and update any
imports/usages across the codebase to use the corrected key so references remain
consistent.

Signed-off-by: shitrerohit <rohit.shitre@ayanworks.com>
Signed-off-by: shitrerohit <rohit.shitre@ayanworks.com>
Signed-off-by: shitrerohit <rohit.shitre@ayanworks.com>
@sonarqubecloud
Copy link

@shitrerohit shitrerohit merged commit 6f4be27 into main Sep 30, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants