Skip to content
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

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

Open
peacekeeper opened this issue Aug 18, 2022 · 4 comments
Open

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

peacekeeper opened this issue Aug 18, 2022 · 4 comments

Comments

@peacekeeper
Copy link
Member

peacekeeper commented Aug 18, 2022

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 #23 is resolved.

@msporny
Copy link
Contributor

msporny commented Aug 29, 2022

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
Copy link
Collaborator

  • The scheme MUST be the value did
  • MUST raise invalidDid error if scheme is not did

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.

  • If "didDocument.id" is not a valid DID, an invalidDid error MUST be raised
  • If verificationMethod.id is not a valid DID URL, an invalidDidUrl error MUST be raised.
  • 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.
  • 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.
  • If verificationMethod.controller is not a valid DID, an invalidDid error MUST be raised.

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
Copy link
Member Author

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
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants