Propagation of Session auto refresh behaviour to e.g. UserManager

[Michael Dürig <md...@apache.org>]

Hi,

With OAK-938 [1] Antonio uncovered another consequence of introducing 
auto refreshing sessions [2]:

Acquiring a UserManager from a auto refreshing session, that UserManager 
instance would not "inherit" the auto refresh setting. That is, although 
the session itself might after a while see a newer revision due to an 
automatic refresh, the UserManager will still stay on the revision the 
session had when it was acquired. See 
https://github.com/apache/jackrabbit-oak/blob/trunk/oak-jcr/src/test/java/org/apache/jackrabbit/oak/jcr/security/user/UserManagerTest.java#L89 
for a test case.

We might have similar issues with other entities tied to a session like 
PrincipalManager, VersionManager, ... Basically all (indirect) callers 
of SessionDelegate#getRoot() are suspect... and that's quite a list.

To me the cleanest solution seems to be changing 
SessionDelegate#getRoot() to SessionDelegate#getRootProvider() thus 
signalling to clients they need to acquire the root on an as need basis.

WDYT?

Michael


[1] https://issues.apache.org/jira/browse/OAK-938
[2] https://issues.apache.org/jira/browse/OAK-803

Re: Propagation of Session auto refresh behaviour to e.g. UserManager

[Jukka Zitting <ju...@gmail.com>]

Hi,

On Tue, Aug 6, 2013 at 12:51 PM, Angela Schreiber <an...@adobe.com> wrote:
> regarding attaching to the SessionDelegate: i definitely disagree.
> in contrast to the various plugins which are not really pluggable
> the various mgr security related implementations must be
> pluggable at runtime. therefore making the implementation depend
> on the internal SessionDelegate is not an option.
> in addition some of those classes are also used within the oak core
> (validators, commit hooks and so forth). i really don't want to
> add any dependency to oak-jcr nor the internals contained therein
> to the various security related parts. i deliberately avoided it.
> this wasn't a coincidence :-)

Fair enough.

My point here is that SessionDelegate.perform() would solve the
OAK-938 problem, and coming up with another solution seems like
unnecessary duplication to me. But I won't stand in the way if people
think that's a better approach.

BR,

Jukka Zitting

Re: Propagation of Session auto refresh behaviour to e.g. UserManager

[Angela Schreiber <an...@adobe.com>]

hi jukka

we are talking about 2 different sessions. with one single session
everything works as expected.

regarding attaching to the SessionDelegate: i definitely disagree.
in contrast to the various plugins which are not really pluggable
the various mgr security related implementations must be
pluggable at runtime. therefore making the implementation depend
on the internal SessionDelegate is not an option.
in addition some of those classes are also used within the oak core
(validators, commit hooks and so forth). i really don't want to
add any dependency to oak-jcr nor the internals contained therein
to the various security related parts. i deliberately avoided it.
this wasn't a coincidence :-)

angela


On 8/6/13 11:42 AM, "Jukka Zitting" <ju...@gmail.com> wrote:

>Hi,
>
>On Tue, Aug 6, 2013 at 12:23 PM, Angela Schreiber <an...@adobe.com>
>wrote:
>> but in this case as michael explained the setup is that one session
>> should be informed about modifications made by another session.
>
>Are we talking about two separate sessions (i.e. javax.jcr.Session
>instances) or a session and the UserManager associated with it?
>
>> this works for all operations that are 'attached' to the SessionDelegate
>> but the refresh doesn't work for those classes that are just associated
>> with the Root (such as e.g. the UserManager which btw. makes transient
>> modifications on the root associated with editing session. the
>>read-write
>> trick doesn't help here).
>
>It sounds to me as if the UserManager should also be attached to the
>SessionDelegate, and use the SessionOperation mechanism whenever
>accessing information bound to that session.
>
>BR,
>
>Jukka Zitting

Re: Propagation of Session auto refresh behaviour to e.g. UserManager

[Jukka Zitting <ju...@gmail.com>]

Hi,

On Tue, Aug 6, 2013 at 12:23 PM, Angela Schreiber <an...@adobe.com> wrote:
> but in this case as michael explained the setup is that one session
> should be informed about modifications made by another session.

Are we talking about two separate sessions (i.e. javax.jcr.Session
instances) or a session and the UserManager associated with it?

> this works for all operations that are 'attached' to the SessionDelegate
> but the refresh doesn't work for those classes that are just associated
> with the Root (such as e.g. the UserManager which btw. makes transient
> modifications on the root associated with editing session. the read-write
> trick doesn't help here).

It sounds to me as if the UserManager should also be attached to the
SessionDelegate, and use the SessionOperation mechanism whenever
accessing information bound to that session.

BR,

Jukka Zitting

Re: Propagation of Session auto refresh behaviour to e.g. UserManager

[Jukka Zitting <ju...@gmail.com>]

Hi,

On Tue, Aug 6, 2013 at 12:36 PM, Michael Dürig <md...@apache.org> wrote:
> There is no perform call in this case. That is the point of the whole story.

The way I see it, there *should* be a perform() call in this case.

BR,

Jukka Zitting

Re: Propagation of Session auto refresh behaviour to e.g. UserManager

[Michael Dürig <md...@apache.org>]

On 6.8.13 11:33, Jukka Zitting wrote:
> Hi,
>
> On Tue, Aug 6, 2013 at 12:21 PM, Michael Dürig <md...@apache.org> wrote:
>> On 6.8.13 10:41, Jukka Zitting wrote:
>>> a) When making transient changes or reading information that can come
>>> from an earlier repository snapshot, use sessionDelegate.getRoot() so
>>> that you see the exact same state as the rest of the session.
>>
>> This is not sufficient for the current case however unless we add a session
>> refresh for every call to SessionDelegate.getRoot().
>
> The refresh (when needed) should happen already in
> SessionDelegate.perform(), before the getRoot() call is made.
>

There is no perform call in this case. That is the point of the whole story.

Michael


>> Furthermore the underlying (somewhat implicit) assumption back then was
>> sessions being stable (i.e. no auto refresh) such that the returned root and
>> trees retrieved from it can be cached. This is now not the case any more and
>> might render many such optimisations invalid.
>
> This is exactly the problem I was worried about when I tried to argue
> earlier that we shouldn't be caching information in memory and instead
> read any information directly from the underlying content tree when
> needed.
>
> BR,
>
> Jukka Zitting
>

Re: Propagation of Session auto refresh behaviour to e.g. UserManager

[Jukka Zitting <ju...@gmail.com>]

Hi,

On Tue, Aug 6, 2013 at 12:21 PM, Michael Dürig <md...@apache.org> wrote:
> On 6.8.13 10:41, Jukka Zitting wrote:
>> a) When making transient changes or reading information that can come
>> from an earlier repository snapshot, use sessionDelegate.getRoot() so
>> that you see the exact same state as the rest of the session.
>
> This is not sufficient for the current case however unless we add a session
> refresh for every call to SessionDelegate.getRoot().

The refresh (when needed) should happen already in
SessionDelegate.perform(), before the getRoot() call is made.

> Furthermore the underlying (somewhat implicit) assumption back then was
> sessions being stable (i.e. no auto refresh) such that the returned root and
> trees retrieved from it can be cached. This is now not the case any more and
> might render many such optimisations invalid.

This is exactly the problem I was worried about when I tried to argue
earlier that we shouldn't be caching information in memory and instead
read any information directly from the underlying content tree when
needed.

BR,

Jukka Zitting

Re: Propagation of Session auto refresh behaviour to e.g. UserManager

[Michael Dürig <md...@apache.org>]

On 6.8.13 10:41, Jukka Zitting wrote:
> Hi,
>
> On Tue, Aug 6, 2013 at 11:24 AM, Michael Dürig <md...@apache.org> wrote:
>> We might have similar issues with other entities tied to a session like
>> PrincipalManager, VersionManager, ... Basically all (indirect) callers of
>> SessionDelegate#getRoot() are suspect... and that's quite a list.
>
> We sorted out a good pattern for doing stuff like this already with
> the namespace registry. Basically:
>
> a) When making transient changes or reading information that can come
> from an earlier repository snapshot, use sessionDelegate.getRoot() so
> that you see the exact same state as the rest of the session.

