[BEEEP] [PM-28808] Fix invalid identity URL in Swagger #6653
Conversation
- in generated JSON (used in help center), only show cloud options - in self-host and dev, only show local option Mixing these will cause CORS issues because server is not expecting requests from those origins.
|
Claude finished @eliykat's task —— View job Code Review CompleteI've reviewed the changes to fix the invalid identity URL in Swagger. The implementation is solid and addresses the stated problem. Here are my findings: SummaryThis PR successfully configures Swagger to support both US and EU cloud servers in the generated FindingsFinding 1: Missing unit tests for new extension method ( The new Suggested test approachCreate a test class
Reference the existing Finding 2: Codecov reports 35 lines missing coverage in Startup.cs (💭) The PreSerializeFilters code in
Consider whether integration tests that actually fetch the swagger.json endpoint would be valuable, though this may be excessive for configuration code. Finding 3: Potential confusion between security schemes for end users (💭) With multiple servers (US/EU) and multiple security definitions (US_server/EU_server), users must manually match the server selection with the correct authorization option. The PR description acknowledges this limitation with the note: "you cannot automatically associate the server with the authorization option." The added description text helps ("Use this option if you've selected the {serverDescription}" and "Note: your authorization must match the server you have selected"), but users could still select US server + EU authorization. This appears to be a limitation of OpenAPI/Swagger UI rather than the implementation. The current approach with clear descriptions is reasonable given the constraints. Finding 4: Security scheme clearing in PreSerializeFilters is correct (👍) The Security Considerations✅ No security issues identified. The changes:
Response to Previous Review FeedbackThe author (eliykat) correctly explained why dani-garcia's simplification suggestion wouldn't work: the CI pipeline uses Development environment for swagger generation but needs to output cloud server configurations. The PreSerializeFilters approach is the correct solution to override configuration based on runtime environment. Code Quality Observations✅ Method naming: Recommendations
Action Items for Author
Overall this is a well-implemented solution that properly addresses the issue within the constraints of the Swagger/OpenAPI specification. The approach of configuring cloud servers by default and overriding for local environments is sound. |
|
New Issues (2)Checkmarx found the following issues in this Pull Request
|
eliykat
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6653 +/- ##
==========================================
- Coverage 53.41% 53.39% -0.03%
==========================================
Files 1911 1911
Lines 85129 85190 +61
Branches 7642 7642
==========================================
+ Hits 45469 45483 +14
- Misses 37909 37956 +47
Partials 1751 1751 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
djsmith85
requested review from
dani-garcia
and removed request for
djsmith85
dani-garcia
left a comment
dani-garcia
left a comment
There was a problem hiding this comment.
Looks good to me, can confirm that the generated schemas work and match what's expected. I've also checked that the SDK works when the schema has multiple servers defined and besides some code duplication that I fixed in bitwarden/sdk-internal#589, everything works great.
I've left a non-blocking suggestion on a possible way to remove the manual modification of the servers list, I've tested it locally and it seems to do the trick but I may have missed some corner case.
| // Configure Bitwarden cloud US and EU servers. These will appear in the swagger.json build artifact | ||
| // used for our help center. These are overwritten with the local server when running in self-hosted | ||
| // or dev mode (see Api Startup.cs). | ||
| config.AddSwaggerServerWithSecurity( |
There was a problem hiding this comment.
I wonder if it wouldn't be easier to change this code to be:
if (globalSettings.SelfHosted)
{
// Configure self-hosted server
config.AddSwaggerServerWithSecurity(
serverId: "self_hosted_server",
serverUrl: globalSettings.BaseServiceUri.Api,
identityTokenUrl: $"{globalSettings.BaseServiceUri.Identity}/connect/token",
serverDescription: "Self-hosted");
}
else
{
// Configure Bitwarden cloud US and EU servers. These will appear in the swagger.json build artifact
// used for our help center. These are overwritten with the local server when running in self-hosted
// or dev mode (see Api Startup.cs).
config.AddSwaggerServerWithSecurity(
serverId: "US_server",
serverUrl: "https://api.bitwarden.com",
identityTokenUrl: "https://identity.bitwarden.com/connect/token",
serverDescription: "US server");
config.AddSwaggerServerWithSecurity(
serverId: "EU_server",
serverUrl: "https://api.bitwarden.eu",
identityTokenUrl: "https://identity.bitwarden.eu/connect/token",
serverDescription: "EU server");
}And then remove the manual modification of the servers that is currently being done using config.PreSerializeFilters in Startup.Cs. Seems to generate the same servers in the schema when run locally.
## 🎟️ Tracking
<!-- Paste the link to the Jira or GitHub issue or otherwise describe /
point to where this change is coming from. -->
## 📔 Objective
The server bindings template iterates over all auth methods of all the
servers and generates code for each of them. This means that if the
schema has two servers defined (like the US and EU servers) it will
generate code like this:
```rust
if let Some(ref token) = config.oauth_access_token {
request = request.bearer_auth(token.to_owned());
};
// This one is just duplicate
if let Some(ref token) = config.oauth_access_token {
request = request.bearer_auth(token.to_owned());
};
```
This won't break anything and is safe to do, but it's kind of wasteful.
Instead, we can just use the first auth method only.
The change doesn't have any effect on the bindings for the moment as
we're only ever defining one server and one auth method, but
bitwarden/server#6653 will add the US and EU
servers to the server API schema, which will result in the above shown
duplication.
## 🚨 Breaking Changes
<!-- Does this PR introduce any breaking changes? If so, please describe
the impact and migration path for clients.
If you're unsure, the automated TypeScript compatibility check will run
when you open/update this PR and provide feedback.
For breaking changes:
1. Describe what changed in the client interface
2. Explain why the change was necessary
3. Provide migration steps for client developers
4. Link to any paired client PRs if needed
Otherwise, you can remove this section. -->
## ⏰ Reminders before review
- Contributor guidelines followed
- All formatters and local linters executed and passed
- Written new unit and / or integration tests where applicable
- Protected functional changes with optionality (feature flags)
- Used internationalization (i18n) for all UI strings
- CI builds passed
- Communicated to DevOps any deployment requirements
- Updated any necessary documentation (Confluence, contributing docs) or
informed the documentation
team
## 🦮 Reviewer guidelines
<!-- Suggested interactions but feel free to use (or not) as you desire!
-->
- 👍 (`:+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
|
Thanks @dani-garcia ! I did mean to ask about how this would affect the SDK so thank you for checking that. I tried your suggestion, but the pipeline generates the swagger jsons using the development environment configuration which means it outputs the |
eliykat
requested review from
dani-garcia
and removed request for
justindbaur
eliykat
deleted the
ac/pm-28808/swagger-download-has-invalid-identity-tokenurl
branch
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-28808
📔 Objective
We have a swagger.json and SwaggerUI hosted on our Help Center for the public API: https://bitwarden.com/help/api/.
However it currently has the following issues:
localhost- so the Authorize button doesn't work and you can't use the "Try it out" functionalityThis PR makes the following changes:
swagger.jsonbuild artifact, which is used by the Help Center.We used to specify the
--hostoption ingenerate_openapi_files.ps1but that is no longer needed (and in any case didn't work because you can't specify the authorization config from the command line).I think this approach is reasonable but I am open to other ways to achieve this. Platform should probably be the one to review this.
📸 Screenshots
Preview of how it should look on the Help Center:
Screen.Recording.2025-11-27.at.1.03.51.pm.mov
Preview of how it should look in dev and self hosted:
Screen.Recording.2025-11-27.at.1.06.04.pm.mov
⏰ 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