Skip to content

Conversation

parikshitdutta
Copy link
Contributor

@parikshitdutta parikshitdutta commented Mar 29, 2021

  • 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

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 29, 2021
@parikshitdutta parikshitdutta force-pushed the gh-9288 branch 4 times, most recently from 8b2135a to a0695d5 Compare March 30, 2021 12:10
@parikshitdutta parikshitdutta changed the title Add supprt for authorization events in DelegatingAuthorizationManager Add support for authorization events in DelegatingAuthorizationManager Mar 30, 2021
@parikshitdutta
Copy link
Contributor Author

@jzheaux one thing I am possibly missing out is configuring the DefaultAuthorizationEventPublisher, which I believe could be part of DelegatingAuthorizationManager.Builder and/or AuthorizeHttpRequestsConfigurer.

Please help me understand that as well, as you share your feedback on other things.

@jzheaux jzheaux self-assigned this Apr 5, 2021
Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -76,14 +80,36 @@ public AuthorizationDecision check(Supplier<Authentication> authentication, Http
if (this.logger.isTraceEnabled()) {
this.logger.trace(LogMessage.format("Checking authorization on %s using %s", request, manager));
}
return manager.check(authentication,
AuthorizationDecision authorizationDecision = manager.check(authentication,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jzheaux 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 Sep 30, 2021
@jzheaux jzheaux added the status: waiting-for-feedback We need additional information before we can continue label Nov 19, 2021
@jzheaux jzheaux removed their assignment Nov 19, 2021
@eleftherias
Copy link
Contributor

Hi @parikshitdutta, have you had the chance to look at the feedback?

@jzheaux jzheaux mentioned this pull request Mar 11, 2022
18 tasks
@jzheaux jzheaux self-assigned this Mar 29, 2022
@jzheaux jzheaux removed the status: waiting-for-feedback We need additional information before we can continue label Mar 29, 2022
@jzheaux jzheaux added this to the 5.7.0-RC1 milestone Mar 29, 2022
@jzheaux
Copy link
Contributor

jzheaux commented Mar 29, 2022

@parikshitdutta, thanks for your contribution!

This is now merged into 5.7.x and main. I also added some polish commits that add documentation, configuration support, and update the contracts according to the feedback specified in my original review.

@jzheaux jzheaux closed this Mar 29, 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
Development

Successfully merging this pull request may close these issues.

DelegatingAuthorizationManager Should Fire Events
4 participants