Upgrading the library to MSAL v3

[seesharprun]

There's a lot here so I wanted to open an early PR to begin discussion and to see if this upgrade is even possible with some of the limitations that are introduced. Here's a checklist of things that need to be discussed:

• [x] Rewrote all seven providers to use the builder pattern introduced in MSAL.NET 3.0.0. This is intended to address #37
• [x] Since the library is targeted to support both .NET 4.5 and .NET Standard 1.3, I was forced to add conditional directives to InteractiveAuthenticationProvider.cs file. This is because the UIParent class was replaced with overloads based on the platform that either accept an object, IWin32Window, or IntPtr.
• [x] Rebuild the test project as a .NET Core 3.0 project using xUnit.
• [x] The test project has less tests than the original project since I was unable to figure out how to successfully mock the classes returned by each fluent builder method. The builder pattern uses a sealed, class with no parameter-less constructor (for example: AcquireTokenInteractiveParameterBuilder).
• [x] Implemented a default device code handle that printed a message to the console in the DeviceCodeProvider.cs file at line 43 to address #14.
• [x] Removed the tokenCache parameter from all seven provider implementations since it seems like you can't set your own cache anymore based on this issue in the MSAL repo: https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/issues/949
• [x] Replaced NationalCloudHelpers with Microsoft.Identity.Client.AzureCloudInstance enumeration.
• [x] Added missing XML comments
• [x] Added sample projects

We can check off things as we discuss them and hopefully they are improved.

[michael-hawker]

@seesharprun did you see they had a link to an example for the TokenCache in the comment here? We should sync with @jmprieur to see if it makes sense to improve MSAL pattern or provide easier context here again.

