diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/controller/api/ToolRecommendationController.java b/app/proprietary/src/main/java/stirling/software/proprietary/controller/api/ToolRecommendationController.java index adab4b539d..5a666d7e6d 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/controller/api/ToolRecommendationController.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/controller/api/ToolRecommendationController.java @@ -4,6 +4,7 @@ import java.util.List; import java.util.Optional; import java.util.regex.Pattern; +import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; import org.springframework.web.bind.annotation.GetMapping; import org.springframework.web.bind.annotation.PostMapping; @@ -20,7 +21,6 @@ import stirling.software.common.annotations.api.ProprietaryUiDataApi; import stirling.software.common.service.UserServiceInterface; import stirling.software.proprietary.service.ToolRecommendationService; import stirling.software.proprietary.service.ToolRecommendationService.ToolRecommendation; -import stirling.software.proprietary.service.ToolRecommendationService.ToolWorkflow; import stirling.software.proprietary.service.ToolUsageTrackingService; /** @@ -40,8 +40,6 @@ public class ToolRecommendationController { public record RecommendationsResponse(List recommendations) {} - public record WorkflowsResponse(List workflows) {} - /** * @param priorChains the tools already applied to each input document, oldest step first and * excluding this run - one entry per input document, empty for a fresh upload. @@ -72,29 +70,6 @@ public class ToolRecommendationController { } } - @GetMapping("/tool-recommendations/workflows") - @Operation( - summary = "Get repeated tool workflows", - description = - "Ordered tool sequences that get applied to the same document over and over," - + " most repeated first. Intended as the basis for suggesting" - + " automations, so each entry says whether the pattern is the" - + " caller's own, their team's, or the whole install's.") - public ResponseEntity getWorkflows( - @RequestParam(value = "minLength", defaultValue = "2") int minLength, - @RequestParam(value = "limit", defaultValue = "6") int limit, - @RequestHeader(value = "X-Browser-Id", required = false) String browserId) { - try { - return ResponseEntity.ok( - new WorkflowsResponse( - recommendationService.getWorkflows( - resolvePrincipal(browserId), minLength, limit))); - } catch (Exception e) { - log.warn("Failed to load tool workflows: {}", e.getMessage()); - return ResponseEntity.ok(new WorkflowsResponse(List.of())); - } - } - @PostMapping("/tool-recommendations/usage") @Operation( summary = "Record a completed tool run", @@ -108,6 +83,10 @@ public class ToolRecommendationController { if (request == null || !ToolUsageTrackingService.isValidToolKey(request.toolKey())) { return ResponseEntity.badRequest().build(); } + if (!trackingService.isRecordingEnabled()) { + // 501 latches the client, so a declining install stops receiving the posts at all. + return ResponseEntity.status(HttpStatus.NOT_IMPLEMENTED).build(); + } trackingService.recordUsage( resolvePrincipal(browserId), request.toolKey(), request.priorChains()); return ResponseEntity.noContent().build(); diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/service/ToolUsageTrackingService.java b/app/proprietary/src/main/java/stirling/software/proprietary/service/ToolUsageTrackingService.java index 4311040b6f..97ed8cda31 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/service/ToolUsageTrackingService.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/service/ToolUsageTrackingService.java @@ -7,7 +7,6 @@ import java.util.Comparator; import java.util.LinkedHashSet; import java.util.List; import java.util.Set; -import java.util.concurrent.TimeUnit; import java.util.regex.Pattern; import org.springframework.dao.DataIntegrityViolationException; @@ -57,6 +56,11 @@ public class ToolUsageTrackingService { && properties.getSystem().isAnalyticsEnabled(); } + /** Lets the controller answer 501 so the browser stops posting on a declining install. */ + public boolean isRecordingEnabled() { + return isUsageDataAllowed(applicationProperties); + } + /** * Counts one completed run of {@code toolKey} against each input document's history. * @@ -177,7 +181,7 @@ public class ToolUsageTrackingService { } /** Daily retention sweep; both tables hold one row per principal, key and day. */ - @Scheduled(fixedDelay = 1, initialDelay = 1, timeUnit = TimeUnit.DAYS) + @Scheduled(cron = "0 15 3 * * *") public void cleanupOldStats() { int retentionDays = applicationProperties.getToolRecommendations().getRetentionDays(); if (retentionDays <= 0) { diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/controller/api/ToolRecommendationControllerTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/controller/api/ToolRecommendationControllerTest.java index d2c86c84b0..6fb3210402 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/controller/api/ToolRecommendationControllerTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/controller/api/ToolRecommendationControllerTest.java @@ -1,13 +1,18 @@ package stirling.software.proprietary.controller.api; import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.ArgumentMatchers.isNull; +import static org.mockito.Mockito.lenient; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; +import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; import java.util.List; import java.util.Optional; @@ -20,15 +25,14 @@ import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; import org.springframework.http.ResponseEntity; +import org.springframework.test.web.servlet.MockMvc; +import org.springframework.test.web.servlet.setup.MockMvcBuilders; import stirling.software.common.service.UserServiceInterface; import stirling.software.proprietary.controller.api.ToolRecommendationController.RecommendationsResponse; import stirling.software.proprietary.controller.api.ToolRecommendationController.UsageRequest; -import stirling.software.proprietary.controller.api.ToolRecommendationController.WorkflowsResponse; import stirling.software.proprietary.service.ToolRecommendationService; import stirling.software.proprietary.service.ToolRecommendationService.ToolRecommendation; -import stirling.software.proprietary.service.ToolRecommendationService.ToolWorkflow; -import stirling.software.proprietary.service.ToolRecommendationService.WorkflowScope; import stirling.software.proprietary.service.ToolUsageTrackingService; @ExtendWith(MockitoExtension.class) @@ -36,6 +40,8 @@ class ToolRecommendationControllerTest { private static final String BROWSER_ID = "0f8fad5b-d9cb-469f-a165-70867728950e"; + private static final String BASE_PATH = "/api/v1/proprietary/ui-data/tool-recommendations"; + /** One input document that has already been through compress. */ private static final List> CHAIN = List.of(List.of("compress")); @@ -46,12 +52,16 @@ class ToolRecommendationControllerTest { @Mock private UserServiceInterface userService; private ToolRecommendationController controller; + private MockMvc mockMvc; @BeforeEach void setUp() { controller = new ToolRecommendationController( trackingService, recommendationService, Optional.of(userService)); + mockMvc = MockMvcBuilders.standaloneSetup(controller).build(); + // Lenient: the 400 and 404 cases never reach the consent check. + lenient().when(trackingService.isRecordingEnabled()).thenReturn(true); } @Nested @@ -137,36 +147,47 @@ class ToolRecommendationControllerTest { verify(trackingService).recordUsage("alice", "ocr", JUNK_CHAIN); } + + @Test + @DisplayName("an install that declined tracking answers 501 so the client stops posting") + void declinedInstallReturns501() { + when(trackingService.isRecordingEnabled()).thenReturn(false); + + ResponseEntity response = + controller.recordUsage(new UsageRequest("ocr", CHAIN), null); + + assertThat(response.getStatusCode().value()).isEqualTo(501); + verify(trackingService, never()).recordUsage(anyString(), anyString(), any()); + } } @Nested - @DisplayName("GET workflows") - class GetWorkflows { + @DisplayName("Cross-principal exposure") + class CrossPrincipalExposure { + /** The chain queries carry no tenant predicate, so no HTTP route may reach them. */ @Test - @DisplayName("returns the repeated workflows for the resolved principal") - void returnsWorkflows() { - when(userService.getCurrentUsername()).thenReturn("alice"); - ToolWorkflow workflow = - new ToolWorkflow(List.of("compress", "ocr"), 4, WorkflowScope.USER); - when(recommendationService.getWorkflows("alice", 2, 6)).thenReturn(List.of(workflow)); + @DisplayName("no route serves other principals' chains") + void workflowsRouteIsGone() throws Exception { + mockMvc.perform(get(BASE_PATH + "/workflows").param("minLength", "2")) + .andExpect(status().isNotFound()); - ResponseEntity response = controller.getWorkflows(2, 6, null); - - assertThat(response.getBody().workflows()).containsExactly(workflow); + verifyNoInteractions(recommendationService); } @Test - @DisplayName("a failure degrades to an empty list rather than breaking the caller") - void failureDegrades() { - when(userService.getCurrentUsername()).thenReturn("alice"); - when(recommendationService.getWorkflows(anyString(), anyInt(), anyInt())) - .thenThrow(new RuntimeException("db down")); + @DisplayName("a spoofed browser id cannot read the logged-in caller out of their own scope") + void browserIdCannotOverrideLoggedInPrincipal() throws Exception { + when(userService.getCurrentUsername()).thenReturn("bob"); + when(recommendationService.getRecommendations(eq("bob"), isNull(), anyInt())) + .thenReturn(List.of()); - ResponseEntity response = controller.getWorkflows(2, 6, null); + mockMvc.perform(get(BASE_PATH).header("X-Browser-Id", BROWSER_ID)) + .andExpect(status().isOk()); - assertThat(response.getStatusCode().value()).isEqualTo(200); - assertThat(response.getBody().workflows()).isEmpty(); + verify(recommendationService).getRecommendations("bob", null, 6); + verify(recommendationService, never()) + .getRecommendations(eq("anon:" + BROWSER_ID), any(), anyInt()); } } diff --git a/frontend/editor/src/core/api/toolRecommendations.test.ts b/frontend/editor/src/core/api/toolRecommendations.test.ts index df3f97c921..fa87cc555f 100644 --- a/frontend/editor/src/core/api/toolRecommendations.test.ts +++ b/frontend/editor/src/core/api/toolRecommendations.test.ts @@ -3,7 +3,6 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; import apiClient from "@app/services/apiClient"; import { fetchToolRecommendations, - fetchToolWorkflows, recordToolUsage, resetToolRecommendationsAvailabilityForTests, } from "@app/api/toolRecommendations"; @@ -19,6 +18,10 @@ const http404 = Object.assign(new Error("not found"), { response: { status: 404 }, }); +const http501 = Object.assign(new Error("not implemented"), { + response: { status: 501 }, +}); + describe("toolRecommendations api", () => { beforeEach(() => { vi.clearAllMocks(); @@ -71,33 +74,6 @@ describe("toolRecommendations api", () => { }); }); - describe("fetchToolWorkflows", () => { - it("returns repeated workflows and passes the filters", async () => { - mockGet.mockResolvedValue({ - data: { - workflows: [ - { tools: ["compress", "watermark"], count: 4, scope: "USER" }, - ], - }, - }); - - const result = await fetchToolWorkflows(3, 10); - - expect(result).toEqual([ - { tools: ["compress", "watermark"], count: 4, scope: "USER" }, - ]); - const url = mockGet.mock.calls[0][0] as string; - expect(url).toContain("minLength=3"); - expect(url).toContain("limit=10"); - }); - - it("returns null on failure", async () => { - mockGet.mockRejectedValue(new Error("network down")); - - expect(await fetchToolWorkflows()).toBeNull(); - }); - }); - describe("recordToolUsage", () => { it("posts the tool and each input document's prior chain", async () => { mockPost.mockResolvedValue({}); @@ -136,5 +112,14 @@ describe("toolRecommendations api", () => { expect(mockPost).toHaveBeenCalledTimes(1); }); + + it("stops posting after a 501 from an install that declined tracking", async () => { + mockPost.mockRejectedValue(http501); + + await recordToolUsage("ocr"); + await recordToolUsage("ocr"); + + expect(mockPost).toHaveBeenCalledTimes(1); + }); }); }); diff --git a/frontend/editor/src/core/api/toolRecommendations.ts b/frontend/editor/src/core/api/toolRecommendations.ts index f8587a0649..b71f8272eb 100644 --- a/frontend/editor/src/core/api/toolRecommendations.ts +++ b/frontend/editor/src/core/api/toolRecommendations.ts @@ -69,37 +69,3 @@ export async function recordToolUsage( markUnavailableOn404(error); } } - -/** Where a repeated workflow was observed. */ -export type WorkflowScope = "USER" | "TEAM" | "GLOBAL"; - -export interface ToolWorkflowDto { - tools: string[]; - count: number; - scope: WorkflowScope; -} - -/** - * Tool sequences applied to the same document over and over - the basis for - * suggesting an automation. Null when the backend cannot serve them. - */ -export async function fetchToolWorkflows( - minLength = 2, - limit = 6, -): Promise { - if (backendUnavailable) return null; - try { - const params = new URLSearchParams({ - minLength: String(minLength), - limit: String(limit), - }); - const response = await apiClient.get<{ workflows: ToolWorkflowDto[] }>( - `${BASE_PATH}/workflows?${params}`, - { suppressErrorToast: true, skipAuthRedirect: true }, - ); - return response.data?.workflows ?? []; - } catch (error) { - markUnavailableOn404(error); - return null; - } -} diff --git a/frontend/editor/src/core/hooks/tools/shared/useToolOperation.ts b/frontend/editor/src/core/hooks/tools/shared/useToolOperation.ts index 02ba298a7c..64ab5d25f4 100644 --- a/frontend/editor/src/core/hooks/tools/shared/useToolOperation.ts +++ b/frontend/editor/src/core/hooks/tools/shared/useToolOperation.ts @@ -445,6 +445,8 @@ export const useToolOperation = ( // Set by both branches so the usage tracker can move each document's // tool chain from the inputs onto the outputs that replaced them. let producedFileIds: FileId[] = []; + // An input that failed stays in the workbench, so it must keep its chain. + let consumedInputIds: FileId[] = []; if (isVersionOp) { // Output is a modified version of the input — link it to the input's version chain. @@ -493,6 +495,7 @@ export const useToolOperation = ( const toConsumeInputIds = successSourceIds.filter((id) => inputFileIds.includes(id), ); + consumedInputIds = toConsumeInputIds; console.debug("[useToolOperation] Consuming files (version)", { inputCount: inputFileIds.length, toConsume: toConsumeInputIds.length, @@ -557,6 +560,7 @@ export const useToolOperation = ( const toConsumeInputIds = successSourceIds.filter((id) => inputFileIds.includes(id), ); + consumedInputIds = toConsumeInputIds; console.debug("[useToolOperation] Consuming files (independent)", { inputCount: inputFileIds.length, toConsume: toConsumeInputIds.length, @@ -598,7 +602,9 @@ export const useToolOperation = ( // the outputs that replaced it. notifyToolCompleted({ toolId: config.operationType, - inputs: inputStirlingFileStubs, + inputs: inputStirlingFileStubs.filter((stub) => + consumedInputIds.includes(stub.id), + ), outputFileIds: producedFileIds, }); } diff --git a/frontend/editor/src/core/services/toolUsageTracker.test.ts b/frontend/editor/src/core/services/toolUsageTracker.test.ts index d95a4e9b51..43a8b46157 100644 --- a/frontend/editor/src/core/services/toolUsageTracker.test.ts +++ b/frontend/editor/src/core/services/toolUsageTracker.test.ts @@ -109,6 +109,15 @@ describe("toolUsageTracker", () => { expect(getDocumentToolChain(uploaded("p2"))).toEqual(["compress", "split"]); }); + it("leaves an input the tool never consumed with its chain intact", () => { + run("compress", [uploaded("a")], ["a2"]); + run("compress", [uploaded("b")], ["b2"]); + // Only a2 succeeded, so b2 stays in the workbench and keeps its lineage. + run("ocr", [uploaded("a2")], ["a3"]); + + expect(getDocumentToolChain(uploaded("b2"))).toEqual(["compress"]); + }); + it("prefers the tracked chain over a stale persisted history", () => { run("compress", [uploaded("a")], ["a2"]);