[ads] Only collect, store and redeem essential metadata #24188

tmancey · 2024-06-13T15:58:18Z

Resolves brave/brave-browser#38955

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on
All relevant documentation has been updated, for instance:

Test Plan:

Also, see brave/brave-browser#38955.

Non-Brave Rewards user:

Test Case #1:

Fresh install
Navigate to search.brave.software
Search for giraffe
EXPECT RESULT: Ad viewed event should not occur
Navigate to https://www.brave.com/vertical-tabs/ to trigger a conversion
EXPECT RESULT: View-through conversion should not occur
The following state should be verified:
- Default/ads_service/client.json state should be empty
- Default/ads_service/confirmations.json state should be empty
- Default/ads_service/database.sqlite:
  - All tables should be empty

--

Test Case #2:

Fresh install
Navigate to search.brave.software
Search for giraffe
Click [Staging] Brave Ads ad
EXPECT RESULT: Clicked search result ad with placement id # and creative instance id # and Successfully saved creative set conversions appear in the console log
Navigate to https://www.brave.com/vertical-tabs/ to trigger a conversion
EXPECT RESULT: Successfully processed conversion confirmation for search_result_ad with transaction id # and creative instance id # appears in the console log and {"conversion":[{"action":"click"}],"creativeInstanceId":"#","transactionId":"#","type":"conversion"} should be sent to
https://search.anonymous.ads.bravesoftware.com/v3/confirmation/#
The following state should be verified:
- Default/ads_service/client.json state should be empty
- Default/ads_service/confirmations.json state should be empty
- Default/ads_service/database.sqlite:
  - The creative_set_conversions table should contain a list of conversions for the associated creative set
  - The ad_events table should only contain click and conversion events
  - All other tables should be empty

--

Test Case #3:

Regression test Brave News ads

Brave Rewards user:

Test Case #1:

Regression test search result ads and Brave News ads

tmancey · 2024-06-14T08:08:48Z

Privacy review https://github.com/brave/reviews/issues/1636

ShivanKaul · 2024-07-16T20:49:42Z

Is https://github.com/brave/brave-browser/wiki/Brave-Ads-Endpoint:-non-Brave-Rewards-users up-to-date?

tmancey · 2024-07-16T20:58:12Z

Is https://github.com/brave/brave-browser/wiki/Brave-Ads-Endpoint:-non-Brave-Rewards-users up-to-date?

@ShivanKaul how about linking this page to https://github.com/brave/brave-core/blob/issues/38955/components/brave_ads/core/internal/account/confirmations/non_reward/README.md? This way, we can avoid the need to keep both pages in sync. Thanks!

fmarier

All of my review comments have been addressed.

ShivanKaul · 2024-07-16T23:32:44Z

components/brave_ads/core/internal/account/confirmations/non_reward/README.md

