Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
139253004:
Add the database field to require additional auth for autofilling passwords. (Closed)
|
Created:
6 years, 10 months ago by Ted C Modified:
6 years, 10 months ago CC:
chromium-reviews, benquan, jam, browser-components-watch_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, Dane Wallinga, dyu1, estade+watch_chromium.org, rouslan+autofillwatch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd the database field to require additional auth for autofilling passwords.
BUG=341492
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=250286
Patch Set 1 #
Total comments: 4
Patch Set 2 : Removed IPC change #Patch Set 3 : Add comment to IPC message about excluded field. #
Messages
Total messages: 34 (0 generated)
PTAL
This looks plausible to me, but I'm not actually very familiar with the password DBs. Hopefully Mike or Stuart can take a look. https://codereview.chromium.org/139253004/diff/1/components/autofill/content/... File components/autofill/content/common/autofill_param_traits_macros.h (right): https://codereview.chromium.org/139253004/diff/1/components/autofill/content/... components/autofill/content/common/autofill_param_traits_macros.h:44: IPC_STRUCT_TRAITS_MEMBER(use_additional_authentication) Why does this need to be passed across IPC? Seems like a purely browser-process bit of info.
https://codereview.chromium.org/139253004/diff/1/components/autofill/content/... File components/autofill/content/common/autofill_param_traits_macros.h (right): https://codereview.chromium.org/139253004/diff/1/components/autofill/content/... components/autofill/content/common/autofill_param_traits_macros.h:44: IPC_STRUCT_TRAITS_MEMBER(use_additional_authentication) On 2014/02/08 01:12:21, Ilya Sherman (catching up...) wrote: > Why does this need to be passed across IPC? Seems like a purely browser-process > bit of info. No direct need. I did it more out of completeness. I can definitely revert this bit.
So, there's an additional complication due to the way we store passwords on Linux. In that case, we don't use the login database at all, so this new field will not be preserved as the CL stands now. If that's OK (e.g. if you only intend to use it on Windows or something) then that's fine. Otherwise you'll need to edit the native_backend_* files to add this field there as well, making sure to preserve backward compatibility. I'm happy to provide help with that if you need it.
On 2014/02/08 02:06:13, Mike Mammarella wrote: > So, there's an additional complication due to the way we store passwords on > Linux. In that case, we don't use the login database at all, so this new field > will not be preserved as the CL stands now. If that's OK (e.g. if you only > intend to use it on Windows or something) then that's fine. Otherwise you'll > need to edit the native_backend_* files to add this field there as well, making > sure to preserve backward compatibility. I'm happy to provide help with that if > you need it. Right now, this is restricted to only android devices, so I guess that is ok. I should just predicate making the above changes as soon as I start thinking that this field synced across devices. Here's the mega patch that contained this that shows the end to end flow (as well as the restriction to android): https://codereview.chromium.org/157983002/
https://codereview.chromium.org/139253004/diff/1/components/autofill/content/... File components/autofill/content/common/autofill_param_traits_macros.h (right): https://codereview.chromium.org/139253004/diff/1/components/autofill/content/... components/autofill/content/common/autofill_param_traits_macros.h:44: IPC_STRUCT_TRAITS_MEMBER(use_additional_authentication) On 2014/02/08 01:16:17, Ted C wrote: > On 2014/02/08 01:12:21, Ilya Sherman (catching up...) wrote: > > Why does this need to be passed across IPC? Seems like a purely > browser-process > > bit of info. > > No direct need. I did it more out of completeness. I can definitely revert > this bit. Updated the change and removed this bit.
OK, based on the design doc where it says that it's OK if passwords synced from the cloud don't preserve this bit, the login database changes LGTM.
The CQ bit was checked by tedchoc@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tedchoc@chromium.org/139253004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
On 2014/02/10 20:09:32, I haz the power (commit-bot) wrote: > Retried try job too often on chromium_presubmit for step(s) presubmit > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p... @isherman - looks like I'll need your OWNERs stamp for components/autofill
LGTM.
The CQ bit was checked by tedchoc@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tedchoc@chromium.org/139253004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) telemetry_perf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by tedchoc@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tedchoc@chromium.org/139253004/120001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tedchoc@chromium.org/139253004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
On 2014/02/10 23:13:56, I haz the power (commit-bot) wrote: > Retried try job too often on chromium_presubmit for step(s) presubmit > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p... Hmm... gcasto@ is in components/autofill/OWNERS, but it keeps complaining about not having a proper OWNER. Am I missing something here?
https://codereview.chromium.org/139253004/diff/1/components/autofill/content/... File components/autofill/content/common/autofill_param_traits_macros.h (right): https://codereview.chromium.org/139253004/diff/1/components/autofill/content/... components/autofill/content/common/autofill_param_traits_macros.h:44: IPC_STRUCT_TRAITS_MEMBER(use_additional_authentication) On 2014/02/10 18:51:32, Ted C wrote: > On 2014/02/08 01:16:17, Ted C wrote: > > On 2014/02/08 01:12:21, Ilya Sherman (catching up...) wrote: > > > Why does this need to be passed across IPC? Seems like a purely > > browser-process > > > bit of info. > > > > No direct need. I did it more out of completeness. I can definitely revert > > this bit. > > Updated the change and removed this bit. Hmm, I'm not sure that it's safe to just remove this change, while still tweaking the underlying struct. My question is: Why does this need to be part of the struct that's used for communicating over IPC? To save time, if the answer is that it's a royal PITA to put this anywhere else, then please make really sure you know what you're doing in terms of IPC. Is it actually safe to omit the struct member? Or is there a way to include the struct member, but always clear it when sending the IPC, so that no information is leaked to the renderer process?
On 2014/02/10 23:32:54, Ilya Sherman (catching up...) wrote: > https://codereview.chromium.org/139253004/diff/1/components/autofill/content/... > File components/autofill/content/common/autofill_param_traits_macros.h (right): > > https://codereview.chromium.org/139253004/diff/1/components/autofill/content/... > components/autofill/content/common/autofill_param_traits_macros.h:44: > IPC_STRUCT_TRAITS_MEMBER(use_additional_authentication) > On 2014/02/10 18:51:32, Ted C wrote: > > On 2014/02/08 01:16:17, Ted C wrote: > > > On 2014/02/08 01:12:21, Ilya Sherman (catching up...) wrote: > > > > Why does this need to be passed across IPC? Seems like a purely > > > browser-process > > > > bit of info. > > > > > > No direct need. I did it more out of completeness. I can definitely revert > > > this bit. > > > > Updated the change and removed this bit. > > Hmm, I'm not sure that it's safe to just remove this change, while still > tweaking the underlying struct. My question is: Why does this need to be part > of the struct that's used for communicating over IPC? > > To save time, if the answer is that it's a royal PITA to put this anywhere else, > then please make really sure you know what you're doing in terms of IPC. Is it > actually safe to omit the struct member? Or is there a way to include the > struct member, but always clear it when sending the IPC, so that no information > is leaked to the renderer process? I've accidentally not included something like this in IPC definition before, and my understanding is that the object is essentially constructed on the other side and all of the fields that are specified here are copied. Unspecified fields are left in whatever state the constructor leaves them in. So I don't think that this is unsafe, though if someone tried to access it from the renderer it may be confusing.
On 2014/02/10 23:20:32, Ted C wrote: > On 2014/02/10 23:13:56, I haz the power (commit-bot) wrote: > > Retried try job too often on chromium_presubmit for step(s) presubmit > > > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p... > > Hmm... gcasto@ is in components/autofill/OWNERS, but it keeps complaining about > not having a proper OWNER. Am I missing something here? I'm not sure what is up with the OWNERS issue. Maybe ask on IRC or chromium-dev?
On 2014/02/10 23:37:12, Garrett Casto wrote: > On 2014/02/10 23:32:54, Ilya Sherman (catching up...) wrote: > > > https://codereview.chromium.org/139253004/diff/1/components/autofill/content/... > > File components/autofill/content/common/autofill_param_traits_macros.h > (right): > > > > > https://codereview.chromium.org/139253004/diff/1/components/autofill/content/... > > components/autofill/content/common/autofill_param_traits_macros.h:44: > > IPC_STRUCT_TRAITS_MEMBER(use_additional_authentication) > > On 2014/02/10 18:51:32, Ted C wrote: > > > On 2014/02/08 01:16:17, Ted C wrote: > > > > On 2014/02/08 01:12:21, Ilya Sherman (catching up...) wrote: > > > > > Why does this need to be passed across IPC? Seems like a purely > > > > browser-process > > > > > bit of info. > > > > > > > > No direct need. I did it more out of completeness. I can definitely > revert > > > > this bit. > > > > > > Updated the change and removed this bit. > > > > Hmm, I'm not sure that it's safe to just remove this change, while still > > tweaking the underlying struct. My question is: Why does this need to be part > > of the struct that's used for communicating over IPC? > > > > To save time, if the answer is that it's a royal PITA to put this anywhere > else, > > then please make really sure you know what you're doing in terms of IPC. Is > it > > actually safe to omit the struct member? Or is there a way to include the > > struct member, but always clear it when sending the IPC, so that no > information > > is leaked to the renderer process? > > I've accidentally not included something like this in IPC definition before, and > my understanding is that the object is essentially constructed on the other side > and all of the fields that are specified here are copied. Unspecified fields are > left in whatever state the constructor leaves them in. So I don't think that > this is unsafe, though if someone tried to access it from the renderer it may be > confusing. Ah, nice -- that actually sounds like exactly what we want :) In that case, it's probably a good idea to document in the IPC macros file that the field is omitted intentionally, rather than as an oversight. With that done, LGTM -- thanks!
On 2014/02/10 23:48:10, Ilya Sherman (catching up...) wrote: > On 2014/02/10 23:37:12, Garrett Casto wrote: > > On 2014/02/10 23:32:54, Ilya Sherman (catching up...) wrote: > > > > > > https://codereview.chromium.org/139253004/diff/1/components/autofill/content/... > > > File components/autofill/content/common/autofill_param_traits_macros.h > > (right): > > > > > > > > > https://codereview.chromium.org/139253004/diff/1/components/autofill/content/... > > > components/autofill/content/common/autofill_param_traits_macros.h:44: > > > IPC_STRUCT_TRAITS_MEMBER(use_additional_authentication) > > > On 2014/02/10 18:51:32, Ted C wrote: > > > > On 2014/02/08 01:16:17, Ted C wrote: > > > > > On 2014/02/08 01:12:21, Ilya Sherman (catching up...) wrote: > > > > > > Why does this need to be passed across IPC? Seems like a purely > > > > > browser-process > > > > > > bit of info. > > > > > > > > > > No direct need. I did it more out of completeness. I can definitely > > revert > > > > > this bit. > > > > > > > > Updated the change and removed this bit. > > > > > > Hmm, I'm not sure that it's safe to just remove this change, while still > > > tweaking the underlying struct. My question is: Why does this need to be > part > > > of the struct that's used for communicating over IPC? > > > > > > To save time, if the answer is that it's a royal PITA to put this anywhere > > else, > > > then please make really sure you know what you're doing in terms of IPC. Is > > it > > > actually safe to omit the struct member? Or is there a way to include the > > > struct member, but always clear it when sending the IPC, so that no > > information > > > is leaked to the renderer process? > > > > I've accidentally not included something like this in IPC definition before, > and > > my understanding is that the object is essentially constructed on the other > side > > and all of the fields that are specified here are copied. Unspecified fields > are > > left in whatever state the constructor leaves them in. So I don't think that > > this is unsafe, though if someone tried to access it from the renderer it may > be > > confusing. > > Ah, nice -- that actually sounds like exactly what we want :) > > In that case, it's probably a good idea to document in the IPC macros file that > the field is omitted intentionally, rather than as an oversight. With that > done, LGTM -- thanks! I'm actually quite surpised we send "blacklisted_by_user". That seems quite scary as well...but a tangent.
On 2014/02/10 23:51:21, Ted C wrote: > I'm actually quite surpised we send "blacklisted_by_user". That seems quite > scary > as well...but a tangent. Yeah... we currently send way too much data over to the renderer. This is essentially a historical accident that's planned to be fixed as part of the OOP iframes effort.
On 2014/02/10 23:58:13, Ilya Sherman (catching up...) wrote: > On 2014/02/10 23:51:21, Ted C wrote: > > I'm actually quite surpised we send "blacklisted_by_user". That seems quite > > scary > > as well...but a tangent. > > Yeah... we currently send way too much data over to the renderer. This is > essentially a historical accident that's planned to be fixed as part of the OOP > iframes effort. Alrighty, I added a comment to the traits macro and hopefully is clear.
LG, thanks.
The CQ bit was checked by tedchoc@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tedchoc@chromium.org/139253004/510001
Message was sent while issue was closed.
Change committed as 250286 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
