Request server public keys to generate secret token.
<rdar://problem/74462955>
Created attachment 420885 [details] Patch
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.
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.
(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.
Created attachment 420930 [details] Patch for landing
Committed r273133: <https://commits.webkit.org/r273133> All reviewed patches have been landed. Closing bug and clearing flags on attachment 420930 [details].