Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 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>
/// Adds the <see cref="DefaultHttpTargetingContextAccessor"/> to be used for targeting and registers the targeting filter to the feature management system.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That's an internal type. It shouldn't show up in a public method summary. The compiler allows it?

@jimmyca15 jimmyca15 Jul 10, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since DefaultHttpTargetingContextAccessor is internal, users won't see its class summary so some of those details would be useful here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point- how about:

Suggested change
/// Adds the <see cref="DefaultHttpTargetingContextAccessor"/> to be used for targeting and registers the targeting filter to the feature management system.
/// Adds the default ASP.NET <see cref="ITargetingContextAccessor"/> to be used for targeting and registers the targeting filter to the feature management system.

?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would say something like.

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
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

@rossgrambo rossgrambo Jul 2, 2024

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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,73 @@
// 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);
Comment thread
rossgrambo marked this conversation as resolved.
Outdated

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

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

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