jackrabbit-oak-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Angela Schreiber <anch...@adobe.com>
Subject Re: svn commit: r1465664 - /jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/core/SecureNodeState.java
Date Tue, 09 Apr 2013 09:45:36 GMT
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) {
>
>

Mime
View raw message