-
Notifications
You must be signed in to change notification settings - Fork 88
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
Groundwork to get issuance work in Koltin Multiplatform. #816
Conversation
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.
Looks great - some questions/comments inline - but think it's self-contained enough to be landed so we can keep iterating on it.
* Configuration for a specific secure area [SecureArea] to use. | ||
*/ | ||
@CborSerializable | ||
sealed class SecureAreaConfiguration( |
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.
Does this limit us to SecureArea
s defined in this library? I guess only if you want to use the applyConfiguration()
methods, right?
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.
With current implementation, it does limit us, but we can always define an open-ended variant that would have some basic fields + Map<String, DataItem>
as an extension point.
identity/build.gradle.kts
Outdated
@@ -68,7 +83,26 @@ kotlin { | |||
} | |||
} | |||
|
|||
val javasharedMain by creating { |
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.
can we call it javaSharedMain
(capital S)?
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.
Done
@@ -0,0 +1,46 @@ | |||
package com.android.identity.device |
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 file should be called DeviceCheck.android.kt
and the iOS one should be called DeviceCheck.ios.kt
(not all files in the project currently follow this convention)
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.
OK
println("Server: $server") | ||
} | ||
}, | ||
content = { Text("Test issuance") } |
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.
Should we call this WalletServerTest instead of IssuanceTest? I mean, as I understand it we're testing the ability to connect to a wallet server... which includes proving to the wallet server that the client is in good standing
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.
I am planning to actually finish the issuance path in the test later
generatedAt = Clock.System.now(), | ||
androidKeystoreAttestKeyAvailable = true, | ||
androidKeystoreStrongBoxAvailable = true, | ||
androidIsEmulator = false |
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.
Maybe add expect val isEmulator: Boolean
to com.android.identity.testapp.Platform
and have it be true if on Android emulator or iOS simulator?
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.
Done
|
||
expect fun platformSecureArea(): SecureArea | ||
|
||
expect fun platformKeySetting(clientId: String): CreateKeySettings |
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 not just add this to the existing Platform.kt
file in com.android.identity.testapp
package?
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.
Done
* Generates statements validating device/OS/app integrity. Details of these | ||
* statements are inherently platform-specific. | ||
*/ | ||
expect object DeviceCheck { |
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.
Since this can only be implemented on iOS and Android should we introduce a uiMain
sourceSet? That way we avoid a dummy DeviceCheck.jvm.kt
file and like also DeviceAttestationJvm.kt
...
(I'm already contemplating doing this for other things, see the mp-nfc-engagement branch...)
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.
Or maybe a better name is mobileOsMain
(naming is hard, news at 11)
431225c
to
71841fa
Compare
Signed-off-by: Peter Sorotokin <[email protected]>
71841fa
to
ad1644c
Compare
Combination of project restructuring and change in protocols that allows US to support iOS wallet app (through Kotlin Multiplatform).
identity-flow
(which is our RPC mechanism) to KMPidentity-issuance
intoidentity-issuance-api
so our wallet server RPC interface is available in KMPidentity
project as abstraction level that allows us to express attestations/assertions in cross-platform waysample.testapp
Note: this tweaks the wallet server protocol in incompatible way.
Another note: iOS attestations/assertions are generated and sent to the server, but not yet validated.
Manually tested.