Skip to content

Conversation

@Y-Sindo
Copy link
Member

@Y-Sindo Y-Sindo commented Nov 28, 2025

  • Refactor global Web PubSub connection Options setup in preparation for AAD
  • fix
  • Update webpubsub input and output binding

…r AAD

* Abstract Web PubSub access credential into a new class, unifies identity-based connection and key-based connection
* Add a util class to create Web PubSub access object from conneciton string or identity-based connection
* Unify global service access info into `WebPubSubServiceAccessOptions`.
Copilot AI review requested due to automatic review settings November 28, 2025 08:37
@Y-Sindo Y-Sindo requested a review from vicancy as a code owner November 28, 2025 08:37
Copilot finished reviewing on behalf of Y-Sindo November 28, 2025 08:40
Copy link
Contributor

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 pull request adds identity-based (Azure Active Directory) authentication support for WebPubSub input and output bindings, expanding beyond the existing connection string-based authentication. The changes refactor the connection configuration system to support both authentication methods with proper fallback logic.

Key Changes

  • New authentication support: Added IdentityCredential and KeyCredential classes to support both AAD token-based and key-based authentication
  • Refactored connection management: Introduced WebPubSubServiceClientFactory and related utility classes to centralize connection creation and validation logic
  • Breaking API changes: Removed [ConnectionString] attribute from binding properties to support configuration section names alongside connection strings

Reviewed changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 12 comments.

Show a summary per file
File Description
WebPubSubServiceClientFactoryTests.cs New tests for client factory with hub and endpoint resolution logic
WebPubSubServiceAccessUtilTests.cs Comprehensive tests for connection string parsing and configuration-based access creation
WebPubSubServiceAccessOptionsSetupTests.cs Tests for options setup with various connection configurations
JobHostEndToEndTests.cs Updated end-to-end tests to cover both global and local connection scenarios with identity support
TestAzureComponentFactory.cs Test utility for creating Azure component factory instances
WebPubSubConnectionAttribute.cs Updated documentation to reflect configuration section name support
WebPubSubAttribute.cs Updated documentation to reflect configuration section name support
Microsoft.Azure.WebJobs.Extensions.WebPubSub.csproj Added Microsoft.Extensions.Azure dependency and removed ApiCompatVersion
Constants.cs Added ServiceUriKey constant for identity-based connection configuration
WebPubSubServiceCredential.cs New credential abstraction supporting key-based and identity-based authentication
WebPubSubServiceClientFactory.cs Factory for creating WebPubSubServiceClient with connection and hub fallback logic
WebPubSubServiceAccessUtil.cs Utility methods for parsing connection strings and creating access objects from configuration
WebPubSubServiceAccessOptionsSetup.cs Options setup to configure default WebPubSub access from configuration
WebPubSubServiceAccessOptions.cs Options class holding default WebPubSub access and hub configuration
WebPubSubServiceAccess.cs Container for service endpoint and credential information
WebPubSubJobsBuilderExtensions.cs Updated to register new services for identity-based connection support
WebPubSubConfigProvider.cs Refactored to use new factory pattern and centralized validation logic
IWebPubSubServiceClientFactory.cs Interface defining client factory contract

Assert.IsNotNull(result);
Assert.AreEqual(new Uri(serviceUri), result.ServiceEndpoint);
Assert.IsInstanceOf<IdentityCredential>(result.Credential);
var identityCredential = (IdentityCredential)result.Credential;
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

In line 169, the IdentityCredential is cast but the result is unused. Either remove this line if it's not needed, or use the variable to perform assertions about the credential.

Suggested change
var identityCredential = (IdentityCredential)result.Credential;
var identityCredential = (IdentityCredential)result.Credential;
Assert.IsNotNull(identityCredential);

Copilot uses AI. Check for mistakes.
Comment on lines +67 to +78
private static WebPubSubServiceClient CreateClient(WebPubSubServiceAccess access, string hub)
{
if (access.Credential is KeyCredential keyCredential)
{
return new WebPubSubServiceClient(access.ServiceEndpoint, hub, new AzureKeyCredential(keyCredential.AccessKey));
}
if (access.Credential is IdentityCredential identityCredential)
{
return new WebPubSubServiceClient(access.ServiceEndpoint, hub, identityCredential.TokenCredential);
}
throw new InvalidOperationException($"Unsupported credential type {access.Credential.GetType().Name} for WebPubSubServiceClient.");
}
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

