[New API] Improved 3.x API leveraging builders

[jmprieur]

Problem to solve: The API needs simplification, it has many overrides and we need more.

Summary

We want to add more information to the constructors of applications, and some flows (AcquireTokenAsync), and therefore we need more overrides. But we already have too many overrides. Also every customer needs to implement the configuration of apps.

This issue proposes to solve these problems.

We would love to get your feedback about these changes

Configuration of apps

Customers (developers) end-up to write a lot of code to write read their own configuration files and transform them in arguments to the constructors of PublicClientApplication and ConfidentialClientApplication. Most of the code is very common, and inspired from our samples

• Some of the configuration on UIParent should really be part of the app construction (embedded browser vs system browsers on platform that support it)
• We want to add more configuration to our apps (HttpClient factory, User Agent string ..), and we don't want to add more overrides

What is configurable in MSAL.NET?

Currently you can override default configuration by calling the application's constructor, and setting properties on the application:

• For Both PublicClientApplication and ConfidentialClientApplication

  • Constructor overload (ClientId, Authority)
  • Properties PublicClientApplication
    • ValidateAuthority
    • Components
    • SliceParameters
    • Logging parameters:
      • DefaultLoggingEnabled,
      • LogLevel,
      • PiiLoggingEnabled),
      • as well as the Log callback
    • Telemetry parameters:
      • TelemetryOnFailureOnly,
      • as well as the telemetry receiver callback

    I would not think that ExtendedLifeTimeEnabled should be part of the configuration as this should probably be set by the application's code dynamically depending on some trigger

• For [ConfidentialClientApplication], in addition to these configurations:

  • redirectUri
  • clientCredentials, which can be either an application password or a certificate.

What other configuration do customer request?

Customers also request the possibility of setting:

• the User Agent
  • See Feature request: change User-Agent header
• a proxy, especially on Core where this is not possible. See:
  • MSAL.NET #552: Explicit definition of proxy
  • MSAL.NET #552: Running on dotnet core does not respect WebRequest.DefaultWebProxy
• user the broker or not

AcquireTokenAsync already has too many overloads

MSAL public clients have many overrides of AcquireTokenAsync, and more needs to be added because we need to support more parameters.

• For instance conditional access is currently supported by using additionalQueryParameters, but that's not a good developer experience.
• As the V2 endpoint is rolled-up, developers will want to acquire tokens for V1 applications, and not only V2, and therefore will need to transform Resource ids to scopes. Ideally we might want to add an API for them to pass Resources to help migration
• We got the feedback from customers that they would want to make the overrides of AcquireTokenAsync cancellable by passing a
  CancellationToken. An example where this is needed is for brokered scenarios when the user cancels the login process by pressing the Home
  button on the device and then reactivate the app, the app iu hung waiting for AcquireTokenAsync to return), for details, see ADAL.NET #1306). Again this would mean having more overrides.

AcquireTokenAsync has a lot of overrides, and we'll need more.

Taking the example of MSAL.NET, PublicClientApplication already has 14 overrides of AcquireTokenAsync.

For details see Acquiring tokens interactively in MSAL.NET's conceptual documentation

The experience for conditional access is not good: we need more overrides

How ADAL does conditional access?

In ADAL, the way conditional access / claim challenges is managed is the following:

• AdalClaimChallengeException is an exception (deriving from AdalServiceException) thrown by the service in case a resource requires more claims from the user (for instance 2-factors authentication). The Claims member contain some json fragment with the claims which are expected.
• The public client application receiving this exception needs to call the AcquireTokenAsync override having a claims parameters. This override of AcquireTokenAsync does not even try to hit the cache, it’s necessarily interactive. The reason is the token in the cache does not have the right claims (otherwise an AdalClaimChallengeException would not have been thrown), so there is no need looking at the cache. Note that the ClaimChallengeException can be received in a WebAPI doing the on-behalf-of flow, whereas the AcquireTokenAsync needs to be called in a public client application calling this Web API.

What's the experience with MSAL today?

• The Claims are already surfaced in the MsalServiceException.
• Almost all the AcquireTokenAsync overrides in MSAL all have extraQueryParameters and the way to go today is to add