This is not sufficient for the current case however unless we add a 
session refresh for every call to SessionDelegate.getRoot().

Furthermore the underlying (somewhat implicit) assumption back then was 
sessions being stable (i.e. no auto refresh) such that the returned root 
and trees retrieved from it can be cached. This is now not the case any 
more and might render many such optimisations invalid.

Michael

>
> b) When persisting changes directly to the repository or reading from
> the latest repository state without interference from transient
> changes, use sessionDelegate.getContentSession().getLatestRoot() and
> follow up with a session.refresh(true) to force the rest of the
> session to keep up.
>
> The abstract method pattern in ReadWriteNamespaceRegistry (or
> something similar) can be used to avoid a direct oak-jcr /
> SessionDelegate dependency.
>
> That pattern should cover the needs of the UserManager and other
> places without the need to manage the states of multiple independent
> Root instances.
>
> BR,
>
> Jukka Zitting
>

Re: Propagation of Session auto refresh behaviour to e.g. UserManager

[Jukka Zitting <ju...@gmail.com>]

Hi,

On Tue, Aug 6, 2013 at 5:26 PM, Michael Dürig <md...@apache.org> wrote:
> I'll start looking into implementing those extra Delegate classes on an as
> need basis.

Great, thanks!

Perhaps it's worth it to try out the briefly discussed idea of
implementing at least some of the required wrappers using reflection
or static code generation, perhaps driven by annotations. If it works
out well, it might be possible to extend such a approach into an
elegant solution to OAK-662.

BR,

Jukka Zitting

Re: Propagation of Session auto refresh behaviour to e.g. UserManager

[Michael Dürig <md...@apache.org>]

On 6.8.13 11:23, Angela Schreiber wrote:
> let's discuss that in the call this afternoon.

Quick summary from that call:

- We need some way of backwards compatibility re. sessions as discussed 
in OAK-803. Otherwise client code could break in very subtle ways 
creating a support legacy.

- Pushing the refresh logic down to Root and related is not a good 
option since we want to keep oak-core strictly MVCC by keeping the 
backwards compatibility tweaks in oak-jcr.

- Pulling the refresh logic up into a separate JCR wrapper would factor 
the solution into a separate component. But it would introduce extra 
complexity, a lot of new classes and duplicate much of the logic we have 
in the current XyzDelegate classes.

- Introducing Delegate implementations for the problematic entities 
(i.e. UserManger) was deemed the solution to go for now. The other 
suspects that came up in a quick usage search are: PrincipalManager, 
VersionManager, ACManager, PermissionProvider, ObservationManager,  and 
PrivilegeManager.

I'll start looking into implementing those extra Delegate classes on an 
as need basis.

Michael

Re: Propagation of Session auto refresh behaviour to e.g. UserManager

[Angela Schreiber <an...@adobe.com>]

hi jukka

IMO the approach in the namespaceregistry is something different:
in that case you have a different write-root that needs to make
sure that the root (and the editing session) gets updated once
the changes are persisted.

