feat: Add support for exchanging oauth code for access token#780
feat: Add support for exchanging oauth code for access token#780mattiapitossi wants to merge 9 commits intoXAMPPRocky:mainfrom
Conversation
src/auth.rs
Outdated
| /// Exchange a code for a user access token | ||
| /// | ||
| /// see: https://docs.github.com/en/developers/apps/identifying-and-authorizing-users-for-github-apps | ||
| pub async fn get_access_token( |
There was a problem hiding this comment.
Is there a reason this is a free function as opposed to a impl method?
There was a problem hiding this comment.
Hi @XAMPPRocky, TBH I didn't see any value in adding an additional structure as it was done for the device flow. In that case, it's useful as the params are received from another REST API. But if you see any case in which I'm not considering I can change this to be a method
There was a problem hiding this comment.
How does this look in octokit.js? We try to follow similar conventions when possible.
There was a problem hiding this comment.
Ok, it seems that in octokit is defined as a struct:
export type ExchangeWebFlowCodeGitHubAppOptions = {
clientType: "github-app";
clientId: string;
clientSecret: string;
code: string;
redirectUrl?: string;
request?: RequestInterface;
};
https://github.com/octokit/oauth-methods.js/blob/main/src/exchange-web-flow-code.ts
Also the redirectUri seems to be opt.
I'll update the MR and replicate this behavior
There was a problem hiding this comment.
Sorry I meant how does it look to get the access token? Like what does the actual method call look like if you used octokit?
There was a problem hiding this comment.
There are two options (GitHub App is the recommended one):
export async function exchangeWebFlowCode(
options: ExchangeWebFlowCodeOAuthAppOptions,
): Promise<ExchangeWebFlowCodeOAuthAppResponse>;
export async function exchangeWebFlowCode(
options: ExchangeWebFlowCodeGitHubAppOptions,
): Promise<ExchangeWebFlowCodeGitHubAppResponse>;
export async function exchangeWebFlowCode(
options:
| ExchangeWebFlowCodeOAuthAppOptions
| ExchangeWebFlowCodeGitHubAppOptions,
): Promise<any> {..}
https://github.com/octokit/oauth-methods.js/blob/main/src/exchange-web-flow-code.ts
There was a problem hiding this comment.
My two cents - the new method should follow the pattern of other post methods and return a Builder to set optional parameters, e.g. redirect_uri. This allows the API to evolve forward without breaking existing usages and is pleasant to use with optional parameters. https://docs.rs/octocrab/latest/octocrab/repos/releases/struct.CreateReleaseBuilder.html, for example
There was a problem hiding this comment.
Hi @XAMPPRocky and @kyle-leonhard, I've updated the changes to use a builder instead of the free function and made the redirect_uri optional.
Please could you take a look whenever you have some time? Thanks!
src/auth.rs
Outdated
| // Strongly recommended. The URL in your application where users will be sent after authorization. | ||
| redirect_uri: &'a str, |
There was a problem hiding this comment.
I think redirect_uri is only strongly recommended when directing the user to https://github.com/login/oauth/access_token to login. When calling https://github.com/login/oauth/access_token, redirect_uri is unused afaict and the documentation doesn't mention strongly recommended.
I suggest making this parameter optional.
There was a problem hiding this comment.
Thanks for the review @kyle-leonhard. I also noticed that in octokit is defined as optional (#780 (comment))
src/auth.rs
Outdated
| /// Exchange a code for a user access token | ||
| /// | ||
| /// see: https://docs.github.com/en/developers/apps/identifying-and-authorizing-users-for-github-apps | ||
| pub async fn get_access_token( |
There was a problem hiding this comment.
My two cents - the new method should follow the pattern of other post methods and return a Builder to set optional parameters, e.g. redirect_uri. This allows the API to evolve forward without breaking existing usages and is pleasant to use with optional parameters. https://docs.rs/octocrab/latest/octocrab/repos/releases/struct.CreateReleaseBuilder.html, for example
kyle-leonhard
left a comment
kyle-leonhard
left a comment
There was a problem hiding this comment.
Looks great to me! I'll use it to replace my own, bespoke implementation! Thanks for doing it!
| } | ||
| } | ||
|
|
||
| #[derive(serde::Serialize)] |
There was a problem hiding this comment.
Similar to DeviceFlow below, leave a comment linking to the input parameters. https://docs.github.com/en/apps/oauth-apps/building-oauth-apps/authorizing-oauth-apps#2-users-are-redirected-back-to-your-site-by-github is the one I found
There was a problem hiding this comment.
Added the second one, thanks!
src/auth.rs
Outdated
| } | ||
|
|
||
| #[derive(serde::Serialize)] | ||
| pub struct ExchangeWebFlowCodeBuilder<'octo, 'client_id, 'code, 'client_secret, 'redirect_uri> { |
There was a problem hiding this comment.
Add one more supported field: code_verifier: https://docs.github.com/en/apps/oauth-apps/building-oauth-apps/authorizing-oauth-apps#2-users-are-redirected-back-to-your-site-by-github and https://docs.github.com/en/apps/creating-github-apps/authenticating-with-a-github-app/generating-a-user-access-token-for-a-github-app#using-the-web-application-flow-to-generate-a-user-access-token
There was a problem hiding this comment.
What timing, PKCE support is brand new: https://github.blog/changelog/2025-07-14-pkce-support-for-oauth-and-github-app-authentication/
There was a problem hiding this comment.
Thanks @kyle-leonhard, I was still relying on the octokit.js implementation which still doesn't use new fields. In the docs, I also noticed that repository_id was also added so I added both (ca9de41)
3 participants
closes #638