[AC-2172] Member modal - limit admin access #8343
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… collections in the org vault edit collections modal
…all collections in ciphers
…ed, readonly, and manage properties
…when in individual vault
… super.loadCipher
…cover custom users with manage access
…ditCipherCollections for better updates to collections modal when flag is off
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8343 +/- ##
==========================================
- Coverage 27.37% 27.37% -0.01%
==========================================
Files 2344 2344
Lines 68455 68463 +8
Branches 12798 12800 +2
==========================================
Hits 18739 18739
- Misses 48312 48320 +8
Partials 1404 1404 ☔ View full report in Codecov by Sentry. |
|
New Issues
Fixed Issues
|
…unassigned does not show in modal
* Return value depends on collection management setting, wrapped behind the v1 feature flag * Update calling locations
|
I had some interesting issues with data flow in Angular. I noticed that disabled rows were still appearing in the value being emitted by the access-selector until the user interacted with it. (The commit exhibiting this problem is 3d406bb.) I tried to fix this in fdbeb48, but that is not a proper fix, and it caused unit tests to start failing because the control was emitting more times than expected. My diagnosis:
After some experimentation, I thought the best way to resolve it was to not pass potentially "disabled" values into the FormControl to begin with. The fixes are in the last 2 commits. The updated logic is:
It does make the logic more complex in some ways, but the LOC impact is still small, and it avoids trying to hack around Angular change detection, which I think is worth it. Interestingly, it reverts all changes to the access-selector - we now set up all our data in the member modal only. Arguably that's a good thing because it reflects the presentational design of the component and avoids adding complexity to it. |
vincentsalucci
previously approved these changes
Apr 9, 2024
|
I noticed that the previous logic did not filter out readonly collections when inviting a new user. This meant they were all selected by default. I moved the initial assignment of |
| ), | ||
| ); | ||
|
|
||
| const accessSelections = mapToAccessSelections(userDetails); | ||
| // Set current collections and groups the user has access to (excluding collections the current user doesn't have | ||
| // permissions to change - they are included as readonly via the CollectionAccessItems |
There was a problem hiding this comment.
Missing a closing ) for this comment.
vincentsalucci
previously approved these changes
Apr 15, 2024
Jingo88
approved these changes
Apr 16, 2024
vincentsalucci
approved these changes
Apr 16, 2024
MGibson1
pushed a commit
that referenced
this pull request
Apr 22, 2024
* limit admin permissions to assign members to collections that the admin doesn't have can manage permissions for
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Type of change
Objective
In the Member modal, if owners and admins cannot manage all collections and items, then you should only be able to edit collection assignments for collections that you have Can Manage access to.
This depends on:
On the plus side, once those are merged, this PR is fairly minimal (if dense).
Sever changes are WIP.
Code changes
member-dialog.component.ts- most of this diff is just adding acollection.canEditcheck inmapCollectionToAccessItemView. However, that requires the org and the v1 flag, so there's a large footprint just getting those values there.access-selector.component.ts-item.readonlyto mean "collection access via a group", because that was the only type of readonly item. However, we now have 2 types (collection access via group, and a collection you don't manage), so some places needed updating to limit their reach.Screenshots
Before you submit