-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
feat(auth): allow Token or Bearer as valid schemes #25397
Conversation
b7eefbe
to
57476d1
Compare
influxdb3/tests/server/auth.rs
Outdated
@@ -143,7 +143,8 @@ async fn auth_grpc() { | |||
|
|||
// Set the authorization header on the client: | |||
client | |||
.add_header(header, &format!("Bearer {TOKEN}")) |
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.
Shouldn't we also have a test to check that Bearer works?
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.
Almost all the other tests use Bearer
, except for this one which I changed to use Token
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.
This test should still pass even without this change, I tried testing locally (without this, but keeping the changes in influxdb3_server/src/http.rs
) and all tests pass, so I think CI is being flaky. The errors look connection related during the E2E tests, so maybe just try re-running CI? Not really sure how to protect from those flaky failures...
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.
As an aside, this actually isn't testing the code you added, since this test is using the gRPC server, which handles token extraction in the core authz
crate here: https://github.com/influxdata/influxdb3_core/blob/1eaa4ed5ea147bc24db98d9686e457c124dfd5b7/authz/src/lib.rs#L27-L44
If you want to test the new code you added, you would need to check using the HTTP server, e.g., in this test:
influxdb/influxdb3/tests/server/auth.rs
Lines 8 to 9 in c4514bf
#[tokio::test] | |
async fn auth() { |
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 @hiltontj - given both http 2/http 1 runs on same port, I wasn't sure if auth handling was shared. I'll update the test.
57476d1
to
c2a3c99
Compare
closes: #25394