[MAGNOLIA-5957] Speed up 5-series login and working for installations with many users Created: 17/Oct/14  Updated: 12/Jan/15  Resolved: 28/Oct/14

Status: Closed
Project: Magnolia
Component/s: security
Affects Version/s: None
Fix Version/s: 5.2.10, 5.3.5

Type: Improvement Priority: Critical
Reporter: Daniel Lipp Assignee: Daniel Lipp
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
causality
caused by MAGNOLIA-5455 Replace use of queries with node trav... Closed
is causing MGNLSTK-1440 AddContactUserRoleTask might no longe... Closed
is causing MAGNOLIA-6040 InstallContext check in RepositoryBac... Closed
relation
is related to MGNLUI-3202 Speed up startup time of ContentApp w... Closed
is related to MAGNOLIA-5994 Retrieving all users or all groups is... Closed
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Release notes required:
Yes
Date of First Response:

 Description   

It was reported that login may easily take > 1 minute in case there's lots of users (e.g. 10k) in the system. Most of the time seems to be spent in RepositoryBackedSecurityManager#findPrincipalNode(String, Session, String, Node) where we don't use queries any longer (since MAGNOLIA-5455).

The main motivation to change the behavior had been that install tasks don't save sessions and hence there'd been situations where e.g. an install task creates a new user but that one could not yet be found by jcr queries as the session had not yet saved.

On this occasion we should finally add the missing RepositoryBackedSecurityManagerTest.



 Comments   
Comment by Aleksandr Pchelintcev [ 22/Oct/14 ]

Reviewed. Couple of minor findings:

  • TraversingMgnlPrincipalManagerFactory#wrap(usermanager) - uses inefficient map traversal internally (entrySet() would be better).
  • RepositoryBackedSecurityManager#findPrincipalNode - String.format() seems to be better than string concat
Comment by Daniel Lipp [ 23/Oct/14 ]

Didn't switch to entrySet yet but as there's only 2-3 entries in the map, this should not be too bad.
For the second case I had only re-activated the pre 5.2 impl but not improved that one yet. Did it now...

Comment by Daniel Lipp [ 24/Oct/14 ]

Reopening because we obviously didn't find all the places where we know have to use the TraversingMgnlPrincipalManagerFactory. Only nigration and ui tests revealed those.

Comment by Daniel Lipp [ 25/Oct/14 ]

Solution is not complete because during install time wrappers might try to read the user-information but as the session might not yet be saved they can fail! This case is not yet covered with the approach of explicitly use the TraversingPrincipalManagers when being called from a InstallTask!

[INFO] [talledLocalContainer] 	at info.magnolia.cms.security.RepositoryBackedSecurityManager.findPrincipalNode(RepositoryBackedSecurityManager.java:269)
[INFO] [talledLocalContainer] 	at info.magnolia.cms.security.MgnlUserManager.findPrincipalNode(MgnlUserManager.java:322)
[INFO] [talledLocalContainer] 	at info.magnolia.cms.security.MgnlUserManager$3.exec(MgnlUserManager.java:241)
[INFO] [talledLocalContainer] 	at info.magnolia.cms.security.MgnlUserManager$3.exec(MgnlUserManager.java:238)
[INFO] [talledLocalContainer] 	at info.magnolia.cms.security.JCRSessionOp.exec(JCRSessionOp.java:67)
[INFO] [talledLocalContainer] 	at info.magnolia.context.MgnlContext.doInSystemContext(MgnlContext.java:385)
[INFO] [talledLocalContainer] 	at info.magnolia.context.MgnlContext.doInSystemContext(MgnlContext.java:371)
[INFO] [talledLocalContainer] 	at info.magnolia.cms.security.MgnlUserManager.getUser(MgnlUserManager.java:238)
[INFO] [talledLocalContainer] 	at info.magnolia.cms.security.SystemUserManager.getOrCreateUser(SystemUserManager.java:191)
[INFO] [talledLocalContainer] 	at info.magnolia.cms.security.SystemUserManager.getSystemUser(SystemUserManager.java:131)
[INFO] [talledLocalContainer] 	at info.magnolia.cms.security.Security.getSystemUser(Security.java:80)
[INFO] [talledLocalContainer] 	at info.magnolia.context.AbstractContext.getUser(AbstractContext.java:72)
[INFO] [talledLocalContainer] 	at info.magnolia.context.MgnlContext.getUser(MgnlContext.java:91)
[INFO] [talledLocalContainer] 	at info.magnolia.jcr.wrapper.MgnlPropertySettingContentDecorator.updateLastModified(MgnlPropertySettingContentDecorator.java:530)
[INFO] [talledLocalContainer] 	at info.magnolia.jcr.wrapper.MgnlPropertySettingNodeWrapper.updateLastModified(MgnlPropertySettingNodeWrapper.java:270)
[INFO] [talledLocalContainer] 	at info.magnolia.jcr.wrapper.MgnlPropertySettingNodeWrapper.orderBefore(MgnlPropertySettingNodeWrapper.java:225)
[INFO] [talledLocalContainer] 	at info.magnolia.jcr.wrapper.DelegateNodeWrapper.orderBefore(DelegateNodeWrapper.java:332)
[INFO] [talledLocalContainer] 	at info.magnolia.jcr.util.NodeUtil.orderAfter(NodeUtil.java:294)
[INFO] [talledLocalContainer] 	at info.magnolia.module.delta.FilterOrderingTask.orderAfterSiblings(FilterOrderingTask.java:112)
[INFO] [talledLocalContainer] 	at info.magnolia.module.delta.FilterOrderingTask.doExecute(FilterOrderingTask.java:106)
[INFO] [talledLocalContainer] 	at info.magnolia.module.delta.AbstractRepositoryTask.execute(AbstractRepositoryTask.java:57)
[INFO] [talledLocalContainer] 	at info.magnolia.module.ModuleManagerImpl.applyDeltas(ModuleManagerImpl.java:514)
[INFO] [talledLocalContainer] 	at info.magnolia.module.ModuleManagerImpl.installOrUpdateModule(ModuleManagerImpl.java:496)

-> I think best compromise between impl effort / simplicity & safeness is to go back to the approach to let RepositoryBackedSecurityManager decide whether to query or traverse depending on whether we're in installation phase or not... After all it's a problem specific to JCR.

Generated at Mon Feb 12 04:09:58 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.