[MGNLHARDLK-74] Locked pages using JCR native API cannot be edited by anyone Created: 23/Aug/18  Updated: 01/Oct/18  Resolved: 01/Oct/18

Status: Closed
Project: Project - Telia - Hard Locking (closed)
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Bug Priority: Neutral
Reporter: Viet Nguyen Assignee: Zdenek Skodik
Resolution: Won't Do Votes: 0
Labels: None
Remaining Estimate: 0d
Time Spent: 0.5d
Original Estimate: Not Specified

Attachments: Java Source File LockInfo.java     Zip Archive org.zip    
Issue Links:
causality
dependency
depends upon MAGNOLIA-7365 Improve MgnlAuditLoggingContentDecora... Open
Template:
Acceptance criteria:
Empty
Date of First Response:

 Description   

After review and refactor hard-locking module, we decided to use JCR native lock support instead of hooking into MagnoliaAccessProvider and provide the custom implementation for locking. This helps end user not to configure their repository access provider anymore and simplify the implementation a lot. However this lead to an issue that:

By using JCR native lock for hard-locking module, user who locked the page cannot edit the page anymore because it is JCR designed purpose. Who lock the item can have the lock token to unlock it and 'lock-superuser' role is the role for the other one to unlock content

JCR function APIs:

  • javax.jcr.lock.LockManager.lock
  • javax.jcr.lock.LockManager.unlock

*This feature is conflicted with our declared function in https://wiki.magnolia-cms.com/display/SERVICES/Hard+Locking+submodule*

Hard Locking nodes ensures that only the user who locked the node is able to make changes to it until it is unlocked again.



 Comments   
Comment by Richard Gange [ 24/Aug/18 ]

To me this does not seem like such a big deal. If the person wants to edit it then they must unlock it to do so. Perhaps there may even be use cases where this makes sense for everyone to be locked out.

I see this as the same type of thing where a user in an application goes into "admin mode" or something like that. The system prompts the user that in order to make the change they must enter a different mode. Perhaps we could do something like that.

Comment by Viet Nguyen [ 28/Aug/18 ]

After evaluated different approaches including


Final solution:
Utilize webapp classloader default mechanism that will load classes under WEB-INF/classes with higher priority than classes under WEB-INF/lib ('jar' files). We provided a fixed LockInfo.class that will override existing Jackrabbit core class and put it under WEB-INF/classes/org/apache/jackrabbit/core/lock/LockInfo.class
This will 'patch' current LockInfo - check lock holder function follow our expectation.

Zip file here: org.zip
How to use it: copy zip file to your deployed WEB-INF/classes folder then unzip it.


This ticket is fixed under this commit:
https://git.magnolia-cms.com/projects/SERVICES/repos/hard-locking/commits/1549a2cedc4b52664d616d52aec6db725724c334

Comment by Zdenek Skodik [ 30/Aug/18 ]

Could you please elaborate on the rationale behind such a decision? I'd rather hesitate to patch JR impl directly, nor even rely on classloading order, which might vary between various app servers so as might be a subject of change at various deployments. We've came up with a custom mgnl AccessControlProvider right to target quirks in JR API and to me it sounds reasonable to expect those who wanna have even more strict provider to configure another one, extending the default one.

Comment by Viet Nguyen [ 30/Aug/18 ]

Yes zdenekskodik,

I'd rather not to patch JR if it is not necessary. If you had a chance to go over our custom AccessControlProvider, you'll see how much better if we can simply patch LockInfo.
I had research on different supported application servers and here is the result for class loading order, updated our README.md also:

Allow lock owner to edit content
An automatic patch will be copy to your configured 'magnolia.home'/WEB-INF/classes/org/apache/jackrabbit/core/lock/LockInfo.class This should automatically work for Tomcat and Websphere, for Weblogic, you'll have to specify your 'prefer-web-inf-classes' option accordingly. Also note that after module installation, user must restart your server or reload your webapp context so that the classloader can load classes again from the beginning. This time, our patch will take affect which means it will not affect right after installation, lock owners are not able to edit their content right away without server restart or webapp context reload.

Tomcat application server

To enable lock owner to edit his locked content, we have to patch Jackrabbit core LockInfo class which is the core class to check for lock holder information. Users would like to use this function would have to apply this patch by copying 'org' folder below 'override' folder to 'WEB-INF/classes' folder under your deployed webapp. We also provided an 'org.zip' file so that you just have to copy it to your 'WEB-INF/classes' folder then unzip it. This will effectively made LockInfo.class (our patched one) available to webapp class-loader and allow it loading this class instead of Jackrabbit provided one.

https://tomcat.apache.org/tomcat-8.5-doc/class-loader-howto.html

Weblogic 12 Configuration - prefer-web-inf-classes

Property 'prefer-web-inf-classes' has to be specified for the classes specified in Web-INF to take precendence over classes present in application library of weblogic server. https://docs.oracle.com/middleware/1212/wls/WLPRG/classloading.htm#WLPRG285

IBM Websphere 8.5

