Skip to content
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

Brave Ads adaptive captcha support #9350

Merged
merged 15 commits into from
Sep 3, 2021
Merged

Brave Ads adaptive captcha support #9350

merged 15 commits into from
Sep 3, 2021

Conversation

emerick
Copy link
Contributor

@emerick emerick commented Jul 7, 2021

Resolves brave/brave-browser#15600

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • 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, npm run lint, 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:

Test Plan:

Setup

When testing on the staging server, run Brave with the following flags (plus whatever else you may need for testing):

brave.exe --brave-ads-staging --rewards=staging=true

You'll need to schedule a captcha for a particular payment ID. To do that, you can issue the following command from your command line, where {paymentId} represents the payment ID (wallet ID) as identified in brave://rewards-internals:

curl -XPUT -H "Authorization: Bearer {authToken}" -H "Content-Type: application/json" https://reputation.rewards.bravesoftware.com/v3/captcha/challenge/{paymentId}?type={0|1}

Please ping me via DM for the {authToken} required above.

The type parameter allows you to specify the type of captcha to schedule. This may be either 0 or 1 (the default is 0 if this parameter is omitted).

Note: Any time that you successfully solve a captcha for this payment ID, you'll need to schedule a new one using this command.

Solving the captcha

  • Clean profile
  • Enable Rewards
  • Schedule a captcha for your payment ID (see Setup section above)
  • Wait for Ads unblinded tokens to refill
  • When captcha notification appears, click Solve to open the Rewards panel and show the captcha
  • Solve the captcha
  • Ensure that captcha goes away
  • Ensure that tokens refill as expected during the next Ads cycle

Snoozing the captcha

  • Clean profile
  • Enable Rewards
  • Schedule a captcha for your payment ID (see Setup section above)
  • Wait for Ads unblinded tokens to refill
  • When captcha notification appears, click Later to snooze the captcha (this option is only allowed once per captcha)
  • Open the Rewards panel to confirm there is no captcha shown
  • Ensure that no ads are shown from this point forward until you successfully solve the captcha below
  • Ensure that the next time Ads tries to refill tokens, the captcha notification displays without the "Later" option
  • Solve the captcha
  • Ensure that captcha goes away
  • Ensure that tokens refill as expected during the next Ads cycle

@emerick emerick self-assigned this Jul 7, 2021
@emerick emerick force-pushed the adaptive-captcha branch 12 times, most recently from b2421af to 8388270 Compare July 9, 2021 15:30
@emerick emerick force-pushed the adaptive-captcha branch 15 times, most recently from b3a328d to f298115 Compare July 23, 2021 23:32
@emerick emerick force-pushed the adaptive-captcha branch 2 times, most recently from f87259b to 74f4fdf Compare July 26, 2021 20:39

#include <string>

#include "brave/components/brave_ads/browser/ads_tooltips_delegate.h"
Copy link
Collaborator

Choose a reason for hiding this comment

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

You're including the file as a header for itself

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, removed.

if (brave_adaptive_captcha_enabled) {
sources += [ "ads_tooltips_delegate.h" ]
deps +=
[ "//brave/components/brave_adaptive_captcha:brave_adaptive_captcha" ]
Copy link
Collaborator

Choose a reason for hiding this comment

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

this file does not need a dep on brave_adaptive_captcha

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

visibility = [ ":*" ]
visibility = [
":*",
"//brave/vendor/bat-native-ads:ads",
Copy link
Collaborator

Choose a reason for hiding this comment

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

these visibility lists exist to prevent people from doing this. This config is accessed as a public config on headers

Copy link
Collaborator

Choose a reason for hiding this comment

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

we probably want to split these defines out into a separate config that is allowed from //brave/vendor/bat-native-ads:ads

Copy link
Collaborator

Choose a reason for hiding this comment

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

they really shouldn't be in a public config anyway because GEMINI_OAUTH_STAGING_URL and BITFLYER_STAGING_URL aren't used outside of bat-native-ledger. The strictly internal ones above should go in the internal config and the new ones should go in a new config that has the :* and //brave/vendor/bat-native-ads:ads

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I think I got misled a bit by the name external_config. Moved these into endpoints_config.

configs += [ ":internal_config" ]
configs += [
":internal_config",
"//brave/vendor/bat-native-ledger:external_config",
Copy link
Collaborator

Choose a reason for hiding this comment

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

see above

config("endpoints_config") {
visibility = [
":*",
"//brave/components/brave_adaptive_captcha:brave_adaptive_captcha",
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this needed in more places now? It can still be a public config

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made it a public config.

@emerick emerick force-pushed the adaptive-captcha branch 2 times, most recently from e3ab1a7 to 6f7b730 Compare September 2, 2021 23:56
@emerick
Copy link
Contributor Author

emerick commented Sep 3, 2021

All CI passed (Windows had an unrelated test failure).

@emerick emerick merged commit def5d8a into master Sep 3, 2021
@emerick emerick deleted the adaptive-captcha branch September 3, 2021 19:51
@emerick emerick added this to the 1.31.x - Nightly milestone Sep 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New Ads Auth Refresh Flow
7 participants