AzureAD / microsoft-authentication-library-for-java

Microsoft Authentication Library (MSAL) for Java http://aka.ms/aadv2
MIT License
284 stars 142 forks source link

[Feature Request] Align the broker public API to MSAL .NET #731

Closed bgavrilMS closed 10 months ago

bgavrilMS commented 10 months ago

MSAL client type

Public

Problem Statement

https://learn.microsoft.com/en-us/entra/msal/dotnet/acquiring-tokens/desktop-mobile/wam

Current API

MsalRuntimeBroker broker = new MsalRuntimeBroker();

PublicClientApplication pca = PublicClientApplication.builder(clientId).
    authority(authority).
    broker(broker).
    build();

Proposed API


// at least 1 OS must be enabled, otherwise throw ex
// if Mac or Linux are enabled, throw NotImplementedException
Broker broker = Broker.builder().   
                                .windowsSupport(true)
                                .macOSSupport(true)
                                .linuxSupport(true)
                                .build();

PublicClientApplication pca = PublicClientApplication.builder(clientId).
    authority(authority).
    broker(broker).
    build();

Thoughts on the above @localden and @Avery-Dunn ?

localden commented 10 months ago

Any reason we don't do BrokerOptions similar to what we do in .NET? What way we don't need to implement 3 extra property setters?

Avery-Dunn commented 10 months ago

@localden : It's not entirely clear to me how the .NET version works or how it's meant to scale with more operating systems. I see BrokerOptions being used like this in a few place in the .NET codebase: pcaBuilder.WithBroker(new BrokerOptions(BrokerOptions.OperatingSystems.Windows)

To my Java brain, it seems like BrokerOptions has a constructor that either takes in one OS at a time, or that BrokerOptions.OperatingSystems will in some way be updated to have every permutation of OS choices (like BrokerOptions.OperatingSystems.WindowsAndLinux or something). There's also what looks like bit-based flags being used to track OS options, which isn't a common thing in Java.

The builder style that Bogdan suggested here and used throughout the rest of the library is more of what Java customers would be used to. I could always rename 'builder' to 'brokerOptions' to get the naming closer (Broker.brokerOptions()...), but I'm pretty sure that goes against some Java standards.

localden commented 10 months ago

@Avery-Dunn I'd definitely lean more on idiomatic Java constructs here, so if bitwise flags are not common in the language then maybe this is not the right approach. That being said, won't we be able to use something like EnumSet here?

Avery-Dunn commented 10 months ago

I haven't used EnumSet much, but if we use it then I believe the customer's code would be something like this: Broker broker = new Broker(EnumSet.of(OsOptions.WINDOWS, OsOptions.LINUX, OsOptions.XYZ));

(and then we have some extra logic in the Broker constructor to parse that EnumSet/use it throughout the code)

If that's similar to how .NET plans to do it then I'd support going with that option. My only concern would be that it's introducing a different style of object creation and customers don't like when things are different, however that shouldn't be too big of a deal since this API is part of a brand new package anyway.

localden commented 10 months ago

Yeah, the default builder pattern is better then @Avery-Dunn - I approved the PR.