bugfix: ensure to mask SSN numbers - #1822
Conversation
📝 WalkthroughWalkthroughAdds SSN masking infrastructure to preserve national identity number privacy in Party API responses. Introduces ChangesParty SSN masking across API responses
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks 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 |
![github-advanced-security[bot]](https://avatars.github.com/in/57789?s=60&v=4){kind=small}
|
We have a struct type for national identity numbers here, if that's relevant: https://github.com/Altinn/app-lib-dotnet/blob/main/src/Altinn.App.Core/Models/NationalIdentityNumber.cs Associated extensions: https://github.com/Altinn/app-lib-dotnet/blob/main/src/Altinn.App.Core/Extensions/NationalIdentityNumberExtensions.cs Food for thought 🥫 🧠 |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Altinn.App.Api/Helpers/PartySsnMasking.cs (1)
26-34: ⚡ Quick winPre-size
maskedPartiesto avoid avoidable reallocations.
MaskPartiescan cheaply initialize capacity when the source is a collection.As per coding guidelines, `**/*.cs`: "Write efficient code - don't allocate unnecessarily (e.g., avoid calling ToString twice, prefer for loops over LINQ when appropriate)".♻️ Proposed change
public static List<Party> MaskParties(IEnumerable<Party> parties) { - List<Party> maskedParties = new List<Party>(); + List<Party> maskedParties = parties is ICollection<Party> collection + ? new List<Party>(collection.Count) + : new List<Party>(); foreach (Party party in parties) { maskedParties.Add(MaskParty(party)); } return maskedParties; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Altinn.App.Api/Helpers/PartySsnMasking.cs` around lines 26 - 34, The MaskParties method creates a List without pre-sizing its capacity, causing unnecessary reallocations as items are added during iteration. To fix this, check if the input parameter `parties` can be cast to ICollection<Party> (or use a similar approach to determine the count efficiently), and then initialize the `maskedParties` list with that capacity by passing the count to the List constructor. This ensures the list has sufficient capacity from the start and eliminates avoidable memory reallocations as MaskParty results are added in the foreach loop.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Altinn.App.Api/Helpers/PartySsnMasking.cs`:
- Around line 50-57: Replace the blanket property cloning approach using
CopyProperties with explicit allowlisted field mapping in the Party cloning
logic (around line 50-57) and in any related Person/ChildParty masking methods
(around lines 117-136). Instead of copying all writable properties and then
selectively masking sensitive ones, explicitly map only the non-sensitive fields
that are safe to expose in the API response, ensuring that any future sensitive
fields added to the Party, Person, or related models will require explicit code
changes to include them, providing compile-time safety against PII leaks.
---
Nitpick comments:
In `@src/Altinn.App.Api/Helpers/PartySsnMasking.cs`:
- Around line 26-34: The MaskParties method creates a List without pre-sizing
its capacity, causing unnecessary reallocations as items are added during
iteration. To fix this, check if the input parameter `parties` can be cast to
ICollection<Party> (or use a similar approach to determine the count
efficiently), and then initialize the `maskedParties` list with that capacity by
passing the count to the List constructor. This ensures the list has sufficient
capacity from the start and eliminates avoidable memory reallocations as
MaskParty results are added in the foreach loop.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 44bd1d04-0c3d-4a21-a06b-34625025e264
📒 Files selected for processing (4)
src/Altinn.App.Api/Controllers/AuthorizationController.cssrc/Altinn.App.Api/Controllers/PartiesController.cssrc/Altinn.App.Api/Helpers/PartySsnMasking.cstest/Altinn.App.Api.Tests/Helpers/PartySsnMaskingTests.cs
|
Test fra localtest: |
…o use Where' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
framitdavid
added
the
feature
framitdavid
changed the title
framitdavid
added
bugfix
|
/publish |
PR release:
|
|
/publish |
PR release:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Altinn.App.Api/Controllers/ProfileController.cs (1)
30-42: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftUse an explicit
...ResponseDTO forGET /profile/user.This action still returns
UserProfiledirectly from a controller endpoint. Please map the masked profile into an API-owned response DTO to keep the HTTP contract decoupled and compliant.Suggested direction
-[ProducesResponseType(typeof(UserProfile), StatusCodes.Status200OK)] +[ProducesResponseType(typeof(GetUserResponse), StatusCodes.Status200OK)] ... - return Ok(PartySsnMasking.MaskUserProfile(details.Profile)); + var maskedProfile = PartySsnMasking.MaskUserProfile(details.Profile); + return Ok(GetUserResponse.From(maskedProfile));As per coding guidelines,
**/src/Altinn.App.Api/**/*.cs: For HTTP APIs, provide...Requestand...ResponseDTOs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Altinn.App.Api/Controllers/ProfileController.cs` around lines 30 - 42, The GetUser action in ProfileController returns UserProfile directly instead of using an explicit response DTO. Create an API-owned response DTO (such as UserProfileResponse) to represent the HTTP contract, then modify the GetUser method to map the masked profile from PartySsnMasking.MaskUserProfile(details.Profile) into this new DTO before returning it with Ok(). This ensures the HTTP API contract remains decoupled from internal domain models as per the coding guidelines for src/Altinn.App.Api.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/Altinn.App.Api/Controllers/ProfileController.cs`:
- Around line 30-42: The GetUser action in ProfileController returns UserProfile
directly instead of using an explicit response DTO. Create an API-owned response
DTO (such as UserProfileResponse) to represent the HTTP contract, then modify
the GetUser method to map the masked profile from
PartySsnMasking.MaskUserProfile(details.Profile) into this new DTO before
returning it with Ok(). This ensures the HTTP API contract remains decoupled
from internal domain models as per the coding guidelines for src/Altinn.App.Api.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b7ae78f7-d200-4284-ae06-c26de74b3a8e
📒 Files selected for processing (6)
src/Altinn.App.Api/Controllers/ProfileController.cssrc/Altinn.App.Api/Helpers/PartySsnMasking.cssrc/Altinn.App.Core/Extensions/NationalIdentityNumberExtensions.cstest/Altinn.App.Api.Tests/Controllers/ProfileControllerTests.User.verified.txttest/Altinn.App.Api.Tests/Helpers/PartySsnMaskingTests.cstest/Altinn.App.Core.Tests/Extensions/NationalIdentityNumberExtensionsTest.cs
✅ Files skipped from review due to trivial changes (1)
- test/Altinn.App.Api.Tests/Controllers/ProfileControllerTests.User.verified.txt
framitdavid
added
the
backport
github-project-automation
Bot
moved this from 🔎 In review
to ✅ Done
in Team Altinn Studio
|
✅ Automatic backport successful! A backport PR has been automatically created for the The release branch The cherry-pick was clean with no conflicts. Please review the backport PR when it appears. |
|
3 participants
Description
What
Masks Norwegian national identity numbers (SSNs) in the party data returned to the frontend, so the full number is never exposed in HTTP responses (e.g.
01039012345→010390*****).Why
Several endpoints returned full
Partyobjects — includingParty.SSNandParty.Person.SSN— in the clear. This reduces unnecessary exposure of personal identifiers to the browser while keeping the birth-date portion (DDMMYY) visible for display purposes.Endpoints covered
/{org}/{app}/api/v1/partiesPartyin the list (incl.Person+ChildParties)/{org}/{app}/api/v1/parties/validateInstantiationValidPartiesin the result/{org}/{app}/api/authorization/parties/current?returnPartyObject=truePartyHow
NationalIdentityNumberExtensions.Mask(string?)(+ToMaskedString()for an already-validatedNationalIdentityNumber). One source of truth, reusable elsewhere (loggingetc.).
PartySsnMasking(API layer) returns masked copies of the party graph (Party→Person→ChildParties). It copies every field generically via reflection and only transforms the SSN, so new fields on the platform model are carried over automatically.Testing
GET api/authorization/parties/currentasserting a maskedssnin the response.Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
New Features
Tests