but in this case as michael explained the setup is that one session
should be informed about modifications made by another session.
this works for all operations that are 'attached' to the SessionDelegate
but the refresh doesn't work for those classes that are just associated
with the Root (such as e.g. the UserManager which btw. makes transient
modifications on the root associated with editing session. the read-write
trick doesn't help here). btw: it's one of the fundamental design decisions
for the oak security code to build the implentations on top of the OAK API.
this contrasts to the Jackrabbit setup which lead to a lot of hacks and
at the end didn't work very well.

so, what we would need is a refresh on the Root instead of just refreshing
upon session access. i just discussed this with Michael and we both fear
that
this will open a huge can of worms...

what we are basically trying to do is to changing one of the fundamental
concepts of OAK (refresh only occurs if manually triggered), which IMHO
needs a careful analysis of the consequences.

let's discuss that in the call this afternoon.
angela


On 8/6/13 10:41 AM, "Jukka Zitting" <ju...@gmail.com> wrote:

>Hi,
>
>On Tue, Aug 6, 2013 at 11:24 AM, Michael Dürig <md...@apache.org> wrote:
>> We might have similar issues with other entities tied to a session like
>> PrincipalManager, VersionManager, ... Basically all (indirect) callers
>>of
>> SessionDelegate#getRoot() are suspect... and that's quite a list.
>
>We sorted out a good pattern for doing stuff like this already with
>the namespace registry. Basically:
>
>a) When making transient changes or reading information that can come
>from an earlier repository snapshot, use sessionDelegate.getRoot() so
>that you see the exact same state as the rest of the session.
>
>b) When persisting changes directly to the repository or reading from
>the latest repository state without interference from transient
>changes, use sessionDelegate.getContentSession().getLatestRoot() and
>follow up with a session.refresh(true) to force the rest of the
>session to keep up.
>
>The abstract method pattern in ReadWriteNamespaceRegistry (or
>something similar) can be used to avoid a direct oak-jcr /
>SessionDelegate dependency.
>
>That pattern should cover the needs of the UserManager and other
>places without the need to manage the states of multiple independent
>Root instances.
>
>BR,
>
>Jukka Zitting

Re: Propagation of Session auto refresh behaviour to e.g. UserManager

[Jukka Zitting <ju...@gmail.com>]

Hi,

On Tue, Aug 6, 2013 at 11:24 AM, Michael Dürig <md...@apache.org> wrote:
> We might have similar issues with other entities tied to a session like
> PrincipalManager, VersionManager, ... Basically all (indirect) callers of
> SessionDelegate#getRoot() are suspect... and that's quite a list.

We sorted out a good pattern for doing stuff like this already with
the namespace registry. Basically:

a) When making transient changes or reading information that can come
from an earlier repository snapshot, use sessionDelegate.getRoot() so
that you see the exact same state as the rest of the session.

b) When persisting changes directly to the repository or reading from
the latest repository state without interference from transient
changes, use sessionDelegate.getContentSession().getLatestRoot() and
follow up with a session.refresh(true) to force the rest of the
session to keep up.

The abstract method pattern in ReadWriteNamespaceRegistry (or
something similar) can be used to avoid a direct oak-jcr /
SessionDelegate dependency.

That pattern should cover the needs of the UserManager and other
places without the need to manage the states of multiple independent
Root instances.

BR,

Jukka Zitting

Re: Propagation of Session auto refresh behaviour to e.g. UserManager

[Angela Schreiber <an...@adobe.com>]

hi michael

either we introduce the rootprovider or we create some sort of
refresh-listener behavior.

regards
angela

On 8/6/13 10:24 AM, "Michael Dürig" <md...@apache.org> wrote:

>
>Hi,
>
>With OAK-938 [1] Antonio uncovered another consequence of introducing
>auto refreshing sessions [2]:
>
>Acquiring a UserManager from a auto refreshing session, that UserManager
>instance would not "inherit" the auto refresh setting. That is, although
>the session itself might after a while see a newer revision due to an
>automatic refresh, the UserManager will still stay on the revision the
>session had when it was acquired. See
>https://github.com/apache/jackrabbit-oak/blob/trunk/oak-jcr/src/test/java/
>org/apache/jackrabbit/oak/jcr/security/user/UserManagerTest.java#L89
>for a test case.
>
>We might have similar issues with other entities tied to a session like
>PrincipalManager, VersionManager, ... Basically all (indirect) callers
>of SessionDelegate#getRoot() are suspect... and that's quite a list.
>
>To me the cleanest solution seems to be changing
>SessionDelegate#getRoot() to SessionDelegate#getRootProvider() thus
>signalling to clients they need to acquire the root on an as need basis.
>
>WDYT?
>
>Michael
>
>
>[1] https://issues.apache.org/jira/browse/OAK-938
>[2] https://issues.apache.org/jira/browse/OAK-803