[PM-22219] - [Vault] [Server] Exclude items in default collections from Admin Console #5992
Conversation
jaasen-livefront
requested review from
Jingo88,
shane-melton,
gbubemismith and
nick-livefront
|
New Issues (5)Checkmarx found the following issues in this Pull Request
Fixed Issues (14)Great job! The following issues were fixed in this Pull Request
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5992 +/- ##
==========================================
+ Coverage 47.85% 53.07% +5.22%
==========================================
Files 1702 1770 +68
Lines 75534 78618 +3084
Branches 6799 6998 +199
==========================================
+ Hits 36144 41724 +5580
+ Misses 37923 35306 -2617
- Partials 1467 1588 +121 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| .Where(c => | ||
| // select ciphers with no collections or none of its collections are default | ||
| !cipherGroups.TryGetValue(c.Id, out var grp) | ||
| || !grp.Any(cc => defaultCollIds.Contains(cc.CollectionId)) |
There was a problem hiding this comment.
(I think switching to !grp.All(...) should work, but there may be a better way)
| public async Task<IEnumerable<CipherOrganizationDetailsWithCollections>> | ||
| GetAllOrganizationCiphersExcludingDefaultUserCollections(Guid orgId) | ||
| { | ||
| var defaultCollIds = (await _collectionRepository |
There was a problem hiding this comment.
💭 🎨 It would probably be beneficial to put this logic in its own sproc to avoid pulling all ciphers, collection ids, and collectionCiphers for the organization from the database every time. This method will be called semi frequently for admins in the AC.
…tants.cs # # It looks like you may be committing a merge. # If this is not correct, please run # git update-ref -d MERGE_HEAD # and try again.
|
@shane-melton Thanks for the feedback! Ready for another look when you have a moment. |
There was a problem hiding this comment.
🎨 Sorry, I probably should have been more clear. I think we should have the sproc perform the logic to filter out the ciphers that only belong to default collection. (e.g. using a CTE or JOINs on the Collection/CollectionCipher tables)
There was a problem hiding this comment.
@shane-melton Ah ok gotcha. Updated. I kept the other sproc as I think it's still useful. Let me know if you feel otherwise.
There was a problem hiding this comment.
I think there may still be some confusion. We should have a sproc like CipherOrganizationDetails_ReadByOrganizationIdWithoutDefaultCollections that queries all the ciphers that belong to an organization AND excludes those ciphers only belong to default collections.
Then the controller/query only has to make a single DB call and it gets exactly the result it needs to return without any filtering/rearranging at the API level.
There was a problem hiding this comment.
Maybe something like this? Would need to verify logic and test
-- View that provides organization cipher details with their collection associations
CREATE OR ALTER VIEW [dbo].[OrganizationCipherDetailsWithCollectionsView]
AS
SELECT
C.[Id],
C.[UserId],
C.[OrganizationId],
C.[Type],
C.[Data],
C.[Attachments],
C.[CreationDate],
C.[RevisionDate],
C.[DeletedDate],
C.[Reprompt],
C.[Key],
CASE
WHEN O.[UseTotp] = 1 THEN 1
ELSE 0
END AS [OrganizationUseTotp],
CC.[CollectionId], -- NULL when cipher has no collections (due to LEFT JOIN)
COL.[Type] AS [CollectionType] -- NULL when cipher has no collections OR when collection has no type set
FROM [dbo].[Cipher] C
INNER JOIN [dbo].[Organization] O ON C.[OrganizationId] = O.[Id]
LEFT JOIN [dbo].[CollectionCipher] CC ON CC.[CipherId] = C.[Id] -- LEFT JOIN ensures ciphers without collections are included
LEFT JOIN [dbo].[Collection] COL ON CC.[CollectionId] = COL.[Id] -- LEFT JOIN to get collection details; NULL when no collection
WHERE C.[UserId] IS NULL; -- Organization ciphers only (exclude user-owned ciphers)
GO
CREATE OR ALTER PROCEDURE
[dbo].[CipherOrganizationDetails_ReadByOrganizationIdExcludingDefaultCollections]
@OrganizationId UNIQUEIDENTIFIER
AS
BEGIN
SET NOCOUNT ON;
WITH CiphersWithNoCollections AS (
-- Ciphers with no collections at all
SELECT
[Id], [UserId], [OrganizationId], [Type], [Data], [Attachments],
[CreationDate], [RevisionDate], [DeletedDate], [Reprompt], [Key], [OrganizationUseTotp]
FROM [dbo].[OrganizationCipherDetailsWithCollectionsView]
WHERE [OrganizationId] = @OrganizationId
AND [CollectionId] IS NULL
),
CiphersWithNullTypeCollections AS (
-- Ciphers with collections that have NULL type (treated as non-default)
SELECT
[Id], [UserId], [OrganizationId], [Type], [Data], [Attachments],
[CreationDate], [RevisionDate], [DeletedDate], [Reprompt], [Key], [OrganizationUseTotp]
FROM [dbo].[OrganizationCipherDetailsWithCollectionsView]
WHERE [OrganizationId] = @OrganizationId
AND [CollectionId] IS NOT NULL
AND [CollectionType] IS NULL
),
CiphersWithNonDefaultCollections AS (
-- Ciphers with collections that are explicitly non-default (Type != 1)
SELECT
[Id], [UserId], [OrganizationId], [Type], [Data], [Attachments],
[CreationDate], [RevisionDate], [DeletedDate], [Reprompt], [Key], [OrganizationUseTotp]
FROM [dbo].[OrganizationCipherDetailsWithCollectionsView]
WHERE [OrganizationId] = @OrganizationId
AND [CollectionId] IS NOT NULL
AND [CollectionType] IS NOT NULL
AND [CollectionType] != 1
)
SELECT DISTINCT
[Id], [UserId], [OrganizationId], [Type], [Data], [Attachments],
[CreationDate], [RevisionDate], [DeletedDate], [Reprompt], [Key], [OrganizationUseTotp]
FROM (
SELECT * FROM CiphersWithNoCollections
UNION ALL -- Use UNION ALL since we'll DISTINCT at the end
SELECT * FROM CiphersWithNullTypeCollections
UNION ALL
SELECT * FROM CiphersWithNonDefaultCollections
) AS AllCiphers;
END
GO
There was a problem hiding this comment.
A filtered index for org only ciphers might help
CREATE INDEX IX_Cipher_OrganizationId_Filtered_OrgCiphersOnly
ON [dbo].[Cipher] ([OrganizationId])
INCLUDE ([Id], [Type], [Data], [Attachments], [CreationDate], [RevisionDate], [DeletedDate], [Reprompt], [Key])
WHERE [UserId] IS NULL;
There was a problem hiding this comment.
@rkac-bw @shane-melton Thanks for the feedback. I've got a solution that I think should suffice. Let me know what you think!
| C.[RevisionDate], | ||
| C.[DeletedDate], | ||
| C.[Reprompt], | ||
| C.[Key], |
There was a problem hiding this comment.
So following our standard - Stored procedures should select from views and views select from tables, and this seems to duplicate the logic in the view
Separation of Concerns
Views (Complex Logic Layer):
- Contains JOIN logic, business rules, calculated fields
- Defines "what data looks like" - the data contract
- Can be reused by multiple procedures
- Changes here don't break procedure signatures
Procedures (Simple Access Layer):
- Contains only parameterized filtering (WHERE clauses)
- Defines "which data to get" - the access pattern
- Focuses on security context and performance
- Changes here don't affect data structure
Both the view and stored procedure have the same join logic
There was a problem hiding this comment.
@rkac-bw Ah yes ofc. Oversight on my part. I've trimmed out the unnecessary joins from the proc.
|
@rkac-bw @shane-melton Ready for re-review! |
|
@rkac-bw @shane-melton One last little tweak, updated the file name to today and added one more layer of filtering to the view:
|
|
@rkac-bw @shane-melton Should be ready for another look when you have time. Ty! |
...l/dbo/Stored Procedures/CollectionCipher_ReadByOrganizationIdExcludingDefaultCollections.sql
Outdated
Show resolved
Hide resolved
Done! |
util/Migrator/DbScripts/2025-08-06_00_CipherOrganizationDetailsExcludingDefaultCollections.sql
Outdated
Show resolved
Hide resolved
...l/dbo/Stored Procedures/CollectionCipher_ReadByOrganizationIdExcludingDefaultCollections.sql
Outdated
Show resolved
Hide resolved
| END; | ||
| GO | ||
|
|
||
| CREATE NONCLUSTERED INDEX IX_Cipher_OrganizationId_Filtered_OrgCiphersOnly |
There was a problem hiding this comment.
Needs to be idempotent, should check if exists in case it needs to rerun:
IF NOT EXISTS (
SELECT 1
FROM sys.indexes
WHERE name = 'IX_Cipher_OrganizationId_Filtered_OrgCiphersOnly'
AND object_id = OBJECT_ID('dbo.Cipher')
)
There was a problem hiding this comment.
Done! 9794cbf
| var query = from c in dbContext.Ciphers.AsNoTracking() | ||
| where c.UserId == null | ||
| && c.OrganizationId == organizationId | ||
| && ( |
There was a problem hiding this comment.
Should this equal sql logic?
&& c.Organization.Enabled
from line 28 in view?
AND O.[Enabled] = 1; -- Only enabled organizations
There was a problem hiding this comment.
Agree! Done. 9794cbf
|
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-22219
📔 Objective
This PR updates the
GetOrganizationCiphersaction in the ciphers controller to filter out the default user collections if theExcludeDefaultUserCollectionsflag is enabled.Summary of changes:
ExcludeDefaultUserCollectionsCipherOrganizationDetails_ReadByOrganizationIdExcludingDefaultCollectionsthat gathers all the required ciphers and data and excludes the default collection ciphers📸 Screenshots
⏰ Reminders before review
🦮 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