Skip to content

Check for null in PrepareAuthorityInstanceForMsal #3338

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Apr 29, 2025
Merged

Check for null in PrepareAuthorityInstanceForMsal #3338

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 6 additions & 1 deletion src/Microsoft.Identity.Web.TokenAcquisition/MergedOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,9 +473,14 @@ internal static void UpdateMergedOptionsFromJwtBearerOptions(JwtBearerOptions jw

public void PrepareAuthorityInstanceForMsal()
{
if (string.IsNullOrEmpty(Instance))
{
return;
Copy link
Collaborator

@jmprieur jmprieur Apr 27, 2025

Choose a reason for hiding this comment

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

this is curing a symptom, not the root cause, which is that the authentication scheme is likely to be wrong, or the developer didn't provide Instance/TenantId or Authority

Maybe we should just throw a meaningful exception instead?
"Intance+TenantId or Authority not provided for authentication scheme/named options {named options}

Copy link
Collaborator

@jmprieur jmprieur Apr 29, 2025

Choose a reason for hiding this comment

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

Sorry, I had forgotten to submit the review ... days ago

Copy link
Contributor Author

@keegan-caruso keegan-caruso Apr 30, 2025

Choose a reason for hiding this comment

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

Fair point, we can revert this and throw a meaningful exception.

}

if (IsB2C && Instance.EndsWith("/tfp/", StringComparison.OrdinalIgnoreCase))
{
#if !NETSTANDARD2_0 && !NET462 && !NET472
#if NETCOREAPP
PreparedInstance = Instance.Replace("/tfp/", string.Empty, StringComparison.OrdinalIgnoreCase).TrimEnd('/') + "/";
#else
PreparedInstance = Instance.Replace("/tfp/", string.Empty).TrimEnd('/') + "/";
Expand Down
61 changes: 57 additions & 4 deletions tests/Microsoft.Identity.Web.Test/MergedOptionsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ namespace Microsoft.Identity.Web.Test
{
public class MergedOptionsTests
{
// appliation options
// application options
private readonly string _appOptionsAuthority = "microsoftIdentityApplicationOptionsAuthority";
private readonly string _appOptionsAzureRegion = "microsoftIdentityApplicationOptionsAzureRegion";
private readonly string[] _appOptionsClientCapabilities = new string[] { "microsoftIdentityApplicationOptionsClientCapabilities" };
Expand All @@ -24,7 +24,7 @@ public class MergedOptionsTests
private readonly string _appOptionsPasswordRestId = "microsoftIdentityApplicationOptionsResetPasswordPolicyId";
private readonly string _appOptionsSuSiPolicyId = "microsoftIdentityApplicationOptionsSignUpSignInPolicyId";
private readonly string _appOptionsTenantId = "microsoftIdentityApplicationOptionsTenantId";
private readonly IEnumerable<CredentialDescription> _appOptionsTokenDecyrptCreds = new CredentialDescription[] { new CredentialDescription() };
private readonly IEnumerable<CredentialDescription> _appOptionsTokenDecryptCreds = new CredentialDescription[] { new CredentialDescription() };

// MS Identity options
private readonly string _msIdentityOptionsAccessDeniedPath = "/microsoftIdentityOptionsAccessDeniedPath";
Expand Down Expand Up @@ -85,7 +85,7 @@ public void UpdateMergedOptionsFromMicrosoftIdentityApplicationOptions_Then_Defa
microsoftIdentityApplicationOptions.SendX5C = true;
microsoftIdentityApplicationOptions.SignUpSignInPolicyId = _appOptionsSuSiPolicyId;
microsoftIdentityApplicationOptions.TenantId = _appOptionsTenantId;
microsoftIdentityApplicationOptions.TokenDecryptionCredentials = _appOptionsTokenDecyrptCreds;
microsoftIdentityApplicationOptions.TokenDecryptionCredentials = _appOptionsTokenDecryptCreds;
microsoftIdentityApplicationOptions.WithSpaAuthCode = true;

// Act
Expand Down Expand Up @@ -118,7 +118,7 @@ public void UpdateMergedOptionsFromMicrosoftIdentityApplicationOptions_Then_Defa
Assert.True(mergedOptions.SendX5C);
Assert.Equal(_appOptionsSuSiPolicyId, mergedOptions.SignUpSignInPolicyId);
Assert.Equal(_appOptionsTenantId, mergedOptions.TenantId);
Assert.Equal(_appOptionsTokenDecyrptCreds, mergedOptions.TokenDecryptionCredentials!);
Assert.Equal(_appOptionsTokenDecryptCreds, mergedOptions.TokenDecryptionCredentials!);
Assert.True(mergedOptions.WithSpaAuthCode);
}

Expand Down Expand Up @@ -239,5 +239,58 @@ public void UpdateMergedOptionsFromMicrosoftIdentityOptions_ThenMicrosoftIdentit
Assert.True(mergedOptions.UseTokenLifetime);
Assert.True(mergedOptions.WithSpaAuthCode);
}

[Theory]
[InlineData("https://login.microsoftonline.com", "https://login.microsoftonline.com/")]
[InlineData("https://login.microsoftonline.com/", "https://login.microsoftonline.com/")] // Already has trailing slash
public void PrepareAuthorityInstanceForMsal_PreparesInstance_WhenInstanceIsSet(
string instance,
string expectedPreparedInstance)
{
// Arrange
var options = new MergedOptions { Instance = instance, TenantId = "common" }; // TenantId is set, so Authority shouldn't be parsed

// Act
options.PrepareAuthorityInstanceForMsal();

// Assert
Assert.Equal(expectedPreparedInstance, options.PreparedInstance);
Assert.Equal(instance, options.Instance); // Original Instance remains unchanged
}

[Fact]
public void PrepareAuthorityInstanceForMsal_DoesNothing_WhenAuthorityAndInstanceAreNull()
{
// Arrange
var options = new MergedOptions();

// Act
options.PrepareAuthorityInstanceForMsal();

// Assert
Assert.Null(options.Instance);
Assert.Null(options.TenantId);
Assert.Null(options.PreparedInstance);
}

[Fact]
public void PrepareAuthorityInstanceForMsal_DoesNotParseAuthority_WhenInstanceAndTenantIdAreSet()
{
// Arrange
var options = new MergedOptions
{
Authority = "https://login.microsoftonline.com/common", // Should be ignored
Instance = "https://login.microsoftonline.us/",
TenantId = "organizations"
};

// Act
options.PrepareAuthorityInstanceForMsal();

// Assert
Assert.Equal("https://login.microsoftonline.us/", options.Instance); // Instance remains unchanged
Assert.Equal("organizations", options.TenantId); // TenantId remains unchanged
Assert.Equal("https://login.microsoftonline.us/", options.PreparedInstance); // PreparedInstance based on original Instance
}
}
}
Loading