UID2-6797 replace allowed sites null to empty with dsp type by caroline-ttd · Pull Request #607 · IABTechLab/uid2-admin

caroline-ttd · 2026-03-25T01:06:01Z

Update:

Remove Reset Allowed Sites To Null

Rename createDefaultKeyset to createKeysetSharedWithDsps
setAdminKeyset will reset allowed_site null to empty but do not add DSP in clientType, it will be determined by admin UI keyset page and portal.

Test:

unit tests
Sanity tests
In the admin test environment, I created a snapshot.
createAndAddKeyset -> createKeysetForClient -> post /api/client/add 0r /api/client/update Or /api/client/roles or /api/client/contact

In createKeysetForClient function

    public AdminKeyset createKeysetForClient(ClientKey client) throws Exception{
        if(!enableKeysets) return null;
        Optional<AdminKeyset> keyset = getAdminKeysetBySiteId(client.getSiteId());
        if (keyset.isPresent()) return keyset.get();

        if(client.hasRole(Role.GENERATOR)) {
            return createAndAddDefaultKeyset(client.getSiteId());
        }
        if(client.hasRole(Role.SHARER)) {
            return createAndAddKeyset(client.getSiteId(), new HashSet<>(), new HashSet<>());
        }
        return null;
    }

2.1 After creating a site and a client key, the keyset appears as follows: (post /api/client/add, Role.SHARER)
empty as expected

{
  "id": 1013,
  "name": "ccm-test-keyset-1",
  "description": "ccm-test-keyset-1",
  "enabled": false,
  "clientTypes": [
    "PUBLISHER"
  ],
  "domain_names": [],
  "app_names": [],
  "visible": true,
  "created": 1774489564,
  "roles": [
    "SHARER"
  ],
  "client_count": 1
}

[
  {
    "keyset_id": 1009,
    "site_id": 1013,
    "name": "",
    "allowed_sites": [],
    "allowed_types": [],
    "created": 1774491044,
    "is_enabled": true,
    "is_default": true
  }
]

2.2 createAndAddDefaultKeyset -> post /api/client_side_keypairs/add or createKeysetForClient as 2.1
After creating a site and a client key, the keyset appears as follows: (post /api/client/add, Role.GENERATOR)

{
  "id": 1012,
  "name": "ccm-test-keyset",
  "description": "ccm-test-keyset",
  "enabled": false,
  "clientTypes": [
    "PUBLISHER"
  ],
  "domain_names": [],
  "app_names": [],
  "visible": true,
  "created": 1774346265,
  "roles": [
    "GENERATOR"
  ],
  "client_count": 1
}


 [
  {
    "keyset_id": 1008,
    "site_id": 1012,
    "name": "",
    "allowed_sites": [],
    "allowed_types": [
      "DSP"
    ],
    "created": 1774484758,
    "is_enabled": true,
    "is_default": true
  }
]

2.3 setAdminKeyset -> POST /api/sharing/keyset or POST /api/sharing/list/:siteId
In admin UI -> Reset Allowed Sites To Null (POST to /api/sharing/keyset)
Before:

[
  {
    "keyset_id": 1009,
    "site_id": 1013,
    "name": "",
    "allowed_sites": [],
    "allowed_types": [],
    "created": 1774491044,
    "is_enabled": true,
    "is_default": true
  }
]

After:

{
  "keyset_id": 1009,
  "site_id": 1013,
  "name": "",
  "allowed_sites": [],
  "allowed_types": [
    "DSP"
  ],
  "created": 1774491837,
  "is_enabled": true,
  "is_default": true
}

Reset Allowed Sites To Empty UI: (Same as before)
Before

{
  "keyset_id": 1009,
  "site_id": 1013,
  "name": "",
  "allowed_sites": [],
  "allowed_types": [
    "DSP"
  ],
  "created": 1774491899,
  "is_enabled": true,
  "is_default": true
}

After

{
  "keyset_id": 1009,
  "site_id": 1013,
  "name": "",
  "allowed_sites": [],
  "allowed_types": [],
  "created": 1774491924,
  "is_enabled": true,
  "is_default": true
}

