Arch/qa env seeding tweaks by MGibson1 · Pull Request #7430 · bitwarden/server

MGibson1 · 2026-04-09T20:58:12Z

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-34880
https://bitwarden.atlassian.net/browse/PM-34881
https://bitwarden.atlassian.net/browse/PM-34886

📔 Objective

Various tweaks to smooth qa automation testing.

📸 Screenshots

github-actions · 2026-04-09T21:11:19Z

Checkmarx One – Scan Summary & Details – afb68beb-f45f-40a1-871b-0af9ef4c0adf

New Issues (2)

Checkmarx found the following issues in this Pull Request

#	Severity	Issue	Source File / Package	Checkmarx Insight
1		CSRF	src/Api/AdminConsole/Controllers/GroupsController.cs: 289	details Method at line 289 of /src/Api/AdminConsole/Controllers/GroupsController.cs gets a parameter from a user request from orgUserId. This parameter ... Attack Vector
2		CSRF	src/Api/Vault/Controllers/CiphersController.cs: 287	details Method at line 287 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This parameter value flows t... Attack Vector

Fixed Issues (1)

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

Severity	Issue	Source File / Package
	~~CSRF~~	src/Api/AdminConsole/Controllers/GroupsController.cs: 275

codecov · 2026-04-09T22:54:12Z

Codecov Report

❌ Patch coverage is 45.83333% with 26 lines in your changes missing coverage. Please review.
✅ Project coverage is 58.98%. Comparing base (4883d49) to head (f0fa000).
⚠️ Report is 63 commits behind head on main.

Files with missing lines	Patch %	Lines
...lling/Services/Implementations/LicensingService.cs	47.36%	16 Missing and 4 partials ⚠️
...SharedWeb/Utilities/ServiceCollectionExtensions.cs	0.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7430      +/-   ##
==========================================
+ Coverage   58.53%   58.98%   +0.45%     
==========================================
  Files        2069     2076       +7     
  Lines       91306    91466     +160     
  Branches     8128     8130       +2     
==========================================
+ Hits        53443    53952     +509     
+ Misses      35954    35590     -364     
- Partials     1909     1924      +15

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

kdenney

Logic looks good! Only thing I'd ask is if you could add some unit tests to cover the new changes? We've had issues in the past with changes to the licensing logic causing regressions so I like to make sure we cover that area pretty thoroughly. Thanks!

kdenney

Approving after our convo in slack. Tests are complicated without giving the licenses to CI. Ideally we refactor this in the future into two services but not needed for this fix.

Co-authored-by: Justin Baur <19896123+justindbaur@users.noreply.github.com>

github-actions · 2026-04-14T21:37:07Z

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR adjusts licensing, PlayId tracking, and data protection services to support QA automation testing in non-production environments. The licensing service now separates creation and verification certificates, allowing self-hosted non-production instances to verify both production- and dev-signed licenses. PlayId services consistently gate on !IsProduction() instead of IsDevelopment(). Data protection configuration was refined to allow file system key persistence in dev while skipping Azure blob storage.

All findings from the previous review round have been addressed in fixup commits, including the build failure from mismatched constant names, the inconsistent PlayIdService/PlayIdSingletonService environment checks, and the old-style signature verification paths.

Code Review Details

No new findings. All previously identified issues have been resolved.

kdenney

After looking at this again, I have a couple questions.

This reverts the potential new environment change identified

this way we don't need to run distinct

kdenney · 2026-04-16T21:44:28Z

+            || _verificationCertificates.Any(c => !allowedThumbprints.Contains(c.Thumbprint)))
+        {
+            throw new Exception("Invalid license verifying certificate.");
+        }


This is clean, nice! 😄

Co-authored-by: Kyle Denney <4227399+kdenney@users.noreply.github.com>

claude · 2026-04-16T21:52:50Z

-            "‎B34876439FCDA2846505B2EFBBA6C4A951313EBE";
+
+        // Load license creation cert
+        var creationCertThumbprint = environment.IsDevelopment() ? developmentCertThumbprint : productionCertThumbprint;


❌ CRITICAL: Build failure -- constant references do not match renamed declarations

Details and fix

Commit 4028107 renamed the constants to _productionCertThumbprint and _developmentCertThumbprint (with underscore prefix), but the references on lines 71, 118, and 119 still use the old names without the prefix. This will fail to compile.

Suggested change

var creationCertThumbprint = environment.IsDevelopment() ? developmentCertThumbprint : productionCertThumbprint;

var creationCertThumbprint = environment.IsDevelopment() ? _developmentCertThumbprint : _productionCertThumbprint;

The same fix is needed on lines 118-119:

CoreHelpers.CleanCertificateThumbprint(_productionCertThumbprint), CoreHelpers.CleanCertificateThumbprint(_developmentCertThumbprint)

sonarqubecloud · 2026-04-16T22:11:10Z

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

