[ 
https://issues.apache.org/jira/browse/OAK-8855?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17024919#comment-17024919
 ] 

Angela Schreiber commented on OAK-8855:
---------------------------------------

[~kunal3112], probably i was mistaken regarding the removal of hidden items.

as far as touching the rep:principalNames are concerned: i would suggest you 
actually change the values to make it less confusing and also add comments 
explaining why you are modifying the properties... essentially adding all the 
explaining you did here. this would also make the final assertions clearer as 
you can test it for the existing principals, which you are not changing 
effectively _and_ the modifications you made to the cug policy.

also i would like you to add addtional ways to test the result i would suggest 
too use {{PermissionProvider}} obtained from the admin 'root' object to verify 
the effective permissions. since adding addtional verifications may result in a 
convoluted, complex test case, i would suggest the following:
 # move all of the setup you need including removal of the nested-cug property 
to a separate private method or to the setup method. essentially making this 
test class only about this very issue.
 # then you can create multiple test-methods to look at and verify the expected 
outcome from different angles:
 ## looking at the test-session (as you already do),
 ## looking at the effective permissions for a given principal (or set of 
principals) (obtain {{PermissionProvider}}from the root would allow you to do 
this){{ }}
 ## use {{RootProvider}} to obtain an read-only wrapper for the {{root}} 
object, which allows you actually to look at the hidden property and verify 
that it's being restored in the way you intended it to be restored (i.e. 
including the modifications).

regarding naming of the class and the test, i would like you to apply the 
following minor changes:
 * the naming of class and method to reflect that you are testing 1 specific 
scenario, where the parent nested-cug property is being removed.
 * please also add a comment linking this test class to this very jira issue.

one minor change: please make sure the formatting is consistent: e.g. 
\{{}finally {}} is lacking a space.

please, adjust the test class accordingly and provide the patch here. thanks.

> Permission evaluation of nodes broken after :nestedCug removed from parent 
> node
> -------------------------------------------------------------------------------
>
>                 Key: OAK-8855
>                 URL: https://issues.apache.org/jira/browse/OAK-8855
>             Project: Jackrabbit Oak
>          Issue Type: Bug
>          Components: authorization-cug
>    Affects Versions: 1.8.7
>            Reporter: Kunal Shubham
>            Assignee: Angela Schreiber
>            Priority: Major
>         Attachments: 0001-Fix-nestedcug-permission-issue.patch, 
> OAK-8855_backport.patch
>
>
> Steps to Reproduce:
>  # Create a node 'a' which has two children nodes 'b1' and 'b2'. The content 
> tree looks as shown: /content/a/b1, /content/a/b2. Create two users user1 and 
> user2.
>  # Apply CUG policy on /content/a.
>  ** Authorize user1 and user2 to read /content/a.
>  ** Authorize user1 to read /content/a/b1.
>  ** Authorize user2 to read /content/a/b2.
>  # Remove :nestedCugs property from /content/a/rep:cugPolicy.
>  # Create a content session, login with user2. Try to read /content/a/b1.
> *Observed behavior* : user2 is able to read /content/a/b1.
> *Expected behavior* : user2 should not be able to read /content/a/b1 as it is 
> unauthorized to do so.
> Please note that :nestedCugs is removed by a mechanism which completely 
> overwrites content tree below "/content/a".



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

Reply via email to