Page tree
Skip to end of metadata
Go to start of metadata

Your Rating: Results: 1 Star2 Star3 Star4 Star5 Star 7 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:

  1. 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
  2. Failing tests for UserManager#getUsersWithGroup, 2 cases:
    1. getting members from super-group
      • currently misses members of sub-groups
    2. getting members from sub-group
      • currently contains members of its super-groups, i.e. non-members of that particular sub-group
  3. Mid-term API changes in GroupManager (i.e. in next major)
    1. #getAllSuperGroups vs. #getAllSubGroups
      • only then deprecate #getAllGroups
    2. implement these two on MgnlGroupManager for now


1b. additional renaming (GroupManager)

DONE

 

Reconsider other naming/apis on GroupManager too?

  1. #getGroupsWithGroup => #getDirectSubGroups
  2. #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
    • (warning) only uncertainty is that presently, this would work around an incomplete custom GroupManager implementation
  • 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 and GroupManager
    • #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:

  • (question) Why only empty-webapp would be affected? (different module order?)
  • (question) *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
  • (question) 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.");
      }
  • (question) LDAP?
  • (lightbulb) Do all GroupManagers have to support "group hierarchies" by nature?
    • or shall we split related APIs to HierarchicalGroupManager?

 

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.

 Click here to expand...

As reported in this ticket, customers may configure multiple UserManagers under the security support configuration (for different "realms", or even different systems).
However, the DelegatingUserManager simply claims to be "delegating to a set of user managers. The first user manager which does not throw an UnsupportedOperationException will be used."

This means that if you have both the SystemUserManager and an ExternalUserManager —in that order—then the system one (extends MgnlUserManager) will support and "swallow" all operations.
e.g. Querying for users with a specific group, to send a message into the pulse, will not make it to the ExternalUserManager (whether implemented or not).

Options:

  1. provide relevant support classes:
    1. AggregatingUserManager
      • read ops:
        • collects results from all user managers
        • acknowledge performance is not particularly tuned
        • "Please overwrite in case it can be implemented in a more efficient way."
      • write ops:
        • we obviously don't want to create users in multiple systems
        • allow one user manager to be the "default" one, most likely the SystemUserManager (if not explicitly configured)
    2. configurability
      • (warning)DelegatingUserManager is hard-coded inside SecuritySupportImpl
      • provide alternative (aggregating) SecuritySupportImpl?
      • built-in configuration property to use delegating vs. aggregating strategy? (most user-friendly imho)
  2. let it be a project impl, if we feel we're making too many assumptions
    • e.g. I expect the scenario described above to be fairly common (i.e. system users/magnolia admins living in magnolia, vs. authors coming from ldap)
    • "verified" at LDAP Connector module docu as well, under UserManager config
    • do feel free to comment if this is doesn't fit obvious cases

 

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.

 Click here to expand...

To void security risks and problems in the future, we think that special characters should be prevented in username. On the other hand, we should define a validator which allows only valid characters.

Additional reference is Google Name and Password Guidelines
+ Usernames can contain letters (a-z), numbers (0-9), dashes (-), underscores (_), apostrophes ('), and periods (.).
+ Usernames can't contain an ampersand (&), equal sign (= ), brackets (<,>), plus sign (+), comma (,), or more than one period (.) in a row.

Other resource also says "A well-designed system doesn't necessarily need to prevent any special characters in usernames."

 

 

  • No labels

2 Comments

  1. 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 on  PermissionUtil or AccessManager -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, the roles collection is empty making the method returning true anyway).

    Thank you very much!

    Cheers,

    Vivian

    1. 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? (smile)

      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 static PermissionUtil).

      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