Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CONTRIBUTORS
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,4 @@ Alex Chau <alexchau@google.com>
Benjamin Franz <bfranz@google.com>
Rebecka Gulliksson <rebecka.gulliksson@gmail.com>
Rahul Ravikumar <rahulrav@google.com>
Henning Nielsen Lund <henning.n.lund@jp.dk>
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@
*
* // blacklist Firefox
* new BrowserBlacklist(
* VersionedBrowserMatcher.FIREFOX_BROWSER);
* VersionedBrowserMatcher.FIREFOX_BROWSER,
* VersionedBrowserMatcher.FIREFOX_CUSTOM_TAB);
*
* // blacklist Dolphin Browser
* new BrowserBlacklist(
Expand Down
33 changes: 31 additions & 2 deletions library/java/net/openid/appauth/browser/BrowserSelector.java
Original file line number Diff line number Diff line change
Expand Up @@ -76,11 +76,21 @@ public final class BrowserSelector {
public static List<BrowserDescriptor> getAllBrowsers(Context context) {
PackageManager pm = context.getPackageManager();
List<BrowserDescriptor> browsers = new ArrayList<>();
String defaultBrowserPackage = null;

int queryFlag = PackageManager.GET_RESOLVED_FILTER;
if (VERSION.SDK_INT >= VERSION_CODES.M) {
queryFlag |= PackageManager.MATCH_ALL;
}
// When requesting all matching activities for an intent from the package manager,
// the user's preferred browser is not guaranteed to be at the head of this list.
// Therefore, the preferred browser must be separately determined and the resultant
// list of browsers reordered to restored this desired property.
ResolveInfo resolvedDefaultActivity =
pm.resolveActivity(BROWSER_INTENT, 0);
if (resolvedDefaultActivity != null) {
defaultBrowserPackage = resolvedDefaultActivity.activityInfo.packageName;
}
List<ResolveInfo> resolvedActivityList =
pm.queryIntentActivities(BROWSER_INTENT, queryFlag);

Expand All @@ -91,15 +101,34 @@ public static List<BrowserDescriptor> getAllBrowsers(Context context) {
}

try {
int defaultBrowserIndex = 0;
PackageInfo packageInfo = pm.getPackageInfo(
info.activityInfo.packageName,
PackageManager.GET_SIGNATURES);

if (hasWarmupService(pm, info.activityInfo.packageName)) {
browsers.add(new BrowserDescriptor(packageInfo, true));
BrowserDescriptor customTabBrowserDescriptor =

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add tests to BrowserSelectorTest that demonstrate:

  • If the user has no default browser, the order of the returned browsers matches the order of matching intents returned by queryIntentActivities. This case is likely already covered by the existing tests, but it will help to make that more explicit.
  • If the user has a default browser, with custom tab support, the two entries for that browser are at the start of the list, with the custom tab descriptor first.
  • If the user has a default browser, without custom tab support, that one entry for that browser is at the start of the list.

The code looks correct to me, but I just want to make sure that the changes are appropriately covered by tests so that future changes don't result in regressions.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests have now been added.

new BrowserDescriptor(packageInfo, true);
if (info.activityInfo.packageName.equals(defaultBrowserPackage)) {
// If the default browser is having a WarmupService,
// will it be added to the beginning of the list.
browsers.add(defaultBrowserIndex, customTabBrowserDescriptor);
defaultBrowserIndex++;
} else {
browsers.add(customTabBrowserDescriptor);
}
}

browsers.add(new BrowserDescriptor(packageInfo, false));
BrowserDescriptor fullBrowserDescriptor =
new BrowserDescriptor(packageInfo, false);
if (info.activityInfo.packageName.equals(defaultBrowserPackage)) {
// The default browser is added to the beginning of the list.
// If there is support for Custom Tabs, will the one disabling Custom Tabs
// be added as the second entry.
browsers.add(defaultBrowserIndex, fullBrowserDescriptor);
} else {
browsers.add(fullBrowserDescriptor);
}
} catch (NameNotFoundException e) {
// a descriptor cannot be generated without the package info
}
Expand Down
16 changes: 15 additions & 1 deletion library/java/net/openid/appauth/browser/Browsers.java
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ public static BrowserDescriptor standaloneBrowser(@NonNull String version) {
}

/**
* Creates a browser descriptor fot the specified version of Chrome, when used as
* Creates a browser descriptor for the specified version of Chrome, when used as
* a custom tab.
*/
public static BrowserDescriptor customTab(@NonNull String version) {
Expand Down Expand Up @@ -97,6 +97,12 @@ public static final class Firefox {
public static final Set<String> SIGNATURE_SET =
Collections.singleton(SIGNATURE_HASH);

/**
* The version in which Custom Tabs were introduced in Firefox.
*/
public static final DelimitedVersion MINIMUM_VERSION_FOR_CUSTOM_TAB =
DelimitedVersion.parse("57");

/**
* Creates a browser descriptor for the specified version of Firefox, when used
* as a standalone browser.
Expand All @@ -105,6 +111,14 @@ public static BrowserDescriptor standaloneBrowser(@NonNull String version) {
return new BrowserDescriptor(PACKAGE_NAME, SIGNATURE_SET, version, false);
}

/**
* Creates a browser descriptor for the specified version of Firefox, when used as
* a custom tab.
*/
public static BrowserDescriptor customTab(@NonNull String version) {
return new BrowserDescriptor(PACKAGE_NAME, SIGNATURE_SET, version, true);
}

private Firefox() {
// no need to construct this class
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,15 @@ public class VersionedBrowserMatcher implements BrowserMatcher {
false,
VersionRange.ANY_VERSION);

/**
* Matches any version of Firefox for use as a custom tab.
*/
public static final VersionedBrowserMatcher FIREFOX_CUSTOM_TAB = new VersionedBrowserMatcher(
Browsers.Firefox.PACKAGE_NAME,
Browsers.Firefox.SIGNATURE_SET,
true,
VersionRange.atLeast(Browsers.Firefox.MINIMUM_VERSION_FOR_CUSTOM_TAB));

/**
* Matches any version of Mozilla Firefox.
*/
Expand Down
102 changes: 101 additions & 1 deletion library/javatests/net/openid/appauth/browser/BrowserSelectorTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@

package net.openid.appauth.browser;

import static junit.framework.Assert.assertFalse;
import static junit.framework.Assert.assertTrue;
import static net.openid.appauth.browser.BrowserSelector.BROWSER_INTENT;
import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.ArgumentMatchers.argThat;
import static org.mockito.ArgumentMatchers.eq;
Expand Down Expand Up @@ -68,6 +71,13 @@ public class BrowserSelectorTest {
.addSignature("FirefoxSignature")
.build();

private static final TestBrowser FIREFOX_CUSTOM_TAB =
new TestBrowserBuilder("org.mozilla.firefox")
.withBrowserDefaults()
.setVersion("57")
.addSignature("FirefoxSignature")
.build();

private static final TestBrowser DOLPHIN =
new TestBrowserBuilder("mobi.mgeek.TunnyBrowser")
.withBrowserDefaults()
Expand Down Expand Up @@ -195,6 +205,96 @@ public void testSelect_noMatchingBrowser() throws NameNotFoundException {
VersionRange.ANY_VERSION));
}

@Test
public void testSelect_defaultBrowserSetNoneSupporting() throws NameNotFoundException {
// Chrome is set as the users default browser, but the version is not supporting Custom Tabs
// BrowserSelector.getAllBrowsers will result in a list, where the Dolphin browser is the
// first element and the other browser, in this case Firefox, as the second element in the list.
setBrowserList(FIREFOX, CHROME);
setBrowsersWithWarmupSupport(NO_BROWSERS);
when(mContext.getPackageManager().resolveActivity(BROWSER_INTENT, 0))
.thenReturn(CHROME.mResolveInfo);
List<BrowserDescriptor> allBrowsers = BrowserSelector.getAllBrowsers(mContext);

assertThat(allBrowsers.get(0).packageName.equals(CHROME.mPackageName));
assertFalse(allBrowsers.get(0).useCustomTab);
assertThat(allBrowsers.get(1).packageName.equals(FIREFOX.mPackageName));
assertFalse(allBrowsers.get(1).useCustomTab);
}

@Test
public void testSelect_defaultBrowserNoCustomTabs() throws NameNotFoundException {
// Firefox is set as the users default browser, but the version is not supporting Custom Tabs
// BrowserSelector.getAllBrowsers will result in a list, where the Firefox browser is the
// first element and the other browser, in this case Chrome, as the second element in the list.
setBrowserList(CHROME, FIREFOX);
setBrowsersWithWarmupSupport(CHROME);
when(mContext.getPackageManager().resolveActivity(BROWSER_INTENT, 0))
.thenReturn(FIREFOX.mResolveInfo);
List<BrowserDescriptor> allBrowsers = BrowserSelector.getAllBrowsers(mContext);

assertThat(allBrowsers.get(0).packageName.equals(FIREFOX.mPackageName));
assertFalse(allBrowsers.get(0).useCustomTab);
assertThat(allBrowsers.get(1).packageName.equals(CHROME.mPackageName));
assertTrue(allBrowsers.get(1).useCustomTab);
}

@Test
public void testSelect_selectDefaultBrowserCustomTabs() throws NameNotFoundException {
// Firefox is set as the users default browser, supporting Custom Tabs
// BrowserSelector.getAllBrowsers will result in a list, where the Firefox browser is the
// first element two elements in the list and the other browser, in this case Chrome,
// as the third element in the list.
setBrowserList(CHROME, FIREFOX_CUSTOM_TAB);
setBrowsersWithWarmupSupport(CHROME, FIREFOX_CUSTOM_TAB);
when(mContext.getPackageManager().resolveActivity(BROWSER_INTENT, 0))
.thenReturn(FIREFOX_CUSTOM_TAB.mResolveInfo);
List<BrowserDescriptor> allBrowsers = BrowserSelector.getAllBrowsers(mContext);

assertThat(allBrowsers.get(0).packageName.equals(FIREFOX_CUSTOM_TAB.mPackageName));
assertTrue(allBrowsers.get(0).useCustomTab);
assertThat(allBrowsers.get(1).packageName.equals(FIREFOX_CUSTOM_TAB.mPackageName));
assertFalse(allBrowsers.get(1).useCustomTab);
assertThat(allBrowsers.get(2).packageName.equals(CHROME.mPackageName));
assertTrue(allBrowsers.get(2).useCustomTab);
}

@Test
public void testSelect_selectDefaultBrowserSetNoneSupporting() throws NameNotFoundException {
// Chrome is set as the users default browser, none of the browsers support Custom Tabs
// BrowserSelector.select will return Chrome as it the default browser.
setBrowserList(FIREFOX, CHROME);
setBrowsersWithWarmupSupport(NO_BROWSERS);
when(mContext.getPackageManager().resolveActivity(BROWSER_INTENT, 0))
.thenReturn(CHROME.mResolveInfo);

checkSelectedBrowser(CHROME, USE_STANDALONE);
}

@Test
public void testSelect_selectDefaultBrowserNoCustomTabs() throws NameNotFoundException {
// Firefox is set as the users default browser, but the version is not supporting Custom Tabs
// BrowserSelector.select will return Chrome as it is supporting Custom Tabs.
setBrowserList(CHROME, FIREFOX);
setBrowsersWithWarmupSupport(CHROME);
when(mContext.getPackageManager().resolveActivity(BROWSER_INTENT, 0))
.thenReturn(FIREFOX.mResolveInfo);

checkSelectedBrowser(CHROME, USE_CUSTOM_TAB);
}

@Test
public void testSelect_defaultBrowserCustomTabs() throws NameNotFoundException {
// Firefox is set as the users default browser, supporting Custom Tabs
// BrowserSelector.select will return Firefox.
setBrowserList(CHROME, FIREFOX_CUSTOM_TAB);
setBrowsersWithWarmupSupport(CHROME, FIREFOX_CUSTOM_TAB);
when(mContext.getPackageManager().resolveActivity(BROWSER_INTENT, 0))
.thenReturn(FIREFOX_CUSTOM_TAB.mResolveInfo);

checkSelectedBrowser(FIREFOX_CUSTOM_TAB, USE_CUSTOM_TAB);
}

/**
* Browsers are expected to be in priority order, such that the default would be first.
*/
Expand All @@ -214,7 +314,7 @@ private void setBrowserList(TestBrowser... browsers) throws NameNotFoundExceptio
}

when(mPackageManager.queryIntentActivities(
BrowserSelector.BROWSER_INTENT,
BROWSER_INTENT,
PackageManager.GET_RESOLVED_FILTER))
.thenReturn(resolveInfos);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ public void testMatches_emptyWhitelist() {
BrowserWhitelist whitelist = new BrowserWhitelist();
assertThat(whitelist.matches(Browsers.Chrome.customTab("46"))).isFalse();
assertThat(whitelist.matches(Browsers.Firefox.standaloneBrowser("10"))).isFalse();
assertThat(whitelist.matches(Browsers.Firefox.customTab("57"))).isFalse();
assertThat(whitelist.matches(Browsers.SBrowser.standaloneBrowser("11"))).isFalse();
}

Expand All @@ -40,6 +41,7 @@ public void testMatches_chromeBrowserOnly() {
assertThat(whitelist.matches(Browsers.Chrome.standaloneBrowser("46"))).isTrue();
assertThat(whitelist.matches(Browsers.Chrome.customTab("46"))).isFalse();
assertThat(whitelist.matches(Browsers.Firefox.standaloneBrowser("10"))).isFalse();
assertThat(whitelist.matches(Browsers.Firefox.customTab("57"))).isFalse();
}

@Test
Expand All @@ -50,17 +52,21 @@ public void testMatches_chromeCustomTabOrBrowser() {
assertThat(whitelist.matches(Browsers.Chrome.standaloneBrowser("46"))).isTrue();
assertThat(whitelist.matches(Browsers.Chrome.customTab("46"))).isTrue();
assertThat(whitelist.matches(Browsers.Firefox.standaloneBrowser("10"))).isFalse();
assertThat(whitelist.matches(Browsers.Firefox.customTab("57"))).isFalse();
}

@Test
public void testMatches_firefoxOrSamsung() {
BrowserWhitelist whitelist = new BrowserWhitelist(
VersionedBrowserMatcher.FIREFOX_BROWSER,
VersionedBrowserMatcher.FIREFOX_CUSTOM_TAB,
VersionedBrowserMatcher.SAMSUNG_BROWSER,
VersionedBrowserMatcher.SAMSUNG_CUSTOM_TAB);
assertThat(whitelist.matches(Browsers.Chrome.standaloneBrowser("46"))).isFalse();
assertThat(whitelist.matches(Browsers.Chrome.customTab("46"))).isFalse();
assertThat(whitelist.matches(Browsers.Firefox.standaloneBrowser("10"))).isTrue();
assertThat(whitelist.matches(Browsers.Firefox.customTab("56"))).isFalse();
assertThat(whitelist.matches(Browsers.Firefox.customTab("57"))).isTrue();
assertThat(whitelist.matches(Browsers.SBrowser.standaloneBrowser("10"))).isTrue();
assertThat(whitelist.matches(Browsers.SBrowser.customTab("10"))).isTrue();
}
Expand Down