RoleHierarchy Comments are misleading

[since1986]

Summary

In the current version (5.1.5), the comments in the org.springframework.security.access.hierarchicalroles.RoleHierarchy class do not indicate their true behavior and are misleading.

Actual Behavior

The correct expression for "role inheritance" should be defined like this: "ROLE_HIGHEST > ROLE_HIGHER > ROLE_LOW > ROLE_LOWER > ROLE_LOWEST".

Expected Behavior

But the comment says it needs to be defined like this: "ROLE_HIGHEST > ROLE_HIGHER and ROLE_HIGHER > ROLE_LOW and ROLE_LOW > ROLE_LOWER and ROLE_LOWER > ROLE_LOWEST". This way of defining expressions is fine in earlier versions, but it is wrong in the current version (I learned from debugging source code).

Configuration

Version

5.1.5

Sample

Please focus on two places:
org.springframework.security.access.hierarchicalroles.RoleHierarchyImpl#buildRolesReachableInOneStepMap
org.springframework.security.access.expression.SecurityExpressionRoot#hasAnyAuthorityName

[rwinch]

Can you you think the issue is? I have added tests to demonstrate this works as expected See 1f7ba47 for the tests

[since1986]

Can you you think the issue is? I have added tests to demonstrate this works as expected See 1f7ba47 for the tests

Sorry, I may not have stated it clearly. I mean, in the comments on the "RoleHierarchy" interface, the expression "role inheritance" needs to have the "and" connector to connect, but in fact, it is not correct to use the "and" connection. (or not exactly right, it was correct in earlier versions)

public interface RoleHierarchy {

	/**
	 * Returns an array of all reachable authorities.
	 * <p>
	 * Reachable authorities are the directly assigned authorities plus all authorities
	 * that are (transitively) reachable from them in the role hierarchy.
	 * <p>
	 * Example:<br>
	 * Role hierarchy: ROLE_A &gt; ROLE_B and ROLE_B &gt; ROLE_C.<br>
	 * Directly assigned authority: ROLE_A.<br>
	 * Reachable authorities: ROLE_A, ROLE_B, ROLE_C.
	 *
	 * @param authorities - List of the directly assigned authorities.
	 * @return List of all reachable authorities given the assigned authorities.
	 */
	public Collection<? extends GrantedAuthority> getReachableGrantedAuthorities(
			Collection<? extends GrantedAuthority> authorities);

}

Please pay attention to the "and" in the comment above.

...
Role hierarchy: ROLE_A &gt; ROLE_B and ROLE_B &gt; ROLE_C.<br>
...

Unit testing also shows that it is wrong to connect with "and"：

	@Test
	public void testRoleHierarchyInterfaceJavadoc() {
		List<GrantedAuthority> flatAuthorities = AuthorityUtils.createAuthorityList("ROLE_HIGHEST");
		List<GrantedAuthority> allAuthorities = AuthorityUtils.createAuthorityList("ROLE_HIGHEST", "ROLE_HIGHER", "ROLE_LOW", "ROLE_LOWER");
		RoleHierarchyImpl roleHierarchyImpl = new RoleHierarchyImpl();
		roleHierarchyImpl.setHierarchy("ROLE_HIGHEST > ROLE_HIGHER and ROLE_HIGHER > ROLE_LOW and ROLE_LOW > ROLE_LOWER");

		assertThat(roleHierarchyImpl.getReachableGrantedAuthorities(flatAuthorities)).containsExactlyInAnyOrderElementsOf(allAuthorities);
	}

java.lang.AssertionError: 
Expecting:
  <[ROLE_HIGHER and ROLE_HIGHER, ROLE_LOWER, ROLE_LOW and ROLE_LOW, ROLE_HIGHEST]>
to contain exactly in any order:
  <[ROLE_HIGHEST, ROLE_HIGHER, ROLE_LOW, ROLE_LOWER]>
elements not found:
  <[ROLE_HIGHER, ROLE_LOW]>
and elements not expected:
  <[ROLE_HIGHER and ROLE_HIGHER, ROLE_LOW and ROLE_LOW]>


	at org.springframework.security.access.hierarchicalroles.RoleHierarchyImplTests.testRoleHierarchyInterfaceJavadoc(RoleHierarchyImplTests.java:269)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
	at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
	at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:68)
	at com.intellij.rt.execution.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:47)
	at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:242)
	at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:70)


Process finished with exit code -1

[since1986]

Or, to put it another way, perhaps the current version of "RoleHierarchyImpl" is not compatible with the definition rules of the old version of the "role inheritance" expression.

[since1986]

With the previous version (forgot the specific version number, maybe 4.x), the "and" connector is supported in the "role inheritance" expression.

[rwinch]

Thanks for the clarification. RoleHiearchy isn't implementation specific. Instead it is trying to convey information rather than the configuration. That said, I can see how it might lead to confusion. Can you think of a way that makes the Javadoc in RoleHiearchy read better? If you do have a better wording, would you be willing to open a PR to change the RoleHiearchy Javadoc?

Or, to put it another way, perhaps the current version of "RoleHierarchyImpl" is not compatible with the definition rules of the old version of the "role inheritance" expression.

Can you clarify why you believe RoleHierarchyImpl worked differently and when it did? The code has gone largely untouched for over 10 years.

[since1986]

Thanks for the clarification. RoleHiearchy isn't implementation specific. Instead it is trying to convey information rather than the configuration. That said, I can see how it might lead to confusion. Can you think of a way that makes the Javadoc in RoleHiearchy read better? If you do have a better wording, would you be willing to open a PR to change the RoleHiearchy Javadoc?

