zdu UID2-1776 Enforce Opt-Out Policy Check for New Participants added after 9/1/2023 #207

zaiweidu · 2023-09-11T20:59:46Z

Token/Generate & Identity/Map

Added logic to derive Site Creation timestamp from the oldest Client Key Creation timestamp.
Enforce opt-out policy for new client added after 9/1

Unit Tests (merged from @Ian-Nara's PR)

New tests to validate desire behavior:

tokenGenerateNewClientNoPolicySpecified
tokenGenerateNewClientWrongPolicySpecified
tokenGenerateNewClientNoPolicySpecifiedOlderKeySuccessful
tokenGenerateNewClientWrongPolicySpecifiedOlderKeySuccessful
identityMapNewClientNoPolicySpecified
identityMapNewClientWrongPolicySpecified
identityMapNewClientNoPolicySpecifiedOlderKeySuccessful
identityMapNewClientWrongPolicySpecifiedOlderKeySuccessful

Existing tests to validate desired behavior:

tokenGenerateRespectOptOutOption
identityMapRespectOptOutOption
other existing identity map and token generate tests that do not specify policy for old participants

cody-constine-ttd

Can you write unit tests for the two endpoints that test this new functionality?

cody-constine-ttd · 2023-09-12T16:10:05Z

src/main/java/com/uid2/operator/vertx/UIDOperatorVerticle.java

+                LOGGER.error("request body misses opt-out policy argument");
+                ResponseUtil.ClientError(rc, "request body misses opt-out policy argument");
+                return;
+            }


Can we make a function for the duplicate code?

Refactored the code as suggested.

The two blocks still look largely similar. How can more stuff be pushed into the common function?

Refactored the similar code into a single method.

cody-constine-ttd · 2023-09-12T16:10:38Z

src/main/java/com/uid2/operator/vertx/UIDOperatorVerticle.java

@@ -1636,6 +1651,11 @@ private TokenGeneratePolicy readTokenGeneratePolicy(JsonObject req) {
                TokenGeneratePolicy.defaultPolicy();
    }

+    private boolean isAfterCutoffDate(long timestamp) {
+        long cutoff = Instant.parse("2023-09-01T00:00:00.00Z").getEpochSecond();


String should be a static member

I'd even suggest to make the whole Instant to be static final member. Or even the long. No reason to do these calculations every time.

Refactored as suggested.

atarassov-ttd · 2023-09-13T08:18:26Z

src/main/java/com/uid2/operator/vertx/UIDOperatorVerticle.java

@@ -777,6 +777,13 @@ private void handleTokenGenerateV2(RoutingContext rc) {
                    }
                }

