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

Add protocol update action #569

Merged
merged 4 commits into from
Oct 24, 2023
Merged

Add protocol update action #569

merged 4 commits into from
Oct 24, 2023

Conversation

diehuxx
Copy link

@diehuxx diehuxx commented Oct 20, 2023

The update action allows an actor to update an existing record. This contrasts with the write action which allows only the creator of a record to update it. In other words:

  1. write allows a DID to create a record and update the record they have created
  2. update allows a DID to update a record, regardless of initial author

@codesandbox
Copy link

codesandbox bot commented Oct 20, 2023

Review or Edit in CodeSandbox

Open the branch in Web EditorVS CodeInsiders

Open Preview

Copy link

Is this distinction necessary? Can't you just use capabilities for this?

Say that Alice creates some record. As I understand it, the DWN node can't create a capability to return to her, but it does know she is the owner. That means Alice can create a "write" capability that she can use. She can also delegate that capability to someone else.

The upshot is that you only need one "write" method at the expense of the owner having to create a root capability. An added advantage is that Alice won't be vulnerable to a confused deputy attack as she would be if she used her ambient authority.

thehenrytsai
thehenrytsai previously approved these changes Oct 23, 2023
Copy link
Contributor

@thehenrytsai thehenrytsai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and test coverage is super thorough!! Just a few superficial comment.

src/interfaces/records-write.ts Outdated Show resolved Hide resolved
tests/vectors/protocol-definitions/author-update.json Outdated Show resolved Hide resolved
src/core/protocol-authorization.ts Outdated Show resolved Hide resolved
@diehuxx
Copy link
Author

diehuxx commented Oct 23, 2023

#569 (comment)

@csuwildcat Any thoughts?

Diane Huxley added 2 commits October 23, 2023 14:08
* main:
  Add codecov coverage job in `integrity-check.yml` (#542)
  Add support for compressed secp256k1 publicKey (#567)
@codecov-commenter
Copy link

Codecov Report

Merging #569 (59c674f) into main (e5a1a31) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #569   +/-   ##
=======================================
  Coverage   97.70%   97.70%           
=======================================
  Files          66       66           
  Lines        7757     7769   +12     
  Branches     1129     1132    +3     
=======================================
+ Hits         7579     7591   +12     
  Misses        170      170           
  Partials        8        8           
Files Coverage Δ
src/core/protocol-authorization.ts 98.45% <100.00%> (-0.02%) ⬇️
src/interfaces/records-write.ts 100.00% <100.00%> (ø)

@LiranCohen
Copy link
Member

LiranCohen commented Oct 23, 2023

@alanhkarp

That means Alice can create a "write" capability that she can use. She can also delegate that capability to someone else.

Yes, you could technically build the same type of behavior with delegation for other aspects of the protocol rules, but it would be difficult to enforce these as a 'protocol' with other implementations.

Say you have a protocol with the following rules:

  1. anyone can create a post.
  2. anyone can add a post/tag to existing posts.
  3. the did who created the post can edit any post/tag relating to the post they created.

Instead of each implementation of this social protocol front-end app explicitly delegating the write capability of the post/tag they created to the author of the post, the protocol rules handle this.

Copy link
Member

@LiranCohen LiranCohen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@alanhkarp
Copy link

@LiranCohen

@alanhkarp

That means Alice can create a "write" capability that she can use. She can also delegate that capability to someone else.

Yes, you could technically build the same type of behavior with delegation for other aspects of the protocol rules, but it would be difficult to enforce these as a 'protocol' with other implementations.

Say you have a protocol with the following rules:

  1. anyone can create a post.
  2. anyone can add a post/tag to existing posts.

In that case, allow the creator of the post to specify that no capability is needed to add a post/tag to the post.

  1. the did who created the post can edit any post/tag relating to the post they created.

Using the did of the creator of the post is effectively using an access control list. (The same goes for using Roles as I pointed out in another issue.) It's not as bad a a pure ACL system, since Alice can create a capability when she wants to delegate some of her rights. The risk, though, is a confused deputy vulnerability.

@csuwildcat
Copy link
Member

csuwildcat commented Oct 24, 2023

@diehuxx looks good to me

@diehuxx diehuxx merged commit 7983ff1 into main Oct 24, 2023
4 checks passed
diehuxx pushed a commit that referenced this pull request Oct 24, 2023
* main:
  Add protocol update action (#569)
@thehenrytsai thehenrytsai deleted the diehuxx/protocol-update branch April 8, 2024 18:35
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.

6 participants