search

microsoftgraph / msgraph-sdk-dotnet-auth

Archived - use the TokenCredential classes provided by Azure.Identity. https://docs.microsoft.com/en-us/dotnet/api/overview/azure/identity-readme
https://graph.microsoft.com
MIT License
78 stars 19 forks source link

Update to MSAL v3 and Builder Pattern #37

Closed michael-hawker closed 5 years ago

michael-hawker commented 5 years ago

MSAL has released v3 now for .NET, so should upgrade?

michael-hawker commented 5 years ago

Think they have a new builder pattern for v3 (don't think this was in 2.7.1).

It'd be great if the Creation factories for the different patterns could return the builder from MSAL vs. the interface so advanced configuration could leverage the same methods. See docs here.

peombwa commented 5 years ago

@michael-hawker We are in the processes of moving our auth providers to Microsoft.Graph.Core SDK and we will upgrade the auth providers to use of MSAL v3 as part of this change. This is currently being tracked under issue #461 in our main DotNet repo.

michael-hawker commented 5 years ago

Thanks @peombwa that's great!

I'm wondering if the client applications created could also just create the provider behind the scenes. Then you wouldn't have to double-check to pass the clientApplication reference into the same matching authentication provider, could simplify the API ab it.

peombwa commented 5 years ago

@michael-hawker Thanks for the suggestion, but the only reason why we can't create auth providers behind the scenes is because the client applications (PublicClientApplication & ConfidentialClientApplication) are used in more than one OAuth flow (which our auth providers are based on) and each auth provider internally invokes a unique MSAL AcquireTokenXXXX method. Nonetheless, we will try and simplify our provider's interfaces to fully take advantage of MSAL's v3 builder pattern.

seesharprun commented 5 years ago

I have a fork in my personal GitHub org where I have implemented most (but not all) of the unit tests and rewrote the classes for MSAL v3: https://github.com/seesharprun/msgraph-sdk-dotnet-auth

Here’s the changes:

There were a couple of hiccups

Once you have your branch ready for MSAL v3, I would be happy to submit a PR and see if I can shine this up enough to get pulled 😅

michael-hawker commented 5 years ago

@seesharprun both the Microsoft.Identity.Client and Microsoft.Graph libraries target lower versions of .NET Standard to support more platforms. I think that's important to keep, so probably shouldn't move to .NET Standard 2.0 yet if not needed.

seesharprun commented 5 years ago

@michael-hawker The good news is that’s an easy fix. I’ll make the fix in my fork and then wait for the new branch to be ready. I can then open a PR so we can shift the conversation there.

darrelmiller commented 5 years ago

Hey Sidney! Thanks for the PR. Peter and I will review. I'm curious about the changes to the TokenCache that you have made. @jmprieur @peombwa and I have been discussing how we can further align what we are doing here with the Microsoft.Identity.Web work that is ongoing.

seesharprun commented 5 years ago

The pull request is open at #39 and I added eight checkboxes to discuss each of the items there.

@darrelmiller, unless I misunderstood, the MSAL team seems to have removed the UseTokenCache(tokenCache) builder method from the v3 implementation.

darrelmiller commented 5 years ago

@seesharprun Ahh yes. I think we will need to pull the precreated TokenCache instance out of the application and wire up our TokenStorageProvider hooks into it. However, we need to make sure we can use the TokenCacheProviders that are being created by the MSAL team.

seesharprun commented 5 years ago

I can add a checkbox for that to the laundry list in the PR ✔️ .

It's going to take a mountain of work to get this library to MSAL v3 but i'm incentivized to try and do this by the end of this month so we can have a rockstar demo session at the InsiderDevTour :smile:

pschaeflein commented 5 years ago

I know it is early, and it may just be my ignorance, but if I have gone thru the exercise of creating a custom token cache, why do i also need to create/implement the graph's ITokenStorageProvider?

The MSAL docs specify a pattern to use, I don't think I should use a different pattern for the Graph SDK.

Wouldn't something like this suffice?

var pca = Microsoft.Graph.Auth.InteractiveAuthenticationProvider.CreateClientApplication(clientId);
pca.UserTokenCache.SetBeforeAccess((TokenCacheNotificationArgs args) => { /* my delegate */});
pca.UserTokenCache.SetAfterAccess((TokenCacheNotificationArgs args) => { /* my delegate */});
var ap = new Microsoft.Graph.Auth.InteractiveAuthenticationProvider(pca);
var gsc = new GraphServiceClient(ap);

Or maybe just pass my ITokenCache to your IAuthenticationProvider factory methods...

michael-hawker commented 5 years ago

@pschaeflein yeah, this threw me a bit too. I think it's a preference of interface vs. events. It's easier/more discoverable to see you need to pass in an instance of something that implements an interface to get the provided functionality (and know which methods to complete) vs. knowing you have to dig-in to two specific paired events. It would just be helpful if the example file based cache mechanism MSAL suggests was just provided. We may want to open an issue in MSAL and have a discussion there about the API?

I also made a suggestion to the factory here in this comment which if MSAL added a method for the cache setter it could be made easier.

pschaeflein commented 5 years ago

OK, so to make the "know what to complete" easier, how about a CreateClientApplication factory method that takes an ITokenCache in addition to the client id. That factory method can then do the wireup.

Just my $.02. ;)

seesharprun commented 5 years ago

I'm a fan of creating a new issue and having a deeper conversation around how to handle ITokenCache in MSAL v3. We can get the rest of the upgrade "out of the way" and have that as the first item to tackle post v3 upgrade. This gives us more time to bring in other interested parties and come up with a solution everyone is happy with.

darrelmiller commented 5 years ago

@pschaeflein Our original plan was to implement standard tokenCache providers because the MSAL team were not going. Then that plan changed. We have yet to synchronize what we started with what they have started and will continue. Also, our plan doesn't work any more because MSAL V3 moves the creation of the TokenCache inside the app. We want to align with MSALs changes.

pschaeflein commented 5 years ago

I think you are saying that I'm getting what I want. If I build an "IClientApplication" using whatever strategy, I can provide that to this SDK and it will use it. ;)

Thanks for the clarification.

michael-hawker commented 5 years ago

Should this issue be closed since the PR was merged?

Should I open a new one for MSAL v4.3?

darrelmiller commented 5 years ago

@michael-hawker Yes that would be great. Thanks.