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

JCRVLT-522 check effect of filter rules on ACLs #162

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

kwin
Copy link
Member

@kwin kwin commented Aug 30, 2021

No description provided.

@kwin kwin force-pushed the feature/JCRVLT-522-filter-rules-for-autorizables-acls branch from 75df2a2 to 43f4a19 Compare August 30, 2021 07:25
@kwin
Copy link
Member Author

kwin commented Aug 30, 2021

I fail to reproduce installation of ACLs not contained in the filter. @anchela Can you come up with a test case?

@kwin kwin requested a review from tripodsan August 30, 2021 10:01
@kwin
Copy link
Member Author

kwin commented Aug 30, 2021

@tripodsan In general filter rules seem to be followed except for one edge case. What is the idea of Importer.postFilter (

)? Is that supposed to filter out everything on an artifact level not contained in a filter and the DocViewSAXImporter only does the filtering on sub artifact level?

If so do you consider Importer.postFilter errorneous as it does not correctly filter out artifacts which are below a contained node (but excluded)?

@tripodsan
Copy link
Contributor

@tripodsan In general filter rules seem to be followed except for one edge case. What is the idea of Importer.postFilter (

)? Is that supposed to filter out everything on an artifact level not contained in a filter and the DocViewSAXImporter only does the filtering on sub artifact level?

I think it is used to find the TXInfo hierarchy that is covered by the filter.... maybe :-)

If so do you consider Importer.postFilter errorneous as it does not correctly filter out artifacts which are below a contained node (but excluded)?

I don't think this will lead to errors. AFAIR, this is only the preliminary scan. the nodes are filtered again when traversing. but I might be wrong. it was a long time ago, when this code was created :-)

@kwin
Copy link
Member Author

kwin commented Aug 30, 2021

Look at the test case: It leads to some ACLs being applied although outside the filter, because no subsequent filtering happens. Bug or Feature?

@tripodsan
Copy link
Contributor

tripodsan commented Aug 31, 2021

Look at the test case: It leads to some ACLs being applied although outside the filter, because no subsequent filtering happens. Bug or Feature?

I would say, if the effective ACL on one of the items included in the filter is affected, then the ACL should be applied.
i.e. in general, all ACLs not included in the filter but that are ancestors of the filtered items should be applied.

all ACLs that are not covered in the filter and do not affect the items in the covered tree should not be modified.

@kwin
Copy link
Member Author

kwin commented Aug 31, 2021

all ACLs not included in the filter but that are ancestors of the filtered items should be applied.

I can follow your argument, but this is not how it works right now. I would like to first clarify how to deal with row 2 in the description table of https://issues.apache.org/jira/browse/JCRVLT-522, before thinking about installing ACLs for more cases not contained in the filter.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 1, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@kwin kwin force-pushed the feature/JCRVLT-522-filter-rules-for-autorizables-acls branch from 3b78195 to 656a321 Compare July 5, 2022 17:01
@kwin kwin requested review from reschke and removed request for tripodsan July 6, 2022 08:21
@kwin
Copy link
Member Author

kwin commented Jul 6, 2022

@reschke Do we again need to introduce a flag for backwards-compatibility where ACLs are applied in some edge cases despite not being contained in the filter?

@kwin kwin marked this pull request as ready for review July 6, 2022 08:22
@kwin kwin requested a review from anchela July 6, 2022 08:22
@kwin
Copy link
Member Author

kwin commented May 2, 2023

If there is no further concern I would go ahead and merge at the end of this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants