feat: replace ID with UID

[sabhas]

BREAKING CHANGE: remove auto incremental ids from user, group and permissions and add a virtual uid property that returns string value of documents object id

Issue

Closes #361.

While addressing #359, the auto-incrementing sequence counter used for
User, Group, and Permission IDs was found to be broken on Cosmos DB
(it set every ID to 1). More generally, sequencing via a shared counter
document is not a good fit: it's a bottleneck on every
insert and a source of race conditions under concurrent writes.

Intent

Replace auto-incremental numeric IDs with a string uid across User,
Group, and Permission, and make the API consistently expose uid
instead of id wherever these entities appear in a response.

Implementation

• Added a virtual uid property to the User, Group, and Permission
  Mongoose models, returning this._id.toString(). No new field is stored
  and no data migration is required — every document already has _id,
  old and new alike.
• Removed the old sequencing mechanism entirely: the Counter model and
  getSequenceNextValue utility are gone, along with the bottleneck/race
  condition they caused.
• Updated all API responses that expose a user, group, or permission
  identifier — including login (POST /SASLogon/login), session
  (GET /SASjsApi/session), and the user/group/permission endpoints
  — to consistently return uid.
• Updated the web frontend (permission management, user profile, session
  restoration on page load) to read uid instead of id.

This is a breaking change for any existing client relying on numeric
IDs from this API — endpoints now return an opaque string identifier
instead.

Checks

• Code is formatted correctly (npm run lint:fix).
• Any new functionality has been unit tested.
• All unit tests are passing (npm test).
• All CI checks are green.
• Reviewer is assigned.

[YuryShkoda]

see comments above

[4gl-reviewer]

Hermes Agent Code Review

Verdict: Request Changes

This PR replaces numeric auto-increment IDs with MongoDB _id-based UIDs (24-char hex strings) across the entire server codebase. The core approach is sound, but there are several critical issues that need to be addressed before merging.

Critical

1. permission.ts line 341 — select: 'groupId name description' not updated to 'uid name description': In updatePermission, the group populate select still says groupId while every other select in the PR was changed to uid. The response will have a missing/null uid for the group object on PATCH permission. Should be .populate({ path: 'group', select: 'uid name description' }).

2. desktop.ts line 6 — regex /^\/SASjsApi\/user\/[0-9]*$/ only matches numeric IDs: UIDs are now 24-char hex strings. In desktop mode, GET/PATCH to /SASjsApi/user/{uid} will be blocked by desktopRestrict, breaking desktop user profile access entirely. This line wasn't modified in the PR but is now broken by the ID→UID change. Update to /^\/SASjsApi\/user\/[0-9a-fA-F]{24}$/.

3. seedDB.ts — ALL_USERS_GROUP name changed from 'AllUsers' to 'all-users' without migration: Existing deployments will get a duplicate group — new users join 'all-users' while existing users remain in 'AllUsers'. Permissions referencing the old group name are orphaned. The comment on line 27 still says 'AllUsers'. Either add a migration to rename the existing group, or keep the original name.

4. Merge regression reverting PR #388: The issue-361 branch was based on a commit before the #388 fix (return 200 with log on SAS session failure). The merge into main reverted the fix — processProgram.ts now throws on SessionState.failed instead of resolving, Execution.ts wraps it in SessionExecutionError producing a 400, and tests were reverted to expect the old throwing behavior.

Warnings

5. swagger.yaml SessionResponse still uses id not uid — Code returns uid but committed swagger shows id. Same for /SASLogon/login user object.
6. Inconsistent param naming in UserController — getUser uses @Path() uid but updateUser/deleteUser use @Path() userId with stale @example userId 1234.
7. Missing uidValidation on POST /:groupUid/:userUid and DELETE /:groupUid/:userUid routes in group.ts.
8. group.ts @example userId "12ByteString" should be @example userUid for the addUserToGroup method.

Looks Good

• Core approach of using MongoDB _id as uid virtual is sound
• Clean removal of Counter model and getSequenceNextValue
• Good uidValidation with Joi (24-char hex) security addition
• Frontend changes are consistent and complete
• Test files properly updated throughout

Reviewed by Hermes Agent (GitHub App)

[4gl-reviewer]

Critical: The group populate select here still says 'groupId name description', but every other select in this PR was changed to 'uid name description'. On a PATCH /SASjsApi/permission response, the group object will have a missing/null uid field. Should be:

.populate({ path: 'group', select: 'uid name description' })

Reviewed by Hermes Agent (GitHub App)

[4gl-reviewer]

Critical: The ALL_USERS_GROUP name was changed from 'AllUsers' to 'all-users' without a migration. Existing deployments will get a duplicate group — new users join 'all-users' while existing users remain in 'AllUsers'. Permissions referencing the old group name are orphaned.

Also, the comment on line 27 still says 'AllUsers'.

Either add a migration to rename the existing group, or keep the original name.

Reviewed by Hermes Agent (GitHub App)

[4gl-reviewer]

Hermes Agent Code Review

Verdict: Request Changes

This is a well-conceived refactor that replaces numeric auto-increment IDs with virtual uid fields backed by MongoDB _id (ObjectId). The virtual-uid-on-_id approach is sound and requires zero data migration for the ID swap itself. However, there are 4 critical bugs that will break runtime behavior or silently break existing deployments.

Critical

• C1: updatePermission still selects the removed groupId field (api/src/controllers/permission.ts:341): The populate select: 'groupId name description' references a field that no longer exists on the Group model (replaced by the uid virtual). Every other populate in this PR was updated to 'uid name description' — this one was missed. PATCH on group-based permissions returns a broken group object. No test covers PATCH on a group-typed permission, which is why CI is green. (Inline comment posted on this line.)

• C2: Desktop mode regex rejects hex UIDs (api/src/middlewares/desktop.ts:6): regexUser = /^\/SASjsApi\/user\/[0-9]*$/ only matches numeric IDs. UIDs are 24-character hex strings (e.g., 507f1f77bcf86cd799439011) containing a-f, which won't match. In Desktop mode, GET /SASjsApi/user/<uid> and PATCH will return 403 — a hard regression for every Desktop-mode install. No spec exercises Desktop mode. Fix: const regexUser = /^\/SASjsApi\/user\/[0-9a-fA-F]{24}$/

• C3: Leftover .id references in tests (user.spec.ts:240,244,251,255, auth.spec.ts:239): These LDAP-related test cases still use dbUser!.id / currentUser.id instead of .uid. The User model no longer has an id field. The auth.spec.ts:239 case is a false-positive test — verifyTokenInDB(currentUser.id, ...) returns undefined, matching toBeUndefined(), but no longer validates that the token was actually removed for that user. Also user.spec.ts:267 uses hardcoded /SASjsApi/user/1234 which now fails uidValidation with 400, not the expected 401.

• C4: PUBLIC_GROUP_NAME renamed 'Public'→'public' and ALL_USERS_GROUP 'AllUsers'→'all-users' — breaks existing deployments: isPublicRoute.ts queries Group.findOne({ name: PUBLIC_GROUP_NAME }). For any existing database seeded under the old name 'Public', this returns null and every public-route permission grant silently stops working on upgrade. seedDB won't find the old 'AllUsers' group and will create a second orphan group. The PR body claims "no data migration is required" — true for the ID→UID swap, but the group-name rename is an undocumented breaking change. Either revert the renames or add a migration step. (Inline comments posted on both lines.)

Warnings

• W1: verifyAdminIfNeeded relies on req.params.uid — tightly coupled to the literal param name. Consider req.params.uid ?? req.params.userId.
• W2: getPreProgramVariables fallback userId: 'demoId' is not a valid ObjectId. Use '000000000000000000000000'.
• W3: publicUser.userId = 'public_user_id' and desktop '1234'/'12345' are sentinel strings, not valid UIDs — add clarifying comments.
• W4: permission.ts:176 JSDoc says @param permissionId The user's identifier — copy-paste from user controller, doesn't match @Path() uid. (Inline comment posted.)

Suggestions

• S1: Extract a shared populateGroup helper — the group-populate string is duplicated in 4 places. C1 happened because one copy drifted.
• S2: Add a test for PATCH permission on a group principal, and at least one Desktop-mode test.
• S3: addUserToGroup/removeUserFromGroup routes don't run uidValidation on groupUid/userUid params.
• S4: crypto.ts is fine security-wise — randomBytes is the correct CSPRNG.
• S5: SessionResponse dropped needsToUpdatePassword from required in Swagger — confirm frontend handles its absence.

Looks Good

• Virtual uid over _id is the right design — no stored duplicate, no migration for the ID swap.
• Counter.ts and getSequenceNextValue.ts removal is clean — zero lingering references.
• uidValidation (Joi.string().length(24).hex().required()) is correctly applied to route params.
• Auth middleware chain is intact — no auth-bypass introduced.
• Frontend migration is complete and consistent across all components.
• GroupAction enum replacing magic strings is a clean improvement.

Reviewed by Hermes Agent (GitHub App)

[4gl-reviewer]

Critical (C1): select: 'groupId name description' — groupId no longer exists on the Group model (replaced by the uid virtual in this PR). Every other populate was updated to 'uid name description' (e.g., Permission.ts:70, the user populate at line 338). This one was missed.

Fix:

select: 'uid name description'

No test covers PATCH on a group-typed permission, which is why this slipped through CI.

[4gl-reviewer]

Critical (C2) — related to this change: the desktopUser.userId is now '12345' (string), but the regexUser at line 6 (/^\/SASjsApi\/user\/[0-9]*$/) only matches numeric IDs. UIDs are 24-character hex strings (e.g., 507f1f77bcf86cd799439011) containing a-f, which won't match [0-9]*.

In Desktop mode, GET /SASjsApi/user/<uid> and PATCH will return 403. Fix the regex at line 6:

const regexUser = /^\/SASjsApi\/user\/[0-9a-fA-F]{24}$/

Or relax to /^\/SASjsApi\/user\/[\w-]+$/ to also tolerate '12345'.

[4gl-reviewer]

Critical (C4): Renaming PUBLIC_GROUP_NAME from 'Public' to 'public' breaks existing deployments. isPublicRoute.ts queries Group.findOne({ name: PUBLIC_GROUP_NAME }) — for any database seeded under the old name 'Public', this returns null and every public-route permission grant silently stops working on upgrade.

Either revert to 'Public' or add a migration step in seedDB.

[4gl-reviewer]

Critical (C4): Renaming ALL_USERS_GROUP.name from 'AllUsers' to 'all-users' breaks existing deployments — seedDB won't find the old 'AllUsers' group and will create a second orphan group.

Also: groupnameSchema is Joi.string().lowercase().alphanum() — 'all-users' contains a hyphen (not alphanum). seedDB bypasses Joi so this won't error, but API-created groups can't use this name format.

[4gl-reviewer]

Warning (W4): JSDoc says @param permissionId The user's identifier — copy-paste from user controller. The param is now uid (via @Path() uid), not permissionId. Same issue on updatePermission.

[4gl-reviewer]

groupId no longer exists on the Group model after this migration — the model now exposes a uid virtual (stringified _id). Every other populate in this file was updated to uid, but this one in updatePermission was missed, so the returned group object will be missing the uid field the frontend expects.

Suggested change

	select: 'groupId name description'
	.populate({ path: 'group', select: 'uid name description' })

[4gl-reviewer]

@Get('{uid}') was renamed in this PR, but @Patch('{userId}') and @Delete('{userId}') below were not. tsoa will generate two separate swagger paths (/SASjsApi/user/{uid} and /SASjsApi/user/{userId}), which is inconsistent and confusing for API consumers. The param name is cosmetic (both map to _id), but for a clean contract they should all be {uid}.

Suggested change

	@Patch('{userId}')
	@Patch('{uid}')

[4gl-reviewer]

@Delete('{userId}') should also be renamed to {uid} for consistency with @Get('{uid}') (same as the @Patch above). Additionally the JSDoc @example userId 1234 above this decorator still uses a numeric example even though userId is now a hex UID string — update it to e.g. "507f1f77bcf86cd799439011".

[4gl-reviewer]

Hermes Agent Code Review

Verdict: Request Changes

Migrating numeric auto-increment IDs to stringified MongoDB ObjectId UIDs is a sound approach — collision-resistant, unpredictable, no auth-bypass risk — and the bulk of the migration (models, controllers, middleware, tests, frontend) is consistently applied. Counter.ts and getSequenceNextValue.ts are cleanly removed with no dangling references. A few real bugs and inconsistencies remain.

Critical

1. permission.ts:341 — broken group populate in updatePermission. .populate({ path: 'group', select: 'groupId name description' }) still references groupId, which no longer exists on the Group model (now a uid virtual). Every other populate in the file was updated to uid; this one was missed, so the updatePermission response returns a group object missing the uid field the frontend expects. Fix: select: 'uid name description'. (Inline comment posted.)

2. desktop.ts:6 — allow-list regex no longer matches UIDs. const regexUser = /^\/SASjsApi\/user\/[0-9]*$/ only matches numeric path segments, but UIDs are now 24-char hex strings. In desktop mode (MODE=desktop), GET /SASjsApi/user/{uid} and PATCH /SASjsApi/user/{uid} will fail the allow-list and be rejected with 403, breaking desktop mode for user routes. The PR touched this file (userId: 12345 → userId: '12345') but didn't update the regex. Fix: /^\/SASjsApi\/user\/[0-9a-f]{24}$/i.

Warnings

3. Swagger path param inconsistency (user.ts). @Get('{uid}') was renamed but @Patch('{userId}') and @Delete('{userId}') were not. tsoa generates two separate swagger entries (/SASjsApi/user/{uid} and /SASjsApi/user/{userId}). The param name is cosmetic (both map to _id), but unify to {uid} for a clean contract. (Inline comment posted.)

4. Stale JSDoc examples. @Delete still has @example userId 1234 (numeric) and @Patch has @example userId "1234" even though the param is now a 24-char hex UID string. Update to a representative value. (Inline comment posted.)

5. No data-migration handling. There's no migration script for existing documents. Old rows retain orphaned id/groupId/permissionId fields, old unique indexes may persist, and the Counter collection is orphaned. External clients persisting numeric IDs will break. If migrations are handled externally, that's fine — but it should be documented in the PR description / release notes.

6. Missing uidValidation on group add/remove-user routes. routes/api/group.ts applies uidValidation to /:uid GET/DELETE but not to the /:groupUid/:userUid POST/DELETE add/remove-user routes. Mongoose rejects invalid ObjectIds safely, so this isn't a security hole, but it's inconsistent.

Suggestions

7. publicUser.userId = 'public_user_id' is not a valid ObjectId (was 0 before, so pre-existing) — fine, but worth a comment so future readers don't assume it's an ObjectId.

Looks Good

• UID = stringified ObjectId: correct, collision-resistant, unpredictable — no auth-bypass concern.
• All findOne({ id: ... }) → findOne({ _id: ... }) applied consistently across controllers, middleware, utils.
• verifyAdminIfNeeded correctly updated to read req.params.uid.
• uidValidation uses Joi.string().length(24).hex() — correct for ObjectId format.
• Tests comprehensively updated with hex-string fixtures across all spec files.
• Frontend correctly migrated: id→uid, groupId→uid, permissionId→uid across components/hooks/types; appContext.tsx made setters optional with null-check in profile.tsx.
• Counter.ts / getSequenceNextValue.ts removed with zero dangling references.

Reviewed by Hermes Agent (GitHub App)

[4gl-reviewer]

Hermes Agent Code Review

Verdict: Request Changes

This PR replaces numeric auto-increment IDs with MongoDB ObjectId-based uid virtuals across the API and web frontend. The migration is thorough, but there are several bugs that will cause runtime failures.

Critical

1. permission.ts:341 — group populate still selects groupId which was removed from the Group model. In updatePermission(): select: 'groupId name description'. Every other group populate was updated to uid; this one was missed. Should be 'uid name description'.

2. permission.ts:300 — populate select excludes _id but uid is a virtual derived from _id. select: 'uid username displayName isAdmin -_id' — with -_id, the virtual uid (which returns this._id.toString()) will be undefined. Remove -_id; the toJSON transform already strips _id from JSON output.

Warnings

3. user.ts:163,195 — tsoa decorators @Patch('{userId}') and @Delete('{userId}') were not updated to {uid}. Express routes use /:uid, but the decorators still say {userId}, producing inconsistent swagger docs (GET uses {uid}, PATCH/DELETE use {userId}).

4. Breaking change: PUBLIC_GROUP_NAME changed from 'Public' to 'public' and ALL_USERS_GROUP.name from 'AllUsers' to 'all-users'. Existing deployments will have orphaned groups.

Suggestions

5. CI only runs lint/build — no test step. The bugs above would only surface at runtime with a real MongoDB.

Reviewed by Hermes Agent (GitHub App)

[4gl-reviewer]

Bug: select: 'groupId name description' — groupId was removed from the Group model in this PR. Every other group populate was updated to uid, but this one in updatePermission() was missed. Change to 'uid name description'.

[4gl-reviewer]

Bug: select: 'uid username displayName isAdmin -_id' — the -_id excludes _id from the query, but uid is a virtual that returns this._id.toString(). With _id excluded, uid will be undefined. Remove -_id — the toJSON transform on the User model already strips _id from JSON output.

[4gl-reviewer]

Inconsistency: The tsoa decorator is still @Patch('{userId}') but the express route uses /:uid. This generates swagger docs showing /SASjsApi/user/{userId} for PATCH while GET uses {uid}. Update to @Patch('{uid}') and @Path() uid: string.

[4gl-reviewer]

Inconsistency: Same as above — @Delete('{userId}') should be @Delete('{uid}') with @Path() uid: string.

[4gl-reviewer]

Breaking change: PUBLIC_GROUP_NAME changed from 'Public' to 'public'. Existing deployments with a group named Public will have it orphaned. Consider a migration script.

[4gl-reviewer]

Hermes Agent Code Review

Verdict: Request Changes

This PR replaces numeric auto-increment IDs with MongoDB ObjectId-based uid virtuals across the API and web frontend. The migration is thorough, but there are several bugs that will cause runtime failures.

Critical

1. permission.ts:341 — group populate still selects groupId which was removed from the Group model. In updatePermission(): select: 'groupId name description'. Every other group populate was updated to uid; this one was missed. Should be 'uid name description'.

2. permission.ts:300 — populate select excludes _id but uid is a virtual derived from _id. select: 'uid username displayName isAdmin -_id' — with -_id, the virtual uid (which returns this._id.toString()) will be undefined. Remove -_id; the toJSON transform already strips _id from JSON output.

Warnings

3. user.ts:163,195 — tsoa decorators @Patch('{userId}') and @Delete('{userId}') were not updated to {uid}. Express routes use /:uid, but the decorators still say {userId}, producing inconsistent swagger docs (GET uses {uid}, PATCH/DELETE use {userId}).

4. Breaking change: PUBLIC_GROUP_NAME changed from 'Public' to 'public' and ALL_USERS_GROUP.name from 'AllUsers' to 'all-users'. Existing deployments will have orphaned groups.

Suggestions

5. CI only runs lint/build — no test step. The bugs above would only surface at runtime with a real MongoDB.

Reviewed by Hermes Agent (GitHub App)

[4gl-reviewer]

Bug: select: 'groupId name description' — groupId was removed from the Group model in this PR. Every other group populate was updated to uid, but this one in updatePermission() was missed. Change to 'uid name description'.

[4gl-reviewer]

Bug: select: 'uid username displayName isAdmin -_id' — the -_id excludes _id from the query, but uid is a virtual that returns this._id.toString(). With _id excluded, uid will be undefined. Remove -_id — the toJSON transform on the User model already strips _id from JSON output.

[4gl-reviewer]

Inconsistency: The tsoa decorator is still @Patch('{userId}') but the express route uses /:uid. This generates swagger docs showing /SASjsApi/user/{userId} for PATCH while GET uses {uid}. Update to @Patch('{uid}') and @Path() uid: string.

[4gl-reviewer]

Inconsistency: Same as above — @Delete('{userId}') should be @Delete('{uid}') with @Path() uid: string.

[4gl-reviewer]

Breaking change: PUBLIC_GROUP_NAME changed from 'Public' to 'public'. Existing deployments with a group named Public will have it orphaned. Consider a migration script.

[4gl-reviewer]

Critical: Incomplete migration — leftover groupId in populate select

select: 'groupId name description'

The Group model no longer has a groupId field (it was removed in this PR and replaced with the uid virtual). This select string references a non-existent field, so the populated group object in the updatePermission response will be missing its uid. Compare with the Permission.get static method in api/src/model/Permission.ts:70 which correctly uses 'uid name description'.

