brave-core icon indicating copy to clipboard operation
brave-core copied to clipboard

[ads] Send HTTP response status code for landed events as part of the confirmation token redemption for Rewards users

Open tmancey opened this issue 11 months ago • 1 comments

Resolves https://github.com/brave/brave-browser/issues/36752

Submitter Checklist:

  • [x] I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • [x] There is a ticket for my issue
  • [x] Used Github auto-closing keywords in the PR description above
  • [x] Wrote a good PR/commit description
  • [x] Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • [x] Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • [x] 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
  • [x] 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:
    • [ ] https://github.com/brave/brave-browser/wiki/Deviations-from-Chromium-(features-we-disable-or-remove)
    • [ ] https://github.com/brave/brave-browser/wiki/Proxy-redirected-URLs
    • [ ] https://github.com/brave/brave-browser/wiki/Fingerprinting-Protections
    • [ ] https://github.com/brave/brave-browser/wiki/Brave%E2%80%99s-Use-of-Referral-Codes
    • [ ] https://github.com/brave/brave-browser/wiki/Web-Compatibility-Exceptions-in-Brave
    • [ ] https://github.com/brave/brave-browser/wiki/QA-Guide
    • [ ] https://github.com/brave/brave-browser/wiki/P3A

Test Plan:

Confirm http response status code is sent in confirmation token redemptions for page lands if the target url fails to load such as 404 NOT FOUND etc.

tmancey avatar Mar 13 '24 17:03 tmancey

Draft until we rebase once https://github.com/brave/brave-core/pull/22574 has been merged

tmancey avatar Mar 13 '24 18:03 tmancey

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

Description

This PR updates the Brave Ads code to include additional user data when handling page land events. Specifically, it adds an is_error_page boolean flag to the NotifyTabDidChange method to indicate if the page load resulted in an error. This flag replaces the previous http_response_status_code parameter. The user data sent with page land events now includes an "errorPage" key if is_error_page is true.

Changes

Changes

browser/brave_ads/tabs/ads_tab_helper.cc

  • Removes http_response_status_code_ member variable
  • Adds is_error_page_ member variable
  • Updates DidFinishNavigation to set is_error_page_ based on the response code class or if it's an error page
  • Updates TabUpdated to pass is_error_page_ instead of http_response_status_code_

browser/brave_ads/tabs/ads_tab_helper.h

  • Replaces http_response_status_code_ with is_error_page_

components/brave_ads/browser/ads_service.h

  • Updates NotifyTabDidChange to take is_error_page instead of http_response_status_code

components/brave_ads/*

  • Updates mocks, interfaces and implementations to pass is_error_page in NotifyTabDidChange

components/brave_ads/core/internal/ad_units/user_data/page_land_user_data.cc

  • Adds new file to build user data for page land events
  • Sets "httpResponseStatusKey" to "errorPage" if is_error_page is true

ios/*

  • Updates iOS code to pass is_error_page instead of http_response_status_code

Security Hotspots

None identified. The changes do not introduce any new untrusted input and the page error status seems to be determined securely on the browser side before being passed to the Brave Ads code.

github-actions[bot] avatar Mar 22 '24 09:03 github-actions[bot]