Skip to content

Consider replacing an inner loop with Set of authority strings in AuthorityAuthorizationManager#isAuthorized #11188

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
evgeniycheban opened this issue May 8, 2022 · 1 comment · Fixed by #11189
Assignees
Labels
in: core An issue in spring-security-core type: enhancement A general enhancement
Milestone

Comments

@evgeniycheban
Copy link
Contributor

Currently AuthorityAuthorizationManager#isAuthorized uses an inner loop to determine if user has required authority:

private boolean isAuthorized(Authentication authentication) {
    for (GrantedAuthority grantedAuthority : authentication.getAuthorities()) {
	for (GrantedAuthority authority : this.authorities) {
	    if (authority.getAuthority().equals(grantedAuthority.getAuthority())) {
		return true;
            }
	}
    }
    return false;
}

It can be replaced with a Set of authority strings which will be more efficient because HashSet has constant lookup time O(1).

@evgeniycheban evgeniycheban added status: waiting-for-triage An issue we've not yet triaged type: enhancement A general enhancement labels May 8, 2022
evgeniycheban added a commit to evgeniycheban/spring-security that referenced this issue May 9, 2022
jzheaux pushed a commit that referenced this issue May 9, 2022
jzheaux pushed a commit that referenced this issue May 9, 2022
@jzheaux jzheaux closed this as completed in dbd96a9 May 9, 2022
@jzheaux jzheaux self-assigned this May 9, 2022
@jzheaux jzheaux added in: core An issue in spring-security-core and removed status: waiting-for-triage An issue we've not yet triaged labels May 9, 2022
@jzheaux jzheaux added this to the 5.7.0 milestone May 9, 2022
@evgeniycheban
Copy link
Contributor Author

@jzheaux I've just found AuthorityUtils#authorityListToSet method that does the same I did in getAuthoritySet, can I open another PR that will add this minor change?

evgeniycheban added a commit to evgeniycheban/spring-security that referenced this issue May 10, 2022
sjohnr pushed a commit that referenced this issue May 12, 2022
sjohnr pushed a commit that referenced this issue May 12, 2022
sjohnr pushed a commit that referenced this issue May 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core An issue in spring-security-core type: enhancement A general enhancement
Projects
None yet
2 participants