-
-
Notifications
You must be signed in to change notification settings - Fork 532
feat: Improve Error Handling #762
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Added additional overloads to simplify usage. - Removed need for reThrowOnError (and "Wrapped error: " prefix).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR standardizes error handling across iOS and Android by introducing language‐independent error codes, consolidating error mapping logic, and replacing the previous CryptoFailedException with a unified KeychainException.
- Added
ERROR_CODEenum and exposed it in the public API. - Implemented platform‐specific error mappings on iOS and Android to return structured
{ code, message }. - Replaced
CryptoFailedExceptionwithKeychainExceptionthroughout Android, updating promise rejects and handlers.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/index.ts | Exported new ERROR_CODE in public API |
| src/enums.ts | Added ERROR_CODE enum with standardized codes |
| ios/RNKeychainManager/RNKeychainManager.m | Mapped LA and Sec errors to codes and unified reject logic |
| android/src/main/java/com/oblador/keychain/resultHandler/ResultHandlerNonInteractive.kt | Switched to KeychainException, removed CryptoFailedException |
| android/src/main/java/com/oblador/keychain/resultHandler/ResultHandlerInteractiveBiometric.kt | Added BiometricPrompt error‐to‐code mapping |
| android/src/main/java/com/oblador/keychain/exceptions/KeychainException.kt | Introduced KeychainException with errorCode support |
| android/src/main/java/com/oblador/keychain/exceptions/CryptoFailedException.kt | Removed legacy CryptoFailedException |
| android/src/main/java/com/oblador/keychain/cipherStorage/CipherStorageKeystoreRsaEcb.kt | Updated to throw KeychainException |
| android/src/main/java/com/oblador/keychain/cipherStorage/CipherStorageKeystoreAesGcm.kt | Updated to throw KeychainException |
| android/src/main/java/com/oblador/keychain/cipherStorage/CipherStorageKeystoreAesCbc.kt | Updated to throw KeychainException |
| android/src/main/java/com/oblador/keychain/cipherStorage/CipherStorageBase.kt | Updated exception types and error codes |
| android/src/main/java/com/oblador/keychain/cipherStorage/CipherStorage.kt | Updated throws annotations to use KeychainException |
| android/src/main/java/com/oblador/keychain/KeychainModule.kt | Refactored promise rejects and removed old error codes |
Comments suppressed due to low confidence (1)
android/src/main/java/com/oblador/keychain/resultHandler/ResultHandlerNonInteractive.kt:14
- [nitpick] When throwing
KeychainExceptionfor interactive mode requirement, consider specifying an expliciterrorCodesuch asErrors.E_AUTH_INTERACTION_NOT_ALLOWEDinstead of relying on the defaultE_INTERNAL_ERROR, to improve error handling clarity.
val failure = KeychainException("Interactive mode required but unavailable.")
| [[LAContext new] canEvaluatePolicy:LAPolicyDeviceOwnerAuthentication error:&aerr]; | ||
| if (aerr) { | ||
| return rejectWithError(reject, aerr); |
Copilot
AI
Jul 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code calls canEvaluatePolicy but ignores its boolean return value. It should capture the return (e.g., BOOL canAuthenticate = ...) and handle the case where canAuthenticate is NO even if error is nil.
| [[LAContext new] canEvaluatePolicy:LAPolicyDeviceOwnerAuthentication error:&aerr]; | |
| if (aerr) { | |
| return rejectWithError(reject, aerr); | |
| LAContext *context = [LAContext new]; | |
| BOOL canAuthenticate = [context canEvaluatePolicy:LAPolicyDeviceOwnerAuthentication error:&aerr]; | |
| if (!canAuthenticate) { | |
| NSError *authError = aerr ?: [NSError errorWithDomain:@"RNKeychainManager" code:-1 userInfo:@{NSLocalizedDescriptionKey: @"Authentication policy cannot be evaluated."}]; | |
| return rejectWithError(reject, authError); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change was to fix a scenario where we were potentially rejecting with a nil error when canAuthenticate was false.
Perhaps I should throw a custom error message when canAuthenticate is false to maintain the fix and prevent the regression 🤔
| throw CryptoFailedException("Unknown error: ${e.message}", e) | ||
| } | ||
| } | ||
| throw KeychainException(e.message, e) |
Copilot
AI
Jul 7, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Throwing a generic KeychainException with only the original exception message loses context such as the alias. Consider including a descriptive message (e.g., "Could not encrypt data for service $alias") to aid debugging.
| throw KeychainException(e.message, e) | |
| throw KeychainException("Could not encrypt data for service $safeAlias: ${e.message}", e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is perhaps an oversight I've made in other areas too. I'll look to fix this soon 🙂
| Log.e(KEYCHAIN_MODULE, e.message, e) | ||
| promise.reject(Errors.E_EMPTY_PARAMETERS, e) | ||
| } catch (e: CryptoFailedException) { | ||
| promise.reject(Errors.E_INVALID_PARAMETERS, e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why it's E_INVALID_PARAMETERS now if we are throwing this error when parameters are empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was to consolidate various cases where an exception is thrown due to invalid parameters, including cases where we get the empty parameters exception.
It's an error code that essentially signals to the developer that they've not configured the library correctly and perhaps need to use methods that help determine what's available on the users device
| val result = innerEncryptedCredentials(safeAlias, password, username) | ||
| handler.onEncrypt(result, null) | ||
| } catch (e: Exception) { | ||
| } catch (fail: Throwable) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switched to Throwable for consistency with AES Cipher implementation
|
Hey @DorianMazur 🙂 I believe I've addressed the issues raised now if you'd like to take a look 👍 I noticed an issue on Android while testing for the |
|
Hey @DorianMazur! 🙂 Have you had chance to take a look at my changes since the initial review? I'm interested to know if there's anything else you'd like me to do on this and what it would take to make it into an upcoming release! |
|
Hey @lewie9021 |
This PR introduces standardised error handling across both iOS and Android. In particular, I've tried to solve the following problems:
My approach to error codes attempts to balance cross-platform consistency for common scenarios (like user cancellation) with platform-specific codes where additional control is needed. The mappings represent my best understanding of the various failure scenarios. I'm more than happy to get feedback on whether we need additional, fewer, or different error codes to better serve the community's needs.
I've made some other notable changes in this PR that I'd like to call out, specifically:
Renamed CryptoFailedException → KeychainException
reThrowOnErrorand "Wrapped error:" prefixesRefactored iOS Error Mapping
codeForErrorandmessageForErrorto help with maintainability.LocalAuthenticationerror codes. I realise these errors will only throw when setting right now. If we ever useLAContext.canEvaluatePolicywhen getting keys in the future, we can instantly provide more granularity around scenarios such as not enrolled (i.e. user disables biometrics for the app) or not available (i.e. perhaps they've denied permission).Existing Error Codes
E_SUPPORTED_BIOMETRY_ERROR: This error code felt unnecessary as the assumption is callees wouldn't get much value out of the two distinct errors.E_EMPTY_PARAMETERS→E_INVALID_PARAMETERS: Clearer intent.E_KEYSTORE_ACCESS_ERROR→E_STORAGE_ACCESS_ERROR: Platform-agnostic given KeyStore suggests Android-only.E_UNKNOWN_ERROR→E_INTERNAL_ERROR: Unknown error suggests they're errors we're not aware of but they include custom errors directly thrown in the code. I initially had both but from a consumer perspective, they mean the same thing - something went wrong and we should probably display a generic message to a user.Usage