fix(webapi): Mask unauthorized attachment URLs in EndUser transmission endpoints #1890
Conversation
📝 WalkthroughWalkthroughThe changes update how attachment URLs are processed in transmission queries. In the GetTransmissionQuery, the URL source shifts from the transmission’s attachments to the mapped DTO’s attachments when the authorization flag is false. In the SearchTransmissionQuery, the returned list of DTOs is iterated to set an authorization flag, and for transmissions that are not authorized, attachment URLs are replaced with a constant unauthorized URI. There are no changes to public entity declarations. Changes
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogTransmissions/Queries/Search/SearchTransmissionQuery.cs (1)
72-88: Verify performance impact of post-mapping modifications.The code maps the entire list of transmissions and then iterates through it to modify unauthorized URLs. This could be optimized by:
- Projecting only necessary fields during the database query
- Handling authorization during mapping
Consider optimizing by creating a custom mapping profile that handles authorization during the mapping process:
- var dto = _mapper.Map<List<TransmissionDto>>(dialog.Transmissions); - - foreach (var transmission in dto) - { - transmission.IsAuthorized = authorizationResult.HasReadAccessToDialogTransmission(transmission.AuthorizationAttribute); - - if (transmission.IsAuthorized) continue; - - var urls = transmission.Attachments.SelectMany(a => a.Urls).ToList(); - foreach (var url in urls) - { - url.Url = Constants.UnauthorizedUri; - } - } - - return dto; + var dto = dialog.Transmissions.Select(transmission => { + var mappedDto = _mapper.Map<TransmissionDto>(transmission); + mappedDto.IsAuthorized = authorizationResult.HasReadAccessToDialogTransmission(transmission.AuthorizationAttribute); + + if (!mappedDto.IsAuthorized) + { + foreach (var attachment in mappedDto.Attachments) + { + foreach (var url in attachment.Urls) + { + url.Url = Constants.UnauthorizedUri; + } + } + } + + return mappedDto; + }).ToList(); + + return dto;src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogTransmissions/Queries/Get/GetTransmissionQuery.cs (1)
86-91: Ensure consistent URL masking across endpoints.The URL masking implementation is similar to SearchTransmissionQuery but with subtle differences in the code structure. While functionally correct, consider extracting this common logic into a shared helper method to ensure consistent behavior and easier maintenance.
Consider creating a shared helper method:
public static class TransmissionUrlHelper { public static void MaskUnauthorizedUrls(TransmissionDto transmission) { if (transmission.IsAuthorized) return; foreach (var url in transmission.Attachments.SelectMany(a => a.Urls)) { url.Url = Constants.UnauthorizedUri; } } }Then use it in both query handlers:
- var urls = dto.Attachments.SelectMany(a => a.Urls).ToList(); - foreach (var url in urls) - { - url.Url = Constants.UnauthorizedUri; - } + TransmissionUrlHelper.MaskUnauthorizedUrls(dto);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogTransmissions/Queries/Get/GetTransmissionQuery.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogTransmissions/Queries/Search/SearchTransmissionQuery.cs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Dry run deploy apps / Deploy job sync-resource-policy-information-job to test
- GitHub Check: Dry run deploy apps / Deploy job sync-subject-resource-mappings-job to test
- GitHub Check: build / build-and-test
🔇 Additional comments (1)
src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogTransmissions/Queries/Get/GetTransmissionQuery.cs (1)
75-79:❓ Verification inconclusive
Verify transmission lookup logic.
The current implementation uses
FirstOrDefault()without any ordering, which might not consistently return the expected transmission when multiple transmissions exist for the dialog.Consider adding explicit ordering or using
SingleOrDefault()with the transmission ID filter:- var transmission = dialog.Transmissions.FirstOrDefault(); + var transmission = dialog.Transmissions.SingleOrDefault();Also, verify that the
Whereclause in the Include statements correctly filters the transmissions:
🏁 Script executed:
#!/bin/bash # Description: Check if there are any cases where multiple transmissions might exist for a dialog # Search for transmission-related queries rg -A 5 "Transmissions\.Where" # Search for transmission ID usage rg -A 5 "TransmissionId"Length of output: 355
Action: Validate Transmission Lookup and Filtering Approach
- Confirm that using
FirstOrDefault()ondialog.Transmissionsreliably returns the intended transmission when more than one is present. If the intent is to ensure a single matching transmission, consider filtering byTransmissionIdand usingSingleOrDefault()instead.- If multiple transmissions are a possibility, introduce explicit ordering (for example, by creation date or another business-relevant field) before selecting the first element.
- Additionally, verify that any
Includestatements employingWhereclauses correctly filter the transmissions as intended.
oskogstad
deleted the
fix/mask-unauthorized-urls-in-enduser-transmission-endpoints
branch
🤖 I have created a release *beep* *boop* --- ## [1.51.0](v1.50.9...v1.51.0) (2025-02-17) ### Features * Add event for transmission created ([#1893](#1893)) ([ae0f7dc](ae0f7dc)) ### Bug Fixes * **webapi:** Mask unauthorized attachment URLs in EndUser transmission endpoints ([#1890](#1890)) ([f2f817b](f2f817b)) ### Miscellaneous Chores * **ci:** Always push new container version tags on release ([#1887](#1887)) ([7d31b3b](7d31b3b)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Description
EndUser transmission endpoints gives out original URLs when the user is not authorized to see them
Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)