Skip to content

Switch to Bitwarden.Server.Sdk.Features#7898

Open
justindbaur wants to merge 7 commits into
mainfrom
use-server-sdk-feature-service
Open

Switch to Bitwarden.Server.Sdk.Features#7898
justindbaur wants to merge 7 commits into
mainfrom
use-server-sdk-feature-service

Conversation

@justindbaur

Copy link
Copy Markdown
Member

🎟️ Tracking

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

📔 Objective

Migrate the server's feature flag plumbing from the in-tree LaunchDarklyFeatureService to the Bitwarden.Server.Sdk.Features package, while keeping the existing IFeatureService API and [RequireFeature] attribute working unchanged for callers.

Changes

  • New: ServerSdkCompatibilityExtensions.ApplyServerCompatibilityLayer() maps GlobalSettings:LaunchDarkly onto the SDK's FeatureFlagOptions, registers the server's known flag keys, and wires up a ServerContextBuilder that resolves ICurrentContext per request.
  • New: DelegatingFeatureService shims the legacy Bit.Core.Services.IFeatureService over the SDK's new service so existing callers and [RequireFeature] continue to work.
  • New: GlobalSettingsServiceCollectionExtensions consolidates GlobalSettings / IGlobalSettings DI registration.
  • Removed: LaunchDarklyFeatureService and its tests; replaced by the SDK provider plus the compat shim.
  • Trimmed the legacy IFeatureService surface (IsOnline(), GetAll()) — no production callers.
    ConfigController / ConfigResponseModel updated to the new IReadOnlyDictionary<string, JsonValue> shape (wire JSON unchanged).
  • Self-hosted instances explicitly clear the SDK key in the compat layer to preserve the previous offline-only invariant.
  • All consuming Startups (Admin, Api, Events, Identity, Scim, Sso) updated to call ApplyServerCompatibilityLayer().

Testing

  • New unit tests: ServerContextBuilderTests, GlobalSettingsServiceCollectionExtensionsTests, ServerSdkCompatibilityExtensionsTests.
  • Updated ConfigControllerTests / ServerSettingsResponseModelTests for the new flag value shape.
  • Manual: verified [RequireFeature] still returns the existing 404 envelope via the OnFeatureCheckFailed hook.

Backward compatibility

  • Existing IFeatureService consumers and [RequireFeature] continue to work via DelegatingFeatureService.
    GlobalSettings:LaunchDarkly:SdkKey and :FlagValues continue to work; FlagDataFilePath is no longer supported and emits a warning directing operators to Features:FlagValues.
  • /config response payload is wire-compatible.

The plan is to let this do this change and then wait a couple months before removing the legacy IFeatureService to avoid being annoying for in-flight PRs. Then I will make a PR to move everyone to the new service.

📸 Screenshots

@codecov

codecov Bot commented Jun 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.02415% with 31 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.92%. Comparing base (2a4c03c) to head (1ea102d).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...e/Services/Implementations/ServerContextBuilder.cs 78.04% 17 Missing and 1 partial ⚠️
...rvices/Implementations/DelegatingFeatureService.cs 57.14% 6 Missing ⚠️
...dWeb/Utilities/ServerSdkCompatibilityExtensions.cs 90.90% 3 Missing and 2 partials ⚠️
src/Admin/Startup.cs 0.00% 1 Missing ⚠️
src/Api/Models/Response/ConfigResponseModel.cs 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7898      +/-   ##
==========================================
+ Coverage   61.35%   65.92%   +4.56%     
==========================================
  Files        2236     2239       +3     
  Lines       98547    98560      +13     
  Branches     8911     8903       -8     
==========================================
+ Hits        60468    64971    +4503     
+ Misses      35943    31358    -4585     
- Partials     2136     2231      +95     

☔ View full report in Codecov by Harness.
📢 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.

Comment thread test/SharedWeb.Test/ServerSdkCompatibilityExtensionsTests.cs Fixed
Comment thread test/SharedWeb.Test/ServerSdkCompatibilityExtensionsTests.cs Fixed
Comment thread test/SharedWeb.Test/ServerSdkCompatibilityExtensionsTests.cs Dismissed
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
@justindbaur justindbaur added the t:tech-debt Change Type - Tech debt label Jun 30, 2026
@sonarqubecloud

Copy link
Copy Markdown

@justindbaur justindbaur marked this pull request as ready for review July 2, 2026 13:55
@justindbaur justindbaur requested review from a team as code owners July 2, 2026 13:55
@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the migration from the in-tree LaunchDarklyFeatureService to the first-party Bitwarden.Server.Sdk.Features package. The DelegatingFeatureService shim, ServerContextBuilder, GlobalSettings DI consolidation, and the IReadOnlyDictionary<string, JsonValue> config-response shape are internally consistent and well covered by new unit tests. Verified the legacy IFeatureService.GetAll()/IsOnline() removals have no remaining production callers and that all consuming apps (including Pam, hosted in Api) inherit the compat layer and UseFeatureFlagChecks().

Code Review Details
  • ❓ : LaunchDarkly.ServerSdk resolves to 8.10.3 across all lock files, down from 8.11.0 — a transitive downgrade forced by Bitwarden.Server.Sdk.Features 1.1.0 pinning 8.10.3; worth a quick confirmation that no needed 8.11.0 fix is lost.
    • src/Api/packages.lock.json

@JaredSnider-Bitwarden JaredSnider-Bitwarden left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 auth files LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

t:tech-debt Change Type - Tech debt

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants