From ef65e6b0159e88b69a070a6c66b0b0ded6040e1e Mon Sep 17 00:00:00 2001 From: Reece Browne <74901996+reecebrowne@users.noreply.github.com> Date: Thu, 11 Jun 2026 21:23:06 +0100 Subject: [PATCH] feat(policies): org-wide policies with admin-only editing (#6625) Co-authored-by: Claude Opus 4.8 (1M context) --- .../policy/config/PolicyAccessGuard.java | 29 ++------- .../policy/controller/PolicyController.java | 39 +++++------ .../policy/engine/PolicyEngine.java | 57 +++++++++++----- .../policy/config/PolicyAccessGuardTest.java | 62 ++++++------------ .../tests/stubbed/policy-admin-gate.spec.ts | 65 +++++++++++++++++++ .../components/policies/PolicyDetailPanel.tsx | 2 +- .../src/proprietary/hooks/usePolicies.ts | 16 ++++- 7 files changed, 160 insertions(+), 110 deletions(-) create mode 100644 frontend/editor/src/core/tests/stubbed/policy-admin-gate.spec.ts diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/policy/config/PolicyAccessGuard.java b/app/proprietary/src/main/java/stirling/software/proprietary/policy/config/PolicyAccessGuard.java index 934379614..f6d0c2417 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/policy/config/PolicyAccessGuard.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/policy/config/PolicyAccessGuard.java @@ -11,9 +11,10 @@ import stirling.software.common.service.UserServiceInterface; import stirling.software.proprietary.policy.model.Policy; /** - * Decides who may act on a stored {@link Policy}: its owner and global admins only, with no - * separate view/edit/run capability. Enforced only when login is enabled; single-user deployments - * pass every check. The owner is assigned server-side, never from client input. + * Policies are org-wide: every user may view and run any stored policy, so reads and runs are open + * to all. Creating, editing, and deleting is gated to admins at the controller (see {@code + * PolicyController#requirePolicyEditingAllowed}). The owner is still recorded server-side (for run + * / usage attribution) but no longer restricts visibility or access. */ @Component @RequiredArgsConstructor @@ -27,27 +28,9 @@ public class PolicyAccessGuard { return enforced() ? userService.getCurrentUsername() : null; } - /** Whether the current user may view, edit, delete, or run the given stored policy. */ - public boolean canAccess(Policy policy) { - if (!enforced() || userService.isCurrentUserAdmin()) { - return true; - } - String current = userService.getCurrentUsername(); - return current != null && current.equals(policy.owner()); - } - - /** - * The subset of {@code policies} the current user may see (their own; everything for an admin). - */ + /** All stored policies are visible to every user (org-wide). */ public List visible(List policies) { - if (!enforced() || userService.isCurrentUserAdmin()) { - return policies; - } - String current = userService.getCurrentUsername(); - if (current == null) { - return List.of(); - } - return policies.stream().filter(policy -> current.equals(policy.owner())).toList(); + return policies; } private boolean enforced() { diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/policy/controller/PolicyController.java b/app/proprietary/src/main/java/stirling/software/proprietary/policy/controller/PolicyController.java index 75137553b..85f4e81ae 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/policy/controller/PolicyController.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/policy/controller/PolicyController.java @@ -39,7 +39,6 @@ import stirling.software.common.model.job.JobResponse; import stirling.software.common.service.UserServiceInterface; import stirling.software.common.util.TempFile; import stirling.software.common.util.TempFileManager; -import stirling.software.proprietary.policy.config.FolderAccessGuard; import stirling.software.proprietary.policy.config.PolicyAccessGuard; import stirling.software.proprietary.policy.engine.PolicyRunHandle; import stirling.software.proprietary.policy.engine.PolicyRunRegistry; @@ -72,7 +71,6 @@ public class PolicyController { private final PolicyRunRegistry runRegistry; private final PolicyStore policyStore; private final PolicyValidator policyValidator; - private final FolderAccessGuard folderAccessGuard; private final PolicyAccessGuard policyAccessGuard; private final UserServiceInterface userService; private final ApplicationProperties applicationProperties; @@ -156,8 +154,8 @@ public class PolicyController { "Stores a policy (trigger config + steps + output + metadata). A blank id is" + " assigned; returns the stored policy with its id.") public ResponseEntity savePolicy(@RequestBody Policy policy) { + requirePolicyEditingAllowed(); Policy owned = resolveOwnership(policy); - requireAuthorizedForFolderAccess(owned); try { policyValidator.validate(owned); } catch (IllegalArgumentException e) { @@ -167,18 +165,16 @@ public class PolicyController { } /** - * Assign the owner: create stamps the current user; update preserves the existing owner after - * an access check. So the client can neither forge ownership on create nor reassign it on - * update. + * Assign the owner: create stamps the current user; update preserves the existing owner — so + * the client can neither forge ownership on create nor reassign it on update. Editing is gated + * to admins by {@link #requirePolicyEditingAllowed}; policies are org-wide, so there is no + * per-owner access check here. */ private Policy resolveOwnership(Policy incoming) { String id = incoming.id(); if (id != null && !id.isBlank()) { Policy existing = policyStore.get(id).orElse(null); if (existing != null) { - if (!policyAccessGuard.canAccess(existing)) { - throw new ResponseStatusException(HttpStatus.NOT_FOUND, "No policy: " + id); - } return withOwner(incoming, existing.owner()); } } @@ -198,28 +194,28 @@ public class PolicyController { } /** - * Folder sources/outputs grant whoever saves the policy access to that path, so gate them to - * admins on multi-user deployments. Single-user (login disabled) trusts the local operator; - * {@link FolderAccessGuard} still enforces SaaS-off and the path allowlist at validation time. + * Creating, editing, pausing/resuming, and deleting policies is admin-only on multi-user + * deployments. Every mutation routes through {@link #savePolicy} (pause/resume re-save with a + * flipped {@code enabled} flag) or {@link #deletePolicy}, so gating those two covers them all; + * runs ({@code /run}) stay open. Single-user deployments (login disabled) have no admin + * concept, so they trust the local operator. The path allowlist for folder sources/outputs is + * enforced separately by {@link PolicyValidator} at validation time. */ - private void requireAuthorizedForFolderAccess(Policy policy) { - if (!folderAccessGuard.usesFolderAccess(policy)) { - return; - } + private void requirePolicyEditingAllowed() { if (!applicationProperties.getSecurity().isEnableLogin()) { return; } if (!userService.isCurrentUserAdmin()) { throw new ResponseStatusException( HttpStatus.FORBIDDEN, - "Folder sources and outputs may only be configured by an administrator"); + "Policies may only be created or modified by an administrator"); } } @GetMapping @Operation( summary = "List policies", - description = "Lists the caller's policies; admins see all.") + description = "Lists all policies (org-wide; every user sees them all).") public List listPolicies() { return policyAccessGuard.visible(policyStore.all()); } @@ -229,7 +225,6 @@ public class PolicyController { public ResponseEntity getPolicy(@PathVariable String policyId) { return policyStore .get(policyId) - .filter(policyAccessGuard::canAccess) .map(ResponseEntity::ok) .orElseGet(() -> ResponseEntity.notFound().build()); } @@ -237,9 +232,8 @@ public class PolicyController { @DeleteMapping("/{policyId}") @Operation(summary = "Delete a policy by id") public ResponseEntity deletePolicy(@PathVariable String policyId) { - boolean accessible = - policyStore.get(policyId).filter(policyAccessGuard::canAccess).isPresent(); - if (accessible && policyStore.delete(policyId)) { + requirePolicyEditingAllowed(); + if (policyStore.get(policyId).isPresent() && policyStore.delete(policyId)) { return ResponseEntity.noContent().build(); } return ResponseEntity.notFound().build(); @@ -259,7 +253,6 @@ public class PolicyController { Policy policy = policyStore .get(policyId) - .filter(policyAccessGuard::canAccess) .orElseThrow( () -> new ResponseStatusException( diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/policy/engine/PolicyEngine.java b/app/proprietary/src/main/java/stirling/software/proprietary/policy/engine/PolicyEngine.java index 1118d0058..e342a25cf 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/policy/engine/PolicyEngine.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/policy/engine/PolicyEngine.java @@ -26,6 +26,7 @@ import stirling.software.common.service.JobQueue; import stirling.software.common.service.ResourceMonitor; import stirling.software.common.service.TaskManager; import stirling.software.common.util.ExecutorFactory; +import stirling.software.common.util.JobContext; import stirling.software.proprietary.policy.model.OutputSpec; import stirling.software.proprietary.policy.model.PipelineDefinition; import stirling.software.proprietary.policy.model.Policy; @@ -74,22 +75,34 @@ public class PolicyEngine { */ public PolicyRunHandle submit( PipelineDefinition definition, PolicyInputs inputs, PolicyProgressListener listener) { - // Ad-hoc run (no stored policy): bill whoever kicked it off. Capture the principal on this - // (request) thread — it does not survive the hop onto the async worker. - return submitForPrincipal(currentActingPrincipal(), definition, inputs, listener); + // Ad-hoc run (no stored policy): bill whoever kicked it off and own the outputs as them + // too. + // Capture the principal on this (request) thread — it does not survive the hop onto the + // async + // worker. + String principal = currentActingPrincipal(); + return submitForPrincipal(principal, principal, definition, inputs, listener); } /** Run a stored policy on demand. {@code enabled} gates triggers, not explicit runs. */ public PolicyRunHandle runPolicy( Policy policy, PolicyInputs inputs, PolicyProgressListener listener) { - // Bill the policy owner. Trigger-fired runs have no security context at all, and even an - // on-demand run executes on a background worker that doesn't inherit the caller's context — - // so the owner (a username stamped at policy creation) is the reliable billing identity. - return submitForPrincipal(policy.owner(), policy.toDefinition(), inputs, listener); + // Bill the policy owner: trigger-fired runs have no security context, and the async worker + // doesn't inherit the caller's, so the owner (stamped at policy creation) is the reliable + // billing identity — and for org-wide policies the org/owner is meant to pay. But own the + // OUTPUT files as the user who triggered the run (captured here on the request thread) so + // they can download their enforced file; otherwise an org-wide policy's output is owned by + // the admin and the triggering user is denied it. Trigger-fired runs have no such user, so + // the owner owns those outputs. + String triggeringUser = currentActingPrincipal(); + String fileOwner = triggeringUser != null ? triggeringUser : policy.owner(); + return submitForPrincipal( + policy.owner(), fileOwner, policy.toDefinition(), inputs, listener); } private PolicyRunHandle submitForPrincipal( - String actingPrincipal, + String billingPrincipal, + String fileOwner, PipelineDefinition definition, PolicyInputs inputs, PolicyProgressListener listener) { @@ -109,7 +122,8 @@ public class PolicyEngine { Runnable task = () -> runAsPrincipal( - actingPrincipal, + billingPrincipal, + fileOwner, () -> runToCompletion(run, inputs, tracking, completion)); // One admission unit per run; steps run synchronously within it, so this gates heavy work @@ -297,21 +311,30 @@ public class PolicyEngine { * dispatch attributes (and charges) usage to that user. A null/blank principal runs as-is. * Restores the previous MDC value afterward (defensive — worker threads aren't pooled). */ - private static void runAsPrincipal(String principal, Runnable body) { - if (principal == null || principal.isBlank()) { - body.run(); - return; + private static void runAsPrincipal(String billingPrincipal, String fileOwner, Runnable body) { + // Billing identity (MDC auditPrincipal) and output-file ownership (JobContext owner) are + // set + // independently: usage is charged to billingPrincipal, but stored output files are owned by + // fileOwner — the user who triggered an org-wide policy — so they can fetch their results. + // Either may be null (e.g. login disabled, or a trigger-fired run); each is applied only + // when present and restored afterward (defensive — worker threads aren't pooled). + String previousPrincipal = MDC.get(AUDIT_PRINCIPAL_MDC_KEY); + String previousOwner = JobContext.getOwner(); + if (billingPrincipal != null && !billingPrincipal.isBlank()) { + MDC.put(AUDIT_PRINCIPAL_MDC_KEY, billingPrincipal); + } + if (fileOwner != null && !fileOwner.isBlank()) { + JobContext.setOwner(fileOwner); } - String previous = MDC.get(AUDIT_PRINCIPAL_MDC_KEY); - MDC.put(AUDIT_PRINCIPAL_MDC_KEY, principal); try { body.run(); } finally { - if (previous != null) { - MDC.put(AUDIT_PRINCIPAL_MDC_KEY, previous); + if (previousPrincipal != null) { + MDC.put(AUDIT_PRINCIPAL_MDC_KEY, previousPrincipal); } else { MDC.remove(AUDIT_PRINCIPAL_MDC_KEY); } + JobContext.setOwner(previousOwner); } } } diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/policy/config/PolicyAccessGuardTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/policy/config/PolicyAccessGuardTest.java index 93720bc7d..90d0135ec 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/policy/config/PolicyAccessGuardTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/policy/config/PolicyAccessGuardTest.java @@ -1,9 +1,7 @@ package stirling.software.proprietary.policy.config; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNull; -import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.when; import java.util.List; @@ -19,8 +17,9 @@ import stirling.software.proprietary.policy.model.OutputSpec; import stirling.software.proprietary.policy.model.Policy; /** - * Tests for {@link PolicyAccessGuard}: owner-or-admin access, no-op when login is disabled, and - * server-side owner assignment. + * Tests for {@link PolicyAccessGuard}. Policies are org-wide: every user sees them all (no + * owner-based filtering). The owner is still assigned server-side — the current user when login is + * enabled, {@code null} otherwise — purely for run/usage attribution. */ @ExtendWith(MockitoExtension.class) class PolicyAccessGuardTest { @@ -34,53 +33,30 @@ class PolicyAccessGuardTest { } @Test - void loginDisabledAllowsEverythingAndAssignsNoOwner() { - PolicyAccessGuard guard = guard(false); + void visibleReturnsEveryPolicyWhenLoginEnabled() { + // Org-wide: a non-admin sees every policy, not just the ones they own. + List all = List.of(ownedBy("alice"), ownedBy("bob"), ownedBy("alice")); + assertEquals(all, guard(true).visible(all)); + } + + @Test + void visibleReturnsEveryPolicyWhenLoginDisabled() { List all = List.of(ownedBy("alice"), ownedBy("bob")); - - assertTrue(guard.canAccess(ownedBy("someone-else"))); - assertNull(guard.ownerForNewPolicy()); - assertEquals(all, guard.visible(all)); + assertEquals(all, guard(false).visible(all)); } @Test - void adminCanAccessAnyPolicy() { - when(userService.isCurrentUserAdmin()).thenReturn(true); - assertTrue(guard(true).canAccess(ownedBy("alice"))); - } - - @Test - void ownerCanAccessTheirOwnPolicy() { - when(userService.isCurrentUserAdmin()).thenReturn(false); - when(userService.getCurrentUsername()).thenReturn("alice"); - assertTrue(guard(true).canAccess(ownedBy("alice"))); - } - - @Test - void nonOwnerNonAdminCannotAccess() { - when(userService.isCurrentUserAdmin()).thenReturn(false); - when(userService.getCurrentUsername()).thenReturn("bob"); - assertFalse(guard(true).canAccess(ownedBy("alice"))); - } - - @Test - void visibleFiltersToOwnedPoliciesForANonAdmin() { - when(userService.isCurrentUserAdmin()).thenReturn(false); - when(userService.getCurrentUsername()).thenReturn("alice"); - - List visible = - guard(true).visible(List.of(ownedBy("alice"), ownedBy("bob"), ownedBy("alice"))); - - assertEquals(2, visible.size()); - assertTrue(visible.stream().allMatch(policy -> "alice".equals(policy.owner()))); - } - - @Test - void ownerForNewPolicyIsTheCurrentUserWhenEnforced() { + void ownerForNewPolicyIsTheCurrentUserWhenLoginEnabled() { when(userService.getCurrentUsername()).thenReturn("alice"); assertEquals("alice", guard(true).ownerForNewPolicy()); } + @Test + void ownerForNewPolicyIsNullWhenLoginDisabled() { + // Single-user deployment: no identity to attribute to. + assertNull(guard(false).ownerForNewPolicy()); + } + private static Policy ownedBy(String owner) { return new Policy("p1", "p", owner, true, null, List.of(), List.of(), OutputSpec.inline()); } diff --git a/frontend/editor/src/core/tests/stubbed/policy-admin-gate.spec.ts b/frontend/editor/src/core/tests/stubbed/policy-admin-gate.spec.ts new file mode 100644 index 000000000..7b1546a68 --- /dev/null +++ b/frontend/editor/src/core/tests/stubbed/policy-admin-gate.spec.ts @@ -0,0 +1,65 @@ +import { test, expect } from "@app/tests/helpers/stub-test-base"; + +/** + * Policy editing is admin-only (mirrors the backend's enforcement on the + * save/delete endpoints). The frontend gate is `canConfigure = !enableLogin || + * isAdmin`, surfaced through `usePolicies()`: + * - login enabled + non-admin → read-only: the setup wizard shows the + * "Managed by your organization" locked state instead of the steps. + * - login enabled + admin → full setup flow ("Step 1 of 2"). + * - login disabled (single-user/desktop) → open to the local operator. + * + * Backend-free: the app-config (`enableLogin`/`isAdmin`) and the empty policy + * list are stubbed via `mockAppApis`, so this asserts the gating wiring without + * a live Spring Boot server. "Security" is the only non-coming-soon policy, so + * it's the one we open. + */ + +const LOCKED_TITLE = "Managed by your organization"; +const LOCKED_DESC = "Contact an admin to change this policy."; + +/** Open the Security policy from the right-sidebar Policies list. */ +async function openSecurityPolicy(page: import("@playwright/test").Page) { + const row = page.locator("button.pol-row").filter({ hasText: "Security" }); + await expect(row).toBeVisible({ timeout: 15_000 }); + await row.click(); + // The wizard header confirms we opened the right policy in either state. + await expect(page.getByText("Set up Security Policy")).toBeVisible(); +} + +test.describe("Policy editing gate — non-admin (login on)", () => { + test.use({ + stubOptions: { enableLogin: true, isAdmin: false }, + seedJwt: true, + }); + + test("non-admin gets the read-only locked state", async ({ page }) => { + await openSecurityPolicy(page); + await expect(page.getByText(LOCKED_TITLE)).toBeVisible(); + await expect(page.getByText(LOCKED_DESC)).toBeVisible(); + // The editable flow must NOT be reachable. + await expect(page.getByText(/Step \d+ of \d+/)).toHaveCount(0); + }); +}); + +test.describe("Policy editing gate — admin (login on)", () => { + test.use({ stubOptions: { enableLogin: true, isAdmin: true }, seedJwt: true }); + + test("admin can reach the setup wizard", async ({ page }) => { + await openSecurityPolicy(page); + await expect(page.getByText(/Step \d+ of \d+/)).toBeVisible(); + await expect(page.getByText(LOCKED_TITLE)).toHaveCount(0); + }); +}); + +test.describe("Policy editing gate — single-user (login off)", () => { + test.use({ stubOptions: { enableLogin: false, isAdmin: false } }); + + test("local operator can reach the setup wizard with no admin role", async ({ + page, + }) => { + await openSecurityPolicy(page); + await expect(page.getByText(/Step \d+ of \d+/)).toBeVisible(); + await expect(page.getByText(LOCKED_TITLE)).toHaveCount(0); + }); +}); diff --git a/frontend/editor/src/proprietary/components/policies/PolicyDetailPanel.tsx b/frontend/editor/src/proprietary/components/policies/PolicyDetailPanel.tsx index e30aa5adf..f24f66ba8 100644 --- a/frontend/editor/src/proprietary/components/policies/PolicyDetailPanel.tsx +++ b/frontend/editor/src/proprietary/components/policies/PolicyDetailPanel.tsx @@ -249,7 +249,7 @@ export function PolicyDetailPanel({ } - description="Managed by your organization. Contact an admin to change settings." + description="Managed by your organization. Contact an admin to change this policy." /> )} diff --git a/frontend/editor/src/proprietary/hooks/usePolicies.ts b/frontend/editor/src/proprietary/hooks/usePolicies.ts index 1e5019248..a1ed78a28 100644 --- a/frontend/editor/src/proprietary/hooks/usePolicies.ts +++ b/frontend/editor/src/proprietary/hooks/usePolicies.ts @@ -7,6 +7,7 @@ */ import { useState, useEffect, useCallback } from "react"; +import { useAppConfig } from "@app/contexts/AppConfigContext"; import { loadPolicies, onPoliciesChange, @@ -63,6 +64,7 @@ function toStoreRequest( export function usePolicies() { const [policies, setPolicies] = useState(loadPolicies); + const { config } = useAppConfig(); useEffect(() => onPoliciesChange(() => setPolicies(loadPolicies())), []); @@ -291,9 +293,17 @@ export function usePolicies() { return folder.id; }, []); - // Configuration is open to signed-in users; real per-org gating is a backend - // concern (the mock owner/admin/member permission model has been removed). - const canConfigure = true; + // Editing/creating policies is admin-only, mirroring the backend's enforcement + // on the save/delete endpoints. On single-user deployments (login disabled) + // there's no admin concept, so the local operator can always configure. When + // login is enabled, only admins can — non-admins get the read-only surface. + // The `config != null` guard keeps the gate CLOSED until app-config resolves: + // config is null while loading (and in bare test renders), and a failed/partial + // fetch can yield `{ enableLogin: true }` with isAdmin omitted. Without it a + // non-admin would briefly see edit controls during load, and an admin would be + // wrongly locked out on a transient config-fetch error. + const canConfigure = + config != null && (!config.enableLogin || config.isAdmin === true); return { policies,