Sandbox-aware Project Settings page#4632
Open
elias-ba wants to merge 16 commits intosandbox-collectionsfrom
Open
Sandbox-aware Project Settings page#4632elias-ba wants to merge 16 commits intosandbox-collectionsfrom
elias-ba wants to merge 16 commits intosandbox-collectionsfrom
Conversation
Introduces a small reusable banner component with three variants (Local, Editable, Inherited) that explains how settings on a tab will or won't flow on merge. Settings LiveView preloads the parent project and exposes both sandbox? and parent_project as assigns so the template can render the sandbox-specific UI.
Per Joe's per-page proposal in #3398: * project: Sandbox Identity panel with parent project link, hide Delete button, show Local banner * credentials, collections: Editable banner * webhook_security: replace auth methods table with explanatory message in sandboxes (V1 doesn't support webhook security in sandboxes) * collaboration: Local banner * security (MFA): Inherited banner, MFA toggle disabled * vcs, data-storage, history-exports: Local banner
Adds Sandboxes.parent_admin?/2 which walks the parent chain and returns true when the user is admin or owner on any ancestor project. Projects.delete_project_user!/1 now raises when removing such a user from a sandbox, so the protection holds even if the LiveView UI guard is bypassed.
LiveView tests cover the per-tab banners, Sandbox Identity panel, disabled MFA toggle, webhook security message, and the parent admin floor rule (UI guard + Projects.delete_project_user! enforcement). Regression tests in sandboxes_test.exs document that the merge pipeline does not propagate Local/Inherited project-level fields (requires_mfa, concurrency, retention, name, description, collaborators) from a sandbox to its parent. These guard against future MergeProjects or Provisioner changes that could accidentally start syncing these fields.
Security ReviewClaude hit the max-turns limit or encountered an error before posting findings. See the workflow run for details. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## sandbox-collections #4632 +/- ##
=======================================================
+ Coverage 89.65% 89.70% +0.04%
=======================================================
Files 444 445 +1
Lines 21609 21675 +66
=======================================================
+ Hits 19374 19443 +69
+ Misses 2235 2232 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The ancestors/1 helper handled a parent project lookup returning nil, but the parent_id FK uses on_delete: :nilify_all so a stale parent_id can't exist. The LiveView's parent_admin?/2 had a second clause fetching a user by user_id when the user wasn't preloaded, but get_project_users!/1 always preloads :user. Both were defensive code for scenarios prevented by FK constraints. Removed per CLAUDE.md guidance not to handle scenarios that can't happen.
Two bugs from the previous "remove dead code" commit: 1. Sandboxes.ancestors/1 needs the nil clause back. The parent_id FK uses :nilify_all but that fires at delete-time in the database. An in-memory project struct loaded before the parent was deleted will still have the old parent_id. The nil clause handles this race gracefully instead of crashing the LiveView. Added a regression test that exercises this branch. 2. The handle_event remove path loaded the project_user without preloading :user, so removing the second parent_admin?/2 clause would have crashed it with FunctionClauseError. Preloading :user at the call site is more targeted than restoring the dead clause.
The view shouldn't reach into Repo. Adds an include opt to get_project_user!/2 so callers can request preloaded associations through the context (mirrors Ecto's :preload but named for the caller's intent). Drops the now-unused Repo alias in the settings LiveView.
Collaborator
josephjclark
left a comment
josephjclark
left a comment
There was a problem hiding this comment.
Fantastic!
It feels a lot better to think about this stuff here than in the issue. The top banner is working great - setting per-group context really helps
Some minor tweaks in comments and maybe we'll want to tune the UI right at the end
Copy and styling (Joe's inline review):
- Drop "won't sync" / "can't be changed here" in favour of "do not sync"
/ "cannot be changed here" in the sandbox settings banner.
- Add a coloured border to each banner variant so they don't look ghosted.
- Reword the Sandbox Identity parent line to "Identifies this sandbox
within its parent: <PARENT>".
- Drop "for now" from the Webhook Security copy.
Banner placement:
- Move the banner under the tab's section_header across all sandbox
panels so the reading order is page -> section -> context -> content.
- For Collections, the section_header lives inside CollectionsComponent,
so pass `sandbox?` into the component and render the banner there.
Fix a misleading "Role based permissions" message:
- The Webhook Security and Security (MFA) sandbox branches were passing
`can_perform_action={false}` to section_header, which forced the role
message even for admins. The block isn't role-based in either case:
webhook security is categorically disabled, MFA is inherited.
- Make `permissions_message` on section_header optional (default nil).
When nil, the component does not render the role message even if
`can_perform_action` is false. Existing callers are unaffected.
- Webhook Security sandbox branch: render a regular content card
(matching other settings cards on the page) with accurate copy that
explains webhook auth methods are shared with and enforced on the
sandbox but only manageable from the parent. Avoids the misleading
"disabled" framing.
- Security sandbox branch: `can_perform_action={@can_edit_project}` with
`permissions_message` nilified in sandbox mode. Role message still
fires for viewers outside sandbox; in sandbox, the Inherited banner
and the disabled MFA toggle carry the meaning without double-messaging.
Tests:
- New regression: the sandbox settings page never renders the role
permissions line on the webhook_security or security tabs.
…x-settings-page # Conflicts: # lib/lightning_web/live/project_live/collections_component.ex # lib/lightning_web/live/project_live/settings.html.heex
The Delete project button was hidden in sandboxes as a design call, but #3398 never asked for that. Brandon's triage in the issue thread put "Delete sandbox" under Local (editable in the sandbox). Un-hide the danger zone so we don't silently remove a feature the spec didn't ask us to remove; flip the sandbox test to match.
Inside a sandbox, the Delete button now reads "Delete sandbox" and goes through Sandboxes.delete_sandbox/2 instead of schedule_project_deletion/1. The difference matters: delete_sandbox is synchronous (no grace period email noise), fires the sandbox-lifecycle metric, and checks the sandbox-specific authorization policy. Reusing it means the in-settings delete flow behaves identically to deleting from the Sandboxes page. - Settings assigns `root_project` (via Projects.root_of/1) so we can redirect to the top-level ancestor's workflows page on success, matching the sandbox_live push_navigate target. - apply_action(:delete) sets up a confirm_changeset in sandbox mode. - Template branches on @sandbox? to render the existing SandboxLive.Components.confirm_delete_modal (name-typing confirm) for sandboxes and ProjectDeletionModal for regular projects. - handle_event("confirm-delete", ...) on a sandbox calls delete_sandbox, flashes the same "Sandbox X and all its associated descendants deleted" message as the Sandboxes page, and push_navigates to /projects/:root_project.id/w. - Test covers the full flow end-to-end including the redirect target and flash message.
The project settings index route lives in the :services live_session while the settings/delete route lives in :default. push_patch can't cross live_session boundaries — it raised ArgumentError when the user hit Close on the sandbox delete modal. Switch close-delete-modal and the delete error paths to push_navigate so they do a full reload back to the settings index.
The shared <.input> component already renders field errors, and the confirm modal was also calling <.errors field=...> right below it, so every validation error showed up twice. Dropping the redundant row.
Five new tests exercise the previously-uncovered branches in
settings.ex's delete handlers:
- confirm-delete-validate with a wrong name renders the error
- confirm-delete submit with a wrong name keeps the sandbox and
re-renders the changeset errors (the else branch)
- close-delete-modal navigates back to the settings index
- delete_sandbox returning {:error, :unauthorized} flashes the
permission message and navigates back to settings
- delete_sandbox returning {:error, _reason} flashes the generic
"could not delete" message and navigates back
The two error-path tests stub Sandboxes.delete_sandbox via Mimic since
the real call succeeds in tests and wouldn't reach those branches
otherwise.
Contributor
Author
|
@josephjclark ready for another look. Addressed the five inline comments. One wording call to flag: you'd suggested "Webhook security is disabled for sandboxes", but auth methods from the parent are still enforced on the sandbox's webhook triggers, so I went with "Webhook authentication is managed in the parent project" instead - happy to swap if you prefer yours. |
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
2 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
This implements the per-page approach we agreed on in #3398. The Project Settings page now behaves differently when you're inside a sandbox: each tab tells you whether your changes will sync on merge, and the UI for some tabs changes shape to match what makes sense for a sandbox (no Delete button, MFA read-only, webhook security managed at the parent level, etc.).
The categorization, agreed in the issue thread:
projectcredentialscollectionswebhook_securitycollaborationsecurity(MFA)vcsdata-storagehistory-exportsA few things worth knowing about how this is implemented, because they affected the scope:
The merge layer is already protective. I started worrying that "Local" and "Inherited" were just UI promises and that a user changing data retention in a sandbox would actually push that value back to the parent on merge. After tracing through
MergeProjects.merge_project/3andProvisioner.import_document/4, that turns out not to be the case. The merge document only takes[:name, :description, :env, :color]from the target (not the source), and the provisioner only casts[:id, :name, :description]on the project itself. Project-level fields likerequires_mfa,concurrency,retention_policy,history_retention_period,dataclip_retention_periodaren't in the cast list, so they never propagate. Webhook auth methods, project_users, and the GitHub repo connection aren't in the merge document at all. I added regression tests insandboxes_test.exsto lock this in so a future change to MergeProjects or the Provisioner doesn't accidentally start syncing them.The parent admin floor rule is enforced server-side, not just in the UI. A user who is admin or owner on any ancestor project (walking the parent chain, so admin on grandparent counts) cannot be removed from a sandbox. The UI disables the Remove button with a tooltip explaining why, but
Projects.delete_project_user!/1also raises anArgumentErrorif you try to do it directly, so a curl bypass or a future code path can't get around it. There's a unit test covering both paths.Webhook auth methods stay enforced but can't be managed from the sandbox. Auth methods flow down from the parent and are still applied to the sandbox's webhook triggers, but the sandbox UI points users back to the parent for create/edit/delete. The tab is still present so users can find it; the panel renders a notice and a link to the parent project's webhook security tab instead of the full management table.
Closes #3398
Validation steps
Setup
adminon this project.You're now inside the sandbox. Every test below happens from here unless I say otherwise.
1. Sandbox Setup tab (Local)
Navigate to Project Settings in the sandbox. The first tab should be Setup.
What you should see:
Sandboxes.delete_sandbox/2and redirects to the root project's workflows page with the flash "Sandbox and all its associated descendants deleted"2. Credentials and Collections (Editable)
These are the two tabs where changes flow back. We'll create one of each in the sandbox now and confirm in step 7 that they show up in the parent after merge.
Credentials
Collections
3. Webhook Security (managed in parent)
Click Webhook Security. Instead of the usual auth methods table, you should see a notice that reads something like:
Followed by a link: "Manage them in the parent project ([Cat Shelter Registry])" pointing back to the parent's webhook security tab.
4. Collaboration and the parent admin floor rule
Click Collaboration. You should see a Local banner.
The collaborators table shows the current sandbox members. The teammate you added as admin in step 3 of Setup should also be listed (they were inherited when the sandbox was provisioned).
Now hover over the Remove Collaborator button next to that admin. The button should be disabled, and the tooltip should read:
For comparison, add another teammate to the sandbox as
editor(not admin on the parent). Their Remove button should be enabled normally.5. Security tab (Inherited)
Click Security. You should see:
not-allowedon hover, the switch reflects the parent's value)The value you see should match what you set on the parent in step 2 of Setup.
6. The other Local tabs
Click through Sync to GitHub, Data Storage, History Exports. Each should show the blue Local banner at the top. The forms remain editable. This is intentional: you can change these in the sandbox to support sandbox-specific workflows, but the values do not sync back.
7. Merge: what flows back, what doesn't
This is the part that worried me when I started, so worth confirming hands-on.
Still in the sandbox:
Credentials (Editable — should sync):
Collections (Editable — should sync):
Setup (Local — should not sync):
Data Storage (Local — should not sync):
Security (Inherited — should not sync):
Editable categories flow back, Local and Inherited categories don't. That asymmetry is what
sandboxes_test.exslocks in at the merge-document/provisioner level.AI Usage
Pre-submission checklist
/reviewwith Claude Code)