…pe DSP

…empty-with-dsp-type

sunnywu

Review: UID2-6797 Replace allowed_sites null with empty + DSP type

Overall the change is logically sound and well-tested. A few items worth addressing:

Issue: pom.xml snapshot version should be reverted

The CI pipeline committed 6.13.2-alpha-226-SNAPSHOT into the PR branch. This version bump should be reverted before merge (back to 6.13.0 or whatever the intended next release version is). Snapshot versions committed to feature branches can cause version ordering problems downstream.

Sunny: I think this reporting of bug is incorrect as we do want to add DSP if allowed_sites = null but just for reference here.

NOT Bug: `setAdminKeyset` – DSP gets added to existing types unexpectedly when only `allowedTypes` is non-empty

In SharingService.java (setAdminKeyset):

Set<ClientType> newAllowedTypes = null;
if(allowedTypes == null || allowedTypes.isEmpty()) {
    newAllowedTypes = new HashSet<>();
} else {
    newAllowedTypes = allowedTypes.stream()...
}

if (allowedSites == null) {
    newAllowedTypes.add(ClientType.DSP);  // DSP always added when allowedSites is null
}

If a caller sends { "allowed_types": ["ADVERTISER"] } with no allowed_sites key, the result is {ADVERTISER, DSP}. The KeysetManagerTest covers this case (createAndAddKeyset_nullAllowedSites_preservesOtherTypesAndAddsDsp), so this appears intentional — but it's worth making explicit in the PR description if so, since it's a non-obvious interaction.

Minor: Missing `SharingServiceTest` for `null allowed_sites + non-empty allowed_types`

KeysetManagerTest covers createAndAddKeyset(42, null, {ADVERTISER}), but there's no corresponding SharingServiceTest verifying that the POST /api/sharing/keyset endpoint merges DSP into existing types when allowed_sites is null/absent from the JSON body. Worth adding for completeness.

Minor: Misleading test method name

createKeysetForSite_newKeyset_hasEmptyAllowedSitesAndNullType — this test actually calls createAndAddKeyset (not createKeysetForSite), and asserts null allowed types when non-null empty allowedSites is passed. The name doesn't match the test body.

Sunny: I think we should keep this just in case, depending how we do the migration.

Nit: Existing null-guard on response serialization is now largely dead code

In SharingService (lines 299-300, 317-318), the response still guards against null getAllowedSites() with allowedSites != null ? ... : null. After this change, newly created/updated keysets will never have null allowed_sites. The guards are still safe for legacy stored data but may be worth a follow-up cleanup.

Positive notes

The fix in handleSetAllowedSites to use newKeyset.getAllowedSites() instead of the raw allowedSites JSON param in the response is correct — it now reflects what was actually persisted.
Sanity test results in the PR description are thorough and clearly demonstrate the behavioral changes.
No NPE risk: newAllowedTypes is always initialized before the if (allowedSites == null) check.
CI passes (build + dependency checks green).

caroline-ttd · 2026-03-26T03:34:00Z

Sunny: I think we should keep this just in case, depending how we do the migration.

Yes, I think those can be cleaned up after we make sure no allowed_site null exists. It is good to keep it for now.

jon8787 · 2026-03-26T03:42:34Z

src/main/java/com/uid2/admin/managers/KeysetManager.java

+            Set<ClientType> typesWithDsp = (allowedTypes == null)
+                    ? new HashSet<>()
+                    : new HashSet<>(allowedTypes);
+            typesWithDsp.add(ClientType.DSP);


this doesn't seem right - we are adding DSP even if caller has asked for allowedTypes containing non-DSP types?

This occurs when allowed_sites is null. In that case, after replacing allow_site null to empty, do we always add DSP to allowed_types, even if other types are already present?

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

jon8787 · 2026-03-26T03:43:57Z

src/main/java/com/uid2/admin/managers/KeysetManager.java

-        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)));


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

Yes, it is equivalent to the old behavior, will add the comment.

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?

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?

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)

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

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?

