Skip to content

Commit

Permalink
Address review comments
Browse files Browse the repository at this point in the history 
Add test cases
  • Loading branch information
@msJinLei
msJinLei committed Jul 14, 2023
1 parent 5003b71 commit 6e8e214
Show file tree
Hide file tree
Showing 7 changed files with 93 additions and 70 deletions.
37 changes: 14 additions & 23 deletions sdk/identity/Azure.Identity/src/Credentials/BrowserCustomizationOptions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,6 @@ public class BrowserCustomizationOptions

internal SystemWebViewOptions SystemBrowserOptions;

internal EmbeddedWebViewOptions EmbeddedBrowserOptions;

private SystemWebViewOptions systemWebViewOptions
{
get
Expand All @@ -32,46 +30,39 @@ private SystemWebViewOptions systemWebViewOptions
}
}

private EmbeddedWebViewOptions embeddedWebViewOptions
/// <summary>
/// Property to set HtmlMessageSuccess of SystemWebViewOptions from MSAL,
/// which the browser will show to the user when the user finishes authenticating successfully.
/// </summary>
public string HtmlMessageSuccess
{
get
{
EmbeddedBrowserOptions ??= new EmbeddedWebViewOptions();
return EmbeddedBrowserOptions;
return systemWebViewOptions.HtmlMessageSuccess;
}
}

/// <summary>
/// Property to set HtmlMessageSuccess of SystemWebViewOptions.
/// </summary>
public string HtmlMessageSuccess
{
set
{
systemWebViewOptions.HtmlMessageSuccess = value;
}
}

/// <summary>
/// Property to set HtmlMessageError of SystemWebViewOptions.
/// Property to set HtmlMessageError of SystemWebViewOptions from MSAL,
/// which the browser will show to the user when the user finishes authenticating, but an error occurred.
/// You can use a string format e.g. "An error has occurred: {0} details: {1}".
/// </summary>
public string HtmlMessageError
{
get
{
return systemWebViewOptions.HtmlMessageError;
}

set
{
systemWebViewOptions.HtmlMessageError = value;
}
}

/// <summary>
/// Default constructor for <see cref="BrowserCustomizationOptions"/>. Will keep the framework default behavior,
/// when you use this constructor.
/// </summary>
public BrowserCustomizationOptions(bool useEmbeddedWebView = false)
{
UseEmbeddedWebView = null;
SystemBrowserOptions = null;
EmbeddedBrowserOptions = null;
}
}
}
10 changes: 3 additions & 7 deletions sdk/identity/Azure.Identity/src/Credentials/InteractiveBrowserCredential.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,7 @@ public class InteractiveBrowserCredential : TokenCredential
internal string[] AdditionallyAllowedTenantIds { get; }
internal string ClientId { get; }
internal string LoginHint { get; }
internal bool? UseEmbeddedWebView { get; }
internal SystemWebViewOptions SystemBrowserOptions { get; }
internal EmbeddedWebViewOptions EmbeddedBrowserOptions {get;}
internal BrowserCustomizationOptions BrowserCustomizedOptions { get; }
internal MsalPublicClient Client { get; }
internal CredentialPipeline Pipeline { get; }
internal bool DisableAutomaticAuthentication { get; }
Expand Down Expand Up @@ -91,9 +89,7 @@ internal InteractiveBrowserCredential(string tenantId, string clientId, TokenCre
Client = client ?? new MsalPublicClient(Pipeline, tenantId, clientId, redirectUrl, options);
AdditionallyAllowedTenantIds = TenantIdResolver.ResolveAddionallyAllowedTenantIds((options as ISupportsAdditionallyAllowedTenants)?.AdditionallyAllowedTenants);
Record = (options as InteractiveBrowserCredentialOptions)?.AuthenticationRecord;
UseEmbeddedWebView = (options as InteractiveBrowserCredentialOptions)?.BrowserCustomizationOptions.UseEmbeddedWebView ?? null;
SystemBrowserOptions = (options as InteractiveBrowserCredentialOptions)?.BrowserCustomizationOptions.SystemBrowserOptions ?? null;
EmbeddedBrowserOptions = (options as InteractiveBrowserCredentialOptions)?.BrowserCustomizationOptions.EmbeddedBrowserOptions ?? null;
BrowserCustomizedOptions = (options as InteractiveBrowserCredentialOptions)?.BrowserCustomizedOptions;
}

