Detailed changes
@@ -261,7 +261,7 @@ impl ContextServerRegistry {
ContextServerStatus::Stopped
| ContextServerStatus::Error(_)
| ContextServerStatus::AuthRequired
- | ContextServerStatus::ClientSecretRequired => {
+ | ContextServerStatus::ClientSecretRequired { .. } => {
if let Some(registered_server) = self.registered_servers.remove(server_id) {
if !registered_server.tools.is_empty() {
cx.emit(ContextServerRegistryEvent::ToolsChanged);
@@ -664,8 +664,10 @@ impl AgentConfiguration {
None
};
let auth_required = matches!(server_status, ContextServerStatus::AuthRequired);
- let client_secret_required =
- matches!(server_status, ContextServerStatus::ClientSecretRequired);
+ let client_secret_required = matches!(
+ server_status,
+ ContextServerStatus::ClientSecretRequired { .. }
+ );
let authenticating = matches!(server_status, ContextServerStatus::Authenticating);
let context_server_store = self.context_server_store.clone();
let workspace = self.workspace.clone();
@@ -689,7 +691,9 @@ impl AgentConfiguration {
ContextServerStatus::Error(_) => AiSettingItemStatus::Error,
ContextServerStatus::Stopped => AiSettingItemStatus::Stopped,
ContextServerStatus::AuthRequired => AiSettingItemStatus::AuthRequired,
- ContextServerStatus::ClientSecretRequired => AiSettingItemStatus::ClientSecretRequired,
+ ContextServerStatus::ClientSecretRequired { .. } => {
+ AiSettingItemStatus::ClientSecretRequired
+ }
ContextServerStatus::Authenticating => AiSettingItemStatus::Authenticating,
};
@@ -931,8 +935,6 @@ impl AgentConfiguration {
.label_size(LabelSize::Small)
.on_click({
let context_server_id = context_server_id.clone();
- let language_registry = language_registry.clone();
- let workspace = workspace.clone();
move |_event, window, cx| {
ConfigureContextServerModal::show_modal_for_existing_server(
context_server_id.clone(),
@@ -406,9 +406,16 @@ fn resolve_context_server_extension(
enum State {
Idle,
Waiting,
- AuthRequired { server_id: ContextServerId },
- ClientSecretRequired { server_id: ContextServerId },
- Authenticating { _server_id: ContextServerId },
+ AuthRequired {
+ server_id: ContextServerId,
+ },
+ ClientSecretRequired {
+ server_id: ContextServerId,
+ error: Option<SharedString>,
+ },
+ Authenticating {
+ server_id: ContextServerId,
+ },
Error(SharedString),
}
@@ -442,11 +449,14 @@ impl ConfigureContextServerModal {
Some(ContextServerStatus::AuthRequired) => State::AuthRequired {
server_id: server_id.clone(),
},
- Some(ContextServerStatus::ClientSecretRequired) => State::ClientSecretRequired {
- server_id: server_id.clone(),
- },
+ Some(ContextServerStatus::ClientSecretRequired { error }) => {
+ State::ClientSecretRequired {
+ server_id: server_id.clone(),
+ error: error.map(SharedString::from),
+ }
+ }
Some(ContextServerStatus::Authenticating) => State::Authenticating {
- _server_id: server_id.clone(),
+ server_id: server_id.clone(),
},
Some(ContextServerStatus::Error(error)) => State::Error(error.into()),
@@ -602,16 +612,12 @@ impl ConfigureContextServerModal {
}
fn confirm(&mut self, _: &menu::Confirm, cx: &mut Context<Self>) {
- if matches!(
- self.state,
- State::Waiting
- | State::AuthRequired { .. }
- | State::ClientSecretRequired { .. }
- | State::Authenticating { .. }
- ) {
+ if matches!(self.state, State::Waiting | State::Authenticating { .. }) {
return;
}
+ self._auth_subscription = None;
+
self.state = State::Idle;
let Some(workspace) = self.workspace.upgrade() else {
return;
@@ -627,7 +633,7 @@ impl ConfigureContextServerModal {
self.state = State::Waiting;
- let existing_server = self.context_server_store.read(cx).get_running_server(&id);
+ let existing_server = self.context_server_store.read(cx).get_server(&id);
if existing_server.is_some() {
self.context_server_store.update(cx, |store, cx| {
store.stop_server(&id, cx).log_err();
@@ -650,8 +656,11 @@ impl ConfigureContextServerModal {
this.state = State::AuthRequired { server_id: id };
cx.notify();
}
- Ok(ContextServerStatus::ClientSecretRequired) => {
- this.state = State::ClientSecretRequired { server_id: id };
+ Ok(ContextServerStatus::ClientSecretRequired { error }) => {
+ this.state = State::ClientSecretRequired {
+ server_id: id,
+ error: error.map(SharedString::from),
+ };
cx.notify();
}
Err(err) => {
@@ -693,65 +702,33 @@ impl ConfigureContextServerModal {
cx.emit(DismissEvent);
}
+ fn cancel_authentication(&mut self, server_id: &ContextServerId, cx: &mut Context<Self>) {
+ self._auth_subscription = None;
+ self.context_server_store.update(cx, |store, cx| {
+ store.stop_server(server_id, cx).log_err();
+ });
+ self.state = State::Idle;
+ cx.notify();
+ }
+
fn authenticate(&mut self, server_id: ContextServerId, cx: &mut Context<Self>) {
self.context_server_store.update(cx, |store, cx| {
store.authenticate_server(&server_id, cx).log_err();
});
-
- self.state = State::Authenticating {
- _server_id: server_id.clone(),
- };
-
- self._auth_subscription = Some(cx.subscribe(
- &self.context_server_store,
- move |this, _, event: &ServerStatusChangedEvent, cx| {
- if event.server_id != server_id {
- return;
- }
- match &event.status {
- ContextServerStatus::Running => {
- this._auth_subscription = None;
- this.state = State::Idle;
- this.show_configured_context_server_toast(event.server_id.clone(), cx);
- cx.emit(DismissEvent);
- }
- ContextServerStatus::AuthRequired => {
- this._auth_subscription = None;
- this.state = State::AuthRequired {
- server_id: event.server_id.clone(),
- };
- cx.notify();
- }
- ContextServerStatus::ClientSecretRequired => {
- this._auth_subscription = None;
- this.state = State::ClientSecretRequired {
- server_id: event.server_id.clone(),
- };
- cx.notify();
- }
- ContextServerStatus::Error(error) => {
- this._auth_subscription = None;
- this.set_error(error.clone(), cx);
- }
- ContextServerStatus::Authenticating
- | ContextServerStatus::Starting
- | ContextServerStatus::Stopped => {}
- }
- },
- ));
-
- cx.notify();
+ self.await_auth_outcome(server_id, cx);
}
fn submit_client_secret(&mut self, server_id: ContextServerId, cx: &mut Context<Self>) {
let secret = self.secret_editor.read(cx).text(cx);
-
self.context_server_store.update(cx, |store, cx| {
store.submit_client_secret(&server_id, secret, cx).log_err();
});
+ self.await_auth_outcome(server_id, cx);
+ }
+ fn await_auth_outcome(&mut self, server_id: ContextServerId, cx: &mut Context<Self>) {
self.state = State::Authenticating {
- _server_id: server_id.clone(),
+ server_id: server_id.clone(),
};
self._auth_subscription = Some(cx.subscribe(
@@ -774,10 +751,11 @@ impl ConfigureContextServerModal {
};
cx.notify();
}
- ContextServerStatus::ClientSecretRequired => {
+ ContextServerStatus::ClientSecretRequired { error } => {
this._auth_subscription = None;
this.state = State::ClientSecretRequired {
server_id: event.server_id.clone(),
+ error: error.clone().map(SharedString::from),
};
cx.notify();
}
@@ -981,13 +959,7 @@ impl ConfigureContextServerModal {
fn render_modal_footer(&self, cx: &mut Context<Self>) -> ModalFooter {
let focus_handle = self.focus_handle(cx);
- let is_busy = matches!(
- self.state,
- State::Waiting
- | State::AuthRequired { .. }
- | State::ClientSecretRequired { .. }
- | State::Authenticating { .. }
- );
+ let is_busy = matches!(self.state, State::Waiting | State::Authenticating { .. });
ModalFooter::new()
.start_slot::<Button>(
@@ -1117,6 +1089,7 @@ impl ConfigureContextServerModal {
fn render_client_secret_required(
&self,
server_id: &ContextServerId,
+ error: Option<SharedString>,
cx: &mut Context<Self>,
) -> Div {
let settings = ThemeSettings::get_global(cx);
@@ -1133,6 +1106,9 @@ impl ConfigureContextServerModal {
v_flex()
.w_full()
.gap_2()
+ .when_some(error, |this, error| {
+ this.child(Self::render_modal_error(error))
+ })
.child(
h_flex()
.gap_1p5()
@@ -1153,6 +1129,12 @@ impl ConfigureContextServerModal {
h_flex()
.w_full()
.gap_2()
+ .capture_action({
+ let server_id = server_id.clone();
+ cx.listener(move |this, _: &editor::actions::Newline, _window, cx| {
+ this.submit_client_secret(server_id.clone(), cx);
+ })
+ })
.child(div().flex_1().child(EditorElement::new(
&self.secret_editor,
EditorStyle {
@@ -1177,6 +1159,39 @@ impl ConfigureContextServerModal {
)
}
+ fn render_authenticating(&self, server_id: &ContextServerId, cx: &mut Context<Self>) -> Div {
+ h_flex()
+ .h_8()
+ .gap_2()
+ .justify_center()
+ .child(
+ h_flex()
+ .gap_1p5()
+ .child(
+ Icon::new(IconName::LoadCircle)
+ .size(IconSize::XSmall)
+ .color(Color::Muted)
+ .with_rotate_animation(3),
+ )
+ .child(
+ Label::new("Authenticating…")
+ .size(LabelSize::Small)
+ .color(Color::Muted),
+ ),
+ )
+ .child(
+ Button::new("cancel-authentication", "Cancel")
+ .style(ButtonStyle::Outlined)
+ .label_size(LabelSize::Small)
+ .on_click({
+ let server_id = server_id.clone();
+ cx.listener(move |this, _event, _window, cx| {
+ this.cancel_authentication(&server_id, cx);
+ })
+ }),
+ )
+ }
+
fn render_modal_error(error: SharedString) -> Div {
h_flex()
.h_8()
@@ -1236,13 +1251,15 @@ impl Render for ConfigureContextServerModal {
State::AuthRequired { server_id } => {
self.render_auth_required(&server_id.clone(), cx)
}
- State::ClientSecretRequired { server_id } => self
- .render_client_secret_required(
+ State::ClientSecretRequired { server_id, error } => {
+ self.render_client_secret_required(
&server_id.clone(),
+ error.clone(),
cx,
- ),
- State::Authenticating { .. } => {
- self.render_loading("Authenticating…")
+ )
+ }
+ State::Authenticating { server_id } => {
+ self.render_authenticating(&server_id.clone(), cx)
}
State::Error(error) => {
Self::render_modal_error(error.clone())
@@ -1280,7 +1297,7 @@ fn wait_for_context_server(
match status {
ContextServerStatus::Running
| ContextServerStatus::AuthRequired
- | ContextServerStatus::ClientSecretRequired => {
+ | ContextServerStatus::ClientSecretRequired { .. } => {
if let Some(tx) = tx.lock().take() {
let _ = tx.send(Ok(status.clone()));
}
@@ -632,6 +632,26 @@ impl TokenResponse {
}
}
+/// An OAuth token error response (RFC 6749 Section 5.2).
+#[derive(Debug, Deserialize)]
+pub struct OAuthTokenError {
+ pub error: String,
+ #[serde(default)]
+ pub error_description: Option<String>,
+}
+
+impl std::fmt::Display for OAuthTokenError {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ write!(f, "OAuth token error: {}", self.error)?;
+ if let Some(description) = &self.error_description {
+ write!(f, " ({description})")?;
+ }
+ Ok(())
+ }
+}
+
+impl std::error::Error for OAuthTokenError {}
+
/// Build the form-encoded body for an authorization code token exchange.
pub fn token_exchange_params(
code: &str,
@@ -760,13 +780,14 @@ pub async fn fetch_auth_server_metadata(
match fetch_json::<AuthServerMetadataResponse>(http_client, url).await {
Ok(response) => {
let reported_issuer = response.issuer.unwrap_or_else(|| issuer.clone());
- // if reported_issuer != *issuer {
- // bail!(
- // "Auth server metadata issuer mismatch: expected {}, got {}",
- // issuer,
- // reported_issuer
- // );
- // }
+
+ if reported_issuer != *issuer {
+ bail!(
+ "Auth server metadata issuer mismatch: expected {}, got {}",
+ issuer,
+ reported_issuer
+ );
+ }
return Ok(AuthServerMetadata {
issuer: reported_issuer,
@@ -962,11 +983,12 @@ async fn post_token_request(
if !response.status().is_success() {
let mut error_body = String::new();
response.body_mut().read_to_string(&mut error_body).await?;
- bail!(
- "token request failed with status {}: {}",
- response.status(),
- error_body
- );
+ let status = response.status();
+ // Try to parse as an OAuth error response (RFC 6749 Section 5.2).
+ if let Ok(token_error) = serde_json::from_str::<OAuthTokenError>(&error_body) {
+ return Err(token_error.into());
+ }
+ bail!("token request failed with status {status}: {error_body}");
}
let mut response_body = String::new();
@@ -55,9 +55,10 @@ pub enum ContextServerStatus {
/// should show an "Authenticate" button.
AuthRequired,
/// The server has a pre-registered OAuth client_id, but a client_secret
- /// is needed and not available in settings or the keychain. The UI should
- /// show a text input to collect it.
- ClientSecretRequired,
+ /// is needed and not available in settings or the keychain.
+ ClientSecretRequired {
+ error: Option<Arc<str>>,
+ },
/// The OAuth browser flow is in progress — the user has been redirected
/// to the authorization server and we're waiting for the callback.
Authenticating,
@@ -71,8 +72,10 @@ impl ContextServerStatus {
ContextServerState::Stopped { .. } => ContextServerStatus::Stopped,
ContextServerState::Error { error, .. } => ContextServerStatus::Error(error.clone()),
ContextServerState::AuthRequired { .. } => ContextServerStatus::AuthRequired,
- ContextServerState::ClientSecretRequired { .. } => {
- ContextServerStatus::ClientSecretRequired
+ ContextServerState::ClientSecretRequired { error, .. } => {
+ ContextServerStatus::ClientSecretRequired {
+ error: error.clone(),
+ }
}
ContextServerState::Authenticating { .. } => ContextServerStatus::Authenticating,
}
@@ -106,11 +109,12 @@ enum ContextServerState {
discovery: Arc<OAuthDiscovery>,
},
/// A pre-registered client_id is configured but no client_secret was found
- /// in settings or the keychain. The user needs to provide it interactively.
+ /// in settings or the keychain.
ClientSecretRequired {
server: Arc<ContextServer>,
configuration: Arc<ContextServerConfiguration>,
discovery: Arc<OAuthDiscovery>,
+ error: Option<Arc<str>>,
},
/// The OAuth browser flow is in progress. The user has been redirected
/// to the authorization server and we're waiting for the callback.
@@ -1018,9 +1022,10 @@ impl ContextServerStore {
_ => anyhow::bail!("Server is not in AuthRequired state"),
};
- // Check if the configuration has pre-registered OAuth credentials that
- // need a client_secret we don't have yet.
- let needs_secret_prompt = match configuration.as_ref() {
+ // If a pre-registered client_id is configured without a client_secret
+ // in settings, we need to check the keychain before proceeding. If the
+ // keychain doesn't have it either, we prompt the user.
+ let needs_keychain_check = match configuration.as_ref() {
ContextServerConfiguration::Http {
url,
oauth: Some(oauth_settings),
@@ -1031,48 +1036,21 @@ impl ContextServerStore {
let id = id.clone();
- if let Some(server_url) = needs_secret_prompt {
- // Check keychain for the secret asynchronously.
- let task = cx.spawn({
- let id = id.clone();
- let server = server.clone();
- let configuration = configuration.clone();
- async move |this, cx| {
+ let task = cx.spawn({
+ let id = id.clone();
+ let server = server.clone();
+ let configuration = configuration.clone();
+ async move |this, cx| {
+ if let Some(server_url) = needs_keychain_check {
let credentials_provider = cx.update(|cx| zed_credentials_provider::global(cx));
- let keychain_secret =
+ let has_keychain_secret =
Self::load_client_secret(&credentials_provider, &server_url, cx)
.await
.ok()
- .flatten();
+ .flatten()
+ .is_some();
- if keychain_secret.is_some() {
- // Secret found in keychain, proceed with OAuth flow.
- let result = Self::run_oauth_flow(
- this.clone(),
- id.clone(),
- discovery.clone(),
- configuration.clone(),
- cx,
- )
- .await;
-
- if let Err(err) = &result {
- log::error!("{} OAuth authentication failed: {:?}", id, err);
- this.update(cx, |this, cx| {
- this.update_server_state(
- id.clone(),
- ContextServerState::AuthRequired {
- server,
- configuration,
- discovery,
- },
- cx,
- )
- })
- .log_err();
- }
- } else {
- // No secret anywhere — prompt the user.
+ if !has_keychain_secret {
this.update(cx, |this, cx| {
this.update_server_state(
id.clone(),
@@ -1080,68 +1058,52 @@ impl ContextServerStore {
server,
configuration,
discovery,
+ error: None,
},
cx,
);
})
.log_err();
+ return;
}
}
- });
- self.update_server_state(
- id,
- ContextServerState::Authenticating {
- server,
- configuration,
- _task: task,
- },
- cx,
- );
- } else {
- // No pre-registration, or secret already in settings — proceed directly.
- let task = cx.spawn({
- let id = id.clone();
- let server = server.clone();
- let configuration = configuration.clone();
- async move |this, cx| {
- let result = Self::run_oauth_flow(
- this.clone(),
- id.clone(),
- discovery.clone(),
- configuration.clone(),
- cx,
- )
- .await;
-
- if let Err(err) = &result {
- log::error!("{} OAuth authentication failed: {:?}", id, err);
- this.update(cx, |this, cx| {
- this.update_server_state(
- id.clone(),
- ContextServerState::AuthRequired {
- server,
- configuration,
- discovery,
- },
- cx,
- )
- })
- .log_err();
- }
+ let result = Self::run_oauth_flow(
+ this.clone(),
+ id.clone(),
+ discovery.clone(),
+ configuration.clone(),
+ cx,
+ )
+ .await;
+
+ if let Err(err) = &result {
+ log::error!("{} OAuth authentication failed: {:?}", id, err);
+ this.update(cx, |this, cx| {
+ this.update_server_state(
+ id.clone(),
+ ContextServerState::Error {
+ server,
+ configuration,
+ error: format!("{err:#}").into(),
+ },
+ cx,
+ )
+ })
+ .log_err();
}
- });
+ }
+ });
- self.update_server_state(
- id,
- ContextServerState::Authenticating {
- server,
- configuration,
- _task: task,
- },
- cx,
- );
- }
+ self.update_server_state(
+ id,
+ ContextServerState::Authenticating {
+ server,
+ configuration,
+ _task: task,
+ },
+ cx,
+ );
Ok(())
}
@@ -1160,6 +1122,7 @@ impl ContextServerStore {
server,
configuration,
discovery,
+ ..
} => (server.clone(), configuration.clone(), discovery.clone()),
_ => anyhow::bail!("Server is not in ClientSecretRequired state"),
};
@@ -1202,18 +1165,47 @@ impl ContextServerStore {
if let Err(err) = &result {
log::error!("{} OAuth authentication failed: {:?}", id, err);
- this.update(cx, |this, cx| {
- this.update_server_state(
- id.clone(),
- ContextServerState::AuthRequired {
- server,
- configuration,
- discovery,
- },
- cx,
- )
- })
- .log_err();
+
+ let is_bad_client_credentials = err
+ .downcast_ref::<oauth::OAuthTokenError>()
+ .is_some_and(|e| e.error == "unauthorized_client");
+
+ if is_bad_client_credentials {
+ // Clear the bad secret from the keychain so the user
+ // gets a fresh prompt.
+ let credentials_provider =
+ cx.update(|cx| zed_credentials_provider::global(cx));
+ Self::clear_client_secret(&credentials_provider, &server_url, cx)
+ .await
+ .log_err();
+
+ this.update(cx, |this, cx| {
+ this.update_server_state(
+ id.clone(),
+ ContextServerState::ClientSecretRequired {
+ server,
+ configuration,
+ discovery,
+ error: Some(format!("{err:#}").into()),
+ },
+ cx,
+ );
+ })
+ .log_err();
+ } else {
+ this.update(cx, |this, cx| {
+ this.update_server_state(
+ id.clone(),
+ ContextServerState::Error {
+ server,
+ configuration,
+ error: format!("{err:#}").into(),
+ },
+ cx,
+ )
+ })
+ .log_err();
+ }
}
}
});
@@ -310,9 +310,8 @@ pub struct OAuthClientSettings {
/// authorization server.
pub client_id: String,
/// The OAuth client secret, if this is a confidential client. For security,
- /// prefer providing this interactively — Zed will prompt and store it in
- /// the system keychain. Only use this setting when keychain storage is not
- /// an option.
+ /// prefer providing this interactively; we will prompt and store it in
+ /// the system keychain.
#[serde(default, skip_serializing_if = "Option::is_none")]
pub client_secret: Option<String>,
}
@@ -447,9 +447,8 @@ pub struct OAuthClientSettings {
/// authorization server.
pub client_id: String,
/// The OAuth client secret, if this is a confidential client. For security,
- /// prefer providing this interactively — Zed will prompt and store it in
- /// the system keychain. Only use this setting when keychain storage is not
- /// an option.
+ /// prefer providing this interactively; we will prompt and store it in
+ /// the system keychain.
#[serde(default, skip_serializing_if = "Option::is_none")]
pub client_secret: Option<String>,
}