Or, to put it another way, perhaps the current version of "RoleHierarchyImpl" is not compatible with the definition rules of the old version of the "role inheritance" expression.

Can you clarify why you believe RoleHierarchyImpl worked differently and when it did? The code has gone largely untouched for over 10 years.

I reviewed my previous code these two days and learned that I used the version 4.2.2 before.
Then, I looked at the source code for 4.2.2 and found out why the "and" connector can be used in the "role inheritance" expression in this version. In fact, in this version, the connector can be any string. Let's look at the code for details:

	/**
	 * Parse input and build the map for the roles reachable in one step: the higher role
	 * will become a key that references a set of the reachable lower roles.
	 */
	private void buildRolesReachableInOneStepMap() {
		Pattern pattern = Pattern.compile("(\\s*([^\\s>]+)\\s*>\\s*([^\\s>]+))");

		Matcher roleHierarchyMatcher = pattern
				.matcher(this.roleHierarchyStringRepresentation);
		this.rolesReachableInOneStepMap = new HashMap<GrantedAuthority, Set<GrantedAuthority>>();

		while (roleHierarchyMatcher.find()) {
			GrantedAuthority higherRole = new SimpleGrantedAuthority(
					roleHierarchyMatcher.group(2));
			GrantedAuthority lowerRole = new SimpleGrantedAuthority(
					roleHierarchyMatcher.group(3));
			Set<GrantedAuthority> rolesReachableInOneStepSet;

			if (!this.rolesReachableInOneStepMap.containsKey(higherRole)) {
				rolesReachableInOneStepSet = new HashSet<GrantedAuthority>();
				this.rolesReachableInOneStepMap.put(higherRole,
						rolesReachableInOneStepSet);
			}
			else {
				rolesReachableInOneStepSet = this.rolesReachableInOneStepMap
						.get(higherRole);
			}
			addReachableRoles(rolesReachableInOneStepSet, lowerRole);

			logger.debug("buildRolesReachableInOneStepMap() - From role " + higherRole
					+ " one can reach role " + lowerRole + " in one step.");
		}
	}

In this code, a regular expression grouping match is used to find a group that matches the rule. In fact, the string to be matched can contain any kind of connector. Any kind of connector will not affect the result of the expression "roleHierarchyMatcher.find()" equal to true.

	@Test
	public void testRegexForRoleHierarchyString() {
		Pattern pattern = Pattern.compile("(\\s*([^\\s>]+)\\s*>\\s*([^\\s>]+))");

		String roleHierarchyStringSplitByAnd = "ROLE_HIGHEST > ROLE_HIGHER and ROLE_HIGHER > ROLE_LOW and ROLE_LOW > ROLE_LOWER";
		String roleHierarchyStringSplitByOr = "ROLE_HIGHEST > ROLE_HIGHER or ROLE_HIGHER > ROLE_LOW or ROLE_LOW > ROLE_LOWER";
		String roleHierarchyStringSplitBySpace = "ROLE_HIGHEST > ROLE_HIGHER ROLE_HIGHER > ROLE_LOW ROLE_LOW > ROLE_LOWER";
		String roleHierarchyStringSplitByWhatever = "ROLE_HIGHEST > ROLE_HIGHER xxx ROLE_HIGHER > ROLE_LOW xxx ROLE_LOW > ROLE_LOWER";

		List<String> roleHierarchyStrings = new LinkedList<String>();
		roleHierarchyStrings.add(roleHierarchyStringSplitByAnd);
		roleHierarchyStrings.add(roleHierarchyStringSplitByOr);
		roleHierarchyStrings.add(roleHierarchyStringSplitBySpace);
		roleHierarchyStrings.add(roleHierarchyStringSplitByWhatever);

		for (String roleHierarchyString : roleHierarchyStrings) {
			Matcher roleHierarchyMatcher = pattern.matcher(roleHierarchyString);
			if (!roleHierarchyMatcher.find()) {
				throw new RuntimeException("I'm dead. X﹏X");
			}
		}
		System.out.println("All pass");
	}

All pass

Process finished with exit code 0

That is to say, in the version 4.2.2 or the adjacent version, the "RoleHierarchy" comment is neither an error nor a correct one, XD.

/**
 * The simple interface of a role hierarchy.
 *
 * @author Michael Mayr
 */
public interface RoleHierarchy {

	/**
	 * Returns an array of all reachable authorities.
	 * <p>
	 * Reachable authorities are the directly assigned authorities plus all authorities
	 * that are (transitively) reachable from them in the role hierarchy.
	 * <p>
	 * Example:<br>
	 * Role hierarchy: ROLE_A &gt; ROLE_B and ROLE_B &gt; ROLE_C.<br>
	 * Directly assigned authority: ROLE_A.<br>
	 * Reachable authorities: ROLE_A, ROLE_B, ROLE_C.
	 *
	 * @param authorities - List of the directly assigned authorities.
	 * @return List of all reachable authorities given the assigned authorities.
	 */
	public Collection<? extends GrantedAuthority> getReachableGrantedAuthorities(
			Collection<? extends GrantedAuthority> authorities);

}

[since1986]

It should be said that the current version of "role inheritance" is more rigorous than before, I think it would be better if the "RoleHierarchy" comment is more rigorous. I will submit a PR.

[rwinch]

Thanks for the clarification and PR! This is now in master