mzieniukbw · 2026-05-08T16:57:49Z

        this IServiceCollection services, IWebHostEnvironment env, GlobalSettings globalSettings)
    {
        var builder = services.AddDataProtection().SetApplicationName("Bitwarden");
-        if (env.IsDevelopment())


@MGibson1 this introduced a regression in development self-hosted environment. It is not impossible to run Api Self-Hosted, which now tries to load data protection from /etc/bitwarden, which does not exist in most dev environments

MGibson1 added 2 commits April 9, 2026 13:49

[PM-34886] allow play id tracking in any non-production build

2628904

[PM-34880] Allow configuration by setting regardless of env

0acd87d

Fixup append vs add on list

c19faae

MGibson1 force-pushed the arch/qa-env-seeding-tweaks branch from 7af0b81 to c19faae Compare April 9, 2026 22:32

MGibson1 marked this pull request as ready for review April 13, 2026 17:40

MGibson1 requested a review from a team as a code owner April 13, 2026 17:40

MGibson1 requested a review from kdenney April 13, 2026 17:40

MGibson1 added the ai-review-vnext Request a Claude code review using the vNext workflow label Apr 13, 2026

kdenney requested changes Apr 13, 2026

View reviewed changes

justindbaur reviewed Apr 13, 2026

View reviewed changes

Comment thread src/Core/Billing/Services/Implementations/LicensingService.cs Outdated

kdenney previously approved these changes Apr 14, 2026

View reviewed changes

Simplify valid issues for licenses

d6abc53

Co-authored-by: Justin Baur <19896123+justindbaur@users.noreply.github.com>

MGibson1 dismissed kdenney’s stale review via d6abc53 April 14, 2026 21:36

Allow prod-signed certs in dev self host instances

05ccc89

claude Bot reviewed Apr 15, 2026

View reviewed changes

Comment thread src/Core/Billing/Services/Implementations/LicensingService.cs Outdated

Use all verification licenses for all verification methods

67ed61f

kdenney reviewed Apr 16, 2026

View reviewed changes

Comment thread src/Core/Billing/Services/Implementations/LicensingService.cs

Comment thread src/Core/Billing/Services/Implementations/LicensingService.cs

amorask-bitwarden reviewed Apr 16, 2026

View reviewed changes

Comment thread src/SharedWeb/Utilities/ServiceCollectionExtensions.cs Outdated

MGibson1 added 3 commits April 16, 2026 10:31

Fixup license logic bug

f4da360

ensure distinct license set

d4802d2

avoid saving data protection certs for dev environments

2749d31

This reverts the potential new environment change identified

MGibson1 requested review from amorask-bitwarden, justindbaur and kdenney April 16, 2026 17:55

claude Bot reviewed Apr 16, 2026

View reviewed changes

Comment thread src/Core/Billing/Services/Implementations/LicensingService.cs Outdated

claude Bot reviewed Apr 16, 2026

View reviewed changes

Comment thread src/Core/Billing/Services/Implementations/LicensingService.cs Outdated

claude Bot reviewed Apr 16, 2026

View reviewed changes

Comment thread src/Core/Billing/Services/Implementations/LicensingService.cs Outdated

MGibson1 added 4 commits April 16, 2026 11:04

remove simple extension

17266e5

private thumbprints

6e97d75

simpler verification thumbprint validation

fd5ee89

switch to hashset for verification certs

f85f0a4

this way we don't need to run distinct

claude Bot reviewed Apr 16, 2026

View reviewed changes

Comment thread src/Core/Services/Play/Implementations/PlayIdSingletonService.cs

amorask-bitwarden previously approved these changes Apr 16, 2026

View reviewed changes

Fixup missed play id non-prod logic

522529e

MGibson1 dismissed amorask-bitwarden’s stale review via 522529e April 16, 2026 18:39

MGibson1 requested a review from amorask-bitwarden April 16, 2026 18:48

kdenney requested changes Apr 16, 2026

View reviewed changes

Update src/Core/Billing/Services/Implementations/LicensingService.cs

4028107

Co-authored-by: Kyle Denney <4227399+kdenney@users.noreply.github.com>

claude Bot reviewed Apr 16, 2026

View reviewed changes

fixup!

f0fa000

kdenney approved these changes Apr 16, 2026

View reviewed changes

MGibson1 merged commit 4346fe2 into main Apr 20, 2026
46 checks passed

MGibson1 deleted the arch/qa-env-seeding-tweaks branch April 20, 2026 13:54

mzieniukbw reviewed May 8, 2026

View reviewed changes

MGibson1 mentioned this pull request May 12, 2026

Add data protection cert override to recommended dev settings #7614

Merged

	var creationCertThumbprint = environment.IsDevelopment() ? developmentCertThumbprint : productionCertThumbprint;
	var creationCertThumbprint = environment.IsDevelopment() ? _developmentCertThumbprint : _productionCertThumbprint;

Conversation

MGibson1 commented Apr 9, 2026

🎟️ Tracking

📔 Objective

📸 Screenshots

Uh oh!

github-actions Bot commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kdenney left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kdenney left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Apr 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🤖 Bitwarden Claude Code Review

Uh oh!

Uh oh!

kdenney left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kdenney Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot Apr 16, 2026

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented Apr 16, 2026

Quality Gate passed

Uh oh!

Uh oh!

mzieniukbw May 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

github-actions Bot commented Apr 9, 2026 •

edited

Loading

codecov Bot commented Apr 9, 2026 •

edited

Loading

github-actions Bot commented Apr 14, 2026 •

edited

Loading

mzieniukbw May 8, 2026 •

edited

Loading