fix(users): bind stellar-address writes to the authenticated caller

src/users/users.controller.ts

@@ -1,4 +1,13 @@
-import { Body, Controller, Get, Param, ParseUUIDPipe, Patch, UseGuards } from '@nestjs/common';
+import {
+  Body,
+  Controller,
+  Get,
+  Param,
+  ParseUUIDPipe,
+  Patch,
+  Req,
+  UseGuards,
+} from '@nestjs/common';
 import { ApiBearerAuth, ApiTags } from '@nestjs/swagger';
 import { IsString } from 'class-validator';
 import { UsersService } from './users.service';
@@ -34,7 +43,12 @@ export class UsersController {
   setStellarAddress(
     @Param('id', new ParseUUIDPipe()) id: string,
     @Body() dto: SetStellarAddressDto,
+    @Req() req: { user: { userId: string } },
   ) {
-    return this.usersService.setStellarAddress(id, dto.stellarAddress);
+    return this.usersService.setStellarAddress(
+      id,
+      dto.stellarAddress,
+      req.user.userId,
+    );
   }
 }

src/users/users.service.spec.ts

@@ -1,13 +1,13 @@
 import { Test, TestingModule } from '@nestjs/testing';
 import { getRepositoryToken } from '@nestjs/typeorm';
-import { NotFoundException } from '@nestjs/common';
+import { ForbiddenException, 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 userRepo: { findOne: jest.Mock; save: jest.Mock; create: jest.Mock; find: jest.Mock };
   let githubAccountRepo: {
     findOne: jest.Mock;
     save: jest.Mock;
@@ -19,6 +19,7 @@ describe('UsersService', () => {
       findOne: jest.fn(),
       save: jest.fn((u: Partial<User>) => Promise.resolve({ id: 'u1', ...u })),
       create: jest.fn((u: Partial<User>) => u),
+      find: jest.fn(),
     };
     githubAccountRepo = {
       findOne: jest.fn(),
@@ -217,45 +218,44 @@ describe('UsersService', () => {
       ).rejects.toThrow(NotFoundException);
     });
 
-    it('sets stellarAddress on the given user and persists it', async () => {
+    it('sets stellarAddress on the given user and persists it for own id', async () => {
       userRepo.findOne.mockResolvedValue({ id: 'u1', stellarAddress: null });
 
-      const user = await service.setStellarAddress('u1', 'GNEWADDRESS');
+      const user = await service.setStellarAddress('u1', 'GNEWADDRESS', 'u1');
 
       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',
+    it('rejects a contributor overwriting someone else and never writes', async () => {
+      userRepo.findOne.mockImplementation(({ where: { id } }: { where: { id: string } }) => {
+        if (id === 'victim') return Promise.resolve({ id: 'victim', stellarAddress: 'GOLD', roles: [UserRole.CONTRIBUTOR] });
+        if (id === 'attacker') return Promise.resolve({ id: 'attacker', stellarAddress: null, roles: [UserRole.CONTRIBUTOR] });
+        return Promise.resolve(null);
+      });
+
+      await expect(
+        service.setStellarAddress('victim', 'GATTACKER', 'attacker'),
+      ).rejects.toThrow(ForbiddenException);
+
+      expect(userRepo.save).not.toHaveBeenCalled();
+    });
+
+    it('allows a maintainer to change another user address', async () => {
+      userRepo.findOne.mockImplementation(({ where: { id } }: { where: { id: string } }) => {
+        if (id === 'victim') return Promise.resolve({ id: 'victim', stellarAddress: 'GOLD', roles: [UserRole.CONTRIBUTOR] });
+        if (id === 'maintainer') return Promise.resolve({ id: 'maintainer', stellarAddress: null, roles: [UserRole.CONTRIBUTOR, UserRole.MAINTAINER] });
+        return Promise.resolve(null);
       });
 
-      // 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');
+      const user = await service.setStellarAddress('victim', 'GMAINTAINER_SET', 'maintainer');
 
-      expect(user.stellarAddress).toBe('GATTACKER');
+      expect(user.stellarAddress).toBe('GMAINTAINER_SET');
+      expect(userRepo.save).toHaveBeenCalledWith(
+        expect.objectContaining({ id: 'victim', stellarAddress: 'GMAINTAINER_SET' }),
+      );
     });
   });
 

src/users/users.service.ts

@@ -1,4 +1,8 @@
-import { Injectable, NotFoundException } from '@nestjs/common';
+import {
+  ForbiddenException,
+  Injectable,
+  NotFoundException,
+} from '@nestjs/common';
 import { InjectRepository } from '@nestjs/typeorm';
 import { Repository } from 'typeorm';
 import { GithubAccount, User } from '../common/entities';
@@ -99,7 +103,20 @@ export class UsersService {
   async setStellarAddress(
     userId: string,
     stellarAddress: string,
+    callerId?: string,
   ): Promise<User> {
+    // A valid token is authentication, not authorization: only the owner
+    // of the payout address — or a maintainer acting explicitly — may
+    // change where a user's bounties get paid. Checked before any write.
+    if (callerId && callerId !== userId) {
+      const caller = await this.findOneRaw(callerId);
+      if (!caller.roles?.includes(UserRole.MAINTAINER)) {
+        throw new ForbiddenException(
+          'Only the account owner or a maintainer can change a payout address',
+        );
+      }
+    }
+
     const user = await this.findOneRaw(userId);
     user.stellarAddress = stellarAddress;
     return this.userRepo.save(user);

test/users.e2e-spec.ts

@@ -1,4 +1,5 @@
 import { Test, TestingModule } from '@nestjs/testing';
+import { ExecutionContext, ForbiddenException } from '@nestjs/common';
 import { INestApplication } from '@nestjs/common';
 import request from 'supertest';
 import { UsersController } from '../src/users/users.controller';
@@ -16,9 +17,7 @@ describe('UsersController (e2e)', () => {
   beforeAll(async () => {
     const moduleFixture: TestingModule = await Test.createTestingModule({
       controllers: [UsersController],
-      providers: [
-        { provide: UsersService, useValue: mockUsersService },
-      ],
+      providers: [{ provide: UsersService, useValue: mockUsersService }],
     })
       .overrideGuard(JwtAuthGuard)
       .useValue({ canActivate: () => false }) // Simulate unauthenticated
@@ -34,9 +33,7 @@ describe('UsersController (e2e)', () => {
 
   describe('GET /users', () => {
     it('should reject unauthenticated requests with 401', () => {
-      return request(app.getHttpServer())
-        .get('/users')
-        .expect(403); // Assuming the guard returns 403 when not authorized
+      return request(app.getHttpServer()).get('/users').expect(403); // Assuming the guard returns 403 when not authorized
     });
   });
 
@@ -48,3 +45,109 @@ describe('UsersController (e2e)', () => {
     });
   });
 });
+
+describe('PATCH /users/:id/stellar-address (e2e)', () => {
+  const userA = 'a0000000-0000-4000-8000-00000000000a';
+  const userB = 'b0000000-0000-4000-8000-00000000000b';
+
+  const mockUsersService = {
+    setStellarAddress: jest.fn(),
+  };
+
+  // One guard override that stamps whichever identity the test selects,
+  // so each case exercises the real controller -> service contract.
+  let currentUser: { userId: string; username: string };
+
+  async function makeApp(authenticated: boolean) {
+    const moduleFixture: TestingModule = await Test.createTestingModule({
+      controllers: [UsersController],
+      providers: [{ provide: UsersService, useValue: mockUsersService }],
+    })
+      .overrideGuard(JwtAuthGuard)
+      .useValue({
+        canActivate: authenticated
+          ? (ctx: ExecutionContext) => {
+              const req = ctx
+                .switchToHttp()
+                .getRequest<{ user: { userId: string; username: string } }>();
+              req.user = currentUser;
+              return true;
+            }
+          : () => false,
+      })
+      .compile();
+
+    const application = moduleFixture.createNestApplication();
+    await application.init();
+    return application;
+  }
+
+  afterEach(() => {
+    jest.resetAllMocks();
+  });
+
+  it('rejects requests without a token', async () => {
+    const app = await makeApp(false);
+    await request(app.getHttpServer())
+      .patch(`/users/${userA}/stellar-address`)
+      .send({ stellarAddress: 'GA_ANON' })
+      .expect(403);
+    await app.close();
+  });
+
+  it('passes the caller identity through to the service for own id', async () => {
+    currentUser = { userId: userA, username: 'alice' };
+    const app = await makeApp(true);
+    mockUsersService.setStellarAddress.mockResolvedValue({ id: userA });
+
+    await request(app.getHttpServer())
+      .patch(`/users/${userA}/stellar-address`)
+      .send({ stellarAddress: 'GA_ALICE' })
+      .expect(200);
+
+    expect(mockUsersService.setStellarAddress).toHaveBeenCalledWith(
+      userA,
+      'GA_ALICE',
+      userA,
+    );
+    await app.close();
+  });
+
+  it('forwards the mismatch so cross-user writes are refused', async () => {
+    currentUser = { userId: userA, username: 'alice' };
+    const app = await makeApp(true);
+    mockUsersService.setStellarAddress.mockImplementation(() => {
+      throw new ForbiddenException();
+    });
+
+    await request(app.getHttpServer())
+      .patch(`/users/${userB}/stellar-address`)
+      .send({ stellarAddress: 'GA_ATTACKER' })
+      .expect(403);
+
+    expect(mockUsersService.setStellarAddress).toHaveBeenCalledWith(
+      userB,
+      'GA_ATTACKER',
+      userA,
+    );
+    await app.close();
+  });
+
+  it('lets a maintainer call through for another user', async () => {
+    currentUser = { userId: 'maintainer-1', username: 'ops' };
+    const app = await makeApp(true);
+    mockUsersService.setStellarAddress.mockResolvedValue({ id: userB });
+
+    await request(app.getHttpServer())
+      .patch(`/users/${userB}/stellar-address`)
+      .send({ stellarAddress: 'GA_OPS_SET' })
+      .expect(200);
+
+    expect(mockUsersService.setStellarAddress).toHaveBeenCalledWith(
+      userB,
+      'GA_OPS_SET',
+      'maintainer-1',
+    );
+    await app.close();
+  });
+});