-
Notifications
You must be signed in to change notification settings - Fork 2.1k
[BUG] fix chroma api auth #4571
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
Conversation
Signed-off-by: Philip I. Thomas <[email protected]>
774f714 to
7f7940c
Compare
|
@philipithomas Thanks for the PR! @Kehrlann Could you review the changes please? Thanks! |
|
@Kehrlann Happy to help set up a free Chroma Cloud org for E2E testing, whether manual or automated - my email is philip at trychroma.com. |
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.
Thanks for the contribution @philipithomas ! The fix seems correct.
[question] I'm a bit confused as to why the TokenSecuredChromaWhereIT does not fail with the current setup. Can you try it on your machine, and try to reproduce?
Is it because it's an outdated Chroma image?
It'd be good to have this correctly covered by tests.
[suggestion, non-blocking] One nice-to-have, good opportunity for a small refactoring. Instead of capturing the token in ChromApi#withToken, we do something similar to ChromaApi#withBasicAuthCredentials, something like:
public ChromaApi withKeyToken(String keyToken) {
this.restClient = this.restClient.mutate()
.defaultHeader(X_CHROMA_TOKEN, keyToken)
.build();
return this;
}And we remove both the keyToken field and the httpHeader method.
| private void httpHeaders(HttpHeaders headers) { | ||
| if (StringUtils.hasText(this.keyToken)) { | ||
| headers.setBearerAuth(this.keyToken); | ||
| headers.set("x-chroma-token", this.keyToken); |
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.
Consider putting x-chroma-token in a static field.
|
@philipithomas Could you check the latest review comments? Thanks |
I believe that the server does not support that environment variable, so it's unauthenticated and responding to the request. |
Signed-off-by: Philip I. Thomas <[email protected]>
1ed555a to
ddbe8fc
Compare
|
@ilayaperumalg Just patched it - ready for re-review |
Got it, too bad then. |
|
@philipithomas Thanks for the PR and @Kehrlann thanks for the review. Rebased, squashed and merged as 0abfedf |
|
Hi @philipithomas ,
Hope it will be soon pushed into maven repo as well. Thanks |
|
@Jayaprabahar Thanks for testing and update! This will be part of 1.1.0-RC1. |
Philip here from the Chroma team.
This SDK currently authenticates via a bearer token.
However, Chroma uses the
x-chroma-tokenheader instead of bearer, leading all authenticated requests with this SDK to fail with a 401 error. (Details in the Chroma OpenAPI spec: https://api.trychroma.com/docs/ )I've patched the authentication method in this PR.