-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add IFeatureService
& Related Utilities
#37
Changes from 4 commits
335c51a
ae3cf97
49cf70c
8956cdb
96d17ad
af18b29
86a3888
4e4208d
aedfad4
9b2c6c4
6c4ac01
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,3 @@ | ||
using System.Text.Json.Nodes; | ||
|
||
namespace Bitwarden.Extensions.Hosting.Features; | ||
|
||
/// <summary> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
using System.Diagnostics.CodeAnalysis; | ||
using System.Security.Claims; | ||
using System.Text.Json; | ||
using System.Text.Json.Nodes; | ||
using Bitwarden.Extensions.Hosting.Utilities; | ||
using LaunchDarkly.Logging; | ||
using LaunchDarkly.Sdk; | ||
using LaunchDarkly.Sdk.Server; | ||
|
@@ -65,8 +65,8 @@ | |
var flagValues = new Dictionary<string, JsonValue>(); | ||
|
||
if (!flagsState.Valid) | ||
{ | ||
return flagValues; | ||
Check warning on line 69 in extensions/Bitwarden.Extensions.Hosting/src/Features/LaunchDarklyFeatureService.cs Codecov / codecov/patchextensions/Bitwarden.Extensions.Hosting/src/Features/LaunchDarklyFeatureService.cs#L68-L69
|
||
} | ||
|
||
foreach (var knownFlag in _featureFlagOptions.CurrentValue.KnownFlags) | ||
|
@@ -100,24 +100,24 @@ | |
static void AddCommon(ContextBuilder contextBuilder, HttpContext httpContext) | ||
{ | ||
if (httpContext.Request.Headers.TryGetValue("bitwarden-client-version", out var clientVersion)) | ||
{ | ||
contextBuilder.Set("client-version", clientVersion); | ||
} | ||
Check warning on line 105 in extensions/Bitwarden.Extensions.Hosting/src/Features/LaunchDarklyFeatureService.cs Codecov / codecov/patchextensions/Bitwarden.Extensions.Hosting/src/Features/LaunchDarklyFeatureService.cs#L103-L105
|
||
|
||
if (httpContext.Request.Headers.TryGetValue("device-type", out var deviceType)) | ||
{ | ||
contextBuilder.Set("device-type", deviceType); | ||
} | ||
Check warning on line 110 in extensions/Bitwarden.Extensions.Hosting/src/Features/LaunchDarklyFeatureService.cs Codecov / codecov/patchextensions/Bitwarden.Extensions.Hosting/src/Features/LaunchDarklyFeatureService.cs#L108-L110
|
||
} | ||
|
||
var httpContext = _httpContextAccessor.HttpContext; | ||
if (httpContext == null) | ||
{ | ||
_logger.LogMissingHttpContext(); | ||
return Context.Builder(AnonymousUser) | ||
.Kind(ContextKind.Default) | ||
.Anonymous(true) | ||
.Build(); | ||
Check warning on line 120 in extensions/Bitwarden.Extensions.Hosting/src/Features/LaunchDarklyFeatureService.cs Codecov / codecov/patchextensions/Bitwarden.Extensions.Hosting/src/Features/LaunchDarklyFeatureService.cs#L115-L120
|
||
} | ||
|
||
// TODO: We need to start enforcing this | ||
|
@@ -135,15 +135,15 @@ | |
} | ||
|
||
// TODO: Need to start enforcing this | ||
var organizations = httpContext.User.FindAll("organization"); | ||
Check warning on line 138 in extensions/Bitwarden.Extensions.Hosting/src/Features/LaunchDarklyFeatureService.cs Codecov / codecov/patchextensions/Bitwarden.Extensions.Hosting/src/Features/LaunchDarklyFeatureService.cs#L138
|
||
|
||
var contextBuilder = Context.Builder(subject) | ||
.Kind(ContextKind.Default) // TODO: This is not right | ||
.Set("organizations", LdValue.ArrayFrom(organizations.Select(c => LdValue.Of(c.Value)))); | ||
Check warning on line 142 in extensions/Bitwarden.Extensions.Hosting/src/Features/LaunchDarklyFeatureService.cs Codecov / codecov/patchextensions/Bitwarden.Extensions.Hosting/src/Features/LaunchDarklyFeatureService.cs#L140-L142
|
||
|
||
AddCommon(contextBuilder, httpContext); | ||
Check warning on line 144 in extensions/Bitwarden.Extensions.Hosting/src/Features/LaunchDarklyFeatureService.cs Codecov / codecov/patchextensions/Bitwarden.Extensions.Hosting/src/Features/LaunchDarklyFeatureService.cs#L144
|
||
|
||
return contextBuilder.Build(); | ||
Check warning on line 146 in extensions/Bitwarden.Extensions.Hosting/src/Features/LaunchDarklyFeatureService.cs Codecov / codecov/patchextensions/Bitwarden.Extensions.Hosting/src/Features/LaunchDarklyFeatureService.cs#L146
|
||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ๐จ Organization and service account client types are missing. |
||
|
||
|
@@ -152,6 +152,7 @@ | |
{ | ||
private readonly ILoggerFactory _loggerFactory; | ||
private readonly IHostEnvironment _hostEnvironment; | ||
private readonly VersionInfo _versionInfo; | ||
|
||
private LdClient _client; | ||
|
||
|
@@ -163,6 +164,15 @@ | |
_loggerFactory = loggerFactory; | ||
_hostEnvironment = hostEnvironment; | ||
|
||
var versionInfo = _hostEnvironment.GetVersionInfo(); | ||
|
||
if (versionInfo == null) | ||
{ | ||
throw new InvalidOperationException("Unable to attain version information for the current application."); | ||
Check warning on line 171 in extensions/Bitwarden.Extensions.Hosting/src/Features/LaunchDarklyFeatureService.cs Codecov / codecov/patchextensions/Bitwarden.Extensions.Hosting/src/Features/LaunchDarklyFeatureService.cs#L170-L171
|
||
} | ||
|
||
_versionInfo = versionInfo; | ||
|
||
BuildClient(featureFlagOptions.CurrentValue); | ||
// Subscribe to options changes. | ||
featureFlagOptions.OnChange(BuildClient); | ||
|
@@ -176,8 +186,8 @@ | |
.ApplicationInfo(Components.ApplicationInfo() | ||
.ApplicationId(_hostEnvironment.ApplicationName) | ||
.ApplicationName(_hostEnvironment.ApplicationName) | ||
.ApplicationVersion(AssemblyHelpers.GetGitHash() ?? $"v{AssemblyHelpers.GetVersion()}") | ||
// .ApplicationVersionName(AssemblyHelpers.GetVersion()) THIS DOESN'T WORK | ||
.ApplicationVersion(_versionInfo.GitHash) | ||
withinfocus marked this conversation as resolved.
Show resolved
Hide resolved
|
||
.ApplicationVersionName(_versionInfo.Version.ToString()) | ||
) | ||
.DataSource(BuildDataSource(featureFlagOptions.FlagValues)) | ||
.Events(Components.NoEvents); | ||
|
@@ -203,13 +213,13 @@ | |
flag.ValueForAll(LdValue.Of(boolValue)); | ||
} | ||
else if (int.TryParse(valueSpan, out var intValue)) | ||
{ | ||
flag.ValueForAll(LdValue.Of(intValue)); | ||
} | ||
Check warning on line 218 in extensions/Bitwarden.Extensions.Hosting/src/Features/LaunchDarklyFeatureService.cs Codecov / codecov/patchextensions/Bitwarden.Extensions.Hosting/src/Features/LaunchDarklyFeatureService.cs#L216-L218
|
||
else | ||
{ | ||
flag.ValueForAll(LdValue.Of(value)); | ||
} | ||
Check warning on line 222 in extensions/Bitwarden.Extensions.Hosting/src/Features/LaunchDarklyFeatureService.cs Codecov / codecov/patchextensions/Bitwarden.Extensions.Hosting/src/Features/LaunchDarklyFeatureService.cs#L220-L222
|
||
|
||
source.Update(flag); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
using System.Reflection; | ||
using Microsoft.Extensions.Hosting; | ||
|
||
namespace Bitwarden.Extensions.Hosting.Utilities; | ||
|
||
internal static class HostEnvironmentExtensions | ||
{ | ||
public static VersionInfo? GetVersionInfo(this IHostEnvironment hostEnvironment) | ||
{ | ||
try | ||
{ | ||
var appAssembly = Assembly.Load(new AssemblyName(hostEnvironment.ApplicationName)); | ||
var assemblyInformationalVersionAttribute = appAssembly | ||
.GetCustomAttribute<AssemblyInformationalVersionAttribute>(); | ||
|
||
if (assemblyInformationalVersionAttribute == null) | ||
{ | ||
return null; | ||
Check warning on line 18 in extensions/Bitwarden.Extensions.Hosting/src/Utilities/HostEnvironmentExtensions.cs Codecov / codecov/patchextensions/Bitwarden.Extensions.Hosting/src/Utilities/HostEnvironmentExtensions.cs#L17-L18
|
||
} | ||
|
||
if (!VersionInfo.TryParse(assemblyInformationalVersionAttribute.InformationalVersion, null, out var versionInfo)) | ||
{ | ||
return null; | ||
Check warning on line 23 in extensions/Bitwarden.Extensions.Hosting/src/Utilities/HostEnvironmentExtensions.cs Codecov / codecov/patchextensions/Bitwarden.Extensions.Hosting/src/Utilities/HostEnvironmentExtensions.cs#L22-L23
|
||
} | ||
|
||
return versionInfo; | ||
} | ||
catch (FileNotFoundException) | ||
{ | ||
return null; | ||
Check warning on line 30 in extensions/Bitwarden.Extensions.Hosting/src/Utilities/HostEnvironmentExtensions.cs Codecov / codecov/patchextensions/Bitwarden.Extensions.Hosting/src/Utilities/HostEnvironmentExtensions.cs#L28-L30
|
||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
using System.Diagnostics.CodeAnalysis; | ||
using System.Text.RegularExpressions; | ||
|
||
namespace Bitwarden.Extensions.Hosting; | ||
|
||
internal sealed partial class VersionInfo : ISpanParsable<VersionInfo> | ||
{ | ||
[GeneratedRegex("[0-9a-f]{5,40}")] | ||
private static partial Regex GitHashRegex(); | ||
|
||
private VersionInfo(Version version, string gitHash) | ||
{ | ||
Version = version; | ||
GitHash = gitHash; | ||
} | ||
|
||
public Version Version { get; } | ||
public string GitHash { get; } | ||
|
||
/// <inheritdoc /> | ||
public static VersionInfo Parse(ReadOnlySpan<char> s, IFormatProvider? provider) | ||
{ | ||
if (!TryParse(s, provider, out var result)) | ||
{ | ||
throw new FormatException(); | ||
} | ||
|
||
return result; | ||
} | ||
|
||
/// <inheritdoc /> | ||
public static VersionInfo Parse(string? s, IFormatProvider? provider) | ||
=> Parse(s.AsSpan(), provider); | ||
|
||
/// <inheritdoc /> | ||
public static bool TryParse( | ||
ReadOnlySpan<char> s, | ||
IFormatProvider? provider, | ||
[MaybeNullWhen(returnValue: false)] out VersionInfo result) | ||
{ | ||
result = null; | ||
var plusIndex = s.IndexOf('+'); | ||
|
||
if (plusIndex == -1) | ||
{ | ||
return false; | ||
} | ||
|
||
if (!Version.TryParse(s[0..plusIndex], out var version)) | ||
{ | ||
return false; | ||
} | ||
|
||
var gitHash = s[++plusIndex..]; | ||
|
||
if (!GitHashRegex().IsMatch(gitHash)) | ||
{ | ||
return false; | ||
} | ||
withinfocus marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
result = new VersionInfo(version, gitHash.ToString()); | ||
return true; | ||
} | ||
|
||
/// <inheritdoc /> | ||
public static bool TryParse(string? s, IFormatProvider? provider, [MaybeNullWhen(returnValue: false)] out VersionInfo result) | ||
=> TryParse(s.AsSpan(), provider, out result); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
namespace Bitwarden.Extensions.Hosting.Tests.Utilities; | ||
|
||
public class VersionInfoTests | ||
{ | ||
[Theory] | ||
[InlineData("1.0.0+af18b2952b5ddf910bd2f729a7c89a04b8d67084", "1.0.0", "af18b2952b5ddf910bd2f729a7c89a04b8d67084")] | ||
[InlineData("1.0.0+af18b", "1.0.0", "af18b")] | ||
public void TryParse_Works(string input, string version, string gitHash) | ||
{ | ||
var success = VersionInfo.TryParse(input, null, out var versionInfo); | ||
|
||
Assert.True(success); | ||
Assert.Equal(version, versionInfo!.Version.ToString()); | ||
Assert.Equal(gitHash, versionInfo.GitHash); | ||
} | ||
|
||
[Theory] | ||
[InlineData("")] | ||
[InlineData(null)] | ||
[InlineData("1.0.0")] | ||
[InlineData("1.0.0+")] | ||
[InlineData("1.0.0+af18")] | ||
[InlineData("1.0.0+XXXXXXX")] | ||
public void TryParse_Fails(string? input) | ||
{ | ||
Assert.False(VersionInfo.TryParse(input, null, out _)); | ||
} | ||
} |
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.
โ What's not right?
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.
So this ties into your comment below about organization and service accounts missing. This is wrong because it hard codes user and I don't want it to. I totally missed putting this in my PR description because I meant to go back to this.
I want to design this in such a way that this services doesn't have to be intimately aware of all the token types and claims structures our application has or could have down the road. Instead I want all the format to have to conform to a pattern that this understands (or just be alright with it not having as much context). It feels a lot more stable in the long run but it requires changes before you could start using this. What I did for now,
sub
andorganization
(where multiple are allowed), but I would also need a claim to indicate the context kind,sub_type
maybe? If we dont' think claims are the best place for this then maybe a request feature? https://learn.microsoft.com/en-us/aspnet/core/fundamentals/request-features?view=aspnetcore-8.0There 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.
Request features feel pretty low-level and not about business logic to me, and I think this needs to be a claim. I thought we had that already but I guess not.