+| creativeInstanceId  | {"creativeInstanceId":"e4958d00-e35c-4134-a408-1fbcf274d5ae"}  | An id that references the specific ad creative that the user engaged with. This will be the same for any user that engages with this ad.  |
+| transactionId  | {"transactionId":"c14d370c-1178-4c73-8385-1cfa17200646"}  | A unique id for the transaction, which is not linkable between confirmation redemptions.  |
+| type  | {"type":"click"}  | Action or interaction that occurred within an advertisement, such as a user clicking the ad.<br><br>Supported types:<br><br>Brave Search ads:<br>- conversion<br><br>Brave News ads:<br>- view<br>- click<br>- landed<br>- conversion &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;  |
+| user data  | {"conversion":[{"action":"click"}]}  | See [user data](../../user_data/README.md#non-brave-rewards-user).  |


Suggested change

| user data | {"conversion":[{"action":"click"}]} | See [user data](../../user_data/README.md#non-brave-rewards-user). |

| user_data | {"conversion":[{"action":"click"}]} | For non-Rewards users, this is minimal. See [user data](../../user_data/README.md#non-brave-rewards-user). |

Regarding user_data see #24188 (comment). Thanks

Regarding "For non-Rewards users, this is minimal," this is not true for Brave News users because they can opt in without joining Brave Rewards.

ShivanKaul · 2024-07-16T23:33:19Z

components/brave_ads/core/internal/account/confirmations/reward/README.md

+| transactionId  | {"transactionId":"c14d370c-1178-4c73-8385-1cfa17200646"}  | A unique id for the transaction, which is not linkable between confirmation token redemptions.  |
+| type  | {"type":"view"}  | Action or interaction that occurred within an advertisement, such as a user clicking the ad.<br><br>Supported types:<br><br>- view<br>- click<br>- landed<br>- conversion<br>- media_play[^1]<br>- media_25[^1]<br>- media_100[^1]<br>- upvote<br>- downvote<br>- flag<br>- bookmark &nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;  |
+| confirmation token  | {"blindedPaymentTokens": ["Ev5JE4/9TZI/5TqyN9JWfJ1To0HBwQw2rWeAPcdjX3Q="],<br> "publicKey": "RJ2i/o/pZkrH+i0aGEMY1G9FXtd7Q7gfRi3YdNRnDDk="}  | See [security and privacy model for ad confirmations](https://github.com/brave/brave-browser/wiki/Security-and-privacy-model-for-ad-confirmations).  |
+| user data  | {"buildChannel":"nightly",<br>"catalog":[{"id":"29e5c8bc0ba319069980bb390d8e8f9b58c05a20"}],<br>"conversion":[{"action":"view"}],<br>"createdAtTimestamp":"2020-11-18T12:00:00.000Z",<br>"httpResponseStatus":"errorPage",<br>"platform":"windows",<br>"rotatingHash":"I6KM54gXOrWqRHyrD518LmhePLHpIk4KSgCKOl0e3sc=",<br>"segment":"sports",<br>"studies":[{"group":"GroupA","name":"BraveAds.FooStudy"},{"group":"GroupB","name":"BraveAds.BarStudy"}],<br>"versionNumber":"1.2.3.4"}  | See [user data](../../user_data/README.md#brave-rewards-users).  |


Suggested change

| user data | {"buildChannel":"nightly", "catalog":[{"id":"29e5c8bc0ba319069980bb390d8e8f9b58c05a20"}], "conversion":[{"action":"view"}], "createdAtTimestamp":"2020-11-18T12:00:00.000Z", "httpResponseStatus":"errorPage", "platform":"windows", "rotatingHash":"I6KM54gXOrWqRHyrD518LmhePLHpIk4KSgCKOl0e3sc=", "segment":"sports", "studies":[{"group":"GroupA","name":"BraveAds.FooStudy"},{"group":"GroupB","name":"BraveAds.BarStudy"}], "versionNumber":"1.2.3.4"} | See [user data](../../user_data/README.md#brave-rewards-users). |

| user_data | {"buildChannel":"nightly", "catalog":[{"id":"29e5c8bc0ba319069980bb390d8e8f9b58c05a20"}], "conversion":[{"action":"view"}], "createdAtTimestamp":"2020-11-18T12:00:00.000Z", "httpResponseStatus":"errorPage", "platform":"windows", "rotatingHash":"I6KM54gXOrWqRHyrD518LmhePLHpIk4KSgCKOl0e3sc=", "segment":"sports", "studies":[{"group":"GroupA","name":"BraveAds.FooStudy"},{"group":"GroupB","name":"BraveAds.BarStudy"}], "versionNumber":"1.2.3.4"} | See [user data](../../user_data/README.md#brave-rewards-users). |

@ShivanKaul, in this context, user data is not a key that is sent to the server, which is why it was written as user data without the underscore. However, I’m happy to change it if you think it would make things clearer. Thanks

ShivanKaul · 2024-07-16T23:38:34Z

@tmancey I updated https://github.com/brave/brave-browser/wiki/Brave-Ads-Endpoint:-non-Brave-Rewards-users

fmarier · 2024-07-17T02:59:15Z

I updated https://github.com/brave/brave-browser/wiki/Brave-Ads-Endpoint:-non-Brave-Rewards-users

Is https://github.com/brave/brave-core/blob/issues/38955/components/brave_ads/core/internal/account/confirmations/non_reward/README.md a stable link? It won't be the latest, that's for sure since it's pointing to an issue.

tmancey · 2024-07-17T03:11:37Z

This would not be the final link. We need to replace issues/38955 with master which will only work once this is merged. Thanks

tmancey · 2024-07-17T13:25:08Z

@fmarier I decoupled CanConvertAdEvent business logic from conversions_util/IsAllowedToConvertAdEvent to conversions_util_internal. And added a guard to page_land_user_data.cc. I also added some additional tests. Could you please review the recent changes? Thanks

components/brave_ads/core/internal/account/user_data/fixed/page_land_user_data.cc

components/brave_ads/core/internal/user_engagement/conversions/conversions_util_internal.cc

Co-authored-by: Francois Marier <francois@brave.com>

…ption payload for Rewards users

…ctual user data

Co-authored-by: Francois Marier <francois@brave.com>

github-actions · 2024-07-17T20:54:02Z

[puLL-Merge] - brave/brave-core@24188

Description

This PR makes significant changes to the Brave Ads codebase, focusing on handling ad conversions and events for both Brave Rewards users and non-Rewards users. The changes aim to improve the handling of different ad types, conversion tracking, and event triggering based on user settings and ad types.

Possible Issues

The changes introduce new logic for handling ad events and conversions for non-Rewards users, which could potentially lead to unexpected behavior if not thoroughly tested across all supported ad types and user scenarios.
The removal of some user data fields (like locale and top segment) might affect existing analytics or reporting systems that rely on this data.

Security Hotspots

The changes to conversion handling and ad event triggering for non-Rewards users should be carefully reviewed to ensure they don't introduce any privacy leaks or unintended data collection.
The modifications to URL parsing and handling in various parts of the code (e.g., for verifiable conversions) should be scrutinized to prevent potential URL manipulation vulnerabilities.

Changes

Changes

account/account.cc and account/account_util.cc:
- Added logic to control deposit behavior based on user type (Rewards vs non-Rewards) and ad type.
user_engagement/ad_events/search_result_ads/:
- Split search result ad event handling into separate files for Rewards and non-Rewards users.
- Added new utility functions for handling search result ad events.
user_engagement/conversions/:
- Added new test files for different ad types (inline content, new tab page, notification, promoted content, search result).
- Implemented new logic for handling conversions based on user type and ad type.
database/database_constants.h:
- Updated database schema version to 41.
Various test files:
- Updated and added new tests to cover the changes in ad event handling and conversions.
Removed files:
- account/user_data/fixed/locale_user_data.*
- account/user_data/fixed/top_segment_user_data.*
- common/locale/country_code_anonymity_util.*
BUILD.gn:
- Updated build configuration to reflect new and removed files.

Overall, this PR represents a significant refactoring of the ad handling and conversion tracking system in Brave Ads, with a focus on improving support for non-Rewards users while maintaining functionality for Rewards users.

tmancey self-assigned this Jun 13, 2024

tmancey force-pushed the issues/38955 branch 8 times, most recently from 48d24e6 to 5a47d09 Compare June 14, 2024 00:09

tmancey force-pushed the issues/38955 branch 10 times, most recently from 7b8edc3 to a058925 Compare June 20, 2024 18:48

tmancey force-pushed the issues/38955 branch 10 times, most recently from c68c858 to 618ee9d Compare June 24, 2024 02:29

tmancey force-pushed the issues/38955 branch from 7b8ab3e to c4230b7 Compare July 16, 2024 19:25

tmancey force-pushed the issues/38955 branch from c4230b7 to ab307e5 Compare July 16, 2024 22:20

fmarier approved these changes Jul 16, 2024

View reviewed changes

ShivanKaul reviewed Jul 16, 2024

View reviewed changes

tmancey force-pushed the issues/38955 branch from ab307e5 to 9993cb2 Compare July 17, 2024 13:12

tmancey force-pushed the issues/38955 branch 2 times, most recently from 592b050 to 905f9f5 Compare July 17, 2024 14:09

fmarier reviewed Jul 17, 2024

View reviewed changes

components/brave_ads/core/internal/account/user_data/fixed/page_land_user_data.cc Outdated Show resolved Hide resolved

components/brave_ads/core/internal/user_engagement/conversions/conversions_util_internal.cc Outdated Show resolved Hide resolved

tmancey and others added 7 commits July 17, 2024 15:52

[ads] Only store essential metadata for non-Rewards users

0ba8225

[ads] Only redeem essential metadata for non-Rewards users

7176e09

[ads] Remove non-essential ad events for non-Rewards users

f073b47

[ads] Only collect essential metadata for non-Rewards users

7b149ce

Co-authored-by: Francois Marier <francois@brave.com>

[ads] Remove topSegment and countryCode from confirmation token redem…

1abe3cf

…ption payload for Rewards users

[ads] Refactor code to ensure it is not misinterpreted as returning a…

3f8a323

…ctual user data

[ads] Update README.md documentation

c0ff2b4

Co-authored-by: Francois Marier <francois@brave.com>

tmancey force-pushed the issues/38955 branch from 905f9f5 to c0ff2b4 Compare July 17, 2024 20:53

fmarier approved these changes Jul 17, 2024

View reviewed changes

ShivanKaul approved these changes Jul 17, 2024

View reviewed changes

tmancey enabled auto-merge July 17, 2024 22:32

tmancey merged commit 6a558b8 into master Jul 17, 2024
16 checks passed

tmancey deleted the issues/38955 branch July 17, 2024 22:37

github-actions bot added this to the 1.70.x - Nightly milestone Jul 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ads] Only collect, store and redeem essential metadata #24188

[ads] Only collect, store and redeem essential metadata #24188

tmancey commented Jun 13, 2024 •

edited

Loading

tmancey commented Jun 14, 2024

ShivanKaul commented Jul 16, 2024

tmancey commented Jul 16, 2024 •

edited

Loading

fmarier left a comment

ShivanKaul Jul 16, 2024 •

edited

Loading

tmancey Jul 17, 2024 •

edited

Loading

tmancey Jul 17, 2024

ShivanKaul Jul 16, 2024 •

edited

Loading

tmancey Jul 17, 2024

ShivanKaul commented Jul 16, 2024

fmarier commented Jul 17, 2024

tmancey commented Jul 17, 2024

tmancey commented Jul 17, 2024 •

edited

Loading

github-actions bot commented Jul 17, 2024

Changes

	\| user data \| {"conversion":[{"action":"click"}]} \| See [user data](../../user_data/README.md#non-brave-rewards-user). \|
	\| user_data \| {"conversion":[{"action":"click"}]} \| For non-Rewards users, this is minimal. See [user data](../../user_data/README.md#non-brave-rewards-user). \|

[ads] Only collect, store and redeem essential metadata #24188

[ads] Only collect, store and redeem essential metadata #24188

Conversation

tmancey commented Jun 13, 2024 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

Non-Brave Rewards user:

Brave Rewards user:

tmancey commented Jun 14, 2024

ShivanKaul commented Jul 16, 2024

tmancey commented Jul 16, 2024 • edited Loading

fmarier left a comment

Choose a reason for hiding this comment

ShivanKaul Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

tmancey Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

tmancey Jul 17, 2024

Choose a reason for hiding this comment

ShivanKaul Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

tmancey Jul 17, 2024

Choose a reason for hiding this comment

ShivanKaul commented Jul 16, 2024

fmarier commented Jul 17, 2024

tmancey commented Jul 17, 2024

tmancey commented Jul 17, 2024 • edited Loading

github-actions bot commented Jul 17, 2024

Description

Possible Issues

Security Hotspots

Changes

tmancey commented Jun 13, 2024 •

edited

Loading

tmancey commented Jul 16, 2024 •

edited

Loading

ShivanKaul Jul 16, 2024 •

edited

Loading

tmancey Jul 17, 2024 •

edited

Loading

ShivanKaul Jul 16, 2024 •

edited

Loading

tmancey commented Jul 17, 2024 •

edited

Loading