Your Rating: |
![]() ![]() ![]() ![]() ![]() |
Results: |
![]() ![]() ![]() ![]() ![]() |
9 | rates |
This page captures a series of fairly precise improvements, in the context of security, user, group and role managers.
Most classes involved here belong to the info.magnolia.cms.security
package.
Table of contents
Participants
1. UserManager & GroupManager APIs
1a. super-groups vs. sub-groups
READY
MAGNOLIA-6615 - Getting issue details... STATUS
MAGNOLIA-6624 - Getting issue details... STATUS
This originated from the aforementioned bug: recursive lookup of groups in UserManager
occurs "the wrong way around".
We concurred that some of GroupManager
's APIs are ambiguous, and incomplete for some use cases.
GroupManager#getAllGroups(String)
is recursive, but for a given group, returns the groups it belongs to, i.e. its super-groups (not its sub-groups, as one may think).- There is no "opposite" API which recursively looks for sub-groups.
GroupManager#getGroupsWithGroup(String)
gets sub-groups, but is not recursive, i.e. only sub-groups with the given group directly assigned, are returned
We do see use cases for both:
- checking groups of a user, and their super-groups (is user a member of that group)
- dumping all users and sub-groups which belong to a given group
We agreed to proceed with the following:
- QA first
- Variable renaming in
MgnlGroupManager
#getAllGroups
and#collectGroups
- "subGroups" variables are confusing, because they're actually used to collect super-groups
- might as well rename
#collectGroups
to#collectSuperGroups
(private)
- Poor javadoc on
GroupManager
's APIs mentioned above
- Variable renaming in
- Failing tests for
UserManager#getUsersWithGroup
, 2 cases:- getting members from super-group
- currently misses members of sub-groups
- getting members from sub-group
- currently contains members of its super-groups, i.e. non-members of that particular sub-group
- getting members from super-group
- Mid-term API changes in
GroupManager
(i.e. in next major)#getAllSuperGroups
vs.#getAllSubGroups
- only then deprecate
#getAllGroups
- only then deprecate
- implement these two on
MgnlGroupManager
for now
1b. additional renaming (GroupManager)
DONE
Reconsider other naming/apis on GroupManager
too?
#getGroupsWithGroup
=>#getDirectSubGroups
- +
#getDirectSuperGroups
?
1c. additional renaming (UserManager)
OPEN
Optionally, we may:
Consider more explicit/differentiated APIs for UserManager
too?
#getUsersWithGroup(String groupName)
- vs.
#getUsersWithGroup(String groupName, boolean transitive)
=> get(All|Direct)GroupUsers/Members
2. Clearing recursive group lookup from MgnlUser
ACCEPTED TO INVESTIGATE
MgnlUser
has the correct recursive lookup of sub-groups; but it seems rather badly located.
- quoting
MgnlUser#getAllGroups
:- // TODO: if the user is just a simple bean, then this method doesn't belong here anymore!!!!
- that method may still be convenient, but it should just delegate the recursive group lookup to the
GroupManager
- currently it has its own private method
#addSubgroups
only uncertainty is that presently, this would work around an incomplete custom
GroupManager
implementation
- currently it has its own private method
MgnlUser#getAllRoles
might go with the flow
3. MgnlUser bean consistency
ACCEPTED TO INVESTIGATE
MAGNOLIA-7028 - Getting issue details... STATUS
MgnlUser
is inconsistent with respect to direct vs. transitive groups and roles.
- direct groups and roles are assigned as bean fields
#getGroups
,#getRoles
- assigned upon user init
- needs logout/login to account for changes
- transitive groups and roles go through
SecuritySupport
andGroupManager
#getAllGroups
,#getAllRoles
- read live upon invocation
- instantly accounting for changes
We tend to favor the first approach for two reasons:
- less overhead with live changes / while login time shouldn't suffer too badly from this
MgnlUser
becomes closer to the raw bean it's supposed to be
Additionally, we might consider taking over the permissions list from the AccessManager
down to a similar level, for the sake of consistency.
If so, then it becomes more critical to address invalidation of user roles and perms.
Any additional input is welcome here.
4. Init-phase security support
TO INVESTIGATE
MAGNOLIA-6593 - Getting issue details... STATUS
- cannot give superuser roles for new modules' workspaces
i.m.module.delta.SetupModuleRepositoriesTask#grantRepositoryToSuperuser
Pending questions:
Why only empty-webapp would be affected? (different module order?)
*When* is
InitSecuritySupportImpl
replaced?
5. SecurityManager strategies
NEEDS WORK
info.magnolia.cms.security.RepositoryBackedSecurityManager
#findPrincipalNode
if #isInstallPhase?
#findPrincipalNodeByTraversal
else
#findPrincipalNodeByQueries
Proposal:
- A "cosmetic refactoring" to split those two strategies into different impls, and replace them throughout startup lifecycle:
- split InitPhase vs. InstallPhase vs. Runtime/Default
on SecuritySupport or SecurityManager level?
6. Assessing impact, custom manager impls
TO INVESTIGATE
Related to future API changes or requirements we have to think about missing impls (e.g. how security tools would dump groups recursively, via LdapUserManager)
- Other User/Group/RoleManager impls, do they support this?
- JAAS:
- ExternalUserManager.java
@Override #getUsersWithGroup(groupName, transitive) { throw new UnsupportedOperationException("Not supported by this user manager."); }
LDAP?
Do all
GroupManagers
have to support "group hierarchies" by nature?- or shall we split related APIs to
HierarchicalGroupManager
?
- or shall we split related APIs to
7. DelegatingUserManager vs. AggregatingUserManager
NOT AN ISSUE
MGNLUI-3794 - Getting issue details... STATUS
Despite the claim in DelegatingUserManager
"The first user manager which does not throw an UnsupportedOperationException will be used", this is not true for get operations returning multiple users. In fact, #getAllUsers
is already aggregating users from all managers—not just from the first one supporting it. Only a few other similar methods are implemented incorrectly, so we will simply fix these.
8. User name validation
We currently fail the save action when user name contains JCR-invalid chars. As an improvement, we could make that explicit with a dedicated validator for such JCR-invalid chars.
This is rather a UI improvement than something in the main security package, but ideally such validation could be delegated to the UserManager
or similar (much like many other JCR operations spread across the UI, yet affecting users, groups or roles).
TO INVESTIGATE
This is yet to be determined how big of an issue this is currently.
Unless it causes any fundamental issue, there is no need for us to restrict user names more than what the JCR spec forbids:
The characters declared invalid within a local name (“
/
”, “:
”, “[
“, “]
”, “|
”, “*
”) represent only those characters which are used as metacharacters in JCR names, paths and name-matching patterns
via section 3.2.4 Naming Restrictions of the JCR 2.0 specification.
For completeness, below is the original comment from Hieu Nguyen Duc.
2 Comments
Vivian Steller
Hi guys,
great article! Really looking forward to those changes! One more thing...
Please consider performance: When refactoring the User API, please also consider implementing some (internal) caching functionality (maybe directly) in the security layer. In fact, were currently facing significant performance issues rooted in calls to
user.getAllRoles()
.For instance, the aforementioned method is used in Availability Checks for actions in apps, e.g. used in the AccessGrantedRule implementation. Deep profiling with JProfiler showed that the call to
user.getAllRoles()
terribly slows down the availability check, which is called hundreds if not thousands of times if you have many actions/nodes in an app! We proofed with tests that accessing the application (opening apps as well as choose dialogs etc.) with superuser permissions is almost 2x faster than with non-superuser privileges. Obviously it seems that there's no caching implemented behind those calls, although (if I remember correctly) some kind of caching is implemented onPermissionUtil
orAccessManager
-Level.So, please consider implementing caching of permissions consistently across the API and if you're about to do it avoid hard coding of
PermissionUtil
usage in favor of DI use.(Side note: the
user.getAllRoles()
invocation in the implementation mentioned above is pretty much useless in most cases, that is, if no roles are defined for the action, theroles
collection is empty making the method returningtrue
anyway).Thank you very much!
Cheers,
Vivian
Mikaël Geljić
Thanks Vivian for elaborating,
This is precisely what section 3 is about: addressing the discrepancy between some "cached" info about direct groups and roles in the user itself, and those lengthy/redundant recursive operations. So this conforts me in assigning these upon user login (keeping logout/expiration as the condition of invalidation for now).
Re: AccessGrantedRule, thanks for the hint, looks like a cheap improvement we can do already. Mind filing a ticket, and eventually a PR if you're willing to?
Re: caching again, there's a low-level cache of permissions in the
AccessManagerImpl
, although that component itself is not cached beyond the lifecycle of the context (eventually re-instantiated again via that staticPermissionUtil
).That said, I have to say there's currently no dedicated focus on those initiatives; as you can see some of them got done or discarded along the way already, but it helps anyhow to have some form of backing for these.
Cheers,
Mika