feat: Create Dialogporten Serviceowner client library #1513
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several modifications to the CI/CD workflows and adds new functionality for publishing NuGet packages. Key changes include the addition of a Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
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
src/Digdir.Library.Dialogporten.WebApiClient/Config/MaskinportenSettings.cs
Outdated
Show resolved
Hide resolved
…enSettings.cs Co-authored-by: Ole Jørgen Skogstad <skogstad@softis.net>
|
Er avhengig av funksjonalitet i refitter som ikke er i preview enda. Bygget refitter fra source og lagret i et lokalt nugetStore. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
🧹 Outside diff range and nitpick comments (21)
src/Digdir.Library.Dialogporten.WebApiClient/Digdir.Library.Dialogporten.WebApiClient.csproj (2)
7-7: Update the package release notes URLThe current value "Link to release notes." appears to be a placeholder. Please update this with the actual URL to the release notes.
3-13: Consider enabling XML documentationSince this is a client library, it would be beneficial to enable XML documentation to provide better IntelliSense support for consumers. Consider adding these properties:
<PropertyGroup> <ImplicitUsings>enable</ImplicitUsings> <Nullable>enable</Nullable> + <GenerateDocumentationFile>true</GenerateDocumentationFile> + <NoWarn>$(NoWarn);CS1591</NoWarn>src/Digdir.Library.Dialogporten.WebApiClient/Interfaces/IDialogportenSettings.cs (3)
1-4: Add interface-level documentation.Consider adding XML documentation for the interface itself to explain its purpose, usage, and any important implementation notes.
namespace Digdir.Library.Dialogporten.WebApiClient.Interfaces; +/// <summary> +/// Defines the configuration settings required for the Dialogporten API client. +/// </summary> public interface IDialogportenSettings
10-13: Enhance Scope property documentation.The current documentation could be more helpful by including:
- Expected format for multiple scopes
- Examples of valid scopes
- Link to scope documentation
/// <summary> -/// Scopes to request. Must be provisioned on the supplied client. +/// Space-separated list of scopes to request. Must be provisioned on the supplied client. /// </summary> +/// <remarks> +/// Example: "scope1 scope2 scope3" +/// See https://docs.digdir.no/docs/Maskinporten/maskinporten_protocol_scope for more information about scopes. +/// </remarks> string Scope { get; set; }
3-29: Consider adding validation support.The interface could benefit from validation support to ensure all required properties are properly configured before use. Consider:
- Adding an
IValidatableinterface- Including validation methods
- Providing a builder pattern implementation
Example implementation:
public interface IValidatable { bool IsValid(); IEnumerable<string> GetValidationErrors(); } public interface IDialogportenSettings : IValidatable { // ... existing properties ... }src/Digdir.Library.Dialogporten.WebApiClient/Services/DialogTokenVerifier.cs (1)
39-39: Add error handling for token body deserializationDeserializing the token body may throw a
JsonExceptionif the token is malformed or the content is invalid. To prevent the application from crashing, add error handling around the deserialization.Modify the code as follows:
-var bodyJson = JsonSerializer.Deserialize<JsonElement>(Base64Url.DecodeFromChars(parts[1])); +JsonElement bodyJson; +try +{ + bodyJson = JsonSerializer.Deserialize<JsonElement>(Base64Url.DecodeFromChars(parts[1])); +} +catch (JsonException) +{ + throw new ArgumentException("Invalid dialog token"); +}src/Digdir.Library.Dialogporten.WebApiClient/Extensions/ServiceCollectionExtensions.cs (2)
25-25: Avoid usingConsole.WriteLinein library codeUsing
Console.WriteLinewithin a library can produce unwanted console output for consuming applications. It's better to remove it or use a logging framework that can be controlled by the application.Remove the unnecessary console output:
-Console.WriteLine(dialogportenSettings);
62-68: Improve exception handling for unknown environmentsThrowing a
NotImplementedExceptionfor unknown environments may not accurately represent the issue. Consider throwing anArgumentExceptionwith a descriptive message.Update the switch expression as follows:
- _ => throw new NotImplementedException() + _ => throw new ArgumentException($"Unknown environment: {dialogportenSettings.Environment}")src/Digdir.Library.Dialogporten.WebApiClient.Sample/Program.cs (1)
96-96: Check for errors before accessing error contentWhen accessing
updateResponse.Error?.Content, ensure that an error has actually occurred. It's good practice to check if the response indicates a failure before attempting to read the error content.Consider adding a conditional check:
if (!updateResponse.IsSuccessStatusCode && updateResponse.Error != null) { Console.WriteLine(updateResponse.Error.Content); }src/Digdir.Library.Dialogporten.WebApiClient/Interfaces/IIdentifiable.cs (1)
3-7: Add XML documentation for better API discoverability.Consider adding XML documentation to describe the purpose of this interface and its properties, especially since this is part of a client library that will be consumed by other developers.
+/// <summary> +/// Represents an entity that can be uniquely identified. +/// </summary> public interface IIdentifiable { + /// <summary> + /// Gets the unique identifier of the entity. + /// </summary> Guid Id { get; } + /// <summary> + /// Gets the revision identifier used for optimistic concurrency control. + /// </summary> Guid RevisionId { get; } }src/Digdir.Library.Dialogporten.WebApiClient.Sample/Dialogs.cs (2)
16-23: Refactor PrintGetDialog for better separation of concerns.The method should return a formatted string instead of writing directly to console, allowing for more flexible usage.
- public static void PrintGetDialog(V1ServiceOwnerDialogsQueriesGet_Dialog dialog) + public static string FormatDialog(V1ServiceOwnerDialogsQueriesGet_Dialog dialog) { - Console.WriteLine($"System Label: {dialog.SystemLabel}"); - Console.WriteLine($"Dialog Status: {dialog.Status}"); - Console.WriteLine($"Dialog Org: {dialog.Org}"); - Console.WriteLine($"Dialog Progress: {dialog.Progress}"); - Console.WriteLine($"Deleted at: {dialog.DeletedAt}"); + return string.Join(Environment.NewLine, + $"System Label: {dialog.SystemLabel}", + $"Dialog Status: {dialog.Status}", + $"Dialog Org: {dialog.Org}", + $"Dialog Progress: {dialog.Progress}", + $"Deleted at: {dialog.DeletedAt}"); }
6-6: Consider adding XML documentation for the Dialogs class.Since this is a sample implementation, documentation would help users understand its purpose and usage.
+/// <summary> +/// Sample implementation demonstrating usage of the Dialogporten Service Owner API. +/// </summary> public sealed class Dialogs(IServiceownerApi client)tests/Digdir.Library.Dialogporten.WebApiClient.Integration.Tests/RefitterInterfaceTests.cs (1)
26-35: Improve solution root folder detectionThe current implementation silently returns null if no solution file is found. Consider throwing an exception with a meaningful message.
private static string? GetSolutionRootFolder() { var currentDirectory = Directory.GetCurrentDirectory(); var solutionFolder = currentDirectory; while (solutionFolder != null && Directory.GetFiles(solutionFolder, "*.sln").Length == 0) { solutionFolder = Directory.GetParent(solutionFolder)?.FullName; } - return solutionFolder; + return solutionFolder ?? throw new InvalidOperationException("Could not find solution root folder. Ensure the test is running from within the solution directory."); }.github/workflows/workflow-publish-nuget.yml (1)
6-9: Add version format validationConsider adding a pattern check for the version input to ensure it follows semantic versioning.
version: description: "Version" required: true type: string + pattern: '^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-((?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\+([0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?$'tests/Digdir.Library.Dialogporten.WebApiClient.Integration.Tests/Digdir.Library.Dialogporten.WebApiClient.Integration.Tests.csproj (1)
11-16: Consider package version alignmentMultiple Microsoft.Extensions packages are used. Consider using a Directory.Packages.props file to manage versions centrally.
This would help maintain consistency across the solution and make updates easier.
src/Digdir.Library.Dialogporten.WebApiClient/README.md (2)
4-4: Format URL as a proper Markdown linkThe URL should be properly formatted as a Markdown link for better readability and maintainability.
-Refit-based client SDK Based on https://github.com/altinn/altinn-apiclient-maskinporten +Refit-based client SDK Based on [Altinn API Client Maskinporten](https://github.com/altinn/altinn-apiclient-maskinporten)🧰 Tools
🪛 Markdownlint (0.37.0)
4-4: null
Bare URL used(MD034, no-bare-urls)
1-205: Enhance documentation with additional sectionsThe README provides good setup and usage examples but could benefit from additional sections:
- Error handling and exceptions
- Rate limiting and throttling
- Authentication and authorization details
- API versioning
- Troubleshooting guide
Would you like me to help generate content for these sections?
🧰 Tools
🪛 Markdownlint (0.37.0)
4-4: null
Bare URL used(MD034, no-bare-urls)
.github/workflows/ci-cd-staging.yml (1)
102-102: Improve project path handlingUsing
findcommand directly in the path parameter could be fragile. Consider moving this logic to the reusable workflow or using a predefined path.- path: $(find . -name '*Digdir.Library.Dialogporten.WebApiClient.csproj' -printf "%p" -quit) + path: 'src/Digdir.Library.Dialogporten.WebApiClient/Digdir.Library.Dialogporten.WebApiClient.csproj'.github/workflows/ci-cd-pull-request.yml (3)
6-8: Consider keeping CHANGELOG.md in paths-ignoreThe removal of CHANGELOG.md from paths-ignore means the workflow will run even when only the changelog is modified. This could lead to unnecessary workflow runs since changelog updates typically don't require CI/CD validation.
paths-ignore: - "tests/k6/**" + - "CHANGELOG.md"
21-21: Remove trailing spacesThere are trailing spaces on these lines that should be removed for consistency.
Also applies to: 26-26, 37-37
🧰 Tools
🪛 yamllint (1.35.1)
[error] 21-21: trailing spaces
(trailing-spaces)
Job dependencies need to be updated for complete workflow integrity
The verification revealed some inconsistencies in the job dependencies:
dry-run-deploy-infradepends onbuild-infrastructure, butbuild-infrastructureonly depends oncheck-for-changesdry-run-deploy-appsdepends ondry-run-deploy-infra, but it should also includebuild-infrastructurein its dependency chain- Both deployment jobs use outputs from
generate-git-short-shaandget-current-version, but these jobs aren't properly included in the dependency chain ofbuild-infrastructureTo ensure workflow integrity:
- Add
generate-git-short-shaandget-current-versiontobuild-infrastructurejob's needs array- Update
dry-run-deploy-appsto includebuild-infrastructurein its needs array when infrastructure changes are detected🔗 Analysis chain
Line range hint
38-98: LGTM! Well-structured job dependencies and conditionsThe job dependency chain and conditional execution logic is well thought out:
- Infrastructure deployment is properly gated by change detection
- App deployment handles both infrastructure-dependent and independent scenarios
- Failure conditions are properly handled with
always()and!failure()Let's verify the job dependencies are complete:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all referenced jobs exist and are properly connected # Check for any jobs referenced in needs that aren't defined rg -o 'needs:.*\[(.*)\]' | tr ',' '\n' | tr -d '[]' | tr -d ' ' > needed_jobs.txt rg '^[[:space:]]*\w+:' | cut -d':' -f1 | tr -d ' ' > defined_jobs.txt comm -23 needed_jobs.txt defined_jobs.txt # Check for jobs that might be missing from delete-github-deployments needs rg -A 5 'dry-run-deploy' | grep 'name:'Length of output: 291
Script:
#!/bin/bash # Let's try a different approach to verify job dependencies # First, let's get all defined jobs rg '^\s+\w+:' .github/workflows/ci-cd-pull-request.yml # Then, let's get all job dependencies rg 'needs:.*\[.*\]' .github/workflows/ci-cd-pull-request.yml # And finally, let's check the specific jobs mentioned in the needs section rg '^\s+(generate-git-short-sha|check-for-changes|get-current-version|build-infrastructure|dry-run-deploy-infra|dry-run-deploy-apps):' .github/workflows/ci-cd-pull-request.ymlLength of output: 4045
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
.github/workflows/ci-cd-main.yml(6 hunks).github/workflows/ci-cd-pull-request.yml(3 hunks).github/workflows/ci-cd-staging.yml(10 hunks).github/workflows/workflow-publish-nuget.yml(1 hunks).refitter(1 hunks)Digdir.Domain.Dialogporten.sln(3 hunks)docs/schema/V1/swagger.verified.json(1 hunks)src/Digdir.Library.Dialogporten.WebApiClient.Sample/Dialogs.cs(1 hunks)src/Digdir.Library.Dialogporten.WebApiClient.Sample/Digdir.Library.Dialogporten.WebApiClient.Sample.csproj(1 hunks)src/Digdir.Library.Dialogporten.WebApiClient.Sample/Program.cs(1 hunks)src/Digdir.Library.Dialogporten.WebApiClient.Sample/appsettings.json(1 hunks)src/Digdir.Library.Dialogporten.WebApiClient/Config/DialogportenSettings.cs(1 hunks)src/Digdir.Library.Dialogporten.WebApiClient/Config/MaskinportenSettings.cs(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/Interfaces/IDialogportenSettings.cs(1 hunks)src/Digdir.Library.Dialogporten.WebApiClient/Interfaces/IIdentifiable.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/.refitter(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/RefitterInterfaceTests.cs(1 hunks)tests/Digdir.Library.Dialogporten.WebApiClient.Integration.Tests/Tests.cs(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- .refitter
- src/Digdir.Library.Dialogporten.WebApiClient.Sample/Digdir.Library.Dialogporten.WebApiClient.Sample.csproj
- src/Digdir.Library.Dialogporten.WebApiClient/Config/MaskinportenSettings.cs
- tests/Digdir.Library.Dialogporten.WebApiClient.Integration.Tests/.refitter
🧰 Additional context used
📓 Learnings (1)
src/Digdir.Library.Dialogporten.WebApiClient.Sample/appsettings.json (2)
Learnt from: elsand
PR: digdir/dialogporten#1182
File: src/Digdir.Domain.Dialogporten.Janitor/appsettings.staging.json:4-4
Timestamp: 2024-11-12T05:32:45.311Z
Learning: In this project, "TODO: Add to local secrets" placeholders in configuration files are intentionally used to ensure that missing configurations fail fast and loud. These settings are overridden by Azure app configuration mechanisms, as documented in `docs/Configuration.md`.
Learnt from: MagnusSandgren
PR: digdir/dialogporten#1277
File: src/Digdir.Domain.Dialogporten.Service/appsettings.staging.json:9-23
Timestamp: 2024-11-12T05:32:45.311Z
Learning: In this project, the scopes defined in `appsettings.staging.json` (such as `altinn:events.publish`, `altinn:events.publish.admin`, `altinn:register/partylookup.admin`, `altinn:authorization/authorize.admin`, and `altinn:accessmanagement/authorizedparties.admin`) are the scopes required by the application from the Altinn platform. The scopes referenced in the code (like `digdir:dialogporten` and `digdir:dialogporten.serviceprovider`) are the scopes required from consumers of the application.
🪛 Markdownlint (0.37.0)
src/Digdir.Library.Dialogporten.WebApiClient/README.md
4-4: null
Bare URL used
(MD034, no-bare-urls)
🪛 Gitleaks (8.21.2)
src/Digdir.Library.Dialogporten.WebApiClient.Sample/Program.cs
33-33: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
🪛 actionlint (1.7.4)
.github/workflows/workflow-publish-nuget.yml
30-30: shellcheck reported issue in this script: SC2086:info:3:30: Double quote to prevent globbing and word splitting
(shellcheck)
41-41: shellcheck reported issue in this script: SC2086:info:1:71: Double quote to prevent globbing and word splitting
(shellcheck)
44-44: shellcheck reported issue in this script: SC2086:info:1:134: Double quote to prevent globbing and word splitting
(shellcheck)
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)
.github/workflows/ci-cd-staging.yml
97-97: secret "NUGET_API_KEY" is required by "./.github/workflows/workflow-publish-nuget.yml" reusable workflow
(workflow-call)
104-104: secret "NUGET_API_TEST_KEY" is not defined in "./.github/workflows/workflow-publish-nuget.yml" reusable workflow. defined secret is "NUGET_API_KEY"
(workflow-call)
🪛 yamllint (1.35.1)
.github/workflows/workflow-publish-nuget.yml
[error] 62-62: trailing spaces
(trailing-spaces)
.github/workflows/ci-cd-main.yml
[error] 120-120: trailing spaces
(trailing-spaces)
.github/workflows/ci-cd-staging.yml
[error] 105-105: trailing spaces
(trailing-spaces)
.github/workflows/ci-cd-pull-request.yml
[error] 21-21: trailing spaces
(trailing-spaces)
[error] 26-26: trailing spaces
(trailing-spaces)
[error] 37-37: trailing spaces
(trailing-spaces)
🔇 Additional comments (8)
src/Digdir.Library.Dialogporten.WebApiClient/Digdir.Library.Dialogporten.WebApiClient.csproj (2)
27-28: Verify the purpose of compilation exclusions
The exclusion of Program.cs and *.received.cs files suggests:
- This is a library project (hence no Program.cs needed)
- Approval tests might be in use (*.received.cs files)
Please confirm if these exclusions are intentional and if approval tests are being used for testing.
Let's check for approval test setup:
✅ Verification successful
Let me verify if approval tests are being used by checking for approval test packages and test files.
Let me check the specific project's test setup and verify if it's using approval tests.
Compilation exclusions are correctly configured
The exclusions are intentional and serve specific purposes:
Program.csexclusion is correct as this is a library project (WebApiClient)*.received.csexclusion is for approval testing, specifically used in the RefitterInterfaceTests which generates and verifies interface files using the Verify testing tool
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for approval test packages
rg -l "ApprovalTests"
# Check for actual test files
fd -e cs -e approved -e received . src/
Length of output: 50953
Script:
#!/bin/bash
# Search for approval test package references in csproj files
rg -l "ApprovalTests" --type csproj
# Search for received/approved files
fd -e received.cs -e approved.cs . src/
# Check for test files that might use approval tests
rg -l "Verify\(" --type cs
Length of output: 828
Script:
#!/bin/bash
# Check for test project associated with the WebApiClient
fd -e csproj . tests/ | grep -i "WebApiClient"
# Look for test files in the WebApiClient project
rg -l "Test" tests/.*WebApiClient.*
# Check content of the RefitterInterfaceTests.cs which seems to use Verify
cat tests/Digdir.Library.Dialogporten.WebApiClient.Integration.Tests/RefitterInterfaceTests.cs
Length of output: 1823
16-16: Verify security vulnerabilities in Maskinporten package
Based on previous comments, version 9.2.1 had known security vulnerabilities related to JWT packages. Version 9.2.0 might have the same issues.
Let's check the dependencies and vulnerabilities:
src/Digdir.Library.Dialogporten.WebApiClient/Services/DialogTokenVerifier.cs (1)
8-8:
Incorrect class constructor syntax
Defining a class with constructor parameters directly after the class name is not supported in C#. To properly initialize the class with parameters, you should define a constructor inside the class body.
Apply this diff to fix the syntax:
-public sealed class DialogTokenVerifier(string kid, PublicKey publicKey)
+public sealed class DialogTokenVerifier
+{
+ private readonly string _kid;
+ private readonly PublicKey _publicKey;
+
+ public DialogTokenVerifier(string kid, PublicKey publicKey)
+ {
+ _kid = kid;
+ _publicKey = publicKey;
+ }Remember to update references in the class to use _kid and _publicKey instead of kid and publicKey.
Likely invalid or redundant comment.
src/Digdir.Library.Dialogporten.WebApiClient/Extensions/ServiceCollectionExtensions.cs (1)
49-55: Verify environment mapping for Maskinporten settings
In the environment mapping for Maskinporten settings, "local" is mapped to "test". Ensure that this mapping aligns with your deployment environments and that it does not cause unintended behavior.
Please confirm that mapping "local" to "test" is correct. If "local" should have distinct settings, update the mapping accordingly.
src/Digdir.Library.Dialogporten.WebApiClient.Sample/Dialogs.cs (1)
1-24: Add unit tests for the Dialogs class.
According to the PR objectives, no tests have been added. Consider adding unit tests to verify the behavior of the Purge method and dialog formatting.
.github/workflows/ci-cd-main.yml (1)
111-120: LGTM! Consider making the environment variable name environment-agnostic.
The new publish-sdk-to-nuget job is well-structured with appropriate dependencies. However, the environment variable NUGET_API_TEST_KEY could be renamed to just NUGET_API_KEY to make it environment-agnostic, as suggested in a previous review comment.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 120-120: trailing spaces
(trailing-spaces)
Digdir.Domain.Dialogporten.sln (1)
70-77: LGTM! Well-structured solution organization.
The solution changes follow best practices:
- Clear separation of concerns with WebApiClient in Libraries folder
- Sample project included for documentation/examples
- Integration tests properly placed in tests folder
- Project naming follows established conventions
Also applies to: 176-187, 219-222
docs/schema/V1/swagger.verified.json (1)
Line range hint 1-6973: LGTM! Comprehensive and well-documented API specification.
The OpenAPI specification is thorough and follows best practices:
- Clear endpoint descriptions and examples
- Proper security scheme definitions
- Complete schema definitions with descriptions
- Consistent error response handling
| /// <summary> | ||
| /// ClientID to use | ||
| /// </summary> | ||
| string ClientId { get; set; } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making properties immutable.
Configuration properties should typically be immutable after initialization to prevent unexpected runtime changes. Consider removing the setters and initializing values through constructor injection.
- string ClientId { get; set; }
+ string ClientId { get; }
- string Scope { get; set; }
+ string Scope { get; }
- string Resource { get; set; }
+ string Resource { get; }
- string Environment { get; set; }
+ string Environment { get; }
- string EncodedJwk { get; set; }
+ string EncodedJwk { get; }Also applies to: 13-13, 18-18, 23-23, 28-28
| /// <summary> | ||
| /// Base64 Encoded Json Web Key | ||
| /// </summary> | ||
| string EncodedJwk { get; set; } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mark sensitive data and improve security documentation.
The EncodedJwk property contains sensitive cryptographic material. Consider:
- Adding security-related XML comments
- Using a secure string type or marking it as sensitive
- Adding validation requirements in the documentation
/// <summary>
-/// Base64 Encoded Json Web Key
+/// Base64 Encoded Json Web Key. This is sensitive cryptographic material that should be stored securely.
/// </summary>
+/// <remarks>
+/// Security considerations:
+/// - Store this value in a secure configuration store or key vault
+/// - Ensure the value is properly Base64 encoded
+/// - Protect this value in memory and logs
+/// </remarks>
string EncodedJwk { get; set; }📝 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.
| /// <summary> | |
| /// Base64 Encoded Json Web Key | |
| /// </summary> | |
| string EncodedJwk { get; set; } | |
| /// <summary> | |
| /// Base64 Encoded Json Web Key. This is sensitive cryptographic material that should be stored securely. | |
| /// </summary> | |
| /// <remarks> | |
| /// Security considerations: | |
| /// - Store this value in a secure configuration store or key vault | |
| /// - Ensure the value is properly Base64 encoded | |
| /// - Protect this value in memory and logs | |
| /// </remarks> | |
| string EncodedJwk { get; set; } |
| /// <summary> | ||
| /// The Maskinporten environment. Valid values are ver1, ver2, test or prod | ||
| /// </summary> | ||
| string Environment { get; set; } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use enum for Environment property.
Instead of using a string for environment values, consider using an enum to enforce valid values at compile-time.
+/// <summary>
+/// Represents the available Maskinporten environments.
+/// </summary>
+public enum MaskinportenEnvironment
+{
+ /// <summary>
+ /// Version 1 environment
+ /// </summary>
+ Ver1,
+
+ /// <summary>
+ /// Version 2 environment
+ /// </summary>
+ Ver2,
+
+ /// <summary>
+ /// Test environment
+ /// </summary>
+ Test,
+
+ /// <summary>
+ /// Production environment
+ /// </summary>
+ Prod
+}
/// <summary>
/// The Maskinporten environment. Valid values are ver1, ver2, test or prod
/// </summary>
-string Environment { get; set; }
+MaskinportenEnvironment Environment { get; }📝 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.
| /// <summary> | |
| /// The Maskinporten environment. Valid values are ver1, ver2, test or prod | |
| /// </summary> | |
| string Environment { get; set; } | |
| /// <summary> | |
| /// Represents the available Maskinporten environments. | |
| /// </summary> | |
| public enum MaskinportenEnvironment | |
| { | |
| /// <summary> | |
| /// Version 1 environment | |
| /// </summary> | |
| Ver1, | |
| /// <summary> | |
| /// Version 2 environment | |
| /// </summary> | |
| Ver2, | |
| /// <summary> | |
| /// Test environment | |
| /// </summary> | |
| Test, | |
| /// <summary> | |
| /// Production environment | |
| /// </summary> | |
| Prod | |
| } | |
| /// <summary> | |
| /// The Maskinporten environment. Valid values are ver1, ver2, test or prod | |
| /// </summary> | |
| MaskinportenEnvironment Environment { get; } |
| var fieldsInfo = typeof(DialogTokenClaimTypes).GetFields().Where(f => f.FieldType == typeof(string)); | ||
| foreach (var fieldInfo in fieldsInfo) | ||
| { | ||
| var value = fieldInfo.GetValue("string"); |
There was a problem hiding this comment.
Incorrect usage of GetValue method in reflection
When retrieving the value of a static field using reflection, you should pass null to GetValue since there is no instance associated with static members. Passing "string" is incorrect and will lead to unexpected behavior.
Apply this diff to correct the method call:
-var value = fieldInfo.GetValue("string");
+var value = fieldInfo.GetValue(null);📝 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 value = fieldInfo.GetValue("string"); | |
| var value = fieldInfo.GetValue(null); |
| return false; | ||
| } | ||
| var signature = Base64Url.DecodeFromChars(parts[2]); | ||
| return SignatureAlgorithm.Ed25519.Verify(publicKey, Encoding.UTF8.GetBytes(parts[0] + '.' + parts[1]), signature); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential exceptions during signature verification
The SignatureAlgorithm.Ed25519.Verify method may throw exceptions if the inputs are invalid or if an error occurs during verification. To ensure the Verify method in your class returns false instead of allowing an exception to propagate, consider wrapping the call in a try-catch block.
Modify the method as follows:
-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;
+}Committable suggestion skipped: line range outside the PR's diff.
| run: dotnet build --configuration Release /p:Version=${{ inputs.version }} ${PROJECT} | ||
|
|
||
| - name: Pack with debug symbols | ||
| run: dotnet pack --configuration Release /p:Version=${{ inputs.version }} -p:IncludeSymbols=true -p:SymbolPackageFormat=snupkg --output . ${PROJECT} |
There was a problem hiding this comment.
Fix command argument quoting
The project path needs proper quoting in dotnet commands.
- run: dotnet build --configuration Release /p:Version=${{ inputs.version }} ${PROJECT}
+ run: dotnet build --configuration Release /p:Version="${{ inputs.version }}" "${PROJECT}"
- run: dotnet pack --configuration Release /p:Version=${{ inputs.version }} -p:IncludeSymbols=true -p:SymbolPackageFormat=snupkg --output . ${PROJECT}
+ run: dotnet pack --configuration Release /p:Version="${{ inputs.version }}" -p:IncludeSymbols=true -p:SymbolPackageFormat=snupkg --output . "${PROJECT}"📝 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.
| run: dotnet build --configuration Release /p:Version=${{ inputs.version }} ${PROJECT} | |
| - name: Pack with debug symbols | |
| run: dotnet pack --configuration Release /p:Version=${{ inputs.version }} -p:IncludeSymbols=true -p:SymbolPackageFormat=snupkg --output . ${PROJECT} | |
| run: dotnet build --configuration Release /p:Version="${{ inputs.version }}" "${PROJECT}" | |
| - name: Pack with debug symbols | |
| run: dotnet pack --configuration Release /p:Version="${{ inputs.version }}" -p:IncludeSymbols=true -p:SymbolPackageFormat=snupkg --output . "${PROJECT}" |
🧰 Tools
🪛 actionlint (1.7.4)
41-41: shellcheck reported issue in this script: SC2086:info:1:71: Double quote to prevent globbing and word splitting
(shellcheck)
44-44: shellcheck reported issue in this script: SC2086:info:1:134: Double quote to prevent globbing and word splitting
(shellcheck)
| run: | | ||
| PROJECT=${{inputs.path}} | ||
| echo "PROJECT ${PROJECT}" | ||
| echo "PROJECT=${PROJECT}" >> $GITHUB_ENV | ||
|
|
There was a problem hiding this comment.
Fix shell script quoting
The PROJECT variable assignment needs proper quoting to handle paths with spaces.
- PROJECT=${{inputs.path}}
+ PROJECT="${{inputs.path}}"
echo "PROJECT ${PROJECT}"
- echo "PROJECT=${PROJECT}" >> $GITHUB_ENV
+ echo "PROJECT=${PROJECT}" >> "$GITHUB_ENV"📝 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.
| run: | | |
| PROJECT=${{inputs.path}} | |
| echo "PROJECT ${PROJECT}" | |
| echo "PROJECT=${PROJECT}" >> $GITHUB_ENV | |
| run: | | |
| PROJECT="${{inputs.path}}" | |
| echo "PROJECT ${PROJECT}" | |
| echo "PROJECT=${PROJECT}" >> "$GITHUB_ENV" |
🧰 Tools
🪛 actionlint (1.7.4)
30-30: shellcheck reported issue in this script: SC2086:info:3:30: Double quote to prevent globbing and word splitting
(shellcheck)
| <!-- New funksjonaliet som ikke har kommet in til preview engang, Compilert lokalt og Hentet fra lokalt Nugetstore --> | ||
| <PackageReference Include="Refitter.SourceGenerator" Version="1.1.0"> | ||
| <PrivateAssets>all</PrivateAssets> | ||
| <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets> | ||
| </PackageReference> | ||
| <!-- <PackageReference Include="Refitter.SourceGenerator" Version="1.4.1">--> | ||
| <!-- <PrivateAssets>all</PrivateAssets>--> | ||
| <!-- <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>--> | ||
| <!-- </PackageReference>--> |
There was a problem hiding this comment.
Address Refitter dependency management
The current setup uses a locally built version of Refitter, which could cause build issues for other developers. Consider:
- Publishing the required Refitter version to a shared feed
- Adding documentation for local build setup
- Removing commented out code
Would you like me to help create a setup guide for building and using Refitter locally?
| "ClientId": "ce3b732a-d4f2-4997-8545-adf8df70fe6c", | ||
| "Scope": "digdir:dialogporten.serviceprovider digdir:dialogporten.serviceprovider.search", | ||
| "EncodedJwk": "eyJwIjoieTRBZEhlVVBxdFEtSFlOWkR5ci0zS09RT3NQajA5TFh2a2hIUFlTdGFYNThkMndIWUJiVXlDTWdMYWtGTHo4UExKNWtscURsanRoczFtM1dFVGJhSWVuY25TalpjZTh4S1Q2SHh3bTNyaDlydWZ1TWVOZDRqaFptTm9WZmJrcGNXcVh0UDFvb1NPTE5zYUNVUWFUUEVKTXlFd3VhdWxMSzgxRG1SSTlMSmVNIiwia3R5IjoiUlNBIiwicSI6InFmOEQ2Uy1Kd19BdVQ0Q2hjQTlDek9WNk1uTW9mc1VCdTkteHJBcVFDRjh4WWZZOTRxQ1ZjQ3llajlkTlN3eXZUZXg1dThIMzNSaU1LMEFWM2tTQlpJLVZqcXJHLUx6YzNfTUlTTVpSVDJfbzNVQlRWVHpqTkUtSkpMX1hKaXJ6ZVhhQjM1UmFZMjFnWVhKQWg3X2tuR3dpRzF3MGxiT2ozQ0FzdnVwaU1BMCIsImQiOiJLVkF1b1Zhd2paTTgwenRYcUxSZUJGZkJ3M3pxVjdkUGFpaWFONWU0RFp6bW5MYTFMNEZJMTgtanVraHN4UVdqR1NFQnBIdTFrOHRPUWMyWjBsSDVaaTBydERqM0JKeEhxeDNsUGdYMWdTNXNiX1EyeXNfb2FKcklSX012MHBDQUFHX3hpa2lUY2kzTHMyeV9femV4QTdLbG0yalNmYW9Udzdhbml1R3RId1d5dHhCSnJnZ0J2c3loaHZIaUVQcnZaMHZBZldYYWI3QUtkUjc1cEtEaVVHOGdGNTdJN0hrWnpJSk9QYXp3MTU1Skx4TG9HcDVzeTFCVVpBNHRiQmlseWVsdG9ONGZINWd1aUktOXJjTE5zUmVYazJ1c3NFbE9EbVZ2Qmx2ZVVfb1ZRMVYtVDRJRnUzZk1BYVJGUFA2Wlo1akJJX2hkOFJOTTJ3eUp5UHVRWVEiLCJlIjoiQVFBQiIsInVzZSI6InNpZyIsImtpZCI6ImRpYWxvZ3BvcnRlbi1zcC1zZGstdGVzdC0yMDI0MTAxMCIsInFpIjoiQm9VS0RlczQ0UTNpXzNyT3Q4aHRrS2NxWkFNem00Njl2cTZuQnJVcHBTU1Ric3YwalZwN1daRGRRR0Q0bU8yMVJVOEFUbmN3NjFPOUt3YXktOGloX082VWFWbGxZN3NHYlVrQ2NVaG43ZDkzSElLZnhybnhWVE9nNUNMWTBka2Zwa3A1V2pyU1VvMTVKQURsY3BRM0ItRlU0Nm9PTG9ydjJ0SVFQekE4OF93IiwiZHAiOiJ1emVaRWZpN2Fqa3JFREhYekZtTThXWFUtZ3RmM1ctN0pnY082MnpWc1JrNTN4QlcxTE1NZlRlN2tlWk9xOEhDN3hTbGktSm9idnR6WGU3Y295ZW9sTXkzTnlydXFhQVp4VTBPMHpHQWQ4UFdjdHNXeDlITHlrU1hNby1QVlVNNkpmZERCaWFtcXk5bGQ0WTRfdzlscEdVWEMyaUFwLXdsWktaSHdrbG1KR3MiLCJhbGciOiJSUzI1NiIsImRxIjoiVENBcV9DMlJuX0RhakRlcUU2aUIzWWVWNVNtMHBMQk1Tbm10OHNENEp3ZVo4YWgzcGhrTFVxUm9qVGw1SDNhYXVtWl9UUmxiaWVNSVFnWDh4UUFnZ1l2YkNYeG9oZEx0aGt3ckZZdlp0WjBEeHJDYm9Md1hjc0Y3Ukwyejl4LWMwSFBGVFAzLVREQWF6UWlBNVVtRmNwYnAzeDYzWGFLSWFuYnVFc0NiSDdFIiwibiI6Imh5Sks4WnE2Wk8tRjFSSklVWVNCdUpfeG9RWkNNV1EyTVhrSFQ1bVROVVJJZmVWWWpCNWMwMzI0Uk5nc3ZPMEtXX0hUejRRSnptLV9rU1VaZ0h1Z2JoR0F3a1Vqc1lwTlJJRTBvLVNtdEExMlMxZHVCZWx6ajg2LVFrZkFzeFlwblNnSzl5OXpTS1B0YVlzMS1EcEVIb0hVdk9BSDJlNktFTXRaYUZPM0J0Yk9WUURXMENMYi1FY0UyaDBQRlFMMUp3NU8zeDhHcXBZeUFhamNoWnptcWlFbjBaSEd1QTNZZ1NyNGxQV1lkTzBNWHZmRFdyaFBTcnVTS3FodzBHMTlBRUpHOFhoek9xTWxLTUFIbW5ybk9XOHM2cWR2Sy1UQ1BiVGJJOU5XUWdFd2JpUFBBdlU0MUFITzZmTEYxUHZzQ3FhNjZTSGdYMkJzS3pvNVhORjhodyJ9" |
There was a problem hiding this comment.
Critical: Remove sensitive information from sample configuration
The configuration file contains actual sensitive values including a ClientId and an EncodedJwk with private key components. This poses a security risk as these credentials could be misused.
Replace the sensitive values with placeholders:
- "ClientId": "ce3b732a-d4f2-4997-8545-adf8df70fe6c",
- "Scope": "digdir:dialogporten.serviceprovider digdir:dialogporten.serviceprovider.search",
- "EncodedJwk": "eyJwIjoieTRBZEhlVVBxdFEtSFlOWkR5ci0zS09RT3NQajA5TFh2a2hIUFlTdGFYNThkMndIWUJiVXlDTWdMYWtGTHo4UExKNWtscURsanRoczFtM1dFVGJhSWVuY25TalpjZTh4S1Q2SHh3bTNyaDlydWZ1TWVOZDRqaFptTm9WZmJrcGNXcVh0UDFvb1NPTE5zYUNVUWFUUEVKTXlFd3VhdWxMSzgxRG1SSTlMSmVNIiwia3R5IjoiUlNBIiwicSI6InFmOEQ2Uy1Kd19BdVQ0Q2hjQTlDek9WNk1uTW9mc1VCdTkteHJBcVFDRjh4WWZZOTRxQ1ZjQ3llajlkTlN3eXZUZXg1dThIMzNSaU1LMEFWM2tTQlpJLVZqcXJHLUx6YzNfTUlTTVpSVDJfbzNVQlRWVHpqTkUtSkpMX1hKaXJ6ZVhhQjM1UmFZMjFnWVhKQWg3X2tuR3dpRzF3MGxiT2ozQ0FzdnVwaU1BMCIsImQiOiJLVkF1b1Zhd2paTTgwenRYcUxSZUJGZkJ3M3pxVjdkUGFpaWFONWU0RFp6bW5MYTFMNEZJMTgtanVraHN4UVdqR1NFQnBIdTFrOHRPUWMyWjBsSDVaaTBydERqM0JKeEhxeDNsUGdYMWdTNXNiX1EyeXNfb2FKcklSX012MHBDQUFHX3hpa2lUY2kzTHMyeV9femV4QTdLbG0yalNmYW9Udzdhbml1R3RId1d5dHhCSnJnZ0J2c3loaHZIaUVQcnZaMHZBZldYYWI3QUtkUjc1cEtEaVVHOGdGNTdJN0hrWnpJSk9QYXp3MTU1Skx4TG9HcDVzeTFCVVpBNHRiQmlseWVsdG9ONGZINWd1aUktOXJjTE5zUmVYazJ1c3NFbE9EbVZ2Qmx2ZVVfb1ZRMVYtVDRJRnUzZk1BYVJGUFA2Wlo1akJJX2hkOFJOTTJ3eUp5UHVRWVEiLCJlIjoiQVFBQiIsInVzZSI6InNpZyIsImtpZCI6ImRpYWxvZ3BvcnRlbi1zcC1zZGstdGVzdC0yMDI0MTAxMCIsInFpIjoiQm9VS0RlczQ0UTNpXzNyT3Q4aHRrS2NxWkFNem00Njl2cTZuQnJVcHBTU1Ric3YwalZwN1daRGRRR0Q0bU8yMVJVOEFUbmN3NjFPOUt3YXktOGloX082VWFWbGxZN3NHYlVrQ2NVaG43ZDkzSElLZnhybnhWVE9nNUNMWTBka2Zwa3A1V2pyU1VvMTVKQURsY3BRM0ItRlU0Nm9PTG9ydjJ0SVFQekE4OF93IiwiZHAiOiJ1emVaRWZpN2Fqa3JFREhYekZtTThXWFUtZ3RmM1ctN0pnY082MnpWc1JrNTN4QlcxTE1NZlRlN2tlWk9xOEhDN3hTbGktSm9idnR6WGU3Y295ZW9sTXkzTnlydXFhQVp4VTBPMHpHQWQ4UFdjdHNXeDlITHlrU1hNby1QVlVNNkpmZERCaWFtcXk5bGQ0WTRfdzlscEdVWEMyaUFwLXdsWktaSHdrbG1KR3MiLCJhbGciOiJSUzI1NiIsImRxIjoiVENBcV9DMlJuX0RhakRlcUU2aUIzWWVWNVNtMHBMQk1Tbm10OHNENEp3ZVo4YWgzcGhrTFVxUm9qVGw1SDNhYXVtWl9UUmxiaWVNSVFnWDh4UUFnZ1l2YkNYeG9oZEx0aGt3ckZZdlp0WjBEeHJDYm9Md1hjc0Y3Ukwyejl4LWMwSFBGVFAzLVREQWF6UWlBNVVtRmNwYnAzeDYzWGFLSWFuYnVFc0NiSDdFIiwibiI6Imh5Sks4WnE2Wk8tRjFSSklVWVNCdUpfeG9RWkNNV1EyTVhrSFQ1bVROVVJJZmVWWWpCNWMwMzI0Uk5nc3ZPMEtXX0hUejRRSnptLV9rU1VaZ0h1Z2JoR0F3a1Vqc1lwTlJJRTBvLVNtdEExMlMxZHVCZWx6ajg2LVFrZkFzeFlwblNnSzl5OXpTS1B0YVlzMS1EcEVIb0hVdk9BSDJlNktFTXRaYUZPM0J0Yk9WUURXMENMYi1FY0UyaDBQRlFMMUp3NU8zeDhHcXBZeUFhamNoWnptcWlFbjBaSEd1QTNZZ1NyNGxQV1lkTzBNWHZmRFdyaFBTcnVTS3FodzBHMTlBRUpHOFhoek9xTWxLTUFIbW5ybk9XOHM2cWR2Sy1UQ1BiVGJJOU5XUWdFd2JpUFBBdlU0MUFITzZmTEYxUHZzQ3FhNjZTSGdYMkJzS3pvNVhORjhodyJ9"
+ "ClientId": "<your-client-id>",
+ "Scope": "<required-scopes>",
+ "EncodedJwk": "<your-encoded-jwk>"📝 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.
| "ClientId": "ce3b732a-d4f2-4997-8545-adf8df70fe6c", | |
| "Scope": "digdir:dialogporten.serviceprovider digdir:dialogporten.serviceprovider.search", | |
| "EncodedJwk": "eyJwIjoieTRBZEhlVVBxdFEtSFlOWkR5ci0zS09RT3NQajA5TFh2a2hIUFlTdGFYNThkMndIWUJiVXlDTWdMYWtGTHo4UExKNWtscURsanRoczFtM1dFVGJhSWVuY25TalpjZTh4S1Q2SHh3bTNyaDlydWZ1TWVOZDRqaFptTm9WZmJrcGNXcVh0UDFvb1NPTE5zYUNVUWFUUEVKTXlFd3VhdWxMSzgxRG1SSTlMSmVNIiwia3R5IjoiUlNBIiwicSI6InFmOEQ2Uy1Kd19BdVQ0Q2hjQTlDek9WNk1uTW9mc1VCdTkteHJBcVFDRjh4WWZZOTRxQ1ZjQ3llajlkTlN3eXZUZXg1dThIMzNSaU1LMEFWM2tTQlpJLVZqcXJHLUx6YzNfTUlTTVpSVDJfbzNVQlRWVHpqTkUtSkpMX1hKaXJ6ZVhhQjM1UmFZMjFnWVhKQWg3X2tuR3dpRzF3MGxiT2ozQ0FzdnVwaU1BMCIsImQiOiJLVkF1b1Zhd2paTTgwenRYcUxSZUJGZkJ3M3pxVjdkUGFpaWFONWU0RFp6bW5MYTFMNEZJMTgtanVraHN4UVdqR1NFQnBIdTFrOHRPUWMyWjBsSDVaaTBydERqM0JKeEhxeDNsUGdYMWdTNXNiX1EyeXNfb2FKcklSX012MHBDQUFHX3hpa2lUY2kzTHMyeV9femV4QTdLbG0yalNmYW9Udzdhbml1R3RId1d5dHhCSnJnZ0J2c3loaHZIaUVQcnZaMHZBZldYYWI3QUtkUjc1cEtEaVVHOGdGNTdJN0hrWnpJSk9QYXp3MTU1Skx4TG9HcDVzeTFCVVpBNHRiQmlseWVsdG9ONGZINWd1aUktOXJjTE5zUmVYazJ1c3NFbE9EbVZ2Qmx2ZVVfb1ZRMVYtVDRJRnUzZk1BYVJGUFA2Wlo1akJJX2hkOFJOTTJ3eUp5UHVRWVEiLCJlIjoiQVFBQiIsInVzZSI6InNpZyIsImtpZCI6ImRpYWxvZ3BvcnRlbi1zcC1zZGstdGVzdC0yMDI0MTAxMCIsInFpIjoiQm9VS0RlczQ0UTNpXzNyT3Q4aHRrS2NxWkFNem00Njl2cTZuQnJVcHBTU1Ric3YwalZwN1daRGRRR0Q0bU8yMVJVOEFUbmN3NjFPOUt3YXktOGloX082VWFWbGxZN3NHYlVrQ2NVaG43ZDkzSElLZnhybnhWVE9nNUNMWTBka2Zwa3A1V2pyU1VvMTVKQURsY3BRM0ItRlU0Nm9PTG9ydjJ0SVFQekE4OF93IiwiZHAiOiJ1emVaRWZpN2Fqa3JFREhYekZtTThXWFUtZ3RmM1ctN0pnY082MnpWc1JrNTN4QlcxTE1NZlRlN2tlWk9xOEhDN3hTbGktSm9idnR6WGU3Y295ZW9sTXkzTnlydXFhQVp4VTBPMHpHQWQ4UFdjdHNXeDlITHlrU1hNby1QVlVNNkpmZERCaWFtcXk5bGQ0WTRfdzlscEdVWEMyaUFwLXdsWktaSHdrbG1KR3MiLCJhbGciOiJSUzI1NiIsImRxIjoiVENBcV9DMlJuX0RhakRlcUU2aUIzWWVWNVNtMHBMQk1Tbm10OHNENEp3ZVo4YWgzcGhrTFVxUm9qVGw1SDNhYXVtWl9UUmxiaWVNSVFnWDh4UUFnZ1l2YkNYeG9oZEx0aGt3ckZZdlp0WjBEeHJDYm9Md1hjc0Y3Ukwyejl4LWMwSFBGVFAzLVREQWF6UWlBNVVtRmNwYnAzeDYzWGFLSWFuYnVFc0NiSDdFIiwibiI6Imh5Sks4WnE2Wk8tRjFSSklVWVNCdUpfeG9RWkNNV1EyTVhrSFQ1bVROVVJJZmVWWWpCNWMwMzI0Uk5nc3ZPMEtXX0hUejRRSnptLV9rU1VaZ0h1Z2JoR0F3a1Vqc1lwTlJJRTBvLVNtdEExMlMxZHVCZWx6ajg2LVFrZkFzeFlwblNnSzl5OXpTS1B0YVlzMS1EcEVIb0hVdk9BSDJlNktFTXRaYUZPM0J0Yk9WUURXMENMYi1FY0UyaDBQRlFMMUp3NU8zeDhHcXBZeUFhamNoWnptcWlFbjBaSEd1QTNZZ1NyNGxQV1lkTzBNWHZmRFdyaFBTcnVTS3FodzBHMTlBRUpHOFhoek9xTWxLTUFIbW5ybk9XOHM2cWR2Sy1UQ1BiVGJJOU5XUWdFd2JpUFBBdlU0MUFITzZmTEYxUHZzQ3FhNjZTSGdYMkJzS3pvNVhORjhodyJ9" | |
| "ClientId": "<your-client-id>", | |
| "Scope": "<required-scopes>", | |
| "EncodedJwk": "<your-encoded-jwk>" |
.github/workflows/ci-cd-staging.yml
Outdated
| 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' }} | ||
| with: | ||
| version: ${{ needs.get-current-version.outputs.version }} | ||
| path: $(find . -name '*Digdir.Library.Dialogporten.WebApiClient.csproj' -printf "%p" -quit) | ||
| secrets: | ||
| NUGET_API_TEST_KEY: ${{ secrets.NUGET_API_TEST_KEY }} |
There was a problem hiding this comment.
Fix secret name mismatch in workflow
The workflow uses NUGET_API_TEST_KEY but the reusable workflow expects NUGET_API_KEY.
- NUGET_API_TEST_KEY: ${{ secrets.NUGET_API_TEST_KEY }}
+ NUGET_API_KEY: ${{ secrets.NUGET_API_KEY }}📝 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.
| 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' }} | |
| with: | |
| version: ${{ needs.get-current-version.outputs.version }} | |
| path: $(find . -name '*Digdir.Library.Dialogporten.WebApiClient.csproj' -printf "%p" -quit) | |
| secrets: | |
| NUGET_API_TEST_KEY: ${{ secrets.NUGET_API_TEST_KEY }} | |
| 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' }} | |
| with: | |
| version: ${{ needs.get-current-version.outputs.version }} | |
| path: $(find . -name '*Digdir.Library.Dialogporten.WebApiClient.csproj' -printf "%p" -quit) | |
| secrets: | |
| NUGET_API_KEY: ${{ secrets.NUGET_API_KEY }} |
🧰 Tools
🪛 actionlint (1.7.4)
97-97: secret "NUGET_API_KEY" is required by "./.github/workflows/workflow-publish-nuget.yml" reusable workflow
(workflow-call)
104-104: secret "NUGET_API_TEST_KEY" is not defined in "./.github/workflows/workflow-publish-nuget.yml" reusable workflow. defined secret is "NUGET_API_KEY"
(workflow-call)
…nto feat/add-WepApiClient-project
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
.github/workflows/ci-cd-staging.yml (1)
105-105: Remove trailing whitespaceRemove trailing spaces on line 105.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 105-105: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci-cd-staging.yml(10 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/ci-cd-staging.yml
[error] 105-105: trailing spaces
(trailing-spaces)
🔇 Additional comments (3)
.github/workflows/ci-cd-staging.yml (3)
23-23: LGTM: Improved job dependency formatting
The reformatting of needs arrays improves readability while maintaining functionality.
Also applies to: 36-36, 53-53, 67-67, 77-77, 108-108, 120-120, 136-136, 148-148, 163-163
104-104:
Fix secret name mismatch in workflow
The workflow uses NUGET_API_TEST_KEY but the reusable workflow expects NUGET_API_KEY.
- NUGET_API_KEY: ${{ secrets.NUGET_API_TEST_KEY }}
+ NUGET_API_KEY: ${{ secrets.NUGET_API_KEY }}96-104: Verify Refitter dependency handling
Since this package depends on Refitter which isn't available in the preview version, we should verify that the package can be built and published successfully in the CI environment.
| 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' }} | ||
| with: | ||
| version: ${{ needs.get-current-version.outputs.version }} | ||
| path: $(find . -name '*Digdir.Library.Dialogporten.WebApiClient.csproj' -printf "%p" -quit) | ||
| secrets: | ||
| NUGET_API_KEY: ${{ secrets.NUGET_API_TEST_KEY }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve robustness of project file path resolution
The current implementation has several potential issues:
- The
$(find ...)command is embedded in thepathparameter and uses shell syntax that might not work consistently in GitHub Actions - The 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 }}
secrets:
NUGET_API_KEY: ${{ secrets.NUGET_API_TEST_KEY }}This approach:
- Uses a dedicated step for path resolution
- Excludes test projects using
-not -path "*/test/*" - Stores the result in an environment variable
- Fails fast if the project file is not found
Committable suggestion skipped: line range outside the PR's diff.
Description
Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)