From 729c34df24b8f3b7f0ed1b5f76c4131969957828 Mon Sep 17 00:00:00 2001 From: Shahzad Lone Date: Mon, 7 Oct 2024 00:48:11 -0400 Subject: [PATCH] PR(MAIN): Assume `read` if only has `write` permission. --- acp/acp.go | 5 +- acp/acp_local_test.go | 40 +++++------ acp/dpi.go | 8 +++ acp/errors.go | 2 + acp/source_hub_client.go | 68 +++++++++++-------- internal/db/collection.go | 6 +- internal/db/collection_acp.go | 4 +- internal/db/collection_delete.go | 2 +- internal/db/fetcher/fetcher.go | 2 +- internal/db/permission/check.go | 4 +- .../doc_actor/add/with_only_write_gql_test.go | 18 +++-- .../doc_actor/add/with_only_write_test.go | 52 ++++++++++---- 12 files changed, 130 insertions(+), 81 deletions(-) diff --git a/acp/acp.go b/acp/acp.go index d30c45d128..0384ac2c7c 100644 --- a/acp/acp.go +++ b/acp/acp.go @@ -89,10 +89,11 @@ type ACP interface { // Otherwise if check failed then an error is returned (and the boolean result should not be used). // // Note(s): - // - permission here is a valid DPI permission we are checking for ("read" or "write"). + // - permissions here is a valid DPI permission we are checking for ("read" or "write"). + // - returns true if has access to ANY of the given permissions. CheckDocAccess( ctx context.Context, - permission DPIPermission, + permissions []DPIPermission, actorID string, policyID string, resourceName string, diff --git a/acp/acp_local_test.go b/acp/acp_local_test.go index fce65e9974..803393428f 100644 --- a/acp/acp_local_test.go +++ b/acp/acp_local_test.go @@ -485,7 +485,7 @@ func Test_LocalACP_InMemory_CheckDocAccess_TrueIfHaveAccessFalseIfNotErrorOtherw // Invalid empty arguments such that we can't check doc access. hasAccess, errCheckDocAccess := localACP.CheckDocAccess( ctx, - ReadPermission, + []DPIPermission{ReadPermission}, identity1.DID, validPolicyID, "", @@ -498,7 +498,7 @@ func Test_LocalACP_InMemory_CheckDocAccess_TrueIfHaveAccessFalseIfNotErrorOtherw // Check document accesss for a document that does not exist. hasAccess, errCheckDocAccess = localACP.CheckDocAccess( ctx, - ReadPermission, + []DPIPermission{ReadPermission}, identity1.DID, validPolicyID, "users", @@ -520,7 +520,7 @@ func Test_LocalACP_InMemory_CheckDocAccess_TrueIfHaveAccessFalseIfNotErrorOtherw // Now check using correct identity if it has access. hasAccess, errCheckDocAccess = localACP.CheckDocAccess( ctx, - ReadPermission, + []DPIPermission{ReadPermission}, identity1.DID, validPolicyID, "users", @@ -532,7 +532,7 @@ func Test_LocalACP_InMemory_CheckDocAccess_TrueIfHaveAccessFalseIfNotErrorOtherw // Now check using wrong identity, it should not have access. hasAccess, errCheckDocAccess = localACP.CheckDocAccess( ctx, - ReadPermission, + []DPIPermission{ReadPermission}, identity2.DID, validPolicyID, "users", @@ -571,7 +571,7 @@ func Test_LocalACP_PersistentMemory_CheckDocAccess_TrueIfHaveAccessFalseIfNotErr // Invalid empty arguments such that we can't check doc access. hasAccess, errCheckDocAccess := localACP.CheckDocAccess( ctx, - ReadPermission, + []DPIPermission{ReadPermission}, identity1.DID, validPolicyID, "", @@ -584,7 +584,7 @@ func Test_LocalACP_PersistentMemory_CheckDocAccess_TrueIfHaveAccessFalseIfNotErr // Check document accesss for a document that does not exist. hasAccess, errCheckDocAccess = localACP.CheckDocAccess( ctx, - ReadPermission, + []DPIPermission{ReadPermission}, identity1.DID, validPolicyID, "users", @@ -606,7 +606,7 @@ func Test_LocalACP_PersistentMemory_CheckDocAccess_TrueIfHaveAccessFalseIfNotErr // Now check using correct identity if it has access. hasAccess, errCheckDocAccess = localACP.CheckDocAccess( ctx, - ReadPermission, + []DPIPermission{ReadPermission}, identity1.DID, validPolicyID, "users", @@ -618,7 +618,7 @@ func Test_LocalACP_PersistentMemory_CheckDocAccess_TrueIfHaveAccessFalseIfNotErr // Now check using wrong identity, it should not have access. hasAccess, errCheckDocAccess = localACP.CheckDocAccess( ctx, - ReadPermission, + []DPIPermission{ReadPermission}, identity2.DID, validPolicyID, "users", @@ -638,7 +638,7 @@ func Test_LocalACP_PersistentMemory_CheckDocAccess_TrueIfHaveAccessFalseIfNotErr // Now check again after the restart using correct identity if it still has access. hasAccess, errCheckDocAccess = localACP.CheckDocAccess( ctx, - ReadPermission, + []DPIPermission{ReadPermission}, identity1.DID, validPolicyID, "users", @@ -650,7 +650,7 @@ func Test_LocalACP_PersistentMemory_CheckDocAccess_TrueIfHaveAccessFalseIfNotErr // Now check again after restart using wrong identity, it should continue to not have access. hasAccess, errCheckDocAccess = localACP.CheckDocAccess( ctx, - ReadPermission, + []DPIPermission{ReadPermission}, identity2.DID, validPolicyID, "users", @@ -696,7 +696,7 @@ func Test_LocalACP_InMemory_AddDocActorRelationship_FalseIfExistsBeforeTrueIfNoO // Other identity does not have access yet. hasAccess, errCheckDocAccess := localACP.CheckDocAccess( ctx, - ReadPermission, + []DPIPermission{ReadPermission}, identity2.DID, validPolicyID, "users", @@ -734,7 +734,7 @@ func Test_LocalACP_InMemory_AddDocActorRelationship_FalseIfExistsBeforeTrueIfNoO // Now the other identity has access. hasAccess, errCheckDocAccess = localACP.CheckDocAccess( ctx, - ReadPermission, + []DPIPermission{ReadPermission}, identity2.DID, validPolicyID, "users", @@ -783,7 +783,7 @@ func Test_LocalACP_PersistentMemory_AddDocActorRelationship_FalseIfExistsBeforeT // Other identity does not have access yet. hasAccess, errCheckDocAccess := localACP.CheckDocAccess( ctx, - ReadPermission, + []DPIPermission{ReadPermission}, identity2.DID, validPolicyID, "users", @@ -821,7 +821,7 @@ func Test_LocalACP_PersistentMemory_AddDocActorRelationship_FalseIfExistsBeforeT // Now the other identity has access. hasAccess, errCheckDocAccess = localACP.CheckDocAccess( ctx, - ReadPermission, + []DPIPermission{ReadPermission}, identity2.DID, validPolicyID, "users", @@ -841,7 +841,7 @@ func Test_LocalACP_PersistentMemory_AddDocActorRelationship_FalseIfExistsBeforeT // Now check again after the restart that the second identity still has access. hasAccess, errCheckDocAccess = localACP.CheckDocAccess( ctx, - ReadPermission, + []DPIPermission{ReadPermission}, identity2.DID, validPolicyID, "users", @@ -900,7 +900,7 @@ func Test_LocalACP_InMemory_DeleteDocActorRelationship_TrueIfFoundAndDeletedFals // Now the other identity has access. hasAccess, errCheckDocAccess := localACP.CheckDocAccess( ctx, - ReadPermission, + []DPIPermission{ReadPermission}, identity2.DID, validPolicyID, "users", @@ -938,7 +938,7 @@ func Test_LocalACP_InMemory_DeleteDocActorRelationship_TrueIfFoundAndDeletedFals // Other identity now has no access again. hasAccess, errCheckDocAccess = localACP.CheckDocAccess( ctx, - ReadPermission, + []DPIPermission{ReadPermission}, identity2.DID, validPolicyID, "users", @@ -1000,7 +1000,7 @@ func Test_LocalACP_PersistentMemory_DeleteDocActorRelationship_TrueIfFoundAndDel // Now the other identity has access. hasAccess, errCheckDocAccess := localACP.CheckDocAccess( ctx, - ReadPermission, + []DPIPermission{ReadPermission}, identity2.DID, validPolicyID, "users", @@ -1038,7 +1038,7 @@ func Test_LocalACP_PersistentMemory_DeleteDocActorRelationship_TrueIfFoundAndDel // Other identity now has no access again. hasAccess, errCheckDocAccess = localACP.CheckDocAccess( ctx, - ReadPermission, + []DPIPermission{ReadPermission}, identity2.DID, validPolicyID, "users", @@ -1058,7 +1058,7 @@ func Test_LocalACP_PersistentMemory_DeleteDocActorRelationship_TrueIfFoundAndDel // Now check again after the restart that the second identity still has no access. hasAccess, errCheckDocAccess = localACP.CheckDocAccess( ctx, - ReadPermission, + []DPIPermission{ReadPermission}, identity2.DID, validPolicyID, "users", diff --git a/acp/dpi.go b/acp/dpi.go index 85da972131..dffdf0debf 100644 --- 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 index 72fbc00b95..c0e1b9bbec 100644 --- 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 index b211214d9f..afa43da6df 100644 --- 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" @@ -336,45 +337,52 @@ func (a *sourceHubBridge) IsDocRegistered( func (a *sourceHubBridge) CheckDocAccess( ctx context.Context, - permission DPIPermission, + permissions []DPIPermission, actorID string, policyID string, resourceName string, docID string, ) (bool, error) { - isValid, err := a.client.VerifyAccessRequest( - ctx, - permission, - actorID, - policyID, - resourceName, - docID, - ) - if err != nil { - return false, NewErrFailedToVerifyDocAccessWithACP(err, "Local", policyID, actorID, resourceName, docID) - } + var hasAccess bool = false + var err error - if isValid { - log.InfoContext( + for _, permission := range permissions { + hasAccess, err = a.client.VerifyAccessRequest( 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), + permission, + actorID, + policyID, + resourceName, + docID, ) - return false, nil + + if err != nil { + return false, NewErrFailedToVerifyDocAccessWithACP( + err, + "Local", + permission.String(), + policyID, + actorID, + resourceName, + docID, + ) + } + + if hasAccess { + break + } } + + log.InfoContext( + ctx, + "Document accessible="+strconv.FormatBool(hasAccess), + 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/internal/db/collection.go b/internal/db/collection.go index 8f78e51429..a95882a5c3 100644 --- a/internal/db/collection.go +++ b/internal/db/collection.go @@ -327,7 +327,7 @@ func (c *collection) getAllDocIDsChan( canRead, err := c.checkAccessOfDocWithACP( ctx, - acp.ReadPermission, + acp.PermissionsThatImplyRead, docID.String(), ) @@ -524,7 +524,7 @@ func (c *collection) update( // Stop the update if the correct permissions aren't there. canUpdate, err := c.checkAccessOfDocWithACP( ctx, - acp.WritePermission, + []acp.DPIPermission{acp.WritePermission}, doc.ID().String(), ) if err != nil { @@ -890,7 +890,7 @@ func (c *collection) exists( ) (exists bool, isDeleted bool, err error) { canRead, err := c.checkAccessOfDocWithACP( ctx, - acp.ReadPermission, + acp.PermissionsThatImplyRead, primaryKey.DocID, ) if err != nil { diff --git a/internal/db/collection_acp.go b/internal/db/collection_acp.go index 9ca432f9aa..a3fba7cb86 100644 --- a/internal/db/collection_acp.go +++ b/internal/db/collection_acp.go @@ -48,7 +48,7 @@ func (c *collection) registerDocWithACP( func (c *collection) checkAccessOfDocWithACP( ctx context.Context, - dpiPermission acp.DPIPermission, + dpiPermissions []acp.DPIPermission, docID string, ) (bool, error) { // If acp is not available, then we have unrestricted access. @@ -61,7 +61,7 @@ func (c *collection) checkAccessOfDocWithACP( identity, c.db.acp.Value(), c, - dpiPermission, + dpiPermissions, docID, ) } diff --git a/internal/db/collection_delete.go b/internal/db/collection_delete.go index 62d7c24e50..5c1bfab2a7 100644 --- a/internal/db/collection_delete.go +++ b/internal/db/collection_delete.go @@ -126,7 +126,7 @@ func (c *collection) applyDelete( // Stop deletion of document if the correct permissions aren't there. canDelete, err := c.checkAccessOfDocWithACP( ctx, - acp.WritePermission, + []acp.DPIPermission{acp.WritePermission}, primaryKey.DocID, ) diff --git a/internal/db/fetcher/fetcher.go b/internal/db/fetcher/fetcher.go index 06e3255e8c..26833d86e0 100644 --- a/internal/db/fetcher/fetcher.go +++ b/internal/db/fetcher/fetcher.go @@ -633,7 +633,7 @@ func (df *DocumentFetcher) fetchNext(ctx context.Context) (EncodedDocument, Exec df.identity, df.acp.Value(), df.col, - acp.ReadPermission, + acp.PermissionsThatImplyRead, df.kv.Key.DocID, ) diff --git a/internal/db/permission/check.go b/internal/db/permission/check.go index b19500f41b..19a5f4a4c2 100644 --- a/internal/db/permission/check.go +++ b/internal/db/permission/check.go @@ -38,7 +38,7 @@ func CheckAccessOfDocOnCollectionWithACP( identity immutable.Option[acpIdentity.Identity], acpSystem acp.ACP, collection client.Collection, - permission acp.DPIPermission, + permissions []acp.DPIPermission, docID string, ) (bool, error) { // Even if acp exists, but there is no policy on the collection (unpermissioned collection) @@ -77,7 +77,7 @@ func CheckAccessOfDocOnCollectionWithACP( // Now actually check using the signature if this identity has access or not. hasAccess, err := acpSystem.CheckDocAccess( ctx, - permission, + permissions, identity.Value().DID, policyID, resourceName, 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 index 36bf181478..9c6649c2c1 100644 --- 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 index 09703f93aa..8333790f3d 100644 --- 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{}, + }, }, }, }