Skip to content
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

Closed
dansiegel opened this issue Apr 18, 2019 · 27 comments
Closed

Comments

@dansiegel
Copy link

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

var pca = PublicClientApplicationBuilder.Create(options.ClientId)
                                        .WithB2CAuthority(authority)
                                        .Build();

var result = await pca.AcquireTokenInteractive(Scopes)
                      .WithParentActivityOrWindow(Parent)
                      .ExecuteAsync();

Expected 3.0 API

var pca = PublicClientApplicationBuilder.Create(options.ClientId)
                                        .WithB2CAuthority(authority)
                                        .WithParentActivityOrWindow(Parent)
                                        .Build();

var result = await pca.AcquireTokenInteractive(Scopes)
                      .ExecuteAsync();
@jmprieur
Copy link
Contributor

@dansiegel
on which platform do you develop?

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.

@dansiegel
Copy link
Author

@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.

@bgavrilMS
Copy link
Member

@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?

@dansiegel
Copy link
Author

dansiegel commented Apr 23, 2019

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;
    }
}

@bgavrilMS
Copy link
Member

bgavrilMS commented Apr 23, 2019

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.

  • MSAL is quite a low level package, so we are very cautious to take dependencies on other packages / assemblies because of diamond dependency issues that occur. For example, if MSAL has a dependency on Plugin.CurrentActivity v1 and your app has a dependency on Plugin.CurrentActivity v2, you'd have to use some binding redirects or to downgrade.

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

@HappyNomad
Copy link

I'm in favor of the WithParentActivityOrWindow method going away. My auth code is in the view-model.
Regardless of possible trickery for obtaining the window reference there, I'm opposed to view objects even passing through that layer.

@bgavrilMS
Copy link
Member

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 activity

var pca = PublicClientApplicationBuilder
                    .Create(ClientId)
                    .WithParentActivityOrWindow(() => return CrossCurrentActivity.Current.Activity)

What do you gentlemen think?

@MarkZuber
Copy link
Contributor

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.

@HappyNomad
Copy link

HappyNomad commented Apr 23, 2019

Yea, a static property is kind of limiting. Let's stick with the idea of doing away with the WithParentActivityOrWindow method. But instead of a static ActivityOrWindow property, how about make it an instance ActivityOrWindow property on PCA?

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.

@MarkZuber
Copy link
Contributor

@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?

@bgavrilMS
Copy link
Member

@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.

@MarkZuber
Copy link
Contributor

@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.

@dansiegel
Copy link
Author

@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;
    }
}

@MarkZuber
Copy link
Contributor

@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).

@dansiegel
Copy link
Author

@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.

@MarkZuber
Copy link
Contributor

@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.

@HappyNomad
Copy link

A WithParentActivityOrWindow method on the PCA builder that accepts a delegate works for me, too. My PCA is created in a constructor invoked in App.xaml.cs, so I could supply such a UI-platform-specific delegate from there.

Does that mean we're all on board with the Option 2 that @bgavrilMS listed?

@bgavrilMS
Copy link
Member

bgavrilMS commented Apr 24, 2019

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.

@jmprieur
Copy link
Contributor

@bgavrilMS @MarkZuber I agree with an override with a delegate
cc: @henrik-me

@henrik-me henrik-me added this to the 4.2 milestone Jun 27, 2019
@MarkZuber MarkZuber self-assigned this Jul 9, 2019
@tipa
Copy link

tipa commented Jul 24, 2019

the new API doesnt seem to be available to me after updating to v4.2, am I missing something?

@henrik-me henrik-me reopened this Jul 24, 2019
@henrik-me
Copy link
Contributor

@MarkZuber investigating

@henrik-me
Copy link
Contributor

Thanks @tipa for raising this. We found the issue and have 4.2.1 out which does this right on all platforms.

@DJSavage
Copy link

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.

@bgavrilMS
Copy link
Member

@DJSavage
Copy link

@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
var userContext = await authenticationService.SignInAsync();
instead of responding to a button click, I would expect it to try to log me in or send me to a signup page. Instead, I get the '..current Activity...' error

Do you have any suggestions on this?

@bgavrilMS
Copy link
Member

bgavrilMS commented Dec 16, 2019

@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

@bgavrilMS
Copy link
Member

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants