[jira] [Commented] (SLING-3524) ResourceResolver.clone(null) should not share the same JCR session
[
https://issues.apache.org/jira/browse/SLING-3524?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16461478#comment-16461478
]
Csaba Varga commented on SLING-3524:
Pushed a new test case that will exercise the JCR login and logout
functionality in all the eight combinations we were talking about. I hope I
could express all API guarantees as tests, so they won't get broken again in
the future.
> ResourceResolver.clone(null) should not share the same JCR session
> --
>
> Key: SLING-3524
> URL: https://issues.apache.org/jira/browse/SLING-3524
> Project: Sling
> Issue Type: Improvement
> Components: API, JCR, ResourceResolver
>Affects Versions: Resource Resolver 1.0.6
>Reporter: Alexander Klimetschek
>Priority: Major
> Fix For: API 2.18.2, JCR Resource 3.0.10, Resource Resolver 1.6.2
>
> Time Spent: 1h
> Remaining Estimate: 0h
>
> {{ResourceResolver.clone()}} will reuse the same JCR session in case it was
> created by passing an existing session using
> {{JcrResourceConstants.AUTHENTICATION_INFO_SESSION}}. If you need a clone of
> the resource resolver to pass into a new, separate thread, and use
> {{ResourceResolver.clone(null)}}, you will actually share the session, but
> this is not obvious. The problem is that a JCR session cannot be shared
> across threads.
> The javadocs of clone() say "the same credential data is used as was used to
> create this instance".
> There are a few problems with this:
> - seeing the session object itself as "credential data" is unintuitive
> - in my code, I have no idea what the original credential data was, so I
> don't know what kind of credential data it was to make the right decision
> - since sharing a JCR session is to be avoided at all times, the resource
> resolver should prevent one from this
> A solution would be if a plain {{ResourceResolver.clone(null)}} would return
> a session that impersonated itself, abstracting this from the resource
> resolver user. Additionally, it might be worth looking that clone always
> returns a new session, unless specifically stated.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (SLING-3524) ResourceResolver.clone(null) should not share the same JCR session
[
https://issues.apache.org/jira/browse/SLING-3524?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16461281#comment-16461281
]
Csaba Varga commented on SLING-3524:
I've added a [new
commit|https://github.com/csaboka/sling-org-apache-sling-resourceresolver/commit/0f444aa5c6537d480badcf4ca7b2af557f09d48a]
to clear the CLONE key from the authentication info before passing it to the
resource resolver. I hope I've caught all the paths from external clients to
the ResourceResolverImpl constructor. The upside is that now the code has a
central place where new "forbidden properties" can be added as necessary.
> ResourceResolver.clone(null) should not share the same JCR session
> --
>
> Key: SLING-3524
> URL: https://issues.apache.org/jira/browse/SLING-3524
> Project: Sling
> Issue Type: Improvement
> Components: API, JCR, ResourceResolver
>Affects Versions: Resource Resolver 1.0.6
>Reporter: Alexander Klimetschek
>Priority: Major
> Fix For: API 2.18.2, JCR Resource 3.0.10, Resource Resolver 1.6.2
>
> Time Spent: 50m
> Remaining Estimate: 0h
>
> {{ResourceResolver.clone()}} will reuse the same JCR session in case it was
> created by passing an existing session using
> {{JcrResourceConstants.AUTHENTICATION_INFO_SESSION}}. If you need a clone of
> the resource resolver to pass into a new, separate thread, and use
> {{ResourceResolver.clone(null)}}, you will actually share the session, but
> this is not obvious. The problem is that a JCR session cannot be shared
> across threads.
> The javadocs of clone() say "the same credential data is used as was used to
> create this instance".
> There are a few problems with this:
> - seeing the session object itself as "credential data" is unintuitive
> - in my code, I have no idea what the original credential data was, so I
> don't know what kind of credential data it was to make the right decision
> - since sharing a JCR session is to be avoided at all times, the resource
> resolver should prevent one from this
> A solution would be if a plain {{ResourceResolver.clone(null)}} would return
> a session that impersonated itself, abstracting this from the resource
> resolver user. Additionally, it might be worth looking that clone always
> returns a new session, unless specifically stated.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (SLING-3524) ResourceResolver.clone(null) should not share the same JCR session
[
https://issues.apache.org/jira/browse/SLING-3524?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16460809#comment-16460809
]
Carsten Ziegeler commented on SLING-3524:
-
Thanks for picking this up, I haven't looked at the JCR part of the patch, but
I agree that this needs adding something to the API for a provider to be picked
up. I think the resource resolver implementation should also clear this new
property from a user provided map if clone is not used. Just to avoid that a
user accidentally is putting such a property in the map
> ResourceResolver.clone(null) should not share the same JCR session
> --
>
> Key: SLING-3524
> URL: https://issues.apache.org/jira/browse/SLING-3524
> Project: Sling
> Issue Type: Improvement
> Components: API, JCR, ResourceResolver
>Affects Versions: Resource Resolver 1.0.6
>Reporter: Alexander Klimetschek
>Priority: Major
> Fix For: API 2.18.2, JCR Resource 3.0.10, Resource Resolver 1.6.2
>
> Time Spent: 50m
> Remaining Estimate: 0h
>
> {{ResourceResolver.clone()}} will reuse the same JCR session in case it was
> created by passing an existing session using
> {{JcrResourceConstants.AUTHENTICATION_INFO_SESSION}}. If you need a clone of
> the resource resolver to pass into a new, separate thread, and use
> {{ResourceResolver.clone(null)}}, you will actually share the session, but
> this is not obvious. The problem is that a JCR session cannot be shared
> across threads.
> The javadocs of clone() say "the same credential data is used as was used to
> create this instance".
> There are a few problems with this:
> - seeing the session object itself as "credential data" is unintuitive
> - in my code, I have no idea what the original credential data was, so I
> don't know what kind of credential data it was to make the right decision
> - since sharing a JCR session is to be avoided at all times, the resource
> resolver should prevent one from this
> A solution would be if a plain {{ResourceResolver.clone(null)}} would return
> a session that impersonated itself, abstracting this from the resource
> resolver user. Additionally, it might be worth looking that clone always
> returns a new session, unless specifically stated.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (SLING-3524) ResourceResolver.clone(null) should not share the same JCR session
[
https://issues.apache.org/jira/browse/SLING-3524?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16460196#comment-16460196
]
Csaba Varga commented on SLING-3524:
{quote}Unit tests covering all paths would be good, I think.
{quote}
I'll try to have a shot at it, then. With a parameterized test class covering
all eight combinations, I could probably get rid of the current testLeakOnSudo
and testNoSessionSharing tests. (They would be covered by the new test class,
which would test leaks and sharing in all the relevant combinations.)
{quote}AFAICS, the cell in bold is the only new combination as part of this
patch.
{quote}
Yes, that's the intention. If any other combination starts working differently,
it's most likely a bug.
> ResourceResolver.clone(null) should not share the same JCR session
> --
>
> Key: SLING-3524
> URL: https://issues.apache.org/jira/browse/SLING-3524
> Project: Sling
> Issue Type: Improvement
> Components: JCR, ResourceResolver
>Affects Versions: Resource Resolver 1.0.6
>Reporter: Alexander Klimetschek
>Priority: Major
> Time Spent: 40m
> Remaining Estimate: 0h
>
> {{ResourceResolver.clone()}} will reuse the same JCR session in case it was
> created by passing an existing session using
> {{JcrResourceConstants.AUTHENTICATION_INFO_SESSION}}. If you need a clone of
> the resource resolver to pass into a new, separate thread, and use
> {{ResourceResolver.clone(null)}}, you will actually share the session, but
> this is not obvious. The problem is that a JCR session cannot be shared
> across threads.
> The javadocs of clone() say "the same credential data is used as was used to
> create this instance".
> There are a few problems with this:
> - seeing the session object itself as "credential data" is unintuitive
> - in my code, I have no idea what the original credential data was, so I
> don't know what kind of credential data it was to make the right decision
> - since sharing a JCR session is to be avoided at all times, the resource
> resolver should prevent one from this
> A solution would be if a plain {{ResourceResolver.clone(null)}} would return
> a session that impersonated itself, abstracting this from the resource
> resolver user. Additionally, it might be worth looking that clone always
> returns a new session, unless specifically stated.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (SLING-3524) ResourceResolver.clone(null) should not share the same JCR session
[
https://issues.apache.org/jira/browse/SLING-3524?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16460184#comment-16460184
]
Alexander Klimetschek commented on SLING-3524:
--
Thanks, LGTM! Unit tests covering all paths would be good, I think.
For reference, I updated the table to make the differences more clear. AFAICS,
the cell in bold is the only new combination as part of this patch.
||Scenario||No clone||Clone||
|Normal login, no sudo|logoutSession: true
no impersonation call
doLogoutSession: true|_same as no clone_|
|Normal login with sudo|logoutSession: true
original session impersonated then closed, USER_IMPERSONATOR set
doLogoutSession: true|_same as no clone_|
|Session login, no sudo|logoutSession: false
session used as-is
doLogoutSession: false|*logoutSession: false*
*session self-impersonated*
*doLogoutSession: true*|
|Session login with sudo|logoutSession: false
session impersonated, USER_IMPERSONATOR set
doLogoutSession: true|_same as no clone_|
> ResourceResolver.clone(null) should not share the same JCR session
> --
>
> Key: SLING-3524
> URL: https://issues.apache.org/jira/browse/SLING-3524
> Project: Sling
> Issue Type: Improvement
> Components: JCR, ResourceResolver
>Affects Versions: Resource Resolver 1.0.6
>Reporter: Alexander Klimetschek
>Priority: Major
> Time Spent: 40m
> Remaining Estimate: 0h
>
> {{ResourceResolver.clone()}} will reuse the same JCR session in case it was
> created by passing an existing session using
> {{JcrResourceConstants.AUTHENTICATION_INFO_SESSION}}. If you need a clone of
> the resource resolver to pass into a new, separate thread, and use
> {{ResourceResolver.clone(null)}}, you will actually share the session, but
> this is not obvious. The problem is that a JCR session cannot be shared
> across threads.
> The javadocs of clone() say "the same credential data is used as was used to
> create this instance".
> There are a few problems with this:
> - seeing the session object itself as "credential data" is unintuitive
> - in my code, I have no idea what the original credential data was, so I
> don't know what kind of credential data it was to make the right decision
> - since sharing a JCR session is to be avoided at all times, the resource
> resolver should prevent one from this
> A solution would be if a plain {{ResourceResolver.clone(null)}} would return
> a session that impersonated itself, abstracting this from the resource
> resolver user. Additionally, it might be worth looking that clone always
> returns a new session, unless specifically stated.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (SLING-3524) ResourceResolver.clone(null) should not share the same JCR session
[
https://issues.apache.org/jira/browse/SLING-3524?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16459515#comment-16459515
]
Csaba Varga commented on SLING-3524:
{quote}I think the "needsCloning/needsSudo/explicitSessionUsed" might need some
explanation in comments, it's not straightforward to follow.
{quote}
Fair point. I've added some comments now. explicitSessionUsed is already
commented in the Javadoc of handleImpersonation(). Does it need some
clarification?
{quote}Are all possible combinations covered correctly?
{quote}
I think so, but I've gone through them this morning to double check. I believe
we have three independent variables to consider, giving eight cases in total:
* Is it a normal login or is a session given in the auth info? (For this
variable, I believe we can treat service logins and administrative logins as
normal logins.)
* Does the login request impersonation or not?
* Is the login caused by a clone() call?
This is what happens in the eight cases:
||Scenario||No clone||Clone||
|Normal login, no sudo|logoutSession: true
no impersonation call
doLogoutSession: true|logoutSession: true
no impersonation call
doLogoutSession: true|
|Normal login with sudo|logoutSession: true
original session impersonated then closed, USER_IMPERSONATOR set
doLogoutSession: true|logoutSession: true
original session impersonated then closed, USER_IMPERSONATOR set
doLogoutSession: true|
|Session login, no sudo|logoutSession: false
session used as-is
doLogoutSession: false|logoutSession: false
session self-impersonated
doLogoutSession: true|
|Session login with sudo|logoutSession: false
session impersonated, USER_IMPERSONATOR set
doLogoutSession: true|logoutSession: false
session impersonated, USER_IMPERSONATOR set
doLogoutSession: true|
The only case when cloning behavior is different from normal behavior is when
you pass a session but you don't want to impersonate. If you don't pass a
session, cloning will just log you in again with your credentials, just like
before the patch. If you pass a session and you request impersonation,
session.impersonate() was already called before this patch, and will keep being
called after it.
Am I missing something? If these are all the factors we need to worry about, do
you think it's worthwhile to build a parameterized unit test that covers all
possible combinations?
> ResourceResolver.clone(null) should not share the same JCR session
> --
>
> Key: SLING-3524
> URL: https://issues.apache.org/jira/browse/SLING-3524
> Project: Sling
> Issue Type: Improvement
> Components: JCR, ResourceResolver
>Affects Versions: Resource Resolver 1.0.6
>Reporter: Alexander Klimetschek
>Priority: Major
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> {{ResourceResolver.clone()}} will reuse the same JCR session in case it was
> created by passing an existing session using
> {{JcrResourceConstants.AUTHENTICATION_INFO_SESSION}}. If you need a clone of
> the resource resolver to pass into a new, separate thread, and use
> {{ResourceResolver.clone(null)}}, you will actually share the session, but
> this is not obvious. The problem is that a JCR session cannot be shared
> across threads.
> The javadocs of clone() say "the same credential data is used as was used to
> create this instance".
> There are a few problems with this:
> - seeing the session object itself as "credential data" is unintuitive
> - in my code, I have no idea what the original credential data was, so I
> don't know what kind of credential data it was to make the right decision
> - since sharing a JCR session is to be avoided at all times, the resource
> resolver should prevent one from this
> A solution would be if a plain {{ResourceResolver.clone(null)}} would return
> a session that impersonated itself, abstracting this from the resource
> resolver user. Additionally, it might be worth looking that clone always
> returns a new session, unless specifically stated.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (SLING-3524) ResourceResolver.clone(null) should not share the same JCR session
[
https://issues.apache.org/jira/browse/SLING-3524?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16458741#comment-16458741
]
Alexander Klimetschek commented on SLING-3524:
--
[~Csaba Varga] I like it! Regarding the jcr resource resolver changes, I think
the "needsCloning/needsSudo/explicitSessionUsed" might need some explanation in
comments, it's not straightforward to follow. Are all possible combinations
covered correctly?
> ResourceResolver.clone(null) should not share the same JCR session
> --
>
> Key: SLING-3524
> URL: https://issues.apache.org/jira/browse/SLING-3524
> Project: Sling
> Issue Type: Improvement
> Components: JCR, ResourceResolver
>Affects Versions: Resource Resolver 1.0.6
>Reporter: Alexander Klimetschek
>Priority: Major
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> {{ResourceResolver.clone()}} will reuse the same JCR session in case it was
> created by passing an existing session using
> {{JcrResourceConstants.AUTHENTICATION_INFO_SESSION}}. If you need a clone of
> the resource resolver to pass into a new, separate thread, and use
> {{ResourceResolver.clone(null)}}, you will actually share the session, but
> this is not obvious. The problem is that a JCR session cannot be shared
> across threads.
> The javadocs of clone() say "the same credential data is used as was used to
> create this instance".
> There are a few problems with this:
> - seeing the session object itself as "credential data" is unintuitive
> - in my code, I have no idea what the original credential data was, so I
> don't know what kind of credential data it was to make the right decision
> - since sharing a JCR session is to be avoided at all times, the resource
> resolver should prevent one from this
> A solution would be if a plain {{ResourceResolver.clone(null)}} would return
> a session that impersonated itself, abstracting this from the resource
> resolver user. Additionally, it might be worth looking that clone always
> returns a new session, unless specifically stated.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (SLING-3524) ResourceResolver.clone(null) should not share the same JCR session
[
https://issues.apache.org/jira/browse/SLING-3524?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16458499#comment-16458499
]
Csaba Varga commented on SLING-3524:
Here are my pull requests for the proposed fix:
[https://github.com/apache/sling-org-apache-sling-jcr-resource/pull/3]
[https://github.com/apache/sling-org-apache-sling-api/pull/3]
[https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/6]
I'm not sure if this is the right approach to the problem, but it seemed the
simplest.
The idea is to introduce a new key to the authentication info map, to indicate
that the login is happening because of a clone() call. Resource providers can
then adjust their behavior based on the presence of this key. The JCR resource
provider checks the presence of this flag, in addition to the presence of an
explicitly provided JCR session in the login info. If both are present, the
session is cloned using the self-impersonation trick to make sure it's not
shared with the originating resolver.
To allow proper behavior for clones of clones, the JCR resource provider also
updates the authentication info with the cloned session. I'm not sure how
"clean" this approach is, since the API doesn't say whether mutation of the
authentication info is supported as part of a provider login. It works with the
current implementation, but it may still violate the spirit of the API.
> ResourceResolver.clone(null) should not share the same JCR session
> --
>
> Key: SLING-3524
> URL: https://issues.apache.org/jira/browse/SLING-3524
> Project: Sling
> Issue Type: Improvement
> Components: JCR, ResourceResolver
>Affects Versions: Resource Resolver 1.0.6
>Reporter: Alexander Klimetschek
>Priority: Major
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> {{ResourceResolver.clone()}} will reuse the same JCR session in case it was
> created by passing an existing session using
> {{JcrResourceConstants.AUTHENTICATION_INFO_SESSION}}. If you need a clone of
> the resource resolver to pass into a new, separate thread, and use
> {{ResourceResolver.clone(null)}}, you will actually share the session, but
> this is not obvious. The problem is that a JCR session cannot be shared
> across threads.
> The javadocs of clone() say "the same credential data is used as was used to
> create this instance".
> There are a few problems with this:
> - seeing the session object itself as "credential data" is unintuitive
> - in my code, I have no idea what the original credential data was, so I
> don't know what kind of credential data it was to make the right decision
> - since sharing a JCR session is to be avoided at all times, the resource
> resolver should prevent one from this
> A solution would be if a plain {{ResourceResolver.clone(null)}} would return
> a session that impersonated itself, abstracting this from the resource
> resolver user. Additionally, it might be worth looking that clone always
> returns a new session, unless specifically stated.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (SLING-3524) ResourceResolver.clone(null) should not share the same JCR session
[
https://issues.apache.org/jira/browse/SLING-3524?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16458477#comment-16458477
]
Csaba Varga commented on SLING-3524:
I believe the problem is really about the statefulness of the authentication
info. The clone() method assumes that none of the authentication info has any
state, so just copying the references into a new map will give you a suitable
new authentication info. This works well most of the time when you authenticate
using strings, but breaks down with a JCR session object because it's _very_
stateful.
I'm working on a proposed patch that lets individual resource providers work
around this limitation by letting them distinguish a clone operation from a
regular login. In case of clone, stateful things like JCR sessions can be
properly cloned to avoid any inadvertent shared state between the old and new
resolvers. Providers that don't use stateful objects in their login info can
just keep working without any changes.
> ResourceResolver.clone(null) should not share the same JCR session
> --
>
> Key: SLING-3524
> URL: https://issues.apache.org/jira/browse/SLING-3524
> Project: Sling
> Issue Type: Improvement
> Components: JCR, ResourceResolver
>Affects Versions: Resource Resolver 1.0.6
>Reporter: Alexander Klimetschek
>Priority: Major
>
> {{ResourceResolver.clone()}} will reuse the same JCR session in case it was
> created by passing an existing session using
> {{JcrResourceConstants.AUTHENTICATION_INFO_SESSION}}. If you need a clone of
> the resource resolver to pass into a new, separate thread, and use
> {{ResourceResolver.clone(null)}}, you will actually share the session, but
> this is not obvious. The problem is that a JCR session cannot be shared
> across threads.
> The javadocs of clone() say "the same credential data is used as was used to
> create this instance".
> There are a few problems with this:
> - seeing the session object itself as "credential data" is unintuitive
> - in my code, I have no idea what the original credential data was, so I
> don't know what kind of credential data it was to make the right decision
> - since sharing a JCR session is to be avoided at all times, the resource
> resolver should prevent one from this
> A solution would be if a plain {{ResourceResolver.clone(null)}} would return
> a session that impersonated itself, abstracting this from the resource
> resolver user. Additionally, it might be worth looking that clone always
> returns a new session, unless specifically stated.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (SLING-3524) ResourceResolver.clone(null) should not share the same JCR session
[
https://issues.apache.org/jira/browse/SLING-3524?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=13983889#comment-13983889
]
Alexander Klimetschek commented on SLING-3524:
--
Note that use of {{ResourceResolverFactory.USER_IMPERSONATION}} (to self) does
not work either, since the
[JcrResourceProviderFactory|https://github.com/apache/sling/blob/trunk/bundles/jcr/resource/src/main/java/org/apache/sling/jcr/resource/internal/helper/jcr/JcrResourceProviderFactory.java#L364]
prevents from impersonating itself and returns the original session.
I think the problem is that the
JcrResourceProviderFactory.getResourceProviderInternal() method that handles
authentication info, is designed for an initial login (resource resolver
creation). A clone has different semantics (such as avoiding a shared session),
so this likely needs a separate code path.
ResourceResolver.clone(null) should not share the same JCR session
--
Key: SLING-3524
URL: https://issues.apache.org/jira/browse/SLING-3524
Project: Sling
Issue Type: Improvement
Components: ResourceResolver
Affects Versions: Resource Resolver 1.0.6
Reporter: Alexander Klimetschek
{{ResourceResolver.clone()}} will reuse the same JCR session in case it was
created by passing an existing session using
{{JcrResourceConstants.AUTHENTICATION_INFO_SESSION}}. If you need a clone of
the resource resolver to pass into a new, separate thread, and use
{{ResourceResolver.clone(null)}}, you will actually share the session, but
this is not obvious. The problem is that a JCR session cannot be shared
across threads.
The javadocs of clone() say the same credential data is used as was used to
create this instance.
There are a few problems with this:
- seeing the session object itself as credential data is unintuitive
- in my code, I have no idea what the original credential data was, so I
don't know what kind of credential data it was to make the right decision
- since sharing a JCR session is to be avoided at all times, the resource
resolver should prevent one from this
A solution would be if a plain {{ResourceResolver.clone(null)}} would return
a session that impersonated itself, abstracting this from the resource
resolver user. Additionally, it might be worth looking that clone always
returns a new session, unless specifically stated.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
