Sandboxes: scope visibility, tighten merge gate, drop superuser bypass

CHANGELOG.md

@@ -105,6 +105,14 @@ and this project adheres to
   action as new sandbox creation, and the Restore button in the sandbox list is
   disabled (with the limiter's tooltip) when the active sandbox count is already
   at the limit.
+- Sandboxes no longer appear in the sandboxes list, the project picker, or via
+  the sandbox URL unless the user has access
+  [#4762](https://github.com/OpenFn/lightning/issues/4762)
+- The Merge button on a sandbox now requires admin or owner on the source
+  sandbox (or admin/owner on the root project)
+  [#4762](https://github.com/OpenFn/lightning/issues/4762)
+- Sandbox policies no longer treat `User.role: :superuser` as a project-access
+  bypass [#4762](https://github.com/OpenFn/lightning/issues/4762)
 - `Cmd/Ctrl+Enter` now runs the workflow directly; `Cmd/Ctrl+Shift+Enter` opens
   "run with custom input". When a retryable run is loaded, the primary action
   switches to retry. [#4736](https://github.com/OpenFn/lightning/issues/4736)

assets/js/picker/Picker.tsx

@@ -33,9 +33,16 @@ export interface PickerItem {
   /**
    * Optional hero icon class for the row (e.g. `hero-credit-card`).
    * When absent, falls back to the project-style default:
-   * `hero-folder` at depth 0 and `hero-beaker` for nested items.
+   * `hero-folder` for root projects and `hero-beaker` for sandboxes.
    */
   icon?: string;
+  /**
+   * Whether the underlying project is structurally a sandbox (has a
+   * real `parent_id` in the database). Drives the default icon
+   * independently of the row's display depth, so a sandbox surfaced as
+   * a user's access root still renders with the sandbox icon.
+   */
+  isSandbox?: boolean;
 }
 
 interface PickerProps {
@@ -203,7 +210,15 @@ export function Picker(props: PickerProps) {
   }, [openPicker, openEvent]);
 
   const go = (href: string, sameSection = false) => {
-    window.location.href = href + (sameSection ? window.location.hash : '');
+    const fullHref = href + (sameSection ? window.location.hash : '');
+    const a = document.createElement('a');
+    a.href = fullHref;
+    a.setAttribute('data-phx-link', 'redirect');
+    a.setAttribute('data-phx-link-state', 'push');
+    a.style.display = 'none';
+    document.body.appendChild(a);
+    a.click();
+    document.body.removeChild(a);
   };
 
   const handleInputKeyDown = useCallback(
@@ -316,7 +331,7 @@ export function Picker(props: PickerProps) {
               const isNested = item.depth > 0;
               const indentPx = item.depth * 10;
               const itemIcon =
-                item.icon ?? (isNested ? 'hero-beaker' : 'hero-folder');
+                item.icon ?? (item.isSandbox ? 'hero-beaker' : 'hero-folder');
 
               return (
                 <li

assets/js/picker/PickerButton.tsx

@@ -36,7 +36,7 @@ export function PickerButton(props: PickerButtonProps) {
   const openEvent = props['data-open-event'];
   const isSandbox = props['data-is-sandbox'] === 'true';
   const accentIcon = props['data-accent-icon'] || icon;
-  const color = props['data-color'] || null;
+  const color = props['data-color'] || 'var(--color-primary-600)';
 
   const handleClick = (e: React.MouseEvent) => {
     e.preventDefault();
@@ -56,7 +56,7 @@ export function PickerButton(props: PickerButtonProps) {
           'text-gray-700 bg-white border border-gray-300 hover:bg-gray-50 hover:border-gray-400',
         isSandbox && 'text-white border border-transparent hover:opacity-90'
       )}
-      style={isSandbox && color ? { backgroundColor: color } : undefined}
+      style={isSandbox ? { backgroundColor: color } : undefined}
     >
       <span
         className={cn(

assets/test/picker/Picker.test.tsx

@@ -165,31 +165,33 @@ describe('Picker search', () => {
 });
 
 describe('Picker navigation', () => {
-  let hrefSetter: ReturnType<typeof vi.fn>;
-  let originalLocation: Location;
+  let clickedAnchors: HTMLAnchorElement[];
+  let clickSpy: ReturnType<typeof vi.spyOn>;
 
   beforeEach(() => {
-    hrefSetter = vi.fn();
-    originalLocation = window.location;
-    delete (window as unknown as { location?: Location }).location;
-    (window as unknown as { location: unknown }).location = {
-      pathname: '/',
-      search: '',
-      hash: '',
-      get href() {
-        return '';
-      },
-      set href(value: string) {
-        hrefSetter(value);
-      },
-    };
+    clickedAnchors = [];
+    clickSpy = vi
+      .spyOn(HTMLAnchorElement.prototype, 'click')
+      .mockImplementation(function (this: HTMLAnchorElement) {
+        clickedAnchors.push(this);
+      });
   });
 
   afterEach(() => {
-    (window as unknown as { location: Location }).location = originalLocation;
+    clickSpy.mockRestore();
     closePicker();
   });
 
+  function lastNavigation() {
+    expect(clickedAnchors).toHaveLength(1);
+    const a = clickedAnchors[0];
+    return {
+      href: a.getAttribute('href'),
+      phxLink: a.getAttribute('data-phx-link'),
+      phxLinkState: a.getAttribute('data-phx-link-state'),
+    };
+  }
+
   test('navigates to the server-provided href on click', () => {
     mountPicker([item('target', 'target', 0, { href: '/projects/target/w' })]);
     openPicker();
@@ -201,7 +203,11 @@ describe('Picker navigation', () => {
       targetOption.click();
     });
 
-    expect(hrefSetter).toHaveBeenCalledWith('/projects/target/w');
+    expect(lastNavigation()).toEqual({
+      href: '/projects/target/w',
+      phxLink: 'redirect',
+      phxLinkState: 'push',
+    });
   });
 
   test('uses data-view-all-href for the view-all row', () => {
@@ -221,12 +227,11 @@ describe('Picker navigation', () => {
       viewAll.click();
     });
 
-    expect(hrefSetter).toHaveBeenCalledWith('/somewhere');
+    expect(lastNavigation().href).toBe('/somewhere');
   });
 
   test('preserves the current hash when item has sameSection=true', () => {
-    (window as unknown as { location: { hash: string } }).location.hash =
-      '#credentials';
+    window.location.hash = '#credentials';
 
     mountPicker([
       item('target', 'target', 0, {
@@ -243,14 +248,11 @@ describe('Picker navigation', () => {
       option.click();
     });
 
-    expect(hrefSetter).toHaveBeenCalledWith(
-      '/projects/target/settings#credentials'
-    );
+    expect(lastNavigation().href).toBe('/projects/target/settings#credentials');
   });
 
   test('drops the hash when item has sameSection=false', () => {
-    (window as unknown as { location: { hash: string } }).location.hash =
-      '#some-anchor';
+    window.location.hash = '#some-anchor';
 
     mountPicker([
       item('target', 'target', 0, {
@@ -267,12 +269,11 @@ describe('Picker navigation', () => {
       option.click();
     });
 
-    expect(hrefSetter).toHaveBeenCalledWith('/projects/target/history');
+    expect(lastNavigation().href).toBe('/projects/target/history');
   });
 
   test('does not preserve hash for the view-all row', () => {
-    (window as unknown as { location: { hash: string } }).location.hash =
-      '#credentials';
+    window.location.hash = '#credentials';
 
     render(
       <Picker
@@ -290,7 +291,7 @@ describe('Picker navigation', () => {
       viewAll.click();
     });
 
-    expect(hrefSetter).toHaveBeenCalledWith('/projects');
+    expect(lastNavigation().href).toBe('/projects');
   });
 });
 

lib/lightning/policies/sandboxes.ex

@@ -2,12 +2,23 @@ defmodule Lightning.Policies.Sandboxes do
   @moduledoc """
   The Bodyguard Policy module for sandbox project operations.
 
-  Sandboxes have different authorization rules than regular projects:
+  Sandbox authorization mirrors regular projects: access is decided by the
+  acting user's role on the project they're acting on (or the workspace
+  root, where the cascade applies). `User.role` (`:user` / `:superuser`)
+  is a user-type for global user-management screens; it is not a
+  project-access bypass and has no effect on sandbox policy decisions, in
+  line with `Lightning.Policies.ProjectUsers`.
+
   - Sandbox owners/admins can manage their own sandboxes
   - Root project owners/admins can manage any sandbox in their workspace
-  - Editors (and above) on the root project can merge sandboxes
   - Editors (and above) on the parent project can provision sandboxes
-  - Superusers can manage any sandbox anywhere
+
+  Destructive actions on a sandbox (delete, update, merge) are scoped to
+  admin/owner on the sandbox itself (or the root cascade above). This
+  matches the rest of Lightning, where destructive actions are admin/owner
+  scoped, and it keeps the merge button on the sandboxes list aligned with
+  the cleanup step that runs after merge submission (which calls
+  `:delete_sandbox` and so requires admin/owner on the source).
   """
   @behaviour Bodyguard.Policy
 
@@ -22,24 +33,27 @@ defmodule Lightning.Policies.Sandboxes do
           | :merge_sandbox
 
   @doc """
-  Authorize sandbox operations based on user role and project hierarchy.
+  Authorize sandbox operations based on the user's role on the project
+  involved.
 
   ## Authorization Rules
 
   ### `:delete_sandbox` and `:update_sandbox`
-  User can perform these actions if they are:
-  - Superuser (can manage any sandbox)
+  User must be one of:
   - Owner/admin of the sandbox itself
   - Owner/admin of the root project (workspace)
 
   ### `:provision_sandbox`
-  User can create sandboxes if they are:
-  - Editor/admin/owner of the parent project they're creating the sandbox under
+  User must be editor/admin/owner of the parent project they're creating
+  the sandbox under.
 
   ### `:merge_sandbox`
-  User can merge a sandbox into a target project if they are:
-  - Editor/admin/owner on the target project
-  - Superuser
+  This check authorises the **target side** of a merge: the user must be
+  editor/admin/owner on the target project (the project being merged
+  *into*). The merge flow also requires admin/owner on the **source
+  sandbox** itself, enforced by `check_manage_permissions/3` (button
+  gate) and by the post-merge cleanup, which calls `:delete_sandbox`
+  to retire the source and so requires admin/owner there.
 
   ## Parameters
   - `action` - The action being attempted
@@ -50,84 +64,60 @@ defmodule Lightning.Policies.Sandboxes do
   @spec authorize(actions(), User.t(), Project.t()) :: boolean
 
   def authorize(:provision_sandbox, %User{} = user, %Project{} = parent_project) do
-    case Projects.get_project_user_role(user, parent_project) do
-      role when role in [:owner, :admin, :editor] -> true
-      _ -> user.role == :superuser
-    end
+    Projects.get_project_user_role(user, parent_project) in [
+      :owner,
+      :admin,
+      :editor
+    ]
   end
 
   def authorize(:merge_sandbox, %User{} = user, %Project{} = target_project) do
-    case Projects.get_project_user_role(user, target_project) do
-      role when role in [:owner, :admin, :editor] -> true
-      _ -> user.role == :superuser
-    end
+    Projects.get_project_user_role(user, target_project) in [
+      :owner,
+      :admin,
+      :editor
+    ]
   end
 
   def authorize(action, %User{} = user, %Project{} = sandbox)
       when action in [:delete_sandbox, :update_sandbox] do
-    cond do
-      user.role == :superuser ->
-        true
-
-      Projects.get_project_user_role(user, sandbox) in [:owner, :admin] ->
-        true
-
-      has_root_project_permission?(sandbox, user) ->
-        true
-
-      true ->
-        false
-    end
+    Projects.get_project_user_role(user, sandbox) in [:owner, :admin] or
+      has_root_project_permission?(sandbox, user)
   end
 
   def authorize(_action, _user, _project), do: false
 
   @doc """
-  Bulk permission check for multiple sandboxes to avoid N+1 queries.
+  Bulk manage check for multiple sandboxes, avoiding N+1 queries.
 
-  Returns a map: sandbox_id => %{update: boolean, delete: boolean, merge: boolean}
+  Returns a map `sandbox_id => boolean()` where `true` means the user can
+  perform any of the destructive sandbox actions (update, delete, merge)
+  on that sandbox. The boolean is `true` when the user is an owner/admin
+  on the sandbox itself, or an owner/admin on the root project (cascade).
 
   Assumes `root_project.project_users` and each `sandbox.project_users`
   are preloaded (as ensured by `Projects.list_workspace_projects/2`).
   """
   @spec check_manage_permissions([Project.t()], User.t(), Project.t()) ::
-          %{
-            binary() => %{update: boolean(), delete: boolean(), merge: boolean()}
-          }
+          %{binary() => boolean()}
   def check_manage_permissions(sandboxes, %User{} = user, root_project) do
-    is_superuser = user.role == :superuser
-
     is_root_owner_or_admin =
       Enum.any?(
         root_project.project_users,
         &(&1.user_id == user.id and &1.role in [:owner, :admin])
       )
 
-    is_root_editor_plus =
-      is_root_owner_or_admin or
-        Enum.any?(
-          root_project.project_users,
-          &(&1.user_id == user.id and &1.role == :editor)
-        )
-
-    has_full_privileges = is_superuser or is_root_owner_or_admin
-
-    if has_full_privileges do
-      Map.new(sandboxes, &{&1.id, %{update: true, delete: true, merge: true}})
+    if is_root_owner_or_admin do
+      Map.new(sandboxes, &{&1.id, true})
     else
       Map.new(sandboxes, fn sandbox ->
-        is_owner_or_admin_here? =
+        can_manage? =
           Enum.any?(
             sandbox.project_users,
             &(&1.user_id == user.id and &1.role in [:owner, :admin])
           )
 
-        {sandbox.id,
-         %{
-           update: is_owner_or_admin_here?,
-           delete: is_owner_or_admin_here?,
-           merge: is_root_editor_plus
-         }}
+        {sandbox.id, can_manage?}
       end)
     end
   end