[PM-35948] Update send openapi to work for sdk#7556
Conversation
coroiu
added
ai-review
…arden/server into update-send-openapi-to-work-for-sdk
|
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 #7556 +/- ##
==========================================
- Coverage 59.73% 59.71% -0.02%
==========================================
Files 2109 2110 +1
Lines 92752 92809 +57
Branches 8246 8251 +5
==========================================
+ Hits 55402 55421 +19
- Misses 35385 35424 +39
+ Partials 1965 1964 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| if (sendAuthResult.Equals(SendAccessResult.PasswordRequired)) | ||
| { | ||
| return new UnauthorizedResult(); | ||
| throw new UnauthorizedAccessException(); |
There was a problem hiding this comment.
suggestion(non-blocking): Align
to also throw for consistency (they return different response bodies)suggestion(non-blocking): Tests don't check this unhappy path, consider adding new ones
|
|
||
| [AllowAnonymous] | ||
| [HttpPost("{encodedSendId}/access/file/{fileId}")] | ||
| [ProducesResponseType<SendFileDownloadDataResponseModel>(StatusCodes.Status200OK)] |
There was a problem hiding this comment.
question: any reason this endpoint kept Task<IActionResult> + the explicit 200 annotation, instead of being refactored to Task<SendFileDownloadDataResponseModel> like Access? The only thing blocking that is the UnauthorizedResult return — same throw switch as Access would unify the two.
There was a problem hiding this comment.
@adudek-bw, in addition to GetSendFileDownloadDataUsingAuth (as pointed out above), there is another action (AccessUsingAuth) that's still returning IActionResult. We should make sure we address returning the correct response model for all the actions in the controller
| /// the OpenAPI generator to emit an explicit Bearer token parameter rather than injecting the | ||
| /// user session token via middleware. | ||
| /// </summary> | ||
| public class SendAccessBearerOperationFilter : IOperationFilter |
There was a problem hiding this comment.
question/issue: Have you generated the bindings to confirm this fix works?
From what I can see using Claude the V2 operations don' get the send-access-bearer security in the generated api.json. Verified by running pwsh dev/generate_openapi_files.ps1 on this branch, the V2 operations have security: [{}] (empty requirement object, equivalent to "no auth required") instead of the expected security: [{"send-access-bearer": []}].
According to Claude the cause is that new OpenApiSecuritySchemeReference("send-access-bearer") is constructed without the host document, so the reference doesn't serialize as a $ref. Fix: convert the filter to IDocumentFilter (which receives the document) and pass it to the reference constructor — same pattern as AddSwaggerServerWithSecurity in SharedWeb/Utilities/ServiceCollectionExtensions.cs:867-870.
There was a problem hiding this comment.
Thanks for the info on the command! I updated the code :)
There was a problem hiding this comment.
question: Is V1 out of scope? Same IDocumentFilter could potentially also override [AllowAnonymous] endpoints in SendsController to security: [], which could let the SDK drop hand-rolled V1 calls
…arden/server into update-send-openapi-to-work-for-sdk
|
4 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-35948
📔 Objective
Update the functions to have actual return values instead of void returns
Update the endpoints to properly handle the different auth types needed
SDK changes: bitwarden/sdk-internal#961
📸 Screenshots