Discussed with @jon8787 , replaced createDefaultKeyset with createKeysetSharedWithDsps

jon8787 · 2026-03-26T03:45:33Z

src/main/java/com/uid2/admin/vertx/service/SharingService.java

        }

+        if (allowedSites == null) {
+            newAllowedTypes.add(ClientType.DSP);


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?

As discussed, we removed this logic

sunnywu · 2026-03-27T02:50:25Z

src/test/java/com/uid2/admin/vertx/SharingServiceTest.java

    }

+    @Test
+    void listSiteSetNewOmittedAllowedSites_emptyAllowedSitesNotNullWithDsp(Vertx vertx, VertxTestContext testContext) {


don't quite understand what is this test testing here?

sunnywu · 2026-03-27T02:52:17Z

src/test/java/com/uid2/admin/vertx/SharingServiceTest.java

    }

    @Test
    void KeysetSetNewNullAllowedSites(Vertx vertx, VertxTestContext testContext) {


this is not the right test name as we aren't testing null alowed sites input any more?

sunnywu · 2026-03-27T02:55:36Z

src/test/java/com/uid2/admin/vertx/SharingServiceTest.java

    }

    @Test
    void KeysetSetNewExplicitlyNullAllowedSites(Vertx vertx, VertxTestContext testContext) {


KeysetSetUsingNewExplicitlyNullAllowedSitesInput?

sunnywu · 2026-03-27T03:37:13Z

src/test/java/com/uid2/admin/managers/KeysetManagerTest.java

    }

+    @Test
+    public void createKeysetForSite_newKeyset_hasNonNullEmptyAllowedSitesAndDspType() throws Exception {


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.

caroline-ttd and others added 8 commits March 24, 2026 14:27

Replace allowed_site is null with allowed_site is empty and client ty…

0126ee6

…pe DSP

Fix unit tests

af7dbc7

Add more unit tests

48d391d

[CI Pipeline] Released Snapshot version: 6.13.1-alpha-225-SNAPSHOT

65f4fd7

Ignore for testing

4df44b5

[CI Pipeline] Released Snapshot version: 6.13.2-alpha-226-SNAPSHOT

7dc2bdf

Remove test

6dbf9d8

Merge branch 'main' into ccm-UID2-6797-replace-allowed-sites-null-to-…

01359ac

…empty-with-dsp-type

sunnywu reviewed Mar 26, 2026

View reviewed changes

jon8787 reviewed Mar 26, 2026

View reviewed changes

caroline-ttd and others added 5 commits March 26, 2026 01:44

Address comments

79a2742

Address comments

94c9ec5

Replaced createDefaultKeyset with createKeysetSharedWithDsps

7ef1043

Remove resetAllowedSitesToNull

42b08f7

[CI Pipeline] Released Snapshot version: 6.13.1-alpha-227-SNAPSHOT

c261274

sunnywu reviewed Mar 27, 2026

View reviewed changes

Conversation

caroline-ttd commented Mar 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sunnywu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Review: UID2-6797 Replace allowed_sites null with empty + DSP type

Issue: pom.xml snapshot version should be reverted

NOT Bug: setAdminKeyset – DSP gets added to existing types unexpectedly when only allowedTypes is non-empty

Minor: Missing SharingServiceTest for null allowed_sites + non-empty allowed_types

Minor: Misleading test method name

Nit: Existing null-guard on response serialization is now largely dead code

Positive notes

Uh oh!

caroline-ttd commented Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sunnywu Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caroline-ttd Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jon8787 Mar 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

caroline-ttd commented Mar 25, 2026 •

edited

Loading

sunnywu left a comment •

edited

Loading

NOT Bug: `setAdminKeyset` – DSP gets added to existing types unexpectedly when only `allowedTypes` is non-empty

Minor: Missing `SharingServiceTest` for `null allowed_sites + non-empty allowed_types`

caroline-ttd commented Mar 26, 2026 •

edited

Loading

sunnywu Mar 26, 2026 •

edited

Loading

caroline-ttd Mar 26, 2026 •

edited

Loading

jon8787 Mar 26, 2026 •

edited

Loading