[fix] SDKS-5172 Defensive recovery for EncryptorError after iCloud device migration

CHANGELOG.md

@@ -1,4 +1,7 @@
 ## [Unreleased]
+#### Fixed
+- Fixed permanent authentication failure after iCloud device migration caused by Secure Enclave key mismatch [SDKS-5172]
+
 #### Added
 - Routed FIDO ceremony logs through the workflow logger by adding a `logger:` parameter to `Fido.register` and `Fido.authenticate`. The DaVinci collectors and Journey callbacks pass the workflow's configured logger so FIDO ceremony state transitions and errors emit through the same logger as the surrounding flow [SDKS-4924]
 

Oidc/Oidc/OidcClient.swift

@@ -11,8 +11,9 @@
 
 import Foundation
 import PingLogger
-import PingOrchestrate
 import PingNetwork
+import PingOrchestrate
+import PingStorage
 
 /// Class representing an OpenID Connect client.
 /// - Property pkce: PKCE  object used for the Authorization call.
@@ -115,23 +116,44 @@ public class OidcClient {
 
         config.logger.i("Getting access token")
         do {
-            if let cached = try await config.storage.get() {
-                if !cached.isExpired(threshold: config.refreshThreshold) {
-                    config.logger.i("Token is not expired. Returning cached token.")
-                    return .success(cached)
-                }
-                config.logger.i("Token is expired. Attempting to refresh.")
-                if let cachedefreshToken = cached.refreshToken {
-                    do {
-                        let refreshedToken = try await refreshToken(cachedefreshToken)
-                        return .success(refreshedToken)
-                    } catch {
-                        config.logger.e("Failed to refresh token. Revoking token and re-authenticating.", error: error)
-                        await revoke(cached)
+            do {
+                if let cached = try await config.storage.get() {
+                    if !cached.isExpired(threshold: config.refreshThreshold) {
+                        config.logger.i("Token is not expired. Returning cached token.")
+                        return .success(cached)
                     }
+                    config.logger.i("Token is expired. Attempting to refresh.")
+                    if let cachedefreshToken = cached.refreshToken {
+                        do {
+                            let refreshedToken = try await refreshToken(cachedefreshToken)
+                            return .success(refreshedToken)
+                        } catch {
+                            config.logger.e("Failed to refresh token. Revoking token and re-authenticating.", error: error)
+                            await revoke(cached)
+                        }
+                    }
+                }
+            } catch let error where error is EncryptorError || error is DecodingError {
+                // The cached bytes are unreadable — either the Secure Enclave key did not
+                // migrate with an iCloud/Quick Start device transfer (EncryptorError), or the
+                // stored payload is corrupt / from an incompatible version (DecodingError).
+                // Clear the unreadable token and fall through to re-authenticate. Other errors
+                // (e.g. errSecInteractionNotAllowed while the device is locked) are transient
+                // and must NOT delete a potentially valid token — they propagate to the outer
+                // catch unchanged.
+                config.logger.w("Cached token is unreadable (device migration or corrupt payload). Clearing corrupted token and re-authenticating.", error: error)
+                do {
+                    try await config.storage.delete()
+                } catch {
+                    // If we cannot clear the corrupted token, re-authenticating would persist a
+                    // new token over (or alongside) the unreadable one and the next token() call
+                    // would loop on the same failure. Surface a real error instead of looping.
+                    config.logger.e("Failed to clear unreadable token. Aborting to avoid a silent retry loop.", error: error)
+                    return .failure(OidcError.authorizeError(cause: error))
                 }
+                // fall through to re-authenticate below
             }
-            
+
             // Authenticate the user
             guard let agent = config.agent else {
                 return .failure(OidcError.authorizeError(message: "Agent not configured"))
@@ -191,7 +213,21 @@ public class OidcClient {
     private func revoke(_ token: Token? = nil) async {
         var accessToken = token
         if accessToken == nil {
-            accessToken = try? await config.storage.get()
+            do {
+                accessToken = try await config.storage.get()
+            } catch where error is EncryptorError || error is DecodingError {
+                // The stored token is permanently unreadable (Secure Enclave key did not migrate,
+                // or the payload is corrupt). There is nothing to revoke on the server, so just
+                // clear the dead entry.
+                config.logger.w("Stored token is unreadable. Clearing corrupted token.", error: error)
+                try? await config.storage.delete()
+                return
+            } catch {
+                // Transient failure (e.g. errSecInteractionNotAllowed while the device is locked).
+                // The token may well be valid — do NOT delete it. Skip this revoke attempt.
+                config.logger.w("Failed to read token for revocation. Leaving stored token intact.", error: error)
+                return
+            }
         }
         if let token = accessToken {
             do {

Oidc/OidcTests/OidcClientTests.swift

@@ -318,6 +318,92 @@ final class OidcClientTests: XCTestCase {
         XCTAssertTrue(revokeCalled, "The /revoke endpoint was not called.")
     }
 
+    // TestRailCase — SDKS-5172 recovery path: corrupted token triggers delete + re-auth
+    func testTokenRecoveryOnDecryptionFailure() async throws {
+        // Arrange: storage that throws EncryptorError.failedToDecrypt on the first get()
+        let throwingStorage = ThrowingMockStorage<Token>()
+        await throwingStorage.throwingMock.set(throwOnGet: true)
+        oidcClientConfig.storage = throwingStorage
+
+        // MockURLProtocol is already configured in setUp() to return valid discovery + token
+        // responses, so re-authentication will succeed after the corrupted token is cleared.
+
+        // Act
+        let result = await oidcClient.token()
+
+        // Assert: re-authentication succeeded
+        switch result {
+        case .success(let token):
+            XCTAssertEqual(token.accessToken, "Dummy AccessToken")
+        case .failure(let error):
+            XCTFail("Expected success after recovery but got failure: \(error)")
+        }
+
+        // Assert: the corrupted token was deleted before re-authenticating
+        let deleted = await throwingStorage.throwingMock.deleteWasCalled
+        XCTAssertTrue(deleted, "delete() should have been called to clear the corrupted token")
+    }
+
+    // TestRailCase — SDKS-5172: if delete() fails during recovery, token() must surface a
+    // failure rather than re-authenticating and looping forever on the same corrupted token.
+    func testTokenRecoveryFailsWhenDeleteFails() async throws {
+        let throwingStorage = ThrowingMockStorage<Token>()
+        await throwingStorage.throwingMock.set(throwOnGet: true)        // EncryptorError on get()
+        await throwingStorage.throwingMock.set(throwOnDelete: true)     // delete() cannot clear it
+        oidcClientConfig.storage = throwingStorage
+
+        let result = await oidcClient.token()
+
+        switch result {
+        case .success:
+            XCTFail("Expected failure when the corrupted token cannot be cleared")
+        case .failure(let error):
+            // Should be the authorizeError wrapping the delete failure, not a silent re-auth.
+            if case .authorizeError = error {
+                // expected
+            } else {
+                XCTFail("Expected .authorizeError but got \(error)")
+            }
+        }
+    }
+
+    // TestRailCase — SDKS-5172: a non-EncryptorError/non-DecodingError from get() is transient
+    // and must NOT trigger recovery (no delete, no re-auth) — it surfaces as a failure with the
+    // stored token left intact.
+    func testTokenDoesNotRecoverOnTransientStorageError() async throws {
+        let throwingStorage = ThrowingMockStorage<Token>()
+        await throwingStorage.throwingMock.set(getError: TransientStorageError.interactionNotAllowed)
+        await throwingStorage.throwingMock.set(throwOnGet: true)
+        oidcClientConfig.storage = throwingStorage
+
+        let result = await oidcClient.token()
+
+        switch result {
+        case .success:
+            XCTFail("Expected failure on a transient storage error")
+        case .failure:
+            break
+        }
+
+        // The transient error must not have deleted the (potentially valid) token.
+        let deleted = await throwingStorage.throwingMock.deleteWasCalled
+        XCTAssertFalse(deleted, "delete() must not be called on a transient storage error")
+    }
+
+    // TestRailCase — SDKS-5172: revoke() with a transient read error must leave the stored token
+    // intact (it may be valid) rather than deleting it.
+    func testRevokeLeavesTokenIntactOnTransientStorageError() async throws {
+        let throwingStorage = ThrowingMockStorage<Token>()
+        await throwingStorage.throwingMock.set(getError: TransientStorageError.interactionNotAllowed)
+        await throwingStorage.throwingMock.set(throwOnGet: true)
+        oidcClientConfig.storage = throwingStorage
+
+        await oidcClient.revoke()
+
+        let deleted = await throwingStorage.throwingMock.deleteWasCalled
+        XCTAssertFalse(deleted, "revoke() must not delete the token on a transient storage error")
+    }
+
     private func makeClient(config: HttpClientConfig = HttpClientConfig()) -> URLSessionHttpClient {
         let sessionConfig = URLSessionConfiguration.ephemeral
         sessionConfig.protocolClasses = [MockURLProtocol.self]

Oidc/OidcTests/mock/MockStorage.swift

@@ -2,7 +2,7 @@
 //  MockStorage.swift
 //  OidcTests
 //
-//  Copyright (c) 2024 - 2025 Ping Identity Corporation. All rights reserved.
+//  Copyright (c) 2024 - 2026 Ping Identity Corporation. All rights reserved.
 //
 //  This software may be modified and distributed under the terms
 //  of the MIT license. See the LICENSE file for details.
@@ -14,15 +14,15 @@ import PingStorage
 
 public actor Mock<T: Codable & Sendable>: Storage {
     private var data: T?
-    
+
     public func save(item: T) async throws {
         data = item
     }
-    
+
     public func get() async throws -> T?  {
         return data
     }
-    
+
     public func delete() async throws {
         data = nil
     }
@@ -34,3 +34,80 @@ public class MockStorage<T: Codable& Sendable>: StorageDelegate<T>, @unchecked S
     }
 }
 
+// MARK: - ThrowingMock
+
+/// A simple error used to simulate a transient (non-`EncryptorError`) storage failure.
+public enum TransientStorageError: Error, Sendable {
+    case interactionNotAllowed
+}
+
+/// A configurable actor-based Storage that can simulate decryption and delete failures.
+/// Used to test recovery paths where a cached token is unreadable (e.g. after device migration)
+/// and the negative paths where the failure is transient or `delete()` itself fails.
+public actor ThrowingMock<T: Codable & Sendable>: Storage {
+    private var data: T?
+
+    /// When `true`, `get()` throws `getError` to simulate a failure reading the cached token.
+    public var throwOnGet: Bool = false
+
+    /// The error thrown by `get()` when `throwOnGet` is `true`. Defaults to a decryption failure,
+    /// matching the Secure Enclave key-mismatch scenario after device migration.
+    public var getError: Error = EncryptorError.failedToDecrypt
+
+    /// When `true`, `delete()` throws `KeychainError.unableToDelete` to simulate a keychain that
+    /// cannot clear the corrupted item (e.g. errSecInteractionNotAllowed while the device is locked).
+    public var throwOnDelete: Bool = false
+
+    /// Set to `true` when `delete()` is called; allows test assertions on cleanup behaviour.
+    public var deleteWasCalled: Bool = false
+
+    public func save(item: T) async throws {
+        data = item
+    }
+
+    public func get() async throws -> T? {
+        if throwOnGet {
+            throw getError
+        }
+        return data
+    }
+
+    public func delete() async throws {
+        deleteWasCalled = true
+        if throwOnDelete {
+            throw KeychainError.unableToDelete
+        }
+        throwOnGet = false
+        data = nil
+    }
+
+    /// Convenience setter so tests can configure `throwOnGet` from outside the actor.
+    public func set(throwOnGet value: Bool) {
+        throwOnGet = value
+    }
+
+    /// Convenience setter so tests can configure `throwOnDelete` from outside the actor.
+    public func set(throwOnDelete value: Bool) {
+        throwOnDelete = value
+    }
+
+    /// Convenience setter so tests can configure the error thrown by `get()`.
+    public func set(getError value: Error) {
+        getError = value
+    }
+}
+
+/// `ThrowingMockStorage` wraps `ThrowingMock` inside a `StorageDelegate` using `.NO_CACHE`,
+/// mirroring the shape of `MockStorage`. Exposes the underlying `ThrowingMock` so tests can
+/// inspect `deleteWasCalled` and toggle `throwOnGet`.
+public class ThrowingMockStorage<T: Codable & Sendable>: StorageDelegate<T>, @unchecked Sendable {
+    /// The underlying actor; use `await throwingMock.<property>` to read test state.
+    public let throwingMock: ThrowingMock<T>
+
+    public init() {
+        let mock = ThrowingMock<T>()
+        self.throwingMock = mock
+        super.init(delegate: mock, cacheStrategy: .NO_CACHE)
+    }
+}
+

Storage/Storage/KeychainStorage.swift

@@ -30,18 +30,38 @@ public actor Keychain<T: Codable & Sendable>: Storage {
     /// - Parameter item: The item to save.
     public func save(item: T) async throws {
         let data = try JSONEncoder().encode(item)
-        var query = [
+
+        // Encrypt BEFORE deleting the existing item. If encryption fails (e.g. the Secure
+        // Enclave key is transiently unavailable under device lock or memory pressure), this
+        // throws here and the previously stored token is left untouched — never leaving the
+        // keychain slot empty.
+        let encrypted = try await encryptor.encrypt(data: data)
+
+        // Delete using primary key only (class + account + service) — no accessibility filter
+        // so ANY pre-existing item is removed regardless of its stored accessibility class.
+        // This prevents errSecDuplicateItem when upgrading from an older SDK version that
+        // stored items with the default kSecAttrAccessibleWhenUnlocked accessibility.
+        let deleteQuery = [
+            kSecClass as String: kSecClassGenericPassword,
+            kSecAttrAccount as String: account,
+            kSecAttrService as String: service
+        ] as [String: Any]
+        SecItemDelete(deleteQuery as CFDictionary)
+
+        // Add the fresh item with device-only accessibility to prevent iCloud/iTunes backup
+        // migration. kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly is chosen over
+        // kSecAttrAccessibleWhenUnlockedThisDeviceOnly to allow background token refresh
+        // (Background App Refresh, silent push) after first device unlock, matching the
+        // accessibility used by SecuredKey for SE-backed keys.
+        let addQuery = [
             kSecClass as String: kSecClassGenericPassword,
             kSecAttrAccount as String: account,
             kSecAttrService as String: service,
-            kSecValueData as String: data
+            kSecAttrAccessible as String: kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly,
+            kSecValueData as String: encrypted
         ] as [String: Any]
-
-        query[kSecValueData  as String] = try await encryptor.encrypt(data: data)
-
-        SecItemDelete(query as CFDictionary) // Remove any existing item
-        let status = SecItemAdd(query as CFDictionary, nil)
-
+
+        let status = SecItemAdd(addQuery as CFDictionary, nil)
         guard status == errSecSuccess else {
             throw KeychainError.unableToSave
         }