Add an activity that allows whitelisted applications to clear all data in Chrome.

Add an activity that allows whitelisted applications to clear all data in Chrome, or ask whether there is data to clear.

The activity can be invoked as part of the child account setup from outside Chrome via an explicit Intent to ResetDataActivity (using Activity.startActivityForResult).

BUG=482889
Committed: https://crrev.com/2ad62c8b6260e07b697cd8aa8802874cecbc417d
Cr-Commit-Position: refs/heads/master@{#336129}

[Bernhard Bauer, 2015-06-08 14:54:48]

bauerb@chromium.org changed reviewers:
+ aurimas@chromium.org

[Bernhard Bauer, 2015-06-08 14:54:49]

Please review. Thanks!

[aurimas (slooooooooow), 2015-06-08 17:29:47]

A few questions to get this started:
1. Are there mocks for this? I prefer no landing UI code if UX hasn't signed off
on it yet.
2. When will this be triggered? Your code is not called from anywhere.
3. Why does it have to be an activity? Can we just have a plain old Dialog if
this will just be called by ChromeTabbedActivity?
4. Can you post a screenshot of what does the dialog look like with this CL?

[Bernhard Bauer, 2015-06-08 18:02:30]

On 2015/06/08 17:29:47, aurimas wrote:
> A few questions to get this started:
> 1. Are there mocks for this? I prefer no landing UI code if UX hasn't signed
off
> on it yet.

There aren't any mocks yet, but we are currently trying to get approved strings
(hence the placeholder in the strings file right now). I just wanted to get the
basic functionality in so other teams can use and test this (see the bug for
some context).

> 2. When will this be triggered? Your code is not called from anywhere.
> 3. Why does it have to be an activity? Can we just have a plain old Dialog if
> this will just be called by ChromeTabbedActivity?

It's meant to be invoked via an Intent from somewhere else (again, the bug has a
bit more context; apologies if I can't go into detail here, as this related to
unreleased products). I updated the CL description to explain that.

> 4. Can you post a screenshot of what does the dialog look like with this CL?

Argh, already went home... I'll take a screenshot tomorrow.

[Bernhard Bauer, 2015-06-09 14:26:17]

On 2015/06/08 18:02:30, Bernhard Bauer wrote:
> On 2015/06/08 17:29:47, aurimas wrote:
> > A few questions to get this started:
> > 1. Are there mocks for this? I prefer no landing UI code if UX hasn't signed
> off
> > on it yet.
> 
> There aren't any mocks yet, but we are currently trying to get approved
strings
> (hence the placeholder in the strings file right now). I just wanted to get
the
> basic functionality in so other teams can use and test this (see the bug for
> some context).
> 
> > 2. When will this be triggered? Your code is not called from anywhere.
> > 3. Why does it have to be an activity? Can we just have a plain old Dialog
if
> > this will just be called by ChromeTabbedActivity?
> 
> It's meant to be invoked via an Intent from somewhere else (again, the bug has
a
> bit more context; apologies if I can't go into detail here, as this related to
> unreleased products). I updated the CL description to explain that.
> 
> > 4. Can you post a screenshot of what does the dialog look like with this CL?
> 
> Argh, already went home... I'll take a screenshot tomorrow.

OK, added a screenshot. I'll ping UI people about the strings.

[aurimas (slooooooooow), 2015-06-09 17:45:03]

It looks fairly reasonable, but let's wait for actual strings before landing
this.

https://codereview.chromium.org/1143313006/diff/1/chrome/android/java/res/val...
File chrome/android/java/res/values-v19/styles.xml (right):

https://codereview.chromium.org/1143313006/diff/1/chrome/android/java/res/val...
chrome/android/java/res/values-v19/styles.xml:10: <item
name="android:windowTranslucentStatus">true</item>
I am pretty sure you can move this to values-v17 and Android will just drop that
attribute on devices pre v21.

https://codereview.chromium.org/1143313006/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ResetDataActivity.java
(right):

https://codereview.chromium.org/1143313006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ResetDataActivity.java:62:
if (Build.VERSION.SDK_INT < Build.VERSION_CODES.KITKAT) {
What will happen on JB devices?

https://codereview.chromium.org/1143313006/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ResetDataDialogFragment.java
(right):

https://codereview.chromium.org/1143313006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ResetDataDialogFragment.java:41:
return new AlertDialog.Builder(mContext)
Use R.style.AlertDialogTheme to match the rest of Chrome dialogs.

https://codereview.chromium.org/1143313006/diff/1/chrome/android/java_staging...
File chrome/android/java_staging/AndroidManifest.xml (right):

https://codereview.chromium.org/1143313006/diff/1/chrome/android/java_staging...
chrome/android/java_staging/AndroidManifest.xml:352: <activity
android:name="org.chromium.chrome.browser.child_accounts.ResetDataActivity"
We are exporting this and we will always have to support this so we need to get
this right.

I would vote for removing the underscore from child_accounts and making it
"childaccounts"
Reasoning why:
- I could not find a single _ (underscore) in any package names in Android.
- Google style guide says not to use underscores (I know Android style guide
does not specify it, but it does not use _ either) [1]

Reasons why it is iffy:
- I know we have bad examples (like dom_distiller), but I dont think it is a
good precedent.
- It will make c++ and java directories mismatch (which we already do in several
places)

[1]
https://google-styleguide.googlecode.com/svn/trunk/javaguide.html#s5.2.1-pack...

[Bernhard Bauer, 2015-06-15 17:07:29]

Thanks for the review! It turned out that any dialog was too much UI for The
Powers That Be, so we will probably change this to be a completely invisible
Activity, and whitelist the starting application via its signature key. I do see
your point about renaming the child_accounts package though. Would you like me
to do that in a separate CL, or right in this one?

https://codereview.chromium.org/1143313006/diff/1/chrome/android/java/res/val...
File chrome/android/java/res/values-v19/styles.xml (right):

https://codereview.chromium.org/1143313006/diff/1/chrome/android/java/res/val...
chrome/android/java/res/values-v19/styles.xml:10: <item
name="android:windowTranslucentStatus">true</item>
On 2015/06/09 17:45:03, aurimas wrote:
> I am pretty sure you can move this to values-v17 and Android will just drop
that
> attribute on devices pre v21.

I get a Lint error when I put this into values-v17:

chrome/android/java/res/values-v17/styles.xml:269
`android:windowTranslucentStatus` requires API level 19 (current min is 16):
NewApi [warning]

https://codereview.chromium.org/1143313006/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ResetDataActivity.java
(right):

https://codereview.chromium.org/1143313006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/child_accounts/ResetDataActivity.java:62:
if (Build.VERSION.SDK_INT < Build.VERSION_CODES.KITKAT) {
On 2015/06/09 17:45:03, aurimas wrote:
> What will happen on JB devices?

We will return an error. In practice, this shouldn't happen, as the use case for
this is L+.

[Bernhard Bauer, 2015-06-16 14:26:46]

Updated. PTAL?

[aurimas (slooooooooow), 2015-06-16 20:23:44]

aurimas@chromium.org changed reviewers:
+ palmer@chromium.org

[aurimas (slooooooooow), 2015-06-16 20:23:46]

Thanks a lot for the package rename. That part looks good.

ResetDataActivity.java seems to expose a fairly scary API so
+palmer@chromium.org to take a look at this file.

https://codereview.chromium.org/1143313006/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/childaccounts/ResetDataActivity.java
(right):

https://codereview.chromium.org/1143313006/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/childaccounts/ResetDataActivity.java:32:
private static final int RESULT_ERROR_UNAUTHORIZED = Activity.RESULT_FIRST_USER;
Are these error codes documented somewhere? Can we put a comment about it? a go/
link maybe?

https://codereview.chromium.org/1143313006/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/childaccounts/ResetDataActivity.java:106:
for (Signature sig : signatures) {
Can we use ExternalAuthUtils#isGoogleSigned() instead? We use that for other
places that we need to verify that the calling app is Google signed.

[Bernhard Bauer, 2015-06-17 14:59:06]

https://codereview.chromium.org/1143313006/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/childaccounts/ResetDataActivity.java
(right):

https://codereview.chromium.org/1143313006/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/childaccounts/ResetDataActivity.java:32:
private static final int RESULT_ERROR_UNAUTHORIZED = Activity.RESULT_FIRST_USER;
On 2015/06/16 20:23:45, aurimas wrote:
> Are these error codes documented somewhere? Can we put a comment about it? a
go/
> link maybe?

Added comments for the values. As this is going to live in an open source
repository, I was just going to link to this file...

https://codereview.chromium.org/1143313006/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/childaccounts/ResetDataActivity.java:106:
for (Signature sig : signatures) {
On 2015/06/16 20:23:45, aurimas wrote:
> Can we use ExternalAuthUtils#isGoogleSigned() instead? We use that for other
> places that we need to verify that the calling app is Google signed.

That seems to work, thanks!

Hm, given that we only require this to be called by a specific application,
should we add an additional check for the package name? But I guess the only
real protection is via the signature anyway...

[palmer, 2015-06-17 21:55:17]

Overall, this does not look bad to me, but I am not sure I fully understand the
requirements yet. Here are some notes in the meantime.

https://codereview.chromium.org/1143313006/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/childaccounts/ResetDataActivity.java
(right):

https://codereview.chromium.org/1143313006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/childaccounts/ResetDataActivity.java:91:
return ExternalAuthUtils.getInstance().isGoogleSigned(getPackageManager(),
Ahh, I see. This is sort of an imperative way to achieve the same thing as I was
describing with the signatureOrSystem Permission, but this solution doesn't
require the caller to have affirmatively asked for the new Permission you would
declare. That may make it easier, since you don't have to coordinate across
teams/PAs.

But, now you're (a) allowing *any* Google-signed package to invoke this; and (b)
relying on ExternalAuthUtils to be correct (as opposed to relying on the Android
platform to be correct). As for (a), maybe that's fine. As for (b), maybe that's
fine, too.

(I have not read ExternalAuthUtils, so I don't know how correct it is. Oh, I see
that I can't fully read it. My instinct is to consider that a bad sign...)

https://codereview.chromium.org/1143313006/diff/40001/chrome/android/java_sta...
File chrome/android/java_staging/AndroidManifest.xml (right):

https://codereview.chromium.org/1143313006/diff/40001/chrome/android/java_sta...
chrome/android/java_staging/AndroidManifest.xml:354:
android:theme="@android:style/Theme.Translucent.NoTitleBar"
As declared, any other package on the device could invoke this Activity. Is that
safe/a good idea/intended?

There is a nice, declarative way to ensure that only the system or Chrome can
invoke the Activity: declare a signatureOrSystem Permission, and then require
that callers of this Activity must have it. Thus, only Chrome (or possibly other
Google packages) or the Android platform could launch the Activity.

http://developer.android.com/guide/topics/manifest/activity-element.html#prmsn

http://developer.android.com/guide/topics/manifest/permission-element.html

[Bernhard Bauer, 2015-06-18 09:08:53]

https://codereview.chromium.org/1143313006/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/childaccounts/ResetDataActivity.java
(right):

https://codereview.chromium.org/1143313006/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/childaccounts/ResetDataActivity.java:91:
return ExternalAuthUtils.getInstance().isGoogleSigned(getPackageManager(),
On 2015/06/17 21:55:17, palmer wrote:
> Ahh, I see. This is sort of an imperative way to achieve the same thing as I
was
> describing with the signatureOrSystem Permission, but this solution doesn't
> require the caller to have affirmatively asked for the new Permission you
would
> declare. That may make it easier, since you don't have to coordinate across
> teams/PAs.
> 
> But, now you're (a) allowing *any* Google-signed package to invoke this; and
(b)
> relying on ExternalAuthUtils to be correct (as opposed to relying on the
Android
> platform to be correct). As for (a), maybe that's fine. As for (b), maybe
that's
> fine, too.
> 
> (I have not read ExternalAuthUtils, so I don't know how correct it is. Oh, I
see
> that I can't fully read it. My instinct is to consider that a bad sign...)

I'm not completely sure about (a) either... Like I suggested earlier, I could
whitelist the calling package name as an additional requirement. My gut feeling
is that for the case of <random malware app> calling this activity to mess with
Chrome, isGoogleSigned() is sufficient, and we get at least some protection
against accidental usage by requiring the dry run flag to be explicitly set to
false.

For (b), the only reason why the implementation of isGoogleSigned() isn't
visible is because it's a first-party API. You can find the implementation on
Code Search (or ping me and I'll show it to you), but it's basically just a
check against a biig list of all the first-party keys in all the Google products
on Android, with some neat additional logic to allow test keys on test images or
userdebug builds.

https://codereview.chromium.org/1143313006/diff/40001/chrome/android/java_sta...
File chrome/android/java_staging/AndroidManifest.xml (right):

https://codereview.chromium.org/1143313006/diff/40001/chrome/android/java_sta...
chrome/android/java_staging/AndroidManifest.xml:354:
android:theme="@android:style/Theme.Translucent.NoTitleBar"
On 2015/06/17 21:55:17, palmer wrote:
> As declared, any other package on the device could invoke this Activity. Is
that
> safe/a good idea/intended?
> 
> There is a nice, declarative way to ensure that only the system or Chrome can
> invoke the Activity: declare a signatureOrSystem Permission, and then require
> that callers of this Activity must have it. Thus, only Chrome (or possibly
other
> Google packages) or the Android platform could launch the Activity.
> 
> http://developer.android.com/guide/topics/manifest/activity-element.html#prmsn
> 
> http://developer.android.com/guide/topics/manifest/permission-element.html

The problem with declaring a permission is that the permission has to be known
to the system before it can be granted to the app, which means that now Chrome
has to be installed before the app that calls this, which might not be the case.

Also, I think signatureOrSystem wouldn't work here, because the calling app is
signed with a different (but still first-party) key than Chrome is. At that
point, we'd have to whitelist a specific key... which is almost the same as I'm
doing here anyway :)

[palmer, 2015-06-18 19:07:25]

Yes, I think this LGTM. It's the best/the only/a reasonable way to do what you
need to do here.

[Bernhard Bauer, 2015-06-23 09:01:51]

Aurimas, do you want to take another look?

[Bernhard Bauer, 2015-06-24 16:13:31]

On 2015/06/23 09:01:51, Bernhard Bauer wrote:
> Aurimas, do you want to take another look?

Friendly ping?

[aurimas (slooooooooow), 2015-06-24 16:43:11]

lgtm

[aurimas (slooooooooow), 2015-06-24 16:43:52]

Don't forget to update the commit message.

[Bernhard Bauer, 2015-06-24 16:46:21]

On 2015/06/24 16:43:52, aurimas wrote:
> Don't forget to update the commit message.

Done (Rietveld keeps the initial title for emails).

[Bernhard Bauer, 2015-06-24 16:46:26]

The CQ bit was checked by bauerb@chromium.org

[commit-bot: I haz the power, 2015-06-24 16:47:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1143313006/40001

[commit-bot: I haz the power, 2015-06-24 16:49:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-24 16:49:36]

Exceeded global retry quota

[Bernhard Bauer, 2015-06-25 09:20:19]

The CQ bit was checked by bauerb@chromium.org

[Bernhard Bauer, 2015-06-25 09:20:20]

The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org,
aurimas@chromium.org
Link to the patchset: https://codereview.chromium.org/1143313006/#ps60001
(title: "sync")

[commit-bot: I haz the power, 2015-06-25 09:20:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1143313006/60001

[commit-bot: I haz the power, 2015-06-25 11:10:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-25 11:10:57]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Bernhard Bauer, 2015-06-25 11:30:33]

The CQ bit was checked by bauerb@chromium.org

[commit-bot: I haz the power, 2015-06-25 11:31:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1143313006/60001

[commit-bot: I haz the power, 2015-06-25 11:58:46]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-06-25 11:59:51]

Patchset 4 (id:??) landed as
https://crrev.com/2ad62c8b6260e07b697cd8aa8802874cecbc417d
Cr-Commit-Position: refs/heads/master@{#336129}