fix(oauth): remove redundant client_id from token exchange request body#4783
fix(oauth): remove redundant client_id from token exchange request body#4783joar wants to merge 1 commit intogoogle:mainfrom
Conversation
adk-bot
added
the
core
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the OAuth token exchange process by removing a redundant client_id parameter from the fetch_token call. This change corrects an issue where client_id was being sent unnecessarily in the request body, leading to authentication failures with certain OAuth providers and duplicate parameters for others. By relying on Authlib's built-in handling of client_id based on the authentication method, the system now adheres more closely to OAuth specifications and improves interoperability. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly removes the redundant client_id parameter from the fetch_token call during the OAuth2 authorization code exchange. The change relies on authlib to handle client authentication, fixing an interoperability issue with some OAuth providers. The changes are correct and the associated unit test has been updated accordingly. For future improvement, consider adding unit tests that cover different token_endpoint_auth_method values (e.g., client_secret_post) to more robustly verify the fix and prevent regressions.
joar
force-pushed
the
fix/oauth-token-exchange-client-id
branch
from
854c784 to
d2f5b60
Compare
Authlib's OAuth2Session already handles client_id placement for all token_endpoint_auth_method values (client_secret_basic puts it in the Authorization header, client_secret_post and none put it in the body). Passing client_id as a kwarg to fetch_token causes it to be added to the POST body unconditionally via prepare_token_request, in addition to whatever the auth method does. This breaks providers like Slack that infer auth method from the presence of client_id in the body, and causes duplicate client_id for client_secret_post. Partially reverts f273517 (PR google#2805), which was based on a misreading of RFC 6749 §4.1.3 — client_id in the body is only REQUIRED "if the client is not authenticating with the authorization server as described in Section 3.2.1." Made-with: Cursor
joar
force-pushed
the
fix/oauth-token-exchange-client-id
branch
from
7b18b09 to
e527317
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses an issue where a redundant client_id was being sent in the token exchange request body. The change removes the explicit client_id parameter from the fetch_token call, correctly relying on the authlib library to handle client authentication as per the configured token_endpoint_auth_method. This resolves interoperability problems with OAuth providers that are strict about the token request format. The corresponding unit tests have been updated to reflect this change. The fix is well-reasoned and correctly implemented.
2 participants
fixes #4782
Summary
client_idkwarg from thefetch_tokencall in_exchange_authorization_code. Authlib'sOAuth2Sessionalready handlesclient_idplacement for alltoken_endpoint_auth_methodvalues, so passing it as a kwarg causes it to be added to the POST body unconditionally (viaprepare_token_request), in addition to whatever the configured auth method does.client_idin the body: when usingclient_secret_basic,client_idappeared in both the Authorization header and the body, causing Slack to ignore Basic auth and reject the request with "bad client auth."client_secret_post, the extra kwarg causedclient_idto appear twice in the body.Background
This partially reverts PR #2805 / commit f273517, which added
client_idto the token request to fix #2806. That change was based on a misreading of RFC 6749 §4.1.3, which states:The OAuth 2.1 draft (§3.2.2) further clarifies:
Authlib already handles all of these cases via its auth method encoders:
client_secret_basic→client_id:client_secretinAuthorization: Basicheaderclient_secret_post→client_idandclient_secretappended to bodynone(public client) →client_idappended to bodyADK does not need to pass
client_idexplicitly.