[PM-13839][PM-13840] Admin Console Collections #11649

nick-livefront · 2024-10-21T20:58:37Z

🎟️ Tracking

📔 Objective

When viewing in Admin Console
- all collections an item belongs to should be shown, even if I don't have access to the collection
When editing in Admin Console
- Only collections I have edit+ access to that an item belongs to (assuming "owners and admins can manage all collections and items" is off)
- When "owners and admins can manage all collections" is on, then all collections can be edited

📸 Screenshots

Update Cipher with Admin/owner can Edit on, then Show when admin/owner is off
admin-console-collections.mov

🦮 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

- When "manage all" option is selected all collections should be editable

codecov · 2024-10-21T21:07:57Z

Codecov Report

Attention: Patch coverage is 47.61905% with 11 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@eb67b73). Learn more about missing BASE report.
Report is 3 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...ipher-form/services/default-cipher-form.service.ts	14.28%	6 Missing ⚠️
libs/common/src/vault/services/cipher.service.ts	0.00%	3 Missing ⚠️
libs/common/src/services/api.service.ts	0.00%	1 Missing ⚠️
...ibs/vault/src/cipher-view/cipher-view.component.ts	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #11649   +/-   ##
=======================================
  Coverage        ?   33.54%           
=======================================
  Files           ?     2814           
  Lines           ?    87435           
  Branches        ?    16675           
=======================================
  Hits            ?    29333           
  Misses          ?    55797           
  Partials        ?     2305

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

github-actions · 2024-10-21T21:10:37Z

Checkmarx One – Scan Summary & Details – f7dd3414-9b8d-4958-bef1-1c7b4e6d8590

No New Or Fixed Issues Found

…ng any other edits - This handles the case where a cipher is moving from unassigned to assigned and needs to have a collection to save any other edits

nick-livefront · 2024-10-22T19:45:45Z

libs/vault/src/cipher-form/services/default-cipher-form.service.ts

This change from saving the collections before any other edits was made in part of PM-13887. I was seeing errors with saving edits before assigning to the collection.

shane-melton

One small edge case we need to check and then this looks good!

shane-melton · 2024-10-25T03:05:48Z

libs/vault/src/cipher-form/services/default-cipher-form.service.ts

-      // Then save the new collection changes separately
-      encryptedCipher.collectionIds = cipher.collectionIds;
-      savedCipher = await this.cipherService.saveCollectionsWithServer(encryptedCipher);
+      savedCipher = await this.cipherService.updateWithServer(encryptedCipher, config.admin);


⚠️ There's a really specific edge case where the user may un-assign themselves from a cipher by removing the last editable collection from the cipher. If this happens, the call to cipherService.updateWithServer will fail due to a lack of permissions after first updating the collections. You'd only run into this if the access all items as admin setting was off and you removed a the last editable collection.

I believe to avoid this, we can still save the collections first. But, when the original cipher is unassigned, we need to update the call to updateWithServer to have a check for an unassigned original cipher.

e.g.

updateWithServer(encryptedCipher, config.admin || originalCollectionIds.size === 0)

This will use the admin endpoint for both of the scenarios that require it.

As always, great catch 🤯 e6c5032

Agh, I misspoke in my comment above, sorry. We should still update the cipher first.

That way if a user makes a change to the cipher AND unassigns themselves, the change to cipher is still persisted. Otherwise, the user is going to unassign themselves and then fail to update the cipher because they no longer have access.

If the cipher is originally unassigned, we can update the cipher first by making the change described above and also updating the call to use the saveCollectionsWithServerAdmin() method to account for the unassigned permissions.

(We should eventually just encapsulate this “admin / not admin” logic internally within the CipherService to avoid this headache repeatedly. But, I don’t think we need to do that here and now, as there will likely be other consequences we’ll have to consider that outside of scope.)

I think I finally wrapped my head around all of the permutations that can happen here 😅 And I was able to get those situations rolling locally so I feel better about that too.

e70380e & 679ceb2

- handling the case where the user un-assigns themselves from a cipher

…rom the cipher

nick-livefront · 2024-10-29T20:24:43Z

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

+          c.canEditItems(organization) &&
+          (this.partialEdit || !c.readOnly || this.config.admin)


@shane-melton The logic behind canEditItems and !c.readOnly || this.config.admin overlaps. Do you think it's better to be explicit here with referencing the config? Or should I update the logic to

c.organizationId === orgId && c.canEditItems(organization) && this.partialEdit

I think its okay to be explicit and reference the config.

With that said, I do think we want to re-arrange these conditions so that partialEdit still supersedes the edit-ability of a collection because the control is disabled.

c.organizationId === orgId && (this.partialEdit || c.canEditItems(organization) || this.config.admin)

👍 fea33b1

…hing it

shane-melton

Changes look good to me! Really nice work on this!

Failing tests

shane-melton

Oops, looks like we have some failing tests

…t/pm-13839-13840/admin-console-collections

nick-livefront · 2024-11-01T00:29:44Z

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

+    } else {
+      collectionsControl.enable();
+      this.showCollectionsControl = true;


@shane-melton Calling this change out. The "should enable/show collection control when an organization is selected" unit test caught the case where orgId is falsy but then truthy. I don't believe that this would happen in the UI but through the logic it's possible so I added the enable/show logic here.

That should be fine and I agree, I don't think it should come up in use, but better safe than sorry!

…t/pm-13839-13840/admin-console-collections

…pher

nick-livefront · 2024-11-04T19:12:04Z

apps/web/src/app/vault/components/vault-item-dialog/vault-item-dialog.component.ts

+    let cipher: Cipher;
+
+    // When the form config is used within the Admin Console, retrieve the cipher from the admin endpoint
+    if (this.formConfig.isAdminConsole) {
+      const cipherResponse = await this.apiService.getCipherAdmin(cipherView.id);
+      const cipherData = new CipherData(cipherResponse);
+      cipher = new Cipher(cipherData);
+    } else {
+      cipher = await this.cipherService.get(cipherView.id);
+    }
+
+    // Store the updated cipher so any following edits use the most up to date cipher
+    this.formConfig.originalCipher = cipher;


This logic stems from defect: PM-14449.

When a cipher was edited within the Admin console and the user doesn't have "direct" access, the cipherService will return null. In this case the getCipherAdmin should be used to refetch the cipher

edit-after-edit.mov

Good catch!

😅 We can all thank Brad!

nick-livefront added 2 commits October 21, 2024 14:49

allow admin console to see all collections when viewing a cipher

958633e

- When "manage all" option is selected all collections should be editable

update cipher form service to use admin endpoints

c6f8b79

nick-livefront requested a review from shane-melton October 21, 2024 20:58

nick-livefront requested a review from a team as a code owner October 21, 2024 20:58

nick-livefront mentioned this pull request Oct 21, 2024

[PM-13839][PM-13840] Admin Console Collections bitwarden/server#4922

Merged

when saving a cipher, choose to move to collections first before savi…

5b02608

…ng any other edits - This handles the case where a cipher is moving from unassigned to assigned and needs to have a collection to save any other edits

nick-livefront commented Oct 22, 2024

View reviewed changes

shane-melton requested changes Oct 25, 2024

View reviewed changes

set admin flag when the original cipher has zero collections

e6c5032

- handling the case where the user un-assigns themselves from a cipher

nick-livefront requested a review from shane-melton October 26, 2024 19:51

nick-livefront added 2 commits October 29, 2024 15:11

add check for the users ability to edit items within the collection

e70380e

save cipher edit first to handle when the user unassigns themselves f…

679ceb2

…rom the cipher

nick-livefront commented Oct 29, 2024

View reviewed changes

nick-livefront and others added 3 commits October 30, 2024 13:39

Merge branch 'main' into vault/pm-13839-13840/admin-console-collections

d58ed74

update filter order of collections

fea33b1

use cipher returned from the collections endpoint rather than re-fetc…

cf96b15

…hing it

shane-melton previously approved these changes Oct 31, 2024

View reviewed changes

shane-melton self-requested a review October 31, 2024 18:49

shane-melton requested changes Oct 31, 2024

View reviewed changes

nick-livefront added 3 commits October 31, 2024 14:45

Merge branch 'main' of https://github.com/bitwarden/clients into vaul…

05ec1cb

…t/pm-13839-13840/admin-console-collections

fix unit tests by adding canEditItems

d14a496

re-enable collection control when orgId is present

496bfc1

nick-livefront commented Nov 1, 2024

View reviewed changes

nick-livefront requested a review from shane-melton November 1, 2024 00:30

shane-melton previously approved these changes Nov 1, 2024

View reviewed changes

nick-livefront added 2 commits November 4, 2024 10:52

Merge branch 'main' of https://github.com/bitwarden/clients into vaul…

b592960

…t/pm-13839-13840/admin-console-collections

fetch the updated cipher from the respective service for editing a ci…

823967a

…pher

nick-livefront dismissed shane-melton’s stale review via 823967a November 4, 2024 18:57

nick-livefront commented Nov 4, 2024

View reviewed changes

nick-livefront requested a review from shane-melton November 4, 2024 19:13

shane-melton approved these changes Nov 4, 2024

View reviewed changes

nick-livefront merged commit b42741f into main Nov 7, 2024
63 of 64 checks passed

nick-livefront deleted the vault/pm-13839-13840/admin-console-collections branch November 7, 2024 16:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PM-13839][PM-13840] Admin Console Collections #11649

[PM-13839][PM-13840] Admin Console Collections #11649

nick-livefront commented Oct 21, 2024 •

edited by jira bot

Loading

codecov bot commented Oct 21, 2024 •

edited

Loading

github-actions bot commented Oct 21, 2024 •

edited

Loading

nick-livefront Oct 22, 2024

shane-melton left a comment

shane-melton Oct 25, 2024

nick-livefront Oct 26, 2024

shane-melton Oct 29, 2024

shane-melton Oct 29, 2024 •

edited

Loading

nick-livefront Oct 29, 2024

nick-livefront Oct 29, 2024

shane-melton Oct 30, 2024

nick-livefront Oct 30, 2024

shane-melton left a comment

shane-melton left a comment

nick-livefront Nov 1, 2024

shane-melton Nov 1, 2024

nick-livefront Nov 4, 2024

nick-livefront Nov 4, 2024

shane-melton Nov 4, 2024

nick-livefront Nov 5, 2024

		c.canEditItems(organization) &&
		(this.partialEdit \|\| !c.readOnly \|\| this.config.admin)

[PM-13839][PM-13840] Admin Console Collections #11649

[PM-13839][PM-13840] Admin Console Collections #11649

Conversation

nick-livefront commented Oct 21, 2024 • edited by jira bot Loading

🎟️ Tracking

📔 Objective

📸 Screenshots

🦮 Reviewer guidelines

codecov bot commented Oct 21, 2024 • edited Loading

Codecov Report

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

No New Or Fixed Issues Found

Choose a reason for hiding this comment

shane-melton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shane-melton Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shane-melton left a comment

Choose a reason for hiding this comment

shane-melton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nick-livefront commented Oct 21, 2024 •

edited by jira bot

Loading

codecov bot commented Oct 21, 2024 •

edited

Loading

github-actions bot commented Oct 21, 2024 •

edited

Loading

shane-melton Oct 29, 2024 •

edited

Loading