Skip to content

Commit

Permalink
Add BannedApiAnalyzers to prevent use of ClaimsIdentity constructors …
Browse files Browse the repository at this point in the history
…and AppContextSwitches for fallback (#2977)

* Add BannedApiAnalyzers to prevent use of ClaimsIdentity constructors

* Add AppContextSwitches.

* Update AccountExtensions to use CsClaimsIdentity.

* Update ClaimsPrincipalFactory to use CsClaimsIdentity

* Update AppServicesAuth to use CsClaimsIdentity.

* Update tests to use CsClaimsIdentity.

* Move const.

---------

Co-authored-by: jennyf19 <jeferrie@microsoft.com>
  • Loading branch information
pmaytak and jennyf19 authored Aug 21, 2024
1 parent fcd715e commit 63bc10e
Show file tree
Hide file tree
Showing 21 changed files with 361 additions and 102 deletions.
3 changes: 3 additions & 0 deletions .editorconfig
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,9 @@ csharp_space_between_square_brackets = false

# Id Web Rules

# RS0030: Do not used banned APIs
dotnet_diagnostic.RS0030.severity = error

# CA1000: Do not declare static members on generic types
dotnet_diagnostic.CA1000.severity = error

Expand Down
10 changes: 10 additions & 0 deletions BannedSymbols.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
M:System.Security.Claims.ClaimsIdentity.#ctor(); Use Microsoft.IdentityModel.Tokens.CaseSensitiveClaimsIdentity instead.
M:System.Security.Claims.ClaimsIdentity.#ctor(System.Security.Principal.IIdentity); Use Microsoft.IdentityModel.Tokens.CaseSensitiveClaimsIdentity instead.
M:System.Security.Claims.ClaimsIdentity.#ctor(System.Collections.Generic.IEnumerable{System.Security.Claims.Claim}); Use Microsoft.IdentityModel.Tokens.CaseSensitiveClaimsIdentity instead.
M:System.Security.Claims.ClaimsIdentity.#ctor(System.String); Use Microsoft.IdentityModel.Tokens.CaseSensitiveClaimsIdentity instead.
M:System.Security.Claims.ClaimsIdentity.#ctor(System.Collections.Generic.IEnumerable{System.Security.Claims.Claim},System.String); Use Microsoft.IdentityModel.Tokens.CaseSensitiveClaimsIdentity instead.
M:System.Security.Claims.ClaimsIdentity.#ctor(System.Security.Principal.IIdentity,System.Collections.Generic.IEnumerable{System.Security.Claims.Claim}); Use Microsoft.IdentityModel.Tokens.CaseSensitiveClaimsIdentity instead.
M:System.Security.Claims.ClaimsIdentity.#ctor(System.String,System.String,System.String); Use Microsoft.IdentityModel.Tokens.CaseSensitiveClaimsIdentity instead.
M:System.Security.Claims.ClaimsIdentity.#ctor(System.Collections.Generic.IEnumerable{System.Security.Claims.Claim},System.String,System.String,System.String); Use Microsoft.IdentityModel.Tokens.CaseSensitiveClaimsIdentity instead.
M:System.Security.Claims.ClaimsIdentity.#ctor(System.Security.Principal.IIdentity,System.Collections.Generic.IEnumerable{System.Security.Claims.Claim},System.String,System.String,System.String); Use Microsoft.IdentityModel.Tokens.CaseSensitiveClaimsIdentity instead.
M:System.Security.Claims.ClaimsIdentity.#ctor(System.IO.BinaryReader); Use Microsoft.IdentityModel.Tokens.CaseSensitiveClaimsIdentity instead.
9 changes: 9 additions & 0 deletions Directory.Build.props
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@
<SystemTextJsonVersion>8.0.4</SystemTextJsonVersion>
<!--CVE-2023-29331-->
<SystemFormatsAsn1Version>8.0.1</SystemFormatsAsn1Version>
<BannedApiAnalyzersVersion>3.3.4</BannedApiAnalyzersVersion>
</PropertyGroup>

<PropertyGroup Condition="'$(TargetFramework)' == 'net9.0'">
Expand Down Expand Up @@ -188,4 +189,12 @@
<MicrosoftExtensionsDependencyInjectionVersion>2.1.0</MicrosoftExtensionsDependencyInjectionVersion>
<MicrosoftExtensionsConfigurationBinderVersion>2.2.4</MicrosoftExtensionsConfigurationBinderVersion>
</PropertyGroup>

<ItemGroup>
<PackageReference Include="Microsoft.CodeAnalysis.BannedApiAnalyzers" Version="$(BannedApiAnalyzersVersion)">
<PrivateAssets>all</PrivateAssets>
<IncludeAssets>runtime; build; native; contentfiles; analyzers</IncludeAssets>
</PackageReference>
<AdditionalFiles Include="$(MSBuildThisFileDirectory)\BannedSymbols.txt" />
</ItemGroup>
</Project>
27 changes: 3 additions & 24 deletions Microsoft.Identity.Web.sln
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "src", "src", "{1DDE1AAC-5AE
EndProject
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Solution Items", "Solution Items", "{E5695A0E-8F0A-4E89-A792-D5F062DEA93F}"
ProjectSection(SolutionItems) = preProject
.editorconfig = .editorconfig
BannedSymbols.txt = BannedSymbols.txt
Directory.Build.props = Directory.Build.props
build\Microsoft.Identity.Web-Source-Assemblies.dgml = build\Microsoft.Identity.Web-Source-Assemblies.dgml
EndProjectSection
Expand All @@ -31,6 +33,7 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.Identity.Web.OWIN
EndProject
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "tests", "tests", "{B4E72F1C-603F-437C-AAA1-153A604CD34A}"
ProjectSection(SolutionItems) = preProject
tests\.editorconfig = tests\.editorconfig
tests\Directory.Build.props = tests\Directory.Build.props
EndProjectSection
EndProject
Expand Down Expand Up @@ -254,34 +257,10 @@ Global
{BFA489C8-A8B8-44B7-9E25-6E9B56E3242C}.Debug|Any CPU.Build.0 = Debug|Any CPU
{BFA489C8-A8B8-44B7-9E25-6E9B56E3242C}.Release|Any CPU.ActiveCfg = Release|Any CPU
{BFA489C8-A8B8-44B7-9E25-6E9B56E3242C}.Release|Any CPU.Build.0 = Release|Any CPU
{16ECE807-E756-412F-AC3C-8F93DFDF98E0}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
{16ECE807-E756-412F-AC3C-8F93DFDF98E0}.Debug|Any CPU.Build.0 = Debug|Any CPU
{16ECE807-E756-412F-AC3C-8F93DFDF98E0}.Release|Any CPU.ActiveCfg = Release|Any CPU
{16ECE807-E756-412F-AC3C-8F93DFDF98E0}.Release|Any CPU.Build.0 = Release|Any CPU
{0AA95549-C7CF-4EE3-A172-71AEE83FB1B2}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
{0AA95549-C7CF-4EE3-A172-71AEE83FB1B2}.Debug|Any CPU.Build.0 = Debug|Any CPU
{0AA95549-C7CF-4EE3-A172-71AEE83FB1B2}.Release|Any CPU.ActiveCfg = Release|Any CPU
{0AA95549-C7CF-4EE3-A172-71AEE83FB1B2}.Release|Any CPU.Build.0 = Release|Any CPU
{3CA5EC4B-FC8B-429D-9271-4E79A49CB7F0}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
{3CA5EC4B-FC8B-429D-9271-4E79A49CB7F0}.Debug|Any CPU.Build.0 = Debug|Any CPU
{3CA5EC4B-FC8B-429D-9271-4E79A49CB7F0}.Release|Any CPU.ActiveCfg = Release|Any CPU
{3CA5EC4B-FC8B-429D-9271-4E79A49CB7F0}.Release|Any CPU.Build.0 = Release|Any CPU
{1273FA20-45CA-4552-8813-148C7D348EC1}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
{1273FA20-45CA-4552-8813-148C7D348EC1}.Debug|Any CPU.Build.0 = Debug|Any CPU
{1273FA20-45CA-4552-8813-148C7D348EC1}.Release|Any CPU.ActiveCfg = Release|Any CPU
{1273FA20-45CA-4552-8813-148C7D348EC1}.Release|Any CPU.Build.0 = Release|Any CPU
{E36F1B95-55D4-49AB-991D-7FD945218A8F}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
{E36F1B95-55D4-49AB-991D-7FD945218A8F}.Debug|Any CPU.Build.0 = Debug|Any CPU
{E36F1B95-55D4-49AB-991D-7FD945218A8F}.Release|Any CPU.ActiveCfg = Release|Any CPU
{E36F1B95-55D4-49AB-991D-7FD945218A8F}.Release|Any CPU.Build.0 = Release|Any CPU
{BF89A3BC-CC1F-4508-8C32-0E4C74E75FAA}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
{BF89A3BC-CC1F-4508-8C32-0E4C74E75FAA}.Debug|Any CPU.Build.0 = Debug|Any CPU
{BF89A3BC-CC1F-4508-8C32-0E4C74E75FAA}.Release|Any CPU.ActiveCfg = Release|Any CPU
{BF89A3BC-CC1F-4508-8C32-0E4C74E75FAA}.Release|Any CPU.Build.0 = Release|Any CPU
{F1A03DB7-254A-498B-9A52-44ACDDF9E48B}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
{F1A03DB7-254A-498B-9A52-44ACDDF9E48B}.Debug|Any CPU.Build.0 = Debug|Any CPU
{F1A03DB7-254A-498B-9A52-44ACDDF9E48B}.Release|Any CPU.ActiveCfg = Release|Any CPU
{F1A03DB7-254A-498B-9A52-44ACDDF9E48B}.Release|Any CPU.Build.0 = Release|Any CPU
{2D24799E-064D-4C19-8FB0-482E8CA0D969}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
{2D24799E-064D-4C19-8FB0-482E8CA0D969}.Debug|Any CPU.Build.0 = Debug|Any CPU
{2D24799E-064D-4C19-8FB0-482E8CA0D969}.Release|Any CPU.ActiveCfg = Release|Any CPU
Expand Down
3 changes: 3 additions & 0 deletions benchmark/Directory.Build.props
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,7 @@
<BenchmarkDotNetDiagnosticsWindowsVersion>0.13.12</BenchmarkDotNetDiagnosticsWindowsVersion>
</PropertyGroup>

<ItemGroup>
<AdditionalFiles Include="$(MSBuildThisFileDirectory)..\BannedSymbols.txt" />
</ItemGroup>
</Project>
19 changes: 16 additions & 3 deletions src/Microsoft.Identity.Web/AccountExtensions.cs
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System;
using System.Security.Claims;
using Microsoft.Identity.Client;
using Microsoft.IdentityModel.Tokens;

namespace Microsoft.Identity.Web
{
Expand All @@ -22,10 +22,23 @@ public static ClaimsPrincipal ToClaimsPrincipal(this IAccount account)
{
_ = Throws.IfNull(account);

ClaimsIdentity identity = new ClaimsIdentity(new[]
ClaimsIdentity identity;

if (AppContextSwitches.UseClaimsIdentityType)
{
#pragma warning disable RS0030 // Do not use banned APIs
identity = new ClaimsIdentity(new[]
{
new Claim(ClaimTypes.Upn, account.Username),
});
#pragma warning restore RS0030 // Do not use banned APIs
} else
{
identity = new CaseSensitiveClaimsIdentity(new[]
{
new Claim(ClaimTypes.Upn, account.Username),
});
});
}

if (!string.IsNullOrEmpty(account.HomeAccountId?.ObjectId))
{
Expand Down
41 changes: 41 additions & 0 deletions src/Microsoft.Identity.Web/AppContextSwitches.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System;
using System.Security.Claims;
using Microsoft.IdentityModel.Tokens;

namespace Microsoft.Identity.Web
{
/// <summary>
/// Identifiers used for switching between different app behaviors within the library.
/// </summary>
/// <remarks>
/// This library uses <see cref="System.AppContext" /> to turn on or off certain API behavioral
/// changes that might have an effect on application compatibility. This class defines the set of switches that are
/// available to modify library behavior. Setting a switch's value can be
/// done programmatically through the <see cref="System.AppContext.SetSwitch" /> method, or through other means such as
/// setting it through MSBuild, app configuration, or registry settings. These alternate methods are described in the
/// <see cref="System.AppContext.SetSwitch" /> documentation.
/// </remarks>
internal static class AppContextSwitches
{
/// <summary>
/// Enables a fallback to the previous behavior of using <see cref="ClaimsIdentity"/> instead of <see cref="CaseSensitiveClaimsIdentity"/> globally.
/// </summary>
internal const string UseClaimsIdentityTypeSwitchName = "Microsoft.IdentityModel.Tokens.UseClaimsIdentityType";

private static bool? s_useClaimsIdentityType;

internal static bool UseClaimsIdentityType => s_useClaimsIdentityType ??= (AppContext.TryGetSwitch(UseClaimsIdentityTypeSwitchName, out bool useClaimsIdentityType) && useClaimsIdentityType);

/// <summary>
/// Used for testing to reset all switches to its default value.
/// </summary>
internal static void ResetState()
{
AppContext.SetSwitch(UseClaimsIdentityTypeSwitchName, false);
s_useClaimsIdentityType = null;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using System.Security.Claims;
using Microsoft.Extensions.Primitives;
using Microsoft.IdentityModel.JsonWebTokens;
using Microsoft.IdentityModel.Tokens;

namespace Microsoft.Identity.Web
{
Expand Down Expand Up @@ -185,11 +186,24 @@ internal static string? Issuer
JsonWebToken jsonWebToken = new JsonWebToken(idToken);
bool isAadV1Token = jsonWebToken.Claims
.Any(c => c.Type == Constants.Version && c.Value == Constants.V1);
claimsPrincipal = new ClaimsPrincipal(new ClaimsIdentity(
jsonWebToken.Claims,
idp,
isAadV1Token ? Constants.NameClaim : Constants.PreferredUserName,
ClaimConstants.Roles));
if (AppContextSwitches.UseClaimsIdentityType)
{
#pragma warning disable RS0030 // Do not use banned APIs
claimsPrincipal = new ClaimsPrincipal(new ClaimsIdentity(
jsonWebToken.Claims,
idp,
isAadV1Token ? Constants.NameClaim : Constants.PreferredUserName,
ClaimConstants.Roles));
#pragma warning restore RS0030 // Do not use banned APIs
}
else
{
claimsPrincipal = new ClaimsPrincipal(new CaseSensitiveClaimsIdentity(
jsonWebToken.Claims,
idp,
isAadV1Token ? Constants.NameClaim : Constants.PreferredUserName,
ClaimConstants.Roles));
}
}
else
{
Expand Down
52 changes: 40 additions & 12 deletions src/Microsoft.Identity.Web/ClaimsPrincipalFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the MIT License.

using System.Security.Claims;
using Microsoft.IdentityModel.Tokens;

namespace Microsoft.Identity.Web
{
Expand Down Expand Up @@ -37,12 +38,26 @@ public static class ClaimsPrincipalFactory
/// </example>
public static ClaimsPrincipal FromHomeTenantIdAndHomeObjectId(string homeTenantId, string homeObjectId)
{
return new ClaimsPrincipal(
new ClaimsIdentity(new[]
{
new Claim(ClaimConstants.UniqueTenantIdentifier, homeTenantId),
new Claim(ClaimConstants.UniqueObjectIdentifier, homeObjectId),
}));
if (AppContextSwitches.UseClaimsIdentityType)
{
#pragma warning disable RS0030 // Do not use banned APIs
return new ClaimsPrincipal(
new ClaimsIdentity(new[]
{
new Claim(ClaimConstants.UniqueTenantIdentifier, homeTenantId),
new Claim(ClaimConstants.UniqueObjectIdentifier, homeObjectId),
}));
#pragma warning restore RS0030 // Do not use banned APIs
}
else
{
return new ClaimsPrincipal(
new CaseSensitiveClaimsIdentity(new[]
{
new Claim(ClaimConstants.UniqueTenantIdentifier, homeTenantId),
new Claim(ClaimConstants.UniqueObjectIdentifier, homeObjectId),
}));
}
}

/// <summary>
Expand Down Expand Up @@ -72,12 +87,25 @@ public static ClaimsPrincipal FromHomeTenantIdAndHomeObjectId(string homeTenantI
/// </example>
public static ClaimsPrincipal FromTenantIdAndObjectId(string tenantId, string objectId)
{
return new ClaimsPrincipal(
new ClaimsIdentity(new[]
{
new Claim(ClaimConstants.Tid, tenantId),
new Claim(ClaimConstants.Oid, objectId),
}));
if (AppContextSwitches.UseClaimsIdentityType)
{
#pragma warning disable RS0030 // Do not use banned APIs
return new ClaimsPrincipal(
new ClaimsIdentity(new[]
{
new Claim(ClaimConstants.Tid, tenantId),
new Claim(ClaimConstants.Oid, objectId),
}));
#pragma warning restore RS0030 // Do not use banned APIs
} else
{
return new ClaimsPrincipal(
new CaseSensitiveClaimsIdentity(new[]
{
new Claim(ClaimConstants.Tid, tenantId),
new Claim(ClaimConstants.Oid, objectId),
}));
}
}
}
}
3 changes: 3 additions & 0 deletions tests/.editorconfig
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
# Code files
[*.{cs,vb}]

# RS0030: Do not used banned APIs
dotnet_diagnostic.RS0030.severity = error

# SA0001: XML comment analysis disabled
dotnet_diagnostic.SA0001.severity = none

Expand Down
7 changes: 7 additions & 0 deletions tests/DevApps/Directory.Build.props
Original file line number Diff line number Diff line change
Expand Up @@ -27,4 +27,11 @@
<SystemDrawingCommon>9.0.0-preview.2.24128.3</SystemDrawingCommon>
</PropertyGroup>

<ItemGroup>
<PackageReference Include="Microsoft.CodeAnalysis.BannedApiAnalyzers" Version="3.3.4">
<PrivateAssets>all</PrivateAssets>
<IncludeAssets>runtime; build; native; contentfiles; analyzers</IncludeAssets>
</PackageReference>
<AdditionalFiles Include="$(MSBuildThisFileDirectory)..\..\BannedSymbols.txt" />
</ItemGroup>
</Project>
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Http.Features;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.IdentityModel.Tokens;

namespace Microsoft.Identity.Web.Test.Common.TestHelpers
{
Expand Down Expand Up @@ -35,7 +36,7 @@ public static HttpContext CreateHttpContext(
var httpContext = CreateHttpContext();

httpContext.User = new ClaimsPrincipal(
new ClaimsIdentity(new Claim[]
new CaseSensitiveClaimsIdentity(new Claim[]
{
new Claim(ClaimConstants.Scope, string.Join(' ', userScopes)),
new Claim(ClaimConstants.Roles, string.Join(' ', userRoles)),
Expand Down
Loading

0 comments on commit 63bc10e

Please sign in to comment.