Skip to content

Add AuthorizationManager #8900

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
rwinch opened this issue Aug 3, 2020 · 8 comments · Fixed by #8996
Closed

Add AuthorizationManager #8900

rwinch opened this issue Aug 3, 2020 · 8 comments · Fixed by #8996
Assignees
Labels
in: core An issue in spring-security-core type: enhancement A general enhancement

Comments

@rwinch
Copy link
Member

rwinch commented Aug 3, 2020

We should add an AuthorizationManager which is an imperative version of ReactiveAuthorizationManager. The class should look something like:

public interface AuthorizationManager<T> {
	AuthorizationDecision check(Supplier<Authentication> authentication, T object);

	default void verify(Supplier<Authentication> authentication, T object) {
		AuthorizationDecision decision = check(authentication, object);
		if (!decision.isGranted()) {
			throw new AccessDeniedException("Access Denied");
		}
	}
}

Using something that allows delaying looking up the Authentication like Supplier<Authentication> vs an Authentication directly.

We should also add support for AuthorizationManager in HttpSecurity.authorizeRequests().

Finally, we should change around the existing classes that use AccessDecisionManager should migrate to AuthorizationManager and AccessDecisionManager should be marked as deprecated.

@evgeniycheban
Copy link
Contributor

@rwinch I would like to work on this.

@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 Aug 31, 2020
@jzheaux
Copy link
Contributor

jzheaux commented Aug 31, 2020

It's yours, @evgeniycheban

@jzheaux jzheaux changed the title AuthorizationManager Add AuthorizationManager Aug 31, 2020
@evgeniycheban
Copy link
Contributor

@jzheaux I created a very draft PR.

Please take a look when you have a moment. Am I moving in the right direction?

@evgeniycheban
Copy link
Contributor

evgeniycheban commented Sep 14, 2020

Thanks for the review @jzheaux

@rwinch @jzheaux I have a few questions.

  1. Should we implement an AuthorizationManager for Method Security within this task?
  2. By default, HttpSecurity.authorizeRequests() uses ExpressionUrlAuthorizationConfigurer, should we add an AuthorizationManager that supports SpEL expressions?

I've currently added support for AuthorizationManager to UrlAuthorizationConfigurer in the c006f21

@jzheaux
Copy link
Contributor

jzheaux commented Sep 16, 2020

Good questions, @evgeniycheban.

  1. Should we implement an AuthorizationManager for Method Security within this task?

One of the goals of this ticket is to deprecate AccessDecisionManager. Given that, we should see if we can eliminate any internal use of it and deprecate any public use.

Ideally, then, AbstractSecurityInterceptor would stop referring to the AccessDecisionManager interface in favor of AuthorizationManager.

Since the public API allows an application to configure an AccessDecisionManager, an AuthorizationManager implementation that adapts to an AccessDecisionManager may be helpful.

One of the tricky parts here seems to be how to deal with the SecurityMetadataSource and its associated ConfigAttributes. Since ConfigAttribute isn't part of AuthorizationManager's contract, this adapter might need to have a SecurityMetadataSource. I also wonder what effect this should have on the events that are fired since some of them depend on knowing the list of ConfigAttributes. What do you think, @rwinch?

  1. By default, HttpSecurity.authorizeRequests() uses ExpressionUrlAuthorizationConfigurer, should we add an AuthorizationManager that supports SpEL expressions?

Yes, I think that makes sense, though I think it should be internal to that class.

@evgeniycheban
Copy link
Contributor

evgeniycheban commented Oct 4, 2020

@jzheaux I've implemented an adapter, but we need to check that an AccessDecisionManager supports a ConfigAttribute in AbstractSecurityInterceptor.validateAttributeDefs. Is this validation still needed?
The adapter code is shown below.

AuthorizationManagerAdapter
public class AuthorizationManagerAdapter<T> implements AuthorizationManager<T> {

	private final AccessDecisionManager accessDecisionManager;

	private final SecurityMetadataSource metadataSource;

	public AuthorizationManagerAdapter(AccessDecisionManager accessDecisionManager,
			SecurityMetadataSource metadataSource) {
		this.accessDecisionManager = accessDecisionManager;
		this.metadataSource = metadataSource;
	}

	@Override
	public AuthorizationDecision check(Supplier<Authentication> authentication, T object) {
		Collection<ConfigAttribute> attributes = this.metadataSource.getAttributes(object);
		try {
			this.accessDecisionManager.decide(authentication.get(), object, attributes);
		}
		catch (AccessDeniedException e) {
			return new AuthorizationDecision(false);
		}
		return new AuthorizationDecision(true);
	}

}

evgeniycheban added a commit to evgeniycheban/spring-security that referenced this issue Dec 15, 2020
jzheaux pushed a commit that referenced this issue Dec 16, 2020
jzheaux added a commit that referenced this issue Dec 16, 2020
@winer77444
Copy link

I added it at the gateway ,However, you can only enter the check method when adding the authentication header,If it is not added, the null pointer exception will be reported if it is forwarded to the corresponding service through the gateway. What's the matter

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
Development

Successfully merging a pull request may close this issue.

4 participants