Bug 222141 – PCM: Request server public key to generate secret token

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Jiewen Tan]

Request server public keys to generate secret token.

[Jiewen Tan]

<rdar://problem/74462955>

[Jiewen Tan]

Created attachment 420885 [details]
Patch

[John Wilander]

Comment on attachment 420885 [details]
Patch

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

r=me with comments.

> Source/WebCore/ChangeLog:3
> +        PCM: Request server public keys to generate secret token

Nit: key, not keys

> Source/WebKit/ChangeLog:28
> +        Teaches the PCMM to send the token public key request.

Nit: PCM, not PCMM.

> Source/WebCore/loader/PrivateClickMeasurement.cpp:41
> +static const char privateClickMeasurementTokenPublicKeyPath[] = "/.well-known/private-click-measurement/unlinkable-token-public-key/";

We should formulate this as an action, so /get-unlinkable-token-public-key/.

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:95
> +    request.setHTTPMethod(httpMethod);

Nice.

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:119
> +static NetworkResourceLoadParameters generateNetworkResourceLoadParametersForPost(URL&& url, Ref<JSON::Object>&& jsonPayload, PrivateClickMeasurement::PcmDataCarried dataTypeCarried)

I would add HTTP, so generateNetworkResourceLoadParametersForHTTPPost.

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:124
> +static NetworkResourceLoadParameters generateNetworkResourceLoadParametersForGet(URL&& url, PrivateClickMeasurement::PcmDataCarried dataTypeCarried)

I would add HTTP, so generateNetworkResourceLoadParametersForHTTPGet.

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:129
> +void PrivateClickMeasurementManager::getTokenPublicKey(PrivateClickMeasurement&& attribution)

I wonder if other WebKit developers will bark at this use of "get" as the leading verb? Maybe we should use the word "retrieve" instead just to avoid the potential complaint? In my personal view it *is* good use of the word "get" since we are getting things from the server. Naming it "fetch" instead doesn't help since it would lead people to think that this is tied to Fetch.

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:135
> +    auto pcmDataCarried = PrivateClickMeasurement::PcmDataCarried::PersonallyIdentifiable;

I would add a comment here saying "This is guaranteed to be close in time to the navigational click which makes it likely to be personally identifiable."

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:159
> +        getSignedUnlinkableToken(attribution);

WTFMove(attribution)?

> Tools/ChangeLog:3
> +        PCM: Request server public keys to generate secret token

Nit: key

> LayoutTests/ChangeLog:14
> +        * http/tests/privateClickMeasurement/store-private-click-measurement-with-source-nonce.html:

Please explain briefly what changed in this test case.

> LayoutTests/http/tests/privateClickMeasurement/resources/signToken.php:4
> +$tokenSigningFile = fopen($tokenSigningFilePath . ".tmp", 'a');

So this will open the file twice and only after the second append rename it so that the polling finds it? Trying to understand the logic. We should add a comment that explains this. Another way of doing it is having a dedicated PHP file for fetching the singing key.

[Jiewen Tan]

Comment on attachment 420885 [details]
Patch

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

Thanks John for r+ this patch.

>> Source/WebCore/ChangeLog:3
>> +        PCM: Request server public keys to generate secret token
> 
> Nit: key, not keys

Will fix.

>> Source/WebKit/ChangeLog:28
>> +        Teaches the PCMM to send the token public key request.
> 
> Nit: PCM, not PCMM.

It actually refers to PCM manager.

>> Source/WebCore/loader/PrivateClickMeasurement.cpp:41
>> +static const char privateClickMeasurementTokenPublicKeyPath[] = "/.well-known/private-click-measurement/unlinkable-token-public-key/";
> 
> We should formulate this as an action, so /get-unlinkable-token-public-key/.

Sure.

>> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:119
>> +static NetworkResourceLoadParameters generateNetworkResourceLoadParametersForPost(URL&& url, Ref<JSON::Object>&& jsonPayload, PrivateClickMeasurement::PcmDataCarried dataTypeCarried)
> 
> I would add HTTP, so generateNetworkResourceLoadParametersForHTTPPost.

Sure.

>> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:124
>> +static NetworkResourceLoadParameters generateNetworkResourceLoadParametersForGet(URL&& url, PrivateClickMeasurement::PcmDataCarried dataTypeCarried)
> 
> I would add HTTP, so generateNetworkResourceLoadParametersForHTTPGet.

Sure.

>> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:129
>> +void PrivateClickMeasurementManager::getTokenPublicKey(PrivateClickMeasurement&& attribution)
> 
> I wonder if other WebKit developers will bark at this use of "get" as the leading verb? Maybe we should use the word "retrieve" instead just to avoid the potential complaint? In my personal view it *is* good use of the word "get" since we are getting things from the server. Naming it "fetch" instead doesn't help since it would lead people to think that this is tied to Fetch.

I think get is good.

>> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:135
>> +    auto pcmDataCarried = PrivateClickMeasurement::PcmDataCarried::PersonallyIdentifiable;
> 
> I would add a comment here saying "This is guaranteed to be close in time to the navigational click which makes it likely to be personally identifiable."

Sure.

>> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:159
>> +        getSignedUnlinkableToken(attribution);
> 
> WTFMove(attribution)?

This one accepts a const &. Let me modify that.

>> Tools/ChangeLog:3
>> +        PCM: Request server public keys to generate secret token
> 
> Nit: key

Will fix.

>> LayoutTests/ChangeLog:14
>> +        * http/tests/privateClickMeasurement/store-private-click-measurement-with-source-nonce.html:
> 
> Please explain briefly what changed in this test case.

Sure.

>> LayoutTests/http/tests/privateClickMeasurement/resources/signToken.php:4
>> +$tokenSigningFile = fopen($tokenSigningFilePath . ".tmp", 'a');
> 
> So this will open the file twice and only after the second append rename it so that the polling finds it? Trying to understand the logic. We should add a comment that explains this. Another way of doing it is having a dedicated PHP file for fetching the singing key.

I can add a comment here. It's not necessary to duplicate that many code here.

[John Wilander]

(In reply to Jiewen Tan from comment #4)
> Comment on attachment 420885 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=420885&action=review
> 
> Thanks John for r+ this patch.
> 
> >> Source/WebCore/ChangeLog:3
> >> +        PCM: Request server public keys to generate secret token
> > 
> > Nit: key, not keys
> 
> Will fix.
> 
> >> Source/WebKit/ChangeLog:28
> >> +        Teaches the PCMM to send the token public key request.
> > 
> > Nit: PCM, not PCMM.
> 
> It actually refers to PCM manager.

Ha, ha! Maybe PCM Mgr? No big deal.

> >> Source/WebCore/loader/PrivateClickMeasurement.cpp:41
> >> +static const char privateClickMeasurementTokenPublicKeyPath[] = "/.well-known/private-click-measurement/unlinkable-token-public-key/";
> > 
> > We should formulate this as an action, so /get-unlinkable-token-public-key/.
> 
> Sure.
> 
> >> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:119
> >> +static NetworkResourceLoadParameters generateNetworkResourceLoadParametersForPost(URL&& url, Ref<JSON::Object>&& jsonPayload, PrivateClickMeasurement::PcmDataCarried dataTypeCarried)
> > 
> > I would add HTTP, so generateNetworkResourceLoadParametersForHTTPPost.
> 
> Sure.
> 
> >> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:124
> >> +static NetworkResourceLoadParameters generateNetworkResourceLoadParametersForGet(URL&& url, PrivateClickMeasurement::PcmDataCarried dataTypeCarried)
> > 
> > I would add HTTP, so generateNetworkResourceLoadParametersForHTTPGet.
> 
> Sure.
> 
> >> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:129
> >> +void PrivateClickMeasurementManager::getTokenPublicKey(PrivateClickMeasurement&& attribution)
> > 
> > I wonder if other WebKit developers will bark at this use of "get" as the leading verb? Maybe we should use the word "retrieve" instead just to avoid the potential complaint? In my personal view it *is* good use of the word "get" since we are getting things from the server. Naming it "fetch" instead doesn't help since it would lead people to think that this is tied to Fetch.
> 
> I think get is good.

OK, let's go with get then.

> >> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:135
> >> +    auto pcmDataCarried = PrivateClickMeasurement::PcmDataCarried::PersonallyIdentifiable;
> > 
> > I would add a comment here saying "This is guaranteed to be close in time to the navigational click which makes it likely to be personally identifiable."
> 
> Sure.
> 
> >> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:159
> >> +        getSignedUnlinkableToken(attribution);
> > 
> > WTFMove(attribution)?
> 
> This one accepts a const &. Let me modify that.
> 
> >> Tools/ChangeLog:3
> >> +        PCM: Request server public keys to generate secret token
> > 
> > Nit: key
> 
> Will fix.
> 
> >> LayoutTests/ChangeLog:14
> >> +        * http/tests/privateClickMeasurement/store-private-click-measurement-with-source-nonce.html:
> > 
> > Please explain briefly what changed in this test case.
> 
> Sure.
> 
> >> LayoutTests/http/tests/privateClickMeasurement/resources/signToken.php:4
> >> +$tokenSigningFile = fopen($tokenSigningFilePath . ".tmp", 'a');
> > 
> > So this will open the file twice and only after the second append rename it so that the polling finds it? Trying to understand the logic. We should add a comment that explains this. Another way of doing it is having a dedicated PHP file for fetching the singing key.
> 
> I can add a comment here. It's not necessary to duplicate that many code
> here.

Sounds good.

[Jiewen Tan]

Created attachment 420930 [details]
Patch for landing

[EWS]

Committed r273133: <https://commits.webkit.org/r273133>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 420930 [details].