diff --git a/api/src/controllers/member.controller.ts b/api/src/controllers/member.controller.ts index ac62d516..1f02d824 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,91 @@ 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 { - 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); + 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; } - response.status(200).json({ returnUsers }); + + // 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); response.status(400).json({ error }); @@ -613,4 +676,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..f0e6a9df --- /dev/null +++ b/api/src/services/member-data/member-data.service.spec.ts @@ -0,0 +1,109 @@ +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']); + }); + + /** + * 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', () => { + 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..9833e034 --- /dev/null +++ b/api/src/services/member-data/member-data.service.ts @@ -0,0 +1,87 @@ +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 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; + } + } + 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..2739b4ca --- /dev/null +++ b/api/src/services/roster/roster.service.spec.ts @@ -0,0 +1,158 @@ +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([]); + }); + + /** + * 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).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 new file mode 100644 index 00000000..423a2d46 --- /dev/null +++ b/api/src/services/roster/roster.service.ts @@ -0,0 +1,103 @@ +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); + + // 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), + 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, + })), + }; + } +} 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);