CredentialTypeRepository #419

keesgeluk · 2023-11-23T13:13:05Z

Features

CredentialTypeRepository in Identity library
Self signed documents in wallet using the CredentialTypeRepository

Fixes #401

It's a good idea to open an issue first for discussion.

[x ] Tests pass

mitrejcevski

Seems all good to me.

I realized that in the SelfSignedDetailsFragment a migration to Compose to build the UI will make it much simpler, but that's outside of the scope of this PR, it should be done separately.

davidz25

The only big comment I have is how we're handling "complex" types.

Also, I would like to see this code being used in the confirmation prompt (AuthConfirmationFragment.kt function stringValueFor) and the reader.

I also note there are no unit tests at all for the library code... any ideas what kind of tests we could add there?

identity/src/main/java/com/android/identity/credentialtype/CredentialTypeRepository.kt

identity/src/main/java/com/android/identity/credentialtype/CredentialType.kt

davidz25 · 2023-11-28T20:29:47Z

identity/src/main/java/com/android/identity/credentialtype/CredentialAttributeType.kt

+    object BOOLEAN : CredentialAttributeType()
+    class StringOptions(val options: List<StringOption>): CredentialAttributeType()
+    class IntegerOptions(val options: List<IntegerOption>): CredentialAttributeType()
+    class ComplexType(val typeName: String, val isArray: Boolean): CredentialAttributeType()


I'm guessing ComplexType is a way to easily support editable driving_privileges from the "Add self-signed document" UI? So, I think that's clearly desirable although I also think we actually don't need it (most users of the "Add self-signed document" won't really edit e.g. driving_privileges). Some thoughts.

First, I think it should be done in a way so it doesn't "leak" into this system... because the main goal of this system isn't really to support "Add self-signed document" UIs. No, the main goal of the system is to support wallets which need to render consent dialogs and readers which need to render "what to request?" dialogs. It's also a goal to support issuance systems but those systems will pull data from an existing System of Record and they'll know how to craft e.g. driving_privileges.

So I think we should simply just have it be object Complex : CredentialAttributeType() which is a signal that none of STRING, NUMBER, DATE, DATE_TIME, PICTURE, BOOLEAN, StringOptions, IntegerOptions apply and the app need special handling if it wants to produce or consume data from that attribute. And then move all the complex processing of the actual type (e.g. driving_privileges) into the wallet app itself... I think that could probably be done in a pretty nice way and without the implicit relationships we have here with e.g. DrivingPrivilege.vehicle_category_code

For example the VehicleRegistration almost does not make sense without complex types. Even the personal data is part of complex types. So I think it should be desirable to have the complete data model in the CredentialTypeRepository and not only the root elements. Also to have a user friendly name for the presentation of the document in a reader app. Maybe the complex type definitions could be a separated list e.g.

class MdocNamespace( val namespace: String, val dataElements: List<MdocDataElement> val complexTypeDefinitions: List<MdocChildElement> // or another type with the identifier splitted in typeName and elementName (instead of the dot in the identifier) )

In this way you can use the dataElements in the context of "What to request" and the complexTypeDefinitions for presentation purposes.

As discussed in the call today, I think it's a problem with those (VehicleRegistration and Vaccination) data models if the data is all in complex data-types... this is because complex data types makes Selective Disclosure more difficult... none of those data models are standardized btw, and my guess is that standards committees would call this out and flatten some of the data so it works better with Selective Discloure.

So I think the way forward is to have the definition of complex data elements as part of the "Add self-signed document" UI in the application.

Ok, just moved the complex types to the holder app. Can this be merged?

identity/src/main/java/com/android/identity/credentialtype/DrivingLicense.kt

davidz25 · 2023-11-28T20:41:22Z

appholder/src/main/java/com/android/identity/wallet/util/SampleDataProvider.kt

+            is CredentialAttributeType.ComplexType -> null
+        }
+    }
+}


I really like how nice and simple SampleDataProvider is!

keesgeluk · 2023-11-29T10:55:28Z

Changed the AuthConfirmationFragment.kt code. Is it ok to create another pull request for the usage of the CredentialTypeRepository in the reader app?

davidz25 · 2023-11-29T16:03:04Z

Changed the AuthConfirmationFragment.kt code. Is it ok to create another pull request for the usage of the CredentialTypeRepository in the reader app?

Yes, that's fine!

- Self signed documents in wallet using the CredentialTypeRepository Signed-off-by: Kees Geluk <[email protected]>

Signed-off-by: Kees Geluk <[email protected]>

- Removed unused RequestDocument stuff Signed-off-by: Kees Geluk <[email protected]>

davidz25

One more round, this is just a bunch of minor changes, nothing major, looks great!

identity/src/main/java/com/android/identity/credentialtype/CredentialAttributeType.kt

identity/src/main/java/com/android/identity/credentialtype/CredentialType.kt

identity/src/main/java/com/android/identity/credentialtype/CredentialTypeRepository.kt

identity/src/main/java/com/android/identity/credentialtype/DrivingLicense.kt

identity/src/main/java/com/android/identity/credentialtype/IntegerOption.kt

Signed-off-by: Kees Geluk <[email protected]>

davidz25

Basically just nits left about documentation. Sorry for being a stickler but since this is going into a library used by multiple different downstreams this is kinda important.

davidz25 · 2023-12-04T16:29:52Z

identity/src/main/java/com/android/identity/credentialtype/CredentialTypeRepository.kt

+ * which can be added using the addCredentialType() method. Applications also may add their own
+ * Credential Types.
+ */
+object CredentialTypeRepository {


This shouldn't be a singleton (generally not a good idea for libraries).

Changed to normal class and added a singleton instance on the HolderApp

davidz25 · 2023-12-04T16:34:17Z

identity/src/main/java/com/android/identity/credentialtype/CredentialTypeRepository.kt

+ * the com.android.identity.credentialtype.knowntypes package there are well known credential types
+ * which can be added using the addCredentialType() method. Applications also may add their own
+ * Credential Types.
+ */


Nit: Whitespace! Generally, each documented item should have a short succinct one-liner (ideally less than 60 characters), then longer text in separate paragraphs. Use blank lines to separate paragraphs, e.g.

/** * Short succinct explanation of something. * * Longer paragraph, bla bla bla * * @param foo also whitespaces before params. */

Also elsewhere.

davidz25 · 2023-12-04T16:35:51Z

identity/src/main/java/com/android/identity/credentialtype/CredentialTypeRepository.kt

+/**
+ * A class that contains the metadata of Credential Types. The repository is initially empty, but in
+ * the com.android.identity.credentialtype.knowntypes package there are well known credential types
+ * which can be added using the addCredentialType() method. Applications also may add their own


I think you are missing brackets around com.android.identity.credentialtype.knowntypes and addCredentialType(). Also elsewhere.

davidz25 · 2023-12-04T16:38:15Z

identity/src/main/java/com/android/identity/credentialtype/CredentialAttribute.kt

+ * @param identifier the identifier of this attribute
+ * @param displayName the name suitable for display of the attribute
+ * @param description a description of the attribute
+ */


Nit: Don't forget periods after each sentence.

davidz25 · 2023-12-04T16:39:18Z

identity/src/main/java/com/android/identity/credentialtype/CredentialAttributeType.kt

+/**
+ * Attributes in credentials can have different types and this enumeration contains a type-system
+ * generic enough to be used across various credential formats. This is useful for wallet and reader
+ * user interfaces which wants to provide UI for inputting our displaying credentials attributes.


Nit: "or" instead of "our" in the last sentence. Also missing initial short sentence (see other comment on CredentialTypeRepository.

davidz25 · 2023-12-04T16:41:44Z

identity/src/main/java/com/android/identity/credentialtype/Options.kt

+/**
+ * Object containing reusable lists of [IntegerOption] or [StringOption]
+ */
+object Options {


This is helpful but it's a lot of data and I'm concerned some downstream consumers will not like it. Please move to knowntypes package so it'll be easier to split into a separate identity-knowntypes library in some point in the future (to save space).

Some changes on documentation Signed-off-by: Kees Geluk <[email protected]>

davidz25

LGTM, thanks!

* - CredentialTypeRepository in Identity library - Self signed documents in wallet using the CredentialTypeRepository Signed-off-by: Kees Geluk <[email protected]> * Changes on the CredentialTypeRepository based on feedback of David Signed-off-by: Kees Geluk <[email protected]> * - Moved complex types to the Holder app - Removed unused RequestDocument stuff Signed-off-by: Kees Geluk <[email protected]> * Changes based on more feedback from David Signed-off-by: Kees Geluk <[email protected]> * CredentialTypeRepository should not be a singleton. Some changes on documentation Signed-off-by: Kees Geluk <[email protected]> --------- Signed-off-by: Kees Geluk <[email protected]>

mitrejcevski approved these changes Nov 26, 2023

View reviewed changes

davidz25 reviewed Nov 28, 2023

View reviewed changes

keesgeluk added 3 commits November 30, 2023 19:54

- CredentialTypeRepository in Identity library

8413ac4

- Self signed documents in wallet using the CredentialTypeRepository Signed-off-by: Kees Geluk <[email protected]>

Changes on the CredentialTypeRepository based on feedback of David

8611cff

Signed-off-by: Kees Geluk <[email protected]>

- Moved complex types to the Holder app

f11cc6c

- Removed unused RequestDocument stuff Signed-off-by: Kees Geluk <[email protected]>

davidz25 requested changes Dec 1, 2023

View reviewed changes

Changes based on more feedback from David

a09a632

Signed-off-by: Kees Geluk <[email protected]>

davidz25 reviewed Dec 4, 2023

View reviewed changes

CredentialTypeRepository should not be a singleton.

58b7920

Some changes on documentation Signed-off-by: Kees Geluk <[email protected]>

davidz25 approved these changes Dec 5, 2023

View reviewed changes

davidz25 merged commit 0c21435 into openwallet-foundation-labs:master Dec 5, 2023
2 checks passed

keesgeluk deleted the feature-401 branch December 6, 2023 08:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CredentialTypeRepository #419

CredentialTypeRepository #419

keesgeluk commented Nov 23, 2023

mitrejcevski left a comment

davidz25 left a comment

davidz25 Nov 28, 2023

keesgeluk Nov 29, 2023

davidz25 Nov 29, 2023

keesgeluk Nov 30, 2023

davidz25 Nov 28, 2023

keesgeluk commented Nov 29, 2023

davidz25 commented Nov 29, 2023

davidz25 left a comment

davidz25 left a comment

davidz25 Dec 4, 2023

keesgeluk Dec 5, 2023

davidz25 Dec 4, 2023

keesgeluk Dec 5, 2023

davidz25 Dec 4, 2023

keesgeluk Dec 5, 2023

davidz25 Dec 4, 2023

keesgeluk Dec 5, 2023

davidz25 Dec 4, 2023

keesgeluk Dec 5, 2023

davidz25 Dec 4, 2023

keesgeluk Dec 5, 2023

davidz25 left a comment

CredentialTypeRepository #419

CredentialTypeRepository #419

Conversation

keesgeluk commented Nov 23, 2023

mitrejcevski left a comment

Choose a reason for hiding this comment

davidz25 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keesgeluk commented Nov 29, 2023

davidz25 commented Nov 29, 2023

davidz25 left a comment

Choose a reason for hiding this comment

davidz25 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidz25 left a comment

Choose a reason for hiding this comment