[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: |
|
||||||||||||||||||||||||||||
| 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 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:
|
| 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. |
| 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. |