Skip to content

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

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

bergjaak
Copy link
Contributor

@bergjaak bergjaak commented Apr 23, 2025

Description of changes:

  • piggy back off of the token refresh and each time write it to a file
  • This file is then read by another process (specifically, MCP servers) on the same machine
  • requests from this process is then authenticated

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.

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)
@bergjaak bergjaak requested a review from a team as a code owner April 23, 2025 01:22
@codecov-commenter
Copy link

codecov-commenter commented Apr 23, 2025

Codecov Report

Attention: Patch coverage is 0% with 63 lines in your changes missing coverage. Please review.

Project coverage is 14.34%. Comparing base (da59663) to head (e59192b).

Files with missing lines Patch % Lines
crates/fig_auth/src/builder_id.rs 0.00% 51 Missing ⚠️
crates/q_cli/src/cli/user.rs 0.00% 8 Missing ⚠️
crates/fig_util/src/directories.rs 0.00% 4 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@dingfeli
Copy link
Contributor

Not entirely sure why the lock file was changed here since it does not seem like you had pulled in new dependencies.
Our CI is run with --lock flag, which menas the lock file would need to be up to date in the commit, otherwise it would fail since the --lock flag prevents it from updating.

When it becomes out of sync for whatever reason, checkout the version of the lock file from main with git checkout main -- Cargo.lock as a baseline and run one of cargo check, cargo build or any commit that would trigger a build to update the lock file. This way all of the existing versions are preserved and you won't run into compatibility warnings like you did there.

@@ -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<()> {
Copy link
Contributor

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.

});

// Ensure ~/.aws/amazonq directory exists
let mut path = PathBuf::from(std::env::var("HOME").unwrap_or_else(|_| "~".to_string()));
Copy link
Contributor

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

Copy link
Member

@chaynabors chaynabors left a 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.

@@ -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 {
Copy link
Member

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

@@ -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 {
Copy link
Member

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

Copy link
Member

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

error!()

@@ -715,4 +785,153 @@ mod tests {
let token = refresh_token().await.unwrap().unwrap();
println!("{:?}", token);
}

fn cleanup_test_file() {
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants