USER ID / Login ID Update for Mobile Number Change - #146
Merged
Conversation
…ring Employee ID Edit no longer silently nulls Employee ID on every save — the edit form doesn't submit that field, and the code was unconditionally copying the (missing) value over the existing one. Also checks the new username isn't already taken by another employee before saving, rather than allowing silent duplicates.
CreatedBy/ModifiedBy across AMRIT store the username as a denormalised string rather than a UserID foreign key, so renaming a user has to be propagated by hand. Adds POST /username/renameUsername plus a /preview variant that reports the same per-table row counts without committing. The sweep covers m_user and 28 RMNCH/FLW tables across db_iemr and db_identity, in one transaction so a partial rename cannot be left behind. Column naming was verified against the AMRIT-DB migrations: db_identity is uniformly CreatedBy/ModifiedBy while db_iemr is split between created_by/updated_by and CreatedBy/ModifiedBy. Each table takes two UPDATEs, one per audit column, rather than a single combined statement. Setting both columns off a CreatedBy match would overwrite ModifiedBy on rows a different user last touched, destroying their attribution, and would miss rows this user only modified. Usernames are bound as parameters; only whitelist constants are interpolated. m_user is updated first so a unique-key rejection on UserName or EmployeeID happens before anything else is written, and the new name is capped at 12 characters when contact numbers are included because m_user.ContactNo is varchar(12). Scope is deliberately these tables only — it is not every table carrying an audit column, so the old username survives elsewhere. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Preview timed out behind nginx on UAT (504). Each of the 28 tables was counted with `createdBy = :u OR modifiedBy = :u`, and an OR across two columns cannot use a single-column index on either — so every table was a full scan. Split into two single-column counts, matching the two UPDATEs the rename already issues, so both can use the new indexes in AMRIT-DB V96/V19. Splitting also makes the numbers honest: a row the user both created and last modified is updated by each statement, so counting it once under-reported what rename() goes on to report. Preview now also counts m_user, which it previously skipped while the rename reported it, so the two line up row for row. Corrects the ELIGIBLE_COUPLE_TRACKING entry to eligible_couple_tracking — the schema creates it lowercase, and table names are case-sensitive on Linux MySQL. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…xact" This reverts commit cd20896.
Reworks the sweep to the supplied query shape: each UPDATE now selects the rows to touch by primary key through a derived table rather than filtering on the audit column directly. The SELECT ... AS temp wrapper is what MySQL requires to read from the same table an UPDATE targets (error 1093), and driving by PK keeps the statements usable under safe-update mode. Carries each table's primary key in the registry; all 28 were verified against the AMRIT-DB migrations. Both audit columns are now set off a CreatedBy match, per the supplied queries, replacing the earlier split into one statement per column. Preview counts CreatedBy alone to match, and still reports m_user, so preview and result line up row for row. Keeps eligible_couple_tracking lower case: the schema creates it that way and MySQL table names are case sensitive on Linux. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Removes POST /username/renameUsername/preview along with the service method behind it, the two counting queries it used, and the now-unused preview flag on the response. /username/renameUsername is unchanged and remains the only endpoint. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
EmployeeID was hardcoded to the new username in both branches of the m_user update. It is now only touched when the request supplies a new value, so renaming a username no longer implies changing the Employee ID. The SET clause is built from what was actually asked for. Splits the uniqueness check in two. UserName and EmployeeID hold separate UNIQUE keys on m_user, so each conflicts only with its own column — the previous combined check rejected a new username merely because some row used it as an Employee ID, which was only correct while the two were forced to match. Both checks exclude the row being renamed, so re-entering the value that row already holds is not reported as a conflict with itself. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds POST /username/checkAvailability so the screen can report a duplicate as it is typed rather than at submit. It reuses the same repository checks rename() runs, so the two cannot disagree — including counting soft-deleted rows, which still hold m_user's UNIQUE keys. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both new values are now optional. Each is normalised to null when blank or already equal to what the row holds, and the m_user UPDATE is built from whichever columns actually change — so a username-only change, an employee-ID-only change, or both are all valid. The request is rejected only when neither would change anything. The 28-table audit sweep is skipped entirely when the username is unchanged: CreatedBy/ModifiedBy record the username, so an employee-ID-only change leaves every audit row already correct. That also keeps such a change off the unindexed scan path. Drops the updateEmployeeId flag (supplying a value is the intent) and the /username/checkAvailability endpoint, since the screen now uses the same m/FindEmployeeByName and m/FindEmployeeDetails checks as Employee Master. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Takes userID on the rename request and uses it as the identity of the row being changed. The username is the very thing being renamed, so a string match is a poor way to address it; the primary key is not. The m_user UPDATE is now keyed on UserID, and both uniqueness checks exclude self by UserID rather than by username — the same way the Employee Master edit path does it via findEmployeeByNameForUpdate. The old username is now read from the row rather than trusted from the request, and a mismatch is rejected: if the row was renamed after the screen loaded its list, sweeping on the stale username would repoint the wrong audit records. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The response carried only the username pair, so a caller had no way to see what happened to the employee ID. Adds oldEmployeeId (read before the update) and newEmployeeId. Also adds userNameUpdated/employeeIdUpdated booleans. OutputResponse re-serialises the envelope with a GsonBuilder that has no serializeNulls, so any null field drops out of the JSON entirely — which is why an unset employee ID appears to be missing rather than null. A primitive boolean always survives that pass, so the caller can tell "unchanged" from "absent". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Removes rowsPerTable, tablesAffected and totalRowsAffected. The response now reports only what changed: the username and employee ID before and after, plus the flags saying whether each was written. The total row count is still summed and logged, so the scale of a rename remains traceable without exposing the table list to callers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…top clearing Employee ID" This reverts commit 4f6371f.
Reverts 4f6371f — the editUserDetails uniqueness check, the Employee ID null-guard and findEmployeeByNameForUpdate. Verified independent of the Change Username feature: nothing in the username package called any of it, so the rename API is unaffected. Drops the comment that pointed at the removed query. Note this restores the prior editUserDetails behaviour, including the unconditional setEmployeeID that nulls Employee ID when the edit form does not submit that field. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Pure deletions — no code changed. GPL license headers kept. Co-Authored-By: Claude Opus 5 <noreply@anthropic.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 ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| + "WHERE %4$s IN (SELECT %4$s FROM (SELECT %4$s FROM %1$s WHERE %2$s = :oldUserName) AS temp)", | ||
| table.getQualifiedName(), table.getCreatedByColumn(), table.getModifiedByColumn(), | ||
| table.getPrimaryKeyColumn()); | ||
| Query query = entityManager.createNativeQuery(sql); |
| } | ||
|
|
||
| Query query = entityManager.createNativeQuery( | ||
| "UPDATE db_iemr.m_user SET " + String.join(", ", assignments) + " WHERE UserID = :userID"); |
Log injection (UsernameRenameServiceImpl): the rename start line logged the old/new username and employee ID straight from the request, so a value containing CR/LF could forge log entries. Those values are now flattened to a safe character set and length-capped before logging, and the line also carries the userID, which is an integer and cannot be tampered with. Dynamic SQL (UsernameRenameRepository): schema, table and column names cannot be bound as parameters, so they are interpolated. Each is now checked against a strict SQL-identifier pattern before it can reach a statement, so anything that is not a plain identifier throws instead of being concatenated in. All caller-supplied data was, and remains, bound. Verified every identifier in the table registry satisfies the pattern. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The sanitising helper did not clear Sonar's taint analysis — a custom regex replace is not recognised as a sanitiser, so the rule still traced request data into the logger and the Quality Gate stayed at Security Rating C. Logs the userID and two booleans saying which columns are changing instead. Nothing request-derived reaches the log, so the taint path is gone rather than filtered. The values themselves are still available in the response and in m_user. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comment on lines
+122
to
+124
| Query query = entityManager.createNativeQuery(String.format( | ||
| "SELECT COUNT(*) FROM db_iemr.m_user WHERE %s = :value AND UserID <> :excludeUserID", | ||
| identifier(column))); |
Sonar reported three MAJOR vulnerabilities for dynamically formatted SQL in UsernameRenameRepository, holding New Code at Security Rating C and failing the Quality Gate. Each query call now receives a constant rather than a formatted string: - the two uniqueness counts become two fixed queries instead of one with an interpolated column name - the m_user update selects one of five constants covering the possible combinations, instead of joining a SET clause at runtime - the per-table rename SQL is built once in AuditTable's constructor and read back through a getter, so the query call takes a plain value Identifier validation moves to AuditTable, where the identifiers now live, and rejects anything that is not a plain SQL identifier. The generated SQL is unchanged: verified byte-identical for the per-table rename and for all eight combinations of the m_user update. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
1 task
vishwab1
approved these changes
Sep 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📋 Description
JIRA ID:
AMM-2434
✅ Type of Change
ℹ️ Additional Information
Please describe how the changes were tested, and include any relevant screenshots, logs, or other information that provides additional context.