Skip to content

feat: make auth configurable#38

Open
mkilgus-at wants to merge 12 commits intomainfrom
feat/authconfig
Open

feat: make auth configurable#38
mkilgus-at wants to merge 12 commits intomainfrom
feat/authconfig

Conversation

@mkilgus-at
Copy link
Copy Markdown
Collaborator

No description provided.

Copilot AI review requested due to automatic review settings March 12, 2026 09:47
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces configurable authentication for the receiver API and moves the public “receive melding” POST endpoint to a new controller/path, alongside a major version bump and dependency/tooling updates.

Changes:

  • Add configurable auth toggle (DisableAuth) with Entra JWT bearer support when enabled.
  • Move POST /meldinger to POST /receive/melding (new ReceiveController) and remove the old POST from MeldingerController.
  • Update dependencies/tooling and document the breaking change (version bump + changelog/docs updates).

Reviewed changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
src/compose.yaml Sets auth-disable flag for local docker compose runs.
src/Publish/Receiver.Publish/Receiver.Publish.csproj Bumps package version to 2.0.0 and updates dependencies.
src/Publish/Receiver.Publish.Test/Receiver.Publish.Test.csproj Updates test dependencies.
src/Publish/Receiver.Publish.Test.ArchUnit/Receiver.Publish.Test.ArchUnit.csproj Updates ArchUnit + test dependencies.
src/Infrastructure/test/Infrastructure.Test.csproj Updates test dependencies (incl. WireMock/Verify versions).
src/Infrastructure/src/Infrastructure.csproj Updates infrastructure library dependencies.
src/Domain/Ports/Infrastructure/Domain.Ports.Infrastructure.csproj Updates Altinn dependency version.
src/Domain/Logic/test/Domain.Logic.Test.csproj Updates test dependencies.
src/Domain/Logic/src/Domain.Logic.csproj Updates domain logic dependencies.
src/CHANGELOG.md Documents the breaking endpoint move and version bump.
src/ArchUnit.Tests/ArchUnit.Tests.csproj Updates ArchUnit + test dependencies.
src/App/test/App.Test.csproj Updates app test dependencies.
src/App/src/appsettings.json Renames auth setting key (but currently placed under root Authentication).
src/App/src/WebApi/Controllers/WebhookController.cs Removes unused options injection from controller ctor.
src/App/src/WebApi/Controllers/ReceiveController.cs Adds new authorized receive endpoint controller.
src/App/src/WebApi/Controllers/MeldingerController.cs Removes old unauthenticated POST endpoint and keeps read endpoints.
src/App/src/Extensions/StartupExtensions.cs Adds auth/authorization wiring and OpenAPI security behavior based on config.
src/App/src/AppSettings.cs Renames/extends auth configuration model (DisableAuth + Entra settings).
src/App/src/App.csproj Adds JwtBearer package reference and updates deps.
global.json Updates pinned .NET SDK version.
documentation/configuration.md Documents new auth-related configuration and endpoint guidance.
.config/dotnet-tools.json Updates csharpier tool version.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment thread documentation/configuration.md Outdated
Comment thread documentation/configuration.md Outdated
Comment on lines 14 to 18
public class WebhookController(
IMeldingService meldingService,
IAltinnAdapter altinnAdapter,
IOptions<InfrastructureConfiguration> options,
ApiMeters apiMeters
) : ControllerBase
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

After removing the IOptions<InfrastructureConfiguration> parameter, this file no longer uses Microsoft.Extensions.Options (and likely the Infrastructure.DependencyInjection import used only for InfrastructureConfiguration). Consider removing the now-unused using directives to keep the file clean.

Copilot uses AI. Check for mistakes.
Comment thread documentation/configuration.md Outdated
Comment thread src/App/src/WebApi/Controllers/ReceiveController.cs
Comment thread src/App/src/appsettings.json Outdated
Comment thread src/App/src/AppSettings.cs Outdated
Comment on lines +58 to +63
LoggerFactory
.Create(builder => builder.AddConsole())
.CreateLogger<Program>()
.LogWarning(
"Authentication is disabled. Update AuthenticationConfiguration to require authentication."
);
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

This creates a new LoggerFactory with its own console provider, bypassing the app’s configured logging pipeline and potentially duplicating console logs. Prefer using the existing host logging (e.g., inject/use an ILogger from DI) to emit this warning.

Copilot uses AI. Check for mistakes.
Comment on lines +82 to +92
"EntraClientId must be set either in appsettings when auth is enabled"
);
}
if (string.IsNullOrEmpty(tenantId))
{
throw new ArgumentException("EntraTenantId must be set when auth is enabled");
}
if (string.IsNullOrEmpty(scope))
{
throw new ArgumentException(
"(Default) EntraScope must be set when auth is enabled, e.g. api://<my.app>/.default"
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

The thrown ArgumentException messages are a bit misleading/incomplete (e.g., they mention appsettings only, and don’t identify the offending parameter). Consider using clearer messages that reference the expected configuration keys/env vars and include paramName for easier diagnostics.

Suggested change
"EntraClientId must be set either in appsettings when auth is enabled"
);
}
if (string.IsNullOrEmpty(tenantId))
{
throw new ArgumentException("EntraTenantId must be set when auth is enabled");
}
if (string.IsNullOrEmpty(scope))
{
throw new ArgumentException(
"(Default) EntraScope must be set when auth is enabled, e.g. api://<my.app>/.default"
"AuthenticationConfiguration.EntraClientId must be set when authentication is enabled. " +
"Configure it in appsettings.json (AuthenticationConfiguration:EntraClientId) or via the " +
"'AuthenticationConfiguration__EntraClientId' environment variable.",
nameof(clientId)
);
}
if (string.IsNullOrEmpty(tenantId))
{
throw new ArgumentException(
"AuthenticationConfiguration.EntraTenantId must be set when authentication is enabled. " +
"Configure it in appsettings.json (AuthenticationConfiguration:EntraTenantId) or via the " +
"'AuthenticationConfiguration__EntraTenantId' environment variable.",
nameof(tenantId)
);
}
if (string.IsNullOrEmpty(scope))
{
throw new ArgumentException(
"AuthenticationConfiguration.EntraScope must be set when authentication is enabled, " +
"for example 'api://<my.app>/.default'. Configure it in appsettings.json " +
"(AuthenticationConfiguration:EntraScope) or via the " +
"'AuthenticationConfiguration__EntraScope' environment variable.",
nameof(scope)

Copilot uses AI. Check for mistakes.
Comment thread src/CHANGELOG.md Outdated
mkilgus-at and others added 6 commits March 12, 2026 10:49
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@sonarqubecloud
Copy link
Copy Markdown

[Authorize]
[ApiController]
[Route("[controller]")]
public class ReceiveController : ControllerBase
Copy link
Copy Markdown
Contributor

@at-bgp at-bgp Mar 12, 2026

Choose a reason for hiding this comment

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

Er det meninga at det skal være receive? Altså uten '-r'?

env.IsDevelopment()
);

if (apiConfiguration.AuthenticationConfiguration.DisableAuth)
Copy link
Copy Markdown
Contributor

@at-bgp at-bgp Mar 12, 2026

Choose a reason for hiding this comment

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

Lurer på om denne blocken er generell nok til at vi burde ha det i dotnet-common (AddOAuth(config) elns)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Arbeidstilsynet/dotnet-common#143
kanskje vi kan prøve å ferdigstille den her først

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

chore: protect POST endpoint with [Authorize] attribute to support entra id authentication if needed

3 participants