Skip to content

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.

Sign up for GitHub

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

Open
caroline-ttd wants to merge 15 commits into
base: main
Choose a base branch
from
+184 −60
Open

UID2-6797 replace allowed sites null to empty with dsp type #607

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
0126ee6
Replace allowed_site is null with allowed_site is empty and client ty…
caroline-ttd Mar 24, 2026
af7dbc7
Fix unit tests
caroline-ttd Mar 24, 2026
48d391d
Add more unit tests
caroline-ttd Mar 25, 2026
65f4fd7
[CI Pipeline] Released Snapshot version: 6.13.1-alpha-225-SNAPSHOT
Mar 25, 2026
4df44b5
Ignore for testing
caroline-ttd Mar 25, 2026
7dc2bdf
[CI Pipeline] Released Snapshot version: 6.13.2-alpha-226-SNAPSHOT
Mar 25, 2026
6dbf9d8
Remove test
caroline-ttd Mar 26, 2026
01359ac
Merge branch 'main' into ccm-UID2-6797-replace-allowed-sites-null-to-…
caroline-ttd Mar 26, 2026
79a2742
Address comments
caroline-ttd Mar 26, 2026
94c9ec5
Address comments
caroline-ttd Mar 26, 2026
7ef1043
Replaced createDefaultKeyset with createKeysetSharedWithDsps
caroline-ttd Mar 26, 2026
42b08f7
Remove resetAllowedSitesToNull
caroline-ttd Mar 26, 2026
c261274
[CI Pipeline] Released Snapshot version: 6.13.1-alpha-227-SNAPSHOT
Mar 26, 2026
869e05f
Update unit test names in SharingServiceTest
caroline-ttd Mar 28, 2026
fe665e1
Merge branch 'ccm-UID2-6797-replace-allowed-sites-null-to-empty-with-…
caroline-ttd Mar 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

<groupId>com.uid2</groupId>
<artifactId>uid2-admin</artifactId>
<version>6.13.0</version>
<version>6.13.1-alpha-227-SNAPSHOT</version>

<properties>
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
Expand Down
8 changes: 4 additions & 4 deletions src/main/java/com/uid2/admin/managers/KeysetManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ public static Integer getMaxKeyset(Map<Integer, AdminKeyset> keysets) {
return max(Collections.max(keysets.keySet()), 3);
}

public static AdminKeyset createDefaultKeyset(int siteId, int keysetId) {
public static AdminKeyset createKeysetSharedWithDsps(int siteId, int keysetId) {
String name = "";

//only set if both siteId and keysetId match our expectation according to the requirements
Expand All @@ -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)));
Copy link
Copy Markdown
Contributor

@jon8787 jon8787 Mar 26, 2026

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

Copy link
Copy Markdown
Contributor Author

@caroline-ttd caroline-ttd Mar 26, 2026

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.

Copy link
Copy Markdown
Contributor

@sunnywu sunnywu Mar 26, 2026 •
edited
Loading

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?

Copy link
Copy Markdown
Contributor Author

@caroline-ttd caroline-ttd Mar 26, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

If allowed_types was e.g. ADVERTISER, then we don't want to add DSP here, regardless of the value of allowed_sites

For existing entries, for example where allowed_type includes ADVERTISER and allowed_sites is null, we add DSP; for future entries, we don’t apply this change, is it correct?

Copy link
Copy Markdown
Contributor

@sunnywu sunnywu Mar 26, 2026

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)

Copy link
Copy Markdown
Contributor

@jon8787 jon8787 Mar 26, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

add/enable DSP type in the admin/portal UI

It might not just be the UI that calls createDefaultKeyset though; need to find all callers to figure out if that change would be ok

Copy link
Copy Markdown
Contributor Author

@caroline-ttd caroline-ttd Mar 26, 2026

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 = [] and allowed_types = [DSP] here. Since this function is createDefaultKeyset (not createKeyset), 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?

Copy link
Copy Markdown
Contributor Author

@caroline-ttd caroline-ttd Mar 26, 2026

Choose a reason for hiding this comment

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

