-
Notifications
You must be signed in to change notification settings - Fork 6k
Option for default event in DefaultAuthenticationEventPublisher #7937
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, @zeeshanadnan! I've left some feedback inline.
@Test | ||
public void defaultAuthenticationFailureEventIsPublished() { | ||
publisher = new DefaultAuthenticationEventPublisher(); | ||
Properties p = new Properties(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these lines necessary that set up the additional exception mappings? I'm thinking that lines 149 thru 152 can be removed.
}, mock(Authentication.class)); | ||
verify(appPublisher).publishEvent(isA(AuthenticationFailureBadCredentialsEvent.class)); | ||
} | ||
|
||
private static final class MockAuthenticationException extends |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you please add a test for a class that is missing the appropriate constructor? That will make sure to test that the exception is thrown appropriately.
Hi @jzheaux added your change requests into a new commit. |
Thanks @zeeshanadnan for the changes. In preparation for merging, will you please squash your commits and change the commit message to match the contribution guidelines? |
@jzheaux Done |
Thanks, @zeeshanadnan, for the PR! This is now merged into Note that I polished the commit message just a bit, though it will still show you as the contributor. I added a small polish via 653400e. |
…Publisher
Fixes gh-7825