fix: Use service resource org, allow admin-scope to fetch/update dialogs #1529
Conversation
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces several modifications across various classes and interfaces within the Changes
Possibly related issues
Possibly related PRs
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
|
elsand
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
tests/k6/common/sentinel.js (1)
15-15: Add a comment explaining the limit choice.Consider adding a comment explaining why 1000 was chosen as the limit value. This helps future maintainers understand if this is an arbitrary value or if there's a specific reason for it.
- let r = getSO('dialogs/?Limit=1000&Search=' + sentinelValue + continuationToken, null, tokenOptions); + // Fetch 1000 items per request to minimize API calls during cleanup while staying within safe memory limits + let r = getSO('dialogs/?Limit=1000&Search=' + sentinelValue + continuationToken, null, tokenOptions);src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/ResourceRegistry/LocalDevelopmentResourceRegistry.cs (1)
Line range hint
7-7: Consider adding documentation about security implicationsSince this implementation is used for local development and relates to admin-scope changes, consider adding XML documentation to:
- Clearly mark this as a development-only implementation
- Document the security implications and differences from production behavior
- Explain the relationship between organization IDs and admin-scope
tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs (1)
Line range hint
1-100: Add test coverage for admin-scope scenariosThe current test file only covers forbidden scenarios. Given that the PR's main objective is to "enhance the handling of the
serviceprovider.admin-scope" and use the service resource's org value, we should add test coverage for:
- Successful dialog creation with admin-scope
- Verification that the correct organization value from the service resource is used
Would you like me to help generate the additional test cases? Here's a suggested structure:
[Fact] public async Task CreateDialogCommand_Should_Succeed_With_Admin_Scope() { // Arrange // ... setup similar to existing tests ... serviceAuthorizationSub .AuthorizeServiceResources(Arg.Any<DialogEntity>(), Arg.Any<CancellationToken>()) .Returns(new Success()); var expectedOrg = "test-org"; var serviceResource = $"resource/{expectedOrg}/service"; var createCommand = DialogGenerator.GenerateSimpleFakeDialog() with { ServiceResource = serviceResource }; resourceRegistrySub .GetResourceInformation(serviceResource, Arg.Any<CancellationToken>()) .Returns(new ServiceResourceInformation(serviceResource, "foo", "912345678", expectedOrg)); // ... rest of the test setup ... // Act var result = await commandHandler.Handle(createCommand, CancellationToken.None); // Assert Assert.True(result.IsT0); // Success // Add assertions to verify the org value was used correctly }tests/k6/tests/serviceowner/authorization.js (2)
29-31: Consider modernizing the scope check.The logging enhancement is good, but the code can be improved:
- Use optional chaining to safely access nested properties
- Use
includes()instead ofindexOf()for better readabilityHere's a suggested improvement:
- if (tokenOptions && tokenOptions["scopes"] && tokenOptions["scopes"].indexOf("admin") > -1) { + if (tokenOptions?.scopes?.includes("admin")) { logSuffix += " (admin)"; }🧰 Tools
🪛 Biome (1.9.4)
[error] 29-29: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Line range hint
109-113: Consider adding search tests with admin scope.The file includes tests for search without proper scopes, but it might be valuable to add test cases that verify search behavior with admin scope to ensure complete coverage of the new functionality.
Would you like me to help generate additional test cases for search functionality with admin scope?
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Get/GetDialogQuery.cs (1)
Line range hint
66-88: Consider performance optimization for eager loading.While the explicit ordering is necessary for consistency, consider these optimizations:
- Split the query into smaller chunks if not all related data is always needed
- Consider adding a sparse fieldset parameter to load only required includes
- Add appropriate indexes for the OrderBy clauses
This could significantly improve performance for large datasets.
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommand.cs (1)
85-88: Consider adding logging for authorization decisionsFor better security auditing and troubleshooting, consider adding debug-level logging when the service owner admin bypass is applied.
.IgnoreQueryFilters() + .WithLogDebug(_logger, x => $"Service owner admin bypass: {_userResourceRegistry.IsCurrentUserServiceOwnerAdmin()}") .WhereIf(!_userResourceRegistry.IsCurrentUserServiceOwnerAdmin(), x => resourceIds.Contains(x.ServiceResource)) .FirstOrDefaultAsync(x => x.Id == request.Id, cancellationToken);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (13)
src/Digdir.Domain.Dialogporten.Application/Externals/IResourceRegistry.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/DialogActivities/Queries/Get/GetActivityQuery.cs(2 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/DialogActivities/Queries/Search/SearchActivityQuery.cs(2 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/DialogTransmissions/Queries/Get/GetTransmissionQuery.cs(2 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/DialogTransmissions/Queries/Search/SearchTransmissionQuery.cs(2 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cs(3 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommand.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Get/GetDialogQuery.cs(2 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/ResourceRegistry/LocalDevelopmentResourceRegistry.cs(3 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/ResourceRegistry/ResourceRegistryClient.cs(1 hunks)tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs(4 hunks)tests/k6/common/sentinel.js(1 hunks)tests/k6/tests/serviceowner/authorization.js(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
tests/k6/tests/serviceowner/authorization.js
[error] 29-29: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (23)
src/Digdir.Domain.Dialogporten.Application/Externals/IResourceRegistry.cs (2)
17-17: LGTM! Property addition aligns with PR objectives.
The new immutable property OwnOrgShortName is well-designed for storing the organization short name from the service resource.
20-26: Consider adding input validation and verify case sensitivity requirements.
While the implementation is functional, consider these improvements:
- Add null/empty validation for the new parameter
- Verify if case normalization is required for
OwnOrgShortNameas it might affect integration with external systems
Here's a suggested improvement:
public ServiceResourceInformation(string resourceId, string resourceType, string ownerOrgNumber, string ownOrgShortName)
{
+ ArgumentException.ThrowIfNullOrEmpty(resourceId);
+ ArgumentException.ThrowIfNullOrEmpty(resourceType);
+ ArgumentException.ThrowIfNullOrEmpty(ownerOrgNumber);
+ ArgumentException.ThrowIfNullOrEmpty(ownOrgShortName);
+
ResourceId = resourceId.ToLowerInvariant();
ResourceType = resourceType.ToLowerInvariant();
OwnerOrgNumber = ownerOrgNumber.ToLowerInvariant();
OwnOrgShortName = ownOrgShortName.ToLowerInvariant();
}Let's verify the usage of this constructor to ensure all callers are updated:
tests/k6/common/sentinel.js (1)
15-15: Verify the impact of increased limit.
The change to fetch 1000 items at once will reduce the number of API calls needed for cleanup, but please verify that this limit:
- Doesn't cause performance issues with the API
- Doesn't exceed memory constraints in the k6 test runner
- Is aligned with any rate limiting on the API
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/DialogActivities/Queries/Search/SearchActivityQuery.cs (2)
3-3: LGTM!
Clean addition of the required extension namespace.
41-41: Verify error handling and consistent behavior across endpoints.
The conditional filtering logic looks good and aligns with the PR objective of allowing admin-scope access while maintaining proper filtering for regular users.
Let's verify the implementation:
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Error handling and access control are consistently implemented across endpoints
The codebase shows consistent patterns for resource access control:
- All query handlers properly fetch user resource IDs via
GetCurrentUserResourceIds - Null handling is managed through the
UserResourceRegistryimplementation - The
WhereIfconditional filtering is consistently used with admin checks - Resource-based authorization is uniformly applied across Dialog, Activity, and related endpoints
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent WhereIf usage and error handling across query handlers
# Check for consistent WhereIf usage pattern across query handlers
echo "Checking for consistent WhereIf usage pattern:"
ast-grep --pattern 'WhereIf(!$userRegistry.IsCurrentUserServiceOwnerAdmin(), $$$)'
# Check for potential null handling of resourceIds
echo "Checking resourceIds null handling:"
rg -A 5 "GetCurrentUserResourceIds"
# Check for similar query patterns in other handlers
echo "Checking for similar query patterns:"
ast-grep --pattern 'class $_QueryHandler : IRequestHandler<$_, $_> {
$$$
.WhereIf($_, $$$)
$$$
}'
Length of output: 15447
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/DialogActivities/Queries/Get/GetActivityQuery.cs (2)
3-3: LGTM: Required import for extension methods
The added import statement is correctly placed and necessary for the new WhereIf extension method usage.
47-47: LGTM: Conditional filtering aligns with admin-scope requirements
The implementation correctly uses WhereIf to bypass resource filtering for admin users while maintaining restrictions for regular users. This aligns with the PR objective of enhancing serviceprovider.admin-scope handling.
Let's verify the test coverage for this change:
Consider adding a code comment explaining the admin bypass behavior for future maintainers:
+ // Bypass resource filtering for service owner admins
.WhereIf(!_userResourceRegistry.IsCurrentUserServiceOwnerAdmin(), x => resourceIds.Contains(x.ServiceResource))✅ Verification successful
Let's also check if there are any existing integration tests:
And verify the UserResourceRegistry usage pattern:
Verified: Admin bypass pattern is consistently implemented across the codebase
The implementation in GetActivityQuery follows a well-established pattern used consistently throughout the codebase for handling service owner admin access:
- The same
WhereIf(!_userResourceRegistry.IsCurrentUserServiceOwnerAdmin(), x => resourceIds.Contains(x.ServiceResource))pattern is used in multiple query handlers includingGetDialogQuery,SearchActivityQuery, andGetTransmissionQuery - Integration tests exist for both ServiceOwner and EndUser contexts in
ActivityLogTests.cs - The
IsCurrentUserServiceOwnerAdmin()check is properly implemented inUserResourceRegistrybased on the service owner admin scope
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related test files and their content
fd -e cs -g "*GetActivityQuery*Test*" | while read -r file; do
echo "=== $file ==="
rg -A 10 "IsCurrentUserServiceOwnerAdmin|WhereIf" "$file"
done
Length of output: 54
Script:
#!/bin/bash
# Let's try a broader search to find test files related to GetActivity
fd -e cs -g "*Activity*Test*" | while read -r file; do
echo "=== $file ==="
rg -A 10 "IsCurrentUserServiceOwnerAdmin|WhereIf|GetActivity" "$file"
done
# Also search for the implementation to understand the test context better
rg -l "class GetActivityQuery"
Length of output: 1931
Script:
#!/bin/bash
rg -l "Integration.*GetActivity|GetActivity.*Integration"
Length of output: 59
Script:
#!/bin/bash
# Find similar patterns of IsCurrentUserServiceOwnerAdmin usage
rg "IsCurrentUserServiceOwnerAdmin\(\)" -A 5
Length of output: 8527
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/ResourceRegistry/LocalDevelopmentResourceRegistry.cs (3)
39-39: LGTM!
The changes correctly implement the new ServiceResourceInformation signature while maintaining the expected behavior for local development.
12-12: Verify if "ttd" is the correct test organization identifier
While this is a local development implementation, we should ensure that "ttd" is the correct identifier for test organization.
✅ Verification successful
"ttd" is correctly used as the test organization identifier
The verification shows consistent usage of "ttd" across the codebase:
- Used in test assertions for Altinn resource URIs (
app_ttd_some-app) - Used in test cases for organization attributes (
urn:altinn:org) - Consistently represents a test organization in both test and local development contexts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other occurrences of "ttd" to verify consistency
rg "ttd" --type cs -C 2
Length of output: 7178
Line range hint 53-58: Verify if OwnOrgShortName should be part of equality comparison
The equality comparer only checks ResourceId and OwnerOrgNumber. Verify if OwnOrgShortName should also be included in the comparison, especially if it can differ for the same OwnerOrgNumber.
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/DialogTransmissions/Queries/Search/SearchTransmissionQuery.cs (2)
3-3: LGTM!
The addition of the Extensions namespace is appropriate for the new WhereIf functionality.
51-51: Implementation looks correct, with some optimization opportunities.
The conditional filtering using WhereIf correctly implements the requirement to allow admin-scope users to access all dialogs while maintaining resource-based filtering for regular users.
Consider splitting this complex query into a separate method for better maintainability, given the extensive Include chain.
Let's verify the admin role check implementation:
✅ Verification successful
Admin role check implementation is secure and correctly used
The implementation is properly secured through OAuth2 scope validation:
IsCurrentUserServiceOwnerAdmin()checks for thedigdir:dialogporten.serviceprovider.adminscope- The scope check is consistently used across all service owner endpoints
- The admin check is properly integrated with the authorization system via
IServiceResourceAuthorizer
The conditional filtering in the query is correctly implemented and aligns with the security model:
- Regular users are filtered by their assigned resources
- Admin users get full access through the scope-based bypass
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the IsCurrentUserServiceOwnerAdmin implementation
# Expected: Find the implementation to ensure it properly checks admin scope
# Search for the admin check implementation
ast-grep --pattern 'IsCurrentUserServiceOwnerAdmin()'
# Look for related tests
rg -l "IsCurrentUserServiceOwnerAdmin.*Test"
Length of output: 102
Script:
#!/bin/bash
# Let's try a broader search for the interface and implementation
# Search for interface definition
rg -A 5 "interface.*UserResourceRegistry"
# Search for implementation class
rg -A 5 "class.*UserResourceRegistry"
# Search for any admin-related methods in the codebase
rg -A 3 "IsCurrentUser.*Admin"
# Search for any admin scope checks
rg -A 3 "ServiceOwnerAdmin"
Length of output: 14703
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/DialogTransmissions/Queries/Get/GetTransmissionQuery.cs (1)
3-3: LGTM: Import addition is appropriate
The added import for Common.Extensions is necessary for the WhereIf extension method and follows proper C# conventions.
tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs (1)
73-73: 🛠️ Refactor suggestion
Verify test relevance with new admin-scope behavior
Similar to the first test method, this test contains legacy dependencies and hardcoded organization values. Additionally, given that the PR objectives mention allowing admin-scope to fetch/update dialogs, we should verify if this non-owner test case is still relevant or needs modification.
Let's verify the admin-scope handling in the actual implementation:
Apply similar cleanup as suggested for the first test method:
- Remove unused
userResourceRegistrySub - Use meaningful organization values from the service resource
- Add test cases for admin-scope scenarios if not covered elsewhere
Also applies to: 86-88
tests/k6/tests/serviceowner/authorization.js (2)
22-23: LGTM! Test matrix covers all scenarios.
The permutations array correctly includes test cases for both regular and admin service owners, with appropriate positive and negative test cases.
8-8: LGTM! Verify scope string consistency across codebase.
The admin scope configuration looks correct and aligns with the PR objectives.
Let's verify the scope string format is consistent across the codebase:
✅ Verification successful
Scope string format is consistent across the codebase
The verification confirms that the scope string format digdir:dialogporten.serviceprovider and its extensions (.admin, .search) are consistently used across:
- Authorization policy definitions in C# code
- Test data and k6 test files
- API documentation
- Application constants
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any variations in the dialogporten.serviceprovider scope strings
# to ensure consistency across the codebase
rg -i "dialogporten\.serviceprovider[^\s\"']*"
Length of output: 5767
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cs (2)
33-33: LGTM: Dependencies properly configured
The new dependencies (IResourceRegistry, IServiceResourceAuthorizer) are correctly injected and validated with null checks, aligning with the PR's objective to use service resource information.
Also applies to: 43-43, 51-52
66-76: 🛠️ Refactor suggestion
Consider improving error handling and performance
Several suggestions for improvement:
-
The code continues execution even after adding the domain error for missing service resource information. This could lead to null reference issues if
dialog.Orgis required downstream. -
The error message could be more specific by including the service resource identifier.
-
Consider combining the service resource authorization and information retrieval to optimize performance.
Here's a suggested improvement:
- var serviceResourceInformation = await _resourceRegistry.GetResourceInformation(dialog.ServiceResource, cancellationToken);
- if (serviceResourceInformation is null)
- {
- _domainContext.AddError(new DomainFailure(nameof(DialogEntity.Org),
- "Cannot find service owner organization shortname for referenced service resource."));
- }
- else
- {
- dialog.Org = serviceResourceInformation.OwnOrgShortName;
- }
+ var serviceResourceInformation = await _resourceRegistry.GetResourceInformation(dialog.ServiceResource, cancellationToken);
+ if (serviceResourceInformation is null)
+ {
+ _domainContext.AddError(new DomainFailure(nameof(DialogEntity.Org),
+ $"Cannot find service owner organization shortname for service resource '{dialog.ServiceResource}'."));
+ return new DomainError(_domainContext.Failures);
+ }
+
+ dialog.Org = serviceResourceInformation.OwnOrgShortName;Let's verify if dialog.Org is required by checking its usage:
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Get/GetDialogQuery.cs (2)
5-5: LGTM!
The added using directive is correctly placed and necessary for the WhereIf extension method.
89-89: LGTM! Verify consistent admin-scope behavior.
The conditional filtering correctly implements the requirement to bypass resource filtering for service owner admins while maintaining security for non-admin users.
Let's verify consistent admin-scope handling across related endpoints:
✅ Verification successful
Admin-scope behavior is consistently implemented across all related endpoints
The verification confirms consistent admin-scope handling across all service owner endpoints. The same WhereIf(!_userResourceRegistry.IsCurrentUserServiceOwnerAdmin(), x => resourceIds.Contains(x.ServiceResource)) pattern is uniformly applied in:
- Dialog queries (Get)
- Dialog commands (Update)
- Dialog activities queries (Get, Search)
- Dialog transmissions queries (Get, Search)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent admin-scope handling patterns across query handlers
# Expected: Similar WhereIf conditions for admin-scope in other handlers
# Search for similar query handlers and their filtering logic
ast-grep --pattern 'class $_QueryHandler : IRequestHandler<$_, $_>
{
$$$
WhereIf($_, $_)
$$$
}'
# Search for other usages of IsCurrentUserServiceOwnerAdmin
rg "IsCurrentUserServiceOwnerAdmin" -A 5
Length of output: 8625
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/ResourceRegistry/ResourceRegistryClient.cs (1)
195-196: LGTM! Verify API contract and constructor usage.
The changes correctly utilize both organization identifiers from the service resource, which aligns with the PR objective of enhancing admin-scope handling. The null check before mapping ensures safe null handling.
Let's verify the API contract and constructor usage:
✅ Verification successful
Let me gather more information about the ServiceResourceInformation constructor to verify its parameter requirements.
Let me search specifically for the ServiceResourceInformation class definition to verify its constructor parameters.
Let me search for the ServiceResourceInformation class in other potential locations and its usages.
Let me try one final search pattern to locate the ServiceResourceInformation class definition.
Based on the search results, I can now provide a final response. The code shows that ServiceResourceInformation is a record type defined in IResourceRegistry.cs with a constructor that takes exactly four parameters: resourceId, resourceType, ownerOrgNumber, and ownOrgShortName. The changes in ResourceRegistryClient.cs correctly map these parameters from the API response, and all usages of the constructor across the codebase follow this pattern.
LGTM! The constructor usage is correct and consistent.
The changes in ResourceRegistryClient.cs properly map the Organization and OrgCode from HasCompetentAuthority to the required constructor parameters. The null check before mapping ensures safe null handling, and all constructor calls across the codebase follow the same parameter pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify ServiceResourceInformation constructor usage and potential null OrgCode
# Check if OrgCode is marked as required in all CompetentAuthority classes
rg -A 2 "class CompetentAuthority"
# Find all ServiceResourceInformation constructor calls to ensure they're updated
ast-grep --pattern 'new ServiceResourceInformation($$$)'
Length of output: 2275
Script:
#!/bin/bash
# Check ServiceResourceInformation class definition and constructor
ast-grep --pattern 'class ServiceResourceInformation'
# Check if there are any null checks or validations for OrgCode
rg -A 5 "ServiceResourceInformation\(" src/Digdir.Domain.Dialogporten.Domain/
Length of output: 100471
Script:
#!/bin/bash
# Find ServiceResourceInformation class definition
rg -A 10 "class ServiceResourceInformation"
# Find files containing ServiceResourceInformation in the domain layer
fd ServiceResourceInformation src/Digdir.Domain.Dialogporten.Domain/
Length of output: 1511
Script:
#!/bin/bash
# Search for ServiceResourceInformation class in all source files
rg -g "*.cs" "public\s+(?:class|record)\s+ServiceResourceInformation" -A 5
# Search for ServiceResourceInformation constructor usages
ast-grep --pattern 'public ServiceResourceInformation($$$)'
Length of output: 138
Script:
#!/bin/bash
# Search for ServiceResourceInformation in all files, including interfaces and records
rg -g "*.cs" "ServiceResourceInformation" -B 2 -A 10
# Find all files containing ServiceResourceInformation in their name
fd -e cs ServiceResourceInformation
Length of output: 27188
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommand.cs (2)
86-86: Verify the service owner admin authorization logic
The addition of the WhereIf condition correctly implements the PR objective by allowing service owner admins to bypass the resource ID filter. However, let's verify the authorization implementation:
- Ensure that
IsCurrentUserServiceOwnerAdmin()correctly identifies admin users - Confirm that the resource filtering is appropriate for non-admin users
✅ Verification successful
Authorization logic is consistently implemented across service owner endpoints
The verification shows that IsCurrentUserServiceOwnerAdmin() is consistently used across multiple service owner endpoints (dialogs, activities, transmissions) with the same pattern:
- All endpoints apply the same resource filtering for non-admin users
- The implementation in
IUserResourceRegistrycorrectly checks for the service owner admin scope - The authorization pattern is uniformly applied in query and command handlers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the service owner admin authorization implementation
# Check for other usages of IsCurrentUserServiceOwnerAdmin
echo "Checking for other usages of IsCurrentUserServiceOwnerAdmin:"
rg "IsCurrentUserServiceOwnerAdmin" -A 2
# Check for related authorization checks
echo "Checking for related authorization patterns:"
ast-grep --pattern 'WhereIf(!$_.$_, x => resourceIds.Contains(x.$_))'
Length of output: 5118
85-88: Verify error handling for unauthorized access
The current implementation will return EntityNotFound when a non-admin user attempts to access an unauthorized dialog. Consider whether this should return a Forbidden result instead for better security practices.
✅ Verification successful
Based on the search results, I can now generate the final response:
Current error handling approach is consistent with codebase patterns
The current implementation returning EntityNotFound when a non-admin user attempts to access an unauthorized dialog is actually the correct approach, as evidenced by the codebase patterns:
- All similar query operations (Get, Search) across different features consistently use the same pattern of
.WhereIf(!IsCurrentUserServiceOwnerAdmin(), x => resourceIds.Contains(x.ServiceResource))followed byEntityNotFoundresponse Forbiddenresponses are specifically used in command operations (Update, Delete, Purge) when explicitly checking resource type permissions, not for query filtering- This pattern effectively implements security through resource filtering rather than explicit authorization checks for queries, which is a valid security approach that doesn't leak information about unauthorized resources
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent authorization error handling patterns
# Look for similar authorization patterns
echo "Checking for similar authorization patterns:"
rg "WhereIf.*IsCurrentUserServiceOwnerAdmin.*resourceIds.Contains" -A 5
# Check for Forbidden results in similar contexts
echo "Checking for Forbidden results:"
rg "return.*Forbidden" -A 2
Length of output: 8699
...main.Dialogporten.Infrastructure/Altinn/ResourceRegistry/LocalDevelopmentResourceRegistry.cs
Show resolved
Hide resolved
...Application/Features/V1/ServiceOwner/DialogTransmissions/Queries/Get/GetTransmissionQuery.cs
Show resolved
Hide resolved
...porten.Application.Unit.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/DialogSeenLogs/Queries/Search/SearchSeenLogQuery.cs (2)
46-46: Consider improving readability of the admin check conditionThe current condition
!_userResourceRegistry.IsCurrentUserServiceOwnerAdmin()uses a double negative which makes it harder to understand at first glance.Consider refactoring to make the logic more intuitive:
- .WhereIf(!_userResourceRegistry.IsCurrentUserServiceOwnerAdmin(), x => resourceIds.Contains(x.ServiceResource)) + .WhereIf(_userResourceRegistry.IsCurrentUserServiceOwnerAdmin() == false, x => resourceIds.Contains(x.ServiceResource))Or even better, extract the condition to a well-named variable:
+ var shouldFilterByResource = !_userResourceRegistry.IsCurrentUserServiceOwnerAdmin(); - .WhereIf(!_userResourceRegistry.IsCurrentUserServiceOwnerAdmin(), x => resourceIds.Contains(x.ServiceResource)) + .WhereIf(shouldFilterByResource, x => resourceIds.Contains(x.ServiceResource))
46-46: Consider adding audit logging for admin accessGiven that admins can now bypass resource filtering, it would be valuable to log these access patterns for security auditing purposes.
Consider adding audit logging when an admin accesses records across resource boundaries:
+ var isAdmin = _userResourceRegistry.IsCurrentUserServiceOwnerAdmin(); + if (isAdmin) + { + _logger.LogInformation("Admin user accessing seen logs across resource boundaries for dialog {DialogId}", request.DialogId); + } .WhereIf(!isAdmin, x => resourceIds.Contains(x.ServiceResource))src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Purge/PurgeDialogCommand.cs (1)
45-48: Consider adding audit logging for admin actions.Since admins can now purge any dialog, it would be valuable to add audit logging to track these administrative actions for security and compliance purposes.
Example implementation:
var dialog = await _db.Dialogs .Include(x => x.Attachments) .Include(x => x.Activities) .WhereIf(!_userResourceRegistry.IsCurrentUserServiceOwnerAdmin(), x => resourceIds.Contains(x.ServiceResource)) .IgnoreQueryFilters() - .FirstOrDefaultAsync(x => x.Id == request.DialogId, cancellationToken); + .FirstOrDefaultAsync(x => x.Id == request.DialogId, cancellationToken); + +if (_userResourceRegistry.IsCurrentUserServiceOwnerAdmin()) +{ + _logger.LogInformation( + "Admin user {UserId} purging dialog {DialogId} owned by {ServiceResource}", + _userResourceRegistry.GetCurrentUserId(), + request.DialogId, + dialog?.ServiceResource); +}tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs (2)
Line range hint
52-52: Improve test assertion readabilityThe
IsT3assertions are not immediately clear about what they're verifying. Consider creating a custom assertion method or constant to make the tests more maintainable and self-documenting.Example improvement:
- Assert.True(result.IsT3); // Forbidden + const int ForbiddenResultIndex = 3; + Assert.True(result.IsT(ForbiddenResultIndex), "Expected a Forbidden result");Or better yet, create a custom assertion extension:
public static class OneOfAssertions { public static void AssertIsForbidden<T0, T1, T2>(this OneOf<T0, T1, T2, Forbidden> result) { Assert.True(result.IsT3, "Expected a Forbidden result"); } } // Usage in test: result.AssertIsForbidden();Also applies to: 89-89
Line range hint
18-90: Add positive test cases to verify organization handlingThe current tests only cover forbidden scenarios. To properly validate the PR objectives, consider adding:
- A positive test case that verifies successful dialog creation
- Verification that the correct organization value from the service resource is being used
- Test cases for different authorization scopes
Example structure for a positive test:
[Fact] public async Task CreateDialogCommand_Should_Use_Service_Resource_Organization() { // Arrange var createCommand = DialogGenerator.GenerateSimpleFakeDialog(); var expectedOrg = createCommand.ServiceResource.Split('/')[1]; resourceRegistrySub .GetResourceInformation(createCommand.ServiceResource, Arg.Any<CancellationToken>()) .Returns(new ServiceResourceInformation( createCommand.ServiceResource, "foo", "912345678", expectedOrg)); serviceAuthorizationSub .AuthorizeServiceResources(Arg.Any<DialogEntity>(), Arg.Any<CancellationToken>()) .Returns(new Success()); // Act var result = await commandHandler.Handle(createCommand, CancellationToken.None); // Assert Assert.True(result.IsT0); // Success var dialog = result.AsT0; Assert.Equal(expectedOrg, dialog.Organization); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/DialogSeenLogs/Queries/Get/GetSeenLogQuery.cs(2 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/DialogSeenLogs/Queries/Search/SearchSeenLogQuery.cs(2 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Delete/DeleteDialogCommand.cs(2 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Purge/PurgeDialogCommand.cs(2 hunks)tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs(4 hunks)
🔇 Additional comments (9)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/DialogSeenLogs/Queries/Search/SearchSeenLogQuery.cs (2)
3-3: LGTM!
The addition of the Extensions namespace import is necessary for the WhereIf extension method and follows proper C# conventions.
46-46: Verify security implications of bypassing resource filtering for admins
While the implementation aligns with the PR objectives, the combination of IgnoreQueryFilters() with conditional filtering for admins requires careful consideration:
- Ensure this doesn't unintentionally expose sensitive data
- Verify that the admin scope check is properly validated upstream
✅ Verification successful
Let me analyze the results and generate additional verification scripts to complete the security assessment.
Let me perform one final verification to understand the user resource registry implementation.
Security implementation for admin access is properly implemented
The verification shows that the security implementation for admin access is robust and safe:
- Admin access is properly controlled through a dedicated scope
digdir:dialogporten.serviceprovider.admin - The
IsCurrentUserServiceOwnerAdmin()check is consistently used across all service owner endpoints - The pattern of using
IgnoreQueryFilters()withWhereIfis implemented uniformly across all service owner queries, ensuring consistent access control - The admin scope validation is performed at the authorization level before reaching the query handlers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of IgnoreQueryFilters to understand the global query filter setup
rg "IgnoreQueryFilters\(\)" -A 5
# Look for admin scope validation in the authentication/authorization pipeline
ast-grep --pattern 'serviceprovider.admin-scope'
Length of output: 17000
Script:
#!/bin/bash
# Search for IsCurrentUserServiceOwnerAdmin implementation and related authorization checks
ast-grep --pattern 'IsCurrentUserServiceOwnerAdmin'
# Look for global query filters that might be bypassed
rg "modelBuilder.*HasQueryFilter" -A 3
# Search for any authorization policies or requirements related to service owner admin
rg "AddPolicy|RequireAuthorizationAsync|AuthorizeAsync" -A 3
Length of output: 8651
Script:
#!/bin/bash
# Search for UserResourceRegistry implementation to understand admin validation
rg -g "*.cs" "class.*UserResourceRegistry" -A 10
# Look for service owner admin scope definition
rg -g "*.cs" "ServiceOwnerAdmin|serviceprovider.admin" -B 2 -A 2
Length of output: 14694
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/DialogSeenLogs/Queries/Get/GetSeenLogQuery.cs (2)
3-3: LGTM: Import addition is appropriate
The addition of the Common.Extensions import is necessary for the WhereIf extension method usage.
48-48: LGTM: Proper implementation of admin-scope handling
The conditional filtering using WhereIf correctly implements the admin-scope requirements by:
- Only applying resource filtering for non-admin users
- Allowing admin users full access to dialog seen logs
Let's verify the consistent implementation of this pattern across other query handlers:
✅ Verification successful
Admin-scope handling pattern is consistently implemented across all dialog-related operations
The verification shows the same WhereIf(!_userResourceRegistry.IsCurrentUserServiceOwnerAdmin(), x => resourceIds.Contains(x.ServiceResource)) pattern is uniformly implemented across all relevant dialog operations:
- Dialog queries (Get, Search)
- Dialog commands (Delete, Update, Purge)
- Dialog activities (Get, Search)
- Dialog transmissions (Get, Search)
- Dialog seen logs (Get, Search)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent WhereIf usage pattern across query handlers
# Expected: Similar patterns in other dialog-related queries
# Search for similar query patterns in other handlers
ast-grep --pattern 'WhereIf(!$userRegistry.IsCurrentUserServiceOwnerAdmin(), $expr)'
# Verify no direct resource filtering without admin checks
rg -l 'Contains\(.*ServiceResource\)' --type cs | xargs rg 'IsCurrentUserServiceOwnerAdmin'
Length of output: 2595
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Delete/DeleteDialogCommand.cs (2)
3-3: LGTM!
The addition of the extensions namespace is necessary for the WhereIf extension method usage.
46-46: Verify the WhereIf implementation and security implications.
The conditional filtering based on admin status aligns with the PR objective of implementing proper handling of serviceprovider.admin-scope. However, as this is a security boundary controlling access to dialogs, we should verify:
- The WhereIf extension method's implementation to ensure it properly translates to SQL
- The IsCurrentUserServiceOwnerAdmin check's implementation to confirm proper authorization
✅ Verification successful
Let me gather more information about the WhereIf implementation and the IsCurrentUserServiceOwnerAdmin method.
Let me search for more specific information about the UserResourceRegistry implementation.
Based on the gathered information, I can now provide a final response about the security implications of the WhereIf implementation.
The WhereIf and IsCurrentUserServiceOwnerAdmin implementation is secure and properly implemented.
- The WhereIf extension method (in QueryableExtensions.cs) is a straightforward conditional Where clause that properly translates to SQL:
public static IQueryable<TSource> WhereIf<TSource>(this IQueryable<TSource> source, bool predicate, Expression<Func<TSource, bool>> queryPredicate)
=> predicate ? source.Where(queryPredicate) : source;- The IsCurrentUserServiceOwnerAdmin check (in UserResourceRegistry.cs) is properly implemented using scope verification:
public bool IsCurrentUserServiceOwnerAdmin() => _user.GetPrincipal().HasScope(Constants.ServiceOwnerAdminScope);-
The security pattern is consistently applied across all service owner endpoints, ensuring uniform access control.
-
The implementation follows the principle of defense in depth:
- Scope-based authorization at the user level
- Resource-based filtering at the query level
- Additional resource type validation in specific operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the WhereIf extension method implementation and admin check
# Check WhereIf extension implementation
echo "Checking WhereIf extension implementation:"
ast-grep --pattern 'public static IQueryable<T> WhereIf<T>($_)'
# Check admin role verification implementation
echo "Checking admin role verification implementation:"
ast-grep --pattern 'IsCurrentUserServiceOwnerAdmin($_)'
# Check for similar security patterns
echo "Checking for similar security patterns in other handlers:"
rg -A 5 "WhereIf.*IsCurrentUserServiceOwnerAdmin"
Length of output: 10577
Script:
#!/bin/bash
# Search for WhereIf extension method implementation
echo "Searching for WhereIf extension method implementation:"
rg -A 5 "public static.*WhereIf.*IQueryable"
# Search for IsCurrentUserServiceOwnerAdmin implementation
echo "Searching for IsCurrentUserServiceOwnerAdmin implementation:"
rg -A 10 "IsCurrentUserServiceOwnerAdmin"
# Search for UserResourceRegistry implementation
echo "Searching for UserResourceRegistry implementation:"
ast-grep --pattern 'class UserResourceRegistry'
Length of output: 100395
Script:
#!/bin/bash
# Search for UserResourceRegistry class implementation
echo "Searching for UserResourceRegistry class implementation:"
rg -A 20 "class UserResourceRegistry"
# Search for IUserResourceRegistry interface
echo "Searching for IUserResourceRegistry interface:"
rg -A 10 "interface IUserResourceRegistry"
Length of output: 4042
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Purge/PurgeDialogCommand.cs (2)
3-3: LGTM!
The import of the extensions namespace is required for the new WhereIf functionality.
46-46: Verify admin-scope behavior across the application.
The implementation correctly allows admin users to bypass the resource ID filter while maintaining the filter for non-admin users. This aligns with the PR objectives.
Let's verify the consistent implementation of admin-scope handling:
✅ Verification successful
Let me generate additional scripts to verify the admin-scope behavior across the application.
Based on the results, I can now generate the final response:
Admin-scope behavior is consistently implemented across the application
The admin-scope check is uniformly implemented across all service owner endpoints using the IsCurrentUserServiceOwnerAdmin() method from IUserResourceRegistry. The pattern is consistently applied in:
- All query operations (Get/Search) for dialogs, activities, transmissions, and seen logs
- All command operations (Create/Update/Delete/Purge) for dialogs
- The core authorization logic in
IServiceResourceAuthorizer
The implementation correctly bypasses resource ID filtering for admin users while maintaining proper access control for non-admin users.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent implementation of admin-scope checks across dialog handlers
# Search for similar patterns in other dialog handlers
ast-grep --pattern 'WhereIf(!$userRegistry.IsCurrentUserServiceOwnerAdmin(), $expr)'
# Look for any direct calls to ensure we're not bypassing the check elsewhere
rg -A 2 "IsCurrentUserServiceOwnerAdmin|ServiceResource"
Length of output: 100161
Script:
#!/bin/bash
# Search for all instances where IsCurrentUserServiceOwnerAdmin is used in WhereIf conditions
ast-grep --pattern 'WhereIf(!$userRegistry.IsCurrentUserServiceOwnerAdmin(), $expr)'
# Search for all usages of IsCurrentUserServiceOwnerAdmin to ensure consistent implementation
ast-grep --pattern '$userRegistry.IsCurrentUserServiceOwnerAdmin()'
# Search for any potential bypass of admin checks
rg -l "ServiceOwnerAdminScope|IsCurrentUserServiceOwnerAdmin"
Length of output: 1643
tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs (1)
39-41: Previous comment about hardcoded organization value remains valid
The hardcoded "ttd" organization doesn't reflect the PR's objective of using the actual service resource's organization value.
...porten.Application.Unit.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs
Show resolved
Hide resolved
|
🤖 I have created a release *beep* *boop* --- ## [1.40.0](v1.39.0...v1.40.0) (2024-11-26) ### Features * **infra:** Upgrade to PostgreSQL v16 ([#1521](#1521)) ([c67dc27](c67dc27)), closes [#1520](#1520) ### Bug Fixes * **app:** Sub-parties sometimes missing from authorized parties ([#1534](#1534)) ([f47112e](f47112e)) * Don't rethrow deserialization exceptions from FusionCache ([#1535](#1535)) ([790feb8](790feb8)) * Use service resource org, allow admin-scope to fetch/update dialogs ([#1529](#1529)) ([25277b5](25277b5)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Description
This implements a proper handling of serviceprovider.admin-scope, where the "org"-value for the actual service resource is used instead of always being "digdir".
This also maintains the possibility for the admin-scope-wielder to access and update the dialog afterwards. The search-endpoint is however not changed (will only display actually owned dialogs, and requiring search-scope)
Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)Summary by CodeRabbit
Release Notes
New Features
OwnOrgShortNameto enhance resource information.Bug Fixes
Documentation
Chores