+                if (isAfterCutoffDate(clientKey.getCreated()) && (!req.containsKey(TOKEN_GENERATE_POLICY_PARAM)


Why is this looking at the client API key used for auth?

Switched to the oldest client key.

atarassov-ttd · 2023-09-13T08:19:22Z

src/main/java/com/uid2/operator/vertx/UIDOperatorVerticle.java

+                if (isAfterCutoffDate(clientKey.getCreated()) && (!req.containsKey(TOKEN_GENERATE_POLICY_PARAM)
+                        || TokenGeneratePolicy.fromValue(req.getInteger(TOKEN_GENERATE_POLICY_PARAM)) != TokenGeneratePolicy.respectOptOut())) {
+                    LOGGER.error("request body misses opt-out policy argument");
+                    ResponseUtil.ClientError(rc, "request body misses opt-out policy argument");


The message can be misleading if one specifies policy=0 in the request.

Updated the error message.

atarassov-ttd · 2023-09-13T08:21:21Z

src/main/java/com/uid2/operator/vertx/UIDOperatorVerticle.java

@@ -1636,6 +1651,11 @@ private TokenGeneratePolicy readTokenGeneratePolicy(JsonObject req) {
                TokenGeneratePolicy.defaultPolicy();
    }

+    private boolean isAfterCutoffDate(long timestamp) {
+        long cutoff = Instant.parse("2023-09-01T00:00:00.00Z").getEpochSecond();


I'd even suggest to make the whole Instant to be static final member. Or even the long. No reason to do these calculations every time.

atarassov-ttd · 2023-09-13T08:22:04Z

src/main/java/com/uid2/operator/vertx/UIDOperatorVerticle.java

@@ -1636,6 +1651,11 @@ private TokenGeneratePolicy readTokenGeneratePolicy(JsonObject req) {
                TokenGeneratePolicy.defaultPolicy();
    }

+    private boolean isAfterCutoffDate(long timestamp) {


In the context of this over-bloated class, isAfterCutoffDate is not descriptive enough. Cut-off for what?

Renamed the method to be more meaningful.

atarassov-ttd · 2023-09-20T07:20:31Z

src/test/java/com/uid2/operator/UIDOperatorVerticleTest.java

@@ -741,6 +748,68 @@ public static void validateAdvertisingToken(String advertisingTokenString, Token
        }
    }

+    @ParameterizedTest
+    @ValueSource(strings = {"NoOutoutPolicySpecified", "WrongOutOutPolicySpecified"})


Can you pass the policy values instead of strings? You should avoid switch (and if) statements in the unit tests.

The org.junit.jupiter.params.provider requires compile-time constant for annotation. Since Enum policy values aren't primitive type, I can't pass them as annotation.

Removed Switch usage by splitting ParameterizedTest.

atarassov-ttd · 2023-09-20T07:20:54Z

src/test/java/com/uid2/operator/UIDOperatorVerticleTest.java

+                req.put("policy", 0);
+                break;
+            default:
+                req.put("policy", 1);


Judging by value source above this case would never get hit?

atarassov-ttd · 2023-09-20T07:21:24Z

src/main/java/com/uid2/operator/vertx/UIDOperatorVerticle.java

@@ -768,6 +771,13 @@ private void handleTokenGenerateV2(RoutingContext rc) {
                    }
                }

+                boolean respsctOptOut = req.containsKey(TOKEN_GENERATE_POLICY_PARAM)


Typo in variable name?

atarassov-ttd · 2023-09-20T07:23:36Z

src/main/java/com/uid2/operator/vertx/UIDOperatorVerticle.java

+                LOGGER.error("request body misses opt-out policy argument");
+                ResponseUtil.ClientError(rc, "request body misses opt-out policy argument");
+                return;
+            }


The two blocks still look largely similar. How can more stuff be pushed into the common function?

zaiweidu requested review from Ian-Nara, scong-ttd and cody-constine-ttd September 11, 2023 21:00

cody-constine-ttd reviewed Sep 12, 2023

View reviewed changes

atarassov-ttd reviewed Sep 13, 2023

View reviewed changes

zaiweidu added 2 commits September 18, 2023 14:50

Enforce opt-out policy check

51441a2

Enforce opt-out policy check

6cfbbe5

zaiweidu force-pushed the zdu-UID2-1776-Add-policy-check-for-Publisher branch from 1256174 to 6cfbbe5 Compare September 18, 2023 22:00

zaiweidu added 4 commits September 19, 2023 10:03

Enforce Opt-Out Policy Check for New Participants added after 9/1/2023

78a5075

Enforce Opt-Out Policy Check for New Participants added after 9/1/2023

eae64d9

Enforce Opt-Out Policy Check for New Participants added after 9/1/2023

8d1b9cc

Enforce Opt-Out Policy Check for New Participants added after 9/1/2023

4c38fd3

zaiweidu requested review from cody-constine-ttd and atarassov-ttd September 20, 2023 01:21

atarassov-ttd reviewed Sep 20, 2023

View reviewed changes

zaiweidu added 6 commits September 20, 2023 15:38

Enforce Opt-Out Policy Check for New Participants added after 9/1/2023

68f36ae

updated pom.xml

6f5a8dc

updated pom.xml

78fae45

updated pom.xml

c0d61a0

Added 2 more unit tests

ba941b5

Added more unit tests

3c3404a

zaiweidu requested a review from atarassov-ttd September 21, 2023 01:35

atarassov-ttd approved these changes Sep 21, 2023

View reviewed changes

Merge branch 'master' into zdu-UID2-1776-Add-policy-check-for-Publisher

84c6309

zaiweidu merged commit a407bc5 into master Sep 21, 2023

zaiweidu mentioned this pull request Sep 21, 2023

add unit tests for policy specification #211

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zdu UID2-1776 Enforce Opt-Out Policy Check for New Participants added after 9/1/2023 #207

zdu UID2-1776 Enforce Opt-Out Policy Check for New Participants added after 9/1/2023 #207

zaiweidu commented Sep 11, 2023 •

edited

Loading

cody-constine-ttd left a comment

cody-constine-ttd Sep 12, 2023

zaiweidu Sep 19, 2023 •

edited

Loading

atarassov-ttd Sep 20, 2023

zaiweidu Sep 21, 2023

cody-constine-ttd Sep 12, 2023

atarassov-ttd Sep 13, 2023

zaiweidu Sep 19, 2023

atarassov-ttd Sep 13, 2023

zaiweidu Sep 20, 2023

atarassov-ttd Sep 13, 2023

zaiweidu Sep 19, 2023

atarassov-ttd Sep 13, 2023

atarassov-ttd Sep 13, 2023

zaiweidu Sep 19, 2023

atarassov-ttd Sep 20, 2023

zaiweidu Sep 21, 2023 •

edited

Loading

atarassov-ttd Sep 20, 2023

zaiweidu Sep 21, 2023 •

edited

Loading

atarassov-ttd Sep 20, 2023

zaiweidu Sep 20, 2023

atarassov-ttd Sep 20, 2023

zdu UID2-1776 Enforce Opt-Out Policy Check for New Participants added after 9/1/2023 #207

zdu UID2-1776 Enforce Opt-Out Policy Check for New Participants added after 9/1/2023 #207

Conversation

zaiweidu commented Sep 11, 2023 • edited Loading

cody-constine-ttd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zaiweidu Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zaiweidu Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zaiweidu Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zaiweidu commented Sep 11, 2023 •

edited

Loading

zaiweidu Sep 19, 2023 •

edited

Loading

zaiweidu Sep 21, 2023 •

edited

Loading

zaiweidu Sep 21, 2023 •

edited

Loading