[PM-25372] Filter out DefaultUserCollections from CiphersController.GetAssignedOrganizationCiphers

[r-tome]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-25372

📔 Objective

The GetAssignedOrganizationCiphers endpoint is exposing collection IDs for DefaultUserCollection (private collections) that users shouldn’t see.

Cause

• The endpoint calls OrganizationCiphersQuerywhich calls _collectionCipherRepository.GetManyByOrganizationIdAsync, which returns all collections for the org.
• This includes private DefaultUserCollection (type = 1), which are meant to stay hidden.
• Other parts of the codebase already filter these out, but this path was missing the filter.

Fix

• Added a new method: GetManySharedByOrganizationIdAsync
• This only returns SharedCollection (type = 0) and excludes DefaultUserCollection.

📸 Screenshots

Bug:

With this fix:

⏰ 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

[codecov]

Codecov Report

❌ Patch coverage is 95.45455% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 53.46%. Comparing base (d2d3e0f) to head (ec2d06c).
⚠️ Report is 14 commits behind head on main.

Files with missing lines	Patch %	Lines
src/Core/Vault/Queries/OrganizationCiphersQuery.cs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6274      +/-   ##
==========================================
+ Coverage   49.41%   53.46%   +4.05%     
==========================================
  Files        1784     1782       -2     
  Lines       78950    79324     +374     
  Branches     7018     7045      +27     
==========================================
+ Hits        39010    42412    +3402     
+ Misses      38425    35310    -3115     
- Partials     1515     1602      +87     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

Checkmarx One – Scan Summary & Details – 61ae2434-dbcc-4310-bd4e-f2a22b73fe27

Fixed Issues (1)

Great job! The following issues were fixed in this Pull Request

Severity	Issue	Source File / Package
	Use_Of_Hardcoded_Password	/src/Core/Constants.cs: 130

[JimmyVo16]

It’s normally wise to only select the columns you need. Even if we need all the columns, we should be explicit about it so that when new columns are added, they don’t automatically get returned.

With that said, I’ve seen companies intentionally use * as part of a pattern, such as when a stored procedure is meant to be all-purpose. There are trade-offs to this, of course.

[r-tome]

You're right about that! I copied the existing sproc CollectionCipher_ReadByOrganizationId and didn't even look.

[JimmyVo16]

Should we add an automated test for this?

[r-tome]

Added an integration test!

[JimmyVo16]

Some questions

[mkincaid-bw]

Couple minor formatting changes.

Also, if the CollectionCipher_ReadSharedByOrganizationId proc is going to be called very frequently, it could benefit by updating the IX_Collection_OrganizationId_IncludeAll index to add [Type] as an included column.

CREATE NONCLUSTERED INDEX [IX_Collection_OrganizationId_IncludeAll] 
ON [dbo].[Collection] ([OrganizationId] ASC)
INCLUDE([CreationDate],[Name],[RevisionDate],[TYPE]) WITH (DROP_EXISTING = ON)
GO

[mkincaid-bw]

⛏️ Object identifiers should have square brackets.

[mkincaid-bw]

⛏️ Object identifiers should have square brackets.

[r-tome]

Couple minor formatting changes.

Also, if the CollectionCipher_ReadSharedByOrganizationId proc is going to be called very frequently, it could benefit by updating the IX_Collection_OrganizationId_IncludeAll index to add [Type] as an included column.

CREATE NONCLUSTERED INDEX [IX_Collection_OrganizationId_IncludeAll] 
ON [dbo].[Collection] ([OrganizationId] ASC)
INCLUDE([CreationDate],[Name],[RevisionDate],[TYPE]) WITH (DROP_EXISTING = ON)
GO

@mkincaid-bw, Great insight, thanks for pointing that out! For consistency I followed our usual pattern of dropping the index first instead of using DROP_EXISTING = ON, but I’m happy to switch to that if you’d prefer.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[mkincaid-bw]

Couple minor formatting changes.
Also, if the CollectionCipher_ReadSharedByOrganizationId proc is going to be called very frequently, it could benefit by updating the IX_Collection_OrganizationId_IncludeAll index to add [Type] as an included column.

CREATE NONCLUSTERED INDEX [IX_Collection_OrganizationId_IncludeAll] 
ON [dbo].[Collection] ([OrganizationId] ASC)
INCLUDE([CreationDate],[Name],[RevisionDate],[TYPE]) WITH (DROP_EXISTING = ON)
GO

@mkincaid-bw, Great insight, thanks for pointing that out! For consistency I followed our usual pattern of dropping the index first instead of using DROP_EXISTING = ON, but I’m happy to switch to that if you’d prefer.

@r-tome Since the net-effect of this change is to add a new column to an existing index, there are a couple issues with the approach of dropping and then re-creating.

First, Once the old index is dropped, any queries using that index will no longer be able to, and queries to that table could be impacted while the new index is being built.

Second, building the new index will take longer since it has to scan the entire table to build the index. If the existing index was still in place, SQL server can use that index to build the new index, which would be much faster (at least twice as fast in my testing).

The query I gave essentially does this behind the scenes:

1. Creates a new index with the new definition
2. Drops the old index
3. Renames the new index to the old name

There are only small schema locks at the beginning and end of the statement so blocking is minimal. The only downside to my create index statement is that if this migration file should ever get run again, it would re-create the same index since there is no conditional check. It would not cause an error or duplicate the index since it would essentially just rebuild the existing index, but it would take some additional time rebuilding it.

If you want to full control over all of that, something like this would give us the best of both worlds, where the new index gets created using the old one (faster), and existing queries won't suffer poor performance during index creation:

IF EXISTS(SELECT name FROM sys.indexes WHERE name = 'IX_Collection_OrganizationId_IncludeAll_V2' AND object_id = OBJECT_ID('[dbo].[Collection]'))
BEGIN
    DROP INDEX [IX_Collection_OrganizationId_IncludeAll_V2] ON [dbo].[Collection]
END

CREATE NONCLUSTERED INDEX [IX_Collection_OrganizationId_IncludeAll_V2]
    ON [dbo].[Collection]([OrganizationId] ASC)
    INCLUDE([CreationDate], [Name], [RevisionDate], [Type])
GO

SET XACT_ABORT ON;

BEGIN TRANSACTION;
BEGIN TRY

    IF EXISTS(SELECT name FROM sys.indexes WHERE name = 'IX_Collection_OrganizationId_IncludeAll' AND object_id = OBJECT_ID('[dbo].[Collection]'))
    BEGIN
        DROP INDEX [IX_Collection_OrganizationId_IncludeAll] ON [dbo].[Collection]
    END

    EXEC sp_rename N'dbo.Collection.IX_Collection_OrganizationId_IncludeAll_V2', N'IX_Collection_OrganizationId_IncludeAll', N'INDEX';

    COMMIT TRANSACTION;
END TRY
BEGIN CATCH
    IF @@TRANCOUNT > 0
        ROLLBACK TRANSACTION;
    
    IF EXISTS(SELECT name FROM sys.indexes WHERE name = 'IX_Collection_OrganizationId_IncludeAll_V2' AND object_id = OBJECT_ID('[dbo].[Collection]'))
    BEGIN
        DROP INDEX [IX_Collection_OrganizationId_IncludeAll_V2] ON [dbo].[Collection]
    END
END CATCH;

Note that re-naming the index isn't technically required (we could just leave the new one named with _V2). The main reason for renaming _V2 back to the old name is on the off-chance that someone, somewhere has put an index hint with that index name on a query. If that were the case, that query would error since the that index name would no longer exist.

[JimmyVo16]

Hey @mkincaid-bw, I’m taking over this PR since Rui is OOO.

I was reading through your comments, and I see the concern about the potential for slow queries during index rebuilding. That’s definitely a valid point. I’m wondering, though, given that we release DB changes during maintenance windows and low-traffic periods, do you think it’s still a concern for our system?

My preference is to keep things simple if we can, but if this does turn out to be an issue, it would be helpful to add documentation to guide devs on when the more complex approach should be considered.