device code flow #7

SomkaPe · 2019-02-19T23:47:03Z

Polling implementation of device code flow.
Refactoring of Callable to Supplier.
Use acquireTokenByWindowsIntegratedAuth instead of acquireTokenByKerberosAuth

sangonzal · 2019-02-20T17:14:43Z

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

@@ -1,45 +1,19 @@
-// Copyright (c) Microsoft Corporation.


Still need a license here?

will change

sangonzal · 2019-02-20T17:15:28Z

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


        return updatedGrant;
    }
+


nit: extra line

sangonzal · 2019-02-20T17:16:52Z

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

        return future;
    }

+


nit: extra line

sangonzal · 2019-02-20T18:02:56Z

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

+        CompletableFuture<AuthenticationResult> future =
                executorService != null ? CompletableFuture.supplyAsync(supplier, executorService)
                        : CompletableFuture.supplyAsync(supplier);
+        futureReference.set(future);


This might be due to lack of knowledge around AtomicReferences, but I'm confused as why you need to set this here, since we are returning future and not futureReference.

I have used AtomicReference to make access to future thread safe, same could be done using volatile modifier

sangonzal · 2019-02-20T18:18:21Z

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

-import java.security.NoSuchAlgorithmException;

-class AcquireTokenCallable extends MsalCallable<AuthenticationResult> {
+public class AcquireTokenSupplier extends MsalSupplier {


Maybe AuthenticationResultSupplier instead of MsalSupplier?

Good point will change

Are you sure that makes sense? Isn't this kind of a base class for AquireToken ? It's not a base class for AuthenticationResult ?

In reply to: 258718420 [](ancestors = 258718420)

This is a base class for Task which produces(supply) AuthenticationResult

henrik-me · 2019-02-20T23:20:04Z

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

+import java.util.concurrent.CompletionException;
+import java.util.function.Supplier;
+
+abstract class MsalSupplier implements Supplier<AuthenticationResult> {


Supplier [](start = 39, length = 8)

interesting #Resolved

henrik-me · 2019-02-20T23:21:57Z

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

     *         Token, Refresh Token and the Access Token's expiration time.
     */
-    public CompletableFuture<AuthenticationResult> acquireTokenByKerberosAuth(Set<String> scopes, String username) {
+    public CompletableFuture<AuthenticationResult> acquireTokenByWindowsIntegratedAuth(Set<String> scopes, String username) {


WindowsIntegratedAuth [](start = 65, length = 21)

after a long naming discussion on this previously (analysis by searching google and bing for most used) : IntegratedWindowsAuth was what this landed on. I recommend calling it the same

ok, will rename

henrik-me · 2019-02-20T23:24:48Z

src/samples/public-client/DeviceCodeFlow.java

 import java.util.concurrent.Future;
+import java.util.function.Consumer;

 public class DeviceCodeFlow {


DeviceCodeFlow [](start = 13, length = 14)

super easy to understand sample code :)

henrik-me · 2019-02-20T23:25:46Z

public class IntegratedAuthFlow {

nit: Given the flow name has been updated should the class as well as the filename as well update?

Refers to: src/samples/public-client/IntegratedAuthFlow.java:30 in d8a9f35. [](commit_id = d8a9f35, deletion_comment = False)

henrik-me · 2019-02-20T23:27:41Z

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

+
+import static com.microsoft.aad.msal4j.AdalErrorCode.AUTHORIZATION_PENDING;
+
+public class AcquireTokenDeviceCodeFlowSupplier extends MsalSupplier {


should this extend from AquireTokenSupplier? (or based on the naming discussion from AuthenticationResultSupplier) ?

I renamed AquireTokenSupplier to AcquireTokenByAuthorisationGrantSupplier - which is a bit more precise(get token by auth grant from token endpoint), but AcquireTokenDeviceCodeFlowSupplier is different it is flow

henrik-me · 2019-02-20T23:29:33Z

It seems like logging was removed? Am I missing something?

henrik-me · 2019-02-20T23:31:10Z

src/test/java/com/microsoft/aad/msal4j/DeviceCodeFlowTest.java

+            Assert.assertNotNull(deviceCode.getExpiresIn(), "900");
+            Assert.assertNotNull(deviceCode.getInterval(), "5");
+            Assert.assertNotNull(deviceCode.getMessage(), "To sign in, use a web browser" +
+                    " to open the page https://aka.ms/devicelogin and enter the code DW83JNP2P to authenticate.");


what does the second parameter do here? You are asserting for not null, however not sure what that second parameter ads, is that some kind of expected value? If so shouldn't the comparison be made against the expected value?

Ohh, it is bug - should be assert equal

henrik-me · 2019-02-20T23:34:33Z

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

+    private ClientAuthentication clientAuth;
+    private String scopes;
+    private Consumer<DeviceCode> deviceCodeConsumer;
+    private AtomicReference<CompletableFuture<AuthenticationResult>> futureReference;


I would assume these are all relevant on the base object? Any reason those are defined here and not on the base? Especially scope is something I would expect to be everywhere

this is used to support cancellation of long running task - polling of Token and point for device code flow

navyasric · 2019-02-23T00:04:49Z

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

-import java.security.NoSuchAlgorithmException;

-class AcquireTokenCallable extends MsalCallable<AuthenticationResult> {
+public class AcquireTokenByAuthorisationGrantSupplier extends AuthenticationResultSupplier {


Nit pick - AcquireTokenByAuthorizationGrantSupplier

navyasric · 2019-02-23T00:18:12Z

src/test/java/com/microsoft/aad/msal4j/DeviceCodeFlowTest.java

+        PowerMock.replay(app);
+
+        AuthenticationResult authResult =
+                app.acquireTokenDeviceCodeFlow(Collections.singleton(AAD_RESOURCE_ID), deviceCodeConsumer).get();


acquireTokenByDeviceCodeFlow similar to acquireTokenByIntegratedWindowsAuth to be consistent with the acquireTokenByxxx format

ok, will rename

henrik-me

device code flow

7daad64

SomkaPe assigned navyasric, henrik-me and sangonzal Feb 19, 2019

SomkaPe requested review from sangonzal, navyasric and henrik-me February 19, 2019 23:51

SomkaPe unassigned navyasric, henrik-me and sangonzal Feb 19, 2019

sangonzal approved these changes Feb 20, 2019

View reviewed changes

henrik-me reviewed Feb 20, 2019

View reviewed changes

addressing pr comments

d8a9f35

henrik-me reviewed Feb 20, 2019

View reviewed changes

addressing pr comments

9d18101

navyasric suggested changes Feb 23, 2019

View reviewed changes

henrik-me approved these changes Feb 23, 2019

View reviewed changes

pr comments

6354ccf

SomkaPe requested a review from navyasric February 26, 2019 02:09

SomkaPe merged commit 6370d5b into dev Feb 27, 2019

sangonzal deleted the somkape/deviceCodePolling branch April 23, 2019 22:25


		import static com.microsoft.aad.msal4j.AdalErrorCode.AUTHORIZATION_PENDING;

		public class AcquireTokenDeviceCodeFlowSupplier extends MsalSupplier {

device code flow #7

device code flow #7

Uh oh!

Conversation

SomkaPe commented Feb 19, 2019

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!

SomkaPe Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

SomkaPe Feb 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henrik-me Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

henrik-me commented Feb 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henrik-me commented Feb 20, 2019

Uh oh!

henrik-me Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

henrik-me left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SomkaPe Feb 20, 2019 •

edited

Loading

SomkaPe Feb 21, 2019 •

edited

Loading

henrik-me Feb 20, 2019 •

edited

Loading

henrik-me Feb 20, 2019 •

edited

Loading