Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Only allow legacy HTML on AditionalInfo content #1210

Merged
merged 11 commits into from
Oct 7, 2024

Conversation

oskogstad
Copy link
Collaborator

@oskogstad oskogstad commented Oct 1, 2024

Description

Added check for content type.
Only add HTML to valid media types if scope is set, and type is AdditionalInfo

Related Issue(s)

Verification

  • Your code builds clean without any errors or warnings
  • Manual testing done (required)
  • Relevant automated test added (if you find this hard, leave it and we'll help out)

Documentation

  • Documentation is updated (either in docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)

Summary by CodeRabbit

  • New Features

    • Enhanced media type validation based on user permissions for dialog content.
    • Introduced flexible initialization for test user claims in integration tests.
    • Added new test cases for dialog creation, specifically validating HTML content handling.
  • Bug Fixes

    • Refined validation rules for media types and content values to ensure proper checks.
  • Tests

    • Expanded integration tests to cover additional scenarios and edge cases for dialog creation.
  • Chores

    • Improved dependency injection setup and initialization process for integration tests.
    • Updated local development configuration to disable various features and enable authentication.

Copy link
Contributor

coderabbitai bot commented Oct 1, 2024

📝 Walkthrough
📝 Walkthrough

Walkthrough

This pull request introduces modifications to the ContentValueDtoValidator, enhancing its media type handling and validation rules based on user permissions. It also updates the DialogApplication class to improve dependency injection for integration tests, introduces flexibility in the IntegrationTestUser class for claims initialization, and expands test coverage in the CreateDialogTests class to validate dialog creation under specific conditions. Additionally, changes to the appsettings.Development.json file adjust local development configurations.

Changes

Files Change Summary
src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Content/ContentValueDtoValidator.cs Modified ContentValueDtoValidator to change LegacyHtmlMediaType from private to public, updated constructor for media type handling based on user permissions, and refined validation rules for media type and value.
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Common/DialogApplication.cs Added ConfigureServiceCollection method for dynamic service registration, modified BuildServiceCollection for service management, and enhanced InitializeAsync for PostgreSQL container management in tests.
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Common/IntegrationTestUser.cs Introduced two constructors for flexible ClaimsPrincipal initialization and added a private method for default claims.
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs Expanded test cases for dialog creation, focusing on HTML media types and user scopes, while introducing helper methods for content value DTO creation.
src/Digdir.Domain.Dialogporten.GraphQL/appsettings.Development.json Modified local development configuration settings, changing several boolean values from true to false and adjusting the DisableAuth setting from true to false.

Possibly related PRs

Suggested reviewers

  • MagnusSandgren: Suggested as a reviewer for their expertise in the relevant code areas.

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@oskogstad oskogstad marked this pull request as ready for review October 1, 2024 14:22
@oskogstad oskogstad requested a review from a team as a code owner October 1, 2024 14:22
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE

📥 Commits

Files that changed from the base of the PR and between dbb79dc and cd3cc8c.

📒 Files selected for processing (4)
  • src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Content/ContentValueDtoValidator.cs (2 hunks)
  • tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Common/DialogApplication.cs (2 hunks)
  • tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Common/IntegrationTestUser.cs (1 hunks)
  • tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs (2 hunks)
🧰 Additional context used
🪛 Gitleaks
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Common/IntegrationTestUser.cs

34-34: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)

🔇 Additional comments (12)
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Common/IntegrationTestUser.cs (4)

8-14: LGTM: Flexible constructor implementation

The new constructor allows for customization of claims while maintaining a set of default claims. This implementation enhances the flexibility of the IntegrationTestUser class, making it more versatile for various testing scenarios.


15-18: LGTM: Default constructor maintains backwards compatibility

The parameterless constructor provides a convenient way to create an IntegrationTestUser with default claims. This maintains backwards compatibility for existing usage while allowing for the new flexible initialization option.


24-39: LGTM: Well-structured default claims method

The GetDefaultClaims method effectively encapsulates the default claims, improving code maintainability. The hardcoded values are appropriate for testing purposes.

