Fix Web Authentication issues #169

lbalmaceda · 2018-07-10T23:12:23Z

Occasionally, no browser app was installed in the device triggering the following exception:

android.content.ActivityNotFoundException: No Activity found to handle Intent { act=android.intent.action.VIEW dat=https://lbalmaceda.auth0.com/authorize?... }

While this is a border case scenario, it can still happen on some modified ROMs where the user is allowed to remove even the default browser app.

Furthermore, when picking the right Custom Tab compatible browser there was a bug where the default browser might be picked even if it was not compatible. Attempting to bind to this app's Custom Tab service resulted in the following exception:

java.lang.IllegalArgumentException: Service Intent must be explicit: Intent { act=android.support.customtabs.action.CustomTabsService }

Both issues should be fixed on this PR.

Closes #138

…h callback.

…alled. Improve testing

lbalmaceda · 2018-07-10T23:12:55Z

auth0/src/main/java/com/auth0/android/provider/CustomTabsController.java

@@ -93,7 +93,7 @@ public void bindService() {
        Log.v(TAG, "Trying to bind the service");
        Context context = this.context.get();
        boolean success = false;
-        if (context != null) {
+        if (context != null && preferredPackage != null) {


no need to bind if there's no available service

lbalmaceda · 2018-07-10T23:13:13Z

auth0/src/main/java/com/auth0/android/provider/CustomTabsController.java

@@ -133,7 +133,7 @@ public void launchUri(@NonNull final Uri uri) {
            public void run() {
                boolean available = false;
                try {
-                    available = sessionLatch.await(MAX_WAIT_TIME_SECONDS, TimeUnit.SECONDS);
+                    available = sessionLatch.await(preferredPackage == null ? 0 : MAX_WAIT_TIME_SECONDS, TimeUnit.SECONDS);


no need to wait for the service connection if not available

lbalmaceda · 2018-07-10T23:14:20Z

auth0/src/main/java/com/auth0/android/provider/CustomTabsController.java

-                    context.startActivity(fallbackIntent);
+                } catch (ActivityNotFoundException ex) {
+                    Log.e(TAG, "No Browser available to open the URI");
+                    throw new IllegalStateException("Could not find any Browser application installed in this device to handle the intent.", ex);


While this would 💥, there's a check performed earlier in the code for this not to happen.

@lbalmaceda I would prefer that you Log.e() this rather than throw the IllegalStateException. We would prefer that over any possibility of a crash. This method, launchUri(), is called from your AuthenticationActivity. As such, it's impossible to catch the exception (without using an UncaughtExceptionHandler).

@billgockeler The only public facing class is WebAuthProvider; then OAuthManager and CustomTabsController are package-private and used internally by this lib. This exception is thrown as part of the Controller logic if no Browser app was installed at the moment the Uri was meant to be launched. However, a previous check at WebAuthProvider.Builder#start asserts a browser is available or calls the callback with the exception, this happens here. So there's no way this library's WebAuth could be used without that check being performed first. The AuthenticationActivity is not meant to be called directly by you.

That said, I don't agree this exception should be caught silently since as I said originally this is an edge case that it doesn't make sense to support. e.g. if there's no browser, the authentication can't be done at all. However, this case is already handled for you and you'll receive the onFailure call if that happens.

@lbalmaceda I fully understand that we're not calling AuthenticationActivity directly. But I think perhaps you're not understanding my point so let me try this another way.

What options do we have to catch the IllegalStateException that might be thrown by CustomTabsController? The answer is none (without installing an UncaughtExceptionHandler). If thrown, it will result in a crash in our app, just as we've been dealing with for months. This is not acceptable.

I understand your code and with this fix the IllegalStateException should never be thrown. If so, then why throw it?

I'd like this to fail fast, as soon as we can check if a browser is present. We do that in the builder when the start method is called. We can fail safely since the callback is on scope. Not doing it early would mean a lot of instances are getting created (WebAuthProvider, OAuthManager, CustomTabsController, the activity, etc) when we know in advance it will fail, so it's a waste of resources IMO. e.g. we could capture this exception on the AuthenticationActivity and notify somehow the WebAuthProvider that it couldn't even be launched because no browser is installed... Waste of resources 🌮

I thought of keeping it as part of this "internal class" logic, since the method is "failing" and someone should be notified about that. I do agree that throwing an exception deliberately without catching it is not correct, and I'm not going to catch it there because of what I explained above. Since we both agree it's unreachable code, as I'm checking for browser apps ASAP and failing safely, the 2 options left are either keep or remove this uncaught exception. I've removed it already ✌️ Thanks for the comments

lbalmaceda · 2018-07-10T23:14:43Z

auth0/src/main/java/com/auth0/android/provider/CustomTabsController.java

@@ -191,7 +191,7 @@ static String getBestBrowserPackage(@NonNull Context context) {
        } else if (!customTabsBrowsers.isEmpty()) {
            return customTabsBrowsers.get(0);
        } else {
-            return defaultBrowser;
+            return null;


can return null meaning no "custom tab compatible browser" is installed

lbalmaceda · 2018-07-10T23:15:11Z

auth0/src/main/java/com/auth0/android/provider/WebAuthProvider.java

@@ -267,6 +269,11 @@ Builder withPKCE(PKCE pkce) {
            return this;
        }

+        static boolean hasBrowserAppInstalled(@NonNull PackageManager packageManager) {
+            Intent intent = new Intent(Intent.ACTION_VIEW, Uri.parse("https://auth0.com"));
+            return intent.resolveActivity(packageManager) != null;


a valid "browser app" can resolve this basic intent

lbalmaceda · 2018-07-10T23:16:46Z

auth0/src/test/java/com/auth0/android/provider/CustomTabsControllerTest.java

        controller = new CustomTabsController(context, DEFAULT_BROWSER_PACKAGE);
    }

    @Test
    public void shouldNotHaveCustomizationOptionsSetByDefault() throws Exception {
-        controller = new CustomTabsController(context, DEFAULT_BROWSER_PACKAGE);
+        CustomTabsController controller = new CustomTabsController(context, DEFAULT_BROWSER_PACKAGE);


don't overwrite the class field

lbalmaceda · 2018-07-10T23:17:47Z

auth0/src/test/java/com/auth0/android/provider/CustomTabsControllerTest.java

@@ -181,6 +183,7 @@ public void shouldBindAndLaunchUri() throws Exception {
        verify(context, timeout(MAX_TEST_WAIT_TIME_MS)).startActivity(launchIntentCaptor.capture());
        Intent intent = launchIntentCaptor.getValue();
        assertThat(intent.getAction(), is(Intent.ACTION_VIEW));
+        assertThat(intent.getPackage(), is(DEFAULT_BROWSER_PACKAGE));


a specified package would imply that this intent is targeting a specific browser app (the custom tab compatible found one)

lbalmaceda · 2018-07-10T23:18:36Z

auth0/src/test/java/com/auth0/android/provider/CustomTabsControllerTest.java

+        Intent intent = launchIntentCaptor.getValue();
+        assertThat(intent.getAction(), is(Intent.ACTION_VIEW));
+        //A null package name would make the OS decide the best app to resolve the intent
+        assertThat(intent.getPackage(), is(nullValue()));


like the comment says, no package means the OS would pick the best app or prompt the user to pick one.

lbalmaceda · 2018-07-10T23:18:56Z

auth0/src/test/java/com/auth0/android/provider/CustomTabsControllerTest.java

@@ -241,31 +259,26 @@ public void shouldLaunchUriWithFallbackIfCustomTabIntentFails() throws Exception
                .when(context).startActivity(any(Intent.class));
        controller.launchUri(uri);

-        verify(context, new Timeout(MAX_TEST_WAIT_TIME_MS, VerificationModeFactory.times(2))).startActivity(launchIntentCaptor.capture());
+        verify(context, timeout(MAX_TEST_WAIT_TIME_MS)).startActivity(launchIntentCaptor.capture());


needed because of threads 🤷‍♂️

lbalmaceda · 2018-07-10T23:19:57Z

auth0/src/test/java/com/auth0/android/provider/CustomTabsControllerTest.java

+        CustomTabsSession session = mock(CustomTabsSession.class);
+        ComponentName componentName = new ComponentName(DEFAULT_BROWSER_PACKAGE, DEFAULT_BROWSER_PACKAGE + ".CustomTabsService");
+        //This depends on an implementation detail but is the only way to test it because of methods visibility
+        PowerMockito.when(session, "getComponentName").thenReturn(componentName);


required to assert the package later in tests

billgockeler · 2018-07-11T15:22:51Z

auth0/src/main/java/com/auth0/android/provider/CustomTabsController.java

-                    context.startActivity(fallbackIntent);
+                } catch (ActivityNotFoundException ex) {
+                    Log.e(TAG, "No Browser available to open the URI");
+                    throw new IllegalStateException("Could not find any Browser application installed in this device to handle the intent.", ex);


@lbalmaceda I would prefer that you Log.e() this rather than throw the IllegalStateException. We would prefer that over any possibility of a crash. This method, launchUri(), is called from your AuthenticationActivity. As such, it's impossible to catch the exception (without using an UncaughtExceptionHandler).

cocojoe

Strange situation to have no browser on an Android device. Thanks for leaving the comments as it's not as easy for me to follow. However, it makes sense and reflected in tests.

lbalmaceda · 2018-07-12T18:12:45Z

@cocojoe can I get your blessing again, please? ✨

cocojoe · 2018-07-13T08:53:18Z

Changes are still approved, fix your CI

lbalmaceda added 3 commits July 10, 2018 19:57

add new internal code to AuthenticationException

20dbdfe

Make the WebAuthProvider check for Browser apps on start and fail wit…

b6ac85f

…h callback.

Avoid binding the service if no Custom Tab compatible browser is inst…

262e763

…alled. Improve testing

lbalmaceda added the CH: Fixed label Jul 10, 2018

lbalmaceda added this to the v1-Next milestone Jul 10, 2018

add missing test

fd12f1e

lbalmaceda commented Jul 10, 2018

View reviewed changes

billgockeler suggested changes Jul 11, 2018

View reviewed changes

cocojoe approved these changes Jul 12, 2018

View reviewed changes

lbalmaceda added 2 commits July 12, 2018 11:47

add VisibleForTesting annotation

8661af7

dont throw an internal exception if the uri cant be launched

e89ed89

lbalmaceda merged commit 928a261 into master Jul 13, 2018

lbalmaceda deleted the fix-webauth branch July 13, 2018 15:19

lbalmaceda mentioned this pull request Jul 13, 2018

CustomTabsController crash auth0/Lock.Android#464

Closed

lbalmaceda modified the milestones: v1-Next, 1.13.1 Jul 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Web Authentication issues #169

Fix Web Authentication issues #169

lbalmaceda commented Jul 10, 2018

lbalmaceda Jul 10, 2018

lbalmaceda Jul 10, 2018

lbalmaceda Jul 10, 2018

billgockeler Jul 11, 2018

lbalmaceda Jul 11, 2018

billgockeler Jul 12, 2018 •

edited

Loading

lbalmaceda Jul 12, 2018

lbalmaceda Jul 10, 2018

lbalmaceda Jul 10, 2018

lbalmaceda Jul 10, 2018

lbalmaceda Jul 10, 2018

lbalmaceda Jul 10, 2018

lbalmaceda Jul 10, 2018

lbalmaceda Jul 10, 2018

billgockeler Jul 11, 2018

cocojoe left a comment

lbalmaceda commented Jul 12, 2018

cocojoe commented Jul 13, 2018

Fix Web Authentication issues #169

Fix Web Authentication issues #169

Conversation

lbalmaceda commented Jul 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billgockeler Jul 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cocojoe left a comment

Choose a reason for hiding this comment

lbalmaceda commented Jul 12, 2018

cocojoe commented Jul 13, 2018

billgockeler Jul 12, 2018 •

edited

Loading