[PM-11162] Assign to Collection Permission Update

[Jingo88]

🎟️ Tracking

PM-11162

📔 Objective

Users with Can Edit Except Password will not be allowed to assign collections to ciphers.

Updates made to CipherDetails_ReadByIdUserId.sql to get accurate ViewPassword value when user assigns/unassigns collections (This addresses bug PM-14046)

NOTE: EntityFramework update being looked at later on.

📸 Screenshots

⏰ 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

[github-actions]

Checkmarx One – Scan Summary & Details – 28e30614-4dfb-430d-998f-137eaee90129

No New Or Fixed Issues Found

[shane-melton]

Changes look good, but looks like we need to update CipherController tests.

[codecov]

Codecov Report

Attention: Patch coverage is 3.22581% with 30 lines in your changes missing coverage. Please review.

Project coverage is 43.32%. Comparing base (0989e7f) to head (c40ce6d).

Files with missing lines	Patch %	Lines
src/Api/Vault/Controllers/CiphersController.cs	3.33%	29 Missing ⚠️
...ture.Dapper/Vault/Repositories/CipherRepository.cs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4844      +/-   ##
==========================================
- Coverage   43.33%   43.32%   -0.02%     
==========================================
  Files        1458     1458              
  Lines       66491    66519      +28     
  Branches     6078     6082       +4     
==========================================
  Hits        28817    28817              
- Misses      36383    36411      +28     
  Partials     1291     1291              

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

[shane-melton]

Changes look good! Just two small nits on the formatting of the sproc and we'll want to update the migration script date.

[shane-melton]

⛏️ Can we fix the formatting here so that the , comes at the end of each line (to match the rest)?

[shane-melton]

⛏️ Extra whitespace we can remove

[shane-melton]

🎨 We'll want to update the date in this migration script to todays date since we already have a few others on main with a more recent date.

[shane-melton]

Thanks! Looks good to go!

[rkac-bw]

lgtm

[shane-melton]

I was looking at this again after I ran into something similar in my current work. I think we may need to tweak another sproc.

[shane-melton]

Looks good!

[shane-melton]

💭 We should ask QA to test a few scenarios where a cipher has both group and user assignments with mixed ReadOnly and HidePasswords permissions to be sure the users is still allowed to perform the action with the most permissive relationship.

I think this query will still work as expected for those scenarios, but its probably something we should confirm with some additional testing.

[rkac-bw]

Merge can be problematic with excessive locking, better just update or delete:

`CREATE OR ALTER PROCEDURE [dbo].[CollectionCipher_UpdateCollections]
@cipherid UNIQUEIDENTIFIER,
@userid UNIQUEIDENTIFIER,
@CollectionIds AS [dbo].[GuidIdArray] READONLY
AS
BEGIN
SET NOCOUNT ON

DECLARE @OrgId UNIQUEIDENTIFIER = (
    SELECT TOP 1
        [OrganizationId]
    FROM
        [dbo].[Cipher]
    WHERE
        [Id] = @CipherId
)

-- Common CTE for available collections
WITH [AvailableCollectionsCTE] AS(
    SELECT
        C.[Id]
    FROM
        [dbo].[Collection] C
    INNER JOIN
        [Organization] O ON O.[Id] = C.[OrganizationId]
    INNER JOIN
        [dbo].[OrganizationUser] OU ON OU.[OrganizationId] = O.[Id] AND OU.[UserId] = @UserId
    LEFT JOIN
        [dbo].[CollectionUser] CU ON CU.[CollectionId] = C.[Id] AND CU.[OrganizationUserId] = OU.[Id]
    LEFT JOIN
        [dbo].[GroupUser] GU ON CU.[CollectionId] IS NULL AND GU.[OrganizationUserId] = OU.[Id]
    LEFT JOIN
        [dbo].[Group] G ON G.[Id] = GU.[GroupId]
    LEFT JOIN
        [dbo].[CollectionGroup] CG ON CG.[CollectionId] = C.[Id] AND CG.[GroupId] = GU.[GroupId]
    WHERE
        O.[Id] = @OrgId
        AND O.[Enabled] = 1
        AND OU.[Status] = 2 -- Confirmed
        AND (
            CU.[ReadOnly] = 0
            OR CG.[ReadOnly] = 0
        ) AND (
            CU.[HidePasswords] = 0
            OR CG.[HidePasswords] = 0
        )
)
-- Insert new collection assignments
INSERT INTO [dbo].[CollectionCipher] (
    [CollectionId],
    [CipherId]
)
SELECT 
    [Id],
    @CipherId
FROM @CollectionIds
WHERE [Id] IN (SELECT [Id] FROM [AvailableCollectionsCTE])
AND NOT EXISTS (
    SELECT 1 
    FROM [dbo].[CollectionCipher]
    WHERE [CollectionId] = [@CollectionIds].[Id]
    AND [CipherId] = @CipherId
);

-- Delete removed collection assignments
DELETE CC
FROM [dbo].[CollectionCipher] CC
WHERE CC.[CipherId] = @CipherId
AND CC.[CollectionId] IN (SELECT [Id] FROM [AvailableCollectionsCTE])
AND CC.[CollectionId] NOT IN (SELECT [Id] FROM @CollectionIds);
IF @OrgId IS NOT NULL
BEGIN
    EXEC [dbo].[User_BumpAccountRevisionDateByOrganizationId] @OrgId
END

END
GO`

[rkac-bw]

lgtm

[rkac-bw]

lgtm