Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11773037:
Implementation of sensitive card information escrowing (Closed)
|
Created:
7 years, 11 months ago by ahutter Modified:
7 years, 11 months ago CC:
chromium-reviews, Raman Kakilate, benquan, dhollowa+watch_chromium.org, browser-components-watch_chromium.org, dbeam+watch-autofill_chromium.org, Dane Wallinga, dyu1, estade+watch_chromium.org, Albert Bodenhamer, Ilya Sherman Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplementation of sensitive card information escrowing
BUG=168818
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=176142
Patch Set 1 #
Total comments: 5
Patch Set 2 : Fixing unit test and changing escrow url #
Total comments: 31
Patch Set 3 : Fixes from code review #Patch Set 4 : Fixing Ilya's final comments #
Messages
Total messages: 16 (0 generated)
bug ID is not specified. https://codereview.chromium.org/11773037/diff/1/chrome/browser/autofill/walle... File chrome/browser/autofill/wallet/wallet_client.cc (right): https://codereview.chromium.org/11773037/diff/1/chrome/browser/autofill/walle... chrome/browser/autofill/wallet/wallet_client.cc:99: MakeWalletRequest(GetEscrowUrl(), post_body, observer, kApplicationMimeType); just being cautious: are these http get requests logged somewhere on chrome ? shouldn't these params be post params ? https://codereview.chromium.org/11773037/diff/1/chrome/browser/autofill/walle... File chrome/browser/autofill/wallet/wallet_service_url.cc (right): https://codereview.chromium.org/11773037/diff/1/chrome/browser/autofill/walle... chrome/browser/autofill/wallet/wallet_service_url.cc:37: GURL(kDefaultWalletSecureServiceUrl); make const GURL ? instead of building one for every call ?
Dane, could you take a look at the URL used for card escrowing. I think it's probably incorrect but I'm not sure how all the SUS stuff ties together.
https://codereview.chromium.org/11773037/diff/1/chrome/browser/autofill/walle... File chrome/browser/autofill/wallet/wallet_client.cc (right): https://codereview.chromium.org/11773037/diff/1/chrome/browser/autofill/walle... chrome/browser/autofill/wallet/wallet_client.cc:99: MakeWalletRequest(GetEscrowUrl(), post_body, observer, kApplicationMimeType); On 2013/01/08 02:10:28, Raman Kakilate wrote: > just being cautious: are these http get requests logged somewhere on chrome ? > shouldn't these params be post params ? They are post params but I'm not entirely sure they aren't logged. I'll have to check with someone else. https://codereview.chromium.org/11773037/diff/1/chrome/browser/autofill/walle... File chrome/browser/autofill/wallet/wallet_service_url.cc (right): https://codereview.chromium.org/11773037/diff/1/chrome/browser/autofill/walle... chrome/browser/autofill/wallet/wallet_service_url.cc:37: GURL(kDefaultWalletSecureServiceUrl); On 2013/01/08 02:10:28, Raman Kakilate wrote: > make const GURL ? instead of building one for every call ? ack.
lgtm https://codereview.chromium.org/11773037/diff/1/chrome/browser/autofill/walle... File chrome/browser/autofill/wallet/wallet_client.cc (right): https://codereview.chromium.org/11773037/diff/1/chrome/browser/autofill/walle... chrome/browser/autofill/wallet/wallet_client.cc:99: MakeWalletRequest(GetEscrowUrl(), post_body, observer, kApplicationMimeType); On 2013/01/09 17:31:18, ahutter wrote: > On 2013/01/08 02:10:28, Raman Kakilate wrote: > > just being cautious: are these http get requests logged somewhere on chrome ? > > shouldn't these params be post params ? > > They are post params but I'm not entirely sure they aren't logged. I'll have to > check with someone else. > Ah. ok. "&" in the format confused me. Please check before submission. https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... File chrome/browser/autofill/wallet/wallet_service_url.cc (right): https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet/wallet_service_url.cc:53: return GetBaseAutocheckoutUrl().Resolve("getWalletItemsJwtless"); Can you DCHECK if the resolved GURL is valid. Here and other places ?
Note: When adding reviewers, please include something like "Albert and Ilya, PTAL" in the message body. Without that, the sent message is literally blank, so I just have to infer that a blank message means I should check to see whether the list of reviewers has changed to include me. This is different from the first message that you send out, where the CL context, including the diff and list of reviewers, is automatically included by the review tool, and you can leave the message body blank. (Yes, it would be nice if the tool would just do the right thing on its own.) https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... File chrome/browser/autofill/wallet/wallet_client.cc (right): https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet/wallet_client.cc:90: DCHECK_EQ(request_type_, NO_PENDING_REQUEST); nit: Swap the argument order. To see why, try changing NO_PENDING_REQUEST to something else, and see what the error message looks like. For whatever reason DCHECK_EQ, expects the reverse argument order than what feels natural (to me, at least) :/ https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet/wallet_client.cc:91: Optional nit: Omit this empty line IMO https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet/wallet_client.cc:271: } nit: No need for curly braces. https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... File chrome/browser/autofill/wallet/wallet_client.h (right): https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet/wallet_client.h:45: virtual void OnEscrowSensitiveInformation( nit: Perhaps OnDidEscrowSensitiveInformation or OnReceivedEscrowSensitiveInformation? https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet/wallet_client.h:96: // Google Wallet. Why not just spell out primary_account_number and card_verification_number for the variable names? The fact that you're having to expand the acronyms in the comment suggests that they are not clear enough to stand on their own. https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... File chrome/browser/autofill/wallet/wallet_client_unittest.cc (right): https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet/wallet_client_unittest.cc:312: DCHECK(fetcher); Unit tests should not use DCHECKs, as they can crash the test binary. Use an ASSERT_ instead. https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet/wallet_client_unittest.cc:313: ASSERT_EQ(kEscrowSensitiveInformationRequest, fetcher->upload_data()); nit: I think Chrome code prefers EXPECT_EQ for this sort of check. https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... File chrome/browser/autofill/wallet/wallet_service_url.cc (right): https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet/wallet_service_url.cc:18: "https://wallet.google.com/"; This constant is now identical to the one above it. Why do you need both fo them? https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet/wallet_service_url.cc:22: std::string base_wallet_service_host = nit: What is a "base host"? Probably just name this variable "wallet_service_hostname" if you don't like "url" in this variable name. https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet/wallet_service_url.cc:28: GURL GetPrefixedWalletUrl() { Optional nit: Perhaps name this function "GetBaseWalletUrl", and the other function "GetWalletHostUrl"? https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet/wallet_service_url.cc:53: return GetBaseAutocheckoutUrl().Resolve("getWalletItemsJwtless"); On 2013/01/09 22:54:21, Raman Kakilate wrote: > Can you DCHECK if the resolved GURL is valid. Here and other places ? What would cause it to be invalid? https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet/wallet_service_url.cc:77: return GetBaseWalletUrl().Resolve("online-secure/temporarydata/cvv?s7e=cvv"); I'm confused. Should this not be GetBaseSecureUrl?
https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... File chrome/browser/autofill/wallet/wallet_client.h (right): https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet/wallet_client.h:45: virtual void OnEscrowSensitiveInformation( There needs to be a bit of info here or elsewhere (maybe I'm missing it) about what it means to "Escrow Sensitive Information" https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... File chrome/browser/autofill/wallet/wallet_service_url.cc (right): https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet/wallet_service_url.cc:53: return GetBaseAutocheckoutUrl().Resolve("getWalletItemsJwtless"); On 2013/01/09 22:54:21, Raman Kakilate wrote: > Can you DCHECK if the resolved GURL is valid. Here and other places ? You could get a bad URL if you passed in a bad command line switch. That should NOT cause a DCHECK. DLOG would be fine, if you think it will be a problem.
https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... File chrome/browser/autofill/wallet/wallet_client.cc (right): https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet/wallet_client.cc:90: DCHECK_EQ(request_type_, NO_PENDING_REQUEST); On 2013/01/09 23:30:48, Ilya Sherman wrote: > nit: Swap the argument order. To see why, try changing NO_PENDING_REQUEST to > something else, and see what the error message looks like. For whatever reason > DCHECK_EQ, expects the reverse argument order than what feels natural (to me, at > least) :/ Done. https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet/wallet_client.cc:91: On 2013/01/09 23:30:48, Ilya Sherman wrote: > Optional nit: Omit this empty line IMO Done. https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet/wallet_client.cc:271: } On 2013/01/09 23:30:48, Ilya Sherman wrote: > nit: No need for curly braces. They're there for consistency but I can drop them if you'd like. https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... File chrome/browser/autofill/wallet/wallet_client.h (right): https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet/wallet_client.h:45: virtual void OnEscrowSensitiveInformation( On 2013/01/09 23:30:48, Ilya Sherman wrote: > nit: Perhaps OnDidEscrowSensitiveInformation or > OnReceivedEscrowSensitiveInformation? Changed to OnDidEscrowSensitiveInformation. Should the rest of the callback methods be named like that? If so, I will do it in a separate CL. https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet/wallet_client.h:45: virtual void OnEscrowSensitiveInformation( On 2013/01/09 23:58:43, Albert Bodenhamer wrote: > There needs to be a bit of info here or elsewhere (maybe I'm missing it) about > what it means to "Escrow Sensitive Information" Added a comment. https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet/wallet_client.h:96: // Google Wallet. On 2013/01/09 23:30:48, Ilya Sherman wrote: > Why not just spell out primary_account_number and card_verification_number for > the variable names? The fact that you're having to expand the acronyms in the > comment suggests that they are not clear enough to stand on their own. Done. https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... File chrome/browser/autofill/wallet/wallet_client_unittest.cc (right): https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet/wallet_client_unittest.cc:312: DCHECK(fetcher); On 2013/01/09 23:30:48, Ilya Sherman wrote: > Unit tests should not use DCHECKs, as they can crash the test binary. Use an > ASSERT_ instead. Done. https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet/wallet_client_unittest.cc:313: ASSERT_EQ(kEscrowSensitiveInformationRequest, fetcher->upload_data()); On 2013/01/09 23:30:48, Ilya Sherman wrote: > nit: I think Chrome code prefers EXPECT_EQ for this sort of check. Done. https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... File chrome/browser/autofill/wallet/wallet_service_url.cc (right): https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet/wallet_service_url.cc:18: "https://wallet.google.com/"; On 2013/01/09 23:30:48, Ilya Sherman wrote: > This constant is now identical to the one above it. Why do you need both fo > them? In development they can be different. https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet/wallet_service_url.cc:22: std::string base_wallet_service_host = On 2013/01/09 23:30:48, Ilya Sherman wrote: > nit: What is a "base host"? Probably just name this variable > "wallet_service_hostname" if you don't like "url" in this variable name. Done. https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet/wallet_service_url.cc:28: GURL GetPrefixedWalletUrl() { On 2013/01/09 23:30:48, Ilya Sherman wrote: > Optional nit: Perhaps name this function "GetBaseWalletUrl", and the other > function "GetWalletHostUrl"? Done. https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet/wallet_service_url.cc:53: return GetBaseAutocheckoutUrl().Resolve("getWalletItemsJwtless"); On 2013/01/09 23:58:43, Albert Bodenhamer wrote: > On 2013/01/09 22:54:21, Raman Kakilate wrote: > > Can you DCHECK if the resolved GURL is valid. Here and other places ? > > You could get a bad URL if you passed in a bad command line switch. That should > NOT cause a DCHECK. DLOG would be fine, if you think it will be a problem. I don't think this is something we need to worry about too much. If the command line switch is bad all requests will 404 and be pretty obvious. https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet/wallet_service_url.cc:77: return GetBaseWalletUrl().Resolve("online-secure/temporarydata/cvv?s7e=cvv"); On 2013/01/09 23:30:48, Ilya Sherman wrote: > I'm confused. Should this not be GetBaseSecureUrl? OTP encryption actually does go through Sugar servers. It was my bad before for thinking it didn't. I updated the comment in chrome_switches.cc to reflect this.
LGTM, thanks https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... File chrome/browser/autofill/wallet/wallet_client.cc (right): https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet/wallet_client.cc:271: } On 2013/01/10 00:24:46, ahutter wrote: > On 2013/01/09 23:30:48, Ilya Sherman wrote: > > nit: No need for curly braces. > > They're there for consistency but I can drop them if you'd like. I don't think they're needed for any of the cases. Even when they are, the style I've seen in other Chrome files is to only include them on specific cases that need 'em. https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... File chrome/browser/autofill/wallet/wallet_client.h (right): https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet/wallet_client.h:45: virtual void OnEscrowSensitiveInformation( On 2013/01/10 00:24:46, ahutter wrote: > On 2013/01/09 23:30:48, Ilya Sherman wrote: > > nit: Perhaps OnDidEscrowSensitiveInformation or > > OnReceivedEscrowSensitiveInformation? > > Changed to OnDidEscrowSensitiveInformation. Should the rest of the callback > methods be named like that? If so, I will do it in a separate CL. Yes, that would be helpful. Thanks. https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... File chrome/browser/autofill/wallet/wallet_service_url.cc (right): https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... chrome/browser/autofill/wallet/wallet_service_url.cc:18: "https://wallet.google.com/"; On 2013/01/10 00:24:46, ahutter wrote: > On 2013/01/09 23:30:48, Ilya Sherman wrote: > > This constant is now identical to the one above it. Why do you need both fo > > them? > > In development they can be different. That sounds like reason to keep both the switches, but not both the constants.
On 2013/01/10 00:35:40, Ilya Sherman wrote: > LGTM, thanks > > https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... > File chrome/browser/autofill/wallet/wallet_client.cc (right): > > https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... > chrome/browser/autofill/wallet/wallet_client.cc:271: } > On 2013/01/10 00:24:46, ahutter wrote: > > On 2013/01/09 23:30:48, Ilya Sherman wrote: > > > nit: No need for curly braces. > > > > They're there for consistency but I can drop them if you'd like. > > I don't think they're needed for any of the cases. Even when they are, the > style I've seen in other Chrome files is to only include them on specific cases > that need 'em. > Removed. > https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... > File chrome/browser/autofill/wallet/wallet_client.h (right): > > https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... > chrome/browser/autofill/wallet/wallet_client.h:45: virtual void > OnEscrowSensitiveInformation( > On 2013/01/10 00:24:46, ahutter wrote: > > On 2013/01/09 23:30:48, Ilya Sherman wrote: > > > nit: Perhaps OnDidEscrowSensitiveInformation or > > > OnReceivedEscrowSensitiveInformation? > > > > Changed to OnDidEscrowSensitiveInformation. Should the rest of the callback > > methods be named like that? If so, I will do it in a separate CL. > > Yes, that would be helpful. Thanks. Now crbug.com/169146 > > https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... > File chrome/browser/autofill/wallet/wallet_service_url.cc (right): > > https://codereview.chromium.org/11773037/diff/6001/chrome/browser/autofill/wa... > chrome/browser/autofill/wallet/wallet_service_url.cc:18: > "https://wallet.google.com/"; > On 2013/01/10 00:24:46, ahutter wrote: > > On 2013/01/09 23:30:48, Ilya Sherman wrote: > > > This constant is now identical to the one above it. Why do you need both fo > > > them? > > > > In development they can be different. > > That sounds like reason to keep both the switches, but not both the constants. Totally didn't read that comment correctly. Removed the secure one.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ahutter@chromium.org/11773037/16001
Retried try job too often on ios_dbg_simulator for step(s) sync_unit_tests, unit_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ahutter@chromium.org/11773037/16001
Message was sent while issue was closed.
Change committed as 176142 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
