Skip to content

PM-35273: feat: Add support for SDK API calls by providing base urls#6805

Merged
david-livefront merged 1 commit into
mainfrom
update-sdk-client-settings
Apr 20, 2026
Merged

PM-35273: feat: Add support for SDK API calls by providing base urls#6805
david-livefront merged 1 commit into
mainfrom
update-sdk-client-settings

Conversation

@david-livefront
Copy link
Copy Markdown
Collaborator

@david-livefront david-livefront commented Apr 16, 2026

🎟️ Tracking

PM-35273

📔 Objective

This PR adds support for instantiating the Bitwarden SDK Client with ClientSettings allowing the SDK to make appropriate network requests.

@david-livefront david-livefront requested a review from a team as a code owner April 16, 2026 16:44
@github-actions github-actions Bot added app:password-manager Bitwarden Password Manager app context app:authenticator Bitwarden Authenticator app context t:feature Change Type - Feature Development labels Apr 16, 2026
},
) : SdkClientManager {
private val userIdToClientMap = mutableMapOf<String?, Client>()
private val userIdToClientMap = mutableMapOf<String, Client>()
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Previously, we would create a single client that was unassociated with a user and reuse it. This will no longer work because we need to make sure the base URLs are appropriately set based on the current settings.

So now we never store the unassociated client.


override suspend fun <T> singleUseClient(
block: suspend Client.() -> T,
): T = clientProvider(null).use { it.block() }
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I have completely removed the ability to fetch a client for a null UserID and added this function to ensure that a short lived client is always closed when it is done with its task.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 16, 2026

Codecov Report

❌ Patch coverage is 90.38462% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.35%. Comparing base (853307e) to head (e34f30d).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
...rden/data/platform/manager/SdkClientManagerImpl.kt 44.44% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6805      +/-   ##
==========================================
+ Coverage   85.33%   85.35%   +0.02%     
==========================================
  Files         962      959       -3     
  Lines       61070    61060      -10     
  Branches     8656     8651       -5     
==========================================
+ Hits        52113    52118       +5     
+ Misses       5957     5942      -15     
  Partials     3000     3000              
Flag Coverage Δ
app-data 17.49% <90.38%> (+0.01%) ⬆️
app-ui-auth-tools 20.68% <0.00%> (+0.02%) ⬆️
app-ui-platform 15.89% <0.00%> (-0.01%) ⬇️
app-ui-vault 26.59% <0.00%> (+0.01%) ⬆️
authenticator 6.69% <0.00%> (-0.05%) ⬇️
lib-core-network-bridge 4.27% <0.00%> (-0.03%) ⬇️
lib-data-ui 1.03% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 16, 2026

Logo
Checkmarx One – Scan Summary & Detailsdc722771-a1d1-4769-84e2-9fa8741a4bf0

Great job! No new security vulnerabilities introduced in this pull request

@david-livefront david-livefront force-pushed the update-sdk-client-settings branch 4 times, most recently from ff65a7b to 58f06ad Compare April 17, 2026 18:20
*/
suspend fun <T> singleUseClient(
userId: String? = null,
accessToken: String? = null,
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The need for this access token is specifically for intermediate states during the login flow.

The login process has not yet completed, so we have not persisted the access token. The SDK still requires the access token to do it's job properly, so we provide it manually here.

@david-livefront david-livefront added the ai-review-vnext Request a Claude code review using the vNext workflow label Apr 17, 2026
@david-livefront david-livefront force-pushed the update-sdk-client-settings branch from 58f06ad to bacb7f0 Compare April 17, 2026 18:40
@david-livefront david-livefront force-pushed the update-sdk-client-settings branch from bacb7f0 to e34f30d Compare April 17, 2026 18:41
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 17, 2026

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the refactor that wires ClientSettings (identity URL, API URL, device identifier, user agent, client version) into the Bitwarden SDK Client so the SDK can issue network requests with the correct base URLs. The change introduces a singleUseClient path on SdkClientManager for short-lived, non-cached clients (used by AuthSdkSource and GeneratorSdkSource), tightens getOrCreateClient to require a non-null userId, and extracts BitwardenServiceClientConfig as a separate Hilt-provided singleton so SdkRepositoryFactory can derive ClientSettings from it. Test coverage is preserved for each converted source, and new tests cover singleUseClient, ClientSettings construction, and the manual-access-token path in SdkTokenRepository.

Code Review Details

No findings.

The three existing unresolved review threads from @david-livefront proactively explain the key design decisions (no longer caching unassociated clients, always closing short-lived clients, manual access-token pathway for intermediate login states), which addressed the questions I would have otherwise raised.

@david-livefront
Copy link
Copy Markdown
Collaborator Author

Thanks @SaintPatrck

@david-livefront david-livefront added this pull request to the merge queue Apr 20, 2026
Merged via the queue into main with commit 6e16daf Apr 20, 2026
26 checks passed
@david-livefront david-livefront deleted the update-sdk-client-settings branch April 20, 2026 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review-vnext Request a Claude code review using the vNext workflow app:authenticator Bitwarden Authenticator app context app:password-manager Bitwarden Password Manager app context t:feature Change Type - Feature Development

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants