-
Notifications
You must be signed in to change notification settings - Fork 347
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
[Enhancement] Allow injecting the Parent Activity/Window in the Client Builder #1095
Comments
@dansiegel We have discussed providing the parent activity or window at app construction, and that looks appealing, but this has drawback that for Android the parent activity should really need the one triggering the Acquire token iteractive, and therefore setting it a at building would not garanty that. For details, see: #918 (comment) but feel free to object and let's discuss the best way. |
@jmprieur I work almost exclusively with Xamarin Forms. So the bulk of my code is done with Dependency Injection to get what I need into my netstandard projects. So my actual authentication service that consumes the IPublicClientApplication would end up having to be cross compiled to support the current strategy where you're trying to inject the parent. |
@dansiegel - I assume that at the lifecycle start of your app, you invoke all the DI logic and construct your objects. But how can you know, at this stage, which Activity will be used to perform the login? Is it always the same activity? |
I would say this perhaps relates to #1104 since both these issues are around the ParentActivityOrWindow. The more I think about it, the more I'm thinking that perhaps we're looking at the entirely wrong places. Perhaps this could all be simplified with a platform specific activity ONLY on those platforms where there is such a need. public static class MSALActivityLocator
{
public static Activity CurrentActivity { get; set; }
}
[Application]
class App : Application, Application.IActivityLifecycleCallbacks
{
public void OnActivityCreated(Activity activity, Bundle savedInstanceState)
{
MSALActivityLocator.CurrentActivity = activity;
}
} Alternatively you could do what I do personally and take a dependency on Plugin.CurrentActivity. The benefit here, is that you're not introducing a new API, and using a dependency most Xamarin.Android developers are already used to using. [Application]
class App : Application, Application.IActivityLifecycleCallbacks
{
public void OnActivityCreated(Activity activity, Bundle savedInstanceState)
{
CrossCurrentActivity.Current.Activity = activity;
}
}
// Alternatively:
public class MainActivity : FormsAppCompatActivity
{
protected override void OnCreate(Bundle bundle)
{
CrossCurrentActivity.Current.Activity = this;
}
} |
I like your second idea best for providing a simplified developer experience. While we cannot take a direct dependency on that package*, we might be able to pull the logic from that package directly into MSAL.
I will experiment a bit with Plugin.CurrentActivity and update the thread. Our current plan is to release a non-preview version of MSAL 3, so we might not be able to take this on in the next few weeks. CC @jennyf19 |
I'm in favor of the |
So it looks like the Plugin.CurrentActivity needs to be initalized with the application, so it's not a great solution for MSAL itself, especially as there is no such plugin for other frameworks. I'd say we have 2 options: Option 1 - MSAL exposes a static for getting hold of the Activity or Window.PublicClientApplication.ActivityOrWindow = /* developer is responsible for setting and keeping this updated */ Option 2 - MSAL allows you pass in a lambda for getting the current activityvar pca = PublicClientApplicationBuilder
.Create(ClientId)
.WithParentActivityOrWindow(() => return CrossCurrentActivity.Current.Activity) What do you gentlemen think? |
I don't like option 1 because the Activity/Window isn't necessarily process-wide and attaching it to the type (via static) instead of the instance is problematic. I'd be fine having the activityorwindow as part of constructing the PublicClientApplication so that you can inject all types up front. So I guess I'm proposing option 3 which is option 2 with the activity directly instead of a lambda. but we do have several customers creating multiple PCA objects within a process space and they may or may not all want to have one global window/activity to set themselves against. |
Yea, a static property is kind of limiting. Let's stick with the idea of doing away with the That'd satisfy my need since I can expose the PCA instance from the view-model to the view. The view can then set it as needed. |
@ChainReactive -- we've tried with the 3.0 API to have configuration on a PCA be immutable once constructed since it's not obvious the intent of changing properties on an ongoing object (possibly with other background operations happening on it concurrently) should be. Wouldn't the "per-call" operation (e.g. on AcquireTokenXXX().WithActivityOrWindow()) of setting the activity/window work from a view model without it needing to be a settable property on the PCA? |
@MarkZuber - the problem with having: var pca = PublicClientApplicationBuilder
.Create(ClientId)
.WithParentActivityOrWindow(activity); is that the activity can change, so you'd need to construct a new PCA each time. I am not an Android developer to be able to tell how often this may happen, but a typical app seems to have several activities. |
@bgavrilMS -- sure, I agree with that. I'm suggesting to have both. Allow it to be set at PCA construction for when that works. And also have it at the per-acquiretoken call level for when it needs to be set only locally or over-ridden for a different case. Having settable configuration on the PCA causes problems for things like an IdentityService that's got PCA entities handling multiple request types and setting a process static or modifying the config after construction can cause undesirable side effects. |
@MarkZuber including WithParentActivityOrWindow is just terrible when you want to use this cross platform and just leads everyone down really bad paths. The best thing is to get rid of it altogether from the API and focus on the important part. MSAL needs to know what the current Activity is... or possibly a Window on another platform. That should be very Platform Specific. There are essentially two ways of going about this... One is with a static property that would be on the developer to update when the Activity changes or to use a locator delegate to return to you what the Activity is like this. [Application]
class App : Application, Application.IActivityLifecycleCallbacks
{
public App()
{
PublicClientApplication.CurrentActivityLocator = () => _currentActivity;
}
private Activity _currentActivity;
public void OnActivityCreated(Activity activity, Bundle savedInstanceState)
{
_currentActivity = activity;
}
} |
@dansiegel That's a fair assessment and a good argument for the delegate case. I still shy away from it being a process-wide static and we're deprecating PublicClientApplication being a public object anyway in favor of getting IPublicClientApplication from the builder (you can't get the raw object from the constructors any longer). |
@MarkZuber I'm not married in any shape way or form to PublicClientApplication… and really that should probably be on the builder to be fair... the point about it being a delegate that you set on the platform is the more important part. |
@dansiegel Awesome. Thanks for your clarification and description of usage patterns. That's very helpful in the discussion. I'm on board with the delegate on the builder as that appears to offer the right level of flexibility. |
A Does that mean we're all on board with the Option 2 that @bgavrilMS listed? |
I'm on board with that. I don't think we'd want to deprecate the existing .WithParentActivityOrWindow just yet, instead the new method with a delegate will take precedence. Let's wait for @jmprieur to also weigh in on this, currently we're locked down for shipping a non-preview version. |
@bgavrilMS @MarkZuber I agree with an override with a delegate |
the new API doesnt seem to be available to me after updating to v4.2, am I missing something? |
@MarkZuber investigating |
Thanks @tipa for raising this. We found the issue and have 4.2.1 out which does this right on all platforms. |
This is where Google eventually sent me when searching for 'On Xamarin.Android, you have to specify the current Activity from which the browser pop-up will be displayed using the WithParentActivityOrWindow method' when implementing ADB2C and after installing Plugin.CurrentActivity. Is there any worked-through documentation and example implementations for your new update? I have found that legacy ADB2C and Xamarin documentation and YouTube tutorials still hanging around offer confusing, contradictory and potentially dangerous information; so it would be nice to have a single source of truth to rely on. |
@DJSavage - here is a sample https://github.com/Azure-Samples/active-directory-b2c-xamarin-native |
@bgavrilMS - thank you. You have pointed me to an updated version of active-directory-b2c-xamarin-native. The previous versions of this were much different (i.e. no msalActivity.cs). However, this version still does not solve the 'On Xamarin.Android, you have to specify the current Activity from which the browser pop-up will be displayed using the WithParentActivityOrWindow method' problem. The issue is that the examples you are coding to do not reflect real-world app ux. Users will expect to open their app and either be logged in automatically, or presented with a signup/signin screen. They do not expect to choose for themselves. If I change the MainPage to call Do you have any suggestions on this? |
@DJSavage I updated the sample to suggest a better design - perhaps you could review the PR and let me know if this better fits real world scenarios? https://github.com/Azure-Samples/active-directory-b2c-xamarin-native/pull/112/files |
@DJSavage - the reason MSAL needs an Activity is that MSAL needs to open up a browser, let the user login and then be redirected back to application and to an Activity. The authentication action cannot happen without an Activity. |
Description
The UI Parent (Window on UWP/Activity on Android) should be injected at the Client Application Builder level and not be required at the AcquireTokenInteractiveParameterBuilder level.
Current 3.0 API
Expected 3.0 API
The text was updated successfully, but these errors were encountered: