From 6b553e4d27505ad2ce9152a30e6fe1256390fc51 Mon Sep 17 00:00:00 2001 From: Marshall Bowers Date: Mon, 16 Feb 2026 19:27:04 -0500 Subject: [PATCH] collab: Remove leftover impersonation code (#49314) This PR removes some code left over from how we used to do impersonation for local development. The local development impersonation path is separate now, so we don't need all of this plumbing in Collab. Release Notes: - N/A --- crates/client/src/client.rs | 8 +- crates/collab/src/auth.rs | 40 +---- crates/collab/src/db/queries/access_tokens.rs | 2 - crates/collab/src/db/tables/access_token.rs | 1 - crates/collab/src/rpc.rs | 16 +- .../collab/tests/integration/collab_tests.rs | 50 ++---- .../tests/integration/db_tests/db_tests.rs | 163 ------------------ 7 files changed, 16 insertions(+), 264 deletions(-) diff --git a/crates/client/src/client.rs b/crates/client/src/client.rs index 2731462a5a4dd5c9da173c637fee773a99925e89..55d7eb5dce2be133e8c48dd23e8f34ebb68c50bf 100644 --- a/crates/client/src/client.rs +++ b/crates/client/src/client.rs @@ -34,7 +34,6 @@ use settings::{RegisterSetting, Settings, SettingsContent}; use std::{ any::TypeId, convert::TryFrom, - fmt::Write as _, future::Future, marker::PhantomData, path::PathBuf, @@ -1390,16 +1389,11 @@ impl Client { // Open the Zed sign-in page in the user's browser, with query parameters that indicate // that the user is signing in from a Zed app running on the same device. - let mut url = http.build_url(&format!( + let url = http.build_url(&format!( "/native_app_signin?native_app_port={}&native_app_public_key={}", port, public_key_string )); - if let Some(impersonate_login) = IMPERSONATE_LOGIN.as_ref() { - log::info!("impersonating user @{}", impersonate_login); - write!(&mut url, "&impersonate={}", impersonate_login).unwrap(); - } - open_url_tx.send(url).log_err(); #[derive(Deserialize)] diff --git a/crates/collab/src/auth.rs b/crates/collab/src/auth.rs index b844bdcd58b3aad706e30611d8f37aad61d29b13..f87f97c453f209de09c77475e73171d2a6863ce7 100644 --- a/crates/collab/src/auth.rs +++ b/crates/collab/src/auth.rs @@ -64,20 +64,7 @@ pub async fn validate_header(mut req: Request, next: Next) -> impl Into ) })?; - // In development, allow impersonation using the admin API token. - // Don't allow this in production because we can't tell who is doing - // the impersonating. - let validate_result = if let (Some(admin_token), true) = ( - access_token.strip_prefix("ADMIN_TOKEN:"), - state.config.is_development(), - ) { - Ok(VerifyAccessTokenResult { - is_valid: state.config.api_token == admin_token, - impersonator_id: None, - }) - } else { - verify_access_token(access_token, user_id, &state.db).await - }; + let validate_result = verify_access_token(access_token, user_id, &state.db).await; if let Ok(validate_result) = validate_result && validate_result.is_valid @@ -88,17 +75,7 @@ pub async fn validate_header(mut req: Request, next: Next) -> impl Into .await? .with_context(|| format!("user {user_id} not found"))?; - if let Some(impersonator_id) = validate_result.impersonator_id { - let admin = state - .db - .get_user_by_id(impersonator_id) - .await? - .with_context(|| format!("user {impersonator_id} not found"))?; - req.extensions_mut() - .insert(Principal::Impersonated { user, admin }); - } else { - req.extensions_mut().insert(Principal::User(user)); - }; + req.extensions_mut().insert(Principal::User(user)); return Ok::<_, Error>(next.run(req).await); } @@ -125,7 +102,6 @@ pub fn hash_access_token(token: &str) -> String { pub struct VerifyAccessTokenResult { pub is_valid: bool, - pub impersonator_id: Option, } /// Checks that the given access token is valid for the given user. @@ -147,8 +123,7 @@ pub async fn verify_access_token( let token: AccessTokenJson = serde_json::from_str(token)?; let db_token = db.get_access_token(token.id).await?; - let token_user_id = db_token.impersonated_user_id.unwrap_or(db_token.user_id); - if token_user_id != user_id { + if db_token.user_id != user_id { return Err(anyhow::anyhow!("no such access token"))?; } let t0 = Instant::now(); @@ -172,12 +147,5 @@ pub async fn verify_access_token( db.update_access_token_hash(db_token.id, &new_hash).await?; } - Ok(VerifyAccessTokenResult { - is_valid, - impersonator_id: if db_token.impersonated_user_id.is_some() { - Some(db_token.user_id) - } else { - None - }, - }) + Ok(VerifyAccessTokenResult { is_valid }) } diff --git a/crates/collab/src/db/queries/access_tokens.rs b/crates/collab/src/db/queries/access_tokens.rs index c7921639d206fe0ff086df790234fa1be7f45658..0e06b67693f505c5883c2b2c52f625ccb17601a9 100644 --- a/crates/collab/src/db/queries/access_tokens.rs +++ b/crates/collab/src/db/queries/access_tokens.rs @@ -7,7 +7,6 @@ impl Database { pub async fn create_access_token( &self, user_id: UserId, - impersonated_user_id: Option, access_token_hash: &str, max_access_token_count: usize, ) -> Result { @@ -18,7 +17,6 @@ impl Database { let token = access_token::ActiveModel { user_id: ActiveValue::set(user_id), - impersonated_user_id: ActiveValue::set(impersonated_user_id), hash: ActiveValue::set(access_token_hash.into()), ..Default::default() } diff --git a/crates/collab/src/db/tables/access_token.rs b/crates/collab/src/db/tables/access_token.rs index 22635fb64d94538687eac590efaf75049c64c864..da7392b98c444f3d83fd549525f9af4a2eb125d3 100644 --- a/crates/collab/src/db/tables/access_token.rs +++ b/crates/collab/src/db/tables/access_token.rs @@ -7,7 +7,6 @@ pub struct Model { #[sea_orm(primary_key)] pub id: AccessTokenId, pub user_id: UserId, - pub impersonated_user_id: Option, pub hash: String, } diff --git a/crates/collab/src/rpc.rs b/crates/collab/src/rpc.rs index 8dea69d973a74472e5a7e8c49a4b8dd53b959218..087dbe2a0ba23851689e75401c62b64775cf2282 100644 --- a/crates/collab/src/rpc.rs +++ b/crates/collab/src/rpc.rs @@ -130,7 +130,6 @@ impl Response { #[derive(Clone, Debug)] pub enum Principal { User(User), - Impersonated { user: User, admin: User }, } impl Principal { @@ -140,11 +139,6 @@ impl Principal { span.record("user_id", user.id.0); span.record("login", &user.github_login); } - Principal::Impersonated { user, admin } => { - span.record("user_id", user.id.0); - span.record("login", &user.github_login); - span.record("impersonator", &admin.github_login); - } } } } @@ -225,14 +219,12 @@ impl Session { fn is_staff(&self) -> bool { match &self.principal { Principal::User(user) => user.admin, - Principal::Impersonated { .. } => true, } } fn user_id(&self) -> UserId { match &self.principal { Principal::User(user) => user.id, - Principal::Impersonated { user, .. } => user.id, } } } @@ -244,10 +236,6 @@ impl Debug for Session { Principal::User(user) => { result.field("user", &user.github_login); } - Principal::Impersonated { user, admin } => { - result.field("user", &user.github_login); - result.field("impersonator", &admin.github_login); - } } result.field("connection_id", &self.connection_id).finish() } @@ -763,7 +751,6 @@ impl Server { connection_id=field::Empty, user_id=field::Empty, login=field::Empty, - impersonator=field::Empty, user_agent=field::Empty, geoip_country_code=field::Empty, release_channel=field::Empty, @@ -872,7 +859,6 @@ impl Server { concurrent_handlers, user_id=field::Empty, login=field::Empty, - impersonator=field::Empty, lsp_query_request=field::Empty, release_channel=field::Empty, { TOTAL_DURATION_MS }=field::Empty, @@ -936,7 +922,7 @@ impl Server { } match &session.principal { - Principal::User(user) | Principal::Impersonated { user, admin: _ } => { + Principal::User(user) => { if !user.connected_once { self.peer.send(connection_id, proto::ShowContacts {})?; self.app_state diff --git a/crates/collab/tests/integration/collab_tests.rs b/crates/collab/tests/integration/collab_tests.rs index c5d3e05c48dff81ea6589c2aa0c65f809c4da4f9..3376f12b203c532cfb77f561f6300a832eafc6e1 100644 --- a/crates/collab/tests/integration/collab_tests.rs +++ b/crates/collab/tests/integration/collab_tests.rs @@ -65,21 +65,12 @@ mod auth_token_tests { const MAX_ACCESS_TOKENS_TO_STORE: usize = 8; - async fn create_access_token( - db: &db::Database, - user_id: UserId, - impersonated_user_id: Option, - ) -> Result { + async fn create_access_token(db: &db::Database, user_id: UserId) -> Result { const VERSION: usize = 1; let access_token = ::rpc::auth::random_token(); let access_token_hash = hash_access_token(&access_token); let id = db - .create_access_token( - user_id, - impersonated_user_id, - &access_token_hash, - MAX_ACCESS_TOKENS_TO_STORE, - ) + .create_access_token(user_id, &access_token_hash, MAX_ACCESS_TOKENS_TO_STORE) .await?; Ok(serde_json::to_string(&AccessTokenJson { version: VERSION, @@ -106,16 +97,13 @@ mod auth_token_tests { .await .unwrap(); - let token = create_access_token(db, user.user_id, None).await.unwrap(); + let token = create_access_token(db, user.user_id).await.unwrap(); assert!(matches!( verify_access_token(&token, user.user_id, db).await.unwrap(), - VerifyAccessTokenResult { - is_valid: true, - impersonator_id: None, - } + VerifyAccessTokenResult { is_valid: true } )); - let old_token = create_previous_access_token(user.user_id, None, db) + let old_token = create_previous_access_token(user.user_id, db) .await .unwrap(); @@ -139,10 +127,7 @@ mod auth_token_tests { verify_access_token(&old_token, user.user_id, db) .await .unwrap(), - VerifyAccessTokenResult { - is_valid: true, - impersonator_id: None, - } + VerifyAccessTokenResult { is_valid: true } )); let hash = db @@ -161,35 +146,20 @@ mod auth_token_tests { verify_access_token(&old_token, user.user_id, db) .await .unwrap(), - VerifyAccessTokenResult { - is_valid: true, - impersonator_id: None, - } + VerifyAccessTokenResult { is_valid: true } )); assert!(matches!( verify_access_token(&token, user.user_id, db).await.unwrap(), - VerifyAccessTokenResult { - is_valid: true, - impersonator_id: None, - } + VerifyAccessTokenResult { is_valid: true } )); } - async fn create_previous_access_token( - user_id: UserId, - impersonated_user_id: Option, - db: &Database, - ) -> Result { + async fn create_previous_access_token(user_id: UserId, db: &Database) -> Result { let access_token = collab::auth::random_token(); let access_token_hash = previous_hash_access_token(&access_token)?; let id = db - .create_access_token( - user_id, - impersonated_user_id, - &access_token_hash, - MAX_ACCESS_TOKENS_TO_STORE, - ) + .create_access_token(user_id, &access_token_hash, MAX_ACCESS_TOKENS_TO_STORE) .await?; Ok(serde_json::to_string(&AccessTokenJson { version: 1, diff --git a/crates/collab/tests/integration/db_tests/db_tests.rs b/crates/collab/tests/integration/db_tests/db_tests.rs index 0b3de7eea6febf14dcb3ed43eea1973b0361d439..e2006b7fb9984c4bd0cf16a62e9321b2f7007e9e 100644 --- a/crates/collab/tests/integration/db_tests/db_tests.rs +++ b/crates/collab/tests/integration/db_tests/db_tests.rs @@ -136,169 +136,6 @@ async fn test_update_or_create_user_by_github_account(db: &Arc) { assert_eq!(user.email_address, Some("user3@example.com".into())); } -test_both_dbs!( - test_create_access_tokens, - test_create_access_tokens_postgres, - test_create_access_tokens_sqlite -); - -async fn test_create_access_tokens(db: &Arc) { - let user_1 = db - .create_user( - "u1@example.com", - None, - false, - NewUserParams { - github_login: "u1".into(), - github_user_id: 1, - }, - ) - .await - .unwrap() - .user_id; - let user_2 = db - .create_user( - "u2@example.com", - None, - false, - NewUserParams { - github_login: "u2".into(), - github_user_id: 2, - }, - ) - .await - .unwrap() - .user_id; - - let token_1 = db.create_access_token(user_1, None, "h1", 2).await.unwrap(); - let token_2 = db.create_access_token(user_1, None, "h2", 2).await.unwrap(); - assert_eq!( - db.get_access_token(token_1).await.unwrap(), - access_token::Model { - id: token_1, - user_id: user_1, - impersonated_user_id: None, - hash: "h1".into(), - } - ); - assert_eq!( - db.get_access_token(token_2).await.unwrap(), - access_token::Model { - id: token_2, - user_id: user_1, - impersonated_user_id: None, - hash: "h2".into() - } - ); - - let token_3 = db.create_access_token(user_1, None, "h3", 2).await.unwrap(); - assert_eq!( - db.get_access_token(token_3).await.unwrap(), - access_token::Model { - id: token_3, - user_id: user_1, - impersonated_user_id: None, - hash: "h3".into() - } - ); - assert_eq!( - db.get_access_token(token_2).await.unwrap(), - access_token::Model { - id: token_2, - user_id: user_1, - impersonated_user_id: None, - hash: "h2".into() - } - ); - assert!(db.get_access_token(token_1).await.is_err()); - - let token_4 = db.create_access_token(user_1, None, "h4", 2).await.unwrap(); - assert_eq!( - db.get_access_token(token_4).await.unwrap(), - access_token::Model { - id: token_4, - user_id: user_1, - impersonated_user_id: None, - hash: "h4".into() - } - ); - assert_eq!( - db.get_access_token(token_3).await.unwrap(), - access_token::Model { - id: token_3, - user_id: user_1, - impersonated_user_id: None, - hash: "h3".into() - } - ); - assert!(db.get_access_token(token_2).await.is_err()); - assert!(db.get_access_token(token_1).await.is_err()); - - // An access token for user 2 impersonating user 1 does not - // count against user 1's access token limit (of 2). - let token_5 = db - .create_access_token(user_2, Some(user_1), "h5", 2) - .await - .unwrap(); - assert_eq!( - db.get_access_token(token_5).await.unwrap(), - access_token::Model { - id: token_5, - user_id: user_2, - impersonated_user_id: Some(user_1), - hash: "h5".into() - } - ); - assert_eq!( - db.get_access_token(token_3).await.unwrap(), - access_token::Model { - id: token_3, - user_id: user_1, - impersonated_user_id: None, - hash: "h3".into() - } - ); - - // Only a limited number (2) of access tokens are stored for user 2 - // impersonating other users. - let token_6 = db - .create_access_token(user_2, Some(user_1), "h6", 2) - .await - .unwrap(); - let token_7 = db - .create_access_token(user_2, Some(user_1), "h7", 2) - .await - .unwrap(); - assert_eq!( - db.get_access_token(token_6).await.unwrap(), - access_token::Model { - id: token_6, - user_id: user_2, - impersonated_user_id: Some(user_1), - hash: "h6".into() - } - ); - assert_eq!( - db.get_access_token(token_7).await.unwrap(), - access_token::Model { - id: token_7, - user_id: user_2, - impersonated_user_id: Some(user_1), - hash: "h7".into() - } - ); - assert!(db.get_access_token(token_5).await.is_err()); - assert_eq!( - db.get_access_token(token_3).await.unwrap(), - access_token::Model { - id: token_3, - user_id: user_1, - impersonated_user_id: None, - hash: "h3".into() - } - ); -} - test_both_dbs!( test_add_contacts, test_add_contacts_postgres,