Actions
Feature #2325
closed
REST endpoint and behavior for password filter
Added by Ondřej Kopr almost 4 years ago. Updated over 3 years ago.
Start date:
06/16/2020
Due date:
08/31/2020
% Done:
100%
Estimated time:
Owner:
Description
Goal of the ticket is implement:
- REST endpoints for password filter (AD library),
- new agenda for password filter definitions,
- behavior for check echos,
- dynamic saved mechanism for saving echos,
- validate password trough password policy with selecting policies for given accounts,
- reverse logic for geting AccAccount from sAMAccountName (uid),
- change password for given accounts list,
- logic for removing echos,
- performance test,
- test,
- documentation.
This is global ticket some part can be separated to smallest tickets.
Whole request for the feature can be founded there: https://wiki.czechidm.com/priv/features/password_filter/navrh_implementace
Related issues
Updated by Ondřej Kopr almost 4 years ago
- % Done changed from 0 to 10
Now is implemented basic fronted agenda for password filter definition.
Now I continue with:- select best hash algorithm, favorite is Scrypt,
- implement rest endpoints for change and validate passwords.
Updated by Ondřej Kopr almost 4 years ago
- % Done changed from 10 to 40
Basic implementation was finished in branch https://github.com/bcvsolutions/CzechIdMng/tree/okopr/2325-password-filter
- agenda,
- new REST endpoints (now without permissions now),
- storage for echos (cache),
- behavior for equals two echo records with timeout,
- catch echo for validate and change (two new REST endpoints),
- set echo for create new entity on system with password,
- set echo for password change in IdM (if IdM is managed),
- set echo for password change against system (only managed),
- set echo for successfully changed password trough new REST endpoints.
- permissions for REST endpoint,
- new frontend component for password change and accounts option,
- resolve http status codes for password filter,
- solve performance TODOS,
- check sonar,
- fix issues founded during tests,
- tests,
- documentation,
- performance tests,
- test with AD.
Updated by Ondřej Kopr almost 4 years ago
After consultation with team was decided that will be made some changes:
- rename whole entities, rest, dtos, controller,services, .. to AccUniformPassword...,
- reimplement behavior with connected system and password filter definition (AccUniformPasswordDefinition),
- add checkbox for intersection table between uniformPassword and system, the checkbox (flag) marks systems that allow password change (all other system is only connected without password filter),
- identity password change form shows united system by password filter definiton now, this isn't acceptable behavior.
Updated by Ondřej Kopr almost 4 years ago
Rename from password filter to uniform password was finished. Now I continue with:
- move properties from uniform passwords agenda to system attribute mapping,
- create new flags for echo,
- refactor behavior with catch echos.
Updated by Ondřej Kopr over 3 years ago
- % Done changed from 40 to 60
Now is completed:
- split uniform password system and password filter functionality,
- flags for echos,
- new entity component for echo and new column in account table,
- refactored rest endpoints and whole flag catch system.
- mssql flyway script,
- clean code,
- testing on AD,
- unit and integration tests,
- documentation.
Updated by Ondřej Kopr over 3 years ago
With Ondra we found new issue for password changes that are placed into provisioning queue, because for these items are setup echos and echo may expired before was the password change sent to system and the whole cycle can be repeated.
We must create some catch logic for retry mechanism and create echos, for provisioning queue.
Updated by Ondřej Kopr over 3 years ago
Together with Ondra we start with testing whole behavior with password filter and IdM:
- change password from AD (by user itself),
- change password from AD (by administrator),
- reset password from AD,
- create user by IdM with password,
- change password from IdM.
After some tests we founded that password changes that was made by IdM, will go to password filter twice. We will start with debugging the scenario. Otherwise basic green line with catching echos works well.
Updated by Ondřej Kopr over 3 years ago
Now missing these things/implementations:
- System (including IdM) can be only in one uniform definition,
- uniform system definition functionality is now only on frontend agenda -> check the uniform definition also on beckend,
- mssql flyway script,
- clean code,
- testing on AD,
- unit and integration tests,
- documentation.
Updated by Ondřej Kopr over 3 years ago
- % Done changed from 60 to 70
Final implementation things was done:
- change timeout to 3 min,
- resolve permissions for read account,
- refactor whole change process to event PASSWORD (IdmIdentityDto),
- clean code,
- tests.
- check frontend component selecbox with array value,
- documentation,
- testing with real test scenarios.
Branch: okopr/2325-password-filter (https://github.com/bcvsolutions/CzechIdMng/tree/okopr/2325-password-filter)
Compare: https://github.com/bcvsolutions/CzechIdMng/compare/okopr/2325-password-filter
Updated by Ondřej Kopr over 3 years ago
Into branch was added some minor fixes founded during testing with AD, like:
- change trough IdM and setup password validity,
- more systems in more uniform password definition,
- different setup for password policy in uniform password system.
Into tests scenario was also added new cases.
Updated by Vít Švanda over 3 years ago
- Status changed from In Progress to Needs feedback
- Assignee changed from Ondřej Kopr to Vít Švanda
Updated by Vít Švanda over 3 years ago
I did review and tested UPC (works awesome). Code looks great, thanks for this big feature.
I found some minor issues:
- DefaultPasswordFilterEncoderConfiguration vs uniformPasswordConfiguration bean name (maybe passwordFilterEncoderConfiguration? )
- AccPasswordFilterEchoItemDto,AccPasswordFilterRequestDto, AccUniformPasswordSystem,AccUniformPassword.serialVersionUID ... I recommending use default 1L.
- IdentityUniformPasswordProcessor:77 - pls format the stream
- IdentityUniformPasswordProcessor:114 - super.conditional(event) missing in the check.
- IdentityUniformPasswordProcessor - @Since missing
- UniformPasswordDeleteProcessor - add the not null assert for AccUniformPasswordDto.ID - prevent delete of all items.
- IdentityPasswordProvisioningProcessor:94 - Are you sure, that you want to clear echo for none Executed results (Canceled, Non-exceuted state?)?
- IdentityPasswordProvisioningProcessor:78 - instead (List<UUID>) event.getProperties() - use getListProperty
- defaultPasswordFilterManager:378 - WITHOUT_ACCOUNTS - instead list of accounts send id of system.
- Missing translation for UNIFORM_PASSWORD permissions.
- Missing translation of result codes.
- Different icons for UPS (filter vs key)
- UPS - FE - I have UPS "testOne" with two system and IdM. In selectbox on change password I see "testOne" and IdM. IdM should be hidden in this case.
- Password filter tab on a mapped attribute should be visible only for identity and only for provisioning.
- Passowrd filter - timeout - add FE validation for positive integer.
- DefaultPasswordFilterManager:83 - TYPO unformPasswordService.
- Echo cache could be distributed.
- AccPasswordFilterEchoItemDto - Using of BaseDto is may be useless here.
- DefaultPasswordFilterManager - You are using transformation from resource on the BE and system on FE. (optional - new one category on the FE and BE will be nice here ;) ).
- AccPasswordFilterEchoItemDto.isPasswordEqual(GuardedString password, PasswordEncoder encoder) - DTO need to some external component (PasswordEncoder). This check should be implemented in the manager not in DTO.
- DefaultPasswordFilterManager:618 TYPO ("sytemId", system.getId()));
- FE - Echo info is show only in accounts table on the identity detail. Maybe this column will usefull in the account table on a system.
Updated by Vít Švanda over 3 years ago
- Status changed from Needs feedback to In Progress
- Assignee changed from Vít Švanda to Ondřej Kopr
Updated by Ondřej Kopr over 3 years ago
- Related to Task #2454: Documentation for uniform password and password filter in IdM added
Updated by Ondřej Kopr over 3 years ago
- Status changed from In Progress to Needs feedback
- Assignee changed from Ondřej Kopr to Vít Švanda
- % Done changed from 70 to 80
Thank you for your big review. There is some my changes:
- ✅ DefaultPasswordFilterEncoderConfiguration vs uniformPasswordConfiguration bean name (maybe passwordFilterEncoderConfiguration? ) renamed,
- ✅ AccPasswordFilterEchoItemDto,AccPasswordFilterRequestDto, AccUniformPasswordSystem,AccUniformPassword.serialVersionUID ... I recommending use default 1L. changed
- ✅ IdentityUniformPasswordProcessor:77 - pls format the stream formatted
- ✅ IdentityUniformPasswordProcessor:114 - super.conditional(event) missing in the check. added
- ✅ IdentityUniformPasswordProcessor - @Since missing you have it :D
- ✅ UniformPasswordDeleteProcessor - add the not null assert for AccUniformPasswordDto.ID - prevent delete of all items. added
- IdentityPasswordProvisioningProcessor:94 - Are you sure, that you want to clear echo for none Executed results (Canceled, Non-exceuted state?)? For now is this enough, the password provisioning operation is not synchronized and if ended with another state than EXECUTED then I need clear echo record
- ✅ IdentityPasswordProvisioningProcessor:78 - instead (List<UUID>) event.getProperties() - use getListProperty changed
- ✅ defaultPasswordFilterManager:378 - WITHOUT_ACCOUNTS - instead list of accounts send id of system. EXCLUDED_SYSTEM system is now used
- ✅ Missing translation for UNIFORM_PASSWORD permissions. localized
- ✅ Missing translation of result codes. NO!
- ✅ Different icons for UPS (filter vs key) changed :D
- ✅ UPS - FE - I have UPS "testOne" with two system and IdM. In selectbox on change password I see "testOne" and IdM. IdM should be hidden in this case. the whole functionality was refactored
- ✅ Password filter tab on a mapped attribute should be visible only for identity and only for provisioning. changed
- ✅ Passowrd filter - timeout - add FE validation for positive integer. validation added
- ✅ DefaultPasswordFilterManager:83 - TYPO unformPasswordService. renamed
- ✅ Echo cache could be distributed. echo cache type changed
- AccPasswordFilterEchoItemDto - Using of BaseDto is may be useless here. I found why I added BaseDto, Whole AccPasswordFilterEchoItemDto is placed into embedded object into AccAccountDto - frontend show echo record and into embedded is allowed put only BaseDto instances
- DefaultPasswordFilterManager - You are using transformation from resource on the BE and system on FE. (optional - new one category on the FE and BE will be nice here ;) ). you have right, but I'm too lazy for create new script category
- ✅ AccPasswordFilterEchoItemDto.isPasswordEqual(GuardedString password, PasswordEncoder encoder) - DTO need to some external component (PasswordEncoder). This check should be implemented in the manager not in DTO.
- ✅ DefaultPasswordFilterManager:618 TYPO ("sytemId", system.getId())); fixed
- FE - Echo info is show only in accounts table on the identity detail. Maybe this column will usefull in the account table on a system. I presented the echo record on frontend for all and nobody protests for this change. After someone will need it I can added the column also on Account table
Thank you for review. Please can you check my answers and then we can continue with test on our environment.
Updated by Ondřej Kopr over 3 years ago
There are all feedback issues solved in compare on github: https://github.com/bcvsolutions/CzechIdMng/compare/345988f12d965d7e1d3c047e81b0b84e2da4a372...3fb50e1038818672806d529f3633db3c0e74c7a4 (branch: okopr/2325-password-filter)
Updated by Vít Švanda over 3 years ago
- Status changed from Needs feedback to Resolved
- Assignee changed from Vít Švanda to Ondřej Kopr
- % Done changed from 80 to 100
I did rereview and tested UPS. Thanks for that.
Documentation will be implemented in th #2454.
You can merge it to the develop, because LGTM.
Updated by Radek Tomiška over 3 years ago
- Status changed from Resolved to Closed
Updated by Radek Tomiška about 3 years ago
- Related to Task #2679: Change minimum number of days for password validity check added
Actions