Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.3.7
    • Component/s: locks
    • Labels:
      None

      Description

      The lock tokens for open scoped locks are currently tied to the session which created the lock. If the session dies (for whatever reason) there is no way to recover the lock and unlock the node.
      There is a theoretical way of adding the lock token to another session, but in most cases the lock token is not available.

      Fortunately, the spec allows to relax this behaviour and I think it would make sense to allow all sessions from the same user to unlock the node - this is still in compliance with the spec but would make unlocked locked nodes possible in a programmatic way.

      1. OpenScopeLockTest.java
        3 kB
        Janandith Uditha Jayawardena
      2. JCR-2859.patch
        5 kB
        Janandith Uditha Jayawardena
      3. JCR-2859.diff
        8 kB
        Julian Reschke
      4. JCR-2859.diff
        14 kB
        Julian Reschke

        Issue Links

          Activity

          Hide
          Cédric Damioli added a comment -

          It could also be possible to store the lock tocken somewhere (on the locked node ?, on the node state ?), so that it may be retrieved on demand and set on the current session.

          From my POV, it may be too "dangerous" to allow any session from the same user to unlock the node.
          What if the session which initially locked the node is still alive ?

          Show
          Cédric Damioli added a comment - It could also be possible to store the lock tocken somewhere (on the locked node ?, on the node state ?), so that it may be retrieved on demand and set on the current session. From my POV, it may be too "dangerous" to allow any session from the same user to unlock the node. What if the session which initially locked the node is still alive ?
          Hide
          Carsten Ziegeler added a comment -

          There is no API in JCR to get the lock token and I think storing it somewhere accessible makes the token available to everyone not just the user who created the lock.

          Now I don't consider this a great danger - having a locked node which can never be unlocked again, sounds more problematic too me.

          But I guess this should be made configurable - default being the old behaviour

          Show
          Carsten Ziegeler added a comment - There is no API in JCR to get the lock token and I think storing it somewhere accessible makes the token available to everyone not just the user who created the lock. Now I don't consider this a great danger - having a locked node which can never be unlocked again, sounds more problematic too me. But I guess this should be made configurable - default being the old behaviour
          Hide
          Stefan Guggisberg added a comment -

          > There is a theoretical way of adding the lock token to another session, but in most cases the lock token is not available.

          why? if a client does create an open-scoped lock (he has to do so explicitly) he's responsible for persisting the lock token
          for future use. if the client doesn't want to do that he should use session-scoped locks instead.

          > There is no API in JCR to get the lock token

          LockManager.lock(...) returns a Lock instance => Lock.getLockToken()

          > I think it would make sense to allow all sessions from the same user to unlock the node

          -1, i'd consider this rather a hack; the current behavior is IMO correct.

          Show
          Stefan Guggisberg added a comment - > There is a theoretical way of adding the lock token to another session, but in most cases the lock token is not available. why? if a client does create an open-scoped lock (he has to do so explicitly) he's responsible for persisting the lock token for future use. if the client doesn't want to do that he should use session-scoped locks instead. > There is no API in JCR to get the lock token LockManager.lock(...) returns a Lock instance => Lock.getLockToken() > I think it would make sense to allow all sessions from the same user to unlock the node -1, i'd consider this rather a hack; the current behavior is IMO correct.
          Hide
          Carsten Ziegeler added a comment -

          Session scoped locks do not work in a clustered env - so either this has to be fixed or that; otherwise locks in a clustered env are a pain to use

          Show
          Carsten Ziegeler added a comment - Session scoped locks do not work in a clustered env - so either this has to be fixed or that; otherwise locks in a clustered env are a pain to use
          Hide
          Carsten Ziegeler added a comment -

          > LockManager.lock(...) returns a Lock instance => Lock.getLockToken()
          Yes I know, what I meant above is: there is no way to get a lock token if you did not lock the node yourself

          Show
          Carsten Ziegeler added a comment - > LockManager.lock(...) returns a Lock instance => Lock.getLockToken() Yes I know, what I meant above is: there is no way to get a lock token if you did not lock the node yourself
          Hide
          Jukka Zitting added a comment -

          I would treat this as an administration operation that normal client sessions should not be able to do. We could for example allow admin sessions (or sessions with some special privilege) to unlock any locks in the repository.

          Show
          Jukka Zitting added a comment - I would treat this as an administration operation that normal client sessions should not be able to do. We could for example allow admin sessions (or sessions with some special privilege) to unlock any locks in the repository.
          Hide
          Cédric Damioli added a comment -

          I tend to agree with Stefan that it's the client responsibility to store lock tockens.
          It's the way I'm managing locks in my applications.

          But it may happen some failures (either in the client code or in JR code) that make the tocken unrecoverable.

          Giving the possibility to have some admin feature to cover such cases in IMHO a very good thing.
          Right now, I have to stop the application (which may be very annoying in production) and modify the locks file by hand.

          So a big +1 to Jukka's proposal, if client code is able to have access to such special sessions.

          Show
          Cédric Damioli added a comment - I tend to agree with Stefan that it's the client responsibility to store lock tockens. It's the way I'm managing locks in my applications. But it may happen some failures (either in the client code or in JR code) that make the tocken unrecoverable. Giving the possibility to have some admin feature to cover such cases in IMHO a very good thing. Right now, I have to stop the application (which may be very annoying in production) and modify the locks file by hand. So a big +1 to Jukka's proposal, if client code is able to have access to such special sessions.
          Hide
          Carsten Ziegeler added a comment -

          I'm fine with Jukka's proposal as well - my idea was based on another repository implementation I've seen - in the end I don't care what the exact way is, as long as it is possible

          Show
          Carsten Ziegeler added a comment - I'm fine with Jukka's proposal as well - my idea was based on another repository implementation I've seen - in the end I don't care what the exact way is, as long as it is possible
          Hide
          Janandith Uditha Jayawardena added a comment -

          Attached a test case for the scenario described in this JIRA,

          I'm working on a patch.

          The solution I'm working on is as follows,

          1. Store lock tokens when a node is locked using LockManager.
          2. Add the locked tokens from the stored location to the session when session is created if principal is AdminPrincipal.
          3. Remove the lock token from the store when the node is unlocked using admin session.

          I would like to know the following,

          1. Will there be any consequences in above solution ?.
          2. What is the best place to store the lock tokens ? . Is it in a node like ex: /locktokens or a file)
          3. Any improvements, suggestions ?.

          Show
          Janandith Uditha Jayawardena added a comment - Attached a test case for the scenario described in this JIRA, I'm working on a patch. The solution I'm working on is as follows, 1. Store lock tokens when a node is locked using LockManager. 2. Add the locked tokens from the stored location to the session when session is created if principal is AdminPrincipal. 3. Remove the lock token from the store when the node is unlocked using admin session. I would like to know the following, 1. Will there be any consequences in above solution ?. 2. What is the best place to store the lock tokens ? . Is it in a node like ex: /locktokens or a file) 3. Any improvements, suggestions ?.
          Hide
          Jukka Zitting added a comment -

          I would rather not add extra lock tokens to an admin session, since that would muddy the waters on who is actually owning or holding a lock at a time. Instead I'd simply give an admin session the right to unlock any open-scoped lock, regardless of whether it holds the lock token or not.

          Note also that the tokens of all open scoped locks are already stored by the repository in a "locks" file so that they will survive over repository restarts. But as mentioned above, you probably don't need to worry about the tokens when implementing this.

          Show
          Jukka Zitting added a comment - I would rather not add extra lock tokens to an admin session, since that would muddy the waters on who is actually owning or holding a lock at a time. Instead I'd simply give an admin session the right to unlock any open-scoped lock, regardless of whether it holds the lock token or not. Note also that the tokens of all open scoped locks are already stored by the repository in a "locks" file so that they will survive over repository restarts. But as mentioned above, you probably don't need to worry about the tokens when implementing this.
          Hide
          Janandith Uditha Jayawardena added a comment - - edited

          Attached a patch. If the session is an admin session and lock is open scoped the session will be
          the lock holder so it can be unlocked.

          Modified AbstractLockTest and OpenScopedLockTest to work with
          new feature.

          Show
          Janandith Uditha Jayawardena added a comment - - edited Attached a patch. If the session is an admin session and lock is open scoped the session will be the lock holder so it can be unlocked. Modified AbstractLockTest and OpenScopedLockTest to work with new feature.
          Hide
          Julian Reschke added a comment -

          > Attached a patch. If the session is an admin session and lock is open scoped the session will be
          the lock holder so it can be unlocked.

          I think this is problematic. Consider processes running in admin sessions trying to synchronize/reserve using locks. If all admin locks are essentially shared by all admin sessions, this will break big time.

          Show
          Julian Reschke added a comment - > Attached a patch. If the session is an admin session and lock is open scoped the session will be the lock holder so it can be unlocked. I think this is problematic. Consider processes running in admin sessions trying to synchronize/reserve using locks. If all admin locks are essentially shared by all admin sessions, this will break big time.
          Hide
          Julian Reschke added a comment -

          I believe there's a better way to do this; which happens to be what's used in WebDAV as well (yeah for consistency):

          a) define a class of users that are "lock breakers", for now, the admin
          b) to these users, provide the lock token (instead of returning null); this is allowed per JSR-283, http://www.day.com/specs/jcr/2.0/17_Locking.html#17.12.4%20Getting%20a%20Lock%20Token
          c) then, the lock breaker can add the lock token to the Session and perform the unlock()

          Show
          Julian Reschke added a comment - I believe there's a better way to do this; which happens to be what's used in WebDAV as well (yeah for consistency): a) define a class of users that are "lock breakers", for now, the admin b) to these users, provide the lock token (instead of returning null); this is allowed per JSR-283, http://www.day.com/specs/jcr/2.0/17_Locking.html#17.12.4%20Getting%20a%20Lock%20Token c) then, the lock breaker can add the lock token to the Session and perform the unlock()
          Hide
          Julian Reschke added a comment -

          Proposed patch, adding a test case.

          Note this also changes quite a few existing tests that assume that getLockToken has to return null, when it doesn't need to according to the spec. I just made those pass for now, but they need an additional look.

          Show
          Julian Reschke added a comment - Proposed patch, adding a test case. Note this also changes quite a few existing tests that assume that getLockToken has to return null, when it doesn't need to according to the spec. I just made those pass for now, but they need an additional look.
          Hide
          Felix Meschberger added a comment -

          I like the lock breaker idea.

          Shouldn't we push this a step forward and rename the isAdmin method to isLockbreaker and introduce a "lockbreaker" group and require the session to be a member of that group ? Kind of like a well known group name like administrators and everyone ?

          Show
          Felix Meschberger added a comment - I like the lock breaker idea. Shouldn't we push this a step forward and rename the isAdmin method to isLockbreaker and introduce a "lockbreaker" group and require the session to be a member of that group ? Kind of like a well known group name like administrators and everyone ?
          Hide
          Julian Reschke added a comment -

          > Shouldn't we push this a step forward and rename the isAdmin method to isLockbreaker and introduce a "lockbreaker" group and require the session to be a member of that group ? Kind of like a well known group name like administrators and everyone ?

          Yes, something like that. And make admin automatically a lockbreaker, I assume.

          Do we have other code that works in a similar way where I could steal code?

          (in the meantime I'll fix JCR-3195 which will make this change smaller)

          Show
          Julian Reschke added a comment - > Shouldn't we push this a step forward and rename the isAdmin method to isLockbreaker and introduce a "lockbreaker" group and require the session to be a member of that group ? Kind of like a well known group name like administrators and everyone ? Yes, something like that. And make admin automatically a lockbreaker, I assume. Do we have other code that works in a similar way where I could steal code? (in the meantime I'll fix JCR-3195 which will make this change smaller)
          Hide
          Julian Reschke added a comment -

          Updated proposed patch, incl. a test case.

          Note this also changes quite a few existing tests that assume that getLockToken has to return null, when it doesn't need to according to the spec.

          What's left to do is to incorporate Felix' proposal to allow this based on the membership in a "lock breaker" group.

          Show
          Julian Reschke added a comment - Updated proposed patch, incl. a test case. Note this also changes quite a few existing tests that assume that getLockToken has to return null, when it doesn't need to according to the spec. What's left to do is to incorporate Felix' proposal to allow this based on the membership in a "lock breaker" group.
          Hide
          angela added a comment -

          imo loosing a lock token should be considered a mistake with the API consumer rather than something that
          occurs on a regular basis. while i am fine with providing a fallback in case the token is indeed lost, i am
          therefore not convinced that having a group that is allowed to see all lock tokens in the repository would be
          a wise move. apart from the fact that i consider this an edge case that should not be used on a regular
          basis, being member of a given group will not guarantee that a given user is allowed to lock/unlock a
          given node but only expose the lock token (in contrast to the admin).

          thus, i'm in favor of the latest patch by julian. however, -1 for allow breaking locks based on group membership.

          Show
          angela added a comment - imo loosing a lock token should be considered a mistake with the API consumer rather than something that occurs on a regular basis. while i am fine with providing a fallback in case the token is indeed lost, i am therefore not convinced that having a group that is allowed to see all lock tokens in the repository would be a wise move. apart from the fact that i consider this an edge case that should not be used on a regular basis, being member of a given group will not guarantee that a given user is allowed to lock/unlock a given node but only expose the lock token (in contrast to the admin). thus, i'm in favor of the latest patch by julian. however, -1 for allow breaking locks based on group membership.
          Hide
          Julian Reschke added a comment -

          +1 for starting with a small change (thus leaving it to admins).

          That being said, maybe we should also try to mitigate the effects of lost lock tokens? For instance, by changing the default timeout from "Infinity" to something reasonable?

          Show
          Julian Reschke added a comment - +1 for starting with a small change (thus leaving it to admins). That being said, maybe we should also try to mitigate the effects of lost lock tokens? For instance, by changing the default timeout from "Infinity" to something reasonable?
          Hide
          angela added a comment -

          > by changing the default timeout from "Infinity" to something reasonable

          yes. that definitely makes sense... maybe we could even make the default timeout part of the workspace
          configuration as the preferred timeout may vary between different types of applications.
          in any case we should clarify that as a general rule it is preferable to specify a reasonable timeout
          suited for the situation when creating a new lock... LockManager#lock always takes a timeout hint, while
          Node.lock (which doesn't support it) has been deprecated as of JSR 283.

          Show
          angela added a comment - > by changing the default timeout from "Infinity" to something reasonable yes. that definitely makes sense... maybe we could even make the default timeout part of the workspace configuration as the preferred timeout may vary between different types of applications. in any case we should clarify that as a general rule it is preferable to specify a reasonable timeout suited for the situation when creating a new lock... LockManager#lock always takes a timeout hint, while Node.lock (which doesn't support it) has been deprecated as of JSR 283.
          Hide
          Julian Reschke added a comment -

          So I have left the patch as proposed, allowing admin users to get the lock token, enabling them to unlock the node.

          Added JCR-3199 as a change request (make the default lock time out configurable).

          Show
          Julian Reschke added a comment - So I have left the patch as proposed, allowing admin users to get the lock token, enabling them to unlock the node. Added JCR-3199 as a change request (make the default lock time out configurable).
          Hide
          Julian Reschke added a comment -

          causes breakage in jcr2dav tests

          Show
          Julian Reschke added a comment - causes breakage in jcr2dav tests
          Hide
          Julian Reschke added a comment -

          The problem is caused by LockInfoImpl in SPI assuming that seeing the lock token implies owning the Lock.

          Show
          Julian Reschke added a comment - The problem is caused by LockInfoImpl in SPI assuming that seeing the lock token implies owning the Lock.
          Hide
          Julian Reschke added a comment -

          proposed patch

          Show
          Julian Reschke added a comment - proposed patch
          Hide
          Jukka Zitting added a comment -

          Merged to the 2.4 branch in revision 1232513.

          Show
          Jukka Zitting added a comment - Merged to the 2.4 branch in revision 1232513.

            People

            • Assignee:
              Julian Reschke
              Reporter:
              Carsten Ziegeler
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development