"&claims={msalServiceException.Claims}"

to the current extraQueryParameters.

Here is an example of the code required today (in C#)

string extraQueryParameters = $"claims={msalServiceException.Claims}";
var accounts = await app.GetAccountsAsync();
result = await app.AcquireTokenAsync(scopes,
                                    accounts.FirstOrDefault(),
                                    new UIBehavior(),
                                    extraQueryParameters);

What experience do we want to propose for conditional access?

We could add another set of overrides with a claims parameter, but this would mean:

• either double the number of overrides
• or only add it to some of the overrides, for instance the most complete, as was done in ADAL.NET
• Another possibility is changing the way we do an use a builder pattern

The override which don't take a loginHint require and account. This is considered as overkill for single-identity applications

As you can see in the snippet above, most overrides of AcquireTokenAsync require an instance of IAccount, and most of the times developers having application managing only one identity either are confused or just take the first account without thinking more taking the first user in the cache (here in C#)

var accounts = await app.GetAccountsAsync();
accounts.FirstOrDefault(),

We got the feedback from users and MVPs that, for single identity applications, they want a simpler API which does not require to pass the account. Unfortunately, this means adding more overrides.

Proposed solution

Given that we want to reduce the number of overloads of the constructors of PublicClientApplication and ConfidentialClientApplication, and also of AcquireTokenAsync, we propose to use the builder pattern.

IPublicClientApplication pca = PublicClientApplicationBuilder
    .Create("yourclientid")
    .WithUserTokenCache(new TokenCache())
    .WithRedirectUri("yourredirecturi")
    .WithLoggingLevel(LogLevel.Info)
    .WithEnablePiiLogging(true)
    .WithAadAuthority(AadAuthorityAudience.AzureAdAndPersonalMicrosoftAccount)
    .Build();

By having the Build() method return an IPublicClientApplication interface we can enable developers to enable better mocking/testing scenarios (an oft-requested feature) and also improve our own dependency-injection and internal construction. This also enables us to remove setter properties from the XXXClientApplication classes to make them immutable after construction which reduces testing scenarios and improves reliability.

In C#, there is a common infrastructure (especially used in asp.net core but also available for other scenarios) for loading configuration data from a json file (or using command line parameters, environment variables, etc to populate the configuration data). This is usually a type of XXXOptions, so PublicClientApplicationOptions or ConfidentialClientApplicationOptions in our case. Many of these are common to both, so there's an abstract ApplicationOptions class to define them.

   public abstract class ApplicationOptions
    {
        public string ClientId { get; set; }
        public string Tenant { get; set; }
        public string RedirectUri { get; set; }
        public LogLevel LogLevel { get; set; }
        public bool EnablePiiLogging { get; set; }
        public bool IsDefaultPlatformLoggingEnabled { get; set; }
        public string SliceParameters { get; set; }
        public string Component { get; set; }
    }

Given this, the developer has the ability to populate the PublicClientApplicationBuilder with these values and then only need to supplement the builder with items (such as logging callbacks) that aren't able to be provided in a
config file.

// This will likely just be serialized in from json file or other config in the application bootstrapping,
// shown here explicitly for demonstration purposes.
var options = new PublicClientApplicationOptions
{
    ClientId = "yourclientid",
    ...
};

IPublicClientApplication pca = PublicClientApplicationBuilder
    .CreateWithApplicationOptions(options)
    .WithUserTokenCache(new TokenCache())
    .WithAadAuthority(AadAuthorityAudience.AzureAdAndPersonalMicrosoftAccount)
    .WithHttpClientFactory(new MyHttpClientFactoryImpl())
    .Build();

This builder concept also extends well into calls to AcquireTokenAsync. This introduces the class AcquireTokenBuilder:

var result = await application.AcquireTokenInteractive(scopes)
 .WithOwnerWindow(parent)
 .WithPrompt(Prompt.SelectAccount) // replaces UIBehavior
 .ExecuteAsync(cancellationToken);

Expected breaking changes

We propose the following breaking changes:

• UIBehavior would be renamed to Prompt (as this is what it is)
• The Token cache would be built by the application constructors, and will be provided by UserTokenCache on both applications types, and also ApplicationTokenCache on ConfidentialClientApplication. Both of these properties are of type ITokenCache, and this interface provides methods to plug-in the custom serialization.
• The logger will now be a delegate and properties (.WithXXX) on the constructor of each application.
• Telemetry will now be a delegate on the constructor of each application
• We would remove the need for SecureString as, in addition to not being secure, the API is very hard to use from managed code. PowerShell is realy the only thing that takes strings and makes them Secure. See https://stackoverflow.com/a/1570451/738188

Proposed work

The work to be done is:
Create Application Builders API surface:

• PublicClientApplicationBuilder to construct an IPublicClientApplication
• ConfidentialClientApplicationBuilder to construct an IConfidentialClientApplication
• Necessary Options classes and other supporting code to enable building/validating/constructing
• New Unit tests for Applciation Builders API Surface
• API XML documentation for Application Builders API Surface

Create AcquireToken Builders API surface

• AcquireTokenBuilder class with static methods for CreateInteractive, CreateSilent, etc for each scenario
• New Unit tests for AcquireToken Builders API Surface
• API XML documentation for AcquireToken Builders API Surface

Update ServiceBundle to take values from IApplicationConfiguration and not store locally

Collapse UiParent / OwnerWindow / OwnerUiParent infrastructure to make adding activity/owner window in builders API possible

Wire in AcquireToken builder methods to implementation

Initially we would leave both the old API (with overrides) and the new API (with builders). This would be MSAL 3.0

Authority Class Updates (see markzuber/config branch in src/microsoft.identity.client/instance directory for spec in code)

• Create AadOpenIdConfigurationEndpointManager to handle Instance Discovery / Caching (extract from Authority Class)
• Create AuthorityEndpointResolutionManager classes to handle endpoint resolution for each authority type (extract from Authority class)
• Create AuthorityEndpoints class and wire it in to the infrastructure

Wrap existing TokenCache object in ITokenCache interface

• Will need to migrate TokenCache ExtensionMethods into TokenCache class directly
• Finalize public API changes for how we want consumers to deserialize cache versus specify us to serialize the cache for them
  • this will be modification to the Create Application Builders API surface and possibly ClientApplicationBase

After review with security team, either remove SecureString or clean up our existing infra

• Remove usernamePasswordInput / IwaInput and just pass around username / un/pw

Collapse PublicClientApplication / ConfidentialClientApplication and other public types into root folder structure for visibility

Deprecate setters (and some getters) on ClientApplicationBase / PCA / CCA

• PCA/CCA should be immutable but debuggable (e.g. we'll want to add IAppConfig as a readonly property to ClientApplicationBase)

Deprecate Logger as a process wide static

• will need to pass ILogger interfaces through to some classes
• Remove CoreLoggerBase and extra infra from before the adal/msal split

Deprecate ITelemetry / Telemetry class as a process wide static

Deprecate ModuleInitializer

Add MaskedConsoleReader class to replace manual key input for console password (test code)

Later (MSAL 4.x) we would:
Deprecate old AcquireToken methods so we're only using the ones with Builders (as appropriate, under review)
Deprecate public constructors on PCA / CCA

• Builders should be only way to construct
• Update unit tests to use the builders and check properties after construction to ensure functionality

More information

See the dev3x branch for the work in progress

[cansado2930]

Hi,
I’m going to share my opinion about this. I’m accord with you. It is more easy and clear with the new proposal method instead of overrides. The part for single account, yes, I can understand other developers although for me it is normal because users may have various accounts on the same device or on the same app. I have developed apps for common users of world as specific cases of business. Both cases, there are users with more than an account, so I have to check it. Some apps may not have sense that have various accounts but I think it is necessary to check what user wish.
In resume, it sounds very well

[MarkZuber]

@cansado2930 thanks for your feedback. I'm glad to hear that you think it's the right direction for the API surface. Please let us know if you have suggestions for improvement or further details on your account cases and how we can make that clearer/easier.

[jennyf19]

MSAL 3.0.0-preview release