Implement PresentationDefinition field filter schema #102

phoebe-lew · 2023-10-27T16:20:54Z

No description provided.

- refactor satisfiesPresentationDefinition to take jwt

* main: (37 commits) refactor test payload generation fix payload generation in test simplify test change test name to be more descriptive Revert "add version hipster badge and usage instructions to README" add version hipster badge and usage instructions to README add TODO version bump appease detekt account for signatures less than 64 bytes Fix null pointer in VC verify (#94) Do also authentication by default. Apply suggestions from code review bump version to 0.0.6 updates Default ion creation sets assertion method (#93) Added more validation and tests Some fixes after testing Assertion method should be the default Better names ...

codecov · 2023-10-27T16:23:28Z

Codecov Report

Merging #102 (b71e073) into main (934eb70) will increase coverage by 1.95%.
The diff coverage is 85.18%.

@@            Coverage Diff             @@
##             main     #102      +/-   ##
==========================================
+ Coverage   70.10%   72.06%   +1.95%     
==========================================
  Files          21       21              
  Lines        1201     1217      +16     
  Branches      137      140       +3     
==========================================
+ Hits          842      877      +35     
+ Misses        292      269      -23     
- Partials       67       71       +4

Components	Coverage Δ
credentials	`75.00% <85.18%> (+7.87%)`	⬆️
crypto	`34.67% <ø> (ø)`
dids	`92.81% <ø> (ø)`
common	`60.83% <ø> (ø)`

nitro-neal · 2023-10-27T16:46:38Z

credentials/src/main/kotlin/web5/sdk/credentials/PresentationExchange.kt

-          }
-        }
-        return satisfied
+      .forEach { inputDescriptorWithFields ->


Does this cover the scenario:
if there are no filter and only a path?

it should via ln79 but will add a test :)

nitro-neal · 2023-10-27T16:48:59Z

credentials/src/test/kotlin/web5/sdk/credentials/TestData.kt

-                          }
-                      ]
-                  }
+const val TBDEX_PD = """


Should add another test where there is no filter, only a path

nitro-neal

Good job! I would say add a few more tests to make sure it works with the old test too basically ( a pd without a filter, just a path)

phoebe-lew · 2023-10-27T17:17:52Z

credentials/src/main/kotlin/web5/sdk/credentials/PresentationExchange.kt

+      }
+
+      when {
+        field.filterSchema != null -> matchedFields.any {vcSatisfiesFieldFilterSchema(it, field.filterSchema!!)}


matched field could be either an array or a single value.
if array, filterSchema needs to be applied to each item in the array. If any items satisfy the schema, return true
if single value, apply schema

matched field could also be an object

andresuribe87 · 2023-10-27T17:39:07Z

credentials/src/main/kotlin/web5/sdk/credentials/PresentationDefinition.kt

@@ -127,6 +127,7 @@ public class SubmissionRequirement(
 /**
 * Enumeration representing presentation rule options.
 */
+// TODO this does not serialize correctly but sub reqs not supported right now


I don't quite follow what this comment means. Would you mind elaborating? And perhaps also opening an issue?

Jackson does not know how to serialize/deserialize enum class Rules and enum class Optionality in a SubmissionRequirement, we probably need to write custom serializers/deserializers. However, the PEX impl throws an exception if SubmissionRequirement is present, so it doesn't seem worth writing the custom modules.

Want to have a convo with @decentralgabe around whether we should drop SubmissionRequirement from our data model entirely.

Can open an issue to track.

I'd just drop it for now - can add it back when/if needed

andresuribe87 · 2023-10-27T17:40:54Z

credentials/src/main/kotlin/web5/sdk/credentials/PresentationExchange.kt

   */
  public fun satisfiesPresentationDefinition(
-    credential: VerifiableCredential,
+    vcJwt: String,


Curious what was the motivation behind this change?

I worry that people may forget to verify the jwt, before doing anything with it. How do you propose we can handle that concern?

I think its because ssome of the filter could be for the actual payload of the vc
$.iss is the correct way to filter by issuer not vc.issuer (this will be removed as per spec)

Context starting here: https://sq-tbd.slack.com/archives/C05SXUR39FE/p1698361441821219?thread_ts=1698337236.592429&cid=C05SXUR39FE

agree w/ the concern, but it's better to decouple the logic IMO

Sounds like you're advocating for leaving as is. I remain cautious about the approach. Agree with the intent of decoupling verification logic from validation.

My proposal is to stick to the following convention:

When a function is dealing with data that's assumed to be trusted, use the VerifiableCredential type.

When a function is dealing with data that needs to be verified, use the secured representation (i.e. vcJwt or DI).

For this function, the data is assumed to be trusted already. Given that, it would fall to the first bucket.
On the other hand, the VerifiableCredential.verify function would follow the second pattern.

Alternatively, we could define types to represent:

A secured verifiable credential: JwVerifiableCredential and DataIntegrityVerifiableCredential.

An unsecured verifiable credential UnsecuredVerifiableCredential.

This would make it very difficult for users to screw up.

Either of these two would be more beneficial IMHO. Those are my suggestions, I'll leave it up to @phoebe-lew to decide whether they're useful or not.

andresuribe87 · 2023-10-27T18:02:11Z

credentials/src/main/kotlin/web5/sdk/credentials/PresentationExchange.kt

+ *
+ * @param message The error message.
+ */
+public class PresentationExchangeError(message: String) : Error(message)


Couldn't find where error is defined, is it an exception?

Error is java.lang.Error, I'm actually not sure when to use error vs exception

From https://docs.oracle.com/javase%2F7%2Fdocs%2Fapi%2F%2F/java/lang/Error.html

An Error is a subclass of Throwable that indicates serious problems that a reasonable application should not try to catch.

That sounds really scary 😱

In other PRs, we've tried to stick subclassing RuntimeException, like what's below https://github.com/TBD54566975/web5-kt/blob/02ea4b31a28cf68a4a3f9a9abc745e3273ceee79/dids/src/main/kotlin/web5/sdk/dids/DidIonManager.kt#L720

andresuribe87 · 2023-10-27T18:02:44Z

credentials/src/test/kotlin/web5/sdk/credentials/PresentationExchangeTest.kt

+  private val holderDid = DidKey.create(keyManager)
+  private val jsonMapper: ObjectMapper = ObjectMapper()
+    .registerKotlinModule()
+    .findAndRegisterModules()


Curious what this does?

Actually don't need this anymore! If we define custom serialization/deserialization modules, this tells the object mapper to pick those up. But managed to just use annotations to get the PD to serialize.

andresuribe87 · 2023-10-27T18:02:54Z

credentials/src/test/kotlin/web5/sdk/credentials/PresentationExchangeTest.kt

+  private val issuerDid = DidKey.create(keyManager)
+  private val holderDid = DidKey.create(keyManager)
+  private val jsonMapper: ObjectMapper = ObjectMapper()
+    .registerKotlinModule()


Curious what this does?

This one gets jackson's objectmapper to work nicely with kotlin

Interesting! First time i've seen this. Thanks!

andresuribe87 · 2023-10-27T18:04:51Z

credentials/src/test/kotlin/web5/sdk/credentials/TestData.kt

Personally, I think it's a better practice to commit the contents of TBDEX_PD into a separate file called tbdex_pd.json. Then you can load from disk and verify. This is advantageous because then we can start reusing test cases in other implementations.

A similar comment applies to VC_JWT.

What do you think?

sure, can do that

andresuribe87

Awesome!

andresuribe87 · 2023-10-27T21:13:41Z

credentials/src/main/kotlin/web5/sdk/credentials/PresentationExchange.kt

   */
  public fun satisfiesPresentationDefinition(
-    credential: VerifiableCredential,
+    vcJwt: String,


Sounds like you're advocating for leaving as is. I remain cautious about the approach. Agree with the intent of decoupling verification logic from validation.

My proposal is to stick to the following convention:

When a function is dealing with data that's assumed to be trusted, use the VerifiableCredential type.

When a function is dealing with data that needs to be verified, use the secured representation (i.e. vcJwt or DI).

For this function, the data is assumed to be trusted already. Given that, it would fall to the first bucket.
On the other hand, the VerifiableCredential.verify function would follow the second pattern.

Alternatively, we could define types to represent:

A secured verifiable credential: JwVerifiableCredential and DataIntegrityVerifiableCredential.

An unsecured verifiable credential UnsecuredVerifiableCredential.

This would make it very difficult for users to screw up.

Either of these two would be more beneficial IMHO. Those are my suggestions, I'll leave it up to @phoebe-lew to decide whether they're useful or not.

andresuribe87 · 2023-10-27T21:18:12Z

credentials/src/main/kotlin/web5/sdk/credentials/PresentationExchange.kt

+ *
+ * @param message The error message.
+ */
+public class PresentationExchangeError(message: String) : Error(message)


From https://docs.oracle.com/javase%2F7%2Fdocs%2Fapi%2F%2F/java/lang/Error.html

An Error is a subclass of Throwable that indicates serious problems that a reasonable application should not try to catch.

That sounds really scary 😱

In other PRs, we've tried to stick subclassing RuntimeException, like what's below https://github.com/TBD54566975/web5-kt/blob/02ea4b31a28cf68a4a3f9a9abc745e3273ceee79/dids/src/main/kotlin/web5/sdk/dids/DidIonManager.kt#L720

andresuribe87 · 2023-10-27T21:20:18Z

credentials/src/test/kotlin/web5/sdk/credentials/PresentationExchangeTest.kt

+  private val issuerDid = DidKey.create(keyManager)
+  private val holderDid = DidKey.create(keyManager)
+  private val jsonMapper: ObjectMapper = ObjectMapper()
+    .registerKotlinModule()


Interesting! First time i've seen this. Thanks!

andresuribe87 · 2023-10-27T21:21:24Z

credentials/src/test/kotlin/web5/sdk/credentials/PresentationExchangeTest.kt

+
+
+  private fun readPd(path: String): String {
+    return File(path).readText().trimIndent()


Suggested change

return File(path).readText().trimIndent()

return File(path).readText()

andresuribe87 · 2023-10-27T21:21:53Z

credentials/src/test/kotlin/web5/sdk/credentials/PresentationExchangeTest.kt

+    "eyJhbGciOiJFZERTQSJ9.eyJpc3MiOiJkaWQ6a2V5Ono2TWtrdU5tSmF0ZUNUZXI1V0JycUhCVUM0YUM3TjlOV1NyTURKNmVkQXY1V0NmMiIsInN1YiI6ImRpZDprZXk6ejZNa2t1Tm1KYXRlQ1RlcjVXQnJxSEJVQzRhQzdOOU5XU3JNREo2ZWRBdjVXQ2YyIiwidmMiOnsiQGNvbnRleHQiOlsiaHR0cHM6Ly93d3cudzMub3JnLzIwMTgvY3JlZGVudGlhbHMvdjEiXSwiaWQiOiIxNjk4NDIyNDAxMzUyIiwidHlwZSI6WyJWZXJpZmlhYmxlQ3JlZGVudGlhbCIsIlNhbmN0aW9uc0NyZWRlbnRpYWwiXSwiaXNzdWVyIjoiZGlkOmtleTp6Nk1ra3VObUphdGVDVGVyNVdCcnFIQlVDNGFDN045TldTck1ESjZlZEF2NVdDZjIiLCJpc3N1YW5jZURhdGUiOiIyMDIzLTEwLTI3VDE2OjAwOjAxWiIsImNyZWRlbnRpYWxTdWJqZWN0Ijp7ImlkIjoiZGlkOmtleTp6Nk1ra3VObUphdGVDVGVyNVdCcnFIQlVDNGFDN045TldTck1ESjZlZEF2NVdDZjIiLCJiZWVwIjoiYm9vcCJ9fX0.Xhd9nDdkGarYFr6FP7wqsgj5CK3oGTfKU2LHNMvFIsvatgYlSucShDPI8uoeJ_G31uYPke-LJlRy-WVIhkudDg"
+
+
+  private fun readPd(path: String): String {


Suggested change

private fun readPd(path: String): String {

private fun readFile(path: String): String {

phoebe-lew added 10 commits October 23, 2023 18:01

Default ion creation sets assertion method

a7516d9

Undo test changes

0cae32b

Remove newline

2efeb93

test cases

b80f4e3

IImplement pex field filter schema

5f6699c

- refactor satisfiesPresentationDefinition to take jwt

Debugging tests

c7c3121

Tests passing

69ad0f5

Remove unused test data

f8f5d24

build passing

8af4043

phoebe-lew requested review from andresuribe87, mistermoe, nitro-neal and tomdaffurn as code owners October 27, 2023 16:20

nitro-neal reviewed Oct 27, 2023

View reviewed changes

nitro-neal approved these changes Oct 27, 2023

View reviewed changes

phoebe-lew commented Oct 27, 2023

View reviewed changes

andresuribe87 reviewed Oct 27, 2023

View reviewed changes

phoebe-lew added 5 commits October 27, 2023 11:39

handle array field values correctly

85c3d1f

PR review changes

a015614

Delete unused

78151c3

Fix other test

46316ce

Remove unused object mapper config

b71e073

phoebe-lew mentioned this pull request Oct 27, 2023

Remove SubmissionRequirement from VC data model #104

Open

phoebe-lew enabled auto-merge (squash) October 27, 2023 19:21

andresuribe87 approved these changes Oct 27, 2023

View reviewed changes

phoebe-lew merged commit a644e6f into main Oct 27, 2023
4 checks passed

phoebe-lew deleted the plew.update-pd-tests branch October 27, 2023 21:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement PresentationDefinition field filter schema #102

Implement PresentationDefinition field filter schema #102

phoebe-lew commented Oct 27, 2023

codecov bot commented Oct 27, 2023 •

edited

Loading

nitro-neal Oct 27, 2023 •

edited

Loading

phoebe-lew Oct 27, 2023

nitro-neal Oct 27, 2023

nitro-neal left a comment

phoebe-lew Oct 27, 2023

phoebe-lew Oct 27, 2023

andresuribe87 Oct 27, 2023

phoebe-lew Oct 27, 2023

decentralgabe Oct 27, 2023

andresuribe87 Oct 27, 2023

nitro-neal Oct 27, 2023 •

edited

Loading

phoebe-lew Oct 27, 2023

decentralgabe Oct 27, 2023

andresuribe87 Oct 27, 2023

andresuribe87 Oct 27, 2023

phoebe-lew Oct 27, 2023

andresuribe87 Oct 27, 2023

andresuribe87 Oct 27, 2023

phoebe-lew Oct 27, 2023

andresuribe87 Oct 27, 2023

phoebe-lew Oct 27, 2023

andresuribe87 Oct 27, 2023

andresuribe87 Oct 27, 2023

phoebe-lew Oct 27, 2023

andresuribe87 left a comment

andresuribe87 Oct 27, 2023

andresuribe87 Oct 27, 2023

andresuribe87 Oct 27, 2023

andresuribe87 Oct 27, 2023

andresuribe87 Oct 27, 2023



		private fun readPd(path: String): String {
		return File(path).readText().trimIndent()

	return File(path).readText().trimIndent()
	return File(path).readText()

		"eyJhbGciOiJFZERTQSJ9.eyJpc3MiOiJkaWQ6a2V5Ono2TWtrdU5tSmF0ZUNUZXI1V0JycUhCVUM0YUM3TjlOV1NyTURKNmVkQXY1V0NmMiIsInN1YiI6ImRpZDprZXk6ejZNa2t1Tm1KYXRlQ1RlcjVXQnJxSEJVQzRhQzdOOU5XU3JNREo2ZWRBdjVXQ2YyIiwidmMiOnsiQGNvbnRleHQiOlsiaHR0cHM6Ly93d3cudzMub3JnLzIwMTgvY3JlZGVudGlhbHMvdjEiXSwiaWQiOiIxNjk4NDIyNDAxMzUyIiwidHlwZSI6WyJWZXJpZmlhYmxlQ3JlZGVudGlhbCIsIlNhbmN0aW9uc0NyZWRlbnRpYWwiXSwiaXNzdWVyIjoiZGlkOmtleTp6Nk1ra3VObUphdGVDVGVyNVdCcnFIQlVDNGFDN045TldTck1ESjZlZEF2NVdDZjIiLCJpc3N1YW5jZURhdGUiOiIyMDIzLTEwLTI3VDE2OjAwOjAxWiIsImNyZWRlbnRpYWxTdWJqZWN0Ijp7ImlkIjoiZGlkOmtleTp6Nk1ra3VObUphdGVDVGVyNVdCcnFIQlVDNGFDN045TldTck1ESjZlZEF2NVdDZjIiLCJiZWVwIjoiYm9vcCJ9fX0.Xhd9nDdkGarYFr6FP7wqsgj5CK3oGTfKU2LHNMvFIsvatgYlSucShDPI8uoeJ_G31uYPke-LJlRy-WVIhkudDg"


		private fun readPd(path: String): String {

	private fun readPd(path: String): String {
	private fun readFile(path: String): String {

Implement PresentationDefinition field filter schema #102

Implement PresentationDefinition field filter schema #102

Conversation

phoebe-lew commented Oct 27, 2023

codecov bot commented Oct 27, 2023 • edited Loading

Codecov Report

nitro-neal Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nitro-neal 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

nitro-neal Oct 27, 2023 • 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

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

andresuribe87 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

codecov bot commented Oct 27, 2023 •

edited

Loading

nitro-neal Oct 27, 2023 •

edited

Loading

nitro-neal Oct 27, 2023 •

edited

Loading