-
Notifications
You must be signed in to change notification settings - Fork 1.6k
fix(auth): fix request session error #16050
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
Changes from all commits
300b417
0867d5d
99b3e2a
9a49f21
af098fb
0c28180
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 | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -166,16 +166,15 @@ def _prepare_request_for_mds(request, use_mtls=False) -> None: | |||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| Args: | ||||||||||||||||||||||||||
| request (google.auth.transport.Request): A callable used to make | ||||||||||||||||||||||||||
| HTTP requests. | ||||||||||||||||||||||||||
| HTTP requests. If mTLS is enabled, and the request supports sessions, | ||||||||||||||||||||||||||
| the request will have the mTLS adapter mounted. Otherwise, there | ||||||||||||||||||||||||||
| will be no change. | ||||||||||||||||||||||||||
| use_mtls (bool): Whether to use mTLS for the request. | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| Returns: | ||||||||||||||||||||||||||
| google.auth.transport.Request: A request object to use. | ||||||||||||||||||||||||||
| If mTLS is enabled, the request will have the mTLS adapter mounted. | ||||||||||||||||||||||||||
| Otherwise, the original request will be returned unchanged. | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||
| # Only modify the request if mTLS is enabled. | ||||||||||||||||||||||||||
| if use_mtls: | ||||||||||||||||||||||||||
| # Only modify the request if mTLS is enabled, and request supports sessions. | ||||||||||||||||||||||||||
| if use_mtls and hasattr(request, "session"): | ||||||||||||||||||||||||||
| # Ensure the request has a session to mount the adapter to. | ||||||||||||||||||||||||||
| if not request.session: | ||||||||||||||||||||||||||
| request.session = requests.Session() | ||||||||||||||||||||||||||
daniel-sanche marked this conversation as resolved.
Show resolved
Hide resolved
Comment on lines
+177
to
180
Contributor
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. Using A more robust approach is to explicitly check if
Suggested change
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
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.
Is this condition required anymore? Since we already check that session is present?
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.
Yes, I believe this is meant to attach a session if it's set to None