search

spring-attic / spring-security-javaconfig

Spring Security Java Configuration Support (to be merged with spring-security-config)
175 stars 121 forks source link

Add support for change AuthenticationEventPublisher #132

Closed ralscha closed 11 years ago

ralscha commented 11 years ago

If I'm not mistaken the AuthenticationManagerBuilder has no way to set an AuthenticationEventPublisher. Unfortunately the default in ProviderManager is NullEventPublisher that does not send any events.

dsyer commented 11 years ago

I foresee a problem that the AuthenticationManager is not necessarily a ProviderManager. My approach has been to @Autowired a ProviderManager and then inject the publisher there. Maybe that is better anyway?

dsyer commented 11 years ago

(Oops didn't mean to close.)

ralscha commented 11 years ago

But the AuthenticationManagerBuilder always creates a ProviderManager in the performBuild method. Could the library not simply support the EventPublisher like the eraseCredentials property.

public AuthenticationManagerBuilder authenticationEventPublisher(AuthenticationEventPublisher eventPublisher) { this.eventPublisher= eventPublisher; return this; }

and in the performBuild method something like this.

ProviderManager providerManager = new ProviderManager(authenticationProviders, parentAuthenticationManager); if(eraseCredentials != null) {

providerManager.setEraseCredentialsAfterAuthentication(eraseCredentials); } if (eventPublisher != null) { providerManager.setAuthenticationEventPublisher(eventPublisher); } return providerManager;

On Mon, Jun 24, 2013 at 10:41 AM, Dave Syer notifications@github.comwrote:

(Oops didn't mean to close.)

— Reply to this email directly or view it on GitHubhttps://github.com/SpringSource/spring-security-javaconfig/issues/132#issuecomment-19895162 .

rwinch commented 11 years ago

I think the reality is that as soon as we introduce the add(AuthenticationProvider) we are already a bit locked into ProviderManager implementation for the AuthenticationManagerBuilder so I went ahead and added a method for setting the AuthenticationEventPublisher. When using the WebSecurityConfigurerAdapter's registerAuthentication method it defaults to DefaultAuthenticationEventPublisher

@ralscha can you please verify this resolves your issue and then close it out or provide additional details?

ralscha commented 11 years ago

It does work now. There is one tiny problem that the AuthenticationFailureBadCredentialsEvent is sent twice if I try to login with an unknown user. Not sure if that is the correct behavior.

First the parent is sending the event in Line 174 and then the ProviderManager itself is sending the event again.

ProviderManager Line 174: result = parent.authenticate(authentication); and ProviderManager Line 200: prepareException(lastException, authentication);

rwinch commented 11 years ago

@ralscha Thanks for catching this :) I have updated the code to only set the publisher on the parent which is consistent with the XML namespace. You will see the test is more precise to ensure that only a single event is fired too. Can you please try out the changes to ensure that everything works properly? If they work well, please close this ticket out. If not, please elaborate on what is not working. Thanks again!

ralscha commented 11 years ago

Everything works fine now. Events are published only once. Thanks for your effort