Microsoft-authentication-library-for-dotnet: AcquireTokenInteractive parent param is not intuitive

[bgavrilMS]

Which Version of MSAL are you using ?
MSAL 3 preview

Platform
Android

What authentication flow has the issue?

• Desktop / Mobile
  
  
  
  • [x] Interactive
  
  
  • [ ] Integrated Windows Auth
  
  
  • [ ] Username Password
  
  
  • [ ] Device code flow (browserless)
  
  
• Web App
  
  
  
  • [ ] Authorization code
  
  
  • [ ] OBO
  
  
• Web API
  
  
  
  • [ ] OBO
  
  

Repro

1. Migrate an interactive request from MSAL 2 to MSAL 3. In MSAL3, the builder API requires you to pass two parameters:

AcquireTokenIntrective(IEnumerable scopes, object parent)

1. The "object parent" is not intuitive. If you come from MSAL2, you will pass a UIParent. MSAL3 will throw an InvalidOperationException on Android and net45.

Expected behavior
a. Better code docs
b. Throw an MSALClientException
c. Rename the second param to "windowOrActivity"
d. windowOrActivity is available on the ClientApplication if specified on the AquireToken call it takes priority
e. continue throwing exception if not specified on platforms where it's needed. exception should tell where it can be set
f. Let Activity and Window be required param on AcquireToken, but allowing null if specified on the CA object. Otherwise throw!

[jmprieur]

Maybe the overrides should be different and explicity depending on the platforms / reference library:

• Activity on Android
• Window on .NET FW
• object on .NET standard?
  what do you think, @bgavrilMS ?

[bgavrilMS]

We can "shape" the public API like you describe, but we need to keep the overload with "object parent" on .NetStandard

[jmprieur]

yes @bgavrilMS : that's what I meant by "object on .NET standard".

[bgavrilMS]

Sounds good. How about an API like:

//NetStandard (this will be on all platforms at runtime, but only on NetStandard at build time)
AquireTokenInteractive(IEnumerable<string> scopes, object windowOrActivity)

//Android 
AquireTokenInteractive(IEnumerable<string> scopes, Activity activity)

//net45 
AquireTokenInteractive(IEnumerable<string> scopes)
AquireTokenInteractive(IEnumerable<string> scopes, IntPtr windowPtr)
AquireTokenInteractive(IEnumerable<string> scopes, IWin32Window window)

//Mac
AquireTokenInteractive(IEnumerable<string> scopes)
AquireTokenInteractive(IEnumerable<string> scopes, NSWindow window)

// iOS
AquireTokenInteractive(IEnumerable<string> scopes)

// UWP
AquireTokenInteractive(IEnumerable<string> scopes)

[jmprieur]

this looks perfect, @bgavrilMS !
Clear and non-ambiguous

@henrik-me : given that this is a breaking change for UWP and IOS, I'd like to take this before we remove the -preview (3.1) ?

[dansiegel]

I think this is entirely the WRONG place to be requiring the Window/Activity... this really should be done in the setup of client. What I would think is that you might have:

var pca = PublicClientApplicationBuilder.Create(options.ClientId)
                    .WithB2CAuthority(authority)
                    .WithActivity(someActivity) // Or other platform specific type
                    .Build();

// Authority only required if its intentional to override the initial authority from the PCA Builder
var result = await pca.AquireTokenInteractive(scopes)
                      .WithAuthority(someAuthority)
                      .WithAccount(account)
                      .ExecuteAsync();

[jmprieur]

thanks @dansiegel this makes sense.
Don't we think that we should be able to center the sign-in dialog on a particular part of the UI (and therefore also an override of acquire token interactive)

[henrik-me]

@dansiegel : thanks a lot for the feedback. To me this makes a lot of sense for a specific platform. Also it makes a lot of sense to specify the UI parameters at the ClientApplication level, with an option to override the behavior at the interactive call if needed.

I'm not currently able to see how code written in a shared module which is used both on Android, Windows, and iOS would handle this platform specific elements if not having some compiler flags or similar. Are you having something special in mind for this scenario?

