Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2739003002:
Change WebView's Safe Browsing opt-in requirements (Closed)
|
Created:
3 years, 9 months ago by Nate Fischer Modified:
3 years, 9 months ago CC:
agrieve+watch_chromium.org, android-webview-reviews_chromium.org, chromium-reviews, vmpstr+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChange WebView's Safe Browsing opt-in requirements
Safe Browsing will now be enabled for any app which opts in by setting
the flag `android.webkit.WebView.EnableSafeBrowsing` to "true",
regardless of Android version or Target SDK.
BUG=699193
Review-Url: https://codereview.chromium.org/2739003002
Cr-Commit-Position: refs/heads/master@{#455958}
Committed: https://chromium.googlesource.com/chromium/src/+/e2521352441e0a396674c0d190e2c2f836ece010
Patch Set 1 #
Total comments: 4
Patch Set 2 : Remove TODO, combine appHasMetadataKeyValue() and appHasOptedIn() #
Total comments: 9
Patch Set 3 : Removing targetSdk logic #Messages
Total messages: 20 (8 generated)
The CQ bit was checked by ntfschr@chromium.org to run a CQ dry run
ntfschr@chromium.org changed reviewers: + sgurun@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL
https://codereview.chromium.org/2739003002/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwSafeBrowsingConfigHelper.java (right): https://codereview.chromium.org/2739003002/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwSafeBrowsingConfigHelper.java:45: return appHasMetadataKeyValue(appContext, OPT_IN_META_DATA_STR, true); no need to split into a separate method. https://codereview.chromium.org/2739003002/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwSafeBrowsingConfigHelper.java:48: // TODO(ntfschr): add support for Android N and lower (crbug/696113) no need to add todo. this is a major feature request tracked in crbug.
PTAL https://codereview.chromium.org/2739003002/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwSafeBrowsingConfigHelper.java (right): https://codereview.chromium.org/2739003002/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwSafeBrowsingConfigHelper.java:45: return appHasMetadataKeyValue(appContext, OPT_IN_META_DATA_STR, true); On 2017/03/08 at 23:37:07, sgurun wrote: > no need to split into a separate method. I merged this and appHasMetadataKeyValue(). I think this is the best approach since we're only checking for opt-in. https://codereview.chromium.org/2739003002/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwSafeBrowsingConfigHelper.java:48: // TODO(ntfschr): add support for Android N and lower (crbug/696113) On 2017/03/08 at 23:37:07, sgurun wrote: > no need to add todo. this is a major feature request tracked in crbug. Done
ntfschr@chromium.org changed reviewers: + torne@chromium.org
torne@: Can you please look at base/android/*?
On 2017/03/08 23:54:09, Nate Fischer wrote: > torne@: Can you please look at base/android/*? aw lgtm
https://codereview.chromium.org/2739003002/diff/20001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwSafeBrowsingConfigHelper.java (right): https://codereview.chromium.org/2739003002/diff/20001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwSafeBrowsingConfigHelper.java:46: || (BuildInfo.targetsAtLeastO(appContext) && appHasOptedIn(appContext)); I'm a bit confused by the logic in this change. I assumed what were going to implement was: 1) Enable it for apps that have opted in 2) Also enable it for apps that target at least O unless they have opted out. i.e. just flip the default value based on targetSDK. Was that not the intention and I misunderstood? https://codereview.chromium.org/2739003002/diff/20001/base/android/java/src/o... File base/android/java/src/org/chromium/base/BuildInfo.java (right): https://codereview.chromium.org/2739003002/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/BuildInfo.java:170: return appContext.getApplicationInfo().targetSdkVersion > 25; This isn't a valid way to check this - API level is not defined yet. I think the best way here is to do "isAtLeastO() && appContext.getApplicationInfo().targetSdkVersion == Build.VERSION_CODES.CUR_DEVELOPMENT". CUR_DEVELOPMENT is 10000, which is what this integer field gets set to when your APK targets a string codename instead of an integer, but you also need to check the OS you're running on, because CUR_DEVELOPMENT is *always* the same number (so for example, N developer preview builds would set this field to CUR_DEVELOPMENT for APKs that targeted "N").
https://codereview.chromium.org/2739003002/diff/20001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwSafeBrowsingConfigHelper.java (right): https://codereview.chromium.org/2739003002/diff/20001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwSafeBrowsingConfigHelper.java:46: || (BuildInfo.targetsAtLeastO(appContext) && appHasOptedIn(appContext)); On 2017/03/09 12:12:32, Torne wrote: > I'm a bit confused by the logic in this change. I assumed what were going to > implement was: > 1) Enable it for apps that have opted in > 2) Also enable it for apps that target at least O unless they have opted out. > > i.e. just flip the default value based on targetSDK. > > Was that not the intention and I misunderstood? no that was not the intention, we want it to be opt-in for the apps at the this time. However there was some confusion about whether to use targetsdk level/build level and how to enable it for older versions of the platform. I discussed it with various stakeholders this morning: Please change this such that safebrowsing is enabled if an app opts in to it, regardless of sdk level. Sorry for the confusion. However your code for targetsdk level is useful, please do not revert. instead fix the https cleartext check target sdk check (will show it to you) do that it uses target sdk level correctly. https://codereview.chromium.org/2739003002/diff/20001/base/android/java/src/o... File base/android/java/src/org/chromium/base/BuildInfo.java (right): https://codereview.chromium.org/2739003002/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/BuildInfo.java:163: && ("O".equals(Build.VERSION.CODENAME) || Build.VERSION.CODENAME.startsWith("OMR")); I know this code is copy paste from Android but I have no idea why it is not Build.VERSION.CODENAME.startsWith("O"). But whatever. https://codereview.chromium.org/2739003002/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/BuildInfo.java:170: return appContext.getApplicationInfo().targetSdkVersion > 25; On 2017/03/09 12:12:32, Torne wrote: > This isn't a valid way to check this - API level is not defined yet. > > I think the best way here is to do "isAtLeastO() && > appContext.getApplicationInfo().targetSdkVersion == > Build.VERSION_CODES.CUR_DEVELOPMENT". CUR_DEVELOPMENT is 10000, which is what > this integer field gets set to when your APK targets a string codename instead > of an integer, but you also need to check the OS you're running on, because > CUR_DEVELOPMENT is *always* the same number (so for example, N developer preview > builds would set this field to CUR_DEVELOPMENT for APKs that targeted "N"). yeah correct. But I think it should be appContext.getApplicationInfo().targetSdkVersion > 25 || isAtLeastO() && appContext.getApplicationInfo().targetSdkVersion == Build.VERSION_CODES.CUR_DEVELOPMENT unless we want to change it in the future after API level for O is set. (By the way, I have the same problem in my https cleartest target sdk check :)
https://codereview.chromium.org/2739003002/diff/20001/base/android/java/src/o... File base/android/java/src/org/chromium/base/BuildInfo.java (right): https://codereview.chromium.org/2739003002/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/BuildInfo.java:163: && ("O".equals(Build.VERSION.CODENAME) || Build.VERSION.CODENAME.startsWith("OMR")); On 2017/03/09 19:25:41, sgurun wrote: > I know this code is copy paste from Android but I have no idea why it is not > Build.VERSION.CODENAME.startsWith("O"). But whatever. Or why it checks that the string isn't REL at the same time, which can't ever be true if it's also O. ;) We should leave it alone, since it's copied ;) https://codereview.chromium.org/2739003002/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/BuildInfo.java:170: return appContext.getApplicationInfo().targetSdkVersion > 25; On 2017/03/09 19:25:41, sgurun wrote: > On 2017/03/09 12:12:32, Torne wrote: > > This isn't a valid way to check this - API level is not defined yet. > > > > I think the best way here is to do "isAtLeastO() && > > appContext.getApplicationInfo().targetSdkVersion == > > Build.VERSION_CODES.CUR_DEVELOPMENT". CUR_DEVELOPMENT is 10000, which is what > > this integer field gets set to when your APK targets a string codename instead > > of an integer, but you also need to check the OS you're running on, because > > CUR_DEVELOPMENT is *always* the same number (so for example, N developer > preview > > builds would set this field to CUR_DEVELOPMENT for APKs that targeted "N"). > > yeah correct. But I think it should be > appContext.getApplicationInfo().targetSdkVersion > 25 || isAtLeastO() && > appContext.getApplicationInfo().targetSdkVersion == > Build.VERSION_CODES.CUR_DEVELOPMENT > unless we want to change it in the future after API level for O is set. No, it shouldn't, because we must not assume what the API level is going to be until it's defined. We already have to update isAtLeastO once the API level is set; we just also have to update this. This is how it's intended to work. > (By the way, I have the same problem in my https cleartest target sdk check :)
Description was changed from ========== Change WebView's Safe Browsing opt-in requirements Safe Browsing will now only be enabled if all of the following conditions are met: * The app targets O or above * The app has explicitly opted-in to the feature with the manifest flag `android.webkit.WebView.EnableSafeBrowsing` set to "true" This change adds a helper method to BuildInfo for checking if the target SDK is at least O. BUG=699193 ========== to ========== Change WebView's Safe Browsing opt-in requirements Safe Browsing will now be enabled for any app which opts in by setting the flag `android.webkit.WebView.EnableSafeBrowsing` to "true", regardless of Android version or Target SDK. BUG=699193 ==========
sgurun, LGTY? Logic has been updated to reflect what we discussed. I've described the logic inside the updated CL description. https://codereview.chromium.org/2739003002/diff/20001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwSafeBrowsingConfigHelper.java (right): https://codereview.chromium.org/2739003002/diff/20001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwSafeBrowsingConfigHelper.java:46: || (BuildInfo.targetsAtLeastO(appContext) && appHasOptedIn(appContext)); On 2017/03/09 at 19:25:41, sgurun wrote: > On 2017/03/09 12:12:32, Torne wrote: > > I'm a bit confused by the logic in this change. I assumed what were going to > > implement was: > > 1) Enable it for apps that have opted in > > 2) Also enable it for apps that target at least O unless they have opted out. > > > > i.e. just flip the default value based on targetSDK. > > > > Was that not the intention and I misunderstood? > > no that was not the intention, we want it to be opt-in for the apps at the this time. However there was some confusion about whether to use targetsdk level/build level and how to enable it for older versions of the platform. > > I discussed it with various stakeholders this morning: Please change this such that safebrowsing is enabled if an app opts in to it, regardless of sdk level. > > Sorry for the confusion. > > However your code for targetsdk level is useful, please do not revert. instead fix the https cleartext check target sdk check (will show it to you) do that it uses target sdk level correctly. Ok, that sounds good to me. The logic is updated. https://codereview.chromium.org/2739003002/diff/20001/base/android/java/src/o... File base/android/java/src/org/chromium/base/BuildInfo.java (right): https://codereview.chromium.org/2739003002/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/BuildInfo.java:170: return appContext.getApplicationInfo().targetSdkVersion > 25; On 2017/03/09 at 19:29:25, Torne wrote: > On 2017/03/09 19:25:41, sgurun wrote: > > On 2017/03/09 12:12:32, Torne wrote: > > > This isn't a valid way to check this - API level is not defined yet. > > > > > > I think the best way here is to do "isAtLeastO() && > > > appContext.getApplicationInfo().targetSdkVersion == > > > Build.VERSION_CODES.CUR_DEVELOPMENT". CUR_DEVELOPMENT is 10000, which is what > > > this integer field gets set to when your APK targets a string codename instead > > > of an integer, but you also need to check the OS you're running on, because > > > CUR_DEVELOPMENT is *always* the same number (so for example, N developer > > preview > > > builds would set this field to CUR_DEVELOPMENT for APKs that targeted "N"). > > > > yeah correct. But I think it should be > > appContext.getApplicationInfo().targetSdkVersion > 25 || isAtLeastO() && > > appContext.getApplicationInfo().targetSdkVersion == > > Build.VERSION_CODES.CUR_DEVELOPMENT > > unless we want to change it in the future after API level for O is set. > > No, it shouldn't, because we must not assume what the API level is going to be until it's defined. We already have to update isAtLeastO once the API level is set; we just also have to update this. This is how it's intended to work. > > > (By the way, I have the same problem in my https cleartest target sdk check :) I implemented this in a separate CL: https://codereview.chromium.org/2745663002. I tagged Torne as the reviewer, although it does update Selim's cleartext check as well.
On 2017/03/09 23:28:18, Nate Fischer wrote: > sgurun, LGTY? Logic has been updated to reflect what we discussed. I've > described the logic inside the updated CL description. > > https://codereview.chromium.org/2739003002/diff/20001/android_webview/java/sr... > File > android_webview/java/src/org/chromium/android_webview/AwSafeBrowsingConfigHelper.java > (right): > > https://codereview.chromium.org/2739003002/diff/20001/android_webview/java/sr... > android_webview/java/src/org/chromium/android_webview/AwSafeBrowsingConfigHelper.java:46: > || (BuildInfo.targetsAtLeastO(appContext) && appHasOptedIn(appContext)); > On 2017/03/09 at 19:25:41, sgurun wrote: > > On 2017/03/09 12:12:32, Torne wrote: > > > I'm a bit confused by the logic in this change. I assumed what were going to > > > implement was: > > > 1) Enable it for apps that have opted in > > > 2) Also enable it for apps that target at least O unless they have opted > out. > > > > > > i.e. just flip the default value based on targetSDK. > > > > > > Was that not the intention and I misunderstood? > > > > no that was not the intention, we want it to be opt-in for the apps at the > this time. However there was some confusion about whether to use targetsdk > level/build level and how to enable it for older versions of the platform. > > > > I discussed it with various stakeholders this morning: Please change this such > that safebrowsing is enabled if an app opts in to it, regardless of sdk level. > > > > Sorry for the confusion. > > > > However your code for targetsdk level is useful, please do not revert. instead > fix the https cleartext check target sdk check (will show it to you) do that it > uses target sdk level correctly. > > Ok, that sounds good to me. The logic is updated. > > https://codereview.chromium.org/2739003002/diff/20001/base/android/java/src/o... > File base/android/java/src/org/chromium/base/BuildInfo.java (right): > > https://codereview.chromium.org/2739003002/diff/20001/base/android/java/src/o... > base/android/java/src/org/chromium/base/BuildInfo.java:170: return > appContext.getApplicationInfo().targetSdkVersion > 25; > On 2017/03/09 at 19:29:25, Torne wrote: > > On 2017/03/09 19:25:41, sgurun wrote: > > > On 2017/03/09 12:12:32, Torne wrote: > > > > This isn't a valid way to check this - API level is not defined yet. > > > > > > > > I think the best way here is to do "isAtLeastO() && > > > > appContext.getApplicationInfo().targetSdkVersion == > > > > Build.VERSION_CODES.CUR_DEVELOPMENT". CUR_DEVELOPMENT is 10000, which is > what > > > > this integer field gets set to when your APK targets a string codename > instead > > > > of an integer, but you also need to check the OS you're running on, > because > > > > CUR_DEVELOPMENT is *always* the same number (so for example, N developer > > > preview > > > > builds would set this field to CUR_DEVELOPMENT for APKs that targeted > "N"). > > > > > > yeah correct. But I think it should be > > > appContext.getApplicationInfo().targetSdkVersion > 25 || isAtLeastO() && > > > appContext.getApplicationInfo().targetSdkVersion == > > > Build.VERSION_CODES.CUR_DEVELOPMENT > > > unless we want to change it in the future after API level for O is set. > > > > No, it shouldn't, because we must not assume what the API level is going to be > until it's defined. We already have to update isAtLeastO once the API level is > set; we just also have to update this. This is how it's intended to work. > > > > > (By the way, I have the same problem in my https cleartest target sdk check > :) > > I implemented this in a separate CL: https://codereview.chromium.org/2745663002. > I tagged Torne as the reviewer, although it does update Selim's cleartext check > as well. lgtm
The CQ bit was checked by ntfschr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489110175444190,
"parent_rev": "8d97710b15a51c4e2cec80be55d97bc722f84fb1", "commit_rev":
"e2521352441e0a396674c0d190e2c2f836ece010"}
Message was sent while issue was closed.
Description was changed from ========== Change WebView's Safe Browsing opt-in requirements Safe Browsing will now be enabled for any app which opts in by setting the flag `android.webkit.WebView.EnableSafeBrowsing` to "true", regardless of Android version or Target SDK. BUG=699193 ========== to ========== Change WebView's Safe Browsing opt-in requirements Safe Browsing will now be enabled for any app which opts in by setting the flag `android.webkit.WebView.EnableSafeBrowsing` to "true", regardless of Android version or Target SDK. BUG=699193 Review-Url: https://codereview.chromium.org/2739003002 Cr-Commit-Position: refs/heads/master@{#455958} Committed: https://chromium.googlesource.com/chromium/src/+/e2521352441e0a396674c0d190e2... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/e2521352441e0a396674c0d190e2... |
