Skip to content

feat: share selected workspaces with users - #626

Open
shenlvkang-collab wants to merge 2 commits into
Ark0N:masterfrom
shenlvkang-collab:feat/shared-workspace-access
Open

shenlvkang-collab wants to merge 2 commits into
Ark0N:masterfrom
shenlvkang-collab:feat/shared-workspace-access

Conversation

@shenlvkang-collab

Copy link
Copy Markdown
Contributor

Summary

  • Add admin-managed grants for selected linked local workspaces.
  • Restrict regular users' CodeMan case/session/file-picker access and project creation to those grants while keeping sessions private to their owner and visible to admins.
  • Persist workspace policies with mode 0600 and document the application-level boundary. Existing bypass-permissions and shell grants intentionally remain effective for trusted collaborators.

Verification

  • npm run typecheck passed.
  • npm run lint passed.
  • npm run format:check passed.
  • git diff --check passed.
  • npm run build could not run because the shared node_modules is missing exceljs/dist/exceljs.min.js and fflate.
  • Tests were not run.

The feature prevents accidental project selection in CodeMan; it is not an OS sandbox because all sessions run under the same OS account.

@Ark0N

Ark0N commented Oct 14, 2026

Copy link
Copy Markdown
Owner

Thanks for this, @shenlvkang-collab. Letting an admin give a regular user access to selected linked projects, and nothing else, is a useful addition to multi-user mode, and the docs you wrote around the trust boundary are clear.

I can't merge it yet. Two things block it, and a few more need fixing first.

Blocking

  1. CI is red: 5 existing tests fail. The new workingDir check in findSessionOrFail / findPersistedSessionOrFail (src/web/route-helpers.ts:343 and :375) runs for every non-admin owner, not only restricted ones. So a regular user's own session outside ~/codeman-users/<name>/cases now returns 404. Failing: test/ownership-scoping.test.ts, test/routes/git-status-routes.test.ts, test/routes/readmymind-routes.test.ts and two in test/routes/session-custom-model.test.ts. All five pass on master. The same thing happens to an admin who is demoted while they have live sessions: the session still shows in GET /api/sessions, but every lookup is 404. Please limit the check to restricted users, and use the same predicate in the list and the lookup. Better still, see the next point.
  2. Synchronous filesystem calls on linked paths in request handlers. isWorkingDirAllowed (route-helpers.ts:194) reads two JSON files with readFileSync and runs realpathSync on the granted linked paths. It now runs on every session request from a regular user, once per session in GET /api/sessions, once per transcript in GET /api/history/sessions, and in quick-start before the bounded probe at session-routes.ts:3951, which exists for exactly this case. A linked project on a network mount that stops answering would freeze the server for everyone. The admin GET/PUT handlers also do unbounded fsp.realpath/fsp.stat per linked path (admin-routes.ts:135, :180). The project rule (fix(cases): bound path probes for linked workspaces and session creation, so an unreachable mount cannot freeze the server #516 in CLAUDE.md) is that user-chosen paths on request paths go through probePath()/probePathKind(). Since PUT already stops out-of-grant sessions and every create path already confines workingDir, I'd drop the per-request check. If you keep it, make it a pure comparison against grant paths resolved once when the policy is saved, and keep the policy in memory instead of re-reading the file on each call.

Please fix

  1. Conversation history leaks between users. GET /api/history/sessions (session-routes.ts:5054, :5073) filters by the transcript's cwd. In a shared workspace that no longer implies ownership, so a granted user sees the admin's and other users' conversations there, with their prompts, and can resume them. For restricted users, either return no host history (as GET /api/sessions/unified already does for non-admins) or keep only transcripts that map to sessions the caller owns.
  2. The policy file fails open. readJsonObject (src/shared-workspace-store.ts:26) treats a corrupt or unreadable file as "no policies", so every restriction disappears. The next admin save then rewrites the file and drops every other user's policy. Treat only ENOENT as empty. Fail closed otherwise, and refuse to overwrite a file that did not parse.
  3. Tests. Please add route tests on the existing multi-user harness (test/ownership-scoping.test.ts, test/admin-routes.test.ts): grant and clear; PUT refusing unlinked names and admins; a restricted user listing only granted cases and getting 403 on create/clone/link; starting a session inside a grant and being refused outside it; PUT stopping out-of-grant sessions; a second restricted user being unable to see or address the first one's session in the same workspace; and unrestricted users unchanged.

Smaller things

  • Grants are keyed by case name, so after an unlink the name stays in each policy, and linking another folder under that name grants it straight away (shared-workspace-store.ts:92). Prune the name on unlink (case-routes.ts:1670), or store the resolved path with it.
  • "Restrict to selected" kills sessions with no confirm, while "Remove restriction" asks first (admin-ui.js:470 vs :479). Please confirm on Save and say how many sessions will stop.
  • Please update the rule docs that this changes: the Filesystem path picker bullet in CLAUDE.md, plus docs/architecture-invariants.md#multi-user-mode and #filesystem-path-picker. Also, folderAllowedForUser (src/folder-listing.ts:155) still lets a restricted user open their private space in the folder grid, which the new wiki text says is hidden.
  • Nits: use Object.hasOwn instead of name in linked (admin-routes.ts:173); pass reply to refuseRestrictedProjectCreation everywhere so the refusal is always a 403 (case-routes.ts:861, :948, :1242, :1435, and the delete and Ralph refusals); return not-found instead of the .codeman-workspace-not-granted placeholder path (case-routes.ts:211); resolve the store's file paths at call time.

One design question before you rework it: should a restriction replace the user's private space, as it does now, or add the granted projects alongside it? I'll answer that here so you aren't guessing. Once the blockers and the history leak are fixed and CI is green, I'll review again.

@Ark0N

Ark0N commented Oct 14, 2026

Copy link
Copy Markdown
Owner

Thanks for the follow-up push, @shenlvkang-collab. This PR lets an admin limit a regular multi-user account to selected linked projects, and the feature itself is a good fit for multi-user mode.

The latest commit (ce6f07b9) only changes how the admin listing deduplicates folders, so the points from my earlier review still apply. Here is the full list against the current head, so you have it in one place.

Blocking

  1. CI is red: 5 existing tests fail. findSessionOrFail and findPersistedSessionOrFail (src/web/route-helpers.ts:344 and :376) run the workingDir check for every non-admin owner, not only restricted ones. An unrestricted user's own session outside ~/codeman-users/<name>/cases (for example a demoted admin's) now answers 404 on every per-session route while GET /api/sessions still lists it. Failing: test/ownership-scoping.test.ts, test/routes/git-status-routes.test.ts, test/routes/readmymind-routes.test.ts and two cases in test/routes/session-custom-model.test.ts; all of them pass on master. Please limit the check to restricted users and use the same predicate in the list and the lookup, or drop it (see the next point).
  2. Synchronous filesystem calls on linked paths in request handlers. isWorkingDirAllowed (route-helpers.ts:192) re-reads two JSON files with readFileSync and runs realpathSync on the granted linked paths. It now runs on every session lookup for a regular user, once per row in GET /api/sessions and GET /api/history/sessions, and in quick-start before the bounded probe at session-routes.ts:3951. A linked project on a network mount that stops answering freezes the server for everyone, which is the fix(cases): bound path probes for linked workspaces and session creation, so an unreachable mount cannot freeze the server #516 rule in CLAUDE.md (user-chosen paths are probed bounded via probePath()/probePathKind()). The admin GET and PUT handlers also call fsp.realpath/fsp.stat unbounded (admin-routes.ts:131-137, :180). Since PUT already stops out-of-grant sessions and every create path already confines workingDir, I'd drop the per-request check. If you keep it, keep the policy in memory, resolve grant paths once when the policy is saved, and make the check a pure path comparison.

