Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Ability to unrelate private documents from actors #3099

Conversation

shahzadlone
Copy link
Member

@shahzadlone shahzadlone commented Oct 3, 2024

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):

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

Result:

{
  "RecordFound": true // <-------------- Indicates a relationship was found and deleted
}

How has this been tested?

CI + Integration Tests + Unit Tests

@shahzadlone shahzadlone added feature New feature or request area/acp Related to the acp (access control) system labels Oct 3, 2024
@shahzadlone shahzadlone added this to the DefraDB v0.14 milestone Oct 3, 2024
@shahzadlone shahzadlone self-assigned this Oct 3, 2024
@shahzadlone shahzadlone requested a review from a team October 3, 2024 17:58
http/handler_acp.go Outdated Show resolved Hide resolved
Copy link

codecov bot commented Oct 3, 2024

Codecov Report

Attention: Patch coverage is 93.42105% with 20 lines in your changes missing coverage. Please review.

Please upload report for BASE (develop@bc93bff). Learn more about missing BASE report.
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 89.47% 4 Missing and 2 partials ⚠️
http/client_acp.go 83.33% 2 Missing and 2 partials ⚠️
internal/db/db.go 90.91% 1 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             develop    #3099   +/-   ##
==========================================
  Coverage           ?   80.08%           
==========================================
  Files              ?      353           
  Lines              ?    28068           
  Branches           ?        0           
==========================================
  Hits               ?    22478           
  Misses             ?     4006           
  Partials           ?     1584           
Flag Coverage Δ
all-tests 80.08% <93.42%> (?)

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.78% <100.00%> (ø)
acp/errors.go 90.08% <100.00%> (ø)
acp/source_hub_client.go 92.92% <100.00%> (ø)
cli/acp_relationship_add.go 100.00% <100.00%> (ø)
cli/acp_relationship_delete.go 100.00% <100.00%> (ø)
cli/cli.go 100.00% <100.00%> (ø)
client/db.go 91.30% <ø> (ø)
http/openapi.go 98.10% <ø> (ø)
internal/db/db.go 66.46% <90.91%> (ø)
http/client_acp.go 83.87% <83.33%> (ø)
... and 2 more

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 bc93bff...9d7e222. Read the comment docs.

Copy link
Contributor

@AndrewSisley AndrewSisley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good, but there are a couple of important todos for you before I can approve. My brain is a little foggy atm too, so it might be wise to wait for someone else to review too.

tests/integration/acp.go Show resolved Hide resolved
@shahzadlone shahzadlone force-pushed the lone/feat/remove-doc-actor-relationship branch 5 times, most recently from 9c519d0 to 7abffb2 Compare October 3, 2024 21:35
@shahzadlone shahzadlone requested review from AndrewSisley and a team October 3, 2024 21:35
Copy link
Contributor

@AndrewSisley AndrewSisley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, but I would prefer you got a second reviewer to look over this one before merge :) (don't wait more than a couple of working days for that though)

Copy link
Member

@nasdf nasdf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two minor things to consider before merging. The documentation and tests are awesome as always!

cli/acp_relationship_delete.go Show resolved Hide resolved
http/handler_acp.go Show resolved Hide resolved
@shahzadlone shahzadlone force-pushed the lone/feat/remove-doc-actor-relationship branch from e3f2770 to 9d7e222 Compare October 4, 2024 21:45
@shahzadlone shahzadlone merged commit 2bdbbd4 into sourcenetwork:develop Oct 4, 2024
43 checks passed
@shahzadlone shahzadlone deleted the lone/feat/remove-doc-actor-relationship branch October 4, 2024 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/acp Related to the acp (access control) system feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Revoke or unshare a document previously created a relation for
3 participants