Skip to content

Commit

Permalink
Renamed protocol actions update -> co-update and delete -> `co-…
Browse files Browse the repository at this point in the history
…delete` (#698)
  • Loading branch information
thehenrytsai committed Mar 5, 2024
1 parent 0ef2c09 commit 8861ead
Show file tree
Hide file tree
Showing 15 changed files with 80 additions and 74 deletions.
6 changes: 3 additions & 3 deletions Q_AND_A.md
Expand Up @@ -94,12 +94,12 @@

This design choice is primarily driven by performance considerations. If we were to make `protocolPath` optional, and it is not specified, we would need to search records across protocol paths. Since protocol rules (protocol rule set) are defined at the per protocol path level, this means we would need to parse the protocol rules for every protocol path in the protocol definition to determine which protocol path the invoked role has access to. Then, we would need to make a database query for each qualified protocol path, which could be quite costly. This is not to say that we should never consider it, but this is the current design choice.

- What is the difference between `write` and `update` actions?
- What is the difference between `write` and `co-update` actions?

(Last update: 2023/11/09)
(Last update: 2024/03/04)

- `write` - allows a DID to create and update the record they have created
- `update` - allows a DID to update a record, regardless of the initial author
- `co-update` - allows a DID to update a record, regardless of the initial author

- What is the difference between the terms "global role" and "context role"?

Expand Down
8 changes: 4 additions & 4 deletions json-schemas/interface-methods/protocol-rule-set.json
Expand Up @@ -43,9 +43,9 @@
"can": {
"type": "string",
"enum": [
"delete",
"co-delete",
"co-update",
"read",
"update",
"write"
]
}
Expand All @@ -64,11 +64,11 @@
"can": {
"type": "string",
"enum": [
"delete",
"co-delete",
"co-update",
"query",
"subscribe",
"read",
"update",
"write"
]
}
Expand Down
22 changes: 12 additions & 10 deletions src/core/protocol-authorization.ts
Expand Up @@ -507,9 +507,11 @@ export class ProtocolAuthorization {

/**
* Returns a list of ProtocolAction(s) based on the incoming message, one of which must be allowed for the message to be authorized.
* NOTE: the reason why there could be multiple actions is because in case of an "update" RecordsWrite by the original record author,
* the RecordsWrite can either be authorized by a `write` or `update` allow rule. It is important to recognize that the `write` access that allowed
* the original record author to create the record maybe revoked (e.g. by role revocation) by the time an "update" by the same author is attempted.
* NOTE: the reason why there could be multiple actions is because in case of a "non-initial" RecordsWrite by the original record author,
* the RecordsWrite can either be authorized by a `write` or `co-update` allow rule.
*
* It is important to recognize that the `write` access that allowed the original record author to create the record maybe revoked
* (e.g. by role revocation) by the time a "non-initial" write by the same author is attempted.
*/
private static async getActionsSeekingARuleMatch(
tenant: string,
Expand All @@ -519,7 +521,7 @@ export class ProtocolAuthorization {

switch (incomingMessage.message.descriptor.method) {
case DwnMethodName.Delete:
return [ProtocolAction.Delete];
return [ProtocolAction.CoDelete];

case DwnMethodName.Query:
return [ProtocolAction.Query];
Expand All @@ -533,14 +535,14 @@ export class ProtocolAuthorization {
case DwnMethodName.Write:
const incomingRecordsWrite = incomingMessage as RecordsWrite;
if (await incomingRecordsWrite.isInitialWrite()) {
// only 'write' allows initial RecordsWrites; 'update' only applies to subsequent RecordsWrites
// only 'write' allows initial RecordsWrites
return [ProtocolAction.Write];
} else if (await incomingRecordsWrite.isAuthoredByInitialRecordAuthor(tenant, messageStore)) {
// Both 'update' and 'write' authorize the incoming message
return [ProtocolAction.Write, ProtocolAction.Update];
// Both 'co-update' and 'write' authorize the incoming message
return [ProtocolAction.Write, ProtocolAction.CoUpdate];
} else {
// Actors other than the initial record author must be authorized to 'update' the message
return [ProtocolAction.Update];
// Actors other than the initial record author must be authorized to 'co-update' the message
return [ProtocolAction.CoUpdate];
}

// default:
Expand Down Expand Up @@ -593,7 +595,7 @@ export class ProtocolAuthorization {
if (incomingMessage.message.descriptor.method === DwnMethodName.Write) {
recordsWriteMessage = incomingMessage.message as RecordsWriteMessage;
} else {
// else the incoming message must be a RecordsDelete because only `update` and `delete` are allowed recipient actions
// else the incoming message must be a RecordsDelete because only `co-update` and `co-delete` are allowed recipient actions
recordsWriteMessage = ancestorMessageChain[ancestorMessageChain.length - 1];
}

Expand Down
19 changes: 10 additions & 9 deletions src/interfaces/protocols-configure.ts
Expand Up @@ -3,11 +3,11 @@ import type { ProtocolDefinition, ProtocolRuleSet, ProtocolsConfigureDescriptor,

import { AbstractMessage } from '../core/abstract-message.js';
import { Message } from '../core/message.js';
import { ProtocolActor } from '../types/protocols-types.js';
import { Time } from '../utils/time.js';
import { DwnError, DwnErrorCode } from '../core/dwn-error.js';
import { DwnInterfaceName, DwnMethodName } from '../enums/dwn-interface-method.js';
import { normalizeProtocolUrl, normalizeSchemaUrl, validateProtocolUrlNormalized, validateSchemaUrlNormalized } from '../utils/url.js';
import { ProtocolAction, ProtocolActor } from '../types/protocols-types.js';

export type ProtocolsConfigureOptions = {
messageTimestamp?: string;
Expand Down Expand Up @@ -166,19 +166,20 @@ export class ProtocolsConfigure extends AbstractMessage<ProtocolsConfigureMessag
);
}

// Validate that if `who === recipient` and `of === undefined`, then `can` is either `delete` or `update`
// We will not use direct recipient for `read`, `write`, or `query` because:
// - Recipients are always allowed to `read`.
// - `write` entails ability to create and update, whereas `update` only allows for updates.
// There is no 'recipient' until the record has been created, so it makes no sense to allow recipient to write.
// - At this time, `query` is only authorized using roles, so allowing direct recipients to query is outside the scope of this PR.
// Validate that if `who === recipient` and `of === undefined`, then `can` is either `co-delete` or `co-update`
// We will allow `read`, `write`, or `query` because:
// - `read` - Recipients are always allowed to `read`.
// - `write` - Entails ability to create and update.
// Since `of` is undefined, it implies the recipient of THIS record,
// there is no 'recipient' until this record has been created, so it makes no sense to allow recipient to write this record.
// - `query` - Only authorized using roles, so allowing direct recipients to query is outside the scope.
if (action.who === ProtocolActor.Recipient &&
action.of === undefined &&
!['update', 'delete'].includes(action.can)
![ProtocolAction.CoUpdate, ProtocolAction.CoDelete].includes(action.can as ProtocolAction)
) {
throw new DwnError(
DwnErrorCode.ProtocolsConfigureInvalidRecipientOfAction,
'Rules for `recipient` without `of` property must have `can` === `delete` or `update`'
'Rules for `recipient` without `of` property must have `can` === `co-delete` or `co-update`'
);
}

Expand Down
6 changes: 3 additions & 3 deletions src/types/protocols-types.ts
Expand Up @@ -37,11 +37,11 @@ export enum ProtocolActor {
}

export enum ProtocolAction {
Delete = 'delete',
CoDelete = 'co-delete',
CoUpdate = 'co-update',
Query = 'query',
Read = 'read',
Subscribe = 'subscribe',
Update = 'update',
Write = 'write'
}

Expand Down Expand Up @@ -90,7 +90,7 @@ export type ProtocolActionRule = {

/**
* Action that the actor can perform.
* May be 'query' | 'read' | 'write' | 'update' | 'delete' | 'subscribe'.
* See {ProtocolAction} for possible values.
* 'query' and 'subscribe' are only supported for `role` rules.
*/
can: string;
Expand Down
5 changes: 3 additions & 2 deletions tests/handlers/records-delete.spec.ts
Expand Up @@ -502,7 +502,7 @@ export function testRecordsDeleteHandler(): void {
});

describe('role based deletes', () => {
it('should allow deletes by invoking a context role', async () => {
it('should allow co-delete by invoking a context role', async () => {
// scenario: Alice adds Bob as a 'thread/admin' role. She writes a 'thread/chat'.
// Bob invokes his admin role to delete the 'thread/chat'. Carol is unable to delete
// the 'thread/chat'.
Expand Down Expand Up @@ -553,6 +553,7 @@ export function testRecordsDeleteHandler(): void {
const chatRecordReply = await dwn.processMessage(alice.did, chatRecord.message, { dataStream: chatRecord.dataStream });
expect(chatRecordReply.status.code).to.equal(202);

// Verifies that Carol cannot delete without appropriate role
const chatDeleteCarol = await TestDataGenerator.generateRecordsDelete({
author : carol,
recordId : chatRecord.message.recordId,
Expand All @@ -571,7 +572,7 @@ export function testRecordsDeleteHandler(): void {
expect(chatDeleteReply.status.code).to.equal(202);
});

it('should allow delete invoking a root-level role', async () => {
it('should allow co-delete invoking a root-level role', async () => {
// scenario: Alice adds Bob as a root-level 'admin' role. She writes a 'chat'.
// Bob invokes his admin role to delete the 'chat'.

Expand Down
26 changes: 14 additions & 12 deletions tests/handlers/records-write.spec.ts
Expand Up @@ -1223,7 +1223,7 @@ export function testRecordsWriteHandler(): void {
expect(bobRecordsQueryReply.entries![0].encodedData).to.equal(Encoder.bytesToBase64Url(bobData));
});

it('should allow update with allow-anyone rule', async () => {
it('should allow co-update with allow-anyone rule', async () => {
// scenario: Alice creates a record on her DWN, and Bob (anyone) is able to update it. Bob is not able to
// create a record.

Expand Down Expand Up @@ -1345,7 +1345,7 @@ export function testRecordsWriteHandler(): void {
.to.equal(base64url.baseEncode(encodedCredentialResponse));
});

it('should allow update with ancestor recipient rule', async () => {
it('should allow co-update with ancestor recipient rule', async () => {
// scenario: Alice creates a post with Bob as recipient. Alice adds a tag to the post. Bob is able to update
// the tag because he is recipient of the post. Bob is not able to create a new tag.

Expand Down Expand Up @@ -1405,9 +1405,11 @@ export function testRecordsWriteHandler(): void {
expect(bobTagRecordsReply.status.detail).to.contain(DwnErrorCode.ProtocolAuthorizationActionNotAllowed);
});

it('should allowed update with direct recipient rule', async () => {
// scenario: Alice creates a 'post' with Bob as recipient. Bob is able to update
// the 'post' because he was recipient of it. Carol is not able to update it.
it('should allow co-update with direct recipient rule', async () => {
// scenario:
// Alice creates a 'post' with Bob as recipient.
// Bob is able to update the 'post' because he was recipient of it.
// Carol is not able to update it.

const protocolDefinition = recipientCanProtocol as ProtocolDefinition;
const alice = await TestDataGenerator.generatePersona();
Expand Down Expand Up @@ -1529,7 +1531,7 @@ export function testRecordsWriteHandler(): void {
.to.equal(base64url.baseEncode(encodedCaption));
});

it('should allow update with ancestor author rule', async () => {
it('should allow co-update with ancestor author rule', async () => {
// scenario: Bob authors a post on Alice's DWN. Alice adds a comment to the post. Bob is able to update the comment,
// since he authored the post.

Expand Down Expand Up @@ -1962,7 +1964,7 @@ export function testRecordsWriteHandler(): void {
expect(chatReply.status.code).to.equal(202);
});

it('uses a root-level role to authorize an update', async () => {
it('uses a root-level role to authorize a co-update', async () => {
// scenario: Alice gives Bob a admin role. Bob invokes his
// admin role in order to update a chat message that Alice wrote

Expand Down Expand Up @@ -2128,7 +2130,7 @@ export function testRecordsWriteHandler(): void {
expect(chatRecordReply.status.code).to.equal(202);
});

it('uses a context role to authorize an update', async () => {
it('uses a context role to authorize a co-update', async () => {
// scenario: Alice creates a thread and adds Bob to the 'thread/admin' role.
// Bob invokes the record to write in the thread

Expand Down Expand Up @@ -2176,14 +2178,14 @@ export function testRecordsWriteHandler(): void {
const chatRecordReply = await dwn.processMessage(alice.did, chatRecord.message, { dataStream: chatRecord.dataStream });
expect(chatRecordReply.status.code).to.equal(202);

// Bob invokes his admin role to update the chat message
const chatUpdateRecord = await TestDataGenerator.generateFromRecordsWrite({
// Bob invokes his admin role to co-update the chat message
const chatCoUpdateRecord = await TestDataGenerator.generateFromRecordsWrite({
author : bob,
existingWrite : chatRecord.recordsWrite,
protocolRole : 'thread/admin',
});
const chatUpdateRecordReply =
await dwn.processMessage(alice.did, chatUpdateRecord.message, { dataStream: chatUpdateRecord.dataStream });
await dwn.processMessage(alice.did, chatCoUpdateRecord.message, { dataStream: chatCoUpdateRecord.dataStream });
expect(chatUpdateRecordReply.status.code).to.equal(202);
});

Expand Down Expand Up @@ -2343,7 +2345,7 @@ export function testRecordsWriteHandler(): void {
expect(newRecordQueryReply.entries![0].encodedData).to.equal(Encoder.bytesToBase64Url(updatedMessageBytes));
});

it('should disallow overwriting existing records by a different author if author is not authorized to `update`', async () => {
it('should disallow overwriting existing records by a different author if author is not authorized to `co-update`', async () => {
// scenario: Bob writes into Alice's DWN given Alice's "message" protocol, Carol then attempts to modify the existing message

// write a protocol definition with an allow-anyone rule
Expand Down
6 changes: 3 additions & 3 deletions tests/vectors/protocol-definitions/anyone-collaborate.json
Expand Up @@ -9,17 +9,17 @@
"$actions": [
{
"who": "anyone",
"can": "update"
"can": "co-update"
},
{
"who": "anyone",
"can": "read"
},
{
"who": "anyone",
"can": "delete"
"can": "co-delete"
}
]
}
}
}
}
6 changes: 3 additions & 3 deletions tests/vectors/protocol-definitions/author-can.json
Expand Up @@ -18,15 +18,15 @@
{
"who": "author",
"of": "post",
"can": "update"
"can": "co-update"
},
{
"who": "author",
"of": "post",
"can": "delete"
"can": "co-delete"
}
]
}
}
}
}
}
4 changes: 2 additions & 2 deletions tests/vectors/protocol-definitions/free-for-all.json
Expand Up @@ -18,7 +18,7 @@
},
{
"who": "anyone",
"can": "delete"
"can": "co-delete"
},
{
"who": "anyone",
Expand All @@ -27,4 +27,4 @@
]
}
}
}
}
4 changes: 2 additions & 2 deletions tests/vectors/protocol-definitions/friend-role.json
Expand Up @@ -41,11 +41,11 @@
},
{
"role": "admin",
"can": "update"
"can": "co-update"
},
{
"role": "admin",
"can": "delete"
"can": "co-delete"
}
]
}
Expand Down
4 changes: 2 additions & 2 deletions tests/vectors/protocol-definitions/post-comment.json
Expand Up @@ -36,10 +36,10 @@
{
"who": "author",
"of": "post",
"can": "delete"
"can": "co-delete"
}
]
}
}
}
}
}
10 changes: 5 additions & 5 deletions tests/vectors/protocol-definitions/recipient-can.json
Expand Up @@ -10,27 +10,27 @@
"$actions": [
{
"who": "recipient",
"can": "update"
"can": "co-update"
},
{
"who": "recipient",
"can": "delete"
"can": "co-delete"
}
],
"tag": {
"$actions": [
{
"who": "recipient",
"of": "post",
"can": "update"
"can": "co-update"
},
{
"who": "recipient",
"of": "post",
"can": "delete"
"can": "co-delete"
}
]
}
}
}
}
}

0 comments on commit 8861ead

Please sign in to comment.