LDAP cahcing #288
LDAP cahcing #288
Conversation
|
|
||
| protected void additionalAuthenticationChecks(UserDetails userDetails, | ||
| UsernamePasswordAuthenticationToken authentication) { | ||
| if (StringUtils.isEmpty(authentication.getCredentials())) { |
There was a problem hiding this comment.
I'd simplify it to something like follows
credentials = authentication.getCredentials();
if (!StrubgUtils.isEmpty(credentials)) {
password = credentials.toString();
if (userDetails.getPassword().equals(password))
// Password matches
return;
}
throw new BacCredentialsException(...
| } | ||
| String presentedPassword = authentication.getCredentials().toString(); | ||
| if (!StringUtils.isEmpty(userDetails.getPassword()) && (!presentedPassword.equals(userDetails.getPassword()))) { | ||
| System.out.println("Authentication failed: password does not match stored value"); |
There was a problem hiding this comment.
System.out... should be replaced by a logger
| @Bean | ||
| public UserCache userCache() { | ||
| // Adjust cache settings as necessary | ||
| return new SpringCacheBasedUserCache(cacheManager().getCache("authenticationCache")); |
There was a problem hiding this comment.
Should always a new instance be created? Is it just a wrapper?
There was a problem hiding this comment.
refactored to use a single isntance
| .userSearchFilter(userSearchFilter) | ||
| .contextSource(ldapContextSource()); | ||
|
|
||
| BindAuthenticator bindAuthenticator = new BindAuthenticator(ldapContextSource()); |
There was a problem hiding this comment.
ldapContextSource() is called twice: lines 140 and 141. Is it intention or could the same context be shared?
There was a problem hiding this comment.
refactored to avoid multiple calls
| @@ -64,6 +95,9 @@ public class SecurityConfig extends WebSecurityConfigurerAdapter { | |||
| @Value("${activedirectory.connectionTimeOut:#{127000}}") | |||
| private Integer ldapTimeOut = DEFAULT_LDAP_CONNECTION_TIMEOUT; | |||
|
|
|||
| @Value("${LdapCacheTTL}") | |||
| private Integer LdapCacheTTL; | |||
There was a problem hiding this comment.
Java conventions should be followed: start with lower-case letter.
| protected void configureGlobal(AuthenticationManagerBuilder auth) throws Exception { | ||
| @Bean | ||
| public UserCache userCache() { | ||
| if (cacheManager().getCache("authenticationCache") == null) { |
There was a problem hiding this comment.
Please, define a constant for "authenticationCache".
| @@ -64,6 +95,9 @@ public class SecurityConfig extends WebSecurityConfigurerAdapter { | |||
| @Value("${activedirectory.connectionTimeOut:#{127000}}") | |||
| private Integer ldapTimeOut = DEFAULT_LDAP_CONNECTION_TIMEOUT; | |||
|
|
|||
| @Value("${LdapCacheTTL}") | |||
There was a problem hiding this comment.
Is there a default TTL? I mean something like for ldapTimeOut, see line 95.
| public CacheManager cacheManager() { | ||
| CaffeineCacheManager cacheManager = new CaffeineCacheManager(); | ||
| cacheManager.setCaffeine(Caffeine.newBuilder() | ||
| .expireAfterWrite(LdapCacheTTL, TimeUnit.MINUTES)); |
There was a problem hiding this comment.
Should it be in minutes? All other timeouts are in seconds. It'll be very confused for users...
| @@ -64,6 +95,9 @@ public class SecurityConfig extends WebSecurityConfigurerAdapter { | |||
| @Value("${activedirectory.connectionTimeOut:#{127000}}") | |||
| private Integer ldapTimeOut = DEFAULT_LDAP_CONNECTION_TIMEOUT; | |||
|
|
|||
| @Value("${LdapCacheTTL}") | |||
There was a problem hiding this comment.
The property should be part of configuration file (including appropriate description).
| protected void configureGlobal(AuthenticationManagerBuilder auth) throws Exception { | ||
| @Bean | ||
| public UserCache userCache() { | ||
| if (cacheManager().getCache("authenticationCache") == null) { |
There was a problem hiding this comment.
Maybe I don't understand it, but
cacheManager().getCache("authenticationCache") == null
should alway be true. But maybe I don't understand something...
I was unable to run it in debugger. We should have a call and discuss that.
Applicable Issues
Description of the Change
Alternate Designs
Possible Drawbacks
Sign-off
Developer's Certificate of Origin 1.1
By making a contribution to this project, I certify that:
(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or
(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or
(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.
(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.
Signed-off-by: <!-- piyush.sadangi@tcs.com