fix: Use correct type filter for LocalDevelopmentUser #1744
Conversation
📝 WalkthroughWalkthroughThe pull request introduces a modification to the authorization scope logic in the Changes
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.WebApi/Common/Authorization/AuthorizationPolicy.cs (2)
26-29: Consider adding validation for non-null and non-empty scopes.While the field filtering logic is correct, consider adding additional validation to ensure we never collect null or empty scope strings, as these could cause issues in authorization checks.
private static ReadOnlyCollection<string> GetAll() => typeof(AuthorizationScope) .GetFields() .Where(x => x.IsLiteral && !x.IsInitOnly && x.FieldType == typeof(string)) .Select(x => (string)x.GetRawConstantValue()!) + .Where(x => !string.IsNullOrEmpty(x)) .ToList() .AsReadOnly();
Line range hint
1-29: Consider documenting the authorization scope format.The authorization scopes follow a specific format (e.g.,
digdir:dialogporten.serviceprovider). Consider adding XML documentation to describe this format and any validation rules.internal static class AuthorizationScope { + /// <summary> + /// Authorization scopes following the format: organization:service[.resource][.action] + /// Examples: digdir:dialogporten.serviceprovider, altinn:system/notifications.condition.check + /// </summary> public const string EndUser = "digdir:dialogporten";
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Digdir.Domain.Dialogporten.WebApi/Common/Authorization/AuthorizationPolicy.cs(1 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.WebApi/Common/Authorization/AuthorizationPolicy.cs (1)
27-27: Verify the impact of the field type filtering change.The change from checking the declaring type to checking the field type (
x.FieldType == typeof(string)) appears to be more precise for filtering string constants. However, we should verify that this doesn't accidentally exclude any required authorization scopes.Let's verify the impact:
✅ Verification successful
The change fixes a critical bug in scope collection
The modification from
x.DeclaringType == typeof(string)tox.FieldType == typeof(string)is actually a critical bug fix. The old condition was incorrect as it checked if the declaring type wasstring(which would never be true since the declaring type isAuthorizationScope). The new condition correctly filters for string constants, which is what we want for collecting authorization scopes.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Compare the authorization scopes before and after the change # by analyzing the git history and checking field declarations # Check the field declarations in the current and previous versions git log -p -- src/Digdir.Domain.Dialogporten.WebApi/Common/Authorization/AuthorizationPolicy.cs | grep -A 1 "public const" # Verify if there are any other string constants in related authorization files rg "public const|internal const" src/**/Authorization/**/*.cs # Check for any test failures related to authorization scopes rg "AuthorizationScope" src/**/*Test*.csLength of output: 234
Script:
#!/bin/bash # Find and show the AuthorizationPolicy file content fd -e cs AuthorizationPolicy.cs | xargs cat # Find all string constants in the codebase that might be related to authorization ast-grep --pattern 'public const string $_ = $_' # Find test files related to authorization fd -e cs -g '*Test*.cs' | xargs rg -l "AuthorizationScope|AuthorizationPolicy"Length of output: 2694
🤖 I have created a release *beep* *boop* --- ## [1.47.3](v1.47.2...v1.47.3) (2025-01-28) ### Bug Fixes * **graphql:** Use correct type filter for LocalDevelopmentUser ([#1745](#1745)) ([14ff138](14ff138)) * Use correct type filter for LocalDevelopmentUser ([#1744](#1744)) ([fa30ebe](fa30ebe)) ### Miscellaneous Chores * **deps:** update dependency npgsql.entityframeworkcore.postgresql to 9.0.3 ([#1734](#1734)) ([195443f](195443f)) * **deps:** update dependency verify.xunit to 28.9.0 ([#1735](#1735)) ([73d1ddb](73d1ddb)) * **deps:** update dependency vitest to v3 ([#1732](#1732)) ([9e67931](9e67931)) * **deps:** update dependency vitest to v3.0.2 ([#1733](#1733)) ([f32a0e2](f32a0e2)) * **deps:** update opentelemetry-dotnet monorepo to 1.11.0 ([#1736](#1736)) ([75c7a24](75c7a24)) * Include chores in the changelog ([#1525](#1525)) ([d9281fc](d9281fc)) * Set 20% otel sample rate for all apps in yt01 ([#1737](#1737)) ([09c9ce9](09c9ce9)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Description
Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)