Fix:

select: 'uid name description'

Note: the existing test permission.spec.ts:382 only asserts res.body.setting and does not check res.body.group.uid, so this bug is not caught by the test suite.

[4gl-reviewer]

Warning: Login response schema still uses id with empty type

user: {properties: {..., id: {}}, required: [..., id], ...}

The actual login response in api/src/controllers/web.ts:137 returns uid: user.uid, not id. The swagger schema here still uses id with an empty schema object ({} — no type specified), which is invalid OpenAPI. It should be uid: {type: string} and the required array should list uid instead of id.

[4gl-reviewer]

Suggestion: JSDoc @param permissionId should be @param uid

   * @param permissionId The permission's identifier

The actual parameter is uid (see @Path() uid: string on line 168), but the JSDoc still references permissionId. Same issue on line 176 for deletePermission. These should be @param uid The permission's identifier.

[4gl-reviewer]

Critical: Desktop mode regexUser won't match new hex UIDs

The desktopUser now has userId: '12345' (a string), but the bigger issue is in desktop.ts:6:

const regexUser = /^\/SASjsApi\/user\/[0-9]*$/ // /SASjsApi/user/1

This regex only matches numeric path segments. After this PR, user identifiers are 24-character hex strings (e.g. /SASjsApi/user/507f1f77bcf86cd799439011). The character class [0-9]* will not match hex strings containing a-f, so desktopRestrict will return 403 "Not Allowed while in Desktop Mode" for GET and PATCH requests to /SASjsApi/user/:uid — breaking the desktop autoExec feature.

Fix:

const regexUser = /^\/SASjsApi\/user\/[0-9a-f]{24}$/i

[4gl-reviewer]

Critical: Group name rename breaks existing deployments — no migration

export const PUBLIC_GROUP_NAME = 'public'

PUBLIC_GROUP_NAME was changed from 'Public' to 'public', and ALL_USERS_GROUP.name was changed from 'AllUsers' to 'all-users' (see api/src/utils/seedDB.ts:85). The Group model enforces name: { unique: true }, and seedDB only creates groups if findOne({ name }) returns null.

For existing deployments, the DB already contains groups named 'Public' and 'AllUsers'. After this change:

• isPublicRoute (api/src/utils/isPublicRoute.ts:9) does Group.findOne({ name: 'public' }) -> returns null -> public route auth bypass silently stops working.
• createUser (api/src/controllers/user.ts:236) does getGroupByName(ALL_USERS_GROUP.name) looking for 'all-users' -> returns null -> new users are not added to the all-users group.
• seedDB will create duplicate groups with the new names (the old ones remain).

There is no migration script in this PR. Either provide a migration to rename existing groups, or keep backward-compatible lookups (e.g. check both names).

[4gl-reviewer]

Warning: Swagger path key inconsistent — {userId} vs {uid}

    '/SASjsApi/user/{userId}':

The GET endpoint is under '/SASjsApi/user/{uid}' (line 1406) but PATCH and DELETE are under a separate path key '/SASjsApi/user/{userId}' (line 1430). The actual Express routes in api/src/routes/api/user.ts all use '/:uid' with uidValidation. The parameter name should be uid (not userId) and both path keys should be merged into a single '/SASjsApi/user/{uid}' entry.

Also, the example: '1234' and example: 1234 on lines 1457 and 1483 should be valid 24-char hex strings.

[4gl-reviewer]

Warning: SessionResponse schema uses id but code returns uid

                id:
                    type: string

The SessionResponse schema defines the field as id, but the actual session() function in api/src/controllers/session.ts:47 returns uid: req.user!.userId. The schema should use uid (consistent with UserResponse which uses uid), and the required list on line 589 should reference uid instead of id.

[4gl-reviewer]

Suggestion: JSDoc @example userId should be @example userUid

   * @example userId "12ByteString"

The parameter is named userUid (see @param userUid on line 102), but the @example tag references userId. This is a copy-paste leftover from the ID->UID migration. Should be @example userUid "12ByteString".

[4gl-reviewer]

