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

Implement support for signed-ietf-json-patch #33

Merged
merged 15 commits into from
Feb 14, 2022

Conversation

allibell
Copy link
Contributor

@allibell allibell commented Feb 6, 2022

From #8: implement support per the IETF JSON patch DID extension. This enables us to modify/patch DID documents using JWS payloads.

Notes

  • core design choices (up for debate):
    • a new PatchedKeyPair struct which wraps KeyPair. this is intuitive, self-explanatory, and minimizes breaking changes a bit.
    • might be obvious but: we are not storing any real patch state here, just storing it in memory as an attribute of a PatchedKeyPair. we are not performing a DID update operation and the responsibility to manage state (check for DID diffs, maintain patches) falls squarely to the callers.
    • get_did_document requests still fail silently; a bad request will return the original document without warning. this is to maintain the existing contract of not throwing errors. on the other hand, resolve requests with bad patches throw an error instead of returning the key. this seems to make sense since resolve is more "internal", i.e. the caller intends to manage PatchedKeyPair state so they should be informed whether that struct has the patches they expect
  • this also fixes a serde deserialization bug we had with VerificationMethods/KeyFormats. we'd previously get Error("missing field 'key_type'", line: 0, column: 0) or Error("invalid value: map, expected map with a single key", line: 0, column: 0) due to custom serializing
  • this introduces a dependency on the json-patch crate . I think this is fine since it avoids a lot of snowflake/boilerplate code and we have dependencies on ~similar crates (e.g. did_url and bls12_381_plus seem to be similar maturity, albeit more necessary). I am open to reimplementing that boilerplate if the tradeoff of minimal dependencies is more important here.

Tests
new chunky unit tests. snipped output from test_json_patch_demo:

--- snip ---
// DID doc before the patch:
{
  "@context": "https://www.w3.org/ns/did/v1",
  "id": "did:key:z6MkeqkjQXF9LDEVXLZB7GSpThb5FmGGvUxhhAM2nSMzLX7h",
--- snip ---
  "capabilityDelegation": [
    "did:key:z6MkqhoqdLucQT6ybuFYX5zTkeooAEQbbyRqT1NVfz3wAeyL#z6MkqhoqdLucQT6ybuFYX5zTkeooAEQbbyRqT1NVfz3wAeyL"
  ],
  "capabilityInvocation": [
    "did:key:z6MkqhoqdLucQT6ybuFYX5zTkeooAEQbbyRqT1NVfz3wAeyL#z6MkqhoqdLucQT6ybuFYX5zTkeooAEQbbyRqT1NVfz3wAeyL"
  ],
--- snip ---

// DID doc after the patch:
// (note it is identical except it has the appended capabilityDelegation and capabilityInvocation)
{
  "@context": "https://www.w3.org/ns/did/v1",
  "id": "did:key:z6MkeqkjQXF9LDEVXLZB7GSpThb5FmGGvUxhhAM2nSMzLX7h",
--- snip ---
  "capabilityDelegation": [
    "did:key:z6MkqhoqdLucQT6ybuFYX5zTkeooAEQbbyRqT1NVfz3wAeyL#z6MkqhoqdLucQT6ybuFYX5zTkeooAEQbbyRqT1NVfz3wAeyL",
    "did:key:z6Mkk7yqnGF3YwTrLpqrW6PGsKci7dNqh1CjnvMbzrMerSeL#z6Mkk7yqnGF3YwTrLpqrW6PGsKci7dNqh1CjnvMbzrMerSeL"
  ],
  "capabilityInvocation": [
    "did:key:z6MkqhoqdLucQT6ybuFYX5zTkeooAEQbbyRqT1NVfz3wAeyL#z6MkqhoqdLucQT6ybuFYX5zTkeooAEQbbyRqT1NVfz3wAeyL",
    "did:key:z6Mkk7yqnGF3YwTrLpqrW6PGsKci7dNqh1CjnvMbzrMerSeL#z6Mkk7yqnGF3YwTrLpqrW6PGsKci7dNqh1CjnvMbzrMerSeL"
  ],
--- snip ---

full output: https://pastebin.com/HNSDybQz

Follow-ups

  • More testing - end-to-end tests for this? (maybe just in callers)
  • Address version-up and security considerations for callers

* existing serde deserialization issues for VerificationMethod
* issues in my JSON patching
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
@allibell
Copy link
Contributor Author

allibell commented Feb 7, 2022

as discussed, retitled BaseKeyPair->KeyPair and KeyPair->PatchedKeyPair. This makes it clearer what the wrapper struct is for. it also has the nice effects of 1) making this PR smaller and 2) reducing breaking changes for people relying on KeyPair object internals. tests still happy.

@tmarkovski tmarkovski merged commit 3cd338a into decentralized-identity:main Feb 14, 2022
@tmarkovski tmarkovski linked an issue Feb 18, 2022 that may be closed by this pull request
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

Successfully merging this pull request may close these issues.

Implement support for signed-ietf-json-patch
2 participants