feat: Ability to relate private documents to actors #2907

shahzadlone · 2024-08-12T19:53:32Z

Relevant issue(s)

Resolves #2762

Description

This PR introduces the ability to make use of the relations defined within a policy to create relationships between an actor and a document within a collection. For users sake, I have made the clients (http, and cli) not consume the policyID and resource name but instead a docID and collection name, since the collection will have the policy and resource information available we can fetch that and make lives easier for the users.

This PR also makes use of the manages feature we have had in our policy. The manages essentially defines who can make the relationship manipulation requests.

There are a lot of tests in this PR due to a lot of edge cases I wanted to have tested specific to manger, and ensuring write and read permissions don't leak (i.e. are accidently granted).

CLI Demo

The following lets the target actor be able to now read the private document:

defradb client acp relationship add \
--collection Users \
--docID bae-ff3ceb1c-b5c0-5e86-a024-dd1b16a4261c \
--relation reader \
--actor did:key:z7r8os2G88XXBNBTLj3kFR5rzUJ4VAesbX7PgsA68ak9B5RYcXF5EZEmjRzzinZndPSSwujXb4XKHG6vmKEFG6ZfsfcQn \
--identity e3b722906ee4e56368f581cd8b18ab0f48af1ea53e635e3f7b8acd076676f6ac

Result:

{
  "ExistedAlready": false // <-------------- Indicates a new relationship was formed
}

Future (out-of-scope of this PR):

Most of write tests will split into delete and update in Split ACP write perm into delete and update #2905
Ability to revoke or delete relation coming in Revoke or unshare a document previously created a relation for #2906
Decide on the can't write if no read permission in An actor granted a write permission still can't write unless also given read permission #2992
Move acp logic to a shared repo: Move ACP testing and extra non-defra specific logic to an other shared repo #2980

How has this been tested?

Integration tests

Specify the platform(s) on which this was tested:

Manjaro WSL2

codecov · 2024-08-12T20:12:52Z

Codecov Report

Attention: Patch coverage is 93.13725% with 21 lines in your changes missing coverage. Please review.

Project coverage is 79.70%. Comparing base (3101c61) to head (5c05eeb).
Report is 1 commits behind head on develop.

Files with missing lines	Patch %	Lines
acp/acp_source_hub.go	74.19%	4 Missing and 4 partials ⚠️
http/handler_acp.go	82.35%	4 Missing and 2 partials ⚠️
http/client_acp.go	83.33%	2 Missing and 2 partials ⚠️
internal/db/db.go	86.96%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2907      +/-   ##
===========================================
+ Coverage    79.42%   79.70%   +0.28%     
===========================================
  Files          346      348       +2     
  Lines        26715    27014     +299     
===========================================
+ Hits         21217    21530     +313     
+ Misses        3964     3957       -7     
+ Partials      1534     1527       -7

