-
Notifications
You must be signed in to change notification settings - Fork 301
Introduce new authentication provider Unauthenticated
#3075
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: JerryNixon <1749983+JerryNixon@users.noreply.github.com>
…and JSON schema Co-authored-by: JerryNixon <1749983+JerryNixon@users.noreply.github.com>
Co-authored-by: JerryNixon <1749983+JerryNixon@users.noreply.github.com>
Unauthenticated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Introduces a new Unauthenticated authentication provider intended to treat all requests as anonymous without requiring JWT configuration.
Changes:
- Added an
UnauthenticatedASP.NET Core auth handler/scheme and wired it intoStartupauth registration paths. - Updated CLI validation and config validation logic to allow
Unauthenticatedwithout JWT (with warnings for non-anonymous role permissions). - Extended schema and CLI tests/snapshots to include the new provider.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Service/Startup.cs | Registers the Unauthenticated auth scheme in both auth configuration paths. |
| src/Core/AuthenticationHelpers/UnauthenticatedAuthenticationHandler/UnauthenticatedAuthenticationHandler.cs | New auth handler that yields an anonymous principal. |
| src/Core/AuthenticationHelpers/UnauthenticatedAuthenticationHandler/UnauthenticatedAuthenticationDefaults.cs | Defines the scheme name constants for the new provider. |
| src/Core/AuthenticationHelpers/UnauthenticatedAuthenticationHandler/UnauthenticatedAuthenticationBuilderExtensions.cs | Adds an AuthenticationBuilder extension to register the new scheme. |
| src/Core/AuthenticationHelpers/SupportedAuthNProviders.cs | Adds Unauthenticated to the supported provider constants. |
| src/Config/ObjectModel/AuthenticationOptions.cs | Adds provider detection helper and updates JWT-required determination logic. |
| src/Cli/Utils.cs | Allows Unauthenticated to omit JWT audience/issuer and updates messaging. |
| src/Cli/ConfigGenerator.cs | Emits a warning when Unauthenticated is used with non-anonymous permissions. |
| src/Cli.Tests/ValidateConfigTests.cs | Adds unit tests for Unauthenticated provider helper methods. |
| src/Cli.Tests/Snapshots/InitTests.EnsureCorrectConfigGenerationWithDifferentAuthenticationProviders_47836da0dfbdc458.verified.txt | Updates snapshot for config generation using Unauthenticated. |
| src/Cli.Tests/InitTests.cs | Adds Unauthenticated as a test case for init/config generation. |
| schemas/dab.draft.schema.json | Adds Unauthenticated to the provider enum and documents it. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@copilot apply changes based on the comments in this thread |
…xplicit Where() filtering, add unit tests Co-authored-by: JerryNixon <1749983+JerryNixon@users.noreply.github.com>
Applied all changes from the review thread in e3fb034:
|
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
|
@RubenCerna2079 you will need to help here and resolve the integration test errs for me. |
src/Cli/ConfigGenerator.cs
Outdated
| { | ||
| foreach (KeyValuePair<string, Entity> entity in config.Entities.Where(e => e.Value.Permissions is not null)) | ||
| { | ||
| foreach (EntityPermission permission in entity.Value.Permissions!.Where(p => !p.Role.Equals("anonymous", StringComparison.OrdinalIgnoreCase))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this foreach redundant? Since all of the entities would be unauthenticated, wouldn't it be better to only give a general warning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplified in f0b25d0. Changed to emit a single general warning instead of per-entity warnings when non-anonymous roles are detected.
| /// <summary> | ||
| /// Returns whether the configured Provider value matches the unauthenticated authentication type. | ||
| /// </summary> | ||
| /// <returns>True when all operations run as anonymous.</returns> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// <returns>True when all operations run as anonymous.</returns> | |
| /// <returns>True if Provider is Unauthenticated type.</returns> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in f0b25d0.
| public const string UNAUTHENTICATED_AUTHENTICATION = "Unauthenticated"; | ||
|
|
||
| /// <summary> | ||
| /// Returns whether the configured Provider value matches the unauthenticated authentication type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// Returns whether the configured Provider value matches the unauthenticated authentication type. | |
| /// Returns whether the configured Provider value matches the Unauthenticated authentication type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in f0b25d0.
| { | ||
| return SimulatorAuthenticationDefaults.AUTHENTICATIONSCHEME; | ||
| } | ||
| else if (string.Equals(configuredProviderName, SupportedAuthNProviders.UNAUTHENTICATED, StringComparison.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonder if this need more testing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added test case ValidateUnauthenticatedProviderIdentification in AuthenticationConfigValidatorUnitTests.cs to validate the Unauthenticated provider detection. Additional coverage is provided through the existing tests that now include Unauthenticated: TestValidateAudienceAndIssuerForAuthenticationProvider, TestBaseRouteIsConfigurableForSWA, and TestUpdateAuthenticationProviderHostSettings.
| using Microsoft.Extensions.Logging; | ||
| using Microsoft.Extensions.Options; | ||
|
|
||
| namespace Azure.DataApiBuilder.Core.AuthenticationHelpers.UnauthenticatedAuthenticationHandler; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change the name of the folder in which this file resides to UnauthenticatedAuthentication
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in f0b25d0. Renamed folder to UnauthenticatedAuthentication and updated all namespace references.
| /// <summary> | ||
| /// This class is used to best integrate with ASP.NET Core AuthenticationHandler base class. | ||
| /// When "Unauthenticated" is configured, this handler authenticates the user as anonymous, | ||
| /// without reading any HTTP authentication headers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// <summary> | |
| /// This class is used to best integrate with ASP.NET Core AuthenticationHandler base class. | |
| /// When "Unauthenticated" is configured, this handler authenticates the user as anonymous, | |
| /// without reading any HTTP authentication headers. | |
| /// <summary> | |
| /// This class is used to best integrate with ASP.NET Core AuthenticationHandler base class. | |
| /// Ref: https://github.com/dotnet/aspnetcore/blob/main/src/Security/Authentication/Core/src/AuthenticationHandler.cs | |
| /// When "Unauthenticated" is configured, this handler authenticates the user as anonymous, | |
| /// without reading any HTTP authentication headers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in f0b25d0. Added reference to ASP.NET Core AuthenticationHandler.
| ClaimsIdentity identity = new(); | ||
| ClaimsPrincipal claimsPrincipal = new(identity); | ||
|
|
||
| AuthenticationTicket ticket = new(claimsPrincipal, UnauthenticatedAuthenticationDefaults.AUTHENTICATIONSCHEME); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to add UnauthenticatedAuthenticationDefaults.AUTHENTICATIONSCHEME for the AuthenticationTicket if the in the ClaimsIdentity we don't use it since it is an unauthenticated user?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The AuthenticationTicket requires a scheme name as part of the ASP.NET Core authentication infrastructure. While the ClaimsIdentity has no authenticationType (making it unauthenticated), the ticket still needs to identify which authentication handler processed the request. This follows the same pattern as SimulatorAuthenticationHandler.
RubenCerna2079
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing tests for the following test methods
TestValidateAudienceAndIssuerForAuthenticationProviderinUtilTests.csTestBaseRouteIsConfigurableForSWAinEndToEndTests.csTestUpdateAuthenticationProviderHostSettingsinConfigureOptionsTests.cs
src/Cli.Tests/ValidateConfigTests.cs
Outdated
| /// Test that the Unauthenticated provider is correctly identified by the IsUnauthenticatedAuthenticationProvider method. | ||
| /// </summary> | ||
| [TestMethod] | ||
| public void TestIsUnauthenticatedAuthenticationProviderMethod() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This tests should be in AuthenticationConfigValidatorUnitTests.cs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved in f0b25d0. Test ValidateUnauthenticatedProviderIdentification now in AuthenticationConfigValidatorUnitTests.cs.
src/Cli.Tests/ValidateConfigTests.cs
Outdated
| /// Test that Unauthenticated provider does not require JWT configuration. | ||
| /// </summary> | ||
| [TestMethod] | ||
| public void TestUnauthenticatedProviderDoesNotRequireJwt() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test should not exist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed in f0b25d0.
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
…ing, add tests Co-authored-by: RubenCerna2079 <32799214+RubenCerna2079@users.noreply.github.com>
Added in f0b25d0:
|
Why make this change?
New authentication provider where all operations run as
anonymous. Useful when DAB is behind an app gateway or APIM where authentication is handled externally.What is this change?
Core Implementation:
IsUnauthenticatedAuthenticationProvider()toAuthenticationOptions.csUnauthenticatedAuthenticationHandlerinUnauthenticatedAuthenticationfolder (follows Simulator pattern, returns unauthenticated ClaimsPrincipal)Startup.csto register the provider in bothConfigureAuthentication()andConfigureAuthenticationV2()ClientRoleHeaderAuthenticationMiddleware.ResolveConfiguredAuthNScheme()for proper scheme selection at request timeCLI & Validation:
Utils.ValidateAudienceAndIssuerForJwtProvider()to accept Unauthenticated without JWTConfigGenerator.IsConfigValid()when used with authenticated/custom roles (not an error)Schema:
Unauthenticatedtodab.draft.schema.jsonprovider enumKey behaviors:
productionmode (unlike Simulator)authenticated/custom role permissions (warning emitted)How was this tested?
ValidateUnauthenticatedProviderIdentificationtest inAuthenticationConfigValidatorUnitTests.csTestValidateAudienceAndIssuerForAuthenticationProvider(UtilsTests.cs)TestBaseRouteIsConfigurableForSWA(EndToEndTests.cs)TestUpdateAuthenticationProviderHostSettings(ConfigureOptionsTests.cs)Sample Request(s)
dab init --database-type mssql --connection-string "..." --auth.provider UnauthenticatedConfig snippet:
{ "runtime": { "host": { "authentication": { "provider": "Unauthenticated" } } } }✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.