Defect #2990
closed
Some bulk actions started by managers process more users/contracts than they should
100%
Description
Tested on 11.1.1 and 11.1.2
When managers run some of the bulk actions for all users (checkbox at the top of the table), the number of really processed items contains all users in IdM, not only their subordinates. Sometimes they can display all the users in the result of the bulk action. In some cases, the action is even processed.
As a managers, I can see this:
The result of the bulk actions:
- IdentityChangeContractGuaranteeBulkAction
- IdentityAddContractGuaranteeBulkAction - displays all contracts in the system
- IdentityChangeContractTreeNodeAndValidityBulkAction - displays all contracts in the system
- IdentityEvaluateStateBulkAction - processes most of the users and display all
- IdentityChangeUserTypeBulkAction - processes all users and display all
- standard userRole
- standard userManagerRole
- role "additional permissions" which adds permissions to update identity and display their own long running tasks (bulk actions)
Export of the roles is attached.
We use only direct managers:
idm.sec.core.filter.IdmIdentity.managersFor.impl=guaranteeManagersFilter
idm.sec.core.filter.IdmIdentity.subordinatesFor.impl=guaranteeSubordinatesFilter
Files
Related issues
Updated by Tomáš Doischer over 2 years ago
- Assignee changed from Radek Tomiška to Tomáš Doischer
- Affected versions 11.2.0, 11.2.1, 11.2.2 added
Updated by Tomáš Doischer over 2 years ago
- Status changed from New to Needs feedback
- Assignee changed from Tomáš Doischer to Radek Tomiška
- % Done changed from 0 to 80
Implemented here: https://github.com/bcvsolutions/CzechIdMng/commit/385019c4d15462dc765c8b2c4927194ebf13ac43
There were a few other bulk actions with the same issue, I looked at all of them in every product module.
Some bulk actions already had tests for no permissions so I only had to change them a little bit. One thing I wasn't quite sure about is that tests in some cases required to have for example DELETE permission for identity and I don't know why. But right now, tests are working. I tested all the changed bulk actions in frontend and they work as intended.
@tomiskar, can you please give me feedback?
Updated by Radek Tomiška over 2 years ago
- Status changed from Needs feedback to In Progress
- Assignee changed from Radek Tomiška to Tomáš Doischer
- Target version set to 12.0.0
- Affected versions 10.6.0 added
Thx for the fix! I did code review and found one issue: Renamed method doesn't call super class method (see previous implementation) => AbstractContractGuaranteeBulkAction permissions (READ, AUTOCOMPLETE) are ignored now. It's saver to call super class in all cases - e.g. all bulk action can be secured (~ disabled) in abstract implementions in future.
Updated by Tomáš Doischer over 2 years ago
- Assignee changed from Tomáš Doischer to Radek Tomiška
Thank you for the feedback, I added this to the changed bulk actions, tests still work.
Commit: https://github.com/bcvsolutions/CzechIdMng/commit/190b21217e8f14ddd3da7b128a669dc71e950f04
Can you give it another look? Thank you.
Updated by Tomáš Doischer over 2 years ago
- Status changed from In Progress to Needs feedback
Updated by Tomáš Doischer over 2 years ago
- Status changed from Needs feedback to In Progress
- Assignee changed from Radek Tomiška to Tomáš Doischer
- % Done changed from 80 to 40
Permissions in bulk actions are more complicated than that. If a bulk actions is meant for identity, everything we put in getAuthoritiesForEntity() will be considered to be permission for identity. This means that right now you need more permissions than what makes sense.
The solution I will try: in bulk actions for contracts, getAuthoritiesForEntity() will not be overridden but READ, AUTOCOMPLETE will be in the abstract class (AbstractContractGuaranteeBulkAction). The permission previously in getAuthoritiesForEntity() in implemented bulk actions will be moved to getPermissions() and permissions will be checked in the body of the bulk actions.
As a result, you will be able to run a bulk action for users you can read, but the process will end in error if you don't have the necessary permissions for the operation. This is more transparent for the user as well.
Updated by Tomáš Doischer over 2 years ago
- Status changed from In Progress to Needs feedback
- Assignee changed from Tomáš Doischer to Radek Tomiška
- % Done changed from 40 to 80
Implemented as suggested in #2990#note-6.
Commit: https://github.com/bcvsolutions/CzechIdMng/commit/915ef185dc997c38a9e0249386ef11b0b7573036
@tomiskar, can you give me feedback?
Updated by Radek Tomiška over 2 years ago
- Status changed from Needs feedback to In Progress
- Assignee changed from Radek Tomiška to Tomáš Doischer
- % Done changed from 80 to 90
I did code review, it looks nice, thx! I found only one issue :) IdentityChangeUserTypeBulkAction should override getAuthoritiesForEntity, it was correct before last commit, if i read it correctly.
Updated by Tomáš Doischer over 2 years ago
- Status changed from In Progress to Needs feedback
- Assignee changed from Tomáš Doischer to Radek Tomiška
Sorry, you're absolutely right, I got a little carried away.
Commit: https://github.com/bcvsolutions/CzechIdMng/commit/54588cb51324bb3936e395b7afef4c50fe5b7053
@tomiskar, can you give me feedback?
Updated by Radek Tomiška over 2 years ago
- Status changed from Needs feedback to Resolved
- Assignee changed from Radek Tomiška to Tomáš Doischer
- % Done changed from 90 to 100
Thx! Merged into develop.
Updated by Radek Tomiška over 2 years ago
- Status changed from Resolved to Closed
Updated by Radek Tomiška over 2 years ago
- Related to Feature #2700: Bulk move users to a different work position and change their contract expiration added
Updated by Radek Tomiška over 2 years ago
- Related to Feature #2664: Bulk add and remove contract manager added
Updated by Radek Tomiška over 2 years ago
- Target version changed from 12.0.0 to 11.2.3