Update did key v7 #3

aljones15 · 2022-08-03T18:22:00Z

Implements all normative statements from did:key spec v7.

NOTE: this does not address missing did resolver functionality, but hey that spec is still kind of in progress.

Features:

es6 exports
tons of validators related to did:key normative statements
better separation of stuff (although it could be better)
better github workflows
c8 for coverage

@davidlehn can you look out for es6 related issues with this PR?
@mattcollier can you look out for any issues this might causes when deployed?

Awaits:

digitalbazaar/did-io#63

…peer deps.

msporny

Large PRs like this make it very difficult to do a thorough review (too much information all at once).

There are a number of design concerns in this PR, most notably there are a few places where there aren't a proper separation of concerns (we might eventually have to resolve 20 different types of DIDs, so keep that in mind -- how would you design the system to handle resolving all of those simultaneously. There is some private code that should probably be sorted out into different files (in the very least) or modules (eventually).

lib/DidResolutionResult.js

msporny · 2022-08-03T20:09:43Z

lib/config.js

-cfg.supportedMethods = ['key', 'v1'];
+// a list of which did:methods we supported
+// experimental is for tests only
+cfg.supportedMethods = ['key', 'v1', 'experimental'];


Suggested change

cfg.supportedMethods = ['key', 'v1', 'experimental'];

cfg.supportedMethods = ['key', 'v1', 'experimental'];

We definitely don't want the experimental DID Method in any of our resolution code -- this is going to go into production and could become an attack vector. Why do we need this here?

We need a method that is not key for testing the did:key statement about the method always being key. We could use v1, but this actually shows a larger issue, right now passing in a did:v1 will result in this implementation throwing as it only allows did:key.

lib/config.js

msporny · 2022-08-03T20:11:18Z

lib/didComponents.js

