You are viewing a plain text version of this content. The canonical link for it is here.
Posted to oak-commits@jackrabbit.apache.org by ju...@apache.org on 2013/04/08 17:21:38 UTC
svn commit: r1465664 -
/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/core/SecureNodeState.java
Author: jukka
Date: Mon Apr 8 15:21:37 2013
New Revision: 1465664
URL: http://svn.apache.org/r1465664
Log:
OAK-709: Consider moving permission evaluation to the node state level
Calculate the readState directly when a SecureNodeState is instantiated
(may need to make it lazy again later, TBD) so we can add the getChildNode()
optimization where we return a fully readable leaf node state directly
without a SecureNodeState wrapper.
Also streamline the property access methods to avoid duplicate
checks when possible.
Modified:
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/core/SecureNodeState.java
Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/core/SecureNodeState.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/core/SecureNodeState.java?rev=1465664&r1=1465663&r2=1465664&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/core/SecureNodeState.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/core/SecureNodeState.java Mon Apr 8 15:21:37 2013
@@ -21,7 +21,6 @@ import static com.google.common.base.Pre
import java.util.Collections;
import javax.annotation.CheckForNull;
import javax.annotation.Nonnull;
-import javax.annotation.Nullable;
import com.google.common.base.Function;
import com.google.common.base.Predicate;
@@ -60,8 +59,23 @@ public class SecureNodeState extends Abs
private final PermissionProvider permissionProvider;
- private ReadStatus readStatus;
+ private final ReadStatus readStatus;
+
+ /**
+ * Predicate for testing whether a given property is readable.
+ */
+ private final Predicate<PropertyState> isPropertyReadable =
+ new Predicate<PropertyState>() {
+ @Override
+ public boolean apply(@Nonnull PropertyState property) {
+ ReadStatus status =
+ permissionProvider.getReadStatus(base, property);
+ return status.isAllow();
+ }
+ };
+
private long childNodeCount = -1;
+
private long propertyCount = -1;
public SecureNodeState(@Nonnull NodeState rootState,
@@ -70,6 +84,7 @@ public class SecureNodeState extends Abs
this.state = checkNotNull(rootState);
this.base = new ImmutableTree(rootState, typeProvider);
this.permissionProvider = permissionProvider;
+ this.readStatus = permissionProvider.getReadStatus(base, null);
}
private SecureNodeState(
@@ -79,36 +94,41 @@ public class SecureNodeState extends Abs
this.base = new ImmutableTree(parent.base, name, nodeState);
this.permissionProvider = parent.permissionProvider;
if (base.getType() == parent.base.getType()) {
- this.readStatus = (ReadStatus.getChildStatus(parent.readStatus));
+ this.readStatus = ReadStatus.getChildStatus(parent.readStatus);
+ } else {
+ this.readStatus = permissionProvider.getReadStatus(base, null);
}
}
@Override
public boolean exists() {
- return getReadStatus().includes(ReadStatus.ALLOW_THIS);
+ return readStatus.includes(ReadStatus.ALLOW_THIS);
}
@Override @CheckForNull
public PropertyState getProperty(String name) {
PropertyState property = state.getProperty(name);
- if (property != null && canReadProperty(property)) {
- return property;
- } else {
- return null;
+ if (property != null
+ && !readStatus.includes(ReadStatus.ALLOW_PROPERTIES)) {
+ if (!readStatus.appliesToThis()
+ || isPropertyReadable.apply(property)) {
+ property = null;
+ }
}
+ return property;
}
@Override
public synchronized long getPropertyCount() {
if (propertyCount == -1) {
- ReadStatus rs = getReadStatus();
- if (rs.includes(ReadStatus.ALLOW_PROPERTIES)) {
+ if (readStatus.includes(ReadStatus.ALLOW_PROPERTIES)) {
propertyCount = state.getPropertyCount();
- } else if (rs.includes(ReadStatus.DENY_PROPERTIES)) {
+ } else if (readStatus.includes(ReadStatus.DENY_PROPERTIES)
+ || !readStatus.appliesToThis()) {
propertyCount = 0;
} else {
propertyCount = count(Iterables.filter(
- state.getProperties(), isPropertyReadable()));
+ state.getProperties(), isPropertyReadable));
}
}
return propertyCount;
@@ -117,14 +137,14 @@ public class SecureNodeState extends Abs
@Nonnull
@Override
public Iterable<? extends PropertyState> getProperties() {
- ReadStatus rs = getReadStatus();
- if (rs.includes(ReadStatus.ALLOW_PROPERTIES)) {
+ if (readStatus.includes(ReadStatus.ALLOW_PROPERTIES)) {
return state.getProperties();
- } else if (rs.includes(ReadStatus.DENY_PROPERTIES)) {
+ } else if (readStatus.includes(ReadStatus.DENY_PROPERTIES)
+ || !readStatus.appliesToThis()) {
return Collections.emptySet();
} else {
return Iterables.filter(
- state.getProperties(), isPropertyReadable());
+ state.getProperties(), isPropertyReadable);
}
}
@@ -137,11 +157,16 @@ public class SecureNodeState extends Abs
public NodeState getChildNode(@Nonnull String name) {
NodeState child = state.getChildNode(name);
if (child.exists()) {
- return new SecureNodeState(this, name, child);
- } else {
- // a non-existing child node
- return child;
+ SecureNodeState secure = new SecureNodeState(this, name, child);
+ if (!secure.readStatus.includes(ReadStatus.ALLOW_ALL)
+ || child.getChildNodeCount() > 0) {
+ // The SecureNodeState wrapper is needed if the child node
+ // 1) exists, 2) has access restrictions, or 3) contains
+ // descendants whose access control status isn't yet known
+ child = secure;
+ }
}
+ return child;
}
@Override
@@ -153,20 +178,9 @@ public class SecureNodeState extends Abs
}
@Override
- public Iterable<String> getChildNodeNames() {
- return Iterables.transform(getChildNodeEntries(), new Function<ChildNodeEntry, String>() {
- @Override
- public String apply(@Nullable ChildNodeEntry cnEntry) {
- return (cnEntry == null) ? null : cnEntry.getName();
- }
- });
- }
-
- @Override
@Nonnull
public Iterable<? extends ChildNodeEntry> getChildNodeEntries() {
- ReadStatus rs = getReadStatus();
- if (rs.includes(ReadStatus.DENY_CHILDREN)) {
+ if (readStatus.includes(ReadStatus.DENY_CHILDREN)) {
return Collections.emptySet();
} else {
// TODO: review if ALLOW_CHILDREN could be used as well although we
@@ -192,40 +206,6 @@ public class SecureNodeState extends Abs
//--------------------------------------------------------------------------
- private ReadStatus getReadStatus() {
- if (readStatus == null) {
- readStatus = permissionProvider.getReadStatus(base, null);
- }
- return readStatus;
- }
-
- private boolean canReadProperty(@Nonnull PropertyState property) {
- if (readStatus == null || readStatus.appliesToThis()) {
- ReadStatus rs = permissionProvider.getReadStatus(this.base, property);
- if (rs.appliesToThis()) {
- // status applies to this property only -> recalc for others
- return rs.isAllow();
- } else {
- readStatus = rs;
- }
- }
- return readStatus.includes(ReadStatus.ALLOW_PROPERTIES);
- }
-
- /**
- * Returns a predicate for testing whether a given property is readable.
- *
- * @return predicate
- */
- private Predicate<PropertyState> isPropertyReadable() {
- return new Predicate<PropertyState>() {
- @Override
- public boolean apply(@Nonnull PropertyState property) {
- return canReadProperty(property);
- }
- };
- }
-
private class ReadableChildNodeEntries implements Function<ChildNodeEntry, ChildNodeEntry> {
@Override
public ChildNodeEntry apply(ChildNodeEntry input) {
Re: svn commit: r1465664 - /jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/core/SecureNodeState.java
Posted by Angela Schreiber <an...@adobe.com>.
> Done in http://svn.apache.org/r1466424.
good... i started to modify the ReadStatus such that ALLOW_ALL
would really mean can-read-all-including-ac-except-for-those-hidden
items. not sure if that works out, but i will give it a try.
and subsequently touch the securitynodestate again, such that the
tree-type will be respected when dealing with
- getProperty
- getProperties
- getPropertyCount
- getChildNode
- getChildNodeCount
- getChildNodeEntries
it will be ugly but we may get a better idea on what we actually
need in terms of readstatus and the different types of items
(hidden, access control content, ...).
angela
Re: svn commit: r1465664 - /jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/core/SecureNodeState.java
Posted by Jukka Zitting <ju...@gmail.com>.
Hi,
On Wed, Apr 10, 2013 at 1:04 PM, Angela Schreiber <an...@adobe.com> wrote:
> sure... that's correct. but the original code didn't apply this
> for leaf-only nodes.
It did, but only after first evaluating the read status, which, as you
correctly pointed out, shouldn't be needed for ancestor nodes.
> if you want to add this optimization, just
> go ahead; READ_THIS_PROPERTIES was in this case sufficient.
Done in http://svn.apache.org/r1466424.
BR,
Jukka Zitting
Re: svn commit: r1465664 - /jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/core/SecureNodeState.java
Posted by Angela Schreiber <an...@adobe.com>.
hi jukka
sure... that's correct. but the original code didn't apply this
for leaf-only nodes. if you want to add this optimization, just
go ahead; READ_THIS_PROPERTIES was in this case sufficient.
regards
angela
On 4/10/13 11:43 AM, Jukka Zitting wrote:
> Hi,
>
> On Wed, Apr 10, 2013 at 12:30 PM, Angela Schreiber<an...@adobe.com> wrote:
>> my point is that permission evaluation should only occur if the
>> corresponding tree or property is really being read.
>
> Pre-evaluating the permissions for a node with no children should be
> fine from that perspective, as it's highly likely that the client is
> going to read that node instead of trying to traverse the tree
> further.
>
>> as i stated before this currently doesn't work with the way the
>> readstatus is being calculated... it would need to check
>> for 'read + read-access-control' in order to be really sure that
>> ALL items including access control content can be read.
>
> Note that by definition a node with no children can have no access
> control content below it. Thus the READ_ALL (or just
> READ_THIS_PROPERTIES) check should be everything that's needed to
> guarantee full access to that node.
>
> BR,
>
> Jukka Zitting
Re: svn commit: r1465664 - /jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/core/SecureNodeState.java
Posted by Jukka Zitting <ju...@gmail.com>.
Hi,
On Wed, Apr 10, 2013 at 12:30 PM, Angela Schreiber <an...@adobe.com> wrote:
> my point is that permission evaluation should only occur if the
> corresponding tree or property is really being read.
Pre-evaluating the permissions for a node with no children should be
fine from that perspective, as it's highly likely that the client is
going to read that node instead of trying to traverse the tree
further.
> as i stated before this currently doesn't work with the way the
> readstatus is being calculated... it would need to check
> for 'read + read-access-control' in order to be really sure that
> ALL items including access control content can be read.
Note that by definition a node with no children can have no access
control content below it. Thus the READ_ALL (or just
READ_THIS_PROPERTIES) check should be everything that's needed to
guarantee full access to that node.
BR,
Jukka Zitting
Re: svn commit: r1465664 - /jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/core/SecureNodeState.java
Posted by Angela Schreiber <an...@adobe.com>.
hi jukka
On 4/10/13 8:26 AM, Jukka Zitting wrote:
> Hi,
>
> On Tue, Apr 9, 2013 at 5:49 PM, Angela Schreiber<an...@adobe.com> wrote:
>>> On Tue, Apr 9, 2013 at 12:45 PM, Angela Schreiber<an...@adobe.com>
>>> How about we change the order of the checks, so that the readStatus is
>>> pre-loaded only when child.getChildNodeCount() == 0? That way we could
>>> avoid unnecessarily evaluating permissions at ancestor nodes, but
>>> still avoid the cost of the SecureNodeState wrapper for most leaf
>>> nodes.
>>
>> sure, we can try that... in general i would prefer to get a better
>> picture on the overall impact first before starting to optimize
>> things and make the code complicated.
>
> The way I see it, the ability to avoid the SecureNodeState wrapper in
> cases like these is an essential feature of this design. Otherwise we
> could just as well keep the access checks in TreeImpl and avoid the
> extra layer of indirection.
ok... i didn't see it that way.
my point is that permission evaluation should only occur if the
corresponding tree or property is really being read. that's what
i keep saying about having random access to repository which IMO
must be really fast without having to evaluate access to all the
ancestors.
> Thus I'd really want to include this
> aspect in the SecureNodeState code right from the beginning.
>
> Here's my proposed change to wrapChildNodeEntry.apply():
>
> SecureNodeState secureChild =
> new SecureNodeState(SecureNodeState.this, name, child);
> if (child.getChildNodeCount() == 0
> && secureChild.getReadStatus().includes(ALLOW_ALL)) {
> return input;
> } else {
> return new MemoryChildNodeEntry(name, secureChild);
> }
as i stated before this currently doesn't work with the way the
readstatus is being calculated... it would need to check
for 'read + read-access-control' in order to be really sure that
ALL items including access control content can be read.
however, i most cases users will not be allowed to read ac content
but just regular content, in which case the ac-content needs to
be filtered out again. the biggest issue: we don't know if there
is any ac-content down in the hierarchy.
don't get me wrong: i am not against having the optimization in
the SecureNodeState but the permission content must first be adjusted
in order to allow for such an optimization... notably the read-status.
>> anyway. the point here is that i would prefer to get it to work
>> first and optimize once we know that it really works.
>
> Based on my above point of view, I wouldn't consider SecureNodeState
> to be working as designed if at least some getChildNode() calls didn't
> return the raw underlying NodeState instance. :-)
i can accept that... but as long as it doesn't work there IMO is no
need in having that extra complexity... that's why i replaced it
with a TODO comment.
to me the whole thing is still a prove of concept. there are too
many very fundamental things broken now (in particular that we can no
longer access Trees and Properties if their parent is not accessible)
or odd (the nodestate keeping an tree internally) that i am fully
convinced that moving it down to the node state level really will work.
angela
> BR,
>
> Jukka Zitting
Re: svn commit: r1465664 - /jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/core/SecureNodeState.java
Posted by Jukka Zitting <ju...@gmail.com>.
Hi,
On Tue, Apr 9, 2013 at 5:49 PM, Angela Schreiber <an...@adobe.com> wrote:
>> On Tue, Apr 9, 2013 at 12:45 PM, Angela Schreiber<an...@adobe.com>
>> How about we change the order of the checks, so that the readStatus is
>> pre-loaded only when child.getChildNodeCount() == 0? That way we could
>> avoid unnecessarily evaluating permissions at ancestor nodes, but
>> still avoid the cost of the SecureNodeState wrapper for most leaf
>> nodes.
>
> sure, we can try that... in general i would prefer to get a better
> picture on the overall impact first before starting to optimize
> things and make the code complicated.
The way I see it, the ability to avoid the SecureNodeState wrapper in
cases like these is an essential feature of this design. Otherwise we
could just as well keep the access checks in TreeImpl and avoid the
extra layer of indirection. Thus I'd really want to include this
aspect in the SecureNodeState code right from the beginning.
Here's my proposed change to wrapChildNodeEntry.apply():
SecureNodeState secureChild =
new SecureNodeState(SecureNodeState.this, name, child);
if (child.getChildNodeCount() == 0
&& secureChild.getReadStatus().includes(ALLOW_ALL)) {
return input;
} else {
return new MemoryChildNodeEntry(name, secureChild);
}
> anyway. the point here is that i would prefer to get it to work
> first and optimize once we know that it really works.
Based on my above point of view, I wouldn't consider SecureNodeState
to be working as designed if at least some getChildNode() calls didn't
return the raw underlying NodeState instance. :-)
BR,
Jukka Zitting
Re: svn commit: r1465664 - /jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/core/SecureNodeState.java
Posted by Angela Schreiber <an...@adobe.com>.
hi jukka
> On Tue, Apr 9, 2013 at 12:45 PM, Angela Schreiber<an...@adobe.com> wrote:
>> i am not convinced this is a wise move. as i keep stating we used
>> to have short lived sessions that rather perform random access to the
>> repository instead of reading the complete ancestor tree. calculating
>> the readstatus for all ancestors may therefore not be required. since
>> currently the method is still an open TODO, i would prefer to retrieve
>> the reastatus on demand.
>
> How about we change the order of the checks, so that the readStatus is
> pre-loaded only when child.getChildNodeCount() == 0? That way we could
> avoid unnecessarily evaluating permissions at ancestor nodes, but
> still avoid the cost of the SecureNodeState wrapper for most leaf
> nodes.
sure, we can try that... in general i would prefer to get a better
picture on the overall impact first before starting to optimize
things and make the code complicated.
for example: moving down the evaluation on the NodeState level
now has an impact on how we deal with the TreeLocation [0]: it's
no longer possible to access an Tree that has a inaccessible
parent node, which basically was the whole story about the TreeLocation!
i am not sure if this is solely related to the security move or
whether the most recent changes to the TreeLocation API is related
as well...
anyway. the point here is that i would prefer to get it to work
first and optimize once we know that it really works.
kind regards
angela
[0] https://issues.apache.org/jira/browse/OAK-766
>> thanks... i will take over and add fixes where needed.
>
> OK, cool.
>
> BR,
>
> Jukka Zitting
Re: svn commit: r1465664 - /jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/core/SecureNodeState.java
Posted by Jukka Zitting <ju...@gmail.com>.
Hi,
On Tue, Apr 9, 2013 at 12:45 PM, Angela Schreiber <an...@adobe.com> wrote:
> i am not convinced this is a wise move. as i keep stating we used
> to have short lived sessions that rather perform random access to the
> repository instead of reading the complete ancestor tree. calculating
> the readstatus for all ancestors may therefore not be required. since
> currently the method is still an open TODO, i would prefer to retrieve
> the reastatus on demand.
How about we change the order of the checks, so that the readStatus is
pre-loaded only when child.getChildNodeCount() == 0? That way we could
avoid unnecessarily evaluating permissions at ancestor nodes, but
still avoid the cost of the SecureNodeState wrapper for most leaf
nodes.
> thanks... i will take over and add fixes where needed.
OK, cool.
BR,
Jukka Zitting
Re: svn commit: r1465664 - /jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/core/SecureNodeState.java
Posted by Angela Schreiber <an...@adobe.com>.
hi jukka
> Calculate the readState directly when a SecureNodeState is instantiated
> (may need to make it lazy again later, TBD) so we can add the getChildNode()
> optimization where we return a fully readable leaf node state directly
> without a SecureNodeState wrapper.
i am not convinced this is a wise move. as i keep stating we used
to have short lived sessions that rather perform random access to the
repository instead of reading the complete ancestor tree. calculating
the readstatus for all ancestors may therefore not be required. since
currently the method is still an open TODO, i would prefer to retrieve
the reastatus on demand.
once we have the real permission eval in place we can make optimizations
that take all the bits and pieces into account.
> Also streamline the property access methods to avoid duplicate
> checks when possible.
thanks... i will take over and add fixes where needed.
regards
angela
> Modified:
> jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/core/SecureNodeState.java
>
> Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/core/SecureNodeState.java
> URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/core/SecureNodeState.java?rev=1465664&r1=1465663&r2=1465664&view=diff
> ==============================================================================
> --- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/core/SecureNodeState.java (original)
> +++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/core/SecureNodeState.java Mon Apr 8 15:21:37 2013
> @@ -21,7 +21,6 @@ import static com.google.common.base.Pre
> import java.util.Collections;
> import javax.annotation.CheckForNull;
> import javax.annotation.Nonnull;
> -import javax.annotation.Nullable;
>
> import com.google.common.base.Function;
> import com.google.common.base.Predicate;
> @@ -60,8 +59,23 @@ public class SecureNodeState extends Abs
>
> private final PermissionProvider permissionProvider;
>
> - private ReadStatus readStatus;
> + private final ReadStatus readStatus;
> +
> + /**
> + * Predicate for testing whether a given property is readable.
> + */
> + private final Predicate<PropertyState> isPropertyReadable =
> + new Predicate<PropertyState>() {
> + @Override
> + public boolean apply(@Nonnull PropertyState property) {
> + ReadStatus status =
> + permissionProvider.getReadStatus(base, property);
> + return status.isAllow();
> + }
> + };
> +
> private long childNodeCount = -1;
> +
> private long propertyCount = -1;
>
> public SecureNodeState(@Nonnull NodeState rootState,
> @@ -70,6 +84,7 @@ public class SecureNodeState extends Abs
> this.state = checkNotNull(rootState);
> this.base = new ImmutableTree(rootState, typeProvider);
> this.permissionProvider = permissionProvider;
> + this.readStatus = permissionProvider.getReadStatus(base, null);
> }
>
> private SecureNodeState(
> @@ -79,36 +94,41 @@ public class SecureNodeState extends Abs
> this.base = new ImmutableTree(parent.base, name, nodeState);
> this.permissionProvider = parent.permissionProvider;
> if (base.getType() == parent.base.getType()) {
> - this.readStatus = (ReadStatus.getChildStatus(parent.readStatus));
> + this.readStatus = ReadStatus.getChildStatus(parent.readStatus);
> + } else {
> + this.readStatus = permissionProvider.getReadStatus(base, null);
> }
> }
>
> @Override
> public boolean exists() {
> - return getReadStatus().includes(ReadStatus.ALLOW_THIS);
> + return readStatus.includes(ReadStatus.ALLOW_THIS);
> }
>
> @Override @CheckForNull
> public PropertyState getProperty(String name) {
> PropertyState property = state.getProperty(name);
> - if (property != null&& canReadProperty(property)) {
> - return property;
> - } else {
> - return null;
> + if (property != null
> +&& !readStatus.includes(ReadStatus.ALLOW_PROPERTIES)) {
> + if (!readStatus.appliesToThis()
> + || isPropertyReadable.apply(property)) {
> + property = null;
> + }
> }
> + return property;
> }
>
> @Override
> public synchronized long getPropertyCount() {
> if (propertyCount == -1) {
> - ReadStatus rs = getReadStatus();
> - if (rs.includes(ReadStatus.ALLOW_PROPERTIES)) {
> + if (readStatus.includes(ReadStatus.ALLOW_PROPERTIES)) {
> propertyCount = state.getPropertyCount();
> - } else if (rs.includes(ReadStatus.DENY_PROPERTIES)) {
> + } else if (readStatus.includes(ReadStatus.DENY_PROPERTIES)
> + || !readStatus.appliesToThis()) {
> propertyCount = 0;
> } else {
> propertyCount = count(Iterables.filter(
> - state.getProperties(), isPropertyReadable()));
> + state.getProperties(), isPropertyReadable));
> }
> }
> return propertyCount;
> @@ -117,14 +137,14 @@ public class SecureNodeState extends Abs
> @Nonnull
> @Override
> public Iterable<? extends PropertyState> getProperties() {
> - ReadStatus rs = getReadStatus();
> - if (rs.includes(ReadStatus.ALLOW_PROPERTIES)) {
> + if (readStatus.includes(ReadStatus.ALLOW_PROPERTIES)) {
> return state.getProperties();
> - } else if (rs.includes(ReadStatus.DENY_PROPERTIES)) {
> + } else if (readStatus.includes(ReadStatus.DENY_PROPERTIES)
> + || !readStatus.appliesToThis()) {
> return Collections.emptySet();
> } else {
> return Iterables.filter(
> - state.getProperties(), isPropertyReadable());
> + state.getProperties(), isPropertyReadable);
> }
> }
>
> @@ -137,11 +157,16 @@ public class SecureNodeState extends Abs
> public NodeState getChildNode(@Nonnull String name) {
> NodeState child = state.getChildNode(name);
> if (child.exists()) {
> - return new SecureNodeState(this, name, child);
> - } else {
> - // a non-existing child node
> - return child;
> + SecureNodeState secure = new SecureNodeState(this, name, child);
> + if (!secure.readStatus.includes(ReadStatus.ALLOW_ALL)
> + || child.getChildNodeCount()> 0) {
> + // The SecureNodeState wrapper is needed if the child node
> + // 1) exists, 2) has access restrictions, or 3) contains
> + // descendants whose access control status isn't yet known
> + child = secure;
> + }
> }
> + return child;
> }
>
> @Override
> @@ -153,20 +178,9 @@ public class SecureNodeState extends Abs
> }
>
> @Override
> - public Iterable<String> getChildNodeNames() {
> - return Iterables.transform(getChildNodeEntries(), new Function<ChildNodeEntry, String>() {
> - @Override
> - public String apply(@Nullable ChildNodeEntry cnEntry) {
> - return (cnEntry == null) ? null : cnEntry.getName();
> - }
> - });
> - }
> -
> - @Override
> @Nonnull
> public Iterable<? extends ChildNodeEntry> getChildNodeEntries() {
> - ReadStatus rs = getReadStatus();
> - if (rs.includes(ReadStatus.DENY_CHILDREN)) {
> + if (readStatus.includes(ReadStatus.DENY_CHILDREN)) {
> return Collections.emptySet();
> } else {
> // TODO: review if ALLOW_CHILDREN could be used as well although we
> @@ -192,40 +206,6 @@ public class SecureNodeState extends Abs
>
> //--------------------------------------------------------------------------
>
> - private ReadStatus getReadStatus() {
> - if (readStatus == null) {
> - readStatus = permissionProvider.getReadStatus(base, null);
> - }
> - return readStatus;
> - }
> -
> - private boolean canReadProperty(@Nonnull PropertyState property) {
> - if (readStatus == null || readStatus.appliesToThis()) {
> - ReadStatus rs = permissionProvider.getReadStatus(this.base, property);
> - if (rs.appliesToThis()) {
> - // status applies to this property only -> recalc for others
> - return rs.isAllow();
> - } else {
> - readStatus = rs;
> - }
> - }
> - return readStatus.includes(ReadStatus.ALLOW_PROPERTIES);
> - }
> -
> - /**
> - * Returns a predicate for testing whether a given property is readable.
> - *
> - * @return predicate
> - */
> - private Predicate<PropertyState> isPropertyReadable() {
> - return new Predicate<PropertyState>() {
> - @Override
> - public boolean apply(@Nonnull PropertyState property) {
> - return canReadProperty(property);
> - }
> - };
> - }
> -
> private class ReadableChildNodeEntries implements Function<ChildNodeEntry, ChildNodeEntry> {
> @Override
> public ChildNodeEntry apply(ChildNodeEntry input) {
>
>