feat(GraphQL): Add DialogToken requirement for subscriptions #1124
Conversation
knuhau
force-pushed
the
chore/add-dialog-token-requirement-on-subscriptions
branch
from
dd19d27 to
cef705a
Compare
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several changes aimed at enhancing authorization mechanisms within the GraphQL schema and related components. Key modifications include adding authorization directives to the Changes
Possibly related issues
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🔇 Additional comments (2)
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
|
oskogstad
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: 11
Outside diff range comments (4)
src/Digdir.Domain.Dialogporten.GraphQL/ServiceCollectionExtensions.cs (1)
Line range hint
9-29: Suggestion: Review entire GraphQL configurationGiven that the PR objectives mention adding a DialogToken requirement for subscriptions, it might be beneficial to review the entire GraphQL configuration in this file. Consider if any other parts of this setup need to be updated to fully implement and enforce the DialogToken requirement for subscriptions. This could include adding specific authorization policies, middleware, or other configuration options related to the DialogToken.
src/Digdir.Domain.Dialogporten.GraphQL/Common/Authorization/AuthorizationPolicy.cs (1)
Line range hint
22-29: Consider optimizing theGetAllmethod inAuthorizationScope.While not directly related to the current changes, the
GetAllmethod uses reflection to collect all string constants. This approach might be less efficient and could potentially miss new constants likeEndUserSubscriptionif they're needed in theAllScopescollection.Consider replacing the reflection-based approach with an explicit list of constants:
private static ReadOnlyCollection<string> GetAll() => new ReadOnlyCollection<string>(new[] { EndUser, ServiceProvider, ServiceProviderSearch, Testing });This approach would be more performant and ensure that all relevant constants are included.
src/Digdir.Domain.Dialogporten.WebApi/appsettings.Development.json (1)
Line range hint
1-80: General observation: Sensitive information managementI noticed several "TODO" placeholders for sensitive information throughout the file. While this is common in development settings, it's crucial to ensure:
- These placeholders are replaced with actual values securely before deployment.
- The team follows best practices for managing secrets, such as using environment-specific secret stores or local user secrets.
- No sensitive information is accidentally committed to version control.
Consider adding a comment at the top of the file reminding developers about proper secret management practices.
src/Digdir.Domain.Dialogporten.Application/Common/ICompactJwsGenerator.cs (1)
Line range hint
104-143: Consider using a standard library for Base64Url encoding/decodingThe custom
Base64Urlclass implementation is well-written, but it might be worth checking if there's a standard library implementation available for Base64Url encoding and decoding in C#. Using a standard library, if available, could potentially improve maintainability and reduce the risk of bugs.Research if there's a built-in or widely used library for Base64Url encoding/decoding in C#. If found, consider replacing the custom implementation with the standard one.
Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Files selected for processing (11)
- docs/schema/V1/schema.verified.graphql (2 hunks)
- src/Digdir.Domain.Dialogporten.Application/Common/ICompactJwsGenerator.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.GraphQL/Common/Authorization/AuthorizationOptionsSetup.cs (3 hunks)
- src/Digdir.Domain.Dialogporten.GraphQL/Common/Authorization/AuthorizationPolicy.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.GraphQL/Common/Authorization/DialogTokenMiddleware.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.GraphQL/Common/Extensions/HotChocolate/DefinitionNodeExtensions.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.GraphQL/EndUser/DialogById/Subscriptions.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.GraphQL/Program.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.GraphQL/ServiceCollectionExtensions.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.GraphQL/appsettings.Development.json (1 hunks)
- src/Digdir.Domain.Dialogporten.WebApi/appsettings.Development.json (1 hunks)
Additional comments not posted (20)
src/Digdir.Domain.Dialogporten.GraphQL/ServiceCollectionExtensions.cs (2)
16-16: Approve: Improved order of GraphQL service configurationThe reordering of
.AddSubscriptionType<Subscriptions>()to come after.AddAuthorization()is a positive change. This ensures that authorization is set up before adding the subscription type, which could potentially resolve any initialization issues if theSubscriptionstype relies on authorization services. It also aligns with the best practice of configuring core services before adding specific features that might depend on them.
16-16: Clarify: How does this change relate to the DialogToken requirement?The PR objectives mention adding a DialogToken requirement for subscriptions, but this file doesn't show any explicit changes related to that. Could you please clarify how this reordering of the subscription type addition relates to the DialogToken requirement? Are there other files in this PR that implement this requirement more directly?
To help clarify this, let's check for any DialogToken-related changes:
src/Digdir.Domain.Dialogporten.GraphQL/Common/Authorization/AuthorizationPolicy.cs (1)
8-8: New authorization policy added for end-user subscriptions.The addition of
EndUserSubscriptionconstant is consistent with the existing naming convention and likely relates to the new DialogToken requirement for subscriptions mentioned in the PR objectives.Let's verify if this new constant is used elsewhere in the codebase:
Verification successful
EndUserSubscription constant usage verified as consistent across the codebase.
The new constant
EndUserSubscriptionis appropriately utilized in authorization setups and within theSubscriptions.csfile, aligning with the project's authorization strategy.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usage of the new EndUserSubscription constant # Test: Search for "EndUserSubscription" or "enduserSubscription" in all files rg --type-add 'csharp:*.{cs,csproj}' --type csharp -i '(EndUserSubscription|enduserSubscription)'Length of output: 580
src/Digdir.Domain.Dialogporten.GraphQL/Common/Extensions/HotChocolate/DefinitionNodeExtensions.cs (2)
5-8: LGTM: Class and method signature are well-designed.The
DefinitionNodeExtensionsclass andTryGetSubscriptionDialogIdmethod are correctly implemented as public and static. The method signature follows the TryParse pattern, which is appropriate for this use case.
9-22: LGTM: Initial setup and iteration logic are well-implemented.The method correctly initializes
dialogIdtoGuid.Emptyand efficiently iterates through the definitions. The type checking and early continue statements for non-matching definitions improve the code's readability and performance.src/Digdir.Domain.Dialogporten.GraphQL/appsettings.Development.json (7)
56-56: LGTM: Disabling local development user aligns with PR objectiveChanging
UseLocalDevelopmentUsertofalseis consistent with the PR's goal of adding a DialogToken requirement for subscriptions. This change moves the development environment closer to production-like conditions by disabling mock users.
57-57: LGTM: Disabling local resource register enhances environment fidelitySetting
UseLocalDevelopmentResourceRegistertofalseis a positive change. It moves the development environment closer to production conditions by using actual resource registers instead of local mocks. This change will help catch potential issues earlier in the development process.
58-58: LGTM: Consistent approach in disabling local development featuresChanging
UseLocalDevelopmentOrganizationRegistertofalseis consistent with the previous changes. It further aligns the development environment with production-like conditions by using actual organization registers instead of local mocks.
59-59: LGTM: Consistent disabling of local development featuresSetting
UseLocalDevelopmentNameRegistertofalsefollows the pattern of disabling local development mocks. This change, along with the previous ones, creates a more realistic development environment that closely mimics production conditions.
60-60: LGTM: Disabling local Altinn authorization supports PR objectiveChanging
UseLocalDevelopmentAltinnAuthorizationtofalsedirectly supports the PR's goal of enhancing authorization mechanisms. This change ensures that the development environment uses actual Altinn authorization, which is crucial for testing the new DialogToken requirement for subscriptions.
62-62: LGTM: Critical changes for implementing DialogToken requirementTwo important changes have been made:
- Setting
UseLocalDevelopmentCompactJwsGeneratortofalsecontinues the trend of disabling local development mocks.- Changing
DisableAuthtofalseenables authentication, which is crucial for implementing and testing the new DialogToken requirement for subscriptions.These changes are essential for achieving the PR's objective and ensure that the development environment closely mirrors production conditions for authentication and authorization.
Also applies to: 65-65
56-65: Overall changes align well with PR objective and improve development environmentThe modifications to the
LocalDevelopmentsettings collectively achieve the following:
- Disable various local development mocks (user, resource register, organization register, name register, Altinn authorization, and compact JWS generator).
- Enable authentication by setting
DisableAuthtofalse.These changes significantly align the development environment with production-like conditions, which is crucial for implementing and testing the new DialogToken requirement for subscriptions. This approach will likely help catch potential issues earlier in the development process and ensure a smoother transition to production.
However, it's important to note that these changes might increase the complexity of local development setup. To mitigate any potential issues:
- Ensure that the necessary production-like services are accessible in the development environment.
- Update the development documentation to reflect these changes and provide guidance on setting up the required services.
- Consider creating a script or tool to help developers easily switch between local mocks and production-like services when needed.
These steps will help ensure a smooth transition for developers working with the updated configuration.
Verification successful
Verification Successful: All services are accessible and documentation is up-to-date
The verification process confirmed that:
Service Accessibility:
- Maskinporten: Accessible (HTTP 200)
- Altinn: Accessible (HTTP 200)
- ID-porten: Accessible (HTTP 200)
Development Documentation:
- The documentation includes necessary references to
LocalDevelopment, ensuring that the setup is well-documented and aligned with the recent configuration changes.These results confirm that the changes to the
appsettings.Development.jsonhave been properly implemented and that the development environment is correctly configured to reflect production-like conditions. No further action is required at this time.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the necessary services are accessible in the development environment echo "Verifying access to required services..." # Check Maskinporten curl -s -o /dev/null -w "%{http_code}" https://test.maskinporten.no/.well-known/oauth-authorization-server/ | grep 200 || echo "Warning: Maskinporten service is not accessible" # Check Altinn curl -s -o /dev/null -w "%{http_code}" https://platform.tt02.altinn.no/authentication/api/v1/openid/.well-known/openid-configuration | grep 200 || echo "Warning: Altinn service is not accessible" # Check ID-porten curl -s -o /dev/null -w "%{http_code}" https://test.idporten.no/.well-known/openid-configuration | grep 200 || echo "Warning: ID-porten service is not accessible" # Check for updated development documentation echo "Checking for updated development documentation..." grep -r "LocalDevelopment" docs/ || echo "Warning: Development documentation may need to be updated to reflect recent changes" echo "Verification complete. Please address any warnings before proceeding."Length of output: 1150
src/Digdir.Domain.Dialogporten.WebApi/appsettings.Development.json (1)
75-75: Approved: Disabling local development compact JWS generatorThis change aligns with the PR objective of adding a DialogToken requirement for subscriptions. It likely enforces stricter token generation in the local development environment.
Please verify the following:
- Ensure that this change doesn't break local development workflows.
- Confirm that all team members are aware of this change and its implications on local testing.
- Update any local development documentation to reflect this change.
Additionally, I noticed that the "DisableAuth" setting is still set to true. Is this intentional, or should it also be set to false to fully enforce authentication in the local environment?
src/Digdir.Domain.Dialogporten.GraphQL/Program.cs (1)
103-103: Please provide more context about DialogTokenMiddlewareThe addition of
DialogTokenMiddlewareto the request pipeline is a significant change. Could you please provide more information about its purpose and functionality? Specifically:
- What does this middleware do, and how does it relate to the DialogToken requirement for subscriptions mentioned in the PR objectives?
- Are there any performance implications of adding this middleware to all requests?
- Should this middleware be conditionally applied only to certain routes (e.g., subscription-related endpoints) rather than all routes?
Also, consider adding a comment in the code explaining the purpose of this middleware and its placement in the pipeline for future maintainability.
To better understand the implementation of
DialogTokenMiddleware, could you provide its source code? You can use the following command:Verification successful
DialogTokenMiddleware Addition Verified
The addition of
DialogTokenMiddlewareto the request pipeline is appropriate and aligns with the PR objectives. This middleware effectively validates theDigDir-Dialog-Tokento ensure secure operations related to GraphQL subscriptions.
- Purpose: Validates the custom DialogToken JWT to authenticate and authorize specific requests.
- Placement: Positioned after authentication and authorization middleware, ensuring it acts on authenticated requests without interfering with global request logging.
- Performance: Minimal impact due to efficient token validation using pre-imported public keys.
- Scope: Currently applied globally; consider conditional application if DialogToken is only required for specific routes to optimize performance.
Consider adding inline comments within the middleware to enhance future maintainability and provide clarity on its functionality and placement.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the DialogTokenMiddleware implementation rg -t cs -A 20 "class DialogTokenMiddleware"Length of output: 2703
Script:
#!/bin/bash # Retrieve the full content of DialogTokenMiddleware.cs for complete analysis cat src/Digdir.Domain.Dialogporten.GraphQL/Common/Authorization/DialogTokenMiddleware.csLength of output: 2544
src/Digdir.Domain.Dialogporten.Application/Common/ICompactJwsGenerator.cs (4)
25-25: Excellent optimization: Initializing signing key in the constructorMoving the
InitSigningKey()call to the constructor is a great improvement. This change ensures that the signing key is initialized only once during object creation, rather than potentially multiple times in theGetCompactJwsmethod. This optimization will likely improve performance, especially ifGetCompactJwsis called frequently.Benefits of this change:
- Improved performance by avoiding redundant initializations.
- Guaranteed initialization state before any method calls.
- Adheres to the principle of performing initialization work in the constructor.
Great job on this optimization!
Line range hint
66-77: Robust JWT verification in VerifyCompactJws methodThe
VerifyCompactJwsmethod in theEd25519Generatorclass performs important security checks, including verifying thekid(Key ID) and the signature. This is a crucial part of ensuring the integrity and authenticity of the JWTs.The implementation looks secure and follows best practices for JWT verification. Good job on including these important checks!
Line range hint
1-143: Overall assessment: Well-implemented JWT handling with room for minor improvementsThe file demonstrates a robust implementation of JWT generation and verification using the Ed25519 algorithm. The recent optimization of initializing the signing key in the constructor is a notable improvement. The code is generally well-structured and follows good practices for security-sensitive operations.
Key points:
- The constructor optimization in
Ed25519Generatoris excellent.- JWT verification in
VerifyCompactJwsis thorough and secure.- The
LocalDevelopmentCompactJwsGeneratorDecoratorneeds careful handling to prevent production use.- Consider replacing the custom
Base64Urlimplementation with a standard library if available.Overall, the code is of good quality with attention to security details. Addressing the minor suggestions would further enhance its robustness and maintainability.
Line range hint
93-102: Ensure LocalDevelopmentCompactJwsGeneratorDecorator is not used in productionThe
LocalDevelopmentCompactJwsGeneratorDecoratorclass bypasses actual JWT generation and verification, which is useful for local development but could pose a significant security risk if accidentally used in a production environment.To ensure this class is not used in production, please run the following script:
If the script returns any results, please review those occurrences to ensure they are properly guarded against production use.
Verification successful
Verified:
LocalDevelopmentCompactJwsGeneratorDecoratoris conditionally applied based on environment settings and is not used in production.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of LocalDevelopmentCompactJwsGeneratorDecorator in production code # Test: Search for LocalDevelopmentCompactJwsGeneratorDecorator usage outside of test files rg --type csharp -g '!**/test/**' 'LocalDevelopmentCompactJwsGeneratorDecorator'Length of output: 561
Script:
#!/bin/bash # Description: Verify if LocalDevelopmentCompactJwsGeneratorDecorator is conditionally applied based on environment # Search for LocalDevelopmentCompactJwsGeneratorDecorator usage and check for surrounding environment conditions rg --type csharp 'LocalDevelopmentCompactJwsGeneratorDecorator' src/Digdir.Domain.Dialogporten.Application/ApplicationExtensions.cs -B5 -A5Length of output: 591
docs/schema/V1/schema.verified.graphql (2)
214-216: Summary: DialogToken requirement successfully implemented for subscriptionsThe changes to the GraphQL schema effectively implement the DialogToken requirement for subscriptions as outlined in the PR objectives. Key points:
- The
Subscriptionstype now includes an authorization directive with the "enduserSubscription" policy.- A comment has been added to clarify the DialogToken header requirement.
- The changes are localized to the
Subscriptionstype and don't affect other parts of the schema.These modifications enhance the security of subscriptions by enforcing proper authorization. The implementation aligns well with the PR's goals and maintains the integrity of the existing schema structure.
216-216: Verify the implementation of the "enduserSubscription" policy.The authorization directive uses the "enduserSubscription" policy, which is not defined in this schema file. To ensure the correct implementation of this new requirement:
- Verify that the "enduserSubscription" policy is properly defined in the application's authorization system.
- Ensure that the policy correctly validates the DialogToken from the 'DigDir-Dialog-Token' header.
- Confirm that the policy provides the appropriate level of access control for subscriptions.
To assist in verifying the policy implementation, you can run the following script:
This script will help locate the relevant code for the policy implementation and token validation.
src/Digdir.Domain.Dialogporten.GraphQL/EndUser/DialogById/Subscriptions.cs
Show resolved
Hide resolved
...igdir.Domain.Dialogporten.GraphQL/Common/Extensions/HotChocolate/DefinitionNodeExtensions.cs
Outdated
Show resolved
Hide resolved
...igdir.Domain.Dialogporten.GraphQL/Common/Extensions/HotChocolate/DefinitionNodeExtensions.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.GraphQL/Common/Authorization/DialogTokenMiddleware.cs
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.GraphQL/Common/Authorization/DialogTokenMiddleware.cs
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.GraphQL/Common/Authorization/DialogTokenMiddleware.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.GraphQL/Common/Authorization/DialogTokenMiddleware.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.GraphQL/Common/Authorization/AuthorizationOptionsSetup.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.GraphQL/Common/Authorization/AuthorizationOptionsSetup.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.GraphQL/Common/Authorization/DialogTokenMiddleware.cs
Outdated
Show resolved
Hide resolved
|
oskogstad
changed the title
oskogstad
changed the title
oskogstad
deleted the
chore/add-dialog-token-requirement-on-subscriptions
branch
🤖 I have created a release *beep* *boop* --- ## [1.20.0](v1.19.0...v1.20.0) (2024-09-30) ### Features * **GraphQL:** Add DialogToken requirement for subscriptions ([#1124](#1124)) ([651ca62](651ca62)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: Are Almaas <arealmaas@gmail.com>
Description
DialogEvents subscription now requires a valid DialogToken
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
dialogEventssubscription with new authorization requirements.DialogTokenMiddlewarefor handling JWT in requests.Bug Fixes
Documentation