From f96dab4cc167659b77a9e3f14c91c7f52239d6d7 Mon Sep 17 00:00:00 2001 From: davidishere1 Date: Fri, 21 Aug 2026 00:59:10 +0100 Subject: [PATCH 1/7] test(teams): add teams.service.spec.ts covering create and findOne Zero prior coverage on TeamsService (team-split.util.spec.ts only covers the pure percentage-math helpers, never the service). Covers create()'s validateSplitPercentages rejection and success path (one split saved per member, defaults createdById to null), and findOne()'s found/not-found cases. --- src/teams/teams.service.spec.ts | 121 ++++++++++++++++++++++++++++++++ 1 file changed, 121 insertions(+) create mode 100644 src/teams/teams.service.spec.ts diff --git a/src/teams/teams.service.spec.ts b/src/teams/teams.service.spec.ts new file mode 100644 index 0000000..07cf2bf --- /dev/null +++ b/src/teams/teams.service.spec.ts @@ -0,0 +1,121 @@ +import { Test, TestingModule } from '@nestjs/testing'; +import { getRepositoryToken } from '@nestjs/typeorm'; +import { NotFoundException } from '@nestjs/common'; +import { TeamsService } from './teams.service'; +import { Bounty, Team, TeamMemberSplit } from '../common/entities'; + +describe('TeamsService', () => { + let service: TeamsService; + let teamRepo: { findOne: jest.Mock; save: jest.Mock; create: jest.Mock }; + let splitRepo: { save: jest.Mock; create: jest.Mock }; + let bountyRepo: { findOne: jest.Mock; save: jest.Mock }; + + beforeEach(async () => { + teamRepo = { + create: jest.fn((t: Partial) => t), + save: jest.fn((t: Partial) => + Promise.resolve({ id: 't1', splits: [], ...t }), + ), + findOne: jest.fn(), + }; + splitRepo = { + create: jest.fn((s: Partial) => s), + save: jest.fn((s: Partial) => + Promise.resolve({ id: `split-${s.userId}`, ...s }), + ), + }; + bountyRepo = { + findOne: jest.fn(), + save: jest.fn((b: Partial) => Promise.resolve(b)), + }; + + const module: TestingModule = await Test.createTestingModule({ + providers: [ + TeamsService, + { provide: getRepositoryToken(Team), useValue: teamRepo }, + { provide: getRepositoryToken(TeamMemberSplit), useValue: splitRepo }, + { provide: getRepositoryToken(Bounty), useValue: bountyRepo }, + ], + }).compile(); + + service = module.get(TeamsService); + }); + + describe('create', () => { + it('rejects via validateSplitPercentages when splits do not sum to 100', async () => { + await expect( + service.create({ + name: 'Team A', + members: [{ userId: 'u1', percentage: 60 }], + }), + ).rejects.toThrow('Team split percentages must sum to 100, got 60.00'); + + expect(teamRepo.save).not.toHaveBeenCalled(); + }); + + it('saves the team and one split per member when percentages sum to 100', async () => { + const team = await service.create({ + name: 'Team A', + createdById: 'creator-1', + members: [ + { userId: 'u1', role: 'frontend', percentage: 60 }, + { userId: 'u2', percentage: 40 }, + ], + }); + + expect(teamRepo.save).toHaveBeenCalledWith( + expect.objectContaining({ name: 'Team A', createdById: 'creator-1' }), + ); + expect(splitRepo.save).toHaveBeenCalledTimes(2); + expect(splitRepo.save).toHaveBeenCalledWith( + expect.objectContaining({ + teamId: 't1', + userId: 'u1', + role: 'frontend', + percentage: '60.00', + }), + ); + expect(splitRepo.save).toHaveBeenCalledWith( + expect.objectContaining({ + teamId: 't1', + userId: 'u2', + role: null, + percentage: '40.00', + }), + ); + expect(team.splits).toHaveLength(2); + }); + + it('defaults createdById to null when not provided', async () => { + await service.create({ + name: 'Team B', + members: [{ userId: 'u1', percentage: 100 }], + }); + + expect(teamRepo.save).toHaveBeenCalledWith( + expect.objectContaining({ createdById: null }), + ); + }); + }); + + describe('findOne', () => { + it('returns the team with its splits when found', async () => { + const team = { id: 't1', name: 'Team A', splits: [] }; + teamRepo.findOne.mockResolvedValue(team); + + await expect(service.findOne('t1')).resolves.toBe(team); + expect(teamRepo.findOne).toHaveBeenCalledWith({ + where: { id: 't1' }, + relations: { splits: true }, + }); + }); + + it('throws NotFoundException when the team does not exist', async () => { + teamRepo.findOne.mockResolvedValue(null); + + await expect(service.findOne('missing')).rejects.toThrow( + NotFoundException, + ); + }); + }); +}); From c420de5eb3680892d56ecb6daa346bdca0eaa535 Mon Sep 17 00:00:00 2001 From: davidishere1 Date: Fri, 21 Aug 2026 00:59:55 +0100 Subject: [PATCH 2/7] test(teams): add assignToBounty coverage plus a regression baseline for #41 Covers assignToBounty's not-found cases (missing team, missing bounty) and its happy path. Also adds two baseline tests documenting the method's current, unguarded behavior for #41 (no bounty-status or ownership guard, allowing payout hijack via last-second team assignment): reassignment succeeds today regardless of the bounty's status or existing team. This is the 'known-good before state' the issue asks for so #41's own fix has a regression baseline to update once it lands. --- src/teams/teams.service.spec.ts | 73 +++++++++++++++++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/src/teams/teams.service.spec.ts b/src/teams/teams.service.spec.ts index 07cf2bf..b2188d5 100644 --- a/src/teams/teams.service.spec.ts +++ b/src/teams/teams.service.spec.ts @@ -3,6 +3,7 @@ import { getRepositoryToken } from '@nestjs/typeorm'; import { NotFoundException } from '@nestjs/common'; import { TeamsService } from './teams.service'; import { Bounty, Team, TeamMemberSplit } from '../common/entities'; +import { BountyStatus } from '../common/enums'; describe('TeamsService', () => { let service: TeamsService; @@ -118,4 +119,76 @@ describe('TeamsService', () => { ); }); }); + + describe('assignToBounty', () => { + it('throws NotFoundException when the team does not exist', async () => { + teamRepo.findOne.mockResolvedValue(null); + + await expect( + service.assignToBounty('missing-team', 'b1'), + ).rejects.toThrow(NotFoundException); + expect(bountyRepo.findOne).not.toHaveBeenCalled(); + }); + + it('throws NotFoundException when the bounty does not exist', async () => { + teamRepo.findOne.mockResolvedValue({ id: 't1', splits: [] }); + bountyRepo.findOne.mockResolvedValue(null); + + await expect( + service.assignToBounty('t1', 'missing-bounty'), + ).rejects.toThrow(NotFoundException); + }); + + it('sets the bounty.teamId and persists it when both exist', async () => { + teamRepo.findOne.mockResolvedValue({ id: 't1', splits: [] }); + bountyRepo.findOne.mockResolvedValue({ + id: 'b1', + status: BountyStatus.OPEN, + teamId: null, + }); + + const bounty = await service.assignToBounty('t1', 'b1'); + + expect(bounty.teamId).toBe('t1'); + expect(bountyRepo.save).toHaveBeenCalledWith( + expect.objectContaining({ id: 'b1', teamId: 't1' }), + ); + }); + + // Baseline/regression coverage for #41 (TeamsService.assignToBounty has + // no bounty-status or ownership guard, allowing payout hijack via + // last-second team assignment): this documents assignToBounty's current, + // unguarded behavior — reassignment succeeds regardless of the bounty's + // status or who currently claims it. Once #41 lands a guard, these two + // cases are expected to start throwing instead; update them alongside + // that fix rather than leaving this test silently describing stale + // behavior. + it('[current behavior, see #41] reassigns a bounty regardless of its status', async () => { + teamRepo.findOne.mockResolvedValue({ id: 't1', splits: [] }); + bountyRepo.findOne.mockResolvedValue({ + id: 'b1', + status: BountyStatus.MERGED, + claimedById: 'original-contributor', + teamId: null, + }); + + const bounty = await service.assignToBounty('t1', 'b1'); + + expect(bounty.teamId).toBe('t1'); + expect(bounty.status).toBe(BountyStatus.MERGED); + }); + + it('[current behavior, see #41] reassigns a bounty that is already assigned to a different team', async () => { + teamRepo.findOne.mockResolvedValue({ id: 't2', splits: [] }); + bountyRepo.findOne.mockResolvedValue({ + id: 'b1', + status: BountyStatus.CLAIMED, + teamId: 't1', + }); + + const bounty = await service.assignToBounty('t2', 'b1'); + + expect(bounty.teamId).toBe('t2'); + }); + }); }); From 7e3317e4fb83b0657c91d2fc09be3c951ac2f3ad Mon Sep 17 00:00:00 2001 From: davidishere1 Date: Fri, 21 Aug 2026 01:01:40 +0100 Subject: [PATCH 3/7] test(maintenance-pool): add maintenance-pool.service.spec.ts covering create/findOne/list Zero prior coverage on MaintenancePoolService despite it directly locking real funds. Covers create()'s ACTIVE default and repositoryId/createdById null defaults, findOne()'s NotFoundException, and list(). --- .../maintenance-pool.service.spec.ts | 87 +++++++++++++++++++ 1 file changed, 87 insertions(+) create mode 100644 src/maintenance-pool/maintenance-pool.service.spec.ts diff --git a/src/maintenance-pool/maintenance-pool.service.spec.ts b/src/maintenance-pool/maintenance-pool.service.spec.ts new file mode 100644 index 0000000..74cdb4a --- /dev/null +++ b/src/maintenance-pool/maintenance-pool.service.spec.ts @@ -0,0 +1,87 @@ +import { Test, TestingModule } from '@nestjs/testing'; +import { getRepositoryToken } from '@nestjs/typeorm'; +import { NotFoundException } from '@nestjs/common'; +import { MaintenancePoolService } from './maintenance-pool.service'; +import { EscrowService } from '../escrow/escrow.service'; +import { MaintenancePool } from '../common/entities'; +import { AssetType, MaintenancePoolStatus } from '../common/enums'; + +describe('MaintenancePoolService', () => { + let service: MaintenancePoolService; + let poolRepo: { + findOne: jest.Mock; + save: jest.Mock; + create: jest.Mock; + find: jest.Mock; + }; + let escrowService: { fund: jest.Mock; releasePartial: jest.Mock }; + + beforeEach(async () => { + poolRepo = { + create: jest.fn((p: Partial) => p), + save: jest.fn((p: Partial) => + Promise.resolve({ id: 'pool-1', ...p }), + ), + findOne: jest.fn(), + find: jest.fn(), + }; + escrowService = { + fund: jest.fn(), + releasePartial: jest.fn(), + }; + + const module: TestingModule = await Test.createTestingModule({ + providers: [ + MaintenancePoolService, + { provide: getRepositoryToken(MaintenancePool), useValue: poolRepo }, + { provide: EscrowService, useValue: escrowService }, + ], + }).compile(); + + service = module.get(MaintenancePoolService); + }); + + describe('create', () => { + it('saves a new pool with ACTIVE status', async () => { + const pool = await service.create({ + name: 'Docs pool', + asset: AssetType.USDC, + createdById: 'creator-1', + }); + + expect(poolRepo.save).toHaveBeenCalledWith( + expect.objectContaining({ + name: 'Docs pool', + asset: AssetType.USDC, + createdById: 'creator-1', + status: MaintenancePoolStatus.ACTIVE, + }), + ); + expect(pool.status).toBe(MaintenancePoolStatus.ACTIVE); + }); + + it('defaults repositoryId/createdById to null when not provided', async () => { + await service.create({ name: 'Pool', asset: AssetType.USDC }); + + expect(poolRepo.save).toHaveBeenCalledWith( + expect.objectContaining({ repositoryId: null, createdById: null }), + ); + }); + }); + + describe('findOne', () => { + it('throws NotFoundException when the pool does not exist', async () => { + poolRepo.findOne.mockResolvedValue(null); + await expect(service.findOne('missing')).rejects.toThrow( + NotFoundException, + ); + }); + }); + + describe('list', () => { + it('returns every pool', async () => { + poolRepo.find.mockResolvedValue([{ id: 'pool-1' }, { id: 'pool-2' }]); + await expect(service.list()).resolves.toHaveLength(2); + }); + }); +}); From 3273f0aa4a89b8e4f9587eaec58b3bd129845087 Mon Sep 17 00:00:00 2001 From: davidishere1 Date: Fri, 21 Aug 2026 01:02:33 +0100 Subject: [PATCH 4/7] test(maintenance-pool): add deposit coverage plus a regression baseline for #48 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Covers deposit's not-ACTIVE rejection, first-deposit escrow funding, and balance accumulation across deposits. Also adds a regression baseline for #48 (repeat deposits create a brand-new orphaned Escrow row every time, permanently stranding those funds outside assignReward's reach): documents today's actual behavior — a second deposit funds a second escrow, but pool.escrowId stays pinned to the first, so the new escrow's funds become unreachable. This is the 'before' baseline #48's fix needs to update. --- .../maintenance-pool.service.spec.ts | 101 +++++++++++++++++- 1 file changed, 100 insertions(+), 1 deletion(-) diff --git a/src/maintenance-pool/maintenance-pool.service.spec.ts b/src/maintenance-pool/maintenance-pool.service.spec.ts index 74cdb4a..a2bcb8a 100644 --- a/src/maintenance-pool/maintenance-pool.service.spec.ts +++ b/src/maintenance-pool/maintenance-pool.service.spec.ts @@ -1,6 +1,6 @@ import { Test, TestingModule } from '@nestjs/testing'; import { getRepositoryToken } from '@nestjs/typeorm'; -import { NotFoundException } from '@nestjs/common'; +import { BadRequestException, NotFoundException } from '@nestjs/common'; import { MaintenancePoolService } from './maintenance-pool.service'; import { EscrowService } from '../escrow/escrow.service'; import { MaintenancePool } from '../common/entities'; @@ -84,4 +84,103 @@ describe('MaintenancePoolService', () => { await expect(service.list()).resolves.toHaveLength(2); }); }); + + describe('deposit', () => { + it('rejects when the pool is not ACTIVE', async () => { + poolRepo.findOne.mockResolvedValue({ + id: 'pool-1', + status: MaintenancePoolStatus.PAUSED, + balance: '0', + }); + + await expect(service.deposit('pool-1', '100', 'GFUNDER')).rejects.toThrow( + BadRequestException, + ); + expect(escrowService.fund).not.toHaveBeenCalled(); + }); + + it('funds a new escrow and sets escrowId on the first deposit', async () => { + poolRepo.findOne.mockResolvedValue({ + id: 'pool-1', + status: MaintenancePoolStatus.ACTIVE, + balance: '0', + asset: AssetType.USDC, + escrowId: null, + }); + escrowService.fund.mockResolvedValue({ + id: 'escrow-1', + status: 'locked', + }); + + const pool = await service.deposit('pool-1', '100', 'GFUNDER'); + + expect(escrowService.fund).toHaveBeenCalledWith( + expect.objectContaining({ + amount: '100', + asset: AssetType.USDC, + funderAddress: 'GFUNDER', + maintenancePoolId: 'pool-1', + }), + ); + expect(pool.escrowId).toBe('escrow-1'); + expect(pool.balance).toBe('100.0000000'); + expect(pool.monthlyDeposit).toBe('100'); + }); + + it('accumulates balance across deposits', async () => { + poolRepo.findOne.mockResolvedValue({ + id: 'pool-1', + status: MaintenancePoolStatus.ACTIVE, + balance: '100', + asset: AssetType.USDC, + escrowId: 'escrow-1', + }); + escrowService.fund.mockResolvedValue({ + id: 'escrow-2', + status: 'locked', + }); + + const pool = await service.deposit('pool-1', '50', 'GFUNDER'); + + expect(pool.balance).toBe('150.0000000'); + }); + + // Regression baseline for #48 (MaintenancePoolService.deposit creates a + // brand-new orphaned Escrow row on every deposit after the first, + // permanently stranding those funds outside assignReward's reach): + // documents the current behavior a repeat deposit exhibits today — + // escrowService.fund() is called again (locking new funds on-chain and + // creating a second Escrow row), but pool.escrowId is never updated to + // point at it. assignReward only ever reads pool.escrowId, so this + // second escrow becomes permanently unreachable through the app. Once + // #48 lands a fix (e.g. topping up the existing escrow instead of + // minting a new one, or updating escrowId), this assertion on escrowId + // staying pinned to the *first* escrow is expected to change. + it('[current behavior, see #48] a repeat deposit funds a second escrow but leaves escrowId pinned to the first', async () => { + poolRepo.findOne.mockResolvedValue({ + id: 'pool-1', + status: MaintenancePoolStatus.ACTIVE, + balance: '100', + asset: AssetType.USDC, + escrowId: 'escrow-1', + }); + escrowService.fund.mockResolvedValue({ + id: 'escrow-2', + status: 'locked', + }); + + const pool = await service.deposit('pool-1', '50', 'GFUNDER'); + + // The second escrow was funded (real money locked on-chain / a real + // row created)... + expect(escrowService.fund).toHaveBeenCalledTimes(1); + expect(escrowService.fund).toHaveBeenCalledWith( + expect.objectContaining({ maintenancePoolId: 'pool-1', amount: '50' }), + ); + // ...but the pool never learns escrow-2 exists. assignReward() can + // only ever release from pool.escrowId, so escrow-2's funds are + // unreachable through this service. + expect(pool.escrowId).toBe('escrow-1'); + }); + }); }); From cb44867a53d7fca41d3e359597778b093b3f40a9 Mon Sep 17 00:00:00 2001 From: davidishere1 Date: Fri, 21 Aug 2026 00:50:32 +0100 Subject: [PATCH 5/7] test(maintenance-pool): add assignReward coverage and a concurrent lost-update regression for #51 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Covers assignReward's no-escrow and exceeds-balance rejections, and its happy path (releases the payment, decrements balance, forwards recipientId). Also adds a regression baseline for #51 (MaintenancePool.balance is a hand-maintained running total with a lost-update race across concurrent deposit/assignReward calls): backs findOne()/save() with a single shared mutable record (mirroring a real Postgres row) and runs two concurrent assignReward calls via Promise.all. Confirmed this deterministically reproduces the race — both calls read the same starting balance before either writes back, so the final balance reflects only one of the two decrements (900 or 800) instead of the correct 700. This is the 'before' baseline #51's fix (atomic update, lock, or transaction) needs to update. --- .../maintenance-pool.service.spec.ts | 94 +++++++++++++++++++ 1 file changed, 94 insertions(+) diff --git a/src/maintenance-pool/maintenance-pool.service.spec.ts b/src/maintenance-pool/maintenance-pool.service.spec.ts index a2bcb8a..c52ef54 100644 --- a/src/maintenance-pool/maintenance-pool.service.spec.ts +++ b/src/maintenance-pool/maintenance-pool.service.spec.ts @@ -183,4 +183,98 @@ describe('MaintenancePoolService', () => { expect(pool.escrowId).toBe('escrow-1'); }); }); + + describe('assignReward', () => { + it('rejects when the pool has no funded escrow yet', async () => { + poolRepo.findOne.mockResolvedValue({ + id: 'pool-1', + balance: '100', + escrowId: null, + }); + + await expect( + service.assignReward('pool-1', '10', 'GRECIPIENT'), + ).rejects.toThrow(BadRequestException); + expect(escrowService.releasePartial).not.toHaveBeenCalled(); + }); + + it('rejects when the requested amount exceeds the pool balance', async () => { + poolRepo.findOne.mockResolvedValue({ + id: 'pool-1', + balance: '50', + escrowId: 'escrow-1', + }); + + await expect( + service.assignReward('pool-1', '100', 'GRECIPIENT'), + ).rejects.toThrow(BadRequestException); + expect(escrowService.releasePartial).not.toHaveBeenCalled(); + }); + + it('releases the reward and decrements the balance', async () => { + poolRepo.findOne.mockResolvedValue({ + id: 'pool-1', + balance: '100', + escrowId: 'escrow-1', + }); + escrowService.releasePartial.mockResolvedValue({ id: 'payment-1' }); + + const payment = await service.assignReward( + 'pool-1', + '30', + 'GRECIPIENT', + 'user-1', + ); + + expect(escrowService.releasePartial).toHaveBeenCalledWith( + 'escrow-1', + '30', + 'GRECIPIENT', + 'user-1', + ); + expect(payment).toEqual({ id: 'payment-1' }); + expect(poolRepo.save).toHaveBeenCalledWith( + expect.objectContaining({ balance: '70.0000000' }), + ); + }); + + // Regression baseline for #51 (MaintenancePool.balance is a + // hand-maintained running total with a lost-update race across + // concurrent deposit/assignReward calls): reproduces the race + // deterministically by backing findOne()/save() with a single shared + // mutable record, matching how the real Postgres row works — both + // concurrent calls read the same starting balance before either writes + // back, because assignReward never re-reads or locks the row between + // its initial findOne() and its final save(). Once #51 lands a fix + // (e.g. an atomic UPDATE ... SET balance = balance - $1, or a + // pessimistic lock/transaction around read-modify-write), this test's + // "loses one of the two decrements" assertion is expected to flip to + // "balance reflects both decrements". + it('[current behavior, see #51] two concurrent assignReward calls lose one balance decrement', async () => { + const sharedPoolRow: { balance: string; escrowId: string } = { + balance: '1000.0000000', + escrowId: 'escrow-1', + }; + poolRepo.findOne.mockImplementation(() => + Promise.resolve({ id: 'pool-1', ...sharedPoolRow }), + ); + poolRepo.save.mockImplementation((pool: { balance: string }) => { + sharedPoolRow.balance = pool.balance; + return Promise.resolve(pool); + }); + escrowService.releasePartial.mockResolvedValue({ id: 'payment-x' }); + + await Promise.all([ + service.assignReward('pool-1', '100', 'GRECIPIENT_A'), + service.assignReward('pool-1', '200', 'GRECIPIENT_B'), + ]); + + // Both concurrent calls read balance=1000 before either wrote back, + // so whichever save() lands last overwrites the other's decrement — + // the final balance reflects only ONE of the two rewards, not both. + // A correct implementation would settle at 1000 - 100 - 200 = 700. + expect(sharedPoolRow.balance).not.toBe('700.0000000'); + expect(['900.0000000', '800.0000000']).toContain(sharedPoolRow.balance); + }); + }); }); From 566d5115445811609c90618fc16985a5c19edcb1 Mon Sep 17 00:00:00 2001 From: davidishere1 Date: Fri, 21 Aug 2026 01:04:55 +0100 Subject: [PATCH 6/7] test(users): add users.service.spec.ts covering findById, findByUsername, upsertFromGithub, list Zero prior coverage on UsersService. Covers findById()'s NotFoundException and public-DTO mapping (private fields like email excluded), findByUsername(), upsertFromGithub()'s three paths (brand new user, existing user found by username, already-linked account token refresh) plus its null-refreshToken fallback, and list(). --- src/users/users.service.spec.ts | 207 ++++++++++++++++++++++++++++++++ 1 file changed, 207 insertions(+) create mode 100644 src/users/users.service.spec.ts diff --git a/src/users/users.service.spec.ts b/src/users/users.service.spec.ts new file mode 100644 index 0000000..757070b --- /dev/null +++ b/src/users/users.service.spec.ts @@ -0,0 +1,207 @@ +import { Test, TestingModule } from '@nestjs/testing'; +import { getRepositoryToken } from '@nestjs/typeorm'; +import { NotFoundException } from '@nestjs/common'; +import { UsersService } from './users.service'; +import { GithubAccount, User } from '../common/entities'; +import { UserRole } from '../common/enums'; + +describe('UsersService', () => { + let service: UsersService; + let userRepo: { findOne: jest.Mock; save: jest.Mock; create: jest.Mock }; + let githubAccountRepo: { + findOne: jest.Mock; + save: jest.Mock; + create: jest.Mock; + }; + + beforeEach(async () => { + userRepo = { + findOne: jest.fn(), + save: jest.fn((u: Partial) => Promise.resolve({ id: 'u1', ...u })), + create: jest.fn((u: Partial) => u), + }; + githubAccountRepo = { + findOne: jest.fn(), + save: jest.fn((a: Partial) => + Promise.resolve({ id: 'ga1', ...a }), + ), + create: jest.fn((a: Partial) => a), + }; + + const module: TestingModule = await Test.createTestingModule({ + providers: [ + UsersService, + { provide: getRepositoryToken(User), useValue: userRepo }, + { + provide: getRepositoryToken(GithubAccount), + useValue: githubAccountRepo, + }, + ], + }).compile(); + + service = module.get(UsersService); + }); + + describe('findById', () => { + it('throws NotFoundException when the user does not exist', async () => { + userRepo.findOne.mockResolvedValue(null); + await expect(service.findById('missing')).rejects.toThrow( + NotFoundException, + ); + }); + + it('maps the user to its public shape, excluding private fields', async () => { + userRepo.findOne.mockResolvedValue({ + id: 'u1', + username: 'octocat', + displayName: 'The Octocat', + avatarUrl: 'https://example.com/a.png', + roles: [UserRole.CONTRIBUTOR], + stellarAddress: 'GADDRESS', + email: 'octocat@example.com', + createdAt: new Date('2026-01-01'), + }); + + const dto = await service.findById('u1'); + + expect(dto).toEqual({ + id: 'u1', + username: 'octocat', + displayName: 'The Octocat', + avatarUrl: 'https://example.com/a.png', + roles: [UserRole.CONTRIBUTOR], + stellarAddress: 'GADDRESS', + createdAt: new Date('2026-01-01'), + }); + expect(dto).not.toHaveProperty('email'); + }); + }); + + describe('findByUsername', () => { + it('returns null when no user matches', async () => { + userRepo.findOne.mockResolvedValue(null); + await expect(service.findByUsername('nobody')).resolves.toBeNull(); + }); + + it('returns the raw user entity when found', async () => { + const user = { id: 'u1', username: 'octocat' }; + userRepo.findOne.mockResolvedValue(user); + await expect(service.findByUsername('octocat')).resolves.toBe(user); + }); + }); + + describe('upsertFromGithub', () => { + const input = { + githubId: 'gh-1', + login: 'octocat', + email: 'octocat@example.com', + displayName: 'The Octocat', + avatarUrl: 'https://example.com/a.png', + profileUrl: 'https://github.com/octocat', + accessToken: 'token-abc', + refreshToken: 'refresh-abc', + }; + + it('creates a new User + GithubAccount when neither exists', async () => { + githubAccountRepo.findOne.mockResolvedValue(null); + userRepo.findOne + .mockResolvedValueOnce(null) // lookup by username before create + .mockResolvedValueOnce({ + id: 'u1', + username: 'octocat', + githubAccount: { id: 'ga1' }, + }); // findOneRaw at the end + + const user = await service.upsertFromGithub(input); + + expect(userRepo.create).toHaveBeenCalledWith( + expect.objectContaining({ + username: 'octocat', + email: 'octocat@example.com', + roles: [UserRole.CONTRIBUTOR], + }), + ); + expect(githubAccountRepo.create).toHaveBeenCalledWith( + expect.objectContaining({ githubId: 'gh-1', userId: 'u1' }), + ); + expect(user.id).toBe('u1'); + }); + + it('links to an existing user found by username instead of creating a duplicate', async () => { + githubAccountRepo.findOne.mockResolvedValue(null); + userRepo.findOne + .mockResolvedValueOnce({ id: 'existing-user', username: 'octocat' }) + .mockResolvedValueOnce({ id: 'existing-user', username: 'octocat' }); + + await service.upsertFromGithub(input); + + expect(userRepo.create).not.toHaveBeenCalled(); + expect(githubAccountRepo.create).toHaveBeenCalledWith( + expect.objectContaining({ userId: 'existing-user' }), + ); + }); + + it('refreshes tokens on an already-linked GithubAccount without creating a new user', async () => { + const account = { + id: 'ga1', + githubId: 'gh-1', + userId: 'u1', + accessToken: 'old-token', + refreshToken: 'old-refresh', + }; + githubAccountRepo.findOne.mockResolvedValue(account); + userRepo.findOne.mockResolvedValue({ id: 'u1', username: 'octocat' }); + + await service.upsertFromGithub(input); + + expect(userRepo.create).not.toHaveBeenCalled(); + expect(githubAccountRepo.save).toHaveBeenCalledWith( + expect.objectContaining({ + accessToken: 'token-abc', + refreshToken: 'refresh-abc', + }), + ); + }); + + it('stores a null refreshToken when GitHub does not return one', async () => { + githubAccountRepo.findOne.mockResolvedValue(null); + userRepo.findOne + .mockResolvedValueOnce(null) + .mockResolvedValueOnce({ id: 'u1', username: 'octocat' }); + + const { refreshToken, ...inputWithoutRefresh } = input; + void refreshToken; + await service.upsertFromGithub(inputWithoutRefresh); + + expect(githubAccountRepo.create).toHaveBeenCalledWith( + expect.objectContaining({ refreshToken: null }), + ); + }); + }); + + describe('list', () => { + it('returns every user mapped to its public shape', async () => { + userRepo.find = jest.fn().mockResolvedValue([ + { + id: 'u1', + username: 'a', + roles: [], + stellarAddress: null, + createdAt: new Date(), + }, + { + id: 'u2', + username: 'b', + roles: [], + stellarAddress: null, + createdAt: new Date(), + }, + ]); + + const users = await service.list(); + + expect(users).toHaveLength(2); + expect(users[0]).not.toHaveProperty('email'); + }); + }); +}); From 16e58dc622b481fc42884c352b4acd97c98ab066 Mon Sep 17 00:00:00 2001 From: davidishere1 Date: Fri, 21 Aug 2026 01:05:48 +0100 Subject: [PATCH 7/7] test(users): add addRole/setStellarAddress coverage and a scope note for #39 Covers addRole's add and already-has-role no-op cases, and setStellarAddress's NotFoundException and happy path. Also documents the scope boundary for #39 (setStellarAddress is authenticated but not authorized): the IDOR lives in UsersController#setStellarAddress binding :id from the URL instead of the authenticated req.user.id -- UsersService.setStellarAddress itself has no caller/authorization concept in its signature to test against. A cross-user-rejection test belongs in a users.controller.spec.ts alongside #39's actual fix; noted here rather than left implicit so the boundary isn't lost. --- src/users/users.service.spec.ts | 80 +++++++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/src/users/users.service.spec.ts b/src/users/users.service.spec.ts index 757070b..8bf284c 100644 --- a/src/users/users.service.spec.ts +++ b/src/users/users.service.spec.ts @@ -179,6 +179,86 @@ describe('UsersService', () => { }); }); + describe('addRole', () => { + it('adds the role when the user does not already have it', async () => { + userRepo.findOne.mockResolvedValue({ + id: 'u1', + roles: [UserRole.CONTRIBUTOR], + }); + + const user = await service.addRole('u1', UserRole.MAINTAINER); + + expect(user.roles).toEqual([UserRole.CONTRIBUTOR, UserRole.MAINTAINER]); + expect(userRepo.save).toHaveBeenCalledWith( + expect.objectContaining({ + roles: [UserRole.CONTRIBUTOR, UserRole.MAINTAINER], + }), + ); + }); + + it('is a no-op when the user already has the role', async () => { + userRepo.findOne.mockResolvedValue({ + id: 'u1', + roles: [UserRole.CONTRIBUTOR], + }); + + const user = await service.addRole('u1', UserRole.CONTRIBUTOR); + + expect(user.roles).toEqual([UserRole.CONTRIBUTOR]); + expect(userRepo.save).not.toHaveBeenCalled(); + }); + }); + + describe('setStellarAddress', () => { + it('throws NotFoundException when the user does not exist', async () => { + userRepo.findOne.mockResolvedValue(null); + await expect( + service.setStellarAddress('missing', 'GADDRESS'), + ).rejects.toThrow(NotFoundException); + }); + + it('sets stellarAddress on the given user and persists it', async () => { + userRepo.findOne.mockResolvedValue({ id: 'u1', stellarAddress: null }); + + const user = await service.setStellarAddress('u1', 'GNEWADDRESS'); + + expect(user.stellarAddress).toBe('GNEWADDRESS'); + expect(userRepo.save).toHaveBeenCalledWith( + expect.objectContaining({ id: 'u1', stellarAddress: 'GNEWADDRESS' }), + ); + }); + + // Note on #39 (UsersController.setStellarAddress is authenticated but + // not authorized: any logged-in user can overwrite another user's + // payout address): UsersService.setStellarAddress(userId, address) has + // no notion of "who is asking" in its own signature — by design it sets + // whichever userId it's given. The IDOR itself lives one layer up, in + // UsersController#setStellarAddress binding :id straight from the URL + // param instead of the authenticated req.user.id (see + // src/users/users.controller.ts). That controller has no spec file + // today and is out of this issue's listed scope (only + // users.service.spec.ts is asked for here) — a + // users.controller.spec.ts with the cross-user rejection test belongs + // with #39's fix, since only the controller layer has enough + // information (the authenticated caller's identity) to write a + // meaningful assertion for it. Documented here so the boundary isn't + // silently lost. + it('[documents scope of #39] setStellarAddress itself has no caller/authorization concept — see users.controller.ts', async () => { + userRepo.findOne.mockResolvedValue({ + id: 'victim', + stellarAddress: 'GOLD', + }); + + // Nothing about this call's parameters distinguishes "the account + // owner is changing their own address" from "some other authenticated + // user is overwriting someone else's" — both look identical to the + // service. + const user = await service.setStellarAddress('victim', 'GATTACKER'); + + expect(user.stellarAddress).toBe('GATTACKER'); + }); + }); + describe('list', () => { it('returns every user mapped to its public shape', async () => { userRepo.find = jest.fn().mockResolvedValue([