From 1a6d0b4599a48bd197b9f58ff64944866b5965b4 Mon Sep 17 00:00:00 2001 From: Shahzad Lone Date: Thu, 2 Jan 2025 15:06:07 -0500 Subject: [PATCH 1/4] PR(MAIN): Update implementation --- acp/acp.go | 2 +- acp/dpi.go | 13 ++++++++----- internal/db/collection.go | 2 +- internal/db/collection_delete.go | 2 +- 4 files changed, 11 insertions(+), 8 deletions(-) diff --git a/acp/acp.go b/acp/acp.go index d30c45d128..88984c56dd 100644 --- a/acp/acp.go +++ b/acp/acp.go @@ -89,7 +89,7 @@ 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"). + // - permission here is a valid DPI permission we are checking for ("read" or "update" or "delete"). CheckDocAccess( ctx context.Context, permission DPIPermission, diff --git a/acp/dpi.go b/acp/dpi.go index c2c18b245a..692bc8b9ea 100644 --- a/acp/dpi.go +++ b/acp/dpi.go @@ -19,22 +19,25 @@ type DPIPermission int // Valid DefraDB Policy Interface Permission Type. const ( ReadPermission DPIPermission = iota - WritePermission + UpdatePermission + DeletePermission ) // 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. +// This is because for DefraDB's purposes if an identity has access to any write permission (delete or update), +// 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, + UpdatePermission, + DeletePermission, } // 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{ "read", - "write", + "update", + "delete", } func (dpiPermission DPIPermission) String() string { diff --git a/internal/db/collection.go b/internal/db/collection.go index ee372db469..d5356871d6 100644 --- a/internal/db/collection.go +++ b/internal/db/collection.go @@ -523,7 +523,7 @@ func (c *collection) update( // Stop the update if the correct permissions aren't there. canUpdate, err := c.checkAccessOfDocWithACP( ctx, - acp.WritePermission, + acp.UpdatePermission, doc.ID().String(), ) if err != nil { diff --git a/internal/db/collection_delete.go b/internal/db/collection_delete.go index a3963be2f4..dbba4fa925 100644 --- a/internal/db/collection_delete.go +++ b/internal/db/collection_delete.go @@ -128,7 +128,7 @@ func (c *collection) applyDelete( // Stop deletion of document if the correct permissions aren't there. canDelete, err := c.checkAccessOfDocWithACP( ctx, - acp.WritePermission, + acp.DeletePermission, primaryKey.DocID, ) From 06e469f3a9a46d85ff7e5145da13eeeabc6126bd Mon Sep 17 00:00:00 2001 From: Shahzad Lone Date: Thu, 2 Jan 2025 15:06:07 -0500 Subject: [PATCH 2/4] PR(DOCS): Update acp readme --- acp/README.md | 37 +++++++++++++++++++++++++++++-------- 1 file changed, 29 insertions(+), 8 deletions(-) diff --git a/acp/README.md b/acp/README.md index 0f73662dd9..6eccb47b3c 100644 --- a/acp/README.md +++ b/acp/README.md @@ -174,6 +174,8 @@ defradb client ... --identity e3b722906ee4e56368f581cd8b18ab0f48af1ea53e635e3f7b We have in `examples/dpi_policy/user_dpi_policy.yml`: ```yaml +name: An Example Policy + description: A Valid DefraDB Policy Interface (DPI) actor: @@ -183,9 +185,11 @@ resources: users: permissions: read: - expr: owner + reader - write: - expr: owner + expr: owner + reader + updater + deleter + update: + expr: owner + updater + delete: + expr: owner + deleter relations: owner: @@ -194,6 +198,12 @@ resources: reader: types: - actor + updater: + types: + - actor + deleter: + types: + - actor ``` CLI Command: @@ -443,8 +453,8 @@ Note: - The collection with the target document must have a valid policy and resource linked. - The target document must be registered with ACP already (private document). - The requesting identity MUST either be the owner OR the manager (manages the relation) of the resource. - - If the specified relation was not granted the miminum DPI permissions (read or write) within the policy, - and a relationship is formed, the subject/actor will still not be able to access (read or write) the resource. + - If the specified relation was not granted the miminum DPI permissions (read or update or delete) within the policy, + and a relationship is formed, the subject/actor will still not be able to access (read or update or delete) the resource. - If the relationship already exists, then it will just be a no-op. Consider the following policy that we have under `examples/dpi_policy/user_dpi_policy_with_manages.yml`: @@ -461,10 +471,13 @@ resources: users: permissions: read: - expr: owner + reader + writer + expr: owner + reader + updater + deleter + + update: + expr: owner + updater - write: - expr: owner + writer + delete: + expr: owner + deleter nothing: expr: dummy @@ -478,6 +491,14 @@ resources: types: - actor + updater: + types: + - actor + + deleter: + types: + - actor + writer: types: - actor From 282e34dbb7a25ed62462a95a1862e4d8c38f520c Mon Sep 17 00:00:00 2001 From: Shahzad Lone Date: Thu, 2 Jan 2025 15:06:07 -0500 Subject: [PATCH 3/4] PR(DOCS): Update Examples --- examples/dpi_policy/user_dpi_policy.json | 19 ++++++++++++++++--- examples/dpi_policy/user_dpi_policy.yml | 14 +++++++++++--- .../user_dpi_policy_with_manages.yml | 17 ++++++++++++++--- 3 files changed, 41 insertions(+), 9 deletions(-) diff --git a/examples/dpi_policy/user_dpi_policy.json b/examples/dpi_policy/user_dpi_policy.json index 96c794b490..45546039b9 100644 --- a/examples/dpi_policy/user_dpi_policy.json +++ b/examples/dpi_policy/user_dpi_policy.json @@ -8,10 +8,13 @@ "users": { "permissions": { "read": { - "expr": "owner + reader" + "expr": "owner + reader + updater + deleter" }, - "write": { - "expr": "owner" + "update": { + "expr": "owner + updater" + } + "delete": { + "expr": "owner + deleter" } }, "relations": { @@ -25,6 +28,16 @@ "actor" ] } + "updater": { + "types": [ + "actor" + ] + } + "deleter": { + "types": [ + "actor" + ] + } } } } diff --git a/examples/dpi_policy/user_dpi_policy.yml b/examples/dpi_policy/user_dpi_policy.yml index 1b1df1e0b9..2b154c84da 100644 --- a/examples/dpi_policy/user_dpi_policy.yml +++ b/examples/dpi_policy/user_dpi_policy.yml @@ -18,9 +18,11 @@ resources: users: permissions: read: - expr: owner + reader - write: - expr: owner + expr: owner + reader + updater + deleter + update: + expr: owner + updater + delete: + expr: owner + deleter relations: owner: @@ -29,3 +31,9 @@ resources: reader: types: - actor + updater: + types: + - actor + deleter: + types: + - actor diff --git a/examples/dpi_policy/user_dpi_policy_with_manages.yml b/examples/dpi_policy/user_dpi_policy_with_manages.yml index 4667660136..23ed2dc019 100644 --- a/examples/dpi_policy/user_dpi_policy_with_manages.yml +++ b/examples/dpi_policy/user_dpi_policy_with_manages.yml @@ -17,10 +17,13 @@ resources: users: permissions: read: - expr: owner + reader + writer + expr: owner + reader + updater + deleter + writer - write: - expr: owner + writer + update: + expr: owner + updater + + delete: + expr: owner + deleter nothing: expr: dummy @@ -34,6 +37,14 @@ resources: types: - actor + updater: + types: + - actor + + deleter: + types: + - actor + writer: types: - actor From 19a52d878a6ad704fba9f930093a922b0822156d Mon Sep 17 00:00:00 2001 From: Shahzad Lone Date: Thu, 2 Jan 2025 15:06:07 -0500 Subject: [PATCH 4/4] PR(TEST-UNIT): Update acp unit tests --- acp/acp_local_test.go | 42 +++++++++++++++++++++++++++++++++++------- 1 file changed, 35 insertions(+), 7 deletions(-) diff --git a/acp/acp_local_test.go b/acp/acp_local_test.go index b3763da2a9..5b9794aa3e 100644 --- a/acp/acp_local_test.go +++ b/acp/acp_local_test.go @@ -29,7 +29,7 @@ var invalidIdentity = identity.Identity{ DID: "did:something", } -var validPolicyID string = "d59f91ba65fe142d35fc7df34482eafc7e99fed7c144961ba32c4664634e61b7" +var validPolicyID string = "87480b693bdaccdbbe1c1334204b31fd1cb44d3ad70b66fab57595283714fbfe" var validPolicy string = ` name: test description: a policy @@ -40,10 +40,12 @@ actor: resources: users: permissions: - write: - expr: owner read: expr: owner + reader + update: + expr: owner + delete: + expr: owner relations: owner: @@ -495,10 +497,23 @@ 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). + // Invalid empty arguments such that we can't check doc access (update). hasAccess, errCheckDocAccess = localACP.CheckDocAccess( ctx, - WritePermission, + UpdatePermission, + identity1.DID, + validPolicyID, + "", + "", + ) + require.Error(t, errCheckDocAccess) + require.ErrorIs(t, errCheckDocAccess, ErrFailedToVerifyDocAccessWithACP) + require.False(t, hasAccess) + + // Invalid empty arguments such that we can't check doc access (delete). + hasAccess, errCheckDocAccess = localACP.CheckDocAccess( + ctx, + DeletePermission, identity1.DID, validPolicyID, "", @@ -594,10 +609,23 @@ 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). + // Invalid empty arguments such that we can't check doc access (update). + hasAccess, errCheckDocAccess = localACP.CheckDocAccess( + ctx, + UpdatePermission, + identity1.DID, + validPolicyID, + "", + "", + ) + require.Error(t, errCheckDocAccess) + require.ErrorIs(t, errCheckDocAccess, ErrFailedToVerifyDocAccessWithACP) + require.False(t, hasAccess) + + // Invalid empty arguments such that we can't check doc access (delete). hasAccess, errCheckDocAccess = localACP.CheckDocAccess( ctx, - WritePermission, + DeletePermission, identity1.DID, validPolicyID, "",