Add container sharing (collaborators) feature by cmyers-mieweb · Pull Request #388 · mieweb/opensource-server

cmyers-mieweb · 2026-06-30T16:15:14Z

Introduce container sharing/collaborators so containers can be shared with additional users.

Details:

Backend: add ContainerCollaborator model and migration with unique (containerId, username) constraint and CASCADE deletion.
DB/model changes: Container.hasMany(ContainerCollaborator) association and eager-load collaborators in container queries/serializer.
Permissions: implement userCanAccess / userCanManage helpers and loadContainerForSession to authorize view vs manage actions.
Listing/filtering: applyOwnershipFilter folds shared containers into the "All" view for non-admins and preserves admin behavior for '*'.
Share API: GET/POST/DELETE /sites/:siteId/containers/:id/collaborators to list, add, and remove collaborators; share/unshare endpoints validate users and return the updated collaborator list.
Creation: Container create accepts additionalOwners array; creator-specified shares validated and stored in the same transaction.
Deletion: container deletion removes collaborator grants explicitly and enforces owner/admin-only deletion.
Client: add queries.shareContainer/unshareContainer, Container.collaborators type, and React components (CollaboratorChips, AddCollaboratorField, CollaboratorsManager); integrate into ContainerFormPage (create/edit) and ContainersListPage (share modal and row action).

This change enables collaborative workflows while enforcing owner/admin management and consistent UI feedback.

Introduce container sharing/collaborators so containers can be shared with additional users. Details: - Backend: add ContainerCollaborator model and migration with unique (containerId, username) constraint and CASCADE deletion. - DB/model changes: Container.hasMany(ContainerCollaborator) association and eager-load collaborators in container queries/serializer. - Permissions: implement userCanAccess / userCanManage helpers and loadContainerForSession to authorize view vs manage actions. - Listing/filtering: applyOwnershipFilter folds shared containers into the "All" view for non-admins and preserves admin behavior for '*'. - Share API: GET/POST/DELETE /sites/:siteId/containers/:id/collaborators to list, add, and remove collaborators; share/unshare endpoints validate users and return the updated collaborator list. - Creation: Container create accepts additionalOwners array; creator-specified shares validated and stored in the same transaction. - Deletion: container deletion removes collaborator grants explicitly and enforces owner/admin-only deletion. - Client: add queries.shareContainer/unshareContainer, Container.collaborators type, and React components (CollaboratorChips, AddCollaboratorField, CollaboratorsManager); integrate into ContainerFormPage (create/edit) and ContainersListPage (share modal and row action). This change enables collaborative workflows while enforcing owner/admin management and consistent UI feedback.

Enhance the Logs, Share, Edit and Delete action buttons in ContainersListPage.tsx with transition and hover color/background classes (including dark mode variants) to improve hover feedback and visual consistency. The changes add Tailwind utility className props to each Button without altering behavior.

cmyers-mieweb · 2026-07-01T21:11:23Z

Modifying "all" container toggle with a more clear and defined filter system to resolve #390

Also additional bug fixes to resolve
#389
#391

cmyers-mieweb · 2026-07-01T21:12:49Z

Also added color over hovered buttons for better UI

Introduce a reusable ContainerFilters component (with a MultiSelect/popover) and wire it into ContainersListPage so filter state lives in the URL (user,status,template,q). ContainersListPage now parses comma-separated user lists (and wildcard '*'), computes user/status/template option lists, performs client-side refinement by status/template/hostname, and updates UI/empty-state logic (removes the old Mine/All toggle). On the API side applyOwnershipFilter was refactored to accept a parsed user list, support wildcard semantics, let admins query arbitrary lists, and restrict non-admins to their own+shared containers. Added helper functions parseUserFilter and ownVisibleClauses to centralize parsing and visible-clause construction.

runleveldev

Regarding my large number of comments in create-a-container/routers/api/v1/containers.js:

runleveldev · 2026-07-02T14:03:45Z

+        label: STATUS_LABELS[s],
+      })),
+    [],
+  );


This is probably fine, but we should probably derive this from the actual statuses from the API to keep the dropdown clean.

runleveldev · 2026-07-02T14:07:34Z