+ *
+ * @returns {object} The components of the did.
+*/
+export const getDidKeyComponents = ({did}) => {


Abstraction here feels wrong... getting method-specific things should probably be placed in method-specific libraries.

Totally agree here, this should probably be in did-method-key-js. I made an issue to start discussion on this: digitalbazaar/did-method-key#46

There are a lot of validation stuff included here that should be in did-method-key-js ed25519-verification-key-2020 and possibly x25519.

Marking this as resolved as getDidKeyComponents is no longer in this library and the way we parse did keys has changed. See digitalbazaar/did-method-key#47 for how did keys are parsed now.

msporny · 2022-08-03T20:12:45Z

lib/didComponents.js

+    scheme,
+    method,
+    // if multibase exists use the version
+    version: multibase ? version : '1',


This seems like it's wrong, you can have version AND multibase exist at the same time. If the code block is right, it feels confusing. Didn't pass the code smell test... something seems off here.

that is exactly what this accounts for did:key:3:zf00 will result in multibase and version being defined at the same time. If multibase is not defined then the string 1 is used as the version. p.s. this results from using the algorithm from the spec that states to separate the components of the identifier using :.

Another problem that needs accounting for here is if someone uses did:key:1:... (version 1 the wrong way) ... that needs to be an error, I'd think.

that actually should not error as the version number is a positive integer unless I'm not understanding this correctly.

msporny · 2022-08-03T20:16:42Z

lib/validators/didErrors.js

+    this.name = 'InvalidDidError';
+    this.code = 'invalidDid';


Suggested change

this.name = 'InvalidDidError';

this.code = 'invalidDid';

this.name = 'InvalidDidError';

this.code = 'invalidDid';

Why do we have an error class whose name and code differ only in the first character? Where is this.name used, when an exception is thrown... in which case, would it be easier to read if this were more human readable? This same question applies to everything below.

Ok so name is a property on any Error in javascript.

new TypeError().name "TypeError"

Those names follow the convention above, they are usually something like SpecificError and repeat the name of the Class.

On the other hand the error names from the spec following this pattern specificError.

So I decided to not buck trends and use typical Javascript Error names and then use the property code for the errors from the spec. I also might add that using the property code makes our errors less likely to be mutated in ways that could create problems down the road.

p.s. it kind of sucks, but you do have to manually set the name:

class SpecificError extends Error {} new SpecificError().name "Error"

Seems like maybe it should be a SyntaxError or something instead, not a new error name?

We could handle this like this:

class DidKeyError extends Error { constructor(message, code) { super(message); this.name = 'DidKeyError'; ths.code = code; } }

This would be similar to the jsonld error pattern: https://github.com/digitalbazaar/jsonld.js/blob/main/lib/JsonLdError.js

@msporny So we're now using DidResolverError or DataError where needed. Can this be considered resolved?

msporny · 2022-08-03T20:18:08Z

lib/validators/index.js

+  _validateDidKey({method, version, multibase});
+  _validateMultibaseEd25519({didKeyComponents});


These should be broken out into other libraries -- what happens if you don't feed a did:key into this method?

Hence issues like this: digitalbazaar/ed25519-verification-key-2020#18

The problem is I couldn't get consensus in any realistic time frame to add these errors to this libraries. So the validators are created in really small functions that can be ported over to other libraries when consensus is achieved.

There validators have been moved to their respective libraries. See digitalbazaar/did-method-key#47

msporny · 2022-08-03T20:19:10Z

lib/validators/index.js

+  if(!_convertSearchParamToBoolean({param: enableExperimentalPublicKeyTypes})) {
+    const verificationFormats = ['Multikey', 'JsonWebKey2020'];
+    //keyAgreement is an encryption verification method
+    if(didDocument.type.includes('KeyAgreementKey')) {


This feels very dangerous... we shouldn't be doing substring matching on types. We should only be doing whole string matching on types.

That seems fair so changed it here: ef6af7a

lib/validators/index.js

msporny · 2022-08-03T20:22:41Z

test/mocha/helpers.js

+};
+
+export const makeRequest = async ({did}) => {
+  const basePath = '/1.0/resolve/identifiers/';


Is this required by the DID Resolution spec, feels off to me. /1.0/identifiers/resolve feels a bit better.

no and not not sure where that came from. The spec gives this example: https://dev.uniresolver.io/1.0/identifiers/

So we can probably just do that.

Changed the route here: 3364e4a

…Error from did-io.

lib/http.js

lib/logger.js

test/mocha/helpers.js

JSAssassin · 2022-08-12T18:22:15Z

test/package.json

-    "cross-env": "^7.0.3",
-    "nyc": "^15.1.0"
+    "c8": "^7.12.0",
+    "cross-env": "^7.0.3"
  },
  "nyc": {


This should be c8 as nyc is no longer being used.

Let's make an issue about this one and update it at a later time. This PR is way beyond crammed with comments right now.

#4

@aljones15 It's just a small change you need to make. You've already removed nyc and added c8 in this PR. All you need to change is "nyc" on L23 to "c8", and you're good.

test/test.config.js

test/test.js

JSAssassin · 2022-08-12T18:31:05Z

lib/didResolver.js

+ *
+ * @param {object} options - Options to use.
+ * @param {string} options.did - The did or didUrl being resolved.
+ * @param {object} options.didOptions - Options passed to the resolver.


Can the description be more clear? To me the description for didOptions sounds the same as description for resolverOptions. How are the two different?

This is an excellent point and I made changes here: e83a8ce

test/mocha/helpers.js

Co-authored-by: Tashi D. Gyeltshen <[email protected]>

lib/didResolver.js

Co-authored-by: Tashi D. Gyeltshen <[email protected]>

aljones15 added 19 commits July 29, 2022 14:44

Start work on did & did:key errors & validation.

74f3911

Add DidResolutionResult class and use it.

e2b3a32

Use more except didKeyComponents over didComponents.

c28c251

Correct mistake Number.parseInt not Number.parse.

38fde21

Update lint stuff.

1299da9

Up engine to 16 & improve error reporting code.

34fc90d

Remove use strict & correct invalid jsdoc param.

a17e0ea

Use bnid for public key length errors.

e26070b

Use @bedrock/did-io latest.

d0b1f22

Convert to es6 module.

29b57ec

Add MULTICODEC ed25519 header and get bytes.

992b590

Add check for invalidDidUrl.

6dc9f64

Add better tests for isDiDUrl & component split.

510f909

Get identifier & isDidUrl.

963ab56

Better separation of did url and identifier logic.

6248cb6

Separate validators better.

bea2e99

Implement 100% success with latest did:key spec.

0a2e89f

Update github actions node versions to 14 and 16 + .npmrc for legacy …

38bc2cf

…peer deps.

Add .npmrc to test project.

98b2212

aljones15 self-assigned this Aug 3, 2022

aljones15 added 4 commits August 3, 2022 14:37

Upgrade github workflows to mongodb 4.4 add more install steps.

cb0c52e

Add statusCodes to errors.

ed6989f

Add basic tests to test project.

e2299ea

Use c8 for coverage.

ad39610

aljones15 marked this pull request as ready for review August 3, 2022 19:57

aljones15 requested review from mattcollier, davidlehn, JSAssassin and msporny August 3, 2022 19:57

msporny requested changes Aug 3, 2022

View reviewed changes

aljones15 added 10 commits August 8, 2022 16:16

Remove didKey and didResolution error files in favor of DidResolution…

a47725f

…Error from did-io.

Add function to turn error codes into statusCodes.

6aea527

Remove validators dir & make it a file.

23e0f63

Switch route to /1.0/identifiers/resolve/:did.

3364e4a

Improve naming of errorCodeToStatusCode & hasDereferencingMetadata.

2c1c0d5

Move metadata detection into DidResolutionResult.

99ca721

Remove request options & move to did-method-key.

a05ff0e

Add formatResponse to handle requests that don't want metadata.

d9d4794

Correct test makeRequest basePath.

db32c3a

Sort status codes ascending & add 501 for methodNotSupported.

40eed63

aljones15 mentioned this pull request Aug 10, 2022

Add did key spec 7 validators digitalbazaar/did-method-key#47

Open

25 tasks

aljones15 added 3 commits August 10, 2022 16:52

Remove experimental from supportedMethods.

16d594f

Make supported methods a Set.

8b06c20

Remove logic for supportedMethods.

d88e2de

aljones15 requested review from dlongley and msporny August 12, 2022 17:21

Use node 16-18 in ci actions.

81a8d70

JSAssassin suggested changes Aug 12, 2022

View reviewed changes

aljones15 and others added 7 commits August 15, 2022 11:21

Remove unneeded line from https.js.

337f2ff

Co-authored-by: Tashi D. Gyeltshen <[email protected]>

Remove 'use strict' from logger.js.

4f16b24

Co-authored-by: Tashi D. Gyeltshen <[email protected]>

Remove extra line in helpers.js.

b25d609

Co-authored-by: Tashi D. Gyeltshen <[email protected]>

Remove 'use strict' from test.config.js.

297950b

Co-authored-by: Tashi D. Gyeltshen <[email protected]>

Remove extra line from test.js.

06560a3

Co-authored-by: Tashi D. Gyeltshen <[email protected]>

Better document did and resolver options.

e83a8ce

Add @db/http-client to test project.

5e918ab

aljones15 requested a review from JSAssassin August 17, 2022 22:58

JSAssassin reviewed Aug 18, 2022

View reviewed changes

lib/didResolver.js Outdated Show resolved Hide resolved

aljones15 and others added 3 commits August 23, 2022 10:33

Change nyc opts to c8 opts.

c4ff86b

Remove assertNoError from lint test globals.

cae41cc

Correct typo in jsdoc string for didOptions.

809aaa3

Co-authored-by: Tashi D. Gyeltshen <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update did key v7 #3

Update did key v7 #3

aljones15 commented Aug 3, 2022 •

edited

Loading

msporny left a comment

msporny Aug 3, 2022

aljones15 Aug 3, 2022 •

edited

Loading

msporny Aug 3, 2022

aljones15 Aug 3, 2022

aljones15 Aug 12, 2022

msporny Aug 3, 2022

aljones15 Aug 3, 2022 •

edited

Loading

dlongley Aug 4, 2022 •

edited

Loading

aljones15 Aug 4, 2022

msporny Aug 3, 2022

aljones15 Aug 3, 2022 •

edited

Loading

dlongley Aug 4, 2022

aljones15 Aug 4, 2022

aljones15 Aug 8, 2022

msporny Aug 3, 2022

aljones15 Aug 3, 2022

aljones15 Aug 12, 2022

msporny Aug 3, 2022

aljones15 Aug 3, 2022

msporny Aug 3, 2022

aljones15 Aug 3, 2022 •

edited

Loading

aljones15 Aug 8, 2022

JSAssassin Aug 12, 2022

aljones15 Aug 15, 2022

JSAssassin Aug 18, 2022 •

edited

Loading

JSAssassin Aug 12, 2022

aljones15 Aug 15, 2022

	cfg.supportedMethods = ['key', 'v1', 'experimental'];
	cfg.supportedMethods = ['key', 'v1', 'experimental'];

		_validateDidKey({method, version, multibase});
		_validateMultibaseEd25519({didKeyComponents});

Update did key v7 #3

Are you sure you want to change the base?

Update did key v7 #3

Conversation

aljones15 commented Aug 3, 2022 • edited Loading

msporny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aljones15 Aug 3, 2022 • 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

aljones15 Aug 3, 2022 • edited Loading

Choose a reason for hiding this comment

dlongley Aug 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aljones15 Aug 3, 2022 • 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

aljones15 Aug 3, 2022 • 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

JSAssassin Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aljones15 commented Aug 3, 2022 •

edited

Loading

aljones15 Aug 3, 2022 •

edited

Loading

aljones15 Aug 3, 2022 •

edited

Loading

dlongley Aug 4, 2022 •

edited

Loading

aljones15 Aug 3, 2022 •

edited

Loading

aljones15 Aug 3, 2022 •

edited

Loading

JSAssassin Aug 18, 2022 •

edited

Loading