Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weโ€™ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PM-11162] Assign to Collection Permission Update #4844

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
c147b5b
updated collections_v2 and bulk-collections endpoint for viewPasswordโ€ฆ
Jingo88 Oct 2, 2024
c705b40
Merge branch 'main' into PM-11162-assign-to-collection-perm-update
Jingo88 Oct 2, 2024
f541285
update ciphers mock in CiphersControllerTests
Jingo88 Oct 4, 2024
0df254a
formatted
Jingo88 Oct 4, 2024
441d2e7
Merge branch 'main' into PM-11162-assign-to-collection-perm-update
Jingo88 Oct 28, 2024
a9d1df6
updates to CipherDetails query for accurate ViewPassword value
Jingo88 Oct 31, 2024
2b55485
Merge branch 'main' into PM-11162-assign-to-collection-perm-update
Jingo88 Nov 6, 2024
786b054
format nit and migration date nit
Jingo88 Nov 6, 2024
5c672ec
Merge branch 'main' into PM-11162-assign-to-collection-perm-update
Jingo88 Nov 8, 2024
5f7fb0c
Merge branch 'main' into PM-11162-assign-to-collection-perm-update
withinfocus Nov 11, 2024
f841ec5
Merge branch 'main' into PM-11162-assign-to-collection-perm-update
kejaeger Nov 13, 2024
f7135fb
Merge branch 'main' into PM-11162-assign-to-collection-perm-update
Jingo88 Nov 14, 2024
710b0e2
Merge branch 'main' into PM-11162-assign-to-collection-perm-update
Jingo88 Nov 15, 2024
7f15dc3
Merge branch 'main' into PM-11162-assign-to-collection-perm-update
kejaeger Nov 21, 2024
2215d45
Merge branch 'main' into PM-11162-assign-to-collection-perm-update
Jingo88 Nov 22, 2024
23d8790
Merge branch 'main' into PM-11162-assign-to-collection-perm-update
Jingo88 Dec 2, 2024
c5d772e
add HidePassword check to CollectionCipher_UpdateCollections
Jingo88 Dec 2, 2024
a86a45a
Merge branch 'main' into PM-11162-assign-to-collection-perm-update
Jingo88 Dec 3, 2024
e498acb
update cipher repository getManyByUserIdAsync to look at Edit and Vieโ€ฆ
Jingo88 Dec 3, 2024
5eb650f
Merge branch 'main' into PM-11162-assign-to-collection-perm-update
kejaeger Dec 9, 2024
92cd2ba
Merge branch 'main' into PM-11162-assign-to-collection-perm-update
Jingo88 Dec 10, 2024
2e92275
update migration file
Jingo88 Dec 10, 2024
2cd32b6
update related sproc
Jingo88 Dec 10, 2024
7de47e4
Merge branch 'main' into PM-11162-assign-to-collection-perm-update
Jingo88 Dec 11, 2024
ced2997
update sproc and migration file for collectionCipher_updateCollection
Jingo88 Dec 11, 2024
a240df3
Merge branch 'main' into PM-11162-assign-to-collection-perm-update
Jingo88 Dec 23, 2024
c40ce6d
remove hidepassword logic from CollectionCipher_UpdateCollections
Jingo88 Dec 23, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 55 additions & 2 deletions src/Api/Vault/Controllers/CiphersController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,59 @@ private async Task<bool> CanAccessUnassignedCiphersAsync(Guid organizationId)
return false;
}

/// <summary>
/// TODO: Move this to its own authorization handler or equivalent service - AC-2062
/// </summary>
private async Task<bool> CanModifyCipherCollectionsAsync(Guid organizationId, IEnumerable<Guid> cipherIds)
{
// If the user can edit all ciphers for the organization, just check they all belong to the org
if (await CanEditAllCiphersAsync(organizationId))
{
// TODO: This can likely be optimized to only query the requested ciphers and then checking they belong to the org
var orgCiphers = (await _cipherRepository.GetManyByOrganizationIdAsync(organizationId)).ToDictionary(c => c.Id);

// Ensure all requested ciphers are in orgCiphers
if (cipherIds.Any(c => !orgCiphers.ContainsKey(c)))
{
return false;
}

return true;
}

// The user cannot access any ciphers for the organization, we're done
if (!await CanAccessOrganizationCiphersAsync(organizationId))
{
return false;
}

var userId = _userService.GetProperUserId(User).Value;
// Select all editable ciphers for this user belonging to the organization
var editableOrgCipherList = (await _cipherRepository.GetManyByUserIdAsync(userId, true))
shane-melton marked this conversation as resolved.
Show resolved Hide resolved
.Where(c => c.OrganizationId == organizationId && c.UserId == null && c.Edit && c.ViewPassword).ToList();

// Special case for unassigned ciphers
if (await CanAccessUnassignedCiphersAsync(organizationId))
{
var unassignedCiphers =
(await _cipherRepository.GetManyUnassignedOrganizationDetailsByOrganizationIdAsync(
organizationId));

// Users that can access unassigned ciphers can also edit them
editableOrgCipherList.AddRange(unassignedCiphers.Select(c => new CipherDetails(c) { Edit = true }));
}

var editableOrgCiphers = editableOrgCipherList
.ToDictionary(c => c.Id);

if (cipherIds.Any(c => !editableOrgCiphers.ContainsKey(c)))
{
return false;
}

return true;
}

/// <summary>
/// TODO: Move this to its own authorization handler or equivalent service - AC-2062
/// </summary>
Expand Down Expand Up @@ -579,7 +632,7 @@ public async Task<OptionalCipherDetailsResponseModel> PutCollections_vNext(Guid
var userId = _userService.GetProperUserId(User).Value;
var cipher = await GetByIdAsync(id, userId);
if (cipher == null || !cipher.OrganizationId.HasValue ||
!await _currentContext.OrganizationUser(cipher.OrganizationId.Value))
!await _currentContext.OrganizationUser(cipher.OrganizationId.Value) || !cipher.ViewPassword)
{
throw new NotFoundException();
}
Expand Down Expand Up @@ -634,7 +687,7 @@ public async Task<CipherMiniDetailsResponseModel> PutCollectionsAdmin(string id,
[HttpPost("bulk-collections")]
public async Task PostBulkCollections([FromBody] CipherBulkUpdateCollectionsRequestModel model)
{
if (!await CanEditCiphersAsync(model.OrganizationId, model.CipherIds) ||
if (!await CanModifyCipherCollectionsAsync(model.OrganizationId, model.CipherIds) ||
!await CanEditItemsInCollections(model.OrganizationId, model.CollectionIds))
{
throw new NotFoundException();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ public async Task<ICollection<CipherDetails>> GetManyByUserIdAsync(Guid userId,

return results
.GroupBy(c => c.Id)
.Select(g => g.OrderByDescending(og => og.Edit).First())
.Select(g => g.OrderByDescending(og => og.Edit).ThenByDescending(og => og.ViewPassword).First())
.ToList();
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ join cg in dbContext.CollectionGroups
from cg in cg_g.DefaultIfEmpty()
where o.Id == _organizationId && o.Enabled && ou.Status == OrganizationUserStatusType.Confirmed
&& (!cu.ReadOnly || !cg.ReadOnly)
&& (!cu.HidePasswords || !cg.HidePasswords)
select c;

return query;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,40 @@ AS
BEGIN
SET NOCOUNT ON

SELECT TOP 1
*
SELECT
[Id],
[UserId],
[OrganizationId],
[Type],
[Data],
[Attachments],
[CreationDate],
[RevisionDate],
[Favorite],
[FolderId],
[DeletedDate],
[Reprompt],
[Key],
[OrganizationUseTotp],
MAX ([Edit]) AS [Edit],
MAX ([ViewPassword]) AS [ViewPassword]
FROM
[dbo].[UserCipherDetails](@UserId)
WHERE
[Id] = @Id
ORDER BY
[Edit] DESC
END
GROUP BY
[Id],
[UserId],
[OrganizationId],
[Type],
[Data],
[Attachments],
[CreationDate],
[RevisionDate],
[Favorite],
[FolderId],
[DeletedDate],
[Reprompt],
[Key],
[OrganizationUseTotp]
END
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ BEGIN
AND (
CU.[ReadOnly] = 0
OR CG.[ReadOnly] = 0
) AND (
CU.[HidePasswords] = 0
OR CG.[HidePasswords] = 0
)
),
[CollectionCiphersCTE] AS(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ private CipherDetails CreateCipherDetailsMock(Guid id, Guid userId)
UserId = userId,
OrganizationId = Guid.NewGuid(),
Type = CipherType.Login,
ViewPassword = true,
Data = @"
{
""Uris"": [
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
CREATE OR ALTER PROCEDURE [dbo].[CipherDetails_ReadByIdUserId]
@Id UNIQUEIDENTIFIER,
@UserId UNIQUEIDENTIFIER
AS
BEGIN
SET NOCOUNT ON

SELECT
[Id],
[UserId],
[OrganizationId],
[Type],
[Data],
[Attachments],
[CreationDate],
[RevisionDate],
[Favorite],
[FolderId],
[DeletedDate],
[Reprompt],
[Key],
[OrganizationUseTotp],
MAX ([Edit]) AS [Edit],
MAX ([ViewPassword]) AS [ViewPassword]
FROM
[dbo].[UserCipherDetails](@UserId)
WHERE
[Id] = @Id
GROUP BY
[Id],
[UserId],
[OrganizationId],
[Type],
[Data],
[Attachments],
[CreationDate],
[RevisionDate],
[Favorite],
[FolderId],
[DeletedDate],
[Reprompt],
[Key],
[OrganizationUseTotp]
END
GO

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
)

;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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

๐Ÿ’ญ 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.

Copy link
Contributor

@rkac-bw rkac-bw Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`

OR CG.[ReadOnly] = 0
) AND (
CU.[HidePasswords] = 0
OR CG.[HidePasswords] = 0
)
),
[CollectionCiphersCTE] AS(
SELECT
[CollectionId],
[CipherId]
FROM
[dbo].[CollectionCipher]
WHERE
[CipherId] = @CipherId
)
MERGE
[CollectionCiphersCTE] AS [Target]
USING
@CollectionIds AS [Source]
ON
[Target].[CollectionId] = [Source].[Id]
AND [Target].[CipherId] = @CipherId
WHEN NOT MATCHED BY TARGET
AND [Source].[Id] IN (SELECT [Id] FROM [AvailableCollectionsCTE]) THEN
INSERT VALUES
(
[Source].[Id],
@CipherId
)
WHEN NOT MATCHED BY SOURCE
AND [Target].[CipherId] = @CipherId
AND [Target].[CollectionId] IN (SELECT [Id] FROM [AvailableCollectionsCTE]) THEN
DELETE
;

IF @OrgId IS NOT NULL
BEGIN
EXEC [dbo].[User_BumpAccountRevisionDateByOrganizationId] @OrgId
END
END
GO
Loading