Concept - Improvements in the security package

Your Rating:

Thanks for voting!

Please Wait

Results:

9

rates

Table of contents

Participants

• Espen Jervidalo
• Philip Mundt
• Mikaël Geljić

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
  •  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:

•  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?

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.