Skip to content
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

feat: add requirement for denying guest users #2179

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
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 @@ -68,6 +68,21 @@ public static class ClaimsPrincipalExtensions
/// </summary>
private const string NameIdentifierId = "http://schemas.xmlsoap.org/ws/2005/05/identity/claims/nameidentifier";

/// <summary>
/// Issuer claim: "iss".
/// </summary>
private const string Iss = "iss";

/// <summary>
/// Identity Provider claim: "idp".
/// </summary>
private const string Idp = "idp";

/// <summary>
/// Old Identity Provider claim: "http://schemas.microsoft.com/identity/claims/identityprovider".
/// </summary>
private const string IdentityProvider = "http://schemas.microsoft.com/identity/claims/identityprovider";

private const string MsaTenantId = "9188040d-6c67-4c5b-b112-36a304b66dad";
private const string Consumers = "consumers";
private const string Organizations = "organizations";
Expand Down Expand Up @@ -116,6 +131,17 @@ public static class ClaimsPrincipalExtensions
return GetClaimValue(claimsPrincipal, Tid, TenantId);
}

/// <summary>
/// Gets the Identity Provider associated with the <see cref="ClaimsPrincipal"/>.
/// </summary>
/// <param name="claimsPrincipal">The <see cref="ClaimsPrincipal"/> from which to retrieve the tenant ID.</param>
/// <returns>Tenant Identity Provider used to log in the user, or <c>null</c> if it cannot be found.</returns>
/// <remarks>This method returns the Identity Provider both in case the developer has enabled or not claims mapping, and if none is present, it returns the Issuer.</remarks>
public static string? GetIdentityProvider(this ClaimsPrincipal claimsPrincipal)
{
return GetClaimValue(claimsPrincipal, Idp, IdentityProvider, Iss);
}

/// <summary>
/// Gets the login-hint associated with a <see cref="ClaimsPrincipal"/>.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System.Security.Claims;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Authorization;

