-
Notifications
You must be signed in to change notification settings - Fork 114
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
Adds default http targeting context accessor #416
Closed
rossgrambo
wants to merge
9
commits into
preview
from
rossgrambo/default-http-targeting-context-accessor
Closed
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
87cb9e2
Adds default targeting context accessor and aligns caches
rossgrambo e5fe69b
Adding newline and adjusted namespace
rossgrambo 387f648
Update src/Microsoft.FeatureManagement.AspNetCore/DefaultHttpTargetin…
rossgrambo 7706d4d
Merge branch 'preview' into rossgrambo/default-http-targeting-context…
rossgrambo 622095b
Adjusts to new design
rossgrambo 01ca93a
Update src/Microsoft.FeatureManagement.AspNetCore/DefaultHttpTargetin…
rossgrambo f419784
Removed unused import
rossgrambo 18a4492
Merge branch 'rossgrambo/default-http-targeting-context-accessor' of …
rossgrambo abc8ebf
Avoided exeption in HttpContext.Items read
rossgrambo File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
73 changes: 73 additions & 0 deletions
73
src/Microsoft.FeatureManagement.AspNetCore/DefaultHttpTargetingContextAccessor.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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> | ||
public sealed class DefaultHttpTargetingContextAccessor : ITargetingContextAccessor | ||
{ | ||
/// <summary> | ||
/// The key used to store and retrieve the <see cref="TargetingContext"/> from the <see cref="HttpContext"/> items. | ||
/// </summary> | ||
public const string TargetingContextLookup = $"Microsoft.FeatureManagement.TargetingContext"; | ||
|
||
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(TargetingContextLookup, 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; | ||
rossgrambo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// | ||
// Treat claims of type Role as groups | ||
IEnumerable<string> groups = httpContext.User.Claims | ||
.Where(c => c.Type == ClaimTypes.Role) | ||
.Select(c => c.Value); | ||
|
||
TargetingContext targetingContext = new TargetingContext | ||
{ | ||
UserId = userId, | ||
Groups = groups | ||
}; | ||
|
||
// | ||
// Cache for subsequent lookup | ||
httpContext.Items[TargetingContextLookup] = targetingContext; | ||
|
||
return new ValueTask<TargetingContext>(targetingContext); | ||
} | ||
} | ||
} |
40 changes: 40 additions & 0 deletions
40
src/Microsoft.FeatureManagement.AspNetCore/FeatureManagementBuilderExtensions.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
// Copyright (c) Microsoft Corporation. | ||
// Licensed under the MIT license. | ||
// | ||
using Microsoft.Extensions.DependencyInjection; | ||
using Microsoft.Extensions.DependencyInjection.Extensions; | ||
using Microsoft.FeatureManagement.FeatureFilters; | ||
using System.Collections.Generic; | ||
using System.Linq; | ||
|
||
namespace Microsoft.FeatureManagement | ||
{ | ||
/// <summary> | ||
/// Extensions used to add feature management functionality. | ||
/// </summary> | ||
public static class FeatureManagementBuilderExtensions | ||
{ | ||
/// <summary> | ||
/// Adds the <see cref="DefaultHttpTargetingContextAccessor"/> to be used for targeting and registers the targeting filter to the feature management system. | ||
/// </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) | ||
{ | ||
// | ||
// 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; | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
It's debatable who should own this cache key. It's odd to see the more generic initializer and middleware reference the cache key from a very specific targeting context accessor.
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.
Agreed... I still am unable to come up with a satisfying place to put this. I suppose the owner could be the middleware- because the middleware's whole goal is to ensure that the
TargetingContext
is available at that key.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 targeting context accessor doesn't need to share a key.
private static object _cacheKey = new object();
should work fine.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.
There's 3 classes that interact with HttpContext.Items for targeting context.
Initializer
,Middleware
, and theTargetingContextAccessor
.Initializer
depends upon theMiddleware
. TheMiddleware
depends upon aTargetingContextAccessor
existing. TheMiddleware
andInitializer
are tightly coupled- so they definitely need to share a key. TheTargetingContextAccessor
doesn't have to share the same key, but it reduces our HttpContext.Items footprint if it does- and is a clear shared cache location.So for
Initializer
andMiddleware
, we are going to have a shared key- it could be either an object ref or the string key"Microsoft.FeatureManagement.TargetingContext"
. I slightly prefer the string key since it's public (on the HttpContext.Items and as a public property). The key needs a home, and if it was just these two classes it would either be on the middleware or a shared constants class.Now the
DefaultTargetingContextAccessor
comes in- which is our default implementation. It needs a cache key as well. It could either define its own key (as a string or object ref) or used a shared constants class.I think the shared constants class makes things the most clear- as FM is defining where
TargetingContext
should live onHttpContext.Items
. I ended up not liking adding the constants class because it introduces yet another public class, and I ended up going with theDefaultTargetingContextAccessor
.