Skip to content

Commit 9e7f8b8

Browse files
feat: add authorization check to listOrganizationAdmins service method
- Update ListOrganizationAdminsRequest to extend RequestorAwareRequest - Implement authorization validation in OrganizationAdminService - Update OrganizationAdminController to pass requestor - Add unit test verifying authorization logic - Document vulnerability in Sentinel journal
1 parent 7b5387c commit 9e7f8b8

5 files changed

Lines changed: 124 additions & 8 deletions

File tree

.jules/sentinel.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,3 +5,9 @@
55
**Vulnerability:** Found a "Fail Open" pattern in `RolePermissionChecker.scopeMatches` where unhandled scope types would default to `true` (allow access), potentially bypassing authorization checks if new scope types were added without updating the logic.
66
**Learning:** Default return values in authorization checks must always be restrictive (`false` or `deny`). Code that relies on "falling through" to a default should fall to a safe state.
77
**Prevention:** Always implement "Fail Closed" logic. When checking permissions, start with `false` and only switch to `true` if an explicit allow condition is met. Use exhaustive checks (like TypeScript's `never` check) in switch statements or if-else chains to ensure all cases are handled, but still default to `false` as a safety net.
8+
9+
## 2026-02-15 - Missing Authorization in Service Method
10+
11+
**Vulnerability:** `OrganizationAdminService.listOrganizationAdmins` exposed an endpoint without any authorization check because the method signature did not accept the requestor, making authorization checks impossible at the service layer.
12+
**Learning:** Service methods that perform sensitive actions must always be "Requestor Aware" to enforce authorization logic closer to the data, rather than relying solely on Controllers (which might forget to check).
13+
**Prevention:** Enforce a pattern where all sensitive service methods accept `RequestorAwareRequest`. Use linting or architectural reviews to flag public service methods that don't take a user context.

app/controllers/src/organization-admin/organization-admin.controller.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,7 @@ export class OrganizationAdminController {
6161
@Get(":organizationName/admins")
6262
async listOrganizationAdmins(
6363
@Param("organizationName") organizationName: string,
64+
@GetAuthenticatedEntity() requestor: AuthenticatedEntity,
6465
@Query("page") page?: string,
6566
@Query("limit") limit?: string
6667
): Promise<{data: OrganizationAdminApi[]; pagination: PaginationApi}> {
@@ -69,7 +70,7 @@ export class OrganizationAdminController {
6970
this.organizationAdminService.listOrganizationAdmins(req)
7071

7172
const eitherAdminsList = await pipe(
72-
{organizationName, page, limit},
73+
{organizationName, page, limit, requestor},
7374
listOrganizationAdminsApiToServiceModel,
7475
TE.fromEither,
7576
TE.chainW(serviceListAdmins),

app/controllers/src/organization-admin/organization-admin.mappers.ts

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ export function listOrganizationAdminsApiToServiceModel(data: {
4545
organizationName: string
4646
page?: string
4747
limit?: string
48+
requestor: AuthenticatedEntity
4849
}): Either<"invalid_number_format", ListOrganizationAdminsRequest> {
4950
const parseNumber = (value: string | undefined): Either<"invalid_number_format", number | undefined> => {
5051
if (value === undefined) return right(undefined)
@@ -58,7 +59,8 @@ export function listOrganizationAdminsApiToServiceModel(data: {
5859
bindW("organizationName", () => right(data.organizationName)),
5960
bindW("page", () => parseNumber(data.page)),
6061
bindW("limit", () => parseNumber(data.limit)),
61-
map(({organizationName, page, limit}) => ({organizationName, page, limit}))
62+
bindW("requestor", () => right(data.requestor)),
63+
map(({organizationName, page, limit, requestor}) => ({organizationName, page, limit, requestor}))
6264
)
6365
}
6466

@@ -127,12 +129,16 @@ export function generateErrorResponseForAddOrganizationAdmin(
127129
}
128130

129131
export function generateErrorResponseForListOrganizationAdmins(
130-
error: OrganizationAdminListError | "invalid_number_format",
132+
error: OrganizationAdminListError | AuthorizationError | "invalid_number_format",
131133
context: string
132134
): HttpException {
133135
const errorCode = error.toUpperCase()
134136

135137
switch (error) {
138+
case "requestor_not_authorized":
139+
return new ForbiddenException(
140+
generateErrorPayload(errorCode, `${context}: You are not authorized to perform this action`)
141+
)
136142
case "organization_not_found":
137143
return new NotFoundException(generateErrorPayload(errorCode, `${context}: Organization not found`))
138144
case "invalid_number_format":
Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
// eslint-disable-next-line node/no-unpublished-import
2+
import {Test, TestingModule} from "@nestjs/testing"
3+
import {OrganizationAdminService} from "./organization-admin.service"
4+
import {ORGANIZATION_ADMIN_REPOSITORY_TOKEN} from "./interfaces"
5+
import * as TE from "fp-ts/TaskEither"
6+
import {isRight, isLeft} from "fp-ts/Either"
7+
import {AuthenticatedEntity, OrgRole, User} from "@domain"
8+
9+
// Manual mock implementation
10+
const mockRepository = {
11+
createOrganizationAdmin: jest.fn(),
12+
listOrganizationAdmins: jest.fn(),
13+
removeOrganizationAdminIfNotLast: jest.fn(),
14+
removeOrganizationAdminByEmailIfNotLast: jest.fn()
15+
}
16+
17+
const mockMemberUser = {
18+
id: "user-123",
19+
email: "member@example.com",
20+
displayName: "Member User",
21+
createdAt: new Date(),
22+
orgRole: OrgRole.MEMBER,
23+
roles: [],
24+
occ: BigInt(1)
25+
} as User & {occ: bigint}
26+
27+
const mockAdminUser = {
28+
id: "admin-123",
29+
email: "admin@example.com",
30+
displayName: "Admin User",
31+
createdAt: new Date(),
32+
orgRole: OrgRole.ADMIN,
33+
roles: [],
34+
occ: BigInt(1)
35+
} as User & {occ: bigint}
36+
37+
const mockMemberEntity: AuthenticatedEntity = {
38+
entityType: "user",
39+
user: mockMemberUser
40+
}
41+
42+
const mockAdminEntity: AuthenticatedEntity = {
43+
entityType: "user",
44+
user: mockAdminUser
45+
}
46+
47+
describe("OrganizationAdminService Security", () => {
48+
let service: OrganizationAdminService
49+
50+
beforeEach(async () => {
51+
// Reset mocks
52+
jest.clearAllMocks()
53+
54+
// Mock successful list response
55+
mockRepository.listOrganizationAdmins.mockReturnValue(
56+
TE.right({
57+
admins: [],
58+
page: 1,
59+
limit: 20,
60+
total: 0
61+
})
62+
)
63+
64+
const module: TestingModule = await Test.createTestingModule({
65+
providers: [
66+
OrganizationAdminService,
67+
{
68+
provide: ORGANIZATION_ADMIN_REPOSITORY_TOKEN,
69+
useValue: mockRepository
70+
}
71+
]
72+
}).compile()
73+
74+
service = module.get<OrganizationAdminService>(OrganizationAdminService)
75+
})
76+
77+
it("should return requestor_not_authorized when user is not admin", async () => {
78+
const result = await service.listOrganizationAdmins({
79+
organizationName: "default",
80+
requestor: mockMemberEntity
81+
})()
82+
83+
expect(isLeft(result)).toBe(true)
84+
if (isLeft(result)) {
85+
expect(result.left).toBe("requestor_not_authorized")
86+
}
87+
88+
// Repository should NOT be called
89+
expect(mockRepository.listOrganizationAdmins).not.toHaveBeenCalled()
90+
})
91+
92+
it("should list admins when user is admin", async () => {
93+
const result = await service.listOrganizationAdmins({
94+
organizationName: "default",
95+
requestor: mockAdminEntity
96+
})()
97+
98+
expect(isRight(result)).toBe(true)
99+
// Repository SHOULD be called
100+
expect(mockRepository.listOrganizationAdmins).toHaveBeenCalled()
101+
})
102+
})

app/services/src/organization-admin/organization-admin.service.ts

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -55,8 +55,9 @@ export class OrganizationAdminService {
5555

5656
listOrganizationAdmins(
5757
request: ListOrganizationAdminsRequest
58-
): TaskEither<OrganizationAdminListError, PaginatedOrganizationAdminsList> {
59-
const validateRequest = (req: ListOrganizationAdminsRequest) => {
58+
): TaskEither<OrganizationAdminListError | AuthorizationError, PaginatedOrganizationAdminsList> {
59+
const validateRequest = (req: ListOrganizationAdminsRequest, requestor: User) => {
60+
if (requestor.orgRole !== "admin") return E.left("requestor_not_authorized" as const)
6061
if (req.organizationName !== SUPPORTED_ORGANIZATION) return E.left("organization_not_found" as const)
6162

6263
const page = req.page ?? 1
@@ -76,8 +77,8 @@ export class OrganizationAdminService {
7677
})
7778

7879
return pipe(
79-
request,
80-
validateRequest,
80+
validateUserEntity(request.requestor),
81+
E.chainW(requestor => validateRequest(request, requestor)),
8182
TE.fromEither,
8283
TE.chainW(fetchAdmins),
8384
logSuccess("Organization admins listed", "OrganizationAdminService", result => ({count: result.admins.length}))
@@ -117,7 +118,7 @@ export interface AddOrganizationAdminRequest extends RequestorAwareRequest {
117118
readonly email: string
118119
}
119120

120-
export interface ListOrganizationAdminsRequest {
121+
export interface ListOrganizationAdminsRequest extends RequestorAwareRequest {
121122
readonly organizationName: string
122123
readonly page?: number
123124
readonly limit?: number

0 commit comments

Comments
 (0)