feat: custom subject validation#291
Merged
Merged
Conversation
Contributor
WalkthroughThis PR adds CustomSubjectRequirement, a new requirement type enabling regex-based subject validation instead of pre-defined Norwegian identifier schemes. It modifies AuthorizationRequestValidatorService and RequirementValidationService to detect and handle custom subjects, adds comprehensive unit tests, and introduces a test plugin endpoint demonstrating the feature. ChangesCustomSubjectRequirement Feature
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
SondreJDigdir
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@Dan.Core/Services/AuthorizationRequestValidatorService.cs`:
- Around line 194-222: In ValidateAndPopulateSubject remove the redundant
self-assignment `_authRequest.Subject = _authRequest.Subject;` (it has no
effect); leave the creation of the custom Party and the assignment
`_authRequest.SubjectParty = customParty` intact so custom subjects remain
unchanged while SubjectParty is populated. Ensure only the unnecessary
assignment is deleted inside the method.
In `@Dan.Core/Services/RequirementValidationService.cs`:
- Around line 573-603: In ValidateCustomSubjectRequirement, avoid calling
Regex.Match(authRequest.Subject, req.SubjectRegex) without a timeout: modify the
logic to perform regex matching with an explicit TimeSpan timeout (e.g.,
Regex.Match overload that accepts a timeout) and wrap the match in a try-catch
for RegexMatchTimeoutException and ArgumentException (invalid pattern); on
exceptions or timeout call AddError(req, $"Subject regex error: {ex.Message}",
evidenceCodeName) and return false so invalid/slow patterns fail safely.
In `@Dan.PluginTest/Plugin.cs`:
- Around line 122-145: Remove or document the stray hardcoded test call to
ccrClientService.IsPublic("923609016", "local") in PluginCustomSubjectTest (and
the similar call in PluginGenericTest if present); specifically, delete the
unused assignment to unit if the call is not required, or replace it with a
clearly commented test hook (or a proper feature flag) that explains why the
service is being exercised and avoids hardcoded identifiers—locate the call by
searching for ccrClientService.IsPublic in the PluginCustomSubjectTest and
PluginGenericTest methods and either remove the statement or wrap it with
explanatory comments/conditional test-only logic.
🪄 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: 20a66213-6615-4205-ae70-2d2348da2542
📒 Files selected for processing (8)
Dan.Common/Models/CustomSubjectRequirement.csDan.Core.UnitTest/Services/AuthorizationRequestValidatorServiceTest.csDan.Core.UnitTest/Services/RequirementValidationServiceTest.csDan.Core/Services/AuthorizationRequestValidatorService.csDan.Core/Services/RequirementValidationService.csDan.PluginTest/Config/PluginConstants.csDan.PluginTest/Metadata.csDan.PluginTest/Plugin.cs
|
Contributor
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)
Dan.Core/Services/AuthorizationRequestValidatorService.cs (1)
57-60:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
EvidenceRequestsbefore constructingrequirements.This now runs before
ValidateEvidenceRequestWellFormed(). Line 58 dereferences_authRequest.EvidenceRequests.Any(...), so a request withEvidenceRequests == nullwill throwNullReferenceExceptioninstead of the expectedInvalidEvidenceRequestException.💡 Minimal fix
_authRequest = authorizationRequest ?? throw new InvalidAuthorizationRequestException(); _registeredEvidenceCodes = await _availableEvidenceCodesService.GetAvailableEvidenceCodes(); + ValidateEvidenceRequestWellFormed(); _evidenceCodesFromRequest = _registeredEvidenceCodes.Where(r => _authRequest.EvidenceRequests.Any(x => x.EvidenceCodeName == r.EvidenceCodeName)).ToList(); var requirements = _evidenceCodesFromRequest.ToDictionary(es => es.EvidenceCodeName, es => es.AuthorizationRequirements);🤖 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 `@Dan.Core/Services/AuthorizationRequestValidatorService.cs` around lines 57 - 60, The code in AuthorizationRequestValidatorService dereferences _authRequest.EvidenceRequests when building requirements (creating _evidenceCodesFromRequest and requirements) before ValidateEvidenceRequestWellFormed runs, which causes a NullReferenceException for null EvidenceRequests; fix by guarding EvidenceRequests (either call ValidateEvidenceRequestWellFormed first or replace uses of _authRequest.EvidenceRequests with a null-safe check/empty sequence) so _evidenceCodesFromRequest and the ToDictionary for requirements only run when EvidenceRequests is non-null (refer to _registeredEvidenceCodes, _evidenceCodesFromRequest, requirements, and ValidateEvidenceRequestWellFormed to locate the correct spot).
🧹 Nitpick comments (1)
Dan.Core/Services/AuthorizationRequestValidatorService.cs (1)
69-71: CustomSubject detection relies on RequirementType, but validator’s normal evidence-code path populates it.
CustomSubjectRequirementdoesn’t setRequirementType, andDan.PluginTest/Metadata.csalso leaves it unset; however,AuthorizationRequestValidatorServicegets its requirements viaAvailableEvidenceCodesService.GetAvailableEvidenceCodes(), which setsRequirementType = x.GetType().Namefor every requirement before caching, so theRequirementType == "CustomSubjectRequirement"check should work for runtime requests.Still,
RequirementTypeis nullable and isn’t enforced by the model—if evidence codes/requirements are ever constructed outside that normalization step (or come from stale cache), the bypass can fail; switching the check to runtime type (req is CustomSubjectRequirement) would make this robust.🤖 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 `@Dan.Core/Services/AuthorizationRequestValidatorService.cs` around lines 69 - 71, The current detection of a CustomSubject uses RequirementType string matching in AuthorizationRequestValidatorService (variable customSubject built from requirements.Values.SelectMany(...)), which can be null or stale because RequirementType is set by AvailableEvidenceCodesService.GetAvailableEvidenceCodes(); change the check to a runtime type check (e.g., test req is CustomSubjectRequirement) so detection is robust even when RequirementType is missing, and ensure any nullability assumptions about RequirementType are removed from this path.
🤖 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 `@Dan.Core/Services/AuthorizationRequestValidatorService.cs`:
- Around line 57-60: The code in AuthorizationRequestValidatorService
dereferences _authRequest.EvidenceRequests when building requirements (creating
_evidenceCodesFromRequest and requirements) before
ValidateEvidenceRequestWellFormed runs, which causes a NullReferenceException
for null EvidenceRequests; fix by guarding EvidenceRequests (either call
ValidateEvidenceRequestWellFormed first or replace uses of
_authRequest.EvidenceRequests with a null-safe check/empty sequence) so
_evidenceCodesFromRequest and the ToDictionary for requirements only run when
EvidenceRequests is non-null (refer to _registeredEvidenceCodes,
_evidenceCodesFromRequest, requirements, and ValidateEvidenceRequestWellFormed
to locate the correct spot).
---
Nitpick comments:
In `@Dan.Core/Services/AuthorizationRequestValidatorService.cs`:
- Around line 69-71: The current detection of a CustomSubject uses
RequirementType string matching in AuthorizationRequestValidatorService
(variable customSubject built from requirements.Values.SelectMany(...)), which
can be null or stale because RequirementType is set by
AvailableEvidenceCodesService.GetAvailableEvidenceCodes(); change the check to a
runtime type check (e.g., test req is CustomSubjectRequirement) so detection is
robust even when RequirementType is missing, and ensure any nullability
assumptions about RequirementType are removed from this path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e176e846-091b-4e20-be55-c5de75c3dd7c
📒 Files selected for processing (2)
Dan.Core/Services/AuthorizationRequestValidatorService.csDan.Core/Services/RequirementValidationService.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- Dan.Core/Services/RequirementValidationService.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
In order to implement data-altinn-no/plugin-tilda#76 we need to have proper support to say that a dataset supports subjects other than the typical Norwegian SSN or Organisation numbers. This feature will add a custom requirement for datasets that allows to specify a regex for what custom subject format should follow. Also adds a flag to say if it should skip the regular SSN/Orgnumber check first. If set to false, it will first check if subject is SSN or Org and set party values as normally before, but if not found or the flag is set to true, then core will only check the subject using the provided custom regex.
Documentation
Summary by CodeRabbit
New Features
Tests