PM-31923 adding the whole report endpoints v2#7228
PM-31923 adding the whole report endpoints v2#7228prograhamming wants to merge 126 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
| /// <returns>An <see cref="OrganizationReportFileResponseModel"/> with upload URL when V2 is enabled, | ||
| /// or an <see cref="OrganizationReportResponseModel"/> otherwise.</returns> | ||
| [HttpPost("{organizationId}")] | ||
| [RequestSizeLimit(Constants.FileSize501mb)] |
There was a problem hiding this comment.
We need to confirm the response can support returning the large data file with the bump in the increased limit. Alternatively, we can remove this since the previous version for v1 (database-stored) reports already had this limitation. I suggest just removing it to keep the legacy save the same as before and reduce additional testing needed.
🤖 Bitwarden Claude Code ReviewOverall Assessment: REQUEST CHANGES This PR introduces V2 organization report endpoints with Azure Blob Storage integration and self-hosted file storage for Access Intelligence. It adds create, update, delete, renew-upload-URL, upload, download, and Azure Event Grid validation endpoints, all gated behind the Code Review DetailsAll findings below were posted in previous review rounds and remain unresolved in the current code:
No new findings were identified in this review round. The above findings from prior rounds should be addressed before merging. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if (fileData == null || fileData.Id != reportFileId) | ||
| { | ||
| throw new NotFoundException("Organization report data not found."); | ||
| throw new NotFoundException(); | ||
| } |
There was a problem hiding this comment.
fileData.Validated check allows re-uploading to already-validated reports
Details and fix
RenewFileUploadUrlAsync (line 382) correctly rejects validated files with fileData.Validated in its guard clause, but this upload endpoint does not. On self-hosted instances, an authorized user could overwrite an already-validated report file with different content. The new file would pass size validation (within the 1 MB leeway) and the report would be silently updated.
Add the same fileData.Validated check used by the renew endpoint:
| if (fileData == null || fileData.Id != reportFileId) | |
| { | |
| throw new NotFoundException("Organization report data not found."); | |
| throw new NotFoundException(); | |
| } | |
| if (fileData == null || fileData.Id != reportFileId || fileData.Validated) | |
| { | |
| throw new NotFoundException(); | |
| } |
There was a problem hiding this comment.
@prograhamming I think this is worth following up on.
…-access-intelligence
…-access-intelligence
| Assert.Equal(100, report.ApplicationCount); | ||
| Assert.Equal(50, report.MemberCount); | ||
| } | ||
| } |
There was a problem hiding this comment.
CreateOrganizationReportCommand.ValidateRequestAsync
Details and fix
The source (CreateOrganizationReportCommand.cs lines 104-117) validates SummaryData, ApplicationData, and ReportMetrics as required fields, but this test file only covers invalid organization and missing ContentEncryptionKey.
Missing test cases:
CreateAsync_MissingSummaryData_ThrowsBadRequestExceptionCreateAsync_MissingApplicationData_ThrowsBadRequestExceptionCreateAsync_MissingReportMetrics_ThrowsBadRequestException
These are the same patterns already tested for UpdateOrganizationReportV2Command (which has full validation coverage in UpdateOrganizationReportV2CommandTests.cs lines 122-219), but they are absent here.
| } | ||
|
|
||
| #endregion | ||
| } |
There was a problem hiding this comment.
UploadReportFileAsync and DownloadReportFileAsync controller endpoints
Details and fix
Two significant new endpoints have zero test coverage:
UploadReportFileAsync (controller line 474) -- the self-hosted file upload endpoint that validates content type, checks report ownership, streams the file, validates size with leeway, and either marks the file as validated or deletes the report. This contains security-relevant logic (authorization, org mismatch check, file size validation with leeway calculation, cleanup on failure).
DownloadReportFileAsync (controller line 546) -- the self-hosted file download endpoint that validates ownership and streams the file. This is a data access path that should verify authorization and org-mismatch rejection.
At minimum, the following test cases should exist for DownloadReportFileAsync:
- Success case returns
FileStreamResult - Report not found throws
NotFoundException - Org mismatch throws
BadRequestException - No file data throws
NotFoundException - Without access throws
NotFoundException
And for UploadReportFileAsync (if Request mocking is feasible with the test harness):
- Missing
reportFileIdthrowsBadRequestException - Report not found throws
NotFoundException - Org mismatch throws
BadRequestException - File ID mismatch throws
NotFoundException
| .DidNotReceive() | ||
| .GetOrganizationReportAsync(Arg.Any<Guid>()); | ||
| Assert.Equal("File size is required.", exception.Message); | ||
| } |
There was a problem hiding this comment.
Details and fix
The controller (OrganizationReportsController.cs lines 114-116) rejects file sizes exceeding Constants.FileSize501mb with a BadRequestException("Max file size is 500 MB."). This file-size cap is a security control preventing storage abuse, but there is no test verifying it.
Add a test case such as:
[Theory, BitAutoData]
public async Task CreateOrganizationReportAsync_V2_FileSizeExceedsMax_ThrowsBadRequestException(
SutProvider<OrganizationReportsController> sutProvider,
Guid orgId,
AddOrganizationReportRequestModel request)
{
request.FileSize = Constants.FileSize501mb + 1;
SetupV2Authorization(sutProvider, orgId);
var exception = await Assert.ThrowsAsync<BadRequestException>(() =>
sutProvider.Sut.CreateOrganizationReportAsync(orgId, request));
Assert.Equal("Max file size is 500 MB.", exception.Message);
}
| 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)
| var applicationDataResponse = await _organizationReportRepo.GetApplicationDataAsync(reportId); | ||
|
|
||
| if (applicationDataResponse == null) | ||
| { | ||
| _logger.LogError(ex, "Error fetching organization report application data for organization {organizationId} and report {reportId}", | ||
| organizationId, reportId); | ||
| throw; | ||
| throw new NotFoundException("Organization report application data not found."); | ||
| } | ||
|
|
||
| return applicationDataResponse; |
There was a problem hiding this comment.
🔴 CRITICAL: IDOR — organizationId is accepted but never used to verify report ownership
The organizationId parameter is validated for emptiness but never used to verify that the fetched report actually belongs to the requesting organization. The repository method GetApplicationDataAsync(reportId) fetches data by reportId alone.
Impact: A user with AccessReports permission on Organization A could retrieve Organization B's application data by calling this endpoint with Org A's ID but Org B's reportId. The controller authorizes the caller against the URL's organizationId, but the query bypasses that by fetching solely on reportId.
Every other endpoint in this controller (e.g., GetOrganizationReportAsync, UpdateOrganizationReportAsync, DeleteOrganizationReportAsync, DownloadReportFileAsync) fetches the report and then checks report.OrganizationId != organizationId — this query is the outlier.
Suggested fix: Fetch the full report first (or add an org filter to the repository query), then verify report.OrganizationId == organizationId before returning the application data.
| var applicationDataResponse = await _organizationReportRepo.GetApplicationDataAsync(reportId); | |
| if (applicationDataResponse == null) | |
| { | |
| _logger.LogError(ex, "Error fetching organization report application data for organization {organizationId} and report {reportId}", | |
| organizationId, reportId); | |
| throw; | |
| throw new NotFoundException("Organization report application data not found."); | |
| } | |
| return applicationDataResponse; | |
| 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); | |
| if (applicationDataResponse == null) | |
| { | |
| throw new NotFoundException("Organization report application data not found."); | |
| } | |
| return applicationDataResponse; |
There was a problem hiding this comment.
| 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
| if (request.ContentEncryptionKey != null) | ||
| { | ||
| existingReport.ContentEncryptionKey = request.ContentEncryptionKey; | ||
| } | ||
|
|
||
| if (request.SummaryData != null) | ||
| { | ||
| existingReport.SummaryData = request.SummaryData; | ||
| } | ||
|
|
||
| if (request.ApplicationData != null) | ||
| { | ||
| existingReport.ApplicationData = request.ApplicationData; | ||
| } | ||
|
|
||
| if (request.ReportMetrics != null) | ||
| { | ||
| existingReport.ApplicationCount = request.ReportMetrics.ApplicationCount; | ||
| existingReport.ApplicationAtRiskCount = request.ReportMetrics.ApplicationAtRiskCount; | ||
| existingReport.CriticalApplicationCount = request.ReportMetrics.CriticalApplicationCount; | ||
| existingReport.CriticalApplicationAtRiskCount = request.ReportMetrics.CriticalApplicationAtRiskCount; | ||
| existingReport.MemberCount = request.ReportMetrics.MemberCount; | ||
| existingReport.MemberAtRiskCount = request.ReportMetrics.MemberAtRiskCount; | ||
| existingReport.CriticalMemberCount = request.ReportMetrics.CriticalMemberCount; | ||
| existingReport.CriticalMemberAtRiskCount = request.ReportMetrics.CriticalMemberAtRiskCount; | ||
| existingReport.PasswordCount = request.ReportMetrics.PasswordCount; | ||
| existingReport.PasswordAtRiskCount = request.ReportMetrics.PasswordAtRiskCount; | ||
| existingReport.CriticalPasswordCount = request.ReportMetrics.CriticalPasswordCount; | ||
| existingReport.CriticalPasswordAtRiskCount = request.ReportMetrics.CriticalPasswordAtRiskCount; | ||
| } |
There was a problem hiding this comment.
🟣 SUGGESTION: Dead conditional branches — validation makes null checks unreachable
ValidateRequestAsync (lines 108-126) requires all four fields (ContentEncryptionKey, SummaryData, ApplicationData, ReportMetrics) to be non-null/non-empty. Since validation runs before update logic, these if (field != null) guards are dead code — the null branch is unreachable.
This also contradicts the [HttpPatch] semantics of the controller endpoint, which implies partial updates should be supported.
If this is intentionally a full replacement (not a partial patch), consider either:
- Removing the null guards (since they're dead code), or
- Changing the HTTP verb to
PUTto accurately reflect semantics
If partial updates are desired, relax the validation to allow optional fields.
| } | ||
|
|
||
| #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.
| /// <param name="reportId">The unique identifier of the report to attach the file to.</param> | ||
| /// <param name="reportFileId">The identifier of the report file entry to upload against.</param> | ||
| [RequireFeature(FeatureFlagKeys.AccessIntelligenceVersion2)] | ||
| [HttpPost("{organizationId}/{reportId}/file/report-data")] |
There was a problem hiding this comment.
| [HttpPost("{organizationId}/{reportId}/file/report-data")] | |
| [HttpPost("{organizationId}/{reportId}/file")] |
| 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); |
| var applicationDataResponse = await _organizationReportRepo.GetApplicationDataAsync(reportId); | ||
|
|
||
| if (applicationDataResponse == null) | ||
| { | ||
| _logger.LogError(ex, "Error fetching organization report application data for organization {organizationId} and report {reportId}", | ||
| organizationId, reportId); | ||
| throw; | ||
| throw new NotFoundException("Organization report application data not found."); | ||
| } | ||
|
|
||
| return applicationDataResponse; |
There was a problem hiding this comment.
| if (request.ContentEncryptionKey != null) | ||
| { | ||
| existingReport.ContentEncryptionKey = request.ContentEncryptionKey; | ||
| } | ||
|
|
||
| if (request.SummaryData != null) | ||
| { | ||
| existingReport.SummaryData = request.SummaryData; | ||
| } | ||
|
|
||
| if (request.ApplicationData != null) | ||
| { | ||
| existingReport.ApplicationData = request.ApplicationData; | ||
| } |
| if (organizationId == Guid.Empty) | ||
| { | ||
| throw new BadRequestException("OrganizationId is required."); | ||
| } | ||
|
|
||
| if (reportId == Guid.Empty) | ||
| { | ||
| throw new BadRequestException("ReportId is required."); | ||
| } | ||
|
|
||
| await AuthorizeAsync(organizationId); | ||
|
|
There was a problem hiding this comment.
⛏️ Duplicated across many endpoints. While not a bug, prefer DRY. Consider extracting to a helper
private async Task<OrganizationReport> GetAuthorizedReportAsync(Guid organizationId, Guid reportId)
{
if (organizationId == Guid.Empty) throw new BadRequestException("OrganizationId is required.");
if (reportId == Guid.Empty) throw new BadRequestException("ReportId is required.");
await AuthorizeAsync(organizationId);
var report = await _organizationReportRepo.GetByIdAsync(reportId);
if (report == null) throw new NotFoundException();
if (report.OrganizationId != organizationId) throw new BadRequestException("Invalid report ID");
return report;
}
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: