From 7b0258106f3957d918a3b24f1eef982d3d3fc4b0 Mon Sep 17 00:00:00 2001 From: Liran Cohen Date: Fri, 4 Oct 2024 10:20:21 -0400 Subject: [PATCH] Enforce requiring all read actions (`read`, `query`, `subscribe`) to exist in a protocol role rule. (#813) Currently there are some inconsistencies being able to fetch record data. If a user can only query for something, they are able to get the data if it is under the encodedData size limit, however if it is not they are unable to issue a `RecordsRead` for that record. This change forces the user to include all 3 "read" actions (read, subscribe, query) when using any one of them in a protocol role-based action rule. --- src/core/dwn-error.ts | 1 + src/interfaces/protocols-configure.ts | 10 ++ tests/handlers/protocols-configure.spec.ts | 155 ++++++++++++++++++ tests/interfaces/protocols-configure.spec.ts | 125 +++++++++++++- .../protocol-definitions/friend-role.json | 2 +- tests/vectors/protocol-definitions/slack.json | 8 +- .../protocol-definitions/thread-role.json | 4 +- 7 files changed, 300 insertions(+), 5 deletions(-) diff --git a/src/core/dwn-error.ts b/src/core/dwn-error.ts index c7ca8130d..8b377b5ea 100644 --- a/src/core/dwn-error.ts +++ b/src/core/dwn-error.ts @@ -91,6 +91,7 @@ export enum DwnErrorCode { ProtocolsConfigureInvalidTagSchema = 'ProtocolsConfigureInvalidTagSchema', ProtocolsConfigureRecordNestingDepthExceeded = 'ProtocolsConfigureRecordNestingDepthExceeded', ProtocolsConfigureRoleDoesNotExistAtGivenPath = 'ProtocolsConfigureRoleDoesNotExistAtGivenPath', + ProtocolsConfigureRoleReadActionMissing = 'ProtocolsConfigureRoleReadActionMissing', ProtocolsGrantAuthorizationQueryProtocolScopeMismatch = 'ProtocolsGrantAuthorizationQueryProtocolScopeMismatch', ProtocolsGrantAuthorizationScopeProtocolMismatch = 'ProtocolsGrantAuthorizationScopeProtocolMismatch', ProtocolsQueryUnauthorized = 'ProtocolsQueryUnauthorized', diff --git a/src/interfaces/protocols-configure.ts b/src/interfaces/protocols-configure.ts index 0ecba08b1..957a28fc9 100644 --- a/src/interfaces/protocols-configure.ts +++ b/src/interfaces/protocols-configure.ts @@ -196,6 +196,16 @@ export class ProtocolsConfigure extends AbstractMessage actionRule.can.includes(action)) && !readActions.every(action => actionRule.can.includes(action))) { + throw new DwnError( + DwnErrorCode.ProtocolsConfigureRoleReadActionMissing, + `Role in action ${JSON.stringify(actionRule)} for rule set ${ruleSetProtocolPath} must contain all read actions (${readActions.join(', ')}).` + ); + } } } diff --git a/tests/handlers/protocols-configure.spec.ts b/tests/handlers/protocols-configure.spec.ts index 283eddceb..61eeb3fc6 100644 --- a/tests/handlers/protocols-configure.spec.ts +++ b/tests/handlers/protocols-configure.spec.ts @@ -437,6 +437,161 @@ export function testProtocolsConfigureHandler(): void { expect(protocolsConfigureReply.status.detail).to.contain(DwnErrorCode.ProtocolsConfigureDuplicateRoleInRuleSet); }); + it('should reject ProtocolsConfigure with role action rule that contain a read action(`read` || `query` || `subscribe`) but not all of the read actions', async () => { + const alice = await TestDataGenerator.generateDidKeyPersona(); + + // without 'subscribe' action + const protocolDefinitionWithoutSubscribe: ProtocolDefinition = { + protocol : 'http://foo', + published : true, + types : { + friend : {}, + foo : {}, + }, + structure: { + friend: { + $role: true + }, + foo: { + $actions: [ + { + role : 'friend', + can : [ProtocolAction.Read, ProtocolAction.Query] // missing `subscribe` + } + ] + } + } + }; + + // manually craft the invalid ProtocolsConfigure message because our library will not let you create an invalid definition + let descriptor: ProtocolsConfigureDescriptor = { + interface : DwnInterfaceName.Protocols, + method : DwnMethodName.Configure, + messageTimestamp : Time.getCurrentTimestamp(), + definition : protocolDefinitionWithoutSubscribe + }; + + let authorization = await Message.createAuthorization({ + descriptor, + signer: Jws.createSigner(alice) + }); + let protocolsConfigureMessage = { descriptor, authorization }; + + const withoutSubscribeResponse = await dwn.processMessage(alice.did, protocolsConfigureMessage); + expect(withoutSubscribeResponse.status.code).to.equal(400); + + // without 'query' action + const protocolDefinitionWithoutQuery: ProtocolDefinition = { + protocol : 'http://foo', + published : true, + types : { + friend : {}, + foo : {}, + }, + structure: { + friend: { + $role: true + }, + foo: { + $actions: [ + { + role : 'friend', + can : [ProtocolAction.Read, ProtocolAction.Subscribe] // missing `query` + } + ] + } + } + }; + + descriptor = { + interface : DwnInterfaceName.Protocols, + method : DwnMethodName.Configure, + messageTimestamp : Time.getCurrentTimestamp(), + definition : protocolDefinitionWithoutQuery + }; + + authorization = await Message.createAuthorization({ + descriptor, + signer: Jws.createSigner(alice) + }); + protocolsConfigureMessage = { descriptor, authorization }; + + const withoutQueryResponse = await dwn.processMessage(alice.did, protocolsConfigureMessage); + expect(withoutQueryResponse.status.code).to.equal(400); + + // without 'read' action + const protocolDefinitionWithoutRead: ProtocolDefinition = { + protocol : 'http://foo', + published : true, + types : { + friend : {}, + foo : {}, + }, + structure: { + friend: { + $role: true + }, + foo: { + $actions: [ + { + role : 'friend', + can : [ProtocolAction.Query, ProtocolAction.Subscribe] // missing `read` + } + ] + } + } + }; + + descriptor = { + interface : DwnInterfaceName.Protocols, + method : DwnMethodName.Configure, + messageTimestamp : Time.getCurrentTimestamp(), + definition : protocolDefinitionWithoutRead + }; + + authorization = await Message.createAuthorization({ + descriptor, + signer: Jws.createSigner(alice) + }); + + protocolsConfigureMessage = { descriptor, authorization }; + + const withoutReadResponse = await dwn.processMessage(alice.did, protocolsConfigureMessage); + expect(withoutReadResponse.status.code).to.equal(400); + + + // sanity, all read actions exist + const protocolDefinitionWithAllReadActions: ProtocolDefinition = { + protocol : 'http://foo', + published : true, + types : { + friend : {}, + foo : {}, + }, + structure: { + friend: { + $role: true + }, + foo: { + $actions: [ + { + role : 'friend', + can : [ProtocolAction.Read, ProtocolAction.Query, ProtocolAction.Subscribe] + } + ] + } + } + }; + + const withAllReadActions = await TestDataGenerator.generateProtocolsConfigure({ + author : alice, + protocolDefinition : protocolDefinitionWithAllReadActions, + }); + + const withAllReadActionsResponse = await dwn.processMessage(alice.did, withAllReadActions.message); + expect(withAllReadActionsResponse.status.code).to.equal(202); + }); + describe('Grant authorization', () => { it('allows an external party to ProtocolsConfigure only if they have a valid grant', async () => { // scenario: diff --git a/tests/interfaces/protocols-configure.spec.ts b/tests/interfaces/protocols-configure.spec.ts index 80c6f513d..66547a4e6 100644 --- a/tests/interfaces/protocols-configure.spec.ts +++ b/tests/interfaces/protocols-configure.spec.ts @@ -1,7 +1,7 @@ import chaiAsPromised from 'chai-as-promised'; import chai, { expect } from 'chai'; -import type { ProtocolsConfigureDescriptor, ProtocolsConfigureMessage } from '../../src/index.js'; +import type { ProtocolDefinition, ProtocolsConfigureDescriptor, ProtocolsConfigureMessage } from '../../src/index.js'; import dexProtocolDefinition from '../vectors/protocol-definitions/dex.json' assert { type: 'json' }; import { Jws } from '../../src/utils/jws.js'; @@ -233,6 +233,129 @@ describe('ProtocolsConfigure', () => { .to.be.rejectedWith(DwnErrorCode.ProtocolsConfigureRoleDoesNotExistAtGivenPath); }); + it('should reject protocol definitions with `role` actions that contain a read action(`read` || `query` || `subscribe`) but not all of the read actions', async () => { + const alice = await TestDataGenerator.generateDidKeyPersona(); + + // without 'subscribe' action + const protocolDefinitionWithoutSubscribe: ProtocolDefinition = { + protocol : 'http://foo', + published : true, + types : { + friend : {}, + foo : {}, + }, + structure: { + friend: { + $role: true + }, + foo: { + $actions: [ + { + role : 'friend', + can : [ProtocolAction.Read, ProtocolAction.Query] // missing `subscribe` + } + ] + } + } + }; + + const withoutSubscribePromise = TestDataGenerator.generateProtocolsConfigure({ + author : alice, + protocolDefinition : protocolDefinitionWithoutSubscribe, + }); + + await expect(withoutSubscribePromise).to.be.rejectedWith(DwnErrorCode.ProtocolsConfigureRoleReadActionMissing); + + // without 'query' action + const protocolDefinitionWithoutQuery: ProtocolDefinition = { + protocol : 'http://foo', + published : true, + types : { + friend : {}, + foo : {}, + }, + structure: { + friend: { + $role: true + }, + foo: { + $actions: [ + { + role : 'friend', + can : [ProtocolAction.Read, ProtocolAction.Subscribe] // missing `query` + } + ] + } + } + }; + + const withoutQueryPromise = TestDataGenerator.generateProtocolsConfigure({ + author : alice, + protocolDefinition : protocolDefinitionWithoutQuery, + }); + + await expect(withoutQueryPromise).to.be.rejectedWith(DwnErrorCode.ProtocolsConfigureRoleReadActionMissing); + + // without 'read' action + const protocolDefinitionWithoutRead: ProtocolDefinition = { + protocol : 'http://foo', + published : true, + types : { + friend : {}, + foo : {}, + }, + structure: { + friend: { + $role: true + }, + foo: { + $actions: [ + { + role : 'friend', + can : [ProtocolAction.Query, ProtocolAction.Subscribe] // missing `read` + } + ] + } + } + }; + + const withoutReadPromise = TestDataGenerator.generateProtocolsConfigure({ + author : alice, + protocolDefinition : protocolDefinitionWithoutRead, + }); + + await expect(withoutReadPromise).to.be.rejectedWith(DwnErrorCode.ProtocolsConfigureRoleReadActionMissing); + + // sanity, all read actions exist + const protocolDefinitionWithAllReadActions: ProtocolDefinition = { + protocol : 'http://foo', + published : true, + types : { + friend : {}, + foo : {}, + }, + structure: { + friend: { + $role: true + }, + foo: { + $actions: [ + { + role : 'friend', + can : [ProtocolAction.Read, ProtocolAction.Query, ProtocolAction.Subscribe] + } + ] + } + } + }; + + const withAllReadActions = await TestDataGenerator.generateProtocolsConfigure({ + author : alice, + protocolDefinition : protocolDefinitionWithAllReadActions, + }); + expect(withAllReadActions.message.descriptor.definition).to.deep.equal(protocolDefinitionWithAllReadActions); + }); + it('rejects protocol definitions with actions that contain `of` and `who` is `anyone`', async () => { const definition = { published : true, diff --git a/tests/vectors/protocol-definitions/friend-role.json b/tests/vectors/protocol-definitions/friend-role.json index f2cc36dd9..b1f567a1b 100644 --- a/tests/vectors/protocol-definitions/friend-role.json +++ b/tests/vectors/protocol-definitions/friend-role.json @@ -22,7 +22,7 @@ { "role": "fan", "can": [ - "read" + "read", "query", "subscribe" ] }, { diff --git a/tests/vectors/protocol-definitions/slack.json b/tests/vectors/protocol-definitions/slack.json index a1407d7ba..63b8da373 100644 --- a/tests/vectors/protocol-definitions/slack.json +++ b/tests/vectors/protocol-definitions/slack.json @@ -57,7 +57,7 @@ { "role": "community/admin", "can": [ - "read" + "read", "query", "subscribe" ] } ], @@ -156,14 +156,16 @@ "can": [ "create", "update", + "read", "query", + "subscribe", "co-delete" ] }, { "role": "community/gatedChannel/participant", "can": [ - "read" + "read", "query", "subscribe" ] } ], @@ -203,6 +205,8 @@ "create", "update", "query", + "read", + "subscribe", "co-delete" ] } diff --git a/tests/vectors/protocol-definitions/thread-role.json b/tests/vectors/protocol-definitions/thread-role.json index 9bd3b6822..20ef9fe29 100644 --- a/tests/vectors/protocol-definitions/thread-role.json +++ b/tests/vectors/protocol-definitions/thread-role.json @@ -17,7 +17,7 @@ { "role": "thread/participant", "can": [ - "read" + "read", "query", "subscribe" ] } ], @@ -31,6 +31,8 @@ "role": "thread/participant", "can": [ "read", + "query", + "subscribe", "create" ] }