Skip to content

added initial changes to register holder for notification - #1541

Merged
RinkalBhojani merged 2 commits into
mainfrom
feat/oid4vp-notification
Jan 12, 2026
Merged

added initial changes to register holder for notification#1541
RinkalBhojani merged 2 commits into
mainfrom
feat/oid4vp-notification

Conversation

@RinkalBhojani

@RinkalBhojani RinkalBhojani commented Jan 12, 2026

Copy link
Copy Markdown
Contributor

What?

  • Schema changes for new table
  • Repository method
  • New API for registering fcmToken against sessionId

Summary by CodeRabbit

Release Notes

  • New Features
    • Added holder notification registration capability, enabling users to register and manage push notifications via Firebase Cloud Messaging tokens and session identifiers
    • Implemented comprehensive notification state tracking system to monitor notification delivery, consumption, and purging across multiple communication channels
    • Enhanced notification infrastructure to support holder-specific notification workflows

✏️ Tip: You can customize this high-level summary in your review settings.

Signed-off-by: Rinkal Bhojani <rinkal.bhojani@ayanworks.com>
@coderabbitai

coderabbitai Bot commented Jan 12, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

This PR introduces a holder notification registration feature by adding new API endpoints, service methods, repository layer, database schema, and supporting type definitions. The changes establish an end-to-end flow for registering holder notifications with sessionId, holderDid, and fcmToken across the API gateway and notification service layers.

Changes

Cohort / File(s) Summary
API Gateway - DTO Definitions
apps/api-gateway/src/notification/dtos/notification.dto.ts
Renamed RegisterHolderCredentalsDto to RegisterHolderForNotificationDto; reorganized fields to include fcmToken, holderDid, and new sessionId field; removed orgId and userKey; refactored other DTOs with explicit property validation and IsUrl constraints
API Gateway - Endpoints
apps/api-gateway/src/notification/notification.controller.ts
Added new POST endpoint registerHolderNotification with decorator and documentation; integrates new RegisterHolderForNotificationDto; returns ApiResponse with CREATED status
API Gateway - Service
apps/api-gateway/src/notification/notification.service.ts
Added new method registerHolderNotification that dispatches NATS message with topic register-holder-notification; imports new DTO and interface types
Notification Service - Repository
apps/notification/src/holder-notification.repository.ts
New repository class with CRUD methods: registerHolderNotification, updateHolderNotificationState, getHolderNotificationBySessionId; includes duplicate checking and error handling
Notification Service - Message Handler
apps/notification/src/notification.controller.ts
Added new message pattern handler registerHolderNotification with cmd register-holder-notification; delegates to service method
Notification Service - Module
apps/notification/src/notification.module.ts
Registered HolderNotificationRepository in providers; import and module export adjustments
Notification Service - Service
apps/notification/src/notification.service.ts
Added new method registerHolderNotification with INITIATED state initialization; injected HolderNotificationRepository dependency
Common - Interface Definitions
libs/common/src/interfaces/holder-notification.interfaces.ts
New file with three interfaces: IHolderNotification (entity model), ICreateHolderNotification (creation payload), IWebhookEndpoint
Common - Response Messages
libs/common/src/response-messages/index.ts
Added new holderNotification section with success messages (register, sendNotification) and error messages (notFound, conflict)
Enums
libs/enum/src/enum.ts
New NotificationStatus enum with 8 states covering lifecycle: INITIATED, DATA_DELIVERED, DATA_DELIVERED_AND_NOTIFIED_WITH_NATS, DATA_DELIVERED_AND_NOTIFIED_WITH_FCM, DATA_PURGED, DATA_PURGED_AND_NOTIFIED_WITH_NATS, DATA_PURGED_AND_NOTIFIED_WITH_FCM, NOTIFICATION_CONSUMED
Database - Schema
libs/prisma-service/prisma/schema.prisma
New holder_notification model with fields: id (UUID PK), sessionId (unique), holderDid, fcmToken, state, createDateTime, lastChangedDateTime, deletedAt (optional)
Database - Migration
libs/prisma-service/prisma/migrations/20260112075514...
SQL migration creating holder_notification table with primary key on id and unique index on sessionId

Sequence Diagram

