Add support for authorization events in DelegatingAuthorizationManager#9527
Add support for authorization events in DelegatingAuthorizationManager#9527parikshitdutta wants to merge 1 commit into
Conversation
8b2135a to
a0695d5
Compare
|
@jzheaux one thing I am possibly missing out is configuring the Please help me understand that as well, as you share your feedback on other things. |
jzheaux
left a comment
There was a problem hiding this comment.
Thanks, @parikshitdutta! Sorry that it took some time to get back to this.
In addition to publishing an event in AuthorizationFilter, I think the same events should be published in AuthorizationManagerBeforeMethodInterceptor and AuthorizationManagerAfterMethodInteceptor. Would you be able to add that as well including tests?
| * @author Parikshit Dutta | ||
| * @since 5.5 | ||
| */ | ||
| public interface AuthorizationEventPublisher { |
There was a problem hiding this comment.
I think this interface should be left out for now. Instead, I think that classes can use ApplicationEventPublisher.
| */ | ||
| public class AuthorizationFailureEvent extends ApplicationEvent { | ||
|
|
||
| public AuthorizationFailureEvent(AuthorizationDecision authorizationDecision) { |
There was a problem hiding this comment.
I think this should be generically typed like AuthorizationManager is. Also, I think it should be modeled after the former AuthorizationFailureEvent with some changes:
public class AuthorizationFailureEvent<T> extends ApplicationEvent {
// ...
public AuthorizationFailureEvent(Supplier<Authentication> authentication, T object, AuthorizationDecision decision)
// ...
}My motivation for the ordering is that it matches the ordering for AuthorizationManager#check.
| * @author Parikshit Dutta | ||
| * @since 5.5 | ||
| */ | ||
| public class AuthorizationSuccessEvent extends ApplicationEvent { |
There was a problem hiding this comment.
I think this should be generically typed like AuthorizationManager is. Also, I think it should be modeled after the former AuthorizedEvent with some changes:
public class AuthorizationSuccessEvent<T> extends ApplicationEvent {
// ...
public AuthorizationSuccessEvent(Supplier<Authentication> authentication, T object, AuthorizationDecision decision)
// ...
}My motivation for the ordering is that it matches the ordering for AuthorizationManager#check.
| this.logger.trace(LogMessage.format("Checking authorization on %s using %s", request, manager)); | ||
| } | ||
| return manager.check(authentication, | ||
| AuthorizationDecision authorizationDecision = manager.check(authentication, |
There was a problem hiding this comment.
Since authorization managers can be reasonably composed, I think it would be better to publish the event in AuthorizationFilter instead. This would also mean changing AuthorizationFilter to call AuthorizationManager#check instead of #verify.
This way, if a custom authorization manager or a composite authorization manager is used, we don't risk multiple events or no events firing.
|
Hi @parikshitdutta, have you had the chance to look at the feedback? |
|
@parikshitdutta, thanks for your contribution! This is now merged into |
added AuthorizationSuccessEvent to represent event for granted AuthorizationDecision
added AuthorizationFailureEvent to represent event for denied AuthorizationDecision
added AuthorizationEventPublisher to represent generic publisher for raising authorization events
added default implementation [DefaultAuthorizationEventPublisher] for AuthorizationEventPublisher
added DefaultAuthorizationEventPublisherTests to handle respective test scenarios
updated DelegatingAuthorizationManager to add AuthorizationEventPublisher to raise authorization events
updated DelegatingAuthorizationManagerTests to add respective test scenarios
Closes #9288 and #9286