fix(webAPI): Specifying EndUserId on the ServiceOwner Search endpoint produces 500 - Internal Server error #1234
Conversation
|
Warning Rate limit exceeded@knuhau has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 34 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve modifications to several classes and interfaces related to authorization within the Changes
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range comments (1)
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs (1)
Line range hint
193-206: Improved claim handling logic.The changes to the
GetOrCreateClaimsBasedOnEndUserIdmethod enhance the flexibility and security of claim handling. The use of a list instead of an array allows for more dynamic claim management. The separation of logic based on the validity ofendUserIdprovides better control over which claims are included.Consider adding a comment explaining the rationale behind adding only specific claims when a valid
endUserIdis provided, as this change in behavior might not be immediately obvious to other developers.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs (2 hunks)
🧰 Additional context used
🔇 Additional comments (1)
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs (1)
Line range hint
1-241: Address lack of tests for the changes.While the changes to the
AltinnAuthorizationClientclass appear to address the 500 Internal Server error issue, it's concerning that no new tests have been added to verify this fix. Given the critical nature of authorization handling, it's important to ensure that these changes are properly tested.Please consider adding the following tests:
- A unit test for the new
AddClaimIfExistsmethod to verify its behavior with various input scenarios.- Integration tests for the
GetOrCreateClaimsBasedOnEndUserIdmethod to ensure it correctly handles differentendUserIdvalues and produces the expected claim sets.- An end-to-end test that simulates the ServiceOwner Search endpoint with a specified EndUserId to confirm that the 500 Internal Server error has been resolved.
Additionally, update the PR description to include information about these new tests once they are added.
To help identify existing tests that might need updating, you can run the following command:
This will help locate any existing test files that might need to be updated to account for the new changes.
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs (2 hunks)
🧰 Additional context used
🔇 Additional comments (1)
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs (1)
Line range hint
1-231: Request for clarification and additional tests.The changes appear to address the issue by simplifying claims handling. However, there are two concerns:
The PR objective mentions fixing an error when specifying EndUserId, but the changes don't explicitly handle EndUserId. Could you clarify how these changes resolve the specific issue with EndUserId?
No new tests were added for this bug fix. Consider adding unit tests to verify the fix and prevent regression.
To help verify the changes, you could add unit tests that:
- Mock the user principal with various claim configurations.
- Call the modified methods with different inputs, including cases where EndUserId is specified.
- Assert that the expected authorization results are returned.
Would you like assistance in drafting these unit tests?
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (5)
- src/Digdir.Domain.Dialogporten.Application/Externals/AltinnAuthorization/IAltinnAuthorization.cs (0 hunks)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Get/GetDialogQuery.cs (0 hunks)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Search/SearchDialogQuery.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs (2 hunks)
- src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/LocalDevelopmentAltinnAuthorization.cs (1 hunks)
💤 Files with no reviewable changes (2)
- src/Digdir.Domain.Dialogporten.Application/Externals/AltinnAuthorization/IAltinnAuthorization.cs
- src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Get/GetDialogQuery.cs
🧰 Additional context used
📓 Learnings (1)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Search/SearchDialogQuery.cs (1)
Learnt from: elsand PR: digdir/dialogporten#1192 File: src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogLabelAssignmentLog/Queries/Search/SearchDialogLabelAssignmentLogQuery.cs:29-31 Timestamp: 2024-10-03T06:14:49.015Z Learning: In this codebase, null checks are used in constructors by convention to ensure proper wiring in non-DI contexts, such as tests.
🔇 Additional comments (7)
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/LocalDevelopmentAltinnAuthorization.cs (6)
24-28: LGTM: Simplified method signatureThe removal of the unused parameter
string? _from theGetDialogDetailsAuthorizationmethod signature is a good cleanup. This change simplifies the method without affecting its functionality.
29-31: LGTM: Simplified method signature and improved usabilityThe removal of the
endUserIdparameter and the addition of a default value forcancellationTokenare good improvements. These changes simplify the method signature and enhance its usability without affecting the core functionality.
Line range hint
38-39: Consider reviewing theTake()limitsThe method uses
Take(1000)for parties and resources, andTake(30)for roles. While these limits prevent excessive data retrieval, they might need review to ensure they meet the application's requirements and scalability needs.Could you provide more context on why these specific limits were chosen? Are there any performance considerations or business requirements that dictate these values?
Also applies to: 41-41
Line range hint
54-55: LGTM: Improved parameter namingRenaming the
__parameter tocancellationTokenenhances code readability. This change makes the method signature more self-explanatory without altering its functionality.
Line range hint
53-55: Consider reviewing theSuppressMessageattributeThe method is marked with
[SuppressMessage("Performance", "CA1822:Mark members as static")]. Given that the method doesn't use any instance members, consider making it static or review why this suppression is necessary.Could you explain why this method isn't marked as static? Is there a specific reason for suppressing this warning instead of addressing it?
Line range hint
1-57: Overall, changes align well with PR objectivesThe modifications to
LocalDevelopmentAltinnAuthorizationclass successfully simplify the code by removing unused parameters and improving method signatures. These changes are consistent with the PR objectives of resolving the 500 Internal Server Error issue and simplifying the code.Key improvements:
- Removal of unused
endUserIdparameter fromGetAuthorizedResourcesForSearch.- Simplified signatures for all methods.
- Improved parameter naming for better readability.
While the changes are generally good, consider addressing the following:
- Review the
Take()limits inGetAuthorizedResourcesForSearch.- Evaluate the need for the
SuppressMessageattribute inGetAuthorizedParties.These considerations could further enhance the code quality and maintainability.
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Search/SearchDialogQuery.cs (1)
Line range hint
1-241: Overall code structure and alignment with PR objectives.The file is well-structured and implements a comprehensive search functionality for dialogs. It aligns with the PR objectives by providing the necessary query parameters and handling logic for the ServiceOwner Search endpoint. The code follows good practices, including proper separation of concerns and use of LINQ for query composition.
While the changes in this file are minimal, the overall implementation supports the goal of resolving the issue with the ServiceOwner Search endpoint. The
SearchDialogQueryHandlerclass, in particular, seems to handle the search logic effectively, including the case where an end user is impersonated.
...ialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Search/SearchDialogQuery.cs
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs
Show resolved
Hide resolved
|
🤖 I have created a release *beep* *boop* --- ## [1.22.0](v1.21.0...v1.22.0) (2024-10-07) ### Features * Add support for supplied transmission attachment ID on create/update ([#1242](#1242)) ([c7bfb07](c7bfb07)) ### Bug Fixes * Only allow legacy HTML on AditionalInfo content ([#1210](#1210)) ([aa4acde](aa4acde)) * **webAPI:** Specifying EndUserId on the ServiceOwner Search endpoint produces 500 - Internal Server error ([#1234](#1234)) ([49c0d34](49c0d34)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
GetOrCreateClaimsBasedOnEndUserId() caused this. Dropping the method entirely fixes the problem and simplifies the code.
Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes