IdStory Identity Manager

modified processor, preValidate, extended on password policy advanced options,

tests completed,
testing:
acc processor and core processor
password policy - Basic (lenght, upper char, number, etc) and Advanced (min rules to ful fill and if password is similar to username, name, etc.)
todo - localization for "policiesNames" pre validation exception parametr

after consulting, I updated localization

please check my solution and write feedback

commits:
https://github.com/bcvsolutions/CzechIdMng/commit/206f8a1e6468c0212d6bdf681eccf63b76127e97
https://github.com/bcvsolutions/CzechIdMng/commit/361f53a2fbf1dfb2a98126ec534016e526f22dd6
https://github.com/bcvsolutions/CzechIdMng/commit/eaecd0e5373f99e5274fe9df6b7d16378ca51d02
https://github.com/bcvsolutions/CzechIdMng/commit/1f30458d63b84caa7b077326a95b6425bb15148c

I made first review:

• I already merged develop branch to your branch and resolve minor conflicts,
• I removed javadoc for overridden method, this javadoc is useless,

• method validateDefinition has invalid javadoc, two backslashes isn't doc comment, see javatpoint.com/java-comments,
• autowire another processor and call his method isn't probably good way, for me better solutions in your case is create abstract parent for this two processors or just separate this method to password policy service, but idk, maybe Radek has some better idea, for me is best way create abstract processor,
• method 'validate' in IdmIdentityService isn't good name choice, for example method enable and disable working only with identity but your method validate working with password, probably better name is validatePassword, or better way is add this method to password service,
• possible null pointer! In PasswordChangeController you add this code:
  

  if (identity == null) {
      identity = new IdmIdentityDto();
  }
  

  
  this isn't really good :(, after you throw new event in acc catch this event with processor IdentityPasswordValidateProcessor and in method validateDefinition you try get id:
  

  filter.setIdentityId(identity.getId());
  

  
  after search entity with null id you get all entities!! If identity is required add some assert, or just throw some exception, but create empty identity dto is probably way to hell.
• i'm not OK with methods preValidate in IdmPasswordPolicyService you copy body of method validate and create very similiar copy. Try to abstract this method, create duplicit code isn't ok,
• next possible null pointer, you again create empty dto:
  

  IdmPasswordPolicyDto policy = new IdmPasswordPolicyDto();
  policy.setSpecialCharBase("");
  List<IdmPasswordPolicyDto> policies = new ArrayList<IdmPasswordPolicyDto>();
  policies.add(policy);
  passwordPolicyService.preValidate(policies);
  

  
  and in method preValidate you check:
  

  if (passwordPolicy.isEnchancedControl()) => null pointer this is java not javascript
  

  
  I dont know why you create on to many places empty password policy dto with filled only special character, maybe I need consultation why you make this,
• show info message on FE doesn't work correctly. I have two policies (one default for IdM and one for system), but message is corretly show only when i choose one policy, see pictures (old password validation works correctly)
  both:
  
  only one:
  
  old validation:
  
  I excepted some merge. You have probably problem with processors order.
• FE, PasswordChange.js you are try get entityId from props, but routes isn't exists this props,
• after you call:
  

  identityManager.getService().preValidate(entityId, requestData)
  

  
  you check in then response status with 204, but from BE you never get this status,
• from public password change a get this request to BE:
  

  idm-backend/api/v1/public/identities/undefined/validate
  

  
  In my localhost exists identity with username undefined, but I change password for another identity. On public change password component is field called username and it is required. Now I understand why you create empty identity DTO (dont do it), you has information about identity username,
• you still show "Allowed special characters: @#$%&*" even it isn't necessary, my policy settings doesn't require special characters,
• after success password change information from password policies missing,
• after failed password change information from password policies missing (password pass all rules but doesn't match see picture)
  

Feel free to consult with me. After you resolve issues try to test your solution and give me this ticket back. After you resolve issues the information message with validation will be awesome.

tests updated, i found a minor bug-> when there are 2 password policies max password lenght sets to 9 and 10, and typed password is 11 char, the message said max password lenght is 10 (not 9).. I just switched parameters of compareInt method

commit: https://github.com/bcvsolutions/CzechIdMng/commit/108a3d6abc5468e7ec3bb4b9d9d752abcda82329

please look on this code again, now it should be all repaired.

Thanks for fixing issues, I made next review:

• interface PasswordChangeProcessor is empty and for now is useless,
• in FE component PasswordChange.js you still get entityId from props :(,
• in FE component Profile (file create.js) you also get entityId from props,
• your processors behavior is pre validation but name of these processors are validation, add pre,
• method EnhancedControlForSimilar has unvalid name: http://www.oracle.com/technetwork/java/codeconventions-135099.html and javadoc missing
• your code isn't formatted,
• method specialCharBase hasn't javadoc and also name isn't fully ideal try something like getSpecialCharBase,
• test are awesome, but try to use testhelper, nicknames in code isn't good,
• I checked rest endpoint "/public/identities/{backendId}/validate" and I mean attribute backendId isn't needed more, remove this attribute from your new rest endpoint,
• you call identityManager.getService().preValidate() directly, create some wrapper method in manager,
• I receive null pointer, when I haven't default password policy or is disable (for some reason):
  

  java.lang.NullPointerException: null
      at eu.bcvsolutions.idm.core.model.service.impl.DefaultIdmPasswordPolicyService.validate(DefaultIdmPasswordPolicyService.java:279)
      at eu.bcvsolutions.idm.core.model.service.impl.DefaultIdmPasswordPolicyService.preValidate(DefaultIdmPasswordPolicyService.java:246)
      at eu.bcvsolutions.idm.core.model.service.impl.DefaultIdmPasswordPolicyService$$FastClassBySpringCGLIB$$f90f724f.invoke(<generated>)
  
  possible problem code:
  if (passwordChangeDto.isIdm() && !passwordPolicyList.contains(defaultPasswordPolicy)) {
      passwordPolicyList.add(defaultPasswordPolicy);
  }
  

• your logic and code with change level in ValidationMessage isn't good readable please at least you change attribute name with number to something better,
• your translation of specialCharactersBase isn't correct. This base is special characters and password must contains some of them, allowed are all characters but only characters from this base is counted.

Merge password policies now working awesome, please fix issues above and then give me feedback back for next code review.

I removed issues you pointed out,
could you please do review again? Thank you.

commit: https://github.com/bcvsolutions/CzechIdMng/commit/d10f3bf80d9da04b257dccf49b00bdd1939b09e6

Please for next time change state of tasks to needs feedback.

I made next review, your feature is almost done! I found probably last issue by my feedbacks:

• when I have one policy for IdM with one required special character (char: '*') and second policy for system with one required special character (char: '!') your prevalidation return empty special characters.

Please fix this little issue.

after consulting this last issue, i implemented special char base like min_rules_to_fulfill, for each password policy is shown char base and it is added even in normal password validation, because it just shown that there are not enought special characters for some policies, but user did not know, which special characters have to add.

special char base is shown for these policies, which required special characters.

commit: https://github.com/bcvsolutions/CzechIdMng/commit/a6c3503dc85f0a65fbf96d44267bce08aae22791

please look in this new upgrade :D and write review.

Please your branch has to many conflicts with develop can you fix it?

conflicts solved.
commits: https://github.com/bcvsolutions/CzechIdMng/commit/a80f0363f93f11a67f611d1d1923ede86c4e58ed
https://github.com/bcvsolutions/CzechIdMng/commit/eda0f1f55400bc041e3d2f009ba753bbf4843a82

Hi Radek, welcome to hell :D please could you made something like "last" review? I don't know if I'm the best person to make a review, I try it, but probably I failed. Selling hotdog would be better than doing feedbacks.

I did test, review and it works. I like it, it's really useful, thx!

Merged into develop. I created few tickets with improvements, they can be implemented later.

Note: When policy has more rules configured, then a lot of info messages is shown above password fields, we need to redesign this in future somehow.