Bug 207169 – [Apple Pay] Provide a redacted billing contact during payment method selection

• Component: New Bugs
• Status: RESOLVED FIXED

[Nikos Mouchtaris]

Feature Request: billingContact - return zip code prior to user auth

[Nikos Mouchtaris]

Created attachment 389609 [details]
Patch

[Nikos Mouchtaris]

Created attachment 389683 [details]
Patch

[Andy Estes]

Comment on attachment 389683 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=389683&action=review

> Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:64
> +ENABLE_APPLE_PAY_SESSION_V9_iphoneos[sdk=iphone*13.*] = ;

APPLE_PAY_SESSION_V9 should be enabled on iOS 13. Since we don't support any iOSes less than 13 in trunk, you can remove this.

> Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:66
> +ENABLE_APPLE_PAY_SESSION_V9_iphonesimulator[sdk=iphone*13.*] = ;

Ditto.

> Source/WTF/wtf/PlatformHave.h:104
> +#if !defined(HAVE_PASSKIT_PAYMENT_METHOD_BILLING_ADDRESS) && ((PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 130000) \

No need to check __IPHONE_OS_VERSION_MIN_REQUIRED in this case.

This would read more easily if you separated the !defined(HAVE_PASSKIT_PAYMENT_METHOD_BILLING_ADDRESS) check and the PLATFORM() checks into two #if blocks.

> Source/WebCore/Modules/applepay/ApplePayPaymentMethod.h:58
> +#define ALLOW_COUNTRY 1
> +#define ALLOW_COUNTRY_CODE 1
> +#define ALLOW_ADMIN_AREA 1
> +#define ALLOW_POSTAL_CODE 1
> +// #define ALLOW_SUB_ADMIN_AREA 1
> +// #define ALLOW_ADDRESS_LINES 1
> +// #define ALLOW_SUB_LOCALITY 1
> +// #define ALLOW_LOCALITY 1

These can move to PaymentMethodCocoa.mm since that's the only place they're used. Also, we don't want to commit commented-out code. You can either define the commented-out defines to 0 or remove them.

After thinking more about it, we probably want to redact postal code for now. In some cases the postal code is a ZIP+4, which can designate a specific location.

> Source/WebCore/Modules/applepay/cocoa/PaymentMethodCocoa.mm:97
> +#ifdef HAVE_PASSKIT_PAYMENT_METHOD_BILLING_ADDRESS

#if HAVE(PASSKIT_PAYMENT_METHOD_BILLING_ADDRESS)

> Source/WebCore/Modules/applepay/cocoa/PaymentMethodCocoa.mm:110
> +    if (NSString *city = postalAddress.value.city)
> +        addressLine.append(city);
> +    if (NSString *state = postalAddress.value.state)
> +        addressLine.append(state);

addressLine only represents the street address. City and state are stored elsewhere.

> Source/WebCore/Modules/applepay/cocoa/PaymentMethodCocoa.mm:125
> +    if (NSArray<CNLabeledValue<CNPhoneNumber*>*> *phoneNumbers = billingContact.phoneNumbers) {
> +        for (CNLabeledValue<CNPhoneNumber*> *phoneNumber in phoneNumbers)
> +            result.phoneNumber = phoneNumber.value.stringValue;
> +    }

Why did you choose to convert the last phone number in the contact? Which does PassKit choose when converting CNContacts to PKContacts?

> Source/WebCore/Modules/applepay/cocoa/PaymentMethodCocoa.mm:130
> +    if (NSArray<CNLabeledValue<NSString*>*> *emailAddresses = billingContact.emailAddresses) {
> +        for (CNLabeledValue<NSString*> *emailAddress in emailAddresses)
> +            result.emailAddress = emailAddress.value;
> +    }

Ditto.

> Source/WebCore/Modules/applepay/cocoa/PaymentMethodCocoa.mm:142
> +        for (CNLabeledValue<CNPostalAddress*> *postalAddress in postalAddresses) {

Ditto.

> Source/WebCore/Modules/applepay/cocoa/PaymentMethodCocoa.mm:143
> +#ifdef ALLOW_ADDRESS_LINES

This just checks if ALLOW_ADDRESS_LINES is defined, but you probably want to check if it's defined to 1. Maybe you could introduce an ALLOW() function macro in this file along the lines of ENABLE(), HAVE(), etc.

> Source/WebCore/Modules/applepay/cocoa/PaymentMethodCocoa.mm:189
> +#ifdef HAVE_PASSKIT_PAYMENT_METHOD_BILLING_ADDRESS

#if HAVE(PASSKIT_PAYMENT_METHOD_BILLING_ADDRESS)

> LayoutTests/http/tests/ssl/applepay/ApplePayBillingAddress.html:105
> +                    debug(event.methodDetails.billingAddress.phoneNumber);
> +                    debug(event.methodDetails.billingAddress.emailAddress);
> +                    debug(event.methodDetails.billingAddress.givenName);
> +                    debug(event.methodDetails.billingAddress.familyName);
> +                    debug(event.methodDetails.billingAddress.phoneticGivenName);
> +                    debug(event.methodDetails.billingAddress.phoneticFamilyName);
> +                    debug(event.methodDetails.billingAddress.addressLines);
> +                    debug(event.methodDetails.billingAddress.subLocality);
> +                    debug(event.methodDetails.billingAddress.locality);
> +                    debug(event.methodDetails.billingAddress.postalCode);
> +                    debug(event.methodDetails.billingAddress.subAdministrativeArea);
> +                    debug(event.methodDetails.billingAddress.country);
> +                    debug(event.methodDetails.billingAddress.countryCode);
> +                    debug(event.methodDetails.network);
> +                    debug(event.methodDetails.type);

I'd use shouldBe() instead of debug() for these.

[Nikos Mouchtaris]

Created attachment 389726 [details]
Patch

[Andy Estes]

Comment on attachment 389726 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=389726&action=review

> Source/WTF/wtf/PlatformHave.h:105
> +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101500)

No need for the ()

> Source/WebCore/Modules/applepay/ApplePayPaymentMethod.h:48
> +    Optional<ApplePayPaymentContact> billingAddress;

I know this is called `billingAddress` in PKPaymentMethod, but let's call it `billingContact` here.

> Source/WebCore/Modules/applepay/ApplePayPaymentMethod.idl:35
> +    ApplePayPaymentContact billingAddress;

Ditto.

> Source/WebCore/Modules/applepay/cocoa/PaymentMethodCocoa.mm:54
> +#define ALLOW(FIELD) (ALLOW_##FIELD == 1)

Please #undef this at the end of the file.

> Source/WebCore/Modules/applepay/cocoa/PaymentMethodCocoa.mm:134
> +    if (NSArray<CNLabeledValue<CNPhoneNumber*>*> *phoneNumbers = billingContact.phoneNumbers) {
> +        if (CNLabeledValue<CNPhoneNumber*> *phoneNumber = [phoneNumbers firstObject])
> +            result.phoneNumber = phoneNumber.value.stringValue;
> +    }

You're probably basing this on the other convert() functions in this file, but I think we can be more concise here. These nil checks are unnecessary, because in Objective-C you can safely send a message to nil and get a return value of 0/nil. 0/nil converted to a WTF::String is the same as WTF::String(), which is the value result.phoneNumber would have if you hadn't set it.

So this is the same as `result.phoneNumber = billingContact.phoneNumbers.firstObject.value.stringValue`.

That's a lot for one statement, though, so maybe I'd break it up like this:

    if (auto firstPhoneNumber = billingContact.phoneNumbers.firstObject)
        result.phoneNumber = firstPhoneNumber.value.stringValue;

