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/db/seed/11-role-assignments.seed.ts b/api/db/seed/11-role-assignments.seed.ts new file mode 100644 index 00000000..f7c98591 --- /dev/null +++ b/api/db/seed/11-role-assignments.seed.ts @@ -0,0 +1,198 @@ +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')}`; +} + +/** + * 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') + .whereRaw(`username LIKE ? ESCAPE '${LIKE_ESCAPE}'`, [ + `${escapeLikeLiteral(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', + ]); + + // 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'); + 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.', + ); + } + + 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({ + 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`); +} 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/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/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/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 faed8e72..b004e988 100644 --- a/api/src/repositories/role/role.repository.ts +++ b/api/src/repositories/role/role.repository.ts @@ -7,10 +7,60 @@ 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.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. + * + * 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, 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.startPopulate(); + await this.roleMapReady; + return this.roleMap; + } + private async populateRoleMap(): Promise { const roles = await this.findAll(); 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..d6ec9200 100644 --- a/api/src/services/block/block.service.ts +++ b/api/src/services/block/block.service.ts @@ -10,6 +10,8 @@ import { } from '../../repositories'; 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() @@ -21,6 +23,8 @@ export class BlockService { private roleAssignmentRepository: RoleAssignmentRepository, private roleRepository: RoleRepository, private memberRepository: MemberRepository, + private roleAssignmentService: RoleAssignmentService, + private placeAccessService: PlaceAccessService, ) {} public async find(blockId: number): Promise { @@ -33,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, @@ -50,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]; @@ -70,29 +84,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 +99,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 { @@ -154,43 +133,28 @@ 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); } + /** + * 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 d2b9cf2f..44e9f831 100644 --- a/api/src/services/colony/colony.service.ts +++ b/api/src/services/colony/colony.service.ts @@ -9,6 +9,8 @@ import { 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() @@ -18,6 +20,8 @@ export class ColonyService { private roleAssignmentRepository: RoleAssignmentRepository, private roleRepository: RoleRepository, private memberRepository: MemberRepository, + private roleAssignmentService: RoleAssignmentService, + private placeAccessService: PlaceAccessService, ) { } public async find(colonyId: number): Promise { @@ -29,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, @@ -46,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]; @@ -66,29 +80,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,72 +95,52 @@ 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); } - }); + } } + /** + * 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); } + /** + * 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 71b84b5a..06c5a22e 100644 --- a/api/src/services/hood/hood.service.ts +++ b/api/src/services/hood/hood.service.ts @@ -10,6 +10,8 @@ import { } from '../../repositories'; 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() @@ -21,6 +23,8 @@ export class HoodService { private roleAssignmentRepository: RoleAssignmentRepository, private roleRepository: RoleRepository, private memberRepository: MemberRepository, + private roleAssignmentService: RoleAssignmentService, + private placeAccessService: PlaceAccessService, ) {} public async find(hoodId: number): Promise { @@ -28,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, @@ -45,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]; @@ -65,29 +79,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 +94,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 { @@ -144,36 +125,29 @@ 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); } + /** + * 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/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..9dd21991 --- /dev/null +++ b/api/src/services/place-access/place-access.service.spec.ts @@ -0,0 +1,261 @@ +import { Container } from 'typedi'; +import { createSpyObj } from 'jest-createspyobj'; + +import { PlaceAccessService } from './place-access.service'; +import { + MapLocationRepository, + PlaceRepository, + PlaceRoleAccessRepository, + RoleAssignmentRepository, + RoleRepository, +} from '../../repositories'; + +describe('PlaceAccessService', () => { + const PLACE_ID = 42; + const MEMBER_ID = 11; + 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. */ + const emptyAccess = () => { + 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', () => { + 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(); + }); + }); + + 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 new file mode 100644 index 00000000..a5ff4bd8 --- /dev/null +++ b/api/src/services/place-access/place-access.service.ts @@ -0,0 +1,244 @@ +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. */ +export type WriteAccessReason = + | 'owner' + | 'deputy' + | 'role-grant' + | 'inherited' + | '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 { + /** + * 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); + } + + /** 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. + * + * 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, + memberId: number, + ownerCode: 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' }; + + 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' }; + } + 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. + 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/services/place/place.service.ts b/api/src/services/place/place.service.ts index 6f3c1090..a0a83a85 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,82 +254,65 @@ 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 }); - } - } - } } + // '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; }); 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 { 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. + * + * 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, 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 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; +}