trending/ever-gauzy
1
0
Fork 0
mirror of https://github.com/ever-co/ever-gauzy.git synced 2026-10-02 01:54:50 +08:00
Code Issues Packages Projects Releases Wiki Activity

fix(security): check organization membership in the holiday and balance listings

Browse Source 
The official-holiday and time-off-balance listings read through the raw
repository, and the organizationId membership validator they inherit from
TenantOrganizationBaseDTO is conditional: a request carrying `sentTo` skips it.
The earlier presence check closed the missing-organization case but still
accepted a sibling organization's UUID, so a holder of TIME_OFF_POLICY_VIEW
could list another organization's holidays, and a holder of
CHANGE_SELECTED_EMPLOYEE could list its leave balances.

Both listings now go through assertCurrentUserBelongsToOrganization, which
repeats the validator's user_organization lookup for the current tenant and
user and fails closed: 400 without an organization, 403 without an
authenticated tenant user or without a membership row. The specs cover the
non-member case with and without `sentTo` and the missing-user case.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Ruslan Konviser
2026-09-17 03:48:07 +02:00
co-authored by Claude Opus 5
parent 0d7a873b3c
commit d51b816c6e
5 changed files with 131 additions and 16 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/core/src/lib/official-holiday/official-holiday.service.spec.ts
+37 -3
View File
@@ -1,11 +1,13 @@
import '../core/entities/internal';
import { BadRequestException } from '@nestjs/common';
import { BadRequestException, ForbiddenException } from '@nestjs/common';
import { RequestContext } from '../core/context';
import { OfficialHolidayService } from './official-holiday.service';
const TENANT_ID = 'b6b0b0a6-2d6d-4f4e-9d5a-7a3f1f2c9e10';
const ORGANIZATION_ID = 'f1b2c3d4-e5f6-4708-8a9b-0c1d2e3f4a5b';
const SIBLING_ORGANIZATION_ID = '0a9b8c7d-6e5f-4a3b-9c2d-1e0f9a8b7c6d';
const USER_ID = '5e6f7a8b-9c0d-4e1f-8a2b-3c4d5e6f7a8b';
/**
* The listing reads through the raw repository, where an undefined `organizationId` is dropped from
@@ -14,12 +16,19 @@ const ORGANIZATION_ID = 'f1b2c3d4-e5f6-4708-8a9b-0c1d2e3f4a5b';
*/
describe('OfficialHolidayService.findAllByFilter organization scope', () => {
let service: OfficialHolidayService;
let repository: { findAndCount: jest.Mock };
let repository: { findAndCount: jest.Mock; manager: { count: jest.Mock } };
beforeEach(() => {
repository = { findAndCount: jest.fn(async () => [[], 0]) };
// The caller is a member of ORGANIZATION_ID only.
repository = {
findAndCount: jest.fn(async () => [[], 0]),
manager: {
count: jest.fn(async (_entity, { where }) => (where.organizationId === ORGANIZATION_ID ? 1 : 0))
}
};
service = new OfficialHolidayService(repository as any, {} as any);
jest.spyOn(RequestContext, 'currentTenantId').mockReturnValue(TENANT_ID);
jest.spyOn(RequestContext, 'currentUserId').mockReturnValue(USER_ID);
});
afterEach(() => {
@@ -38,6 +47,31 @@ describe('OfficialHolidayService.findAllByFilter organization scope', () => {
expect(repository.findAndCount).not.toHaveBeenCalled();
});
it('refuses an organization the caller is not a member of, even when `sentTo` skipped the DTO check', async () => {
await expect(
service.findAllByFilter({ organizationId: SIBLING_ORGANIZATION_ID } as any)
).rejects.toBeInstanceOf(ForbiddenException);
await expect(
service.findAllByFilter({ organizationId: SIBLING_ORGANIZATION_ID, sentTo: 'someone@example.com' } as any)
).rejects.toBeInstanceOf(ForbiddenException);
expect(repository.manager.count.mock.calls[0][1].where).toEqual({
tenantId: TENANT_ID,
userId: USER_ID,
organizationId: SIBLING_ORGANIZATION_ID
});
expect(repository.findAndCount).not.toHaveBeenCalled();
});
it('refuses when there is no authenticated user to check membership for', async () => {
jest.spyOn(RequestContext, 'currentUserId').mockReturnValue(null);
await expect(service.findAllByFilter({ organizationId: ORGANIZATION_ID } as any)).rejects.toBeInstanceOf(
ForbiddenException
);
expect(repository.findAndCount).not.toHaveBeenCalled();
});
it('scopes the query to the tenant and the organization when one is supplied', async () => {
await service.findAllByFilter({ organizationId: ORGANIZATION_ID } as any);
packages/core/src/lib/official-holiday/official-holiday.service.ts
+5 -6
View File
@@ -1,8 +1,9 @@
import { BadRequestException, Injectable } from '@nestjs/common';
import { Injectable } from '@nestjs/common';
import { Between } from 'typeorm';
import { IOfficialHoliday, IOfficialHolidayFindInput, IPagination } from '@gauzy/contracts';
import { RequestContext } from './../core/context';
import { TenantAwareCrudService } from './../core/crud';
import { assertCurrentUserBelongsToOrganization } from './../user-organization/assert-organization-membership';
import { OfficialHoliday } from './official-holiday.entity';
import { MikroOrmOfficialHolidayRepository } from './repository/mikro-orm-official-holiday.repository';
import { TypeOrmOfficialHolidayRepository } from './repository/type-orm-official-holiday.repository';
@@ -35,11 +36,9 @@ export class OfficialHolidayService extends TenantAwareCrudService<OfficialHolid
// This reads through the raw repository, so the organization is not injected for us, and an
// undefined key is DROPPED from a TypeORM where object rather than matching nothing — the
// listing would silently widen to every organization of the tenant. The query DTO does not
// close this on its own: `sentTo` suppresses the conditional `organizationId` validation it
// inherits. Fail closed instead.
if (!organizationId) {
throw new BadRequestException('organizationId is required');
}
// close this on its own: `sentTo` suppresses the conditional `organizationId` presence AND
// membership validation it inherits, so both are enforced here. Fail closed.
await assertCurrentUserBelongsToOrganization(this.typeOrmRepository.manager, organizationId);
const base: Record<string, unknown> = { tenantId, organizationId };
packages/core/src/lib/time-off-balance/time-off-balance.service.spec.ts
+41 -3
View File
@@ -1,6 +1,6 @@
import '../core/entities/internal';
import { BadRequestException } from '@nestjs/common';
import { BadRequestException, ForbiddenException } from '@nestjs/common';
import { PermissionsEnum } from '@gauzy/contracts';
import { RequestContext } from '../core/context';
import { TimeOffBalanceService } from './time-off-balance.service';
@@ -8,6 +8,8 @@ import { TimeOffBalanceService } from './time-off-balance.service';
const TENANT_ID = '4d3c2b1a-9f8e-4d7c-8b6a-5e4f3d2c1b0a';
const ORGANIZATION_ID = '11223344-5566-4778-899a-bbccddeeff00';
const EMPLOYEE_ID = '7c9e1d20-3b4a-4c5d-8e6f-90a1b2c3d4e5';
const SIBLING_ORGANIZATION_ID = 'aabbccdd-eeff-4011-8223-344556677889';
const USER_ID = '2f3e4d5c-6b7a-4980-a1b2-c3d4e5f60718';
/**
* The listing reads through the raw repository, where an undefined `organizationId` is dropped from
@@ -16,12 +18,19 @@ const EMPLOYEE_ID = '7c9e1d20-3b4a-4c5d-8e6f-90a1b2c3d4e5';
*/
describe('TimeOffBalanceService.findAllByFilter organization scope', () => {
let service: TimeOffBalanceService;
let repository: { findAndCount: jest.Mock };
let repository: { findAndCount: jest.Mock; manager: { count: jest.Mock } };
beforeEach(() => {
repository = { findAndCount: jest.fn(async () => [[], 0]) };
// The caller is a member of ORGANIZATION_ID only.
repository = {
findAndCount: jest.fn(async () => [[], 0]),
manager: {
count: jest.fn(async (_entity, { where }) => (where.organizationId === ORGANIZATION_ID ? 1 : 0))
}
};
service = new TimeOffBalanceService(repository as any, {} as any);
jest.spyOn(RequestContext, 'currentTenantId').mockReturnValue(TENANT_ID);
jest.spyOn(RequestContext, 'currentUserId').mockReturnValue(USER_ID);
jest.spyOn(RequestContext, 'currentEmployeeId').mockReturnValue(EMPLOYEE_ID);
jest.spyOn(RequestContext, 'hasPermission').mockImplementation(
(permission) => permission !== PermissionsEnum.CHANGE_SELECTED_EMPLOYEE
@@ -42,6 +51,35 @@ describe('TimeOffBalanceService.findAllByFilter organization scope', () => {
expect(repository.findAndCount).not.toHaveBeenCalled();
});
it('refuses an organization the caller is not a member of, even when `sentTo` skipped the DTO check', async () => {
// A holder of CHANGE_SELECTED_EMPLOYEE is not pinned to their own balances, so without the membership
// check they would read every balance of a sibling organization.
jest.spyOn(RequestContext, 'hasPermission').mockReturnValue(true);
await expect(
service.findAllByFilter({ organizationId: SIBLING_ORGANIZATION_ID } as any)
).rejects.toBeInstanceOf(ForbiddenException);
await expect(
service.findAllByFilter({ organizationId: SIBLING_ORGANIZATION_ID, sentTo: 'someone@example.com' } as any)
).rejects.toBeInstanceOf(ForbiddenException);
expect(repository.manager.count.mock.calls[0][1].where).toEqual({
tenantId: TENANT_ID,
userId: USER_ID,
organizationId: SIBLING_ORGANIZATION_ID
});
expect(repository.findAndCount).not.toHaveBeenCalled();
});
it('refuses when there is no authenticated user to check membership for', async () => {
jest.spyOn(RequestContext, 'currentUserId').mockReturnValue(null);
await expect(service.findAllByFilter({ organizationId: ORGANIZATION_ID } as any)).rejects.toBeInstanceOf(
ForbiddenException
);
expect(repository.findAndCount).not.toHaveBeenCalled();
});
it('scopes the query to the tenant, the organization and the caller when one is supplied', async () => {
await service.findAllByFilter({ organizationId: ORGANIZATION_ID } as any);
packages/core/src/lib/time-off-balance/time-off-balance.service.ts
+4 -4
View File
@@ -14,6 +14,7 @@ import { RequestContext } from './../core/context';
import { TenantAwareCrudService } from './../core/crud';
import { Employee, TimeOffPolicy } from './../core/entities/internal';
import { prepareSQLQuery as p } from './../database/database.helper';
import { assertCurrentUserBelongsToOrganization } from './../user-organization/assert-organization-membership';
import { TimeOffBalance } from './time-off-balance.entity';
import { MikroOrmTimeOffBalanceRepository } from './repository/mikro-orm-time-off-balance.repository';
import { TypeOrmTimeOffBalanceRepository } from './repository/type-orm-time-off-balance.repository';
@@ -56,10 +57,9 @@ export class TimeOffBalanceService extends TenantAwareCrudService<TimeOffBalance
// Raw repository read: nothing injects the organization, and an undefined key is DROPPED from
// a TypeORM where object instead of matching nothing, so a missing organization would widen
// the listing to every organization of the tenant. `sentTo` on the query DTO suppresses the
// conditional `organizationId` validation, so the DTO cannot be relied on here. Fail closed.
if (!organizationId) {
throw new BadRequestException('organizationId is required');
}
// conditional `organizationId` presence AND membership validation, so the DTO cannot be relied
// on for either. Fail closed.
await assertCurrentUserBelongsToOrganization(this.typeOrmRepository.manager, organizationId);
const where: Record<string, unknown> = { tenantId, organizationId };
packages/core/src/lib/user-organization/assert-organization-membership.ts
+44
View File
@@ -0,0 +1,44 @@
import { BadRequestException, ForbiddenException } from '@nestjs/common';
import { EntityManager } from 'typeorm';
import { ID } from '@gauzy/contracts';
import { RequestContext } from '../core/context';
import { UserOrganization } from '../core/entities/internal';
/**
* Refuse a read scoped to an organization the current user is not a member of.
*
* Services that list through the raw repository cannot lean on the query DTO for this: the
* `organizationId` membership validator on `TenantOrganizationBaseDTO` is conditional and is
* skipped entirely whenever the request carries `sentTo`, so a caller could name a sibling
* organization of their tenant and read it. This repeats the check the validator would have made
* (a `user_organization` row for this tenant, user and organization) and fails closed when it
* cannot reach a verdict: no organization, no tenant or no user in the request context all refuse.
*
* @param manager the TypeORM entity manager to run the membership lookup with
* @param organizationId the organization the read is about to be scoped to
* @throws BadRequestException when no organization is supplied
* @throws ForbiddenException when there is no authenticated tenant user, or they are not a member
*/
export async function assertCurrentUserBelongsToOrganization(
manager: EntityManager,
organizationId?: ID
): Promise<void> {
if (!organizationId) {
throw new BadRequestException('organizationId is required');
}
const tenantId = RequestContext.currentTenantId();
const userId = RequestContext.currentUserId();
if (!tenantId || !userId) {
throw new ForbiddenException('You are not a member of this organization');
}
const memberships = await manager.count(UserOrganization, {
where: { tenantId, userId, organizationId }
});
if (memberships === 0) {
throw new ForbiddenException('You are not a member of this organization');
}
}