From 20e04dc288a278bcfbc32f4910561265f05b667a Mon Sep 17 00:00:00 2001 From: DJAscendance Date: Thu, 30 Jul 2026 08:09:46 -0400 Subject: [PATCH 1/6] refactor: single reconcilePrimaryRole, replacing 18 copy-pasted blocks member.primary_role_id records which of a member's roles they display; role_assignment is the authority for what they actually hold. Keeping the two consistent was open-coded in 18 places across hood, block, colony, place and admin, and the shared logic was wrong in three ways. 1. It compared the *revoked* role against primary_role_id and nulled on a match. A member holding Block Leader and Neighborhood Deputy who lost Block Leader had their displayed role cleared despite still holding another. Reconciling against the assignments that remain fixes this. 2. The deputy loops were forEach callbacks containing un-awaited promise chains, so the write could land after the request had returned. Converted to for loops that await. 3. admin.fireRole inspected primary_role_id *before* deleting the assignment, deciding against state it was about to change. Now removes, then reconciles. Also hoists the old-owner removal out of the if/else in the four geographic services, which duplicated it identically in both branches, and adds memberRepository.getPrimaryRoleId -- getPrimaryRoleName INNER JOINs role, so it cannot distinguish "no role displayed" from "member not found". Adds five tests, including one covering the multi-role case from (1). No behaviour change intended beyond those three fixes. --- .../repositories/member/member.repository.ts | 16 ++++ api/src/services/admin/admin.services.ts | 14 +-- api/src/services/block/block.service.ts | 85 ++++++------------- api/src/services/colony/colony.service.ts | 85 ++++++------------- api/src/services/hood/hood.service.ts | 83 ++++++------------ api/src/services/place/place.service.ts | 84 ++++++------------ .../role-assignment.service.spec.ts | 65 +++++++++++++- .../role-assignment.service.ts | 29 +++++++ 8 files changed, 220 insertions(+), 241 deletions(-) diff --git a/api/src/repositories/member/member.repository.ts b/api/src/repositories/member/member.repository.ts index 45ac93ed..20f85f52 100644 --- a/api/src/repositories/member/member.repository.ts +++ b/api/src/repositories/member/member.repository.ts @@ -127,6 +127,22 @@ export class MemberRepository { .join('role', 'member.primary_role_id', 'role.id'); } + /** + * Returns the member's primary_role_id, or null. + * + * Deliberately separate from getPrimaryRoleName, which INNER JOINs role and so returns + * an empty set when the column is null -- indistinguishable from "member not found". + * Reconciliation needs to tell those apart. + */ + public async getPrimaryRoleId(memberId: number): Promise { + const row = await this.db.knex + .select('primary_role_id') + .from('member') + .where('id', memberId) + .first(); + return row ? row.primary_role_id : null; + } + /** * This is to assist with the pagination of the user search * @param search diff --git a/api/src/services/admin/admin.services.ts b/api/src/services/admin/admin.services.ts index 9847c7c5..bd3f345d 100644 --- a/api/src/services/admin/admin.services.ts +++ b/api/src/services/admin/admin.services.ts @@ -15,6 +15,7 @@ import { TransactionRepository, WalletRepository, } from '../../repositories'; +import { RoleAssignmentService } from '../role-assignment/role-assignment.service'; @Service() export class AdminService { @@ -32,6 +33,7 @@ export class AdminService { private objectInstanceRepository: ObjectInstanceRepository, private transactionRepository: TransactionRepository, private walletRepository: WalletRepository, + private roleAssignmentService: RoleAssignmentService, ) {} public async addBan(ban_member_id, time_frame, type, assigner_member_id, reason): Promise { @@ -61,14 +63,12 @@ export class AdminService { } public async fireRole(member_id: number, role_id: number, place_id: number): Promise { - const response: any = await this.memberRepository.getPrimaryRoleName(member_id); - if (response.length !== 0) { - const primaryRoleId = response[0].primary_role_id; - if (role_id === primaryRoleId){ - await this.memberRepository.update(member_id, {primary_role_id: null}); - } - } + // Remove first, then reconcile. The previous version inspected primary_role_id + // before deleting the assignment, deciding against state it was about to change -- + // and it only cleared the column when the fired role happened to be the displayed + // one, leaving a member who still held other roles with no display role at all. await this.roleAssignmentRepository.removeIdFromAssignment(place_id, member_id, role_id); + await this.roleAssignmentService.reconcilePrimaryRole(member_id); return; } diff --git a/api/src/services/block/block.service.ts b/api/src/services/block/block.service.ts index 681f25e8..34da31cb 100644 --- a/api/src/services/block/block.service.ts +++ b/api/src/services/block/block.service.ts @@ -10,6 +10,7 @@ import { } from '../../repositories'; import {Member, Place} from '../../types/models'; import {includes} from 'lodash'; +import { RoleAssignmentService } from '../role-assignment/role-assignment.service'; /** Service for dealing with blocks */ @Service() @@ -21,6 +22,7 @@ export class BlockService { private roleAssignmentRepository: RoleAssignmentRepository, private roleRepository: RoleRepository, private memberRepository: MemberRepository, + private roleAssignmentService: RoleAssignmentService, ) {} public async find(blockId: number): Promise { @@ -70,29 +72,14 @@ export class BlockService { newOwner = result[0].id; } } + // Both branches previously removed the old owner identically, so the removal is + // hoisted out rather than duplicated. + if (oldOwner !== 0) { + await this.roleAssignmentRepository.removeIdFromAssignment(blockId, oldOwner, ownerCode); + await this.roleAssignmentService.reconcilePrimaryRole(oldOwner); + } if (newOwner !== 0) { - if (oldOwner !== 0) { - await this.roleAssignmentRepository.removeIdFromAssignment(blockId, oldOwner, ownerCode); - const response: any = await this.memberRepository.getPrimaryRoleName(oldOwner); - if (response.length !== 0) { - const primaryRoleId = response[0].primary_role_id; - if (ownerCode === primaryRoleId){ - await this.memberRepository.update(oldOwner, {primary_role_id: null}); - } - } - } await this.roleAssignmentRepository.addIdToAssignment(blockId, newOwner, ownerCode); - } else { - if (oldOwner !== 0) { - await this.roleAssignmentRepository.removeIdFromAssignment(blockId, oldOwner, ownerCode); - const response: any = await this.memberRepository.getPrimaryRoleName(oldOwner); - if (response.length !== 0) { - const primaryRoleId = response[0].primary_role_id; - if (ownerCode === primaryRoleId){ - await this.memberRepository.update(oldOwner, {primary_role_id: null}); - } - } - } } data.deputies.forEach((deputies, index) => { oldDeputies[index] = deputies.member_id; @@ -100,46 +87,26 @@ export class BlockService { for (let i = 0; i < givenDeputies.length; i++) { newDeputies[i] = await this.updateDeputyId(givenDeputies[i]); } - oldDeputies.forEach((oldDeputies, index) => { - if (oldDeputies !== newDeputies[index]) { - if (newDeputies[index] === 0) { - try { - this.roleAssignmentRepository.removeIdFromAssignment(blockId, oldDeputies, deputyCode); - } catch (e) { - console.log(e); - } - if (oldDeputies !== 0) { - this.memberRepository.getPrimaryRoleName(oldDeputies) - .then((response: any) => { - if (response.length !== 0) { - const primaryRoleId = response[0].primary_role_id; - if (primaryRoleId && deputyCode === primaryRoleId) { - this.memberRepository.update(oldDeputies, {primary_role_id: null}); - } - } - }); - } - } else { - try { - this.roleAssignmentRepository.removeIdFromAssignment(blockId, oldDeputies, deputyCode); - this.memberRepository.getPrimaryRoleName(oldDeputies) - .then((response: any) => { - if (response.length !== 0) { - const primaryRoleId = response[0].primary_role_id; - if (deputyCode === primaryRoleId) { - this.memberRepository.update(oldDeputies, {primary_role_id: null}); - } - } - }); - this - .roleAssignmentRepository - .addIdToAssignment(blockId, newDeputies[index], deputyCode); - } catch (e) { - console.log(e); - } + // Was a forEach containing un-awaited promise chains, so the primary-role write + // could land after the request had already returned. A for loop lets these await. + for (let index = 0; index < oldDeputies.length; index++) { + const oldDeputy = oldDeputies[index]; + const newDeputy = newDeputies[index]; + if (oldDeputy === newDeputy) continue; + try { + if (oldDeputy !== 0) { + await this.roleAssignmentRepository + .removeIdFromAssignment(blockId, oldDeputy, deputyCode); + await this.roleAssignmentService.reconcilePrimaryRole(oldDeputy); + } + if (newDeputy !== 0) { + await this.roleAssignmentRepository + .addIdToAssignment(blockId, newDeputy, deputyCode); } + } catch (e) { + console.log(e); } - }); + } } public async getMapLocationAndPlaces(blockId: number): Promise { diff --git a/api/src/services/colony/colony.service.ts b/api/src/services/colony/colony.service.ts index d2b9cf2f..de6d6e16 100644 --- a/api/src/services/colony/colony.service.ts +++ b/api/src/services/colony/colony.service.ts @@ -9,6 +9,7 @@ import { import { Place } from '../../types/models'; import * as console from 'console'; import { includes } from 'lodash'; +import { RoleAssignmentService } from '../role-assignment/role-assignment.service'; /** Service for dealing with colony */ @Service() @@ -18,6 +19,7 @@ export class ColonyService { private roleAssignmentRepository: RoleAssignmentRepository, private roleRepository: RoleRepository, private memberRepository: MemberRepository, + private roleAssignmentService: RoleAssignmentService, ) { } public async find(colonyId: number): Promise { @@ -66,29 +68,14 @@ export class ColonyService { newOwner = result[0].id; } } + // Both branches previously removed the old owner identically, so the removal is + // hoisted out rather than duplicated. + if (oldOwner !== 0) { + await this.roleAssignmentRepository.removeIdFromAssignment(colonyId, oldOwner, ownerCode); + await this.roleAssignmentService.reconcilePrimaryRole(oldOwner); + } if (newOwner !== 0) { - if (oldOwner !== 0) { - await this.roleAssignmentRepository.removeIdFromAssignment(colonyId, oldOwner, ownerCode); - const response: any = await this.memberRepository.getPrimaryRoleName(oldOwner); - if (response.length !== 0) { - const primaryRoleId = response[0].primary_role_id; - if (ownerCode === primaryRoleId) { - await this.memberRepository.update(oldOwner, { primary_role_id: null }); - } - } - } await this.roleAssignmentRepository.addIdToAssignment(colonyId, newOwner, ownerCode); - } else { - if (oldOwner !== 0) { - await this.roleAssignmentRepository.removeIdFromAssignment(colonyId, oldOwner, ownerCode); - const response: any = await this.memberRepository.getPrimaryRoleName(oldOwner); - if (response.length !== 0) { - const primaryRoleId = response[0].primary_role_id; - if (ownerCode === primaryRoleId) { - await this.memberRepository.update(oldOwner, { primary_role_id: null }); - } - } - } } data.deputies.forEach((deputies, index) => { oldDeputies[index] = deputies.member_id; @@ -96,46 +83,26 @@ export class ColonyService { for (let i = 0; i < givenDeputies.length; i++) { newDeputies[i] = await this.updateDeputyId(givenDeputies[i]); } - oldDeputies.forEach((oldDeputies, index) => { - if (oldDeputies !== newDeputies[index]) { - if (newDeputies[index] === 0) { - try { - this.roleAssignmentRepository.removeIdFromAssignment(colonyId, oldDeputies, deputyCode); - } catch (e) { - console.log(e); - } - if (oldDeputies !== 0) { - this.memberRepository.getPrimaryRoleName(oldDeputies) - .then((response: any) => { - if (response.length !== 0) { - const primaryRoleId = response[0].primary_role_id; - if (primaryRoleId && deputyCode === primaryRoleId) { - this.memberRepository.update(oldDeputies, { primary_role_id: null }); - } - } - }); - } - } else { - try { - this.roleAssignmentRepository.removeIdFromAssignment(colonyId, oldDeputies, deputyCode); - this.memberRepository.getPrimaryRoleName(oldDeputies) - .then((response: any) => { - if (response.length !== 0) { - const primaryRoleId = response[0].primary_role_id; - if (deputyCode === primaryRoleId) { - this.memberRepository.update(oldDeputies, { primary_role_id: null }); - } - } - }); - this - .roleAssignmentRepository - .addIdToAssignment(colonyId, newDeputies[index], deputyCode); - } catch (e) { - console.log(e); - } + // Was a forEach containing un-awaited promise chains, so the primary-role write + // could land after the request had already returned. A for loop lets these await. + for (let index = 0; index < oldDeputies.length; index++) { + const oldDeputy = oldDeputies[index]; + const newDeputy = newDeputies[index]; + if (oldDeputy === newDeputy) continue; + try { + if (oldDeputy !== 0) { + await this.roleAssignmentRepository + .removeIdFromAssignment(colonyId, oldDeputy, deputyCode); + await this.roleAssignmentService.reconcilePrimaryRole(oldDeputy); + } + if (newDeputy !== 0) { + await this.roleAssignmentRepository + .addIdToAssignment(colonyId, newDeputy, deputyCode); } + } catch (e) { + console.log(e); } - }); + } } public async canAdmin(colonyId: number, memberId: number): Promise { diff --git a/api/src/services/hood/hood.service.ts b/api/src/services/hood/hood.service.ts index 71b84b5a..23299969 100644 --- a/api/src/services/hood/hood.service.ts +++ b/api/src/services/hood/hood.service.ts @@ -10,6 +10,7 @@ import { } from '../../repositories'; import { Place } from '../../types/models'; import {includes} from 'lodash'; +import { RoleAssignmentService } from '../role-assignment/role-assignment.service'; /** Service for dealing with blocks */ @Service() @@ -21,6 +22,7 @@ export class HoodService { private roleAssignmentRepository: RoleAssignmentRepository, private roleRepository: RoleRepository, private memberRepository: MemberRepository, + private roleAssignmentService: RoleAssignmentService, ) {} public async find(hoodId: number): Promise { @@ -65,29 +67,14 @@ export class HoodService { newOwner = result[0].id; } } + // Both branches previously removed the old owner identically, so the removal is + // hoisted out rather than duplicated. + if (oldOwner !== 0) { + await this.roleAssignmentRepository.removeIdFromAssignment(hoodId, oldOwner, ownerCode); + await this.roleAssignmentService.reconcilePrimaryRole(oldOwner); + } if (newOwner !== 0) { - if (oldOwner !== 0) { - await this.roleAssignmentRepository.removeIdFromAssignment(hoodId, oldOwner, ownerCode); - const response: any = await this.memberRepository.getPrimaryRoleName(oldOwner); - if (response.length !== 0) { - const primaryRoleId = response[0].primary_role_id; - if (ownerCode === primaryRoleId){ - await this.memberRepository.update(oldOwner, {primary_role_id: null}); - } - } - } await this.roleAssignmentRepository.addIdToAssignment(hoodId, newOwner, ownerCode); - } else { - if (oldOwner !== 0) { - await this.roleAssignmentRepository.removeIdFromAssignment(hoodId, oldOwner, ownerCode); - const response: any = await this.memberRepository.getPrimaryRoleName(oldOwner); - if (response.length !== 0) { - const primaryRoleId = response[0].primary_role_id; - if (ownerCode === primaryRoleId){ - await this.memberRepository.update(oldOwner, {primary_role_id: null}); - } - } - } } data.deputies.forEach((deputies, index) => { oldDeputies[index] = deputies.member_id; @@ -95,44 +82,26 @@ export class HoodService { for (let i = 0; i < givenDeputies.length; i++) { newDeputies[i] = await this.updateDeputyId(givenDeputies[i]); } - oldDeputies.forEach((oldDeputies, index) => { - if (oldDeputies !== newDeputies[index]) { - if (newDeputies[index] === 0) { - try { - this.roleAssignmentRepository.removeIdFromAssignment(hoodId, oldDeputies, deputyCode); - } catch (e) { - console.log(e); - } - if (oldDeputies !== 0) { - this.memberRepository.getPrimaryRoleName(oldDeputies) - .then((response: any) => { - if (response.length !== 0) { - const primaryRoleId = response[0].primary_role_id; - if (primaryRoleId && deputyCode === primaryRoleId) { - this.memberRepository.update(oldDeputies, {primary_role_id: null}); - } - } - }); - } - } else { - try { - this.roleAssignmentRepository.removeIdFromAssignment(hoodId, oldDeputies, deputyCode); - this.memberRepository.getPrimaryRoleName(oldDeputies) - .then((response: any) => { - if (response.length !== 0) { - const primaryRoleId = response[0].primary_role_id; - if (deputyCode === primaryRoleId) { - this.memberRepository.update(oldDeputies, {primary_role_id: null}); - } - } - }); - this.roleAssignmentRepository.addIdToAssignment(hoodId, newDeputies[index], deputyCode); - } catch (e) { - console.log(e); - } + // Was a forEach containing un-awaited promise chains, so the primary-role write + // could land after the request had already returned. A for loop lets these await. + for (let index = 0; index < oldDeputies.length; index++) { + const oldDeputy = oldDeputies[index]; + const newDeputy = newDeputies[index]; + if (oldDeputy === newDeputy) continue; + try { + if (oldDeputy !== 0) { + await this.roleAssignmentRepository + .removeIdFromAssignment(hoodId, oldDeputy, deputyCode); + await this.roleAssignmentService.reconcilePrimaryRole(oldDeputy); + } + if (newDeputy !== 0) { + await this.roleAssignmentRepository + .addIdToAssignment(hoodId, newDeputy, deputyCode); } + } catch (e) { + console.log(e); } - }); + } } public async getColony(hoodId: number): Promise { diff --git a/api/src/services/place/place.service.ts b/api/src/services/place/place.service.ts index 6f3c1090..393f3407 100644 --- a/api/src/services/place/place.service.ts +++ b/api/src/services/place/place.service.ts @@ -16,6 +16,7 @@ import { ClubMemberRepository, } from '../../repositories'; import { Place, ObjectInstance } from '../../types/models'; +import { RoleAssignmentService } from '../role-assignment/role-assignment.service'; /** Service for dealing with blocks */ @Service() @@ -34,6 +35,7 @@ export class PlaceService { private mapLocationRepository: MapLocationRepository, private homeRepository: HomeRepository, private clubMemberRepository: ClubMemberRepository, + private roleAssignmentService: RoleAssignmentService, ) { } public async canAdmin(slug: string, placeId: number, memberId: number): @@ -252,29 +254,14 @@ export class PlaceService { newOwner = result[0].id; } } + // Both branches previously removed the old owner identically, so the removal is + // hoisted out rather than duplicated. + if (oldOwner !== 0) { + await this.roleAssignmentRepository.removeIdFromAssignment(placeId, oldOwner, ownerCode); + await this.roleAssignmentService.reconcilePrimaryRole(oldOwner); + } if (newOwner !== 0) { - if (oldOwner !== 0) { - await this.roleAssignmentRepository.removeIdFromAssignment(placeId, oldOwner, ownerCode); - const response: any = await this.memberRepository.getPrimaryRoleName(oldOwner); - if (response.length !== 0) { - const primaryRoleId = response[0].primary_role_id; - if (ownerCode === primaryRoleId) { - await this.memberRepository.update(oldOwner, { primary_role_id: null }); - } - } - } await this.roleAssignmentRepository.addIdToAssignment(placeId, newOwner, ownerCode); - } else { - if (oldOwner !== 0) { - await this.roleAssignmentRepository.removeIdFromAssignment(placeId, oldOwner, ownerCode); - const response: any = await this.memberRepository.getPrimaryRoleName(oldOwner); - if (response.length !== 0) { - const primaryRoleId = response[0].primary_role_id; - if (ownerCode === primaryRoleId) { - await this.memberRepository.update(oldOwner, { primary_role_id: null }); - } - } - } } data.deputies.forEach((deputies, index) => { oldDeputies[index] = deputies.member_id; @@ -282,45 +269,26 @@ export class PlaceService { for (let i = 0; i < givenDeputies.length; i++) { newDeputies[i] = await this.updateDeputyId(givenDeputies[i]); } - oldDeputies.forEach((oldDeputies, index) => { - if (oldDeputies !== newDeputies[index]) { - if (newDeputies[index] === 0) { - try { - this.roleAssignmentRepository.removeIdFromAssignment(placeId, oldDeputies, deputyCode); - } catch (e) { - console.log(e); - } - if (oldDeputies !== 0) { - this.memberRepository.getPrimaryRoleName(oldDeputies) - .then((response: any) => { - if (response.length !== 0) { - const primaryRoleId = response[0].primary_role_id; - if (primaryRoleId && deputyCode === primaryRoleId) { - this.memberRepository.update(oldDeputies, { primary_role_id: null }); - } - } - }); - } - } else { - try { - this.roleAssignmentRepository.removeIdFromAssignment(placeId, oldDeputies, deputyCode); - this.memberRepository.getPrimaryRoleName(oldDeputies) - .then((response: any) => { - if (response.length !== 0) { - const primaryRoleId = response[0].primary_role_id; - if (deputyCode === primaryRoleId) { - this.memberRepository.update(oldDeputies, { primary_role_id: null }); - } - } - }); - this.roleAssignmentRepository - .addIdToAssignment(placeId, newDeputies[index], deputyCode); - } catch (e) { - console.log(e); - } + // Was a forEach containing un-awaited promise chains, so the primary-role write + // could land after the request had already returned. A for loop lets these await. + for (let index = 0; index < oldDeputies.length; index++) { + const oldDeputy = oldDeputies[index]; + const newDeputy = newDeputies[index]; + if (oldDeputy === newDeputy) continue; + try { + if (oldDeputy !== 0) { + await this.roleAssignmentRepository + .removeIdFromAssignment(placeId, oldDeputy, deputyCode); + await this.roleAssignmentService.reconcilePrimaryRole(oldDeputy); + } + if (newDeputy !== 0) { + await this.roleAssignmentRepository + .addIdToAssignment(placeId, newDeputy, deputyCode); } + } catch (e) { + console.log(e); } - }); + } } public async updatePlaces(placeinfo: any): Promise { diff --git a/api/src/services/role-assignment/role-assignment.service.spec.ts b/api/src/services/role-assignment/role-assignment.service.spec.ts index 8a2b8b05..ac596032 100644 --- a/api/src/services/role-assignment/role-assignment.service.spec.ts +++ b/api/src/services/role-assignment/role-assignment.service.spec.ts @@ -2,15 +2,18 @@ import { Container } from 'typedi'; import { createSpyObj } from 'jest-createspyobj'; import { RoleAssignmentService } from './role-assignment.service'; -import { RoleAssignmentRepository } from '../../repositories'; +import { MemberRepository, RoleAssignmentRepository } from '../../repositories'; describe('RoleAssignmentService', () => { + let memberRepository: jest.Mocked; let roleAssignmentRepository: jest.Mocked; let service: RoleAssignmentService; beforeEach(() => { + memberRepository = createSpyObj(MemberRepository); roleAssignmentRepository = createSpyObj(RoleAssignmentRepository); Container.reset(); + Container.set(MemberRepository, memberRepository); Container.set(RoleAssignmentRepository, roleAssignmentRepository); service = Container.get(RoleAssignmentService); }); @@ -18,4 +21,64 @@ describe('RoleAssignmentService', () => { it('should create', () => { expect(service).toBeTruthy(); }); + + describe('reconcilePrimaryRole', () => { + const MEMBER_ID = 11; + const BLOCK_LEADER = 18; + const HOOD_DEPUTY = 20; + + const assignments = (...roleIds: number[]) => + roleIds.map(role_id => ({ member_id: MEMBER_ID, role_id, place_id: 1 })) as any; + + describe('when the displayed role is still held', () => { + it('leaves it alone', async () => { + memberRepository.getPrimaryRoleId.mockResolvedValue(BLOCK_LEADER); + roleAssignmentRepository.getByMemberId + .mockResolvedValue(assignments(BLOCK_LEADER, HOOD_DEPUTY)); + await service.reconcilePrimaryRole(MEMBER_ID); + expect(memberRepository.update).not.toHaveBeenCalled(); + }); + }); + + describe('when the displayed role is no longer held', () => { + it('clears it', async () => { + memberRepository.getPrimaryRoleId.mockResolvedValue(BLOCK_LEADER); + roleAssignmentRepository.getByMemberId.mockResolvedValue(assignments(HOOD_DEPUTY)); + await service.reconcilePrimaryRole(MEMBER_ID); + expect(memberRepository.update) + .toHaveBeenCalledWith(MEMBER_ID, { primary_role_id: null }); + }); + }); + + /** + * The bug in the code this replaced. It compared the *revoked* role against + * primary_role_id, so losing Block Leader cleared the display role of a member who + * still held Neighborhood Deputy. Reconciling against remaining assignments keeps + * a role the member still holds. + */ + describe('when another role is lost but the displayed one is retained', () => { + it('does not clear the displayed role', async () => { + memberRepository.getPrimaryRoleId.mockResolvedValue(HOOD_DEPUTY); + roleAssignmentRepository.getByMemberId.mockResolvedValue(assignments(HOOD_DEPUTY)); + await service.reconcilePrimaryRole(MEMBER_ID); + expect(memberRepository.update).not.toHaveBeenCalled(); + }); + }); + + describe('when no role is displayed', () => { + it('does nothing and does not query assignments', async () => { + memberRepository.getPrimaryRoleId.mockResolvedValue(null); + await service.reconcilePrimaryRole(MEMBER_ID); + expect(roleAssignmentRepository.getByMemberId).not.toHaveBeenCalled(); + expect(memberRepository.update).not.toHaveBeenCalled(); + }); + }); + + describe('when given a falsy member id', () => { + it('does nothing', async () => { + await service.reconcilePrimaryRole(0); + expect(memberRepository.getPrimaryRoleId).not.toHaveBeenCalled(); + }); + }); + }); }); diff --git a/api/src/services/role-assignment/role-assignment.service.ts b/api/src/services/role-assignment/role-assignment.service.ts index 954e62e6..6ac5f1d4 100644 --- a/api/src/services/role-assignment/role-assignment.service.ts +++ b/api/src/services/role-assignment/role-assignment.service.ts @@ -20,6 +20,35 @@ export class RoleAssignmentService { const response = this.roleAssignmentRepository.getByMemberId(memberId); return response; } + + /** + * Clears member.primary_role_id if it is no longer a role the member holds. + * + * Call after any change to a member's role assignments. role_assignment is the + * authority for what a member holds; primary_role_id only records which of those they + * chose to display, so it has to be re-checked whenever assignments change. + * + * Replaces a block that was copy-pasted across the hood, block, colony, place and + * admin services. That version compared the *revoked* role against primary_role_id + * and nulled the column on a match, which is wrong in one direction: a member holding + * both Block Leader and Neighborhood Deputy who lost Block Leader had their displayed + * role cleared even though they still held another. Checking the assignments that + * remain, rather than the single one just removed, fixes that. + * + * The old version was also fire-and-forget inside forEach callbacks, so the write + * could land after the request completed. This awaits. + */ + public async reconcilePrimaryRole(memberId: number): Promise { + if (!memberId) return; + const current = await this.memberRepository.getPrimaryRoleId(memberId); + if (current === null || current === undefined) return; + const assignments = await this.roleAssignmentRepository.getByMemberId(memberId); + const stillHeld = assignments + .some(assignment => Number(assignment.role_id) === Number(current)); + if (!stillHeld) { + await this.memberRepository.update(memberId, { primary_role_id: null }); + } + } /** * Grabs all payments due to users from database 50 at a time and From 5fc6caad4a824157ea4c576cf998f6d79c8d627c Mon Sep 17 00:00:00 2001 From: DJAscendance Date: Thu, 30 Jul 2026 08:19:37 -0400 Subject: [PATCH 2/6] feat: seed role_assignment so offices are actually held roles (05/06/09) and places (02/03/04) were both seeded but nothing joined them, so role_assignment was empty and no member held a place-scoped office. That is why access rights looked broken: the table is correctly shaped -- (member_id, role_id, place_id), structurally the CS 4.x rolemember record -- and simply had no rows. Synthetic fixtures, so permission behaviour is testable now; real officeholders come later via the admin UI. Roles resolve BY NAME, never by id. roles_data.json carries only {name, income_xp, income_cc}, so ids come from auto-increment insert order and hardcoding them would silently repoint every assignment if that file were ever reordered. role has a UNIQUE(name) index, so name lookup is stable. The fixture members cannot be logged into: their password column holds a bcrypt hash of a random value discarded at authoring time, so no password matches. Emails use the reserved .invalid TLD. They exist to hold roles, not to be used. Verified against a throwaway MySQL 5.7: 53 assignments over 12 colonies, 6 hoods and 8 blocks; every role lands on the correct place type; the city-wide City Guide role carries a null place_id; and re-running leaves counts unchanged at 12 members / 53 assignments / 12 wallets. --- api/db/seed/11-role-assignments.seed.ts | 177 ++++++++++++++++++++++++ 1 file changed, 177 insertions(+) create mode 100644 api/db/seed/11-role-assignments.seed.ts diff --git a/api/db/seed/11-role-assignments.seed.ts b/api/db/seed/11-role-assignments.seed.ts new file mode 100644 index 00000000..d602cec0 --- /dev/null +++ b/api/db/seed/11-role-assignments.seed.ts @@ -0,0 +1,177 @@ +import { Knex } from 'knex'; + +/** + * Seeds role_assignment, which nothing previously populated. + * + * roles (05/06/09) and places (02/03/04) are both seeded, but nothing joined them, so + * role_assignment was empty and no member held a place-scoped office. That is why access + * rights appeared broken: the table is correctly shaped -- (member_id, role_id, place_id), + * structurally the CS 4.x `rolemember` record -- and simply had no rows. + * + * These are synthetic fixtures so permission behaviour is testable now; real officeholders + * come later through the admin UI. Consistent with the rest of this directory, the seed is + * destructive and dev-only: 04-places.hoods.seed.ts already deletes every map_location and + * all hood/block places, so nothing here is safe to run against production either. + * + * Three deliberate choices: + * + * 1. Roles are resolved BY NAME, never by id. roles_data.json carries only + * {name, income_xp, income_cc} -- ids come from auto-increment insert order, so + * hardcoding them would silently repoint every assignment if that file were ever + * reordered. The `role` table has a UNIQUE(name) index, so name lookup is stable. + * + * 2. The fixture members cannot be logged into. Their password column holds a bcrypt hash + * of a random value that was discarded at authoring time, so no password matches. They + * exist to hold roles, not to be used as accounts. Give one a real password through the + * app if you need to sign in as it. + * + * 3. Emails use the reserved .invalid TLD (RFC 2606) so they can never collide with, or be + * mistaken for, a real address. + */ + +/** bcrypt hash of a discarded random string -- intentionally unmatchable. */ +const UNUSABLE_PASSWORD = '$2b$10$dl2N8WvzlGiQdf/AZzNwZejA9a/aRXZKWAJaOrycJt/wP1eScczKS'; + +const FIXTURE_PREFIX = 'fixture_'; +const FIXTURE_COUNT = 12; + +/** How many hoods and blocks to staff. Kept small so the fixture stays readable. */ +const HOODS_TO_STAFF = 6; +const BLOCKS_TO_STAFF = 8; + +type RoleIds = Record; + +async function resolveRoles(knex: Knex, names: string[]): Promise { + const rows = await knex('role').select('id', 'name').whereIn('name', names); + const byName: RoleIds = {}; + for (const row of rows) byName[row.name] = row.id; + + const missing = names.filter(name => !(name in byName)); + if (missing.length) { + throw new Error( + `role_assignment seed: roles not found by name: ${missing.join(', ')}. ` + + 'Run the role seeds (05/09) first.', + ); + } + return byName; +} + +function fixtureUsername(index: number): string { + return `${FIXTURE_PREFIX}${String(index).padStart(2, '0')}`; +} + +async function removePreviousFixtures(knex: Knex): Promise { + const existing = await knex('member') + .select('id', 'wallet_id') + .where('username', 'like', `${FIXTURE_PREFIX}%`); + if (!existing.length) return; + + const memberIds = existing.map(member => member.id); + const walletIds = existing.map(member => member.wallet_id).filter(Boolean); + + console.log(`Removing ${memberIds.length} previous fixture members`); + await knex('role_assignment').whereIn('member_id', memberIds).del(); + await knex('member').whereIn('id', memberIds).del(); + if (walletIds.length) await knex('wallet').whereIn('id', walletIds).del(); +} + +async function createFixtureMembers(knex: Knex): Promise { + const memberIds: number[] = []; + for (let index = 1; index <= FIXTURE_COUNT; index++) { + const username = fixtureUsername(index); + // member.wallet_id is notNullable, unique and a foreign key, so each needs its own. + const [walletId] = await knex('wallet').insert({}); + const [memberId] = await knex('member').insert({ + username, + email: `${username}@example.invalid`, + password: UNUSABLE_PASSWORD, + wallet_id: walletId, + }); + memberIds.push(memberId); + } + console.log( + `Created ${memberIds.length} fixture members ` + + `(${fixtureUsername(1)}..${fixtureUsername(FIXTURE_COUNT)})`, + ); + return memberIds; +} + +export async function seed(knex: Knex): Promise { + console.log('Seeding role assignments'); + + const roles = await resolveRoles(knex, [ + 'Colony Leader', 'Colony Deputy', + 'Neighborhood Leader', 'Neighborhood Deputy', + 'Block Leader', 'Block Deputy', + 'City Guide', + ]); + + await removePreviousFixtures(knex); + const members = await createFixtureMembers(knex); + + // Ordered deterministically so re-running produces the same assignments. + const colonies = await knex('place').select('id', 'name') + .where('type', 'colony').orderBy('id'); + const hoods = await knex('place').select('id', 'name') + .where('type', 'hood').orderBy('id').limit(HOODS_TO_STAFF); + const blocks = await knex('place').select('id', 'name') + .where('type', 'block').orderBy('id').limit(BLOCKS_TO_STAFF); + + if (!colonies.length) { + throw new Error( + 'role_assignment seed: no colony places found. Run the place seeds (02/03/04) first.', + ); + } + + const assignments: { member_id: number; role_id: number; place_id: number }[] = []; + const assign = (memberIndex: number, roleId: number, placeId: number) => + assignments.push({ + member_id: members[memberIndex % members.length], + role_id: roleId, + place_id: placeId, + }); + + // Colonies: leader + deputy, drawn from the front of the pool. + colonies.forEach((colony, i) => { + assign(i, roles['Colony Leader'], colony.id); + assign(i + 1, roles['Colony Deputy'], colony.id); + }); + + // Hoods and blocks: offset into the pool so the same members pick up several offices at + // different levels. That overlap is the point -- it is what exercises multi-role + // reconciliation and, once hierarchical inheritance lands, authority flowing downward. + hoods.forEach((hood, i) => { + assign(i + 2, roles['Neighborhood Leader'], hood.id); + assign(i + 3, roles['Neighborhood Deputy'], hood.id); + }); + blocks.forEach((block, i) => { + assign(i + 5, roles['Block Leader'], block.id); + assign(i + 6, roles['Block Deputy'], block.id); + }); + + // A city-wide role with no place scope. place_id is nullable precisely for these: the RE + // distinguishes city offices from the per-instance geographic roles above. + assign(FIXTURE_COUNT - 1, roles['City Guide'], null as any); + + await knex('role_assignment').insert(assignments); + + const perRole = assignments.reduce>((acc, a) => { + acc[a.role_id] = (acc[a.role_id] || 0) + 1; + return acc; + }, {}); + const nameById = Object.fromEntries(Object.entries(roles).map(([n, id]) => [id, n])); + + console.log(`Inserted ${assignments.length} role assignments across ` + + `${colonies.length} colonies, ${hoods.length} hoods, ${blocks.length} blocks:`); + for (const [roleId, count] of Object.entries(perRole)) { + console.log(` ${count.toString().padStart(3)} ${nameById[roleId]}`); + } + + const multiOffice = Object.values( + assignments.reduce>((acc, a) => { + acc[a.member_id] = (acc[a.member_id] || 0) + 1; + return acc; + }, {}), + ).filter(count => count > 1).length; + console.log(` ${multiOffice} fixture members hold more than one office`); +} From e514c60eac3ba2b5528e8011e6e853b0ed1a93ba Mon Sep 17 00:00:00 2001 From: DJAscendance Date: Thu, 30 Jul 2026 08:36:19 -0400 Subject: [PATCH 3/6] feat: add the role-grant access axis (place_role_access) CS 4.x gives every place two independent access axes. CTR had one. Axis 1, identity: an owner slot plus up to eight deputy slots, held in role_assignment and read via getAccessInfoByID. Already present. Axis 2, role grant: any role check-marked to grant write access to EVERY holder. Had no representation at all, so "let every City Guide write here" could not be expressed and place owners had to name eight individuals instead. That is a delegation ceiling, not a cosmetic gap. Adds place_role_access (place_id, role_id) with UNIQUE(place_id, role_id), a repository, and PlaceAccessService.canWrite resolving owner -> deputy -> role grant, in the original's order. The unconfigured default is OPEN, matching the shipped UI: if no nickname and no role is set, all members may write. canWrite therefore refuses a falsy member id outright -- the rule is that all MEMBERS may write, and a visitor carries only the Visitor bit, which satisfies nothing. Tested explicitly. Two deliberate omissions, both documented in the migration: - No capability bitfield (read/change/write/delete). Presence of a row means write, matching a UI that offers one checkbox per role. If capabilities are ever added they must carry the denial bookkeeping the original omitted: 4.1's delete branch recorded grants but never denials, so an explicit denial was indistinguishable from silence and fell through to the hierarchical walk, which could grant it from an ancestor. Reproduce the model, not the bug. - No foreign key on place_id. 04-places.hoods.seed.ts deletes and recreates every hood and block, and an FK would block that exactly as the vote_list FK already does. Orphans are swept by pruneOrphans instead. The hierarchical walk is still absent -- that is the next task. Verified against a throwaway MySQL 5.7 with the full migrate+seed chain: table built with the intended keys; granting City Guide at a block resolves that holder as 'role-grant' while an unrelated member and a visitor are both denied; setGrantedRoles replaces and dedupes; pruneOrphans removes dangling rows. Suite 20 -> 29 passing, same 5 pre-existing DB-dependent failures. --- ...20260730130000_create_place_role_access.ts | 61 +++++++++ api/src/db/db.class.ts | 3 + api/src/repositories/index.ts | 1 + .../place-role-access.repository.ts | 98 ++++++++++++++ api/src/services/index.ts | 1 + .../place-access/place-access.service.spec.ts | 121 ++++++++++++++++++ .../place-access/place-access.service.ts | 121 ++++++++++++++++++ api/src/types/models/index.ts | 1 + .../types/models/place-role-access.model.ts | 14 ++ 9 files changed, 421 insertions(+) create mode 100644 api/db/migrations/20260730130000_create_place_role_access.ts create mode 100644 api/src/repositories/place-role-access/place-role-access.repository.ts create mode 100644 api/src/services/place-access/place-access.service.spec.ts create mode 100644 api/src/services/place-access/place-access.service.ts create mode 100644 api/src/types/models/place-role-access.model.ts diff --git a/api/db/migrations/20260730130000_create_place_role_access.ts b/api/db/migrations/20260730130000_create_place_role_access.ts new file mode 100644 index 00000000..98b1d5d0 --- /dev/null +++ b/api/db/migrations/20260730130000_create_place_role_access.ts @@ -0,0 +1,61 @@ +import { Knex } from 'knex'; + +/** + * The second access axis: roles check-marked to grant write access at a place. + * + * CS 4.x gives every place two independent axes (see the CS 4.1 research notes, + * "Access rights"): + * + * 1. Up to eight identity entries, each a Group or a Member. CTR already has this as + * owner-plus-deputies in role_assignment, resolved by + * RoleAssignmentRepository.getAccessInfoByID. + * 2. Any role may be check-marked to grant write access to EVERYONE holding it, across + * the full role list. CTR had no representation for this at all, which is why + * "let every City Guide write here" could not be expressed and place owners had to + * name eight individuals instead. + * + * This table is axis 2. A row means "holders of role_id may write at place_id". + * + * Deliberately NOT included: the capability bitfield (read 0x01 / change 0x02 / + * write 0x04 / delete 0x08). Presence of a row means write access, matching the shipped + * UI, which offers a checkbox per role and nothing finer. Adding capabilities is tracked + * separately, and the research notes are emphatic that if they are added they must come + * with the denial bookkeeping the original omitted -- the 4.1 delete branch recorded + * grants but never denials, so an explicit denial was indistinguishable from silence and + * fell through to the hierarchical walk, which could then grant it from an ancestor. + * That is an authority-escalation path. Reproduce the model, not the bug. + * + * place_id has no foreign key on purpose: place rows are deleted and recreated wholesale + * by 04-places.hoods.seed.ts, and an FK here would block that the same way the vote_list + * FK already does. Orphan rows are pruned by pruneOrphans in the repository. + */ + +const COLLATE = 'utf8mb4_unicode_ci'; +const tableName = 'place_role_access'; + +export async function up(knex: Knex): Promise { + if (await knex.schema.hasTable(tableName)) return; + + console.log(`Creating ${tableName} table`); + await knex.schema.createTable(tableName, table => { + table.collate(COLLATE); + table.increments('id').primary(); + table.timestamps(false, true); + + table.integer('place_id').unsigned().notNullable(); + + table.integer('role_id').unsigned().notNullable(); + table.foreign('role_id').references('role.id'); + + // One grant per (place, role); re-granting is a no-op rather than a duplicate. + table.unique(['place_id', 'role_id']); + // Every read is "which roles are granted at this place". + table.index(['place_id']); + }); +} + +export async function down(knex: Knex): Promise { + if (!await knex.schema.hasTable(tableName)) return; + console.log(`Dropping ${tableName} table`); + await knex.schema.dropTable(tableName); +} diff --git a/api/src/db/db.class.ts b/api/src/db/db.class.ts index 42a8104e..37ef08af 100644 --- a/api/src/db/db.class.ts +++ b/api/src/db/db.class.ts @@ -49,6 +49,9 @@ export class Db { get roleAssignment() { return this.knex('role_assignment'); } + get placeRoleAccess() { + return this.knex('place_role_access'); + } get transaction() { return this.knex('transaction'); } diff --git a/api/src/repositories/index.ts b/api/src/repositories/index.ts index 7936f133..e4958cdb 100644 --- a/api/src/repositories/index.ts +++ b/api/src/repositories/index.ts @@ -14,6 +14,7 @@ export * from './object/object.repository'; export * from './object-instance/object-instance.repository'; export * from './role/role.repository'; export * from './role-assignment/role-assignment.repository'; +export * from './place-role-access/place-role-access.repository'; export * from './place/place.repository'; export * from './transaction/transaction.repository'; export * from './vote/vote.repository'; diff --git a/api/src/repositories/place-role-access/place-role-access.repository.ts b/api/src/repositories/place-role-access/place-role-access.repository.ts new file mode 100644 index 00000000..078d152a --- /dev/null +++ b/api/src/repositories/place-role-access/place-role-access.repository.ts @@ -0,0 +1,98 @@ +import { Service } from 'typedi'; + +import { Db } from '../../db'; +import { PlaceRoleAccess } from '../../types/models'; + +/** + * Reads and writes the role-grant access axis: which roles are check-marked to give + * write access at a place. + * + * Complements RoleAssignmentRepository.getAccessInfoByID, which covers the other axis + * (the owner and deputy identity slots). + */ +@Service() +export class PlaceRoleAccessRepository { + constructor(private db: Db) {} + + /** Role ids granted write access at this place. */ + public async getRoleIdsByPlace(placeId: number): Promise { + const rows = await this.db.knex('place_role_access') + .select('role_id') + .where('place_id', placeId) + .orderBy('role_id'); + return rows.map(row => row.role_id); + } + + /** Granted roles with their names, for rendering the checkbox list. */ + public async getRolesByPlace(placeId: number): Promise<{ id: number; name: string }[]> { + return this.db.knex('place_role_access') + .select('role.id', 'role.name') + .where('place_role_access.place_id', placeId) + .innerJoin('role', 'place_role_access.role_id', 'role.id') + .orderBy('role.name'); + } + + /** + * Replaces the grants for a place with exactly `roleIds`. + * + * A single transaction, so a failure part-way cannot leave a place with a half-applied + * access list -- which would silently widen or narrow who can write there. + */ + public async setRolesForPlace(placeId: number, roleIds: number[]): Promise { + const unique = [...new Set(roleIds.map(Number))].filter(id => Number.isInteger(id) && id > 0); + await this.db.knex.transaction(async trx => { + await trx('place_role_access').where('place_id', placeId).del(); + if (unique.length) { + await trx('place_role_access').insert( + unique.map(roleId => ({ place_id: placeId, role_id: roleId })), + ); + } + }); + } + + /** + * True if the member holds at least one role granted at this place. + * + * Joins role_assignment, which is the authority for what a member holds. Note the + * member's assignment may be scoped to a different place: holding "City Guide" anywhere + * satisfies a City Guide grant here, which is the point of the axis -- it grants by + * role, not by locality. Locality is what the owner/deputy axis and the hierarchy walk + * are for. + */ + public async memberHasGrantedRole(placeId: number, memberId: number): Promise { + const row = await this.db.knex('place_role_access') + .select('place_role_access.id') + .where('place_role_access.place_id', placeId) + .innerJoin( + 'role_assignment', + 'place_role_access.role_id', + 'role_assignment.role_id', + ) + .where('role_assignment.member_id', memberId) + .first(); + return !!row; + } + + /** Every grant for a place, used by callers that need the raw rows. */ + public async findByPlace(placeId: number): Promise { + return this.db.placeRoleAccess.where({ place_id: placeId }); + } + + public async removeAllForPlace(placeId: number): Promise { + await this.db.knex('place_role_access').where('place_id', placeId).del(); + } + + /** + * Deletes grants pointing at places that no longer exist. + * + * place_id deliberately carries no foreign key -- 04-places.hoods.seed.ts deletes and + * recreates every hood and block, and an FK would block that the way the vote_list FK + * already does. The cost of that choice is orphans, so they are swept here rather than + * left to accumulate. + */ + public async pruneOrphans(): Promise { + return this.db.knex('place_role_access') + .whereNotIn('place_id', this.db.knex('place').select('id')) + .del(); + } +} diff --git a/api/src/services/index.ts b/api/src/services/index.ts index 9c0b10ec..fbee5763 100644 --- a/api/src/services/index.ts +++ b/api/src/services/index.ts @@ -15,6 +15,7 @@ export * from './object-instance/object-instance.service'; export * from './role/role.service'; export * from './role-assignment/role-assignment.service'; export * from './place/place.service'; +export * from './place-access/place-access.service'; export * from './wallet/wallet.service'; export * from './messageboard/messageboard.service'; export * from './inbox/inbox.service'; diff --git a/api/src/services/place-access/place-access.service.spec.ts b/api/src/services/place-access/place-access.service.spec.ts new file mode 100644 index 00000000..39d1970e --- /dev/null +++ b/api/src/services/place-access/place-access.service.spec.ts @@ -0,0 +1,121 @@ +import { Container } from 'typedi'; +import { createSpyObj } from 'jest-createspyobj'; + +import { PlaceAccessService } from './place-access.service'; +import { + PlaceRoleAccessRepository, + RoleAssignmentRepository, +} from '../../repositories'; + +describe('PlaceAccessService', () => { + const PLACE_ID = 42; + const MEMBER_ID = 11; + const OWNER_CODE = 18; + const DEPUTY_CODE = 19; + + let placeRoleAccessRepository: jest.Mocked; + let roleAssignmentRepository: jest.Mocked; + let service: PlaceAccessService; + + /** Nobody in the identity slots, no role grants: the unconfigured baseline. */ + const emptyAccess = () => { + roleAssignmentRepository.getAccessInfoByID.mockResolvedValue({ owner: [], deputies: [] }); + placeRoleAccessRepository.memberHasGrantedRole.mockResolvedValue(false); + placeRoleAccessRepository.getRoleIdsByPlace.mockResolvedValue([]); + }; + + beforeEach(() => { + placeRoleAccessRepository = createSpyObj(PlaceRoleAccessRepository); + roleAssignmentRepository = createSpyObj(RoleAssignmentRepository); + Container.reset(); + Container.set(PlaceRoleAccessRepository, placeRoleAccessRepository); + Container.set(RoleAssignmentRepository, roleAssignmentRepository); + service = Container.get(PlaceAccessService); + emptyAccess(); + }); + + it('should create', () => { + expect(service).toBeTruthy(); + }); + + describe('canWrite', () => { + describe('when the member is the owner', () => { + it('allows, without consulting the role grants', async () => { + roleAssignmentRepository.getAccessInfoByID.mockResolvedValue({ + owner: [{ member_id: MEMBER_ID }], deputies: [], + }); + const result = await service.canWrite(PLACE_ID, MEMBER_ID, OWNER_CODE, DEPUTY_CODE); + expect(result).toEqual({ allowed: true, reason: 'owner' }); + expect(placeRoleAccessRepository.memberHasGrantedRole).not.toHaveBeenCalled(); + }); + }); + + describe('when the member is a deputy', () => { + it('allows', async () => { + roleAssignmentRepository.getAccessInfoByID.mockResolvedValue({ + owner: [{ member_id: 999 }], deputies: [{ member_id: MEMBER_ID }], + }); + const result = await service.canWrite(PLACE_ID, MEMBER_ID, OWNER_CODE, DEPUTY_CODE); + expect(result).toEqual({ allowed: true, reason: 'deputy' }); + }); + }); + + /** The whole point of the axis: grant by role, not by naming individuals. */ + describe('when the member holds a granted role but is neither owner nor deputy', () => { + it('allows via the role grant', async () => { + roleAssignmentRepository.getAccessInfoByID.mockResolvedValue({ + owner: [{ member_id: 999 }], deputies: [], + }); + placeRoleAccessRepository.memberHasGrantedRole.mockResolvedValue(true); + const result = await service.canWrite(PLACE_ID, MEMBER_ID, OWNER_CODE, DEPUTY_CODE); + expect(result).toEqual({ allowed: true, reason: 'role-grant' }); + }); + }); + + /** + * The shipped UI's rule: if no nickname and no role is set, all members may write. + * Faithful to the original, and the reason canWrite refuses a falsy member id. + */ + describe('when neither axis is configured', () => { + it('allows any member', async () => { + const result = await service.canWrite(PLACE_ID, MEMBER_ID, OWNER_CODE, DEPUTY_CODE); + expect(result).toEqual({ allowed: true, reason: 'unrestricted' }); + }); + }); + + describe('when the place is configured and the member matches nothing', () => { + it('denies', async () => { + roleAssignmentRepository.getAccessInfoByID.mockResolvedValue({ + owner: [{ member_id: 999 }], deputies: [{ member_id: 998 }], + }); + const result = await service.canWrite(PLACE_ID, MEMBER_ID, OWNER_CODE, DEPUTY_CODE); + expect(result).toEqual({ allowed: false, reason: 'denied' }); + }); + + it('denies when only a role grant is configured and the member lacks it', async () => { + placeRoleAccessRepository.getRoleIdsByPlace.mockResolvedValue([OWNER_CODE]); + const result = await service.canWrite(PLACE_ID, MEMBER_ID, OWNER_CODE, DEPUTY_CODE); + expect(result).toEqual({ allowed: false, reason: 'denied' }); + }); + }); + + /** + * A visitor must never benefit from the open default. The original gives an + * unauthenticated caller only the Visitor bit, which satisfies nothing. + */ + describe('when there is no member id (a visitor)', () => { + it('denies even on a completely unconfigured place', async () => { + const result = await service.canWrite(PLACE_ID, 0, OWNER_CODE, DEPUTY_CODE); + expect(result).toEqual({ allowed: false, reason: 'denied' }); + expect(roleAssignmentRepository.getAccessInfoByID).not.toHaveBeenCalled(); + }); + }); + }); + + describe('memberHasGrantedRole', () => { + it('is false without a member id, and does not hit the database', async () => { + expect(await service.memberHasGrantedRole(PLACE_ID, 0)).toBe(false); + expect(placeRoleAccessRepository.memberHasGrantedRole).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/api/src/services/place-access/place-access.service.ts b/api/src/services/place-access/place-access.service.ts new file mode 100644 index 00000000..dffa46f5 --- /dev/null +++ b/api/src/services/place-access/place-access.service.ts @@ -0,0 +1,121 @@ +import { Service } from 'typedi'; + +import { + PlaceRoleAccessRepository, + RoleAssignmentRepository, +} from '../../repositories'; + +/** Why a write was allowed or refused. Useful in logs and worth surfacing in the UI. */ +export type WriteAccessReason = + | 'owner' + | 'deputy' + | 'role-grant' + | 'unrestricted' + | 'denied'; + +export interface WriteAccessResult { + allowed: boolean; + reason: WriteAccessReason; +} + +/** + * Resolves write access at a place across both CS 4.x access axes. + * + * Axis 1, identity: the owner slot plus up to eight deputy slots, stored in + * role_assignment and read via RoleAssignmentRepository.getAccessInfoByID. CTR already + * had this. + * + * Axis 2, role grant: any role check-marked to grant write access to every holder, + * stored in place_role_access. CTR had no representation for this, so + * "let every City Guide write here" was inexpressible and owners had to name eight + * individuals instead. + * + * Resolution order follows the original (see the CS 4.1 research notes, "Access rights"): + * owner, then the deputy slots, then the role grant. The original then consulted a + * rolemask and, failing that, walked up the place tree; that walk is not implemented here + * yet -- see the note in canWrite. + * + * The default when nothing is configured is OPEN, not closed: the shipped UI states that + * if no nickname and no role is set, all members may write. That is deliberately + * faithful, and it is why canWrite must be given a real member id -- see below. + */ +@Service() +export class PlaceAccessService { + constructor( + private placeRoleAccessRepository: PlaceRoleAccessRepository, + private roleAssignmentRepository: RoleAssignmentRepository, + ) {} + + /** Roles currently granted write access at this place, with names for display. */ + public async getGrantedRoles(placeId: number): Promise<{ id: number; name: string }[]> { + return this.placeRoleAccessRepository.getRolesByPlace(placeId); + } + + /** Replaces the granted roles for a place with exactly this set. */ + public async setGrantedRoles(placeId: number, roleIds: number[]): Promise { + await this.placeRoleAccessRepository.setRolesForPlace(placeId, roleIds); + } + + /** True if the member holds any role granted at this place. */ + public async memberHasGrantedRole(placeId: number, memberId: number): Promise { + if (!placeId || !memberId) return false; + return this.placeRoleAccessRepository.memberHasGrantedRole(placeId, memberId); + } + + /** + * Resolves whether the member may write at this place. + * + * ownerCode and deputyCode are the role ids that mean "owner of" and "deputy of" for + * this kind of place -- Block Leader / Block Deputy, Colony Leader / Colony Deputy and + * so on. They are passed in rather than derived because CTR already resolves them + * per place type at the call site (see PlaceService.findRoleIdsBySlug and the + * per-service getAccessInfoByUsername methods), and duplicating that mapping here + * would create a second source of truth for it. + * + * A falsy memberId is treated as a visitor and always refused. That matters because + * the unconfigured default is open: the original's rule is that all MEMBERS may write, + * and visitors never qualify -- an unauthenticated caller carries only the Visitor bit, + * which never satisfies any grant. + * + * Not yet implemented: the hierarchical walk. In the original, a request neither + * granted nor denied locally re-checks the parent place, three levels up to district, + * so a district leader holds authority beneath without a per-place grant. Until that + * lands, authority does not flow downward through this method -- though note + * BlockService.canAdmin and its siblings already open-code a block -> hood -> colony + * check of their own. + */ + public async canWrite( + placeId: number, + memberId: number, + ownerCode: number, + deputyCode: number, + ): Promise { + if (!memberId) return { allowed: false, reason: 'denied' }; + + const identity = await this.roleAssignmentRepository + .getAccessInfoByID(placeId, ownerCode, deputyCode); + + if (identity.owner.some(entry => entry.member_id === memberId)) { + return { allowed: true, reason: 'owner' }; + } + if (identity.deputies.some(entry => entry.member_id === memberId)) { + return { allowed: true, reason: 'deputy' }; + } + if (await this.memberHasGrantedRole(placeId, memberId)) { + return { allowed: true, reason: 'role-grant' }; + } + + // Nothing configured on either axis: the original leaves the place open to all + // members. Only reached once both axes have been checked and found empty. + const grantedRoles = await this.placeRoleAccessRepository.getRoleIdsByPlace(placeId); + const unconfigured = + identity.owner.length === 0 && + identity.deputies.length === 0 && + grantedRoles.length === 0; + if (unconfigured) { + return { allowed: true, reason: 'unrestricted' }; + } + + return { allowed: false, reason: 'denied' }; + } +} diff --git a/api/src/types/models/index.ts b/api/src/types/models/index.ts index f6e807f6..4f9dec26 100644 --- a/api/src/types/models/index.ts +++ b/api/src/types/models/index.ts @@ -13,6 +13,7 @@ export * from './object-instance.position.model'; export * from './object-instance.rotation.model'; export * from './role.model'; export * from './role-assignment.model'; +export * from './place-role-access.model'; export * from './place.model'; export * from './transaction.model'; export * from './wallet.model'; diff --git a/api/src/types/models/place-role-access.model.ts b/api/src/types/models/place-role-access.model.ts new file mode 100644 index 00000000..11125ef9 --- /dev/null +++ b/api/src/types/models/place-role-access.model.ts @@ -0,0 +1,14 @@ +import { Model } from './model'; + +/** + * A role check-marked to grant write access at a place. + * + * The second access axis. A row means every holder of role_id may write at place_id, + * independently of the owner/deputy identity slots. + */ +export interface PlaceRoleAccess extends Model { + /** ID of the place the grant applies to */ + place_id: number; + /** ID of the role being granted write access */ + role_id: number; +} From 74a64810cfe482878b73d28fdda02059cf5aa014 Mon Sep 17 00:00:00 2001 From: DJAscendance Date: Thu, 30 Jul 2026 08:51:03 -0400 Subject: [PATCH 4/6] feat: generalise hierarchical authority into one walk Authority inherits DOWN the place tree: a colony leader holds it over the hoods and blocks beneath them without appearing in any of those places' owner or deputy slots. That was already true in CTR, but written three times -- ColonyService, HoodService and BlockService each open-coded the same logic at depths 1, 2 and 3 with hardcoded role sets, and a fourth place type would have needed a fourth copy. Replaced by PlaceAccessService.getAncestry + hasGeographicAuthority, which walk map_location and drive the per-level offices from a table keyed on place.type. The three canAdmin methods now delegate. Behaviour is preserved: global Admin / Colony Representative, or the Leader/Deputy pair for a level held at that level's place. Two things the shared version gets right that the copies did not: - Non-recursing place types. The research notes are specific that city, office and club places do not recurse, so the walk stops at a club rather than letting a colony leader inherit into it. None of the three copies could express that. - roleMap is no longer read directly. RoleRepository populates it from an un-awaited constructor call, so for a window after startup it is `{}`, every lookup is undefined, and `[undefined, ...].includes(role_id)` is false -- which quietly DENIES real admins. Added RoleRepository.awaitRoleMap, which joins that same population instead of racing it. Fails closed, so a correctness bug rather than a security hole, but a confusing one. canWrite now also consults inherited authority, reported as reason 'inherited'. Deliberately NOT tightened: the original consults the hierarchy only when a request was neither granted nor denied locally, so an explicit local denial stops the walk. Here inherited authority applies even to a place with an access list the member is absent from, matching what CTR's existing canAdmin already does. Tightening it so a block owner could shut out their colony leader is a product decision, not something a refactor should slip in. canManageAccess is left alone in all three services: its role sets are deliberately narrower (Leader but not Deputy), so it is a different question. 21 tests, including authority flowing down but not up, an office being scoped to its own place, clubs not recursing, and cycle termination. Verified against a throwaway MySQL 5.7 with real seeded geography: block 36 -> hood 35 -> colony 23 resolved from map_location; colony and hood leaders both reach the block; the block leader does not reach the colony. Suite 29 -> 41 passing, same 5 pre-existing DB-dependent failures. --- api/src/repositories/role/role.repository.ts | 24 ++- api/src/services/block/block.service.ts | 45 ++---- api/src/services/colony/colony.service.ts | 32 ++-- api/src/services/hood/hood.service.ts | 39 ++--- .../place-access/place-access.service.spec.ts | 140 ++++++++++++++++++ .../place-access/place-access.service.ts | 132 ++++++++++++++++- 6 files changed, 326 insertions(+), 86 deletions(-) diff --git a/api/src/repositories/role/role.repository.ts b/api/src/repositories/role/role.repository.ts index faed8e72..8af2992f 100644 --- a/api/src/repositories/role/role.repository.ts +++ b/api/src/repositories/role/role.repository.ts @@ -7,10 +7,32 @@ import { Role } from '../../types/models'; @Service() export class RoleRepository { constructor(private db: Db) { - this.populateRoleMap(); + // Kept eager so existing direct readers of roleMap behave as before; the promise is + // retained so awaitRoleMap can join this same population rather than starting another. + this.roleMapReady = this.populateRoleMap(); } public roleMap: any = {}; + /** Memoized in-flight/settled population, so awaitRoleMap resolves once and is shared. */ + private roleMapReady: Promise | null = null; + + /** + * Resolves once roleMap is populated, then returns it. + * + * The constructor kicks off populateRoleMap without awaiting it -- it cannot await, and + * typedi gives no async construction hook. So for a window after startup roleMap is + * still `{}`, every lookup on it is `undefined`, and an authorization test of the form + * `[roleMap.Admin, ...].includes(assignment.role_id)` is comparing against undefined and + * quietly returns false. That denies legitimate admins until the query settles. It fails + * closed rather than open, so it is a correctness bug rather than a security hole, but + * new code should await this instead of reading roleMap directly. + */ + public async awaitRoleMap(): Promise> { + if (!this.roleMapReady) this.roleMapReady = this.populateRoleMap(); + await this.roleMapReady; + return this.roleMap; + } + private async populateRoleMap(): Promise { const roles = await this.findAll(); diff --git a/api/src/services/block/block.service.ts b/api/src/services/block/block.service.ts index 34da31cb..9a6c1e71 100644 --- a/api/src/services/block/block.service.ts +++ b/api/src/services/block/block.service.ts @@ -11,6 +11,7 @@ import { import {Member, Place} from '../../types/models'; import {includes} from 'lodash'; import { RoleAssignmentService } from '../role-assignment/role-assignment.service'; +import { PlaceAccessService } from '../place-access/place-access.service'; /** Service for dealing with blocks */ @Service() @@ -23,6 +24,7 @@ export class BlockService { private roleRepository: RoleRepository, private memberRepository: MemberRepository, private roleAssignmentService: RoleAssignmentService, + private placeAccessService: PlaceAccessService, ) {} public async find(blockId: number): Promise { @@ -121,40 +123,17 @@ export class BlockService { return await this.mapLocationRepository.createAvailableLocation(blockId, location); } + /** + * Delegates to the shared hierarchy walk, which resolves block -> hood -> colony from + * map_location instead of the two hand-written lookups this used to do. + * + * Behaviour is unchanged: global Admin / Colony Representative, or the Leader/Deputy pair + * for any level at that level's place. It also picks up a fix -- the old version read + * roleRepository.roleMap directly, which is populated by an un-awaited constructor call + * and so is empty for a window after startup, quietly denying real admins. + */ public async canAdmin(blockId: number, memberId: number): Promise { - const roleAssignments = await this.roleAssignmentRepository.getByMemberId(memberId); - const hood = await this.getHood(blockId); - const hoodMapLocation = await this.mapLocationRepository.findPlaceIdMapLocation(hood.id); - const colonyId = hoodMapLocation.parent_place_id; - - if ( - roleAssignments.find(assignment => { - return ( - [ - this.roleRepository.roleMap.Admin, - this.roleRepository.roleMap.ColonyRepresentative, - ].includes(assignment.role_id) || - ([ - this.roleRepository.roleMap.ColonyLeader, - this.roleRepository.roleMap.ColonyDeputy, - ].includes(assignment.role_id) && - assignment.place_id === colonyId) || - ([ - this.roleRepository.roleMap.NeighborhoodDeputy, - this.roleRepository.roleMap.NeighborhoodLeader, - ].includes(assignment.role_id) && - assignment.place_id === hood.id) || - ([ - this.roleRepository.roleMap.BlockDeputy, - this.roleRepository.roleMap.BlockLeader, - ].includes(assignment.role_id) && - assignment.place_id === blockId) - ); - }) - ) { - return true; - } - return false; + return this.placeAccessService.hasGeographicAuthority(blockId, memberId); } public async canManageAccess(blockId: number, memberId: number): Promise { diff --git a/api/src/services/colony/colony.service.ts b/api/src/services/colony/colony.service.ts index de6d6e16..e7bc7408 100644 --- a/api/src/services/colony/colony.service.ts +++ b/api/src/services/colony/colony.service.ts @@ -10,6 +10,7 @@ import { Place } from '../../types/models'; import * as console from 'console'; import { includes } from 'lodash'; import { RoleAssignmentService } from '../role-assignment/role-assignment.service'; +import { PlaceAccessService } from '../place-access/place-access.service'; /** Service for dealing with colony */ @Service() @@ -20,6 +21,7 @@ export class ColonyService { private roleRepository: RoleRepository, private memberRepository: MemberRepository, private roleAssignmentService: RoleAssignmentService, + private placeAccessService: PlaceAccessService, ) { } public async find(colonyId: number): Promise { @@ -105,27 +107,17 @@ export class ColonyService { } } + /** + * Delegates to the shared hierarchy walk. Previously open-coded here, in HoodService and + * in BlockService as three copies of the same logic at depths 1, 2 and 3. + * + * Behaviour is unchanged: global Admin / Colony Representative, or Colony Leader / + * Colony Deputy held at this colony. It also picks up a fix -- the old version read + * roleRepository.roleMap directly, which is populated by an un-awaited constructor call + * and so is empty for a window after startup, quietly denying real admins. + */ public async canAdmin(colonyId: number, memberId: number): Promise { - const roleAssignments = await this.roleAssignmentRepository.getByMemberId(memberId); - - if ( - roleAssignments.find(assignment => { - return ( - [ - this.roleRepository.roleMap.Admin, - this.roleRepository.roleMap.ColonyRepresentative, - ].includes(assignment.role_id) || - ([ - this.roleRepository.roleMap.ColonyLeader, - this.roleRepository.roleMap.ColonyDeputy, - ].includes(assignment.role_id) && - assignment.place_id === colonyId) - ); - }) - ) { - return true; - } - else return false; + return this.placeAccessService.hasGeographicAuthority(colonyId, memberId); } public async canManageAccess(colonyId: number, memberId: number): Promise { diff --git a/api/src/services/hood/hood.service.ts b/api/src/services/hood/hood.service.ts index 23299969..ac5a4246 100644 --- a/api/src/services/hood/hood.service.ts +++ b/api/src/services/hood/hood.service.ts @@ -11,6 +11,7 @@ import { import { Place } from '../../types/models'; import {includes} from 'lodash'; import { RoleAssignmentService } from '../role-assignment/role-assignment.service'; +import { PlaceAccessService } from '../place-access/place-access.service'; /** Service for dealing with blocks */ @Service() @@ -23,6 +24,7 @@ export class HoodService { private roleRepository: RoleRepository, private memberRepository: MemberRepository, private roleAssignmentService: RoleAssignmentService, + private placeAccessService: PlaceAccessService, ) {} public async find(hoodId: number): Promise { @@ -113,33 +115,18 @@ export class HoodService { return await this.hoodRepository.getBlocks(hoodId); } + /** + * Delegates to the shared hierarchy walk, which resolves the hood -> colony chain from + * map_location rather than fetching the colony by hand. + * + * Behaviour is unchanged: global Admin / Colony Representative, Colony Leader or Deputy + * at the parent colony, or Neighborhood Leader or Deputy at this hood. It also picks up + * a fix -- the old version read roleRepository.roleMap directly, which is populated by an + * un-awaited constructor call and so is empty for a window after startup, quietly + * denying real admins. + */ public async canAdmin(hoodId: number, memberId: number): Promise { - const roleAssignments = await this.roleAssignmentRepository.getByMemberId(memberId); - const colony = await this.getColony(hoodId); - - if ( - roleAssignments.find(assignment => { - return ( - [ - this.roleRepository.roleMap.Admin, - this.roleRepository.roleMap.ColonyRepresentative, - ].includes(assignment.role_id) || - ([ - this.roleRepository.roleMap.ColonyLeader, - this.roleRepository.roleMap.ColonyDeputy, - ].includes(assignment.role_id) && - assignment.place_id === colony.id) || - ([ - this.roleRepository.roleMap.NeighborhoodDeputy, - this.roleRepository.roleMap.NeighborhoodLeader, - ].includes(assignment.role_id) && - assignment.place_id === hoodId) - ); - }) - ) { - return true; - } - return false; + return this.placeAccessService.hasGeographicAuthority(hoodId, memberId); } public async canManageAccess(hoodId: number, memberId: number): Promise { diff --git a/api/src/services/place-access/place-access.service.spec.ts b/api/src/services/place-access/place-access.service.spec.ts index 39d1970e..9dd21991 100644 --- a/api/src/services/place-access/place-access.service.spec.ts +++ b/api/src/services/place-access/place-access.service.spec.ts @@ -3,8 +3,11 @@ import { createSpyObj } from 'jest-createspyobj'; import { PlaceAccessService } from './place-access.service'; import { + MapLocationRepository, + PlaceRepository, PlaceRoleAccessRepository, RoleAssignmentRepository, + RoleRepository, } from '../../repositories'; describe('PlaceAccessService', () => { @@ -13,8 +16,26 @@ describe('PlaceAccessService', () => { const OWNER_CODE = 18; const DEPUTY_CODE = 19; + /** Role ids as the seeds would produce them -- arbitrary, resolved by name. */ + const ROLE_IDS: Record = { + Admin: 1, + ColonyRepresentative: 2, + ColonyLeader: 3, + ColonyDeputy: 4, + NeighborhoodLeader: 5, + NeighborhoodDeputy: 6, + BlockLeader: 7, + BlockDeputy: 8, + }; + const COLONY_ID = 100; + const HOOD_ID = 200; + const BLOCK_ID = 300; + + let mapLocationRepository: jest.Mocked; + let placeRepository: jest.Mocked; let placeRoleAccessRepository: jest.Mocked; let roleAssignmentRepository: jest.Mocked; + let roleRepository: jest.Mocked; let service: PlaceAccessService; /** Nobody in the identity slots, no role grants: the unconfigured baseline. */ @@ -22,16 +43,41 @@ describe('PlaceAccessService', () => { roleAssignmentRepository.getAccessInfoByID.mockResolvedValue({ owner: [], deputies: [] }); placeRoleAccessRepository.memberHasGrantedRole.mockResolvedValue(false); placeRoleAccessRepository.getRoleIdsByPlace.mockResolvedValue([]); + roleAssignmentRepository.getByMemberId.mockResolvedValue([]); + }; + + /** block(300) inside hood(200) inside colony(100), wired through map_location. */ + const geography = (blockType = 'block') => { + const places: Record = { + [BLOCK_ID]: { id: BLOCK_ID, type: blockType }, + [HOOD_ID]: { id: HOOD_ID, type: 'hood' }, + [COLONY_ID]: { id: COLONY_ID, type: 'colony' }, + }; + const parents: Record = { [BLOCK_ID]: HOOD_ID, [HOOD_ID]: COLONY_ID }; + placeRepository.findById.mockImplementation( + async (id: number) => places[id] as any, + ); + mapLocationRepository.findPlaceIdMapLocation.mockImplementation( + async (id: number) => ({ parent_place_id: parents[id] }) as any, + ); }; beforeEach(() => { + mapLocationRepository = createSpyObj(MapLocationRepository); + placeRepository = createSpyObj(PlaceRepository); placeRoleAccessRepository = createSpyObj(PlaceRoleAccessRepository); roleAssignmentRepository = createSpyObj(RoleAssignmentRepository); + roleRepository = createSpyObj(RoleRepository); + roleRepository.awaitRoleMap.mockResolvedValue(ROLE_IDS); Container.reset(); + Container.set(MapLocationRepository, mapLocationRepository); + Container.set(PlaceRepository, placeRepository); Container.set(PlaceRoleAccessRepository, placeRoleAccessRepository); Container.set(RoleAssignmentRepository, roleAssignmentRepository); + Container.set(RoleRepository, roleRepository); service = Container.get(PlaceAccessService); emptyAccess(); + geography(); }); it('should create', () => { @@ -118,4 +164,98 @@ describe('PlaceAccessService', () => { expect(placeRoleAccessRepository.memberHasGrantedRole).not.toHaveBeenCalled(); }); }); + + describe('getAncestry', () => { + it('walks block -> hood -> colony and stops at the colony', async () => { + expect(await service.getAncestry(BLOCK_ID)).toEqual([ + { id: BLOCK_ID, type: 'block' }, + { id: HOOD_ID, type: 'hood' }, + { id: COLONY_ID, type: 'colony' }, + ]); + }); + + /** + * The research notes are specific that city, office and club places do not recurse, so + * a colony leader must not gain authority inside a club merely because it sits beneath + * them in the map. + */ + it('does not recurse out of a club', async () => { + geography('club'); + expect(await service.getAncestry(BLOCK_ID)).toEqual([{ id: BLOCK_ID, type: 'club' }]); + }); + + it('terminates on a cycle rather than looping', async () => { + placeRepository.findById.mockImplementation( + async (id: number) => ({ id, type: 'block' }) as any, + ); + mapLocationRepository.findPlaceIdMapLocation.mockImplementation( + async () => ({ parent_place_id: BLOCK_ID }) as any, + ); + const chain = await service.getAncestry(BLOCK_ID); + expect(chain).toEqual([{ id: BLOCK_ID, type: 'block' }]); + }); + }); + + describe('hasGeographicAuthority', () => { + const holding = (roleId: number, placeId: number | null) => + roleAssignmentRepository.getByMemberId.mockResolvedValue( + [{ member_id: MEMBER_ID, role_id: roleId, place_id: placeId }] as any, + ); + + /** The point of the whole task: authority flows downward. */ + it('grants a colony leader authority over a block beneath them', async () => { + holding(ROLE_IDS.ColonyLeader, COLONY_ID); + expect(await service.hasGeographicAuthority(BLOCK_ID, MEMBER_ID)).toBe(true); + }); + + it('grants a hood deputy authority over a block beneath them', async () => { + holding(ROLE_IDS.NeighborhoodDeputy, HOOD_ID); + expect(await service.hasGeographicAuthority(BLOCK_ID, MEMBER_ID)).toBe(true); + }); + + it('grants a block leader authority over their own block', async () => { + holding(ROLE_IDS.BlockLeader, BLOCK_ID); + expect(await service.hasGeographicAuthority(BLOCK_ID, MEMBER_ID)).toBe(true); + }); + + /** Authority must not flow the other way. */ + it('denies a block leader authority over the hood above them', async () => { + holding(ROLE_IDS.BlockLeader, BLOCK_ID); + expect(await service.hasGeographicAuthority(HOOD_ID, MEMBER_ID)).toBe(false); + }); + + /** An office is scoped to its own place, not to the level generally. */ + it('denies a leader of a different colony', async () => { + holding(ROLE_IDS.ColonyLeader, 999); + expect(await service.hasGeographicAuthority(BLOCK_ID, MEMBER_ID)).toBe(false); + }); + + it('grants global Admin everywhere, without walking the tree', async () => { + holding(ROLE_IDS.Admin, null); + expect(await service.hasGeographicAuthority(BLOCK_ID, MEMBER_ID)).toBe(true); + expect(placeRepository.findById).not.toHaveBeenCalled(); + }); + + it('denies a member holding no roles', async () => { + expect(await service.hasGeographicAuthority(BLOCK_ID, MEMBER_ID)).toBe(false); + }); + + it('denies a visitor without querying', async () => { + expect(await service.hasGeographicAuthority(BLOCK_ID, 0)).toBe(false); + expect(roleAssignmentRepository.getByMemberId).not.toHaveBeenCalled(); + }); + }); + + describe('canWrite with inherited authority', () => { + it('allows a colony leader to write in a block owned by someone else', async () => { + roleAssignmentRepository.getAccessInfoByID.mockResolvedValue({ + owner: [{ member_id: 999 }], deputies: [], + }); + roleAssignmentRepository.getByMemberId.mockResolvedValue( + [{ member_id: MEMBER_ID, role_id: ROLE_IDS.ColonyLeader, place_id: COLONY_ID }] as any, + ); + const result = await service.canWrite(BLOCK_ID, MEMBER_ID, OWNER_CODE, DEPUTY_CODE); + expect(result).toEqual({ allowed: true, reason: 'inherited' }); + }); + }); }); diff --git a/api/src/services/place-access/place-access.service.ts b/api/src/services/place-access/place-access.service.ts index dffa46f5..c6887dfe 100644 --- a/api/src/services/place-access/place-access.service.ts +++ b/api/src/services/place-access/place-access.service.ts @@ -1,8 +1,11 @@ import { Service } from 'typedi'; import { + MapLocationRepository, + PlaceRepository, PlaceRoleAccessRepository, RoleAssignmentRepository, + RoleRepository, } from '../../repositories'; /** Why a write was allowed or refused. Useful in logs and worth surfacing in the UI. */ @@ -10,6 +13,7 @@ export type WriteAccessReason = | 'owner' | 'deputy' | 'role-grant' + | 'inherited' | 'unrestricted' | 'denied'; @@ -41,11 +45,115 @@ export interface WriteAccessResult { */ @Service() export class PlaceAccessService { + /** + * The offices that confer authority over a place of each type -- and, because authority + * inherits downward, over everything beneath it. + * + * Names, not ids: role ids come from auto-increment insert order in roles_data.json, so + * hardcoding them would break silently if that file were reordered. + */ + private static readonly OFFICES_BY_TYPE: Record = { + colony: ['ColonyLeader', 'ColonyDeputy'], + hood: ['NeighborhoodLeader', 'NeighborhoodDeputy'], + block: ['BlockLeader', 'BlockDeputy'], + }; + + /** Roles holding authority everywhere, independent of the hierarchy. */ + private static readonly GLOBAL_OFFICES = ['Admin', 'ColonyRepresentative']; + + /** + * Place types that participate in the upward walk: home -> block -> hood -> colony, + * mirroring the original's property -> block -> neighborhood -> district. + * + * Deliberately excludes club, public and storage. The research notes are specific that + * city, office and club places do not recurse, so a colony leader does not automatically + * gain authority inside a club that happens to sit beneath them. + */ + private static readonly RECURSING_TYPES = ['home', 'block', 'hood', 'colony']; + + /** Safety stop. The real hierarchy is four deep; anything longer means a cycle. */ + private static readonly MAX_DEPTH = 8; + constructor( + private mapLocationRepository: MapLocationRepository, + private placeRepository: PlaceRepository, private placeRoleAccessRepository: PlaceRoleAccessRepository, private roleAssignmentRepository: RoleAssignmentRepository, + private roleRepository: RoleRepository, ) {} + /** + * The place itself followed by its ancestors, nearest first: home, block, hood, colony. + * + * Walks map_location upward. Stops as soon as a place's type is not one that recurses, + * so a club or a public place terminates the chain instead of leaking authority in from + * the colony above it. + */ + public async getAncestry(placeId: number): Promise<{ id: number; type: string }[]> { + const chain: { id: number; type: string }[] = []; + const seen = new Set(); + let currentId = placeId; + + for (let depth = 0; depth < PlaceAccessService.MAX_DEPTH; depth++) { + if (!currentId || seen.has(currentId)) break; + seen.add(currentId); + + const place = await this.placeRepository.findById(currentId); + if (!place) break; + chain.push({ id: place.id, type: place.type }); + + // Only recursing types continue upward. Note the check is on the place we just + // added: a club stops the walk at the club, it does not inherit from its parent. + if (!PlaceAccessService.RECURSING_TYPES.includes(place.type)) break; + if (place.type === 'colony') break; // district is the top; nothing above it inherits + + const location = await this.mapLocationRepository.findPlaceIdMapLocation(currentId); + if (!location || !location.parent_place_id) break; + currentId = location.parent_place_id; + } + return chain; + } + + /** + * True if the member holds an office granting authority over this place, either globally + * or at the place itself or any ancestor of it. + * + * This is the inherent-authority axis, distinct from a place's own access list: a colony + * leader holds authority over the hoods and blocks beneath them without appearing in any + * of those places' owner or deputy slots. The research notes put it plainly -- rights + * inherit up the place hierarchy, and a district leader therefore holds change rights + * over everything beneath. + * + * Replaces three separately hand-written walks in ColonyService, HoodService and + * BlockService, which implemented depths 1, 2 and 3 of this same logic with hardcoded + * role sets. Those could not be extended to a new place type without a fourth copy. + */ + public async hasGeographicAuthority(placeId: number, memberId: number): Promise { + if (!placeId || !memberId) return false; + + const [assignments, roleIds] = await Promise.all([ + this.roleAssignmentRepository.getByMemberId(memberId), + this.roleRepository.awaitRoleMap(), + ]); + if (!assignments.length) return false; + + const idsFor = (names: string[]) => + names.map(name => roleIds[name]).filter(id => id !== undefined); + + const globalIds = idsFor(PlaceAccessService.GLOBAL_OFFICES); + if (assignments.some(a => globalIds.includes(a.role_id))) return true; + + const ancestry = await this.getAncestry(placeId); + for (const place of ancestry) { + const officeIds = idsFor(PlaceAccessService.OFFICES_BY_TYPE[place.type] || []); + if (!officeIds.length) continue; + if (assignments.some(a => officeIds.includes(a.role_id) && a.place_id === place.id)) { + return true; + } + } + return false; + } + /** Roles currently granted write access at this place, with names for display. */ public async getGrantedRoles(placeId: number): Promise<{ id: number; name: string }[]> { return this.placeRoleAccessRepository.getRolesByPlace(placeId); @@ -77,12 +185,21 @@ export class PlaceAccessService { * and visitors never qualify -- an unauthenticated caller carries only the Visitor bit, * which never satisfies any grant. * - * Not yet implemented: the hierarchical walk. In the original, a request neither - * granted nor denied locally re-checks the parent place, three levels up to district, - * so a district leader holds authority beneath without a per-place grant. Until that - * lands, authority does not flow downward through this method -- though note - * BlockService.canAdmin and its siblings already open-code a block -> hood -> colony - * check of their own. + * One deliberate divergence from the original, worth stating because it is a loosening. + * + * The original consults the hierarchy only when a request was neither granted nor denied + * locally -- an explicit local denial stops the walk. (Or should: 4.1's delete branch + * never recorded denials, so denial was indistinguishable from silence and fell through + * to the walk anyway, which is the escalation defect the research notes say to fix + * rather than reproduce.) + * + * Here, hasGeographicAuthority is consulted even when the place has a configured access + * list that the member does not appear in. That matches what CTR already does -- the + * existing per-service canAdmin methods treat a colony leader's authority as + * unconditional -- and tightening it so a block owner could shut their colony leader out + * is a product decision, not something a refactor should slip in. If that tightening is + * ever wanted, it belongs above this comment, and it must record denials explicitly so + * "denied" and "no opinion" stay distinguishable. */ public async canWrite( placeId: number, @@ -104,6 +221,9 @@ export class PlaceAccessService { if (await this.memberHasGrantedRole(placeId, memberId)) { return { allowed: true, reason: 'role-grant' }; } + if (await this.hasGeographicAuthority(placeId, memberId)) { + return { allowed: true, reason: 'inherited' }; + } // Nothing configured on either axis: the original leaves the place open to all // members. Only reached once both axes have been checked and found empty. From 6e3e131b5c704ae834d9f8166c70fdf02afd0a94 Mon Sep 17 00:00:00 2001 From: Ryan Bundy Date: Thu, 30 Jul 2026 10:13:39 -0400 Subject: [PATCH 5/6] Address CodeRabbit review on access rights Three of the four findings from a local CodeRabbit pass. The fourth is a pre-existing bug in four services and is left for a decision rather than folded in here; see below. The fixture cleanup no longer deletes real accounts. FIXTURE_PREFIX is 'fixture_', and '_' is a single-character wildcard in LIKE, so `LIKE 'fixture_%'` also matched usernames like 'fixtures' or 'fixtureBob'. That query feeds a DELETE that takes the member row, their role_assignment rows and their wallet, so an over-match destroys a real account. The prefix is now escaped with an explicit ESCAPE clause. canManageAccess awaits roleMap in all three place services. It compares assignment.role_id against roleRepository.roleMap.Admin and friends, and roleMap is populated by an un-awaited constructor call -- so for a window after startup every lookup is undefined, `[undefined].includes(role_id)` is false, and a legitimate admin is quietly denied. canAdmin already got this fix by moving to placeAccessService; canManageAccess kept reading the map directly. Fixed with awaitRoleMap rather than by delegating to placeAccessService, because manage-access is deliberately narrower than canAdmin (Leader, not Deputy) and delegating would widen it. updatePlaceAccess no longer writes a deputy assignment for places that have no deputy role. findRoleIdsBySlug declared `deputy: number` but returns nothing for 'jail' and 'cityhall', which have an owner role and no deputy. The deputy sync would then call addIdToAssignment with an undefined role code, creating a role_assignment row pointing at no role. The sync is now skipped wholesale for those slugs -- a place with no deputy role has nothing to reconcile -- and the signature says `deputy?: number` so the next caller is told the truth by the compiler rather than by a bad row. Verified: tsc reports only the pre-existing missing 'sharp' module. eslint 0 errors. Suite compared against a stashed baseline suite-for- suite, not by count: identical, 41 passing and the same five failures, all MySQL connection errors needing a live database. Deliberately not done -- the fourth finding. The deputy reconciliation loop pairs old and new deputies BY INDEX, so it is order-sensitive: given old [A, B] and new [B, A], index 0 removes A and adds B, then index 1 removes B -- which just got added and is meant to stay -- and adds A. B loses the role and, worse, takes a reconcilePrimaryRole call while still a deputy, which is precisely the primary-role clearing this PR exists to fix. Set comparison is the correct model. Not fixed here because: it predates this PR (origin/master has the same pairing in a forEach), it is duplicated verbatim in block, hood, colony AND place, so fixing it properly means one shared helper plus tests rather than four edits, and it changes role_assignment write behaviour, which deserves its own reviewable commit rather than riding along in a review-response. --- api/db/seed/11-role-assignments.seed.ts | 17 ++++++++++++++++- api/src/services/block/block.service.ts | 8 ++++++++ api/src/services/colony/colony.service.ts | 10 ++++++++++ api/src/services/hood/hood.service.ts | 8 ++++++++ api/src/services/place/place.service.ts | 13 ++++++++++++- 5 files changed, 54 insertions(+), 2 deletions(-) diff --git a/api/db/seed/11-role-assignments.seed.ts b/api/db/seed/11-role-assignments.seed.ts index d602cec0..a673eb4d 100644 --- a/api/db/seed/11-role-assignments.seed.ts +++ b/api/db/seed/11-role-assignments.seed.ts @@ -60,10 +60,25 @@ function fixtureUsername(index: number): string { return `${FIXTURE_PREFIX}${String(index).padStart(2, '0')}`; } +/** + * Escapes the LIKE metacharacters in a literal prefix. + * + * FIXTURE_PREFIX ends in '_', which LIKE reads as "any single character" -- so the raw + * pattern also matched real usernames such as 'fixtures' or 'fixtureBob'. This function + * feeds a DELETE, so an over-match takes a real member's account, role assignments and + * wallet with it. + */ +const LIKE_ESCAPE = '!'; +function escapeLikeLiteral(value: string): string { + return value.replace(/[!%_]/g, character => `${LIKE_ESCAPE}${character}`); +} + async function removePreviousFixtures(knex: Knex): Promise { const existing = await knex('member') .select('id', 'wallet_id') - .where('username', 'like', `${FIXTURE_PREFIX}%`); + .whereRaw(`username LIKE ? ESCAPE '${LIKE_ESCAPE}'`, [ + `${escapeLikeLiteral(FIXTURE_PREFIX)}%`, + ]); if (!existing.length) return; const memberIds = existing.map(member => member.id); diff --git a/api/src/services/block/block.service.ts b/api/src/services/block/block.service.ts index 9a6c1e71..4b337e8c 100644 --- a/api/src/services/block/block.service.ts +++ b/api/src/services/block/block.service.ts @@ -136,7 +136,15 @@ export class BlockService { return this.placeAccessService.hasGeographicAuthority(blockId, memberId); } + /** + * Kept on its own role set rather than delegated to placeAccessService: manage-access is + * deliberately narrower than canAdmin (Leader, not Deputy). + * + * roleMap is awaited because the constructor populates it without awaiting, so for a + * window after startup every lookup is undefined and a real admin is denied. + */ public async canManageAccess(blockId: number, memberId: number): Promise { + await this.roleRepository.awaitRoleMap(); const roleAssignments = await this.roleAssignmentRepository.getByMemberId(memberId); const hood = await this.getHood(blockId); const hoodMapLocation = await this.mapLocationRepository.findPlaceIdMapLocation(hood.id); diff --git a/api/src/services/colony/colony.service.ts b/api/src/services/colony/colony.service.ts index e7bc7408..3fe94827 100644 --- a/api/src/services/colony/colony.service.ts +++ b/api/src/services/colony/colony.service.ts @@ -120,7 +120,17 @@ export class ColonyService { return this.placeAccessService.hasGeographicAuthority(colonyId, memberId); } + /** + * Left with its own role set rather than delegated to placeAccessService: manage-access is + * deliberately narrower than canAdmin (Leader, not Deputy), and that difference is the + * point of the method. + * + * The roleMap read is awaited for the same reason canAdmin no longer reads it directly -- + * it is populated by an un-awaited constructor call, so for a window after startup every + * lookup is undefined and `[undefined].includes(role_id)` denies a real admin. + */ public async canManageAccess(colonyId: number, memberId: number): Promise { + await this.roleRepository.awaitRoleMap(); const roleAssignments = await this.roleAssignmentRepository.getByMemberId(memberId); if ( diff --git a/api/src/services/hood/hood.service.ts b/api/src/services/hood/hood.service.ts index ac5a4246..18731c30 100644 --- a/api/src/services/hood/hood.service.ts +++ b/api/src/services/hood/hood.service.ts @@ -129,7 +129,15 @@ export class HoodService { return this.placeAccessService.hasGeographicAuthority(hoodId, memberId); } + /** + * Kept on its own role set rather than delegated to placeAccessService: manage-access is + * deliberately narrower than canAdmin (Leader, not Deputy). + * + * roleMap is awaited because the constructor populates it without awaiting, so for a + * window after startup every lookup is undefined and a real admin is denied. + */ public async canManageAccess(hoodId: number, memberId: number): Promise { + await this.roleRepository.awaitRoleMap(); const roleAssignments = await this.roleAssignmentRepository.getByMemberId(memberId); const colony = await this.getColony(hoodId); diff --git a/api/src/services/place/place.service.ts b/api/src/services/place/place.service.ts index 393f3407..c31cdde7 100644 --- a/api/src/services/place/place.service.ts +++ b/api/src/services/place/place.service.ts @@ -263,6 +263,12 @@ export class PlaceService { if (newOwner !== 0) { await this.roleAssignmentRepository.addIdToAssignment(placeId, newOwner, ownerCode); } + // 'jail' and 'cityhall' have an owner role but no deputy role, so findRoleIdsBySlug + // returns deputy: undefined for them. The sync below would then write a role_assignment + // whose role_id is undefined -- a row pointing at no role at all. Skipped wholesale + // rather than guarded per-branch: a place with no deputy role has no deputies to + // reconcile, so there is nothing for the loop to do either way. + if (deputyCode === undefined || deputyCode === null) return; data.deputies.forEach((deputies, index) => { oldDeputies[index] = deputies.member_id; }); @@ -295,7 +301,12 @@ export class PlaceService { return await this.placeRepository.updatePlaces(placeinfo); } - private async findRoleIdsBySlug(slug: string): Promise<{ owner: number, deputy: number }> { + /** + * `deputy` is optional because it genuinely is: 'jail' and 'cityhall' below have an owner + * role and no deputy role. The signature previously promised a number for every slug, + * which is how an undefined deputy role reached a role_assignment write. + */ + private async findRoleIdsBySlug(slug: string): Promise<{ owner: number, deputy?: number }> { const roleId = { bank: { owner: this.roleRepository.roleMap.BankManager, From 4871469e17d099614eb6acd608da84e14cdbaf1b Mon Sep 17 00:00:00 2001 From: Ryan Bundy Date: Thu, 30 Jul 2026 10:57:19 -0400 Subject: [PATCH 6/6] Address CodeRabbit review round two on access rights Four of five findings from the CodeRabbit App review of #6. The fifth is a heavy lift that needs its own change; see below. awaitRoleMap no longer poisons itself. The constructor's population promise had no .catch(), so a transient database error at startup was an unobserved rejection -- a warning normally, fatal under --unhandled-rejections=throw. Worse, the rejected promise stayed memoized in roleMapReady, so every later awaitRoleMap re-awaited the same rejection and the process could not recover without a restart. That turned the canManageAccess fix from the previous commit into a liability: those call sites would throw rather than fail closed. Population is now started through a helper that clears the memo on failure so the next caller retries, with an identity check so a late-settling older attempt cannot clear a newer one's memo. The eager rejection is observed and discarded. Kept deliberately: awaitRoleMap still rejects rather than returning a half-empty map. Returning {} would put callers back on the silent-denial path this method exists to close, telling a real admin "no" instead of "could not determine". The rejection reaches the controllers' existing try/catch, and the cleared memo means the next request retries. Six role-code reads were awaiting a number. `await roleMap.BlockDeputy` awaits an already-resolved value and waits for nothing, so it read the unpopulated map exactly as a bare access would -- the await was pure decoration. getAccessInfoByUsername and postAccessInfo in all three place services now await the map itself. place.service is fixed at findRoleIdsBySlug, which is the single point where every slug's codes are resolved, so all four of its callers are covered by one await. getAccessInfoByID no longer throws for places with no deputy role. 'jail' (Security Chief) and 'cityhall' (City Council) have an owner role and no deputy, and `.where('role_id', undefined)` makes knex throw "Undefined binding(s) detected", taking down the owner lookup along with it. The deputy query is skipped instead. Guarded in the repository because every caller had the same exposure -- and because the guard added for this in the previous commit sat AFTER the getAccessInfoByID call in place.service, so it could never have been reached. The seed validates before it destroys. removePreviousFixtures and createFixtureMembers ran before the place queries, so seeding a database with no colonies deleted the existing fixtures and only then threw, leaving it emptier than a failed seed found it. Reads and validation now come first, so a failed precondition is a no-op. Verified: tsc reports only the pre-existing missing 'sharp' module. eslint 0 errors on the eight changed files. Suite compared against a stashed baseline test-name by test-name: no regressions, and three tests went from failing to passing -- MemberService > createMemberAndLogin > {should not store the provided member password in clear text, should return a session token for the new member, should tell the database to create a member with the provided name and email}. Those were being killed by the uncaught constructor rejection, which is independent evidence the first finding was real and reached past authorization. Totals 41 -> 44 passing, 5 -> 2 failing. Deliberately not done -- the fifth finding. reconcilePrimaryRole observes a temporary gap: the callers remove an assignment, reconcile, then insert the replacement, so a member who keeps a role at a DIFFERENT place has primary_role_id cleared even though the final state still holds it. The fix is to reconcile after the complete mutation set inside one transaction, which changes the shape of every caller and wants a transfer regression test. It also affects syncDeputies on #9, which removes and adds in two passes. Doing it here would mix a transaction boundary change into a review-response commit. --- api/db/seed/11-role-assignments.seed.ts | 12 ++++-- .../role-assignment.repository.ts | 18 ++++++++- api/src/repositories/role/role.repository.ts | 38 ++++++++++++++++--- api/src/services/block/block.service.ts | 18 +++++++-- api/src/services/colony/colony.service.ts | 18 +++++++-- api/src/services/hood/hood.service.ts | 18 +++++++-- .../place-access/place-access.service.ts | 5 ++- api/src/services/place/place.service.ts | 6 +++ 8 files changed, 111 insertions(+), 22 deletions(-) diff --git a/api/db/seed/11-role-assignments.seed.ts b/api/db/seed/11-role-assignments.seed.ts index a673eb4d..f7c98591 100644 --- a/api/db/seed/11-role-assignments.seed.ts +++ b/api/db/seed/11-role-assignments.seed.ts @@ -121,9 +121,12 @@ export async function seed(knex: Knex): Promise { 'City Guide', ]); - await removePreviousFixtures(knex); - const members = await createFixtureMembers(knex); - + // Every prerequisite is read and validated BEFORE anything is deleted or created. The + // previous order removed the existing fixtures and recreated the member pool first, so a + // database with no colonies threw only after the old fixtures were already gone -- leaving + // it emptier than before a seed that failed. Reads first, then destructive work, so a + // failed precondition is a no-op. + // // Ordered deterministically so re-running produces the same assignments. const colonies = await knex('place').select('id', 'name') .where('type', 'colony').orderBy('id'); @@ -138,6 +141,9 @@ export async function seed(knex: Knex): Promise { ); } + await removePreviousFixtures(knex); + const members = await createFixtureMembers(knex); + const assignments: { member_id: number; role_id: number; place_id: number }[] = []; const assign = (memberIndex: number, roleId: number, placeId: number) => assignments.push({ diff --git a/api/src/repositories/role-assignment/role-assignment.repository.ts b/api/src/repositories/role-assignment/role-assignment.repository.ts index 480d5328..09589184 100644 --- a/api/src/repositories/role-assignment/role-assignment.repository.ts +++ b/api/src/repositories/role-assignment/role-assignment.repository.ts @@ -45,10 +45,23 @@ export class RoleAssignmentRepository { ); } + /** + * Owner and deputy holders at a place. + * + * deputyCode is optional because some places have an owner role and no deputy role at all: + * 'jail' (Security Chief) and 'cityhall' (City Council). Passing undefined through to + * `.where('role_id', undefined)` makes knex throw "Undefined binding(s) detected when + * compiling SELECT", which took down the entire call -- including the owner lookup, which + * would otherwise have been fine. A place with no deputy role has no deputies, so that + * query is skipped and `deputies` comes back empty instead. + * + * Guarded here rather than at each call site because every caller has the same exposure: + * canWrite, and postAccessInfo in the block, hood, colony and place services. + */ public async getAccessInfoByID( placeId, ownerCode, - deputyCode): Promise<{ owner: any[]; deputies: any[] }> { + deputyCode?): Promise<{ owner: any[]; deputies: any[] }> { const owner: any[] = await this.db.knex .select( 'member_id', @@ -56,6 +69,9 @@ export class RoleAssignmentRepository { .from('role_assignment') .where('place_id', placeId) .where('role_id', ownerCode); + if (deputyCode === undefined || deputyCode === null) { + return { deputies: [], owner }; + } const deputies: any[] = await this.db.knex .select( 'member_id', diff --git a/api/src/repositories/role/role.repository.ts b/api/src/repositories/role/role.repository.ts index 8af2992f..b004e988 100644 --- a/api/src/repositories/role/role.repository.ts +++ b/api/src/repositories/role/role.repository.ts @@ -9,13 +9,36 @@ export class RoleRepository { constructor(private db: Db) { // Kept eager so existing direct readers of roleMap behave as before; the promise is // retained so awaitRoleMap can join this same population rather than starting another. - this.roleMapReady = this.populateRoleMap(); + this.roleMapReady = this.startPopulate(); + // Nobody awaits the eager attempt, so its rejection would be an unobserved promise + // rejection -- a warning normally, and fatal under --unhandled-rejections=throw. It is + // observed and discarded here; startPopulate has already cleared the memo, so the next + // awaitRoleMap caller starts a fresh attempt rather than inheriting this failure. + this.roleMapReady.catch(() => undefined); } public roleMap: any = {}; /** Memoized in-flight/settled population, so awaitRoleMap resolves once and is shared. */ private roleMapReady: Promise | null = null; + /** + * Starts a population attempt and memoizes it, clearing the memo if it fails. + * + * Without the reset a single transient database error at startup was permanent: the + * rejected promise stayed in roleMapReady, so every later awaitRoleMap re-awaited the same + * rejection and the process could not recover without a restart. + * + * The identity check matters -- a late-settling older attempt must not clear a newer one's + * memo, which would leave two populations racing with no shared result. + */ + private startPopulate(): Promise { + const pending = this.populateRoleMap().catch(error => { + if (this.roleMapReady === pending) this.roleMapReady = null; + throw error; + }); + return pending; + } + /** * Resolves once roleMap is populated, then returns it. * @@ -23,12 +46,17 @@ export class RoleRepository { * typedi gives no async construction hook. So for a window after startup roleMap is * still `{}`, every lookup on it is `undefined`, and an authorization test of the form * `[roleMap.Admin, ...].includes(assignment.role_id)` is comparing against undefined and - * quietly returns false. That denies legitimate admins until the query settles. It fails - * closed rather than open, so it is a correctness bug rather than a security hole, but - * new code should await this instead of reading roleMap directly. + * quietly returns false. That denies legitimate admins until the query settles, so new + * code should await this instead of reading roleMap directly. + * + * This REJECTS if population fails rather than returning a half-empty map. That is + * deliberate: returning `{}` would put callers back on the silent-denial path this method + * exists to close, where a real admin is told "no" instead of "could not determine". The + * rejection reaches the controllers' existing try/catch as an error response, and because + * the memo is cleared the following request retries rather than inheriting the failure. */ public async awaitRoleMap(): Promise> { - if (!this.roleMapReady) this.roleMapReady = this.populateRoleMap(); + if (!this.roleMapReady) this.roleMapReady = this.startPopulate(); await this.roleMapReady; return this.roleMap; } diff --git a/api/src/services/block/block.service.ts b/api/src/services/block/block.service.ts index 4b337e8c..d6ec9200 100644 --- a/api/src/services/block/block.service.ts +++ b/api/src/services/block/block.service.ts @@ -37,8 +37,13 @@ export class BlockService { } public async getAccessInfoByUsername(blockId: number): Promise { - const deputyCode = await this.roleRepository.roleMap.BlockDeputy; - const ownerCode = await this.roleRepository.roleMap.BlockLeader; + // awaitRoleMap, not a bare roleMap read. The previous `await roleMap.X` awaited a + // NUMBER, which resolves immediately and waits for nothing -- so during the startup + // window before population these were both undefined and the role codes below + // silently addressed no role at all. + const roleMap = await this.roleRepository.awaitRoleMap(); + const deputyCode = roleMap.BlockDeputy; + const ownerCode = roleMap.BlockLeader; return await this.roleAssignmentRepository.getAccessInfoByUsername( blockId, ownerCode, @@ -54,8 +59,13 @@ export class BlockService { * old is coming from database * new is coming from access rights page */ - const deputyCode = await this.roleRepository.roleMap.BlockDeputy; - const ownerCode = await this.roleRepository.roleMap.BlockLeader; + // awaitRoleMap, not a bare roleMap read. The previous `await roleMap.X` awaited a + // NUMBER, which resolves immediately and waits for nothing -- so during the startup + // window before population these were both undefined and the role codes below + // silently addressed no role at all. + const roleMap = await this.roleRepository.awaitRoleMap(); + const deputyCode = roleMap.BlockDeputy; + const ownerCode = roleMap.BlockLeader; let oldOwner = null; let newOwner = 0; const oldDeputies = [0,0,0,0,0,0,0,0]; diff --git a/api/src/services/colony/colony.service.ts b/api/src/services/colony/colony.service.ts index 3fe94827..44e9f831 100644 --- a/api/src/services/colony/colony.service.ts +++ b/api/src/services/colony/colony.service.ts @@ -33,8 +33,13 @@ export class ColonyService { } public async getAccessInfoByUsername(colonyId: number): Promise { - const deputyCode = await this.roleRepository.roleMap.ColonyDeputy; - const ownerCode = await this.roleRepository.roleMap.ColonyLeader; + // awaitRoleMap, not a bare roleMap read. The previous `await roleMap.X` awaited a + // NUMBER, which resolves immediately and waits for nothing -- so during the startup + // window before population these were both undefined and the role codes below + // silently addressed no role at all. + const roleMap = await this.roleRepository.awaitRoleMap(); + const deputyCode = roleMap.ColonyDeputy; + const ownerCode = roleMap.ColonyLeader; return await this.roleAssignmentRepository.getAccessInfoByUsername( colonyId, ownerCode, @@ -50,8 +55,13 @@ export class ColonyService { * old is coming from database * new is coming from access rights page */ - const deputyCode = await this.roleRepository.roleMap.ColonyDeputy; - const ownerCode = await this.roleRepository.roleMap.ColonyLeader; + // awaitRoleMap, not a bare roleMap read. The previous `await roleMap.X` awaited a + // NUMBER, which resolves immediately and waits for nothing -- so during the startup + // window before population these were both undefined and the role codes below + // silently addressed no role at all. + const roleMap = await this.roleRepository.awaitRoleMap(); + const deputyCode = roleMap.ColonyDeputy; + const ownerCode = roleMap.ColonyLeader; let oldOwner = null; let newOwner = 0; const oldDeputies = [0, 0, 0, 0, 0, 0, 0, 0]; diff --git a/api/src/services/hood/hood.service.ts b/api/src/services/hood/hood.service.ts index 18731c30..06c5a22e 100644 --- a/api/src/services/hood/hood.service.ts +++ b/api/src/services/hood/hood.service.ts @@ -32,8 +32,13 @@ export class HoodService { } public async getAccessInfoByUsername(hoodId: number): Promise { - const deputyCode = await this.roleRepository.roleMap.NeighborhoodDeputy; - const ownerCode = await this.roleRepository.roleMap.NeighborhoodLeader; + // awaitRoleMap, not a bare roleMap read. The previous `await roleMap.X` awaited a + // NUMBER, which resolves immediately and waits for nothing -- so during the startup + // window before population these were both undefined and the role codes below + // silently addressed no role at all. + const roleMap = await this.roleRepository.awaitRoleMap(); + const deputyCode = roleMap.NeighborhoodDeputy; + const ownerCode = roleMap.NeighborhoodLeader; return await this.roleAssignmentRepository.getAccessInfoByUsername( hoodId, ownerCode, @@ -49,8 +54,13 @@ export class HoodService { * old is coming from database * new is coming from access rights page */ - const deputyCode = await this.roleRepository.roleMap.NeighborhoodDeputy; - const ownerCode = await this.roleRepository.roleMap.NeighborhoodLeader; + // awaitRoleMap, not a bare roleMap read. The previous `await roleMap.X` awaited a + // NUMBER, which resolves immediately and waits for nothing -- so during the startup + // window before population these were both undefined and the role codes below + // silently addressed no role at all. + const roleMap = await this.roleRepository.awaitRoleMap(); + const deputyCode = roleMap.NeighborhoodDeputy; + const ownerCode = roleMap.NeighborhoodLeader; let oldOwner = null; let newOwner = 0; const oldDeputies = [0,0,0,0,0,0,0,0]; diff --git a/api/src/services/place-access/place-access.service.ts b/api/src/services/place-access/place-access.service.ts index c6887dfe..a5ff4bd8 100644 --- a/api/src/services/place-access/place-access.service.ts +++ b/api/src/services/place-access/place-access.service.ts @@ -205,7 +205,10 @@ export class PlaceAccessService { placeId: number, memberId: number, ownerCode: number, - deputyCode: number, + // Optional: 'jail' and 'cityhall' have an owner role and no deputy role. The repository + // skips the deputy query rather than letting knex throw on an undefined binding, so this + // resolves to no deputies rather than to an error. + deputyCode?: number, ): Promise { if (!memberId) return { allowed: false, reason: 'denied' }; diff --git a/api/src/services/place/place.service.ts b/api/src/services/place/place.service.ts index c31cdde7..a0a83a85 100644 --- a/api/src/services/place/place.service.ts +++ b/api/src/services/place/place.service.ts @@ -305,8 +305,14 @@ export class PlaceService { * `deputy` is optional because it genuinely is: 'jail' and 'cityhall' below have an owner * role and no deputy role. The signature previously promised a number for every slug, * which is how an undefined deputy role reached a role_assignment write. + * + * roleMap is awaited here because this is the single place every slug's role codes are + * resolved -- all four callers get the fix from this one await. Without it the whole table + * below is built from an unpopulated map during the startup window, so every owner and + * deputy code is undefined. */ private async findRoleIdsBySlug(slug: string): Promise<{ owner: number, deputy?: number }> { + await this.roleRepository.awaitRoleMap(); const roleId = { bank: { owner: this.roleRepository.roleMap.BankManager,