+ * A compact checkbox-popover multiselect. Selected values are controlled by the
+ * parent so filter state stays in the URL. Closes on outside click.
+ */
+function MultiSelect({


@mieweb/ui already has a MutliSelect https://ui.mieweb.org/?path=/story/components-forms-inputs-dropdown--multi-select&globals=theme:dark

This has a "select all" option which I would prefer to the custom "Everone" option we have.

runleveldev

This is happening when loading the containers list page. If I get my way I really want the behavior of /api/v1/sites/:id/containers to be equivelent to the ?user=* form (making user=* redundant and obselete and hopefully removing the need for the * special handling in the backend).

Move container UI bits into reusable components and add dedicated components (HttpLinks, Meta, NodeLink, RowActions, SshLinks, StatusBadge, shared helpers). Normalize sharing terminology from "additionalOwners" to "collaborators" across client types, forms and OpenAPI, and wire the UI to the new components (including ServiceBadge and Dropdown multiSelect). Change list filtering to send user filters as an array and update client queries/types accordingly. Update server-side router/model logic: loadSite signature, authorize via Container.canView/canEdit, apply ownership filter using an IN (SELECT …) subquery for shared visibility, and adjust container loading/authorization to use the new semantics. Minor UI/behavior tweaks to filter summaries and owner visibility in the list view.

runleveldev · 2026-07-02T17:26:33Z

+        <Link to={`/jobs/${c.creationJobId}`}>
+          <Button
+            variant="ghost"
+            size="sm"
+            className="transition-colors hover:bg-slate-100 hover:text-slate-700 dark:hover:bg-slate-800 dark:hover:text-slate-200"
+          >
+            Logs
+          </Button>
+        </Link>


I think the builtin Button fade is being overridden by the Link wrapper. Can we fix that instead of coding our own transition?

I've fixed where it was being overridden, however I kept similar logic for the delete button so that it would have a distinct color when hovered over.

Client: Simplify RowActions buttons by removing custom transition CSS from several ghost buttons and slightly adjust the Delete button's hover classes (keep built-in ghost transition and add a comment). Server: Consolidate and harden container-list visibility logic by folding the previous ownership-filter helpers into `buildContainerListWhere(query, nodeIds, session)`. The function now documents behavior, accepts `session`, understands `query.user` (array) and enforces admin vs non-admin visibility rules (admins may narrow by owner; non-admins are limited to their own and shared containers and cannot widen visibility). The shared set is implemented as an `IN (SELECT ...)` subquery built with the dialect query generator. Updated router to default `req.query.user` and call the new `buildContainerListWhere` with `req.session`. Overall: makes the list endpoint the single arbiter of what containers a caller may see and prevents non-admins from widening visibility via the `user` filter.

runleveldev

Requesting changes on two connected themes, both aimed at shrinking this PR and enforcing authorization in one place:

Adopt your own helper. loadContainerForSession (added by this PR) is re-implemented inline in GET/PUT/DELETE /:id. Converting those three routes to call it trims ~40 lines, keeps the access rules in a single spot, and fixes the missing id validation in PUT/DELETE (id: NaN currently reaches the DB).
Move view-authorization into the query. The container-loading query should return a container if and only if the caller may at least view it — no loading first and checking after (404 when invisible). Keep the canEdit check app-level so a viewer attempting an owner-only action gets a 403 instead of 404; that leaks nothing since they can already see the container. Details in the comment on loadContainerForSession, which consolidates my three older threads on this (now resolved as superseded).

Out of scope for this PR: the near-identical service-creation blocks in POST and PUT predate this branch — please leave that for a follow-up rather than growing this diff.

Disclosure: this review was AI-assisted (OpenCode). My prompts, in brief: map which routes in containers.js call which helper functions and identify routes re-implementing helper logic; resolve my earlier review threads that the newer commits had addressed or that this review supersedes; and draft the line-anchored comments here — the adopt-the-helper suggestions and the query-level-visibility + 403-on-edit design. I reviewed the findings and comments before submitting.

runleveldev · 2026-07-06T14:30:33Z

    const c = await Container.findOne({
-      where: { id: containerId, username: req.session.user },
+      where: { id: containerId },
      include: CONTAINER_INCLUDE,
    });
-    if (!c || !c.node || c.node.siteId !== site.id) {
+    if (
+      !c ||
+      !c.node ||
+      c.node.siteId !== site.id ||
+      !(req.session.isAdmin || c.canView(req.session.user))
+    ) {
      throw new ApiError(404, 'not_found', 'Container not found');
    }


This fetch+auth block (together with the loadSite call and id guard above) re-implements loadContainerForSession, which this PR adds but only uses for the collaborator routes. It can collapse to:

const { site, container: c } = await loadContainerForSession(req, { include: CONTAINER_INCLUDE });

CONTAINER_INCLUDE already eager-loads node + collaborators, which is everything the helper's checks and canView need.

runleveldev · 2026-07-06T14:30:44Z

+    const site = await loadSite(req.params.siteId);
    const container = await Container.findOne({
-      where: { id: parseInt(req.params.id, 10), username: req.session.user },
-      include: [{ model: Node, as: 'node', where: { siteId: site.id } }],
+      where: { id: parseInt(req.params.id, 10) },
+      include: [
+        { model: Node, as: 'node', where: { siteId: site.id } },
+        { association: 'collaborators' },
+      ],
    });
-    if (!container) throw new ApiError(404, 'not_found', 'Container not found');
+    if (!container || !(req.session.isAdmin || container.canView(req.session.user))) {
+      throw new ApiError(404, 'not_found', 'Container not found');
+    }


Same duplication here — this is exactly loadContainerForSession's default behavior ([node, collaborators] include, canView rule):

const { site, container } = await loadContainerForSession(req);

Bonus: the helper validates the id, which this route doesn't — PUT .../containers/abc currently reaches the DB as id: NaN.

runleveldev · 2026-07-06T14:30:54Z

+    // Deleting is owner/admin only; collaborators may use but not destroy a
+    // shared container.
+    if (!(req.session.isAdmin || container.canEdit(req.session.user))) {
+      throw new ApiError(404, 'not_found', 'Container not found');
+    }


Third copy — here it's the helper with requireManage: true (same canEdit rule as this check):

const { site, container } = await loadContainerForSession(req, { requireManage: true, include: [ { association: 'node' }, { association: 'collaborators' }, /* existing services -> httpService -> externalDomain graph */ ], });

Replaces the fetch and both 404 checks, and adds the id validation this route is also missing.

runleveldev · 2026-07-06T14:37:25Z

+  const container = await Container.findByPk(id, {
+    include: include || [{ association: 'node' }, { association: 'collaborators' }],
+  });
+  if (!container || !container.node || container.node.siteId !== site.id) {
+    throw new ApiError(404, 'not_found', 'Container not found');
+  }
+  const authorized =
+    req.session.isAdmin ||
+    (requireManage
+      ? container.canEdit(req.session.user)
+      : container.canView(req.session.user));
+  if (!authorized) throw new ApiError(404, 'not_found', 'Container not found');


Consolidating my three resolved "auth should live in the query" threads into one actionable comment, with a refinement:

View access belongs in this query. If the caller can't at least view the container, the row should never come back — the query itself enforces visibility and we 404 without loading anything:

const where = { id }; if (!req.session.isAdmin) { // owner OR shared-with-me — the same visibility rule buildContainerListWhere builds where[Sequelize.Op.or] = visibleToClauses(req.session.user); } const container = await Container.findOne({ where, include: include || [...] }); if (!container || !container.node || container.node.siteId !== site.id) { throw new ApiError(404, 'not_found', 'Container not found'); } if (requireManage && !(req.session.isAdmin || container.canEdit(req.session.user))) { throw new ApiError(403, 'forbidden', 'Only the owner may manage this container'); }

Keep the canEdit check app-level, but make it 403, not 404. A caller who reached this point can already view the container, so a 403 leaks nothing new — it just tells a collaborator why they can't delete/share. (Update the docstring above, which currently promises 404-on-everything.)

Two notes:

I know I asked you to fold ownVisibleClauses into buildContainerListWhere — now that there's a second consumer, pull just the clause-building fragment (no query, it only returns the [{ username }, { id: IN (...) }] array) back out into a small shared function so the owner-or-shared rule lives in exactly one place.

This lands fully once GET/PUT/DELETE /:id adopt this helper, per my other comments in this review.

Implemented as suggested: visibleToClauses is the shared owner-or-shared fragment, view access is enforced in the loader's query, and the app-level canEdit check returns 403 with the docstring updated.

cmyers-mieweb · 2026-07-06T16:15:04Z

I removed the model-level canView helper and introduced visibleToClauses to centralize the owner-or-shared visibility rule as Sequelize WHERE clauses. Now we use this helper in list and single-container loaders so view access is enforced in DB queries (non-admins only load containers they own or that are shared with them). Updated loadContainerForSession to support requireManage which returns 403 for manage-only operations (owner/admin), and adjust routes (GET/PUT/DELETE and collaborators endpoints) to use the new loader and stricter checks. Also updated OpenAPI docs to document 403 responses for delete/share/unshare. Misc: simplify query-building by replacing inline subquery logic with visibleToClauses and tidy eager-loads.

Remove the model-level canView helper and introduce visibleToClauses to centralize the owner-or-shared visibility rule as Sequelize WHERE clauses. Use this helper in list and single-container loaders so view access is enforced in DB queries (non-admins only load containers they own or that are shared with them). Update loadContainerForSession to support requireManage which returns 403 for manage-only operations (owner/admin), and adjust routes (GET/PUT/DELETE and collaborators endpoints) to use the new loader and stricter checks. Also update OpenAPI docs to document 403 responses for delete/share/unshare. Misc: simplify query-building by replacing inline subquery logic with visibleToClauses and tidy eager-loads.

Avoid mutating req.query directly because Express 5 exposes req.query as a getter that re-parses on each access. Capture req.query into a local const, default query.user to an empty array, and pass that captured object to buildContainerListWhere. Also update surrounding comments to explain the behavior and rationale for the change.

Add an owner prop to ResourcesSection and use it to detect whether the current session user is the container owner. If the viewer is not the owner, effective resource queries are disabled, the edit/request controls are disabled, and a notice is shown explaining that only owners can make resource requests. Also pass the container owner from ContainerFormPage when editing an existing container. This prevents collaborators from creating resource requests that must be keyed to the owner's account.

Prevent collaborators from editing shared containers across the stack. UI: ContainerFormPage now uses session info to detect read-only collaborators, shows an informational alert, disables the form when appropriate, and hides the save/cancel footer. Server/API: containers PUT route now requires manage permission (loadContainerForSession(..., { requireManage: true })) and the OpenAPI spec adds a 403 response for forbidden edits. Model docs updated to clarify collaborators have a read-only view.

Add a foreign-key from ContainerCollaborators.username -> Users.uid (with ON DELETE/UPDATE CASCADE) in model and migration so the DB enforces collaborator existence and user deletions cascade. Update API behavior: change loadContainerForSession signature to accept siteId, containerId and session, tighten container id validation, and centralize view/manage checks. Replace per-collaborator existence checks with bulk insert (bulkCreate / ignoreDuplicates) and map FK failures to 404 user_not_found; make POST /collaborators idempotent (sharing an existing collaborator is a no-op and returns the current list). Remove explicit collaborator row cleanup on container delete (rely on cascade). Also introduce normalizeShareUsername and isUnknownUserError helpers and update all router call sites accordingly. Client change: only show statuses present in the loaded rows in the status filter dropdown so it stays concise. Minor OpenAPI cleanup to document idempotency and adjust error descriptions.

cmyers-mieweb requested a review from runleveldev June 30, 2026 16:15

runleveldev requested changes Jul 2, 2026

View reviewed changes

runleveldev requested changes Jul 6, 2026

View reviewed changes

cmyers-mieweb added 3 commits July 6, 2026 09:15

cmyers-mieweb marked this pull request as ready for review July 6, 2026 17:52

cmyers-mieweb added 2 commits July 6, 2026 11:00

Uh oh!

Conversation

cmyers-mieweb commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmyers-mieweb commented Jul 1, 2026

Uh oh!

cmyers-mieweb commented Jul 1, 2026

Uh oh!

runleveldev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

runleveldev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

runleveldev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmyers-mieweb commented Jul 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cmyers-mieweb commented Jun 30, 2026 •

edited

Loading