diff --git a/api/src/main/controllers/UserController.ts b/api/src/main/controllers/UserController.ts index ff82e5a..917fadf 100644 --- a/api/src/main/controllers/UserController.ts +++ b/api/src/main/controllers/UserController.ts @@ -55,7 +55,7 @@ class UserController { return res.status(401).send({ error: 'Authentication required' }); } - const user = await this.userService.findByUsername(req.user.username); + const user = await this.userService.findByUsername(req.user.username, req.user); res.json({ username: user.username, role: user.role }); } catch (err: any) { res.status(500).send({ error: err.message }); @@ -89,7 +89,7 @@ class UserController { .send({ error: 'INVALID DATA: Offset must be a non-negative number' }); } - const users = await this.userService.getUsers(q, searchLimit, searchOffset); + const users = await this.userService.getUsers(req.user, q, searchLimit, searchOffset); const total = await this.userService.countUsers(q); return res.json({ data: users, @@ -108,7 +108,7 @@ class UserController { async getByUsername(req: any, res: any) { try { - const user = await this.userService.findByUsername(req.params.username); + const user = await this.userService.findByUsername(req.params.username, req.user); res.json(user); } catch (err: any) { res.status(404).send({ error: err.message }); diff --git a/api/src/main/repositories/mongoose/OrganizationRepository.ts b/api/src/main/repositories/mongoose/OrganizationRepository.ts index 54ffab7..270154f 100644 --- a/api/src/main/repositories/mongoose/OrganizationRepository.ts +++ b/api/src/main/repositories/mongoose/OrganizationRepository.ts @@ -29,7 +29,7 @@ class OrganizationRepository extends RepositoryBase { const organization = await OrganizationMongoose.findOne({ _id: organizationId }) .populate({ path: 'ownerDetails', - select: '-password', + select: '-password -apiKey -role', }) .exec(); @@ -65,7 +65,7 @@ class OrganizationRepository extends RepositoryBase { }) .populate({ path: 'ownerDetails', - select: '-password', + select: '-password -apiKey -role', }) .exec(); diff --git a/api/src/main/repositories/mongoose/UserRepository.ts b/api/src/main/repositories/mongoose/UserRepository.ts index b78c883..21a3b56 100644 --- a/api/src/main/repositories/mongoose/UserRepository.ts +++ b/api/src/main/repositories/mongoose/UserRepository.ts @@ -9,7 +9,7 @@ class UserRepository extends RepositoryBase { async findAll(query?: any): Promise { try { - const users = await UserMongoose.find(query || {}, { password: 0 }).exec(); + const users = await UserMongoose.find(query || {}, { password: 0, apiKey: 0 }).exec(); return users.map(user => user.toObject({ getters: true, virtuals: true, versionKey: false }) as unknown as LeanUser); } catch (err) { return []; @@ -18,7 +18,7 @@ class UserRepository extends RepositoryBase { async find(username: string, limit: number = 10, offset: number = 0): Promise { try { - const users = await UserMongoose.find({ username: { $regex: username, $options: 'i' } }, { password: 0 }).skip(offset).limit(limit).exec(); + const users = await UserMongoose.find({ username: { $regex: username, $options: 'i' } }).select({ password: 0 }).skip(offset).limit(limit).exec(); return users.map(user => user.toObject({ getters: true, virtuals: true, versionKey: false }) as unknown as LeanUser); } catch (err) { return []; @@ -35,12 +35,13 @@ class UserRepository extends RepositoryBase { async findByUsername(username: string): Promise { try { - const user = (await this.find(username))[0]; + const user = await UserMongoose.findOne({ username }) + .select({ password: 0 }) + .exec(); if (!user) return null; - - return user as unknown as LeanUser; + return user.toObject({ getters: true, virtuals: true, versionKey: false }) as unknown as LeanUser; } catch (err) { return null; } @@ -48,7 +49,7 @@ class UserRepository extends RepositoryBase { async findByRole(role: UserRole): Promise { try { - const users = await UserMongoose.find({ role: role }, { password: 0 }).exec(); + const users = await UserMongoose.find({ role: role }, { password: 0, apiKey: 0 }).exec(); return users.map(user => user.toObject({ getters: true, virtuals: true, versionKey: false }) as unknown as LeanUser); } catch (err) { return []; @@ -95,7 +96,7 @@ class UserRepository extends RepositoryBase { async update(username: string, userData: any) { const updatedUser = await UserMongoose.findOneAndUpdate({ username: username }, userData, { new: true, - projection: { password: 0 }, + projection: { password: 0, apiKey: 0 }, }) if (!updatedUser) { diff --git a/api/src/main/services/UserService.ts b/api/src/main/services/UserService.ts index 56dd11f..9e788bd 100644 --- a/api/src/main/services/UserService.ts +++ b/api/src/main/services/UserService.ts @@ -9,25 +9,44 @@ class UserService { private userRepository: UserRepository; private organizationService: OrganizationService; + private sanitizeUserForRequester(user: LeanUser, requester?: LeanUser, targetUsername?: string): LeanUser { + if (!requester || requester.role === 'ADMIN') { + return user; + } + + const effectiveTargetUsername = targetUsername || user.username; + const isOwnData = effectiveTargetUsername === requester.username; + + if (isOwnData) { + return user; + } + + const { role, apiKey, ...safeUser } = user as any; + return safeUser as LeanUser; + } + constructor() { this.userRepository = container.resolve('userRepository'); this.organizationService = container.resolve('organizationService'); } - async getUsers(query: string = '', limit: number = 10, offset: number = 0): Promise { - return await this.userRepository.find(query.trim(), limit, offset); + async getUsers(reqUser: LeanUser, query: string = '', limit: number = 10, offset: number = 0): Promise { + + const users = await this.userRepository.find(query.trim(), limit, offset); + + return users.map(user => this.sanitizeUserForRequester(user, reqUser, user.username)); } async countUsers(query: string = '') { return this.userRepository.count(query.trim()); } - async findByUsername(username: string) { + async findByUsername(username: string, reqUser: LeanUser): Promise { const user = await this.userRepository.findByUsername(username); if (!user) { throw new Error('INVALID DATA: User not found'); } - return user; + return this.sanitizeUserForRequester(user, reqUser, username); } async findByApiKey(apiKey: string) { @@ -61,7 +80,7 @@ class UserService { createdUser ); - return createdUser; + return this.sanitizeUserForRequester(createdUser, creatorData, createdUser.username); } async update(username: string, userData: any, creatorData: LeanUser) { @@ -106,7 +125,7 @@ class UserService { await this.organizationService.updateUsername(username, userData.username); } - return updatedUser; + return this.sanitizeUserForRequester(updatedUser, creatorData, username); } async regenerateApiKey(username: string, reqUser: LeanUser): Promise { @@ -150,7 +169,8 @@ class UserService { } } - return this.userRepository.changeRole(username, role); + const updatedUser = await this.userRepository.changeRole(username, role); + return this.sanitizeUserForRequester(updatedUser, creatorData, username); } async authenticate(username: string, password: string): Promise { diff --git a/api/src/test/user.test.ts b/api/src/test/user.test.ts index 8bc794e..1f2e268 100644 --- a/api/src/test/user.test.ts +++ b/api/src/test/user.test.ts @@ -27,6 +27,17 @@ describe('User API routes', function () { } }; + const expectNoSensitiveUserFields = (user: any) => { + expect(user.role).toBeUndefined(); + expect(user.apiKey).toBeUndefined(); + }; + + const expectNoSensitiveFieldsForOtherUsers = (users: any[], requesterUsername: string) => { + users + .filter(user => user?.username && user.username !== requesterUsername) + .forEach(user => expectNoSensitiveUserFields(user)); + }; + beforeAll(async function () { app = await getApp(); adminUser = await createTestUser('ADMIN'); @@ -732,6 +743,18 @@ describe('User API routes', function () { }); it('returns 403 when trying to demote the last admin', async function () { + const allUsersResponse = await request(app) + .get(`${baseUrl}/users?limit=50`) + .set('x-api-key', adminApiKey); + + const allAdmins = allUsersResponse.body.data.filter((u: any) => u.role === 'ADMIN'); + + for (const admin of allAdmins) { + if (admin.username !== adminUser.username) { + await deleteTestUser(admin.username); + } + } + const response = await request(app) .put(`${baseUrl}/users/${adminUser.username}/role`) .set('x-api-key', adminApiKey) @@ -980,4 +1003,133 @@ describe('User API routes', function () { expect(response.body.error).toContain('API Key'); }); }); + + describe('USER privacy on user endpoints', function () { + it('GET /users with search never exposes role or apiKey for other users', async function () { + const requester = await createTestUser('USER', `privacy_viewer_${Date.now()}`); + const otherUser1 = await createTestUser('USER', `privacy_target_1_${Date.now()}`); + const otherUser2 = await createTestUser('USER', `privacy_target_2_${Date.now()}`); + + trackUserForCleanup(requester); + trackUserForCleanup(otherUser1); + trackUserForCleanup(otherUser2); + + const response = await request(app) + .get(`${baseUrl}/users?q=privacy_target_`) + .set('x-api-key', requester.apiKey); + + expect(response.status).toBe(200); + expect(Array.isArray(response.body.data)).toBeTruthy(); + expect(response.body.data.length).toBeGreaterThanOrEqual(2); + expectNoSensitiveFieldsForOtherUsers(response.body.data, requester.username); + }); + + it('GET /users/:username never exposes role or apiKey when USER requests another user', async function () { + const requester = await createTestUser('USER', `privacy_reader_${Date.now()}`); + const targetUser = await createTestUser('USER', `privacy_profile_target_${Date.now()}`); + + trackUserForCleanup(requester); + trackUserForCleanup(targetUser); + + const response = await request(app) + .get(`${baseUrl}/users/${targetUser.username}`) + .set('x-api-key', requester.apiKey); + + expect(response.status).toBe(200); + expect(response.body.username).toBe(targetUser.username); + expectNoSensitiveUserFields(response.body); + }); + + it('POST /users never exposes role or apiKey when USER creates another user', async function () { + const creator = await createTestUser('USER', `privacy_creator_${Date.now()}`); + trackUserForCleanup(creator); + + const userData = { + username: `privacy_created_${Date.now()}`, + password: 'password123', + role: USER_ROLES[USER_ROLES.length - 1], + }; + + const response = await request(app) + .post(`${baseUrl}/users`) + .set('x-api-key', creator.apiKey) + .send(userData); + + expect(response.status).toBe(201); + expect(response.body.username).toBe(userData.username); + expectNoSensitiveUserFields(response.body); + trackUserForCleanup(response.body); + }); + + it('PUT /users/:username never exposes role or apiKey when USER updates another user', async function () { + const requester = await createTestUser('USER', `privacy_updater_${Date.now()}`); + const targetUser = await createTestUser('USER', `privacy_update_target_${Date.now()}`); + const updatedUsername = `upd_${Date.now()}_${Math.floor(Math.random() * 10000)}`; + + trackUserForCleanup(requester); + trackUserForCleanup(targetUser); + + const response = await request(app) + .put(`${baseUrl}/users/${targetUser.username}`) + .set('x-api-key', requester.apiKey) + .send({ username: updatedUsername }); + + expect([200, 403, 404, 422]).toContain(response.status); + expect(response.body.role).toBeUndefined(); + expect(response.body.apiKey).toBeUndefined(); + + if (response.status === 200) { + expect(response.body.username).toBe(updatedUsername); + trackUserForCleanup(response.body); + } + }); + + it('PUT /users/:username/api-key forbidden response does not expose sensitive user fields', async function () { + const requester = await createTestUser('USER', `privacy_apikey_req_${Date.now()}`); + const targetUser = await createTestUser('USER', `privacy_apikey_target_${Date.now()}`); + + trackUserForCleanup(requester); + trackUserForCleanup(targetUser); + + const response = await request(app) + .put(`${baseUrl}/users/${targetUser.username}/api-key`) + .set('x-api-key', requester.apiKey); + + expect(response.status).toBe(403); + expect(response.body.role).toBeUndefined(); + expect(response.body.apiKey).toBeUndefined(); + }); + + it('PUT /users/:username/role forbidden response does not expose sensitive user fields', async function () { + const requester = await createTestUser('USER', `privacy_role_req_${Date.now()}`); + const targetUser = await createTestUser('USER', `privacy_role_target_${Date.now()}`); + + trackUserForCleanup(requester); + trackUserForCleanup(targetUser); + + const response = await request(app) + .put(`${baseUrl}/users/${targetUser.username}/role`) + .set('x-api-key', requester.apiKey) + .send({ role: USER_ROLES[USER_ROLES.length - 1] }); + + expect(response.status).toBe(403); + expect(response.body.role).toBeUndefined(); + expect(response.body.apiKey).toBeUndefined(); + }); + + it('DELETE /users/:username returns no body for USER deleting another non-admin user', async function () { + const requester = await createTestUser('USER', `privacy_delete_req_${Date.now()}`); + const targetUser = await createTestUser('USER', `privacy_delete_target_${Date.now()}`); + + trackUserForCleanup(requester); + trackUserForCleanup(targetUser); + + const response = await request(app) + .delete(`${baseUrl}/users/${targetUser.username}`) + .set('x-api-key', requester.apiKey); + + expect(response.status).toBe(204); + expect(response.body).toEqual({}); + }); + }); }); diff --git a/docker-compose.yml b/docker-compose.yml index c539156..405265c 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -108,7 +108,7 @@ services: networks: - space-network healthcheck: - test: ["CMD", "curl", "-f", "http://localhost:5403/${BASE_URL_PATH:-}/api/v1/healthcheck"] + test: ["CMD", "curl", "-f", "http://localhost:5403${BASE_PATH:-}/api/v1/healthcheck"] interval: 5s timeout: 5s retries: 3