Conversation
Signed-off-by: bhavanakarwade <bhavana.karwade@ayanworks.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update introduces support for Single Sign-On (SSO) client alias management across the system. It adds environment variable templates, database schema and migration for storing client aliases and URLs, new endpoints and service methods to fetch these aliases, and utilities for handling SSO client credentials. The email verification flow is enhanced to support client alias selection and redirection. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API-Gateway
participant AuthzService
participant UserService
participant UserRepository
participant DB
Client->>API-Gateway: GET /clientAliases
API-Gateway->>AuthzService: getClientAlias()
AuthzService->>UserService: NATS message: get-client-alias-and-url
UserService->>UserRepository: fetchClientAliases()
UserRepository->>DB: SELECT * FROM client_aliases
DB-->>UserRepository: client_aliases[]
UserRepository-->>UserService: client_aliases[]
UserService-->>AuthzService: client_aliases[]
AuthzService-->>API-Gateway: client_aliases[]
API-Gateway-->>Client: Response with aliases and URLs
sequenceDiagram
participant Client
participant API-Gateway
participant AuthzService
participant UserService
participant Utils
Client->>API-Gateway: POST /verification-mail?clientAlias=ALIAS
API-Gateway->>AuthzService: sendVerificationMail({email, ..., clientAlias})
AuthzService->>UserService: NATS message: sendVerificationMail with clientAlias
UserService->>Utils: getCredentialsByAlias(clientAlias)
Utils-->>UserService: client credentials
UserService->>UserService: sendEmailForVerification(..., clientAlias)
UserService-->>AuthzService: user
AuthzService-->>API-Gateway: user
API-Gateway-->>Client: Response
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (8)
libs/prisma-service/prisma/schema.prisma (1)
524-530: Consider adding indexes for better query performance.The
client_aliasesmodel structure is appropriate for SSO functionality. Consider adding unique constraints or indexes ifclientAliasshould be unique or if queries will frequently filter by these fields.If
clientAliasshould be unique, apply this diff:model client_aliases { id String @id @default(uuid()) @db.Uuid createDateTime DateTime @default(now()) @db.Timestamptz(6) lastChangedDateTime DateTime @default(now()) @db.Timestamptz(6) - clientAlias String? + clientAlias String? @unique clientUrl String }libs/prisma-service/prisma/migrations/20250701025741_added_client_alias/migration.sql (1)
1-11: Consider adding indexes for query performance.The table schema is well-designed for storing client alias mappings. Consider adding indexes on frequently queried columns like
clientAliasif this table will be queried often.If frequent lookups are expected, consider adding an index:
CREATE INDEX "client_aliases_clientAlias_idx" ON "client_aliases"("clientAlias");apps/user/repositories/user.repository.ts (1)
38-42: Improve JSDoc documentation.The JSDoc comment is incomplete and doesn't follow the established pattern in the file.
Apply this diff to improve the documentation:
- /** - * - * @returns Client alias and its url - */ + /** + * Fetches all client aliases from the database + * @returns Promise<client_aliases[]> Array of client alias records + */apps/api-gateway/src/authz/authz.controller.ts (2)
46-46: Fix typo in JSDoc comment- * Fetch client aliase. + * Fetch client aliases.
54-54: Fix typo in API operation description- description: 'Fetch client aliases and itr url' + description: 'Fetch client aliases and its url'apps/user/src/user.service.ts (1)
92-95: Move the eslint-disable comment to the correct positionThe eslint-disable comment should be placed immediately before the line that uses snake_case.
- - // eslint-disable-next-line camelcase - async getClientAliases(): Promise<client_aliases[]> { + // eslint-disable-next-line camelcase + async getClientAliases(): Promise<client_aliases[]> {apps/user/src/user.controller.ts (2)
39-39: Fix typo in JSDoc comment- * Description: Fetch client aliases are its url + * Description: Fetch client aliases and its url
44-46: Fix eslint-disable comment placementThe eslint-disable comment should be placed immediately before the line that uses snake_case.
- // eslint-disable-next-line camelcase - async getClientAliases(): Promise<client_aliases[]> { + // eslint-disable-next-line camelcase + async getClientAliases(): Promise<client_aliases[]> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
.env.demo(1 hunks).env.sample(1 hunks)apps/api-gateway/src/authz/authz.controller.ts(6 hunks)apps/api-gateway/src/authz/authz.service.ts(3 hunks)apps/api-gateway/src/authz/decorators/user-auth-client.ts(1 hunks)apps/api-gateway/src/user/dto/create-user.dto.ts(1 hunks)apps/api-gateway/src/user/utils/index.ts(1 hunks)apps/organization/repositories/organization.repository.ts(1 hunks)apps/user/repositories/user.repository.ts(6 hunks)apps/user/src/user.controller.ts(6 hunks)apps/user/src/user.service.ts(29 hunks)apps/user/templates/user-email-template.ts(2 hunks)docker-compose.yml(1 hunks)libs/common/src/cast.helper.ts(3 hunks)libs/common/src/common.module.ts(1 hunks)libs/common/src/interfaces/user.interface.ts(1 hunks)libs/common/src/response-messages/index.ts(1 hunks)libs/prisma-service/prisma/migrations/20250701025741_added_client_alias/migration.sql(1 hunks)libs/prisma-service/prisma/schema.prisma(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
libs/common/src/common.module.ts (1)
libs/logger/src/logger.interface.ts (1)
Logger(6-20)
apps/user/repositories/user.repository.ts (1)
apps/user/interfaces/user.interface.ts (1)
IUserDeletedActivity(210-218)
apps/api-gateway/src/user/utils/index.ts (2)
libs/common/src/interfaces/user.interface.ts (1)
IClientDetailsSSO(24-29)libs/common/src/cast.helper.ts (1)
encryptClientCredential(442-456)
libs/common/src/interfaces/user.interface.ts (1)
apps/user/interfaces/user.interface.ts (4)
IVerifyUserEmail(173-176)ISendVerificationEmail(51-56)IOrgRole(22-26)IOrganisation(27-38)
apps/api-gateway/src/user/dto/create-user.dto.ts (2)
libs/common/src/cast.helper.ts (2)
trim(39-43)toLowerCase(35-37)libs/validations/maxLength.ts (1)
MaxLength(3-28)
apps/user/src/user.controller.ts (6)
libs/common/src/interfaces/user.interface.ts (6)
ISendVerificationEmail(13-22)IVerifyUserEmail(9-12)ISignInUser(1-8)IResetPasswordResponse(54-57)IUserInvitations(30-33)ISignUpUserResponse(59-61)apps/user/interfaces/user.interface.ts (10)
ISendVerificationEmail(51-56)IVerifyUserEmail(173-176)IUserSignIn(178-182)IUserResetPassword(184-190)IUserForgotPassword(191-196)Payload(167-171)IOrgUsers(112-115)ICheckUserDetails(104-110)IUserInformation(58-65)IUserDeletedActivity(210-218)apps/api-gateway/src/authz/authz.service.ts (1)
refreshToken(68-70)apps/organization/interfaces/organization.interface.ts (1)
Payload(144-149)libs/user-activity/interface/index.ts (1)
IUsersActivity(1-9)apps/api-gateway/src/user/dto/add-user.dto.ts (1)
AddPasskeyDetailsDto(45-51)
🔇 Additional comments (27)
apps/organization/repositories/organization.repository.ts (1)
685-686: LGTM! Clean addition of tenant ID for multi-tenant support.The addition of
tenantIdto theorg_agentsselection is appropriate for the SSO and client alias functionality being introduced. This change expands the data retrieved for organization agents without affecting existing logic or performance.docker-compose.yml (1)
64-64: LGTM! Proper service dependency management.The addition of
redisas a dependency for theissuanceservice ensures proper startup ordering and aligns with the SSO functionality enhancements.libs/common/src/cast.helper.ts (2)
1-2: LGTM! Required imports for encryption functionality.The CryptoJS import supports the new client credential encryption feature.
28-28: LGTM! Logger instance for encryption operations.The logger instance will be used for debugging and error reporting in the encryption function.
libs/common/src/response-messages/index.ts (1)
7-7: LGTM! Proper response message addition.The new success message for fetching client aliases follows the existing pattern and supports the SSO functionality.
libs/common/src/common.module.ts (3)
1-1: LGTM! Required imports for logging functionality.The Logger import supports the encryption operations added in cast.helper.ts.
5-5: LGTM! LoggerModule import for proper module integration.The LoggerModule import enables logging capabilities throughout the common module.
8-9: LGTM! Proper module configuration for logging.The LoggerModule is correctly added to imports and Logger service to providers, enabling consistent logging across the module.
libs/prisma-service/prisma/schema.prisma (1)
522-522: LGTM! Fixes unclosed enum.The closing brace for
CloudWalletTypeenum was missing and is now properly added..env.demo (1)
160-180: Well-structured SSO configuration template.The SSO configuration section is well-organized with clear documentation and follows good practices for multi-client support. The template structure makes it easy to add additional clients.
apps/user/repositories/user.repository.ts (1)
6-16: Approve import reorganization and formatting improvements.The import reorganization and formatting improvements (including consistent arrow function parentheses) enhance code readability.
Also applies to: 780-796
apps/api-gateway/src/authz/decorators/user-auth-client.ts (1)
1-27: Excellent implementation of validation pipe.This validation pipe follows NestJS best practices with:
- Proper environment variable parsing and filtering
- Correct handling of optional values
- Clear error messages including allowed values
- Uppercase conversion for consistency
- Clean constructor initialization
The implementation is robust and will provide good user feedback for invalid client aliases.
.env.sample (1)
180-200: Consistent SSO configuration template.The SSO configuration section maintains consistency with .env.demo and provides clear documentation for multi-client setup.
apps/user/templates/user-email-template.ts (4)
2-11: LGTM! Method signature properly expanded for multi-client SSO support.The addition of optional
redirectToandclientAliasparameters aligns well with the broader SSO client alias functionality being implemented across the system.
17-23: LGTM! Query parameter handling is secure and well-implemented.The conditional appending of
redirectToandclientAliasparameters is properly implemented with appropriate checks to prevent adding undefined values to the URL.
80-82: Good improvement in error handling.Replacing silent failure with a descriptive error message is a best practice that will help with debugging and monitoring.
12-12: Ignore path-determination-env-check concernThere is no prior comparison of
clientIdto an environment variable inURLUserEmailTemplate.getUserURLTemplate. The truthy check onclientIdis the intended behavior (use'/verify-email-success'when a client ID is provided, otherwise leave the base path untouched). You can disregard the suggestion about an env-based comparison here.Likely an incorrect or invalid review comment.
apps/api-gateway/src/authz/authz.service.ts (3)
8-19: LGTM! Import consolidation improves code organization.The import statements are better organized with proper grouping and the addition of the
usertype from Prisma aligns with the service method changes.
32-34: LGTM! New method follows established service patterns.The
getClientAlias()method properly follows the same NATS messaging pattern used by other methods in this service.
41-41: No return type inconsistencies detected
AllsendVerificationMailimplementations and calls consistently usePromise<user>as their return type across:
apps/api-gateway/src/authz/authz.service.tsapps/user/src/user.service.tsapps/user/src/user.controller.tsThe recent change affected only the input DTO (
UserEmailVerificationDto), not the return type. No further changes are required.apps/api-gateway/src/user/utils/index.ts (2)
4-9: LGTM! Default client configuration properly encrypts credentials.The
getDefaultClientfunction correctly uses environment variables and encrypts sensitive credentials using theencryptClientCredentialhelper function.
41-43: Good error handling for missing configuration.The validation and error throwing for incomplete client configuration helps prevent runtime issues and provides clear feedback about missing environment variables.
libs/common/src/interfaces/user.interface.ts (3)
24-29: LGTM! Well-defined interface for SSO client details.The
IClientDetailsSSOinterface is properly structured with all necessary fields for client authentication and identification.
63-69: LGTM! Database-aligned interface for client aliases.The
IClientAliasesinterface properly reflects the database schema structure with appropriate timestamp and identifier fields.
59-61: Good fix for missing semicolon.The formatting correction in
ISignUpUserResponseimproves consistency with other interfaces.apps/api-gateway/src/user/dto/create-user.dto.ts (1)
1-1: LGTM! Better import organization.Moving
ApiPropertyandApiPropertyOptionalto the top improves code readability and follows common conventions.apps/user/src/user.service.ts (1)
134-134: No additional try-catch needed forgetCredentialsByAlias
ThegetCredentialsByAliasimplementation already throws anErrorwhen the alias is invalid or missing required fields (clientId,clientSecret,domain). No further error handling is required at the call site insendVerificationMailunless you need to transform the error or provide a fallback.
| export const encryptClientCredential = async (clientCredential: string): Promise<string> => { | ||
| try { | ||
| const encryptedToken = CryptoJS.AES.encrypt( | ||
| JSON.stringify(clientCredential), | ||
| process.env.CRYPTO_PRIVATE_KEY | ||
| ).toString(); | ||
|
|
||
| logger.debug('Client credentials encrypted successfully'); | ||
|
|
||
| return encryptedToken; | ||
| } catch (error) { | ||
| logger.error('An error occurred during encryptClientCredential:', error); | ||
| throw error; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add environment variable validation for security.
The encryption function is well-structured but lacks validation for the CRYPTO_PRIVATE_KEY environment variable. This could cause runtime failures if the variable is undefined.
Apply this diff to add proper validation:
export const encryptClientCredential = async (clientCredential: string): Promise<string> => {
try {
+ if (!process.env.CRYPTO_PRIVATE_KEY) {
+ throw new Error('CRYPTO_PRIVATE_KEY environment variable is not configured');
+ }
+
const encryptedToken = CryptoJS.AES.encrypt(
JSON.stringify(clientCredential),
process.env.CRYPTO_PRIVATE_KEY
).toString();📝 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.
| export const encryptClientCredential = async (clientCredential: string): Promise<string> => { | |
| try { | |
| const encryptedToken = CryptoJS.AES.encrypt( | |
| JSON.stringify(clientCredential), | |
| process.env.CRYPTO_PRIVATE_KEY | |
| ).toString(); | |
| logger.debug('Client credentials encrypted successfully'); | |
| return encryptedToken; | |
| } catch (error) { | |
| logger.error('An error occurred during encryptClientCredential:', error); | |
| throw error; | |
| } | |
| }; | |
| export const encryptClientCredential = async (clientCredential: string): Promise<string> => { | |
| try { | |
| if (!process.env.CRYPTO_PRIVATE_KEY) { | |
| throw new Error('CRYPTO_PRIVATE_KEY environment variable is not configured'); | |
| } | |
| const encryptedToken = CryptoJS.AES.encrypt( | |
| JSON.stringify(clientCredential), | |
| process.env.CRYPTO_PRIVATE_KEY | |
| ).toString(); | |
| logger.debug('Client credentials encrypted successfully'); | |
| return encryptedToken; | |
| } catch (error) { | |
| logger.error('An error occurred during encryptClientCredential:', error); | |
| throw error; | |
| } | |
| }; |
🤖 Prompt for AI Agents
In libs/common/src/cast.helper.ts around lines 442 to 456, the
encryptClientCredential function uses the CRYPTO_PRIVATE_KEY environment
variable without validating its presence, which can cause runtime errors. Add a
check at the start of the function to verify that process.env.CRYPTO_PRIVATE_KEY
is defined and throw a clear error if it is missing. This ensures the function
fails fast and securely if the key is not set.
| function getClientDetails(alias: string): IClientDetailsSSO { | ||
| const clientIdKey = `${alias}_KEYCLOAK_MANAGEMENT_CLIENT_ID`; | ||
| const clientSecretKey = `${alias}_KEYCLOAK_MANAGEMENT_CLIENT_SECRET`; | ||
| const domainKey = `${alias}_DOMAIN`; | ||
| const aliasNameKey = `${alias}_ALIAS`; | ||
|
|
||
| const clientId = process.env[clientIdKey]; | ||
| const clientSecret = process.env[clientSecretKey]; | ||
| const domain = process.env[domainKey]; | ||
| const aliasName = process.env[aliasNameKey] || alias; | ||
|
|
||
| const clientDetails: IClientDetailsSSO = { | ||
| clientId, | ||
| clientSecret, | ||
| domain, | ||
| alias: aliasName | ||
| }; | ||
|
|
||
| return clientDetails; | ||
| } |
There was a problem hiding this comment.
Critical: Inconsistent credential encryption in client details.
The getClientDetails function returns unencrypted clientId and clientSecret from environment variables, while getDefaultClient encrypts these same credentials. This creates an inconsistency in how credentials are handled.
-function getClientDetails(alias: string): IClientDetailsSSO {
+async function getClientDetails(alias: string): Promise<IClientDetailsSSO> {
const clientIdKey = `${alias}_KEYCLOAK_MANAGEMENT_CLIENT_ID`;
const clientSecretKey = `${alias}_KEYCLOAK_MANAGEMENT_CLIENT_SECRET`;
const domainKey = `${alias}_DOMAIN`;
const aliasNameKey = `${alias}_ALIAS`;
const clientId = process.env[clientIdKey];
const clientSecret = process.env[clientSecretKey];
const domain = process.env[domainKey];
const aliasName = process.env[aliasNameKey] || alias;
const clientDetails: IClientDetailsSSO = {
- clientId,
- clientSecret,
+ clientId: await encryptClientCredential(clientId),
+ clientSecret: await encryptClientCredential(clientSecret),
domain,
alias: aliasName
};
return clientDetails;
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/api-gateway/src/user/utils/index.ts around lines 12 to 31, the
getClientDetails function returns clientId and clientSecret directly from
environment variables without encryption, causing inconsistency with
getDefaultClient which encrypts these credentials. To fix this, modify
getClientDetails to encrypt clientId and clientSecret before returning them,
using the same encryption method as getDefaultClient to ensure consistent
credential handling.
Signed-off-by: bhavanakarwade <bhavana.karwade@ayanworks.com>
Signed-off-by: bhavanakarwade <bhavana.karwade@ayanworks.com>
|
* feat: single sign on Signed-off-by: bhavanakarwade <bhavana.karwade@ayanworks.com> * fix: rabbit suggestions Signed-off-by: bhavanakarwade <bhavana.karwade@ayanworks.com> * update: send verification mail template Signed-off-by: bhavanakarwade <bhavana.karwade@ayanworks.com> --------- Signed-off-by: bhavanakarwade <bhavana.karwade@ayanworks.com>
3 participants
What
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes / Style
Chores