Project

General

Profile

Actions

Task #845

closed

Last successful and unsuccessful attempts for IdmPassword

Added by Ondřej Kopr about 7 years ago. Updated almost 7 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Petr Hanák
Category:
Password
Target version:
Start date:
11/22/2017
Due date:
% Done:

100%

Estimated time:
Owner:

Description

Please add two new attribute for IdmPasswordDto and IdmPassword:
  • 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)

Actions #1

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.

Actions #2

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

Actions #4

Updated by Ondřej Kopr about 7 years ago

  • Status changed from In Progress to Needs feedback
Actions #5

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,
please fix:
  • 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.

Actions #6

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?

Actions #7

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!

Actions #8

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.

Actions #9

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.

Actions

Also available in: Atom PDF