diff --git a/src/main/java/it/cnr/isti/workflow/manager/assistant/AssistantSelectionResolver.java b/src/main/java/it/cnr/isti/workflow/manager/assistant/AssistantSelectionResolver.java index 511d187..90762c7 100644 --- a/src/main/java/it/cnr/isti/workflow/manager/assistant/AssistantSelectionResolver.java +++ b/src/main/java/it/cnr/isti/workflow/manager/assistant/AssistantSelectionResolver.java @@ -65,15 +65,17 @@ final class AssistantSelectionResolver { .filter(Objects::nonNull).distinct().sorted().toList())); } + /** + * A provider that does not require authorization simply has none - no special case for + * {@code InternalOllama} by name, so a remote provider that also needs no key (a credential-free + * Ollama, say) is usable from the assistant exactly as it is from a flow. Previously any + * provider other than the one hardcoded name was rejected with 409 even when it plainly did not + * need a credential. + */ static String resolveProviderAuthorization(LLMProvider provider, String owner, String credentialId, UserSecretService userSecretService) { - if (FlowAssistantService.INTERNAL_PROVIDER_NAME.equalsIgnoreCase(provider.getName())) { - return null; - } if (!provider.requiresAuthorization()) { - throw new ResponseStatusException(HttpStatus.CONFLICT, - "Assistant provider " + provider.getName() - + " does not declare support for user credentials"); + return null; } if (owner == null || owner.isBlank()) { throw new ResponseStatusException(HttpStatus.UNAUTHORIZED, diff --git a/src/main/java/it/cnr/isti/workflow/manager/assistant/FlowAssistantService.java b/src/main/java/it/cnr/isti/workflow/manager/assistant/FlowAssistantService.java index 6be533d..3a66bd6 100644 --- a/src/main/java/it/cnr/isti/workflow/manager/assistant/FlowAssistantService.java +++ b/src/main/java/it/cnr/isti/workflow/manager/assistant/FlowAssistantService.java @@ -76,7 +76,6 @@ public class FlowAssistantService { private static final Logger log = LoggerFactory.getLogger(FlowAssistantService.class); - static final String INTERNAL_PROVIDER_NAME = "InternalOllama"; static final String SHARED_MEMORY_SESSION_NAME = "sharedMemorySession"; private static final int DEFAULT_PROVIDER_RETRY_ATTEMPTS = 3; private static final int DEFAULT_MAX_REPAIR_ATTEMPTS = 2; diff --git a/src/test/java/it/cnr/isti/workflow/manager/assistant/AssistantSelectionResolverTest.java b/src/test/java/it/cnr/isti/workflow/manager/assistant/AssistantSelectionResolverTest.java new file mode 100644 index 0000000..607c79b --- /dev/null +++ b/src/test/java/it/cnr/isti/workflow/manager/assistant/AssistantSelectionResolverTest.java @@ -0,0 +1,113 @@ +// 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.assistant; + +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.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 org.junit.jupiter.api.Test; +import org.springframework.web.server.ResponseStatusException; + +import it.cnr.isti.workflow.manager.llms.providers.LLMProvider; +import it.cnr.isti.workflow.manager.vault.UserSecretService; + +/** + * {@code resolveProviderAuthorization} used to special-case the name {@code "InternalOllama"} as + * the only provider usable without a credential, and reject every other one with 409 even when it + * plainly {@code requiresAuthorization() == false} - which would have rejected a credential-free + * remote Ollama or an OpenAI-compatible gateway configured with no key, from the assistant only. + * These tests pin the fix: the rule is the capability, never the name. + */ +class AssistantSelectionResolverTest { + + private final UserSecretService userSecretService = mock(UserSecretService.class); + + private LLMProvider provider(String name, boolean requiresAuthorization) { + return new LLMProvider() { + @Override + public String getName() { + return name; + } + + @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 aCredentialFreeProviderNeedsNoCredentialWhateverItIsCalled() { + // The point of the fix: this is not "InternalOllama", yet it needs nothing from the vault + // because it says so itself. + LLMProvider remoteOllamaWithNoKey = provider("RemoteOllamaOnMyLan", false); + + String authorization = AssistantSelectionResolver.resolveProviderAuthorization( + remoteOllamaWithNoKey, "alice", null, userSecretService); + + assertNull(authorization); + verify(userSecretService, never()).resolveValue(any(), any(), any()); + } + + @Test + void internalOllamaIsNoLongerASpecialCaseByName() { + // Same provider, same behaviour, reached the same way as any other credential-free + // provider - no branch keyed on this string exists anymore. + LLMProvider internalOllama = provider("InternalOllama", false); + + assertNull(AssistantSelectionResolver.resolveProviderAuthorization( + internalOllama, "alice", null, userSecretService)); + } + + @Test + void aCredentialRequiringProviderStillNeedsAnAuthenticatedUser() { + LLMProvider gemini = provider("Gemini", true); + + ResponseStatusException error = assertThrows(ResponseStatusException.class, + () -> AssistantSelectionResolver.resolveProviderAuthorization(gemini, null, "cred-1", userSecretService)); + + assertEquals(401, error.getStatusCode().value()); + } + + @Test + void aCredentialRequiringProviderStillNeedsACredentialId() { + LLMProvider gemini = provider("Gemini", true); + + ResponseStatusException error = assertThrows(ResponseStatusException.class, + () -> AssistantSelectionResolver.resolveProviderAuthorization(gemini, "alice", " ", userSecretService)); + + assertEquals(400, error.getStatusCode().value()); + } + + @Test + void aCredentialRequiringProviderResolvesThroughTheVault() { + LLMProvider gemini = provider("Gemini", true); + when(userSecretService.resolveValue("alice", "cred-1", "Gemini")).thenReturn("sk-secret"); + + String authorization = AssistantSelectionResolver.resolveProviderAuthorization( + gemini, "alice", "cred-1", userSecretService); + + assertEquals("sk-secret", authorization); + } + +}