delete telemetry and add portal res enum

evanliu048 · evanliu048 · commit 41bda6e16db5 · 2025-10-22T14:37:40.000-07:00
diff --git a/crates/chat-cli/src/auth/builder_id.rs b/crates/chat-cli/src/auth/builder_id.rs
@@ -210,7 +210,6 @@ impl DeviceRegistration {
         secret_store
             .set_secret(Self::SECRET_KEY, &serde_json::to_string(&self)?)
             .await?;
-        info!("save builderid token");
         Ok(())
     }
 }
diff --git a/crates/chat-cli/src/auth/index.html b/crates/chat-cli/src/auth/index.html
@@ -161,7 +161,7 @@ <h4>Request denied</h4>
                     return
                 }
 
-                const productName = 'KIRO CLI'
+                const productName = 'Amazon Q for CLI'
                 document.getElementById(
                     'approvalMessage'
                 ).innerText = `${productName} have been given requested permissions`
diff --git a/crates/chat-cli/src/auth/pkce.rs b/crates/chat-cli/src/auth/pkce.rs
@@ -319,14 +319,6 @@ impl PkceRegistration {
             None => Err(AuthError::OAuthMissingCode),
         }
     }
-
-    pub fn issuer_url(&self) -> &str {
-        &self.issuer_url
-    }
-
-    pub fn region(&self) -> &Region {
-        &self.region
-    }
 }
 
 type CodeSender = std::sync::Arc<tokio::sync::mpsc::Sender<Result<(String, String), AuthError>>>;
diff --git a/crates/chat-cli/src/auth/portal.rs b/crates/chat-cli/src/auth/portal.rs
@@ -46,15 +46,21 @@ const USER_AGENT: &str = "Kiro-CLI";
 struct AuthPortalCallback {
     login_option: String,
     code: Option<String>,
-    issuer_uri: Option<String>,
+    issuer_url: Option<String>,
     sso_region: Option<String>,
     state: String,
     path: String,
 }
 
 pub enum PortalResult {
+    /// User authenticated with social provider (Google/GitHub)
     Social(SocialProvider),
-    Internal { issuer_uri: String, idc_region: String },
+    /// User selected BuilderID authentication
+    BuilderId { issuer_url: String, idc_region: String },
+    /// User selected AWS Identity Center authentication
+    AwsIdc { issuer_url: String, idc_region: String },
+    /// User selected internal authentication (Amazon-only)
+    Internal { issuer_url: String, idc_region: String },
 }
 
 /// Local-only: open unified portal and handle single callback
@@ -119,15 +125,41 @@ pub async fn start_unified_auth(db: &mut Database) -> Result<PortalResult, AuthE
             Ok(PortalResult::Social(provider))
         },
         "internal" => {
-            let issuer_uri = callback
-                .issuer_uri
-                .ok_or_else(|| AuthError::OAuthCustomError("Missing issuer_uri for internal auth".into()))?;
+            let issuer_url = callback
+                .issuer_url
+                .ok_or_else(|| AuthError::OAuthCustomError("Missing issuer_url for internal auth".into()))?;
             let sso_region = callback
                 .sso_region
                 .ok_or_else(|| AuthError::OAuthCustomError("Missing sso_region for internal auth".into()))?;
-            // DO NOT register here. Let caller run start_pkce_authorization(issuer_uri, sso_region).
+            // DO NOT register here. Let caller run start_pkce_authorization(issuer_url, sso_region).
             Ok(PortalResult::Internal {
-                issuer_uri,
+                issuer_url,
+                idc_region: sso_region,
+            })
+        },
+        "awsidc" => {
+            let issuer_url: String = callback
+                .issuer_url
+                .ok_or_else(|| AuthError::OAuthCustomError("Missing issuer_url for awsIdc auth".into()))?;
+            let sso_region = callback
+                .sso_region
+                .ok_or_else(|| AuthError::OAuthCustomError("Missing sso_region for awsIdc auth".into()))?;
+            // DO NOT register here. Let caller run start_pkce_authorization(issuer_url, sso_region).
+            Ok(PortalResult::AwsIdc {
+                issuer_url,
+                idc_region: sso_region,
+            })
+        },
+        "builderid" => {
+            let issuer_url = callback
+                .issuer_url
+                .ok_or_else(|| AuthError::OAuthCustomError("Missing issuer_url for builderId auth".into()))?;
+            let sso_region = callback
+                .sso_region
+                .ok_or_else(|| AuthError::OAuthCustomError("Missing sso_region for builderId auth".into()))?;
+            // DO NOT register here. Let caller run start_pkce_authorization(issuer_url, sso_region).
+            Ok(PortalResult::BuilderId {
+                issuer_url,
                 idc_region: sso_region,
             })
         },
@@ -201,7 +233,7 @@ impl Service<Request<Incoming>> for AuthCallbackService {
                 let callback = AuthPortalCallback {
                     login_option: query_params.get("login_option").cloned().unwrap_or_default(),
                     code: query_params.get("code").cloned(),
-                    issuer_uri: query_params.get("issuer_uri").cloned(),
+                    issuer_url: query_params.get("issuer_url").cloned(),
                     sso_region: query_params.get("idc_region").cloned(),
                     state: query_params.get("state").cloned().unwrap_or_default(),
                     path: path.to_string(),
@@ -210,7 +242,7 @@ impl Service<Request<Incoming>> for AuthCallbackService {
                 debug!(
                     login_option=%callback.login_option,
                     code_present=%callback.code.is_some(),
-                    issuer_uri=?callback.issuer_uri,
+                    issuer_url=?callback.issuer_url,
                     state=%callback.state,
                     "Parsed portal callback query"
                 );
diff --git a/crates/chat-cli/src/cli/mod.rs b/crates/chat-cli/src/cli/mod.rs
@@ -234,7 +234,9 @@ impl Cli {
             },
             log_to_stdout: std::env::var_os("Q_LOG_STDOUT").is_some() || self.verbose > 0,
             log_file_path: match subcommand {
-                RootSubcommand::Chat { .. } => Some(logs_dir().expect("home dir must be set").join("qchat.log")),
+                RootSubcommand::Chat { .. } | RootSubcommand::Login { .. } => {
+                    Some(logs_dir().expect("home dir must be set").join("qchat.log"))
+                },
                 _ => None,
             },
             delete_old_log_file: false,
diff --git a/crates/chat-cli/src/cli/user.rs b/crates/chat-cli/src/cli/user.rs
@@ -106,45 +106,27 @@ impl LoginArgs {
             match start_unified_auth(&mut os.database).await? {
                 PortalResult::Social(provider) => {
                     pre_portal_spinner.stop_with_message(format!("Logged in with {}", provider));
-                    os.telemetry.send_user_logged_in(None, None, Some(provider)).ok();
+                    os.telemetry.send_user_logged_in().ok();
                     return Ok(ExitCode::SUCCESS);
                 },
-                PortalResult::Internal { issuer_uri, idc_region } => {
-                    pre_portal_spinner.stop();
-                    // EXACTLY same with original PKCE path: this registers client + completes auth.
-                    let (client, registration) =
-                        start_pkce_authorization(Some(issuer_uri.clone()), Some(idc_region.clone())).await?;
-
-                    match crate::util::open::open_url_async(&registration.url).await {
-                        // If it succeeded, finish PKCE.
-                        Ok(()) => {
-                            let mut spinner = Spinner::new(vec![
-                                SpinnerComponent::Spinner,
-                                SpinnerComponent::Text(" Logging in...".into()),
-                            ]);
-                            let issuer_url = registration.issuer_url().to_string();
-                            let region = registration.region().to_string();
-                            let ctrl_c_stream = ctrl_c();
-                            tokio::select! {
-                                res = registration.finish(&client, Some(&mut os.database)) => res?,
-                                Ok(_) = ctrl_c_stream => {
-                                    #[allow(clippy::exit)]
-                                    exit(1);
-                                },
-                            }
-                            os.telemetry
-                                .send_user_logged_in(Some(issuer_url), Some(region), None)
-                                .ok();
-                            spinner.stop_with_message("Logged in".into());
-                        },
-                        // If we are unable to open the link with the browser, then fallback to
-                        // the device code flow.
-                        Err(err) => {
-                            // Try device code flow.
-                            error!(%err, "Failed to open URL with browser, falling back to device code flow");
-                            try_device_authorization(os, Some(issuer_uri.clone()), Some(idc_region.clone())).await?;
-                        },
-                    }
+                PortalResult::BuilderId { issuer_url, idc_region } => {
+                    pre_portal_spinner.stop_with_message("".into());
+                    info!("Completing BuilderID authentication");
+                    complete_sso_auth(os, issuer_url, idc_region, false).await?;
+                },
+                PortalResult::AwsIdc { issuer_url, idc_region } => {
+                    pre_portal_spinner.stop_with_message("".into());
+                    info!("Completing AWS Identity Center authentication");
+                    // Save IdC credentials for future use
+                    let _ = os.database.set_start_url(issuer_url.clone());
+                    let _ = os.database.set_idc_region(idc_region.clone());
+
+                    complete_sso_auth(os, issuer_url, idc_region, true).await?;
+                },
+                PortalResult::Internal { issuer_url, idc_region } => {
+                    pre_portal_spinner.stop_with_message("".into());
+                    info!("Completing internal authentication");
+                    complete_sso_auth(os, issuer_url, idc_region, true).await?;
                 },
             }
         } else {
@@ -202,6 +184,7 @@ impl LoginArgs {
 
                             (Some(start_url), Some(region))
                         },
+                        AuthMethod::Social(_) => unreachable!(),
                     };
 
                     // Remote machine won't be able to handle browser opening and redirects,
@@ -212,13 +195,60 @@ impl LoginArgs {
                         select_profile_interactive(os, true).await?;
                     }
                 },
+                AuthMethod::Social(_) => unreachable!(),
             }
         }
 
         Ok(ExitCode::SUCCESS)
     }
 }
 
