[Activity log] Expose URL incognito state to private API

[Activity log] Expose URL incognito state to private API

Expose the incognito state of URLs to the activity log API.  Any
recorded URLs which were from an incognito window are prefixed with
"<incognito>", as is done in the database storage.

Re-enable the incognito subtests in the end-to-end activity log test.

BUG=272920
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221415

[mvrable, 2013-08-27 23:14:11]

Can you review this change to fix up handling of incognito URLs and the private
API?  I've re-enabled the incognito browser tests too.

Could you also provide feedback on what the desired sanitization of the data is?
 I'm currently using StripPrivacySensitiveFields before passing data along--that
does a few other things besides just removing incognito URLs.  Should I be doing
fewer modifications?  More?  What precisely are the privacy concerns we have
with data passed along via the monitoring stream?  We want to balance privacy
and usefulness, and since the data is not stored persistently we might be able
to keep more than the counting policy does.

--Michael Vrable

[felt, 2013-08-28 17:51:55]

On 2013/08/27 23:14:11, mvrable wrote:
> Can you review this change to fix up handling of incognito URLs and the
private
> API?  I've re-enabled the incognito browser tests too.
> 
> Could you also provide feedback on what the desired sanitization of the data
is?
>  I'm currently using StripPrivacySensitiveFields before passing data
along--that
> does a few other things besides just removing incognito URLs.  Should I be
doing
> fewer modifications?  More?  What precisely are the privacy concerns we have
> with data passed along via the monitoring stream?  We want to balance privacy
> and usefulness, and since the data is not stored persistently we might be able
> to keep more than the counting policy does.
> 
> --Michael Vrable

Basically, we want the user to see exactly what is being stored. Based on that,
I agree that StripPrivacySensitiveFields is the right method to use.

[mvrable, 2013-08-28 18:04:52]

On 2013/08/28 17:51:55, felt wrote:
> Basically, we want the user to see exactly what is being stored. Based on
that,
> I agree that StripPrivacySensitiveFields is the right method to use.

I'm a little lost, since that wasn't my understanding.  For example, if we're
using the counting policy, do we want to be streaming the coalesced records with
counts?  If a user wants that view, why not just read from the database instead?
 Do we want to strip out arguments in most cases as well, the same way the
counting policy does?  I had thought we wanted to provide a slightly richer view
of the data; since the streamed data is stored persistently anywhere there are
fewer privacy risks (basically, make the real-time stream good enough for some
developer debugging without having to run with
--enable-extension-activity-log-testing).

If we do want to stream exactly what is being stored, we'll want the policies to
be responsible for streaming the data instead after they have had a chance to
filter/transform the data (this also means what is available in the stream will
depend on which policy is selected).  I'd have to look at how to structure the
code for that.

I can implement whatever, but I want to make sure my understanding matches yours
before I implement.

--Michael Vrable

[felt, 2013-08-28 18:12:17]

On 2013/08/28 18:04:52, mvrable wrote:
> On 2013/08/28 17:51:55, felt wrote:
> > Basically, we want the user to see exactly what is being stored. Based on
> that,
> > I agree that StripPrivacySensitiveFields is the right method to use.
> 
> I'm a little lost, since that wasn't my understanding.  For example, if we're
> using the counting policy, do we want to be streaming the coalesced records
with
> counts?  If a user wants that view, why not just read from the database
instead?
>  Do we want to strip out arguments in most cases as well, the same way the
> counting policy does?  I had thought we wanted to provide a slightly richer
view
> of the data; since the streamed data is stored persistently anywhere there are
> fewer privacy risks (basically, make the real-time stream good enough for some
> developer debugging without having to run with
> --enable-extension-activity-log-testing).
> 
> If we do want to stream exactly what is being stored, we'll want the policies
to
> be responsible for streaming the data instead after they have had a chance to
> filter/transform the data (this also means what is available in the stream
will
> depend on which policy is selected).  I'd have to look at how to structure the
> code for that.
> 
> I can implement whatever, but I want to make sure my understanding matches
yours
> before I implement.
> 
> --Michael Vrable

I'm not worried about the privacy risks of streaming. What I'm worried about is
someone clearing the whole DB (or uninstalling the app) after they see an
incognito URL in the developer view.

You're right about the args, which I'd forgotten about -- since I was focusing
on the incognito URLs. That would require some structuring.

I suppose an alternative to sanitization is to put a disclaimer in the Watchdog
app's developer view telling people that this data isn't all being recorded.

[mvrable, 2013-08-28 18:19:32]

On 2013/08/28 18:12:17, felt wrote:
> I'm not worried about the privacy risks of streaming. What I'm worried about
is
> someone clearing the whole DB (or uninstalling the app) after they see an
> incognito URL in the developer view.
> 
> You're right about the args, which I'd forgotten about -- since I was focusing
> on the incognito URLs. That would require some structuring.
> 
> I suppose an alternative to sanitization is to put a disclaimer in the
Watchdog
> app's developer view telling people that this data isn't all being recorded.

Do you have a preferred approach (more sanitization vs. adding a disclaimer in
the developer view)?  Is it worth scheduling a quick chat about this?

--Michael Vrable

[felt, 2013-08-28 21:30:39]

On 2013/08/28 18:19:32, mvrable wrote:
> On 2013/08/28 18:12:17, felt wrote:
> > I'm not worried about the privacy risks of streaming. What I'm worried about
> is
> > someone clearing the whole DB (or uninstalling the app) after they see an
> > incognito URL in the developer view.
> > 
> > You're right about the args, which I'd forgotten about -- since I was
focusing
> > on the incognito URLs. That would require some structuring.
> > 
> > I suppose an alternative to sanitization is to put a disclaimer in the
> Watchdog
> > app's developer view telling people that this data isn't all being recorded.
> 
> Do you have a preferred approach (more sanitization vs. adding a disclaimer in
> the developer view)?  Is it worth scheduling a quick chat about this?
> 
> --Michael Vrable

Hey Michael,

I thought about this some more, and I think you're right -- stripping all this
data will make the developer tool less useful. The onus will have to be on the
UI to make the privacy situation clear. I'll brainstorm to figure something out.
Sorry to ask you to write the CL without thinking through the issue with
arguments!

Adrienne

[felt, 2013-08-28 22:42:49]

new version lgtm

[commit-bot: I haz the power, 2013-08-28 22:58:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mvrable@chromium.org/23618010/4001

[commit-bot: I haz the power, 2013-08-28 23:14:25]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[mvrable, 2013-08-28 23:33:59]

Matt,

Adrienne has mostly reviewed this CL but isn't an OWNER for 
chrome/test/data/extensions/api_test/activity_log_private/test/test.js.

Could you check the changes on that file (some updates to the activity log
tests) and approve if everything looks good to you?

--Michael Vrable

[Matt Perry, 2013-08-29 00:38:11]

LGTM

[commit-bot: I haz the power, 2013-09-05 03:23:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mvrable@chromium.org/23618010/4001

[commit-bot: I haz the power, 2013-09-05 03:23:42]

Failed to apply patch for
chrome/browser/extensions/activity_log/activity_log.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/extensions/activity_log/activity_log.cc
  Hunk #1 FAILED at 337.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/extensions/activity_log/activity_log.cc.rej

Patch:       chrome/browser/extensions/activity_log/activity_log.cc
Index: chrome/browser/extensions/activity_log/activity_log.cc
diff --git a/chrome/browser/extensions/activity_log/activity_log.cc
b/chrome/browser/extensions/activity_log/activity_log.cc
index
1c2afc4ffef5c6e5790d3f2428c13053739545d5..8eac84f3b8308bff2ec77e2efc6b1b755180d37a
100644
--- a/chrome/browser/extensions/activity_log/activity_log.cc
+++ b/chrome/browser/extensions/activity_log/activity_log.cc
@@ -337,9 +337,6 @@ void ActivityLog::LogAction(scoped_refptr<Action> action) {
     LookupTabIds(action, profile_);
   }
 
-  // TODO(mvrable): Add any necessary processing of incognito URLs here, for
-  // crbug.com/253368
-
   if (policy_)
     policy_->ProcessAction(action);
   observers_->Notify(&Observer::OnExtensionActivity, action);

[commit-bot: I haz the power, 2013-09-05 03:30:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mvrable@chromium.org/23618010/27001

[commit-bot: I haz the power, 2013-09-05 03:30:52]

Failed to apply patch for
chrome/browser/extensions/activity_log/activity_log.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/extensions/activity_log/activity_log.cc
  Hunk #1 FAILED at 354.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/extensions/activity_log/activity_log.cc.rej

Patch:       chrome/browser/extensions/activity_log/activity_log.cc
Index: chrome/browser/extensions/activity_log/activity_log.cc
diff --git a/chrome/browser/extensions/activity_log/activity_log.cc
b/chrome/browser/extensions/activity_log/activity_log.cc
index
4e3350f77ce11c93392ae86d9fd351a96db56c9e..a75af87093eb1def87cc476dc3de3b2f66146117
100644
--- a/chrome/browser/extensions/activity_log/activity_log.cc
+++ b/chrome/browser/extensions/activity_log/activity_log.cc
@@ -354,9 +354,6 @@ void ActivityLog::LogAction(scoped_refptr<Action> action) {
     LookupTabIds(action, profile_);
   }
 
-  // TODO(mvrable): Add any necessary processing of incognito URLs here, for
-  // crbug.com/253368
-
   if (policy_)
     policy_->ProcessAction(action);
   observers_->Notify(&Observer::OnExtensionActivity, action);

[commit-bot: I haz the power, 2013-09-05 03:40:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mvrable@chromium.org/23618010/32001

[commit-bot: I haz the power, 2013-09-05 04:15:29]

Retried try job too often on ios_dbg_simulator for step(s) ui_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[commit-bot: I haz the power, 2013-09-05 14:07:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mvrable@chromium.org/23618010/32001

[commit-bot: I haz the power, 2013-09-05 14:19:15]

Change committed as 221415