[webkit-reviews] review granted: [Bug 222208] PCM: Store and report source unlinkable tokens : [Attachment 421084] Patch

[bugzilla-daemon at webkit.org]

John Wilander <wilander at apple.com> has granted Jiewen Tan
<jiewen_tan at apple.com>'s request for review:
Bug 222208: PCM: Store and report source unlinkable tokens
https://bugs.webkit.org/show_bug.cgi?id=222208

Attachment 421084: Patch

https://bugs.webkit.org/attachment.cgi?id=421084&action=review




--- Comment #11 from John Wilander <wilander at apple.com> ---
Comment on attachment 421084
  --> https://bugs.webkit.org/attachment.cgi?id=421084
Patch

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

LGTM. See comments.

> Source/WTF/ChangeLog:3
> +	   PCM: Turn the fraud prevention on by default

I'm confused. Is this a change log that shouldn't be part of this patch and
really only be part of the patch for
https://bugs.webkit.org/show_bug.cgi?id=222224? I think so.

> Source/WebCore/ChangeLog:12
> +	   Covered by existing tests.

This should say "Existing tests updated."

> Source/WebCore/ChangeLog:16
> +	   Adds for mock testing.

I would flesh this out a bit. "Mocks the [...] for testing."

> Source/WebKit/ChangeLog:28
> +	   Adds some mock test infrastructures.

Here I would add "... so that we can test [...] without [...]"

> Source/WebCore/loader/PrivateClickMeasurement.cpp:220
> +    reportDetails->setString("source_secret_token"_s,
m_sourceSecretToken.valueBase64URL);

We'll have to work through the naming of these things later. I my view, the
secret is kept secret until the attribution report is sent. In the old blinded
signature terminology, this would be the "blinded secret," which in our new
terminology is called "unlinkable token." That's what the server signs. Then in
the attribution report the server receives the "unblinded secret" or in our
terminology the "secret token." But we can work that out in a later patch.

> Source/WebKit/NetworkProcess/NetworkSession.cpp:361
> +void NetworkSession::setFraudPreventionValuesForTesting(String&&
secretToken, String&& unlinkableToken, String&& signature, String&& keyID)

Let's add this comment here:
// FIXME: Switch to non-mocked test data once the right cryptography library is
available in open source.

> Source/WebKit/NetworkProcess/PrivateClickMeasurementManager.h:94
> +    struct TestingFraudPreventionValues {

Nice to do it as a struct.