-
Notifications
You must be signed in to change notification settings - Fork 83
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Putting Generic Authentication flow to the Core #694
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
package io.quarkiverse.langchain4j.runtime.auth; | ||
|
||
import java.net.URI; | ||
import java.util.List; | ||
import java.util.Map; | ||
import java.util.Optional; | ||
|
||
import jakarta.enterprise.inject.Instance; | ||
import jakarta.enterprise.inject.spi.CDI; | ||
|
||
import io.quarkiverse.langchain4j.ModelName; | ||
|
||
public interface ModelAuthProvider { | ||
String getAuthorization(Input input); | ||
|
||
interface Input { | ||
String method(); | ||
|
||
URI uri(); | ||
|
||
Map<String, List<Object>> headers(); | ||
} | ||
|
||
static Optional<ModelAuthProvider> resolve(String modelName) { | ||
Instance<ModelAuthProvider> beanInstance = modelName == null | ||
? CDI.current().select(ModelAuthProvider.class) | ||
: CDI.current().select(ModelAuthProvider.class, ModelName.Literal.of(modelName)); | ||
|
||
//get the first one without causing a bean1 resolution exception | ||
ModelAuthProvider authorizer = null; | ||
for (var handle : beanInstance.handles()) { | ||
authorizer = handle.get(); | ||
break; | ||
} | ||
return Optional.ofNullable(authorizer); | ||
} | ||
|
||
static Optional<ModelAuthProvider> resolve() { | ||
return resolve(null); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -58,6 +58,7 @@ | |
import dev.ai4j.openai4j.moderation.ModerationRequest; | ||
import dev.ai4j.openai4j.moderation.ModerationResponse; | ||
import io.quarkiverse.langchain4j.QuarkusJsonCodecFactory; | ||
import io.quarkiverse.langchain4j.runtime.auth.ModelAuthProvider; | ||
import io.quarkus.rest.client.reactive.ClientExceptionMapper; | ||
import io.smallrye.mutiny.Multi; | ||
import io.smallrye.mutiny.Uni; | ||
|
@@ -182,35 +183,25 @@ public boolean test(SseEvent<String> event) { | |
} | ||
} | ||
|
||
interface AuthProvider { | ||
String getAuthorization(Input input); | ||
|
||
interface Input { | ||
String method(); | ||
|
||
URI uri(); | ||
|
||
MultivaluedMap<String, Object> headers(); | ||
} | ||
} | ||
|
||
class OpenAIRestAPIFilter implements ResteasyReactiveClientRequestFilter { | ||
AuthProvider authorizer; | ||
ModelAuthProvider authorizer; | ||
|
||
public OpenAIRestAPIFilter(AuthProvider authorizer) { | ||
public OpenAIRestAPIFilter(ModelAuthProvider authorizer) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMHO it should be aligned with how it is done for Vertex AI Gemini We confirmed with Georgios that using ManagedExecutot helps in the secure-fraud-detection demo There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point! |
||
this.authorizer = authorizer; | ||
} | ||
|
||
@Override | ||
public void filter(ResteasyReactiveClientRequestContext requestContext) { | ||
requestContext.getHeaders().putSingle("Authorization", authorizer.getAuthorization( | ||
new AuthInputImpl(requestContext.getMethod(), requestContext.getUri(), requestContext.getHeaders()))); | ||
requestContext | ||
.getHeaders() | ||
.putSingle("Authorization", authorizer.getAuthorization(new AuthInputImpl(requestContext.getMethod(), | ||
requestContext.getUri(), requestContext.getHeaders()))); | ||
} | ||
|
||
private record AuthInputImpl( | ||
String method, | ||
URI uri, | ||
MultivaluedMap<String, Object> headers) implements AuthProvider.Input { | ||
MultivaluedMap<String, Object> headers) implements ModelAuthProvider.Input { | ||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @csotiriou @geoand, only a couple of questions/suggestions for you to consider.
I was thinking, can we expect situations going forward where local models will require a secure token access, and where a token is passed, for example, as a property ?
I guess one can say that if
Input
is null then do not return aBearer
prefix in the response... This is related to the next question...The other question, so
ModelAuthProvider
is expected to return a complete HTTPAuthorization
header value, including the scheme,Bearer <token>
, instead of only<token>
. In #708, moving toModelAuthProvider
has an impact of having to update the test filter to includeBearer
- I don't think it is a breaking change though, since the currentAuthProvider
is not public.I wonder, should we expect providers returns the token only, and then filters, depending on a context (access to the remote or local model) will either add
Bearer
or not.IMHO it might be a bit better, and a little bit simpler for custom auth providers too...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to worry about that now, it's very early days to be able to know exactly how systems using auth will evolve.
For now, I think the proposal in this PR is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@geoand Sure, Ok then, this PR works well for #708 so it is good to go from my perspective, thanks