Task #845
closedLast successful and unsuccessful attempts for IdmPassword
100%
Description
- lastSuccessLogin,
- unsuccessfulAttemps.
when identity success login to IdM (any authenticator) set new datetime to lastSuccessLogin and also set zero to unsuccessfulAttemps, after unsuccessful login attemp increment unsuccessfulAttemps. No next logic not needed yet. Just set these two attributes.
(edit frontend not needed)
Updated by Petr Hanák about 7 years ago
- Subject changed from Last success and unsuccessful attempts for IdmPassword to Last successful and unsuccessful attempts for IdmPassword
- Assignee changed from Petr Hanák to Ondřej Kopr
Password is verified in method authenticateOverAuthenticator in DefaultAutenticationManager,
then it depends on results..
If password is incorrect, it'll call passwordService method for increasing count of attempts in IdMPasswordDTO.
If the password is correct, It will call passwordService method for setting new timestamp in IdMPasswordDTO and will reset counting of attempts to 0.
Updated by Ondřej Kopr about 7 years ago
- Status changed from New to In Progress
- Assignee changed from Ondřej Kopr to Petr Hanák
- % Done changed from 0 to 90
Please add branch and if it is possible also commits. Thanks
Updated by Petr Hanák about 7 years ago
- Assignee changed from Petr Hanák to Ondřej Kopr
branch:
https://github.com/bcvsolutions/CzechIdMng/commits/phanak/845-password-attemp-counter
commits:
https://github.com/bcvsolutions/CzechIdMng/commit/763f215628ee03b9666dafbae6ab5472fd2df06a
https://github.com/bcvsolutions/CzechIdMng/commit/94fbe1e2c9f2c4a834c9a5404ed5e34520fc4874
https://github.com/bcvsolutions/CzechIdMng/commit/25ce6b3076c7cdb832a28aeaf362c548a2a2908b
https://github.com/bcvsolutions/CzechIdMng/commit/acde08d5c1b7c9f3636460dea5bd146d807271f0
Updated by Ondřej Kopr about 7 years ago
- Status changed from In Progress to Needs feedback
Updated by Ondřej Kopr about 7 years ago
- Status changed from Needs feedback to In Progress
- Assignee changed from Ondřej Kopr to Petr Hanák
- % Done changed from 90 to 70
I made first feedback:
things that is already done:- merged actual develop to your branch and renamed V1_00_034__add-login-count-and-date.sql to V7_07_003__add-login-count-and-date.sql,
- audit table hasn't any default value and not null constraint i removed it,
- method private IdmPasswordDto getPasswordByIdentity(String username) is probably useless, you can remove it,
- check your commit before you add it into branch, you created unnecessary commit like: https://github.com/bcvsolutions/CzechIdMng/commit/5d8b2cc6cee057031042f75b785985ba18f88b27 (don't correct this, only for next time)
- test missing,
- null pointer when login, or change password for identity that hasn't password in idm (some identities hasn't password in idm and login trough some system for example ldap) you can probably skip your new behavior for these identities,
java.lang.NullPointerException: null at eu.bcvsolutions.idm.core.model.service.impl.DefaultIdmPasswordService.increaseUnsuccessfulAttempts(DefaultIdmPasswordService.java:128) at eu.bcvsolutions.idm.core.model.service.impl.DefaultIdmPasswordService$$FastClassBySpringCGLIB$$4f6face1.invoke(<generated>) at org.springframework.cglib.proxy.MethodProxy.invoke(MethodProxy.java:204) at org.springframework.aop.framework.CglibAopProxy$DynamicAdvisedInterceptor.intercept(CglibAopProxy.java:651) at eu.bcvsolutions.idm.core.model.service.impl.DefaultIdmPasswordService$$EnhancerBySpringCGLIB$$f908b07.increaseUnsuccessfulAttempts(<generated>) at eu.bcvsolutions.idm.core.model.service.impl.DefaultAuthenticationManager.authenticateOverAuthenticator(DefaultAuthenticationManager.java:102) at eu.bcvsolutions.idm.core.model.service.impl.DefaultAuthenticationManager.authenticate(DefaultAuthenticationManager.java:48) at eu.bcvsolutions.idm.core.rest.impl.PasswordChangeController.passwordChange(PasswordChangeController.java:106)
Behavior with increment and set last successful date works perfectly, please fix these issues and give me ticket back for second feedback.
Updated by Petr Hanák almost 7 years ago
- Status changed from In Progress to Needs feedback
- Assignee changed from Petr Hanák to Ondřej Kopr
Commits:
https://github.com/bcvsolutions/CzechIdMng/commit/6d45088e91f502c7bf41bf68133a15a25bb38959
https://github.com/bcvsolutions/CzechIdMng/commit/ab8b100e76d43d0c1294042d0b85b8d3ee13ceda
Not sure about deleting method private IdmPasswordDto getPasswordByIdentity(String username).
It's the only way how I can find password during login in in IdM or?
Updated by Ondřej Kopr almost 7 years ago
- Status changed from Needs feedback to In Progress
- Assignee changed from Ondřej Kopr to Petr Hanák
- % Done changed from 70 to 80
I merged develop with your branch and resolve conflicts.
- with method getPasswordByIdentity I mean, you can remove it and add this attribute (username) to filter, but let it be as it, at least please change method name (getPasswordByIdentity) to getPasswordByIdentityUsername and add javadoc. For method (findOneByIdentity_username) in repository add also javadoc,
- you set last successful login only when authenticator return sufficient, please add this set also after iterate over all authenticator,
- null pointer isn't throws, but you little bit changed behavior methods increaseUnsuccessfulAttempts and setLastSuccessfulLogin please update their javadoc.
All these issues is only minor, please after you fix it add this thicket to Radek for last review.
Thank you for awesome feature.
Behavior with increment and set date works awesome!
Updated by Petr Hanák almost 7 years ago
- Status changed from In Progress to Needs feedback
- Assignee changed from Petr Hanák to Radek Tomiška
Commit:
https://github.com/bcvsolutions/CzechIdMng/commit/377d222d816ec102d85a7b255301a3f48559021e
I've edited javadocs and added setter of successful login timestamp to authenticateOverAuthenticator method.
Updated by Radek Tomiška almost 7 years ago
- Status changed from Needs feedback to Closed
- Assignee changed from Radek Tomiška to Petr Hanák
- Target version set to Garnet (7.8.0)
- % Done changed from 80 to 100
Thx for this new feature, it works and code looks nice, good job!
I changed only minors:
- some hard coded identities in integration tests,
- add clearing unsuccessful attempts, when password is changed (e.g. by admin), integration test is added.
commit:
https://github.com/bcvsolutions/CzechIdMng/commit/906c75115804cdcbbacfc7b06ba0814b85b5bca1
Merged into develop.