namespace Microsoft.Identity.Web
{
/// <summary>
/// Handler for the <see cref="DenyGuestsAuthorizationRequirement"/>.
/// Will prioritize checking the "acct" claim. If it is not present, it will check the "iss" and "idp" claims.
/// </summary>
internal class DenyGuestsAuthorizationsHandler : AuthorizationHandler<DenyGuestsAuthorizationRequirement>
{
/// <summary>
/// Account status claim: "acct"
/// "0" indicates the user is a member of the tenant.
/// "1" indicates the user is a guest of the tenant.
/// </summary>
private const string Acct = "acct";

/// <summary>
/// Tenant Member value of the Account Status claim: "0"
/// </summary>
private const string TenantMember = "0";

/// <summary>
/// Issuer claim: "iss"
/// </summary>
private const string Iss = "iss";

/// <summary>
/// Makes a decision if authorization is allowed based on a specific requirement.
/// </summary>
/// <param name="context">AuthorizationHandlerContext.</param>
/// <param name="requirement">Deny Guests authorization requirement.</param>
/// <returns>Task.</returns>
protected override Task HandleRequirementAsync(
AuthorizationHandlerContext context,
DenyGuestsAuthorizationRequirement requirement)
{
_ = Throws.IfNull(context);

_ = Throws.IfNull(requirement);

// acct is an optional claim
// if it is present, it dictates wether the user is a guest or not
var acct = context.User.FindFirstValue(Acct);

if (!string.IsNullOrEmpty(acct))
{
if (acct == TenantMember)
{
context.Succeed(requirement);
}

return Task.CompletedTask;
}

// if acct is not present
// we can use the iss and idp claim to determine if the user is a guest
var iss = context.User.FindFirstValue(Iss);
var idp = context.User.GetIdentityProvider();

if (!string.IsNullOrEmpty(iss) && iss == idp)
{
context.Succeed(requirement);
return Task.CompletedTask;
}

return Task.CompletedTask;
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using Microsoft.AspNetCore.Authorization;

namespace Microsoft.Identity.Web
{
/// <summary>
/// Implements an <see cref="IAuthorizationRequirement"/>
/// which requires the current user to be a member of the tenant.
/// </summary>
public class DenyGuestsAuthorizationRequirement : IAuthorizationRequirement
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not locked on the naming of this. I see many requirements are prefixed with Require, but RequireTenantMembers or RequireNonGuests sounds weird and isn't as explicit in what it tries to achieve.

{
}
}
29 changes: 29 additions & 0 deletions src/Microsoft.Identity.Web/Policy/DenyGuestsExtensions.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using Microsoft.AspNetCore.Authorization;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.DependencyInjection.Extensions;

namespace Microsoft.Identity.Web
{
/// <summary>
/// Extensions for building the deny guest requirement during application startup.
/// </summary>
public static class DenyGuestsExtensions
{
/// <summary>
/// This method adds support for the deny guests requirement.
/// </summary>
/// <param name="services">The services being configured.</param>
/// <returns>Services.</returns>
public static IServiceCollection AddDenyGuestsAuthorization(this IServiceCollection services)
{
services.AddAuthorization();

services.TryAddEnumerable(ServiceDescriptor.Singleton<IAuthorizationHandler, DenyGuestsAuthorizationsHandler>());

return services;
}
}
}
17 changes: 15 additions & 2 deletions src/Microsoft.Identity.Web/Policy/PolicyBuilderExtensions.cs
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System;
using System.Collections.Generic;
using Azure.Core;
using Microsoft.AspNetCore.Authorization;

namespace Microsoft.Identity.Web
Expand All @@ -21,6 +19,21 @@ namespace Microsoft.Identity.Web
/// </example>
public static class PolicyBuilderExtensions
{
/// <summary>
/// Adds a <see cref="DenyGuestsAuthorizationRequirement"/> to the current instance which requires
/// that the current user is a member of the tenant.
/// </summary>
/// <param name="authorizationPolicyBuilder">Used for building policies during application startup.</param>
/// <returns>A reference to this instance after the operation has completed.</returns>
public static AuthorizationPolicyBuilder DenyGuests(this AuthorizationPolicyBuilder authorizationPolicyBuilder)
{
_ = Throws.IfNull(authorizationPolicyBuilder);

authorizationPolicyBuilder.Requirements.Add(new DenyGuestsAuthorizationRequirement());

return authorizationPolicyBuilder;
}

/// <summary>
/// Adds a <see cref="ScopeAuthorizationRequirement"/> to the current instance which requires
/// that the current user has the specified claim and that the claim value must be one of the allowed values.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ private static void AddMicrosoftIdentityWebApiImplementation(
builder.Services.AddHttpContextAccessor();
builder.Services.AddHttpClient();
builder.Services.TryAddSingleton<MicrosoftIdentityIssuerValidatorFactory>();
builder.Services.AddDenyGuestsAuthorization();
builder.Services.AddRequiredScopeAuthorization();
builder.Services.AddRequiredScopeOrAppPermissionAuthorization();
builder.Services.AddOptions<AadIssuerValidatorOptions>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ public static class TestConstants
public const string AadIssuer = AadInstance + "/" + TenantIdAsGuid + "/v2.0";
public const string UsGovIssuer = "https://login.microsoftonline.us/" + UsGovTenantId + "/v2.0";
public const string UsGovTenantId = "72f988bf-86f1-41af-91ab-2d7cd011db47";
public const string V1Issuer = "https://sts.windows.net/f645ad92-e38d-4d1a-b510-d1b09a74a8ca/";
public const string V1Issuer = "https://sts.windows.net/" + TenantIdAsGuid + "/";
public const string GraphBaseUrlBeta = "https://graph.microsoft.com/beta";

// B2C
Expand Down
169 changes: 169 additions & 0 deletions tests/Microsoft.Identity.Web.Test/Resource/DenyGuestsPolicyTests.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,169 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Security.Claims;
using System.Text;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Authorization;
using Microsoft.Identity.Web.Test.Common;
using Xunit;

namespace Microsoft.Identity.Web.Test.Resource
{
/*
* The purpose of this test class is to verify the following scenarios:
* V1 and V2 Azure Tokens
* Check optional acct claim if its present
* Application Tokens should succeed
* Tenant Members should succeed
* Guest Users should fail
*/
public class DenyGuestsPolicyTests
{
private readonly DenyGuestsAuthorizationsHandler _handler = new();

private const string IdentityProvider = "http://schemas.microsoft.com/identity/claims/identityprovider";
private const string Idp = "idp";
private const string Iss = "iss";
private const string Acct = "acct";

private const string GuestIdentityProvider = $"https://sts.windows.net/{TestConstants.UsGovTenantId}/";

[Fact]
public async Task DenyGuestsPolicy_TenantMembersWithAcctClaim_SucceedsAsync()
{
// Arrange
var user = CreateClaimsPrincipal(new Claim[] { new Claim(Acct, "0") });

var context = CreateAuhtorizationHandlerContext(user);

// Act
await _handler.HandleAsync(context);

// Assert
Assert.True(context.HasSucceeded);
}

[Fact]
public async Task DenyGuestsPolicy_GuestUsersWithAcctClaim_FailsAsync()
{
// Arrange
var user = CreateClaimsPrincipal(new Claim[] { new Claim(Acct, "1") });

var context = CreateAuhtorizationHandlerContext(user);

// Act
await _handler.HandleAsync(context);

// Assert
Assert.False(context.HasSucceeded);
}

// V1 Application Tokens
// Test both with and without idp transformation
[Theory]
[InlineData(Idp)]
[InlineData(IdentityProvider)]
public async Task DenyGuestPolicy_V1ApplicationTokens_SucceedsAsync(string idpClaimName)
{
// Arrange
// V1 Application Tokens includes both IDP and Issuer with the same value
var user = CreateClaimsPrincipal(new Claim[]
{
new Claim(Iss, TestConstants.V1Issuer),
new Claim(idpClaimName, TestConstants.V1Issuer)
});

var context = CreateAuhtorizationHandlerContext(user);

// Act
await _handler.HandleAsync(context);

// Assert
Assert.True(context.HasSucceeded);
}

// V2 Application Tokens
[Fact]
public async Task DenyGuestPolicy_V2ApplicationTokens_SucceedsAsync()
{
// Arrange
// V2 Application Tokens does not contain the IDP claim
var user = CreateClaimsPrincipal(new Claim[]
{
new Claim(Iss, TestConstants.AadIssuer)
});

var context = CreateAuhtorizationHandlerContext(user);

// Act
await _handler.HandleAsync(context);

// Assert
Assert.True(context.HasSucceeded);
}

// Tenant Members
// This should work with both v1 and v2 issuer
[Theory]
[InlineData(TestConstants.AadIssuer)]
[InlineData(TestConstants.V1Issuer)]
public async Task DenyGuestPolicy_TenantMemberTokens_SucceedsAsync(string issuer)
{
// Arrange
// Tenant Member Tokens does not contain the IDP claim in either version
var user = CreateClaimsPrincipal(new Claim[]
{
new Claim(Iss, issuer)
});

var context = CreateAuhtorizationHandlerContext(user);

// Act
await _handler.HandleAsync(context);

// Assert
Assert.True(context.HasSucceeded);
}

// Guest Users
// Test both token versions with and without idp transformation
[Theory]
[InlineData(TestConstants.AadIssuer, Idp)]
[InlineData(TestConstants.AadIssuer, IdentityProvider)]
[InlineData(TestConstants.V1Issuer, Idp)]
[InlineData(TestConstants.V1Issuer, IdentityProvider)]
public async Task DenyGuestPolicy_GuestUserTokens_FailsAsync(string issuer, string idpClaimName)
{
// Arrange
var user = CreateClaimsPrincipal(new Claim[]
{
new Claim(Iss, issuer),
// The guest IDP is the same in both versions
new Claim(idpClaimName, GuestIdentityProvider)
});

var context = CreateAuhtorizationHandlerContext(user);

// Act
await _handler.HandleAsync(context);

// Assert
Assert.False(context.HasSucceeded);
}

private static ClaimsPrincipal CreateClaimsPrincipal(IEnumerable<Claim> claims)
{
return new ClaimsPrincipal(new ClaimsIdentity(claims));
}

private AuthorizationHandlerContext CreateAuhtorizationHandlerContext(ClaimsPrincipal user)
{
var requirement = new DenyGuestsAuthorizationRequirement();
return new AuthorizationHandlerContext(new[] { requirement }, user, null);
}
}
}