Skip to content

Adds Default Targeting Accessor and extension method WithTargeting #466

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

Merged
merged 5 commits into from
Jul 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ protected override Task<AuthenticateResult> HandleAuthenticateAsync()

foreach (string group in groups)
{
identity.AddClaim(new Claim(ClaimTypes.GroupName, group));
identity.AddClaim(new Claim(ClaimTypes.Role, group));
}

Logger.LogInformation($"Assigning the following groups '{string.Join(", ", groups)}' to the request.");
Expand Down
10 changes: 0 additions & 10 deletions examples/FeatureFlagDemo/ClaimTypes.cs

This file was deleted.

66 changes: 0 additions & 66 deletions examples/FeatureFlagDemo/HttpContextTargetingContextAccessor.cs

This file was deleted.

2 changes: 1 addition & 1 deletion examples/FeatureFlagDemo/Startup.cs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public void ConfigureServices(IServiceCollection services)

services.AddFeatureManagement()
.AddFeatureFilter<BrowserFilter>()
.WithTargeting<HttpContextTargetingContextAccessor>()
.WithTargeting()
.UseDisabledFeaturesHandler(new FeatureNotEnabledDisabledHandler());

services.AddMvc(o =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,12 @@
//
using Microsoft.AspNetCore.Mvc.Filters;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.DependencyInjection.Extensions;
using Microsoft.FeatureManagement.FeatureFilters;
using Microsoft.FeatureManagement.Mvc;
using System;
using System.Collections.Generic;
using System.Linq;

namespace Microsoft.FeatureManagement
{
Expand Down Expand Up @@ -44,5 +47,28 @@ public static IFeatureManagementBuilder UseDisabledFeaturesHandler(this IFeature

return builder;
}

/// <summary>
/// Enables the use of targeting within the application and adds a targeting context accessor that extracts targeting details from a request's HTTP context.
/// </summary>
/// <param name="builder">The <see cref="IFeatureManagementBuilder"/> used to customize feature management functionality.</param>
/// <returns>A <see cref="IFeatureManagementBuilder"/> that can be used to customize feature management functionality.</returns>
public static IFeatureManagementBuilder WithTargeting(this IFeatureManagementBuilder builder)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we call AddHttpContextAccessor in this method since our default targeting accessor depends on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a great question. I originally thought no- so we're in parity with what we have today. However it would remove another hurdle to get started so I think it's a good idea.

Copy link
Member

Choose a reason for hiding this comment

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

I'd like to decouple that.

By the way, is this method name the one that we discussed so many options and couldn't settle on one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, let's keep it decoupled. Could always do that in a separate PR anyway.

By the way, is this method name the one that we discussed so many options and couldn't settle on one?

Yes it is. The last design review it was discussed up was the caching meeting last month. We lightly aligned on .WithTelemetry<DefaultTargetingContextAccessor>(), but some smaller discussion afterwards lead to .WithTelemetry() so we didn't have to make DefaultTargetingContextAccessor public.

Although I'm having trouble finding a paper trail to confirm that we did commit to this.

Copy link
Contributor Author

@rossgrambo rossgrambo Jul 2, 2024

Choose a reason for hiding this comment

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

@zhenlan do you have thoughts on the .WithTelemetry() WithTargeting() over .WithTargeting<DefaultTargetingContextAccessor>()

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this would have been referring to WithTargeting, not WithTelemetry

Copy link
Member

Choose a reason for hiding this comment

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

Would a blazor user take a dependency on this package and see this method? My thought is that if we go with WithTargeting, any application that would depend on this package should most likely use this method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My fault- yes .WithTargeting.

Blazor Webassembly is just a client side thing- so it would work fine with an ASP.NET backend requiring this.

Blazor Server would not work as expected- and I wouldn't expect the dev to use the ASP.NET package. Although- Blazor Server are built on top of ASP.NET Core so it's not unreasonable a developer would try this.

Copy link
Member

Choose a reason for hiding this comment

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

We could add it by default if 1) it will not break any existing applications and 2) users can easily override it if they want to use their own targeting context accessor.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, having it with just WithTargeting sounds reasonable to me.

{
//
// Register the targeting context accessor with the same lifetime as the feature manager
if (builder.Services.Any(descriptor => descriptor.ServiceType == typeof(IFeatureManager) && descriptor.Lifetime == ServiceLifetime.Scoped))
{
builder.Services.TryAddScoped<ITargetingContextAccessor, DefaultHttpTargetingContextAccessor>();
}
else
{
builder.Services.TryAddSingleton<ITargetingContextAccessor, DefaultHttpTargetingContextAccessor>();
}

builder.AddFeatureFilter<TargetingFilter>();

return builder;
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.
//
using Microsoft.AspNetCore.Http;
using Microsoft.FeatureManagement.FeatureFilters;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Security.Claims;
using System.Threading.Tasks;

namespace Microsoft.FeatureManagement
{
/// <summary>
/// Provides a default implementation of <see cref="ITargetingContextAccessor"/> that creates <see cref="TargetingContext"/> using info from the current HTTP request.
/// </summary>
internal sealed class DefaultHttpTargetingContextAccessor : ITargetingContextAccessor
{
/// <summary>
/// The key used to store and retrieve the <see cref="TargetingContext"/> from the <see cref="HttpContext"/> items.
/// </summary>
private static object _cacheKey = new object();

private readonly IHttpContextAccessor _httpContextAccessor;

/// <summary>
/// Creates an instance of the DefaultHttpTargetingContextAccessor
/// </summary>
public DefaultHttpTargetingContextAccessor(IHttpContextAccessor httpContextAccessor)
{
_httpContextAccessor = httpContextAccessor ?? throw new ArgumentNullException(nameof(httpContextAccessor));
}

/// <summary>
/// Gets <see cref="TargetingContext"/> from the current HTTP request.
/// </summary>
public ValueTask<TargetingContext> GetContextAsync()
{
HttpContext httpContext = _httpContextAccessor.HttpContext;

//
// Try cache lookup
if (httpContext.Items.TryGetValue(_cacheKey, out object value))
{
return new ValueTask<TargetingContext>((TargetingContext)value);
}

//
// Treat user identity name as user id
ClaimsPrincipal user = httpContext.User;

string userId = user?.Identity?.Name;

//
// Treat claims of type Role as groups
IEnumerable<string> groups = httpContext.User.Claims
.Where(c => c.Type == ClaimTypes.Role)
.Select(c => c.Value)
.ToList();

TargetingContext targetingContext = new TargetingContext
{
UserId = userId,
Groups = groups
};

//
// Cache for subsequent lookup
httpContext.Items[_cacheKey] = targetingContext;

return new ValueTask<TargetingContext>(targetingContext);
}
}
}