-
Notifications
You must be signed in to change notification settings - Fork 6
UID2-6797 replace allowed sites null to empty with dsp type #607
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
base: main
Are you sure you want to change the base?
Changes from 8 commits
0126ee6
af7dbc7
48d391d
65f4fd7
4df44b5
7dc2bdf
6dbf9d8
01359ac
79a2742
94c9ec5
7ef1043
42b08f7
c261274
869e05f
fe665e1
ed04322
5906d52
2006f27
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -67,8 +67,8 @@ else if(siteId == Const.Data.RefreshKeySiteId && keysetId == Const.Data.RefreshK | |
| else if(siteId == Const.Data.AdvertisingTokenSiteId && keysetId == Const.Data.FallbackPublisherKeysetId) { | ||
| name = FallbackPublisherKeysetName; | ||
| } | ||
| return new AdminKeyset(keysetId, siteId, name, null, Instant.now().getEpochSecond(), | ||
| true, true, new HashSet<>()); | ||
| return new AdminKeyset(keysetId, siteId, name, new HashSet<>(), Instant.now().getEpochSecond(), | ||
| true, true, new HashSet<>(Set.of(ClientType.DSP))); | ||
| } | ||
|
|
||
| public static Keyset adminKeysetToKeyset(AdminKeyset adminKeyset, Map<ClientType, Set<Integer>> siteIdsByType) { | ||
|
|
@@ -122,6 +122,14 @@ private Optional<AdminKeyset> getAdminKeysetBySiteId(int siteId) { | |
|
|
||
| public AdminKeyset createAndAddKeyset(Integer siteId, Set<Integer> allowedSites, Set<ClientType> allowedTypes) throws Exception{ | ||
| if(!enableKeysets) return null; | ||
| if (allowedSites == null) { | ||
| allowedSites = new HashSet<>(); | ||
| Set<ClientType> typesWithDsp = (allowedTypes == null) | ||
| ? new HashSet<>() | ||
| : new HashSet<>(allowedTypes); | ||
| typesWithDsp.add(ClientType.DSP); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this doesn't seem right - we are adding DSP even if caller has asked for
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This occurs when
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If |
||
| allowedTypes = typesWithDsp; | ||
| } | ||
| int newKeysetId = getNextKeysetId(); | ||
| AdminKeyset keyset = new AdminKeyset(newKeysetId, siteId, "", allowedSites, | ||
| Instant.now().getEpochSecond(), true, true, allowedTypes); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -380,7 +380,7 @@ private void handleSetAllowedSites(RoutingContext rc) { | |
| AdminKeyset newKeyset = setAdminKeyset(rc, allowedSites, allowedTypes, siteId, keysetId, name); | ||
| if(newKeyset == null) return; | ||
| JsonObject jo = new JsonObject(); | ||
| jo.put("allowed_sites", allowedSites); | ||
| jo.put("allowed_sites", newKeyset.getAllowedSites()); | ||
| jo.put("allowed_types", newKeyset.getAllowedTypes()); | ||
| jo.put("hash", newKeyset.hashCode()); | ||
|
|
||
|
|
@@ -430,7 +430,7 @@ private AdminKeyset setAdminKeyset(RoutingContext rc, JsonArray allowedSites, Js | |
| .boxed() | ||
| .collect(Collectors.toSet()); | ||
| } else { | ||
| newlist = null; | ||
| newlist = new HashSet<>(); | ||
| } | ||
|
|
||
| Set<ClientType> newAllowedTypes = null; | ||
|
|
@@ -447,6 +447,10 @@ private AdminKeyset setAdminKeyset(RoutingContext rc, JsonArray allowedSites, Js | |
| } | ||
| } | ||
|
|
||
| if (allowedSites == null) { | ||
| newAllowedTypes.add(ClientType.DSP); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the logic of "DSP" being the default seems to repeat a few times in this codebase, could we move this knowledge to a common function?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As discussed, we removed this logic |
||
| } | ||
|
|
||
| final AdminKeyset newKeyset = new AdminKeyset(keysetId, siteId, name, | ||
| newlist, Instant.now().getEpochSecond(), true, true, newAllowedTypes); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -149,20 +149,22 @@ public void testCreateKeysetForClient() throws Exception { | |
| assertTrue(sharerKeyset.equals(returnedKeyset)); | ||
| assertEquals(sharerKeyset.getAllowedSites(), Set.of()); | ||
|
|
||
| // Generator makes a null list | ||
| // Generator makes an empty allowed_sites list with default allowed_types [DSP] | ||
| ClientKey generator = new ClientKey("", "", "", "", "", Instant.now(), Set.of(Role.GENERATOR), 8, false, "key-id-8"); | ||
| returnedKeyset = keysetManager.createKeysetForClient(generator); | ||
| AdminKeyset generatorKeyset = keysets.get(returnedKeyset.getKeysetId()); | ||
| assertTrue(generatorKeyset.equals(returnedKeyset)); | ||
| assertNull(generatorKeyset.getAllowedSites()); | ||
| assertEquals(Set.of(), generatorKeyset.getAllowedSites()); | ||
| assertEquals(Set.of(ClientType.DSP), generatorKeyset.getAllowedTypes()); | ||
|
|
||
| // Generator takes priority of sharer | ||
| ClientKey sharerGenerator = new ClientKey("", "", "", "", "", Instant.now(), Set.of(Role.SHARER, Role.GENERATOR), 9, false, "key-id-9"); | ||
| keysetManager.createKeysetForClient(sharerGenerator); | ||
| returnedKeyset = keysetManager.createKeysetForClient(sharerGenerator); | ||
| AdminKeyset bothKeyset = keysets.get(returnedKeyset.getKeysetId()); | ||
| assertTrue(bothKeyset.equals(returnedKeyset)); | ||
| assertNull(bothKeyset.getAllowedSites()); | ||
| assertEquals(Set.of(), bothKeyset.getAllowedSites()); | ||
| assertEquals(Set.of(ClientType.DSP), bothKeyset.getAllowedTypes()); | ||
|
|
||
| // If keyset already exists none gets added | ||
| returnedKeyset = keysetManager.createKeysetForClient(sharer); | ||
|
|
@@ -172,6 +174,54 @@ public void testCreateKeysetForClient() throws Exception { | |
| assertEquals(7, keysets.keySet().size()); | ||
| } | ||
|
|
||
| @Test | ||
| public void createAndAddKeyset_nullAllowedSites_storesEmptySetNotNullAndAddsDspWhenTypesNull() throws Exception { | ||
| setKeysets(new HashMap<>()); | ||
| KeysetManager keysetManager = new KeysetManager(keysetProvider, keysetStoreWriter, keysetKeyManager, true); | ||
|
|
||
| AdminKeyset created = keysetManager.createAndAddKeyset(42, null, null); | ||
|
|
||
| assertNotNull(created.getAllowedSites()); | ||
| assertTrue(created.getAllowedSites().isEmpty()); | ||
| assertEquals(Set.of(ClientType.DSP), created.getAllowedTypes()); | ||
| } | ||
|
|
||
| @Test | ||
| public void createAndAddKeyset_nullAllowedSites_preservesOtherTypesAndAddsDsp() throws Exception { | ||
| setKeysets(new HashMap<>()); | ||
| KeysetManager keysetManager = new KeysetManager(keysetProvider, keysetStoreWriter, keysetKeyManager, true); | ||
|
|
||
| AdminKeyset created = keysetManager.createAndAddKeyset(42, null, new HashSet<>(Set.of(ClientType.ADVERTISER))); | ||
|
|
||
| assertNotNull(created.getAllowedSites()); | ||
| assertTrue(created.getAllowedSites().isEmpty()); | ||
| assertTrue(created.getAllowedTypes().contains(ClientType.DSP)); | ||
| assertTrue(created.getAllowedTypes().contains(ClientType.ADVERTISER)); | ||
| } | ||
|
|
||
| @Test | ||
| public void createKeysetForSite_newKeyset_hasNonNullEmptyAllowedSitesAndDspType() throws Exception { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you create some end to end tests like in SharingServiceTest that would test calling an http endpoint on when creating a new site or new client key which will go thru the old workflow to generate keyset that now has allowed_type = DSP? testing these are not enough i think. |
||
| setKeysets(new HashMap<>()); | ||
| KeysetManager keysetManager = new KeysetManager(keysetProvider, keysetStoreWriter, keysetKeyManager, true); | ||
|
|
||
| AdminKeyset created = keysetManager.createKeysetForSite(99); | ||
|
|
||
| assertNotNull(created.getAllowedSites()); | ||
| assertTrue(created.getAllowedSites().isEmpty()); | ||
| assertTrue(created.getAllowedTypes().contains(ClientType.DSP)); | ||
| } | ||
|
|
||
| @Test | ||
| public void createKeysetForSite_newKeyset_hasEmptyAllowedSitesAndNullType() throws Exception { | ||
| setKeysets(new HashMap<>()); | ||
| KeysetManager keysetManager = new KeysetManager(keysetProvider, keysetStoreWriter, keysetKeyManager, true); | ||
|
|
||
| AdminKeyset created = keysetManager.createAndAddKeyset(42, new HashSet<>(), null); | ||
|
|
||
| assertNotNull(created.getAllowedSites()); | ||
| assertTrue(created.getAllowedSites().isEmpty()); | ||
| assertNull(created.getAllowedTypes()); | ||
| } | ||
|
|
||
| @Test | ||
| public void testLookUpKeyset() { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,8 +26,7 @@ | |
| import java.util.stream.Stream; | ||
|
|
||
| import static com.uid2.admin.vertx.Endpoints.API_SHARING_KEYSETS_RELATED; | ||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertNotNull; | ||
| import static org.junit.jupiter.api.Assertions.*; | ||
| import static org.mockito.Mockito.*; | ||
|
|
||
| public class SharingServiceTest extends ServiceTestBase { | ||
|
|
@@ -52,7 +51,7 @@ private void compareKeysetTypeListToResult(AdminKeyset keyset, JsonArray actualL | |
| .collect(Collectors.toSet()); | ||
| assertEquals(keyset.getAllowedTypes(), actualSet); | ||
| } | ||
|
|
||
| private void mockSiteExistence(Integer... sites){ | ||
| for(Integer site : sites) { | ||
| doReturn(new Site(site, "test-name", true, null)).when(siteProvider).getSite(site); | ||
|
|
@@ -250,6 +249,37 @@ void listSiteSetNew(Vertx vertx, VertxTestContext testContext) { | |
| }); | ||
| } | ||
|
|
||
| @Test | ||
| void listSiteSetNewOmittedAllowedSites_emptyAllowedSitesNotNullWithDsp(Vertx vertx, VertxTestContext testContext) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. don't quite understand what is this test testing here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test case: when calling |
||
| fakeAuth(Role.SHARING_PORTAL); | ||
|
|
||
| Map<Integer, AdminKeyset> keysets = new HashMap<Integer, AdminKeyset>() {{ | ||
| put(3, new AdminKeyset(3, 5, "test", Set.of(4, 6, 7), Instant.now().getEpochSecond(), true, true, new HashSet<>())); | ||
| put(4, new AdminKeyset(4, 7, "test", Set.of(12), Instant.now().getEpochSecond(), true, true, new HashSet<>())); | ||
| put(5, new AdminKeyset(5, 4, "test", Set.of(5), Instant.now().getEpochSecond(), true, true, new HashSet<>())); | ||
| }}; | ||
|
|
||
| setAdminKeysets(keysets); | ||
| mockSiteExistence(5, 7, 4, 8); | ||
|
|
||
| String body = " {\n" + | ||
| " \"hash\": 0\n" + | ||
| " }"; | ||
|
|
||
| post(vertx, testContext, "api/sharing/list/8", body, response -> { | ||
| assertEquals(200, response.statusCode()); | ||
|
|
||
| AdminKeyset expected = new AdminKeyset(6, 8, "", Set.of(), Instant.now().getEpochSecond(), true, true, Set.of(ClientType.DSP)); | ||
| compareKeysetListToResult(expected, response.bodyAsJsonObject().getJsonArray("allowed_sites")); | ||
| compareKeysetTypeListToResult(expected, response.bodyAsJsonObject().getJsonArray("allowed_types")); | ||
| assertEquals(expected.getAllowedSites(), keysets.get(6).getAllowedSites()); | ||
| assertEquals(expected.getAllowedTypes(), keysets.get(6).getAllowedTypes()); | ||
|
|
||
| verify(keysetKeyManager).addKeysetKey(6); | ||
| testContext.completeNow(); | ||
| }); | ||
| } | ||
|
|
||
| @Test | ||
| void listSiteSetNewWithType(Vertx vertx, VertxTestContext testContext) { | ||
| fakeAuth(Role.SHARING_PORTAL); | ||
|
|
@@ -1131,8 +1161,13 @@ void KeysetSetNewNullAllowedSites(Vertx vertx, VertxTestContext testContext) { | |
| post(vertx, testContext, "api/sharing/keyset", body, response -> { | ||
| assertEquals(200, response.statusCode()); | ||
|
|
||
| AdminKeyset expected = new AdminKeyset(2, 1, "test", null, Instant.now().getEpochSecond(), true, true, new HashSet<>()); | ||
| assertEquals(null, response.bodyAsJsonObject().getJsonArray("allowed_sites")); | ||
| // Next keyset id is max(existing ids, 3) + 1 => 4 when only keyset 1 exists | ||
| int newKeysetId = 4; | ||
| AdminKeyset expected = new AdminKeyset(newKeysetId, 3, "", new HashSet<>(), Instant.now().getEpochSecond(), true, true, Set.of(ClientType.DSP)); | ||
| compareKeysetListToResult(expected, response.bodyAsJsonObject().getJsonArray("allowed_sites")); | ||
| compareKeysetTypeListToResult(expected, response.bodyAsJsonObject().getJsonArray("allowed_types")); | ||
| assertEquals(expected.getAllowedSites(), keysets.get(newKeysetId).getAllowedSites()); | ||
| assertEquals(expected.getAllowedTypes(), keysets.get(newKeysetId).getAllowedTypes()); | ||
|
|
||
| testContext.completeNow(); | ||
| }); | ||
|
|
@@ -1158,8 +1193,12 @@ void KeysetSetNewExplicitlyNullAllowedSites(Vertx vertx, VertxTestContext testCo | |
| post(vertx, testContext, "api/sharing/keyset", body, response -> { | ||
| assertEquals(200, response.statusCode()); | ||
|
|
||
| AdminKeyset expected = new AdminKeyset(2, 1, "test", null, Instant.now().getEpochSecond(), true, true, new HashSet<>()); | ||
| assertEquals(null, response.bodyAsJsonObject().getJsonArray("allowed_sites")); | ||
| int newKeysetId = 4; | ||
| AdminKeyset expected = new AdminKeyset(newKeysetId, 3, "", new HashSet<>(), Instant.now().getEpochSecond(), true, true, Set.of(ClientType.DSP)); | ||
| compareKeysetListToResult(expected, response.bodyAsJsonObject().getJsonArray("allowed_sites")); | ||
| compareKeysetTypeListToResult(expected, response.bodyAsJsonObject().getJsonArray("allowed_types")); | ||
| assertEquals(expected.getAllowedSites(), keysets.get(newKeysetId).getAllowedSites()); | ||
| assertEquals(expected.getAllowedTypes(), keysets.get(newKeysetId).getAllowedTypes()); | ||
|
|
||
| testContext.completeNow(); | ||
| }); | ||
|
|
@@ -1184,8 +1223,11 @@ void KeysetSetUpdateNullAllowedSites(Vertx vertx, VertxTestContext testContext) | |
| post(vertx, testContext, "api/sharing/keyset", body, response -> { | ||
| assertEquals(200, response.statusCode()); | ||
|
|
||
| AdminKeyset expected = new AdminKeyset(1, 5, "test", null, Instant.now().getEpochSecond(), true, true, new HashSet<>()); | ||
| assertEquals(null, response.bodyAsJsonObject().getJsonArray("allowed_sites")); | ||
| AdminKeyset expected = new AdminKeyset(1, 5, "test", Set.of(), Instant.now().getEpochSecond(), true, true, Set.of(ClientType.DSP)); | ||
| compareKeysetListToResult(expected, response.bodyAsJsonObject().getJsonArray("allowed_sites")); | ||
| compareKeysetTypeListToResult(expected, response.bodyAsJsonObject().getJsonArray("allowed_types")); | ||
| assertEquals(expected.getAllowedSites(), keysets.get(1).getAllowedSites()); | ||
| assertEquals(expected.getAllowedTypes(), keysets.get(1).getAllowedTypes()); | ||
|
|
||
| testContext.completeNow(); | ||
| }); | ||
|
|
@@ -1211,8 +1253,11 @@ void KeysetSetUpdateExplicitlyNullAllowedSites(Vertx vertx, VertxTestContext tes | |
| post(vertx, testContext, "api/sharing/keyset", body, response -> { | ||
| assertEquals(200, response.statusCode()); | ||
|
|
||
| AdminKeyset expected = new AdminKeyset(1, 5, "test", null, Instant.now().getEpochSecond(), true, true, new HashSet<>()); | ||
| assertEquals(null, response.bodyAsJsonObject().getJsonArray("allowed_sites")); | ||
| AdminKeyset expected = new AdminKeyset(1, 5, "test", Set.of(), Instant.now().getEpochSecond(), true, true, Set.of(ClientType.DSP)); | ||
| compareKeysetListToResult(expected, response.bodyAsJsonObject().getJsonArray("allowed_sites")); | ||
| compareKeysetTypeListToResult(expected, response.bodyAsJsonObject().getJsonArray("allowed_types")); | ||
| assertEquals(expected.getAllowedSites(), keysets.get(1).getAllowedSites()); | ||
| assertEquals(expected.getAllowedTypes(), keysets.get(1).getAllowedTypes()); | ||
|
|
||
| testContext.completeNow(); | ||
| }); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a comment why we're adding DSP here? is it because it's equivalent to the old behaviour?
e.g. it's not correct for a keyset belonging to a DATA_PROVIDER or ADVERTISER to automatically share with DSPs, but if we're doing it to match old behaviour then it's reasonable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is equivalent to the old behavior, will add the comment.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
having a second thought - we should backport adding DSP type into existing sites when appropriate, but going forward, for any new site, we should by default add/enable DSP type in the admin/portal UI and not relying on checking allowed_sites = null from the JSON array input from admin UI.
So that the behaviour is by default, add DSP type to allowed_types and site needs to explicitly disable the DSP type?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For existing entries, for example where
allowed_typeincludes ADVERTISER andallowed_sitesis null, we add DSP; for future entries, we don’t apply this change, is it correct?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes that's what i'm proposing - for futre entries by default in the admin UI/portal, we enable DSP but user can disable/un-choose it, but whatever allowed_types remain in the call is the actual allowed_types we are setting (not adding DSP regardless)
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might not just be the UI that calls
createDefaultKeysetthough; need to find all callers to figure out if that change would be okThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think maybe we can keep the logic that sets
allowed_sites = []andallowed_types = [DSP]here. Since this function iscreateDefaultKeyset(notcreateKeyset), it represents a specific default behavior. Using these values for the default keyset should be clear and not cause confusion in the future. What do you think?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed with @jon8787 , replaced
createDefaultKeysetwithcreateKeysetSharedWithDsps