Task#0000 To Make User Data Anonymized - #792
Open
Tusharmahajan12 wants to merge 11 commits into
Open
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis change adds an authenticated batch anonymization endpoint. It validates up to 100 email addresses, processes them with bounded concurrency, updates Keycloak and PostgreSQL, clears PII, synchronizes Elasticsearch, and reports per-email outcomes. ChangesUser anonymization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant UserController
participant PostgresUserService
participant Keycloak
participant PostgreSQL
participant Elasticsearch
UserController->>PostgresUserService: Submit validated emails and reason
PostgresUserService->>Keycloak: Obtain shared token
PostgresUserService->>Keycloak: Replace identity and invalidate sessions
PostgresUserService->>PostgreSQL: Archive user and clear PII
PostgresUserService->>Elasticsearch: Synchronize anonymized fields
PostgresUserService-->>UserController: Return ordered per-email outcomes
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
src/user/dto/user-anonymize.dto.ts (1)
24-30: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winBound the length of
reason.
reasonis persisted toUser.reason. That column is declared as@Column({ nullable: true }), which maps to a boundedvarcharin Postgres. A longreasonthen fails at the database layer, after Keycloak and session invalidation already ran. Add@MaxLengthto reject the value during validation.♻️ Proposed refactor
- `@IsString`() - `@IsNotEmpty`() - reason: string; + `@IsString`() + `@IsNotEmpty`() + `@MaxLength`(255) + reason: string;Add
MaxLengthto theclass-validatorimport list.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/user/dto/user-anonymize.dto.ts` around lines 24 - 30, Update the reason field in the anonymization DTO with a class-validator `@MaxLength` constraint matching the User.reason database limit, and add MaxLength to the existing class-validator imports so oversized values are rejected before persistence.src/user/user.controller.ts (1)
322-322: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the response message constant.
Line 322 repeats the literal
'Users processed for anonymization.'. The same text now exists asAPI_RESPONSES.USER_ANONYMIZE_SUCCESSFULLYinsrc/common/utils/response.messages.ts, andPostgresUserService.anonymizeUsersreturns that constant. Reference the constant so the documented description and the runtime message stay identical.♻️ Proposed refactor
- `@ApiOkResponse`({ description: 'Users processed for anonymization.' }) + `@ApiOkResponse`({ description: API_RESPONSES.USER_ANONYMIZE_SUCCESSFULLY })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/user/user.controller.ts` at line 322, Update the `@ApiOkResponse` decorator in the user controller to use API_RESPONSES.USER_ANONYMIZE_SUCCESSFULLY instead of the duplicated literal, keeping the documented description aligned with PostgresUserService.anonymizeUsers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/adapters/postgres/user-adapter.ts`:
- Around line 2972-2977: The documentation for anonymization must match the
Keycloak behavior: in src/adapters/postgres/user-adapter.ts lines 2972-2977,
remove “no session invalidation” and state that Keycloak sessions are
invalidated while the account’s enabled status remains unchanged; in
src/user/user.controller.ts lines 314-315, add session invalidation to the
Keycloak scope or remove the implementation detail and refer readers to the
adapter.
- Around line 3086-3102: The account-deletion flow currently updates Keycloak
before its Postgres writes, leaving systems inconsistent when database
operations fail. In the surrounding deletion method, reorder the operations so
the Postgres changes execute atomically in a transaction before
updateUserInKeyCloak; if retaining the current order, add compensation that
restores the Keycloak profile on Postgres failure and reports whether
restoration succeeded.
- Around line 3064-3071: Update the user lookup in the anonymization flow to use
the repository’s multi-row query instead of findOne, then iterate over every
matching user and aggregate each row’s outcome into the returned result.
Preserve the NOT_FOUND response when no users match, and ensure duplicate email
records cannot leave unanonymized PII behind.
- Around line 3117-3131: Update the account-anonymization flow around
updateBasicUserDetails to replace gender: null with a permitted User.gender enum
value, avoiding the non-null database constraint; also change the corresponding
gender value in the Elasticsearch payload so the index matches the persisted
user data.
- Around line 3008-3018: Bound concurrency in the batch around
anonymizeSingleUser instead of passing all emails directly to
Promise.allSettled. Process emails in sequential chunks with a small fixed batch
size, awaiting each chunk’s allSettled results before starting the next, while
preserving per-user failure isolation and collecting the existing settlement
outcomes.
In `@src/common/utils/keycloak.adapter.util.ts`:
- Around line 378-388: Set an explicit 10-second timeout on the Axios request
configuration used by the logout call, matching fetchNewToken. Also add the same
timeout to the existing configuration in updateUserInKeyCloak so both Keycloak
requests fail promptly when the service is unresponsive.
In `@src/common/utils/pii-fields.constant.ts`:
- Around line 11-30: Update isPiiCustomField and PII_CUSTOM_FIELD_NAME_PATTERNS
to normalize separator and casing differences so configured snake_case patterns
match camelCase names such as addressLine1 and postCode, while avoiding broad
substring matches such as mobile_notifications_opt_in and whatsapp_consent.
Replace the fixed pattern-only behavior with tenant-configurable exact-name or
explicitly bounded matching, preserving false for unrelated fields.
In `@src/user/dto/user-anonymize.dto.ts`:
- Around line 18-22: Normalize each value in the UserAnonymize DTO’s emails
field to lowercase during validation transformation, before
PostgresUserService.anonymizeSingleUser performs the exact email lookup.
Preserve the existing array and email validations so submitted mixed-case
addresses resolve to the lowercase values stored by saveUserToDatabase.
In `@src/user/user.controller.ts`:
- Around line 316-331: Update the anonymizeUsers route to apply RbacAuthGuard
before JwtAuthGuard, ensuring the RBAC policy rejects non-administrative roles
before anonymization executes; leave the handler and existing validation
unchanged.
---
Nitpick comments:
In `@src/user/dto/user-anonymize.dto.ts`:
- Around line 24-30: Update the reason field in the anonymization DTO with a
class-validator `@MaxLength` constraint matching the User.reason database limit,
and add MaxLength to the existing class-validator imports so oversized values
are rejected before persistence.
In `@src/user/user.controller.ts`:
- Line 322: Update the `@ApiOkResponse` decorator in the user controller to use
API_RESPONSES.USER_ANONYMIZE_SUCCESSFULLY instead of the duplicated literal,
keeping the documented description aligned with
PostgresUserService.anonymizeUsers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b23ecb49-8c5d-4a53-add4-62255377f463
📒 Files selected for processing (8)
src/adapters/postgres/user-adapter.tssrc/adapters/userservicelocator.tssrc/common/utils/api-id.config.tssrc/common/utils/keycloak.adapter.util.tssrc/common/utils/pii-fields.constant.tssrc/common/utils/response.messages.tssrc/user/dto/user-anonymize.dto.tssrc/user/user.controller.ts
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src/common/utils/pii-fields.constant.ts (2)
22-26: 🔒 Security & Privacy | 🟠 MajorReplace unordered token inclusion with bounded matching.
patternTokens.every((token) => tokens.has(token))matches any field that contains a generic token.mobile_notifications_opt_inmatchesmobile, andaddress_verification_statusmatchesaddresswhen the field type is missing or is not exactly lowercase"checkbox". The anonymizer can clear non-PII data.Use exact normalized field names or explicit bounded combinations. Normalize
fieldTypebefore the set lookup when case variations are valid.Also applies to: 40-54
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/common/utils/pii-fields.constant.ts` around lines 22 - 26, Update the PII field-name matching logic around patternTokens, tokens, and NON_PII_FIELD_TYPES to stop treating unordered token inclusion as a match. Use exact normalized field names or explicitly bounded token combinations so names such as mobile_notifications_opt_in and address_verification_status are not classified as PII. Normalize fieldType before checking NON_PII_FIELD_TYPES, preserving the checkbox exclusion regardless of valid casing.
1-20: 🔒 Security & Privacy | 🟠 MajorSplit letter-digit boundaries in
tokenize.
tokenize("addressLine1")returns["address", "line1"], while theaddress_linepattern requires alinetoken. The detector therefore misses this PII field.tokenize("whatsApp")also returns["whats", "app"], so aSplit letter-digit boundaries and add normalized aliases for compound names. Add regression tests for
addressLine1,postCode, andAlso applies to: 28-34
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/common/utils/pii-fields.constant.ts` around lines 1 - 20, Update tokenize to split both letter-to-digit and digit-to-letter boundaries, then normalize compound tokens so patterns such as address_line, post_code, and whatsapp match addressLine1, postCode, and whatsApp. Extend PII_CUSTOM_FIELD_NAME_PATTERNS with any required normalized aliases, and add regression tests covering all three examples.
🧹 Nitpick comments (1)
src/adapters/postgres/user-adapter.ts (1)
3111-3115: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winOne unexpected rejection discards the sibling rows' outcomes.
anonymizeUserRowreturns status objects for handled failures, butupdateUserInKeyCloak(line 3175) andlogoutUserInKeyCloak(line 3195) run outside atryblock. If either throws,Promise.allrejects immediately. The email then reportsFAILED, even when other rows for the same email were anonymized. The response hides the partial state.Use
Promise.allSettledand map a rejection to aFAILEDrow result.♻️ Proposed refactor
- const rowResults = await Promise.all( - users.map((user) => - this.anonymizeUserRow(user, reason, loggedInUserId, keycloakToken) - ) - ); + const settledRows = await Promise.allSettled( + users.map((user) => + this.anonymizeUserRow(user, reason, loggedInUserId, keycloakToken) + ) + ); + const rowResults = settledRows.map((r) => + r.status === 'fulfilled' + ? r.value + : { + status: 'FAILED', + message: r.reason?.message || 'Anonymization failed', + } + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/adapters/postgres/user-adapter.ts` around lines 3111 - 3115, Update the row aggregation around anonymizeUserRow to use Promise.allSettled instead of Promise.all, mapping rejected promises to FAILED row results while preserving fulfilled status objects. Ensure one rejection from updateUserInKeyCloak or logoutUserInKeyCloak does not discard outcomes from sibling rows or hide partial anonymization state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/adapters/postgres/user-adapter.ts`:
- Around line 3168-3169: Run Prettier on the affected user-adapter code,
including the added anonymization checks and blocks near the referenced symbols,
so quote style and line wrapping match repository formatting rules. Preserve the
existing logic and behavior while applying only formatter changes.
- Around line 3102-3109: Update anonymizeSingleUser to normalize the incoming
email to lowercase before calling usersRepository.find, matching
saveUserToDatabase's storage behavior. Use the normalized value for the lookup
while preserving the existing not-found response and anonymization flow.
- Around line 3211-3227: The anonymization update payload must clear all
remaining PII fields before marking the user archived. In the anonymization
method containing the shown User update, set district, state, address, pincode,
mobile_country_code, deviceId, and auto_tags to their sanitized empty or null
values, and ensure the corresponding Elasticsearch update uses the same values;
otherwise document an explicit permitted-retention reason for each field.
In `@src/user/dto/user-anonymize.dto.ts`:
- Line 2: Apply the repository formatter to the four reported string literals:
use double quotes for the class-transformer import and the "string" literal in
user-anonymize.dto.ts, and for the "checkbox" value and "$1_$2" replacement
string in pii-fields.constant.ts. Review each listed site for Google JavaScript
style and Node.js/NestJS formatting conformity; no other changes are needed.
---
Duplicate comments:
In `@src/common/utils/pii-fields.constant.ts`:
- Around line 22-26: Update the PII field-name matching logic around
patternTokens, tokens, and NON_PII_FIELD_TYPES to stop treating unordered token
inclusion as a match. Use exact normalized field names or explicitly bounded
token combinations so names such as mobile_notifications_opt_in and
address_verification_status are not classified as PII. Normalize fieldType
before checking NON_PII_FIELD_TYPES, preserving the checkbox exclusion
regardless of valid casing.
- Around line 1-20: Update tokenize to split both letter-to-digit and
digit-to-letter boundaries, then normalize compound tokens so patterns such as
address_line, post_code, and whatsapp match addressLine1, postCode, and
whatsApp. Extend PII_CUSTOM_FIELD_NAME_PATTERNS with any required normalized
aliases, and add regression tests covering all three examples.
---
Nitpick comments:
In `@src/adapters/postgres/user-adapter.ts`:
- Around line 3111-3115: Update the row aggregation around anonymizeUserRow to
use Promise.allSettled instead of Promise.all, mapping rejected promises to
FAILED row results while preserving fulfilled status objects. Ensure one
rejection from updateUserInKeyCloak or logoutUserInKeyCloak does not discard
outcomes from sibling rows or hide partial anonymization state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e5701a27-bf79-4d8a-bd67-8fb8fb39e69a
📒 Files selected for processing (3)
src/adapters/postgres/user-adapter.tssrc/common/utils/pii-fields.constant.tssrc/user/dto/user-anonymize.dto.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/common/utils/pii-fields.constant.ts`:
- Around line 12-19: Update the PII_CUSTOM_FIELD_NAME_PATTERNS list to include
the "whatsapp" pattern, ensuring isPiiCustomField anonymizes text fields such as
whatsapp_number and whatsappPhone while the existing checkbox exclusion for
whatsapp_consent remains unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8d43429c-4e42-40ec-8a0d-8c86ff42e610
📒 Files selected for processing (3)
src/adapters/postgres/user-adapter.tssrc/common/utils/pii-fields.constant.tssrc/user/dto/user-anonymize.dto.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/user/dto/user-anonymize.dto.ts
- src/adapters/postgres/user-adapter.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/auth/auth.service.ts`:
- Around line 71-78: Run the configured formatter on the changed conditional and
error-message block in the authentication service, including the strings used to
build errorMessage, so it follows the repository’s double-quote style and clears
the Prettier lint errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3d79f363-8cab-49de-9be4-6bf4bc157e97
📒 Files selected for processing (1)
src/auth/auth.service.ts
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit