Skip to content

Commit b59c825

Browse files
committed
disallow updating user properties via User.updateMe mutation of the user was provisioned via SCIM
1 parent 27de476 commit b59c825

6 files changed

Lines changed: 137 additions & 65 deletions

File tree

integration-tests/testkit/flow.ts

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2268,3 +2268,26 @@ export function createPersonalAccessToken(
22682268
`),
22692269
});
22702270
}
2271+
2272+
export function updateMe(input: GraphQLSchema.UpdateMeInput, authToken: string) {
2273+
return execute({
2274+
document: graphql(`
2275+
mutation TestKit_UpdateMeMutation($input: UpdateMeInput!) {
2276+
updateMe(input: $input) {
2277+
error {
2278+
message
2279+
}
2280+
ok {
2281+
updatedUser {
2282+
id
2283+
displayName
2284+
fullName
2285+
}
2286+
}
2287+
}
2288+
}
2289+
`),
2290+
variables: { input },
2291+
authToken,
2292+
});
2293+
}

integration-tests/tests/api/auth/scim.spec.ts

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import {
44
createMemberRole,
55
createPersonalAccessToken,
66
readProjectInfo,
7+
updateMe,
78
updateMemberRole,
89
} from 'testkit/flow';
910
import { ResourceAssignmentModeType } from 'testkit/gql/graphql';
@@ -3370,6 +3371,52 @@ test.concurrent('user cannot login via OIDC if SCIM user provisioning is require
33703371
invariant(signInUpResult.type === 'success', 'Expected sign in/up to succeed.');
33713372
});
33723373

3374+
test.concurrent.only(
3375+
'provisioned user cannot update their profile via GraphQL',
3376+
async ({ expect }) => {
3377+
const seed = initSeed();
3378+
const owner = await seed.createOwner();
3379+
const org = await owner.createOrg();
3380+
const oidc = await org.createOIDCIntegration();
3381+
const oidcMock = await oidc.createMockServerAndUpdateIntegrationEndpoints();
3382+
const domain = await oidc.registerFakeDomain();
3383+
const accessToken = await org.createOrganizationAccessToken({
3384+
permissions: ['member:describe', 'member:modify'],
3385+
resources: { mode: ResourceAssignmentModeType.Granular },
3386+
});
3387+
const scim = createScimTestkit({
3388+
baseUrl,
3389+
headers: {
3390+
'Content-Type': 'application/scim+json',
3391+
Authorization: `Bearer ${accessToken.privateAccessKey}`,
3392+
},
3393+
});
3394+
const email = `profile@${domain}`;
3395+
const externalId = crypto.randomUUID();
3396+
3397+
const scimUser = await scim.createUser({
3398+
externalId,
3399+
emails: [{ primary: true, type: 'work', value: email }],
3400+
userName: email,
3401+
});
3402+
3403+
oidcMock.setUser({ email, userIdClaim: externalId });
3404+
const auth = await oidcMock.runGetAuthorizationUrl();
3405+
const signInResult = await oidcMock.runSignInUp({ state: auth.state });
3406+
invariant(signInResult.type === 'success', 'Expected sign in to succeed.');
3407+
3408+
const result = await updateMe(
3409+
{
3410+
displayName: 'updated-display-name',
3411+
fullName: 'Updated Full Name',
3412+
},
3413+
signInResult.accessToken,
3414+
).then(r => r.expectNoGraphQLErrors());
3415+
expect(result.updateMe.ok).toEqual(null);
3416+
expect(result.updateMe.error?.message).toEqual('Provisioned users can not be modified.');
3417+
},
3418+
);
3419+
33733420
test.concurrent(
33743421
'organization admin can still sign in via non-oidc method even if login through the identity provider is enforced',
33753422
async () => {

integration-tests/tests/api/user.spec.ts

Lines changed: 7 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1,37 +1,16 @@
1-
import { graphql } from 'testkit/gql';
2-
import { execute } from 'testkit/graphql';
1+
import { updateMe } from 'testkit/flow';
32
import { initSeed } from 'testkit/seed';
43

5-
const UpdateMeMutation = graphql(`
6-
mutation UsersSpecUpdateMe($input: UpdateMeInput!) {
7-
updateMe(input: $input) {
8-
error {
9-
message
10-
}
11-
ok {
12-
updatedUser {
13-
id
14-
displayName
15-
fullName
16-
}
17-
}
18-
}
19-
}
20-
`);
21-
224
test('can update full name and display name', async () => {
235
const { ownerToken } = await initSeed().createOwner();
246

25-
const result = await execute({
26-
document: UpdateMeMutation,
27-
variables: {
28-
input: {
29-
displayName: 'vegapunk',
30-
fullName: 'Vincent Vega',
31-
},
7+
const result = await updateMe(
8+
{
9+
displayName: 'vegapunk',
10+
fullName: 'Vincent Vega',
3211
},
33-
authToken: ownerToken,
34-
}).then(res => res.expectNoGraphQLErrors());
12+
ownerToken,
13+
).then(res => res.expectNoGraphQLErrors());
3514

3615
expect(result).toEqual({
3716
updateMe: {

packages/services/api/src/modules/auth/providers/auth-manager.ts

Lines changed: 42 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,11 @@
11
import DataLoader from 'dataloader';
22
import { Injectable, Scope } from 'graphql-modules';
3-
import type { User } from '../../../shared/entities';
3+
import { z } from 'zod';
44
import { AccessError } from '../../../shared/errors';
55
import { Storage } from '../../shared/providers/storage';
66
import { Session } from '../lib/authz';
77
import { OrganizationAccessScope, ProjectAccessScope, TargetAccessScope } from './scopes';
8-
import { UserManager } from './user-manager';
8+
import { displayNameLengthBoundaries, fullNameLengthBoundaries, UserManager } from './user-manager';
99

1010
export interface OrganizationAccessSelector {
1111
organizationId: string;
@@ -93,16 +93,51 @@ export class AuthManager {
9393
return owner === selector.userId;
9494
}
9595

96-
async updateCurrentUser(input: { displayName: string; fullName: string }): Promise<User> {
96+
async updateCurrentUser(input: { displayName: string; fullName: string }) {
97+
const InputModel = z.object({
98+
displayName: z
99+
.string()
100+
.min(displayNameLengthBoundaries.min)
101+
.max(displayNameLengthBoundaries.max),
102+
fullName: z.string().min(fullNameLengthBoundaries.min).max(fullNameLengthBoundaries.max),
103+
});
104+
const result = InputModel.safeParse(input);
105+
106+
if (!result.success) {
107+
return {
108+
type: 'error' as const,
109+
error: {
110+
message: 'Please check your input.',
111+
inputErrors: {
112+
displayName: result.error.formErrors.fieldErrors.displayName?.[0],
113+
fullName: result.error.formErrors.fieldErrors.fullName?.[0],
114+
},
115+
},
116+
};
117+
}
118+
97119
const actor = await this.session.getActor();
98120
if (actor.type !== 'user') {
99121
throw new AccessError('Action can only be performed by user.');
100122
}
101123

102-
return this.userManager.updateUser({
103-
id: actor.user.id,
104-
...input,
105-
});
124+
if (actor.user.provisionedByOrganizationId !== null) {
125+
return {
126+
type: 'error' as const,
127+
error: {
128+
message: 'Provisioned users can not be modified.',
129+
inputErrors: {},
130+
},
131+
};
132+
}
133+
134+
return {
135+
type: 'success' as const,
136+
user: await this.userManager.updateUser({
137+
id: actor.user.id,
138+
...input,
139+
}),
140+
};
106141
}
107142
}
108143

Lines changed: 5 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,42 +1,25 @@
1-
import { z } from 'zod';
21
import { AuthManager } from '../../providers/auth-manager';
3-
import {
4-
displayNameLengthBoundaries,
5-
fullNameLengthBoundaries,
6-
} from '../../providers/user-manager';
72
import type { MutationResolvers } from './../../../../__generated__/types';
83

94
export const updateMe: NonNullable<MutationResolvers['updateMe']> = async (
105
_,
116
{ input },
127
{ injector },
138
) => {
14-
const InputModel = z.object({
15-
displayName: z
16-
.string()
17-
.min(displayNameLengthBoundaries.min)
18-
.max(displayNameLengthBoundaries.max),
19-
fullName: z.string().min(fullNameLengthBoundaries.min).max(fullNameLengthBoundaries.max),
20-
});
21-
const result = InputModel.safeParse(input);
9+
const updateResult = await injector.get(AuthManager).updateCurrentUser(input);
2210

23-
if (!result.success) {
11+
if (updateResult.type === 'error') {
2412
return {
2513
error: {
26-
message: 'Please check your input.',
27-
inputErrors: {
28-
displayName: result.error.formErrors.fieldErrors.displayName?.[0],
29-
fullName: result.error.formErrors.fieldErrors.fullName?.[0],
30-
},
14+
inputErrors: updateResult.error.inputErrors,
15+
message: updateResult.error.message,
3116
},
3217
};
3318
}
3419

35-
const updatedUser = await injector.get(AuthManager).updateCurrentUser(input);
36-
3720
return {
3821
ok: {
39-
updatedUser,
22+
updatedUser: updateResult.user,
4023
},
4124
};
4225
};

packages/web/app/src/components/ui/user-menu.tsx

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,9 @@ const UserMenu_MeFragment = graphql(`
7979
provider
8080
isAdmin
8181
canSwitchOrganization
82+
provisionInfo {
83+
__typename
84+
}
8285
}
8386
`);
8487

@@ -184,14 +187,16 @@ export function UserMenu(props: {
184187
</a>
185188
</DropdownMenuItem>
186189

187-
<DropdownMenuItem
188-
onClick={() => {
189-
toggleUserSettingsModalOpen();
190-
}}
191-
>
192-
<SettingsIcon className="mr-2 size-4" />
193-
Profile settings
194-
</DropdownMenuItem>
190+
{me?.provisionInfo ? null : (
191+
<DropdownMenuItem
192+
onClick={() => {
193+
toggleUserSettingsModalOpen();
194+
}}
195+
>
196+
<SettingsIcon className="mr-2 size-4" />
197+
Profile settings
198+
</DropdownMenuItem>
199+
)}
195200
<DropdownMenuSeparator />
196201
<ThemeSwitcher />
197202
<DropdownMenuSeparator />

0 commit comments

Comments
 (0)