Add AuthorizationManager #8996

evgeniycheban · 2020-09-07T01:36:27Z

jzheaux

Thanks, @evgeniycheban, this is going in the right direction. I've left a couple of comments inline.

...va/org/springframework/security/web/access/RequestMatcherDelegatingAuthorizationManager.java

web/src/main/java/org/springframework/security/web/access/IpAddressAuthorizationManager.java

jzheaux

Great progress, @evgeniycheban, it's nice to see this starting to take shape. I've left some additional feedback inline.

...ringframework/security/config/annotation/web/configurers/AbstractInterceptUrlConfigurer.java

jzheaux · 2020-10-05T16:44:07Z

...src/main/java/org/springframework/security/access/intercept/AbstractSecurityInterceptor.java

@@ -115,6 +118,8 @@

 	private AccessDecisionManager accessDecisionManager;


Let's remove this reference once it's not used anymore.

It might make sense to consider introducing a new filter that will use an AuthorizationManager similar to AuthorizationWebFilter.

Agreed that it's worth considering. It doesn't change the fact that AbstractSecurityInterceptor needs to stop using AccessDecisionManager, but it would probably be worth it to file a ticket and start that conversation.

...src/main/java/org/springframework/security/access/intercept/AbstractSecurityInterceptor.java

jzheaux · 2020-10-06T22:32:54Z

...in/java/org/springframework/security/authorization/AffirmativeBasedAuthorizationManager.java

+ */
+public final class AffirmativeBasedAuthorizationManager<T> implements AuthorizationManager<T> {
+
+	private final List<AccessDecisionVoter<?>> decisionVoters;


I think the ideal would be to initialize instances of AuthorizationManager with the correct security metadata instead of retrieving the security metadata each time an authentication is being checked.

Instead of routing the rules configured in the DSL through the ConfigAttribute API, have you already considered having the DSL construct a delegating AuthorizationManager that is composed of delegates that already have the security metadata?

I think this implementation requires less changes and we can reuse voters.
I'm not sure how this can be implemented for Method Security, the AffirmativeBasedAuthorizationManager is also used in GlobalMethodSecurityConfiguration.

While I agree that this might be less change, we don't want to introduce a public API that continues to have the same level of complexity as the existing API. The primary goal of this PR is to simplify the authorization API. Generally speaking, we ought to, going forward, avoid using the ConfigAttribute approach.

As you mentioned, this is more deeply embedded in method security. I wonder if this class could be package-private to method security configuration, and we can introduce another ticket to continue to rework method security.

I forget that AffirmativeBasedAuthorizationManager is also used for Web Socket configuration in AbstractSecurityWebSocketMessageBrokerConfigurer, maybe for now we can use an adapter for AccessDecisionManager and rework Method Security and Web Socket configurations in another ticket?

core/src/main/java/org/springframework/security/authorization/AuthorizationManager.java

...ringframework/security/config/annotation/web/configurers/AbstractInterceptUrlConfigurer.java

jzheaux · 2020-12-11T00:15:55Z

Thanks, @evgeniycheban! Let's now have @rwinch take a look as I anticipate that he will also have some feedback.

rwinch

Thanks for the PR @evgeniycheban! I'm really happy how this turned out.

I think we should revisit #8996 (comment)

If it is a bug, then I think we should create a separate ticket and commit with tests for it. This makes it easier for users to track changes (that it changed and why it changed). It also makes it easier for us to backport bug fixes.

rwinch · 2020-11-11T16:51:59Z

core/src/main/java/org/springframework/security/authorization/AuthorizationManager.java

+	/**
+	 * Determines if access should be granted for a specific authentication and object.
+	 * @param authentication the {@link Supplier} of the {@link Authentication} to check
+	 * @param object the {@link T} object to check
+	 * @return true if access is granted, false if denied
+	 */
+	default boolean isGranted(Supplier<Authentication> authentication, T object) {
+		AuthorizationDecision decision = check(authentication, object);
+		return decision == null || decision.isGranted();
+	}
+


Suggested change

/**

* Determines if access should be granted for a specific authentication and object.

* @param authentication the {@link Supplier} of the {@link Authentication} to check

* @param object the {@link T} object to check

* @return true if access is granted, false if denied

*/

default boolean isGranted(Supplier<Authentication> authentication, T object) {

AuthorizationDecision decision = check(authentication, object);

return decision == null || decision.isGranted();

}

I'd prefer to leave this out to keep the interface as simple as possible and provide better symmetry to ReactiveAuthorizationManager. We can always add it later if necessary since it would be passive.

This is an old diff, please see the last (62283faa) commit.

core/src/main/java/org/springframework/security/authorization/AuthorizationDecision.java

rwinch · 2020-11-11T17:11:32Z

.../src/main/java/org/springframework/security/authorization/AuthorityAuthorizationManager.java

+
+	private final Collection<String> authorities;
+
+	public AuthorityAuthorizationManager(Collection<String> authorities) {


I think it might be better to make the constructor package scope and add some static factory methods like AuthorityReactiveAuthorizationManager does.

This is an old diff, please see the last (62283fa) commit.

evgeniycheban · 2020-12-15T22:02:19Z

@rwinch I reverted changes for MvcRequestMatcher and AntPathRequestMatcher.

Closes spring-projectsgh-8900

evgeniycheban · 2020-12-16T13:49:07Z

@rwinch Currently, MvcRequestMatcher.matcher doesn't check HttpMethod and servletPath like MvcRequestMatcher.matches does. Should I create a separate ticket for this or leave my changes in this PR?

jzheaux · 2020-12-16T16:28:04Z

@evgeniycheban If the changes aren't required for this PR, then it's best to file a separate ticket, especially since the changes aren't related to the ticket that this PR is resolving.

The reason for that is it simplifies backporting bug fixes to other branches. Also, it makes release notes clearer for users that are affected by the bugs as they'll see a separate line item in the notes.

jzheaux · 2020-12-16T22:12:14Z

@evgeniycheban, I added #9284 and #9285 to address MvcRequestMatcher and AntPathRequestMatcher, respectively.

jzheaux · 2020-12-16T23:02:47Z

Thanks for all your work on this PR, @evgeniycheban! It's very exciting.

In addition to the above two tasks, I've created #9286, #9287, and #9288 for other improvements that we identified during the PR.

evgeniycheban marked this pull request as draft September 7, 2020 01:36

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 7, 2020

evgeniycheban force-pushed the gh-8900 branch 3 times, most recently from 0030029 to 3011ece Compare September 8, 2020 11:19

jzheaux requested changes Sep 11, 2020

View reviewed changes

...va/org/springframework/security/web/access/RequestMatcherDelegatingAuthorizationManager.java Outdated Show resolved Hide resolved

web/src/main/java/org/springframework/security/web/access/IpAddressAuthorizationManager.java Outdated Show resolved Hide resolved

evgeniycheban force-pushed the gh-8900 branch 4 times, most recently from 2275b04 to c006f21 Compare September 14, 2020 19:19

evgeniycheban force-pushed the gh-8900 branch 11 times, most recently from 036bff5 to 5ae5b68 Compare October 5, 2020 15:39

evgeniycheban marked this pull request as ready for review October 5, 2020 15:40

evgeniycheban force-pushed the gh-8900 branch from 5ae5b68 to a280ffd Compare October 5, 2020 16:12

evgeniycheban requested a review from jzheaux October 5, 2020 16:44

evgeniycheban force-pushed the gh-8900 branch 5 times, most recently from 3528c21 to b4ad09b Compare October 6, 2020 02:06

jzheaux requested changes Oct 7, 2020

View reviewed changes

evgeniycheban force-pushed the gh-8900 branch 4 times, most recently from 04925c1 to 8b929ee Compare December 5, 2020 16:20

rwinch self-requested a review December 7, 2020 20:49

evgeniycheban force-pushed the gh-8900 branch from 8b929ee to 5e6d080 Compare December 9, 2020 16:40

rwinch mentioned this pull request Dec 9, 2020

Expose *InvocationExpressionAttribute classes #9206

Closed

evgeniycheban force-pushed the gh-8900 branch from 5e6d080 to 62283fa Compare December 15, 2020 13:32

jzheaux approved these changes Dec 15, 2020

View reviewed changes

rwinch requested changes Dec 15, 2020

View reviewed changes

evgeniycheban force-pushed the gh-8900 branch from 62283fa to 14c01a4 Compare December 15, 2020 21:59

Add AuthorizationManager

5a6f74f

Closes spring-projectsgh-8900

evgeniycheban force-pushed the gh-8900 branch from 14c01a4 to 5a6f74f Compare December 15, 2020 22:22

evgeniycheban requested a review from rwinch December 15, 2020 22:24

This was referenced Dec 16, 2020

Consider Aligning MvcRequestMatcher's matching methods #9284

Closed

Reconsider AntPathRequestMatcher matching logic #9285

Closed

jzheaux merged commit 34b4b10 into spring-projects:master Dec 16, 2020

jzheaux self-assigned this Dec 17, 2020

jzheaux added in: core An issue in spring-security-core type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Dec 17, 2020

jzheaux added this to the 5.5.0-M2 milestone Dec 17, 2020

jzheaux mentioned this pull request Dec 17, 2020

Add Reasons to AuthorizationDecisions #9287

Closed

jzheaux mentioned this pull request Mar 12, 2021

HttpSecurityConfigurer should have a no-parameter method for authorizeHttpRequests #9498

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AuthorizationManager #8996

Add AuthorizationManager #8996

evgeniycheban commented Sep 7, 2020 •

edited

Loading

jzheaux left a comment

jzheaux left a comment

jzheaux Oct 5, 2020

evgeniycheban Oct 11, 2020

jzheaux Oct 16, 2020

jzheaux Oct 6, 2020

evgeniycheban Oct 8, 2020

jzheaux Oct 8, 2020

evgeniycheban Oct 9, 2020 •

edited

Loading

jzheaux commented Dec 11, 2020

rwinch left a comment

rwinch Nov 11, 2020

evgeniycheban Dec 15, 2020

rwinch Nov 11, 2020

evgeniycheban Dec 15, 2020

evgeniycheban commented Dec 15, 2020

evgeniycheban commented Dec 16, 2020

jzheaux commented Dec 16, 2020

jzheaux commented Dec 16, 2020

jzheaux commented Dec 16, 2020

		@@ -115,6 +118,8 @@

		private AccessDecisionManager accessDecisionManager;


		private final Collection<String> authorities;

		public AuthorityAuthorizationManager(Collection<String> authorities) {

Add AuthorizationManager #8996

Add AuthorizationManager #8996

Conversation

evgeniycheban commented Sep 7, 2020 • edited Loading

jzheaux left a comment

Choose a reason for hiding this comment

jzheaux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evgeniycheban Oct 9, 2020 • edited Loading

Choose a reason for hiding this comment

jzheaux commented Dec 11, 2020

rwinch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evgeniycheban commented Dec 15, 2020

evgeniycheban commented Dec 16, 2020

jzheaux commented Dec 16, 2020

jzheaux commented Dec 16, 2020

jzheaux commented Dec 16, 2020

evgeniycheban commented Sep 7, 2020 •

edited

Loading

evgeniycheban Oct 9, 2020 •

edited

Loading