@bgavrilMS @jmprieur : does it make sense to add .WithActivityOrWindow, .WithUiParent, or similar to the PCA as well as the ATI ? And then allowing overriding it in the AquireTokenInteractive builder? I suppose the reason we don't do this in the ATI call is that it's a required param and we have designed the interface to have required params in the c'tor.

[jmprieur]

@henrik-me : this makes sense to me. even .WithParentActivityOrWindow and strongly typed overrides in the case of the window (and at both levels). Now there is also the case of .NET Standard, and I believe that @bgavrilMS had some thought on this. We'll discuss it today together.

We have a precedent of exclusive required parameters: AcquireTokenSilent accepts requires either an IAccount (which can be null) or a loginHint.

cc: @dansiegel

[dansiegel]

It seems to me that really the client has to be constructed from Platform code... how exactly you might go about that would probably depend on your architecture... for me I'd use a DI Container... if there is a need to know something platform specific like the Activity or Current Window then I don't see this as something that we should ever need to be able to override from the specific authorization call like AquireTokenInteractive.

Also if any of you will be at the MVP Summit, I'd be happy to sit down with you next week and discuss more in detail. As well as to cover some of the difficulties that I've been encountering.

[henrik-me]

@jennyf19 says : the iOS suggestion is missing the ViewController

triage: updating description to include what's discussed

[jmprieur]

@henrik-me @bgavrilMS @jennyf19 @MarkZuber @trwalke
I would like us not to postpone this one again :)

[HappyNomad]

I'm putting all the MSAL client code in my view-model. Because of that, I won't have a reference to any view objects. I'm hoping that the API you decide on doesn't require me to break the MVVM design pattern. Please keep this scenario in mind.

[bgavrilMS]

@ChainReactive - MSAL pops up a UI and on some frameworks we need a parent view , i.e. on Xamarin.Android you need to pass in the Activity. You need not do this on any other platform. You may pass a reference to the window on .net desktop (e.g. WPF) and we will use it to center the browser, but nothing more.

I understand the advantages of MVVM / MVC and one of the ways to overcome this is to abstract getting the current view in a service. For example on Android you can use https://github.com/jamesmontemagno/CurrentActivityPlugin

[bgavrilMS]

https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/pull/1059

This exposes both platform specific and a shared (netstandard) way for passing in the UI parent.

We discussed about exposing this on the PCA level as well as the AcquireTokenInteractive level, and decided to expose it only on AcquireTokenInteractive level. The reason is that on Android, and (soon) on iOS, developers NEED to pass the Activity / ViewController that started the authentication flow. The PCA Activity / ViewController will likely NOT be that one. This will lead to bugs.

I don't think this goes against DI or prevents having all auth in platform specific code. Developers need to keep track of the current Activity or ViewController that would kick off interactive auth. Interactive auth is a builder API, you can create a partial builder at the start of the app and then finalize it by passing the Activity.

[bgavrilMS]

Impact in term of DevEx:

Samples to fix: (all in their Msal3x branch)

https://github.com/azure-samples/active-directory-dotnet-desktop-msgraph-v2
https://github.com/azure-samples/active-directory-dotnet-native-uwp-v2
https://github.com/azure-samples/active-directory-xamarin-native-v2
Quickstarts to update [Needs to happen quickly because impacts in-portal experience]

https://docs.microsoft.com/en-us/azure/active-directory/develop/quickstart-v2-uwp
https://docs.microsoft.com/en-us/azure/active-directory/develop/quickstart-v2-windows-desktop
Tutorials to update

https://docs.microsoft.com/en-us/azure/active-directory/develop/tutorial-v2-windows-uwp
https://docs.microsoft.com/en-us/azure/active-directory/develop/tutorial-v2-windows-desktop
FYI @bgavrilMS
cc: @jennyf19

[jmprieur]

And also the B2C samples:

• https://github.com/Azure-Samples/active-directory-b2c-dotnet-desktop
• https://github.com/Azure-Samples/active-directory-b2c-xamarin-native

cc: @bgavrilMS @jennyf19 @valnav
I tested these 2 and provided a PR to update to MSAL 3.0.4

[bgavrilMS]

Can I do any work to help with the tutorials and quickstarts?