The error handling in CreateClient method doesn't account for the case when access.Credential is null. If WebPubSubServiceAccess is initialized with a null credential (which could happen based on the validation logic), this will throw a NullReferenceException instead of the intended InvalidOperationException. Consider adding a null check before checking the credential type.

Copilot uses AI. Check for mistakes.
Comment on lines +124 to +147
private void ValidateWebPubSubConnectionCore(string attributeConnection, string attributeHub, string extensionType)
{
var connectionString = Utilities.FirstOrDefault(attribute.Connection, _options.ConnectionString);
var hubName = Utilities.FirstOrDefault(attribute.Hub, _options.Hub);
return new WebPubSubService(connectionString, hubName);
var webPubSubAccessExists = true;
if (attributeConnection == null)
{
if (_accessOptions.CurrentValue.WebPubSubAccess == null)
{
webPubSubAccessExists = false;
}
}
else
{
if (!WebPubSubServiceAccessUtil.CanCreateFromIConfiguration(_configuration.GetSection(attributeConnection)))
{
webPubSubAccessExists = false;
}
}
if (!webPubSubAccessExists)
{
throw new InvalidOperationException(
$"Connection must be specified through one of the following:" + Environment.NewLine +
$" * Set '{extensionType}.Connection' property to the name of a config section that contains a Web PubSub connection." + Environment.NewLine +
$" * Set a Web PubSub connection under '{Constants.WebPubSubConnectionStringName}'.");
}
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

The validation logic has inconsistent handling: when attributeConnection is null, it checks if _accessOptions.CurrentValue.WebPubSubAccess == null, but when attributeConnection is not null, it only checks if the configuration section can create an access object. This means if a non-null but empty connection name is provided, and the global connection is configured, the validation will fail even though it could fall back to the global connection. Consider aligning the validation logic with the actual client creation logic in WebPubSubServiceClientFactory.Create().

Copilot uses AI. Check for mistakes.
/// <returns><see cref="IWebJobsBuilder"/>.</returns>
public static IWebJobsBuilder AddWebPubSub(this IWebJobsBuilder builder)
{
;
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

Empty statement (;) should be removed. This appears to be an accidental addition.

Suggested change
;

Copilot uses AI. Check for mistakes.
Comment on lines +89 to +90
var tokenCrential = azureComponentFactory.CreateTokenCredential(section);
result = new WebPubSubServiceAccess(endpoint, new IdentityCredential(tokenCrential));
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

Typo in variable name: tokenCrential should be tokenCredential.

Suggested change
var tokenCrential = azureComponentFactory.CreateTokenCredential(section);
result = new WebPubSubServiceAccess(endpoint, new IdentityCredential(tokenCrential));
var tokenCredential = azureComponentFactory.CreateTokenCredential(section);
result = new WebPubSubServiceAccess(endpoint, new IdentityCredential(tokenCredential));

Copilot uses AI. Check for mistakes.
namespace Microsoft.Azure.WebJobs.Extensions.WebPubSub;

/// <summary>
/// Can be key-based credential, identity-based connection, or null (A connection string without access key provided to Web PubSub trigger)
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

The comment "or null (A connection string without access key provided to Web PubSub trigger)" suggests that null credentials are possible, but the WebPubSubServiceAccess constructor requires a non-null credential parameter (no null-check or nullable annotation). This creates an inconsistency. Either the constructor should accept nullable credentials with proper handling, or the comment should be updated to clarify that null is not actually supported.

Suggested change
/// Can be key-based credential, identity-based connection, or null (A connection string without access key provided to Web PubSub trigger)
/// Can be key-based credential or identity-based connection.

Copilot uses AI. Check for mistakes.
<Version>1.10.0-beta.1</Version>
<!--The ApiCompatVersion is managed automatically and should not generally be modified manually.-->
<ApiCompatVersion>1.9.0</ApiCompatVersion>
<!--Removed ApiCompatVersion to skip ApiCompatability check to update binding attributes. They'll be added back automatically after release.-->
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

[nitpick] The comment states the ApiCompatVersion will be "added back automatically after release", but this is misleading. API compatibility version is typically managed through the build/release pipeline and may need manual intervention. Consider clarifying what the actual process is, or simply note that it was removed to allow breaking changes for identity-based connection support.

Suggested change
<!--Removed ApiCompatVersion to skip ApiCompatability check to update binding attributes. They'll be added back automatically after release.-->
<!--Removed ApiCompatVersion to skip ApiCompatability check to update binding attributes for identity-based connection support. Add it back after release to re-enable API compatibility checks.-->

Copilot uses AI. Check for mistakes.
{
/// <summary>
/// Target Web PubSub service connection string.
/// The configuration section name that resolves to the service endpoint URI or connection string.
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

The [ConnectionString] attribute was removed from the Connection property, which is a breaking change in the attribute's behavior. This should be documented in a changelog or migration guide to help users understand that this property now accepts configuration section names in addition to connection strings.

Suggested change
/// The configuration section name that resolves to the service endpoint URI or connection string.
/// The configuration section name that resolves to the service endpoint URI or connection string.
/// <para>
/// <b>Breaking change:</b> The <c>[ConnectionString]</c> attribute was removed from this property.
/// This property now accepts configuration section names in addition to connection strings.
/// Please update your configuration accordingly.
/// </para>

Copilot uses AI. Check for mistakes.
/// <summary>
/// The connection of target Web PubSub service.
/// The connection name that resolves to the service endpoint URI or connection string.
/// </summary>
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

The [ConnectionString] attribute was removed from the Connection property, which is a breaking change in the attribute's behavior. This should be documented in a changelog or migration guide to help users understand that this property now accepts configuration section names in addition to connection strings.

Suggested change
/// </summary>
/// </summary>
// BREAKING CHANGE: The [ConnectionString] attribute was removed from the Connection property.
// The Connection property now accepts configuration section names in addition to connection strings.
// Please see the changelog or migration guide for more information.

Copilot uses AI. Check for mistakes.
Copy link
Member

Choose a reason for hiding this comment

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

You usually cannot remove an attribute like this without it being a binary breaking change. Most likely, you'll need to continue to have the member as is and mark it as EditorBrowsable.Never and potentially Obsolete. You'd then add a new member with the new behavior. It's sometimes ok for the behavior to change and throw a NotSupportedException, but you cannot make a binary breaking change to the API.

Please reach out to @christothes to discuss the right path forward.


Assert.IsNotNull(result);
Assert.AreEqual(new Uri(endpoint), result.ServiceEndpoint);
Assert.IsNull((result.Credential as KeyCredential).AccessKey);
Copy link

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

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

This test attempts to access AccessKey property on a null credential, which will throw a NullReferenceException. When the connection string doesn't contain an AccessKey, the credential is a KeyCredential with a null access key. The assertion should check if the credential itself is not null before checking the AccessKey property, or verify that (result.Credential as KeyCredential)?.AccessKey is null.

Suggested change
Assert.IsNull((result.Credential as KeyCredential).AccessKey);
Assert.IsNull((result.Credential as KeyCredential)?.AccessKey);

Copilot uses AI. Check for mistakes.
<Version>1.10.0-beta.1</Version>
<!--The ApiCompatVersion is managed automatically and should not generally be modified manually.-->
<ApiCompatVersion>1.9.0</ApiCompatVersion>
<!--Removed ApiCompatVersion to skip ApiCompatability check to update binding attributes. They'll be added back automatically after release.-->
Copy link
Member

Choose a reason for hiding this comment

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

Please revert. API Compat is required, as you should not see failures unless you're making breaking changes - which are not allowed.

/// <summary>
/// The connection of target Web PubSub service.
/// The connection name that resolves to the service endpoint URI or connection string.
/// </summary>
Copy link
Member

Choose a reason for hiding this comment

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

You usually cannot remove an attribute like this without it being a binary breaking change. Most likely, you'll need to continue to have the member as is and mark it as EditorBrowsable.Never and potentially Obsolete. You'd then add a new member with the new behavior. It's sometimes ok for the behavior to change and throw a NotSupportedException, but you cannot make a binary breaking change to the API.

Please reach out to @christothes to discuss the right path forward.

@jsquire jsquire requested a review from christothes November 28, 2025 17:13
@jsquire
Copy link
Member

jsquire commented Nov 28, 2025

//cc: @christothes, for architect guidance

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants