fix(identity-wallet): address PR review feedback for identity_store

+1 -1

apps/identity-wallet/CLAUDE.md

··· 243 243 - `SessionInfo` serializes with `#[serde(rename_all = "camelCase")]` -- TypeScript receives `{ did, handle, email, emailConfirmed, didDoc }`; the TypeScript `SessionInfo` type in `ipc.ts` must match exactly 244 244 - `log_out` deletes exactly three Keychain accounts: `"oauth-access-token"`, `"oauth-refresh-token"`, `"did"` -- adding or removing items from this list changes what data survives a logout 245 245 - Keychain account `"managed-dids"` stores a JSON array of all managed DID strings (e.g. `["did:plc:abc","did:plc:def"]`); the single source of truth for which identities are registered in `IdentityStore` 246 - - Per-DID Keychain accounts follow the `"{did}:suffix"` pattern with six suffixes: `device-key` (P-256 private key scalar or SE metadata), `device-key-pub` (compressed public key, SE path only), `device-key-app-label` (SE application_label, SE path only), `did-doc` (opaque DID document JSON), `plc-log` (opaque PLC audit log JSON), `oauth-tokens` (reserved for per-DID OAuth tokens) 246 + - Per-DID Keychain accounts follow the `"{did}:suffix"` pattern with six suffixes: `device-key` (P-256 private key scalar, software path only; not written on SE path), `device-key-pub` (compressed public key, SE path only), `device-key-app-label` (SE application_label, SE path only), `did-doc` (opaque DID document JSON), `plc-log` (opaque PLC audit log JSON), `oauth-tokens` (reserved for per-DID OAuth tokens) 247 247 - `IdentityStore` P-256 multicodec prefix `[0x80, 0x24]` is duplicated from `crates/crypto/src/keys.rs` (same rationale as `device_key.rs` -- `pub(crate)` constant cannot be imported cross-crate) 248 248 249 249 ## Key Files

+56 -29

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

··· 85 85 fn load_managed_dids(&self) -> Result<Vec<String>, IdentityStoreError> { 86 86 match crate::keychain::get_item(MANAGED_DIDS_ACCOUNT) { 87 87 Ok(bytes) => serde_json::from_slice::<Vec<String>>(&bytes).map_err(|e| { 88 + tracing::error!(error = %e, "managed-dids Keychain entry contains invalid JSON"); 88 89 IdentityStoreError::SerializationError { 89 90 message: format!("failed to deserialize managed-dids: {e}"), 90 91 } ··· 140 141 141 142 /// Remove a managed identity and all associated Keychain entries. 142 143 /// 143 - /// Deletes the DID from the managed-dids index and performs best-effort 144 - /// deletion of all per-DID prefixed entries (ignores not-found errors). 144 + /// Updates the managed-dids index first, then performs best-effort deletion 145 + /// of all per-DID prefixed entries. Index-first ordering ensures that on 146 + /// partial failure the DID is unregistered (orphaned entries are benign) 147 + /// rather than registered-but-empty (confusing for callers). 145 148 /// 146 149 /// Returns `Err(IdentityNotFound)` if the DID is not in the managed list. 147 150 pub fn remove_identity(&self, did: &str) -> Result<(), IdentityStoreError> { ··· 151 154 return Err(IdentityStoreError::IdentityNotFound); 152 155 } 153 156 154 - // Delete all per-DID Keychain entries (best-effort; ignore not-found errors). 155 - let entries = vec![ 157 + // Remove DID from index first — this is the authoritative state change. 158 + dids.retain(|d| d != did); 159 + self.save_managed_dids(&dids)?; 160 + 161 + // Best-effort cleanup of per-DID Keychain entries. Not-found errors are 162 + // expected (entry may never have been created). Transient OS errors are 163 + // logged but do not fail the operation — the DID is already unregistered. 164 + let entries = [ 156 165 device_key_account(did), 157 166 device_key_pub_account(did), 158 167 device_key_app_label_account(did), ··· 162 171 ]; 163 172 164 173 for entry in entries { 165 - let _ = crate::keychain::delete_item(&entry); 174 + if let Err(e) = crate::keychain::delete_item(&entry) { 175 + if !crate::keychain::is_not_found(&e) { 176 + tracing::warn!(did = did, entry = entry, error = %e, "transient Keychain error during identity cleanup"); 177 + } 178 + } 166 179 } 167 - 168 - // Remove DID from index and save. 169 - dids.retain(|d| d != did); 170 - self.save_managed_dids(&dids)?; 171 180 172 181 Ok(()) 173 182 } ··· 298 307 Ok(bytes) => bytes, 299 308 Err(e) if crate::keychain::is_not_found(&e) => { 300 309 // No key yet — generate a new P-256 keypair via the crypto crate. 301 - let keypair = crypto::generate_p256_keypair() 302 - .map_err(|_| IdentityStoreError::KeyGenerationFailed)?; 310 + let keypair = crypto::generate_p256_keypair().map_err(|e| { 311 + tracing::error!(did = did, error = %e, "P-256 key generation failed"); 312 + IdentityStoreError::KeyGenerationFailed 313 + })?; 303 314 // to_vec(): Deref gives &[u8; 32], coerces to &[u8], allocates into Vec<u8>. 304 315 let bytes = keypair.private_key_bytes.to_vec(); 305 316 crate::keychain::store_item(&account, &bytes).map_err(|e| { ··· 318 329 319 330 // Reconstruct the public key from stored private bytes. 320 331 let signing_key = 321 - SigningKey::from_slice(&private_bytes).map_err(|_| IdentityStoreError::KeychainError { 322 - message: "invalid stored key bytes".into(), 332 + SigningKey::from_slice(&private_bytes).map_err(|e| { 333 + tracing::error!(did = did, error = %e, "stored device key bytes are not a valid P-256 scalar"); 334 + IdentityStoreError::SerializationError { 335 + message: "invalid stored key bytes".into(), 336 + } 323 337 })?; 324 338 let encoded = signing_key.verifying_key().to_encoded_point(true); // compressed (33 bytes) 325 339 let compressed = encoded.as_bytes(); ··· 394 408 Some(ProtectionMode::AccessibleWhenUnlockedThisDeviceOnly), 395 409 1 << 30, // kSecAccessControlPrivateKeyUsage 396 410 ) 397 - .map_err(|_| IdentityStoreError::KeyGenerationFailed)?; 411 + .map_err(|e| { 412 + tracing::error!(did = did, error = %e, "SecAccessControl creation failed"); 413 + IdentityStoreError::KeyGenerationFailed 414 + })?; 398 415 399 416 let mut opts = GenerateKeyOptions::default(); 400 417 opts.set_key_type(KeyType::ec()) ··· 404 421 .set_location(Location::DataProtectionKeychain) 405 422 .set_access_control(access_control); // takes ownership (by value) 406 423 407 - let priv_key = SecKey::new(&opts).map_err(|_| IdentityStoreError::KeyGenerationFailed)?; 424 + let priv_key = SecKey::new(&opts).map_err(|e| { 425 + tracing::error!(did = did, error = %e, "Secure Enclave key generation failed"); 426 + IdentityStoreError::KeyGenerationFailed 427 + })?; 408 428 409 429 // Retrieve the public key and its external representation. 410 430 // SecKeyCopyExternalRepresentation on the *public* key returns the uncompressed ··· 436 456 437 457 // Get and store application_label. Roll back pub_account if this fails. 438 458 let app_label = priv_key.application_label().ok_or_else(|| { 459 + tracing::error!(did = did, "SE key created but application_label returned None"); 439 460 let _ = crate::keychain::delete_item(&pub_account); 440 461 IdentityStoreError::KeychainError { 441 462 message: "SE key created but application_label returned None; do not retry".into(), ··· 480 501 let _ = crate::keychain::delete_item(&oauth_tokens_account(did)); 481 502 } 482 503 483 - // ── Task 1: add_identity, remove_identity, list_identities ──────────────── 504 + // ── Identity lifecycle (add / remove / list) ────────────────────────────── 484 505 485 506 #[test] 486 507 fn add_identity_and_list() { ··· 573 594 assert!(json5.contains(r#""code":"SERIALIZATION_ERROR""#)); 574 595 } 575 596 576 - // ── Task 2: get_or_create_device_key ─────────────────────────────────────── 597 + // ── Per-DID device key ───────────────────────────────────────────────────── 577 598 578 599 #[test] 579 600 fn get_or_create_device_key_success() { ··· 591 612 assert!(key.key_id.starts_with("did:key:z")); 592 613 593 614 // Validate multibase decoding to 33 bytes 594 - if let Ok(decoded) = multibase::decode(&key.multibase) { 595 - assert_eq!( 596 - decoded.1.len(), 597 - 33, 598 - "compressed P-256 point should be 33 bytes" 599 - ); 600 - } 615 + let (_, decoded) = multibase::decode(&key.multibase).expect("multibase decode failed"); 616 + assert_eq!(decoded.len(), 33, "compressed P-256 point should be 33 bytes"); 601 617 } 602 618 603 619 #[test] ··· 649 665 assert!(matches!(result, Err(IdentityStoreError::IdentityNotFound))); 650 666 } 651 667 652 - // ── Task 3: DID document and PLC log persistence ──────────────────────────── 668 + // ── Document and log persistence ─────────────────────────────────────────── 653 669 654 670 #[test] 655 671 fn did_doc_round_trip() { ··· 760 776 assert!(store.add_identity(did).is_ok()); 761 777 clear_per_did_entries(did); 762 778 763 - // Store some data. 779 + // Store some data and generate a device key. 764 780 let doc = r#"{"id":"did:plc:test1"}"#; 765 781 let log = r#"[]"#; 766 782 assert!(store.store_did_doc(did, doc).is_ok()); 767 783 assert!(store.store_plc_log(did, log).is_ok()); 768 784 769 - // Also trigger device key generation to populate private key storage. 770 - // (On simulator, this stores in the per-did:device-key account.) 771 - let _ = store.get_or_create_device_key(did); 785 + // Record the device key before removal so we can verify cleanup. 786 + let key_before = store 787 + .get_or_create_device_key(did) 788 + .expect("device key generation failed"); 772 789 773 790 // Remove the identity. 774 791 assert!(store.remove_identity(did).is_ok()); ··· 777 794 assert!(store.add_identity(did).is_ok()); 778 795 assert!(store.get_did_doc(did).unwrap().is_none()); 779 796 assert!(store.get_plc_log(did).unwrap().is_none()); 797 + 798 + // A new device key should be generated (different from the old one), 799 + // proving the old key material was cleaned up. 800 + let key_after = store 801 + .get_or_create_device_key(did) 802 + .expect("device key generation after re-add failed"); 803 + assert_ne!( 804 + key_before.multibase, key_after.multibase, 805 + "device key should differ after remove + re-add" 806 + ); 780 807 } 781 808 }

+5 -5

docs/test-plans/2026-03-28-plc-key-management.md

··· 19 19 |------|--------|----------| 20 20 | 1 | Build the app for a physical device: `cd apps/identity-wallet && cargo tauri ios build` | Build succeeds, IPA produced | 21 21 | 2 | Install the app on the physical iOS device via Xcode (Window > Devices and Simulators > Install App) | App appears on device home screen | 22 - | 3 | Launch the app. Complete relay configuration (enter a valid relay URL, e.g. the staging relay). Proceed through onboarding to the point where an identity is created (complete claim code, email, handle, password, DID ceremony steps). | Onboarding flow completes. A biometric prompt (Face ID or Touch ID) appears during the DID ceremony step when the device key is generated. | 22 + | 3 | Launch the app. Complete relay configuration (enter a valid relay URL, e.g. the staging relay). Proceed through onboarding to the point where an identity is created (complete claim code, email, handle, password, DID ceremony steps). | Onboarding flow completes. Device key generation succeeds (the SE path uses `kSecAccessControlPrivateKeyUsage` without biometric flags, so a biometric prompt is not expected during key generation itself -- it may appear during signing operations in later phases). | 23 23 | 4 | After DID ceremony succeeds, navigate to the identity detail screen (via home screen). | The device key `multibase` value is displayed, starts with `z`. The `keyId` starts with `did:key:z`. | 24 24 25 25 ## Phase 2: Secure Enclave Key Idempotency (AC2.4) ··· 34 34 35 35 | Step | Action | Expected | 36 36 |------|--------|----------| 37 - | 1 | From the home screen, add a second identity (this requires a second valid claim code and distinct email/handle from the relay). Complete the full onboarding flow for the second identity. | Second identity is created. A biometric prompt appears during device key generation. | 37 + | 1 | From the home screen, add a second identity (this requires a second valid claim code and distinct email/handle from the relay). Complete the full onboarding flow for the second identity. | Second identity is created. Device key generation succeeds without error. | 38 38 | 2 | Navigate to the second identity's detail screen and note its device key `multibase` value. | The `multibase` value differs from the first identity's value recorded in Phase 2 step 1. | 39 39 40 40 ## Phase 4: Remove Identity Cleans Up (AC2.3) ··· 52 52 53 53 1. Start with a fresh app install on a physical iOS device (delete and reinstall if previously installed). 54 54 2. Configure relay URL on first launch. 55 - 3. Create first identity (full onboarding flow). Verify biometric prompt appears, device key is generated, DID ceremony succeeds. 55 + 3. Create first identity (full onboarding flow). Verify device key is generated and DID ceremony succeeds. 56 56 4. Kill and relaunch the app. Verify the first identity's device key is unchanged (idempotency). 57 57 5. Create a second identity. Verify its device key differs from the first. 58 58 6. Store and retrieve a DID document for the first identity (if the UI supports this; otherwise verify via the DID document screen). ··· 72 72 73 73 | Acceptance Criterion | Automated Test(s) | Manual Step(s) | 74 74 |----------------------|-------------------|----------------| 75 - | AC2.1 -- add_identity stores DID + generates device key | `add_identity_and_list`, `get_or_create_device_key_success` | Phase 1 (SE path) | 75 + | AC2.1 -- add_identity stores DID; get_or_create_device_key generates per-DID device key | `add_identity_and_list`, `get_or_create_device_key_success` | Phase 1 (SE path) | 76 76 | AC2.2 -- list_identities returns all DIDs | `list_multiple_identities` | N/A | 77 77 | AC2.3 -- remove_identity removes DID + Keychain entries | `remove_identity_from_list`, `remove_identity_cleans_up_all_entries` | Phase 4 (SE cleanup) | 78 78 | AC2.4 -- get_or_create_device_key is idempotent | `get_or_create_device_key_idempotent` | Phase 2 (SE idempotency) | ··· 80 80 | AC2.6 -- DID document round-trip | `did_doc_round_trip` | N/A | 81 81 | AC2.7 -- PLC log round-trip | `plc_log_round_trip` | N/A | 82 82 | AC2.8 -- get_did_doc/get_plc_log return None when unset | `get_did_doc_returns_none_if_not_stored`, `get_plc_log_returns_none_if_not_stored` | N/A | 83 - | AC2.9 -- error cases for nonexistent/duplicate DIDs | 8 tests (see automated coverage) | N/A | 83 + | AC2.9 -- error cases for nonexistent/duplicate DIDs | 7 tests + `error_serialization` (see automated coverage) | N/A |

Configure Feed

Configure Feed