From a9f4f3f70403952f5db85fbe966bff63df9dbb2f Mon Sep 17 00:00:00 2001 From: DJAscendance Date: Thu, 30 Jul 2026 09:33:41 -0400 Subject: [PATCH 1/3] feat: roster visibility rules and the hide-yourself privacy flag Closes two gaps that share the same surface, the online roster. ROSTER RULES (gaps 04, 05) Visitors get the COUNT and nothing else. message/count.html emits the roster link only when NNM != "Visitor", so an unauthenticated caller learns how many people are online and no more. CTR gated the whole endpoint on a session, so visitors got a 400 rather than the count they were meant to see. The response now carries `returnUsers: null` for a visitor -- null rather than [] so the client can tell "not permitted to see" from "nobody online". Buddies render bold: the BU_ loop flag in message/list.html gated a wrapper, and that bold name was the entire visual buddy affordance in the classic UI. Exposed as isBuddy, with isSelf alongside it since the viewer's own name renders as plain text rather than a link. The client owns the markup; the API supplies flags. Serving both members and visitors from one endpoint needed a session probe that does not respond on failure -- decryptSession writes a 400 as a side effect, which would pre-empt the visitor response. Added MemberService.peekSession for that. PRIVACY FLAG (gap 08) One boolean, backed by the IMS attribute. That single checkbox is the WHOLE privacy model in the original: no per-buddy blocking, no appear-offline-to-some, no ignore list. The simplicity is the design, so this deliberately stops there. A hidden member appears OFFLINE, so they are excluded from the entries AND from the count. Omitting the name while still counting them would leak their presence, because the count would exceed the visible names and reveal that someone is hiding. Tested. The viewer always sees themselves regardless of their own flag, so turning "hide me" on does not make you vanish from your own roster. ALSO - Centralised the online window. `5 * 60000` was written out three times in member.service; now one constant. Value unchanged, so this is behaviour-preserving -- note in-code that the original is 120 s against a 30 s heartbeat, which cannot be corrected without adding the heartbeat at the same time or active users would drop off the roster. - MemberDataRepository.getForMembers batches one attribute across many members. The roster needs every online member's privacy flag at once, and this endpoint already carries two N+1s; it did not need a third. - Buddy slots are read-only here and stay SPARSE: an empty slot 3 does not shift 4..9 down, because the slot index is part of the original's model. Managing the list is a separate task. 22 tests. Suite 15 -> 37 passing, with the same 5 pre-existing DB-dependent failures -- verified failure-for-failure against the merge base, since this adds a constructor dependency to MemberService. --- api/src/controllers/member.controller.ts | 97 +++++++++--- .../member-data/member-data.repository.ts | 22 +++ api/src/routes/member.routes.ts | 6 + api/src/services/index.ts | 2 + .../member-data/member-data.service.spec.ts | 90 +++++++++++ .../member-data/member-data.service.ts | 81 ++++++++++ api/src/services/member/member.service.ts | 52 ++++++- .../services/roster/roster.service.spec.ts | 146 ++++++++++++++++++ api/src/services/roster/roster.service.ts | 96 ++++++++++++ 9 files changed, 568 insertions(+), 24 deletions(-) create mode 100644 api/src/services/member-data/member-data.service.spec.ts create mode 100644 api/src/services/member-data/member-data.service.ts create mode 100644 api/src/services/roster/roster.service.spec.ts create mode 100644 api/src/services/roster/roster.service.ts diff --git a/api/src/controllers/member.controller.ts b/api/src/controllers/member.controller.ts index ac62d516..1a1edff1 100644 --- a/api/src/controllers/member.controller.ts +++ b/api/src/controllers/member.controller.ts @@ -6,7 +6,7 @@ import validator from 'validator'; import * as badwords from 'badwords-list'; import { sendPasswordResetEmail, sendPasswordResetUnknownEmail } from '../libs'; -import { MemberService, HomeService, PlaceService } from '../services'; +import { MemberService, HomeService, PlaceService, MemberDataService } from '../services'; import { SessionInfo } from 'session-info.interface'; import {parseInt} from 'lodash'; @@ -31,7 +31,8 @@ class MemberController { constructor( private memberService: MemberService, private homeService: HomeService, - private placeService: PlaceService) {} + private placeService: PlaceService, + private memberDataService: MemberDataService) {} public async getAdminLevel(request: Request, response: Response): Promise { const session = this.memberService.decryptSession(request, response); @@ -462,29 +463,80 @@ class MemberController { } } - public async getOnlineUsers(request: Request, response: Response): Promise { + /** + * Reads the caller's hide-yourself flag. + * + * One boolean, because one checkbox (IMS) is the whole privacy model in the original -- + * no per-buddy blocking, no appear-offline-to-some, no ignore list. + */ + public async getPrivacy(request: Request, response: Response): Promise { const session = this.memberService.decryptSession(request, response); if (!session) return; try { + response.status(200).json({ hidden: await this.memberDataService.isHidden(session.id) }); + } catch (error) { + console.log(error); + response.status(400).json({ error: 'Error reading privacy setting' }); + } + } + + /** Sets the caller's hide-yourself flag. Only ever affects the caller's own record. */ + public async updatePrivacy(request: Request, response: Response): Promise { + const session = this.memberService.decryptSession(request, response); + if (!session) return; + const { hidden } = request.body; + if (typeof hidden !== 'boolean') { + response.status(400).json({ error: 'hidden must be a boolean' }); + return; + } + try { + await this.memberDataService.setHidden(session.id, hidden); + response.status(200).json({ message: 'success', hidden }); + } catch (error) { + console.log(error); + response.status(400).json({ error: 'Error updating privacy setting' }); + } + } + + /** + * The online roster. + * + * Serves visitors as well as members, which is why it uses peekSession rather than + * decryptSession -- the latter writes a 400 when no token is present, so a visitor would + * get an error instead of the count they are meant to see. + * + * A visitor receives `{ count, returnUsers: null }`: message/count.html in the original + * emits the roster link only when NNM != "Visitor", so an unauthenticated caller learns + * how many people are online and nothing more. `null` rather than `[]` so the client can + * tell "not permitted to see" from "nobody online". + */ + public async getOnlineUsers(request: Request, response: Response): Promise { + const session = this.memberService.peekSession(request); + try { + const roster = await this.memberService.getRoster(session ? session.id : null); + + if (roster.entries === null) { + response.status(200).json({ count: roster.count, returnUsers: null }); + return; + } + const returnUsers = []; - const users = await this.memberService.getOnlineUsers(); - for (const user of users) { - const hasHome = await this.homeService.getHome(user.id); - const accessLevel = await this.memberService.getAccessLevel(user.id); - if(hasHome){ - user.hasHome = true; - } else { - user.hasHome = false; - } - if(accessLevel && accessLevel === 'security'){ - user.security = true; - } else { - user.security = false; - } - user.id = null; - returnUsers.push(user); + for (const entry of roster.entries) { + const hasHome = await this.homeService.getHome(entry.id); + const accessLevel = await this.memberService.getAccessLevel(entry.id); + returnUsers.push({ + username: entry.username, + hasHome: !!hasHome, + security: !!accessLevel && accessLevel === 'security', + // Buddies render bold and the viewer's own name renders as plain text rather + // than a link; the client owns that markup, so it gets the flags. + isBuddy: entry.isBuddy, + isSelf: entry.isSelf, + // Preserved from the previous shape: member ids are not exposed here. + id: null, + }); } - response.status(200).json({ returnUsers }); + response.status(200).json({ count: roster.count, returnUsers }); } catch (error) { console.log(error); response.status(400).json({ error }); @@ -613,4 +665,7 @@ class MemberController { const memberService = Container.get(MemberService); const homeService = Container.get(HomeService); const placeService = Container.get(PlaceService); -export const memberController = new MemberController(memberService, homeService, placeService); +const memberDataService = Container.get(MemberDataService); +export const memberController = new MemberController( + memberService, homeService, placeService, memberDataService, +); diff --git a/api/src/repositories/member-data/member-data.repository.ts b/api/src/repositories/member-data/member-data.repository.ts index 178d3798..15241688 100644 --- a/api/src/repositories/member-data/member-data.repository.ts +++ b/api/src/repositories/member-data/member-data.repository.ts @@ -56,6 +56,28 @@ export class MemberDataRepository { }, {} as Record); } + /** + * One attribute across many members, as memberId -> value. + * + * Batched deliberately: the roster needs every online member's privacy flag at once, and + * fetching that per member would add an N+1 to an endpoint that already has two. Members + * with the attribute unset are simply absent from the map. + */ + public async getForMembers( + memberIds: number[], + name: string, + ): Promise> { + const result = new Map(); + if (!memberIds.length) return result; + + const rows = await this.db.knex('member_data') + .select('member_id', 'value') + .whereIn('member_id', memberIds) + .andWhere('name', name); + for (const row of rows) result.set(row.member_id, row.value); + return result; + } + /** * Sets an attribute, replacing any existing value. * diff --git a/api/src/routes/member.routes.ts b/api/src/routes/member.routes.ts index 30c0c3bf..2abee0ad 100644 --- a/api/src/routes/member.routes.ts +++ b/api/src/routes/member.routes.ts @@ -28,6 +28,12 @@ memberRoutes.get('/session', (request, response) => memberController.session(req memberRoutes.post('/update_password', (request, response) => memberController.updatePassword(request, response), ); +memberRoutes.get('/get_privacy', (request, response) => + memberController.getPrivacy(request, response), +); +memberRoutes.post('/update_privacy', (request, response) => + memberController.updatePrivacy(request, response), +); memberRoutes.post('/update_role', (request, response) => memberController.updatePrimaryRoleId(request, response), ); diff --git a/api/src/services/index.ts b/api/src/services/index.ts index 9c0b10ec..50cd675a 100644 --- a/api/src/services/index.ts +++ b/api/src/services/index.ts @@ -9,6 +9,8 @@ export * from './home/home.service'; export * from './hood/hood.service'; export * from './mall/mall.service'; export * from './member/member.service'; +export * from './member-data/member-data.service'; +export * from './roster/roster.service'; export * from './message/message.service'; export * from './object/object.service'; export * from './object-instance/object-instance.service'; diff --git a/api/src/services/member-data/member-data.service.spec.ts b/api/src/services/member-data/member-data.service.spec.ts new file mode 100644 index 00000000..0f0ddb07 --- /dev/null +++ b/api/src/services/member-data/member-data.service.spec.ts @@ -0,0 +1,90 @@ +import { Container } from 'typedi'; +import { createSpyObj } from 'jest-createspyobj'; + +import { MemberDataService } from './member-data.service'; +import { MemberDataRepository } from '../../repositories'; + +describe('MemberDataService', () => { + const MEMBER = 11; + let memberDataRepository: jest.Mocked; + let service: MemberDataService; + + beforeEach(() => { + memberDataRepository = createSpyObj(MemberDataRepository); + Container.reset(); + Container.set(MemberDataRepository, memberDataRepository); + service = Container.get(MemberDataService); + }); + + describe('isHidden', () => { + it('is true only for the exact stored value "1"', async () => { + memberDataRepository.get.mockResolvedValue('1'); + expect(await service.isHidden(MEMBER)).toBe(true); + }); + it('is false when unset', async () => { + memberDataRepository.get.mockResolvedValue(null); + expect(await service.isHidden(MEMBER)).toBe(false); + }); + /** A stray truthy value must not read as hidden -- '0' is the disabled state. */ + it('is false for "0"', async () => { + memberDataRepository.get.mockResolvedValue('0'); + expect(await service.isHidden(MEMBER)).toBe(false); + }); + it('is false without a member id, and does not query', async () => { + expect(await service.isHidden(0)).toBe(false); + expect(memberDataRepository.get).not.toHaveBeenCalled(); + }); + }); + + describe('setHidden', () => { + it('stores "1" when hiding', async () => { + await service.setHidden(MEMBER, true); + expect(memberDataRepository.set).toHaveBeenCalledWith(MEMBER, 'IMS', '1'); + }); + /** null so the repository deletes the row: "unset" keeps one representation. */ + it('clears the attribute when unhiding', async () => { + await service.setHidden(MEMBER, false); + expect(memberDataRepository.set).toHaveBeenCalledWith(MEMBER, 'IMS', null); + }); + }); + + describe('getBuddySlots', () => { + it('always returns exactly ten slots', async () => { + memberDataRepository.getByPrefix.mockResolvedValue({}); + const slots = await service.getBuddySlots(MEMBER); + expect(slots).toHaveLength(10); + expect(slots.every(s => s === null)).toBe(true); + }); + + /** The slot INDEX is part of the model: a gap must not shift later slots down. */ + it('keeps slots sparse rather than compacting them', async () => { + memberDataRepository.getByPrefix.mockResolvedValue({ BU0: 'a', BU3: 'b', BU9: 'c' }); + const slots = await service.getBuddySlots(MEMBER); + expect(slots[0]).toBe('a'); + expect(slots[1]).toBeNull(); + expect(slots[3]).toBe('b'); + expect(slots[9]).toBe('c'); + }); + + /** Guards against an out-of-range stored name corrupting the array. */ + it('ignores names outside the ten-slot range', async () => { + memberDataRepository.getByPrefix.mockResolvedValue({ BU0: 'a', BU10: 'x', BUxx: 'y' }); + const slots = await service.getBuddySlots(MEMBER); + expect(slots).toHaveLength(10); + expect(slots.filter(Boolean)).toEqual(['a']); + }); + }); + + describe('getBuddyNameSet', () => { + it('lowercases for case-insensitive nickname matching', async () => { + memberDataRepository.getByPrefix.mockResolvedValue({ BU0: 'HawK', BU1: 'scott99' }); + const set = await service.getBuddyNameSet(MEMBER); + expect(set.has('hawk')).toBe(true); + expect(set.has('scott99')).toBe(true); + }); + it('drops empty slots', async () => { + memberDataRepository.getByPrefix.mockResolvedValue({ BU0: 'a', BU1: null }); + expect((await service.getBuddyNameSet(MEMBER)).size).toBe(1); + }); + }); +}); diff --git a/api/src/services/member-data/member-data.service.ts b/api/src/services/member-data/member-data.service.ts new file mode 100644 index 00000000..5f152129 --- /dev/null +++ b/api/src/services/member-data/member-data.service.ts @@ -0,0 +1,81 @@ +import { Service } from 'typedi'; + +import { MemberDataRepository } from '../../repositories'; + +/** + * Named per-member attributes, with the CS 4.x attribute names kept in one place. + * + * Callers should use these methods rather than passing raw attribute names around, so + * what 'IMS' and 'BU3' mean is defined here and nowhere else. + */ +@Service() +export class MemberDataService { + /** + * The hide-yourself privacy flag. + * + * A single checkbox in the original (message/config.html) and that is the ENTIRE privacy + * model -- no per-buddy blocking, no appear-offline-to-some, no ignore list. The + * simplicity is the design, so resist growing it. + */ + public static readonly HIDDEN = 'IMS'; + + /** Buddy slot prefix. Exactly ten slots, BU0..BU9, holding nicknames. */ + public static readonly BUDDY_PREFIX = 'BU'; + public static readonly BUDDY_SLOTS = 10; + + constructor(private memberDataRepository: MemberDataRepository) {} + + /** True if the member has chosen to appear offline. */ + public async isHidden(memberId: number): Promise { + if (!memberId) return false; + return (await this.memberDataRepository.get(memberId, MemberDataService.HIDDEN)) === '1'; + } + + /** Sets or clears the hide-yourself flag. */ + public async setHidden(memberId: number, hidden: boolean): Promise { + await this.memberDataRepository.set( + memberId, + MemberDataService.HIDDEN, + hidden ? '1' : null, + ); + } + + /** + * The member's buddy nicknames, by slot index. + * + * Sparse on purpose: slot 3 being empty does not shift slots 4..9 down, because the slot + * index is part of the original's model. Index is the array position; empty slots are + * null. + * + * Read-only here. Managing the list (adding, removing, the "buddy entered" notification) + * is a separate task. + */ + public async getBuddySlots(memberId: number): Promise<(string | null)[]> { + const slots: (string | null)[] = new Array(MemberDataService.BUDDY_SLOTS).fill(null); + if (!memberId) return slots; + + const stored = await this.memberDataRepository + .getByPrefix(memberId, MemberDataService.BUDDY_PREFIX); + for (const [name, value] of Object.entries(stored)) { + const index = Number(name.slice(MemberDataService.BUDDY_PREFIX.length)); + if (Number.isInteger(index) && index >= 0 && index < MemberDataService.BUDDY_SLOTS) { + slots[index] = value; + } + } + return slots; + } + + /** + * The member's buddy nicknames lowercased, for membership tests. + * + * Buddies are stored by NICKNAME, not id, so comparison has to be case-insensitive -- + * the original's own field set includes NNK, a lowercased nickname, precisely because + * nicknames are matched case-insensitively. + */ + public async getBuddyNameSet(memberId: number): Promise> { + const slots = await this.getBuddySlots(memberId); + return new Set( + slots.filter((name): name is string => !!name).map(name => name.toLowerCase()), + ); + } +} diff --git a/api/src/services/member/member.service.ts b/api/src/services/member/member.service.ts index 6bf20281..1010d124 100644 --- a/api/src/services/member/member.service.ts +++ b/api/src/services/member/member.service.ts @@ -20,6 +20,7 @@ import { import { Member } from '../../types/models'; import { MemberInfoView, MemberAdminView } from '../../types/views'; import { SessionInfo } from 'session-info.interface'; +import { RosterService, RosterView } from '../roster/roster.service'; import { Request, Response } from 'express'; /** Service for dealing with members */ @@ -37,6 +38,16 @@ export class MemberService { public static readonly PASSWORD_RESET_EXPIRATION_DURATION = 15; /** Number of times to salt member passwords */ private static readonly SALT_ROUNDS = 10; + /** + * How recently a member must have been seen to count as online. + * + * Was written out as `5 * 60000` in three separate places; centralised so there is one + * thing to change. NOTE: the original's value is 120 s expiry against a 30 s client + * heartbeat (global.cfg g_MsRefresh). Left at the existing 5 minutes here so this change + * is behaviour-preserving -- correcting it needs the heartbeat added at the same time, + * or active users would start dropping off the roster. + */ + public static readonly ONLINE_WINDOW_MS = 5 * 60000; constructor( private avatarRepository: AvatarRepository, @@ -50,6 +61,7 @@ export class MemberService { private objectInstanceRepository: ObjectInstanceRepository, private roleRepository: RoleRepository, private voteRepository: VoteRepository, + private rosterService: RosterService, ) { } public async canAdmin(memberId: number): Promise { @@ -517,7 +529,7 @@ export class MemberService { public async getActivePlaces(): Promise { const returnPlaces = []; const placeIds = []; - const activeTime = new Date(Date.now() - 5 * 60000); + const activeTime = new Date(Date.now() - MemberService.ONLINE_WINDOW_MS); const places = await this.memberRepository.getActivePlaces(activeTime); for (const place of places) { if (placeIds.indexOf(place.place_id) === -1) { @@ -538,6 +550,27 @@ export class MemberService { return returnPlaces; } + /** + * Decodes the session token if one is present and valid, without responding on failure. + * + * For endpoints that serve BOTH members and visitors. decryptSession cannot be used + * there: it writes a 400 as a side effect when the token is missing or bad, so a visitor + * would receive an error instead of the visitor-shaped response. This returns null and + * lets the caller decide. + * + * @param request Express request object + * @returns session info, or null for an absent or invalid token + */ + public peekSession(request: Request): SessionInfo | null { + const { apitoken } = request.headers; + if (!apitoken || typeof apitoken !== 'string') return null; + try { + return this.decodeMemberToken(apitoken) || null; + } catch (error) { + return null; + } + } + /** * Attempts to decode the session token present in the request and automatically responds with a * 400 error if decryption is unsuccessful @@ -575,14 +608,27 @@ export class MemberService { } } + /** + * The online roster as the viewer is allowed to see it. + * + * Delegates to RosterService, which applies the visitor/buddy/hidden rules. Pass null for + * an unauthenticated caller: they get a count and no names. + */ + public async getRoster(viewerMemberId: number | null): Promise { + return this.rosterService.getRoster( + viewerMemberId, + new Date(Date.now() - MemberService.ONLINE_WINDOW_MS), + ); + } + public async getOnlineUsers(): Promise { - const activeTime = new Date(Date.now() - 5 * 60000); + const activeTime = new Date(Date.now() - MemberService.ONLINE_WINDOW_MS); const users = await this.memberRepository.findOnlineUsers(activeTime); return users; } public async getDirectory(search: string, limit: number, offset: number): Promise { - const activeTime = new Date(Date.now() - 5 * 60000).getTime(); + const activeTime = new Date(Date.now() - MemberService.ONLINE_WINDOW_MS).getTime(); const [members, total] = await Promise.all([ this.memberRepository.searchDirectory(search, limit, offset), this.memberRepository.getDirectoryTotal(search), diff --git a/api/src/services/roster/roster.service.spec.ts b/api/src/services/roster/roster.service.spec.ts new file mode 100644 index 00000000..a7beb5ac --- /dev/null +++ b/api/src/services/roster/roster.service.spec.ts @@ -0,0 +1,146 @@ +import { Container } from 'typedi'; +import { createSpyObj } from 'jest-createspyobj'; + +import { RosterService } from './roster.service'; +import { MemberDataService } from '../member-data/member-data.service'; +import { MemberDataRepository, MemberRepository } from '../../repositories'; + +describe('RosterService', () => { + const VIEWER = 11; + const BUDDY = 22; + const STRANGER = 33; + const HIDDEN = 44; + const ACTIVE_SINCE = new Date(0); + + let memberRepository: jest.Mocked; + let memberDataRepository: jest.Mocked; + let memberDataService: jest.Mocked; + let service: RosterService; + + const online = (...members: { id: number; username: string }[]) => + memberRepository.findOnlineUsers.mockResolvedValue(members as any); + + const hidden = (...ids: number[]) => + memberDataRepository.getForMembers.mockResolvedValue( + new Map(ids.map(id => [id, '1'])), + ); + + beforeEach(() => { + memberRepository = createSpyObj(MemberRepository); + memberDataRepository = createSpyObj(MemberDataRepository); + memberDataService = createSpyObj(MemberDataService); + memberDataService.getBuddyNameSet.mockResolvedValue(new Set()); + Container.reset(); + Container.set(MemberRepository, memberRepository); + Container.set(MemberDataRepository, memberDataRepository); + Container.set(MemberDataService, memberDataService); + service = Container.get(RosterService); + + online( + { id: VIEWER, username: 'Viewer' }, + { id: BUDDY, username: 'Buddy' }, + { id: STRANGER, username: 'Stranger' }, + ); + hidden(); + }); + + /** + * Gap 04. message/count.html emits the roster link only when NNM != "Visitor", so a + * visitor learns HOW MANY are online and nothing else. + */ + describe('for a visitor', () => { + it('returns the count and no names', async () => { + const roster = await service.getRoster(null, ACTIVE_SINCE); + expect(roster.count).toBe(3); + expect(roster.entries).toBeNull(); + }); + + it('does not read buddy slots', async () => { + await service.getRoster(null, ACTIVE_SINCE); + expect(memberDataService.getBuddyNameSet).not.toHaveBeenCalled(); + }); + + it('still excludes hidden members from the count', async () => { + online( + { id: VIEWER, username: 'Viewer' }, + { id: HIDDEN, username: 'Ghost' }, + ); + hidden(HIDDEN); + const roster = await service.getRoster(null, ACTIVE_SINCE); + expect(roster.count).toBe(1); + }); + }); + + /** Gap 05. The BU_ loop flag gated a wrapper -- the whole visual buddy affordance. */ + describe('for a member', () => { + it('flags buddies, case-insensitively', async () => { + memberDataService.getBuddyNameSet.mockResolvedValue(new Set(['buddy'])); + const roster = await service.getRoster(VIEWER, ACTIVE_SINCE); + const byName = Object.fromEntries(roster.entries.map(e => [e.username, e])); + expect(byName['Buddy'].isBuddy).toBe(true); + expect(byName['Stranger'].isBuddy).toBe(false); + }); + + it('flags the viewer as self', async () => { + const roster = await service.getRoster(VIEWER, ACTIVE_SINCE); + const byName = Object.fromEntries(roster.entries.map(e => [e.username, e])); + expect(byName['Viewer'].isSelf).toBe(true); + expect(byName['Stranger'].isSelf).toBe(false); + }); + + it('count matches the number of entries returned', async () => { + const roster = await service.getRoster(VIEWER, ACTIVE_SINCE); + expect(roster.count).toBe(roster.entries.length); + }); + }); + + /** + * The privacy flag. A hidden member appears OFFLINE, so they are absent from the entries + * AND from the count -- counting them while omitting the name would leak that someone is + * hiding, because the count would exceed the visible names. + */ + describe('hidden members', () => { + beforeEach(() => { + online( + { id: VIEWER, username: 'Viewer' }, + { id: HIDDEN, username: 'Ghost' }, + { id: STRANGER, username: 'Stranger' }, + ); + hidden(HIDDEN); + }); + + it('are omitted from the entries', async () => { + const roster = await service.getRoster(VIEWER, ACTIVE_SINCE); + expect(roster.entries.map(e => e.username)).not.toContain('Ghost'); + }); + + it('are omitted from the count, so the count cannot leak them', async () => { + const roster = await service.getRoster(VIEWER, ACTIVE_SINCE); + expect(roster.count).toBe(2); + expect(roster.count).toBe(roster.entries.length); + }); + + /** Turning "hide me" on must not make you vanish from your own roster. */ + it('still see themselves', async () => { + const roster = await service.getRoster(HIDDEN, ACTIVE_SINCE); + const self = roster.entries.find(e => e.username === 'Ghost'); + expect(self).toBeDefined(); + expect(self.isSelf).toBe(true); + }); + }); + + describe('when nobody is online', () => { + it('returns an empty roster for a member, not null', async () => { + online(); + const roster = await service.getRoster(VIEWER, ACTIVE_SINCE); + expect(roster.count).toBe(0); + expect(roster.entries).toEqual([]); + }); + + it('does not query attributes for an empty member list', async () => { + online(); + await service.getRoster(null, ACTIVE_SINCE); + expect(memberDataRepository.getForMembers).toHaveBeenCalledWith([], 'IMS'); + }); + }); +}); diff --git a/api/src/services/roster/roster.service.ts b/api/src/services/roster/roster.service.ts new file mode 100644 index 00000000..1174e3f0 --- /dev/null +++ b/api/src/services/roster/roster.service.ts @@ -0,0 +1,96 @@ +import { Service } from 'typedi'; + +import { MemberDataRepository, MemberRepository } from '../../repositories'; +import { MemberDataService } from '../member-data/member-data.service'; + +/** One visible occupant of the roster. */ +export interface RosterEntry { + id: number; + username: string; + /** The viewer has this nickname in one of their ten buddy slots. */ + isBuddy: boolean; + /** This entry is the viewer. Rendered as plain text, not a link. */ + isSelf: boolean; +} + +export interface RosterView { + /** How many members are visibly online. */ + count: number; + /** + * The occupants, or null for a visitor. + * + * null rather than [] on purpose: an empty array says "nobody is online", whereas null + * says "you may not see who is online". The count still comes back either way. + */ + entries: RosterEntry[] | null; +} + +/** + * Builds the online roster, applying the original's visibility rules. + * + * Three rules, all from the shipped 4.1 templates: + * + * 1. Visitors get the COUNT and nothing else. message/count.html emits the roster link + * only when NNM != "Visitor", so an unauthenticated caller learns how many people are + * online and no more. CTR previously gated the whole endpoint on a session, so visitors + * got nothing at all -- not even the count they were meant to see. + * + * 2. Buddies render bold. The BU_ loop flag in message/list.html gates a wrapper, and + * that bold name was the entire visual buddy affordance in the classic UI. Exposed here + * as isBuddy so the client decides the markup. + * + * 3. The viewer's own name is plain text, not a link. Exposed as isSelf. + * + * And the privacy flag (IMS): a hidden member appears OFFLINE, so they are excluded from + * the entries AND from the count. Excluding them from the list but still counting them + * would leak their presence -- the count would exceed the visible names and reveal that + * someone is hiding. The viewer always sees themselves regardless of their own flag, so + * turning "hide me" on does not make you vanish from your own roster. + */ +@Service() +export class RosterService { + constructor( + private memberRepository: MemberRepository, + private memberDataRepository: MemberDataRepository, + private memberDataService: MemberDataService, + ) {} + + /** + * @param viewerMemberId the signed-in member, or null/0 for a visitor + * @param activeWithin how recently a member must have been seen to count as online + */ + public async getRoster( + viewerMemberId: number | null, + activeWithin: Date, + ): Promise { + const online: { id: number; username: string }[] = + await this.memberRepository.findOnlineUsers(activeWithin); + + // One batched read rather than a per-member lookup. + const hiddenFlags = await this.memberDataRepository.getForMembers( + online.map(member => member.id), + MemberDataService.HIDDEN, + ); + const visible = online.filter( + member => + hiddenFlags.get(member.id) !== '1' || + (!!viewerMemberId && member.id === viewerMemberId), + ); + + if (!viewerMemberId) { + return { count: visible.length, entries: null }; + } + + const buddies = await this.memberDataService.getBuddyNameSet(viewerMemberId); + return { + count: visible.length, + entries: visible.map(member => ({ + id: member.id, + username: member.username, + // Buddies are stored by nickname, so match case-insensitively. + isBuddy: buddies.has((member.username || '').toLowerCase()), + isSelf: member.id === viewerMemberId, + })), + }; + } +} From 7ebc305cb28bf9bd20cbc72a339f2436d1cb2016 Mon Sep 17 00:00:00 2001 From: Ryan Bundy Date: Thu, 30 Jul 2026 10:08:50 -0400 Subject: [PATCH 2/3] Address CodeRabbit review on the online roster Three fixes from a local CodeRabbit pass, each verified against the code before applying. Buddy slot names are now matched as text, not coerced. The suffix check ran Number() first and asked whether the result was an integer in range, which is far more permissive than the ten slot names are. Number('') is 0, so an attribute named exactly 'BU' populated slot 0. Number() also reads '01' and '1e0' as 1, so BU01 and BU1e0 both collided with BU1. getByPrefix matches on prefix, so any of these can arrive from imported data, and any future BU* attribute that is not a slot would have been silently read as one. Requiring exactly one digit is the actual rule. The existing range test covered BU10 and BUxx, which the old code already rejected, so it did not catch this; added a case for the three that got through. The roster's `security` flag was dead. getAccessLevel returns string[], so `accessLevel === 'security'` compared an array against a string and could never be true -- every entry has come back security:false since the flag was introduced in 1a35fec (Jan 2025), which this PR carried forward verbatim when it rewrote getOnlineUsers. Every other consumer in the tree already uses .includes(), including getRoles four hundred lines up in this same controller. This is a visible behaviour change: security members will now actually be marked on the roster, which is what the field was added to do. getOnlineUsers no longer issues two queries per person. hasHome is now one batched findMemberIdsWithHome call, matching getDirectory directly below it. getAccessLevel is left per-entry but resolved with Promise.all instead of awaited in a loop -- it fans out into canAdmin, canLeader and a role lookup, so genuinely batching it means batching those three, and that is a larger change than this cleanup should carry. Verified: tsc clean apart from the pre-existing missing 'sharp' module, which fails identically on feat/member-data-store. eslint 0 errors. Suite compared failure-for-failure against a stashed baseline rather than by count: the same five suites fail before and after, all on MySQL connection errors needing a live database, and passing tests go 37 -> 38 with the one test added here. Deliberately not done: getAccessLevel's return type is still `any` and its shape is only knowable by reading it, which is what allowed the dead comparison to survive review. Typing it string[] would surface any other bad comparison at compile time -- worth doing, but it touches call sites outside this PR. --- api/src/controllers/member.controller.ts | 43 ++++++++++++------- .../member-data/member-data.service.spec.ts | 19 ++++++++ .../member-data/member-data.service.ts | 10 ++++- 3 files changed, 54 insertions(+), 18 deletions(-) diff --git a/api/src/controllers/member.controller.ts b/api/src/controllers/member.controller.ts index 1a1edff1..1f02d824 100644 --- a/api/src/controllers/member.controller.ts +++ b/api/src/controllers/member.controller.ts @@ -520,22 +520,33 @@ class MemberController { return; } - const returnUsers = []; - for (const entry of roster.entries) { - const hasHome = await this.homeService.getHome(entry.id); - const accessLevel = await this.memberService.getAccessLevel(entry.id); - returnUsers.push({ - username: entry.username, - hasHome: !!hasHome, - security: !!accessLevel && accessLevel === 'security', - // Buddies render bold and the viewer's own name renders as plain text rather - // than a link; the client owns that markup, so it gets the flags. - isBuddy: entry.isBuddy, - isSelf: entry.isSelf, - // Preserved from the previous shape: member ids are not exposed here. - id: null, - }); - } + // Batched rather than one getHome per entry, matching getDirectory below. The roster + // is every visibly-online member, so the per-entry version issued two queries per + // person on every poll. + const memberIdsWithHome = await this.homeService.findMemberIdsWithHome( + roster.entries.map(entry => entry.id), + ); + // getAccessLevel is still one call per entry -- it fans out into canAdmin, canLeader + // and a role lookup, so batching it means batching those. Resolved in parallel here + // rather than in series; a real batch is a separate change. + const accessLevels = await Promise.all( + roster.entries.map(entry => this.memberService.getAccessLevel(entry.id)), + ); + const returnUsers = roster.entries.map((entry, i) => ({ + username: entry.username, + hasHome: memberIdsWithHome.has(entry.id), + // getAccessLevel returns string[], so the previous `accessLevel === 'security'` + // compared an array to a string and was never true -- the flag has been dead since + // it was added. Every other consumer (App.vue, admin.vue, the admin panels) already + // uses .includes(), which is the correct test. + security: accessLevels[i].includes('security'), + // Buddies render bold and the viewer's own name renders as plain text rather + // than a link; the client owns that markup, so it gets the flags. + isBuddy: entry.isBuddy, + isSelf: entry.isSelf, + // Preserved from the previous shape: member ids are not exposed here. + id: null, + })); response.status(200).json({ count: roster.count, returnUsers }); } catch (error) { console.log(error); diff --git a/api/src/services/member-data/member-data.service.spec.ts b/api/src/services/member-data/member-data.service.spec.ts index 0f0ddb07..f0e6a9df 100644 --- a/api/src/services/member-data/member-data.service.spec.ts +++ b/api/src/services/member-data/member-data.service.spec.ts @@ -73,6 +73,25 @@ describe('MemberDataService', () => { expect(slots).toHaveLength(10); expect(slots.filter(Boolean)).toEqual(['a']); }); + + /** + * The suffix must be exactly one digit, checked as text. Number() is far more permissive + * than the slot names are: '' is 0, and '01' and '1e0' are both 1. getByPrefix matches on + * prefix, so any of these can arrive from stored data or a future non-slot BU* attribute. + */ + it('rejects prefix matches that are not single-digit slot names', async () => { + memberDataRepository.getByPrefix.mockResolvedValue({ + BU: 'bare prefix, Number("") is 0', + BU01: 'leading zero, Number() reads 1', + BU1e0: 'exponent, Number() reads 1', + BU2: 'the only real slot here', + }); + const slots = await service.getBuddySlots(MEMBER); + expect(slots[0]).toBeNull(); + expect(slots[1]).toBeNull(); + expect(slots[2]).toBe('the only real slot here'); + expect(slots.filter(Boolean)).toEqual(['the only real slot here']); + }); }); describe('getBuddyNameSet', () => { diff --git a/api/src/services/member-data/member-data.service.ts b/api/src/services/member-data/member-data.service.ts index 5f152129..9833e034 100644 --- a/api/src/services/member-data/member-data.service.ts +++ b/api/src/services/member-data/member-data.service.ts @@ -57,8 +57,14 @@ export class MemberDataService { const stored = await this.memberDataRepository .getByPrefix(memberId, MemberDataService.BUDDY_PREFIX); for (const [name, value] of Object.entries(stored)) { - const index = Number(name.slice(MemberDataService.BUDDY_PREFIX.length)); - if (Number.isInteger(index) && index >= 0 && index < MemberDataService.BUDDY_SLOTS) { + const suffix = name.slice(MemberDataService.BUDDY_PREFIX.length); + // Exactly one digit, matched as text before any numeric conversion. A prefix match is + // not a slot name: Number('') is 0, so a bare 'BU' would otherwise land in slot 0, and + // Number() also accepts 'BU01' and 'BU1e0' as 1, colliding with BU1. Any future BU* + // attribute that is not a slot would be silently read as one too. + if (!/^[0-9]$/.test(suffix)) continue; + const index = Number(suffix); + if (index < MemberDataService.BUDDY_SLOTS) { slots[index] = value; } } From 1ae407f04a01afdee142aa08a1241761bed9ab2e Mon Sep 17 00:00:00 2001 From: Ryan Bundy Date: Thu, 30 Jul 2026 11:04:30 -0400 Subject: [PATCH 3/3] Fix the visitor regression this PR introduced in the online list Copilot review of #8. Three findings, all verified. The visitor case was broken, not just changed. This PR started returning `{ count, returnUsers: null }` to an unauthenticated caller instead of a 400, and CitizenOnlineModal assigns returnUsers straight to this.users and then calls .forEach on it, with no try/catch. So a visitor went from a rejected request that left the list empty -- degraded but working, "0 Citizens Online" -- to a TypeError on null, with the template's users.length failing too. That is worse than what it replaced. Fixed on the client rather than by softening the API. null is the right answer: it distinguishes "you may not see who is online" from "nobody is online", and the endpoint now returns a count to visitors precisely so they can be shown something. The modal records that as a canSeeUsers flag, keeps this.users an array either way, takes its heading from `count` rather than users.length, and shows "Sign in to see who is online." in place of the list. message/count.html in the original emitted the roster link only when NNM != "Visitor", so a visitor seeing a number and no names is the intended behaviour. The heading also stops saying "0 Citizen". It read `v-if="users.length > 1"`, so zero took the singular branch; `count !== 1` is the actual rule. RosterService now short-circuits when nobody is online. It was issuing the batched hidden-flag read with an empty id list, and for a member also fetching their buddy set, to build an empty roster. This endpoint is polled, so the empty case should cost nothing. The spec for that asserted the opposite of its own name. It was called 'does not query attributes for an empty member list' while asserting getForMembers HAD been called with [] -- so it documented and locked in the wasted round trip. Inverted, plus a companion test that a member with nobody online does not trigger the buddy lookup either. Verified: tsc clean apart from the pre-existing missing 'sharp' module. eslint 0 errors on the changed api files. Suite compared against a stashed baseline test-name by test-name: identical failures, no regressions; roster and member-data specs 23 -> 24. The modal's eslint errors went 21 -> 20: all pre-existing, none introduced, and the max-len on the heading line is gone because that line was rewritten. Deliberately not done: the modal still ignores the isBuddy and isSelf flags this PR added to each entry -- buddies should render bold and the viewer's own name as plain text rather than a link. The server side is done; wiring the markup is a separate change. spa/node_modules was symlinked into this worktree from the main checkout so the .vue file could be linted at all. --- .../services/roster/roster.service.spec.ts | 14 ++++++++- api/src/services/roster/roster.service.ts | 7 +++++ .../components/modals/CitizenOnlineModal.vue | 31 ++++++++++++++++--- 3 files changed, 47 insertions(+), 5 deletions(-) diff --git a/api/src/services/roster/roster.service.spec.ts b/api/src/services/roster/roster.service.spec.ts index a7beb5ac..2739b4ca 100644 --- a/api/src/services/roster/roster.service.spec.ts +++ b/api/src/services/roster/roster.service.spec.ts @@ -137,10 +137,22 @@ describe('RosterService', () => { expect(roster.entries).toEqual([]); }); + /** + * The roster endpoint is polled, so the nobody-online case should cost nothing. This + * previously asserted getForMembers HAD been called with [], which contradicted the + * test's own name and locked in the wasted round trip. + */ it('does not query attributes for an empty member list', async () => { online(); await service.getRoster(null, ACTIVE_SINCE); - expect(memberDataRepository.getForMembers).toHaveBeenCalledWith([], 'IMS'); + expect(memberDataRepository.getForMembers).not.toHaveBeenCalled(); + }); + + it('does not fetch buddies for a member when nobody is online', async () => { + online(); + const roster = await service.getRoster(VIEWER, ACTIVE_SINCE); + expect(memberDataService.getBuddyNameSet).not.toHaveBeenCalled(); + expect(roster.entries).toEqual([]); }); }); }); diff --git a/api/src/services/roster/roster.service.ts b/api/src/services/roster/roster.service.ts index 1174e3f0..423a2d46 100644 --- a/api/src/services/roster/roster.service.ts +++ b/api/src/services/roster/roster.service.ts @@ -66,6 +66,13 @@ export class RosterService { const online: { id: number; username: string }[] = await this.memberRepository.findOnlineUsers(activeWithin); + // Nobody online means there are no attributes to fetch and no buddy list to compare + // against. This endpoint is polled, so the empty case is worth not paying for -- and a + // visitor sees the same shape either way. + if (!online.length) { + return { count: 0, entries: viewerMemberId ? [] : null }; + } + // One batched read rather than a per-member lookup. const hiddenFlags = await this.memberDataRepository.getForMembers( online.map(member => member.id), diff --git a/spa/src/components/modals/CitizenOnlineModal.vue b/spa/src/components/modals/CitizenOnlineModal.vue index 4fa1ae92..a2ed21eb 100644 --- a/spa/src/components/modals/CitizenOnlineModal.vue +++ b/spa/src/components/modals/CitizenOnlineModal.vue @@ -7,7 +7,11 @@
-

{{ users.length }} CitizensCitizen Online

+

+ {{ count }} + CitizensCitizen + Online +

@@ -58,14 +62,22 @@ -->

-
    +
    • {{ user.username }} {{ user.username }} - +
    + +

    + Sign in to see who is online. +

@@ -110,12 +122,23 @@ export default Vue.extend({ users: [], security: [], jailId: null, + /** Visibly-online count, which the endpoint returns to visitors and members alike. */ + count: 0, + /** False for a visitor: they get the count and no names. */ + canSeeUsers: true, }; }, methods: { async getOnlineMembers(){ const onlineUsers = await this.$http.get("/member/online_users"); - this.users = onlineUsers.data.returnUsers; + this.count = onlineUsers.data.count || 0; + const returnUsers = onlineUsers.data.returnUsers; + // null means "you may not see who is online", which is not the same as [] meaning + // "nobody is online" -- so it is recorded as a flag rather than coerced to an empty + // list. It must not be assigned to this.users directly either: the template iterates + // it and the old code called .forEach on it, so a visitor got a TypeError. + this.canSeeUsers = returnUsers !== null && returnUsers !== undefined; + this.users = this.canSeeUsers ? returnUsers : []; this.users.forEach((user) => { if(user.security){ this.security.push(user);