-
-
Notifications
You must be signed in to change notification settings - Fork 327
feat: Add support for exchanging oauth code for access token #780
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
05950c3
374cdf4
c56462c
7acda1d
b14d9bb
c0b5dad
ca9de41
70201d5
f3223da
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 |
|---|---|---|
|
|
@@ -258,6 +258,31 @@ impl DeviceCodes { | |
| } | ||
| } | ||
|
|
||
| /// 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( | ||
| crab: &crate::Octocrab, | ||
| client_id: &SecretString, | ||
| code: String, | ||
| client_secret: &SecretString, | ||
| redirect_uri: String, | ||
| ) -> Result<OAuth> { | ||
| let data: OAuth = crab | ||
| .post( | ||
| "/login/oauth/access_token", | ||
| Some(&ExchangeCodeForTokenParams { | ||
| client_id: client_id.expose_secret(), | ||
| client_secret: client_secret.expose_secret(), | ||
| code: &code, | ||
| redirect_uri: &redirect_uri, | ||
| }), | ||
| ) | ||
| .await?; | ||
|
|
||
| Ok(data) | ||
| } | ||
|
|
||
| /// See https://docs.github.com/en/developers/apps/building-oauth-apps/authorizing-oauth-apps#input-parameters | ||
| #[derive(Serialize)] | ||
| struct DeviceFlow<'a> { | ||
|
|
@@ -301,3 +326,15 @@ struct PollForDevice<'a> { | |
| /// Required. The grant type must be urn:ietf:params:oauth:grant-type:device_code. | ||
| grant_type: &'static str, | ||
| } | ||
|
|
||
| #[derive(Serialize)] | ||
| struct ExchangeCodeForTokenParams<'a> { | ||
| /// Required. The client ID for your GitHub App. | ||
| client_id: &'a str, | ||
| /// Required. The client secret for your GitHub App. | ||
| client_secret: &'a str, | ||
| /// Required. The code received from the POST <https://github.com/login/oauth/authorize?client_id=CLIENT_ID> request. | ||
| code: &'a str, | ||
| // Strongly recommended. The URL in your application where users will be sent after authorization. | ||
| redirect_uri: &'a str, | ||
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| mod mock_error; | ||
|
|
||
| use mock_error::setup_error_handler; | ||
| use octocrab::{auth, Octocrab}; | ||
| use secrecy::SecretString; | ||
| use serde_json::json; | ||
| use wiremock::{ | ||
| matchers::{method, path}, | ||
| Mock, MockServer, ResponseTemplate, | ||
| }; | ||
|
|
||
| async fn setup_post_api(template: ResponseTemplate) -> MockServer { | ||
| let mock_server = MockServer::start().await; | ||
|
|
||
| Mock::given(method("POST")) | ||
| .and(path(format!("/login/oauth/access_token"))) | ||
| .respond_with(template.clone()) | ||
| .mount(&mock_server) | ||
| .await; | ||
|
|
||
| setup_error_handler( | ||
| &mock_server, | ||
| &format!("POST on /login/oauth/access_token was not received"), | ||
| ) | ||
| .await; | ||
| mock_server | ||
| } | ||
|
|
||
| fn setup_octocrab(uri: &str) -> Octocrab { | ||
| Octocrab::builder().base_uri(uri).unwrap().build().unwrap() | ||
| } | ||
|
|
||
| const CLIENT_SECRET: &str = "some_secret"; | ||
| const CLIENT_ID: &str = "some_client_id"; | ||
| const CODE: &str = "a_code"; | ||
| const REDIRECT_URI: &str = "https://yourapp/auth/callback-example"; | ||
|
|
||
| #[tokio::test] | ||
| async fn should_return_access_token() { | ||
| let expected_response = json!({ | ||
| "access_token":"gho_16C7e42F292c6912E7710c838347Ae178B4a", | ||
| "scope":"repo,gist", | ||
| "token_type":"bearer" | ||
| } | ||
| ); | ||
| let template = ResponseTemplate::new(201).set_body_json(expected_response); | ||
| let mock_server = setup_post_api(template).await; | ||
| let client = setup_octocrab(&mock_server.uri()); | ||
|
|
||
| let result = auth::get_access_token( | ||
| &client, | ||
| &SecretString::from(CLIENT_ID), | ||
| CODE.to_owned(), | ||
| &SecretString::from(CLIENT_SECRET), | ||
| REDIRECT_URI.to_owned(), | ||
| ) | ||
| .await; | ||
|
|
||
| assert!(result.is_ok()); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn should_fail_when_receving_a_server_error() { | ||
| let template = ResponseTemplate::new(500); | ||
| let mock_server = setup_post_api(template).await; | ||
| let client = setup_octocrab(&mock_server.uri()); | ||
|
|
||
| let result = auth::get_access_token( | ||
| &client, | ||
| &SecretString::from(CLIENT_ID), | ||
| CODE.to_owned(), | ||
| &SecretString::from(CLIENT_SECRET), | ||
| REDIRECT_URI.to_owned(), | ||
| ) | ||
| .await; | ||
|
|
||
| assert!(result.is_err()); | ||
| } |
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 there a reason this is a free function as opposed to a impl method?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this look in octokit.js? We try to follow similar conventions when possible.
Uh oh!
There was an error while loading. Please reload this page.
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.
Ok, it seems that in octokit is defined as a struct:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Uh oh!
There was an error while loading. Please reload this page.
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.
There are two options (GitHub App is the recommended one):
https://github.com/octokit/oauth-methods.js/blob/main/src/exchange-web-flow-code.ts
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
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.
Looks great to me! @XAMPPRocky ptal