CREDENTIAL: Convert federations from URLs to origins throughout.

CREDENTIAL: Convert federations from URLs to origins throughout.

We've been loosely treating federations as URLs, as it was simple and
easy. That laxness, however, has caused some issues with the Android
integration, as they are doing exact string comparisons as opposed to
origin comparisons. In particular, they expect the serialization stored
to sync to exclude the trailing '/'.

This patch converts the field to an origin throughout, which ensures
both that the serialization matches Android's expectations, but also
that we're properly excluding paths and etc. from the comparisons we
make in various places in the codebase.

BUG=589016
R=vabr@chromium.org, vasilii@chromium.org, jochen@chromium.org
TBR=marq@chromium.org

Committed: https://crrev.com/0604e9e30c88836aaab577189b7c1f19f2ab55d8
Cr-Commit-Position: refs/heads/master@{#377834}

[Mike West, 2016-02-23 13:50:25]

vabr@, vasilii@: Would you mind skimming this patch? It's mostly mechanical.

jochen@: Can you review the Blink bits (and stamp the rest so I don't have to
TBR it once vabr@ and vasilii@ are happy? :))

[vabr (Chromium), 2016-02-23 14:33:54]

Passwords LGTM once the bots are happy.

I was wondering whether we would need any migration for the already stored
credentials, but it seems that all we do when reading it from the store is
running it additionally through the GURL->url::Origin constructor, so there
should be no problem.

Cheers,
Vaclav

[jochen (gone - plz use gerrit), 2016-02-23 14:35:53]

blink lgtm

[vasilii, 2016-02-23 14:59:21]

https://codereview.chromium.org/1723583004/diff/1/chrome/browser/password_man...
File chrome/browser/password_manager/native_backend_gnome_x.cc (right):

https://codereview.chromium.org/1723583004/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/native_backend_gnome_x.cc:371: "application",
app_string, nullptr);
Nice formatting please.

https://codereview.chromium.org/1723583004/diff/1/chrome/browser/password_man...
File chrome/browser/password_manager/native_backend_libsecret.cc (right):

https://codereview.chromium.org/1723583004/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/native_backend_libsecret.cc:474:
form.generation_upload_status, "form_data", form_data.c_str(),
Please return the nice formatting.

https://codereview.chromium.org/1723583004/diff/1/components/autofill/core/co...
File components/autofill/core/common/password_form.cc (right):

https://codereview.chromium.org/1723583004/diff/1/components/autofill/core/co...
components/autofill/core/common/password_form.cc:122:
federation_origin.Serialize() == form.federation_origin.Serialize() &&
what about operator== ?

https://codereview.chromium.org/1723583004/diff/1/components/password_manager...
File
components/password_manager/core/browser/credential_manager_pending_request_task.cc
(right):

https://codereview.chromium.org/1723583004/diff/1/components/password_manager...
components/password_manager/core/browser/credential_manager_pending_request_task.cc:26:
const std::vector<GURL>& request_federations,
What about vector of Origin?

https://codereview.chromium.org/1723583004/diff/1/components/password_manager...
File components/password_manager/core/browser/login_database_unittest.cc
(right):

https://codereview.chromium.org/1723583004/diff/1/components/password_manager...
components/password_manager/core/browser/login_database_unittest.cc:68:
url::Origin(GURL("https://accounts.google.com/federation"));
Here and below should be fixed.

https://codereview.chromium.org/1723583004/diff/1/components/password_manager...
File
components/password_manager/core/browser/password_syncable_service_unittest.cc
(right):

https://codereview.chromium.org/1723583004/diff/1/components/password_manager...
components/password_manager/core/browser/password_syncable_service_unittest.cc:57:
const char kFederationUrl[] = "https://fb.com/federation_url";
Need to be fixed.

https://codereview.chromium.org/1723583004/diff/1/components/password_manager...
File components/password_manager/core/common/credential_manager_types.cc
(right):

https://codereview.chromium.org/1723583004/diff/1/components/password_manager...
components/password_manager/core/common/credential_manager_types.cc:67:
federation.Serialize() == rhs.federation.Serialize());
Why aren't you happy with operator== ?

[Mike West, 2016-02-25 09:56:16]

https://codereview.chromium.org/1723583004/diff/1/chrome/browser/password_man...
File chrome/browser/password_manager/native_backend_gnome_x.cc (right):

https://codereview.chromium.org/1723583004/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/native_backend_gnome_x.cc:371: "application",
app_string, nullptr);
On 2016/02/23 at 14:59:20, vasilii wrote:
> Nice formatting please.

If you liked it, you should have put a clang-format off on it. :)