/// <summary>
Expand Down Expand Up @@ -238,7 +234,7 @@ private async Task<AccessToken> GetTokenViaBrowserLoginAsync(TokenRequestContext

var tenantId = TenantIdResolver.Resolve(TenantId ?? Record?.TenantId, context, AdditionallyAllowedTenantIds);
AuthenticationResult result = await Client
.AcquireTokenInteractiveAsync(context.Scopes, context.Claims, prompt, LoginHint, tenantId, context.IsCaeEnabled, UseEmbeddedWebView, SystemBrowserOptions, EmbeddedBrowserOptions, async, cancellationToken)
.AcquireTokenInteractiveAsync(context.Scopes, context.Claims, prompt, LoginHint, tenantId, context.IsCaeEnabled, BrowserCustomizedOptions, async, cancellationToken)
.ConfigureAwait(false);

Record = new AuthenticationRecord(result, ClientId);
Expand Down
18 changes: 1 addition & 17 deletions sdk/identity/Azure.Identity/src/Credentials/InteractiveBrowserCredentialOptions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,25 +66,9 @@ public string TenantId
/// <inheritdoc/>
public bool DisableInstanceDiscovery { get; set; }

private BrowserCustomizationOptions _browserCustomizationOptions = null;

/// <summary>
/// The options for customizing the browser for interactive authentication.
/// </summary>
public BrowserCustomizationOptions BrowserCustomizationOptions
{
get
{
if (_browserCustomizationOptions == null)
{
_browserCustomizationOptions = new BrowserCustomizationOptions();
}
return _browserCustomizationOptions;
}
set
{
_browserCustomizationOptions = value;
}
}
public BrowserCustomizationOptions BrowserCustomizedOptions { get; set; }
}
}
29 changes: 14 additions & 15 deletions sdk/identity/Azure.Identity/src/MsalPublicClient.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Linq;
using System.Threading;
using System.Threading.Tasks;

using Microsoft.Identity.Client;

namespace Azure.Identity
Expand Down Expand Up @@ -123,7 +124,7 @@ protected virtual async ValueTask<AuthenticationResult> AcquireTokenSilentCoreAs
.ConfigureAwait(false);
}

