[PM-27886] - Archive Revamp: Replace ArchiveDate from Cipher Entity and SQL Files #6589

jaasen-livefront · 2025-11-17T21:07:37Z

🎟️ Tracking

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

📔 Objective

This PR migrates per-user archive state out of the Cipher table into a new CipherArchive table.
Key changes:

Wire UserCipherDetailsQuery and CipherDetailsQuery to pull ArchivedDate from CipherArchive via left joins instead of the Cipher table.
Refactor archive/unarchive logic in CipherRepository.ToggleArchiveCipherStatesAsync to insert/delete rows in CipherArchive and then “touch” ciphers via RevisionDate only.
Update CipherDetails and UserCipherDetails TVFs to join CipherArchive
Rewrite Cipher_Archive and Cipher_Unarchive sprocs to operate purely on CipherArchive using GuidIdArray and UserId, keeping the account revision bump.
Update Cipher_Update sproc to remove the ArchivedDate parameter and column update.
Add migrator script to deploy the new proc definitions.

📸 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

claude · 2025-11-17T21:08:49Z

Claude finished @jaasen-livefront's task —— View job

Code Review Complete

Summary of Changes Since Last Review

The author has addressed shane-melton's feedback by moving the CipherArchive join into the TVFs (CipherDetails and UserCipherDetails), eliminating duplication across stored procedures. The PR successfully migrates archive state to a new per-user CipherArchive table with proper left joins in both SQL and EF Core queries.

Critical Issues

Finding 1: Missing data migration from Cipher.ArchivedDate to CipherArchive

❌ BLOCKING - The migration script updates stored procedures but does not migrate existing ArchivedDate values from the Cipher table (line 16 in Cipher.sql shows this column still exists). Any currently archived ciphers will lose their archive state after deployment.

Required Fix

Add this at the beginning of util/Migrator/DbScripts/2025-11-14_00_AddArchiveCipherToCipherDetails.sql (before line 1):

-- Migrate existing archived ciphers from Cipher.ArchivedDate to CipherArchive table
-- Note: Cipher.ArchivedDate was not per-user. We migrate to the cipher's owner (UserId).
-- Organizational ciphers (UserId = NULL) with ArchivedDate are skipped since CipherArchive requires UserId.
IF EXISTS (SELECT 1 FROM sys.columns WHERE object_id = OBJECT_ID(N'[dbo].[Cipher]') AND name = 'ArchivedDate')
BEGIN
    INSERT INTO [dbo].[CipherArchive] (CipherId, UserId, ArchivedDate)
    SELECT c.Id, c.UserId, c.ArchivedDate
    FROM [dbo].[Cipher] c
    WHERE c.ArchivedDate IS NOT NULL 
      AND c.UserId IS NOT NULL
      AND NOT EXISTS (
          SELECT 1 FROM [dbo].[CipherArchive] ca 
          WHERE ca.CipherId = c.Id AND ca.UserId = c.UserId
      );
END
GO

Pre-deployment verification needed:

Query production to check if any ciphers have ArchivedDate IS NOT NULL
Check if organizational ciphers (UserId = NULL) have ArchivedDate set
Plan for eventual removal of Cipher.ArchivedDate column in a future migration after verifying successful data migration

Finding 2: Potential KeyNotFoundException in CipherDetailsQuery

⚠️ Minor Issue - Line 74 in CipherDetailsQuery.cs performs dictionary access [userId] which can throw KeyNotFoundException if the Folders JSON doesn't contain the userId key. While the string check at line 72 provides some protection, it's not foolproof for malformed JSON.

UserCipherDetailsQuery handles this correctly with a safe helper method (lines 135-155). Consider applying the same pattern.

Suggested Fix

Replace lines 70-74 with:

FolderId = GetFolderIdSafe(c.Folders, userId, _ignoreFolders),

Then add helper method:

private static Guid? GetFolderIdSafe(string foldersJson, Guid userId, bool ignoreFolders)
{
    if (ignoreFolders || string.IsNullOrWhiteSpace(foldersJson))
        return null;
        
    try
    {
        if (!foldersJson.ToLowerInvariant().Contains(userId.ToString()))
            return null;
            
        var folders = CoreHelpers.LoadClassFromJsonData<Dictionary<Guid, Guid>>(foldersJson);
        return folders?.TryGetValue(userId, out var folder) == true ? folder : null;
    }
    catch
    {
        return null;
    }
}

Improvements

Finding 3: Missing unit tests for archive functionality

♻️ Technical Debt - Per CLAUDE.md line 25, unit tests should be added for new feature development. The archive/unarchive logic in CipherRepository.ToggleArchiveCipherStatesAsync (lines 781-865) represents significant new functionality but has no corresponding test coverage additions.

Consider adding unit tests covering:

