Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 13 commits into from
Sep 21, 2023
Merged
4 changes: 4 additions & 0 deletions src/main/java/com/uid2/operator/model/IdentityMapPolicy.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,8 @@ public static com.uid2.operator.model.IdentityMapPolicy fromValue(int value) {
public static com.uid2.operator.model.IdentityMapPolicy defaultPolicy() {
return JustMap;
}

public static IdentityMapPolicy respectOptOut() {
return RespectOptOut;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,8 @@ public static TokenGeneratePolicy fromValue(int value) {
public static TokenGeneratePolicy defaultPolicy() {
return JustGenerate;
}

public static TokenGeneratePolicy respectOptOut() {
return RespectOptOut;
}
}
32 changes: 31 additions & 1 deletion src/main/java/com/uid2/operator/vertx/UIDOperatorVerticle.java
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import com.uid2.shared.store.IClientKeyProvider;
import com.uid2.shared.store.IClientSideKeypairStore;
import com.uid2.shared.store.ISaltProvider;
import com.uid2.shared.store.reader.RotatingClientKeyProvider;
import com.uid2.shared.vertx.RequestCapturingHandler;
import io.micrometer.core.instrument.Counter;
import io.micrometer.core.instrument.DistributionSummary;
Expand Down Expand Up @@ -94,6 +95,7 @@ public class UIDOperatorVerticle extends AbstractVerticle {
private final ITokenEncoder encoder;
private final ISaltProvider saltProvider;
private final IOptOutStore optOutStore;
private final IClientKeyProvider clientKeyProvider;
private final Clock clock;
protected IUIDOperatorService idService;
private final Map<String, DistributionSummary> _identityMapMetricSummaries = new HashMap<>();
Expand All @@ -111,9 +113,9 @@ public class UIDOperatorVerticle extends AbstractVerticle {
private final IStatsCollectorQueue _statsCollectorQueue;
private final KeyManager keyManager;
private final SecureLinkValidatorService secureLinkValidatorService;

private final boolean cstgDoDomainNameCheck;
public final static int MASTER_KEYSET_ID_FOR_SDKS = 9999999; //this is because SDKs have an issue where they assume keyset ids are always positive; that will be fixed.
public final static long OPT_OUT_CHECK_CUTOFF_DATE = Instant.parse("2023-09-01T00:00:00.00Z").getEpochSecond();

public UIDOperatorVerticle(JsonObject config,
boolean clientSideTokenGenerate,
Expand Down Expand Up @@ -149,6 +151,7 @@ public UIDOperatorVerticle(JsonObject config,
this.tcfVendorId = config.getInteger("tcf_vendor_id", 21);
this.cstgDoDomainNameCheck = config.getBoolean("client_side_token_generate_domain_name_check_enabled", true);
this._statsCollectorQueue = statsCollectorQueue;
this.clientKeyProvider = clientKeyProvider;
}

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

boolean respsctOptOut = req.containsKey(TOKEN_GENERATE_POLICY_PARAM)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in variable name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops...

&& TokenGeneratePolicy.fromValue(req.getInteger(TOKEN_GENERATE_POLICY_PARAM)) == TokenGeneratePolicy.respectOptOut();
if (!respsctOptOut && isOptOutCheckRequired(rc, respsctOptOut)) {
ResponseUtil.ClientError(rc, "Required opt-out policy argument is missing or not set to 1");
return;
}

final TokenGeneratePolicy tokenGeneratePolicy = readTokenGeneratePolicy(req);
final IdentityTokens t = this.idService.generateIdentity(
new IdentityRequest(
Expand Down Expand Up @@ -1274,6 +1284,13 @@ private void handleIdentityMapV2(RoutingContext rc) {
return;
}

boolean respectOptOut = requestJsonObject.containsKey(IDENTITY_MAP_POLICY_PARAM)
&& IdentityMapPolicy.fromValue(requestJsonObject.getInteger(IDENTITY_MAP_POLICY_PARAM)) == IdentityMapPolicy.respectOptOut();
if (!respectOptOut && isOptOutCheckRequired(rc, respectOptOut)) {
ResponseUtil.ClientError(rc, "Required opt-out policy argument is missing or not set to 1");
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make a function for the duplicate code?

Copy link
Contributor Author

@zaiweidu zaiweidu Sep 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored the code as suggested.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored the similar code into a single method.


IdentityMapPolicy identityMapPolicy = readIdentityMapPolicy(requestJsonObject);
recordIdentityMapPolicy(getApiContact(rc), identityMapPolicy);

Expand Down Expand Up @@ -1616,6 +1633,19 @@ private TokenGeneratePolicy readTokenGeneratePolicy(JsonObject req) {
TokenGeneratePolicy.defaultPolicy();
}

private boolean isOptOutCheckRequired(RoutingContext rc, boolean respectOptOut) {
final ClientKey clientKey = (ClientKey) AuthMiddleware.getAuthClient(rc);
final ClientKey oldestClientKey = this.clientKeyProvider.getOldestClientKey(clientKey.getSiteId());
boolean newClient = oldestClientKey.getCreated() >= OPT_OUT_CHECK_CUTOFF_DATE;

if (newClient && !respectOptOut) {
// log policy violation
LOGGER.warn(String.format("Failed to respect opt-out policy: siteId=%d, clientKeyName=%s, clientKeyCreated=%d",
oldestClientKey.getSiteId(), oldestClientKey.getName(), oldestClientKey.getCreated()));
}
return newClient;
}

private static final String IDENTITY_MAP_POLICY_PARAM = "policy";

private IdentityMapPolicy readIdentityMapPolicy(JsonObject req) {
Expand Down
71 changes: 70 additions & 1 deletion src/test/java/com/uid2/operator/UIDOperatorVerticleTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
import static com.uid2.operator.ClientSideTokenGenerateTestUtil.decrypt;
import static com.uid2.operator.service.EncodingUtils.getSha256;
import static com.uid2.operator.service.V2RequestUtil.V2_REQUEST_TIMESTAMP_DRIFT_THRESHOLD_IN_MINUTES;
import static com.uid2.operator.vertx.UIDOperatorVerticle.OPT_OUT_CHECK_CUTOFF_DATE;
import static com.uid2.shared.Const.Data.*;
import static org.junit.jupiter.api.Assertions.*;
import static org.mockito.ArgumentMatchers.any;
Expand All @@ -68,6 +69,8 @@
@ExtendWith(VertxExtension.class)
public class UIDOperatorVerticleTest {
private final Instant now = Instant.now().truncatedTo(ChronoUnit.MILLIS);
private static final Instant legacyClientCreationDateTime = Instant.ofEpochSecond(OPT_OUT_CHECK_CUTOFF_DATE).minus(1, ChronoUnit.SECONDS);
private static final Instant newClientCreationDateTime = Instant.ofEpochSecond(OPT_OUT_CHECK_CUTOFF_DATE).plus(1, ChronoUnit.SECONDS);
private static final String firstLevelSalt = "first-level-salt";
private static final SaltEntry rotatingSalt123 = new SaltEntry(123, "hashed123", 0, "salt123");
private static final Duration identityExpiresAfter = Duration.ofMinutes(10);
Expand Down Expand Up @@ -148,18 +151,22 @@ private static byte[] makeAesKey(String prefix) {
}

protected void fakeAuth(int siteId, Role... roles) {
fakeAuth(siteId, legacyClientCreationDateTime, roles);
}
protected void fakeAuth(int siteId, Instant created, Role... roles) {
ClientKey clientKey = new ClientKey(
"test-key",
"UID2-C-L-999-fCXrMM.fsR3mDqAXELtWWMS+xG1s7RdgRTMqdOH2qaAo=",
"fsSGnDxa/V9eJZ9Tas+dowwyO/X1UsC68RN9qM2xUu9ZOaKEOv9EVd7pkt3As/nE5B6TRu0PzK+IDzSQhD1+rw==",
Utils.toBase64String(clientSecret),
"test-contact",
now,
created,
Set.of(roles),
siteId
);
when(clientKeyProvider.get(any())).thenReturn(clientKey);
when(clientKeyProvider.getClientKey(any())).thenReturn(clientKey);
when(clientKeyProvider.getOldestClientKey(anyInt())).thenReturn(clientKey);
}

private void clearAuth() {
Expand Down Expand Up @@ -741,6 +748,68 @@ public static void validateAdvertisingToken(String advertisingTokenString, Token
}
}

@ParameterizedTest
@ValueSource(strings = {"NoOutoutPolicySpecified", "WrongOutOutPolicySpecified"})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@zaiweidu zaiweidu Sep 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 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.
  2. Removed Switch usage by splitting ParameterizedTest.

void identityMapOptOutPolicyCheckForNewClient(String testRun, Vertx vertx, VertxTestContext testContext) {
final int clientSiteId = 201;
fakeAuth(clientSiteId, newClientCreationDateTime, Role.MAPPER);
setupSalts();
setupKeys();

JsonObject req = new JsonObject();
JsonArray emails = new JsonArray();
emails.add("[email protected]");
req.put("email", emails);
switch (testRun) {
case "NoOutoutPolicySpecified":
break;
case "WrongOutOutPolicySpecified":
req.put("policy", 0);
break;
default:
req.put("policy", 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@zaiweidu zaiweidu Sep 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

break;
}

send("v2", vertx, "v2/identity/map", false, null, req, 400, respJson -> {
assertFalse(respJson.containsKey("body"));
assertEquals("client_error", respJson.getString("status"));
assertEquals("Required opt-out policy argument is missing or not set to 1", respJson.getString("message"));
testContext.completeNow();
});
}

@ParameterizedTest
@ValueSource(strings = {"NoPolicySpecified", "WrongPolicySpecified"})
void tokenGenerateOptOutPolicyCheckForNewClient(String testRun, Vertx vertx, VertxTestContext testContext) {
final int clientSiteId = 201;
fakeAuth(clientSiteId, newClientCreationDateTime, Role.GENERATOR);
setupSalts();
setupKeys();

JsonObject v2Payload = new JsonObject();
v2Payload.put("email", "[email protected]");
switch (testRun) {
case "NoPolicySpecified":
break;
case "WrongPolicySpecified":
v2Payload.put("policy", 0);
break;
default:
v2Payload.put("policy", 1);
break;
}

sendTokenGenerate("v2", vertx,
"", v2Payload, 400,
json -> {
assertFalse(json.containsKey("body"));
assertEquals("client_error", json.getString("status"));
assertEquals("Required opt-out policy argument is missing or not set to 1", json.getString("message"));
testContext.completeNow();
});
}

@ParameterizedTest
@ValueSource(strings = {"v1", "v2"})
void tokenGenerateForEmail(String apiVersion, Vertx vertx, VertxTestContext testContext) {
Expand Down