public async ValueTask<AuthenticationResult> AcquireTokenInteractiveAsync(string[] scopes, string claims, Prompt prompt, string loginHint, string tenantId, bool enableCae, bool? useEmbeddedWebView, SystemWebViewOptions systemWebViewOptions, EmbeddedWebViewOptions embeddedWebViewOptions, bool async, CancellationToken cancellationToken)
public async ValueTask<AuthenticationResult> AcquireTokenInteractiveAsync(string[] scopes, string claims, Prompt prompt, string loginHint, string tenantId, bool enableCae, BrowserCustomizationOptions browserOptions, bool async, CancellationToken cancellationToken)
{
if (Thread.CurrentThread.GetApartmentState() == ApartmentState.STA && !IdentityCompatSwitches.DisableInteractiveBrowserThreadpoolExecution)
{
Expand All @@ -137,7 +138,7 @@ public async ValueTask<AuthenticationResult> AcquireTokenInteractiveAsync(string
#pragma warning disable AZC0102 // Do not use GetAwaiter().GetResult().
return Task.Run(async () =>
{
var result = await AcquireTokenInteractiveCoreAsync(scopes, claims, prompt, loginHint, tenantId, enableCae, useEmbeddedWebView, systemWebViewOptions, embeddedWebViewOptions, true, cancellationToken).ConfigureAwait(false);
var result = await AcquireTokenInteractiveCoreAsync(scopes, claims, prompt, loginHint, tenantId, enableCae, browserOptions, true, cancellationToken).ConfigureAwait(false);
LogAccountDetails(result);
return result;
}).GetAwaiter().GetResult();
Expand All @@ -146,13 +147,12 @@ public async ValueTask<AuthenticationResult> AcquireTokenInteractiveAsync(string

AzureIdentityEventSource.Singleton.InteractiveAuthenticationExecutingInline();

var result = await AcquireTokenInteractiveCoreAsync(scopes, claims, prompt, loginHint, tenantId, enableCae, useEmbeddedWebView, systemWebViewOptions, embeddedWebViewOptions, async, cancellationToken).ConfigureAwait(false);
var result = await AcquireTokenInteractiveCoreAsync(scopes, claims, prompt, loginHint, tenantId, enableCae, browserOptions, async, cancellationToken).ConfigureAwait(false);
LogAccountDetails(result);
return result;
}

protected virtual async ValueTask<AuthenticationResult> AcquireTokenInteractiveCoreAsync(string[] scopes, string claims, Prompt prompt, string loginHint, string tenantId, bool enableCae, bool? useEmbeddedWebView, SystemWebViewOptions systemWebViewOptions, EmbeddedWebViewOptions embeddedWebViewOptions,
bool async, CancellationToken cancellationToken)
protected virtual async ValueTask<AuthenticationResult> AcquireTokenInteractiveCoreAsync(string[] scopes, string claims, Prompt prompt, string loginHint, string tenantId, bool enableCae, BrowserCustomizationOptions browserOptions, bool async, CancellationToken cancellationToken)
{
IPublicClientApplication client = await GetClientAsync(enableCae, async, cancellationToken).ConfigureAwait(false);

Expand All @@ -169,17 +169,16 @@ protected virtual async ValueTask<AuthenticationResult> AcquireTokenInteractiveC
{
builder.WithAuthority(AuthorityHost.AbsoluteUri, tenantId);
}
if (useEmbeddedWebView != null)
{
builder.WithUseEmbeddedWebView(useEmbeddedWebView.Value);
}
if (systemWebViewOptions != null)
if (browserOptions != null)
{
builder.WithSystemWebViewOptions(systemWebViewOptions);
}
if (embeddedWebViewOptions != null)
{
builder.WithEmbeddedWebViewOptions(embeddedWebViewOptions);
if (browserOptions.UseEmbeddedWebView.HasValue)
{
builder.WithUseEmbeddedWebView(browserOptions.UseEmbeddedWebView.Value);
}
if (browserOptions.SystemBrowserOptions != null)
{
builder.WithSystemWebViewOptions(browserOptions.SystemBrowserOptions);
}
}
return await builder
.ExecuteAsync(async, cancellationToken)
Expand Down
2 changes: 1 addition & 1 deletion sdk/identity/Azure.Identity/tests/CredentialTestBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -448,7 +448,7 @@ public void TestSetup(TokenCredentialOptions options = null)
// Assert.AreEqual(tenantId, tId);
return publicResult;
};
mockPublicMsalClient.InteractiveAuthFactory = (_, _, _, _, tenant, _, _) =>
mockPublicMsalClient.InteractiveAuthFactory = (_, _, _, _, tenant, _, _, _) =>
{
Assert.AreEqual(expectedTenantId, tenant, "TenantId passed to msal should match");
return result;
Expand Down
57 changes: 56 additions & 1 deletion sdk/identity/Azure.Identity/tests/InteractiveBrowserCredentialTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using Azure.Core.TestFramework;
using Azure.Identity.Tests.Mock;
using Microsoft.Identity.Client;

using NUnit.Framework;

namespace Azure.Identity.Tests
Expand Down Expand Up @@ -186,7 +187,7 @@ public async Task LoginHint([Values(null, "[email protected]")] string loginHint
{
var mockMsalClient = new MockMsalPublicClient
{
InteractiveAuthFactory = (_, _, prompt, hintArg, _, _, _) =>
InteractiveAuthFactory = (_, _, prompt, hintArg, _, _, _, _) =>
{
Assert.AreEqual(loginHint == null ? Prompt.SelectAccount : Prompt.NoPrompt, prompt);
Assert.AreEqual(loginHint, hintArg);
Expand Down Expand Up @@ -305,5 +306,59 @@ public async Task InvokesBeforeBuildClientOnExtendedOptions()

Assert.True(beforeBuildClientInvoked);
}

[Test]
public async Task BrowserCustomizedOptionsHtmlMessage([Values(null, "<p> Login Successfully.</p>")] string htmlMessageSuccess, [Values(null, "<p> An error occured: {0}. Details {1}</p>")] string htmlMessageError)
{
var mockMsalClient = new MockMsalPublicClient
{
InteractiveAuthFactory = (_, _, _, _, _, _, browserOptions, _) =>
{
Assert.AreEqual(false, browserOptions.UseEmbeddedWebView);
Assert.AreEqual(htmlMessageSuccess, browserOptions.HtmlMessageSuccess);
Assert.AreEqual(htmlMessageError, browserOptions.HtmlMessageError);
return AuthenticationResultFactory.Create(Guid.NewGuid().ToString(), expiresOn: DateTimeOffset.UtcNow.AddMinutes(5));
}
};
var options = new InteractiveBrowserCredentialOptions()
{
BrowserCustomizedOptions = new BrowserCustomizationOptions()
{
UseEmbeddedWebView = false,
HtmlMessageSuccess = htmlMessageSuccess,
HtmlMessageError = htmlMessageError
}
};

var credential = InstrumentClient(new InteractiveBrowserCredential(default, "", options, default, mockMsalClient));

await credential.GetTokenAsync(new TokenRequestContext(MockScopes.Default));
}

[Test]
public async Task BrowserCustomizedUseEmbeddedWebView([Values(null, true, false)] bool useEmbeddedWebView, [Values(null, "<p> An error occured: {0}. Details {1}</p>")] string htmlMessageError)
{
var mockMsalClient = new MockMsalPublicClient
{
InteractiveAuthFactory = (_, _, _, _, _, _, browserOptions, _) =>
{
Assert.AreEqual(useEmbeddedWebView, browserOptions.UseEmbeddedWebView);
Assert.AreEqual(htmlMessageError, browserOptions.HtmlMessageError);
return AuthenticationResultFactory.Create(Guid.NewGuid().ToString(), expiresOn: DateTimeOffset.UtcNow.AddMinutes(5));
}
};
var options = new InteractiveBrowserCredentialOptions()
{
BrowserCustomizedOptions = new BrowserCustomizationOptions()
{
UseEmbeddedWebView = useEmbeddedWebView,
HtmlMessageError = htmlMessageError
}
};

var credential = InstrumentClient(new InteractiveBrowserCredential(default, "", options, default, mockMsalClient));

await credential.GetTokenAsync(new TokenRequestContext(MockScopes.Default));
}
}
}
10 changes: 4 additions & 6 deletions sdk/identity/Azure.Identity/tests/Mock/MockMsalPublicClient.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ internal class MockMsalPublicClient : MsalPublicClient
public List<IAccount> Accounts { get; set; }
public Func<string[], string, AuthenticationResult> AuthFactory { get; set; }
public Func<string[], string, AuthenticationResult> UserPassAuthFactory { get; set; }
public Func<string[], string, Prompt, string, string, bool, CancellationToken, AuthenticationResult> InteractiveAuthFactory { get; set; }
public Func<string[], string, Prompt, string, string, bool, BrowserCustomizationOptions, CancellationToken, AuthenticationResult> InteractiveAuthFactory { get; set; }
public Func<string[], string, AuthenticationResult> SilentAuthFactory { get; set; }
public Func<string[], string, IAccount, string, bool, CancellationToken, AuthenticationResult> ExtendedSilentAuthFactory { get; set; }
public Func<DeviceCodeInfo, CancellationToken, AuthenticationResult> DeviceCodeAuthFactory { get; set; }
Expand All @@ -30,7 +30,7 @@ public MockMsalPublicClient(AuthenticationResult result)
{
AuthFactory = (_, _) => result;
UserPassAuthFactory = (_, _) => result;
InteractiveAuthFactory = (_, _, _, _, _, _, _) => result;
InteractiveAuthFactory = (_, _, _, _, _, _, _, _) => result;
SilentAuthFactory = (_, _) => result;
ExtendedSilentAuthFactory = (_, _, _, _, _, _) => result;
DeviceCodeAuthFactory = (_, _) => result;
Expand Down Expand Up @@ -72,9 +72,7 @@ protected override ValueTask<AuthenticationResult> AcquireTokenInteractiveCoreAs
string loginHint,
string tenantId,
bool enableCae,
bool? useEmbeddedWebView,
SystemWebViewOptions systemWebViewOptions,
EmbeddedWebViewOptions embeddedWebViewOptions,
BrowserCustomizationOptions browserOptions,
bool async,
CancellationToken cancellationToken)
{
Expand All @@ -83,7 +81,7 @@ protected override ValueTask<AuthenticationResult> AcquireTokenInteractiveCoreAs

if (interactiveAuthFactory != null)
{
return new ValueTask<AuthenticationResult>(interactiveAuthFactory(scopes, claims, prompt, loginHint, tenantId, async, cancellationToken));
return new ValueTask<AuthenticationResult>(interactiveAuthFactory(scopes, claims, prompt, loginHint, tenantId, async, browserOptions, cancellationToken));
}
if (authFactory != null)
{
Expand Down

0 comments on commit 6e8e214

Please sign in to comment.