> Source/WebCore/Modules/applepay/cocoa/PaymentMethodCocoa.mm:139
> +    if (NSArray<CNLabeledValue<NSString*>*> *emailAddresses = billingContact.emailAddresses) {
> +        if (CNLabeledValue<NSString*> *emailAddress = [emailAddresses firstObject])
> +            result.emailAddress = emailAddress.value;
> +    }

Ditto.

> Source/WebCore/Modules/applepay/cocoa/PaymentMethodCocoa.mm:142
> +    if (NSString *givenName = billingContact.givenName)
> +        result.givenName = givenName;

`result.givenName = billingContact.givenName`

> Source/WebCore/Modules/applepay/cocoa/PaymentMethodCocoa.mm:149
> +    if (NSString *familyName = billingContact.familyName)
> +        result.familyName = familyName;
> +    
> +    if (NSString *phoneticGivenName = billingContact.phoneticGivenName)
> +        result.phoneticGivenName = phoneticGivenName;
> +    if (NSString *phoneticFamilyName = billingContact.phoneticFamilyName)
> +        result.phoneticFamilyName = phoneticFamilyName;

Ditto.

> Source/WebCore/Modules/applepay/cocoa/PaymentMethodCocoa.mm:182
> +#if ALLOW(SUB_LOCALITY)
> +            if (NSString *subLocality = postalAddress.value.subLocality)
> +                result.subLocality = subLocality;
> +#endif
> +#if ALLOW(LOCALITY)
> +            if (NSString *locality = postalAddress.value.city)
> +                result.locality = locality;
> +#endif
> +#if ALLOW(SUB_ADMIN_AREA)
> +            if (NSString *subAdministrativeArea = postalAddress.value.subAdministrativeArea)
> +                result.subAdministrativeArea = subAdministrativeArea;
> +#endif
> +#if ALLOW(ADMIN_AREA)
> +            if (NSString *administrativeArea = postalAddress.value.state)
> +                result.administrativeArea = administrativeArea;
> +#endif
> +#if ALLOW(POSTAL_CODE)
> +            if (NSString *postalCode = postalAddress.value.postalCode)
> +                result.postalCode = postalCode;
> +#endif
> +#if ALLOW(COUNTRY)
> +            if (NSString *country = postalAddress.value.country)
> +                result.country = country;
> +#endif
> +#if ALLOW(COUNTRY_CODE)
> +            if (NSString *ISOCountryCode = postalAddress.value.ISOCountryCode)
> +                result.countryCode = ISOCountryCode;
> +#endif

I think these should move into the convert function for CNPostalAddress.

> LayoutTests/http/tests/ssl/applepay/ApplePayBillingAddress.html:105
> +                    shouldBe("event.methodDetails.billingAddress.phoneNumber", "'12345678910'");
> +                    shouldBe("event.methodDetails.billingAddress.emailAddress", "'[e-mail]'");
> +                    shouldBe("event.methodDetails.billingAddress.givenName", "'Web'");
> +                    shouldBe("event.methodDetails.billingAddress.familyName", "'Test'");
> +                    shouldBe("event.methodDetails.billingAddress.phoneticGivenName", "'Web'");
> +                    shouldBe("event.methodDetails.billingAddress.phoneticFamilyName", "'Test'");
> +                    shouldBe("event.methodDetails.billingAddress.addressLines", "['1 wpt street']");
> +                    shouldBe("event.methodDetails.billingAddress.subLocality", "'AA'");
> +                    shouldBe("event.methodDetails.billingAddress.locality", "'BB'");
> +                    shouldBe("event.methodDetails.billingAddress.postalCode", "'12345'");
> +                    shouldBe("event.methodDetails.billingAddress.subAdministrativeArea", "'CC'");
> +                    shouldBe("event.methodDetails.billingAddress.country", "'United States'");
> +                    shouldBe("event.methodDetails.billingAddress.countryCode", "'US'");
> +                    shouldBe("event.methodDetails.network","'visa'");
> +                    shouldBe("event.methodDetails.type","'credit'");

You can use the variables defined on lines 48-60 as the expected values here.

[Nikos Mouchtaris]

