Sandboxes: scope visibility, tighten merge gate, drop superuser bypass#4763
Merged
elias-ba merged 82 commits intoMay 18, 2026
Conversation
Security Review ✅
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4763 +/- ##
=======================================
+ Coverage 90.1% 90.2% +0.1%
=======================================
Files 450 441 -9
Lines 22593 22443 -150
=======================================
- Hits 20356 20247 -109
+ Misses 2237 2196 -41 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
brandonjackson
approved these changes
May 15, 2026
elias-ba
force-pushed
the
fix/hide-non-accessible-sandboxes-4762
branch
3 times, most recently
from
fab9d72 to
350a2ba
Compare
Mirrors how the projects list filters out projects the user is not a member of. Superusers and root-project owners/admins keep full visibility over the workspace; everyone else only sees sandboxes they have a project_users row on. Closes #4762
get_project_tree_for_user/1 used to return the full subtree under every root the user is a member of, so the picker dropdown leaked sandboxes the user had no project_users row on. The function now filters descendants with the same rule used by the sandboxes list: superusers and root owners/admins (and support users on support-access roots) keep cascading access; everyone else only sees descendants they hold a project_users row on. Closes the picker half of #4762.
The picker filter and the sandboxes-list filter encoded the same rule twice with a subtle divergence: only the picker cascaded for support users on a support-access root, so a support user opening the sandboxes page saw an empty list while the picker showed every descendant. Sandboxes.visible_sandboxes/3 now owns the rule (including the support-user branch). Projects.get_project_tree_for_user/1 groups descendants by their root and delegates to the policy per root, so both surfaces stay in lockstep. Adds a picker render test asserting that hidden sandboxes do not appear in the dropdown items.
Use Floki to extract the data-items attribute from the rendered picker, decode it, and assert UUID membership in the id array. Previously the test matched on the bare UUID substring in the full HTML, which would have also passed if the id appeared inside an unrelated tag.
The policy modules in lib/lightning/policies/ are reserved for Bodyguard-style authorize/3 callbacks; context modules route to them through Lightning.Policies.Permissions.can?/4. Reaching directly into a specific policy module from a context (Lightning.Projects calling Lightning.Policies.Sandboxes) had no precedent and created a soft cycle because the Sandboxes policy already depends on Projects for root_of/1 and get_project_user_role/2. visible_sandboxes/3 isn't an action authorization decision; it filters a list of Project rows by hierarchy and membership. That belongs alongside get_projects_for_user, projects_for_user_query, and get_project_tree_for_user in Lightning.Projects. Tests move with it.
The deletion cascade in Sandboxes.do_schedule_sandbox_deletion/1 sets scheduled_deletion on a sandbox AND every descendant in one transaction, so the supported API can never produce a non-scheduled descendant whose parent chain is broken by a scheduled (and thus filtered) intermediate. Combined with descendant_ids/1 only returning descendants of roots the user already has, every chain is guaranteed to terminate at a root in root_lookup. Replace Map.fetch/Map.get + fallback branches with Map.fetch!. If a future change breaks the invariant we get a loud KeyError instead of silently dropping sandboxes from the picker.
* Separate cascading_visibility?/2 and member?/2 from visible_sandboxes/3. The cascading rule (superuser, support user on support-access root, root owner/admin) is now a named predicate; the function reads as "give back the whole list when access cascades, otherwise keep only members." * Replace Enum.uniq_by with Enum.sort_by(& &1.name) in the support-user variant of roots_for_user_tree/1. SQL UNION already dedupes; the remaining concern was unstable ordering relative to the non-support variant, which the sort fixes. * Add a multi-root test that exercises per-root grouping with one cascading workspace and one membership-only workspace. * Use Lightning.Factories.insert consistently in the picker test instead of mixing three fixture conventions.
Three follow-ups from a deeper audit of visibility surfaces: * get_project_tree_for_user/1 now uses a scoped recursive CTE that filters is_nil(scheduled_deletion) in both the initial and recursion arms (active_descendants_query/1). The walk prunes at a scheduled ancestor, so the visibility filter can never receive a non-scheduled descendant whose parent chain is broken by a scheduled intermediate. The invariant the deletion cascade enforces at the write path is now also enforced at the read path, so Map.fetch! in filter_descendants_for_user/3 holds by construction, not by trust. * get_merge_target_options/2 now rejects scheduled-for-deletion sandboxes. They could previously appear as merge targets in the modal. * list_sandboxes/1's docstring claimed list_workspace_projects/2 filters scheduled rows out. It doesn't; the sandboxes list shows scheduled rows in a "Recently Deleted" section and the picker filters them via get_project_tree_for_user/1. Doc updated to reflect reality. descendant_ids/1 and descendants_query/1 stay unchanged - they're still the right tool for credential uniqueness across the tree and for the hard-delete cascade in Sandboxes.subtree_ids/1. Adds two regression tests: one for the scheduled-ancestor pruning in the tree fetch, one for scheduled sandboxes being excluded from merge target options.
ancestor_root_id/3 (formerly root_id_of/2) now stops at any project in root_lookup rather than only at parent_id: nil. The function's contract becomes "walk up until you find a root," which is what it always meant. Removes the silent coupling between filter_descendants_for_user/3 and the data-fetch invariant that roots are always top-level.
Two follow-ups from an external review that surfaced gaps in the "hide non-accessible sandboxes" rule. Sandbox-only and orphan-descendant cases for the picker ------------------------------------------------------- Before, get_project_tree_for_user/1 returned only roots the user was a member of (parent_id IS NULL) and walked their descendants. That broke two real scenarios: * A user added directly to a deep sandbox without membership on its ancestors got an empty picker, because their sandbox was not a root. * A user on root R plus on grandchild B but not on intermediate A saw only R in the picker; the tree walk could not descend through hidden A to reach B. roots_for_user_tree/1 now computes *access roots* via candidate_roots_query/1 and shadowing through has_candidate_ancestor?/2: every project the user holds a project_users row on (any depth), plus support-access roots for support users, minus any candidate that has a member ancestor (so a root member shadows their own descendants). get_project_tree_for_user/1 then reshapes the returned parent_id field of every project for picker-friendly rendering. Access roots get parent_id: nil; visible descendants get parent_id rewritten via reparent_for_picker/3 to their nearest visible ancestor (intermediate ancestors the user cannot see are elided). The picker walk reverts to the simple "start at parent_id: nil" form because the tree is now correctly shaped at the data layer. Merge modal surfaces hidden descendants --------------------------------------- The merge modal listed descendants of the source sandbox by walking the user-visibility-filtered workspace_projects. A user who can merge S but cannot see all of S's descendants would see an incomplete list, while the server-side merge still affects the full subtree. Adds Projects.list_descendants/1 (a workspace-root-agnostic query for "every descendant of this project") and switches the merge modal to it, so the user is shown what their action will actually affect. Removes the now-unused get_all_descendants/2 and local descendant_of?/3 helpers from SandboxLive.Index. Tests ----- * Reparenting onto the nearest visible ancestor (projects unit and picker render). * Sandbox-only access root (projects unit and picker render). * Prefer-topmost-membership when the user holds rows on both root and descendant. * Merge modal lists descendants the current viewer cannot otherwise see.
The previous has_candidate_ancestor?/2 walked parent chains with one Repo.get/2 per hop, producing N_candidates * max_depth round-trips every time the picker rendered. Replace with a single depth-bounded recursive CTE (ancestor_ids_by_starting_id/1) that returns every ancestor id paired with its starting candidate, grouped in Elixir and intersected against the candidate set. The CTE selects the starting and ancestor ids cast to Ecto.UUID so the returned map keys are the string form Project.id uses elsewhere; raw binary keys would silently miss every Map.get/Map.has_key? lookup against candidate_id_set and let shadowed sandboxes leak through as duplicate access roots. Strengthens the existing "topmost membership wins" test to assert the reshaped parent_id chain (root -> middle -> grandchild) and the absence of duplicates, and adds a support-user shadow test so the support_user branch of candidate_roots_query/1 is exercised end-to-end. Tightens the visible_sandboxes/3 docstring to call out both preload guarantors (list_workspace_projects/2 and get_project_tree_for_user/1).
mix format --check-formatted was failing on CI. Local check runs were reading the wrong exit code (pipe to tail dropped mix's exit status), so the formatter drift never surfaced.
The picker runs on every page render via get_project_tree_for_user/1, and the previous preload loaded every membership row on every project the user could see, just so cascading_visibility?/2 and member?/2 could check for one row. Scope the preload to the current user so each project comes back with at most one project_users row. cascading_visibility?/2 and member?/2 only ever look for a row matching the current user's id, so a scoped preload satisfies the contract on visible_sandboxes/3. The only two callers (LightningWeb.SandboxLive.Index via list_workspace_projects/2 and Projects.filter_descendants_for_user/3 via this function) both honor the contract for the current user.
Adds a regression test that constructs the "scheduled middle, active leaf" state directly via Repo.update_all (bypassing Sandboxes.do_schedule_sandbox_deletion/1's atomic subtree cascade) and asserts the picker still prunes the entire subtree. Documents the read-side guarantee independently of the write-side invariant.
visible_sandboxes/3 now sits on Lightning.Projects (a public,
broadly-imported context), so a future caller is one easy mistake away
from a Protocol.UndefinedError deep inside Enum.any?/2. Guard at the
function entry: if root_project.project_users or any
sandbox.project_users is %Ecto.Association.NotLoaded{}, raise an
ArgumentError that names the unloaded association and points at the
function docstring.
Existing internal callers
(LightningWeb.SandboxLive.Index.load_workspace_projects/1 and
Projects.filter_descendants_for_user/3) already honor the contract,
so this is purely a friendlier failure mode at the API boundary.
The top-level function now reads as the algorithm it implements: get roots, load descendants, filter for visibility, shape for picker. * load_active_descendants/2 owns the descendant fetch (scoped preload, ordering). * shape_for_picker/3 owns the access-root nil-ification and descendant reparenting. * roots_for_user_tree/1 names its shadow predicate (shadowed_by_candidate_ancestor?/3) and its loading stage (load_candidate_roots/1) instead of inlining both. No behavior change; existing tests cover every path.
active_descendants_query/1: rename `initial` and `recursion` to direct_children and next_level_down. The names now describe what each CTE arm produces rather than its role in the recursive walk. ancestor_ids_by_starting_id/1: split the bundled function into three named pieces: * the orchestrator now reads as a 5-line pipeline (to_list -> ancestor_pairs_query -> Repo.all -> group_ancestors_by_starting_id), * ancestor_pairs_query/1 owns the SQL with starting_rows / next_ancestor_up bindings, * group_ancestors_by_starting_id/1 owns the post-query grouping. No behavior change, no extra round-trips, no extra traversals.
list_descendants/1 and visible_sandboxes/3 named the specific callers
and described usage scenarios ("used by surfaces that need to
communicate...", "single source of truth across both the sandboxes
list and the global project picker", "Both call sites take care of
this: ..."). Caller details rot as the codebase evolves; the
function contracts stand on their own.
elias-ba
force-pushed
the
fix/hide-non-accessible-sandboxes-4762
branch
from
350a2ba to
be98f37
Compare
The function used to return %Project{} structs with parent_id rewritten
for picker display. That worked but exposed a foot-gun: a future caller
that pipes the result into a changeset, Repo.update, or any permissions
helper that reads parent_id would corrupt the persisted hierarchy.
Introduce a small ProjectTreeItem struct (id, name, color, parent_id)
and return that instead. The picker walk consumes the same fields, so
the rendering contract is unchanged; the type signature now states
plainly that the result is a tree-rendering shape, not a list of
persistable projects.
Collaborator
|
@elias-ba my instinct is that the behaviours here are all correct. I'd like to test a bit deeper but I think this is a good change. |
Collaborator
|
This will fix #4745 right? |
elias-ba
changed the title
Joe (review) flagged that the original PR bundled two concerns: a visibility bug (sandboxes appearing for users with no row on them) and four access-rule changes (delete/rename cascade, merge gate, parent-admin floor, superuser bypass). The visibility fix is uncontroversial and lands here; the access-rule discussion moves to #4768. Revert to main: - lib/lightning/policies/sandboxes.ex (restores root cascade and check_manage_permissions/3) - lib/lightning/projects/sandboxes.ex (restores parent_admin? helper) - lib/lightning_web/live/project_live/settings.ex (restores parent-admin floor in collaboration tab) - test/lightning/policies/sandbox_permissions_test.exs - test/lightning_web/live/project_live/sandbox_settings_test.exs Surgically restore old behavior: - lib/lightning/projects.ex: re-add the parent_admin? raise in delete_project_user! - lib/lightning_web/live/sandbox_live/index.ex: replace manage_authority/2 with check_manage_permissions/3 (cascade-aware, keyed on workspace_root); revert decorate_for_render to the {update, delete, merge} shape; restore the superuser bypass in get_merge_target_options - test/lightning/sandboxes_test.exs: restore the "superuser becomes sandbox owner" provisioning test - test/lightning_web/live/sandbox_live/index_test.exs: restore the "editor on root can see and use merge button" assertion Keep the visibility scope intact: visible_sandboxes filter, access_root breadcrumb truncation, restricted-parent placeholder, picker collapsing hidden intermediates, soft-navigation, and the merge confirmation modal counting descendants without naming them. CHANGELOG trimmed to describe only the visibility fix.
The placeholder added a stub to the Sandboxes page when the user's
access root has a parent they can't reach. Two reasons to drop it:
- The global project picker hides inaccessible ancestors silently;
surfacing them only on the Sandboxes page made the two surfaces
inconsistent.
- The information isn't actionable; the user can't request access
or navigate up. The breadcrumb already starts at their access
root, which is enough signal.
This trims a piece of new UI that wasn't strictly required to fix the
visibility bug.
The settings.html.heex was reverted to main as part of the scope reduction; the test assertions were still expecting the new wording introduced by the access-policy commits and were failing against the old copy. Revert the test to main's assertions so it matches.
Fix a partial-action bug: today an editor on the root project can click
Merge on a sandbox, the merge data sync runs, but the cleanup that
retires the source sandbox calls Sandboxes.schedule_sandbox_deletion/2
which requires admin/owner on the source. The merge ends up succeeding
for the data but the source stays in place.
This is a behaviour change, but bundled with the visibility fix because:
- It's a real bug (the button promises something the action can't
complete).
- It aligns with the rest of Lightning, where destructive actions are
admin/owner scoped.
- The fix is small and self-contained: one boolean in
check_manage_permissions/3.
Two tests in sandbox_permissions_test and one in sandbox_live/index_test
asserted the old "editor on root can merge" behaviour; updated to the
new rule. CHANGELOG and PR description updated.
Now that update, delete, and merge all share the same predicate
(admin/owner on the sandbox, with the cascade above), the per-key map
was storing three copies of the same boolean. Collapse the return to
%{sandbox_id => boolean()} and rename the local in decorate_for_render
to match.
Tests updated to assert on the flat boolean.
Aligns sandbox authorization with Lightning.Policies.ProjectUsers, which
governs regular projects and ignores User.role entirely when deciding
project access. Superuser is a type of user (used for global
user-management screens), not a project role. Treating it as a
project-access bypass was a one-off in the sandbox policy stack with no
equivalent on regular projects.
Updated:
- :provision_sandbox, :merge_sandbox, :delete_sandbox/:update_sandbox
in Lightning.Policies.Sandboxes (drop the user.role == :superuser
branches)
- check_manage_permissions/3 (drop is_superuser; has_full_privileges
collapses to is_root_owner_or_admin)
- get_merge_target_options/2 in SandboxLive.Index (drop the superuser
branch from the target filter)
- Sandboxes moduledoc and projects/sandboxes.ex authorization docs
- Five policy tests + the provisioning test in sandboxes_test (invert
the assertions: superuser without a project role gets no rights)
CHANGELOG and PR description updated.
Contributor
Author
Contributor
Author
Scoped the PR down. The four access-rule changes (cascade, parent-admin floor, merge gate, superuser bypass) were originally bundled together. After your review, only the visibility fix plus the two access changes that I'd call straight bug fixes survived: the merge button now matches the action it triggers, and |
elias-ba
changed the title
…n-accessible-sandboxes-4762
Joe (PR review) noted that the :merge_sandbox section only documented the target-side gate (editor+ on the target). The source-side gate (admin/owner on the sandbox itself) was implied but not surfaced. The merge flow has two gates: this authorize/3 clause checks the target, and check_manage_permissions/3 plus the post-merge :delete_sandbox cleanup check the source. Document both explicitly so the rule is visible from the docstring.
josephjclark
approved these changes
May 18, 2026
Collaborator
josephjclark
left a comment
josephjclark
left a comment
There was a problem hiding this comment.
Looks great ,diff is clean and changes look like proper bug fixes, with the behavioral questions factored out.
Thanks @elias-ba
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Problem
Today, having any role on a parent project automatically surfaces every sandbox underneath, in the Sandboxes page, the global project picker, and the breadcrumb. The actual entry policy is already direct-membership-only on
main, so users see sandboxes they can't actually open. The mismatch is confusing and leaks names of sandboxes a user has no business seeing.Fix
Sandboxes only show up for users with direct membership on that sandbox (or for support users when the sandbox's own support-access flag is on). Visibility now matches what the entry policy already enforces.
Two alignments bundled in
Both are pre-existing bugs/inconsistencies rather than new policy decisions, so I bundled them here:
main, an editor on the root could click Merge, but the cleanup that retires the source requires admin/owner, so the merge silently leaves the source in place. The button now matches what the action can complete.Lightning.Policies.ProjectUsersalready ignoresUser.rolefor regular projects, so this aligns sandboxes with the rest of Lightning. Superuser is a type of user, not a project role.The bigger access-rule decisions (parent-admin floor, root cascade) live in #4768 for separate discussion.
Closes #4762.
Closes #4745.
Validation steps
Setup
Seed the demo users. Either set
IS_RESETTABLE_DEMO=yesand runmix run priv/repo/demo.exs, or from IEx:Either gives you four users (all password
welcome12345): Amy Admin (demo@openfn.org, owner of DHIS2), Esther Editor, Vikram Viewer, and Sizwe Super (superuser).Sign in as Amy, open DHIS2's Sandboxes page (left sidebar), and click + New sandbox to provision one. To add or remove members on a project, open its Settings in the left sidebar and pick the Collaboration tab. Reset Esther's and Vikram's memberships between scenarios as needed.
1. A user without a sandbox membership cannot see the sandbox
Sign in as Amy and add Esther to DHIS2 as an Admin using DHIS2's Collaboration tab. Do not open the sandbox's Collaboration tab; Esther stays absent from the sandbox itself.
The sandbox is absent from the Sandboxes list, and the picker shows only DHIS2 with no nested entries. Esther's Admin role on DHIS2 does not surface the sandbox. Visiting the sandbox URL directly (
/projects/<sandbox_id>/w) redirects her away.2. Adding the user directly to the sandbox makes it visible
Still signed in as Amy, open the sandbox's Collaboration tab and add Esther as a Viewer.
The sandbox now appears in both the Sandboxes list and the picker. Visiting the sandbox URL directly now loads it.
3. Support-user access follows the sandbox's own support-access toggle
Sign in as Sizwe, open
/settings/users, edit a fresh user with noproject_usersrow anywhere, tick Support user, and save./projects/<sandbox_id>/w).The sandbox inherits DHIS2's
allow_support_accessat provisioning. While the sandbox's flag istrue, the support user can open it. As Amy, open the sandbox's Settings and toggleallow_support_accesstofalse:4. The breadcrumb and picker hide ancestors the user can't reach
Two related observations on the same underlying rule (ancestors you can't access aren't shown).
Breadcrumb truncation at the access root. As Amy, provision sandbox-A under DHIS2 (if you haven't) and add Esther to sandbox-A as an Editor (no role on DHIS2). As Esther, open sandbox-A, then its Sandboxes page, and provision a nested sandbox-B (Esther becomes its Owner at creation). Open any page inside sandbox-B. The top-left breadcrumb shows
sandbox-A/sandbox-B, notDHIS2/sandbox-A/sandbox-B.Picker collapses hidden intermediates. As Amy, reset Esther's memberships, then re-add her: Editor on DHIS2, Viewer on sandbox-B, no role on sandbox-A. As Esther, open the project picker. It shows DHIS2 at the top level with sandbox-B nested directly under it (one level of indentation); sandbox-A is absent.
Additional notes for the reviewer
AI Usage
You can read more details in our
Responsible AI Policy
Pre-submission checklist
/reviewwith Claude Code)
(e.g.,
:owner,:admin,:editor,:viewer)