https://codereview.chromium.org/1723583004/diff/1/chrome/browser/password_man...
File chrome/browser/password_manager/native_backend_libsecret.cc (right):

https://codereview.chromium.org/1723583004/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/native_backend_libsecret.cc:474:
form.generation_upload_status, "form_data", form_data.c_str(),
On 2016/02/23 at 14:59:20, vasilii wrote:
> Please return the nice formatting.

Ditto.

https://codereview.chromium.org/1723583004/diff/1/components/autofill/core/co...
File components/autofill/core/common/password_form.cc (right):

https://codereview.chromium.org/1723583004/diff/1/components/autofill/core/co...
components/autofill/core/common/password_form.cc:122:
federation_origin.Serialize() == form.federation_origin.Serialize() &&
On 2016/02/23 at 14:59:20, vasilii wrote:
> what about operator== ?

unique origins don't `==` each other, which means that two password credentials
would never `==` each other if we didn't look at the origin's serialization.

https://codereview.chromium.org/1723583004/diff/1/components/password_manager...
File
components/password_manager/core/browser/credential_manager_pending_request_task.cc
(right):

https://codereview.chromium.org/1723583004/diff/1/components/password_manager...
components/password_manager/core/browser/credential_manager_pending_request_task.cc:26:
const std::vector<GURL>& request_federations,
On 2016/02/23 at 14:59:20, vasilii wrote:
> What about vector of Origin?

This was already a huge patch. I guess I can do this as well. *shrug*

https://codereview.chromium.org/1723583004/diff/1/components/password_manager...
File components/password_manager/core/browser/login_database_unittest.cc
(right):

https://codereview.chromium.org/1723583004/diff/1/components/password_manager...
components/password_manager/core/browser/login_database_unittest.cc:68:
url::Origin(GURL("https://accounts.google.com/federation"));
On 2016/02/23 at 14:59:20, vasilii wrote:
> Here and below should be fixed.

Dropped the '/federation' (which I think is what you meant?)

https://codereview.chromium.org/1723583004/diff/1/components/password_manager...
File components/password_manager/core/common/credential_manager_types.cc
(right):

https://codereview.chromium.org/1723583004/diff/1/components/password_manager...
components/password_manager/core/common/credential_manager_types.cc:67:
federation.Serialize() == rhs.federation.Serialize());
On 2016/02/23 at 14:59:20, vasilii wrote:
> Why aren't you happy with operator== ?

Same as above.

[vasilii, 2016-02-25 12:38:01]

lgtm

https://codereview.chromium.org/1723583004/diff/1/chrome/browser/password_man...
File chrome/browser/password_manager/native_backend_gnome_x.cc (right):

https://codereview.chromium.org/1723583004/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/native_backend_gnome_x.cc:371: "application",
app_string, nullptr);
On 2016/02/25 09:56:15, Mike West wrote:
> On 2016/02/23 at 14:59:20, vasilii wrote:
> > Nice formatting please.
> 
> If you liked it, you should have put a clang-format off on it. :)

I didn't know about that directive.

https://codereview.chromium.org/1723583004/diff/1/components/autofill/core/co...
File components/autofill/core/common/password_form.cc (right):

https://codereview.chromium.org/1723583004/diff/1/components/autofill/core/co...
components/autofill/core/common/password_form.cc:122:
federation_origin.Serialize() == form.federation_origin.Serialize() &&
On 2016/02/25 09:56:16, Mike West wrote:
> On 2016/02/23 at 14:59:20, vasilii wrote:
> > what about operator== ?
> 
> unique origins don't `==` each other, which means that two password
credentials
> would never `==` each other if we didn't look at the origin's serialization.

Acknowledged.

https://codereview.chromium.org/1723583004/diff/1/components/password_manager...
File
components/password_manager/core/browser/credential_manager_pending_request_task.cc
(right):

https://codereview.chromium.org/1723583004/diff/1/components/password_manager...
components/password_manager/core/browser/credential_manager_pending_request_task.cc:26:
const std::vector<GURL>& request_federations,
On 2016/02/25 09:56:16, Mike West wrote:
> On 2016/02/23 at 14:59:20, vasilii wrote:
> > What about vector of Origin?
> 
> This was already a huge patch. I guess I can do this as well. *shrug*

Acknowledged.

[Mike West, 2016-02-25 19:11:23]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2016-02-25 19:11:23]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
jochen@chromium.org, vasilii@chromium.org
Link to the patchset: https://codereview.chromium.org/1723583004/#ps60001
(title: "iOS2")

[commit-bot: I haz the power, 2016-02-25 19:17:00]

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

[commit-bot: I haz the power, 2016-02-25 19:46:31]

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

[commit-bot: I haz the power, 2016-02-25 19:46:33]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Mike West, 2016-02-26 06:33:26]

