MemberJunction / MemberJunction/MJ
UserCache.GetSystemUser() returns a deactivated system user, and its type says it cannot be undefined
- Dominant language
- TSQL
- Stars
- 29
- Forks
- 6
- Avg merge
- 2d 1h
- Merged PRs (30d)
- 323
Description
**Found by** the PR gauntlet on #4231 (`fix/4209-provisioning-context-user`), 2026-09-04. Not caused by that PR: verified pre-existing at `938cd9e9e8f9425924dc7d7bd3b55c90dfd90e46`.
### Where
`packages/GenericDatabaseProvider/src/UserCache.ts:57-59` (`GetSystemUser`) and `:141-148`
(`UserByName`).
### What happens
```ts
public GetSystemUser(): UserInfo {
return this.Users.find((u) => u.ID.toLowerCase() === UserCache.Instance.SYSTEM_USER_ID.toLowerCase()) as UserInfo;
}
```
- **No `IsActive` test**, so a deactivated system user is still returned as a principal to the
~25 call sites that use it.
- `u.ID.toLowerCase()` throws on a null `ID` instead of using `UUIDsEqual`, MJ's canonical
null-safe, case-insensitive UUID comparison (`guides/UUID_COMPARISON_GUIDE.md`).
- The return type lies: it is `UserInfo` via `as`, but `find` returns `undefined` on a cold or
Owner-less cache, so every caller that trusts the signature dereferences `undefined`.
`UserByName` calls `u.Name.trim()` with no guard, so a row reaching the cache through the public
`SetUsers` seam with a null `Name` makes it throw.
### Why it matters
Returning a disabled account as a principal contradicts the fail-closed policy elsewhere in the
same subsystem (`MagicLinkService.isInviterActive`) and the `IsActive` requirement PR #4231 put on
every rung of `auth/principals.ts`. `GetSystemUser()` is now the *only* system-user lookup in
MJServer that does not check it, which makes the guarantee depend on which helper a caller picked.
### Repro
```ts
UserCache.Instance.SetUsers([{ ID: 'x', Name: null } as unknown as UserInfo]);
UserCache.Instance.UserByName('anyone'); // TypeError: Cannot read properties of null (reading 'trim')
```
For the `IsActive` half: seed a scratch database with `UPDATE __mj.User SET IsActive = 0` on the
system user, boot, and observe `GetSystemUser()` still returning it.
### Evidence
`plans/provisioning-context-user-resolution.md` §4 records both as failure modes 7 and 11, and §6
lists them as deliberately out of PR #4231's scope. `principals.ts` avoids both by re-implementing
the lookup with `UUIDsEqual` and an `IsActive === true` filter rather than calling this method.
### Suggested fix
In `GetSystemUser()`: add `&& u.IsActive`, switch the comparison to `UUIDsEqual`, and change the
return type to `UserInfo | undefined` so callers must handle the miss (this will surface real
call-site bugs — that is the point). Null-guard `UserByName`. The `IsActive` change is a behaviour
change and needs its own changeset entry.
### Definition of done
- [ ] A failing test that reproduces it, then green
- [ ] `GetSystemUser()` returns `undefined` for a deactivated system user, and its signature says so
- [ ] `UserByName` returns `undefined` rather than throwing on a null `Name`
- [ ] Existing suite and gates green; every call site updated for the widened return type
### Verify by
`cd packages/GenericDatabaseProvider && pnpm test`, then `pnpm run build` across the workspace to
confirm no call site silently swallowed the widened type.
Contributor guide
Research direction
Start in packages/GenericDatabaseProvider/src/UserCache.ts at GetSystemUser and UserByName, then inspect the roughly 25 call sites and the related logic in auth/principals.ts. Add regression coverage for deactivated system users and null names, run cd packages/GenericDatabaseProvider && pnpm test, and finish with the workspace build passing after all widened return types are handled.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- typescript
- Domain
- authentication, backend
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 55/100