fix(identity-wallet): address code review feedback for pds_client

+99 -76

1 changed file

expand all

apps

identity-wallet

src-tauri

src

pds_client.rs

+99 -76

apps/identity-wallet/src-tauri/src/pds_client.rs

··· 14 14 /// 15 15 /// Serializes to frontend with `#[serde(tag = "code", rename_all = "SCREAMING_SNAKE_CASE")]`, 16 16 /// matching the `OAuthError` / `IdentityStoreError` pattern. 17 - #[derive(Debug, PartialEq, Serialize)] 17 + #[derive(Debug, thiserror::Error, Serialize)] 18 18 #[serde(tag = "code", rename_all = "SCREAMING_SNAKE_CASE")] 19 19 pub enum PdsClientError { 20 20 /// Neither DNS nor HTTP resolution succeeded for the handle. 21 + #[error("handle not found")] 21 22 HandleNotFound, 22 23 23 24 /// plc.directory returned 404 for the DID. 25 + #[error("did not found")] 24 26 DidNotFound, 25 27 26 28 /// PDS endpoint is down or unreachable. 29 + #[error("pds unreachable: {reason}")] 27 30 PdsUnreachable { 28 31 /// Reason for unreachability (transport error, connection refused, etc.). 29 32 /// Not serialized to frontend (serde skip). 30 33 #[serde(skip)] 31 - source: String, 34 + reason: String, 32 35 }, 33 36 34 37 /// Transport-level failure (DNS timeout, connection refused, etc.). 38 + #[error("network error: {message}")] 35 39 NetworkError { message: String }, 36 40 37 41 /// Response body couldn't be parsed or was missing expected fields. 42 + #[error("invalid response: {message}")] 38 43 InvalidResponse { message: String }, 39 44 40 45 /// PAR or token exchange failed. 46 + #[error("oauth failed: {message}")] 41 47 OAuthFailed { message: String }, 42 48 } 43 49 44 - impl std::fmt::Display for PdsClientError { 45 - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { 46 - match self { 47 - Self::HandleNotFound => write!(f, "handle not found"), 48 - Self::DidNotFound => write!(f, "did not found"), 49 - Self::PdsUnreachable { source } => write!(f, "pds unreachable: {}", source), 50 - Self::NetworkError { message } => write!(f, "network error: {}", message), 51 - Self::InvalidResponse { message } => write!(f, "invalid response: {}", message), 52 - Self::OAuthFailed { message } => write!(f, "oauth failed: {}", message), 53 - } 54 - } 55 - } 56 - 57 - impl std::error::Error for PdsClientError {} 58 - 59 50 /// PLC directory DID document response. 60 51 /// 61 52 /// Returned from `GET {plc_directory_url}/{did}`. ··· 145 136 /// 146 137 /// Stateless except for the HTTP client which pools connections. 147 138 #[allow(dead_code)] 139 + // TODO(Phase 4): Remove #[allow(dead_code)] once Tauri commands call PdsClient methods 148 140 pub struct PdsClient { 149 141 client: Client, 150 142 plc_directory_url: String, ··· 255 247 .timeout(Duration::from_secs(5)) 256 248 .build() 257 249 .map_err(|e| PdsClientError::PdsUnreachable { 258 - source: format!("failed to create HTTP client: {}", e), 250 + reason: format!("failed to create HTTP client: {}", e), 259 251 })?; 260 252 261 253 timeout_client ··· 263 255 .send() 264 256 .await 265 257 .map_err(|e| PdsClientError::PdsUnreachable { 266 - source: format!("failed to reach PDS endpoint: {}", e), 258 + reason: format!("failed to reach PDS endpoint: {}", e), 267 259 })?; 268 260 269 261 Ok((pds_endpoint.to_string(), doc)) ··· 287 279 .send() 288 280 .await 289 281 .map_err(|e| PdsClientError::PdsUnreachable { 290 - source: format!("failed to fetch OAuth metadata: {}", e), 282 + reason: format!("failed to fetch OAuth metadata: {}", e), 291 283 })?; 292 284 293 285 if !response.status().is_success() { 294 286 return Err(PdsClientError::PdsUnreachable { 295 - source: format!( 287 + reason: format!( 296 288 "OAuth metadata fetch returned {} from {}", 297 289 response.status(), 298 290 pds_url ··· 370 362 .client 371 363 .post(&par_url) 372 364 .header("DPoP", dpop_proof) 373 - .header("Content-Type", "application/x-www-form-urlencoded") 374 365 .form(&form_data) 375 366 .send() 376 367 .await ··· 427 418 self.client 428 419 .post(token_url) 429 420 .header("DPoP", dpop_proof) 430 - .header("Content-Type", "application/x-www-form-urlencoded") 431 421 .form(&form_data) 432 422 .send() 433 423 .await ··· 559 549 &serde_json::json!({}), 560 550 ) 561 551 .await 562 - .map_err(|_| PdsClientError::NetworkError { 563 - message: "request_plc_operation_signature failed".to_string(), 552 + .map_err(|e| PdsClientError::NetworkError { 553 + message: format!("request_plc_operation_signature failed: {}", e), 564 554 })?; 565 555 566 556 if resp.status().is_success() { ··· 580 570 let resp = client 581 571 .post("/xrpc/com.atproto.identity.signPlcOperation", request) 582 572 .await 583 - .map_err(|_| PdsClientError::NetworkError { 584 - message: "sign_plc_operation failed".to_string(), 573 + .map_err(|e| PdsClientError::NetworkError { 574 + message: format!("sign_plc_operation failed: {}", e), 585 575 })?; 586 576 587 577 if !resp.status().is_success() { ··· 604 594 let resp = client 605 595 .get("/xrpc/com.atproto.identity.getRecommendedDidCredentials") 606 596 .await 607 - .map_err(|_| PdsClientError::NetworkError { 608 - message: "get_recommended_did_credentials failed".to_string(), 597 + .map_err(|e| PdsClientError::NetworkError { 598 + message: format!("get_recommended_did_credentials failed: {}", e), 609 599 })?; 610 600 611 601 if !resp.status().is_success() { ··· 683 673 .json_body(did_doc_json); 684 674 }); 685 675 686 - // Mock the PDS reachability check 676 + // Mock the PDS reachability check (HEAD request for the service endpoint) 687 677 mock_server.mock(|when, then| { 688 - when.method(GET).path("/pds"); 678 + when.method(httpmock::Method::HEAD).path("/pds"); 689 679 then.status(200); 690 680 }); 691 681 ··· 969 959 } 970 960 } 971 961 972 - /// AC3.2: HTTP response trimming logic verification 973 - /// 974 - /// Verifies that HTTP responses with leading/trailing whitespace 975 - /// are correctly trimmed to just the DID value. 976 - #[test] 977 - fn test_http_response_parsing_with_whitespace() { 978 - // This test verifies the trim logic works correctly 979 - let test_cases = vec![ 980 - ("did:plc:test123", "did:plc:test123"), 981 - (" did:plc:test123 ", "did:plc:test123"), 982 - ("\ndid:plc:test123\n", "did:plc:test123"), 983 - ("\t did:plc:test123 \t", "did:plc:test123"), 984 - ]; 962 + // ============================================================================ 963 + // TASK 2 & 3: resolve_handle tests with httpmock 964 + // ============================================================================ 965 + 966 + /// AC3.2: HTTP fallback resolves handle to DID 967 + #[tokio::test] 968 + async fn test_try_resolve_http_success() { 969 + let mock_server = MockServer::start(); 970 + 971 + // Mock server returns a valid DID on the well-known endpoint 972 + mock_server.mock(|when, then| { 973 + when.method(httpmock::Method::GET) 974 + .path("/.well-known/atproto-did"); 975 + then.status(200).body("did:plc:test123"); 976 + }); 977 + 978 + let client = reqwest::Client::new(); 979 + let url = format!("{}/.well-known/atproto-did", mock_server.base_url()); 980 + let result = try_resolve_http(&client, &url).await; 981 + 982 + assert!(result.is_ok()); 983 + assert_eq!(result.unwrap(), Some("did:plc:test123".to_string())); 984 + } 985 + 986 + /// AC3.2: HTTP fallback with whitespace trimming 987 + #[tokio::test] 988 + async fn test_try_resolve_http_with_whitespace() { 989 + let mock_server = MockServer::start(); 990 + 991 + // Mock server returns DID with surrounding whitespace 992 + mock_server.mock(|when, then| { 993 + when.method(httpmock::Method::GET) 994 + .path("/.well-known/atproto-did"); 995 + then.status(200).body(" did:plc:test123\n "); 996 + }); 997 + 998 + let client = reqwest::Client::new(); 999 + let url = format!("{}/.well-known/atproto-did", mock_server.base_url()); 1000 + let result = try_resolve_http(&client, &url).await; 985 1001 986 - for (input, expected) in test_cases { 987 - let trimmed = input.trim().to_string(); 988 - assert_eq!(trimmed, expected); 989 - } 1002 + assert!(result.is_ok()); 1003 + assert_eq!(result.unwrap(), Some("did:plc:test123".to_string())); 990 1004 } 991 1005 992 - /// AC3.2 & AC3.3: Test resolve_handle with fake handles 993 - /// 994 - /// These tests verify the orchestration logic without actual network access. 995 - /// They test that resolve_handle returns HANDLE_NOT_FOUND when both DNS and HTTP fail. 1006 + /// AC3.3: HTTP fallback returns Ok(None) on 404 996 1007 #[tokio::test] 997 - async fn test_resolve_handle_orchestration_with_nonexistent_handle() { 998 - let client = PdsClient::new(); 1008 + async fn test_try_resolve_http_not_found() { 1009 + let mock_server = MockServer::start(); 999 1010 1000 - // Use a handle that will fail both DNS and HTTP (valid domain structure but non-existent) 1001 - let result = client 1002 - .resolve_handle("test-nonexistent-12345.example.com") 1003 - .await; 1011 + // Mock server returns 404 1012 + mock_server.mock(|when, then| { 1013 + when.method(httpmock::Method::GET) 1014 + .path("/.well-known/atproto-did"); 1015 + then.status(404); 1016 + }); 1004 1017 1005 - // Should return HandleNotFound since both DNS and HTTP will fail 1006 - match result { 1007 - Err(PdsClientError::HandleNotFound) => { 1008 - // Correct: both methods returned None 1009 - } 1010 - Ok(did) => { 1011 - panic!("Unexpected success: got {}", did); 1012 - } 1013 - Err(e) => { 1014 - // Could be network error if network is completely unavailable 1015 - // but the pattern should eventually return HandleNotFound 1016 - eprintln!("Got different error (may be expected in sandbox): {}", e); 1017 - } 1018 - } 1018 + let client = reqwest::Client::new(); 1019 + let url = format!("{}/.well-known/atproto-did", mock_server.base_url()); 1020 + let result = try_resolve_http(&client, &url).await; 1021 + 1022 + assert!(result.is_ok()); 1023 + assert_eq!(result.unwrap(), None); 1024 + } 1025 + 1026 + /// AC3.3: HTTP fallback returns Ok(None) on 500 1027 + #[tokio::test] 1028 + async fn test_try_resolve_http_server_error() { 1029 + let mock_server = MockServer::start(); 1030 + 1031 + // Mock server returns 500 1032 + mock_server.mock(|when, then| { 1033 + when.method(httpmock::Method::GET) 1034 + .path("/.well-known/atproto-did"); 1035 + then.status(500); 1036 + }); 1037 + 1038 + let client = reqwest::Client::new(); 1039 + let url = format!("{}/.well-known/atproto-did", mock_server.base_url()); 1040 + let result = try_resolve_http(&client, &url).await; 1041 + 1042 + assert!(result.is_ok()); 1043 + assert_eq!(result.unwrap(), None); 1019 1044 } 1020 1045 1021 1046 // ============================================================================ ··· 1030 1055 let mock_par = mock_server.mock(|when, then| { 1031 1056 when.method(httpmock::Method::POST) 1032 1057 .path("/oauth/par") 1033 - .header_exists("DPoP") 1034 - .header_exists("Content-Type"); 1058 + .header_exists("DPoP"); 1035 1059 then.status(200).json_body(serde_json::json!({ 1036 1060 "request_uri": "urn:ietf:params:oauth:request_uri:test", 1037 1061 "expires_in": 60 ··· 1161 1185 mock_server.mock(|when, then| { 1162 1186 when.method(httpmock::Method::POST) 1163 1187 .path("/oauth/token") 1164 - .header_exists("DPoP") 1165 - .header_exists("Content-Type"); 1188 + .header_exists("DPoP"); 1166 1189 then.status(200).json_body(serde_json::json!({ 1167 1190 "access_token": "test_access_token", 1168 1191 "token_type": "DPoP",

Configure Feed

Configure Feed