Please fix

  1. Conversation history leaks between users. GET /api/history/sessions (session-routes.ts:5054 and :5073) filters transcripts by cwd. In a shared workspace that no longer implies ownership, so a granted user sees the admin's and other users' conversations there, with their first prompts. The listed id can also be resumed, because POST /api/sessions only checks that the transcript file exists (session-routes.ts:1231-1260). For restricted users, return no host history (as GET /api/sessions/unified already does for non-admins) or keep only rows that map to sessions the caller owns.
  2. The policy file fails open. readJsonObject (src/shared-workspace-store.ts:26) treats a corrupt or unreadable file as "no policies". I checked it: after truncating shared-workspaces.json, every restricted user becomes unrestricted, and the next admin save rewrites the file with only that one user's policy. Treat only ENOENT as empty, fail closed otherwise, and refuse to overwrite a file that did not parse.
  3. Tests. Please add route tests on the existing multi-user harness (test/ownership-scoping.test.ts, test/admin-routes.test.ts, app.inject): grant and clear; PUT refusing unlinked names and admins; a restricted user listing only granted cases and getting 403 on create, clone, link and the docker routes; quick-start allowed inside a grant and refused outside it; PUT stopping out-of-grant sessions; a second restricted user unable to see or address the first one's session in the same workspace; history not leaking; a corrupt policy file failing closed; unrestricted users unchanged.

Smaller things

  • The new dedup in GET /api/admin/users/:username/workspaces (admin-routes.ts:136) keeps only the first name per folder. A grant held under a second name for the same folder is then not shown in the modal, and the next Save drops it and stops those sessions. List every name, or carry granted names through.
  • Grants are keyed by case name, so after an unlink the name stays in each policy and linking another folder under that name grants it straight away (shared-workspace-store.ts:62). Prune the name on unlink (case-routes.ts:1671), or store the resolved path with it.
  • "Restrict to selected" stops sessions with no confirm, while "Remove restriction" asks first (admin-ui.js:470 vs :479). Please confirm on Save and say how many sessions will stop.
  • folderAllowedForUser (src/folder-listing.ts:155) still lets a restricted user open their private space in the folder grid, and the terminal WebSocket (ws-routes.ts:106) checks ownership only, so a session PUT failed to stop stays attachable.
  • Please update the rule docs this changes: the Filesystem path picker bullet in CLAUDE.md, and docs/architecture-invariants.md#multi-user-mode and #filesystem-path-picker.
  • Nits: pass reply to refuseRestrictedProjectCreation everywhere so the refusal is always a 403 (case-routes.ts:861, :948, :1242, :1435, the delete refusal at :1616 and the Ralph refusal at ralph-routes.ts:302); use Object.hasOwn instead of name in linked (admin-routes.ts:173); return not-found instead of the .codeman-workspace-not-granted placeholder path (case-routes.ts:211); resolve the store's file paths at call time.

I'll answer the open design question (should a restriction replace the user's private space, as it does now, or add the granted projects alongside it) in this thread before you rework the case listing. Once the blocking items and the history leak are fixed and CI is green, I'll review again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants