Extract SecureArea-specific UI into an interface #393
Conversation
mitrejcevski
force-pushed
the
feat/common-interface
branch
from
3b2ee24 to
a20eed1
Compare
|
|
||
| interface SecureAreaSupport { | ||
|
|
||
| fun createKeyUnlockData(value: String): SecureArea.KeyUnlockData |
There was a problem hiding this comment.
This still leaves SecureArea-specific code in the app (in AuthConfirmationFragment.kt for example) and I think this could be abstracted even further.
Basically, what we need here is a method that will make the user do all the necessary things to unlock the key and it should return a SecureArea.KeyUnlockData. The AndroidKeystoreSecureARea implementation would do the BiometricPrompt thing, the BouncyCastleSecureArea implementation would do a passphrase prompt, and something like a ExternalSecurityKeySecureArea (using e.g. a FIDO key) would prompt the user to touch their Security Key. This operation can fail or be canceled, I don't think we need to distinguish between the two so just returning null is sufficient to convey that, I think.
So probably this should be simply be called
fun unlockKey(alias: String): SecureArea.KeyUnlockData?
|
|
||
| @Composable | ||
| fun SecureAreaAuthUi() | ||
| } |
There was a problem hiding this comment.
Normally I'm fine with not having documentation for non-library code... but this is one of those cases where I think it would be helpful to have full doc coverage of everything here.
| import com.android.identity.wallet.composables.state.MdocAuthOption | ||
|
|
||
| class AndroidSecureAreaSupport( | ||
| private val capabilities: KeystoreUtil.DeviceCapabilities |
There was a problem hiding this comment.
I know it's a longer word but I think calling it AndroidKeystoreSecureAreaSupport is better and also more precise. Ditto for AndroidKeystoreSecureAreaSupportState.
| createAndroidKeystoreSettings( | ||
| keyInfo.isUserAuthenticationRequired, | ||
| AddSelfSignedScreenState.MdocAuthStateOption.valueOf(mDocAuthOption), | ||
| MdocAuthStateOption.valueOf(mDocAuthOption), | ||
| keyInfo.userAuthenticationTimeoutMillis, | ||
| keyInfo.userAuthenticationType, | ||
| keyInfo.isStrongBoxBacked, |
There was a problem hiding this comment.
Looks like there's still some AndroidKeystoreSecureArea and BouncyCastleSecureArea specific code in here. That should all be moved to the relevant implementations of SecureAreaSupport interface.
mitrejcevski
force-pushed
the
feat/common-interface
branch
4 times, most recently
from
db4ba90 to
4c574f8
Compare
mitrejcevski
force-pushed
the
feat/common-interface
branch
from
4c574f8 to
2cd1eb6
Compare
| is SoftwareSecureArea -> R.string.keystore_software | ||
| else -> throw IllegalStateException("Unknown Secure Area Implementation") | ||
| } | ||
| } |
There was a problem hiding this comment.
Instead of this, add getDisplayName(): String on SecureArea interface in the library (and implement it for the two implementations we have) and use that instead. Yes, there are i18n issues with that but we can live with that (I don't think there's any way around doing i18n at the library level).
| is AndroidKeystoreSecureArea -> SecureAreaImplementationState.Android | ||
| is SoftwareSecureArea -> SecureAreaImplementationState.Software | ||
| else -> throw IllegalStateException("Unknown Secure Area Implementation") | ||
| } |
There was a problem hiding this comment.
We can't throw here, it won't work if using a SecureArea the application doesn't yet know about.
| val softwareSecureArea = SoftwareSecureArea(storageEngine) | ||
|
|
||
| keystoreEngineRepository.addImplementation(androidKeystoreSecureArea) | ||
| keystoreEngineRepository.addImplementation(softwareSecureArea) |
There was a problem hiding this comment.
I don't think this is the right place to populate the SecureArea implementations the application wishes to use. I think that belongs in HolderApp.onCreate().
| @@ -14,7 +14,7 @@ import com.android.identity.wallet.theme.YellowGradient | |||
| fun keystoreNameFor(implementation: SecureAreaImplementationState): Int { | |||
| return when (implementation) { | |||
| is SecureAreaImplementationState.Android -> R.string.keystore_android | |||
| is SecureAreaImplementationState.BouncyCastle -> R.string.keystore_bouncy_castle | |||
| is SecureAreaImplementationState.Software -> R.string.keystore_software | |||
| } | |||
There was a problem hiding this comment.
I don't see how this is going to work, I think SecureAreaImplementationState will need to carry the name of the Secure Area...
mitrejcevski
force-pushed
the
feat/common-interface
branch
2 times, most recently
from
3f03b49 to
600f62d
Compare
mitrejcevski
force-pushed
the
feat/common-interface
branch
from
600f62d to
7e0960f
Compare
mitrejcevski
force-pushed
the
feat/common-interface
branch
from
7e0960f to
0d59f83
Compare
Fixes #380