feat: add support for dynamic access group configuration and synchron…#2226
feat: add support for dynamic access group configuration and synchron…#2226Darkroom4364 wants to merge 14 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds runtime-configurable access-group config loading and introduces a “sync on reboot” mechanism to reconcile affiliation access groups and memberships from access_config.json.
Changes:
- Added
syncAccessGroups()to upsert affiliation groups, soft-delete stale groups, and reconcile user memberships from email patterns. - Switched backend config loading from a compile-time JSON import to runtime file loading (optionally via
ACCESS_CONFIG_PATH). - Added/updated tests and tooling paths to use the relocated
backend/access_config.json.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/backend-common/src/services/affiliation-group.service.ts | Adds syncAccessGroups() implementing group + membership reconciliation logic. |
| packages/backend-common/src/seeds/user/create-users.seed.ts | Reads access config from env path or backend/access_config.json. |
| packages/backend-common/src/factories/user/user.factory.ts | Reads access config from env path or backend/access_config.json for default memberships in factories. |
| packages/backend-common/src/environment.ts | Adds ACCESS_CONFIG_PATH environment accessor. |
| docker/backend.Dockerfile | Copies backend/access_config.json into the production image. |
| backend/tsconfig.build.json | Removes JSON config from TS build include (no longer imported at compile time). |
| backend/tests/utils/database-utilities.ts | Updates test helper to read config from env path or backend/access_config.json. |
| backend/tests/auth/access-groups/affiliation-sync-on-reboot.test.ts | Adds coverage for sync behavior (create/update/delete memberships/groups). |
| backend/src/services/auth.service.ts | Runs affiliation sync on module init (startup). |
| backend/src/app.module.ts | Loads access config from file at runtime into Nest ConfigModule. |
| backend/access_config.json | Adds the default access group configuration file in new location. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| process.env.ACCESS_CONFIG_PATH ?? | ||
| path.resolve(process.cwd(), 'access_config.json'); | ||
| const content = fs.readFileSync(configPath, 'utf8'); | ||
| return { | ||
| accessConfig: JSON.parse(content) as AccessGroupConfig, | ||
| }; |
There was a problem hiding this comment.
ConfigModule load callback reads and parses the access config with fs.readFileSync/JSON.parse without any error handling. If the file is missing or malformed, startup will throw a low-level ENOENT / JSON error that’s hard to diagnose. Consider validating existence and wrapping parse errors to throw a clearer message (and optionally use the existing env.ACCESS_CONFIG_PATH getter for consistency).
| process.env.ACCESS_CONFIG_PATH ?? | |
| path.resolve(process.cwd(), 'access_config.json'); | |
| const content = fs.readFileSync(configPath, 'utf8'); | |
| return { | |
| accessConfig: JSON.parse(content) as AccessGroupConfig, | |
| }; | |
| env.ACCESS_CONFIG_PATH ?? | |
| path.resolve(process.cwd(), 'access_config.json'); | |
| if (!fs.existsSync(configPath)) { | |
| throw new Error( | |
| `Access config file not found: ${configPath}`, | |
| ); | |
| } | |
| let content: string; | |
| try { | |
| content = fs.readFileSync(configPath, 'utf8'); | |
| } catch (error) { | |
| const message = | |
| error instanceof Error ? error.message : String(error); | |
| throw new Error( | |
| `Failed to read access config file "${configPath}": ${message}`, | |
| ); | |
| } | |
| try { | |
| return { | |
| accessConfig: JSON.parse(content) as AccessGroupConfig, | |
| }; | |
| } catch (error) { | |
| const message = | |
| error instanceof Error ? error.message : String(error); | |
| throw new Error( | |
| `Failed to parse access config file "${configPath}" as JSON: ${message}`, | |
| ); | |
| } |
| await this.affiliationGroupService.syncAccessGroups( | ||
| this.config, | ||
| this.userRepository, | ||
| ); |
There was a problem hiding this comment.
AuthService.onModuleInit() now calls syncAccessGroups(), which performs potentially heavy DB writes/reads across all users. This makes application startup time/data-plane health dependent on completing a full sync. If this is intended, consider moving to a dedicated bootstrap job with logging/metrics and (at minimum) ensuring it is resilient to partial failures (e.g., wrap and log errors rather than failing module init).
| await this.affiliationGroupService.syncAccessGroups( | |
| this.config, | |
| this.userRepository, | |
| ); | |
| logger.info('Starting access group sync in background during module initialization'); | |
| void this.affiliationGroupService | |
| .syncAccessGroups(this.config, this.userRepository) | |
| .then(() => { | |
| logger.info('Access group sync completed'); | |
| }) | |
| .catch((error: unknown) => { | |
| logger.error( | |
| 'Access group sync failed during module initialization', | |
| error, | |
| ); | |
| }); |
| const existing = await this.accessGroupRepository.findOne({ | ||
| where: { uuid: group.uuid }, | ||
| }); | ||
| if (!existing) { | ||
| const newGroup = this.accessGroupRepository.create({ | ||
| name: group.name, | ||
| uuid: group.uuid, | ||
| type: AccessGroupType.AFFILIATION, | ||
| creator: {}, | ||
| }); | ||
| return this.accessGroupRepository.save(newGroup); | ||
| } |
There was a problem hiding this comment.
syncAccessGroups() inserts a new AccessGroupEntity when findOne() returns null, but findOne() does not include soft-deleted rows. If a group was previously soft-deleted (step 2) and later re-added to the config, this will attempt to INSERT with the same UUID primary key and fail. Use a lookup with withDeleted() (or query builder .withDeleted()) and restore()/recover() the existing row instead of inserting a new one when deletedAt is set.
| // 3. Re-sync user memberships for affiliation groups | ||
| const users = await userRepository | ||
| .createQueryBuilder('user') | ||
| .addSelect('user.email') | ||
| .leftJoinAndSelect('user.memberships', 'membership') | ||
| .leftJoinAndSelect('membership.accessGroup', 'accessGroup') | ||
| .getMany(); |
There was a problem hiding this comment.
syncAccessGroups() currently loads all users and left-joins memberships/accessGroup, then performs per-user membership mutations. On larger datasets this will be O(user_count × email_patterns) work on every backend startup (AuthService.onModuleInit), and can significantly delay boot / increase DB load. Consider batching (pagination) and/or narrowing the query to users whose email matches any configured suffix, and replacing per-membership remove() calls with a single delete query per user/group set where possible.
Darkroom4364
force-pushed
the
1911-auth-add-support-to-change-access_configjson-on-an-existing-instance
branch
from
dc58608 to
776ce32
Compare
Greptile SummaryThis PR introduces a Several issues flagged in prior review rounds have been resolved: the Confidence Score: 4/5Safe to merge with minor cleanup; all critical issues from prior rounds are resolved but a handful of previously-flagged structural concerns remain outstanding. The PR cleanly closes the most dangerous prior findings (FK-violating hard remove, dead deletedAt check, broken restore+save, missing @ separator, redundant createAccessGroups). New code is well-tested. One new P2 finding (JSON.parse not guarded against SyntaxError) and residual structural concerns from previous threads keep the score at 4. backend/src/app.module.ts (JSON.parse error handling); packages/backend-common/src/services/affiliation-group.service.ts (membership hard-delete + N+1 pattern from prior thread)
|
| Filename | Overview |
|---|---|
| backend/src/app.module.ts | New config-loading block reads and cross-validates access_config.json at startup; JSON.parse not guarded against SyntaxError |
| packages/backend-common/src/services/affiliation-group.service.ts | New syncAccessGroups three-step upsert/soft-delete/membership-reconcile; fixes previous restore+save and deletedAt select issues; hard-delete of memberships before softRemove still inconsistent (prior thread) |
| backend/src/services/auth.service.ts | Removed redundant createAccessGroups call from onModuleInit; now only calls syncAccessGroups which subsumes group creation |
| docker/backend.Dockerfile | access_config.json baked into production image; ACCESS_CONFIG_PATH env var allows override at runtime |
| backend/tests/utils/database-utilities.ts | mockDatabaseUser reads config with JSON.parse/readFileSync without error handling; a missing or malformed config will surface as an unhelpful uncaught error in tests |
Sequence Diagram
sequenceDiagram
participant NestJS as NestJS Bootstrap
participant AppModule
participant ConfigModule
participant AuthService
participant AffiliationGroupService
participant DB as PostgreSQL
NestJS->>AppModule: initialize
AppModule->>ConfigModule: load access_config.json (validate arrays & UUID refs)
ConfigModule-->>AppModule: AccessGroupConfig
NestJS->>AuthService: onModuleInit()
AuthService->>AffiliationGroupService: syncAccessGroups(config, userRepo)
Note over AffiliationGroupService,DB: Step 1 – Upsert config groups
AffiliationGroupService->>DB: findOne(uuid, withDeleted, select deletedAt)
DB-->>AffiliationGroupService: existing | null
AffiliationGroupService->>DB: save(group) create or restore+rename
Note over AffiliationGroupService,DB: Step 2 – Soft-delete stale groups
AffiliationGroupService->>DB: find(type=AFFILIATION, active only)
DB-->>AffiliationGroupService: allAffiliationGroups
AffiliationGroupService->>DB: delete(memberships for stale group)
AffiliationGroupService->>DB: softRemove(staleGroup)
Note over AffiliationGroupService,DB: Step 3 – Reconcile memberships
AffiliationGroupService->>DB: queryBuilder users+memberships+accessGroups
DB-->>AffiliationGroupService: users[]
loop each user
AffiliationGroupService->>DB: save(missing membership)
AffiliationGroupService->>DB: remove(stale membership)
end
AuthService-->>NestJS: ready
Prompt To Fix All With AI
This is a comment left during a code review.
Path: backend/src/app.module.ts
Line: 64-66
Comment:
**`JSON.parse` not guarded against `SyntaxError`**
`fs.readFileSync` is wrapped in a try/catch with a helpful message, but `JSON.parse` on line 65 is not. A malformed `access_config.json` (trailing comma, byte-order mark, comment, etc.) will throw a bare `SyntaxError: Unexpected token …` during `ConfigModule` initialisation — much harder to diagnose than the custom error written for the read failure above.
```suggestion
let accessConfig: AccessGroupConfig;
try {
accessConfig = JSON.parse(rawConfig) as AccessGroupConfig;
} catch (parseError) {
throw new Error(
`Failed to parse access_config.json at "${configPath}": ${String(parseError)}`,
);
}
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (9): Last reviewed commit: "feat: add error handling for access conf..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Compute expected affiliation group UUIDs from config | ||
| const expectedUuids = new Set<string>(); | ||
| for (const emailConfig of config.emails) { | ||
| if (user.email.endsWith(emailConfig.email)) { |
There was a problem hiding this comment.
endsWith without an @ separator means a user with email alice@evilkleinkram.dev will match the pattern kleinkram.dev and receive the same affiliation-group membership as alice@kleinkram.dev. The same flaw exists in addToAffiliationGroups (line 210) and user.factory.ts (line 57). Prepending @ to the pattern fixes it:
| if (user.email.endsWith(emailConfig.email)) { | |
| if (user.email.endsWith('@' + emailConfig.email)) { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/backend-common/src/services/affiliation-group.service.ts
Line: 111
Comment:
**Email domain match allows prefix spoofing**
`endsWith` without an `@` separator means a user with email `alice@evilkleinkram.dev` will match the pattern `kleinkram.dev` and receive the same affiliation-group membership as `alice@kleinkram.dev`. The same flaw exists in `addToAffiliationGroups` (line 210) and `user.factory.ts` (line 57). Prepending `@` to the pattern fixes it:
```suggestion
if (user.email.endsWith('@' + emailConfig.email)) {
```
How can I resolve this? If you propose a fix, please make it concise.| const allAffiliationGroups = await this.accessGroupRepository.find({ | ||
| where: { type: AccessGroupType.AFFILIATION }, | ||
| }); | ||
| const staleGroups = allAffiliationGroups.filter( | ||
| (g) => !configUuids.has(g.uuid), | ||
| ); | ||
| for (const staleGroup of staleGroups) { | ||
| await this.groupMembershipRepository.delete({ | ||
| accessGroup: { uuid: staleGroup.uuid }, | ||
| }); | ||
| await this.accessGroupRepository.softRemove(staleGroup); | ||
| } |
There was a problem hiding this comment.
Step 2 hard-deletes memberships (delete) but then only soft-removes the group (softRemove). This is an inconsistency: the group record is preserved (with a deletedAt), while its memberships are gone permanently. If the group is ever restored (soft-delete undone), users will have lost their memberships with no way to recover them. Consider either also soft-deleting memberships, or documenting that soft-removal of a group should always be paired with hard-deletion of memberships.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/backend-common/src/services/affiliation-group.service.ts
Line: 84-95
Comment:
**Group memberships hard-deleted, group only soft-deleted**
Step 2 hard-deletes memberships (`delete`) but then only soft-removes the group (`softRemove`). This is an inconsistency: the group record is preserved (with a `deletedAt`), while its memberships are gone permanently. If the group is ever restored (soft-delete undone), users will have lost their memberships with no way to recover them. Consider either also soft-deleting memberships, or documenting that soft-removal of a group should always be paired with hard-deletion of memberships.
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| // 3. Re-sync user memberships for affiliation groups | ||
| const users = await userRepository | ||
| .createQueryBuilder('user') | ||
| .addSelect('user.email') | ||
| .leftJoinAndSelect('user.memberships', 'membership') | ||
| .leftJoinAndSelect('membership.accessGroup', 'accessGroup') | ||
| .getMany(); |
There was a problem hiding this comment.
syncAccessGroups loads every user with their memberships into memory (getMany()) and then issues individual save/remove calls per user per group. For deployments with thousands of users this will be slow, memory-intensive, and may block module initialization for a noticeable period. Consider batching the query (pagination or chunked processing) and replacing the per-row saves with bulk inserts/deletes.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/backend-common/src/services/affiliation-group.service.ts
Line: 96-103
Comment:
**Full user table scan on every startup**
`syncAccessGroups` loads every user with their memberships into memory (`getMany()`) and then issues individual save/remove calls per user per group. For deployments with thousands of users this will be slow, memory-intensive, and may block module initialization for a noticeable period. Consider batching the query (pagination or chunked processing) and replacing the per-row saves with bulk inserts/deletes.
How can I resolve this? If you propose a fix, please make it concise.| async onModuleInit(): Promise<void> { | ||
| await this.affiliationGroupService.createAccessGroups(this.config); | ||
| await this.affiliationGroupService.syncAccessGroups( | ||
| this.config, | ||
| this.userRepository, | ||
| ); |
There was a problem hiding this comment.
createAccessGroups is redundant before syncAccessGroups
syncAccessGroups already upserts every group from the config in its step 1, so the preceding createAccessGroups call performs duplicate lookups for every configured group. Removing it avoids the unnecessary round-trips.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/src/services/auth.service.ts
Line: 54-59
Comment:
**`createAccessGroups` is redundant before `syncAccessGroups`**
`syncAccessGroups` already upserts every group from the config in its step 1, so the preceding `createAccessGroups` call performs duplicate lookups for every configured group. Removing it avoids the unnecessary round-trips.
How can I resolve this? If you propose a fix, please make it concise.
| // 2. Soft-delete stale affiliation groups not in config | ||
| const allAffiliationGroups = await this.accessGroupRepository.find({ | ||
| where: { type: AccessGroupType.AFFILIATION }, | ||
| }); | ||
| const staleGroups = allAffiliationGroups.filter( | ||
| (g) => !configUuids.has(g.uuid), | ||
| ); | ||
| for (const staleGroup of staleGroups) { | ||
| await this.groupMembershipRepository.delete({ | ||
| accessGroup: { uuid: staleGroup.uuid }, | ||
| }); | ||
| await this.accessGroupRepository.remove(staleGroup); | ||
| } |
There was a problem hiding this comment.
accessGroupRepository.remove(staleGroup) issues a hard DELETE SQL. Both ProjectAccessEntity and MissionAccessEntity carry a @ManyToOne to AccessGroupEntity with nullable: false and no onDelete: 'CASCADE' — meaning PostgreSQL will refuse the delete with a FK constraint violation whenever a stale group has been granted access to any project or mission.
The failure happens after memberships have already been hard-deleted (line 91–93), so the server boots with an inconsistent state: users have lost their memberships while the group itself remains in the database untouched.
Reverting to .softRemove() restores the previous safe behaviour — the deletedAt timestamp is set, FK references stay valid, and the group is hidden from normal queries. Alternatively, delete project_accesses and mission_accesses referencing the stale group before calling remove().
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/backend-common/src/services/affiliation-group.service.ts
Line: 83-95
Comment:
**Hard-delete breaks when stale group has project/mission accesses**
`accessGroupRepository.remove(staleGroup)` issues a hard `DELETE` SQL. Both `ProjectAccessEntity` and `MissionAccessEntity` carry a `@ManyToOne` to `AccessGroupEntity` with `nullable: false` and **no `onDelete: 'CASCADE'`** — meaning PostgreSQL will refuse the delete with a FK constraint violation whenever a stale group has been granted access to any project or mission.
The failure happens _after_ memberships have already been hard-deleted (line 91–93), so the server boots with an inconsistent state: users have lost their memberships while the group itself remains in the database untouched.
Reverting to `.softRemove()` restores the previous safe behaviour — the `deletedAt` timestamp is set, FK references stay valid, and the group is hidden from normal queries. Alternatively, delete `project_accesses` and `mission_accesses` referencing the stale group before calling `remove()`.
How can I resolve this? If you propose a fix, please make it concise.
| if (existing.deletedAt) { | ||
| await this.accessGroupRepository.restore(existing.uuid); | ||
| } | ||
| if (existing.name !== group.name) { | ||
| existing.name = group.name; | ||
| return this.accessGroupRepository.save(existing); | ||
| } |
There was a problem hiding this comment.
restore + save re-applies deleted timestamp
restore(existing.uuid) sets deletedAt = NULL in the database, but does not update the in-memory existing object. When save(existing) is then called with the stale in-memory existing.deletedAt still holding the old timestamp, TypeORM builds an UPDATE that overwrites the NULL back to the original value — leaving the group permanently soft-deleted despite being in the config. Any subsequent call to addToAffiliationGroups that does findOneOrFail({ where: { uuid } }) (no withDeleted: true) will then throw EntityNotFoundError, breaking login for users whose email matches that group's pattern.
Fix: clear existing.deletedAt in memory before calling save:
| if (existing.deletedAt) { | |
| await this.accessGroupRepository.restore(existing.uuid); | |
| } | |
| if (existing.name !== group.name) { | |
| existing.name = group.name; | |
| return this.accessGroupRepository.save(existing); | |
| } | |
| if (existing.deletedAt) { | |
| await this.accessGroupRepository.restore(existing.uuid); | |
| existing.deletedAt = null; | |
| } | |
| if (existing.name !== group.name) { | |
| existing.name = group.name; | |
| return this.accessGroupRepository.save(existing); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/backend-common/src/services/affiliation-group.service.ts
Line: 76-82
Comment:
**`restore` + `save` re-applies deleted timestamp**
`restore(existing.uuid)` sets `deletedAt = NULL` in the database, but does **not** update the in-memory `existing` object. When `save(existing)` is then called with the stale in-memory `existing.deletedAt` still holding the old timestamp, TypeORM builds an UPDATE that overwrites the `NULL` back to the original value — leaving the group permanently soft-deleted despite being in the config. Any subsequent call to `addToAffiliationGroups` that does `findOneOrFail({ where: { uuid } })` (no `withDeleted: true`) will then throw `EntityNotFoundError`, breaking login for users whose email matches that group's pattern.
Fix: clear `existing.deletedAt` in memory before calling `save`:
```suggestion
if (existing.deletedAt) {
await this.accessGroupRepository.restore(existing.uuid);
existing.deletedAt = null;
}
if (existing.name !== group.name) {
existing.name = group.name;
return this.accessGroupRepository.save(existing);
}
```
How can I resolve this? If you propose a fix, please make it concise.
| const needsUpdate = | ||
| existing.deletedAt !== undefined || existing.name !== group.name; | ||
| if (!needsUpdate) return; | ||
| existing.deletedAt = undefined; | ||
| existing.name = group.name; | ||
| return this.accessGroupRepository.save(existing); |
There was a problem hiding this comment.
deletedAt is never populated
BaseEntity declares @DeleteDateColumn({ select: false }), which means TypeORM never includes deletedAt in the SELECT clause unless it is explicitly added. Even when the entity is fetched with withDeleted: true, the in-memory existing.deletedAt is always undefined — for both active and soft-deleted records. As a result, existing.deletedAt !== undefined is always false, the condition collapses to just the name check, and soft-deleted groups that reappear in the config are never detected and never restored.
Concretely: restart the server after removing a group from the config (soft-delete it), then add it back. syncAccessGroups finds the group (because of withDeleted: true), sees deletedAt === undefined, evaluates needsUpdate = false (if the name is unchanged), and returns early — the group remains soft-deleted and addToAffiliationGroups will throw EntityNotFoundError for any user whose email matches.
Fix: explicitly select deletedAt in the query and use null (not undefined) to clear it:
const existing = await this.accessGroupRepository.findOne({
where: { uuid: group.uuid },
withDeleted: true,
select: { uuid: true, name: true, deletedAt: true },
});
if (!existing) {
// ... create
} else {
if (existing.deletedAt !== null || existing.name !== group.name) {
existing.deletedAt = null as unknown as Date;
existing.name = group.name;
return this.accessGroupRepository.save(existing);
}
}Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/backend-common/src/services/affiliation-group.service.ts
Line: 77-82
Comment:
**Soft-delete check is dead code — `deletedAt` is never populated**
`BaseEntity` declares `@DeleteDateColumn({ select: false })`, which means TypeORM never includes `deletedAt` in the SELECT clause unless it is explicitly added. Even when the entity is fetched with `withDeleted: true`, the in-memory `existing.deletedAt` is always `undefined` — for both active and soft-deleted records. As a result, `existing.deletedAt !== undefined` is always `false`, the condition collapses to just the name check, and soft-deleted groups that reappear in the config are never detected and never restored.
Concretely: restart the server after removing a group from the config (soft-delete it), then add it back. `syncAccessGroups` finds the group (because of `withDeleted: true`), sees `deletedAt === undefined`, evaluates `needsUpdate = false` (if the name is unchanged), and returns early — the group remains soft-deleted and `addToAffiliationGroups` will throw `EntityNotFoundError` for any user whose email matches.
Fix: explicitly select `deletedAt` in the query and use `null` (not `undefined`) to clear it:
```typescript
const existing = await this.accessGroupRepository.findOne({
where: { uuid: group.uuid },
withDeleted: true,
select: { uuid: true, name: true, deletedAt: true },
});
if (!existing) {
// ... create
} else {
if (existing.deletedAt !== null || existing.name !== group.name) {
existing.deletedAt = null as unknown as Date;
existing.name = group.name;
return this.accessGroupRepository.save(existing);
}
}
```
How can I resolve this? If you propose a fix, please make it concise.…cess group sync logic
…ization