Feature/request option refactor #107

n0900 · 2024-08-12T22:17:27Z

Merges OidcSiopVerifier.RequestOptions and WalletService.RequestOptions into a single class and into a seperate file which also contains all helper functions which were used to deal with manipulating RequestOptions only.

Furthermore, after looking through https://identity.foundation/presentation-exchange/spec/v1.0.0/#input-descriptor-object again I found that InputDescriptor.schema is actually MUST and thus can be used to get our credential scheme way easier than previously. Maybe I have misread - please double check. This breaks two interop tests, namely 'EUDI AuthnRequest can be parsed' and 'Idemia Interop Request'.

JesusMcCloud · 2024-08-13T06:12:45Z

given the interop tests are the failing ones, I'm pretty sure we made InputDescriptor.schema optional to successfully interop with other parties. Still, eve if this is the case, we should explicitly indicate it in the code

JesusMcCloud

renaming of method required

vck-openid/src/commonMain/kotlin/at/asitplus/wallet/lib/oidc/RequestOptions.kt

n0900 · 2024-08-13T12:49:02Z

given the interop tests are the failing ones, I'm pretty sure we made InputDescriptor.schema optional to successfully interop with other parties. Still, eve if this is the case, we should explicitly indicate it in the code

I want to add that if we decide to keep the parameter nullable for interop, we can add an Exception to RequestOptions.fromInputParameters so that it still works. So either choice is fine.

n0900 · 2024-08-14T14:37:43Z

In order to unify OidcSiopVerifier.RequestOptions and WalletService.RequestOptions I added clock and set state to not nullable. This is not a real change since RequestOptions is an internal class and by default it got a value automatically - null has never been used.

JesusMcCloud · 2024-08-16T13:33:32Z

The way I read this, it seems way cleaner and makes a lot of sense. Regarding interop: Maybe warn, default to the PID schema from the implementing acts and hope for the best is a valid strategy

fix testcases Fix naming Unify RequestOptions Add test file Enforce CLA fix CHANGELOG.md Release/4.1.1 (#105) * 4.1.1: fix JSON serializer * rename kmp-crypto references to signum * easier migration fro renamed serializers

* 4.1.1: fix JSON serializer * rename kmp-crypto references to signum * easier migration fro renamed serializers (cherry picked from commit 519bb06)

n0900 · 2024-08-19T16:05:54Z

Split pull request into two. Please take a look at #109 for the second part.

JesusMcCloud

Fine for me, but I am pretty sure @nodh will tell us that the property was nullable on prupose to make interop tests green, so we he will probably request changes. The fix will probably be defaulting to EU PID if it is indeed null. Of course, this requires thorough documentation

JesusMcCloud · 2024-08-19T19:30:05Z

.github/workflows/cla.yml

just to add context for @nodh: this was required to not make github stuck, even though it is run on the target branch. makes no sense to me, but that's the way it is

nodh · 2024-08-20T08:48:54Z

Fine for me, but I am pretty sure @nodh will tell us that the property was nullable on prupose to make interop tests green, so we he will probably request changes. The fix will probably be defaulting to EU PID if it is indeed null. Of course, this requires thorough documentation

Indeed, even now, the EUDIW verifier at https://verifier.eudiw.dev/pid-full generates authn requests that do not contain a schema, i.e. click on requesting specific attributes, and expand the presentation definition to

{
	"id": "33ac795e-599b-460c-ab3d-db4c1ccf73fd",
	"input_descriptors": [
		{
			"id": "eu.europa.ec.eudi.pid.1",
			"name": "EUDI PID",
			"purpose": "We need to verify your identity",
			"format": {
				"mso_mdoc": {
					"alg": [
						"ES256",
						"ES384",
						"ES512"
					]
				}
			},
			"constraints": {
				"fields": [
					{
						"path": [
							"$['eu.europa.ec.eudi.pid.1']['age_over_18']"
						],
						"intent_to_retain": false
					}
				]
			}
		}
	]
}

But we should also not default to the PID scheme (which we can't, because that's an external artifact?)

nodh · 2024-08-20T08:38:39Z

CHANGELOG.md

+* Refactor `OidcSiopVerifier.RequestOptions` and `WalletService.RequestOptions` into one separate class `RequestOptions`
+  * Adds `InputParameter.toRequestOptions()` functionality
+
+Release 4.1.1 (Bugfix Release):


But 4.1.1 is now duplicated in the changelog?

nodh · 2024-08-20T08:39:16Z

gradle.properties

@@ -12,7 +12,7 @@ kotlin.mpp.enableCInteropCommonization=true
 kotlin.mpp.stability.nowarn=true
 kotlin.native.ignoreDisabledTargets=true

-artifactVersion = 4.2.0-SNAPSHOT
+artifactVersion = 4.1.1


Why is this 4.1.1 on a feature branch?

merging error. mb

nodh · 2024-08-20T08:41:37Z

vck-openid/src/commonMain/kotlin/at/asitplus/wallet/lib/openid/RequestOptions.kt

+)
+
+private fun ConstantIndex.CredentialScheme.toVcConstraint() = if (supportsVcJwt) ConstraintField(
+    path = listOf("$.type"), filter = ConstraintFilter(


Indendation seems off

nodh · 2024-08-20T08:41:41Z

vck-openid/src/commonMain/kotlin/at/asitplus/wallet/lib/openid/RequestOptions.kt

+    credentialScheme: ConstantIndex.CredentialScheme?,
+): Collection<ConstraintField> = map {
+    if (credentialRepresentation == ISO_MDOC) credentialScheme.toConstraintField(
+        it


Indendation seems off

nodh · 2024-08-20T08:41:53Z

vck-openid/src/commonMain/kotlin/at/asitplus/wallet/lib/openid/RequestOptions.kt

+
+private fun ConstantIndex.CredentialScheme.toSdJwtConstraint() = if (supportsSdJwt) ConstraintField(
+    path = listOf("$.vct"), filter = ConstraintFilter(
+        type = "string", pattern = sdJwtType!!


Indendation seems off

nodh · 2024-08-20T08:43:00Z

vck-openid/src/commonMain/kotlin/at/asitplus/wallet/lib/openid/RequestOptions.kt

+import kotlinx.datetime.Clock
+
+
+data class RequestOptions(


But now not all parameters here make sense for both, SIOPv2 and OID4VCI, right?

OID4VCI does not use responseMode, clientMetadataUrl and encryption, however by their default values they do not interfere with this use case (nullable) and they are ignored for all OID4VCI use-cases.
The same goes for SIOPv2 and clock. Since we never use RequestOptions as object but rather access it's members directly this does not influence the usage. Additionally we can make clock transient for serialization to make sure that it is never leaked.

Should i add this to the variable documentation?

nodh · 2024-08-20T08:50:01Z

vck/src/commonMain/kotlin/at/asitplus/wallet/lib/data/dif/InputDescriptor.kt

@@ -21,7 +21,7 @@ data class InputDescriptor(
    @SerialName("format")
    val format: FormatHolder? = null,
    @SerialName("schema")
-    val schema: Collection<SchemaReference>? = null,
+    val schema: Collection<SchemaReference>,


So (see comment) better leave it nullable?

JesusMcCloud · 2024-08-20T10:40:08Z

Fine for me, but I am pretty sure @nodh will tell us that the property was nullable on prupose to make interop tests green, so we he will probably request changes. The fix will probably be defaulting to EU PID if it is indeed null. Of course, this requires thorough documentation

Indeed, even now, the EUDIW verifier at https://verifier.eudiw.dev/pid-full generates authn requests that do not contain a schema, i.e. click on requesting specific attributes, and expand the presentation definition to
{
	"id": "33ac795e-599b-460c-ab3d-db4c1ccf73fd",
	"input_descriptors": [
		{
			"id": "eu.europa.ec.eudi.pid.1",
			"name": "EUDI PID",
			"purpose": "We need to verify your identity",
			"format": {
				"mso_mdoc": {
					"alg": [
						"ES256",
						"ES384",
						"ES512"
					]
				}
			},
			"constraints": {
				"fields": [
					{
						"path": [
							"$['eu.europa.ec.eudi.pid.1']['age_over_18']"
						],
						"intent_to_retain": false
					}
				]
			}
		}
	]
}
But we should also not default to the PID scheme (which we can't, because that's an external artifact?)

I think we need to make a general decision: do we want to be lenient or spec-conforming. This particular issue should be reported at the eudiw verifier. The simple fact that having this property simplifies processing considerably is reason enough to think that there's a very good reason why the spec demands it (assuming we did not misinterpret it).
Put differently:

Somebody is wrong on the Internet!

n0900 · 2024-08-20T12:32:06Z

Fine for me, but I am pretty sure @nodh will tell us that the property was nullable on prupose to make interop tests green, so we he will probably request changes. The fix will probably be defaulting to EU PID if it is indeed null. Of course, this requires thorough documentation

Indeed, even now, the EUDIW verifier at https://verifier.eudiw.dev/pid-full generates authn requests that do not contain a schema, i.e. click on requesting specific attributes, and expand the presentation definition to
{
	"id": "33ac795e-599b-460c-ab3d-db4c1ccf73fd",
	"input_descriptors": [
		{
			"id": "eu.europa.ec.eudi.pid.1",
			"name": "EUDI PID",
			"purpose": "We need to verify your identity",
			"format": {
				"mso_mdoc": {
					"alg": [
						"ES256",
						"ES384",
						"ES512"
					]
				}
			},
			"constraints": {
				"fields": [
					{
						"path": [
							"$['eu.europa.ec.eudi.pid.1']['age_over_18']"
						],
						"intent_to_retain": false
					}
				]
			}
		}
	]
}
But we should also not default to the PID scheme (which we can't, because that's an external artifact?)
I think we need to make a general decision: do we want to be lenient or spec-conforming. This particular issue should be reported at the eudiw verifier. The simple fact that having this property simplifies processing considerably is reason enough to think that there's a very good reason why the spec demands it (assuming we did not misinterpret it). Put differently:

Somebody is wrong on the Internet!

It turns out I was wrong on the internet since since we cannot use schema-matching according to v2.0.0 or later. We need to update our references to the presentation exchange spec

n0900 added 2 commits August 12, 2024 14:03

Refactor RequestOptions

b928a69

Fix test cases

c9a1cd4

n0900 requested review from nodh, JesusMcCloud and acrusage-iaik August 12, 2024 22:17

n0900 self-assigned this Aug 12, 2024

JesusMcCloud requested changes Aug 13, 2024

View reviewed changes

vck-openid/src/commonMain/kotlin/at/asitplus/wallet/lib/oidc/RequestOptions.kt Outdated Show resolved Hide resolved

n0900 marked this pull request as draft August 14, 2024 14:38

n0900 requested a review from JesusMcCloud August 14, 2024 14:46

n0900 force-pushed the feature/requestOptionRefactor branch from 33c8d31 to 57c23d5 Compare August 19, 2024 13:54

n0900 and others added 2 commits August 19, 2024 15:59

Extend fromInputDescriptor

84c7728

fix testcases Fix naming Unify RequestOptions Add test file Enforce CLA fix CHANGELOG.md Release/4.1.1 (#105) * 4.1.1: fix JSON serializer * rename kmp-crypto references to signum * easier migration fro renamed serializers

Release/4.1.1 (#105)

c5b6f56

* 4.1.1: fix JSON serializer * rename kmp-crypto references to signum * easier migration fro renamed serializers (cherry picked from commit 519bb06)

n0900 force-pushed the feature/requestOptionRefactor branch from 57c23d5 to c5b6f56 Compare August 19, 2024 13:59

n0900 marked this pull request as ready for review August 19, 2024 14:38

Split into two pull requests

2fd7e31

JesusMcCloud approved these changes Aug 19, 2024

View reviewed changes

nodh requested changes Aug 20, 2024

View reviewed changes

nodh reviewed Aug 20, 2024

View reviewed changes

n0900 removed the request for review from acrusage-iaik August 20, 2024 12:04

n0900 marked this pull request as draft August 20, 2024 12:32

n0900 closed this Aug 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/request option refactor #107

Feature/request option refactor #107

n0900 commented Aug 12, 2024 •

edited

Loading

JesusMcCloud commented Aug 13, 2024

JesusMcCloud left a comment

n0900 commented Aug 13, 2024

n0900 commented Aug 14, 2024

JesusMcCloud commented Aug 16, 2024

n0900 commented Aug 19, 2024

JesusMcCloud left a comment

JesusMcCloud Aug 19, 2024

nodh commented Aug 20, 2024 •

edited

Loading

nodh Aug 20, 2024

nodh Aug 20, 2024

n0900 Aug 20, 2024

nodh Aug 20, 2024

nodh Aug 20, 2024

nodh Aug 20, 2024

nodh Aug 20, 2024

n0900 Aug 20, 2024

n0900 Aug 20, 2024

nodh Aug 20, 2024

JesusMcCloud commented Aug 20, 2024

n0900 commented Aug 20, 2024

Feature/request option refactor #107

Feature/request option refactor #107

Conversation

n0900 commented Aug 12, 2024 • edited Loading

JesusMcCloud commented Aug 13, 2024

JesusMcCloud left a comment

Choose a reason for hiding this comment

n0900 commented Aug 13, 2024

n0900 commented Aug 14, 2024

JesusMcCloud commented Aug 16, 2024

n0900 commented Aug 19, 2024

JesusMcCloud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nodh commented Aug 20, 2024 • edited Loading

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

JesusMcCloud commented Aug 20, 2024

n0900 commented Aug 20, 2024

n0900 commented Aug 12, 2024 •

edited

Loading

nodh commented Aug 20, 2024 •

edited

Loading