Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Dynamic Authorization in Streamable HTTP Client #700
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
Dynamic Authorization in Streamable HTTP Client #700
Changes from all commits
7d9a84a
289c03a
c99d4f7
785964e
d3f0dea
c4fb621
28ae4f7
8ab1d66
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
BTW I would expect any auth headers passed in
self.headers
to take precedence over theauth_provider
headers - in many APIs explicitly setting a value (e.g. explicitly setting the value of theAuthorization
header viaheaders
) takes precedence over implicitly inferring it. For example, I've seen that pattern often in MLflow. But will defer to the maintainers' opinion on that - in either case, it'd be good to have a test that exercises the behaviorThere 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.
Good call, I added the behaviour to not override passed in headers, and add a test case as well
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.
Dumb question, where do we verify the token is actually updated?
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.
It is really hard in this testing environment to get the headers and verify the implementation. There is a mock server which hosts a list and set tools. We create a session, and add our messages to the write stream. This is then read by our transport layer and a request is sent to the server. I could not find a way to intercept or inspect this request object to verify the headers. So I just ensured the method was being called
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.
We could try to build a custom app, that looks at the header, then calls the server, and returns the auth headers in the response headers. I will wait for the maintainer to chime in if they have better ideas on how to test this.
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.
off the cuff
I suspect you'd want to patch:
In the appropriate places to catch all calls with headers, and assert your headers from the provider are there. I think the mocks could just pass through to the original function