Fix PII pseudonymizer enrichment + unit tests

[benjben]

The test failing is due to http://snowplow-hosted-assets.s3.amazonaws.com/third-party/maxmind/GeoLite2-City.mmdb being removed/empty

This will be fixed in another commit.

[chuwy]

Looks great!

[dilyand]

@dilyand don't we want to leave an empty string empty in the hashed output ?

@benjben I'm not sure. But:

• If we leave an empty string empty but hash all other strings, we are making a decision that an empty string is not PII and so does not need to be pseudonymized.
• Depending on context, we might make that call in other, more complex situations. For example, if the field is email_address but the value does not match a 'valid email' regex, then it's not PII and we can leave it unhashed.
• Fundamentally, both of the above are the same type of sanitisation, the only difference is the complexity of the 'regex' that needs to be matched.
• Since we're not doing 2, we shouldn't be doing 1.
• Maybe in some situations knowing that a field is an empty string will reveal some PII? (Though this point is weakened by the fact that all empty strings will have the same hash, so they will be easy to recognise.)
• The case with null value is different. There we return no value and not a hash, because there is nothing to hash, not because we decide it's not sensitive.

I'm happy to consider counterpoints, but with the above in mind I'm leaning towards hashing the empty string.

WDYT?

[benjben]

If we leave an empty string empty but hash all other strings, we are making a decision that an empty string is not PII

IMHO that's the case. And hashing the empty strings will always produce the same hash.

if the field is email_address but the value does not match a 'valid email' regex, then it's not PII and we can leave it unhashed.

IMHO hashing of PII is not related to data validation and we should hash whatever there is to hash.

[dilyand]

In that case, the test reveals a bug...

I think I agree with:

hashing of PII is not related to data validation and we should hash whatever there is to hash

but we differ in that I think this includes all values, including empty string.

Btw, do you want to keep this test? I can rebase to merge my commit with your first one.

[benjben]

In that case, the test reveals a bug...

But that doesn't reproduce the error "" => object 🤔

do you want to keep this test?

Sure !! The more testing the better !

[chuwy]

@dilyand, @benjben let me know when its ready for review again - I'd like to start putting 1.4.0 together.

[chuwy]

Hey @benjben! Are you ok to fix the #351 in this branch and potentially incorporate Dilyan's commit.

[benjben]

Sure @chuwy ! So just to be sure that we don't duplicate work, this is what you first wanted to take care of but finally I do right ?

[chuwy]

@benjben nope, or at least I don't remember taking care of it. I implemented the recovery for a client.

[chuwy]

Oh, actually you're right - I wanted to do this! Sorry. Yeah, let's decide tomorrow - I'm just signing off today and thought you'll be working on something. Ignore me.

[chuwy]

Not sure I understand evertying that is going on here, but happy that we have tests.

My implementation of removeAddedFields is here just for a reference, feel free to add or ignore, although it's a bit shorter.

[oguzhanunlu]

LGTM!