Attention is currently required from: Siyu An.
Vipul Koul would like Siyu An to review this change.
[CVV Storage][Settings] Adding cvv field to credit card object & methods
Bug:1464441
Change-Id: Id914887ec02ff7117385bea69d728af184c6e412
---
M chrome/browser/extensions/api/autofill_private/autofill_private_api.cc
M chrome/browser/extensions/api/autofill_private/autofill_util.cc
M chrome/common/extensions/api/autofill_private.idl
M tools/typescript/definitions/autofill_private.d.ts
4 files changed, 12 insertions(+), 0 deletions(-)
To view, visit change 4865585. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Siyu An.
Vipul Koul uploaded patch set #2 to this change.
[CVV Storage][Settings] Adding cvv field to credit card object & methods
Design doc- go/pay-autofill-cvv-storage-chrome
UX mocks- go/autofill-cvv-storage-ux
Bug:1464441
Change-Id: Id914887ec02ff7117385bea69d728af184c6e412
---
M chrome/browser/extensions/api/autofill_private/autofill_private_api.cc
M chrome/browser/extensions/api/autofill_private/autofill_util.cc
M chrome/common/extensions/api/autofill_private.idl
M tools/typescript/definitions/autofill_private.d.ts
4 files changed, 12 insertions(+), 0 deletions(-)
To view, visit change 4865585. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Demetrios Papadopoulos, Giovanni Ortuno Urquidi.
Vipul Koul would like Giovanni Ortuno Urquidi and Demetrios Papadopoulos to review this change.
[CVV Storage][Settings] Adding cvv field to credit card object & methods
Design doc- go/pay-autofill-cvv-storage-chrome
UX mocks- go/autofill-cvv-storage-ux
Bug:1464441
Change-Id: Id914887ec02ff7117385bea69d728af184c6e412
---
M chrome/browser/extensions/api/autofill_private/autofill_private_api.cc
M chrome/browser/extensions/api/autofill_private/autofill_util.cc
M chrome/common/extensions/api/autofill_private.idl
M tools/typescript/definitions/autofill_private.d.ts
4 files changed, 12 insertions(+), 0 deletions(-)
To view, visit change 4865585. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Demetrios Papadopoulos, Giovanni Ortuno Urquidi.
1 comment:
Patchset:
Hey folks,
Can you please review this CL for-
dpapad- all except autofill_private.idl
ortuno- autofill_private.idl
Thanks!
To view, visit change 4865585. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Demetrios Papadopoulos, Vipul Koul.
Patch set 2:Code-Review +1
1 comment:
Patchset:
chrome/common/extensions/api/autofill_private.idl lgtm, didn't look at the rest.
To view, visit change 4865585. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Vipul Koul.
Vipul Koul has uploaded this change for review.
[CVV Storage][Settings] Adding cvv field to credit card object & methods
Design doc- go/pay-autofill-cvv-storage-chrome
UX mocks- go/autofill-cvv-storage-ux
Bug:1464441
Change-Id: Id914887ec02ff7117385bea69d728af184c6e412
---
M chrome/browser/extensions/api/autofill_private/autofill_private_api.cc
M chrome/browser/extensions/api/autofill_private/autofill_util.cc
M chrome/common/extensions/api/autofill_private.idl
M tools/typescript/definitions/autofill_private.d.ts
4 files changed, 12 insertions(+), 0 deletions(-)
To view, visit change 4865585. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Vipul Koul.
Patch set 2:Code-Review +1
1 comment:
Patchset:
FWIW, I think is best for such a change to be reviewed primarily by someone in components/autofill/OWNERS, instead of fallback WebUI OWNERS.
cc'ing battre@ as FYI.
To view, visit change 4865585. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Vipul Koul.
1 comment:
File chrome/browser/extensions/api/autofill_private/autofill_util.cc:
if (!credit_card.cvc().empty()) {
card.cvc = base::UTF16ToUTF8(credit_card.cvc());
}
Shouldn't this be gated on the user having entered their OS password / PIN / fingerprint / ...?
See https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/extensions/api/passwords_private/passwords_private_api.h;l=84;drc=6a3b664aa1d6af7d818898cf17b2c470ecdea8fb for how the password manager does this.
To view, visit change 4865585. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Dominic Battre.
1 comment:
File chrome/browser/extensions/api/autofill_private/autofill_util.cc:
if (!credit_card.cvc().empty()) {
card.cvc = base::UTF16ToUTF8(credit_card.cvc());
}
Shouldn't this be gated on the user having entered their OS password / PIN / fingerprint / ...? […]
I think the current implementation should work. There are 3 cases to consider here:
- Local cards (mandatory reauth is off):
CVC and the card details for these cards are visible on the settings page and it can be autofilled without any auth. This doesn't satisfy the check, but this is expected current behavior and also documented in the design doc- https://docs.google.com/document/d/1YcOXV_EiwXoL7v5mnYMgCEMbQ8V_ktYFZjz9kvzZMjg/edit?resourcekey=0-Zaz3xGfnm1Eh4YV-vZVXBA#heading=h.wl88afs9xs3e
Thanks for bringing this up Dominic and please let me know if i missed out any edge case here.
To view, visit change 4865585. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Vipul Koul.
1 comment:
File chrome/browser/extensions/api/autofill_private/autofill_util.cc:
if (!credit_card.cvc().empty()) {
card.cvc = base::UTF16ToUTF8(credit_card.cvc());
}
I think the current implementation should work. […]
Open chrome://settings/payments, open DevTools and type `await chrome.autofillPrivate.getCreditCardList();`.
My point is that we should not disclose the CVC via DevTools without a reauth if it's generally required to access it on the page.
Do I miss anything?
To view, visit change 4865585. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Demetrios Papadopoulos, Giovanni Ortuno Urquidi, Siyu An, Vipul Koul.
Vipul Koul uploaded patch set #3 to this change.
The following approvals got outdated and were removed: Code-Review+1 by Demetrios Papadopoulos, Code-Review+1 by Giovanni Ortuno Urquidi, Code-Review+1 by Siyu An
The change is no longer submittable: Code-Review is unsatisfied now.
[CVV Storage][Settings] Adding cvv field to credit card object & methods
Design doc- go/pay-autofill-cvv-storage-chrome
UX mocks- go/autofill-cvv-storage-ux
Bug:1464441
Change-Id: Id914887ec02ff7117385bea69d728af184c6e412
---
M chrome/browser/extensions/api/autofill_private/autofill_private_api.cc
M chrome/browser/extensions/api/autofill_private/autofill_util.cc
M chrome/common/extensions/api/autofill_private.idl
M chrome/test/data/extensions/api_test/autofill_private/test.js
M tools/typescript/definitions/autofill_private.d.ts
5 files changed, 32 insertions(+), 5 deletions(-)
To view, visit change 4865585. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Dominic Battre, Siyu An.
Vipul Koul would like Dominic Battre to review this change.
[CVV Storage][Settings] Adding cvv field to credit card object & methods
Design doc- go/pay-autofill-cvv-storage-chrome
UX mocks- go/autofill-cvv-storage-ux
Bug:1464441
Change-Id: Id914887ec02ff7117385bea69d728af184c6e412
---
M chrome/browser/extensions/api/autofill_private/autofill_private_api.cc
M chrome/browser/extensions/api/autofill_private/autofill_util.cc
M chrome/common/extensions/api/autofill_private.idl
M chrome/test/data/extensions/api_test/autofill_private/test.js
M tools/typescript/definitions/autofill_private.d.ts
5 files changed, 32 insertions(+), 5 deletions(-)
Attention is currently required from: Dominic Battre, Siyu An.
2 comments:
Patchset:
Hey folks,
Can you please re-review this CL? I have added the masking mechanism for the cvc now.
Thanks!
File chrome/browser/extensions/api/autofill_private/autofill_util.cc:
if (!credit_card.cvc().empty()) {
card.cvc = base::UTF16ToUTF8(credit_card.cvc());
}
Open chrome://settings/payments, open DevTools and type `await chrome.autofillPrivate. […]
Updated that and added the masking-https://screenshot.googleplex.com/5raFLiTnkNA7yMq
Thanks for bringing this up, Dominic!!
To view, visit change 4865585. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Dominic Battre, Siyu An.
Vipul Koul uploaded patch set #4 to this change.
[CVV Storage][Settings] Adding cvv field to credit card object & methods
Design doc- go/pay-autofill-cvv-storage-chrome
UX mocks- go/autofill-cvv-storage-ux
Bug:1464441
Change-Id: Id914887ec02ff7117385bea69d728af184c6e412
---
M chrome/browser/extensions/api/autofill_private/autofill_private_api.cc
M chrome/browser/extensions/api/autofill_private/autofill_util.cc
M chrome/common/extensions/api/autofill_private.idl
M chrome/test/data/extensions/api_test/autofill_private/test.js
M tools/typescript/definitions/autofill_private.d.ts
5 files changed, 37 insertions(+), 8 deletions(-)
To view, visit change 4865585. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Dominic Battre, Siyu An, Vipul Koul.
Patch set 4:Code-Review +1
1 comment:
Patchset:
LGTM for tools/typescript/definitions/autofill_private.d.ts.
To view, visit change 4865585. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Dominic Battre, Siyu An, Vipul Koul.
Vipul Koul uploaded patch set #5 to this change.
[CVV Storage][Settings] Adding cvv field to credit card object & methods
Design doc- go/pay-autofill-cvv-storage-chrome
UX mocks- go/autofill-cvv-storage-ux
Bug:1464441
Change-Id: Id914887ec02ff7117385bea69d728af184c6e412
---
M chrome/browser/extensions/api/autofill_private/autofill_private_api.cc
M chrome/browser/extensions/api/autofill_private/autofill_util.cc
M chrome/common/extensions/api/autofill_private.idl
M chrome/test/data/extensions/api_test/autofill_private/test.js
M tools/typescript/definitions/autofill_private.d.ts
5 files changed, 38 insertions(+), 10 deletions(-)
To view, visit change 4865585. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Dominic Battre, Siyu An, Vipul Koul.
Vipul Koul uploaded patch set #6 to this change.
The following approvals got outdated and were removed: Commit-Queue+1 by Vipul Koul
[CVV Storage][Settings] Adding cvv field to credit card object & methods
Design doc- go/pay-autofill-cvv-storage-chrome
UX mocks- go/autofill-cvv-storage-ux
Bug:1464441
Change-Id: Id914887ec02ff7117385bea69d728af184c6e412
---
M chrome/browser/extensions/api/autofill_private/autofill_private_api.cc
M chrome/browser/extensions/api/autofill_private/autofill_util.cc
M chrome/common/extensions/api/autofill_private.idl
M chrome/test/data/extensions/api_test/autofill_private/test.js
M tools/typescript/definitions/autofill_private.d.ts
5 files changed, 37 insertions(+), 8 deletions(-)
To view, visit change 4865585. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Siyu An, Vipul Koul.
3 comments:
File chrome/browser/extensions/api/autofill_private/autofill_util.cc:
Patch Set #4, Line 366: re2::RE2::GlobalReplace(&(*(card.cvc)), ".", "*");
opt:
```
card.cvc = std::u16string(u'*', card.cvc->size());
```
Can you please make user that masked CVCs are shown as masked passwords in chrome://password-manager?
File chrome/browser/extensions/api/autofill_private/autofill_util.cc:
Patch Set #7, Line 359: . Always do this
delete these words?
To view, visit change 4865585. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Siyu An, Vipul Koul.
Vipul Koul uploaded patch set #8 to this change.
[CVV Storage][Settings] Adding cvv field to credit card object & methods
Design doc- go/pay-autofill-cvv-storage-chrome
UX mocks- go/autofill-cvv-storage-ux
Bug:1464441
Change-Id: Id914887ec02ff7117385bea69d728af184c6e412
---
M chrome/browser/extensions/api/autofill_private/autofill_private_api.cc
M chrome/browser/extensions/api/autofill_private/autofill_util.cc
M chrome/common/extensions/api/autofill_private.idl
M chrome/test/data/extensions/api_test/autofill_private/test.js
M tools/typescript/definitions/autofill_private.d.ts
5 files changed, 38 insertions(+), 9 deletions(-)
To view, visit change 4865585. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Siyu An, Vipul Koul.
Vipul Koul uploaded patch set #9 to this change.
The following approvals got outdated and were removed: Commit-Queue+1 by Vipul Koul
[CVV Storage][Settings] Adding cvv field to credit card object & methods
Design doc- go/pay-autofill-cvv-storage-chrome
UX mocks- go/autofill-cvv-storage-ux
Bug:1464441
Change-Id: Id914887ec02ff7117385bea69d728af184c6e412
---
M chrome/browser/extensions/api/autofill_private/autofill_private_api.cc
M chrome/browser/extensions/api/autofill_private/autofill_util.cc
M chrome/common/extensions/api/autofill_private.idl
M chrome/test/data/extensions/api_test/autofill_private/test.js
M tools/typescript/definitions/autofill_private.d.ts
5 files changed, 37 insertions(+), 8 deletions(-)
To view, visit change 4865585. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Dominic Battre, Siyu An.
3 comments:
File chrome/browser/extensions/api/autofill_private/autofill_util.cc:
Patch Set #4, Line 366: re2::RE2::GlobalReplace(&(*(card.cvc)), ".", "*");
opt: […]
I tried this but it would only show * instead of ***.
Impl- `card.cvc = std::string("*", card.cvc->size());`
Can you please make user that masked CVCs are shown as masked passwords in chrome://password-manager […]
Sorry i didn't get what you meant here.
If you are asking me to replace "*" with the passwords dot, we won't show this string/text to the user(unless they goto chrome dev tools and try `await chrome.autofillPrivate.getCreditCardList()`), so we should be good here!
For server cards: We won't show CVC at all in the settings page For local cards:
File chrome/browser/extensions/api/autofill_private/autofill_util.cc:
Patch Set #7, Line 359: . Always do this
delete these words?
Done
To view, visit change 4865585. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Siyu An, Vipul Koul.
Patch set 9:Code-Review +1
3 comments:
Patchset:
LGTM % use of password dots.
File chrome/browser/extensions/api/autofill_private/autofill_util.cc:
Patch Set #4, Line 366: re2::RE2::GlobalReplace(&(*(card.cvc)), ".", "*");
I tried this but it would only show * instead of ***. […]
Sorry, I swapped the order of parameters.
https://en.cppreference.com/w/cpp/string/basic_string/basic_string -> constructor 2
Note: not `"*"` but `u'*'` (the second parameter is a character)
Sorry i didn't get what you meant here. […]
Correct, my request is to replace the star with a password dot.
To view, visit change 4865585. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Vipul Koul.
Patch set 9:Code-Review +1
2 comments:
File chrome/browser/extensions/api/autofill_private/autofill_util.cc:
Patch Set #9, Line 34: #include "third_party/re2/src/re2/re2.h"
Remove this if you used the suggestion below to generate string instead of replacing char with *
File chrome/common/extensions/api/autofill_private.idl:
Patch Set #9, Line 260: // Credit card's cvc.
Update comment to mention it is masked cvc?
To view, visit change 4865585. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Vipul Koul.
Vipul Koul uploaded patch set #10 to this change.
[CVV Storage][Settings] Adding cvv field to credit card object & methods
Design doc- go/pay-autofill-cvv-storage-chrome
UX mocks- go/autofill-cvv-storage-ux
Bug:1464441
Change-Id: Id914887ec02ff7117385bea69d728af184c6e412
---
M chrome/browser/extensions/api/autofill_private/autofill_private_api.cc
M chrome/browser/extensions/api/autofill_private/autofill_util.cc
M chrome/common/extensions/api/autofill_private.idl
M chrome/test/data/extensions/api_test/autofill_private/test.js
M tools/typescript/definitions/autofill_private.d.ts
5 files changed, 35 insertions(+), 6 deletions(-)
To view, visit change 4865585. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Dominic Battre.
4 comments:
File chrome/browser/extensions/api/autofill_private/autofill_util.cc:
Patch Set #4, Line 366: re2::RE2::GlobalReplace(&(*(card.cvc)), ".", "*");
Sorry, I swapped the order of parameters. […]
Done, thanks for the suggestion Dominic!!
Correct, my request is to replace the star with a password dot.
I tried to do that but i am getting encoding issues during tests run- https://screenshot.googleplex.com/9FhVgkgLJiWEoiS
File chrome/browser/extensions/api/autofill_private/autofill_util.cc:
Patch Set #9, Line 34: #include "third_party/re2/src/re2/re2.h"
Remove this if you used the suggestion below to generate string instead of replacing char with *
Done
File chrome/common/extensions/api/autofill_private.idl:
Patch Set #9, Line 260: // Credit card's cvc.
Update comment to mention it is masked cvc?
Done
To view, visit change 4865585. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Vipul Koul.
Patch set 10:-Code-Review
1 comment:
File chrome/browser/extensions/api/autofill_private/autofill_util.cc:
I tried to do that but i am getting encoding issues during tests run- https://screenshot.googleplex. […]
can you share what you tried? exactly?
You probably need to write:
card.cvc = base::UTF16ToUTF8(std::u16string(card.cvc->size(), u'•'));
If you use the std::string constructor, the u'•' is downcasted to an 8 bit character.
To view, visit change 4865585. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Vipul Koul.
Vipul Koul uploaded patch set #11 to this change.
[CVV Storage][Settings] Adding cvv field to credit card object & methods
Design doc- go/pay-autofill-cvv-storage-chrome
UX mocks- go/autofill-cvv-storage-ux
Bug:1464441
Change-Id: Id914887ec02ff7117385bea69d728af184c6e412
---
M chrome/browser/extensions/api/autofill_private/autofill_private_api.cc
M chrome/browser/extensions/api/autofill_private/autofill_util.cc
M chrome/common/extensions/api/autofill_private.idl
M chrome/test/data/extensions/api_test/autofill_private/test.js
M tools/typescript/definitions/autofill_private.d.ts
5 files changed, 36 insertions(+), 6 deletions(-)
To view, visit change 4865585. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Dominic Battre.
1 comment:
File chrome/browser/extensions/api/autofill_private/autofill_util.cc:
I had used the symbol from here- https://www.w3schools.com/charsets/ref_utf_geometric.asp
can you share what you tried? exactly?
`card.cvc = std::string(card.cvc->size(), u'●');`
card.cvc = base::UTF16ToUTF8(std::u16string(card.cvc->size(), u'•'));
This worked but with a small caveat in test.js, LMK if i am missing something there. Thanks Dominic!!
To view, visit change 4865585. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Vipul Koul.
Patch set 11:Code-Review +1
1 comment:
File chrome/test/data/extensions/api_test/autofill_private/test.js:
// MASKED_CVC = '•••' which is encoded for tests.
var MASKED_CVC = decodeURI('%E2%80%A2%E2%80%A2%E2%80%A2');
Please put `var MASKED_CVC = '•••';` here and add the line
```
<meta charset="utf-8">
```
before the `<script>` tag in `chrome/test/data/extensions/api_test/autofill_private/main.html`.
I think that's more readable.
I have no idea why Chrome guesses the character set wrong.
To view, visit change 4865585. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Demetrios Papadopoulos, Dominic Battre, Siyu An.
Vipul Koul uploaded patch set #12 to this change.
The following approvals got outdated and were removed: Code-Review+1 by Demetrios Papadopoulos, Code-Review+1 by Dominic Battre, Code-Review+1 by Siyu An
The change is no longer submittable: Code-Review is unsatisfied now.
[CVV Storage][Settings] Adding cvv field to credit card object & methods
Design doc- go/pay-autofill-cvv-storage-chrome
UX mocks- go/autofill-cvv-storage-ux
Bug:1464441
Change-Id: Id914887ec02ff7117385bea69d728af184c6e412
---
M chrome/browser/extensions/api/autofill_private/autofill_private_api.cc
M chrome/browser/extensions/api/autofill_private/autofill_util.cc
M chrome/common/extensions/api/autofill_private.idl
M chrome/test/data/extensions/api_test/autofill_private/main.html
M chrome/test/data/extensions/api_test/autofill_private/test.js
M tools/typescript/definitions/autofill_private.d.ts
6 files changed, 34 insertions(+), 6 deletions(-)
To view, visit change 4865585. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Demetrios Papadopoulos, Dominic Battre, Siyu An.
// MASKED_CVC = '•••' which is encoded for tests.
var MASKED_CVC = decodeURI('%E2%80%A2%E2%80%A2%E2%80%A2');
Please put `var MASKED_CVC = '•••';` here and add the line […]
That worked, thanks Dominic!!
To view, visit change 4865585. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Demetrios Papadopoulos, Siyu An, Vipul Koul.
Patch set 13:Code-Review +1
Attention is currently required from: Siyu An, Vipul Koul.
Patch set 13:Code-Review +1
To view, visit change 4865585. To unsubscribe, or for help writing mail filters, visit settings.
Attention is currently required from: Vipul Koul.
Patch set 13:Code-Review +1
To view, visit change 4865585. To unsubscribe, or for help writing mail filters, visit settings.
Patch set 14:Commit-Queue +2
Chromium LUCI CQ submitted this change.
13 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
[CVV Storage][Settings] Adding cvv field to credit card object & methods
Design doc- go/pay-autofill-cvv-storage-chrome
UX mocks- go/autofill-cvv-storage-ux
Bug: 1464441
Change-Id: Id914887ec02ff7117385bea69d728af184c6e412
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4865585
Commit-Queue: Vipul Koul <koul...@google.com>
Reviewed-by: Siyu An <si...@chromium.org>
Reviewed-by: Giovanni Ortuno Urquidi <ort...@chromium.org>
Reviewed-by: Dominic Battre <bat...@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpa...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1213917}
---
M chrome/browser/extensions/api/autofill_private/autofill_private_api.cc
M chrome/browser/extensions/api/autofill_private/autofill_util.cc
M chrome/common/extensions/api/autofill_private.idl
M chrome/test/data/extensions/api_test/autofill_private/main.html
M chrome/test/data/extensions/api_test/autofill_private/test.js
M tools/typescript/definitions/autofill_private.d.ts
6 files changed, 34 insertions(+), 6 deletions(-)