MSALRuntime integration by Avery-Dunn · Pull Request #530 · AzureAD/microsoft-authentication-library-for-java

Avery-Dunn · 2022-08-31T18:15:27Z

Draft PR for the work-in-progress MSALRuntime integration. Most of the new files will be split off into either a new Broker repository or the existing MSAL C++ repository, and are just here for easier initial code review

This PR currently has:

IBroker.java an interface used to reference methods implemented by a broker package, such as our upcoming WAMBroker that allows using WAM via MSALRuntime
Changes in some existing flows to demonstrate the use of IBroker
MSALRuntimeLibrary.java, which will be used to make the actual calls to the MSALRuntime API and will be placed in the MSAL C++ repo (where it will be known as the 'interop layer', and the PR for it is: https://github.com/AzureAD/microsoft-authentication-library-for-cpp/pull/3091)

…tion-library-for-java into avdunn/msalruntime-integration

JonathanGiles · 2022-08-31T22:07:58Z

src/main/java/com/microsoft/aad/msal4j/IBroker.java

+/**
+ * Used to define the basic set of methods that all Brokers must implement
+ */
+public interface IBroker {


The I prefix is quite an outdated approach. I would suggest you consider calling this Broker instead.

Agreed that it is an outdated approach but we try to be in sync with other MSALs. This IBroker interface is introduced to be in sync with MSAL .NET.

I would argue to not follow conventions of another language, if you can help it.

I also agree that it's outdated, but it's also something that all the other interfaces in our library follow. Consistency with the other MSALs is also a big focus for this feature, since most of the libraries will be also be integrating with MSALRuntime we want to keep things as similar as we can

JonathanGiles · 2022-08-31T22:10:01Z

src/main/java/com/microsoft/aad/msal4j/IBroker.java

+    Account signInSilently(PublicClientApplication clientApplication, InteractiveRequestParameters interactiveRequestParameters);
+
+    Account signInInteractively(PublicClientApplication clientApplication, InteractiveRequestParameters interactiveRequestParameters);
+
+    AuthenticationResult acquireTokenSilently(PublicClientApplication clientApplication, InteractiveRequestParameters interactiveRequestParameters, Account account);
+
+    AuthenticationResult acquireTokenInteractively(PublicClientApplication clientApplication, InteractiveRequestParameters interactiveRequestParameters, Account account);
+
+    void signOutSilently(PublicClientApplication clientApplication, InteractiveRequestParameters interactiveRequestParameters);
+
+    //TODO: too specific to MSALRuntime? Should it be split into a generic getAccount(something) and getAllAccounts(clientId)?
+    void discoverAccounts(PublicClientApplication clientApplication, InteractiveRequestParameters interactiveRequestParameters);


Because all methods seem to require a PublicClientApplication, I wonder if this should be a constructor parameter in implementations of this interface? Do you expect for an instance of Broker that the PublicClientApplication will always be the same?

This has been completely refactored from this initial generic design to something more like the rest of MSAL Java, where there are a series of acquireToken() methods overloaded for each type of request (leaving it up to a broker to figure out how to handle each type of request).

JonathanGiles · 2022-08-31T22:11:07Z

src/main/java/com/microsoft/aad/msal4j/AcquireTokenByInteractiveFlowSupplier.java

-        AuthorizationResult authorizationResult = getAuthorizationResult();
-        validateState(authorizationResult);
-        return acquireTokenWithAuthorizationCode(authorizationResult);
+        if (clientApplication.allowBroker()) {


We should have boolean getters prefixed with is, so perhaps this method should be named isBrokerAllowed() or similar?

This is just the flag decided on in the design doc (mainly according to conventions in Python and .NET since they were the frontrunners on this). Normally I'd agree that it's weird and change it to something like 'isBrokerAllowed', but consistency across MSALs is a focus on this project, and we other parameters like this in our library already so it shouldn't be too confusing.

JonathanGiles · 2022-08-31T22:11:53Z

src/main/java/com/microsoft/aad/msal4j/MSALRuntimeAuthResult.java

+    public LongByReference authParamsHandle;
+    public LongByReference accountHandle;
+    public LongByReference authResultHandle;
+    public MSALRuntimeUtils.StringByReference idToken = new MSALRuntimeUtils.StringByReference(new WString(""));
+    public MSALRuntimeUtils.StringByReference accessToken = new MSALRuntimeUtils.StringByReference(new WString(""));
+    public LongByReference accessTokenExpirationTime;
+    public MSALRuntimeUtils.StringByReference accountId = new MSALRuntimeUtils.StringByReference(new WString(""));//TODO: split AuthenticationResult into SignInResult (id token/account) and TokenResult (access tokens)? Or keep it simple all-in-one like MSAL Java's AuthenticationResult?
+    public MSALRuntimeUtils.StringByReference accountClientInfo = new MSALRuntimeUtils.StringByReference(new WString(""));


We shouldn't have public instance fields in our APIs, if at all possible

These fields (and the class itself) need to be public due to how JNA handles converting Java classes to C structs. Basically, JNA will ignore any field that isn't public when creating the native struct, so anything on the Java side tried to access native structs JNA is handling you'd get an error about the struct not having a field named XYZ.

src/main/java/com/microsoft/aad/msal4j/MSALRuntimeAuthResult.java

src/main/java/com/microsoft/aad/msal4j/MSALRuntimeUtils.java

…d on code review

Avery-Dunn · 2022-10-23T23:48:36Z

Enough design changes have happened to make this version obsolete, new PR IBroker and it's implementation for MSALRuntime can be found here: #563

Avery-Dunn added 2 commits August 25, 2022 13:54

Initial code for MSALRuntime integration

285ab08

Merge branch 'dev' of https://github.com/AzureAD/microsoft-authentica…

ef604a5

…tion-library-for-java into avdunn/msalruntime-integration

JonathanGiles reviewed Aug 31, 2022

View reviewed changes

Avery-Dunn added 2 commits September 11, 2022 20:29

Finished implementation for silent/interactive flows, refactored base…

b4b9335

…d on code review

Add defaults to IBroker

0c7633f

Avery-Dunn closed this Oct 23, 2022

Avery-Dunn deleted the avdunn/msalruntime-integration branch January 31, 2023 22:11

Conversation

Avery-Dunn commented Aug 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Avery-Dunn commented Oct 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Avery-Dunn commented Aug 31, 2022 •

edited

Loading