+/// Complete SSO authentication (BuilderID, IdC, or Internal) after portal selection
+///
+/// # Arguments
+/// * `requires_profile` - Whether to prompt for profile selection after login (IdC only)
+async fn complete_sso_auth(os: &mut Os, issuer_url: String, idc_region: String, requires_profile: bool) -> Result<()> {
+    let (client, registration) = start_pkce_authorization(Some(issuer_url.clone()), Some(idc_region.clone())).await?;
+
+    match crate::util::open::open_url_async(&registration.url).await {
+        Ok(()) => {
+            // Browser opened successfully, wait for PKCE flow to complete
+            let mut spinner = Spinner::new(vec![
+                SpinnerComponent::Spinner,
+                SpinnerComponent::Text(" Logging in...".into()),
+            ]);
+
+            let ctrl_c_stream = ctrl_c();
+            tokio::select! {
+                res = registration.finish(&client, Some(&mut os.database)) => res?,
+                Ok(_) = ctrl_c_stream => {
+                    #[allow(clippy::exit)]
+                    exit(1);
+                },
+            }
+
+            os.telemetry.send_user_logged_in().ok();
+            spinner.stop_with_message("Logged in".into());
+
+            // Prompt for profile selection if needed (IdC only)
+            if requires_profile {
+                select_profile_interactive(os, true).await?;
+            }
+        },
+        Err(err) => {
+            // Failed to open browser, fallback to device code flow
+            error!(%err, "Failed to open URL, falling back to device code flow");
+            try_device_authorization(os, Some(issuer_url), Some(idc_region)).await?;
+
+            if requires_profile {
+                select_profile_interactive(os, true).await?;
+            }
+        },
+    }
+
+    Ok(())
+}
+
 pub async fn logout(os: &mut Os) -> Result<ExitCode> {
     let _ = crate::auth::logout(&mut os.database).await;
     let _ = crate::auth::social::logout_social(&os.database).await;
@@ -301,7 +331,7 @@ impl WhoamiArgs {
 
 #[derive(Debug, Clone, Copy, PartialEq, Eq, clap::ValueEnum)]
 pub enum LicenseType {
-    /// Free license (Builder ID)
+    /// Free license with Builder ID
     Free,
     /// Pro license with Identity Center
     Pro,
@@ -325,13 +355,18 @@ enum AuthMethod {
     BuilderId,
     /// IdC (enterprise)
     IdentityCenter,
+    /// Social login
+    #[allow(dead_code)]
+    Social(SocialProvider),
 }
 
 impl Display for AuthMethod {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         match self {
             AuthMethod::BuilderId => write!(f, "Use for Free with Builder ID"),
             AuthMethod::IdentityCenter => write!(f, "Use with Pro license"),
+            AuthMethod::Social(SocialProvider::Google) => write!(f, "Use with Google"),
+            AuthMethod::Social(SocialProvider::Github) => write!(f, "Use with GitHub"),
         }
     }
 }
@@ -382,7 +417,7 @@ async fn try_device_authorization(os: &mut Os, start_url: Option<String>, region
         {
             PollCreateToken::Pending => {},
             PollCreateToken::Complete => {
-                os.telemetry.send_user_logged_in(start_url, region, None).ok();
+                os.telemetry.send_user_logged_in().ok();
                 spinner.stop_with_message("Logged in".into());
                 break;
             },
diff --git a/crates/chat-cli/src/telemetry/core.rs b/crates/chat-cli/src/telemetry/core.rs
@@ -82,13 +82,12 @@ impl Event {
 
     pub fn into_metric_datum(self) -> Option<MetricDatum> {
         match self.ty {
-            EventType::UserLoggedIn { social_provider } => Some(
+            EventType::UserLoggedIn {} => Some(
                 CodewhispererterminalUserLoggedIn {
                     create_time: self.created_time,
                     value: None,
                     credential_start_url: self.credential_start_url.map(Into::into),
                     codewhispererterminal_in_cloudshell: None,
-                    social_provider: social_provider.map(Into::into),
                 }
                 .into_metric_datum(),
             ),
@@ -594,9 +593,7 @@ pub struct AgentConfigInitArgs {
 #[serde(rename_all = "camelCase")]
 #[serde(tag = "type")]
 pub enum EventType {
-    UserLoggedIn {
-        social_provider: Option<String>,
-    },
+    UserLoggedIn {},
     RefreshCredentials {
         request_id: String,
         result: TelemetryResult,
diff --git a/crates/chat-cli/src/telemetry/mod.rs b/crates/chat-cli/src/telemetry/mod.rs
@@ -57,7 +57,6 @@ use crate::api_client::{
     ApiClientError,
 };
 use crate::auth::builder_id::get_start_url_and_region;
-use crate::auth::social::SocialProvider;
 use crate::aws_common::app_name;
 use crate::cli::RootSubcommand;
 use crate::database::settings::Setting;
@@ -232,23 +231,8 @@ impl TelemetryThread {
         Ok(())
     }
 
-    pub fn send_user_logged_in(
-        &self,
-        start_url: Option<String>,
-        sso_region: Option<String>,
-        provider: Option<SocialProvider>,
-    ) -> Result<(), TelemetryError> {
-        let mut telemetry_event = Event::new(EventType::UserLoggedIn {
-            social_provider: provider.map(|p| p.to_string()),
-        });
-        if let Some(url) = start_url {
-            telemetry_event.set_start_url(url);
-        }
-        if let Some(region) = sso_region {
-            telemetry_event.set_sso_region(region);
-        }
-
-        Ok(self.tx.send(telemetry_event)?)
+    pub fn send_user_logged_in(&self) -> Result<(), TelemetryError> {
+        Ok(self.tx.send(Event::new(EventType::UserLoggedIn {}))?)
     }
 
     pub fn send_daily_heartbeat(&self) -> Result<(), TelemetryError> {
@@ -806,7 +790,7 @@ mod test {
         let thread = TelemetryThread::new(&Env::new(), &Fs::new(), &mut database)
             .await
             .unwrap();
-        thread.send_user_logged_in(None, None, None).ok();
+        thread.send_user_logged_in().ok();
         drop(thread);
 
         assert!(!logs_contain("ERROR"));
@@ -825,7 +809,7 @@ mod test {
             .await
             .unwrap();
 
-        thread.send_user_logged_in(None, None, None).ok();
+        thread.send_user_logged_in().ok();
         thread
             .send_cli_subcommand_executed(&database, &RootSubcommand::Version { changelog: None })
             .await
diff --git a/crates/chat-cli/telemetry_definitions.json b/crates/chat-cli/telemetry_definitions.json

Original file line number	Diff line number	Diff line change
`@@ -210,7 +210,6 @@ impl DeviceRegistration {`
`210`	`210`	`secret_store`
`211`	`211`	`.set_secret(Self::SECRET_KEY, &serde_json::to_string(&self)?)`
`212`	`212`	`.await?;`
`213`		`- info!("save builderid token");`
`214`	`213`	`Ok(())`
`215`	`214`	`}`
`216`	`215`	`}`
Original file line number	Diff line number	Diff line change
`@@ -161,7 +161,7 @@ <h4>Request denied</h4>`
`161`	`161`	`return`
`162`	`162`	`}`
`163`	`163`
`164`		`- const productName = 'KIRO CLI'`
	`164`	`+ const productName = 'Amazon Q for CLI'`
`165`	`165`	`document.getElementById(`
`166`	`166`	`'approvalMessage'`
`167`	`167`	).innerText = `${productName} have been given requested permissions`
Original file line number	Diff line number	Diff line change
`@@ -319,14 +319,6 @@ impl PkceRegistration {`
`319`	`319`	`None => Err(AuthError::OAuthMissingCode),`
`320`	`320`	`}`
`321`	`321`	`}`
`322`		`-`
`323`		`- pub fn issuer_url(&self) -> &str {`
`324`		`- &self.issuer_url`
`325`		`- }`
`326`		`-`
`327`		`- pub fn region(&self) -> &Region {`
`328`		`- &self.region`
`329`		`- }`
`330`	`322`	`}`
`331`	`323`
`332`	`324`	`type CodeSender = std::sync::Arc<tokio::sync::mpsc::Sender<Result<(String, String), AuthError>>>;`