sequenceDiagram
    participant Client
    participant APIGateway as API Gateway
    participant NotifService as Notification Service
    participant Database as Database

    Client->>APIGateway: POST /register/holder-notification<br/>(fcmToken, holderDid, sessionId)
    APIGateway->>APIGateway: Validate RegisterHolderForNotificationDto
    APIGateway->>NotifService: NATS Message<br/>cmd: register-holder-notification
    NotifService->>NotifService: Set state to INITIATED
    NotifService->>Database: Check for existing sessionId
    alt Duplicate sessionId
        Database-->>NotifService: ConflictException
        NotifService-->>APIGateway: Error
        APIGateway-->>Client: 409 Conflict
    else New sessionId
        NotifService->>Database: Create holder_notification record
        Database-->>NotifService: IHolderNotification
        NotifService-->>APIGateway: IHolderNotification
        APIGateway-->>Client: 201 Created
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested labels

feature

Suggested reviewers

  • shitrerohit
  • GHkrishna

Poem

🐰 A holder's notification takes flight,
With sessionId, did, and tokens so bright,
Database models and messages in place,
The registration flow runs at a proper pace! ✨

🚥 Pre-merge checks | ✅ 3
✅ 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 accurately describes the main change: adding initial functionality to register holders for notifications. It directly aligns with the primary objective of adding a new API endpoint, repository method, schema changes, and DTO structure for holder notification registration.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🤖 Fix all issues with AI agents
In @apps/api-gateway/src/notification/dtos/notification.dto.ts:
- Around line 28-46: Rename the DTO class RegisterOrgWebhhookEndpointDto to
RegisterOrgWebhookEndpointDto (fix the double 'h') and update all usages and
imports that reference this symbol (e.g., in notification.service.ts,
notification.controller.ts, and any other files importing or typing it); ensure
the exported class name, its import specifiers, parameter types, and any
ctor/validator references are adjusted consistently so compilation and runtime
references match the new RegisterOrgWebhookEndpointDto identifier.

In @apps/api-gateway/src/notification/notification.controller.ts:
- Around line 101-107: The registerHolderNotification endpoint in
NotificationController is missing authentication; add the
@UseGuards(AuthGuard('jwt')) decorator to the registerHolderNotification method
(and similarly protect the other notification endpoints in this controller) and
import UseGuards from @nestjs/common and AuthGuard from @nestjs/passport so the
route requires a valid JWT before accepting FCM token, holder DID, and session
ID; keep the existing method signature but ensure the guard is applied at the
method or controller level to enforce authentication.

In @apps/notification/src/holder-notification.repository.ts:
- Around line 17-21: The JSDoc above the method in
holder-notification.repository.ts incorrectly says "Register organization
webhook endpoint"; update the comment to accurately describe this method's
purpose (registering holder notifications), e.g., change the description line to
"Register holder notification" and adjust param/returns wording to reference the
holder notification payload and stored holder notification data (keep the
existing @param and @returns tags but make their text reflect holder
notification semantics and the actual payload/return types used by the method).
- Around line 22-48: registerHolderNotification is vulnerable to a race
condition because it calls prisma.holder_notification.findFirst followed by
create without a DB unique constraint on sessionId; add a unique constraint for
sessionId in the Prisma schema (model holder_notification @@unique or field
@unique) and then modify registerHolderNotification to handle unique-constraint
violations atomically by either using prisma.holder_notification.upsert (if
updating is acceptable) or catching Prisma unique-violation error code P2002 on
create and returning/throwing a ConflictException; update the error handling in
registerHolderNotification to detect error.code === 'P2002' (or use upsert) and
map it to ResponseMessages.holderNotification.error.conflict.
🧹 Nitpick comments (9)
libs/prisma-service/prisma/schema.prisma (1)

732-741: Consider adding an index on sessionId for query performance.

Based on the repository pattern that queries by sessionId, this column will be frequently queried. Without an index, lookups will degrade as the table grows.

Also, lastChangedDateTime uses @default(now()) but lacks @updatedAt, so it won't automatically update when records are modified.

♻️ Suggested improvements
 model holder_notification {
   id                  String    @id @default(uuid()) @db.Uuid
   sessionId           String
   holderDid           String
   fcmToken            String
   state               String
   createDateTime      DateTime  @default(now()) @db.Timestamptz(6)
-  lastChangedDateTime DateTime  @default(now()) @db.Timestamptz(6)
+  lastChangedDateTime DateTime  @updatedAt @db.Timestamptz(6)
   deletedAt           DateTime? @db.Timestamp(6)
+
+  @@index([sessionId])
 }
libs/common/src/interfaces/holder-notification.interfaces.ts (2)

3-12: Type inconsistency: state field uses different types across interfaces.

IHolderNotification.state is typed as string (line 8), while ICreateHolderNotification.state is typed as NotificationStatus (line 18). Consider using the enum consistently for better type safety.

♻️ Suggested fix
+import { NotificationStatus } from '@credebl/enum/enum';
+
 export interface IHolderNotification {
   id: string;
   sessionId: string;
   holderDid: string;
   fcmToken: string;
-  state: string;
+  state: NotificationStatus;
   createDateTime: Date;
   lastChangedDateTime: Date;
   deletedAt?: Date;
 }

21-24: Consider moving IWebhookEndpoint to a separate file.

This interface relates to organization-level webhooks rather than holder notifications. Consider placing it in a more appropriate location for better code organization.

libs/enum/src/enum.ts (1)

353-362: Inconsistent casing in enum values.

INITIATED uses lowercase ('initiated'), while other values use PascalCase (e.g., 'DataDelivered'). This inconsistency could cause confusion when comparing or displaying these values.

♻️ Suggested fix for consistency
 export enum NotificationStatus {
-  INITIATED = 'initiated',
+  INITIATED = 'Initiated',
   DATA_DELIVERED = 'DataDelivered',
   DATA_DELIVERED_AND_NOTIFIED_WITH_NATS = 'DataDeliveredAndNotifiedWithNATS',
   DATA_DELIVERED_AND_NOTIFIED_WITH_FCM = 'DataDeliveredAndNotifiedWithFCM',
   DATA_PURGED = 'DataPurged',
   DATA_PURGED_AND_NOTIFIED_WITH_NATS = 'DataPurgedAndNotifiedWithNATS',
   DATA_PURGED_AND_NOTIFIED_WITH_FCM = 'DataPurgedAndNotifiedWithFCM',
   NOTIFICATION_CONSUMED = 'NotificationConsumed'
 }
apps/api-gateway/src/notification/notification.controller.ts (1)

94-100: Remove commented-out code or clarify intent.

The commented-out @ApiExcludeEndpoint() decorator on line 95 appears to be intentionally removed to expose this endpoint in Swagger documentation. If this is the intended behavior, please remove the commented line entirely. If this endpoint should be hidden from Swagger (like the other notification endpoints), uncomment the decorator.

♻️ Suggested change
  @Post('/register/holder-notification')
-  // @ApiExcludeEndpoint()
  @ApiOperation({
    summary: `Register holder for notification`,
    description: `Register holder for notification`
  })
apps/notification/src/notification.service.ts (2)

7-10: Use consistent import alias.

The import uses a relative path (../../../libs/common/src/interfaces/holder-notification.interfaces) while other imports in this file use the @credebl/ alias pattern. This should be consistent with the codebase convention.

♻️ Suggested change
-import {
-  ICreateHolderNotification,
-  IHolderNotification
-} from '../../../libs/common/src/interfaces/holder-notification.interfaces';
+import {
+  ICreateHolderNotification,
+  IHolderNotification
+} from '@credebl/common/interfaces/holder-notification.interfaces';

92-96: Avoid mutating the input payload directly.

Line 94 mutates the payload.state property directly, which is a side effect that can lead to unexpected behavior if the payload object is used elsewhere. Create a new object instead.

