1384 create report endpoint in broker #793
Conversation
📝 WalkthroughWalkthroughAdds a statistics API key across infra and CI/CD, wires it into the container app, and introduces a secured endpoint that generates and returns a daily Parquet summary report with per‑IP rate limiting; includes report generation handlers, models, repository queries, integration adjustments, tests updates, and documentation. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.2.334).azure/modules/containerApp/main.bicepThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
taihuy
added
the
kind/feature
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // Check rate limiting first | ||
| if (rateLimitInfo.Remaining <= 0) | ||
| { | ||
| _logger.LogWarning("Rate limit exceeded for IP: {ClientIp}", clientIp); |
Check failure
Code scanning / CodeQL
Log entries created from user input High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
To fix this problem, we need to sanitize the value of clientIp before it is written to the logs. Specifically, newline characters (\r, \n) and other suspicious characters should be removed or replaced, so an attacker cannot insert line breaks or control the formatting of log entries. This should be applied just before logging, or more generally, as soon as the client IP is sourced from user-controlled headers. The simplest robust fix is to add a helper function that strips any \r and \n characters, and then to use the sanitized value in all log statements involving clientIp.
To maintain local reasoning, we'll (a) add a static private method, e.g., SanitizeForLog(string input), that removes \r and \n from the string, and (b) sanitize clientIp before it's logged. The fix should be applied to all log lines that use clientIp, not just the one at line 39.
| @@ -19,6 +19,14 @@ | ||
| private const int RateLimitMaxRequests = 10; // Maximum 10 requests | ||
| private const int RateLimitWindowMinutes = 1; // Within 1 minute | ||
|
|
||
| /// <summary> | ||
| /// Sanitizes a string for logging by removing carriage return and newline characters. | ||
| /// </summary> | ||
| private static string SanitizeForLog(string input) | ||
| { | ||
| return input.Replace("\r", "").Replace("\n", ""); | ||
| } | ||
|
|
||
| public StatisticsApiKeyFilter(ILogger<StatisticsApiKeyFilter> logger, IOptions<StatisticsApiKeyOptions> options) | ||
| { | ||
| _logger = logger; | ||
| @@ -28,6 +36,7 @@ | ||
| public void OnAuthorization(AuthorizationFilterContext context) | ||
| { | ||
| var clientIp = GetClientIpAddress(context.HttpContext); | ||
| var sanitizedClientIp = SanitizeForLog(clientIp); | ||
| var rateLimitInfo = GetRateLimitInfo(clientIp); | ||
|
|
||
| // Add rate limit headers to response | ||
| @@ -36,7 +45,7 @@ | ||
| // Check rate limiting first | ||
| if (rateLimitInfo.Remaining <= 0) | ||
| { | ||
| _logger.LogWarning("Rate limit exceeded for IP: {ClientIp}", clientIp); | ||
| _logger.LogWarning("Rate limit exceeded for IP: {ClientIp}", sanitizedClientIp); | ||
| context.Result = new ContentResult | ||
| { | ||
| StatusCode = (int)HttpStatusCode.TooManyRequests, | ||
| @@ -48,7 +57,7 @@ | ||
| // Check API key | ||
| if (!context.HttpContext.Request.Headers.TryGetValue("X-API-Key", out var extractedApiKey)) | ||
| { | ||
| _logger.LogWarning("Missing API key in request from IP: {ClientIp}", clientIp); | ||
| _logger.LogWarning("Missing API key in request from IP: {ClientIp}", sanitizedClientIp); | ||
| context.Result = new ContentResult | ||
| { | ||
| StatusCode = (int)HttpStatusCode.Unauthorized, | ||
| @@ -59,7 +68,7 @@ | ||
|
|
||
| if (!string.Equals(extractedApiKey, _validApiKey, StringComparison.Ordinal)) | ||
| { | ||
| _logger.LogWarning("Invalid API key attempt from IP: {ClientIp}", clientIp); | ||
| _logger.LogWarning("Invalid API key attempt from IP: {ClientIp}", sanitizedClientIp); | ||
| context.Result = new ContentResult | ||
| { | ||
| StatusCode = (int)HttpStatusCode.Forbidden, | ||
| @@ -71,7 +80,7 @@ | ||
| // Log this successful request for rate limiting | ||
| LogRequest(clientIp); | ||
|
|
||
| _logger.LogInformation("Successful API key authentication from IP: {ClientIp}", clientIp); | ||
| _logger.LogInformation("Successful API key authentication from IP: {ClientIp}", sanitizedClientIp); | ||
| } | ||
|
|
||
| private static RateLimitInfo GetRateLimitInfo(string clientIp) |
| // Check API key | ||
| if (!context.HttpContext.Request.Headers.TryGetValue("X-API-Key", out var extractedApiKey)) | ||
| { | ||
| _logger.LogWarning("Missing API key in request from IP: {ClientIp}", clientIp); |
Check failure
Code scanning / CodeQL
Log entries created from user input High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
To fix the problem, we should sanitize the clientIp variable before logging it, specifically removing any newline (\r, \n) characters. This ensures a malicious user cannot inject new log entries or cause log forging. The best place to sanitize is directly before logging. To prevent future mistakes, a helper method can be added to consistently clean loggable input. Edits should be made:
- In all logger calls with
{ClientIp}(lines 39, 51, 62, 74), sanitize theclientIpargument. - Introduce a static method (
SanitizeForLog(string input)) to encapsulate the newline removal, inserted in the same file. - Update logger calls to use the sanitized value.
| @@ -15,6 +15,14 @@ | ||
| { | ||
| private readonly ILogger<StatisticsApiKeyFilter> _logger; | ||
| private readonly string _validApiKey; | ||
|
|
||
| /// <summary> | ||
| /// Sanitizes input for safe logging (removes new line characters) | ||
| /// </summary> | ||
| private static string SanitizeForLog(string input) | ||
| { | ||
| return input.Replace("\r", "").Replace("\n", ""); | ||
| } | ||
| private static readonly ConcurrentDictionary<string, Queue<DateTime>> _requestLog = new(); | ||
| private const int RateLimitMaxRequests = 10; // Maximum 10 requests | ||
| private const int RateLimitWindowMinutes = 1; // Within 1 minute | ||
| @@ -36,7 +44,7 @@ | ||
| // Check rate limiting first | ||
| if (rateLimitInfo.Remaining <= 0) | ||
| { | ||
| _logger.LogWarning("Rate limit exceeded for IP: {ClientIp}", clientIp); | ||
| _logger.LogWarning("Rate limit exceeded for IP: {ClientIp}", SanitizeForLog(clientIp)); | ||
| context.Result = new ContentResult | ||
| { | ||
| StatusCode = (int)HttpStatusCode.TooManyRequests, | ||
| @@ -48,7 +56,7 @@ | ||
| // Check API key | ||
| if (!context.HttpContext.Request.Headers.TryGetValue("X-API-Key", out var extractedApiKey)) | ||
| { | ||
| _logger.LogWarning("Missing API key in request from IP: {ClientIp}", clientIp); | ||
| _logger.LogWarning("Missing API key in request from IP: {ClientIp}", SanitizeForLog(clientIp)); | ||
| context.Result = new ContentResult | ||
| { | ||
| StatusCode = (int)HttpStatusCode.Unauthorized, | ||
| @@ -59,7 +67,7 @@ | ||
|
|
||
| if (!string.Equals(extractedApiKey, _validApiKey, StringComparison.Ordinal)) | ||
| { | ||
| _logger.LogWarning("Invalid API key attempt from IP: {ClientIp}", clientIp); | ||
| _logger.LogWarning("Invalid API key attempt from IP: {ClientIp}", SanitizeForLog(clientIp)); | ||
| context.Result = new ContentResult | ||
| { | ||
| StatusCode = (int)HttpStatusCode.Forbidden, | ||
| @@ -71,7 +79,7 @@ | ||
| // Log this successful request for rate limiting | ||
| LogRequest(clientIp); | ||
|
|
||
| _logger.LogInformation("Successful API key authentication from IP: {ClientIp}", clientIp); | ||
| _logger.LogInformation("Successful API key authentication from IP: {ClientIp}", SanitizeForLog(clientIp)); | ||
| } | ||
|
|
||
| private static RateLimitInfo GetRateLimitInfo(string clientIp) |
|
|
||
| if (!string.Equals(extractedApiKey, _validApiKey, StringComparison.Ordinal)) | ||
| { | ||
| _logger.LogWarning("Invalid API key attempt from IP: {ClientIp}", clientIp); |
Check failure
Code scanning / CodeQL
Log entries created from user input High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
The best way to fix this issue is to sanitize the value of clientIp before logging it. Since the log entry is plain text structured logging (not HTML), the appropriate mitigation is to remove newline characters and other potentially suspicious characters from the IP string. This can be achieved by defining a helper method to clean the string: remove \r, \n, and possibly trim whitespace. Then, pass the cleaned IP string to all log statements referencing clientIp.
Changes should be made in the file src/Altinn.Broker.API/Filters/StatisticsApiKeyFilter.cs:
- Create a private static helper method, e.g.,
SanitizeForLog(string input), that strips any newline (and carriage return) characters from input. - In all
_logger.Log...calls whereclientIpis used (lines 39, 51, 62, 74), passSanitizeForLog(clientIp)instead ofclientIp.
Required: The helper method and adapting 4 log statements.
| @@ -19,6 +19,14 @@ | ||
| private const int RateLimitMaxRequests = 10; // Maximum 10 requests | ||
| private const int RateLimitWindowMinutes = 1; // Within 1 minute | ||
|
|
||
| /// <summary> | ||
| /// Sanitizes input for logging: strips newlines and trims whitespace. | ||
| /// </summary> | ||
| private static string SanitizeForLog(string input) | ||
| { | ||
| return input.Replace("\r", "").Replace("\n", "").Trim(); | ||
| } | ||
|
|
||
| public StatisticsApiKeyFilter(ILogger<StatisticsApiKeyFilter> logger, IOptions<StatisticsApiKeyOptions> options) | ||
| { | ||
| _logger = logger; | ||
| @@ -36,7 +44,7 @@ | ||
| // Check rate limiting first | ||
| if (rateLimitInfo.Remaining <= 0) | ||
| { | ||
| _logger.LogWarning("Rate limit exceeded for IP: {ClientIp}", clientIp); | ||
| _logger.LogWarning("Rate limit exceeded for IP: {ClientIp}", SanitizeForLog(clientIp)); | ||
| context.Result = new ContentResult | ||
| { | ||
| StatusCode = (int)HttpStatusCode.TooManyRequests, | ||
| @@ -48,7 +56,7 @@ | ||
| // Check API key | ||
| if (!context.HttpContext.Request.Headers.TryGetValue("X-API-Key", out var extractedApiKey)) | ||
| { | ||
| _logger.LogWarning("Missing API key in request from IP: {ClientIp}", clientIp); | ||
| _logger.LogWarning("Missing API key in request from IP: {ClientIp}", SanitizeForLog(clientIp)); | ||
| context.Result = new ContentResult | ||
| { | ||
| StatusCode = (int)HttpStatusCode.Unauthorized, | ||
| @@ -59,7 +67,7 @@ | ||
|
|
||
| if (!string.Equals(extractedApiKey, _validApiKey, StringComparison.Ordinal)) | ||
| { | ||
| _logger.LogWarning("Invalid API key attempt from IP: {ClientIp}", clientIp); | ||
| _logger.LogWarning("Invalid API key attempt from IP: {ClientIp}", SanitizeForLog(clientIp)); | ||
| context.Result = new ContentResult | ||
| { | ||
| StatusCode = (int)HttpStatusCode.Forbidden, | ||
| @@ -71,7 +79,7 @@ | ||
| // Log this successful request for rate limiting | ||
| LogRequest(clientIp); | ||
|
|
||
| _logger.LogInformation("Successful API key authentication from IP: {ClientIp}", clientIp); | ||
| _logger.LogInformation("Successful API key authentication from IP: {ClientIp}", SanitizeForLog(clientIp)); | ||
| } | ||
|
|
||
| private static RateLimitInfo GetRateLimitInfo(string clientIp) |
| // Log this successful request for rate limiting | ||
| LogRequest(clientIp); | ||
|
|
||
| _logger.LogInformation("Successful API key authentication from IP: {ClientIp}", clientIp); |
Check failure
Code scanning / CodeQL
Log entries created from user input High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
To address the vulnerability, any user input used in log statements—here, the value of clientIp which originates from X-Forwarded-For—should be sanitized to remove newline and carriage return characters, which can be used to forge new log entries.
General approach:
- Sanitize
clientIpbefore logging, removing line breaks and marking or quoting user input in log messages.
Best detailed fix:
- Add a helper method to sanitize/untaint IP address strings, stripping all
\rand\n(and optionally trimming whitespace). - Apply sanitization in all log statements where
clientIpis used, especially line 74 (and optionally, lines 39, 51, 62).
File/region/lines to change:
src/Altinn.Broker.API/Filters/StatisticsApiKeyFilter.cs- Add a static method to sanitize strings (above/on line 140, before end of class).
- In
OnAuthorization, ensure all log statements usingclientIplog the sanitized value.
Imports/definitions needed:
- No new imports required, as
string.Replaceis sufficient.
| @@ -28,6 +28,7 @@ | ||
| public void OnAuthorization(AuthorizationFilterContext context) | ||
| { | ||
| var clientIp = GetClientIpAddress(context.HttpContext); | ||
| var sanitizedClientIp = SanitizeForLog(clientIp); // strip line breaks from user input | ||
| var rateLimitInfo = GetRateLimitInfo(clientIp); | ||
|
|
||
| // Add rate limit headers to response | ||
| @@ -36,7 +37,7 @@ | ||
| // Check rate limiting first | ||
| if (rateLimitInfo.Remaining <= 0) | ||
| { | ||
| _logger.LogWarning("Rate limit exceeded for IP: {ClientIp}", clientIp); | ||
| _logger.LogWarning("Rate limit exceeded for IP: {ClientIp}", sanitizedClientIp); | ||
| context.Result = new ContentResult | ||
| { | ||
| StatusCode = (int)HttpStatusCode.TooManyRequests, | ||
| @@ -48,7 +49,7 @@ | ||
| // Check API key | ||
| if (!context.HttpContext.Request.Headers.TryGetValue("X-API-Key", out var extractedApiKey)) | ||
| { | ||
| _logger.LogWarning("Missing API key in request from IP: {ClientIp}", clientIp); | ||
| _logger.LogWarning("Missing API key in request from IP: {ClientIp}", sanitizedClientIp); | ||
| context.Result = new ContentResult | ||
| { | ||
| StatusCode = (int)HttpStatusCode.Unauthorized, | ||
| @@ -59,7 +60,7 @@ | ||
|
|
||
| if (!string.Equals(extractedApiKey, _validApiKey, StringComparison.Ordinal)) | ||
| { | ||
| _logger.LogWarning("Invalid API key attempt from IP: {ClientIp}", clientIp); | ||
| _logger.LogWarning("Invalid API key attempt from IP: {ClientIp}", sanitizedClientIp); | ||
| context.Result = new ContentResult | ||
| { | ||
| StatusCode = (int)HttpStatusCode.Forbidden, | ||
| @@ -71,7 +72,7 @@ | ||
| // Log this successful request for rate limiting | ||
| LogRequest(clientIp); | ||
|
|
||
| _logger.LogInformation("Successful API key authentication from IP: {ClientIp}", clientIp); | ||
| _logger.LogInformation("Successful API key authentication from IP: {ClientIp}", sanitizedClientIp); | ||
| } | ||
|
|
||
| private static RateLimitInfo GetRateLimitInfo(string clientIp) | ||
| @@ -137,6 +138,15 @@ | ||
| // Fallback to RemoteIpAddress | ||
| return context.Connection.RemoteIpAddress?.ToString() ?? "unknown"; | ||
| } | ||
| /// <summary> | ||
| /// Removes \r and \n characters to prevent log forging attacks. Trims whitespace. | ||
| /// </summary> | ||
| private static string SanitizeForLog(string value) | ||
| { | ||
| return string.IsNullOrEmpty(value) | ||
| ? value | ||
| : value.Replace("\r", "").Replace("\n", "").Trim(); | ||
| } | ||
| } | ||
|
|
||
| /// <summary> |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Altinn.Broker.Integrations/Altinn/ResourceRegistry/AltinnResourceRegistryRepository.cs (1)
41-55: Treat 404/204 as “not found”, don’t throw in GetResourceEntity.If the registry returns NotFound/NoContent, GetResource returns null. Throwing here misclassifies “not found” as a 500. Return null and let callers handle absence.
Apply this diff:
- if (altinnResourceResponse is null) - { - _logger.LogError("Failed to deserialize response from Altinn Resource Registry"); - throw new BadHttpRequestException("Failed to process response from Altinn Resource Registry"); - } + if (altinnResourceResponse is null) + { + return null; + }
🧹 Nitpick comments (17)
.github/workflows/deploy-to-environment.yml (1)
103-103: Consider if this is necessary.Unlike other secrets (e.g.,
MASKINPORTEN_JWK,PLATFORM_SUBSCRIPTION_KEY,SLACK_URL), which are only passed to the infrastructure update step and then read from Key Vault during application deployment,STATISTICS_API_KEYis passed to both infrastructure and release steps. Since the infrastructure deployment stores the secret in Key Vault, the application deployment should be able to reference it from there without requiring the secret value again.Review whether passing
STATISTICS_API_KEYto the release-version step is necessary, or if it can be removed to follow the pattern established by other secrets..github/actions/release-version/action.yml (1)
39-41: Consider if this input is necessary.Similar to the concern raised in the workflow file, this action receives
STATISTICS_API_KEYeven though other secrets (stored in Key Vault during infrastructure deployment) are not passed as inputs here. The application deployment typically references secrets from Key Vault rather than receiving them as deployment-time inputs.Verify whether this input is required or can be removed to align with the pattern used for other secrets.
.azure/applications/api/params.bicepparam (1)
14-14: Consider if this parameter is necessary.This parameter appears in the application deployment configuration, but examining the container app module reveals that while
statisticsApiKeyis accepted as a parameter, it is not directly used—the secret is instead referenced from Key Vault. Other secrets (e.g.,maskinporten-jwk,platform-subscription-key) are not passed as parameters to the application deployment; they are only defined in infrastructure deployment.Review whether this parameter can be removed to follow the established pattern.
.azure/applications/api/main.bicep (2)
23-24: Parameter may be unnecessary.This parameter is passed to the
containerAppmodule (Line 109), but the module does not use it directly—instead, it references the secret from Key Vault. Consider removing this parameter if it serves no purpose, aligning with how other secrets are handled.
109-109: Verify if this parameter is used.The
statisticsApiKeyis passed to thecontainerAppmodule, but examining.azure/modules/containerApp/main.bicepshows the parameter is declared but not used—the secret is retrieved from Key Vault instead. If the parameter is not utilized, consider removing it here and from the module definition..azure/modules/containerApp/main.bicep (1)
22-23: Parameter appears unused.The
statisticsApiKeyparameter is declared but not referenced anywhere in the module. The secret is retrieved from Key Vault via thesecretsarray (Lines 137-141) and referenced in the environment variables (Line 66). Consider removing this unused parameter, unless it serves a purpose not evident from the current code.src/Altinn.Broker.Core/Repositories/IFileTransferRepository.cs (1)
33-38: Consider the return type for potentially large datasets.The method returns
Task<List<DailySummaryData>>, which loads all matching records into memory. If the summary data grows large over time, this could cause memory pressure.Consider:
- Adding pagination parameters (skip/take or cursor-based)
- Using
IReadOnlyList<DailySummaryData>to prevent modification- For very large datasets, consider
IAsyncEnumerable<DailySummaryData>Verify the expected data volumes with the following search for the implementation:
#!/bin/bash # Find the implementation of GetDailySummaryData ast-grep --pattern $'GetDailySummaryData($$$) { $$$ }' # Check for any pagination or limit logic rg -n -A 10 'GetDailySummaryData' --type cssrc/Altinn.Broker.Application/Altinn.Broker.Application.csproj (2)
16-16: Upgrade Parquet.Net to latest stable (5.2.0)
Current csproj pins 5.0.2—latest non-prerelease is 5.2.0. No known security vulnerabilities found.
12-12: Update or remove explicitMicrosoft.AspNetCore.Http.Abstractionsreference
- src/Altinn.Broker.Application/Altinn.Broker.Application.csproj line 12: bump version from 2.2.0 to 2.3.0.
- Or switch to
<Project Sdk="Microsoft.NET.Sdk.Web" />and drop this PackageReference to rely on the shared framework.README_REPORT_GENERATION.md (1)
30-94: Docs/impl mismatches and CI lint/secret scanners
- Behavior mismatch: README states 404 (No file transfers), but current handler throws and returns 500. Recommend handler to return a dedicated NoFileTransfersFound error instead of throwing, and controller will propagate 404.
- Unbounded dataset: Endpoint “no filters” loads all history into memory. Risky for large datasets. Recommend optional query params (fromDate, toDate, resourceId) and enforce at least a date range. Align README accordingly.
- gitleaks: Example API keys in curl can trigger scanners. Replace with placeholders like <API_KEY> and avoid showing a concrete dev key in docs.
- markdownlint: Add code fence languages for HTTP response examples.
Examples:
-Content-Disposition: attachment; filename=broker_20251014_141120_daily_summary_report_production.parquet +Content-Disposition: attachment; filename=broker_20251014_141120_daily_summary_report_production.parquetAnnotate fenced blocks:
-``` +```http HTTP/1.1 200 OK ...Sanitize keys: ```diff - -H "X-API-Key: dev-statistics-api-key-12345" + -H "X-API-Key: <DEV_API_KEY>"Recommend documenting optional query params:
- fromDate (ISO 8601, UTC), toDate, resourceId
And update status codes to match actual error handling once implemented. See GenerateDailySummaryReportHandler for the 404 path.
Also applies to: 95-166, 277-364
src/Altinn.Broker.Application/GenerateReport/ReportMetadata.cs (1)
24-28: Optional: Prefer DateTimeOffset for UTC timestamp clarityGeneratedAt is set with UtcNow; using DateTimeOffset improves precision/intent and serializing to "O" remains fine.
- public DateTime GeneratedAt { get; init; } + public DateTimeOffset GeneratedAt { get; init; }src/Altinn.Broker.API/Controllers/StatisticsController.cs (1)
54-85: Return 404 when no data; consider cache headers
- Current handler throws on empty data and maps to 500. Align with README by returning a dedicated NoFileTransfersFound error (404) from handler instead of throwing; controller mapping then works as-is.
- Optional: Add [ResponseCache(NoStore = true, Location = ResponseCacheLocation.None)] to prevent unintended caching of per-request report outputs.
Proposed handler adjustment (outside this file):
- if (summaryData.Count == 0) - { - throw new InvalidOperationException("No file transfers found"); - } + if (summaryData.Count == 0) + { + return Errors.NoFileTransfersFoundForReport; // maps to 404 + }src/Altinn.Broker.API/Filters/StatisticsApiKeyFilter.cs (2)
128-139: Don’t trust X-Forwarded-For blindly; use ForwardedHeaders middleware or restrict trusted proxies.An arbitrary client can spoof X-Forwarded-For and evade per‑IP limits. Prefer RemoteIpAddress with ASP.NET Core ForwardedHeaders configured, or only honor XFF when coming from known proxies.
You can verify middleware setup in Program.cs for:
- app.UseForwardedHeaders(...)
- ForwardedHeadersOptions with KnownProxies/KnownNetworks
16-21: Prevent unbounded growth of _requestLog and align rate-limit headers with RFC.
- _requestLog retains empty queues forever (memory growth). Remove entries when a queue becomes empty.
- Consider standard “RateLimit-*” headers (RFC 9568) for better interoperability.
Example adjustment inside GetRateLimitInfo:
lock (requests) { // Remove requests older than the rate limit window while (requests.Count > 0 && requests.Peek() < now.AddMinutes(-RateLimitWindowMinutes)) { requests.Dequeue(); } + var count = requests.Count; + if (count == 0) + { + // best-effort cleanup + _requestLog.TryRemove(clientIp, out _); + } - var remaining = Math.max(0, RateLimitMaxRequests - requests.Count); + var remaining = Math.Max(0, RateLimitMaxRequests - count);Also applies to: 77-101, 115-126
src/Altinn.Broker.Integrations/Altinn/ResourceRegistry/AltinnResourceRegistryRepository.cs (1)
26-40: Optional: downgrade error body logging or guard size.Logging full response bodies for non‑OK can leak details and bloat logs. Consider logging status code and a truncated body at Debug level.
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs (2)
51-55: Avoid throwing on empty result; return an empty Parquet file or 204.Throwing yields a 500. Prefer returning an empty Parquet (valid file with 0 rows) or signaling NoContent to the controller.
87-98: Avoid magic string for HttpContext.Items key.Define a const string or a typed accessor to prevent typos across layers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
.azure/applications/api/main.bicep(2 hunks).azure/applications/api/params.bicepparam(1 hunks).azure/infrastructure/main.bicep(2 hunks).azure/infrastructure/params.bicepparam(1 hunks).azure/modules/containerApp/main.bicep(3 hunks).github/actions/release-version/action.yml(2 hunks).github/actions/update-infrastructure/action.yml(2 hunks).github/workflows/deploy-to-environment.yml(2 hunks)README_REPORT_GENERATION.md(1 hunks)src/Altinn.Broker.API/Controllers/StatisticsController.cs(1 hunks)src/Altinn.Broker.API/Filters/StatisticsApiKeyFilter.cs(1 hunks)src/Altinn.Broker.API/Program.cs(3 hunks)src/Altinn.Broker.Application/Altinn.Broker.Application.csproj(1 hunks)src/Altinn.Broker.Application/DependencyInjection.cs(2 hunks)src/Altinn.Broker.Application/Errors.cs(1 hunks)src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs(1 hunks)src/Altinn.Broker.Application/GenerateReport/ParquetDailySummaryData.cs(1 hunks)src/Altinn.Broker.Application/GenerateReport/ReportMetadata.cs(1 hunks)src/Altinn.Broker.Core/Domain/DailySummaryData.cs(1 hunks)src/Altinn.Broker.Core/Repositories/IAltinnResourceRepoistory.cs(1 hunks)src/Altinn.Broker.Core/Repositories/IFileTransferRepository.cs(1 hunks)src/Altinn.Broker.Integrations/Altinn/ResourceRegistry/AltinnResourceRegistryRepository.cs(3 hunks)src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs(1 hunks)src/Altinn.Broker.Persistence/Repositories/ResourceRepository.cs(1 hunks)tests/Altinn.Broker.Tests/Helpers/CustomWebApplicationFactory.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2024-10-29T12:12:50.346Z
Learnt from: Ceredron
PR: Altinn/altinn-broker#560
File: tests/Altinn.Broker.Tests/Factories/FileTransferEntityFactory.cs:8-45
Timestamp: 2024-10-29T12:12:50.346Z
Learning: When suggesting code improvements in `tests/Altinn.Broker.Tests/Factories/FileTransferEntityFactory.cs`, avoid adding excessive comments and unnecessary code changes.
Applied to files:
tests/Altinn.Broker.Tests/Helpers/CustomWebApplicationFactory.cs
📚 Learning: 2024-10-29T12:51:11.540Z
Learnt from: Ceredron
PR: Altinn/altinn-broker#560
File: tests/Altinn.Broker.Tests/ManifestDownloadStreamTests.cs:24-39
Timestamp: 2024-10-29T12:51:11.540Z
Learning: In `tests/Altinn.Broker.Tests/ManifestDownloadStreamTests.cs`, refactoring suggestions aimed at enhancing test organization and coverage may be considered less readable and of dubious value.
Applied to files:
tests/Altinn.Broker.Tests/Helpers/CustomWebApplicationFactory.cs
📚 Learning: 2024-11-14T07:12:14.214Z
Learnt from: Ceredron
PR: Altinn/altinn-broker#582
File: src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs:176-177
Timestamp: 2024-11-14T07:12:14.214Z
Learning: In `src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs`, avoid suggesting to extract inline SQL queries into constants.
Applied to files:
src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs
🧬 Code graph analysis (9)
tests/Altinn.Broker.Tests/Helpers/CustomWebApplicationFactory.cs (2)
tests/Altinn.Broker.Tests/Helpers/TestConstants.cs (1)
TestConstants(2-21)src/Altinn.Broker.Core/Domain/ResourceEntity.cs (1)
ResourceEntity(3-17)
src/Altinn.Broker.Core/Repositories/IFileTransferRepository.cs (1)
src/Altinn.Broker.Core/Domain/DailySummaryData.cs (1)
DailySummaryData(6-13)
src/Altinn.Broker.API/Program.cs (1)
src/Altinn.Broker.API/Filters/StatisticsApiKeyFilter.cs (3)
StatisticsApiKeyOptions(145-153)StatisticsApiKeyFilter(14-140)StatisticsApiKeyFilter(22-26)
src/Altinn.Broker.Persistence/Repositories/FileTransferRepository.cs (1)
src/Altinn.Broker.Core/Domain/DailySummaryData.cs (1)
DailySummaryData(6-13)
src/Altinn.Broker.Application/DependencyInjection.cs (1)
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs (1)
GenerateDailySummaryReportHandler(12-121)
src/Altinn.Broker.Core/Repositories/IAltinnResourceRepoistory.cs (1)
src/Altinn.Broker.Core/Domain/ResourceEntity.cs (1)
ResourceEntity(3-17)
src/Altinn.Broker.Integrations/Altinn/ResourceRegistry/AltinnResourceRegistryRepository.cs (4)
src/Altinn.Broker.Core/Repositories/IAltinnResourceRepoistory.cs (2)
Task(6-6)Task(7-7)src/Altinn.Broker.Persistence/Repositories/ResourceRepository.cs (9)
Task(10-55)Task(57-69)Task(71-81)Task(83-93)Task(95-105)Task(107-117)Task(119-129)Task(131-141)Task(143-153)src/Altinn.Broker.Integrations/Altinn/ResourceRegistry/GetResourceResponse.cs (2)
GetResourceResponse(5-57)HasCompetentAuthority(59-69)src/Altinn.Broker.Core/Domain/ResourceEntity.cs (1)
ResourceEntity(3-17)
src/Altinn.Broker.API/Controllers/StatisticsController.cs (3)
src/Altinn.Broker.API/Filters/StatisticsApiKeyFilter.cs (2)
StatisticsApiKeyFilter(14-140)StatisticsApiKeyFilter(22-26)src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs (1)
GenerateDailySummaryReportHandler(12-121)src/Altinn.Broker.Application/GenerateReport/ReportMetadata.cs (1)
ReportMetadata(7-28)
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs (6)
src/Altinn.Broker.Core/Repositories/IAltinnResourceRepoistory.cs (2)
Task(6-6)Task(7-7)src/Altinn.Broker.Core/Repositories/IFileTransferRepository.cs (10)
Task(7-18)Task(19-19)Task(20-20)Task(21-21)Task(22-22)Task(23-23)Task(24-24)Task(25-31)Task(32-32)Task(33-38)src/Altinn.Broker.Integrations/Altinn/ResourceRegistry/AltinnResourceRegistryRepository.cs (3)
Task(26-40)Task(41-55)Task(57-65)src/Altinn.Broker.Application/Errors.cs (1)
Errors(7-33)src/Altinn.Broker.Application/GenerateReport/ParquetDailySummaryData.cs (1)
ParquetDailySummaryData(9-88)src/Altinn.Broker.Application/GenerateReport/ReportMetadata.cs (1)
ReportMetadata(7-28)
🪛 Gitleaks (8.28.0)
README_REPORT_GENERATION.md
[high] 169-170: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 177-178: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 198-199: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 215-216: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 222-223: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 360-361: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
🪛 LanguageTool
README_REPORT_GENERATION.md
[grammar] ~7-~7: There might be a mistake here.
Context: ...res Implemented ### 1. Data Aggregation - Aggregates file transfers by day, resour...
(QB_NEW_EN)
[grammar] ~8-~8: There might be a mistake here.
Context: ...s by day, resource ID, and service owner - Counts the number of file transfers for ...
(QB_NEW_EN)
[grammar] ~9-~9: There might be a mistake here.
Context: ...r of file transfers for each combination - Returns all historical data (no filters ...
(QB_NEW_EN)
[grammar] ~10-~10: There might be a mistake here.
Context: ...storical data (no filters or parameters) - Joins through: file_transfer → `storag...
(QB_NEW_EN)
[grammar] ~14-~14: There might be a mistake here.
Context: ...es Parquet.Net library (version 5.0.2) - Generates efficient columnar storage for...
(QB_NEW_EN)
[grammar] ~15-~15: There might be a mistake here.
Context: ...erates efficient columnar storage format - Uses JsonPropertyName attributes to ma...
(QB_NEW_EN)
[grammar] ~16-~16: There might be a mistake here.
Context: ...erties to lowercase parquet column names - Schema includes: - date: Date of the...
(QB_NEW_EN)
[grammar] ~17-~17: There might be a mistake here.
Context: ... parquet column names - Schema includes: - date: Date of the file transfers (yyyy-MM-dd...
(QB_NEW_EN)
[grammar] ~18-~18: There might be a mistake here.
Context: ...f the file transfers (yyyy-MM-dd format) - year, month, day: Date components - `s...
(QB_NEW_EN)
[grammar] ~19-~19: There might be a mistake here.
Context: ... year, month, day: Date components - serviceownerorgnr: Service owner organization number - ...
(QB_NEW_EN)
[grammar] ~20-~20: There might be a mistake here.
Context: ...rgnr: Service owner organization number - serviceownercode: Service owner name - serviceresourc...
(QB_NEW_EN)
[grammar] ~21-~21: There might be a mistake here.
Context: ...- serviceownercode: Service owner name - serviceresourceid: Resource identifier - `serviceresour...
(QB_NEW_EN)
[grammar] ~22-~22: There might be a mistake here.
Context: ...serviceresourceid: Resource identifier - serviceresourcetitle: Resource title (from Altinn Resource R...
(QB_NEW_EN)
[grammar] ~23-~23: There might be a mistake here.
Context: ...ce title (from Altinn Resource Registry) - recipienttype: Hardcoded as "Organization" - `costc...
(QB_NEW_EN)
[grammar] ~24-~24: There might be a mistake here.
Context: ...ipienttype: Hardcoded as "Organization" - costcenter`: Hardcoded as "Altinn3" (AltinnVersion)...
(QB_NEW_EN)
[grammar] ~25-~25: There might be a mistake here.
Context: ...: Hardcoded as "Altinn3" (AltinnVersion) - numberoffiletransfers: Count of file transfers - `databases...
(QB_NEW_EN)
[grammar] ~26-~26: There might be a mistake here.
Context: ...ffiletransfers: Count of file transfers - databasestoragebytes: Set to 0 (not tracked) - attachment...
(QB_NEW_EN)
[grammar] ~27-~27: There might be a mistake here.
Context: ...asestoragebytes: Set to 0 (not tracked) - attachmentstoragebytes`: Set to 0 (not tracked) ### 3. Direct ...
(QB_NEW_EN)
[grammar] ~33-~33: There might be a mistake here.
Context: ... daily summary report as a Parquet file. - Method: GET - Authorization: API K...
(QB_NEW_EN)
[grammar] ~34-~34: There might be a mistake here.
Context: ...ort as a Parquet file. - Method: GET - Authorization: API Key via X-API-Key...
(QB_NEW_EN)
[grammar] ~35-~35: There might be a mistake here.
Context: ...PI Key via X-API-Key header (required) - Rate Limiting: 10 requests per minute ...
(QB_NEW_EN)
[grammar] ~36-~36: There might be a mistake here.
Context: ...*: 10 requests per minute per IP address - Query Parameters: None - Response:...
(QB_NEW_EN)
[grammar] ~37-~37: There might be a mistake here.
Context: ... IP address - Query Parameters: None - Response: Parquet file as `application...
(QB_NEW_EN)
[grammar] ~38-~38: There might be a mistake here.
Context: ...cation/octet-streamfor direct download - **Filename Format**:broker_YYYYMMDD_HHmm...
(QB_NEW_EN)
[grammar] ~39-~39: There might be a mistake here.
Context: ... direct download - Filename Format: broker_YYYYMMDD_HHmmss_daily_summary_report_{environment}.parquet - Example: `broker_20251014_141120_daily_s...
(QB_NEW_EN)
[grammar] ~46-~46: There might be a mistake here.
Context: ...te limiting: Report Metadata Headers: - X-Report-Total-Records: Total number of rows in the parquet fi...
(QB_NEW_EN)
[grammar] ~47-~47: There might be a mistake here.
Context: ...Total number of rows in the parquet file - X-Report-Total-FileTransfers: Sum of all file transfers across all r...
(QB_NEW_EN)
[grammar] ~48-~48: There might be a mistake here.
Context: ...of all file transfers across all records - X-Report-Total-ServiceOwners: Count of unique service owners in the ...
(QB_NEW_EN)
[grammar] ~49-~49: There might be a mistake here.
Context: ...t of unique service owners in the report - X-Report-Generated-At: ISO 8601 timestamp when the report was...
(QB_NEW_EN)
[uncategorized] ~52-~52: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... when the report was generated (UTC) Rate Limiting Headers: - X-RateLimit-Limit: Maxim...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[grammar] ~52-~52: There might be a mistake here.
Context: ...enerated (UTC) Rate Limiting Headers: - X-RateLimit-Limit: Maximum number of requests allowed per...
(QB_NEW_EN)
[grammar] ~53-~53: There might be a mistake here.
Context: ...mber of requests allowed per minute (10) - X-RateLimit-Remaining: Number of requests remaining in the cu...
(QB_NEW_EN)
[grammar] ~54-~54: There might be a mistake here.
Context: ...requests remaining in the current window - X-RateLimit-Reset: Unix timestamp when the rate limit win...
(QB_NEW_EN)
[grammar] ~55-~55: There might be a mistake here.
Context: ...estamp when the rate limit window resets - Retry-After: Seconds to wait before retrying (only ...
(QB_NEW_EN)
[grammar] ~74-~74: There might be a mistake here.
Context: ...ndpoint requires API key authentication: - Header: X-API-Key: your-api-key-here...
(QB_NEW_EN)
[grammar] ~75-~75: There might be a mistake here.
Context: ...s API key authentication: - Header: X-API-Key: your-api-key-here - The API key is configured via Azure Key ...
(QB_NEW_EN)
[grammar] ~81-~81: There might be a mistake here.
Context: ...ate limiting is enforced per IP address: - Limit: Maximum 10 requests per minute ...
(QB_NEW_EN)
[grammar] ~82-~82: There might be a mistake here.
Context: ...Limit*: Maximum 10 requests per minute - Scope: Per IP address (using `X-Forwar...
(QB_NEW_EN)
[grammar] ~83-~83: There might be a mistake here.
Context: ...warded-Forheader orRemoteIpAddress`) - Response: HTTP 429 Too Many Requests w...
(QB_NEW_EN)
[grammar] ~98-~98: There might be a mistake here.
Context: ...ler**: Main handler with single method: - Process(): Returns OneOf<Stream, Error>` - just ...
(QB_NEW_EN)
[grammar] ~101-~101: There might be a mistake here.
Context: ...inn Resource Registry - Data Models: - DailySummaryData: Internal data model with clean propert...
(QB_NEW_EN)
[grammar] ~107-~107: There might be a mistake here.
Context: ...ase parquet column names ### Core Layer - Domain: - DailySummaryData: Domain...
(QB_NEW_EN)
[grammar] ~108-~108: There might be a mistake here.
Context: ...lumn names ### Core Layer - Domain: - DailySummaryData: Domain entity representing aggregated ...
(QB_NEW_EN)
[grammar] ~109-~109: There might be a mistake here.
Context: ...ain entity representing aggregated data from database - Repository Extension: ...
(QB_NEW_EN)
[grammar] ~109-~109: There might be a mistake here.
Context: ...presenting aggregated data from database - Repository Extension: - `IFileTransf...
(QB_NEW_EN)
[grammar] ~110-~110: There might be a mistake here.
Context: ...rom database - Repository Extension: - IFileTransferRepository.GetDailySummaryData(): Database query for aggregated data -...
(QB_NEW_EN)
[grammar] ~111-~111: There might be a mistake here.
Context: ...a(): Database query for aggregated data - Uses reader.GetOrdinal("column_name")` ...
(QB_NEW_EN)
[grammar] ~114-~114: There might be a mistake here.
Context: ...afe column access ### Persistence Layer - SQL Query: ```sql SELECT DA...
(QB_NEW_EN)
[grammar] ~142-~142: There might be a mistake here.
Context: ...e Key Vault** (Production/Staging/Test): - Secret name: statistics-api-key - C...
(QB_NEW_EN)
[grammar] ~143-~143: There might be a mistake here.
Context: ...uction/Staging/Test): - Secret name: statistics-api-key - Configured via GitHub secret `STATISTICS...
(QB_NEW_EN)
[grammar] ~144-~144: There might be a mistake here.
Context: ...-key - Configured via GitHub secretSTATISTICS_API_KEY` - Deployed through bicep infrastructure fi...
(QB_NEW_EN)
[grammar] ~148-~148: There might be a mistake here.
Context: ...in appsettings.Development.json under StatisticsApiKey.ApiKey - Default development key: `dev-statistics...
(QB_NEW_EN)
[uncategorized] ~151-~151: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ey: dev-statistics-api-key-12345 ### Rate Limiting Configuration Rate limiting is hard-co...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[grammar] ~153-~153: There might be a mistake here.
Context: ... hard-coded in StatisticsApiKeyFilter: - Maximum requests: 10 per minute - Scope:...
(QB_NEW_EN)
[grammar] ~154-~154: There might be a mistake here.
Context: ...lter`: - Maximum requests: 10 per minute - Scope: Per IP address - Can be modified ...
(QB_NEW_EN)
[grammar] ~155-~155: There might be a mistake here.
Context: ...s: 10 per minute - Scope: Per IP address - Can be modified by updating constants in...
(QB_NEW_EN)
[grammar] ~160-~160: There might be a mistake here.
Context: ...r the specified criteria (404 Not Found) - Error 25 (ReportGenerationFailed): F...
(QB_NEW_EN)
[grammar] ~305-~305: There might be a mistake here.
Context: ...S_API_KEYsecret ### Persistence Layer - ExtendedIFileTransferRepository` with ...
(QB_NEW_EN)
[grammar] ~319-~319: There might be a mistake here.
Context: ...eader.GetOrdinal("column_name")` pattern 9. Clean separation: Domain model (internal...
(QB_NEW_EN)
[grammar] ~320-~320: There might be a mistake here.
Context: ... names) → Parquet model (external names) 10. API key stored in Azure Key Vault and de...
(QB_NEW_EN)
[grammar] ~327-~327: There might be a mistake here.
Context: ...agement 1. Production/Staging/Test: - API key is stored as a GitHub secret per...
(QB_NEW_EN)
[grammar] ~332-~332: There might be a mistake here.
Context: ... the configuration 2. Key Rotation: - Update the GitHub secret `STATISTICS_API...
(QB_NEW_EN)
[grammar] ~336-~336: There might be a mistake here.
Context: ...ck up the new key 3. Rate Limiting: - Prevents abuse by limiting requests to 1...
(QB_NEW_EN)
[grammar] ~353-~353: There might be a mistake here.
Context: ...y> ``` 2. Deploy Infrastructure: - The workflow will automatically deploy t...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.18.1)
README_REPORT_GENERATION.md
59-59: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
183-183: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
348-348: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (csharp)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (18)
.azure/infrastructure/params.bicepparam (1)
17-17: LGTM!The parameter follows the same pattern as other secrets and correctly reads from the environment variable.
.azure/infrastructure/main.bicep (2)
26-27: LGTM!The parameter is correctly marked as
@secure()and follows the established pattern for secret parameters.
62-65: LGTM!The secret entry correctly references the parameter and follows the naming convention used by other secrets in the array.
.github/workflows/deploy-to-environment.yml (1)
75-75: LGTM!The secret is correctly propagated to the infrastructure update step.
.github/actions/release-version/action.yml (1)
72-72: Verify usage of this environment variable.This passes
STATISTICS_API_KEYto the deployment step. Confirm that the Bicep templates require this value during deployment, or if the application can retrieve it from Key Vault instead (following the pattern of other secrets)..github/actions/update-infrastructure/action.yml (2)
48-50: LGTM!The input is correctly defined and follows the pattern established by other secrets.
97-97: LGTM!The secret is correctly propagated to the infrastructure deployment, which will store it in Key Vault for the container app to consume.
.azure/modules/containerApp/main.bicep (2)
66-66: LGTM!The environment variable is correctly configured with a secret reference, following the .NET Core configuration convention (
__for nested properties). This will allow the application to read the API key from the container's secrets.
137-141: LGTM!The secret entry correctly references Key Vault using the managed identity and follows the established pattern for other secrets in the container app configuration.
src/Altinn.Broker.Application/Errors.cs (1)
32-32: LGTM!The new error code follows the existing pattern and uses an appropriate status code for report generation failures.
src/Altinn.Broker.Core/Repositories/IAltinnResourceRepoistory.cs (1)
6-7: LGTM!The method rename from
GetResourcetoGetResourceEntityimproves clarity, and the newGetServiceOwnerNameOfResourcemethod provides useful functionality for retrieving service owner names. The changes are consistently applied across the codebase based on the other files in this PR.src/Altinn.Broker.API/Program.cs (2)
97-97: LGTM!The configuration binding follows the existing pattern and correctly uses the
SectionNameconstant fromStatisticsApiKeyOptions.
106-108: Missing StatisticsApiKey configuration
No occurrences ofStatisticsApiKeyorSTATISTICS_API_KEYwere found in any appsettings*.json. Confirm thatStatisticsApiKey:ApiKeyis defined in all environment-specific configuration (appsettings.{Environment}.json, user secrets, pipeline variables, or environment variables).src/Altinn.Broker.Application/DependencyInjection.cs (1)
35-35: LGTM!The registration of
GenerateDailySummaryReportHandlerfollows the existing pattern for other handlers in the application.src/Altinn.Broker.Persistence/Repositories/ResourceRepository.cs (1)
44-44: LGTM!The method call has been correctly updated to match the renamed interface method
GetResourceEntity.tests/Altinn.Broker.Tests/Helpers/CustomWebApplicationFactory.cs (1)
80-107: LGTM!The test mocks have been correctly updated to use
GetResourceEntityinstead ofGetResource, maintaining the same behavior and return values.src/Altinn.Broker.Application/GenerateReport/ParquetDailySummaryData.cs (2)
41-46: JsonPropertyName values don’t match property semantics
- ParquetDailySummaryData.cs line 44:
ServiceOwnerName→"serviceownercode"implies a code, not a name. Should this be"serviceownername"?- ParquetDailySummaryData.cs line 68:
AltinnVersion→"costcenter"seems unrelated. Should this be"altinnversion"?
1-1: JsonPropertyName controls Parquet column names in Parquet.Net v5
ParquetSerializer v5 honors System.Text.Json’s JsonPropertyName for renaming; no ParquetMember attribute exists. Your existing JsonPropertyName annotations are sufficient.Likely an incorrect or invalid review comment.
| public void OnAuthorization(AuthorizationFilterContext context) | ||
| { | ||
| var clientIp = GetClientIpAddress(context.HttpContext); | ||
| var rateLimitInfo = GetRateLimitInfo(clientIp); | ||
|
|
||
| // Add rate limit headers to response | ||
| AddRateLimitHeaders(context.HttpContext, rateLimitInfo); | ||
|
|
There was a problem hiding this comment.
Rate limiter currently bypassed by invalid/missing API keys. Count every attempt.
Requests without/with wrong X-API-Key are not added to the queue, so attackers can spam without hitting 429. Log/enqueue before checks; remove the later enqueue to avoid double counting.
Apply this diff:
public void OnAuthorization(AuthorizationFilterContext context)
{
- var clientIp = GetClientIpAddress(context.HttpContext);
- var rateLimitInfo = GetRateLimitInfo(clientIp);
+ var clientIp = GetClientIpAddress(context.HttpContext);
+ // Count every attempt (valid or not)
+ LogRequest(clientIp);
+ var rateLimitInfo = GetRateLimitInfo(clientIp);
// Add rate limit headers to response
AddRateLimitHeaders(context.HttpContext, rateLimitInfo);
@@
- // Log this successful request for rate limiting
- LogRequest(clientIp);
-
_logger.LogInformation("Successful API key authentication from IP: {ClientIp}", clientIp);Also applies to: 71-75
| // Fetch aggregated data from repository (all data, no filters) | ||
| var summaryData = await fileTransferRepository.GetDailySummaryData( | ||
| fromDate: null, | ||
| toDate: null, | ||
| resourceId: null, | ||
| cancellationToken); | ||
|
|
There was a problem hiding this comment.
Don’t fetch unbounded data; restrict to a daily window.
The handler requests “all data” (null range). For a daily report, bound the query, e.g., previous UTC day.
Apply this diff:
- // Fetch aggregated data from repository (all data, no filters)
- var summaryData = await fileTransferRepository.GetDailySummaryData(
- fromDate: null,
- toDate: null,
- resourceId: null,
- cancellationToken);
+ // Fetch aggregated data for the previous UTC day
+ var toDate = DateTimeOffset.UtcNow.Date;
+ var fromDate = toDate.AddDays(-1);
+ var summaryData = await fileTransferRepository.GetDailySummaryData(
+ fromDate: fromDate,
+ toDate: toDate,
+ resourceId: null,
+ cancellationToken);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Fetch aggregated data from repository (all data, no filters) | |
| var summaryData = await fileTransferRepository.GetDailySummaryData( | |
| fromDate: null, | |
| toDate: null, | |
| resourceId: null, | |
| cancellationToken); | |
| // Fetch aggregated data for the previous UTC day | |
| var toDate = DateTimeOffset.UtcNow.Date; | |
| var fromDate = toDate.AddDays(-1); | |
| var summaryData = await fileTransferRepository.GetDailySummaryData( | |
| fromDate: fromDate, | |
| toDate: toDate, | |
| resourceId: null, | |
| cancellationToken); |
🤖 Prompt for AI Agents
In
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs
around lines 44 to 50, the call to GetDailySummaryData currently passes null for
fromDate/toDate which fetches unbounded data; change it to restrict to the
previous UTC day by computing fromDate = DateTime.UtcNow.Date.AddDays(-1) (start
of previous day) and toDate = DateTime.UtcNow.Date (start of current day) so the
query covers the full previous UTC day, then pass those variables into
GetDailySummaryData while keeping resourceId and cancellationToken as before.
| ResourceTitle = GetResourceTitle(data.ResourceId), | ||
| RecipientType = "Organization", | ||
| AltinnVersion = "Altinn3", | ||
| FileTransferCount = data.FileTransferCount, |
There was a problem hiding this comment.
ResourceTitle currently fetches service owner name; use the resource “title” field instead and avoid sync over async.
- GetResourceTitle calls GetServiceOwnerNameOfResource, so “serviceresourcetitle” is filled with owner name, not the resource’s title. Wrong data.
- Method blocks with GetAwaiter().GetResult() and ignores cancellation.
Prefer an async method that resolves resource titles (Title nb/nn/en) and prefetches for distinct resourceIds to avoid N+1 calls. If changing interfaces isn’t feasible now, at least pass the cancellation token.
Minimal fix to pass the token:
- ResourceTitle = GetResourceTitle(data.ResourceId),
+ ResourceTitle = GetResourceTitle(data.ResourceId, cancellationToken),- private string GetResourceTitle(string? resourceId)
+ private string GetResourceTitle(string? resourceId, CancellationToken cancellationToken)
{
if (string.IsNullOrEmpty(resourceId) || resourceId == "unknown")
{
return "Unknown";
}
try
{
- var resourceTitle = altinnResourceRepository.GetServiceOwnerNameOfResource(resourceId, CancellationToken.None).GetAwaiter().GetResult();
+ var resourceTitle = altinnResourceRepository.GetServiceOwnerNameOfResource(resourceId, cancellationToken).GetAwaiter().GetResult();
return resourceTitle ?? $"Unknown ({resourceId})";
}Follow-up (recommended): extend IAltinnResourceRepository with a GetResourceTitleOfResource(resourceId, ct) that reads GetResourceResponse.Title with nb→nn→en fallback, and switch to fully async prefetch for distinct IDs.
Also applies to: 103-120
🤖 Prompt for AI Agents
In
src/Altinn.Broker.Application/GenerateReport/GenerateDailySummaryReportHandler.cs
around lines 66 to 69, ResourceTitle is populated by GetResourceTitle which
currently calls GetServiceOwnerNameOfResource (so the title becomes the owner
name) and synchronously blocks with GetAwaiter().GetResult(), ignoring
cancellation; minimally fix this by passing the existing CancellationToken
through to the repository call instead of blocking (replace
GetAwaiter().GetResult() usage with an awaited call or pass ct into the sync
wrapper), and as a follow-up implement an async
IAltinnResourceRepository.GetResourceTitleOfResource(resourceId,
CancellationToken) that reads GetResourceResponse.Title using nb→nn→en fallback
and update the handler to asynchronously prefetch titles for distinct
resourceIds (to avoid N+1 calls) and await them with the cancellation token.
| public class DailySummaryData | ||
| { | ||
| public required DateTimeOffset Date { get; set; } | ||
| public required string ServiceOwnerId { get; set; } | ||
| public required string ServiceOwnerName { get; set; } | ||
| public required string ResourceId { get; set; } | ||
| public required int FileTransferCount { get; set; } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use long for FileTransferCount to match PostgreSQL COUNT and prevent overflow
COUNT(*) is bigint; mapping to int risks exceptions/overflow. Use long.
public class DailySummaryData
{
public required DateTimeOffset Date { get; set; }
public required string ServiceOwnerId { get; set; }
public required string ServiceOwnerName { get; set; }
public required string ResourceId { get; set; }
- public required int FileTransferCount { get; set; }
+ public required long FileTransferCount { get; set; }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public class DailySummaryData | |
| { | |
| public required DateTimeOffset Date { get; set; } | |
| public required string ServiceOwnerId { get; set; } | |
| public required string ServiceOwnerName { get; set; } | |
| public required string ResourceId { get; set; } | |
| public required int FileTransferCount { get; set; } | |
| } | |
| public class DailySummaryData | |
| { | |
| public required DateTimeOffset Date { get; set; } | |
| public required string ServiceOwnerId { get; set; } | |
| public required string ServiceOwnerName { get; set; } | |
| public required string ResourceId { get; set; } | |
| public required long FileTransferCount { get; set; } | |
| } |
🤖 Prompt for AI Agents
In src/Altinn.Broker.Core/Domain/DailySummaryData.cs around lines 6 to 13,
FileTransferCount is defined as int but PostgreSQL COUNT(*) returns bigint;
change the property type to long (Int64) and update any usages/serialization
contracts that expect int so they accept/serialize a long to prevent overflow
and runtime exceptions.
| public async Task<List<DailySummaryData>> GetDailySummaryData( | ||
| DateTimeOffset? fromDate, | ||
| DateTimeOffset? toDate, | ||
| string? resourceId, | ||
| CancellationToken cancellationToken) | ||
| { | ||
| var commandString = new StringBuilder(@" | ||
| SELECT | ||
| DATE(f.created) as date, | ||
| so.service_owner_id_pk, | ||
| so.service_owner_name, | ||
| f.resource_id, | ||
| COUNT(*) as count | ||
| FROM broker.file_transfer f | ||
| INNER JOIN broker.storage_provider sp ON sp.storage_provider_id_pk = f.storage_provider_id_fk | ||
| INNER JOIN broker.service_owner so ON so.service_owner_id_pk = sp.service_owner_id_fk | ||
| WHERE 1=1"); | ||
|
|
||
| if (fromDate.HasValue) | ||
| { | ||
| commandString.AppendLine(" AND f.created >= @fromDate"); | ||
| } | ||
|
|
||
| if (toDate.HasValue) | ||
| { | ||
| commandString.AppendLine(" AND f.created <= @toDate"); | ||
| } | ||
|
|
||
| if (!string.IsNullOrWhiteSpace(resourceId)) | ||
| { | ||
| commandString.AppendLine(" AND f.resource_id = @resourceId"); | ||
| } | ||
|
|
||
| commandString.AppendLine(@" | ||
| GROUP BY DATE(f.created), so.service_owner_id_pk, so.service_owner_name, f.resource_id | ||
| ORDER BY date DESC, so.service_owner_id_pk, f.resource_id"); | ||
|
|
||
| return await commandExecutor.ExecuteWithRetry(async (ct) => | ||
| { | ||
| await using var command = dataSource.CreateCommand(commandString.ToString()); | ||
|
|
||
| if (fromDate.HasValue) | ||
| { | ||
| command.Parameters.AddWithValue("@fromDate", fromDate.Value); | ||
| } | ||
|
|
||
| if (toDate.HasValue) | ||
| { | ||
| command.Parameters.AddWithValue("@toDate", toDate.Value); | ||
| } | ||
|
|
||
| if (!string.IsNullOrWhiteSpace(resourceId)) | ||
| { | ||
| command.Parameters.AddWithValue("@resourceId", resourceId); | ||
| } | ||
|
|
||
| var results = new List<DailySummaryData>(); | ||
|
|
||
| await using var reader = await command.ExecuteReaderAsync(ct); | ||
| while (await reader.ReadAsync(ct)) | ||
| { | ||
| var date = reader.GetDateTime(reader.GetOrdinal("date")); | ||
| var serviceOwnerId = reader.GetString(reader.GetOrdinal("service_owner_id_pk")); | ||
| var serviceOwnerName = reader.GetString(reader.GetOrdinal("service_owner_name")); | ||
| var resourceId = reader.GetString(reader.GetOrdinal("resource_id")); | ||
| var count = reader.GetInt32(reader.GetOrdinal("count")); | ||
|
|
||
| results.Add(new DailySummaryData | ||
| { | ||
| Date = new DateTimeOffset(date, TimeSpan.Zero), | ||
| ServiceOwnerId = serviceOwnerId, | ||
| ServiceOwnerName = serviceOwnerName, | ||
| ResourceId = resourceId, | ||
| FileTransferCount = count | ||
| }); | ||
| } | ||
|
|
||
| return results; | ||
| }, cancellationToken); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Fix COUNT(*) type (PostgreSQL returns bigint) and align DTO; plus validate date parameter types and timezone grouping
- Critical: COUNT(*) is bigint in PostgreSQL. Reading with GetInt32 (Line 743) will throw/cast incorrectly. Read Int64 and make DTO property long.
- Verify column type for f.created. You pass DateTimeOffset (Lines 721, 726) without NpgsqlDbType. If f.created is timestamp without time zone, prefer DateTime (UTC) with NpgsqlDbType.Timestamp; if timestamptz, specify TimestampTz. Current AddWithValue could cause implicit conversions.
- Grouping on DATE(f.created) may be timezone-sensitive. Ensure day-buckets are in UTC consistently (e.g., DATE(f.created AT TIME ZONE 'UTC') if f.created is timestamptz; or explicitly document/standardize).
- Minor: Local variable resourceId (Line 742) shadows method parameter; rename for clarity.
Apply this diff to fix COUNT mapping and shadowing:
- while (await reader.ReadAsync(ct))
+ while (await reader.ReadAsync(ct))
{
- var date = reader.GetDateTime(reader.GetOrdinal("date"));
- var serviceOwnerId = reader.GetString(reader.GetOrdinal("service_owner_id_pk"));
- var serviceOwnerName = reader.GetString(reader.GetOrdinal("service_owner_name"));
- var resourceId = reader.GetString(reader.GetOrdinal("resource_id"));
- var count = reader.GetInt32(reader.GetOrdinal("count"));
+ var date = reader.GetDateTime(reader.GetOrdinal("date"));
+ var serviceOwnerId = reader.GetString(reader.GetOrdinal("service_owner_id_pk"));
+ var serviceOwnerName = reader.GetString(reader.GetOrdinal("service_owner_name"));
+ var resourceIdDb = reader.GetString(reader.GetOrdinal("resource_id"));
+ var count = reader.GetInt64(reader.GetOrdinal("count"));
results.Add(new DailySummaryData
{
Date = new DateTimeOffset(date, TimeSpan.Zero),
ServiceOwnerId = serviceOwnerId,
ServiceOwnerName = serviceOwnerName,
- ResourceId = resourceId,
+ ResourceId = resourceIdDb,
FileTransferCount = count
});
}Optional parameter typing (choose based on f.created type):
- if (fromDate.HasValue)
- {
- command.Parameters.AddWithValue("@fromDate", fromDate.Value);
- }
+ if (fromDate.HasValue)
+ {
+ // If f.created is TIMESTAMP WITHOUT TIME ZONE:
+ // command.Parameters.Add(new NpgsqlParameter("@fromDate", NpgsqlDbType.Timestamp) { Value = fromDate.Value.UtcDateTime });
+ // If f.created is TIMESTAMP WITH TIME ZONE:
+ // command.Parameters.Add(new NpgsqlParameter("@fromDate", NpgsqlDbType.TimestampTz) { Value = fromDate.Value.UtcDateTime });
+ }
- if (toDate.HasValue)
- {
- command.Parameters.AddWithValue("@toDate", toDate.Value);
- }
+ if (toDate.HasValue)
+ {
+ // If f.created is TIMESTAMP WITHOUT TIME ZONE:
+ // command.Parameters.Add(new NpgsqlParameter("@toDate", NpgsqlDbType.Timestamp) { Value = toDate.Value.UtcDateTime });
+ // If f.created is TIMESTAMP WITH TIME ZONE:
+ // command.Parameters.Add(new NpgsqlParameter("@toDate", NpgsqlDbType.TimestampTz) { Value = toDate.Value.UtcDateTime });
+ }And consider (UTC) day bucketing (verify column type first):
-- If f.created is timestamptz:
SELECT DATE(f.created AT TIME ZONE 'UTC') as date, ...
-- Else (timestamp without time zone) today’s query is fine but ensure values are stored in UTC.Please confirm f.created’s PostgreSQL type and desired timezone for daily grouping. Based on learnings.
🏁 Script executed:
#!/bin/bash
set -e
# Locate DailySummaryData class definition
rg -nP --type=cs 'class\s+DailySummaryData\b' -C3
# Locate FileTransferCount property in that class
rg -nP --type=cs 'FileTransferCount\b' -C3Length of output: 4661
Fix COUNT(*) mapping and parameter types
- Change FileTransferCount from int to long (in DailySummaryData and ParquetDailySummaryData) and use reader.GetInt64 for the count column.
- Specify NpgsqlDbType (Timestamp vs TimestampTz) when adding @fromDate/@Todate to match file_transfer.f.created’s PostgreSQL type.
- For UTC-consistent daily buckets, use
DATE(f.created AT TIME ZONE 'UTC')if f.created is timestamptz. - Rename the loop’s local resourceId variable to avoid shadowing the method parameter.
2 participants
Description
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
New Features
Documentation
Chores