[PM-11162] Assign To Collections Permission Update

[Jingo88]

🎟️ Tracking

PM-11162

📔 Objective

Only users with Can Edit permissions will be allowed to Assign To Collections. If the user has Can Edit Except Password they should not see Assign To Collections in the menu of the item row.

UPDATE:
After speaking with product and design we have decided to move away from hiding Can Edit Except Passwords collections from the dropdown.

If an item belongs to only a Can Edit Except Passwords collection, the user will not see the Assign to Collections action in the item row menu, and the collections dropdown will be disabled

If an item belongs to multiple collections with a higher permission (e.g. item is in Can Edit Except Password and Can Manage) the user will see the Assign to Collections action, and the collections dropdown in the edit modal will show all available collections

📸 Screen Recording

PM-11162.mov

Desktop Permissions

PM-11162-Desktop.mov

Screenshot of CLI error when trying to add collection to Can Edit Except PW item UPDATED (shows new error text)

Removed Assign option from Can Edit Except PW items in browser

PM-14165-browser-assign-remove.mov

Recording of Collections dropdown disable for item with only Can Edit Except PW. And dropdown available for item with multiple collections including a Manage/Edit permission.

PM-11162-edit-except-pw-PM-AC.mov

⏰ Reminders before review

• Contributor guidelines followed
• All formatters and local linters executed and passed
• Written new unit and / or integration tests where applicable
• Protected functional changes with optionality (feature flags)
• Used internationalization (i18n) for all UI strings
• CI builds passed
• Communicated to DevOps any deployment requirements
• Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

• 👍 (:+1:) or similar for great changes
• 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
• ❓ (:question:) for questions
• 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
• 🎨 (:art:) for suggestions / improvements
• ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
• 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
• ⛏ (:pick:) for minor or nitpick changes

[codecov]

Codecov Report

Attention: Patch coverage is 35.71429% with 18 lines in your changes missing coverage. Please review.

Project coverage is 34.38%. Comparing base (7b496cc) to head (ce5add0).
Report is 1 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...lt/components/vault-items/vault-items.component.ts	0.00%	4 Missing ⚠️
...ult/src/components/assign-collections.component.ts	0.00%	4 Missing ⚠️
libs/common/src/vault/models/view/cipher.view.ts	0.00%	3 Missing ⚠️
apps/cli/src/commands/edit.command.ts	0.00%	2 Missing ⚠️
...mponents/vault-items/vault-cipher-row.component.ts	0.00%	2 Missing ⚠️
...2/item-more-options/item-more-options.component.ts	0.00%	1 Missing ⚠️
apps/cli/src/models/response.ts	0.00%	1 Missing ⚠️
...nts/item-details/item-details-section.component.ts	90.90%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #11367      +/-   ##
==========================================
- Coverage   34.38%   34.38%   -0.01%     
==========================================
  Files        2950     2950              
  Lines       90416    90437      +21     
  Branches    16973    16980       +7     
==========================================
+ Hits        31091    31093       +2     
- Misses      56861    56881      +20     
+ Partials     2464     2463       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[github-actions]

Checkmarx One – Scan Summary & Details – 776d680b-6f33-4f7e-b3db-16d70c95d4e2

Great job, no security vulnerabilities found in this Pull Request

[shane-melton]

The change to vault-items looks good!

However, we need to make a similar change to the item-details-section.component for the new CipherForm. It also allows users to modify a cipher's collections without the "Assign To Collections" dialog so we need to adjust that component to disable the collections control when !cipher.viewPassword

[shane-melton]

⚠️ this.originalCipherView can be null when creating a new cipher and this will throw.

[shane-melton]

⚠️ We only want to disable the collection control when they don't have access to assign to other collections. Disabling allows the user to still see what its assigned to, otherwise there is no way to view that information.

[eliykat]

I don't follow this - if the cipher isn't in an organization, you can't assign it to a collection.

[shane-melton]

Looks good to go! There do seem to be a couple merge conflicts though

[eliykat]

I don't understand the logic here, even with the comment. It seems like this method is doing too much if the boolean logic is this complicated. It's also unclear how the new parameter works.

I can see that the getter is called in different flows for different purposes:

• org-vault/vault.component.html - show/hide the "Add item" button in an empty collection. I assume we don't care about this, because the ticket is only about assigning an existing collection item, not creating a new one. (correct me if I'm wrong? maybe I'm reading the ticket too closely)
• item-details-section.component.ts - filtering for readonly collections when creating an item (also an add item flow)
• assign-collections.component.ts - filtering for readonly collections when editing an item. This is about assigning an existing item, which is what this PR is about; however it seems to be called for the collection that the item is being assigned to, not the collections the item is currently in (which is what the permission check should be based on).

Can you please help me understand how/why this getter is being used? I thought the cipher-level checks would be sufficient but I'm probably missing something as these flows are fairly complex.

Assuming you do need some extra logic here, I would recommend splitting the getter into separate methods for specific use cases. e.g. canAssignItemsInCollection, canCreateItemsInCollection.

[Jingo88]

After speaking with product and design we have decided to move away from hiding Can Edit Except Passwords collections from the dropdown.

If an item belongs to only a Can Edit Except Passwords collection, the user will not see the Assign to Collections action in the item row menu, and the collections dropdown will be disabled

If an item belongs to multiple collections with a higher permission (e.g. item is in Can Edit Except Password and Can Manage) the user will see the Assign to Collections action, and the collections dropdown in the edit modal will show all available collections

Will provide updated description and screen recordings.

[cd-bitwarden]

These changes look good to me 👍

[eliykat]

A few questions/suggestions but they are non-blocking as I am no longer a codeowner in this review. In general it looks much clearer since my last review, good job 👍

[cd-bitwarden]

Looks good