Bug: groupId is the old field name that no longer exists on the Group schema. This should be uid to match the rest of the migration (e.g. line 338 for user and lines 70/76 in model/Permission.ts). As written, the populate will not select the virtual uid field, so updatedPermission.group.uid will be undefined for group-based permissions.

Fix:

-      select: 'groupId name description'
+      select: 'uid name description'

[4gl-reviewer]

Hermes Agent Code Review

Verdict: Request Changes

Critical

• api/src/controllers/permission.ts:341 — select: 'groupId name description' still references the old groupId field, which no longer exists on the Group schema. This should be uid name description (matching line 338 for user and the populate in model/Permission.ts). As written, updatedPermission.group.uid will be undefined for group-based permissions after an update, breaking the response shape. The existing update test only uses a user principal and only asserts res.body.setting, so this bug is not covered by tests.

• api/src/middlewares/desktop.ts:6 — const regexUser = /^\/SASjsApi\/user\/[0-9]*$/ was not updated for the UID migration. UIDs are now 24-char hex strings (Joi.string().length(24).hex()), so GET /SASjsApi/user/<uid> and PATCH /SASjsApi/user/<uid> will fail reqAllowedInDesktopMode and return 403 Not Allowed while in Desktop Mode. in desktop mode. Update to e.g. /^\/SASjsApi\/user\/[0-9a-fA-F]{24}$/.

Suggestions

• api/src/controllers/user.ts:163 — the @Patch('{userId}') / @Delete('{userId}') route templates and @Path() userId param names still say userId while the @Get('{uid}') path uses uid. Consider renaming for consistency (cosmetic; functionally correct since the value is the UID string).
• api/src/controllers/user.ts:193 — JSDoc @example userId 1234 and @example userId "1234" are stale; the example value should be a 24-char hex string to match the new contract.

Looks Good

• The virtual uid property (this._id.toString()) on User/Group/Permission is a clean, zero-migration approach — no stored field, no data migration needed.
• toJSON transform consistently delete ret._id / delete ret.id, so the old id won't leak in responses.
• Authz checks (verifyAdminIfNeeded, authenticateToken, authorize) correctly use user.userId (now a string) for comparisons, matching the new string UID in the JWT/session.
• Web frontend migration to uid is complete (permission table, add/update/delete, login, session restore).
• Test specs updated to use dbUser.uid / dbGroup.uid and 24-hex validation.

Reviewed by Hermes Agent (GitHub App)

[4gl-reviewer]

🔴 Bug: stale groupId in select. Group.groupId was removed in this PR (replaced by the uid virtual), so select('groupId name description') will not populate the group's uid. The updatePermission response returns a group object with a missing/undefined uid, inconsistent with createPermission (which correctly uses 'uid ...') and with the PermissionDetailsResponse contract that requires uid.

Fix:

.populate({
  path: 'group',
  select: 'uid name description'
})

[4gl-reviewer]

⚠️ API contract mismatch: the runtime now returns uid here, but api/public/swagger.yaml SessionResponse still documents an id field (string) and lists id in required (not uid). API clients generated from the swagger will look for id and miss the session user identifier.

Fix: in swagger.yaml SessionResponse, rename id → uid (and update required).

[4gl-reviewer]

✅ Security: good choice. Using _id.toString() (a 12-byte Mongo ObjectId) as the UID means it is unguessable (5 bytes of randomness + timestamp + counter) and inherently unique — no separate unique index needed. This avoids the IDOR risk a sequential numeric id carried.

⚠️ Caveat: because uid is a Mongoose virtual, it will not be present on documents returned by .lean() or raw find() projections that don't include virtuals. All current call sites use Mongoose documents (with toJSON: { virtuals: true }), so this works today, but any future .lean() query would silently return objects without uid. Worth a brief code comment here noting that.

[4gl-reviewer]

✅ Good: this prevents NoSQL injection. Requiring length(24).hex() means object-form payloads like { \$gt: "" } cannot slip through into Mongo _id queries. The same constraint on principalId (line 121) is correct.

💡 Suggestion: the /:groupUid/:userUid route in routes/api/group.ts destructures groupUid/userUid from req.params and passes them straight to the controller without running uidValidation. If those params are ever used in a Mongo query (they are: Group.findOne({ _id: groupUid })), they should be validated with the same 24-hex check first to keep the injection guard consistent across all UID-bearing routes.

[4gl-reviewer]

Hermes Agent Code Review

Request changes — the refactor is sound and well-scoped, but a couple of leftover stale references to the removed id/groupId fields will produce undefined values at runtime and in tests.

Critical

• api/src/controllers/permission.ts:341 — updatePermission still calls .populate({ path: 'group', select: 'groupId name description' }). Group.groupId no longer exists (replaced by the uid virtual), so the populated group.uid will be undefined in the response, breaking the PermissionDetailsResponse contract. Should be select: 'uid name description' (matching createPermission). (inline comment posted)

• Stale .id reads in test files — api/src/routes/api/spec/auth.spec.ts:239 (currentUser.id) and api/src/routes/api/spec/user.spec.ts:240,244,255,259 (dbUser!.id). The id field was removed from the User model (now a virtual uid), so these evaluate to undefined. Consequences:

  • auth.spec.ts: verifyTokenInDB(undefined, …) does not actually verify the current user's tokens were removed.
  • user.spec.ts: generateAndSaveToken(undefined) mints a token for userId: undefined, and PATCH /SASjsApi/user/undefined hits the new uidValidation (24-hex) and returns 400, so these tests pass for the wrong reason rather than exercising the 405 path they intend.

  Fix: replace every remaining dbUser.id / currentUser.id with .uid.

Warnings

• Swagger SessionResponse mismatch — api/public/swagger.yaml SessionResponse still documents id (string) in properties and required, but the runtime (session.ts:47) now returns uid. Generated clients will look for id and miss the identifier. Rename to uid in the swagger. (inline comment posted)
• /:groupUid/:userUid route lacks validation — routes/api/group.ts validates :uid with uidValidation on GET/DELETE group, but the add/remove-user routes (POST /:groupUid/:userUid, DELETE /:groupUid/:userUid) destructure the params and pass them directly into Group.findOne({ _id: groupUid }) / User.findOne({ _id: userUid }) without the 24-hex check. For consistency and to keep the NoSQL-injection guard complete, validate both params there too. (inline comment posted)
• PUBLIC_GROUP_NAME changed from Public to public and ALL_USERS_GROUP from AllUsers to all-users. This is a behaviour change for existing deployments — groups seeded under the old names will no longer match the lookup in createUser (which calls getGroupByName(ALL_USERS_GROUP.name)), so new users may not be added to the all-users group. Confirm this is intended and that a migration updates existing group names; otherwise existing data will be orphaned.

Suggestions

• No unit tests for UID generation/format. The UIDs are Mongo ObjectIds (inherently unique), but a small test asserting randomBytesHexString(12) returns a 24-char hex string and that two calls differ would lock in the contract. Currently randomBytesHexString is only exercised incidentally via auth.spec.ts.
• uid is a Mongoose virtual (this._id.toString()). It will not appear on results from .lean() or raw projections lacking virtuals: true. All current call sites use full documents, so this is fine today — a one-line comment on each model noting this would help future contributors. (inline comment posted on User.ts)
• Unused import nit: api/src/model/User.ts imports ObjectId from mongoose but it is not referenced anywhere in the file.

Looks Good

• Security posture improved. Replacing a sequential numeric id with an unguessable 12-byte Mongo ObjectId as the public identifier removes a real IDOR/enumeration vector. The uidValidation (Joi.string().length(24).hex().required()) and the same rule on principalId correctly prevent NoSQL injection via object-form payloads.
• Auth/permission checks are consistent. authorize.ts, authenticateToken.ts, verifyTokenInDB.ts, verifyAdminIfNeeded.ts all query by _id and compare userId (string) with strict ===/!== — no undefined === undefined → true bypass, no type-coercion gap.
• Counter model and getSequenceNextValue fully removed — a repo-wide search confirms no remaining imports or callers.
• Frontend switched consistently — appContext.tsx, profile.tsx, permissionTable.tsx, useFilterPermissions.tsx, helper.ts, types.ts, and the permission hooks all read uid/permission.uid with no stale id/groupId/permissionId reads remaining on the web side.
• profile.tsx guard (if (appContext.userId)) is a nice fix — avoids fetching /SASjsApi/user/ with an empty uid on first render.

Reviewed by Hermes Agent (GitHub App)