Skip to content

Commit 549595f

Browse files
haraldschillyharaldschilly
committed
project/collaborators: check the api in a similar way
1 parent caf1d2e commit 549595f

File tree

4 files changed

+404
-81
lines changed

4 files changed

+404
-81
lines changed

‎src/packages/server/projects/collab.ts‎

Lines changed: 46 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* This file is part of CoCalc: Copyright © 2020 Sagemath, Inc.
2+
* This file is part of CoCalc: Copyright © 2020 - 2025 Sagemath, Inc.
33
* License: MS-RSL – see LICENSE.md for details
44
*/
55

@@ -8,6 +8,10 @@ import { is_array, is_valid_uuid_string } from "@cocalc/util/misc";
88
import { callback2 } from "@cocalc/util/async-utils";
99
import isSandbox from "@cocalc/server/projects/is-sandbox";
1010
import idleSandboxUsers from "@cocalc/server/projects/idle-sandbox-users";
11+
import {
12+
ensureCanManageCollaborators,
13+
ensureCanRemoveUser,
14+
} from "./ownership-checks";
1115

1216
const GROUPS = ["owner", "collaborator"] as const;
1317

@@ -40,6 +44,19 @@ export async function add_collaborators_to_projects(
4044
Also, the input is uuid's, which typescript can't check. */
4145
verify_types(account_id, accounts, projects);
4246

47+
// Check strict_collaborator_management setting before database changes
48+
// Only check if not using tokens (tokens have their own permission system)
49+
if (!tokens) {
50+
for (const project_id of new Set(projects)) {
51+
if (!project_id) continue; // skip empty strings
52+
await ensureCanManageCollaborators({
53+
project_id,
54+
account_id,
55+
db,
56+
});
57+
}
58+
}
59+
4360
// We now know that account_id is allowed to add users to all of the projects,
4461
// *OR* at that there are valid tokens to permit adding users.
4562

@@ -71,7 +88,7 @@ export async function remove_collaborators_from_projects(
7188
db: PostgreSQL,
7289
account_id: string,
7390
accounts: string[],
74-
projects: string[], // can be empty strings if tokens specified (since they determine project_id)
91+
projects: string[],
7592
): Promise<void> {
7693
try {
7794
// Ensure user is allowed to modify project(s)
@@ -92,6 +109,33 @@ export async function remove_collaborators_from_projects(
92109
Also, the input is uuid's, which typescript can't check. */
93110
verify_types(account_id, accounts, projects);
94111

112+
// Check strict_collaborator_management setting before database changes
113+
// Skip check if the user is only removing themselves
114+
const is_self_remove_only =
115+
accounts.length === 1 && accounts[0] === account_id;
116+
if (!is_self_remove_only) {
117+
for (const project_id of new Set(projects)) {
118+
if (!project_id) continue; // skip empty strings
119+
await ensureCanManageCollaborators({
120+
project_id,
121+
account_id,
122+
db,
123+
});
124+
}
125+
}
126+
127+
// CRITICAL: Verify that no target users are owners
128+
// Owners must be demoted to collaborator first, then removed
129+
for (const i in projects) {
130+
const project_id: string = projects[i];
131+
const target_account_id: string = accounts[i];
132+
await ensureCanRemoveUser({
133+
project_id,
134+
target_account_id,
135+
db,
136+
});
137+
}
138+
95139
// Remove users from projects
96140
//
97141
for (const i in projects) {

‎src/packages/server/projects/collaborators-ownership.test.ts‎

Lines changed: 250 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,10 @@ import {
1616
inviteCollaboratorWithoutAccount,
1717
removeCollaborator,
1818
} from "@cocalc/server/projects/collaborators";
19+
import {
20+
add_collaborators_to_projects,
21+
remove_collaborators_from_projects,
22+
} from "@cocalc/server/projects/collab";
1923
import {
2024
resetServerSettingsCache,
2125
getServerSettings,
@@ -320,3 +324,249 @@ describe("invite permissions", () => {
320324
).rejects.toMatchObject({ code: OwnershipErrorCode.NOT_OWNER });
321325
});
322326
});
327+
328+
describe("REST API level protection (collab.ts functions)", () => {
329+
test("add_collaborators_to_projects bypasses check when using tokens", async () => {
330+
const { projectId } = await createProjectWithOwner();
331+
const outsideUserId = await createUser("outsider");
332+
333+
// Create a project invite token
334+
const { rows } = await getPool().query(
335+
"INSERT INTO project_invite_tokens (token, project_id, usage_limit) VALUES ($1, $2, $3) RETURNING token",
336+
["test-token-123", projectId, 10],
337+
);
338+
const token = rows[0].token;
339+
340+
// Enable site-wide strict collaborator management
341+
await setSiteStrictCollab("yes");
342+
343+
// Outside user should be able to add themselves using a token
344+
// even though they're not a collaborator or owner
345+
await expect(
346+
add_collaborators_to_projects(
347+
db(),
348+
outsideUserId,
349+
[outsideUserId],
350+
[""], // empty project_id because token determines the project
351+
[token],
352+
),
353+
).resolves.toBeUndefined();
354+
355+
// Verify the user was actually added
356+
const { rows: projectRows } = await getPool().query(
357+
"SELECT users FROM projects WHERE project_id=$1",
358+
[projectId],
359+
);
360+
expect(projectRows[0].users[outsideUserId]).toBeDefined();
361+
});
362+
363+
test("add_collaborators_to_projects enforces strict_collaborator_management site setting", async () => {
364+
const { ownerId, projectId } = await createProjectWithOwner();
365+
const collaboratorId = await createUser("collab");
366+
const newCollabId = await createUser("newcollab");
367+
368+
// Add first collaborator as owner
369+
await add_collaborators_to_projects(
370+
db(),
371+
ownerId,
372+
[collaboratorId],
373+
[projectId],
374+
);
375+
376+
// Enable site-wide strict collaborator management
377+
await setSiteStrictCollab("yes");
378+
379+
// Collaborator should not be able to add another user
380+
await expect(
381+
add_collaborators_to_projects(
382+
db(),
383+
collaboratorId,
384+
[newCollabId],
385+
[projectId],
386+
),
387+
).rejects.toThrow("Only owners can manage collaborators");
388+
});
389+
390+
test("add_collaborators_to_projects enforces project-level manage_users_owner_only", async () => {
391+
const { ownerId, projectId } = await createProjectWithOwner();
392+
const collaboratorId = await createUser("collab");
393+
const newCollabId = await createUser("newcollab");
394+
395+
// Add first collaborator as owner
396+
await add_collaborators_to_projects(
397+
db(),
398+
ownerId,
399+
[collaboratorId],
400+
[projectId],
401+
);
402+
403+
// Enable project-level strict management
404+
await getPool().query(
405+
"UPDATE projects SET manage_users_owner_only=$1 WHERE project_id=$2",
406+
[true, projectId],
407+
);
408+
409+
// Collaborator should not be able to add another user
410+
await expect(
411+
add_collaborators_to_projects(
412+
db(),
413+
collaboratorId,
414+
[newCollabId],
415+
[projectId],
416+
),
417+
).rejects.toThrow("Only owners can manage collaborators");
418+
});
419+
420+
test("remove_collaborators_from_projects enforces strict_collaborator_management site setting", async () => {
421+
const { ownerId, projectId } = await createProjectWithOwner();
422+
const collaborator1Id = await createUser("collab1");
423+
const collaborator2Id = await createUser("collab2");
424+
425+
// Add collaborators as owner
426+
await add_collaborators_to_projects(
427+
db(),
428+
ownerId,
429+
[collaborator1Id, collaborator2Id],
430+
[projectId, projectId],
431+
);
432+
433+
// Enable site-wide strict collaborator management
434+
await setSiteStrictCollab("yes");
435+
436+
// Collaborator should not be able to remove another collaborator
437+
await expect(
438+
remove_collaborators_from_projects(
439+
db(),
440+
collaborator1Id,
441+
[collaborator2Id],
442+
[projectId],
443+
),
444+
).rejects.toThrow("Only owners can manage collaborators");
445+
});
446+
447+
test("remove_collaborators_from_projects allows self-removal even with strict_collaborator_management", async () => {
448+
const { ownerId, projectId } = await createProjectWithOwner();
449+
const collaboratorId = await createUser("collab");
450+
451+
// Add collaborator as owner
452+
await add_collaborators_to_projects(
453+
db(),
454+
ownerId,
455+
[collaboratorId],
456+
[projectId],
457+
);
458+
459+
// Enable site-wide strict collaborator management
460+
await setSiteStrictCollab("yes");
461+
462+
// Collaborator should be able to remove themselves
463+
await expect(
464+
remove_collaborators_from_projects(
465+
db(),
466+
collaboratorId,
467+
[collaboratorId],
468+
[projectId],
469+
),
470+
).resolves.toBeUndefined();
471+
});
472+
473+
test("remove_collaborators_from_projects enforces project-level manage_users_owner_only", async () => {
474+
const { ownerId, projectId } = await createProjectWithOwner();
475+
const collaborator1Id = await createUser("collab1");
476+
const collaborator2Id = await createUser("collab2");
477+
478+
// Add collaborators as owner
479+
await add_collaborators_to_projects(
480+
db(),
481+
ownerId,
482+
[collaborator1Id, collaborator2Id],
483+
[projectId, projectId],
484+
);
485+
486+
// Enable project-level strict management
487+
await getPool().query(
488+
"UPDATE projects SET manage_users_owner_only=$1 WHERE project_id=$2",
489+
[true, projectId],
490+
);
491+
492+
// Collaborator should not be able to remove another collaborator
493+
await expect(
494+
remove_collaborators_from_projects(
495+
db(),
496+
collaborator1Id,
497+
[collaborator2Id],
498+
[projectId],
499+
),
500+
).rejects.toThrow("Only owners can manage collaborators");
501+
});
502+
503+
test("add_collaborators_to_projects allows owners to add when strict management is enabled", async () => {
504+
const { ownerId, projectId } = await createProjectWithOwner();
505+
const newCollabId = await createUser("newcollab");
506+
507+
// Enable site-wide strict collaborator management
508+
await setSiteStrictCollab("yes");
509+
510+
// Owner should be able to add a collaborator
511+
await expect(
512+
add_collaborators_to_projects(db(), ownerId, [newCollabId], [projectId]),
513+
).resolves.toBeUndefined();
514+
});
515+
516+
test("remove_collaborators_from_projects allows owners to remove when strict management is enabled", async () => {
517+
const { ownerId, projectId } = await createProjectWithOwner();
518+
const collaboratorId = await createUser("collab");
519+
520+
// Add collaborator
521+
await add_collaborators_to_projects(
522+
db(),
523+
ownerId,
524+
[collaboratorId],
525+
[projectId],
526+
);
527+
528+
// Enable site-wide strict collaborator management
529+
await setSiteStrictCollab("yes");
530+
531+
// Owner should be able to remove a collaborator
532+
await expect(
533+
remove_collaborators_from_projects(
534+
db(),
535+
ownerId,
536+
[collaboratorId],
537+
[projectId],
538+
),
539+
).resolves.toBeUndefined();
540+
});
541+
542+
test("remove_collaborators_from_projects blocks removing owners", async () => {
543+
const { ownerId, projectId } = await createProjectWithOwner();
544+
const secondOwnerId = await createUser("owner2");
545+
546+
// Add second owner
547+
await add_collaborators_to_projects(
548+
db(),
549+
ownerId,
550+
[secondOwnerId],
551+
[projectId],
552+
);
553+
await changeUserType({
554+
account_id: ownerId,
555+
opts: {
556+
project_id: projectId,
557+
target_account_id: secondOwnerId,
558+
new_group: "owner",
559+
},
560+
});
561+
562+
// Even the first owner should not be able to directly remove the second owner -- owners have to demote to collaborator first
563+
await expect(
564+
remove_collaborators_from_projects(
565+
db(),
566+
ownerId,
567+
[secondOwnerId],
568+
[projectId],
569+
),
570+
).rejects.toThrow("Cannot remove an owner");
571+
});
572+
});

0 commit comments

Comments
 (0)