Flag	Coverage Δ
all-tests	`79.70% <93.14%> (+0.28%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
acp/acp_local.go	`97.56% <100.00%> (+0.29%)`	⬆️
acp/errors.go	`88.07% <100.00%> (+3.02%)`	⬆️
acp/source_hub_client.go	`91.15% <100.00%> (+2.95%)`	⬆️
cli/acp_relationship.go	`100.00% <100.00%> (ø)`
cli/acp_relationship_add.go	`100.00% <100.00%> (ø)`
cli/cli.go	`100.00% <100.00%> (ø)`
client/db.go	`91.30% <ø> (ø)`
client/errors.go	`59.72% <ø> (ø)`
internal/db/permission/check.go	`85.19% <100.00%> (ø)`
internal/db/permission/permission.go	`100.00% <100.00%> (ø)`
... and 5 more

... and 21 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3101c61...5c05eeb. Read the comment docs.

shahzadlone · 2024-08-26T21:28:10Z

tests/integration/acp.go

+		}
+
+		var docID string
+		if action.DocID == -1 {


discussion: I understand if people prefer having DocIDIndex and DocID to test these edge cases rather than -1 to test empty/invalid DocID input. I don't have any strong preferences here, happy to follow consensus.

Same for other -1 inputs as well uses as well.

it sort of breaks consistency with immutable.Option, but I see how it simplifies test cases. I also don't have strong preference.

immutable.Option is for optional fields. This is not an optional field, it's more to test invalid input of that field. Is why I was wondering if others prefer non-index input as well (i.e. DocIDIndex for normal cases and DocID for actual invalid testing).

shahzadlone · 2024-08-26T21:34:30Z

tests/integration/acp/relationship/add_doc_actor_test/add_doc_actor_with_only_write_test.go

+                      users:
+                        permissions:
+                          read:
+                            expr: owner + reader


discussion: Since in these tests writer won't get read permission, even if a relation is made to make an actor a writer, they won't be able to write because the policy doesn't model ability to read. This behavior we can try manipulate on defradb level to change (however not in this PR), but opening this here to start a discussion.

islamaliev

Good work so far. I'm just not sure why it's still in Draft state.
There are a lot of tests here, but I'm not sure if all them are necessary.
A lot of repetitive code in tests, which makes it hard to read every detail.

islamaliev · 2024-08-23T21:50:26Z

cli/acp_relationship_add.go

+defradb client ... --identity e3b722906ee4e56368f581cd8b18ab0f48af1ea53e635e3f7b8acd076676f6ac
+
+
+Example: Let another actor read my private document:


praise: thanks for examples

tests/integration/acp.go

islamaliev · 2024-08-27T15:35:04Z

tests/integration/acp.go

+		}
+
+		var docID string
+		if action.DocID == -1 {


it sort of breaks consistency with immutable.Option, but I see how it simplifies test cases. I also don't have strong preference.

tests/integration/acp/relationship/add_doc_actor_test/add_doc_actor_with_delete_test.go

tests/integration/acp/relationship/add_doc_actor_test/add_doc_actor_with_manager_test.go

tests/integration/acp/relationship/add_doc_actor_test/add_doc_actor_with_reader_test.go

tests/integration/acp/p2p/replicator_with_doc_actor_relationship_test.go

acp/source_hub_client.go

islamaliev

Approving assuming tests will be moved as discussed

internal/db/db.go

client/db.go

acp/source_hub_client.go

AndrewSisley

LGTM, thanks Shahzad!

This is because we will add some more acp related stuff to this file.

## Relevant issue(s) Resolves #2906 ## Description Follow-up to #2907, the ability to delete a relationship (in order to revoke access from an identity). ## CLI Demo The following revokes the target actors 'relational' access to the private doc (can't read anymore): ```bash defradb client acp relationship delete \ --collection Users \ --docID bae-ff3ceb1c-b5c0-5e86-a024-dd1b16a4261c \ --relation reader \ --actor did:key:z7r8os2G88XXBNBTLj3kFR5rzUJ4VAesbX7PgsA68ak9B5RYcXF5EZEmjRzzinZndPSSwujXb4XKHG6vmKEFG6ZfsfcQn \ --identity e3b722906ee4e56368f581cd8b18ab0f48af1ea53e635e3f7b8acd076676f6ac ``` Result: ```json { "RecordFound": true // <-------------- Indicates a relationship was found and deleted } ``` ## How has this been tested? CI + Integration Tests + Unit Tests

shahzadlone added feature New feature or request area/acp Related to the acp (access control) system labels Aug 12, 2024

shahzadlone self-assigned this Aug 12, 2024

shahzadlone force-pushed the lone/doc-share-acp branch from e4083eb to b54963a Compare August 12, 2024 19:55

shahzadlone force-pushed the lone/doc-share-acp branch 4 times, most recently from 2ab1edc to 043d9e6 Compare August 13, 2024 02:49

shahzadlone mentioned this pull request Aug 26, 2024

bot: Bump github.com/sourcenetwork/acp_core from 0.0.0-20240607160510-47a5306b2ad2 to 0.1.0 #2960

Closed

shahzadlone commented Aug 26, 2024

View reviewed changes

islamaliev reviewed Aug 27, 2024

View reviewed changes

shahzadlone force-pushed the lone/doc-share-acp branch from 043d9e6 to 322d982 Compare August 29, 2024 07:37

shahzadlone added this to the DefraDB v0.14 milestone Aug 29, 2024

shahzadlone marked this pull request as ready for review August 29, 2024 17:26

shahzadlone force-pushed the lone/doc-share-acp branch from 9659075 to 30b7be5 Compare August 29, 2024 17:37

shahzadlone requested a review from a team August 29, 2024 17:38

shahzadlone force-pushed the lone/doc-share-acp branch from 30b7be5 to 71bc530 Compare August 29, 2024 17:58

AndrewSisley reviewed Aug 29, 2024

View reviewed changes

tests/integration/acp/p2p/replicator_with_doc_actor_relationship_test.go Show resolved Hide resolved

AndrewSisley reviewed Aug 29, 2024

View reviewed changes

acp/source_hub_client.go Outdated Show resolved Hide resolved

shahzadlone force-pushed the lone/doc-share-acp branch 2 times, most recently from f3edce2 to f5efbd0 Compare August 29, 2024 18:23

islamaliev approved these changes Sep 9, 2024

View reviewed changes

AndrewSisley reviewed Sep 9, 2024

View reviewed changes

internal/db/db.go Outdated Show resolved Hide resolved

AndrewSisley reviewed Sep 9, 2024

View reviewed changes

client/db.go Outdated Show resolved Hide resolved

AndrewSisley reviewed Sep 9, 2024

View reviewed changes

acp/source_hub_client.go Outdated Show resolved Hide resolved

shahzadlone force-pushed the lone/doc-share-acp branch from f5efbd0 to f9ed0ca Compare September 18, 2024 21:19

shahzadlone mentioned this pull request Sep 24, 2024

bug: consecutive source-hub acp operations fail for http client due to an identity auth error #3065

Closed

shahzadlone force-pushed the lone/doc-share-acp branch from f9ed0ca to 7ebd623 Compare September 26, 2024 07:48

shahzadlone force-pushed the lone/doc-share-acp branch 6 times, most recently from 14480ed to f227499 Compare September 30, 2024 18:56

AndrewSisley approved these changes Sep 30, 2024

View reviewed changes

shahzadlone added 7 commits October 1, 2024 15:49

PR(-): Rename acp_policy cli make command

c55d0b8

PR(-): Rename isPermissioned to IsPermissioned

62fbccf

PR(-): Move policy.go stuff all under acp.go

0a6fa15

This is because we will add some more acp related stuff to this file.

PR(MAIN): Implement local & sourcehub acp doc sharing

ccddba3

PR(TEST): Add doc actor relationship testing

84412ce

PR(TEST): Detect as an ACP test

333ada0

PR(FIX): Fix identity seeding bug that left empty identities

afb27a7

shahzadlone force-pushed the lone/doc-share-acp branch 4 times, most recently from 782e70c to a42ca82 Compare October 1, 2024 23:04

shahzadlone added 5 commits October 1, 2024 19:23

PR(TEST): Add doc actor relationship integration tests

2d9086e

PR(TEST): Add doc actor relationship unit tests

23274af

PR(TEST): Add p2p <> doc actor relationship tests

c885284

PR(MOCKS): Generate mocks

640b288

PR(DOCS): Update Documentation + make docs

5c05eeb

shahzadlone force-pushed the lone/doc-share-acp branch from a42ca82 to 5c05eeb Compare October 1, 2024 23:24

shahzadlone merged commit c986312 into sourcenetwork:develop Oct 2, 2024
42 of 43 checks passed

shahzadlone deleted the lone/doc-share-acp branch October 2, 2024 00:22

shahzadlone mentioned this pull request Oct 3, 2024

feat: Ability to unrelate private documents from actors #3099

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Ability to relate private documents to actors #2907

feat: Ability to relate private documents to actors #2907

shahzadlone commented Aug 12, 2024 •

edited

Loading

codecov bot commented Aug 12, 2024 •

edited

Loading

shahzadlone Aug 26, 2024 •

edited

Loading

islamaliev Aug 27, 2024

shahzadlone Aug 28, 2024

shahzadlone Aug 26, 2024

islamaliev left a comment

islamaliev Aug 23, 2024

islamaliev Aug 27, 2024

islamaliev left a comment

AndrewSisley left a comment

		defradb client ... --identity e3b722906ee4e56368f581cd8b18ab0f48af1ea53e635e3f7b8acd076676f6ac


		Example: Let another actor read my private document:

feat: Ability to relate private documents to actors #2907

feat: Ability to relate private documents to actors #2907

Conversation

shahzadlone commented Aug 12, 2024 • edited Loading

Relevant issue(s)

Description

CLI Demo

Future (out-of-scope of this PR):

How has this been tested?

codecov bot commented Aug 12, 2024 • edited Loading

Codecov Report

shahzadlone Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

islamaliev Aug 27, 2024

Choose a reason for hiding this comment

shahzadlone Aug 28, 2024

Choose a reason for hiding this comment

shahzadlone Aug 26, 2024

Choose a reason for hiding this comment

islamaliev left a comment

Choose a reason for hiding this comment

islamaliev Aug 23, 2024

Choose a reason for hiding this comment

islamaliev Aug 27, 2024

Choose a reason for hiding this comment

islamaliev left a comment

Choose a reason for hiding this comment

AndrewSisley left a comment

Choose a reason for hiding this comment

shahzadlone commented Aug 12, 2024 •

edited

Loading

codecov bot commented Aug 12, 2024 •

edited

Loading

shahzadlone Aug 26, 2024 •

edited

Loading