Discussed with @jon8787 , replaced createDefaultKeyset with createKeysetSharedWithDsps

}

public static Keyset adminKeysetToKeyset(AdminKeyset adminKeyset, Map<ClientType, Set<Integer>> siteIdsByType) {
Expand Down Expand Up @@ -138,7 +138,7 @@ private AdminKeyset createAndAddDefaultKeyset(Integer siteId) throws Exception{

this.keysetProvider.loadContent();
int newKeysetId = getNextKeysetId();
AdminKeyset newKeyset = KeysetManager.createDefaultKeyset(siteId, newKeysetId);
AdminKeyset newKeyset = KeysetManager.createKeysetSharedWithDsps(siteId, newKeysetId);
addOrReplaceKeyset(newKeyset);
return newKeyset;
}
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/uid2/admin/vertx/service/EncryptionKeyService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -548,7 +548,7 @@ else if(siteId == Const.Data.RefreshKeySiteId) {
else if(siteId == Const.Data.AdvertisingTokenSiteId) {
newKeysetId = Const.Data.FallbackPublisherKeysetId;
}
keyset = createDefaultKeyset(siteId, newKeysetId);
keyset = createKeysetSharedWithDsps(siteId, newKeysetId);
currentKeysets.put(newKeysetId, keyset);
keysetStoreWriter.upload(currentKeysets, null);
}
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/com/uid2/admin/vertx/service/SharingService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());

Expand Down Expand Up @@ -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;
Expand Down
74 changes: 50 additions & 24 deletions src/test/java/com/uid2/admin/managers/KeysetManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,16 +58,16 @@ public void testAddOrReplaceKeyset() throws Exception{
keysetKeyManager, true);

Map<Integer, AdminKeyset> keysets = new HashMap<Integer, AdminKeyset>() {{
put(1, KeysetManager.createDefaultKeyset(3, 1));
put(2, KeysetManager.createDefaultKeyset(4, 2));
put(3, KeysetManager.createDefaultKeyset(5, 3));
put(4, KeysetManager.createDefaultKeyset(6, 4));
put(1, KeysetManager.createKeysetSharedWithDsps(3, 1));
put(2, KeysetManager.createKeysetSharedWithDsps(4, 2));
put(3, KeysetManager.createKeysetSharedWithDsps(5, 3));
put(4, KeysetManager.createKeysetSharedWithDsps(6, 4));
}};

setKeysets(keysets);
final int keysetId = 5;
// add new keyset
AdminKeyset keyset1 = KeysetManager.createDefaultKeyset(7, keysetId);
AdminKeyset keyset1 = KeysetManager.createKeysetSharedWithDsps(7, keysetId);
keysetManager.addOrReplaceKeyset(keyset1);
assertTrue(keysets.containsKey(5));
assertTrue(keyset1.equals(keysets.get(5)));
Expand Down Expand Up @@ -102,7 +102,7 @@ public void createsKeysetWhenNoneExists() throws Exception {

@Test
public void doesNotCreateKeysetWhenOneExists() throws Exception {
final AdminKeyset keyset = KeysetManager.createDefaultKeyset(1, 1);
final AdminKeyset keyset = KeysetManager.createKeysetSharedWithDsps(1, 1);
final HashMap<Integer, AdminKeyset> keysets = new HashMap<>();
keysets.put(1, keyset);

Expand Down Expand Up @@ -135,10 +135,10 @@ public void testCreateKeysetForClient() throws Exception {
keysetKeyManager, true);

Map<Integer, AdminKeyset> keysets = new HashMap<Integer, AdminKeyset>() {{
put(1, KeysetManager.createDefaultKeyset(3, 1));
put(2, KeysetManager.createDefaultKeyset(4, 2));
put(3, KeysetManager.createDefaultKeyset(5, 3));
put(4, KeysetManager.createDefaultKeyset(6, 4));
put(1, KeysetManager.createKeysetSharedWithDsps(3, 1));
put(2, KeysetManager.createKeysetSharedWithDsps(4, 2));
put(3, KeysetManager.createKeysetSharedWithDsps(5, 3));
put(4, KeysetManager.createKeysetSharedWithDsps(6, 4));
}};

setKeysets(keysets);
Expand All @@ -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);
Expand All @@ -172,6 +174,30 @@ public void testCreateKeysetForClient() throws Exception {
assertEquals(7, keysets.keySet().size());
}

@Test
public void createKeysetForSite_newKeyset_hasNonNullEmptyAllowedSitesAndDspType() throws Exception {
Copy link
Copy Markdown
Contributor

@sunnywu sunnywu Mar 27, 2026

Choose a reason for hiding this comment

The 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 createAndAddKeyset_emptyAllowedSites_nullAllowedTypes_hasEmptySitesAndNullAllowedTypes()
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() {
Expand Down Expand Up @@ -228,33 +254,33 @@ public void testGetMaxKeyset() {
public void testKeysetNameCreation() {

//expected cases of special keysets when site id and keyset id match our expectations
AdminKeyset keyset = createDefaultKeyset(-1, -1);
AdminKeyset keyset = createKeysetSharedWithDsps(-1, -1);
assertEquals(keyset.getName(), KeysetManager.MasterKeysetName);
keyset = createDefaultKeyset(-2, -2);
keyset = createKeysetSharedWithDsps(-2, -2);
assertEquals(keyset.getName(), KeysetManager.RefreshKeysetName);
keyset = createDefaultKeyset(2, 2);
keyset = createKeysetSharedWithDsps(2, 2);
assertEquals(keyset.getName(), KeysetManager.FallbackPublisherKeysetName);

//only site id matches but keyset id aren't the same as what we expected
keyset = createDefaultKeyset(-1, 3);
keyset = createKeysetSharedWithDsps(-1, 3);
assertEquals(keyset.getName(), "");
keyset = createDefaultKeyset(-2, 34);
keyset = createKeysetSharedWithDsps(-2, 34);
assertEquals(keyset.getName(), "");
keyset = createDefaultKeyset(2, 56);
keyset = createKeysetSharedWithDsps(2, 56);
assertEquals(keyset.getName(), "");

//only keyset id matches but site Id aren't the same as what we expected
keyset = createDefaultKeyset(-5, 1);
keyset = createKeysetSharedWithDsps(-5, 1);
assertEquals(keyset.getName(), "");
keyset = createDefaultKeyset(-3, 2);
keyset = createKeysetSharedWithDsps(-3, 2);
assertEquals(keyset.getName(), "");
keyset = createDefaultKeyset(20, 3);
keyset = createKeysetSharedWithDsps(20, 3);
assertEquals(keyset.getName(), "");

//for any other normal keyset creation
keyset = createDefaultKeyset(6, 7);
keyset = createKeysetSharedWithDsps(6, 7);
assertEquals(keyset.getName(), "");
keyset = createDefaultKeyset(9, 23);
keyset = createKeysetSharedWithDsps(9, 23);
assertEquals(keyset.getName(), "");
}

Expand Down
3 changes: 2 additions & 1 deletion src/test/java/com/uid2/admin/vertx/EncryptionKeyServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import com.uid2.admin.vertx.test.ServiceTestBase;
import com.uid2.shared.Const;
import com.uid2.shared.auth.Role;
import com.uid2.shared.model.ClientType;
import com.uid2.shared.model.EncryptionKey;
import com.uid2.shared.model.KeysetKey;
import io.vertx.core.Vertx;
Expand Down Expand Up @@ -150,7 +151,7 @@ void addSiteKeyAddsKeysetAndKey() throws Exception {
setKeysetKeys(123);
final EncryptionKey key = keyService.addSiteKey(5);

AdminKeyset expected = new AdminKeyset(4, 5, "", null, Instant.now().getEpochSecond(), true, true, new HashSet<>());
AdminKeyset expected = new AdminKeyset(4, 5, "", new HashSet<>(), Instant.now().getEpochSecond(), true, true, Set.of(ClientType.DSP));
assertNotNull(keysets.get(4));
assertEquals(expected, keysets.get(4));
verify(keysetKeyStoreWriter).upload(collectionOfSize(1), eq(124));
Expand Down
Loading
Loading