Skip to content

Commit bd68eec

Browse files
wilsonriverawilsonrivera
authored
feat: prevent privilege escalation (#2428)
1 parent 8237c8d commit bd68eec

File tree

7 files changed

+156
-8
lines changed

7 files changed

+156
-8
lines changed

‎controlplane/src/core/bufservices/api-key/createAPIKey.ts‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import { ApiKeyGenerator } from '../../services/ApiGenerator.js';
99
import { enrichLogger, getLogger, handleError } from '../../util.js';
1010
import { OrganizationGroupRepository } from '../../repositories/OrganizationGroupRepository.js';
1111
import { UnauthorizedError } from '../../errors/errors.js';
12+
import { RBACEvaluator } from '../../services/RBACEvaluator.js';
1213

1314
export function createAPIKey(
1415
opts: RouterOptions,
@@ -70,8 +71,18 @@ export function createAPIKey(
7071
};
7172
}
7273

73-
const generatedAPIKey = ApiKeyGenerator.generate();
74+
const rbac = new RBACEvaluator([orgGroup]);
75+
if (rbac.isOrganizationAdmin && !authContext.rbac.isOrganizationAdmin) {
76+
return {
77+
response: {
78+
code: EnumStatusCode.ERR,
79+
details: `You don't have access to create an API key with the group "${orgGroup.name}"`,
80+
},
81+
apiKey: '',
82+
};
83+
}
7484

85+
const generatedAPIKey = ApiKeyGenerator.generate();
7586
await apiKeyRepo.addAPIKey({
7687
name: keyName,
7788
organizationID: authContext.organizationId,

‎controlplane/src/core/bufservices/api-key/deleteAPIKey.ts‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@ import { AuditLogRepository } from '../../repositories/AuditLogRepository.js';
77
import type { RouterOptions } from '../../routes.js';
88
import { enrichLogger, getLogger, handleError } from '../../util.js';
99
import { UnauthorizedError } from '../../errors/errors.js';
10+
import { OrganizationGroupRepository } from '../../repositories/OrganizationGroupRepository.js';
11+
import { RBACEvaluator } from '../../services/RBACEvaluator.js';
1012

1113
export function deleteAPIKey(
1214
opts: RouterOptions,
@@ -21,6 +23,7 @@ export function deleteAPIKey(
2123

2224
const apiKeyRepo = new ApiKeyRepository(opts.db);
2325
const auditLogRepo = new AuditLogRepository(opts.db);
26+
const groupRepo = new OrganizationGroupRepository(opts.db);
2427

2528
if (authContext.organizationDeactivated) {
2629
throw new UnauthorizedError();
@@ -40,6 +43,26 @@ export function deleteAPIKey(
4043
throw new UnauthorizedError();
4144
}
4245

46+
if (apiKey.group?.id) {
47+
const group = await groupRepo.byId({
48+
organizationId: authContext.organizationId,
49+
groupId: apiKey.group.id,
50+
});
51+
52+
if (group) {
53+
const rbac = new RBACEvaluator([group]);
54+
if (rbac.isOrganizationAdmin && !authContext.rbac.isOrganizationAdmin) {
55+
return {
56+
response: {
57+
code: EnumStatusCode.ERR,
58+
details: `You don't have access to remove the API key "${apiKey.name}"`,
59+
},
60+
apiKey: '',
61+
};
62+
}
63+
}
64+
}
65+
4366
await apiKeyRepo.removeAPIKey({
4467
name: req.name,
4568
organizationID: authContext.organizationId,

‎controlplane/src/core/bufservices/api-key/updateAPIKey.ts‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import type { RouterOptions } from '../../routes.js';
88
import { enrichLogger, getLogger, handleError } from '../../util.js';
99
import { OrganizationGroupRepository } from '../../repositories/OrganizationGroupRepository.js';
1010
import { UnauthorizedError } from '../../errors/errors.js';
11+
import { RBACEvaluator } from '../../services/RBACEvaluator.js';
1112

1213
export function updateAPIKey(
1314
opts: RouterOptions,
@@ -56,8 +57,18 @@ export function updateAPIKey(
5657
};
5758
}
5859

59-
await apiKeyRepo.updateAPIKeyGroup({ apiKeyId: apiKey.id, groupId: orgGroup.groupId });
60+
const rbac = new RBACEvaluator([orgGroup]);
61+
if (rbac.isOrganizationAdmin && !authContext.rbac.isOrganizationAdmin) {
62+
return {
63+
response: {
64+
code: EnumStatusCode.ERR,
65+
details: `You don't have access to update the API key group to "${orgGroup.name}"`,
66+
},
67+
apiKey: '',
68+
};
69+
}
6070

71+
await apiKeyRepo.updateAPIKeyGroup({ apiKeyId: apiKey.id, groupId: orgGroup.groupId });
6172
await auditLogRepo.addAuditLog({
6273
organizationId: authContext.organizationId,
6374
organizationSlug: authContext.organizationSlug,

‎controlplane/src/core/repositories/ApiKeyRepository.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,7 @@ export class ApiKeyRepository {
8787
expiresAt: apiKeys.expiresAt,
8888
createdBy: users.email,
8989
creatorUserID: users.id,
90+
groupId: apiKeys.groupId,
9091
})
9192
.from(apiKeys)
9293
.innerJoin(users, eq(users.id, apiKeys.userId))
@@ -105,6 +106,7 @@ export class ApiKeyRepository {
105106
expiresAt: key[0].expiresAt?.toISOString() ?? '',
106107
createdBy: key[0].createdBy,
107108
creatorUserID: key[0].creatorUserID,
109+
group: { id: key[0].groupId },
108110
} as APIKeyDTO;
109111
}
110112

‎controlplane/test/api-keys.test.ts‎

Lines changed: 103 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -110,20 +110,20 @@ describe('API Keys', (ctx) => {
110110
const orgGroupsResponse = await client.getOrganizationGroups({});
111111
expect(orgGroupsResponse.response?.code).toBe(EnumStatusCode.OK);
112112

113-
const adminGroup = orgGroupsResponse.groups.find((g) => g.name === 'admin')!;
114113
const developerGroup = orgGroupsResponse.groups.find((g) => g.name === 'developer')!;
114+
const viewerGroup = orgGroupsResponse.groups.find((g) => g.name === 'viewer')!;
115115

116116
authenticator.changeUserWithSuppliedContext({
117117
...users[TestUser.adminAliceCompanyA],
118118
rbac: createTestRBACEvaluator(createTestGroup({ role: role as OrganizationRole })),
119119
});
120120

121-
// Create the API key with the `admin` group
121+
// Create the API key with the `viewer` group
122122
const apiKeyName = uid();
123123
const createApiKeyResponse = await client.createAPIKey({
124124
name: apiKeyName,
125125
expires: ExpiresAt.NEVER,
126-
groupId: adminGroup.groupId,
126+
groupId: viewerGroup.groupId,
127127
});
128128

129129
expect(createApiKeyResponse.response?.code).toBe(EnumStatusCode.OK);
@@ -136,7 +136,7 @@ describe('API Keys', (ctx) => {
136136

137137
expect(updateApiKeyResponse.response?.code).toBe(EnumStatusCode.OK);
138138

139-
// Ensure that the API key have the correct group
139+
// Ensure that the API key has the correct group
140140
let getApiKeysResponse = await client.getAPIKeys({});
141141
let apiKey = getApiKeysResponse.apiKeys?.find((k) => k.name === apiKeyName);
142142

@@ -150,7 +150,7 @@ describe('API Keys', (ctx) => {
150150

151151
expect(deleteApiKeyResponse.response?.code).toBe(EnumStatusCode.OK);
152152

153-
// Ensure the API key have been deleted
153+
// Ensure the API key has been deleted
154154
getApiKeysResponse = await client.getAPIKeys({});
155155
apiKey = getApiKeysResponse.apiKeys?.find((k) => k.name === apiKeyName);
156156

@@ -160,6 +160,104 @@ describe('API Keys', (ctx) => {
160160
await server.close();
161161
});
162162

163+
test('that an "organization-apikey-manager" cannot create API keys with admin role', async () => {
164+
const { client, server, users, authenticator } = await SetupTest({ dbname, enableMultiUsers: true });
165+
166+
const orgGroupsResponse = await client.getOrganizationGroups({});
167+
expect(orgGroupsResponse.response?.code).toBe(EnumStatusCode.OK);
168+
169+
const adminGroup = orgGroupsResponse.groups.find((g) => g.name === 'admin')!;
170+
171+
authenticator.changeUserWithSuppliedContext({
172+
...users[TestUser.adminAliceCompanyA],
173+
rbac: createTestRBACEvaluator(createTestGroup({ role: 'organization-apikey-manager' })),
174+
});
175+
176+
// Create the API key with the `admin` group
177+
const apiKeyName = uid();
178+
const createApiKeyResponse = await client.createAPIKey({
179+
name: apiKeyName,
180+
expires: ExpiresAt.NEVER,
181+
groupId: adminGroup.groupId,
182+
});
183+
184+
expect(createApiKeyResponse.response?.code).toBe(EnumStatusCode.ERR);
185+
expect(createApiKeyResponse.response?.details).toBe(`You don't have access to create an API key with the group "admin"`);
186+
187+
await server.close();
188+
});
189+
190+
test('that an "organization-apikey-manager" cannot update API keys with admin role', async () => {
191+
const { client, server, users, authenticator } = await SetupTest({ dbname, enableMultiUsers: true });
192+
193+
const orgGroupsResponse = await client.getOrganizationGroups({});
194+
expect(orgGroupsResponse.response?.code).toBe(EnumStatusCode.OK);
195+
196+
const adminGroup = orgGroupsResponse.groups.find((g) => g.name === 'admin')!;
197+
const viewerGroup = orgGroupsResponse.groups.find((g) => g.name === 'viewer')!;
198+
199+
authenticator.changeUserWithSuppliedContext({
200+
...users[TestUser.adminAliceCompanyA],
201+
rbac: createTestRBACEvaluator(createTestGroup({ role: 'organization-apikey-manager' })),
202+
});
203+
204+
// Create the API key with the `viewer` group
205+
const apiKeyName = uid();
206+
const createApiKeyResponse = await client.createAPIKey({
207+
name: apiKeyName,
208+
expires: ExpiresAt.NEVER,
209+
groupId: viewerGroup.groupId,
210+
});
211+
212+
expect(createApiKeyResponse.response?.code).toBe(EnumStatusCode.OK);
213+
214+
// Update the API key to the `admin` group
215+
const updateApiKeyResponse = await client.updateAPIKey({
216+
name: apiKeyName,
217+
groupId: adminGroup.groupId,
218+
});
219+
220+
expect(updateApiKeyResponse.response?.code).toBe(EnumStatusCode.ERR);
221+
expect(updateApiKeyResponse.response?.details).toBe(`You don't have access to update the API key group to "admin"`);
222+
223+
await server.close();
224+
});
225+
226+
test('that an "organization-apikey-manager" cannot delete an API key with admin role', async () => {
227+
const { client, server, users, authenticator } = await SetupTest({ dbname, enableMultiUsers: true });
228+
229+
const orgGroupsResponse = await client.getOrganizationGroups({});
230+
expect(orgGroupsResponse.response?.code).toBe(EnumStatusCode.OK);
231+
232+
const adminGroup = orgGroupsResponse.groups.find((g) => g.name === 'admin')!;
233+
authenticator.changeUserWithSuppliedContext({
234+
...users[TestUser.adminAliceCompanyA],
235+
});
236+
237+
// Create the API key with the `admin` group
238+
const apiKeyName = uid();
239+
const createApiKeyResponse = await client.createAPIKey({
240+
name: apiKeyName,
241+
expires: ExpiresAt.NEVER,
242+
groupId: adminGroup.groupId,
243+
});
244+
245+
expect(createApiKeyResponse.response?.code).toBe(EnumStatusCode.OK);
246+
247+
// Try to delete the API key
248+
authenticator.changeUserWithSuppliedContext({
249+
...users[TestUser.adminAliceCompanyA],
250+
rbac: createTestRBACEvaluator(createTestGroup({ role: 'organization-apikey-manager' })),
251+
});
252+
253+
const deleteApiKeyResponse = await client.deleteAPIKey({ name: apiKeyName });
254+
255+
expect(deleteApiKeyResponse.response?.code).toBe(EnumStatusCode.ERR);
256+
expect(deleteApiKeyResponse.response?.details).toBe(`You don't have access to remove the API key "${apiKeyName}"`);
257+
258+
await server.close();
259+
});
260+
163261
test.each([
164262
'organization-developer',
165263
'organization-viewer',

‎studio/src/components/group-select.tsx‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import { useQuery } from "@connectrpc/connect-query";
44
import { getOrganizationGroups } from "@wundergraph/cosmo-connect/dist/platform/v1/platform-PlatformService_connectquery";
55
import { EnumStatusCode } from "@wundergraph/cosmo-connect/dist/common/common_pb";
66
import { Button } from "@/components/ui/button";
7+
import { useIsAdmin } from "@/hooks/use-is-admin";
78

89
export function GroupSelect({ id, value, disabled = false, groups, onValueChange }: {
910
id?: string;
@@ -12,6 +13,7 @@ export function GroupSelect({ id, value, disabled = false, groups, onValueChange
1213
groups?: OrganizationGroup[];
1314
onValueChange(group: OrganizationGroup): void;
1415
}) {
16+
const isAdmin = useIsAdmin();
1517
const { data, isPending, error, refetch } = useQuery(getOrganizationGroups, {}, { enabled: groups === undefined });
1618
if (isPending) {
1719
return (
@@ -60,6 +62,7 @@ export function GroupSelect({ id, value, disabled = false, groups, onValueChange
6062
<SelectItem
6163
key={`group-${group.groupId}`}
6264
value={group.groupId}
65+
disabled={!isAdmin && group.rules.some((rule) => rule.role === 'organization-admin')}
6366
>
6467
{group.name}
6568
</SelectItem>

‎studio/src/pages/[organizationSlug]/members.tsx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,7 @@ const InviteForm = ({ onSuccess }: { onSuccess: () => void }) => {
132132
</div>
133133

134134
<div className="space-y-2">
135-
<span>What group should the member be added to?</span>
135+
<span>What group(s) should the member be added to?</span>
136136
<MultiGroupSelect
137137
disabled={isPending}
138138
value={selectedGroups}

0 commit comments

Comments
 (0)