[PM-11162] Assign To Collections Permission Update #11367

Jingo88 · 2024-10-02T18:52:09Z

🎟️ Tracking

📔 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.

📸 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

⏰ 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 · 2024-10-02T19:00:53Z

Codecov Report

Attention: Patch coverage is 12.50000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 33.19%. Comparing base (0377775) to head (b94ee53).
Report is 26 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 ⚠️
...mponents/vault-items/vault-cipher-row.component.ts	0.00%	1 Missing ⚠️
libs/common/src/vault/models/view/cipher.view.ts	0.00%	1 Missing ⚠️
...nts/item-details/item-details-section.component.ts	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #11367      +/-   ##
==========================================
+ Coverage   33.16%   33.19%   +0.03%     
==========================================
  Files        2779     2780       +1     
  Lines       86228    86417     +189     
  Branches    16421    16472      +51     
==========================================
+ Hits        28597    28688      +91     
- Misses      55365    55443      +78     
- Partials     2266     2286      +20

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

github-actions · 2024-10-02T19:03:24Z

Checkmarx One – Scan Summary & Details – acff5776-e0cc-4b89-ae14-8bbab38f11af

New Issues

Severity	Issue	Source File / Package	Checkmarx Insight
	Client_Privacy_Violation	/apps/web/src/app/tools/reports/pages/weak-passwords-report.component.html: 19	Attack Vector
	Client_Privacy_Violation	/libs/tools/send/send-ui/src/send-form/components/options/send-options.component.ts: 52	Attack Vector

Fixed Issues

Severity	Issue	Source File / Package
	Client_Privacy_Violation	/apps/web/src/app/tools/access-intelligence/password-health.component.html: 50
	Client_Privacy_Violation	/apps/web/src/app/tools/access-intelligence/password-health.component.html: 45
	Client_Privacy_Violation	/apps/web/src/app/tools/access-intelligence/password-health.component.html: 40
	Client_Privacy_Violation	/apps/web/src/app/tools/access-intelligence/password-health.component.html: 45

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 · 2024-10-07T21:55:07Z

libs/vault/src/cipher-form/components/item-details/item-details-section.component.ts

