PCM: PrivateClickMeasurementManager::getTokenPublicKey() should not use PrivateClickMeasurement::PcmDataCarried::PersonallyIdentifiable when validating the token before the attribution report is sent

[John Wilander]

PrivateClickMeasurementManager::getTokenPublicKey() is currently configured to use PrivateClickMeasurement::PcmDataCarried::PersonallyIdentifiable when minting the unlinkable and secret tokens. However, PrivateClickMeasurementManager::getTokenPublicKey() is used a second time when validating the secret token before the attribution report is sent. On the second occasion, PrivateClickMeasurement::PcmDataCarried::NonPersonallyIdentifiable should be used. Which to use should probably be controlled by the caller.

[Radar WebKit Bug Importer]

<rdar://problem/76086936>

[John Wilander]

Created attachment 424872 [details]
Patch

[John Wilander]

This patch also fixes https://bugs.webkit.org/show_bug.cgi?id=223957.

[Jiewen Tan]

Comment on attachment 424872 [details]
Patch

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

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:143
> +    auto pcmDataCarried = UNLIKELY(debugModeEnabled()) ? PrivateClickMeasurement::PcmDataCarried::PersonallyIdentifiable : dataCarried;

Can we do this within generateNetworkLoadParameters? such that we can handle this logic for all traffics?

[youenn fablet]

Comment on attachment 424872 [details]
Patch

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

>> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:143
>> +    auto pcmDataCarried = UNLIKELY(debugModeEnabled()) ? PrivateClickMeasurement::PcmDataCarried::PersonallyIdentifiable : dataCarried;
> 
> Can we do this within generateNetworkLoadParameters? such that we can handle this logic for all traffics?

Looking in the code, I do not see where is used pcmDataCarried?
Can you detail its use?
Would it make sense to pass pcmDataCarried directly to m_networkLoadFunction and remove it from NetworkLoadParameters.
I do not see it set in WebProcess for instance.

[John Wilander]

(In reply to youenn fablet from comment #5)
> Comment on attachment 424872 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=424872&action=review
> 
> >> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:143
> >> +    auto pcmDataCarried = UNLIKELY(debugModeEnabled()) ? PrivateClickMeasurement::PcmDataCarried::PersonallyIdentifiable : dataCarried;
> > 
> > Can we do this within generateNetworkLoadParameters? such that we can handle this logic for all traffics?
> 
> Looking in the code, I do not see where is used pcmDataCarried?
> Can you detail its use?
> Would it make sense to pass pcmDataCarried directly to m_networkLoadFunction
> and remove it from NetworkLoadParameters.
> I do not see it set in WebProcess for instance.

It's is not in use now and intended for future things. Since PCM makes network requests that are not tied to any page load, we wanted to provide some guidance for the network loader on the privacy guarantees of the requests. That will allow WebKit clients with control over the network stack to make decisions on how to handle those requests. I will also mention this in the update to the spec once we have the whole fraud prevention thing settled.

[John Wilander]

(In reply to Jiewen Tan from comment #4)
> Comment on attachment 424872 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=424872&action=review
> 
> > Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:143
> > +    auto pcmDataCarried = UNLIKELY(debugModeEnabled()) ? PrivateClickMeasurement::PcmDataCarried::PersonallyIdentifiable : dataCarried;
> 
> Can we do this within generateNetworkLoadParameters? such that we can handle
> this logic for all traffics?

Nice idea. Let me see if that's possible.

[John Wilander]

(In reply to John Wilander from comment #7)
> (In reply to Jiewen Tan from comment #4)
> > Comment on attachment 424872 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=424872&action=review
> > 
> > > Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.cpp:143
> > > +    auto pcmDataCarried = UNLIKELY(debugModeEnabled()) ? PrivateClickMeasurement::PcmDataCarried::PersonallyIdentifiable : dataCarried;
> > 
> > Can we do this within generateNetworkLoadParameters? such that we can handle
> > this logic for all traffics?
> 
> Nice idea. Let me see if that's possible.

generateNetworkLoadParameters() is static so we have to send the flag in as a parameter. New patch coming up.

[John Wilander]

Created attachment 424952 [details]
Patch

[John Wilander]

Failing layout tests are unrelated.

[John Wilander]

Comment on attachment 424952 [details]
Patch

Thanks, Youenn and Jiewen!

[EWS]

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

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