Reviewing failures is a leader-level capability, gated the same way policy editing is: see - * {@link #requireFailureReviewAllowed()}. + *
Every endpoint is open to any authenticated user and scoped in the service instead: a leader
+ * reads and closes the whole team's failures, everyone else their own. Nothing here decides who may
+ * do what, so the two cannot drift apart.
*/
@Slf4j
@RestController
@@ -46,21 +44,21 @@ public class FileRunEventController {
private static final int DEFAULT_LIMIT = 50;
private final FileRunEventService service;
- private final PolicyManagementAuthority policyManagementAuthority;
- private final ApplicationProperties applicationProperties;
@GetMapping
@Operation(
summary = "List recorded failures",
description =
- "Failures recorded for the caller's team, newest first. Each row carries its"
- + " available actions already resolved.")
+ "Failures the caller may see, newest first: their team's for a leader, their own"
+ + " for everyone else. Each row carries its available actions already"
+ + " resolved.")
public FileRunEventsResponse list(
// Spring's converter 400s on a value outside the enum, so no hand-rolled parse.
@RequestParam(required = false) FileRunEventStatus status,
@RequestParam(required = false) String kindId,
@RequestParam(required = false) Integer limit) {
- requireFailureReviewAllowed();
+ // No role gate: the service scopes the read instead, so a member gets their own failures
+ // and a leader the team's.
int cappedLimit = Math.min(limit == null ? DEFAULT_LIMIT : Math.max(1, limit), MAX_LIMIT);
List Login disabled means a single-user deployment with no roles to tell apart, the same
- * carve-out the policy endpoints make. Team scoping is separate, and lives in the service.
- */
- private void requireFailureReviewAllowed() {
- if (!applicationProperties.getSecurity().isEnableLogin()) {
- return;
- }
- if (!policyManagementAuthority.canEditPolicies()) {
- throw new ResponseStatusException(
- HttpStatus.FORBIDDEN,
- "Recorded failures may only be reviewed by a team leader");
- }
- }
-
/**
* A closed row is a conflict rather than a bad request: the request was well-formed and would
* have been valid a moment earlier.
diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventRepository.java b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventRepository.java
index 1bee2398f7..1930b36e6a 100644
--- a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventRepository.java
+++ b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventRepository.java
@@ -20,26 +20,33 @@ public interface FileRunEventRepository extends JpaRepository Team scoping mirrors {@code PolicyAccessGuard}: everyone sees only their own team's rows, the
* team always comes from the authenticated principal, and scoping applies only when login is
* enabled so single-user deployments keep working. When the team cannot be resolved the caller
- * reads nothing; see {@link #scope()}.
+ * reads nothing; see {@link #readScope()}.
*/
@Slf4j
@Service
@@ -41,7 +43,9 @@ public class FileRunEventService {
*/
public List Narrowed to the caller's own rows however senior they are, which is why it passes {@link
+ * #currentActor()} rather than the read scope's actor: file ids are minted by each client, so a
+ * leader reading with a null actor would match every unattributed row in the team.
+ *
* @return how many incidents were closed
*/
public int forgetFiles(List A null team id means two different things. Login disabled is the self-hosted setup with no
+ * users or teams, where unteamed rows are everyone's, as {@code PolicyAccessGuard} also treats
+ * them. Login enabled with no resolvable team reads nothing, because unteamed rows there are
+ * shared by every team's ad-hoc runs.
*/
- private TeamScope scope() {
+ private ReadScope readScope() {
if (!enforced()) {
- return TeamScope.of(null);
+ return ReadScope.wholeTeam(null);
}
- Long teamId = policyManagementAuthority.currentUserTeamId();
- return teamId == null ? TeamScope.denied() : TeamScope.of(teamId);
+ Long teamId = currentTeamId();
+ if (teamId == null) {
+ return ReadScope.denied();
+ }
+ if (policyManagementAuthority.canEditPolicies()) {
+ return ReadScope.wholeTeam(teamId);
+ }
+ // Narrowing to "mine" needs a name to narrow by. Without one the filter would be dropped
+ // and a member would read the whole team, so refuse rather than widen.
+ String actor = currentActor();
+ return actor == null ? ReadScope.denied() : ReadScope.mine(teamId, actor);
}
/**
- * The caller's readable team, or a refusal. {@code teamId} is only meaningful when permitted.
+ * What the caller may read. {@code actor} is the person to narrow to, or null for the whole
+ * team; both are only meaningful when permitted.
*/
- private record TeamScope(boolean permitted, Long teamId) {
+ private record ReadScope(boolean permitted, Long teamId, String actor) {
- static TeamScope of(Long teamId) {
- return new TeamScope(true, teamId);
+ static ReadScope wholeTeam(Long teamId) {
+ return new ReadScope(true, teamId, null);
}
- static TeamScope denied() {
- return new TeamScope(false, null);
+ static ReadScope mine(Long teamId, String actor) {
+ return new ReadScope(true, teamId, actor);
}
+
+ static ReadScope denied() {
+ return new ReadScope(false, null, null);
+ }
+ }
+
+ /** The team a row belongs to, which is nobody's when there are no teams to belong to. */
+ private Long currentTeamId() {
+ return enforced() ? policyManagementAuthority.currentUserTeamId() : null;
}
private String currentActor() {
diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventStore.java b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventStore.java
index cf336ab76e..2ed86e61b2 100644
--- a/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventStore.java
+++ b/app/proprietary/src/main/java/stirling/software/proprietary/failure/FileRunEventStore.java
@@ -121,16 +121,19 @@ public class FileRunEventStore {
*
* Both filters live in the query, before the limit: filtering an already-limited page could
* return nothing while matching rows exist.
+ *
+ * {@code actor} narrows to one person's own failures, or reads the whole team when null. Who
+ * gets which is the service's decision, not this method's.
*/
@Transactional(readOnly = true)
public List The actor is the run's triggering user, not the MDC audit principal: that carries the
+ * BILLING identity, which for a stored policy is always its owner. Reading it here filed every
+ * failure under the owner — hiding an attended failure from the member who caused it and holds
+ * the document, and leaving an unattended sweep's failure looking attended.
*/
private void recordFailure(PolicyRun run, String message, Throwable cause) {
failureRecorder.recordRunFailure(
@@ -386,7 +408,7 @@ public class PolicyEngine {
run.getPolicyId(),
run.getSourceId(),
run.getFileIdentity(),
- MDC.get(AUDIT_PRINCIPAL_MDC_KEY),
+ run.getTriggeringUser(),
message,
cause);
}
diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/policy/model/PolicyRun.java b/app/proprietary/src/main/java/stirling/software/proprietary/policy/model/PolicyRun.java
index 65cfa658ca..8eb0cfcc73 100644
--- a/app/proprietary/src/main/java/stirling/software/proprietary/policy/model/PolicyRun.java
+++ b/app/proprietary/src/main/java/stirling/software/proprietary/policy/model/PolicyRun.java
@@ -35,6 +35,14 @@ public class PolicyRun {
*/
private final String fileIdentity;
+ /**
+ * The user who triggered this run, or null when nothing attended it (a trigger-fired sweep).
+ * Recorded as a failure's actor, so an attended failure is handed to the person holding the
+ * document. Deliberately not the billing principal: a shared policy is billed to its owner, who
+ * may never have touched the file.
+ */
+ private final String triggeringUser;
+
private final Instant createdAt = Instant.now();
private volatile PolicyRunStatus status = PolicyRunStatus.PENDING;
@@ -63,22 +71,24 @@ public class PolicyRun {
private volatile Instant updatedAt = Instant.now();
/**
- * Both references are required rather than defaulted: a run with neither is a real case (a
- * user's upload, an ad-hoc pipeline), but it should be stated at the call site. Overloads that
- * omitted them would make losing the attribution the frictionless option, which is how both
- * fields went unpopulated in the first place.
+ * All three attribution references are required rather than defaulted: a run with none is a
+ * real case (an unattended sweep of a generator pipeline), but it should be stated at the call
+ * site. Overloads that omitted them would make losing the attribution the frictionless option,
+ * which is how {@code sourceId} and {@code fileIdentity} went unpopulated in the first place.
*/
public PolicyRun(
String runId,
String policyId,
PipelineDefinition definition,
String sourceId,
- String fileIdentity) {
+ String fileIdentity,
+ String triggeringUser) {
this.runId = runId;
this.policyId = policyId;
this.sourceId = sourceId;
this.definition = definition;
this.fileIdentity = fileIdentity;
+ this.triggeringUser = triggeringUser;
}
public int stepCount() {
diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventControllerTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventControllerTest.java
index 088cffe8ec..58b8d1b408 100644
--- a/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventControllerTest.java
+++ b/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventControllerTest.java
@@ -51,9 +51,7 @@ class FileRunEventControllerTest {
List.of(new AcknowledgeAction(store), new DismissAction(store)));
controller =
new FileRunEventController(
- new FileRunEventService(store, registry, authority, userService, props),
- authority,
- props);
+ new FileRunEventService(store, registry, authority, userService, props));
lenient().when(authority.canEditPolicies()).thenReturn(true);
lenient().when(authority.currentUserTeamId()).thenReturn(TEAM);
@@ -61,14 +59,27 @@ class FileRunEventControllerTest {
}
private FileRunEvent given(FailureKind kind, Long teamId, String fileId) {
+ return recorded("author@example.com", kind, teamId, fileId, "run-1");
+ }
+
+ /**
+ * As {@link #given} but naming who hit it, in its own run. A RUN-scoped kind keys on the run,
+ * so two rows sharing one run id are one incident, however they differ otherwise.
+ */
+ private FileRunEvent givenHitBy(String actor, FailureKind kind, Long teamId, String fileId) {
+ return recorded(actor, kind, teamId, fileId, "run-" + fileId);
+ }
+
+ private FileRunEvent recorded(
+ String actor, FailureKind kind, Long teamId, String fileId, String runId) {
return store.record(
new RecordFailure(
kind,
FailureOrigin.POLICY,
teamId,
- "author@example.com",
+ actor,
"policy-1",
- "run-1",
+ runId,
null,
fileId,
"the raw failure message"));
@@ -281,48 +292,68 @@ class FileRunEventControllerTest {
}
@Nested
- @DisplayName("only a team leader may review failures")
+ @DisplayName("a leader reviews the team's failures, everyone else their own")
class Authorization {
@Test
- void aMemberCannotListThem() {
+ void aMemberSeesTheirOwnFailuresAndNobodyElses() {
+ // A member can report a failure, so they get to see it back. It must not widen to a
+ // colleague's.
+ givenHitBy("reviewer@example.com", FailureKind.UNKNOWN, TEAM, "mine");
+ givenHitBy("colleague@example.com", FailureKind.UNKNOWN, TEAM, "theirs");
when(authority.canEditPolicies()).thenReturn(false);
- assertThatThrownBy(() -> controller.list(null, null, null))
- .isInstanceOf(ResponseStatusException.class)
- .satisfies(
- e ->
- assertThat(((ResponseStatusException) e).getStatusCode())
- .isEqualTo(HttpStatus.FORBIDDEN));
+ assertThat(controller.list(null, null, null).events())
+ .extracting(FileRunEventView::fileId)
+ .containsExactly("mine");
}
@Test
- void aMemberCannotDispatchAnAction() {
- // The read being refused is not enough on its own: an id learned any other way must
- // not let a member close another user's failure.
- FileRunEvent event = given(FailureKind.UNKNOWN, TEAM, "f-1");
- when(authority.canEditPolicies()).thenReturn(false);
+ void aLeaderSeesTheWholeTeams() {
+ givenHitBy("reviewer@example.com", FailureKind.UNKNOWN, TEAM, "mine");
+ givenHitBy("colleague@example.com", FailureKind.UNKNOWN, TEAM, "theirs");
+ when(authority.canEditPolicies()).thenReturn(true);
- assertThatThrownBy(() -> controller.act(event.id(), "DISMISS", null))
- .isInstanceOf(ResponseStatusException.class)
- .satisfies(
- e ->
- assertThat(((ResponseStatusException) e).getStatusCode())
- .isEqualTo(HttpStatus.FORBIDDEN));
+ assertThat(controller.list(null, null, null).events())
+ .extracting(FileRunEventView::fileId)
+ .containsExactlyInAnyOrder("mine", "theirs");
}
@Test
- void theRegistryIsAlsoLeaderOnly() {
+ void aMemberMayCloseTheirOwn() {
+ // Someone who fixes their own problem should not have to ask a leader to clear the row.
+ FileRunEvent mine =
+ givenHitBy("reviewer@example.com", FailureKind.UNKNOWN, TEAM, "mine");
when(authority.canEditPolicies()).thenReturn(false);
- assertThatThrownBy(() -> controller.kinds())
- .isInstanceOf(ResponseStatusException.class);
+ assertThat(controller.act(mine.id(), "DISMISS", null).status())
+ .isEqualTo(FileRunEventStatus.DISMISSED);
+ }
+
+ @Test
+ void aMemberCannotCloseAColleaguesEvenKnowingTheId() {
+ // Refusing the read is not enough on its own: an id learned any other way must not work
+ // either. Answered as not-found rather than forbidden, so trying does not confirm the
+ // row exists.
+ FileRunEvent theirs =
+ givenHitBy("colleague@example.com", FailureKind.UNKNOWN, TEAM, "theirs");
+ when(authority.canEditPolicies()).thenReturn(false);
+
+ assertThat(statusOf(() -> controller.act(theirs.id(), "DISMISS", null)))
+ .isEqualTo(HttpStatus.NOT_FOUND);
+ }
+
+ @Test
+ void theRegistryIsOpenBecauseItIsCopyNotData() {
+ // A member renders the failures they can see, so they need the labels for them. No role
+ // stub: the point is that kinds() never asks.
+ assertThat(controller.kinds()).isNotEmpty();
}
@Test
void loginDisabledTrustsTheLocalOperator() {
- // A single-user deployment has no roles to distinguish, so the role gate must not lock
- // the only user out of their own failures.
+ // A single-user deployment has no roles to distinguish, so the narrowing must not leave
+ // the only user reading nothing.
ApplicationProperties unsecured = new ApplicationProperties();
unsecured.getSecurity().setEnableLogin(false);
FileRunEventController noLogin =
@@ -335,9 +366,7 @@ class FileRunEventControllerTest {
new DismissAction(store))),
authority,
userService,
- unsecured),
- authority,
- unsecured);
+ unsecured));
assertThatCode(() -> noLogin.list(null, null, null)).doesNotThrowAnyException();
// Not merely permitted: the role is never consulted at all, which is what makes the
@@ -382,9 +411,7 @@ class FileRunEventControllerTest {
new DismissAction(store))),
authority,
userService,
- unsecured),
- authority,
- unsecured);
+ unsecured));
given(FailureKind.UNKNOWN, null, "unteamed");
given(FailureKind.UNKNOWN, TEAM, "teamed");
@@ -399,9 +426,9 @@ class FileRunEventControllerTest {
class Reporting {
@Test
- void aMemberMayReportEvenThoughTheyMayNotRead() {
- // The asymmetry is the point: anyone whose work failed can say so, but only a leader
- // reviews the queue.
+ void aMemberMayReportAndThenSeeTheirOwnReport() {
+ // Reporting was always open to a member; reading their own back is the round trip that
+ // makes the report worth anything to them.
when(authority.canEditPolicies()).thenReturn(false);
assertThatCode(
@@ -410,8 +437,9 @@ class FileRunEventControllerTest {
new EditorFailureReport(
"compress", "E004", List.of("f-1"), "boom")))
.doesNotThrowAnyException();
- assertThatThrownBy(() -> controller.list(null, null, null))
- .isInstanceOf(ResponseStatusException.class);
+ assertThat(controller.list(null, null, null).events())
+ .extracting(FileRunEventView::fileId)
+ .containsExactly("f-1");
}
@Test
@@ -441,7 +469,7 @@ class FileRunEventControllerTest {
new EditorFailureReport("compress", "E004", atLimit, "boom");
assertThat(controller.report(report).getStatusCode()).isEqualTo(HttpStatus.NO_CONTENT);
- assertThat(store.list(TEAM, null, null, EditorFailureReport.MAX_FILE_IDS + 10))
+ assertThat(store.list(TEAM, null, null, null, EditorFailureReport.MAX_FILE_IDS + 10))
.hasSize(EditorFailureReport.MAX_FILE_IDS);
}
@@ -463,7 +491,7 @@ class FileRunEventControllerTest {
overLimit,
"boom"))))
.isEqualTo(HttpStatus.BAD_REQUEST);
- assertThat(store.list(TEAM, null, null, EditorFailureReport.MAX_FILE_IDS + 10))
+ assertThat(store.list(TEAM, null, null, null, EditorFailureReport.MAX_FILE_IDS + 10))
.isEmpty();
}
diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventHttpIntegrationTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventHttpIntegrationTest.java
index b3ee4b7d15..375e46f4ad 100644
--- a/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventHttpIntegrationTest.java
+++ b/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventHttpIntegrationTest.java
@@ -438,11 +438,8 @@ class FileRunEventHttpIntegrationTest {
}
@Bean
- FileRunEventController fileRunEventController(
- FileRunEventService service,
- PolicyManagementAuthority authority,
- ApplicationProperties props) {
- return new FileRunEventController(service, authority, props);
+ FileRunEventController fileRunEventController(FileRunEventService service) {
+ return new FileRunEventController(service);
}
}
}
diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventServiceTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventServiceTest.java
index fbbe8e6359..4e6508bed9 100644
--- a/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventServiceTest.java
+++ b/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventServiceTest.java
@@ -54,6 +54,9 @@ class FileRunEventServiceTest {
lenient().when(authority.currentUserTeamId()).thenReturn(TEAM);
lenient().when(userService.getCurrentUsername()).thenReturn(ACTOR);
+ // A leader unless a test says otherwise: most of these are about team scoping, which is
+ // what a leader sees. The member narrowing has its own tests.
+ lenient().when(authority.canEditPolicies()).thenReturn(true);
}
private FileRunEvent given(FailureKind kind, Long teamId, String fileId) {
@@ -291,7 +294,7 @@ class FileRunEventServiceTest {
}
@Nested
- @DisplayName("team scoping")
+ @DisplayName("read scoping")
class Scoping {
@Test
@@ -304,6 +307,68 @@ class FileRunEventServiceTest {
.containsExactly("mine");
}
+ @Test
+ void aMemberReadsOnlyTheFailuresTheyCaused() {
+ // Reporting is open to a member, so reading their own back is what lets us tell them
+ // anything at all. A colleague's must not come with it.
+ store.record(RecordFailure.forEditor(FailureKind.UNKNOWN, TEAM, ACTOR, "mine", "boom"));
+ store.record(
+ RecordFailure.forEditor(
+ FailureKind.UNKNOWN, TEAM, "colleague@example.com", "theirs", "boom"));
+ when(authority.canEditPolicies()).thenReturn(false);
+
+ assertThat(service.list(null, null, 50))
+ .extracting(FileRunEvent::fileId)
+ .containsExactly("mine");
+ }
+
+ @Test
+ void aMemberWithNoResolvableNameReadsNothingRatherThanEverything() {
+ // Narrowing to "mine" needs a name to narrow by. Dropping the filter would hand the
+ // whole team to someone who may not have it.
+ given(FailureKind.UNKNOWN, TEAM, "mine");
+ when(authority.canEditPolicies()).thenReturn(false);
+ when(userService.getCurrentUsername()).thenReturn(null);
+
+ assertThat(service.list(null, null, 50)).isEmpty();
+ }
+
+ @Test
+ void aMemberCannotActOnAColleaguesRowEvenKnowingItsId() {
+ // Refusing the read is not enough on its own: an id learned any other way must not work
+ // either. Reported as not-found, so trying does not confirm the row exists.
+ FileRunEvent theirs =
+ store.record(
+ RecordFailure.forEditor(
+ FailureKind.UNKNOWN,
+ TEAM,
+ "colleague@example.com",
+ "theirs",
+ "boom"));
+ when(authority.canEditPolicies()).thenReturn(false);
+
+ assertThatThrownBy(() -> service.dispatch(theirs.id(), "DISMISS", Map.of()))
+ .isInstanceOf(FailureActionException.class)
+ .extracting(e -> ((FailureActionException) e).getReason())
+ .isEqualTo(FailureActionException.Reason.EVENT_NOT_FOUND);
+
+ assertThat(store.find(theirs.id(), TEAM).orElseThrow().status())
+ .isEqualTo(FileRunEventStatus.NEW);
+ }
+
+ @Test
+ void aMemberMayCloseTheirOwn() {
+ // Someone who fixes their own problem should not have to ask a leader to clear the row.
+ FileRunEvent mine =
+ store.record(
+ RecordFailure.forEditor(
+ FailureKind.UNKNOWN, TEAM, ACTOR, "mine", "boom"));
+ when(authority.canEditPolicies()).thenReturn(false);
+
+ assertThat(service.dispatch(mine.id(), "DISMISS", Map.of()).status())
+ .isEqualTo(FileRunEventStatus.DISMISSED);
+ }
+
@Test
void aCallerWhoseTeamCannotBeResolvedReadsNothing() {
// A run with no stored policy is recorded unteamed, and those rows are shared by every
@@ -399,7 +464,7 @@ class FileRunEventServiceTest {
service.report(
new EditorFailureReport("remove-password", "E004", List.of("f-1"), "boom"));
- FileRunEvent event = store.list(TEAM, null, null, 10).getFirst();
+ FileRunEvent event = store.list(TEAM, null, null, null, 10).getFirst();
assertThat(event.kind()).isEqualTo(FailureKind.INPUT_PASSWORD_PROTECTED);
assertThat(event.origin()).isEqualTo(FailureOrigin.TOOL);
assertThat(event.fileId()).isEqualTo("f-1");
@@ -412,16 +477,32 @@ class FileRunEventServiceTest {
// session.
service.report(new EditorFailureReport("compress", "E004", List.of("f-1"), "boom"));
- FileRunEvent event = store.list(TEAM, null, null, 10).getFirst();
+ FileRunEvent event = store.list(TEAM, null, null, null, 10).getFirst();
assertThat(event.teamId()).isEqualTo(TEAM);
assertThat(event.actor()).isEqualTo(ACTOR);
}
+ @Test
+ void stillFilesTheRowUnderTheTeamWhenTheReporterCannotBeNamed() {
+ // Recording is open to everyone and takes the caller's team, not their read scope: a
+ // reporter who cannot be named reads nothing back, but the row is still the team's
+ // rather than dropping into the unteamed bucket every team shares. No role stub either,
+ // since recording never asks.
+ when(userService.getCurrentUsername()).thenReturn(null);
+
+ service.report(new EditorFailureReport("compress", "E004", List.of("f-1"), "boom"));
+
+ assertThat(store.list(TEAM, null, null, null, 10))
+ .singleElement()
+ .extracting(FileRunEvent::teamId)
+ .isEqualTo(TEAM);
+ }
+
@Test
void recordsAnUnrecognisedCodeAsUnknownRatherThanDroppingIt() {
service.report(new EditorFailureReport("ocr", "E999", List.of("f-1"), "no idea"));
- assertThat(store.list(TEAM, null, null, 10).getFirst().kind())
+ assertThat(store.list(TEAM, null, null, null, 10).getFirst().kind())
.isEqualTo(FailureKind.UNKNOWN);
}
@@ -429,7 +510,7 @@ class FileRunEventServiceTest {
void recordsAnAbsentCodeAsUnknown() {
service.report(new EditorFailureReport("ocr", null, List.of("f-1"), "network died"));
- assertThat(store.list(TEAM, null, null, 10).getFirst().kind())
+ assertThat(store.list(TEAM, null, null, null, 10).getFirst().kind())
.isEqualTo(FailureKind.UNKNOWN);
}
@@ -439,7 +520,7 @@ class FileRunEventServiceTest {
new EditorFailureReport(
"compress", "E004", List.of("f-1", "f-2", "f-3"), "boom"));
- assertThat(store.list(TEAM, null, null, 10))
+ assertThat(store.list(TEAM, null, null, null, 10))
.hasSize(3)
.extracting(FileRunEvent::fileId)
.containsExactlyInAnyOrder("f-1", "f-2", "f-3");
@@ -451,7 +532,7 @@ class FileRunEventServiceTest {
service.report(
new EditorFailureReport("compress", "E004", List.of("f-1"), "boom again"));
- assertThat(store.list(TEAM, null, null, 10))
+ assertThat(store.list(TEAM, null, null, null, 10))
.singleElement()
.extracting(FileRunEvent::occurrences)
.isEqualTo(2);
@@ -461,7 +542,7 @@ class FileRunEventServiceTest {
void recordsOneUnattributedIncidentWhenNoFileWasNamed() {
service.report(new EditorFailureReport("compress", "E004", List.of(), "boom"));
- assertThat(store.list(TEAM, null, null, 10))
+ assertThat(store.list(TEAM, null, null, null, 10))
.singleElement()
.extracting(FileRunEvent::fileId)
.isNull();
@@ -476,7 +557,7 @@ class FileRunEventServiceTest {
service.report(new EditorFailureReport("compress", "E004", many, "boom"));
- assertThat(store.list(TEAM, null, null, 200)).hasSize(60);
+ assertThat(store.list(TEAM, null, null, null, 200)).hasSize(60);
}
@Test
@@ -486,7 +567,7 @@ class FileRunEventServiceTest {
service.report(
new EditorFailureReport("remove-password", "E004", List.of("f-1"), "boom"));
- FileRunEvent event = store.list(TEAM, null, null, 10).getFirst();
+ FileRunEvent event = store.list(TEAM, null, null, null, 10).getFirst();
assertThat(event.detail()).contains("remove-password");
assertThat(event.fileId()).isEqualTo("f-1");
}
@@ -499,7 +580,7 @@ class FileRunEventServiceTest {
new EditorFailureReport(
"compress", "E004", List.of("f-1"), "Failed on Q4 report.pdf"));
- assertThat(store.list(TEAM, null, null, 10).getFirst().detail())
+ assertThat(store.list(TEAM, null, null, null, 10).getFirst().detail())
.isEqualTo("compress: Failed on Q4 report.pdf");
}
@@ -540,7 +621,7 @@ class FileRunEventServiceTest {
reportedBy("alice@example.com", "a-1");
reportedBy("bob@example.com", "b-1");
- assertThat(store.list(TEAM, null, null, 10))
+ assertThat(store.list(TEAM, null, null, null, 10))
.extracting(FileRunEvent::actor)
.containsExactlyInAnyOrder("alice@example.com", "bob@example.com");
}
@@ -550,7 +631,7 @@ class FileRunEventServiceTest {
reportedBy("alice@example.com", "a-1");
reportedBy("alice@example.com", "a-2");
- assertThat(store.list(TEAM, null, null, 10))
+ assertThat(store.list(TEAM, null, null, null, 10))
.extracting(FileRunEvent::fileId)
.containsExactlyInAnyOrder("a-1", "a-2");
}
@@ -560,7 +641,7 @@ class FileRunEventServiceTest {
reportedBy("alice@example.com", "a-1");
reportedBy("alice@example.com", "a-1");
- assertThat(store.list(TEAM, null, null, 10))
+ assertThat(store.list(TEAM, null, null, null, 10))
.singleElement()
.extracting(FileRunEvent::occurrences)
.isEqualTo(2);
@@ -612,8 +693,11 @@ class FileRunEventServiceTest {
}
@Test
- void aColleaguesIncidentIsUntouched() {
- // File ids come from the client, so naming one must not close someone else's row.
+ void aColleaguesIncidentIsUntouchedEvenForALeader() {
+ // File ids come from the client, so naming one must not close someone else's row. The
+ // caller here is a leader, who reads the whole team: this path narrows to their own
+ // rows
+ // regardless, since a null actor would otherwise match every unattributed row.
store.record(
RecordFailure.forEditor(
FailureKind.UNKNOWN, TEAM, "employee@example.com", "f-1", "theirs"));
diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventStoreDbTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventStoreDbTest.java
index 949904dfb9..982d804354 100644
--- a/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventStoreDbTest.java
+++ b/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventStoreDbTest.java
@@ -55,11 +55,16 @@ class FileRunEventStoreDbTest {
}
private RecordFailure failure(FailureKind kind, Long teamId, String fileId) {
+ return failure(kind, teamId, "author@example.com", fileId);
+ }
+
+ /** As {@link #failure} but naming the actor, which is what the read scope narrows by. */
+ private RecordFailure failure(FailureKind kind, Long teamId, String actor, String fileId) {
return new RecordFailure(
kind,
FailureOrigin.POLICY,
teamId,
- "author@example.com",
+ actor,
"policy-1",
"run-1",
null,
@@ -74,16 +79,37 @@ class FileRunEventStoreDbTest {
store.record(failure(FailureKind.UNKNOWN, OTHER_TEAM, "theirs"));
store.record(failure(FailureKind.UNKNOWN, null, "unteamed"));
- assertThat(store.list(TEAM, null, null, 10))
+ assertThat(store.list(TEAM, null, null, null, 10))
.extracting(FileRunEvent::fileId)
.containsExactly("ours");
// A plain `e.teamId = :teamId` would return nothing here: SQL equality against NULL is
// never true, which is what the explicit null branch in the JPQL exists for.
- assertThat(store.list(null, null, null, 10))
+ assertThat(store.list(null, null, null, null, 10))
.extracting(FileRunEvent::fileId)
.containsExactly("unteamed");
}
+ @Test
+ @DisplayName("actor narrowing is enforced by the query, within the team")
+ void actorNarrowingIsEnforcedBySql() {
+ // The clause that makes a member read only their own rows. Exercised here rather than only
+ // against the in-memory repository, which reimplements the filter in Java and would agree
+ // with a query that had lost it.
+ store.record(failure(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "mine@example.com", "f1"));
+ store.record(
+ failure(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "theirs@example.com", "f2"));
+ store.record(failure(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, null, "f3"));
+
+ assertThat(store.list(TEAM, null, null, "mine@example.com", 10))
+ .extracting(FileRunEvent::fileId)
+ .containsExactly("f1");
+ // A null actor is "no filter", which is what a leader reads with: the whole team, including
+ // the rows nobody is named on.
+ assertThat(store.list(TEAM, null, null, null, 10))
+ .extracting(FileRunEvent::fileId)
+ .containsExactlyInAnyOrder("f1", "f2", "f3");
+ }
+
@Test
@DisplayName("a fold lands on the row's current state, not the caller's snapshot")
void foldTargetsTheCurrentRowNotACallersSnapshot() {
@@ -170,7 +196,7 @@ class FileRunEventStoreDbTest {
assertThat(replacement.id()).isNotEqualTo(first.id());
assertThat(replacement.occurrences()).isEqualTo(1);
- assertThat(store.list(TEAM, null, null, 10)).hasSize(1);
+ assertThat(store.list(TEAM, null, null, null, 10)).hasSize(1);
}
@Test
@@ -188,7 +214,7 @@ class FileRunEventStoreDbTest {
FileRunEvent folded = store.record(secondSweep);
assertThat(folded.occurrences()).isEqualTo(2);
- assertThat(store.list(TEAM, null, null, 10))
+ assertThat(store.list(TEAM, null, null, null, 10))
.as("one incident per document, however many runs it failed in")
.extracting(FileRunEvent::fileId)
.containsExactlyInAnyOrder("file-hash-a", "file-hash-b");
@@ -267,7 +293,7 @@ class FileRunEventStoreDbTest {
store.record(failure(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "newer-" + i));
}
- assertThat(store.list(TEAM, null, "UNKNOWN", 1))
+ assertThat(store.list(TEAM, null, "UNKNOWN", null, 1))
.extracting(FileRunEvent::fileId)
.containsExactly("old-unknown");
}
diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventStoreTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventStoreTest.java
index 714d65c29c..97de1952e9 100644
--- a/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventStoreTest.java
+++ b/app/proprietary/src/test/java/stirling/software/proprietary/failure/FileRunEventStoreTest.java
@@ -281,7 +281,7 @@ class FileRunEventStoreTest {
store.record(failure(FailureKind.UNKNOWN, TEAM, "mine", "a"));
store.record(failure(FailureKind.UNKNOWN, OTHER_TEAM, "theirs", "b"));
- assertThat(store.list(TEAM, null, null, 50))
+ assertThat(store.list(TEAM, null, null, null, 50))
.extracting(FileRunEvent::fileId)
.containsExactly("mine");
}
@@ -293,7 +293,7 @@ class FileRunEventStoreTest {
store.record(failure(FailureKind.UNKNOWN, null, "unteamed", "a"));
store.record(failure(FailureKind.UNKNOWN, TEAM, "teamed", "b"));
- assertThat(store.list(null, null, null, 50))
+ assertThat(store.list(null, null, null, null, 50))
.extracting(FileRunEvent::fileId)
.containsExactly("unteamed");
}
@@ -305,8 +305,8 @@ class FileRunEventStoreTest {
FileRunEvent event = store.record(failure(FailureKind.UNKNOWN, TEAM, "f1", "boom"));
store.applyStatus(event.id(), TEAM, FileRunEventStatus.DISMISSED, "reviewer");
- assertThat(store.list(TEAM, null, null, 10)).isEmpty();
- assertThat(store.list(TEAM, FileRunEventStatus.DISMISSED, null, 10)).hasSize(1);
+ assertThat(store.list(TEAM, null, null, null, 10)).isEmpty();
+ assertThat(store.list(TEAM, FileRunEventStatus.DISMISSED, null, null, 10)).hasSize(1);
}
@Test
@@ -314,7 +314,7 @@ class FileRunEventStoreTest {
FileRunEvent event = store.record(failure(FailureKind.UNKNOWN, TEAM, "f1", "boom"));
store.applyStatus(event.id(), TEAM, FileRunEventStatus.ACKNOWLEDGED, "reviewer");
- assertThat(store.list(TEAM, null, null, 10)).hasSize(1);
+ assertThat(store.list(TEAM, null, null, null, 10)).hasSize(1);
}
@Test
@@ -323,7 +323,7 @@ class FileRunEventStoreTest {
store.record(failure(FailureKind.INPUT_PASSWORD_PROTECTED, TEAM, "closed", "b"));
store.applyStatus(open.id(), TEAM, FileRunEventStatus.ACKNOWLEDGED, "me");
- assertThat(store.list(TEAM, FileRunEventStatus.ACKNOWLEDGED, null, 50))
+ assertThat(store.list(TEAM, FileRunEventStatus.ACKNOWLEDGED, null, null, 50))
.extracting(FileRunEvent::fileId)
.containsExactly("open");
}
diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/failure/InMemoryFileRunEventRepository.java b/app/proprietary/src/test/java/stirling/software/proprietary/failure/InMemoryFileRunEventRepository.java
index 1873f6265b..6dbb16322b 100644
--- a/app/proprietary/src/test/java/stirling/software/proprietary/failure/InMemoryFileRunEventRepository.java
+++ b/app/proprietary/src/test/java/stirling/software/proprietary/failure/InMemoryFileRunEventRepository.java
@@ -54,9 +54,18 @@ class InMemoryFileRunEventRepository implements FileRunEventRepository {
return kindId == null || kindId.equals(entity.getKindId());
}
+ /** Null means the whole team, matching the JPQL's {@code :actor is null} branch. */
+ private static boolean sameActor(FileRunEventEntity entity, String actor) {
+ return actor == null || actor.equals(entity.getActor());
+ }
+
@Override
public List This is what makes this PR's promise hold. Reads are narrowed to the rows the caller is the
+ * actor on, so if the engine names the wrong person, a member reads nothing at all.
+ *
+ * The bug it exists for: the engine recorded the BILLING principal as the actor, which for a
+ * stored policy is always its owner. So an attended failure was filed under someone who never
+ * touched the document, and the member who did could not see it.
+ */
+@ExtendWith(MockitoExtension.class)
+class PolicyFailureAttributionTest {
+
+ private static final String ROTATE = "/api/v1/general/rotate-pdf";
+ private static final Long TEAM = 3L;
+
+ @Mock private InternalApiClient internalApiClient;
+ @Mock private ToolMetadataService toolMetadataService;
+ @Mock private TaskManager taskManager;
+ @Mock private FileStorage fileStorage;
+ @Mock private JobOwnershipService jobOwnershipService;
+ @Mock private ResourceMonitor resourceMonitor;
+ @Mock private JobQueue jobQueue;
+ @Mock private PolicyStore policyStore;
+ @Mock private PolicyManagementAuthority authority;
+ @Mock private UserServiceInterface userService;
+
+ @TempDir Path tempDir;
+
+ private PolicyEngine engine;
+ private FileRunEventService service;
+
+ @BeforeEach
+ void setUp() {
+ ApplicationProperties props = new ApplicationProperties();
+ props.getSecurity().setEnableLogin(true);
+ props.getSystem().getTempFileManagement().setBaseTmpDir(tempDir.toString());
+ props.getSystem().getTempFileManagement().setPrefix("failure-attribution-test-");
+
+ FileRunEventStore store = new FileRunEventStore(new InMemoryFileRunEventRepository());
+ service =
+ new FileRunEventService(
+ store,
+ new FailureActionRegistry(
+ List.of(new AcknowledgeAction(store), new DismissAction(store))),
+ authority,
+ userService,
+ props);
+
+ PolicyFailureRecorder recorder =
+ new PolicyFailureRecorder(
+ new FailureClassifier(JsonMapper.builder().build()), store, policyStore);
+ PolicyExecutor executor =
+ new PolicyExecutor(
+ internalApiClient,
+ toolMetadataService,
+ new TempFileManager(new TempFileRegistry(), props),
+ JsonMapper.builder().build());
+ engine =
+ new PolicyEngine(
+ executor,
+ taskManager,
+ new PolicyRunRegistry(new ApplicationProperties()),
+ recorder,
+ fileStorage,
+ jobOwnershipService,
+ List.of(new InlineOutputSink(fileStorage)),
+ new PolicyOutputResolver(new InProcessSourceStore()),
+ resourceMonitor,
+ jobQueue,
+ new PolicyAssetResolver(new InProcessPolicyAssetStore()));
+
+ lenient()
+ .when(jobOwnershipService.createScopedJobKey(anyString()))
+ .thenAnswer(invocation -> invocation.getArgument(0));
+ lenient().when(resourceMonitor.shouldQueueJob(anyInt())).thenReturn(false);
+ lenient().when(toolMetadataService.isMultiInput(anyString())).thenReturn(false);
+ // The team is resolved from the policy, so the recorded row lands in the reader's team.
+ lenient().when(policyStore.get(anyString())).thenReturn(Optional.of(sharedPolicy()));
+ lenient().when(authority.currentUserTeamId()).thenReturn(TEAM);
+ }
+
+ /** Alice's policy, shared with her team. Bob is a member of it and does not own it. */
+ private static Policy sharedPolicy() {
+ return new Policy(
+ "p1",
+ "rotate",
+ "alice",
+ true,
+ List.of(),
+ List.of(new PipelineStep(ROTATE, Map.of())),
+ OutputSpec.inline(),
+ TEAM);
+ }
+
+ /**
+ * Run the shared policy so its single tool step fails, as {@code triggeredBy} (null = sweep).
+ */
+ private void runAndFail(String triggeredBy, String sourceId, String fileIdentity)
+ throws Exception {
+ when(internalApiClient.post(eq(ROTATE), any())).thenThrow(new RuntimeException("boom"));
+ if (triggeredBy != null) {
+ MDC.put("auditPrincipal", triggeredBy);
+ }
+ try {
+ engine.runPolicy(
+ sharedPolicy(),
+ PolicyInputs.of(List.of(pdf())),
+ PolicyProgressListener.NOOP,
+ sourceId,
+ fileIdentity)
+ .completion()
+ .get(10, TimeUnit.SECONDS);
+ } finally {
+ MDC.remove("auditPrincipal");
+ }
+ }
+
+ private static ByteArrayResource pdf() {
+ return new ByteArrayResource("input".getBytes()) {
+ @Override
+ public String getFilename() {
+ return "input.pdf";
+ }
+ };
+ }
+
+ /** Read as a plain member, who is narrowed to the rows they are the actor on. */
+ private FileRunEvent asMember(String reader) {
+ lenient().when(userService.getCurrentUsername()).thenReturn(reader);
+ lenient().when(authority.canEditPolicies()).thenReturn(false);
+ List