-
Notifications
You must be signed in to change notification settings - Fork 329
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove the UI Parent #676
Comments
This will be considered part of the token request |
Great idea, but I'd like to propose a different approach / API. We could have a factory / builder for UIParent that is a superset of all platform specific UIParent objects, i.e. // Platform agnostic code ...
UIParent uiParent = UIParentBuilder
.UseEmbeddedBrowser(true) // only affects iOS and Android
.UseCorporateNetwork(false) // only affects UWP
.WithOwnerWindow(windowAsObject) // pass Activity on Android, IWindow on net45 and UWP, other platforms not affected Note that OwnerWindow is of type object and will be up-casted on each platform (to Activity on Android etc.). An exception will be thrown if the upcast fails. For example, on a Xamarin app that has several heads (Android, iOS and UWP) plus a shared project (NetStandard), you'd wrap you auth code in method that allows you to pass the owner window, as follows: // In the shared NetStandard code
public string PerformAuthAndGetToken(object parentWindow)
{
// construct UIParent like above
// construct PublicClientApp
// try to get token silent
// get token interactive
}
// In Android head, from an activity:
app.PerformAuthAndGetToken(this);
// In UWP head
app.PerformAuthAndGetToken(this); As such, UIParent does not need to be injected. |
I don't mind the pattern here, or even if it's implemented on the Platform level... What I would like to avoid is having to track an object (the UIParent or "Parent Window") from my shared (NetStandard) code base. The onus should be on MSAL not the consuming developer. public class UIParentBuilder
{
private static UIParentBuilder _instance;
internal static UIParent BuildParent()
{
// use builder to create UIParent
}
public static UIParentBuilder UseEmbeddedBrowser(bool useEmbedded)
{
// set this in the instance;
return _instance;
}
} For Xamarin projects I might have some initialization like: public class MainActivity : FormsAppCompatActivity
{
protected override void OnCreate(Bundle savedInstanceState)
{
UIParentBuilder.UseEmbeddedBrowser(true)
.WithActivity(this);
// perhaps an overload=> WithActivity(Func<Activity> getCurrentActivity)
}
} Thus freeing me up in shared (NetStandard) code to simply worry about Scopes, and any current account so I might have: var account = await _pca.GetAccountAsync("serviceId");
var result = await _pca.AquireTokenAsync(scopes, account);
// or if I have a single user scheme, let MSAL get the account automatically
var result = await _pca.AquireTokenAsync(scopes); |
I have fixed this with AzureAD/azure-activedirectory-library-for-dotnet#1428 - there is now a UIParent ctor that takes in (object parent, bool useEmbeddedWebview). This is available on all platforms, including netstandard (note that it's hidden on some platforms using a bait-and-switch technique). Will be available with the next release of MSAL 2.6.0-preview, for which we are now testing. |
This is now solved in MSAL.NET 2.6.0 |
Description
While I understand that the UI Parent may be necessary for the underlying API it would seem to me that this could be removed from the public API and what the developer needs to track.
The UIParent seems to be responsible for 2 things:
This seems to me that some minor updates could be added which would then eliminate the need for users to track a UIParent which cannot be initialized in shared code, but rather must be initialized on the platform and somehow injected into shared code.
Proposed API
For the Android specific issue this could easily be handled by a platform initializer something like:
On the Embedded Browser Front, simply adding a property to the PCA seems to be the better solution here since this could be ignored on platforms where it isn't relevant but could be used on platforms that do use it.
The text was updated successfully, but these errors were encountered: