Repository navigation
Conversation
AlexRubik
added
t:feature
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
| SET NOCOUNT ON | ||
|
|
||
| DECLARE @UserIdKey VARCHAR(50) = CONCAT('"', @UserId, '"') | ||
| DECLARE @UserIdPath VARCHAR(50) = CONCAT('$.', @UserIdKey) |
There was a problem hiding this comment.
@UserIdPath is assigned but never read in CipherDetails_Create, so this local variable is unused.
| DECLARE @UserIdPath VARCHAR(50) = CONCAT('$.', @UserIdKey) |
Details
✨ AI Reasoning
The procedure constructs a JSON path in a local variable, but never uses that value. This leaves dead code that can be removed without changing behavior.
Reply on this comment to give feedback or ignore the issue.
More info
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE I re-reviewed the database layer for browser health exclusions at the current head. The scope was the Code Review DetailsNo new findings. Open discussions I did not raise again:
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8669 +/- ##
==========================================
+ Coverage 68.42% 74.82% +6.39%
==========================================
Files 2741 2743 +2
Lines 117469 117565 +96
Branches 10769 10781 +12
==========================================
+ Hits 80379 87965 +7586
+ Misses 34548 26929 -7619
- Partials 2542 2671 +129 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
AlexRubik
changed the title
lastbestdev
left a comment
lastbestdev
left a comment
There was a problem hiding this comment.
A few small things to double check/clean up, otherwise this is looking really good!
I went back and forth with whether or not to request changing the BIT value we're storing in the ReportExclusions JSON into a DATETIME like Archives does, but ultimately decided against because YAGNI 😄
| @@ -0,0 +1,33 @@ | |||
| CREATE PROCEDURE [dbo].[Cipher_ExcludeFromReports] | |||
There was a problem hiding this comment.
Should be named something like Cipher_UpdateManyReportExclusions. See docs.
There was a problem hiding this comment.
Thanks, renamed to Cipher_UpdateManyBrowserHealthExclusions. We also moved every name in this PR from "report exclusions" to "browser health exclusions", so it is clear which feature the data is for.
| ) | ||
| FROM | ||
| [dbo].[Cipher] C | ||
| INNER JOIN @Ids I ON I.[Id] = C.[Id] |
There was a problem hiding this comment.
❓ Do we have a cap on the number of ID's somewhere? An update to a large number of rows could need to a table lock escalation, which would be bad.
There was a problem hiding this comment.
Not in this PR, because nothing can send more than one id yet. The only caller is the endpoint in #8700, which takes a single cipher id from the route. If we add a bulk endpoint later, we will cap it at 500 like the archive endpoints.
| [Key] VARCHAR(MAX) NULL, | ||
| [ArchivedDate] DATETIME2 (7) NULL, | ||
| [Archives] NVARCHAR(MAX) NULL, | ||
| [ReportExclusions] NVARCHAR(MAX) NULL, |
There was a problem hiding this comment.
Can you please align the types?
| [ReportExclusions] NVARCHAR(MAX) NULL, | |
| [Id] UNIQUEIDENTIFIER NOT NULL, | |
| [UserId] UNIQUEIDENTIFIER NULL, | |
| [OrganizationId] UNIQUEIDENTIFIER NULL, | |
| [Type] TINYINT NOT NULL, | |
| [Data] NVARCHAR (MAX) NOT NULL, | |
| [Favorites] NVARCHAR (MAX) NULL, | |
| [Folders] NVARCHAR (MAX) NULL, | |
| [Attachments] NVARCHAR (MAX) NULL, | |
| [CreationDate] DATETIME2 (7) NOT NULL, | |
| [RevisionDate] DATETIME2 (7) NOT NULL, | |
| [DeletedDate] DATETIME2 (7) NULL, | |
| [Reprompt] TINYINT NULL, | |
| [Key] VARCHAR(MAX) NULL, | |
| [ArchivedDate] DATETIME2 (7) NULL, | |
| [Archives] NVARCHAR(MAX) NULL, | |
| [ReportExclusions] NVARCHAR(MAX) NULL, |
| [Key] VARCHAR(MAX) NULL, | ||
| [ArchivedDate] DATETIME2 (7) NULL, | ||
| [Archives] NVARCHAR(MAX) NULL, | ||
| [ReportExclusions] NVARCHAR(MAX) NULL, |
There was a problem hiding this comment.
❓ Is NVARCHAR(MAX) the appropriate type for this? Seems like it's only storing {"<userId>":true}. Do we plan on adding support for Orgs too? If not, this seems like an odd way to store a true/false value.
There was a problem hiding this comment.
Today only the owner of a personal login can exclude it, so the map has at most one key. We kept the JSON shape on purpose: it matches Favorites and Archives, which the Vault team asked us to follow, and it lets us add per-member exclusions for organization items later without another column on Cipher. Organization support is not planned in this ticket.
There was a problem hiding this comment.
@mkincaid-bw I think this is a great point and had a similar thought while dev'ing on this. Raising with the team for extra discussion.
| UPDATE | ||
| [dbo].[Cipher] | ||
| SET | ||
| [UserId] = NULL, |
There was a problem hiding this comment.
❓ When this is moved to an Org, do we intentionally want to keep value in ReportExclusions?
There was a problem hiding this comment.
Yes, that is intentional. The browser health page only checks personal logins, so the value has no effect after a share, and items cannot move back to a personal vault. Clearing it would mean changing the Vault share procedure, which we wanted to avoid.
|
I'm concerned this is touching our busiest and largest table for a feature that will be used by a relatively small number of users. Can we challenge the requirement for this, and look to storing this exclusion in a different way in the organization or collection domain, which is the only way Access Intelligence is ... accessible? |
Hey @withinfocus, a few comments here:
|
|
Good to know, thanks -- I thought this was an org-only build. I definitely agree that a new table is not appropriate. This is still pretty high risk change and I mentioned it to my team in case they wanted to weigh in, and DbOps would as well. |
AlexRubik
changed the title
4b1477a to
d363d16
Compare
4ca8638 to
4b1477a
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
4b1477a to
f52fc32
Compare
lastbestdev
left a comment
lastbestdev
left a comment
There was a problem hiding this comment.
A few adds that would be nice in the sproc so I don't have to do them and retouch db code again in upcoming PRs
| @@ -0,0 +1,33 @@ | |||
| CREATE PROCEDURE [dbo].[Cipher_UpdateManyBrowserHealthExclusions] | |||
| @Ids AS [dbo].[GuidIdArray] READONLY, | |||
| @UserId AS UNIQUEIDENTIFIER | |||
There was a problem hiding this comment.
Can we add a parameter to set the exclude value with?
| @UserId AS UNIQUEIDENTIFIER | |
| @UserId AS UNIQUEIDENTIFIER, | |
| @Exclude AS BIT |
| [BrowserHealthExclusions] = JSON_MODIFY( | ||
| COALESCE(C.[BrowserHealthExclusions], N'{}'), | ||
| CONCAT('$."', @UserId, '"'), | ||
| CAST(1 AS BIT) |
There was a problem hiding this comment.
And use value here:
| CAST(1 AS BIT) | |
| @Exclude |
Per-user JSON map of cipher report exclusions, a sibling of Archives. Vault write procedures accept the new parameter but never write it, so saving a cipher cannot erase another user's exclusion.
…iphers Reads this user's entry from ReportExclusions into CipherDetails and the cipher response, so the standard sync delivers every exclusion. EF replaces never write the column.
DIRT-owned writer for Cipher.ReportExclusions. Only the user's own personal, not-deleted logins can be excluded, and only the account revision is bumped, so other devices sync without an edit conflict.
PUT ciphers/{id}/exclude-from-reports, behind the health report
exclusions flag. Requires premium, and pushes a cipher sync so the
user's other devices pick up the exclusion.
Repository tests run on every database and pin the scope rules, the per-user read, the revision date behavior, and that Vault replaces keep the exclusion. Adds command and controller unit tests.
… organizations The EF GetManyByUserIdAsync branch for users without organizations rebuilds CipherDetails field by field and dropped ReportExcludedDate, so sync on Postgres, MySQL and SQLite returned null for users with no org. MSSQL was unaffected (its procedure selects every column).
Store { "<userId>": true } instead of the exclusion date, matching the
tech breakdown and the Favorites pattern. Nothing reads the date, and a
flag tells the server less. Ciphers now expose a per-user
ExcludedFromReports boolean in place of ReportExcludedDate.
This PR now holds only the database and data-access changes, so it can merge before the endpoint. The command, controller and their unit tests move to a stacked PR.
Main gained newer migrations, so the SQL script moves to 2026-10-23 and the three EF migrations are regenerated on top of main's model.
Follow the SQL naming guide: Cipher_UpdateManyBrowserHealthExclusions.
…edure Lets the restore feature reuse the procedure. Passing 0 removes the user's key, because the SQL Server read treats any stored value as excluded.
Store { "<userId>": ["<riskCategory>"] } instead of { "<userId>": true }.
The procedure and the EF repository take the risk category, and reads
return the user's array as ExcludedRiskCategories.
An exclude without a risk category now throws instead of saving [null].
Restore does not need a category.
Excluding a login adds the risk category to the user's array instead of replacing it, matching the tech breakdown's PUT. A category that is already in the array is kept once. Restore still removes the user's entry.
Use the name agreed in the tech breakdown review: Cipher.Exclusions, and Cipher_UpdateManyExclusions for the procedure that writes it. The EF migrations are regenerated as AddCipherExclusions.
Rename the cipher response property to Exclusions, so the JSON field is exclusions, as the tech breakdown and the SDK cipher model expect.
The duplicate check used the database's case-insensitive collation, so SQL Server treated "Weak" as a duplicate of "weak" while EF stored both. Compare with Latin1_General_BIN2 so both tracks match the exact text.
VARCHAR turns non-Latin letters into "?", while EF stores the full text. NVARCHAR(50) keeps any category the API sends, matching the NVARCHAR(MAX) column it is written to.
…on-level reads main added 2026-11-02_00_ApplyHighestPermissionLevelInReads.sql, which redefines CipherDetails_ReadByIdUserId without ExcludedRiskCategories. Our 2026-10-27 script ran before it, so a new database lost the column. Re-date the script to 2026-11-05 and copy the current CipherDetails_ReadByIdUserId into it, so it keeps the highest permission level from main and adds ExcludedRiskCategories.
b93d133 to
0d82e85
Compare
HOLDING FOR ENCRYPTION OPEN QUESTION
🎟️ Tracking
PM-41931
📔 Objective
This PR adds the database layer for browser health exclusions. A user can exclude a personal login from the browser health page. The endpoint is in #8700, which is stacked on this PR.
This PR adds:
Cipher.Exclusions. It holds{ "<userId>": ["weak", "exposed"] }: per user, the risk categories the login was excluded from. It is a per-user JSON map, likeFavoritesandArchives.CipherDetails(ExcludedRiskCategories) and on the cipher response (exclusions): the user's array, or null when the login is not excluded. The normal sync returns it.Cipher_UpdateManyExclusionsandICipherBrowserHealthExclusionRepository. These are the only writers. They change only the user's own personal, not-deleted logins. An exclude adds the risk category to the user's array, and a category that is already there (exact match) is not added again. An exclude needs a non-empty category: without one, it throws an error instead of saving[null]. A restore does not need a category.Vault save paths do not write the column. The Vault procedures take the parameter as
-- not used, and EF replaces mark it as unmodified. Integration tests cover this on all four databases.Vault-owned files:
Cipher.sql,CipherDetails.sql, eightCipherprocedures,Cipher.cs,CipherDetails.cs,CipherResponseModel.csand three EF query files.Users cannot write data until #8700 merges.
Sync
An exclude bumps
User.AccountRevisionDate, notCipher.RevisionDate. The push that tells the user's other devices to sync is in #8700. Vault confirmed this on 2026-11-04: exclusions are per user and do not change the encrypted cipher data. This answers item D6 in the tech breakdown.