feat: Create Dialogporten Serviceowner client library #1513
Conversation
|
Warning Rate limit exceeded@Fargekritt has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 43 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. 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive update to the Dialogporten project, focusing on creating a new Web API client SDK. The changes span multiple aspects of the project, including workflow configurations, solution structure, and new client libraries. A significant addition is 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
|
src/Digdir.Library.Dialogporten.WebApiClient/Digdir.Library.Dialogporten.WebApiClient.csproj
Show resolved
Hide resolved
Fargekritt
force-pushed
the
feat/add-WepApiClient-project
branch
from
b859eb3 to
d33461f
Compare
# Conflicts: # src/Digdir.Domain.Dialogporten.WebApi/Digdir.Domain.Dialogporten.WebApi.csproj
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (6)
src/Digdir.Library.Dialogporten.WebApiClient/Digdir.Library.Dialogporten.WebApiClient.csproj (1)
15-15:⚠️ Potential issueSecurity vulnerability in Maskinporten package.
Based on the past review comments, version 9.2.0 likely has the same moderate severity vulnerabilities as 9.2.1 in its JWT dependencies.
src/Digdir.Library.Dialogporten.WebApiClient.Sample/Program.cs (2)
31-33:⚠️ Potential issueRemove hardcoded JSON Web Token (JWT)
Including a hardcoded JWT in the code poses a significant security risk. Tokens should be securely obtained at runtime and not stored in source code.
Apply this diff to replace the hardcoded token with a secure retrieval method:
-var token = - "eyJhbGciOiJFZERTQSIsInR5cCI6IkpXVCIsImtpZCI6ImRldi1wcmltYXJ5LXNpZ25pbmcta2V5In0.eyJqdGkiOiIzNGZhMGViNS0xZGVmLTQxMDYtYWY4YS0xMjljYjNiNTliNDYiLCJjIjoidXJuOmFsdGlubjpwZXJzb246aWRlbnRpZmllci1ubzowODg5NTY5OTY4NCIsImwiOjMsInAiOiJ1cm46YWx0aW5uOnBlcnNvbjppZGVudGlmaWVyLW5vOjA4ODk1Njk5Njg0IiwicyI6InVybjphbHRpbm46cmVzb3VyY2U6c3VwZXItc2ltcGxlLXNlcnZpY2UiLCJpIjoiMDE5MzI1MzgtMzEzZC03NGI1LTg1ZWMtMWI5MGIxMjYzNWRjIiwiYSI6InJlYWQiLCJpc3MiOiJodHRwczovL2xvY2FsaG9zdDo3MjE0L2FwaS92MSIsImlhdCI6MTczMTU3ODk5OCwibmJmIjoxNzMxNTc4OTk4LCJleHAiOjE3MzE1Nzk1OTh9.fL-rpDsXqwOSVk5zMizLZRaFugaz2VfVNf0CjOxIhSdwrkAhh1UfRu5RcD2OK4ddnRrCuz8iKKJyadkek9UGAg"; +var token = await RetrieveTokenAsync(); // Implement a secure token retrieval methodEnsure that
RetrieveTokenAsyncsecurely obtains the token at runtime.🧰 Tools
🪛 Gitleaks (8.21.2)
32-32: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
59-59:⚠️ Potential issueIncorrect assignment to
SystemLabelpropertyAssigning an enum value within array brackets is incorrect. The
SystemLabelproperty expects a singleSystemLabelvalue, not an array.Apply this diff to correct the assignment:
- SystemLabel = [DialogEndUserContextsEntities_SystemLabel.Default] + SystemLabel = DialogEndUserContextsEntities_SystemLabel.Default.github/workflows/ci-cd-staging.yml (1)
103-104:⚠️ Potential issueFix secret name mismatch in workflow.
The workflow uses
NUGET_API_TEST_KEYbut the reusable workflow expectsNUGET_API_KEY.secrets: - NUGET_API_KEY: ${{ secrets.NUGET_API_TEST_KEY }} + NUGET_API_KEY: ${{ secrets.NUGET_API_KEY }}.github/workflows/ci-cd-main.yml (2)
123-123:⚠️ Potential issueImprove robustness of project file path resolution.
The current implementation using
findcommand embedded in thepathparameter could be unreliable:
- Shell syntax might not work consistently in GitHub Actions.
- Pattern could match multiple files if there are test projects with similar names.
Consider using a dedicated step to find the project file:
publish-sdk-to-nuget: uses: ./.github/workflows/workflow-publish-nuget.yml needs: [ get-current-version, generate-git-short-sha, check-for-changes ] if: ${{ needs.check-for-changes.outputs.hasBackendChanges == 'true' || needs.check-for-changes.outputs.hasTestChanges == 'true' }} + run: | + PROJECT_PATH=$(find . -name "Digdir.Library.Dialogporten.WebApiClient.csproj" -not -path "*/test/*" -print -quit) + echo "PROJECT_PATH=$PROJECT_PATH" >> $GITHUB_ENV with: version: ${{ needs.get-current-version.outputs.version }}-rc.${{ needs.generate-git-short-sha.outputs.gitShortSha }} - path: $(find . -name '*Digdir.Library.Dialogporten.WebApiClient.csproj' -printf "%p" -quit) + path: ${{ env.PROJECT_PATH }}
124-125:⚠️ Potential issueFix secret name mismatch in workflow.
The workflow uses
NUGET_API_TEST_KEYbut the reusable workflow expectsNUGET_API_KEY.secrets: - NUGET_API_KEY: ${{ secrets.NUGET_API_TEST_KEY }} + NUGET_API_KEY: ${{ secrets.NUGET_API_KEY }}
🧹 Nitpick comments (9)
src/Digdir.Library.Dialogporten.WebApiClient/.refitter (2)
16-16: Consider using RFC 3339 format with timezone offset.The current date format
yyyy-MM-ddTHH:mm:ssZassumes UTC (Z). Consider usingyyyy-MM-ddTHH:mm:sszzzto support explicit timezone offsets, which is more flexible for international deployments.- "dateFormat": "yyyy-MM-ddTHH:mm:ssZ" + "dateFormat": "yyyy-MM-ddTHH:mm:sszzz"
1-18: Add documentation comments.Consider adding a header comment block to document:
- The purpose of this configuration
- The dependency on Refitter version
- Any special considerations for developers
Add this comment block at the top of the file:
+// Configuration for Refitter code generation tool +// Generates Web API client for Dialogporten Serviceowner +// Note: Requires Refitter built from source: https://github.com/christianhelle/refitter + {src/Digdir.Library.Dialogporten.WebApiClient.Sample/Program.cs (1)
125-240: Refactor duplicated code inCreateCommand()andUpdateCommand()methodsThe methods
CreateCommand()andUpdateCommand()contain duplicated code. Consider refactoring shared code into helper methods or using a builder pattern to improve maintainability and reduce duplication.Also applies to: 243-356
tests/Digdir.Library.Dialogporten.WebApiClient.Integration.Tests/WebApiClientTests.cs (1)
283-397: Refactor duplicated code inCreateCommand()andUpdateCommand()methodsThe methods
CreateCommand()andUpdateCommand()have similar implementations. Refactoring common code into shared methods or utilizing a builder pattern can reduce code duplication and enhance maintainability.Also applies to: 398-553
.github/workflows/ci-cd-pull-request.yml (1)
21-21: Remove trailing spaces.There are trailing spaces at the end of these lines.
Apply this diff:
- + - + - +Also applies to: 32-32, 43-43
🧰 Tools
🪛 yamllint (1.35.1)
[error] 21-21: trailing spaces
(trailing-spaces)
src/Digdir.Library.Dialogporten.WebApiClient/README.md (2)
4-4: Fix bare URL formatting.Use proper Markdown link syntax for the URL.
Apply this diff:
-Refit-based client SDK Based on https://github.com/altinn/altinn-apiclient-maskinporten +Refit-based client SDK Based on [altinn-apiclient-maskinporten](https://github.com/altinn/altinn-apiclient-maskinporten)🧰 Tools
🪛 Markdownlint (0.37.0)
4-4: null
Bare URL used(MD034, no-bare-urls)
39-56: Add example values in configuration.The configuration example should include sample values to help users understand the expected format.
Consider adding example values like this:
"DialogportenSettings": { - "Environment": "", + "Environment": "test", "Maskinporten": { - "ClientId": "", - "Scope": "", - "EncodedJwk": "" + "ClientId": "my-client-id", + "Scope": "altinn:dialog/write", + "EncodedJwk": "base64-encoded-jwk" } }, "Ed25519Keys": { "Primary": { - "Kid": "", - "PublicComponent": "" + "Kid": "primary-key-id", + "PublicComponent": "base64-encoded-public-key" }, "Secondary": { - "Kid": "", - "PublicComponent": "" + "Kid": "secondary-key-id", + "PublicComponent": "base64-encoded-public-key" } }.github/workflows/ci-cd-staging.yml (1)
105-105: Remove trailing whitespace.Remove the trailing whitespace on this line.
- +🧰 Tools
🪛 yamllint (1.35.1)
[error] 105-105: trailing spaces
(trailing-spaces)
.github/workflows/ci-cd-main.yml (1)
126-126: Remove trailing whitespace.Remove the trailing whitespace on this line.
- +🧰 Tools
🪛 yamllint (1.35.1)
[error] 126-126: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
.github/workflows/ci-cd-main.yml(2 hunks).github/workflows/ci-cd-pull-request.yml(3 hunks).github/workflows/ci-cd-staging.yml(1 hunks).github/workflows/workflow-build-and-test.yml(2 hunks).github/workflows/workflow-publish-nuget.yml(1 hunks)Digdir.Domain.Dialogporten.sln(3 hunks)src/Digdir.Domain.Dialogporten.WebApi/Digdir.Domain.Dialogporten.WebApi.csproj(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/EndUser/DialogActivities/Get/GetDialogActivityEndpointSummary.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Program.cs(5 hunks)src/Digdir.Domain.Dialogporten.WebApi/nswag.json(1 hunks)src/Digdir.Library.Dialogporten.WebApiClient.Sample/Program.cs(1 hunks)src/Digdir.Library.Dialogporten.WebApiClient/.refitter(1 hunks)src/Digdir.Library.Dialogporten.WebApiClient/Digdir.Library.Dialogporten.WebApiClient.csproj(1 hunks)src/Digdir.Library.Dialogporten.WebApiClient/Extensions/ServiceCollectionExtensions.cs(1 hunks)src/Digdir.Library.Dialogporten.WebApiClient/README.md(1 hunks)src/Digdir.Library.Dialogporten.WebApiClient/Services/DialogTokenVerifier.cs(1 hunks)tests/Digdir.Library.Dialogporten.WebApiClient.Integration.Tests/Digdir.Library.Dialogporten.WebApiClient.Integration.Tests.csproj(1 hunks)tests/Digdir.Library.Dialogporten.WebApiClient.Integration.Tests/WebApiClientTests.cs(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/EndUser/DialogActivities/Get/GetDialogActivityEndpointSummary.cs
- src/Digdir.Domain.Dialogporten.WebApi/nswag.json
- src/Digdir.Domain.Dialogporten.WebApi/Program.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- Digdir.Domain.Dialogporten.sln
- tests/Digdir.Library.Dialogporten.WebApiClient.Integration.Tests/Digdir.Library.Dialogporten.WebApiClient.Integration.Tests.csproj
- src/Digdir.Library.Dialogporten.WebApiClient/Extensions/ServiceCollectionExtensions.cs
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/ci-cd-main.yml
[error] 126-126: trailing spaces
(trailing-spaces)
.github/workflows/ci-cd-pull-request.yml
[error] 21-21: trailing spaces
(trailing-spaces)
[error] 32-32: trailing spaces
(trailing-spaces)
[error] 43-43: trailing spaces
(trailing-spaces)
.github/workflows/ci-cd-staging.yml
[error] 105-105: trailing spaces
(trailing-spaces)
.github/workflows/workflow-publish-nuget.yml
[error] 62-62: trailing spaces
(trailing-spaces)
🪛 Markdownlint (0.37.0)
src/Digdir.Library.Dialogporten.WebApiClient/README.md
4-4: null
Bare URL used
(MD034, no-bare-urls)
🪛 actionlint (1.7.4)
.github/workflows/workflow-publish-nuget.yml
61-61: shellcheck reported issue in this script: SC2035:info:1:19: Use ./glob or -- glob so names with dashes won't become options
(shellcheck)
🪛 Gitleaks (8.21.2)
src/Digdir.Library.Dialogporten.WebApiClient.Sample/Program.cs
32-32: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
🔇 Additional comments (12)
src/Digdir.Library.Dialogporten.WebApiClient/.refitter (2)
3-3: Verify namespace alignment with project conventions.The namespace
Digdir.Library.Dialogporten.WebApiClient.Features.V1should align with the project's namespace conventions.✅ Verification successful
Namespace follows project conventions ✅
The namespace
Digdir.Library.Dialogporten.WebApiClient.Features.V1perfectly aligns with the project's established namespace hierarchy and versioning pattern.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check namespace conventions across the project # Search for other namespace declarations to verify consistency rg -t cs "namespace.*Digdir\.Library\.Dialogporten" --no-filename | sort | uniqLength of output: 456
8-10: Verify completeness of included tags.The configuration only includes the "Serviceowner" tag. Verify if this covers all required API endpoints for the service owner client library.
✅ Verification successful
The "Serviceowner" tag inclusion is correct and complete
This is a service owner-specific client library, and the tag configuration correctly includes only the endpoints needed for this purpose. The separation from end-user and metadata endpoints is intentional and follows good API design practices.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Extract and compare available tags from swagger.json # Find all available tags in swagger.json fd swagger.json -x jq -r '.tags[].name' {} \;Length of output: 50
Script:
#!/bin/bash # Search for OpenAPI/Swagger specification files with various extensions echo "=== Looking for API specification files ===" fd -e json -e yaml -e yml | grep -i 'swagger\|openapi' echo -e "\n=== Searching for API endpoint attributes and tags in code ===" # Look for API controller attributes and tag usage rg -l "(\[ApiController\]|\[Route\]|\[Tags?\(|Serviceowner)" --type-add 'cs:*.cs' -tcs echo -e "\n=== Examining .refitter files ===" # Look at all .refitter files for context fd .refitter -x cat {}Length of output: 1009
Script:
#!/bin/bash echo "=== API Tags from swagger.verified.json ===" jq -r '.tags[] | "Tag: \(.name)\nDescription: \(.description)\n"' docs/schema/V1/swagger.verified.json echo -e "\n=== Paths grouped by tags ===" jq -r '.paths | to_entries[] | select(.value[].tags) | "\(.key): \(.value[].tags[])"' docs/schema/V1/swagger.verified.json | sortLength of output: 3209
src/Digdir.Library.Dialogporten.WebApiClient/Digdir.Library.Dialogporten.WebApiClient.csproj (2)
30-32: LGTM! Good practice excluding test artifacts.Correctly excludes
.received.csfiles which are typically generated during snapshot testing.
16-17: Verify .NET 9 compatibility.The project uses .NET 9 packages which are currently in preview. Consider using stable .NET 8 versions for production use:
- <PackageReference Include="Microsoft.Extensions.Caching.Memory" Version="9.0.0"/> - <PackageReference Include="Microsoft.Extensions.Configuration.Json" Version="9.0.0"/> + <PackageReference Include="Microsoft.Extensions.Caching.Memory" Version="8.0.0"/> + <PackageReference Include="Microsoft.Extensions.Configuration.Json" Version="8.0.0"/>.github/workflows/workflow-build-and-test.yml (2)
21-21: Verify the 'test' environment configurationConfirm that the 'test' environment specified is correctly set up in GitHub and that the necessary protection rules and secrets are configured appropriately.
37-43: Ensure secrets are not exposed in logs during testingWhen passing secrets through environment variables, make sure that these are not inadvertently logged during the test execution. Review the test configurations to prevent logging of sensitive information.
.github/workflows/workflow-publish-nuget.yml (2)
1-21: LGTM! Well-structured workflow with clear inputs and secrets.The workflow is well-organized with appropriate input validation and defaults. The use of a default NuGet test source is a good practice.
61-62:⚠️ Potential issueFix NuGet push command and remove trailing spaces.
The NuGet push command needs proper globbing and the file has trailing whitespace.
Apply this diff:
- run: dotnet nuget push *.nupkg --source "${{ inputs.source }}" --api-key ${{secrets.NUGET_API_KEY}} - + run: dotnet nuget push "./*.nupkg" --source "${{ inputs.source }}" --api-key "${{secrets.NUGET_API_KEY}}"Likely invalid or redundant comment.
🧰 Tools
🪛 actionlint (1.7.4)
61-61: shellcheck reported issue in this script: SC2035:info:1:19: Use ./glob or -- glob so names with dashes won't become options
(shellcheck)
🪛 yamllint (1.35.1)
[error] 62-62: trailing spaces
(trailing-spaces)
src/Digdir.Library.Dialogporten.WebApiClient/Services/DialogTokenVerifier.cs (1)
26-28: 🛠️ Refactor suggestionAdd error handling for signature verification.
The
SignatureAlgorithm.Ed25519.Verifymethod may throw exceptions if the inputs are invalid. Consider wrapping it in a try-catch block.Apply this diff:
- var signature = Base64Url.DecodeFromChars(parts[2]); - return SignatureAlgorithm.Ed25519.Verify(publicKey, Encoding.UTF8.GetBytes(parts[0] + '.' + parts[1]), signature); + try + { + var signature = Base64Url.DecodeFromChars(parts[2]); + return SignatureAlgorithm.Ed25519.Verify(publicKey, Encoding.UTF8.GetBytes(parts[0] + '.' + parts[1]), signature); + } + catch + { + return false; + }Likely invalid or redundant comment.
src/Digdir.Domain.Dialogporten.WebApi/Digdir.Domain.Dialogporten.WebApi.csproj (1)
16-20: LGTM! Well-configured NSwag setup for OpenAPI generation.The NSwag packages and target are properly configured to generate OpenAPI specifications during Release builds.
Also applies to: 43-45
.github/workflows/ci-cd-staging.yml (1)
102-102:⚠️ Potential issueImprove robustness of project file path resolution.
The current implementation using
findcommand embedded in thepathparameter could be unreliable:
- Shell syntax might not work consistently in GitHub Actions.
- Pattern could match multiple files if there are test projects with similar names.
Consider using a dedicated step to find the project file:
publish-sdk-to-nuget: uses: ./.github/workflows/workflow-publish-nuget.yml needs: [ get-current-version, check-for-changes ] if: ${{ needs.check-for-changes.outputs.hasBackendChanges == 'true' || needs.check-for-changes.outputs.hasTestChanges == 'true' }} + run: | + PROJECT_PATH=$(find . -name "Digdir.Library.Dialogporten.WebApiClient.csproj" -not -path "*/test/*" -print -quit) + echo "PROJECT_PATH=$PROJECT_PATH" >> $GITHUB_ENV with: version: ${{ needs.get-current-version.outputs.version }} - path: $(find . -name '*Digdir.Library.Dialogporten.WebApiClient.csproj' -printf "%p" -quit) + path: ${{ env.PROJECT_PATH }}Likely invalid or redundant comment.
.github/workflows/ci-cd-main.yml (1)
45-50: LGTM! Well-structured secrets configuration.The secrets are properly organized and named according to their purpose.
| @@ -0,0 +1,18 @@ | |||
| { | |||
| "openApiPath": "../Digdir.Domain.Dialogporten.WebApi/swagger.json", | |||
There was a problem hiding this comment.
💡 Codebase verification
Update the swagger.json path in .refitter configuration
The configured path ../Digdir.Domain.Dialogporten.WebApi/swagger.json is incorrect. The swagger specification file is actually located at docs/schema/V1/swagger.verified.json. Update the openApiPath setting to point to the correct location:
"openApiPath": "../../docs/schema/V1/swagger.verified.json",🔗 Analysis chain
Verify the swagger.json path exists and is up-to-date.
The relative path to swagger.json might be brittle. Let's verify its existence and location.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the swagger.json file exists and check its content
# Find the swagger.json file
fd swagger.json
# If found, validate it's a proper OpenAPI/Swagger file
fd swagger.json -x jq -e 'has("openapi") or has("swagger")' {} \;
Length of output: 88
Script:
#!/bin/bash
# Check if the WebApi project exists and look for swagger/openapi files
echo "Looking for WebApi project:"
fd Digdir.Domain.Dialogporten.WebApi
echo -e "\nLooking for swagger/openapi files:"
fd -e json -e yaml -e yml | grep -i "swagger\|openapi"
Length of output: 653
| <PropertyGroup> | ||
| <ImplicitUsings>enable</ImplicitUsings> | ||
| <Nullable>enable</Nullable> | ||
| <PackageReadmeFile>README.md</PackageReadmeFile> | ||
| <PackageLicenseFile>LICENSE</PackageLicenseFile> | ||
| <Company>Digitaliseringsdirektoratet</Company> | ||
| <PackageTags>digdir;altinn;dialogporten</PackageTags> | ||
| <RepositoryType>git</RepositoryType> | ||
| <RepositoryUrl>https://github.com/digdir/dialogporten</RepositoryUrl> | ||
| </PropertyGroup> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add package version property.
The project properties are missing the <Version> or <PackageVersion> property which is crucial for NuGet package versioning. Consider adding semantic versioning.
<PropertyGroup>
<ImplicitUsings>enable</ImplicitUsings>
<Nullable>enable</Nullable>
+ <Version>1.0.0</Version>
<PackageReadmeFile>README.md</PackageReadmeFile>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <PropertyGroup> | |
| <ImplicitUsings>enable</ImplicitUsings> | |
| <Nullable>enable</Nullable> | |
| <PackageReadmeFile>README.md</PackageReadmeFile> | |
| <PackageLicenseFile>LICENSE</PackageLicenseFile> | |
| <Company>Digitaliseringsdirektoratet</Company> | |
| <PackageTags>digdir;altinn;dialogporten</PackageTags> | |
| <RepositoryType>git</RepositoryType> | |
| <RepositoryUrl>https://github.com/digdir/dialogporten</RepositoryUrl> | |
| </PropertyGroup> | |
| <PropertyGroup> | |
| <ImplicitUsings>enable</ImplicitUsings> | |
| <Nullable>enable</Nullable> | |
| <Version>1.0.0</Version> | |
| <PackageReadmeFile>README.md</PackageReadmeFile> | |
| <PackageLicenseFile>LICENSE</PackageLicenseFile> | |
| <Company>Digitaliseringsdirektoratet</Company> | |
| <PackageTags>digdir;altinn;dialogporten</PackageTags> | |
| <RepositoryType>git</RepositoryType> | |
| <RepositoryUrl>https://github.com/digdir/dialogporten</RepositoryUrl> | |
| </PropertyGroup> |
| <PackageReference Include="Altinn.ApiClients.Maskinporten" Version="9.2.0"/> | ||
| <PackageReference Include="Microsoft.Extensions.Caching.Memory" Version="9.0.0"/> | ||
| <PackageReference Include="Microsoft.Extensions.Configuration.Json" Version="9.0.0"/> | ||
| <PackageReference Include="NSec.Cryptography" Version="24.4.0"/> | ||
| <PackageReference Include="Refit" Version="8.0.0"/> | ||
| <PackageReference Include="Refit.HttpClientFactory" Version="8.0.0"/> | ||
| <None Include="README.md" Pack="true" PackagePath="/"/> | ||
| <None Include="../../LICENSE" Pack="true" PackagePath="/"/> | ||
| <None Include="../../docs/schema/v1/swagger.json" Pack="true" PackagePath="/"/> | ||
| <PackageReference Include="Refitter.SourceGenerator" Version="1.5.0"> | ||
| <PrivateAssets>all</PrivateAssets> | ||
| <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets> | ||
| </PackageReference> | ||
| </ItemGroup> |
There was a problem hiding this comment.
💡 Codebase verification
Critical security vulnerabilities found in package dependencies
Multiple high-severity vulnerabilities detected:
- Microsoft.Extensions.* packages (9.0.0) are affected by multiple Remote Code Execution vulnerabilities (CVE-2025-21171, CVE-2025-21172, CVE-2025-21176)
- Refit and Refit.HttpClientFactory (8.0.0) are potentially vulnerable to CRLF injection attacks
Recommended updates:
- Update Microsoft.Extensions.* packages to version 9.0.1
- Update Refit packages to latest version (>= 8.0.1)
🔗 Analysis chain
Verify package versions and security vulnerabilities.
Let's check the compatibility and security of the package dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check package versions and known vulnerabilities
# Check latest versions on NuGet
echo "Checking latest stable versions..."
for pkg in "Altinn.ApiClients.Maskinporten" "Microsoft.Extensions.Caching.Memory" "Microsoft.Extensions.Configuration.Json" "NSec.Cryptography" "Refit" "Refit.HttpClientFactory" "Refitter.SourceGenerator"; do
curl -s "https://api.nuget.org/v3-flatcontainer/$pkg/index.json" | jq -r '.versions[]' | grep -v '-' | tail -n 1
done
# Check for known vulnerabilities
echo "Checking for known vulnerabilities..."
gh api graphql -f query='
{
securityVulnerabilities(first: 10, ecosystem: NUGET) {
nodes {
package {
name
}
advisory {
summary
severity
}
vulnerableVersionRange
}
}
}' | jq '.data.securityVulnerabilities.nodes[] | select(.package.name | test("Altinn.ApiClients.Maskinporten|Microsoft.Extensions|Refit"))'
Length of output: 3306
Script:
#!/bin/bash
# Check individual packages using NuGet v3 API
for pkg in "Altinn.ApiClients.Maskinporten" "Microsoft.Extensions.Caching.Memory" "Microsoft.Extensions.Configuration.Json" "NSec.Cryptography" "Refit" "Refit.HttpClientFactory" "Refitter.SourceGenerator"; do
echo "Checking $pkg..."
curl -s "https://api.nuget.org/v3/registration5-gz-semver2/$pkg/index.json" | jq -r '.items[0].upper'
done
# Check security advisories for specific packages
echo -e "\nChecking security advisories..."
for pkg in "Altinn.ApiClients.Maskinporten" "Microsoft.Extensions.Caching.Memory" "Microsoft.Extensions.Configuration.Json" "Refit" "Refit.HttpClientFactory"; do
gh api \
-H "Accept: application/vnd.github+json" \
-H "X-GitHub-Api-Version: 2022-11-28" \
"/advisories?ecosystem=NUGET&package=$pkg" | jq '.[].summary'
done
Length of output: 6383
Script:
#!/bin/bash
# Try different NuGet API endpoint format
echo "Checking package versions..."
for pkg in "Altinn.ApiClients.Maskinporten" "Microsoft.Extensions.Caching.Memory" "Microsoft.Extensions.Configuration.Json" "NSec.Cryptography" "Refit" "Refit.HttpClientFactory" "Refitter.SourceGenerator"; do
echo -n "$pkg: "
curl -s "https://api.nuget.org/v3/catalog0/data/2024.01.01.00.00.00/$pkg.json" | jq -r '.version'
done
echo -e "\nChecking security advisories..."
for pkg in "Altinn.ApiClients.Maskinporten" "Microsoft.Extensions.Caching.Memory" "Microsoft.Extensions.Configuration.Json" "Refit" "Refit.HttpClientFactory"; do
echo "Checking $pkg..."
gh api \
-H "Accept: application/vnd.github+json" \
-H "X-GitHub-Api-Version: 2022-11-28" \
"/advisories?ecosystem=nuget&package=$pkg"
done
Length of output: 70927
|
|
||
| var headerJson = JsonSerializer.Deserialize<JsonElement>(header); | ||
|
|
||
| if (!headerJson.TryGetProperty("kid", out var value) && value.GetString() != kid) |
There was a problem hiding this comment.
Fix logical operator in header validation.
The condition uses incorrect logical operator which could lead to false negatives.
Apply this diff:
- if (!headerJson.TryGetProperty("kid", out var value) && value.GetString() != kid)
+ if (!headerJson.TryGetProperty("kid", out var value) || value.GetString() != kid)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!headerJson.TryGetProperty("kid", out var value) && value.GetString() != kid) | |
| if (!headerJson.TryGetProperty("kid", out var value) || value.GetString() != kid) |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
tests/Digdir.Domain.Dialogporten.WebApi.Integration.Tests/Features/V1/SwaggerSnapshotTests.cs (4)
5-5: Scope the warning suppression to the specific method.The CS1998 warning suppression should be moved to the
FailIfSwaggerSnapshotDoesNotMatchmethod to avoid suppressing the warning globally.-#pragma warning disable CS1998 // Async method lacks 'await' operators and will run synchronously namespace Digdir.Domain.Dialogporten.WebApi.Integration.Tests.Features.V1; public class SwaggerSnapshotTests : IClassFixture<WebApplicationFactory<Program>> { // ... + [SuppressMessage("ReSharper", "CS1998")] [Fact] public async Task FailIfSwaggerSnapshotDoesNotMatch()
29-29: Avoid hardcoding file paths.The hardcoded path to
net9.0could break if the target framework version changes. Consider making this configurable or deriving it from the project's target framework.- var newSwaggerPath = Path.Combine(rootPath!, "src/Digdir.Domain.Dialogporten.WebApi/bin/Release/net9.0/swagger.json"); + var targetFramework = typeof(Program).Assembly.GetCustomAttribute<TargetFrameworkAttribute>()?.FrameworkName ?? "net9.0"; + var newSwaggerPath = Path.Combine(rootPath!, "src/Digdir.Domain.Dialogporten.WebApi/bin/Release", targetFramework, "swagger.json");
47-47: Improve the DEBUG mode error message.The error message could be more constructive by suggesting how to run the test in RELEASE mode.
- Assert.Fail("Swagger snapshot tests are not supported in DEBUG mode. Swagger is NOT generated in DEBUG mode, this is to keep build times low. therefore this test will always fail. Run in RELEASE mode to enable."); + Assert.Fail("Swagger snapshot tests require RELEASE mode.\n" + + "To run this test:\n" + + "1. Build the project in RELEASE mode: `dotnet build -c Release`\n" + + "2. Run the test in RELEASE mode: `dotnet test -c Release --filter SwaggerSnapshotTests`");
Line range hint
9-19: Add XML documentation for the test class and method.Adding documentation would help other developers understand:
- The purpose of these snapshot tests
- When and why to run them in RELEASE mode
- The expected outcomes and how to handle failures
+/// <summary> +/// Validates that changes to the Swagger/OpenAPI specification are intentional by comparing against a verified snapshot. +/// These tests must be run in RELEASE mode as Swagger generation is disabled in DEBUG mode for performance reasons. +/// </summary> public class SwaggerSnapshotTests : IClassFixture<WebApplicationFactory<Program>> { private readonly WebApplicationFactory<Program> _webApplicationFactory; public SwaggerSnapshotTests(WebApplicationFactory<Program> webApplicationFactory) { _webApplicationFactory = webApplicationFactory; } + /// <summary> + /// Verifies that the current Swagger/OpenAPI specification matches the approved snapshot. + /// This test helps prevent unintended breaking changes to the API contract. + /// </summary> + /// <remarks> + /// To update the snapshot after intentional API changes: + /// 1. Build in RELEASE mode + /// 2. Run the test + /// 3. Review and accept the changes using the Verify tool + /// </remarks> [Fact] public async Task FailIfSwaggerSnapshotDoesNotMatch()
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Digdir.Domain.Dialogporten.WebApi/Digdir.Domain.Dialogporten.WebApi.csproj(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Program.cs(5 hunks)src/Digdir.Library.Dialogporten.WebApiClient/.refitter(1 hunks)src/Digdir.Library.Dialogporten.WebApiClient/Digdir.Library.Dialogporten.WebApiClient.csproj(1 hunks)tests/Digdir.Domain.Dialogporten.WebApi.Integration.Tests/Features/V1/SwaggerSnapshotTests.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Digdir.Library.Dialogporten.WebApiClient/.refitter
- src/Digdir.Library.Dialogporten.WebApiClient/Digdir.Library.Dialogporten.WebApiClient.csproj
- src/Digdir.Domain.Dialogporten.WebApi/Program.cs
- src/Digdir.Domain.Dialogporten.WebApi/Digdir.Domain.Dialogporten.WebApi.csproj
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build / build-and-test
🔇 Additional comments (1)
tests/Digdir.Domain.Dialogporten.WebApi.Integration.Tests/Features/V1/SwaggerSnapshotTests.cs (1)
43-45: 🛠️ Refactor suggestionVerify file existence before processing.
Add file existence checks to provide better error messages when files are missing.
+ if (!File.Exists(newSwaggerPath)) + { + throw new FileNotFoundException($"Swagger file not found at {newSwaggerPath}. Ensure you've built the project in RELEASE mode."); + } + await Verify(newSwagger, extension: "json") .UseFileName("swagger") .UseDirectory(swaggerPath);Likely invalid or redundant comment.
| private static readonly JsonSerializerOptions SerializerOptions = new() | ||
| { | ||
| WriteIndented = true | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused JSON sorting functionality.
The SerializerOptions, SortJson, and SortElement methods appear to be unused since the JSON sorting functionality is commented out. If this functionality is no longer needed, these methods should be removed to maintain code cleanliness.
Also applies to: 57-89
| // Amund: Stemmer dette lenger? | ||
| // The order of the properties in the swagger.json file is not cross-platform deterministic. | ||
| // Running client.GetAsync("/swagger/v1/swagger.json"); on Windows and Mac will produce | ||
| // different ordering of the results (although the content is the same). So we force an | ||
| // alphabetical ordering of the properties to make the test deterministic. | ||
| // Ref: https://github.com/altinn/dialogporten/issues/996 | ||
| var orderedSwagger = SortJson(newSwagger); | ||
| // var orderedSwagger = SortJson(newSwagger); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove or update the outdated comment.
The comment is in Norwegian and refers to potentially outdated information. If the JSON property ordering issue is still relevant, update the comment in English. If not, remove the commented code and its associated comments.
- // Amund: Stemmer dette lenger?
- // The order of the properties in the swagger.json file is not cross-platform deterministic.
- // Running client.GetAsync("/swagger/v1/swagger.json"); on Windows and Mac will produce
- // different ordering of the results (although the content is the same). So we force an
- // alphabetical ordering of the properties to make the test deterministic.
- // Ref: https://github.com/altinn/dialogporten/issues/996
- // var orderedSwagger = SortJson(newSwagger);
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/Digdir.Domain.Dialogporten.WebApi.Integration.Tests/Features/V1/SwaggerSnapshotTests.cs (1)
46-47: Enhance DEBUG mode failure message with solution steps.The failure message could be more helpful by providing specific steps to run the test in RELEASE mode.
-Assert.Fail("Swagger snapshot tests are not supported in DEBUG mode. Swagger is NOT generated in DEBUG mode, this is to keep build times low. therefore this test will always fail. Run in RELEASE mode to enable."); +Assert.Fail(@"Swagger snapshot tests are not supported in DEBUG mode. +Reason: Swagger is NOT generated in DEBUG mode to keep build times low. + +To run this test: +1. Switch to RELEASE configuration +2. Rebuild the solution +3. Run the test again");
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/Digdir.Domain.Dialogporten.WebApi.Integration.Tests/Features/V1/SwaggerSnapshotTests.cs(2 hunks)
🔇 Additional comments (2)
tests/Digdir.Domain.Dialogporten.WebApi.Integration.Tests/Features/V1/SwaggerSnapshotTests.cs (2)
5-5: LGTM! Well-documented warning suppression.The pragma warning suppression for CS1998 is appropriately documented, explaining that synchronous execution is expected in DEBUG mode.
51-54: LGTM! Clear serializer options configuration.The JSON serializer options are appropriately configured for the sorting functionality, which is necessary for cross-platform deterministic testing.
| // Act | ||
| var response = await client.GetAsync("/swagger/v1/swagger.json"); | ||
| var newSwagger = await response.Content.ReadAsStringAsync(); | ||
| var newSwagger = File.ReadAllText(newSwaggerPath); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for file reading operation.
The file reading operation should be wrapped in a try-catch block to handle potential IO exceptions.
-var newSwagger = File.ReadAllText(newSwaggerPath);
+string newSwagger;
+try
+{
+ newSwagger = File.ReadAllText(newSwaggerPath);
+}
+catch (IOException ex)
+{
+ throw new InvalidOperationException($"Failed to read swagger file at {newSwaggerPath}: {ex.Message}", ex);
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var newSwagger = File.ReadAllText(newSwaggerPath); | |
| string newSwagger; | |
| try | |
| { | |
| newSwagger = File.ReadAllText(newSwaggerPath); | |
| } | |
| catch (IOException ex) | |
| { | |
| throw new InvalidOperationException($"Failed to read swagger file at {newSwaggerPath}: {ex.Message}", ex); | |
| } |
| var swaggerPath = Path.Combine(rootPath!, "docs/schema/V1"); | ||
|
|
||
| var client = _webApplicationFactory | ||
| .WithWebHostBuilder(builder => builder.UseEnvironment("test")) | ||
| .CreateClient(); | ||
|
|
||
| var newSwaggerPath = Path.Combine(rootPath!, "src/Digdir.Domain.Dialogporten.WebApi/bin/Release/net9.0/swagger.json"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for file path operations.
The code assumes the existence of specific paths without proper error handling. Consider adding checks and providing meaningful error messages if the files are not found.
-var swaggerPath = Path.Combine(rootPath!, "docs/schema/V1");
-var newSwaggerPath = Path.Combine(rootPath!, "src/Digdir.Domain.Dialogporten.WebApi/bin/Release/net9.0/swagger.json");
+var swaggerPath = Path.Combine(rootPath!, "docs/schema/V1");
+if (!Directory.Exists(swaggerPath))
+ throw new DirectoryNotFoundException($"Schema directory not found at: {swaggerPath}");
+
+var newSwaggerPath = Path.Combine(rootPath!, "src/Digdir.Domain.Dialogporten.WebApi/bin/Release/net9.0/swagger.json");
+if (!File.Exists(newSwaggerPath))
+ throw new FileNotFoundException($"Swagger file not found at: {newSwaggerPath}");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var swaggerPath = Path.Combine(rootPath!, "docs/schema/V1"); | |
| var client = _webApplicationFactory | |
| .WithWebHostBuilder(builder => builder.UseEnvironment("test")) | |
| .CreateClient(); | |
| var newSwaggerPath = Path.Combine(rootPath!, "src/Digdir.Domain.Dialogporten.WebApi/bin/Release/net9.0/swagger.json"); | |
| var swaggerPath = Path.Combine(rootPath!, "docs/schema/V1"); | |
| if (!Directory.Exists(swaggerPath)) | |
| throw new DirectoryNotFoundException($"Schema directory not found at: {swaggerPath}"); | |
| var newSwaggerPath = Path.Combine(rootPath!, "src/Digdir.Domain.Dialogporten.WebApi/bin/Release/net9.0/swagger.json"); | |
| if (!File.Exists(newSwaggerPath)) | |
| throw new FileNotFoundException($"Swagger file not found at: {newSwaggerPath}"); |
Renamed WepApi Integration test to WepApi Unit test
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/Digdir.Domain.Dialogporten.WebApi.Unit.Tests/Features/V1/SwaggerSnapshotTests.cs (1)
13-40: LGTM: Smart approach to conditional compilation.The separation between DEBUG and RELEASE modes is well thought out, preventing unnecessary swagger generation during development while maintaining verification in release builds.
Consider using path utilities for more robust path handling.
The hardcoded path segments could be fragile across different environments.
Consider this improvement:
- var swaggerPath = Path.Combine(rootPath!, "docs/schema/V1"); - var newSwaggerPath = Path.Combine(rootPath!, "src/Digdir.Domain.Dialogporten.WebApi/bin/Release/net9.0/swagger.json"); + var swaggerPath = Path.Combine(rootPath!, "docs", "schema", "V1"); + var newSwaggerPath = Path.Combine(rootPath!, "src", "Digdir.Domain.Dialogporten.WebApi", "bin", "Release", "net9.0", "swagger.json");
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Digdir.Domain.Dialogporten.sln(4 hunks)docs/schema/V1/swagger.verified.json(1 hunks)tests/Digdir.Domain.Dialogporten.WebApi.Unit.Tests/Features/V1/SwaggerSnapshotTests.cs(1 hunks)tests/Digdir.Domain.Dialogporten.WebApi.Unit.Tests/Utils.cs(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- tests/Digdir.Domain.Dialogporten.WebApi.Unit.Tests/Utils.cs
- docs/schema/V1/swagger.verified.json
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Dry run deploy infrastructure / Deploy to test
- GitHub Check: build / build-and-test
🔇 Additional comments (4)
tests/Digdir.Domain.Dialogporten.WebApi.Unit.Tests/Features/V1/SwaggerSnapshotTests.cs (2)
3-7: LGTM: Namespace change aligns with test type.The transition from integration tests to unit tests is properly reflected in the namespace.
43-46: LGTM: Well-structured JSON serialization configuration.The serialization options are properly configured for consistent formatting.
Digdir.Domain.Dialogporten.sln (2)
66-73: LGTM: Well-organized project structure for the new WebApiClient.The solution structure follows good practices:
- Client library isolated in dedicated solution folder
- Sample project included for documentation/testing
- Integration tests properly separated
164-175: LGTM: Project configurations are properly set up.All new projects are correctly configured with Debug/Release configurations and properly nested in their respective solution folders.
Also applies to: 205-208
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
.github/workflows/ci-cd-main.yml (1)
126-126: Remove trailing whitespace.There are trailing spaces on this line that should be removed.
- +🧰 Tools
🪛 yamllint (1.35.1)
[error] 126-126: trailing spaces
(trailing-spaces)
tests/Digdir.Domain.Dialogporten.WebApi.Unit.Tests/Features/V1/SwaggerSnapshotTests.cs (2)
23-24: Consider making file paths configurable.Hard-coded paths can be problematic for maintainability and cross-platform compatibility. Consider moving these paths to configuration:
+private const string RelativeSwaggerDocsPath = "docs/schema/V1"; +private const string RelativeSwaggerBuildPath = "src/Digdir.Domain.Dialogporten.WebApi/bin/Release/net9.0/swagger.json"; -var swaggerPath = Path.Combine(rootPath!, "docs/schema/V1"); -var newSwaggerPath = Path.Combine(rootPath!, "src/Digdir.Domain.Dialogporten.WebApi/bin/Release/net9.0/swagger.json"); +var swaggerPath = Path.Combine(rootPath!, RelativeSwaggerDocsPath); +var newSwaggerPath = Path.Combine(rootPath!, RelativeSwaggerBuildPath);
10-44: Appropriate architectural shift from integration to unit testing.The transition from integration tests to unit tests is well-executed. The conditional compilation effectively manages the different testing requirements between DEBUG and RELEASE modes, while maintaining the core functionality of Swagger documentation verification.
Consider documenting this architectural decision in the project's documentation to help other developers understand:
- Why the shift from integration to unit testing was made
- When to run tests in RELEASE mode
- The relationship between these tests and the npm package publication process
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci-cd-main.yml(2 hunks)tests/Digdir.Domain.Dialogporten.WebApi.Unit.Tests/Features/V1/SwaggerSnapshotTests.cs(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/ci-cd-main.yml
[error] 126-126: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Dry run deploy infrastructure / Deploy to test
- GitHub Check: build / build-and-test
🔇 Additional comments (4)
.github/workflows/ci-cd-main.yml (2)
45-50: LGTM! Secrets are well-structured and properly scoped.The new secrets for Maskinporten and ED25519 keys follow consistent naming conventions and are properly referenced.
125-125: Make secret name environment agnostic.Remove the environment suffix from the secret name to make it reusable across environments.
- NUGET_API_KEY: ${{ secrets.NUGET_API_TEST_KEY }} + NUGET_API_KEY: ${{ secrets.NUGET_API_KEY }}tests/Digdir.Domain.Dialogporten.WebApi.Unit.Tests/Features/V1/SwaggerSnapshotTests.cs (2)
1-8: LGTM! Appropriate compiler directive usage and namespace change.The compiler directive correctly suppresses CS1998 in DEBUG mode, and the namespace accurately reflects the transition from integration to unit testing.
47-50: LGTM! Well-structured JSON serialization configuration.The JsonSerializerOptions configuration with WriteIndented = true is appropriate for generating readable and consistent JSON output.
| if: ${{ needs.check-for-changes.outputs.hasBackendChanges == 'true' || needs.check-for-changes.outputs.hasSchemaChanges == 'true' }} | ||
| with: | ||
| version: ${{ needs.get-current-version.outputs.version }}-rc.${{ needs.generate-git-short-sha.outputs.gitShortSha }} | ||
| path: $(find . -name '*Digdir.Library.Dialogporten.WebApiClient.csproj' -printf "%p" -quit) |
There was a problem hiding this comment.
Replace shell command with predefined path.
Using a shell command in YAML value is problematic as it:
- May fail on Windows runners
- Makes the workflow less maintainable
Consider using a predefined path or workflow input parameter instead:
- path: $(find . -name '*Digdir.Library.Dialogporten.WebApiClient.csproj' -printf "%p" -quit)
+ path: 'src/Digdir.Library.Dialogporten.WebApiClient/Digdir.Library.Dialogporten.WebApiClient.csproj'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| path: $(find . -name '*Digdir.Library.Dialogporten.WebApiClient.csproj' -printf "%p" -quit) | |
| path: 'src/Digdir.Library.Dialogporten.WebApiClient/Digdir.Library.Dialogporten.WebApiClient.csproj' |
| var newSwaggerPath = Path.Combine(rootPath!, "src/Digdir.Domain.Dialogporten.WebApi/bin/Release/net9.0/swagger.json"); | ||
| // Act | ||
| var response = await client.GetAsync("/swagger/v1/swagger.json"); | ||
| var newSwagger = await response.Content.ReadAsStringAsync(); | ||
| var newSwagger = await File.ReadAllTextAsync(newSwaggerPath); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add file existence check and error handling.
The code assumes the existence of the Release build artifacts without validation. Consider adding appropriate checks and error handling:
var newSwaggerPath = Path.Combine(rootPath!, "src/Digdir.Domain.Dialogporten.WebApi/bin/Release/net9.0/swagger.json");
+if (!File.Exists(newSwaggerPath))
+{
+ throw new FileNotFoundException(
+ "Swagger file not found. Ensure you've built the project in Release mode.",
+ newSwaggerPath);
+}
var newSwagger = await File.ReadAllTextAsync(newSwaggerPath);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var newSwaggerPath = Path.Combine(rootPath!, "src/Digdir.Domain.Dialogporten.WebApi/bin/Release/net9.0/swagger.json"); | |
| // Act | |
| var response = await client.GetAsync("/swagger/v1/swagger.json"); | |
| var newSwagger = await response.Content.ReadAsStringAsync(); | |
| var newSwagger = await File.ReadAllTextAsync(newSwaggerPath); | |
| var newSwaggerPath = Path.Combine(rootPath!, "src/Digdir.Domain.Dialogporten.WebApi/bin/Release/net9.0/swagger.json"); | |
| if (!File.Exists(newSwaggerPath)) | |
| { | |
| throw new FileNotFoundException( | |
| "Swagger file not found. Ensure you've built the project in Release mode.", | |
| newSwaggerPath); | |
| } | |
| // Act | |
| var newSwagger = await File.ReadAllTextAsync(newSwaggerPath); |
| <PackageReference Include="OpenTelemetry.Instrumentation.Runtime" Version="1.10.0" /> | ||
| <PackageReference Include="Npgsql.OpenTelemetry" Version="9.0.2" /> | ||
| <PackageReference Include="ZiggyCreatures.FusionCache.OpenTelemetry" Version="1.4.1" /> | ||
| <PackageReference Include="Microsoft.AspNetCore.Authentication.JwtBearer" Version="9.0.1"/> |
There was a problem hiding this comment.
Hva har skjedd her? Det er litt vanskelig å se hvilke endringer denne fila faktisk inneholder når space på slutten av hver package reference er blitt fjernet og alt er å regne som en endring. Kan du gjøre slik at change settet kun inneholder det som faktisk er endret?
| Modifiers = | ||
| { | ||
| IgnoreEmptyCollections | ||
| } |
There was a problem hiding this comment.
Er dette autoformatering?
| .AddNewtonsoftJson() | ||
| .Services | ||
| .AddNewtonsoftJson() | ||
| .Services |
There was a problem hiding this comment.
Flisespikk: Ta tilbake indenteringen
| .WithPubCapabilities() | ||
| .Build() | ||
| .WithPubCapabilities() | ||
| .Build() |
There was a problem hiding this comment.
Flisespikk: Ta tilbake indenteringen
| @@ -66,7 +66,7 @@ static void BuildAndRun(string[] args) | |||
| .ReadFrom.Configuration(context.Configuration) | |||
| .ReadFrom.Services(services) | |||
| .Enrich.WithEnvironmentName() | |||
| .Enrich.FromLogContext() | |||
| .Enrich.FromLogContext() | |||
There was a problem hiding this comment.
Flisespikk: Ta tilbake indenteringen
| "Maskinporten": { | ||
| "ClientId": "ce3b732a-d4f2-4997-8545-adf8df70fe6c", | ||
| "Scope": "digdir:dialogporten.serviceprovider digdir:dialogporten.serviceprovider.search", | ||
| "EncodedJwk": "eyJwIjoieTRBZEhlVVBxdFEtSFlOWkR5ci0zS09RT3NQajA5TFh2a2hIUFlTdGFYNThkMndIWUJiVXlDTWdMYWtGTHo4UExKNWtscURsanRoczFtM1dFVGJhSWVuY25TalpjZTh4S1Q2SHh3bTNyaDlydWZ1TWVOZDRqaFptTm9WZmJrcGNXcVh0UDFvb1NPTE5zYUNVUWFUUEVKTXlFd3VhdWxMSzgxRG1SSTlMSmVNIiwia3R5IjoiUlNBIiwicSI6InFmOEQ2Uy1Kd19BdVQ0Q2hjQTlDek9WNk1uTW9mc1VCdTkteHJBcVFDRjh4WWZZOTRxQ1ZjQ3llajlkTlN3eXZUZXg1dThIMzNSaU1LMEFWM2tTQlpJLVZqcXJHLUx6YzNfTUlTTVpSVDJfbzNVQlRWVHpqTkUtSkpMX1hKaXJ6ZVhhQjM1UmFZMjFnWVhKQWg3X2tuR3dpRzF3MGxiT2ozQ0FzdnVwaU1BMCIsImQiOiJLVkF1b1Zhd2paTTgwenRYcUxSZUJGZkJ3M3pxVjdkUGFpaWFONWU0RFp6bW5MYTFMNEZJMTgtanVraHN4UVdqR1NFQnBIdTFrOHRPUWMyWjBsSDVaaTBydERqM0JKeEhxeDNsUGdYMWdTNXNiX1EyeXNfb2FKcklSX012MHBDQUFHX3hpa2lUY2kzTHMyeV9femV4QTdLbG0yalNmYW9Udzdhbml1R3RId1d5dHhCSnJnZ0J2c3loaHZIaUVQcnZaMHZBZldYYWI3QUtkUjc1cEtEaVVHOGdGNTdJN0hrWnpJSk9QYXp3MTU1Skx4TG9HcDVzeTFCVVpBNHRiQmlseWVsdG9ONGZINWd1aUktOXJjTE5zUmVYazJ1c3NFbE9EbVZ2Qmx2ZVVfb1ZRMVYtVDRJRnUzZk1BYVJGUFA2Wlo1akJJX2hkOFJOTTJ3eUp5UHVRWVEiLCJlIjoiQVFBQiIsInVzZSI6InNpZyIsImtpZCI6ImRpYWxvZ3BvcnRlbi1zcC1zZGstdGVzdC0yMDI0MTAxMCIsInFpIjoiQm9VS0RlczQ0UTNpXzNyT3Q4aHRrS2NxWkFNem00Njl2cTZuQnJVcHBTU1Ric3YwalZwN1daRGRRR0Q0bU8yMVJVOEFUbmN3NjFPOUt3YXktOGloX082VWFWbGxZN3NHYlVrQ2NVaG43ZDkzSElLZnhybnhWVE9nNUNMWTBka2Zwa3A1V2pyU1VvMTVKQURsY3BRM0ItRlU0Nm9PTG9ydjJ0SVFQekE4OF93IiwiZHAiOiJ1emVaRWZpN2Fqa3JFREhYekZtTThXWFUtZ3RmM1ctN0pnY082MnpWc1JrNTN4QlcxTE1NZlRlN2tlWk9xOEhDN3hTbGktSm9idnR6WGU3Y295ZW9sTXkzTnlydXFhQVp4VTBPMHpHQWQ4UFdjdHNXeDlITHlrU1hNby1QVlVNNkpmZERCaWFtcXk5bGQ0WTRfdzlscEdVWEMyaUFwLXdsWktaSHdrbG1KR3MiLCJhbGciOiJSUzI1NiIsImRxIjoiVENBcV9DMlJuX0RhakRlcUU2aUIzWWVWNVNtMHBMQk1Tbm10OHNENEp3ZVo4YWgzcGhrTFVxUm9qVGw1SDNhYXVtWl9UUmxiaWVNSVFnWDh4UUFnZ1l2YkNYeG9oZEx0aGt3ckZZdlp0WjBEeHJDYm9Md1hjc0Y3Ukwyejl4LWMwSFBGVFAzLVREQWF6UWlBNVVtRmNwYnAzeDYzWGFLSWFuYnVFc0NiSDdFIiwibiI6Imh5Sks4WnE2Wk8tRjFSSklVWVNCdUpfeG9RWkNNV1EyTVhrSFQ1bVROVVJJZmVWWWpCNWMwMzI0Uk5nc3ZPMEtXX0hUejRRSnptLV9rU1VaZ0h1Z2JoR0F3a1Vqc1lwTlJJRTBvLVNtdEExMlMxZHVCZWx6ajg2LVFrZkFzeFlwblNnSzl5OXpTS1B0YVlzMS1EcEVIb0hVdk9BSDJlNktFTXRaYUZPM0J0Yk9WUURXMENMYi1FY0UyaDBQRlFMMUp3NU8zeDhHcXBZeUFhamNoWnptcWlFbjBaSEd1QTNZZ1NyNGxQV1lkTzBNWHZmRFdyaFBTcnVTS3FodzBHMTlBRUpHOFhoek9xTWxLTUFIbW5ybk9XOHM2cWR2Sy1UQ1BiVGJJOU5XUWdFd2JpUFBBdlU0MUFITzZmTEYxUHZzQ3FhNjZTSGdYMkJzS3pvNVhORjhodyJ9" |
There was a problem hiding this comment.
Dette er en hemmelighet som ikke burde sjekkes inn. Nå som det først er gjort burde denne klientens JWK bli rotert slik at den som står her nå blir invalid.
|
|
||
| <ItemGroup> | ||
| <Folder Include="Features\V1\" /> | ||
| </ItemGroup> |
Description
Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)