PM-31923 adding the whole report endpoints v2#7228
PM-31923 adding the whole report endpoints v2#7228prograhamming wants to merge 133 commits intomainfrom
Conversation
…-access-intelligence
…-access-intelligence
…-access-intelligence
…elligence' of github.com:bitwarden/server into dirt/PM-31923-whole-report-data-v2-endpoints-access-intelligence
…-access-intelligence
…elligence' of github.com:bitwarden/server into dirt/PM-31923-whole-report-data-v2-endpoints-access-intelligence
…-access-intelligence
…-access-intelligence
* refactor(billing): update seat logic * test(billing): update tests for seat logic
…-access-intelligence
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
* Return WebAuthn credential record in create response * Make CreateWebAuthnLoginCredentialCommand null-safe
…#7123) * Remove emergency access from all organization users on policy enable, or when accepted/restored * Use correct policy save system * Add additional tests * Implement both PreUpsert and OnSave side effects
* Add coupon support to invoice preview and subscription creation * Fix the build lint error * Resolve the initial review comments * fix the failing test * fix the build lint error * Fix the failing test * Resolve the unaddressed issues * Fixed the deconstruction error * Fix the lint issue * Fix the lint error * Fix the lint error * Fix the build lint error * lint error resolved * remove the setting file * rename the variable name validatedCoupon * Remove the owner property * Update OrganizationBillingService tests to align with recent refactoring - Remove GetMetadata tests as method no longer exists - Remove Owner property references from OrganizationSale (removed in d761336) - Update coupon validation to use SubscriptionDiscountRepository instead of SubscriptionDiscountService - Add missing imports for SubscriptionDiscount entities - Rename test for clarity: Finalize_WithNullOwner_SkipsValidation → Finalize_WithCouponOutsideDateRange_IgnoresCouponAndProceeds All tests passing (14/14) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * Fix the lint error * Making the owner non nullable * fix the failing unit test * Make the owner nullable * Fix the bug for coupon in Stripe with no audience restrictions(PM-32756) * Return validation message for invalid coupon * Update the valid token message * Fix the failing unit test * Remove the duplicate method * Fix the failing build and test * Resolve the failing test * Add delete of invalid coupon * Add the expired error message * Delete on invalid coupon in stripe * Fix the lint errors * return null if we get exception from stripe * remove the auto-delete change * fix the failing test * Fix the lint build error --------- Co-authored-by: Claude <noreply@anthropic.com>
feat: add MasterPasswordSalt column to User table - Add MasterPasswordSalt column to User table in both Dapper and EF implementations - Update User stored procedures (Create, Update, UpdateMasterPassword) to handle salt column - Add EF migrations and update UserView with dependent views - Set MaxLength constraint on MasterPasswordSalt column - Update UserRepository implementations to manage salt field - Add comprehensive test coverage for salt handling and normalization
…-access-intelligence
…elligence' of github.com:bitwarden/server into dirt/PM-31923-whole-report-data-v2-endpoints-access-intelligence
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| throw new BadRequestException("ReportId is required."); | ||
| } | ||
|
|
||
| var applicationDataResponse = await _organizationReportRepo.GetApplicationDataAsync(reportId); |
There was a problem hiding this comment.
❌ CRITICAL: The organizationId parameter is accepted but never used to verify the report belongs to that organization, creating an IDOR (Insecure Direct Object Reference) vulnerability.
Details and fix
A user with access to Organization A could call GET /reports/organizations/{orgA}/data/application/{reportId-from-orgB} and retrieve Organization B's application data. The controller authorizes the caller against organizationId, but this query fetches data solely by reportId without verifying ownership.
Compare with the controller's GetOrganizationReportAsync endpoint (line 199) which correctly checks report.OrganizationId != organizationId after fetching.
Suggested fix: After fetching the data, verify the report belongs to the requested organization. This requires either:
- Having the repository return the
OrganizationIdalongside the application data so it can be checked here, or - Fetching the full report first to verify ownership, then returning the application data:
var report = await _organizationReportRepo.GetByIdAsync(reportId);
if (report == null || report.OrganizationId != organizationId)
{
throw new NotFoundException("Organization report application data not found.");
}
var applicationDataResponse = await _organizationReportRepo.GetApplicationDataAsync(reportId);Note: The same pattern exists in GetOrganizationReportSummaryDataQuery (outside this review scope).
| if (request.ContentEncryptionKey != null) | ||
| { | ||
| existingReport.ContentEncryptionKey = request.ContentEncryptionKey; | ||
| } | ||
|
|
||
| if (request.SummaryData != null) | ||
| { | ||
| existingReport.SummaryData = request.SummaryData; | ||
| } | ||
|
|
||
| if (request.ApplicationData != null) | ||
| { | ||
| existingReport.ApplicationData = request.ApplicationData; | ||
| } |
There was a problem hiding this comment.
ValidateRequestAsync already rejects requests where any of these fields are null or whitespace.
Details and fix
The validation at lines 108-127 requires ContentEncryptionKey, SummaryData, ApplicationData, and ReportMetrics to all be non-null and non-whitespace. Since validation runs before this code, none of these if (request.X != null) branches can ever evaluate to false.
This creates a contradiction with the PATCH HTTP method used by the controller endpoint, which conventionally implies partial updates. As written, every update requires all fields, making it behave like a PUT.
Either:
- Remove the required validations in
ValidateRequestAsyncto allow true partial updates (consistent with PATCH semantics and this conditional logic), or - Remove the conditional null-checks here and assign unconditionally (consistent with the current validation requiring all fields)
There was a problem hiding this comment.
Keeping it a PATCH HTTP method. Making the fix now.
| var fileData = report.GetReportFile(); | ||
| if (fileData == null) | ||
| { | ||
| throw new BadRequestException("Report ID in the request body must match the route parameter"); | ||
| throw new NotFoundException(); | ||
| } |
There was a problem hiding this comment.
🟡 IMPORTANT: Download endpoint serves unvalidated files (self-hosted)
DownloadReportFileAsync does not check fileData.Validated before serving the file. The cloud path in GetOrganizationReportAsync (line 212) and GetLatestOrganizationReportAsync (line 165) only exposes download URLs when Validated == true. Self-hosted instances would serve files that haven't passed size validation.
Consider adding:
| var fileData = report.GetReportFile(); | |
| if (fileData == null) | |
| { | |
| throw new BadRequestException("Report ID in the request body must match the route parameter"); | |
| throw new NotFoundException(); | |
| } | |
| var fileData = report.GetReportFile(); | |
| if (fileData == null || !fileData.Validated) | |
| { | |
| throw new NotFoundException(); | |
| } |
There was a problem hiding this comment.
Will be addressed in iterative work for file validation
| } | ||
|
|
||
| #endregion | ||
| } |
There was a problem hiding this comment.
🟡 IMPORTANT: Missing test coverage for UploadReportFileAsync and DownloadReportFileAsync
The two new self-hosted endpoints (UploadReportFileAsync and DownloadReportFileAsync) have zero test coverage. These endpoints contain authorization checks, org-ownership validation, file-size validation with cleanup-on-failure logic, and the validated-file guard that is currently missing (see other comment). Tests for these paths would help catch the inconsistencies flagged elsewhere in this review.
Additionally, the V2 create path's file-size cap (Constants.FileSize501mb rejection at controller line 114-116) is untested.
…-access-intelligence
Banrion
left a comment
Banrion
left a comment
There was a problem hiding this comment.
Only one blocking change request, the IDOR comment.
| if (fileData == null || fileData.Id != reportFileId) | ||
| { | ||
| throw new NotFoundException("Organization report data not found."); | ||
| throw new NotFoundException(); | ||
| } |
There was a problem hiding this comment.
@prograhamming I think this is worth following up on.
|
|
||
| #endregion | ||
|
|
||
| [HttpPatch("{organizationId}/data/application/{reportId}")] |
There was a problem hiding this comment.
📝 The reportId position in all routes is non-standard. It should be "{organizationId}/{reportId}/data/application". These are existing routes for v1 and currently matches client routes. No changes needed but wanted to call it out for future work
| throw new BadRequestException("ReportId is required."); | ||
| } | ||
|
|
||
| var applicationDataResponse = await _organizationReportRepo.GetApplicationDataAsync(reportId); |
| if (request.ContentEncryptionKey != null) | ||
| { | ||
| existingReport.ContentEncryptionKey = request.ContentEncryptionKey; | ||
| } | ||
|
|
||
| if (request.SummaryData != null) | ||
| { | ||
| existingReport.SummaryData = request.SummaryData; | ||
| } | ||
|
|
||
| if (request.ApplicationData != null) | ||
| { | ||
| existingReport.ApplicationData = request.ApplicationData; | ||
| } |
…-access-intelligence
| } | ||
|
|
||
| public Task<string> GetReportFileUploadUrlAsync(OrganizationReport report, ReportFile fileData) | ||
| => Task.FromResult($"/reports/organizations/{report.OrganizationId}/{report.Id}/file/report-data"); |
There was a problem hiding this comment.
❌ CRITICAL: Upload URL path does not match the controller route — self-hosted uploads will 404
Details and fix
The controller upload endpoint was renamed in this PR from [HttpPost("{organizationId}/{reportId}/file/report-data")] to [HttpPost("{organizationId}/{reportId}/file")] (controller line 375), but this method still returns the old path with the /report-data suffix. There is no route registered for POST /reports/organizations/{orgId}/{reportId}/file/report-data, so every self-hosted client that follows this URL receives 404.
Additionally, UploadReportFileAsync reads reportFileId from the query string (line 379) and rejects the request if it is missing, but the generated URL does not include that query parameter.
| => Task.FromResult($"/reports/organizations/{report.OrganizationId}/{report.Id}/file/report-data"); | |
| public Task<string> GetReportFileUploadUrlAsync(OrganizationReport report, ReportFile fileData) | |
| => Task.FromResult($"{_apiBaseUrl}/reports/organizations/{report.OrganizationId}/{report.Id}/file?reportFileId={Uri.EscapeDataString(fileData.Id!)}"); |
Note the existing test GetReportFileUploadUrlAsync_ReturnsApiEndpoint at test/Core.Test/Dirt/Reports/Services/LocalOrganizationReportStorageServiceTests.cs:71 asserts the stale URL and will also need updating. This parallels the previously-fixed download URL mismatch (thread PRRT_kwDOAsltcc52eQH6) — the same rename needs to be applied here.
…-access-intelligence
|
20 participants
🎟️ Tracking
This is a PR for user story PM-31923
📔 Objective
Creating new V2 endpoints for read and update operations on the whole report in the database. This will also include the logic for saving a reportData file in Azure Blob storage and server if self-hosted.
Documentation: