Allow an external URL with FROM_API qualifier to be forwarded to ARC

Allow an external URL with FROM_API qualifier to be forwarded to ARC

for better AppAuth compatibility.

In a highly unusual situation, allowing FROM_API could trigger
infinite Chrome tab creation. This CL also adds code for detecting
and stopping the loop, just in case. End users will almost never
see the safety checks to kick in, though.

BUG=670465
TEST=unit_tests passed, also manually confirmed that when
 |always_ask_user| is true, "Always" preference set by
 the user is ignored.

Committed: https://crrev.com/0c7e06f5b85f0304bd62f61a9b2ae391e97f3183
Cr-Commit-Position: refs/heads/master@{#436017}

[Yusuke Sato, 2016-12-01 00:49:21]

The CQ bit was checked by yusukes@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 00:49:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 01:15:43]

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

[commit-bot: I haz the power, 2016-12-01 01:15:43]

Dry run: This issue passed the CQ dry run.

[Yusuke Sato, 2016-12-01 22:30:35]

The CQ bit was checked by yusukes@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 22:31:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yusuke Sato, 2016-12-01 22:31:13]

Description was changed from

==========
tmp

BUG=
==========

to

==========
Allow an external URL with FROM_API qualifier to be forwarded to ARC

for better AppAuth compatibility.

In a highly unusual situation, allowing FROM_API could trigger
infinite Chrome tab creation. This CL also adds code for detecting
and stopping the loop, just in case. End users will almost never
see the safety checks to kick in, though.

BUG=670465
TEST=unit_tests passed, also manually confirmed that when
 |always_ask_user| is true, "Always" preference set by
 the user is ignored.
==========

[Yusuke Sato, 2016-12-01 22:33:36]

yusukes@chromium.org changed reviewers:
+ djacobo@chromium.org, lhchavez@chromium.org

[Yusuke Sato, 2016-12-01 22:33:37]

PTAL. This is for b/33208965

[commit-bot: I haz the power, 2016-12-01 23:05:45]

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

[commit-bot: I haz the power, 2016-12-01 23:05:46]

Dry run: This issue passed the CQ dry run.

[djacobo, 2016-12-01 23:47:23]

LGTM

w/ some comments.

https://codereview.chromium.org/2540013004/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc
(right):

https://codereview.chromium.org/2540013004/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc:404:
// this heuristics.
nit: these*

https://codereview.chromium.org/2540013004/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc:419:
return url.scheme() != last_url.scheme();
qq: Does this means that two consecutive calls to the intent picker via
arc_external_protocol_dialog cannot be considered safe since their scheme is the
same? 

We certainly don't have a NavigationHandle here, so emulating something like
this could be the proper way to go?
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/intent_helpe...


I am ok with the fix, I could take a look at the TODO soon, actually I also
think this is a very unlikely situation but certainly needs proper handling.

https://codereview.chromium.org/2540013004/diff/20001/components/arc/intent_h...
File components/arc/intent_helper/page_transition_util.h (right):

https://codereview.chromium.org/2540013004/diff/20001/components/arc/intent_h...
components/arc/intent_helper/page_transition_util.h:18: // TODO(djacobo): Move
this to ui/base/page_transition_types.cc.
will do :)

[Yusuke Sato, 2016-12-02 00:24:17]

https://codereview.chromium.org/2540013004/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc
(right):

https://codereview.chromium.org/2540013004/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc:419:
return url.scheme() != last_url.scheme();
On 2016/12/01 23:47:23, djacobo wrote:
> qq: Does this means that two consecutive calls to the intent picker via
> arc_external_protocol_dialog cannot be considered safe since their scheme is
the
> same? 

Yes, but it's considered "possibly unsafe" only when the previous and current
navigations are FROM_API (which is very rare), and the previous navigation was
handled by ARC's preferred app without showing the disambig dialog.

> 
> We certainly don't have a NavigationHandle here, so emulating something like
> this could be the proper way to go?
>
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/intent_helpe...
> 
> 
> I am ok with the fix, I could take a look at the TODO soon, actually I also
> think this is a very unlikely situation but certainly needs proper handling. 

Re right fix:
When |url| is sent to ArcIntentHelper, the helper converts the URL to an Intent
object, right? I'm wondering if it is possible to add some EXTRAs to the intent
object so that ArcIntentHelper can ignore the intent when it is bounced back to
ArcIntentHelper. Just a random idea, and I'm not really sure if it is feasible
though.

https://codereview.chromium.org/2540013004/diff/20001/components/arc/intent_h...
File components/arc/intent_helper/page_transition_util.h (right):

https://codereview.chromium.org/2540013004/diff/20001/components/arc/intent_h...
components/arc/intent_helper/page_transition_util.h:18: // TODO(djacobo): Move
this to ui/base/page_transition_types.cc.
On 2016/12/01 23:47:23, djacobo wrote:
> will do :)

Thanks!

[Yusuke Sato, 2016-12-02 00:44:18]

The CQ bit was checked by yusukes@chromium.org to run a CQ dry run

[Yusuke Sato, 2016-12-02 00:44:38]

https://codereview.chromium.org/2540013004/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc
(right):

https://codereview.chromium.org/2540013004/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/intent_helper/arc_external_protocol_dialog.cc:404:
// this heuristics.
On 2016/12/01 23:47:23, djacobo wrote:
> nit: these*

Done.

[commit-bot: I haz the power, 2016-12-02 00:44:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-02 01:13:58]

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

[commit-bot: I haz the power, 2016-12-02 01:13:58]

Dry run: This issue passed the CQ dry run.

[Yusuke Sato, 2016-12-02 19:13:45]

Luis, cold you do an owner review?

[Luis Héctor Chávez, 2016-12-02 19:19:24]

lgtm

[Yusuke Sato, 2016-12-02 20:13:43]

The CQ bit was checked by yusukes@chromium.org

[Yusuke Sato, 2016-12-02 20:13:44]

The patchset sent to the CQ was uploaded after l-g-t-m from djacobo@chromium.org
Link to the patchset: https://codereview.chromium.org/2540013004/#ps40001
(title: "address comment")

[commit-bot: I haz the power, 2016-12-02 20:14:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-02 21:12:35]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480709623837440,
"parent_rev": "6c5738f08520ee964ad07bc0d970ab847f73e678", "commit_rev":
"60057cf167d62683f28ff54b611afbc57eb0f780"}

[commit-bot: I haz the power, 2016-12-02 21:13:03]

Description was changed from

==========
Allow an external URL with FROM_API qualifier to be forwarded to ARC

for better AppAuth compatibility.

In a highly unusual situation, allowing FROM_API could trigger
infinite Chrome tab creation. This CL also adds code for detecting
and stopping the loop, just in case. End users will almost never
see the safety checks to kick in, though.

BUG=670465
TEST=unit_tests passed, also manually confirmed that when
 |always_ask_user| is true, "Always" preference set by
 the user is ignored.
==========

to

==========
Allow an external URL with FROM_API qualifier to be forwarded to ARC

for better AppAuth compatibility.

In a highly unusual situation, allowing FROM_API could trigger
infinite Chrome tab creation. This CL also adds code for detecting
and stopping the loop, just in case. End users will almost never
see the safety checks to kick in, though.

BUG=670465
TEST=unit_tests passed, also manually confirmed that when
 |always_ask_user| is true, "Always" preference set by
 the user is ignored.
==========

[commit-bot: I haz the power, 2016-12-02 21:13:04]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-02 21:16:38]

Description was changed from

==========
Allow an external URL with FROM_API qualifier to be forwarded to ARC

for better AppAuth compatibility.

In a highly unusual situation, allowing FROM_API could trigger
infinite Chrome tab creation. This CL also adds code for detecting
and stopping the loop, just in case. End users will almost never
see the safety checks to kick in, though.

BUG=670465
TEST=unit_tests passed, also manually confirmed that when
 |always_ask_user| is true, "Always" preference set by
 the user is ignored.
==========

to

==========
Allow an external URL with FROM_API qualifier to be forwarded to ARC

for better AppAuth compatibility.

In a highly unusual situation, allowing FROM_API could trigger
infinite Chrome tab creation. This CL also adds code for detecting
and stopping the loop, just in case. End users will almost never
see the safety checks to kick in, though.

BUG=670465
TEST=unit_tests passed, also manually confirmed that when
 |always_ask_user| is true, "Always" preference set by
 the user is ignored.

Committed: https://crrev.com/0c7e06f5b85f0304bd62f61a9b2ae391e97f3183
Cr-Commit-Position: refs/heads/master@{#436017}
==========

[commit-bot: I haz the power, 2016-12-02 21:16:39]

Patchset 3 (id:??) landed as
https://crrev.com/0c7e06f5b85f0304bd62f61a9b2ae391e97f3183
Cr-Commit-Position: refs/heads/master@{#436017}