♻️ Suggested change
  async registerHolderNotification(payload: ICreateHolderNotification): Promise<IHolderNotification> {
    try {
-      payload.state = NotificationStatus.INITIATED;
-      const storeHolderNotification = await this.holderNotificationRepository.registerHolderNotification(payload);
+      const notificationPayload: ICreateHolderNotification = {
+        ...payload,
+        state: NotificationStatus.INITIATED
+      };
+      const storeHolderNotification = await this.holderNotificationRepository.registerHolderNotification(notificationPayload);
      return storeHolderNotification;
apps/notification/src/notification.controller.ts (1)

34-38: Fix JSDoc return description.

The JSDoc comment says "@returns Get notification details" but this method registers a holder notification and returns the stored data. Update it to match the registration pattern used in other methods.

📝 Suggested change
  /**
   * Register notification for holder
   * @param payload
-   * @returns Get notification details
+   * @returns Stored notification data
   */
apps/notification/src/holder-notification.repository.ts (1)

50-70: Consider using update instead of updateMany for consistency.

The method uses updateMany but then returns a single record via getHolderNotificationBySessionId (which uses findFirst). If sessionId is intended to be unique, using update with a unique constraint would be more appropriate and consistent. If multiple records per sessionId are possible, the returned record is indeterminate.

Proposed refactor (assuming sessionId is unique)
   async updateHolderNotificationState(sessionId: string, state: NotificationStatus): Promise<IHolderNotification> {
     try {
-      const updateNotification = await this.prisma.holder_notification.updateMany({
+      const updateNotification = await this.prisma.holder_notification.update({
         where: {
           sessionId
         },
         data: {
           state
         }
       });

-      if (0 === updateNotification.count) {
-        throw new NotFoundException(ResponseMessages.holderNotification.error.notFound);
-      }
-
-      return this.getHolderNotificationBySessionId(sessionId);
+      return updateNotification;
     } catch (error) {
+      if (error.code === 'P2025') {
+        throw new NotFoundException(ResponseMessages.holderNotification.error.notFound);
+      }
       this.logger.error(`Error in updateHolderNotificationState: ${error.message} `);
       throw error;
     }
   }
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8f893a2 and bbd9afe.

📒 Files selected for processing (12)
  • apps/api-gateway/src/notification/dtos/notification.dto.ts
  • apps/api-gateway/src/notification/notification.controller.ts
  • apps/api-gateway/src/notification/notification.service.ts
  • apps/notification/src/holder-notification.repository.ts
  • apps/notification/src/notification.controller.ts
  • apps/notification/src/notification.module.ts
  • apps/notification/src/notification.service.ts
  • libs/common/src/interfaces/holder-notification.interfaces.ts
  • libs/common/src/response-messages/index.ts
  • libs/enum/src/enum.ts
  • libs/prisma-service/prisma/migrations/20260111135647_added_holder_notification_table/migration.sql
  • libs/prisma-service/prisma/schema.prisma
🧰 Additional context used
🧬 Code graph analysis (4)
apps/api-gateway/src/notification/notification.service.ts (1)
libs/common/src/interfaces/holder-notification.interfaces.ts (1)
  • IHolderNotification (3-12)
apps/api-gateway/src/notification/notification.controller.ts (1)
libs/common/src/interfaces/response.interface.ts (1)
  • IResponse (8-13)
apps/notification/src/notification.service.ts (2)
apps/notification/src/holder-notification.repository.ts (1)
  • Injectable (10-95)
libs/common/src/interfaces/holder-notification.interfaces.ts (2)
  • ICreateHolderNotification (14-19)
  • IHolderNotification (3-12)
apps/api-gateway/src/notification/dtos/notification.dto.ts (1)
libs/common/src/cast.helper.ts (1)
  • trim (39-43)
🔇 Additional comments (10)
libs/common/src/response-messages/index.ts (1)

456-465: LGTM!

The new holderNotification response messages follow the established patterns and provide clear, appropriate messages for the holder notification workflow.

libs/prisma-service/prisma/migrations/20260111135647_added_holder_notification_table/migration.sql (1)

2-13: Add an index on sessionId to the migration for query performance.

The repository performs frequent lookups and updates on the holder_notification table using sessionId as a filter (findFirst and updateMany queries). Adding an index will improve the performance of these operations.

apps/notification/src/notification.module.ts (1)

16-16: LGTM!

The HolderNotificationRepository is correctly imported and added to the providers array, enabling proper dependency injection throughout the notification module.

Also applies to: 36-36

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

46-59: LGTM!

The new registerHolderNotification method follows the established pattern of using NATS messaging for inter-service communication, consistent with the existing registerOrgWebhookEndpoint and sendNotification methods.

apps/notification/src/notification.controller.ts (1)

39-42: LGTM!

The new registerHolderNotification message handler correctly delegates to the service and follows the established pattern of the other handlers in this controller.

apps/notification/src/holder-notification.repository.ts (2)

1-15: LGTM!

Imports and constructor are well-structured with proper dependency injection of PrismaService and Logger.


72-94: LGTM with suggestion.

The implementation is correct. If sessionId is guaranteed to be unique (via database constraint), consider using findUnique instead of findFirst for clarity and potential performance benefits.

apps/api-gateway/src/notification/dtos/notification.dto.ts (3)

7-26: LGTM!

The DTO is well-structured with appropriate validation decorators for all fields.


48-66: LGTM!

Properly validated DTO with all required decorators.


68-80: LGTM!

Properly validated DTO with appropriate decorators.

Comment on lines 28 to 46
export class RegisterOrgWebhhookEndpointDto {
@ApiProperty()
@Transform(({ value }) => trim(value))
@IsNotEmpty({ message: 'orgId is required.' })
@IsString({ message: 'orgId must be in string format.' })
orgId: string;

@ApiProperty()
@Transform(({ value }) => trim(value))
@IsNotEmpty({ message: 'orgId is required.' })
@IsString({ message: 'orgId must be in string format.' })
orgId: string;

@ApiProperty()
@Transform(({ value }) => trim(value))
@IsNotEmpty({ message: 'notificationWebhook is required.' })
@IsString({ message: 'notificationWebhook must be in string format.' })
@IsUrl({
// eslint-disable-next-line camelcase
require_protocol: true, // require URL protocol (e.g., http:// or https://)
// eslint-disable-next-line camelcase
require_tld: true // require top-level domain (e.g., .com, .net)

})
notificationWebhook: string;
@ApiProperty()
@Transform(({ value }) => trim(value))
@IsNotEmpty({ message: 'notificationWebhook is required.' })
@IsString({ message: 'notificationWebhook must be in string format.' })
@IsUrl({
// eslint-disable-next-line camelcase
require_protocol: true, // require URL protocol (e.g., http:// or https://)
// eslint-disable-next-line camelcase
require_tld: true // require top-level domain (e.g., .com, .net)
})
notificationWebhook: string;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

rg -n 'RegisterOrgWebhhookEndpointDto' --type ts --type js

Repository: credebl/platform

Length of output: 627


🏁 Script executed:

rg -n 'Webhhook' --type ts --type js

Repository: credebl/platform

Length of output: 1097


🏁 Script executed:

# Also check for the correctly spelled variant to see if both exist
rg -n 'RegisterOrgWebhookEndpointDto' --type ts --type js

Repository: credebl/platform

Length of output: 42


Typo in class name: "Webhhook" should be "Webhook".

The class name contains a double 'h' that must be corrected. This typo appears in the class definition and is propagated across the codebase in imports, parameters, and method signatures in notification.service.ts and notification.controller.ts. Update the class name and all related references for consistency.

Proposed fix
-export class RegisterOrgWebhhookEndpointDto {
+export class RegisterOrgWebhookEndpointDto {
📝 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
export class RegisterOrgWebhhookEndpointDto {
@ApiProperty()
@Transform(({ value }) => trim(value))
@IsNotEmpty({ message: 'orgId is required.' })
@IsString({ message: 'orgId must be in string format.' })
orgId: string;
@ApiProperty()
@Transform(({ value }) => trim(value))
@IsNotEmpty({ message: 'orgId is required.' })
@IsString({ message: 'orgId must be in string format.' })
orgId: string;
@ApiProperty()
@Transform(({ value }) => trim(value))
@IsNotEmpty({ message: 'notificationWebhook is required.' })
@IsString({ message: 'notificationWebhook must be in string format.' })
@IsUrl({
// eslint-disable-next-line camelcase
require_protocol: true, // require URL protocol (e.g., http:// or https://)
// eslint-disable-next-line camelcase
require_tld: true // require top-level domain (e.g., .com, .net)
})
notificationWebhook: string;
@ApiProperty()
@Transform(({ value }) => trim(value))
@IsNotEmpty({ message: 'notificationWebhook is required.' })
@IsString({ message: 'notificationWebhook must be in string format.' })
@IsUrl({
// eslint-disable-next-line camelcase
require_protocol: true, // require URL protocol (e.g., http:// or https://)
// eslint-disable-next-line camelcase
require_tld: true // require top-level domain (e.g., .com, .net)
})
notificationWebhook: string;
}
export class RegisterOrgWebhookEndpointDto {
@ApiProperty()
@Transform(({ value }) => trim(value))
@IsNotEmpty({ message: 'orgId is required.' })
@IsString({ message: 'orgId must be in string format.' })
orgId: string;
@ApiProperty()
@Transform(({ value }) => trim(value))
@IsNotEmpty({ message: 'notificationWebhook is required.' })
@IsString({ message: 'notificationWebhook must be in string format.' })
@IsUrl({
// eslint-disable-next-line camelcase
require_protocol: true, // require URL protocol (e.g., http:// or https://)
// eslint-disable-next-line camelcase
require_tld: true // require top-level domain (e.g., .com, .net)
})
notificationWebhook: string;
}
🤖 Prompt for AI Agents
In @apps/api-gateway/src/notification/dtos/notification.dto.ts around lines 28 -
46, Rename the DTO class RegisterOrgWebhhookEndpointDto to
RegisterOrgWebhookEndpointDto (fix the double 'h') and update all usages and
imports that reference this symbol (e.g., in notification.service.ts,
notification.controller.ts, and any other files importing or typing it); ensure
the exported class name, its import specifiers, parameter types, and any
ctor/validator references are adjusted consistently so compilation and runtime
references match the new RegisterOrgWebhookEndpointDto identifier.

Comment on lines +101 to +107
async registerHolderNotification(
@Body() registerHolderForNotificationDto: RegisterHolderForNotificationDto,
@Res() res: Response
): Promise<Response> {
const registerNotificationdata = await this.notificationService.registerHolderNotification(
registerHolderForNotificationDto
);

@coderabbitai coderabbitai Bot Jan 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's read the notification controller file to see the actual implementation
cat -n apps/api-gateway/src/notification/notification.controller.ts | head -120

Repository: credebl/platform

Length of output: 5082


🏁 Script executed:

# Search for auth guards in the codebase
rg "AuthGuard|UseGuards|JwtAuthGuard" --type ts -C3 | head -100

Repository: credebl/platform

Length of output: 9790


🏁 Script executed:

# Check if there's a global auth middleware or configuration
rg "middleware|auth|guard" apps/api-gateway/src --type ts -l | head -20

Repository: credebl/platform

Length of output: 1208


🏁 Script executed:

# Look for the controller class definition to see class-level decorators
rg -A5 "@Controller\(" apps/api-gateway/src/notification/notification.controller.ts

Repository: credebl/platform

Length of output: 348


Add authentication guard to this endpoint.

This endpoint accepts sensitive data (FCM token, holder DID, session ID) without authentication protection. All three endpoints in the notification controller currently lack @UseGuards, unlike other controllers in the codebase that use @UseGuards(AuthGuard('jwt')) for similar operations. Add appropriate authentication to prevent unauthorized registration of notification tokens.

🤖 Prompt for AI Agents
In @apps/api-gateway/src/notification/notification.controller.ts around lines
101 - 107, The registerHolderNotification endpoint in NotificationController is
missing authentication; add the @UseGuards(AuthGuard('jwt')) decorator to the
registerHolderNotification method (and similarly protect the other notification
endpoints in this controller) and import UseGuards from @nestjs/common and
AuthGuard from @nestjs/passport so the route requires a valid JWT before
accepting FCM token, holder DID, and session ID; keep the existing method
signature but ensure the guard is applied at the method or controller level to
enforce authentication.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Adding this in future PR

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@RinkalBhojani, understood! Since this is a security-related enhancement, would you like me to create a GitHub issue to track adding authentication guards to the notification endpoints? This will help ensure it doesn't get lost for the follow-up PR.

Comment thread apps/notification/src/holder-notification.repository.ts
Comment thread apps/notification/src/holder-notification.repository.ts
Signed-off-by: Rinkal Bhojani <rinkal.bhojani@ayanworks.com>
@sonarqubecloud

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In
@libs/prisma-service/prisma/migrations/20260112075514_added_holder_notification_table/migration.sql:
- Around line 8-10: The timestamp types are inconsistent: "createDateTime" and
"lastChangedDateTime" use TIMESTAMPTZ(6) but "deletedAt" uses TIMESTAMP(6);
change the "deletedAt" column to TIMESTAMPTZ(6) to make it timezone-aware and
consistent with createDateTime and lastChangedDateTime (preserve the nullable
semantics if any) so all three columns use the same timezone-aware type.
🧹 Nitpick comments (2)
libs/prisma-service/prisma/migrations/20260112075514_added_holder_notification_table/migration.sql (2)

7-7: Consider adding a CHECK constraint for the state column.

Based on the AI summary, there's a NotificationStatus enum defined in libs/enum/src/enum.ts. Using a CHECK constraint would enforce valid state values at the database level, preventing invalid data from being inserted.

Example constraint
     "state" TEXT NOT NULL,
+    CONSTRAINT "holder_notification_state_check" CHECK ("state" IN ('pending', 'sent', 'failed'))

Adjust the allowed values based on your NotificationStatus enum.


15-16: Unique index on sessionId looks good.

The unique constraint aligns with the PR objective. Depending on your query patterns, you may also want to consider adding an index on holderDid if you frequently query notifications by holder, or on fcmToken if you need to look up or deduplicate by token.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bbd9afe and a2fd611.

📒 Files selected for processing (3)
  • apps/notification/src/holder-notification.repository.ts
  • libs/prisma-service/prisma/migrations/20260112075514_added_holder_notification_table/migration.sql
  • libs/prisma-service/prisma/schema.prisma
🚧 Files skipped from review as they are similar to previous changes (2)
  • libs/prisma-service/prisma/schema.prisma
  • apps/notification/src/holder-notification.repository.ts

Comment on lines +8 to +10
"createDateTime" TIMESTAMPTZ(6) NOT NULL DEFAULT CURRENT_TIMESTAMP,
"lastChangedDateTime" TIMESTAMPTZ(6) NOT NULL DEFAULT CURRENT_TIMESTAMP,
"deletedAt" TIMESTAMP(6),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Inconsistent timestamp types: deletedAt uses TIMESTAMP while others use TIMESTAMPTZ.

createDateTime and lastChangedDateTime are timezone-aware (TIMESTAMPTZ), but deletedAt is timezone-naive (TIMESTAMP). This inconsistency can cause subtle bugs when comparing timestamps or when the application runs in different timezones.

Proposed fix
-    "deletedAt" TIMESTAMP(6),
+    "deletedAt" TIMESTAMPTZ(6),
📝 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
"createDateTime" TIMESTAMPTZ(6) NOT NULL DEFAULT CURRENT_TIMESTAMP,
"lastChangedDateTime" TIMESTAMPTZ(6) NOT NULL DEFAULT CURRENT_TIMESTAMP,
"deletedAt" TIMESTAMP(6),
"createDateTime" TIMESTAMPTZ(6) NOT NULL DEFAULT CURRENT_TIMESTAMP,
"lastChangedDateTime" TIMESTAMPTZ(6) NOT NULL DEFAULT CURRENT_TIMESTAMP,
"deletedAt" TIMESTAMPTZ(6),
🤖 Prompt for AI Agents
In
@libs/prisma-service/prisma/migrations/20260112075514_added_holder_notification_table/migration.sql
around lines 8 - 10, The timestamp types are inconsistent: "createDateTime" and
"lastChangedDateTime" use TIMESTAMPTZ(6) but "deletedAt" uses TIMESTAMP(6);
change the "deletedAt" column to TIMESTAMPTZ(6) to make it timezone-aware and
consistent with createDateTime and lastChangedDateTime (preserve the nullable
semantics if any) so all three columns use the same timezone-aware type.

@RinkalBhojani
RinkalBhojani merged commit eb18631 into main Jan 12, 2026
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.

2 participants