-
Notifications
You must be signed in to change notification settings - Fork 94
chore: write credentials to a file #1320
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?
Conversation
Add functionality to write user credentials to ~/.aws/amazonq/creds.json when a user logs in or when their credentials are refreshed. This allows other tools to access the authentication information. 🤖 Assisted by [Amazon Q Developer](https://aws.amazon.com/q/developer)
Add unit tests to verify that credentials are correctly written to ~/.aws/amazonq/creds.json for both BuilderId and IamIdentityCenter token types. Tests ensure that the file contains the correct token information. 🤖 Assisted by [Amazon Q Developer](https://aws.amazon.com/q/developer)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1320 +/- ##
==========================================
- Coverage 14.36% 14.34% -0.02%
==========================================
Files 2368 2368
Lines 206348 206405 +57
Branches 186712 186769 +57
==========================================
- Hits 29633 29605 -28
- Misses 175257 175343 +86
+ Partials 1458 1457 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Not entirely sure why the lock file was changed here since it does not seem like you had pulled in new dependencies. When it becomes out of sync for whatever reason, checkout the version of the lock file from main with |
crates/fig_auth/src/builder_id.rs
Outdated
@@ -537,14 +537,73 @@ pub async fn poll_create_token( | |||
} | |||
} | |||
|
|||
/// Write credentials to ~/.aws/amazonq/creds.json | |||
pub fn write_credentials_to_file(token: &BuilderIdToken) -> Result<()> { |
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.
Since the callee of this function already has an async scope we should probably subscribe to the same IO behavior here and make this async here as well. Otherwise we would be blocking the executing thread for as long as the file write takes.
crates/fig_auth/src/builder_id.rs
Outdated
}); | ||
|
||
// Ensure ~/.aws/amazonq directory exists | ||
let mut path = PathBuf::from(std::env::var("HOME").unwrap_or_else(|_| "~".to_string())); |
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.
I don't think tilde expansion works out of the box like this.
See https://github.com/aws/amazon-q-developer-cli/blob/main/crates/fig_desktop_api/src/util.rs#L10-L17
…into feature/write-creds
…into feature/write-creds
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.
I think we might be missing handling for auth through the desktop app. I'll check and get back to you.
crates/q_cli/src/cli/user.rs
Outdated
@@ -244,6 +247,13 @@ pub async fn login_interactive(args: LoginArgs) -> Result<()> { | |||
]); | |||
registration.finish(&client, Some(&secret_store)).await?; | |||
fig_telemetry::send_user_logged_in().await; | |||
|
|||
// Write credentials to file after successful login | |||
if let Ok(Some(_)) = fig_auth::builder_id_token().await { |
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 aren't logging the failure case
crates/q_cli/src/cli/user.rs
Outdated
@@ -306,6 +316,12 @@ async fn try_device_authorization( | |||
PollCreateToken::Complete(_) => { | |||
fig_telemetry::send_user_logged_in().await; | |||
spinner.stop_with_message("Logged in successfully".into()); | |||
|
|||
// Write credentials to file after successful device authorization | |||
if let Ok(Some(_)) = fig_auth::builder_id_token().await { |
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.
Should probably log this if it fails, a match instead of an if let
condition
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.
I can't remember if error level messages are output to the terminal by default, can you test to see if it's user facing and let me know?
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.
error!()
crates/fig_auth/src/builder_id.rs
Outdated
@@ -715,4 +785,153 @@ mod tests { | |||
let token = refresh_token().await.unwrap().unwrap(); | |||
println!("{:?}", token); | |||
} | |||
|
|||
fn cleanup_test_file() { |
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.
Tests are ran in parallel. I don't think you can do this because you'll encounter race conditions between tests.
Description of changes:
The purpose of this is to allow MCP servers to use credentials to make requests to AWS services, such as CodeWhisperer, which will be needed to support future features.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.