I know one of the issues (#38) I hit with the original implementation of the TokenCache is the interface provided here in the Graph was setup to be Async, but that wasn't supported by MSAL v2.7.1, so if you actually made the call async it wouldn't work. That was the pitfall I fell into. However, in MSAL v3, they provide an Async caller on the ITokenCache now so we could support it.

[seesharprun]

@michael-hawker I saw the TokenCacheProvider examples but couldn’t “connect the dots” on how they would be used to replace the cache in one of the Auth library providers. I’ll wait to see what @jmprieur says.

For now we can review, amend and finalize some of the other items on the list.

[michael-hawker]

One thing that's bugged me to is the tied nature of having to call the static method on the class and then the constructor with the result:

_clientApp = InteractiveAuthenticationProvider.CreateClientApplication(_clientId);
_clientApp.RedirectUri = "...";

var authProvider = new InteractiveAuthenticationProvider(_clientApp, _scopes); // Duplicate that needs to match

var graph = new GraphServiceClient(authProvider);

It'd be nice to consolidate this to reduce chance of error by mixing and matching and allow for adding on to the build, e.g.

var clientApp = InteractiveAuthenticationProvider.Create(out var provider, _clientId, _scopes)
    .WithRedirectUri("...");
    .Build();

var graph = new GraphServiceClient(provider);

Thoughts?

[seesharprun]

Looking at the builder implementation in MSAL v3, I struggle to see why we even need the static factory methods anymore.

If you build the IClientApplicationBase instance yourself, the code is:

var clientApplication = PublicClientApplicationBuilder
    .Create(clientId)
    .WithAuthority(AzureCloudInstance.AzurePublic, AadAuthorityAudience.AzureAdMultipleOrgs)
    .WithRedirectUri("https://login.microsoftonline.com/common/oauth2/nativeclient")
    .Build();

var authProvider = new InteractiveAuthenticationProvider(clientApplication, scopes);

var graphClient = new GraphServiceClient(authProvider);

If I had to make a recommendation, I would recommend having method overloads where you can either accept the instance built automatically:

new InteractiveAuthenticationProvider(string clientId, ...<optional parameters>... , string scopes = null)

or provide your own:

new InteractiveAuthenticationProvider(IPublicClientApplication clientApplication, ...<optional parameters>... , string scopes = null)

Then I would recommend making the IClientApplicationBase instance from the base class public so that it can be used and modified by library consumers:

string clientId = "<client-id-guid>";

var authProvider = new InteractiveAuthenticationProvider(clientId, redirectUri: "https://login.microsoftonline.com/common/oauth2/nativeclient", scopes: new List<string> { "User.ReadBasic.All" })

authProvider.ClientApplication...

Thoughts?

[darrelmiller]

It was never necessary to use the static methods to create the MSAL application instance. That was by design. The only purpose of the static methods were to be helpers to avoid having to know exactly what needed to be set for each scenario.

Since the introduction of the Builder pattern in MSAL 3.x we have been seriously considering dropping the static factory methods completely.

Creating an MSAL application should always be the starting point. Auth providers are simply adapters. They do not hide MSAL.

[seesharprun]

Dropping the static methods completely is definitely the "path of least resistance" for me.

Is it OK for me to go ahead and remove those methods in this PR?

[darrelmiller]

@seesharprun I'm fine with that. @peombwa do you have any concerns? The only issue that I am aware of and Peter will be able to clarify if it is a problem is that we are in the process of moving these auth providers into the core SDK library, which we are splitting into a new repo that is separate from the generated service library code. I just want to make sure that we are not going to create huge merge conflicts due to bad timing.

[peombwa]

@seesharprun I'm ok with dropping the static methods.

@darrelmiller It's perfectly fine to merge the changes here first, then we will move them to the core SDK once we've upgraded the providers to MSAL v3.

[MIchaelMainer]

We will want to have these changes season in this preview library for a while before we merge and bake this into Microsoft.Graph.Core (dropping the preview status of this integration).

I'm okay with the dropping the static methods as well.

Note for us After the merge into core, this preview library latest version will need to have target version nuspec updated to a range of Microsoft.Graph.Core version that support this preview library, and add deemphasis notes about the use of this Microsoft.Graph.Auth library.

Related question for us What impact can we expect for existing enterprise customer apps and the move from NetStandard 1.1 to 1.3, basically requiring that if they want to use the latest version of core (post M.G.Auth integration), they will need to move to Net Framework 4.6? For slow to upgrade large orgs, this could block upgrading to a later Microsoft.Graph.Core. Fortunately, decoupling Microsoft.Graph (generated library) will be independent so the latest Microsoft Graph functionality will still be enabled.

[seesharprun]

Just removed the static methods and finished the MSAL v3 implementations. I also added sample projects so you can try it out.

The only thing I have on my checklist left to talk about is how to test the MSAL builder implementation since the classes are parameter-less and sealed. Moq is not enough for this.

[michael-hawker]

@seesharprun how much do we need to test MSAL independently of what they do? Maybe I'm misunderstanding what you want to test?

@MIchaelMainer MSAL.NET is already on .NET Standard 1.3 so that should be fine as it's a dependency, eh? That still supports all Win10 devices (including Hub). Has updating incremental .NET versions in the 4.x family been that big of a shift for corporations?

[seesharprun]

@michael-hawker We are not testing MSAL, but the seven providers have a hard dependency on MSAL since it calls MSAL methods within the code. Ideally, we would stub the MSAL out and provide fake behavior so that we can test the methods in isolation.

[MIchaelMainer]

@michael-hawker Microsoft.Graph.Auth is optional right now for users of Microsoft.Graph.Core. Microsoft.Graph.Core targets 1.1. Once we include the auth providers in Microsoft.Graph.Core, this will make MSAL a required dependency and we must target 1.3.

I think a move to higher .net standard could affect a minuscule customer segment -- I don't have data. I ask the question since it is a requirement change. It requires customers to use a higher .Net standard than we currently require. Anecdotally, I've heard that some companies move slowly and are resistant to software config changes. Restated, I don't think it will be a problem.

[michael-hawker]

@MIchaelMainer agreed; .NET 4.6 came out in 2015, so think it's pretty safe to have that as a min requirement.

[seesharprun]

So I guess my next question is... what's next to get this PR merged? We can do a version change in the next PR

[darrelmiller]

@seesharprun Next steps... I'm going to review :-) I took a day off yesterday so I was a bit distracted. BTW, thank for all this awesome effort. It is much appreciated.

Regarding stubbing MSAL, I'm a bit cautious about introducing more interfaces. Let's see how far we can get with just the MSAL interfaces IConfidentialClientApplication and IPublicClientApplication.

[seesharprun]

@darrelmiller I’m open to suggestions on how to test the token acquisition logic. We can use Moq and replace IConfidentialClientApplication or IPublicClientApplication just fine. The thing I cannot seem to figure out is how to Moq the builder methods that return instances of types that are sealed and do not include a parameter-less constructor.

[MIchaelMainer]

• [ ] Create documentation update PR to match the changes in this PR before we merge into dev branch.

[seesharprun]

I'm happy to help with the documentation and create a PR. Is that just the documentation on readme.md or does it include other Markdown pages in this repo or other places?

[MIchaelMainer]

We will need to update the readme and the same information in the official docs: https://docs.microsoft.com/en-us/graph/sdks/choose-authentication-providers?tabs=CS.

Actually, let's just provide a link in the readme to the documentation and delete that dup content from the readme.

[darrelmiller]

I'm not sure I am comfortable with pushing developers to install a preview of .net core 3.0 just to be able to run the tests.

[seesharprun]

Fair point, should I bump the tests down to .NET Core 2.x?

[darrelmiller]

I think that would be good. And we should create a secondary solution file that contains the samples, as one of the samples also relies on netcore30.

[seesharprun]

Ran into an issue that I didn't think of before. If I put the samples into another solution, how will they reference the library? The version on NuGet is based on MSAL v2.

Am I missing something here?

I could update this PR to not include the samples and then submit them as a new PR as soon as this one is accepted and released on NuGet.

[darrelmiller]

@seesharprun I could create one solution without the samples and one with the samples and the all the source projects.

[seesharprun]

Just finished the second solution for the sample projects.

[seesharprun]

Just created the docs.com update in microsoftgraph/microsoft-graph-docs#4684.

[seesharprun]

I guess the next step is to have someone reference the updated documentation at https://docs.microsoft.com/en-us/graph/sdks/choose-authentication-providers?tabs=CS in the readme.md file in this repository. It doesn't seem appropriate for me to submit a PR for such a major change to the readme.

BTW: here's the preview url for the updated documentation (https://review.docs.microsoft.com/en-us/graph/sdks/choose-authentication-providers?branch=pr-en-us-4684)

[seesharprun]

Woot! 2/3 approvals

Is there anything I need to do on my end to facilitate the syncing of the docs PR and this PR? I assumed no, but wanted to check in case I forgot something obvious

[seesharprun]

All fair points. You won’t see any argument from me.