-
Notifications
You must be signed in to change notification settings - Fork 151
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
Debug login problems #252
base: master
Are you sure you want to change the base?
Debug login problems #252
Conversation
@@ -77,7 +84,12 @@ | |||
<appender-ref ref="main"/> | |||
</category> | |||
|
|||
<category name="br.com.caelum.vraptor.simplemail.aws"> | |||
<category name="org.mamute.auth"> | |||
<priority value="DEBUG"/> |
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.
should this be DEBUG by default?
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.
@cpfeiffer change here to info and debug
I like this change in general - I had some login issues myself due to connection problems to our LDAP server. |
@@ -227,7 +228,7 @@ private void updateAvatarImage(LDAPResource ldap, Entry entry, User user) { | |||
private void createUserIfNeeded(LDAPResource ldap, String cn) throws LdapException { | |||
Entry ldapUser = ldap.getUser(cn); | |||
String email = ldap.getAttribute(ldapUser, emailAttr); | |||
User user = users.findByEmail(email); | |||
User user = email != null ? users.findByEmail(email) : null; |
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.
Add some unit tests to this case that email is null
@cpfeiffer can you add the unit tests that I mentioned up there? |
Yes, sorry, I will eventually do it, I'm just too busy with other things at the moment. I had to debug login problems at work and needed to fnid out the actual ldap error cause, that's why I quickly updated the patch. |
No description provided.