Regarding the static analysis hint:
The flagged "Generic API Key" in line 34 is a false positive. The value "0192:991825827" appears to be a test identifier within the JSON structure of the "consumer" claim, not an actual API key.

🧰 Tools
🪛 Gitleaks

34-34: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)


8-39: Summary: Improved flexibility and maintainability of IntegrationTestUser

The changes to the IntegrationTestUser class significantly enhance its flexibility and maintainability:

  1. The new constructor allowing custom claims enables more diverse testing scenarios.
  2. The default constructor maintains backwards compatibility.
  3. The GetDefaultClaims method improves code organization and maintainability.

These improvements align well with the PR objectives and support better integration testing capabilities. The changes are well-implemented and follow good coding practices.

🧰 Tools
🪛 Gitleaks

34-34: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.

(generic-api-key)

src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Content/ContentValueDtoValidator.cs (2)

20-20: Changing LegacyHtmlMediaType to public is appropriate

Making LegacyHtmlMediaType public allows it to be accessed outside the class, which is necessary for the updated validation logic involving user permissions.


72-75: Conditional check correctly restricts LegacyHtmlMediaType to AdditionalInfo content

The added conditional ensures that LegacyHtmlMediaType is only appended to the allowed media types when the contentType.Id is DialogContentType.Values.AdditionalInfo. This aligns with the PR objective to allow legacy HTML only on AdditionalInfo content types.

tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Common/DialogApplication.cs (2)

22-22: Importing the necessary extension methods

The addition of using Microsoft.Extensions.DependencyInjection.Extensions; is required for the RemoveAll<T>() method used later in the code.


89-92: Verify the impact of QuerySplittingBehavior.SplitQuery

The use of QuerySplittingBehavior.SplitQuery changes how EF Core executes queries involving related data, potentially impacting performance and data consistency in tests.

Run the following script to identify all usages of QuerySplittingBehavior in the codebase and ensure it's used appropriately:

✅ Verification successful

Usage of QuerySplittingBehavior.SplitQuery Verified

The application of QuerySplittingBehavior.SplitQuery is limited to the intended files:

  • src/Digdir.Domain.Dialogporten.Infrastructure/InfrastructureExtensions.cs
  • tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Common/DialogApplication.cs

No additional instances were found in the codebase, ensuring that its usage is controlled and deliberate.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find all occurrences of QuerySplittingBehavior configurations.

# Test: Search for QuerySplittingBehavior usage.
# Expect: Only intentional configurations are present.

rg --type cs 'UseQuerySplittingBehavior\(QuerySplittingBehavior\.SplitQuery\)'

Length of output: 421

tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs (4)

290-290: Ensure proper collection initialization for AdditionalInfo

When assigning CreateHtmlContentValueDto() to AdditionalInfo, make sure that any collection properties within are properly initialized to avoid runtime errors.

Please double-check that all collection properties are correctly initialized.


305-320: Verify that the correct scope allows HTML content for AdditionalInfo

The test checks that HTML content can be created with the correct scope. Ensure that the scope is correctly set and that the test would fail if the scope were missing or incorrect.

Please confirm that removing or changing the scope causes this test to fail as expected.


323-342: Clarify test method name and purpose

The test method name Cannot_Create_Title_Content_With_Html_MediaType_With_Correct_Scope might be confusing since it suggests that even with the correct scope, creating title content with HTML media type is not allowed. Ensure this behavior is intended and aligns with the business rules.

Please confirm that it is indeed expected that Title content cannot have HTML media type, even with the correct scope. If so, the test name is appropriate.


327-327: ⚠️ Potential issue

Fix invalid collection initialization in Content.Title

Similar to previous instances, ensure that collections are initialized correctly when assigning CreateHtmlContentValueDto() to Content.Title.

Apply this diff to correct the syntax:

-createDialogCommand.Content.Title = CreateHtmlContentValueDto();
+createDialogCommand.Content.Title = CreateHtmlContentValueDto();

[Note: If CreateHtmlContentValueDto() returns an object with properly initialized collections after fixing previous issues, this line may be correct.]

Likely invalid or redundant comment.

arealmaas
arealmaas previously approved these changes Oct 1, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE

📥 Commits

Files that changed from the base of the PR and between cd3cc8c and 8168153.

📒 Files selected for processing (1)
  • src/Digdir.Domain.Dialogporten.GraphQL/appsettings.Development.json (1 hunks)
🔇 Additional comments (1)
src/Digdir.Domain.Dialogporten.GraphQL/appsettings.Development.json (1)

64-64: Authentication enabled in development environment

The DisableAuth setting has been changed to false, enabling authentication in the development environment. This change enhances security but may require additional setup for developers. Please ensure that:

  1. Clear instructions are provided for setting up and managing authentication in the development environment.
  2. Any potential impact on automated tests or CI/CD pipelines is addressed.
  3. Developers have the necessary credentials or mock authentication services to work in this environment.

To verify the impact of this change, please run the following script:

#!/bin/bash
# Description: Check for any hardcoded auth bypasses or test-specific auth configurations

echo "Checking for potential auth-related issues:"
rg --type csharp -i "(disableauth|bypassauth|testauth)"

This script will help identify any code that might be relying on disabled authentication or using test-specific auth configurations.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE

📥 Commits

Files that changed from the base of the PR and between 8168153 and 926e194.

📒 Files selected for processing (2)
  • tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Common/DialogApplication.cs (4 hunks)
  • tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs (2 hunks)
🧰 Additional context used
📓 Learnings (1)
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs (1)
Learnt from: oskogstad
PR: digdir/dialogporten#1210
File: tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs:282-282
Timestamp: 2024-10-01T17:03:49.291Z
Learning: Using square brackets `[]` for collection initialization is acceptable in the codebase's C# code.
🔇 Additional comments (3)
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs (3)

279-285: Helper method CreateHtmlContentValueDto is well implemented

The method provides a reusable way to create HTML content values for testing purposes.


287-304: Test Cannot_Create_AdditionalInfo_Content_With_Html_MediaType_Without_Correct_Scope correctly verifies unauthorized access

The test accurately checks that creating AdditionalInfo content with HTML media type fails without the correct scope.


306-326: Test Can_Create_AdditionalInfo_Content_With_Html_MediaType_With_Correct_Scope successfully validates authorized access

The test confirms that a user with the appropriate scope can create AdditionalInfo content with HTML media type.

Copy link

sonarqubecloud bot commented Oct 4, 2024

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE

📥 Commits

Files that changed from the base of the PR and between 926e194 and a467e2a.

📒 Files selected for processing (1)
  • tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Common/DialogApplication.cs (4 hunks)
🔇 Additional comments (4)
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Common/DialogApplication.cs (4)

38-38: Introduction of _fixtureRootProvider

The addition of _fixtureRootProvider provides a consistent service provider for the fixture, enhancing test reliability and isolation.


95-97: Verify the use of SplitQuery behavior

The UseQuerySplittingBehavior(QuerySplittingBehavior.SplitQuery) configuration changes how EF Core handles query splitting, which can impact performance and data retrieval patterns.

Ensure that this setting aligns with your application's requirements and that it's necessary for your integration tests.


187-187: Proper disposal of _fixtureRootProvider

Including await _fixtureRootProvider.DisposeAsync(); in DisposeAsync() ensures that all resources are correctly released, preventing potential memory leaks.


210-214: Resetting _rootProvider to maintain test isolation

By disposing _rootProvider and resetting it to _fixtureRootProvider in ResetState(), you effectively prevent state leakage between tests caused by singleton services, enhancing test reliability.

@oskogstad oskogstad merged commit aa4acde into main Oct 7, 2024
22 checks passed
@oskogstad oskogstad deleted the fix/only-aditional-info-should-support-legacy-html branch October 7, 2024 08:59
oskogstad pushed a commit that referenced this pull request Oct 7, 2024
🤖 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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants