[PM-29144] Add server communication config to /api/config
#6892
Conversation
|
Claude finished @dereknance's task in 1m 21s —— View job Code Review: PM-29144 Add server communication config to
|
| Severity | Finding | Location |
|---|---|---|
Missing unit tests for CommunicationSettings.Build() logic |
ConfigResponseModel.cs |
Analysis
What this PR does well:
- Clean integration with existing
ConfigResponseModelstructure - Proper null safety with null-conditional operators
- Case-insensitive bootstrap type matching
- Graceful handling of unknown/unconfigured bootstrap types (returns
null) - Configuration exposed is non-sensitive (URLs, cookie names/domains for client bootstrapping)
Security Assessment:
The configuration values exposed (IdpLoginUrl, CookieName, CookieDomain) are appropriate for an unauthenticated endpoint as they contain no secrets and are required for clients to properly bootstrap their connection.
Reviewed by Claude via Bitwarden Code Review
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
New Issues (4)Checkmarx found the following issues in this Pull Request
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6892 +/- ##
==========================================
- Coverage 56.07% 56.06% -0.02%
==========================================
Files 1968 1968
Lines 86927 86950 +23
Branches 7742 7746 +4
==========================================
+ Hits 48744 48746 +2
- Misses 36383 36401 +18
- Partials 1800 1803 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dereknance
force-pushed
the
pm-29144-communication-in-config-api
branch
from
8c8e2b0 to
d0398e4
Compare
| Type = "ssoCookieVendor", | ||
| IdpLoginUrl = globalSettings.Communication?.SsoCookieVendor?.IdpLoginUrl, | ||
| CookieName = globalSettings.Communication?.SsoCookieVendor?.CookieName, | ||
| CookieDomain = globalSettings.Communication?.SsoCookieVendor?.CookieDomain |
There was a problem hiding this comment.
This setup will yield camel case, which is consistent with the current /api/config payload, but different than the lower snake case example in the description of PM-29144.
If that's a deal-breaker, I'm happy to make this work with lower snake case.
| Type = "ssoCookieVendor", | ||
| IdpLoginUrl = globalSettings.Communication?.SsoCookieVendor?.IdpLoginUrl, | ||
| CookieName = globalSettings.Communication?.SsoCookieVendor?.CookieName, | ||
| CookieDomain = globalSettings.Communication?.SsoCookieVendor?.CookieDomain |
🎟️ Tracking
PM-29144
📔 Objective
This PR adds the new config from #6880 to the
/api/configendpoint so clients may learn if the server requires extra work to establish a connection.endpoint payload comparison
Without
ssoCookieVendorThe
communicationproperty is present, but null.{ "version": "2026.1.0", "gitHash": "f9c0cec8", "server": null, "environment": { "cloudRegion": null, "vault": "https://localhost:8080", "api": "http://localhost:4000", "identity": "http://localhost:33656", "notifications": "http://localhost:61840", "sso": "http://localhost:51822" }, "featureStates": { "web-push": false }, "push": { "pushTechnology": 0, "vapidPublicKey": null }, "communication": null, "settings": { "disableUserRegistration": false }, "object": "config" }With
ssoCookieVendorThe
communicationproperty is present and includes the additional required properties for that type.{ "version": "2026.1.0", "gitHash": "f9c0cec8", "server": null, "environment": { "cloudRegion": null, "vault": "https://localhost:8080", "api": "http://localhost:4000", "identity": "http://localhost:33656", "notifications": "http://localhost:61840", "sso": "http://localhost:51822" }, "featureStates": { "web-push": false }, "push": { "pushTechnology": 0, "vapidPublicKey": null }, "communication": { "type": "ssoCookieVendor", "idpLoginUrl": "", "cookieName": "", "cookieDomain": "" }, "settings": { "disableUserRegistration": false }, "object": "config" }⏰ Reminders before review
🦮 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