[AC-2499] Add permission checks on bulk actions menu

[eliykat]

Type of change

- [ ] Bug fix
- [ ] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

The bulk actions menus (in the individual and organization vaults) did not consistently check that the user had permissions to carry out the selected action. I suspect we have some bugs around this today, but it mostly worked because owners and admins could manage all collections and items.

However, Flexible Collections v1 changes restrict owner/admin permissions, so you're more likely to come across the situation where you don't have permissions to carry out the bulk action.

I've reviewed all the bulk actions and added permission checks where required - usually one of the collectionView getters (canDelete, canEdit) or cipher.edit. These incorporate feature flag checks where required, so the changes in this PR are not themselves feature flagged.

Notably, a user must have sufficient permissions for all items/collections selected. We are not allowing any partial success.

Other, minor changes:

• remove the header on error toasts, per design feedback. This includes existing error toasts in these components for consistency
• change the bulk access error from You have not selected anything to You have not selected any collections - minor bugfix for the situation where the user has selected items but no collections.

Code changes

• file.ext: Description of what was changed and why

Screenshots

Before you submit

• Please add unit tests where it makes sense to do so (encouraged but not required)
• If this change requires a documentation update - notify the documentation team
• If this change has particular deployment requirements - notify the DevOps team
• Ensure that all UI additions follow WCAG AA requirements

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 0% with 98 lines in your changes are missing coverage. Please review.

Project coverage is 27.77%. Comparing base (3eeafc0) to head (5ca126c).

Files	Patch %	Lines
...pps/web/src/app/vault/org-vault/vault.component.ts	0.00%	39 Missing :warning:
.../src/app/vault/individual-vault/vault.component.ts	0.00%	36 Missing :warning:
...log/bulk-collection-assignment-dialog.component.ts	0.00%	6 Missing :warning:
...lt/components/vault-items/vault-items.component.ts	0.00%	5 Missing :warning:
...bs/common/src/vault/models/view/collection.view.ts	0.00%	5 Missing :warning:
.../src/app/vault/core/views/collection-admin.view.ts	0.00%	4 Missing :warning:
...ents/vault-items/vault-collection-row.component.ts	0.00%	3 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8912      +/-   ##
==========================================
- Coverage   27.79%   27.77%   -0.03%     
==========================================
  Files        2422     2422              
  Lines       70262    70328      +66     
  Branches    13101    13124      +23     
==========================================
  Hits        19531    19531              
- Misses      49211    49277      +66     
  Partials     1520     1520              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[github-actions[bot]]

Checkmarx One – Scan Summary & Details – 2f9dc8fc-0847-442e-a96b-24f2d013ca4e

New Issues

Severity	Issue	Source File / Package	Checkmarx Insight
	Client_Privacy_Violation	/apps/browser/src/auth/popup/account-switching/account.component.ts: 24	Attack Vector

[eliykat]

Converting to draft while I clarify requirements and potentially add additional flagging.

[eliykat]

@shane-melton I've made further changes after our discussions and going round in circles a bit.

The problem is that the individual vault should only evaluate individual permissions, not admin permissions. I considered just not touching the individual vault at all, because really the v1 collection management setting only limits admin actions in Admin Console. However, we're already using collectionView.canEdit/canDelete in the individual vault, and that includes admin permissions, so there's already leakage between the two contexts. Ignoring individual vault here would probably just cause unintended consequences/bugs/questions from QA.

Instead I went with your suggestion of updating CollectionView to only use individual permissions, and CollectionAdminView to use admin permissions. While I was at it, I removed the pre-FC logic in these getters. I agree with you that this should basically fix all the leakage here and make sure this ticket (and subsequent changes) work as intended.

One downside here is that many methods in the org VaultComponent use CollectionView in their signature, but receive a CollectionAdminView, so it's not clear which getters are being used. That can't be updated without breaking other things (e.g. VaultItemEvent) which are used in both contexts. I guess that's how polymorphism works though.

It will require QA before merge, but I'd come to the conclusion that that would be required anyway.

[eliykat]

I'm going to wait until #9100 is merged, as that will probably be merged first and will cause conflicts.

[eliykat]

@shane-melton Hopefully the final set of changes:

1. merge in main after various other PRs have been merged, particularly #9100
2. feature flag the checkbox changes
3. feature flag all the permission checks in the VaultComponents (maybe overkill, but it's safest to ensure new code isn't run)
4. feature flag the refactor to collectionView.canDelete and collectionView.canEdit. Now FC MVP logic is the default, but if the FC v1 flag is on it'll use the new code which only uses individual permissions. CollectionAdminView is not feature flagged, but by always calling super() the worst it'll do is repeat the org-level check again if FCv1 is not enabled.