From 247de4485bcc852eda6fd4850acbf0bd42997d43 Mon Sep 17 00:00:00 2001 From: Lucio Lelii Date: Thu, 17 Sep 2026 12:28:12 +0200 Subject: [PATCH] Resolve a ProviderCredential everywhere a provider is called, not a string LLMCredentialResolver.resolve now returns ProviderCredential instead of a bare String, so the endpoint travels with the value from the vault all the way to the provider that needs it. Every one of the eight call sites had to change to compile - there was no way to touch only one - so all of them now pass the resolved credential straight through instead of unwrapping it first. That turned out to be the right amount of change, not more than necessary. Where an endpoint-aware provider is not actually reachable yet (the interaction simulator and the bias judge choose their descriptor after the execution already exists, and never had their authorization requirement computed up front to begin with - a separate, pre-existing gap this does not close), a missing credential fails exactly as it always did: LLMCredentialResolver still throws "Missing saved credential" when the authorizations map has no entry for the provider's key, whether the caller then unwraps .value() or keeps the whole ProviderCredential makes no difference to that failure. The only place behaviour actually changes is the success case, and only for a provider that reads the endpoint at all - every existing provider still only reads .value() through the interface's own default unwrapping, so Gemini, InternalOllama and every test stub keep behaving exactly as before. LLMCredentialResolverTest is new: this resolver was previously exercised only indirectly, through a full execution. Co-Authored-By: Claude Sonnet 5 --- .../executions/bias/BiasImpactJudge.java | 11 +- .../blocks/ChatInteractionExecutor.java | 15 +-- .../executors/blocks/ConditionalExecutor.java | 5 +- .../blocks/HumanDecisionExecutor.java | 5 +- .../blocks/HumanInteractionExecutor.java | 5 +- .../executors/blocks/LLMExecutor.java | 5 +- .../blocks/MCPAgentChatExecutor.java | 7 +- .../blocks/SimulatedChatSupport.java | 11 +- .../executors/blocks/SwitchExecutor.java | 5 +- .../manager/llms/LLMCredentialResolver.java | 10 +- .../llms/LLMCredentialResolverTest.java | 110 ++++++++++++++++++ 11 files changed, 157 insertions(+), 32 deletions(-) create mode 100644 src/test/java/it/cnr/isti/workflow/manager/llms/LLMCredentialResolverTest.java diff --git a/src/main/java/it/cnr/isti/workflow/manager/executions/bias/BiasImpactJudge.java b/src/main/java/it/cnr/isti/workflow/manager/executions/bias/BiasImpactJudge.java index 4d4cb11..10afd7c 100644 --- a/src/main/java/it/cnr/isti/workflow/manager/executions/bias/BiasImpactJudge.java +++ b/src/main/java/it/cnr/isti/workflow/manager/executions/bias/BiasImpactJudge.java @@ -30,6 +30,7 @@ import it.cnr.isti.workflow.manager.flows.model.bias.BlockBiasAnnotation; import it.cnr.isti.workflow.manager.flows.validation.ValidationErrorCode; import it.cnr.isti.workflow.manager.llms.LLMCredentialResolver; import it.cnr.isti.workflow.manager.llms.LLMDescriptor; +import it.cnr.isti.workflow.manager.llms.ProviderCredential; import it.cnr.isti.workflow.manager.llms.providers.LLMProvider; import tools.jackson.databind.JsonNode; @@ -79,7 +80,7 @@ public class BiasImpactJudge { public BiasJudgeSummary evaluate(BiasImpactReport comparison, LLMDescriptor descriptor, ExecutionObject baseline, ExecutionObject biased) { LLMProvider provider = resolveProvider(descriptor.provider()); - String authorization = resolveAuthorization(provider, baseline); + ProviderCredential authorization = resolveAuthorization(provider, baseline); String interventions = describeInterventions(biased, comparison.annotationIds()); List targets = BiasJudgeTarget.collect(comparison); @@ -117,7 +118,7 @@ public class BiasImpactJudge { errors, verdicts); } - private BiasJudgeVerdict judgeTarget(LLMProvider provider, LLMDescriptor descriptor, String authorization, + private BiasJudgeVerdict judgeTarget(LLMProvider provider, LLMDescriptor descriptor, ProviderCredential authorization, String interventions, BiasJudgeTarget target, List errors) { String prompt = pairPrompt(interventions, target); try { @@ -150,7 +151,7 @@ public class BiasImpactJudge { * this, where the answer is the real one - and the extractor below pulls the object out of * whatever prose it arrives wrapped in. */ - private String askForJson(LLMProvider provider, LLMDescriptor descriptor, String authorization, String prompt) { + private String askForJson(LLMProvider provider, LLMDescriptor descriptor, ProviderCredential authorization, String prompt) { String json = null; try { json = provider.generateJson(descriptor.model(), prompt, authorization, descriptor.parameters()); @@ -183,7 +184,7 @@ public class BiasImpactJudge { * per-pair verdicts in code, so that re-reading a report cannot show a different headline than * the pairs it is made of. */ - private String narrate(LLMProvider provider, LLMDescriptor descriptor, String authorization, String interventions, + private String narrate(LLMProvider provider, LLMDescriptor descriptor, ProviderCredential authorization, String interventions, BiasImpactReport comparison, Map verdicts, BiasJudgeImpactLevel impact, BiasJudgeAttribution attribution, List errors) { String prompt = """ @@ -504,7 +505,7 @@ public class BiasImpactJudge { * run is reached the same way as the models that produced it, and the internal provider - the * one a local install has - needs no credential at all. */ - private String resolveAuthorization(LLMProvider provider, ExecutionObject baseline) { + private ProviderCredential resolveAuthorization(LLMProvider provider, ExecutionObject baseline) { try { return credentialResolver.resolve(provider, baseline.getProvidedAuthorizations(), baseline.getContext().getResolvedExecutionVariables()); diff --git a/src/main/java/it/cnr/isti/workflow/manager/executions/executors/blocks/ChatInteractionExecutor.java b/src/main/java/it/cnr/isti/workflow/manager/executions/executors/blocks/ChatInteractionExecutor.java index ea829f7..565a7da 100644 --- a/src/main/java/it/cnr/isti/workflow/manager/executions/executors/blocks/ChatInteractionExecutor.java +++ b/src/main/java/it/cnr/isti/workflow/manager/executions/executors/blocks/ChatInteractionExecutor.java @@ -29,6 +29,7 @@ import it.cnr.isti.workflow.manager.llms.ChatMessage; import it.cnr.isti.workflow.manager.llms.LLMDescriptor; import it.cnr.isti.workflow.manager.llms.LLMDescriptorInputBinding; import it.cnr.isti.workflow.manager.llms.LLMCredentialResolver; +import it.cnr.isti.workflow.manager.llms.ProviderCredential; import it.cnr.isti.workflow.manager.llms.providers.LLMProvider; @Component @@ -65,7 +66,7 @@ public class ChatInteractionExecutor implements BlockExecutor messages = history.stream().map(this::parseHistoryLine).collect(Collectors.toList()); @@ -91,12 +92,12 @@ public class ChatInteractionExecutor implements BlockExecutor updatedHistory = new ArrayList<>(history); diff --git a/src/main/java/it/cnr/isti/workflow/manager/executions/executors/blocks/ConditionalExecutor.java b/src/main/java/it/cnr/isti/workflow/manager/executions/executors/blocks/ConditionalExecutor.java index db6c325..8ffa4de 100644 --- a/src/main/java/it/cnr/isti/workflow/manager/executions/executors/blocks/ConditionalExecutor.java +++ b/src/main/java/it/cnr/isti/workflow/manager/executions/executors/blocks/ConditionalExecutor.java @@ -27,6 +27,7 @@ import it.cnr.isti.workflow.manager.executions.bias.runtime.BiasRuntimeSupport; import it.cnr.isti.workflow.manager.llms.LLMDescriptor; import it.cnr.isti.workflow.manager.llms.LLMDescriptorInputBinding; import it.cnr.isti.workflow.manager.llms.LLMCredentialResolver; +import it.cnr.isti.workflow.manager.llms.ProviderCredential; import it.cnr.isti.workflow.manager.llms.providers.LLMProvider; @Component @@ -84,12 +85,12 @@ public class ConditionalExecutor implements BlockExecutor ExecutionEventLogger eventLogger) { LLMDescriptor llmDescriptor = LLMDescriptorInputBinding.resolve(config.getLlmDescriptor(), inputValues, executionVariables); LLMProvider llmProvider = SimulatedChatSupport.resolveProvider(llmProviders, llmDescriptor.provider()); - String authorization = credentialResolver.resolve(llmProvider, authorizations, executionVariables); + ProviderCredential credential = credentialResolver.resolve(llmProvider, authorizations, executionVariables); String prompt = buildLlmPrompt(config, inputValues, executionVariables); ModelParameterReporting.reportUnsupported(llmProvider, llmDescriptor.parameters(), llmDescriptor.model(), eventLogger); - String response = llmProvider.generate(llmDescriptor.model(), prompt, authorization, + String response = llmProvider.generate(llmDescriptor.model(), prompt, credential, llmDescriptor.parameters()); try { return parseBooleanResponse(response); diff --git a/src/main/java/it/cnr/isti/workflow/manager/executions/executors/blocks/HumanDecisionExecutor.java b/src/main/java/it/cnr/isti/workflow/manager/executions/executors/blocks/HumanDecisionExecutor.java index c9ddd21..83fcc23 100644 --- a/src/main/java/it/cnr/isti/workflow/manager/executions/executors/blocks/HumanDecisionExecutor.java +++ b/src/main/java/it/cnr/isti/workflow/manager/executions/executors/blocks/HumanDecisionExecutor.java @@ -26,6 +26,7 @@ import it.cnr.isti.workflow.manager.executions.steps.Input; import it.cnr.isti.workflow.manager.executions.bias.runtime.BiasRuntimeSupport; import it.cnr.isti.workflow.manager.llms.LLMDescriptor; import it.cnr.isti.workflow.manager.llms.LLMCredentialResolver; +import it.cnr.isti.workflow.manager.llms.ProviderCredential; import it.cnr.isti.workflow.manager.llms.providers.LLMProvider; @Component @@ -64,7 +65,7 @@ public class HumanDecisionExecutor implements BlockExecutor "%s= %s".formatted(input.getDescriptor().getName(), input.getValue())) @@ -87,7 +88,7 @@ public class HumanDecisionExecutor implements BlockExecutor { if (llmProvider == null) { throw new IllegalArgumentException("Provider not found: " + llmDescriptor.provider()); } - String authorization = credentialResolver.resolve(llmProvider, authorizations, executionVariables); + ProviderCredential credential = credentialResolver.resolve(llmProvider, authorizations, executionVariables); ModelParameterReporting.reportUnsupported(llmProvider, llmDescriptor.parameters(), llmDescriptor.model(), eventLogger); - String response = llmProvider.generate(llmDescriptor.model(), prompt, authorization, + String response = llmProvider.generate(llmDescriptor.model(), prompt, credential, llmDescriptor.parameters()); if (eventLogger != null) { diff --git a/src/main/java/it/cnr/isti/workflow/manager/executions/executors/blocks/MCPAgentChatExecutor.java b/src/main/java/it/cnr/isti/workflow/manager/executions/executors/blocks/MCPAgentChatExecutor.java index 8037176..75a2a9d 100644 --- a/src/main/java/it/cnr/isti/workflow/manager/executions/executors/blocks/MCPAgentChatExecutor.java +++ b/src/main/java/it/cnr/isti/workflow/manager/executions/executors/blocks/MCPAgentChatExecutor.java @@ -28,6 +28,7 @@ import it.cnr.isti.workflow.manager.executions.bias.runtime.BiasRuntimeSupport; import it.cnr.isti.workflow.manager.llms.ConfigurableInputBinding; import it.cnr.isti.workflow.manager.llms.LLMDescriptor; import it.cnr.isti.workflow.manager.llms.LLMCredentialResolver; +import it.cnr.isti.workflow.manager.llms.ProviderCredential; import it.cnr.isti.workflow.manager.llms.providers.LLMProvider; import it.cnr.isti.workflow.manager.mcp.MCPAgentService; import it.cnr.isti.workflow.manager.mcp.MCPSharedSessionRegistry; @@ -74,7 +75,7 @@ public class MCPAgentChatExecutor implements BlockExecutor new IllegalArgumentException("Provider not found: " + providerName)); } - static String resolveAuthorization(LLMCredentialResolver credentialResolver, LLMProvider provider, + static ProviderCredential resolveAuthorization(LLMCredentialResolver credentialResolver, LLMProvider provider, Map authorizations, Map executionVariables) { return credentialResolver.resolve(provider, authorizations, executionVariables); } @@ -72,7 +73,7 @@ final class SimulatedChatSupport { } static String generateSimulatorMessage(LLMProvider simulatorProvider, LLMDescriptor simulatorDescriptor, - String simulatorAuthorization, String goalDescription, List inputs, List history, int turn, + ProviderCredential simulatorCredential, String goalDescription, List inputs, List history, int turn, int maxTurns, String blockLabel) { String prompt = """ ###SIMULATED_CHAT_MESSAGE### @@ -89,7 +90,7 @@ final class SimulatedChatSupport { Return only the next user message for the conversation. """.formatted(goalDescription, turn, maxTurns, formatInputs(inputs), formatHistory(history)); - String response = simulatorProvider.generate(simulatorDescriptor.model(), prompt, simulatorAuthorization, + String response = simulatorProvider.generate(simulatorDescriptor.model(), prompt, simulatorCredential, simulatorDescriptor.parameters()); if (!StringUtils.hasText(response)) { throw new IllegalArgumentException("Simulated " + blockLabel + " produced an empty message"); @@ -98,7 +99,7 @@ final class SimulatedChatSupport { } static String generateSimulatorFinalResponse(LLMProvider simulatorProvider, LLMDescriptor simulatorDescriptor, - String simulatorAuthorization, String goalDescription, List inputs, List history, String blockLabel) { + ProviderCredential simulatorCredential, String goalDescription, List inputs, List history, String blockLabel) { String prompt = """ ###SIMULATED_CHAT_FINAL### Goal: @@ -112,7 +113,7 @@ final class SimulatedChatSupport { Return only the final response value that the simulated user would submit. """.formatted(goalDescription, formatInputs(inputs), formatHistory(history)); - String response = simulatorProvider.generate(simulatorDescriptor.model(), prompt, simulatorAuthorization, + String response = simulatorProvider.generate(simulatorDescriptor.model(), prompt, simulatorCredential, simulatorDescriptor.parameters()); if (!StringUtils.hasText(response)) { throw new IllegalArgumentException("Simulated " + blockLabel + " produced an empty final response"); diff --git a/src/main/java/it/cnr/isti/workflow/manager/executions/executors/blocks/SwitchExecutor.java b/src/main/java/it/cnr/isti/workflow/manager/executions/executors/blocks/SwitchExecutor.java index 934be2a..4b9723f 100644 --- a/src/main/java/it/cnr/isti/workflow/manager/executions/executors/blocks/SwitchExecutor.java +++ b/src/main/java/it/cnr/isti/workflow/manager/executions/executors/blocks/SwitchExecutor.java @@ -33,6 +33,7 @@ import it.cnr.isti.workflow.manager.ios.IOType; import it.cnr.isti.workflow.manager.llms.LLMDescriptor; import it.cnr.isti.workflow.manager.llms.LLMDescriptorInputBinding; import it.cnr.isti.workflow.manager.llms.LLMCredentialResolver; +import it.cnr.isti.workflow.manager.llms.ProviderCredential; import it.cnr.isti.workflow.manager.llms.providers.LLMProvider; @Component @@ -112,12 +113,12 @@ public class SwitchExecutor implements BlockExecutor { ExecutionEventLogger eventLogger) { LLMDescriptor llmDescriptor = LLMDescriptorInputBinding.resolve(config.getLlmDescriptor(), inputValues, executionVariables); LLMProvider llmProvider = SimulatedChatSupport.resolveProvider(llmProviders, llmDescriptor.provider()); - String authorization = credentialResolver.resolve(llmProvider, authorizations, executionVariables); + ProviderCredential credential = credentialResolver.resolve(llmProvider, authorizations, executionVariables); String prompt = buildLlmPrompt(config, inputValues, executionVariables, allowedOutputs); ModelParameterReporting.reportUnsupported(llmProvider, llmDescriptor.parameters(), llmDescriptor.model(), eventLogger); - String response = llmProvider.generate(llmDescriptor.model(), prompt, authorization, + String response = llmProvider.generate(llmDescriptor.model(), prompt, credential, llmDescriptor.parameters()); return parseSelectedOutput(response, allowedOutputs); } diff --git a/src/main/java/it/cnr/isti/workflow/manager/llms/LLMCredentialResolver.java b/src/main/java/it/cnr/isti/workflow/manager/llms/LLMCredentialResolver.java index be9046a..c3f4a85 100644 --- a/src/main/java/it/cnr/isti/workflow/manager/llms/LLMCredentialResolver.java +++ b/src/main/java/it/cnr/isti/workflow/manager/llms/LLMCredentialResolver.java @@ -23,7 +23,13 @@ public class LLMCredentialResolver { this.userSecretService = userSecretService; } - public String resolve(LLMProvider provider, Map authorizations, + /** + * Returns a {@link ProviderCredential}, carrying the endpoint alongside the secret value for a + * provider whose {@code requiresEndpoint()} is true. Not yet called from every executor: see + * {@code docs/llm-providers-openai-remote-ollama-plan-2026-09-17.md} step 11 for which call + * sites still resolve through the plain secret value only, and why. + */ + public ProviderCredential resolve(LLMProvider provider, Map authorizations, Map executionVariables) { if (!provider.requiresAuthorization()) { return null; @@ -38,7 +44,7 @@ public class LLMCredentialResolver { throw new IllegalArgumentException( "A user-owned execution is required to resolve credential for provider: " + provider.getName()); } - return userSecretService.resolveValue(owner, credentialId.trim(), provider.getName()); + return userSecretService.resolveCredential(owner, credentialId.trim(), provider.getName()); } private String asText(Object value) { diff --git a/src/test/java/it/cnr/isti/workflow/manager/llms/LLMCredentialResolverTest.java b/src/test/java/it/cnr/isti/workflow/manager/llms/LLMCredentialResolverTest.java new file mode 100644 index 0000000..2cf8ca2 --- /dev/null +++ b/src/test/java/it/cnr/isti/workflow/manager/llms/LLMCredentialResolverTest.java @@ -0,0 +1,110 @@ +// SPDX-FileCopyrightText: 2025-2026 Lucio Lelii - ISTI-CNR +// SPDX-License-Identifier: AGPL-3.0-or-later +// Attribution term under AGPL-3.0 section 7(b): see LICENSE-ADDENDUM. + +package it.cnr.isti.workflow.manager.llms; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.util.List; +import java.util.Map; + +import org.junit.jupiter.api.Test; + +import it.cnr.isti.workflow.manager.executions.ExecutionRuntimeContextSupport; +import it.cnr.isti.workflow.manager.llms.providers.LLMProvider; +import it.cnr.isti.workflow.manager.vault.UserSecretService; + +/** + * Not previously covered on its own - only ever exercised indirectly through a full execution. + * This pins the branching {@code resolve} itself does (no authorization required, no credential + * id, no owner) before it becomes even harder to isolate from what calls it. + */ +class LLMCredentialResolverTest { + + private final UserSecretService userSecretService = mock(UserSecretService.class); + private final LLMCredentialResolver resolver = new LLMCredentialResolver(userSecretService); + + private LLMProvider provider(boolean requiresAuthorization) { + return new LLMProvider() { + @Override + public String getName() { + return "Test"; + } + + @Override + public List getRegisteredModels() { + return List.of(); + } + + @Override + public String generate(String model, String prompt) { + throw new UnsupportedOperationException(); + } + + @Override + public boolean requiresAuthorization() { + return requiresAuthorization; + } + }; + } + + @Test + void returnsNullWithoutTouchingTheVaultWhenTheProviderNeedsNoAuthorization() { + ProviderCredential result = resolver.resolve(provider(false), Map.of(), Map.of()); + + assertNull(result); + verify(userSecretService, never()).resolveCredential(any(), any(), any()); + } + + @Test + void throwsWhenTheAuthorizationsMapHasNoEntryForTheProvidersKey() { + LLMProvider provider = provider(true); + + assertThrows(IllegalArgumentException.class, + () -> resolver.resolve(provider, Map.of(), Map.of(ExecutionRuntimeContextSupport.EXECUTION_OWNER, "alice"))); + } + + @Test + void throwsWhenTheCredentialIdIsBlank() { + LLMProvider provider = provider(true); + Map authorizations = Map.of(provider.authorizationKey(), " "); + + assertThrows(IllegalArgumentException.class, + () -> resolver.resolve(provider, authorizations, + Map.of(ExecutionRuntimeContextSupport.EXECUTION_OWNER, "alice"))); + } + + @Test + void throwsWhenThereIsNoOwnedExecutionContext() { + LLMProvider provider = provider(true); + Map authorizations = Map.of(provider.authorizationKey(), "secret-1"); + + assertThrows(IllegalArgumentException.class, () -> resolver.resolve(provider, authorizations, Map.of())); + } + + @Test + void resolvesThroughUserSecretServiceAndReturnsItsCredentialAsIs() { + LLMProvider provider = provider(true); + Map authorizations = Map.of(provider.authorizationKey(), " secret-1 "); + Map executionVariables = Map.of(ExecutionRuntimeContextSupport.EXECUTION_OWNER, "alice"); + ProviderCredential expected = new ProviderCredential("api-key", "https://gateway.example.com"); + when(userSecretService.resolveCredential("alice", "secret-1", "Test")).thenReturn(expected); + + ProviderCredential result = resolver.resolve(provider, authorizations, executionVariables); + + assertEquals(expected, result); + // The id is trimmed before being used - a pasted value with surrounding whitespace must + // still resolve. + verify(userSecretService).resolveCredential(eq("alice"), eq("secret-1"), eq("Test")); + } + +}