Stop the assistant special-casing one provider by name for a credential
resolveProviderAuthorization used to name "InternalOllama" as the only provider that could be used from the assistant without a credential, and reject every other one with 409 - including a provider that plainly declares requiresAuthorization() false, such as a credential-free remote Ollama. The rule is now exactly that capability: !requiresAuthorization() means no credential is asked for, whatever the provider is called. The now-dead INTERNAL_PROVIDER_NAME constant goes with it - nothing else referenced it. AssistantSelectionResolverTest is new: this method had never been tested in isolation, only indirectly through AssistantControllerTest, which never exercised a credential-free provider under any name but the one that used to be hardcoded. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
parent
5bd2caf884
commit
dec3295d6d
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -0,0 +1,113 @@
|
|||
// SPDX-FileCopyrightText: 2025-2026 Lucio Lelii <lucio.lelii@isti.cnr.it> - 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<String> 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);
|
||||
}
|
||||
|
||||
}
|
||||
Loading…
Reference in New Issue