feat: TokenStorage#24
Conversation
jaredperreault-okta
force-pushed
the
jp-rn-tokenstorage
branch
3 times, most recently
from
fb18b8d to
69b8d28
Compare
jaredperreault-okta
force-pushed
the
jp-rn-tokenstorage
branch
from
69b8d28 to
e6d588d
Compare
|
|
||
| import android.content.Context | ||
| import android.content.SharedPreferences | ||
| import androidx.security.crypto.EncryptedSharedPreferences |
There was a problem hiding this comment.
EncryptedSharedPreferences module is not supported anymore. It is using Google's tink library which has issues on various devices. We'll need to write our own by using the default encryption from the device to avoid all the bugs.
There was a problem hiding this comment.
Does it make sense to use SharedPreferences or SQLlite as the backing store for this?
| // Request hardware-backed keystore if available | ||
| // Falls back to software keystore on devices without secure hardware | ||
| setIsStrongBoxBacked(false) |
There was a problem hiding this comment.
this should be set to true. based on the comments. Also in keyGenerator.generateKey() you should wrap it in a try/catch. If it fails to generate a key try again with setIsStrongBoxBacked(false). Android does not have a way to check if the device has a TPM. so fallback has to be handled with a try/catch
There was a problem hiding this comment.
I was having trouble getting things to compile when trying to export 2 bridges in the same library. This has been fixed in the follow up PR #31
There was a problem hiding this comment.
I was having trouble getting things to compile when trying to export 2 bridges in the same library. This has been fixed in the follow up PR #31
|
|
||
| @interface RCT_EXTERN_MODULE(BrowserSessionBridge, NSObject) | ||
|
|
||
| RCT_EXTERN_METHOD(launchBrowserSession:(NSString *)url |
There was a problem hiding this comment.
We may need to supply an argument indicating whether or not the user wants ephemeral sessions
| let webViewController = UIViewController() | ||
| let webView = WKWebView(frame: webViewController.view.bounds) | ||
| webView.load(URLRequest(url: urlObj)) |
There was a problem hiding this comment.
We shouldn't use WKWebView. Instead, we need to use Apple's ASWebAuthenticationSession to perform the sign-in. The use of webviews for sign-in is considered an insecure practice.
There was a problem hiding this comment.
This has been address in the follow-up #31
| try KeychainHelper.save( | ||
| service: Self.SERVICE_TOKENS, | ||
| key: id, | ||
| value: tokenData, | ||
| accessibility: kSecAttrAccessibleWhenUnlockedThisDeviceOnly | ||
| ) |
There was a problem hiding this comment.
We should consider factoring the okta-mobile-swift Keychain utilities into a separate github package, and reusing them here. Something to consider for the future ...
There was a problem hiding this comment.
We briefly discussed this a few months ago and we seemed to come to the conclusion the juice wasn't worth the squeeze. It can definitely be re-visited though
There was a problem hiding this comment.
This is good for a first pass, but we'll need a way for the developer to supply custom keychain access rules and biometric requirements. Apple provides a richer set of controls for when keychain items are accessible (e.g. when the device is locked, after first unlock, etc), as well as "access groups" (different apps / extensions can share keychain items), biometric options, etc.
There was a problem hiding this comment.
Ultimately this is a first pass at Storage for a beta release. There's definitely room for iteration and feature expansion
3 participants
This includes changes from #23
The relevant commit(s) to this PR is