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

Property rules documentation #1076

Merged
merged 8 commits into from
Jun 12, 2024
Merged

Conversation

HannesSandberg
Copy link
Contributor

No description provided.

@neo-technology-commit-status-publisher
Copy link
Collaborator

This PR includes documentation updates.

You can view the updated docs at https://neo4j-docs-operations-1076.surge.sh

Copy link
Contributor

@phil198 phil198 left a comment

Choose a reason for hiding this comment

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

review comments so far

Copy link
Contributor

@Hunterness Hunterness left a comment

Choose a reason for hiding this comment

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

Just have the main file left to look at but you can have these comments for now XD

Copy link
Contributor

@Hunterness Hunterness left a comment

Choose a reason for hiding this comment

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

Initial comments on main file

@renetapopova
Copy link
Contributor

@HannesSandberg, Which version does this PR apply to? I'll review it after you finish discussing it.

@renetapopova renetapopova self-requested a review October 13, 2023 08:21
@HannesSandberg
Copy link
Contributor Author

Hello! Thank you @renetapopova

it does not belong to any version yet. The feature is released behind a feature flag in 5.13 but we do not know yet when it will properly released.

@HannesSandberg
Copy link
Contributor Author

@renetapopova Therese have approved this now, feel free to take a look when you have time.

Copy link
Contributor

@renetapopova renetapopova left a comment

Choose a reason for hiding this comment

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

I added some editorial comments.

@@ -332,3 +332,42 @@ Due to this, each node with the `:Person` label needs to be accessed and examine
Due to the additional data accesses that the security checks need to do, this operation will be slower compared to executing the query as an unrestricted user.
Copy link
Contributor

Choose a reason for hiding this comment

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

There are repetitions of "due to" and "make sure", which would be good to address, but I can't add a suggestion there.

@@ -148,6 +181,13 @@ For example if you want to grant the ability to read the properties `language` a
GRANT MATCH { language, length } ON GRAPH neo4j NODES Message TO regularUsers
----

The following query exemplifies how you can grant the ability to find nodes `Post` and `Likes` where property `secret` is set to `false`, as well as reading all their properties to the role `regularUsers`:
Copy link
Contributor

@renetapopova renetapopova Oct 18, 2023

Choose a reason for hiding this comment

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

OK, now I remember why I've changed it - "grants" is too far from "the regularUsers role."

@HannesSandberg
Copy link
Contributor Author

Nice suggestions! Thanks @renetapopova

Copy link
Contributor

@renetapopova renetapopova left a comment

Choose a reason for hiding this comment

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

@renetapopova renetapopova self-assigned this Oct 30, 2023
@renetapopova
Copy link
Contributor

@HannesSandberg, when are we going to merge this one?

@HannesSandberg
Copy link
Contributor Author

Hello @renetapopova! Not yet, this feature is still under early access and not yet publicly available.

I have a question for you as well. Me and @phil198 are making further improvements for this feature that must be documented as well.

Do you want that to be added to this pull request?

In that case I suggest that I squash all of the commits in this PR into one that are approved, and then we can add new ones on top of that one.

@renetapopova
Copy link
Contributor

Hello @renetapopova! Not yet, this feature is still under early access and not yet publicly available.

I have a question for you as well. Me and @phil198 are making further improvements for this feature that must be documented as well.

Do you want that to be added to this pull request?

In that case I suggest that I squash all of the commits in this PR into one that are approved, and then we can add new ones on top of that one.

Sounds good to me. Thanks!

@HannesSandberg HannesSandberg force-pushed the property-rules branch 2 times, most recently from ed2372f to 547d12b Compare November 29, 2023 12:21
@HannesSandberg
Copy link
Contributor Author

We have added the new syntax and an example in the last commit. Could you please re-review @Hunterness @renetapopova

Copy link
Contributor

@Hunterness Hunterness left a comment

Choose a reason for hiding this comment

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

updates looks good

----
([var][:label["|" ...]] "{" property: value "}")
| (var[:label["|" ...]]) WHERE [NOT] var.property { { = | <> | > | >= | < | <= } value | IS NULL | IS NOT NULL }
| (var[:label["|" ...]] WHERE [NOT] var.property { { = | <> | > | >= | < | <= } value | IS NULL | IS NOT NULL } )
Copy link
Contributor

Choose a reason for hiding this comment

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

one thing to keep in mind for when this is ready for merge/GA is that we'll need to update the additions, deprecations, removals section (currently in cypher manual, but I think there was some talk about making something for the operations manual/administration section as well since it moved manual so I guess wherever it is at time of GA)

Copy link
Contributor

@renetapopova renetapopova 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 👍

Copy link
Contributor

@Hunterness Hunterness left a comment

Choose a reason for hiding this comment

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

Updates for IN looks good

@HannesSandberg
Copy link
Contributor Author

Added new syntax in the last commit. Could you please review @renetapopova ?

@renetapopova
Copy link
Contributor

Added new syntax in the last commit. Could you please review @renetapopova ?

It looks good!

HannesSandberg and others added 5 commits May 22, 2024 15:00
add property rules example to limitations section

add new section for property-based access control

recomend to use Block Storage

review fixes: limitations page

update images for granting/denying privilege syntax

Update modules/ROOT/pages/authentication-authorization/manage-privileges.adoc

Co-authored-by: Phil Wright <[email protected]>

review fixes

review fixes

review fix

review fixes

change header in limitations page

Update modules/ROOT/pages/authentication-authorization/property-based-access-control.adoc

Co-authored-by: Phil Wright <[email protected]>

Apply suggestions from code review

Co-authored-by: Reneta Popova <[email protected]>
@HannesSandberg
Copy link
Contributor Author

Made a small update, feel free to review @Hunterness and @renetapopova.

Also changing this from draft to PR now since the feature will be released in 5.21.

@HannesSandberg HannesSandberg marked this pull request as ready for review May 22, 2024 13:36
Copy link
Contributor

@renetapopova renetapopova left a comment

Choose a reason for hiding this comment

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

A minor suggestion from me. Also, please don't merge it into dev, as dev still refers to 5.20.

@@ -30,6 +30,8 @@ Some of the factors that can worsen the impact on performance when having proper
For performance-critical scenarios, it is recommended to design privileges based on labels.
For more information, see xref:authentication-authorization/privileges-reads.adoc[Read privileges].

When using property-based access control, ensure the property used for the rule cannot be modified. Users who can change this property can affect granted property-based privileges.
Copy link
Contributor

Choose a reason for hiding this comment

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

One sentence per line and a small correction.

When using property-based access control, ensure the property used for the rule cannot be modified.
Users who can change this property can affect the granted property-based privileges.

@renetapopova renetapopova removed the 5.21 label Jun 11, 2024
@neo-technology-commit-status-publisher
Copy link
Collaborator

Thanks for the documentation updates.

The preview documentation has now been torn down - reopening this PR will republish it.

@renetapopova renetapopova merged commit 8fabf8d into neo4j:dev Jun 12, 2024
8 checks passed
renetapopova added a commit to renetapopova/docs-operations that referenced this pull request Jun 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants