-
Notifications
You must be signed in to change notification settings - Fork 146
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
Feat: Filter allowed CustomTabs browsers #353
Conversation
…in / Logout Adding support to override the browser packages to be used during Login / Logout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
proposed public API, subject to change
//Define the filter
BrowserPicker browserPicker = BrowserPicker.newBuilder()
.withAllowedPackages(Arrays.asList("com.google.chrome"))
.build();
//Set the filter
CustomTabsOptions customTabsOptions = CustomTabsOptions.newBuilder()
.withBrowserPicker(browserPicker)
.build();
//Launch the authentication
WebAuthProvider.init(account)
.withCustomTabsOptions(customTabsOptions)
.start(activity, callback, REQUEST_CODE);
auth0/src/main/java/com/auth0/android/provider/AuthenticationActivity.java
Outdated
Show resolved
Hide resolved
@@ -167,21 +155,6 @@ public void shouldInitWithContext() { | |||
assertNotNull(WebAuthProvider.getManagerInstance()); | |||
} | |||
|
|||
@SuppressWarnings("deprecation") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed as they are duplicate. see existing tests:
@Test
public void shouldFailToResumeLoginWithIntentWithoutFirstInitProvider() {
WebAuthProvider.resetManagerInstance();
Intent intent = createAuthIntent("");
assertFalse(WebAuthProvider.resume(intent));
}
@SuppressWarnings("deprecation")
@Test
public void shouldFailToResumeLoginWithRequestCodeWithoutFirstInitProvider() {
WebAuthProvider.resetManagerInstance();
Intent intent = createAuthIntent("");
assertFalse(WebAuthProvider.resume(REQUEST_CODE, Activity.RESULT_OK, intent));
}
public class BrowserPicker implements Parcelable { | ||
|
||
@NonNull | ||
private static final List<String> CHROME_BROWSERS = Arrays.asList( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are for the legacy scenarios
private Builder() { | ||
} | ||
|
||
//TODO: JAVADOCS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will add soon
|
||
//If the browser packages were filtered, use the allowed packages list as order preference. | ||
//A user-selected default browser will always be picked up first. | ||
List<String> preferenceList = allowedPackages; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the order of preference is dictated by either the "allowed packages" in the case that the developer has provided those, or the default browser + chrome browsers, in the case of the legacy scenario
manager.useFullScreen(true); | ||
assertTrue(manager.useFullScreen()); | ||
} | ||
|
||
@Test | ||
public void shouldNotHaveCustomTabsOptionsByDefault() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
@@ -121,13 +104,23 @@ public void shouldHaveValidState() { | |||
|
|||
@Test | |||
public void shouldHaveInvalidState() { | |||
exception.expect(AuthenticationException.class); | |||
OAuthManager.assertValidState("0987654321", "1234567890"); | |||
Assert.assertThrows(AuthenticationException.class, new ThrowingRunnable() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trying out the new way of asserting exceptions. The rule for "ExpectedException.none()" is deprecated
|
||
//Next line is needed to tell a Browser app is installed | ||
prepareBrowserApp(true, null); | ||
BrowserPickerTest.setupBrowserContext(activity, Arrays.asList("com.auth0.browser"), null, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tells that 1 browser, not custom tab compatible (doesn't matter), is installed
//** ** ** ** ** ** **// | ||
|
||
@Test | ||
public void shouldHaveBrowserAppInstalled() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved to BrowserPicker tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good, just one question about the removal of the setCustomTabsOptions
from the OAuthManager
.
@@ -110,14 +110,13 @@ public BrowserPicker build() { | |||
@Nullable | |||
String getBestBrowserPackage(@NonNull PackageManager pm) { | |||
Intent browserIntent = new Intent(Intent.ACTION_VIEW, Uri.parse("http://www.example.com")); | |||
ResolveInfo webHandler = pm.resolveActivity(browserIntent, | |||
Build.VERSION.SDK_INT >= Build.VERSION_CODES.M ? PackageManager.MATCH_ALL : PackageManager.MATCH_DEFAULT_ONLY); | |||
ResolveInfo webHandler = pm.resolveActivity(browserIntent, PackageManager.MATCH_DEFAULT_ONLY); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix: this line's purpose is to find the "default browser app" in the device. "Matching all" would return all the browsers and pick one from there, which is not always the default app.
String defaultBrowser = null; | ||
if (webHandler != null) { | ||
defaultBrowser = webHandler.activityInfo.packageName; | ||
} | ||
|
||
final List<ResolveInfo> availableBrowsers = pm.queryIntentActivities(browserIntent, 0); | ||
final List<ResolveInfo> availableBrowsers = pm.queryIntentActivities(browserIntent, Build.VERSION.SDK_INT >= Build.VERSION_CODES.M ? PackageManager.MATCH_ALL : 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix: when listing the installed browsers on marshmallow and above, the ALL flag should be used. Otherwise, the returned list has only one result.
@@ -327,7 +328,7 @@ static void setupBrowserContext(@NonNull Context context, @NonNull List<String> | |||
PackageManager pm = mock(PackageManager.class); | |||
when(context.getPackageManager()).thenReturn(pm); | |||
ResolveInfo defaultPackage = resolveInfoForPackageName(defaultBrowserPackage); | |||
when(pm.resolveActivity(any(Intent.class), anyInt())).thenReturn(defaultPackage); | |||
when(pm.resolveActivity(any(Intent.class), eq(PackageManager.MATCH_DEFAULT_ONLY))).thenReturn(defaultPackage); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better matchers, only by the values that we know we use. Will help catch changes like changing the prod code flags by mistake in the future 👍
Changes
Introduces a
BrowserPicker
class that can be set through theCustomTabsOptions
object used in theWebAuthProvider
methods. By constructing a new instance, the developers would be able to filter the browsers used to launch the web auth flow.Public API
Based on the linked PR, but using builders to make it extensible, more testable, and future proof.
If the user-selected default browser is custom tabs compatible and included in this "allowed packages" list, it will be picked. Otherwise, the order of the items in the "allowed packages" list matters and will be used to choose the first browser available that is contained there.
References
Supersedes #338
Testing
There is a lot of unit tests involved for keeping the legacy behavior untouched and for testing that the introduced filtering logic is there. I plan to test this manually before publishing it.
This change adds unit test coverage
This change adds integration test coverage
This change has been tested on the latest version of the platform/language or why not
Checklist
I have read the Auth0 general contribution guidelines
I have read the Auth0 Code of Conduct
All existing and new tests complete without errors