w3c-ccg / did-key-test-suite

Interoperability test suite for did:key
https://w3c-ccg.github.io/did-key-test-suite/
BSD 3-Clause "New" or "Revised" License
1 stars 4 forks source link

Some tests here are not specific to `did:key` #22

Open peacekeeper opened 2 years ago

peacekeeper commented 2 years ago

I see several tests of features that apply to DID Resolution in general, not just the did:key method. I think those should be moved into the DID Resolution test suite, and/or be re-used consistently across future DID method test suites as well.

Here's a list where this might be the case:

  1. The scheme MUST be the value did
  2. MUST raise invalidDid error if scheme is not did
  3. If "didDocument.id" is not a valid DID, an invalidDid error MUST be raised
  4. If verificationMethod.id is not a valid DID URL, an invalidDidUrl error MUST be raised.
  5. For Signature Verification Methods, if options.enableExperimentalPublicKeyTypes is set to false and publicKeyFormat is not Multikey, JsonWebKey2020, or Ed25519VerificationKey2020, an invalidPublicKeyType error MUST be raised.
  6. For Encryption Verification Methods, if options.enableExperimentalPublicKeyTypes is set to false and publicKeyFormat is not Multikey, JsonWebKey2020, or X25519KeyAgreementKey2020, an invalidPublicKeyType error MUST be raised.
  7. If verificationMethod.controller is not a valid DID, an invalidDid error MUST be raised.

For 5. and 6., don't test this yet until https://github.com/w3c-ccg/did-key-test-suite/issues/23 is resolved.

msporny commented 2 years ago

Yes, agreed that those are generalized tests, and as you said it's debatable where they should go. The options include:

  1. In the DID Resolution test suite.
  2. In each DID Method test suite (as a basic sanity check since the requirements come for DID Core).
  3. In both places.

We put it in the did:key Method test suite because we wanted to make sure that the DID Document followed the normative requirements in DID Core as well as those layered in the did:key Method specification.

My preference here is to put them in both places to ensure we don't have testing gaps. Should the DID Resolution test suite enforce normative requirements provided by DID Core? I believe the answer to that is "Yes". Should each DID Method test suite enforce normative requirements provided by DID Core? I believe the answer to that is "Yes".

The danger in putting them in both places is that the tests might get out of sync. To combat that in the DID Method test suites, we have "shared test bundles" that can be added to every DID Method test suite (if they follow the did:key test suite pattern).

aljones15 commented 2 years ago

Both of these seem to be covered by existing normative statements in did resolution:

Validate that the input DID conforms to the did rule of the DID Syntax. If not, the DID resolver MUST return the following result:

didResolutionMetadata: «[ "error" → "invalidDid" ]» didDocument: null didDocumentMetadata: «[ ]»

I think their inclusion in did the key spec/test suite is do to the fact that an implementer might decide to use a did key method driver with out using an implementation of did resolution.

It seems like these could be did resolution normative statements, basically once the didDocument is read it would need to be validated. Another option is to create a spec for didDocument creation / read for the representation that the various did method drivers seem to share. These would then be checks that occur in the creation of the didDocument. I'm not sure if the did:key representation of a didDocument has a spec somewhere, but if it does these statements are high level enough to be placed there.

peacekeeper commented 2 years ago

I think I agree that having some tests in both places may not be a bad thing. So I guess the burden is on us to add missing tests to the DID Resolution test suite, without necessarily removing them from DID method test suite(s).

I also agree that we could potentially add a few more requirements to the DID Resolution specification, such as validation of a DID document. Of course you could also argue that for DID methods like did:key, the DID document is by definition always valid, unless your method implementation is buggy. Whether or not there is additional code around your "driver" that performs extra validation feels more like an implementation detail.

aljones15 commented 2 years ago

I think I agree that having some tests in both places may not be a bad thing. So I guess the burden is on us to add missing tests to the DID Resolution test suite, without necessarily removing them from DID method test suite(s).

I also agree that we could potentially add a few more requirements to the DID Resolution specification, such as validation of a DID document. Of course you could also argue that for DID methods like did:key, the DID document is by definition always valid, unless your method implementation is buggy. Whether or not there is additional code around your "driver" that performs extra validation feels more like an implementation detail.

I believe the validators in did:key spec are there in case the underlying key library or multicodec encoder/decoder is buggy and perhaps outputs a 35 byte publicKey buffer or the multibase encoding is incorrect. One thing I like about those validators is that they will throw in the exact step they happen, so if the controller of your verificationMethod is not a valid DID Url it will throw from that exact step which makes debugging a lot easier.