feat: Inherit read permission if only write access (sourcenetwork…

…#3108) ## Relevant issue(s) Resolves sourcenetwork#2992 ## Description An actor granted a write permission still couldn't write unless also given `read` permission Example Policy where reader can strictly only read and writer can strictly only write: ```yaml name: Test Policy description: A Policy actor: name: actor resources: users: permissions: read: expr: owner + reader write: expr: owner + writer relations: owner: types: - actor reader: types: - actor writer: types: - actor ``` Then the policy above (assume `XYZ` is resulting `policyID`) is linked in a schema that is loaded: ```gql type Users @Policy(id: XYZ, resource: "users") { name: String age: Int } ``` Now if the `owner` (index `1`) makes a relationship giving `write` access to the `second` actor (index `2`) in our testing frame work like syntax: ```go testUtils.AddDocActorRelationship{ DocID: 0, RequestorIdentity: 1, TargetIdentity: 2, Relation: "writer", } ``` The identity `2` still could not mutate due to lack of read permission. ```go testUtils.UpdateDoc{ Identity: immutable.Some(2), // This identity can still not update. DocID: 0, Doc: ` { "name": "Shahzad Lone" } `, ExpectedError: "document not found or not authorized to access", } ``` Some existing tests that documented this have now been updated with the new behavior: - `TestACP_OwnerGivesUpdateWriteAccessToAnotherActorWithoutExplicitReadPerm_OtherActorCantUpdate` -> `TestACP_OwnerGivesUpdateWriteAccessToAnotherActorWithoutExplicitReadPerm_OtherActorCanUpdate` ## How has this been tested? - CI & Fixed the asserted test that documented this behavior
nasdf · Oct 8, 2024 · bc68f57 · bc68f57
1 parent 655e1a4
commit bc68f57
Show file tree

Hide file tree

Showing 6 changed files with 159 additions and 45 deletions.
diff --git a/acp/acp_local_test.go b/acp/acp_local_test.go
@@ -482,7 +482,7 @@ func Test_LocalACP_InMemory_CheckDocAccess_TrueIfHaveAccessFalseIfNotErrorOtherw
 		policyID,
 	)
 
-	// Invalid empty arguments such that we can't check doc access.
+	// Invalid empty arguments such that we can't check doc access (read).
 	hasAccess, errCheckDocAccess := localACP.CheckDocAccess(
 		ctx,
 		ReadPermission,
@@ -495,6 +495,19 @@ func Test_LocalACP_InMemory_CheckDocAccess_TrueIfHaveAccessFalseIfNotErrorOtherw
 	require.ErrorIs(t, errCheckDocAccess, ErrFailedToVerifyDocAccessWithACP)
 	require.False(t, hasAccess)
 
+	// Invalid empty arguments such that we can't check doc access (write).
+	hasAccess, errCheckDocAccess = localACP.CheckDocAccess(
+		ctx,
+		WritePermission,
+		identity1.DID,
+		validPolicyID,
+		"",
+		"",
+	)
+	require.Error(t, errCheckDocAccess)
+	require.ErrorIs(t, errCheckDocAccess, ErrFailedToVerifyDocAccessWithACP)
+	require.False(t, hasAccess)
+
 	// Check document accesss for a document that does not exist.
 	hasAccess, errCheckDocAccess = localACP.CheckDocAccess(
 		ctx,
@@ -568,7 +581,7 @@ func Test_LocalACP_PersistentMemory_CheckDocAccess_TrueIfHaveAccessFalseIfNotErr
 		policyID,
 	)
 
-	// Invalid empty arguments such that we can't check doc access.
+	// Invalid empty arguments such that we can't check doc access (read).
 	hasAccess, errCheckDocAccess := localACP.CheckDocAccess(
 		ctx,
 		ReadPermission,
@@ -581,6 +594,19 @@ func Test_LocalACP_PersistentMemory_CheckDocAccess_TrueIfHaveAccessFalseIfNotErr
 	require.ErrorIs(t, errCheckDocAccess, ErrFailedToVerifyDocAccessWithACP)
 	require.False(t, hasAccess)
 
+	// Invalid empty arguments such that we can't check doc access (write).
+	hasAccess, errCheckDocAccess = localACP.CheckDocAccess(
+		ctx,
+		WritePermission,
+		identity1.DID,
+		validPolicyID,
+		"",
+		"",
+	)
+	require.Error(t, errCheckDocAccess)
+	require.ErrorIs(t, errCheckDocAccess, ErrFailedToVerifyDocAccessWithACP)
+	require.False(t, hasAccess)
+
 	// Check document accesss for a document that does not exist.
 	hasAccess, errCheckDocAccess = localACP.CheckDocAccess(
 		ctx,

diff --git a/acp/dpi.go b/acp/dpi.go
@@ -22,6 +22,14 @@ const (
 	WritePermission
 )
 
+// permissionsThatImplyRead is a list of any permissions that if we have, we assume that the user can read.
+// This is because for DefraDB's purposes if an identity has access to the write permission, then they don't
+// need to explicitly have read permission inorder to read, we can just imply that they have read access.
+var permissionsThatImplyRead = []DPIPermission{
+	ReadPermission,
+	WritePermission,
+}
+
 // List of all valid DPI permissions, the order of permissions in this list must match
 // the above defined ordering such that iota matches the index position within the list.
 var dpiRequiredPermissions = []string{

diff --git a/acp/errors.go b/acp/errors.go
@@ -129,6 +129,7 @@ func NewErrFailedToCheckIfDocIsRegisteredWithACP(
 func NewErrFailedToVerifyDocAccessWithACP(
 	inner error,
 	Type string,
+	permission string,
 	policyID string,
 	actorID string,
 	resourceName string,
@@ -138,6 +139,7 @@ func NewErrFailedToVerifyDocAccessWithACP(
 		errFailedToVerifyDocAccessWithACP,
 		inner,
 		errors.NewKV("Type", Type),
+		errors.NewKV("Permission", permission),
 		errors.NewKV("PolicyID", policyID),
 		errors.NewKV("ActorID", actorID),
 		errors.NewKV("ResourceName", resourceName),

diff --git a/acp/source_hub_client.go b/acp/source_hub_client.go
@@ -12,6 +12,7 @@ package acp
 
 import (
 	"context"
+	"strconv"
 
 	protoTypes "github.com/cosmos/gogoproto/types"
 	"github.com/sourcenetwork/corelog"
@@ -342,39 +343,86 @@ func (a *sourceHubBridge) CheckDocAccess(
 	resourceName string,
 	docID string,
 ) (bool, error) {
-	isValid, err := a.client.VerifyAccessRequest(
+	// We grant "read" access even if the identity does not explicitly have the "read" permission,
+	// as long as they have any of the permissions that imply read access.
+	if permission == ReadPermission {
+		var canRead bool = false
+		var withPermission string
+		var err error
+
+		for _, permissionThatImpliesRead := range permissionsThatImplyRead {
+			canRead, err = a.client.VerifyAccessRequest(
+				ctx,
+				permissionThatImpliesRead,
+				actorID,
+				policyID,
+				resourceName,
+				docID,
+			)
+
+			if err != nil {
+				return false, NewErrFailedToVerifyDocAccessWithACP(
+					err,
+					"Local",
+					permissionThatImpliesRead.String(),
+					policyID,
+					actorID,
+					resourceName,
+					docID,
+				)
+			}
+
+			if canRead {
+				withPermission = permissionThatImpliesRead.String()
+				break
+			}
+		}
+
+		log.InfoContext(
+			ctx,
+			"Document readable="+strconv.FormatBool(canRead),
+			corelog.Any("Permission", withPermission),
+			corelog.Any("PolicyID", policyID),
+			corelog.Any("Resource", resourceName),
+			corelog.Any("ActorID", actorID),
+			corelog.Any("DocID", docID),
+		)
+
+		return canRead, nil
+	}
+
+	hasAccess, err := a.client.VerifyAccessRequest(
 		ctx,
 		permission,
 		actorID,
 		policyID,
 		resourceName,
 		docID,
 	)
-	if err != nil {
-		return false, NewErrFailedToVerifyDocAccessWithACP(err, "Local", policyID, actorID, resourceName, docID)
-	}
 
-	if isValid {
-		log.InfoContext(
-			ctx,
-			"Document accessible",
-			corelog.Any("PolicyID", policyID),
-			corelog.Any("ActorID", actorID),
-			corelog.Any("Resource", resourceName),
-			corelog.Any("DocID", docID),
-		)
-		return true, nil
-	} else {
-		log.InfoContext(
-			ctx,
-			"Document inaccessible",
-			corelog.Any("PolicyID", policyID),
-			corelog.Any("ActorID", actorID),
-			corelog.Any("Resource", resourceName),
-			corelog.Any("DocID", docID),
+	if err != nil {
+		return false, NewErrFailedToVerifyDocAccessWithACP(
+			err,
+			"Local",
+			permission.String(),
+			policyID,
+			actorID,
+			resourceName,
+			docID,
 		)
-		return false, nil
 	}
+
+	log.InfoContext(
+		ctx,
+		"Document accessible="+strconv.FormatBool(hasAccess),
+		corelog.Any("Permission", permission),
+		corelog.Any("PolicyID", policyID),
+		corelog.Any("Resource", resourceName),
+		corelog.Any("ActorID", actorID),
+		corelog.Any("DocID", docID),
+	)
+
+	return hasAccess, nil
 }
 
 func (a *sourceHubBridge) AddDocActorRelationship(

diff --git a/tests/integration/acp/relationship/doc_actor/add/with_only_write_gql_test.go b/tests/integration/acp/relationship/doc_actor/add/with_only_write_gql_test.go
@@ -19,12 +19,12 @@ import (
 	testUtils "github.com/sourcenetwork/defradb/tests/integration"
 )
 
-func TestACP_OwnerGivesUpdateWriteAccessToAnotherActorWithoutExplicitReadPerm_GQL_OtherActorCantUpdate(t *testing.T) {
+func TestACP_OwnerGivesUpdateWriteAccessToAnotherActorWithoutExplicitReadPerm_GQL_OtherActorCanUpdate(t *testing.T) {
 	expectedPolicyID := "0a243b1e61f990bccde41db7e81a915ffa1507c1403ae19727ce764d3b08846b"
 
 	test := testUtils.TestCase{
 
-		Description: "Test acp, owner gives write(update) access to another actor, without explicit read permission",
+		Description: "Test acp, owner gives write(update) access without explicit read permission, can still update",
 
 		SupportedMutationTypes: immutable.Some([]testUtils.MutationType{
 			// GQL mutation will return no error when wrong identity is used so test that separately.
@@ -161,7 +161,7 @@ func TestACP_OwnerGivesUpdateWriteAccessToAnotherActorWithoutExplicitReadPerm_GQ
 			testUtils.UpdateDoc{
 				CollectionID: 0,
 
-				Identity: immutable.Some(2), // This identity can still not update.
+				Identity: immutable.Some(2), // This identity can now update.
 
 				DocID: 0,
 
@@ -170,12 +170,10 @@ func TestACP_OwnerGivesUpdateWriteAccessToAnotherActorWithoutExplicitReadPerm_GQ
 						"name": "Shahzad Lone"
 					}
 				`,
-
-				SkipLocalUpdateEvent: true,
 			},
 
 			testUtils.Request{
-				Identity: immutable.Some(2), // This identity can still not read.
+				Identity: immutable.Some(2), // This identity can now also read.
 
 				Request: `
 					query {
@@ -188,7 +186,13 @@ func TestACP_OwnerGivesUpdateWriteAccessToAnotherActorWithoutExplicitReadPerm_GQ
 				`,
 
 				Results: map[string]any{
-					"Users": []map[string]any{},
+					"Users": []map[string]any{
+						{
+							"_docID": "bae-9d443d0c-52f6-568b-8f74-e8ff0825697b",
+							"name":   "Shahzad Lone", // Note: updated name
+							"age":    int64(28),
+						},
+					},
 				},
 			},
 		},

diff --git a/tests/integration/acp/relationship/doc_actor/add/with_only_write_test.go b/tests/integration/acp/relationship/doc_actor/add/with_only_write_test.go
@@ -19,12 +19,12 @@ import (
 	testUtils "github.com/sourcenetwork/defradb/tests/integration"
 )
 
-func TestACP_OwnerGivesUpdateWriteAccessToAnotherActorWithoutExplicitReadPerm_OtherActorCantUpdate(t *testing.T) {
+func TestACP_OwnerGivesUpdateWriteAccessToAnotherActorWithoutExplicitReadPerm_OtherActorCanUpdate(t *testing.T) {
 	expectedPolicyID := "0a243b1e61f990bccde41db7e81a915ffa1507c1403ae19727ce764d3b08846b"
 
 	test := testUtils.TestCase{
 
-		Description: "Test acp, owner gives write(update) access to another actor, without explicit read permission",
+		Description: "Test acp, owner gives write(update) access without explicit read permission, can still update",
 
 		SupportedMutationTypes: immutable.Some([]testUtils.MutationType{
 			testUtils.CollectionNamedMutationType,
@@ -161,7 +161,7 @@ func TestACP_OwnerGivesUpdateWriteAccessToAnotherActorWithoutExplicitReadPerm_Ot
 			testUtils.UpdateDoc{
 				CollectionID: 0,
 
-				Identity: immutable.Some(2), // This identity can still not update.
+				Identity: immutable.Some(2), // This identity can now update.
 
 				DocID: 0,
 
@@ -170,12 +170,10 @@ func TestACP_OwnerGivesUpdateWriteAccessToAnotherActorWithoutExplicitReadPerm_Ot
 						"name": "Shahzad Lone"
 					}
 				`,
-
-				ExpectedError: "document not found or not authorized to access",
 			},
 
 			testUtils.Request{
-				Identity: immutable.Some(2), // This identity can still not read.
+				Identity: immutable.Some(2), // This identity can now also read.
 
 				Request: `
 					query {
@@ -188,7 +186,13 @@ func TestACP_OwnerGivesUpdateWriteAccessToAnotherActorWithoutExplicitReadPerm_Ot
 				`,
 
 				Results: map[string]any{
-					"Users": []map[string]any{},
+					"Users": []map[string]any{
+						{
+							"_docID": "bae-9d443d0c-52f6-568b-8f74-e8ff0825697b",
+							"name":   "Shahzad Lone", // Note: updated name
+							"age":    int64(28),
+						},
+					},
 				},
 			},
 		},
@@ -197,12 +201,12 @@ func TestACP_OwnerGivesUpdateWriteAccessToAnotherActorWithoutExplicitReadPerm_Ot
 	testUtils.ExecuteTestCase(t, test)
 }
 
-func TestACP_OwnerGivesDeleteWriteAccessToAnotherActorWithoutExplicitReadPerm_OtherActorCantDelete(t *testing.T) {
+func TestACP_OwnerGivesDeleteWriteAccessToAnotherActorWithoutExplicitReadPerm_OtherActorCanDelete(t *testing.T) {
 	expectedPolicyID := "0a243b1e61f990bccde41db7e81a915ffa1507c1403ae19727ce764d3b08846b"
 
 	test := testUtils.TestCase{
 
-		Description: "Test acp, owner gives write(delete) access to another actor, without explicit read permission",
+		Description: "Test acp, owner gives write(delete) access without explicit read permission, can still delete",
 
 		Actions: []any{
 			testUtils.AddPolicy{
@@ -326,7 +330,7 @@ func TestACP_OwnerGivesDeleteWriteAccessToAnotherActorWithoutExplicitReadPerm_Ot
 			},
 
 			testUtils.Request{
-				Identity: immutable.Some(2), // This identity can still not read.
+				Identity: immutable.Some(2), // This identity can now read.
 
 				Request: `
 					query {
@@ -339,18 +343,40 @@ func TestACP_OwnerGivesDeleteWriteAccessToAnotherActorWithoutExplicitReadPerm_Ot
 				`,
 
 				Results: map[string]any{
-					"Users": []map[string]any{},
+					"Users": []map[string]any{
+						{
+							"_docID": "bae-9d443d0c-52f6-568b-8f74-e8ff0825697b",
+							"name":   "Shahzad",
+							"age":    int64(28),
+						},
+					},
 				},
 			},
 
 			testUtils.DeleteDoc{
 				CollectionID: 0,
 
-				Identity: immutable.Some(2), // This identity can still not delete.
+				Identity: immutable.Some(2), // This identity can now delete.
 
 				DocID: 0,
+			},
 
-				ExpectedError: "document not found or not authorized to access",
+			testUtils.Request{
+				Identity: immutable.Some(2), // Check if actually deleted.
+
+				Request: `
+					query {
+						Users {
+							_docID
+							name
+							age
+						}
+					}
+				`,
+
+				Results: map[string]any{
+					"Users": []map[string]any{},
+				},
 			},
 		},
 	}