Archiving ciphers creates CipherArchive records
Unarchiving removes CipherArchive records
Archive state is per-user (multiple users can have different archive states for org ciphers)
Edge cases (already archived, non-existent cipher, permission checks)

Finding 4: Organizational cipher archiving behavior unclear

💭 Question - The Cipher_Archive stored procedure (line 21) only archives ciphers where C.[UserId] = @UserId, meaning organizational ciphers (UserId = NULL) cannot be archived. The CipherArchive table schema enforces this with a NOT NULL UserId constraint.

Questions:

Is this intentional? Should users be unable to archive organizational/shared ciphers?
Or is per-user archive state for org ciphers planned in a future phase?

If organizational ciphers should NOT support archiving, consider adding a comment in the stored procedure explaining this business rule for future maintainers.

Good Practices Observed

SQL joins properly moved to TVFs per reviewer feedback (lines 160-162 in CipherDetails.sql, lines 65-66 in UserCipherDetails.sql)
EF Core queries correctly use left joins to handle null archive states
CASCADE DELETE on CipherArchive ensures data integrity when ciphers/users are deleted
Archive/unarchive operations properly "touch" cipher RevisionDate for sync (line 857 in CipherRepository.cs)

Action Items for Author

❌ Add data migration to preserve existing ArchivedDate values (Finding 1)
⚠️ Add safe Folders JSON parsing to CipherDetailsQuery (Finding 2)
♻️ Consider adding unit tests for archive functionality (Finding 3)
💭 Clarify organizational cipher archiving behavior (Finding 4)

src/Sql/dbo/Vault/Functions/CipherDetails.sql

util/Migrator/DbScripts/2025-11-14_00_AddArchiveCipherToCipherDetails.sql

src/Sql/dbo/Vault/Stored Procedures/Cipher/Cipher_Unarchive.sql

src/Sql/dbo/Vault/Stored Procedures/Cipher/Cipher_Archive.sql

src/Infrastructure.EntityFramework/Repositories/Queries/UserCipherDetailsQuery.cs

github-actions · 2025-11-17T21:19:14Z

Checkmarx One – Scan Summary & Details – 7d8f97d3-e6c1-4b6b-898b-c70df3f0e356

New Issues (2)

Checkmarx found the following issues in this Pull Request

Severity	Issue	Source File / Package	Checkmarx Insight
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 300	details Method at line 300 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from model. This parameter value flow... `ID: hsURHKn%2BTOjd4DhDsizIE4jsa4I%3D` Attack Vector
	CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 300	details Method at line 300 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This parameter value flows t... `ID: zm7shnku5DA3GTK5vayfmboa6PU%3D` Attack Vector

…ection

util/Migrator/DbScripts/2025-11-14_00_AddArchiveCipherToCipherDetails.sql

src/Infrastructure.EntityFramework/Repositories/Queries/UserCipherDetailsQuery.cs

src/Sql/dbo/Vault/Stored Procedures/Cipher/Cipher_Archive.sql

src/Infrastructure.EntityFramework/Vault/Repositories/Queries/CipherDetailsQuery.cs

util/Migrator/DbScripts/2025-11-14_00_AddArchiveCipherToCipherDetails.sql

src/Sql/dbo/Vault/Functions/CipherDetails.sql

src/Infrastructure.EntityFramework/Vault/Repositories/Queries/CipherDetailsQuery.cs

src/Infrastructure.EntityFramework/Repositories/Queries/UserCipherDetailsQuery.cs

codecov · 2025-11-18T00:10:47Z

Codecov Report

❌ Patch coverage is 63.55140% with 39 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.93%. Comparing base (bde0ab4) to head (6a09715).

Files with missing lines	Patch %	Lines
...k/Vault/Repositories/Queries/CipherDetailsQuery.cs	59.61%	20 Missing and 1 partial ⚠️
...tyFramework/Vault/Repositories/CipherRepository.cs	65.90%	11 Missing and 4 partials ⚠️
...ork/Repositories/Queries/UserCipherDetailsQuery.cs	72.72%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           PM-27884    #6589      +/-   ##
============================================
- Coverage     57.01%   56.93%   -0.08%     
============================================
  Files          1916     1914       -2     
  Lines         85265    85155     -110     
  Branches       7626     7617       -9     
============================================
- Hits          48616    48486     -130     
- Misses        34815    34833      +18     
- Partials       1834     1836       +2

☔ 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.

shane-melton · 2025-11-18T00:15:43Z

src/Sql/dbo/Vault/Stored Procedures/Cipher/CipherDetails_ReadByIdUserId.sql

🎨 I think it would be preferable to put the additional join in the CipherDetails.sql function that way we don't have to repeat it in all these CipherDetails_* sprocs (or anywhere else the function is used).

@shane-melton Totally agree. I had done that originally but then had a bunch of failing sql and tests that couldn't seem to handle the nested join. I'll see if I can get them to work again.

@shane-melton this is now resolved in da0131c