Skip to content

Commit 450c20f

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

File tree

4 files changed

+328
-58
lines changed

4 files changed

+328
-58
lines changed

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

Lines changed: 31 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,7 @@ 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 { ensureCanManageCollaborators } from "./ownership-checks";
1112

1213
const GROUPS = ["owner", "collaborator"] as const;
1314

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

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

@@ -71,7 +85,7 @@ export async function remove_collaborators_from_projects(
7185
db: PostgreSQL,
7286
account_id: string,
7387
accounts: string[],
74-
projects: string[], // can be empty strings if tokens specified (since they determine project_id)
88+
projects: string[],
7589
): Promise<void> {
7690
try {
7791
// Ensure user is allowed to modify project(s)
@@ -92,6 +106,21 @@ export async function remove_collaborators_from_projects(
92106
Also, the input is uuid's, which typescript can't check. */
93107
verify_types(account_id, accounts, projects);
94108

109+
// Check strict_collaborator_management setting before database changes
110+
// Skip check if the user is only removing themselves
111+
const is_self_remove_only =
112+
accounts.length === 1 && accounts[0] === account_id;
113+
if (!is_self_remove_only) {
114+
for (const project_id of new Set(projects)) {
115+
if (!project_id) continue; // skip empty strings
116+
await ensureCanManageCollaborators({
117+
project_id,
118+
account_id,
119+
db,
120+
});
121+
}
122+
}
123+
95124
// Remove users from projects
96125
//
97126
for (const i in projects) {

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

Lines changed: 219 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,218 @@ 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+
});

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

Lines changed: 7 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -25,63 +25,14 @@ import {
2525
OwnershipErrorCode,
2626
} from "@cocalc/util/project-ownership";
2727
import { query } from "@cocalc/database/postgres/query";
28-
import { getServerSettings } from "@cocalc/database/settings/server-settings";
28+
import {
29+
ensureCanManageCollaborators,
30+
OwnershipError,
31+
} from "./ownership-checks";
2932

3033
const logger = getLogger("project:collaborators");
3134

32-
export class OwnershipError extends Error {
33-
code: OwnershipErrorCode;
34-
constructor(message: string, code: OwnershipErrorCode) {
35-
super(message);
36-
this.name = "OwnershipError";
37-
this.code = code;
38-
}
39-
}
40-
41-
/**
42-
* Helper function to check if a user can manage collaborators based on the
43-
* manage_users_owner_only setting.
44-
*
45-
* @throws {OwnershipError} If the project is not found or the user is not allowed to manage collaborators
46-
*/
47-
async function ensureCanManageCollaborators(opts: {
48-
project_id: string;
49-
account_id: string;
50-
database?: any;
51-
}): Promise<void> {
52-
const { project_id, account_id, database = db() } = opts;
53-
const serverSettings = await getServerSettings();
54-
const siteEnforced = !!serverSettings.strict_collaborator_management;
55-
56-
const project = await query({
57-
db: database,
58-
table: "projects",
59-
select: ["users", "manage_users_owner_only"],
60-
where: { project_id },
61-
one: true,
62-
});
63-
64-
if (!project) {
65-
throw new OwnershipError(
66-
"Project not found",
67-
OwnershipErrorCode.INVALID_PROJECT_STATE,
68-
);
69-
}
70-
71-
const manage_users_owner_only = project.manage_users_owner_only ?? false;
72-
const requesting_user_group = project.users?.[account_id]?.group as
73-
| UserGroup
74-
| undefined;
75-
76-
const restrictToOwners = siteEnforced || manage_users_owner_only;
77-
78-
if (restrictToOwners && requesting_user_group !== "owner") {
79-
throw new OwnershipError(
80-
"Only owners can manage collaborators when this setting is enabled",
81-
OwnershipErrorCode.NOT_OWNER,
82-
);
83-
}
84-
}
35+
export { OwnershipError } from "./ownership-checks";
8536

8637
export async function removeCollaborator({
8738
account_id,
@@ -311,7 +262,7 @@ export async function inviteCollaborator({
311262
await ensureCanManageCollaborators({
312263
project_id: opts.project_id,
313264
account_id,
314-
database,
265+
db: database,
315266
});
316267

317268
// Actually add user to project
@@ -412,7 +363,7 @@ export async function inviteCollaboratorWithoutAccount({
412363
await ensureCanManageCollaborators({
413364
project_id: opts.project_id,
414365
account_id,
415-
database,
366+
db: database,
416367
});
417368

418369
if (opts.to.length > 1024) {

0 commit comments

Comments
 (0)