Description was changed from

==========
CREDENTIAL: Convert federations from URLs to origins throughout.

We've been loosely treating federations as URLs, as it was simple and
easy. That laxness, however, has caused some issues with the Android
integration, as they are doing exact string comparisons as opposed to
origin comparisons. In particular, they expect the serialization stored
to sync to exclude the trailing '/'.

This patch converts the field to an origin throughout, which ensures
both that the serialization matches Android's expectations, but also
that we're properly excluding paths and etc. from the comparisons we
make in various places in the codebase.

BUG=589016
R=vabr@chromium.org, vasilii@chromium.org, jochen@chromium.org
==========

to

==========
CREDENTIAL: Convert federations from URLs to origins throughout.

We've been loosely treating federations as URLs, as it was simple and
easy. That laxness, however, has caused some issues with the Android
integration, as they are doing exact string comparisons as opposed to
origin comparisons. In particular, they expect the serialization stored
to sync to exclude the trailing '/'.

This patch converts the field to an origin throughout, which ensures
both that the serialization matches Android's expectations, but also
that we're properly excluding paths and etc. from the comparisons we
make in various places in the codebase.

BUG=589016
R=vabr@chromium.org, vasilii@chromium.org, jochen@chromium.org
TBR=marq@chromium.org
==========

[Mike West, 2016-02-26 06:33:27]

mkwst@chromium.org changed reviewers:
+ marq@chromium.org

[Mike West, 2016-02-26 06:34:44]

On 2016/02/25 at 19:46:33, commit-bot wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

TBRing the mechanical iOS changes that vabr@ doesn't own directly to marq@. Hi,
marq@!

[Mike West, 2016-02-26 06:34:49]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2016-02-26 06:36:19]

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

[commit-bot: I haz the power, 2016-02-26 06:46:31]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-02-26 06:47:41]

Description was changed from

==========
CREDENTIAL: Convert federations from URLs to origins throughout.

We've been loosely treating federations as URLs, as it was simple and
easy. That laxness, however, has caused some issues with the Android
integration, as they are doing exact string comparisons as opposed to
origin comparisons. In particular, they expect the serialization stored
to sync to exclude the trailing '/'.

This patch converts the field to an origin throughout, which ensures
both that the serialization matches Android's expectations, but also
that we're properly excluding paths and etc. from the comparisons we
make in various places in the codebase.

BUG=589016
R=vabr@chromium.org, vasilii@chromium.org, jochen@chromium.org
TBR=marq@chromium.org
==========

to

==========
CREDENTIAL: Convert federations from URLs to origins throughout.

We've been loosely treating federations as URLs, as it was simple and
easy. That laxness, however, has caused some issues with the Android
integration, as they are doing exact string comparisons as opposed to
origin comparisons. In particular, they expect the serialization stored
to sync to exclude the trailing '/'.

This patch converts the field to an origin throughout, which ensures
both that the serialization matches Android's expectations, but also
that we're properly excluding paths and etc. from the comparisons we
make in various places in the codebase.

BUG=589016
R=vabr@chromium.org, vasilii@chromium.org, jochen@chromium.org
TBR=marq@chromium.org

Committed: https://crrev.com/0604e9e30c88836aaab577189b7c1f19f2ab55d8
Cr-Commit-Position: refs/heads/master@{#377834}
==========

[commit-bot: I haz the power, 2016-02-26 06:47:42]

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

[melandory, 2016-03-11 10:01:05]

melandory@chromium.org changed reviewers:
+ melandory@chromium.org

[melandory, 2016-03-11 10:01:06]

melandory@chromium.org changed reviewers:
+ melandory@chromium.org

[melandory, 2016-03-11 10:01:06]

https://codereview.chromium.org/1723583004/diff/60001/chrome/browser/password...
File chrome/browser/password_manager/save_password_infobar_delegate.cc (right):

https://codereview.chromium.org/1723583004/diff/60001/chrome/browser/password...
chrome/browser/password_manager/save_password_infobar_delegate.cc:101: :
PasswordTittleType::UPDATE_PASSWORD;
Sad story about the fix which didn't survive
https://codereview.chromium.org/1717263002/ :)

[melandory, 2016-03-11 10:01:07]

https://codereview.chromium.org/1723583004/diff/60001/chrome/browser/password...
File chrome/browser/password_manager/save_password_infobar_delegate.cc (right):

https://codereview.chromium.org/1723583004/diff/60001/chrome/browser/password...
chrome/browser/password_manager/save_password_infobar_delegate.cc:101: :
PasswordTittleType::UPDATE_PASSWORD;
Sad story about the fix which didn't survive
https://codereview.chromium.org/1717263002/ :)