+      return org.id === this.originalCipherView.organizationId;
+    });
+
+    const filteredCollections = this.originalCipherView.collectionIds.find((id) => {


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

shane-melton · 2024-10-07T22:03:32Z

libs/vault/src/cipher-form/components/item-details/item-details-section.component.ts

+      filteredCollections?.length > 0 ||
+      (this.originalCipherView.edit && this.originalCipherView.viewPassword)
+    ) {
+      this.showCollectionsControl = true;


⚠️ 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.

shane-melton · 2024-10-07T22:07:31Z

libs/vault/src/cipher-form/components/item-details/item-details-section.component.ts

🎨 I think we can put a check in the initFromExistingCipher method after the call to updateCollectionOptions() to check if originalCipher.edit && originalCipher.viewPassword. If that is false then we should call this.itemDetailsForm.controls.collectionIds.disable() which will prevent the user from changing the value.

Then we wouldn't need to modify the updateCollectionOptions() method.

shane-melton · 2024-10-07T22:10:15Z

apps/web/src/app/vault/components/vault-items/vault-items.component.ts

@@ -191,7 +191,10 @@ export class VaultItemsComponent {
    }

    const organization = this.allOrganizations.find((o) => o.id === cipher.organizationId);
-    return (organization.canEditAllCiphers && this.viewingOrgVault) || cipher.edit;
+    return (


💭 I noticed this returned value is used by the vault-cipher-row.component to show the attachments button. I don't think we want to disable that with when viewPassword == false so we may need to introduce a new option specifically for assigning collections (e.g. canAssignCollections)

…ot remove assign to collections based on permissions

…ons in desktop client

eliykat · 2024-10-16T04:46:42Z

libs/angular/src/admin-console/components/collections.component.ts

+      this.canAssignCollections = true;
+    }
+
+    this.organizationService.get$(cipher.organizationId).subscribe((org) => {


This observable is not unsubscribed.

eliykat · 2024-10-16T04:49:34Z

libs/angular/src/admin-console/components/collections.component.ts

+    if (cipher?.organizationId == null) {
+      this.canAssignCollections = true;


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

eliykat · 2024-10-16T04:52:44Z

libs/angular/src/admin-console/components/collections.component.ts

@shane-melton this is the file that tagged me for a review, but I think this would fall under Vault ownership under the team boundaries we discussed. Do you agree? If so I can just rubber stamp this review once Vault have approved, but if you could then move it into Vault ownership that would be really helpful.

Also, I was trying to figure out how this is used. I found it in desktop, but there's also a subclass in web which appears unused: https://github.com/bitwarden/clients/blob/main/apps/web/src/app/vault/individual-vault/collections.component.ts#L1.

eliykat · 2024-10-16T04:53:42Z

libs/angular/src/admin-console/components/collections.component.ts

+    }
+
+    this.organizationService.get$(cipher.organizationId).subscribe((org) => {
+      this.canAssignCollections = org.canEditAllCiphers || (cipher.edit && cipher.viewPassword);


org.canEditAllCiphers checks admin permissions. Do we recognise admin permissions from the individual vault on desktop? I thought we only recognised admin permissions in the Admin Console, regardless of client.

Also, a canAssignToCollections() getter on CipherView might be a useful abstraction, but that's very much your call.

Ah apologies! All this was a first draft in my brain and I was going to come back to it after working on some CLI stuff.

Thanks for the review, it was super helpful and got me reducing a ton of that code.

Jingo88 added 2 commits October 2, 2024 12:22

updated canEditCipher permission logic inside vault-items

51d15cd

Merge branch 'main' into PM-11162-assign-to-collections-perm

be5ac61

Jingo88 requested a review from a team as a code owner October 2, 2024 18:52

Jingo88 requested review from gbubemismith and shane-melton October 2, 2024 18:52

shane-melton requested changes Oct 2, 2024

View reviewed changes

Jingo88 added the needs-qa Marks a PR as requiring QA approval label Oct 2, 2024

Jingo88 added 3 commits October 2, 2024 17:41

Merge branch 'main' into PM-11162-assign-to-collections-perm

c852cd9

add permission logic to assign to collections for browser cipher edit

a7eb84c

update spec for new perm logic in item details section

3812613

Jingo88 requested a review from shane-melton October 4, 2024 19:01

shane-melton requested changes Oct 7, 2024

View reviewed changes

Jingo88 added 3 commits October 8, 2024 16:51

Merge branch 'main' into PM-11162-assign-to-collections-perm

f53b337

updates to vault items and item details section to only disable and n…

6308eca

…ot remove assign to collections based on permissions

fix reversed logic for canEdit and canAssign in vault-items

97efd35

Jingo88 requested a review from shane-melton October 9, 2024 18:32

Jingo88 added 2 commits October 14, 2024 11:22

Merge branch 'main' into PM-11162-assign-to-collections-perm

65caab7

update collections component to disable collections based on permissi…

2a93e72

…ons in desktop client

Jingo88 requested a review from a team as a code owner October 14, 2024 19:31

Jingo88 requested a review from eliykat October 14, 2024 19:31

eliykat requested changes Oct 16, 2024

View reviewed changes

Jingo88 added 2 commits October 16, 2024 12:02

reduce logic for can assign for desktop

8cd5cac

update naming for new cipherView getter

b94ee53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PM-11162] Assign To Collections Permission Update #11367

[PM-11162] Assign To Collections Permission Update #11367

Jingo88 commented Oct 2, 2024 •

edited

Loading

codecov bot commented Oct 2, 2024 •

edited

Loading

github-actions bot commented Oct 2, 2024 •

edited

Loading

shane-melton left a comment

shane-melton Oct 7, 2024

shane-melton Oct 7, 2024

shane-melton Oct 7, 2024

shane-melton Oct 7, 2024

eliykat Oct 16, 2024

eliykat Oct 16, 2024

eliykat Oct 16, 2024

eliykat Oct 16, 2024 •

edited

Loading

eliykat Oct 16, 2024 •

edited

Loading

Jingo88 Oct 16, 2024 •

edited

Loading

		if (cipher?.organizationId == null) {
		this.canAssignCollections = true;

[PM-11162] Assign To Collections Permission Update #11367

Are you sure you want to change the base?

[PM-11162] Assign To Collections Permission Update #11367

Conversation

Jingo88 commented Oct 2, 2024 • edited Loading

🎟️ Tracking

📔 Objective

📸 Screen Recording

⏰ Reminders before review

🦮 Reviewer guidelines

codecov bot commented Oct 2, 2024 • edited Loading

Codecov Report

github-actions bot commented Oct 2, 2024 • edited Loading

New Issues

Fixed Issues

shane-melton left a comment

Choose a reason for hiding this comment

shane-melton Oct 7, 2024

Choose a reason for hiding this comment

shane-melton Oct 7, 2024

Choose a reason for hiding this comment

shane-melton Oct 7, 2024

Choose a reason for hiding this comment

shane-melton Oct 7, 2024

Choose a reason for hiding this comment

eliykat Oct 16, 2024

Choose a reason for hiding this comment

eliykat Oct 16, 2024

Choose a reason for hiding this comment

eliykat Oct 16, 2024

Choose a reason for hiding this comment

eliykat Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

eliykat Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Jingo88 Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Jingo88 commented Oct 2, 2024 •

edited

Loading

codecov bot commented Oct 2, 2024 •

edited

Loading

github-actions bot commented Oct 2, 2024 •

edited

Loading

eliykat Oct 16, 2024 •

edited

Loading

eliykat Oct 16, 2024 •

edited

Loading

Jingo88 Oct 16, 2024 •

edited

Loading