Skip to content

AuthorityReactiveAuthorizationManager compares GrantedAuthority instances using Object.equals instead of comparing the String value returned by getAuthority() #10596

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ghost opened this issue Dec 8, 2021 · 2 comments
Labels
status: waiting-for-triage An issue we've not yet triaged type: bug A general bug

Comments

@ghost
Copy link

ghost commented Dec 8, 2021

Describe the bug
When using custom implementations of GrantedAuthority with a custom AuthenticationManager, they will not be authorized, even though they contain the correct authority.

While debugging this behavior I found the following method implementation in AuthorityReactiveAuthorizationManager. I added some extra comments.

	@Override
	public Mono<AuthorizationDecision> check(Mono<Authentication> authentication, T object) {
		return authentication.filter((a) -> a.isAuthenticated())
                                // Flat mapping to Flux<? extends GrantedAuthority>
				.flatMapIterable(Authentication::getAuthorities)
                                // Checking if this.authorities contains any value from the Flux.
				.any(this.authorities::contains)
				.map((granted) -> ((AuthorizationDecision) new AuthorityAuthorizationDecision(granted, this.authorities)))
				.defaultIfEmpty(new AuthorityAuthorizationDecision(false, this.authorities));
	}

The line .any(this.authorities::contains) actually uses Objects.equals() to compare the elements from the flux to the elements in this.authorities. Since GrantedAuthority is an interface the acutal behavior of equals() depends on the implementation. AuthorityReactiveAuthorizationManager uses SimpleGrantedAuthority as implementation for this.authorities, which implement equals() as follows and in a way that excludes other implementations of GrantedAuthority altogether.

	@Override
	public boolean equals(Object obj) {
		if (this == obj) {
			return true;
		}
		if (obj instanceof SimpleGrantedAuthority) {
			return this.role.equals(((SimpleGrantedAuthority) obj).role);
		}
		return false;
	}

To Reproduce

  1. Create own subclass of GrantedAuthority.
  2. Implement getAuthority() method, returing a String value of e.g. "SOMETHING".
  3. Implement custom AuthenticationManager that authenticates a request, returning an Authentication instance containing a GrantedAuthority with value "SOMETHING" and isAuthenticated() == true.
  4. Configure ServerHttpSecurity to authorize requests on some path to be only allowed with authority "SOMETHING"
.pathMatchers("/some-path/**").hasAuthority("SOMETHING")
  1. Try making a valid authenticated request.

Expected behavior
Requested should be authorized.

AuthorityReactiveAuthorizationManager should not compare instances of GrantedAuthority using Objects.equals by utilizing .any(this.authorities::contains), but instead compare the String values returned from getAuthority().

When using instances of SimpleGrantedAuthority for own authentications, the authorization works because of the specific implementation of equals() in that class.

Sample
None yet. If the description is not sufficient enough, I can provide a sample.

@ghost ghost added status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Dec 8, 2021
@terminux
Copy link
Contributor

terminux commented Dec 9, 2021

@ghost
Copy link
Author

ghost commented Dec 9, 2021

Thanks! Sorry about the duplicate. I guess I didn't remove the "issue:open" filter, when searching for existing issues regarding this problem.

@ghost ghost closed this as completed Dec 9, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting-for-triage An issue we've not yet triaged type: bug A general bug
Projects
None yet
Development

No branches or pull requests

1 participant