The sequence is similar to Tomcat 8, also it's configurable. https://www.ibm.com/support/knowledgecenter/en/SSAW57_8.5.5/com.ibm.websphere.nd.multiplatform.doc/ae/trun_classload_web.html

Comment by Zdenek Skodik [ 31/Aug/18 ]

I dream about the world where it's up to the instance owner to drive their appservers, I won't be happy as well if something from outside demands me to configure classloaders this or that way, there might be internal policies for not doing it that way, or another intentions on using the appservers. It's quite fine if they deploy their local changes that way, but external solutions should ideally be delivered in a clean and standard format - jar, than having patches here or there. 

Maintenance wise there would also be an extra effort required any time a major will bring new JR into play.

Also It's not an appservers agnostic solution as it used to be. We could have a hard locking parent webapp/pom providing the JR config files with preconfigured LockAwareMagnoliaAccessProvider customers can just use/extend as an alternative option, without having to follow some post-installation steps. IMHO having to reconfigure your appservers is a bit more demanding that updating few workspace.xml files. 

Sure there are some harder times on distributing the lock permissions at the provider, still workaround-able and better to me than maintaining a fork.

 

Comment by Viet Nguyen [ 31/Aug/18 ]

I agree with you zdenekskodik, however we spent quite a lot of time on this issue and also as rgange mentioned, to a certain extent, hard-locking module support JCR hard lock, user have to unlock to edit his content.
I have implemented an installation task that automatically copy patch file to its folder to free them from doing deployment. So customer want to edit locked content by his lock owner (the missing function) have to accept our patch temporarily. LockAwareAccessProvider messing with principals and permissions while not actually 'lock' the node as it should be. Also it introduced lots of complication which hard for maintenance and support, so rather not to use.
A proper way after deeply investigation is that our Core open up the possibility to configure MgnlAuditLoggingContentDecoratorSessionWrapper or MgnlAuditLoggingContentDecoratorWorspaceWrapper so that

  • For each node / property operation, we support before change and after change interceptors
  • For each interceptor, we support synchronous / asynchronous handlers to those events
  • Then to support custom locking and transactional locking, we'll register our custom content lock before apply any change action

--> I'll create an improvement on Core and link this ticket into it.

Comment by Viet Nguyen [ 31/Aug/18 ]

Link ticket created - MAGNOLIA-7365

Comment by Zdenek Skodik [ 10/Sep/18 ]

Please correct me if i'm wrong - hard locking aka i lock the page so that nobody else can edit it to concurrently override my writes i'm going to do. If JCR native lock doesn't meet this criteria I have hard time to understand why to follow this way. I believe that was considered in the early stages as well, but dropped. Ideally we want to have hard locking available so that ppl can use it. It doesn't have to necessarily reach the highest state of art with its very first release, keeping some room to properly implement improvements later on. From my point of view, inheriting the initial AccessProvider is simply better than forking JR with the first release. Later on the module can eventually introduce its own RepositoryManager allowing you to have a tailored ContentDecorator. rgange ?

Comment by Richard Gange [ 10/Sep/18 ]

I'm in total agreement with Zdenek. As far as I know the Hard Locking was working for TS. I think what we should do is keep the code that you have created using the native lock. Perhaps down the road we can still use some of it. But let's move back to the original working design. I know that you (Viet) have put a lot of time into this moving to the new concept but it appears we have hit a wall here. It was a good attempt at a new solution but let's not get too crazy with changing lower level stuff to make the new concept work. This could be a really nice feature to be part of the product someday.

Comment by Viet Nguyen [ 10/Sep/18 ]

So, we can revert back to our 1st merge PR here for our original version before making any API changes
Merge pull request #1 in SERVICES/hard-locking from SERVICESRD-48 to master

  • commit '9f143e6e5ac2c854c0fd0e928528aba9065bcefe'
Comment by Viet Nguyen [ 11/Sep/18 ]

zdenekskodik could you review changes, QA and close this issue, fixed as discussion.

Comment by Zdenek Skodik [ 11/Sep/18 ]

Yup.. rgange is there any QA workflow already in place for extension modules or review&preintQA can be done by a single guy?

Comment by Zdenek Skodik [ 11/Sep/18 ]

So after a review we'll try to treat the status quo by evaluating a dedicated repository manager wrapping the sessions hard-lock way, as if possible we'd prefer to not loose all the work done since than just because of those few lines at JR. 

Comment by Richard Gange [ 11/Sep/18 ]

Typically I will install the module and try it out. This way I can better create the required docs if I have firsthand usage of the module. viet.nguyen if you could put a rough draft of the docs together I will use that as my starting point.

Comment by Viet Nguyen [ 12/Sep/18 ]

Yes rgange, please reference to README.md file in the project for the installation and usage draft, we'll try to keep it up to date as a practice. Also let's consider reuse Javadoc to alleviate Docs team, maybe somehow auto generate configuration parts and technical detail level implementation notes.

Comment by Viet Nguyen [ 25/Sep/18 ]

Improvement mechanism for this in DEV-816.

Comment by Richard Gange [ 01/Oct/18 ]

Reverted changes that led to this issue. Meaning, the native locking.

Generated at Mon Feb 12 10:32:38 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.