Created attachment 389864 [details]
Patch

[Andy Estes]

Comment on attachment 389864 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=389864&action=review

> Source/WebCore/ChangeLog:3
> +        Feature Request: billingContact - return zip code prior to user auth

I think this bug (and the associated radar) could use a better title. Perhaps something like "[Apple Pay] Provide a redacted billing contact during payment method selection".

> Source/WebCore/Modules/applepay/cocoa/PaymentMethodCocoa.mm:117
> +        result.addressLine = addressLine;

result.addressLine = WTFMove(addressLine)

> Source/WebCore/Modules/applepay/cocoa/PaymentMethodCocoa.mm:122
> +    if (NSString *subLocality = postalAddress.value.subLocality)
> +        result.subLocality = subLocality;

No need for the if statement. I'd just do `result.subLocality = postalAddress.value.subLocality`.

> Source/WebCore/Modules/applepay/cocoa/PaymentMethodCocoa.mm:146
> +    if (NSString *locality = postalAddress.value.city)
> +        result.locality = locality;
> +#endif
> +#if ALLOW(SUB_ADMIN_AREA)
> +    if (NSString *subAdministrativeArea = postalAddress.value.subAdministrativeArea)
> +        result.subAdministrativeArea = subAdministrativeArea;
> +#endif
> +#if ALLOW(ADMIN_AREA)
> +    if (NSString *administrativeArea = postalAddress.value.state)
> +        result.administrativeArea = administrativeArea;
> +#endif
> +#if ALLOW(POSTAL_CODE)
> +    if (NSString *postalCode = postalAddress.value.postalCode)
> +        result.postalCode = postalCode;
> +#endif
> +#if ALLOW(COUNTRY)
> +    if (NSString *country = postalAddress.value.country)
> +        result.country = country;
> +#endif
> +#if ALLOW(COUNTRY_CODE)
> +    if (NSString *ISOCountryCode = postalAddress.value.ISOCountryCode)
> +        result.countryCode = ISOCountryCode;

Ditto for each of these assignments.

> Source/WebCore/Modules/applepay/cocoa/PaymentMethodCocoa.mm:171
> +    if (NSString *givenName = billingContact.givenName)
> +        result.givenName = billingContact.givenName;
> +    if (NSString *familyName = billingContact.familyName)
> +        result.familyName = billingContact.familyName;
> +    
> +    if (NSString *phoneticGivenName = billingContact.phoneticGivenName)
> +        result.phoneticGivenName = phoneticGivenName;
> +    if (NSString *phoneticFamilyName = billingContact.phoneticFamilyName)
> +        result.phoneticFamilyName = phoneticFamilyName;

And these.

[Nikos Mouchtaris]

Created attachment 390003 [details]
Patch

[EWS]

Comment on attachment 390003 [details]
Patch

Rejecting attachment 390003 [details] from commit-queue.

[e-mail] does not have committer permissions according to https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Commit Bot]

Comment on attachment 390003 [details]
Patch

Clearing flags on attachment: 390003

Committed r256041: <https://trac.webkit.org/changeset/256041>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/59265585>

[Truitt Savell]

It looks like the changes in https://trac.webkit.org/changeset/256041/webkit

broke the Catalina build: https://build.webkit.org/builders/Apple-Catalina-Release-Build/builds/3270

error:
./Modules/applepay/cocoa/PaymentMethodCocoa.mm:178:51: error: property 'billingAddress' not found on object of type 'PKPaymentMethod *'

[Nikos Mouchtaris]

Reopening to attach new patch.

[Nikos Mouchtaris]

Created attachment 390130 [details]
Patch

[Nikos Mouchtaris]

Created attachment 390131 [details]
Patch

[Ryan Haddad]

Comment on attachment 390131 [details]
Patch

Clearing flags on attachment: 390131

Committed r256071: <https://trac.webkit.org/changeset/256071>

[Ryan Haddad]

All reviewed patches have been landed.  Closing bug.