Add support for referral promos #428

emerick · 2018-09-10T14:45:50Z

Fixes brave/brave-browser#287

Also relies on brave/brave-browser#1244

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).
Tagged reviewers and labelled the pull request as needed.
Request a security/privacy review as needed.
Add appropriate QA labels (QA/Needed or QA/No-QA-Needed) to include the closed issue in milestone

Automated Test Plan

npm run test -- brave_unit_tests --filter=BraveReferralsNetworkDelegateHelperTest.*

Test Plan:

Install a version of Brave with an associated referral code in its filename (on Mac, this requires running the .pkg installer not the .dmg)
- Verify that after installation but before running Brave for the first time, promoCode file is in user-data-directory and contains the appropriate code. Also verify that this works as expected when the installer is run from a folder containing spaces.
Run installed Brave
- Verify that promo-appropriate URL (if any) opens in a new browser tab.
- Verify that when visiting a referral domain, we send the promo-appropriate request headers.
- Verify that we no longer send referral code to update server after 90 days.
- Verify that promoCode file is deleted after promo code is read and stored in local_state preferences.
- Verify that service checks for referral finalization 30 days after referral initialization.
- Verify that service retries checking for referral finalization up to 30 times (with 24-hour pauses between attempts) if it doesn't receive "true" on its first try.
- Verify that stats.brave.com includes referral code when appropriate.

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

emerick · 2018-09-10T18:22:23Z

patches/chrome-installer-mini_installer-mini_installer.cc.patch

+   // on to setup.exe
+   AppendCommandLineFlags(configuration.command_line(), &cmd_line);
+
+#if defined(BRAVE_CHROMIUM_BUILD)


I wanted to implement this via a chromium_src override, but RunSetup is both defined and called in this file so it wasn't working correctly. My other thought was to provide our own implementation of CreateProcess, which is called exactly once in this file and has the argument that I need to override.

bbondy · 2018-09-22T13:32:20Z

@emerick can you do a brain dump on where this is at? Thanks!

After parsing the referral code, we pass it to setup.exe via the --brave-referral-code flag. The setup program will write the referral code out to user-data-dir.

The mini installer passes the referral code to setup via the --brave-referral-code command line option

When run against the release channel, this creates a signed install pkg that parses a referral code (if any) from its filename and writes it to user-data-dir. When run against any other channel, no pkg is created.

Retrieving the time is a blocking operation and will trigger an assertion on the main thread. Instead, post a task to retrieve the first run time and when it completes post tasks back to the main thread to perform associated bookkeeping (these need to run on the main thread as they interact with the preference service).

diracdeltas

non-security/privacy question: are we planning on migrating referral codes from muon brave to brave-core?

diracdeltas · 2018-09-24T23:40:06Z

browser/referrals/brave_referrals_service.h

+  std::unique_ptr<network::SimpleURLLoader> referral_headers_loader_;
+  std::unique_ptr<network::SimpleURLLoader> referral_init_loader_;
+  std::unique_ptr<network::SimpleURLLoader> referral_finalization_check_loader_;
+  std::unique_ptr<base::RepeatingTimer> fetch_referral_headers_timer_;


[Privacy review] We should add a randomized delay to the timing so as to prevent the server from being able to correlate these requests as being from the same user. The same goes for all Brave requests that occur on a regular interval.

Makes sense, I'll get that in there.

Added 0649d184e33c070625c2a537d70a95ec662100ef to address this.

diracdeltas · 2018-09-24T23:42:54Z

build/mac/pkg-scripts/postinstall

+# (who may not be an admin).
+sudo chmod -R 775 "$installationAppPath"
+sudo chown -R $userName "$installationAppPath"
+sudo chgrp -R admin "$installationAppPath"


[Security review] How does this compare to Chrome's directory permissions? In general we want to match that (or be more restrictive)

This is how Chrome installs in my /Applications folder on Mac:

$ ls -l | grep Chrome drwxrwxr-x@ 3 emerick admin 96 Sep 15 04:30 Google Chrome.app/

So I think we're exactly matching the permissions/owner/group of Chrome.

That is correct, as far as I know- @petemill did the initial work on this (browser-laptop) and I remember digging in with him. We wanted to match Chrome

diracdeltas · 2018-09-24T23:49:22Z

browser/net/brave_static_redirect_network_delegate_helper.cc

@@ -35,6 +35,8 @@ int OnBeforeURLRequest_StaticRedirectWork(
  static std::vector<URLPattern> allowed_patterns({
    // Brave updates
    URLPattern(URLPattern::SCHEME_HTTPS, "https://go-updater.brave.com/*"),
+    // Brave updates staging
+    URLPattern(URLPattern::SCHEME_HTTPS, "https://laptop-updates-staging.herokuapp.com/*"),


Not a big deal since this is just for debugging right now, but can we add a way to restrict whitelisting of staging endpoints to non-prod builds?

The only problem is that we also rely on this URL for unit tests, but maybe we can add a define for that.

Actually, now that I'm looking closer at this code I noticed that it's wrapped in the following:

#if !defined(NDEBUG) #endif

So it will only be called in debug versions. There's a comment at the end of the block to turn this into a DCHECK eventually, but that would also mean this block is compiled out in release builds.

We normally fetch every 24 hours, this will add a random delay to prevent the server from being able to correlate these requests as coming from the same user.

bsclifton

Feedback addressed; LGTM! Re-approving 😄

@evq

comments addressed. @evq will take a quick look but lgtm otherwise.

bsclifton · 2018-09-26T04:47:00Z

master 09572b2
0.55.x 0cbf1b1

Add support for referral promos

bbondy · 2018-09-26T05:15:27Z

WHOOOOOOOOO!!!! 🎉

diracdeltas · 2018-09-26T06:16:34Z

q i had which i don't think was addressed: what is the plan for migrating referred users from b-l-b? for instance if someone had downloaded a Dow Jones promo build and then updates to brave-core, do their promo benefits continue?

emerick · 2018-09-26T13:01:59Z

@diracdeltas That's a really good question. If we do want to migrate, I think we could import the local_state settings without too much trouble as I followed the b-l-b state settings pretty closely.

@bsclifton Any thoughts here?

bsclifton · 2018-09-26T18:46:12Z

@diracdeltas good question- I captured brave/brave-browser#1294 to address this 😄

emerick added the pr/work-in-progress label Sep 10, 2018

emerick self-assigned this Sep 10, 2018

emerick commented Sep 10, 2018

View reviewed changes

emerick force-pushed the referral-promo-support branch from 727758e to 08a5167 Compare September 14, 2018 18:55

emerick closed this Sep 15, 2018

emerick force-pushed the referral-promo-support branch from 718b5af to 84a4955 Compare September 15, 2018 12:12

emerick reopened this Sep 15, 2018

emerick force-pushed the referral-promo-support branch 13 times, most recently from 31ce3cf to f713eb1 Compare September 21, 2018 16:57

emerick requested a review from bbondy September 21, 2018 17:05

emerick force-pushed the referral-promo-support branch 5 times, most recently from 7fdbb2a to bb8787f Compare September 21, 2018 23:07

emerick closed this Sep 21, 2018

emerick force-pushed the referral-promo-support branch from bb8787f to 87d5e6f Compare September 21, 2018 23:22

emerick reopened this Sep 21, 2018

emerick dismissed bsclifton’s stale review via 7780d5c September 24, 2018 20:14

emerick force-pushed the referral-promo-support branch from 2346225 to 7780d5c Compare September 24, 2018 20:14

emerick added 9 commits September 24, 2018 19:11

On Windows, parse referral code from mini installer's filename

59d53b5

After parsing the referral code, we pass it to setup.exe via the --brave-referral-code flag. The setup program will write the referral code out to user-data-dir.

On Windows, write referral code out to user-data-dir

b3f3354

The mini installer passes the referral code to setup via the --brave-referral-code command line option

On Mac, create an install pkg to parse referral code

021a3e6

When run against the release channel, this creates a signed install pkg that parses a referral code (if any) from its filename and writes it to user-data-dir. When run against any other channel, no pkg is created.

Set custom referral headers when domain matches

df9f6fc

Delete promo code preference after 90 days

2478e82

Pass BRAVE_REFERRALS_API_KEY from config when building and use it

3aebb6b

Add unit tests for referrals network delegate

8fc5ea8

Validate response_body before attempting to log its contents

4bd8eba

emerick force-pushed the referral-promo-support branch from 7780d5c to a0c1a7e Compare September 24, 2018 23:12

diracdeltas previously requested changes Sep 24, 2018

View reviewed changes

diracdeltas reviewed Sep 24, 2018

View reviewed changes

emerick added 2 commits September 25, 2018 11:58

Add laptop-updates-staging to network delegate whitelist

a52d415

Add a randomized 0-10 minute delay when fetching referral headers

c87b146

We normally fetch every 24 hours, this will add a random delay to prevent the server from being able to correlate these requests as coming from the same user.

emerick force-pushed the referral-promo-support branch from 0649d18 to c87b146 Compare September 25, 2018 15:58

bsclifton approved these changes Sep 25, 2018

View reviewed changes

bsclifton merged commit 09572b2 into master Sep 26, 2018

bsclifton deleted the referral-promo-support branch September 26, 2018 04:42

bsclifton added a commit that referenced this pull request Sep 26, 2018

Merge pull request #428 from brave/referral-promo-support

0cbf1b1

Add support for referral promos

bsclifton added the merged/0.55.x label Sep 26, 2018

bsclifton mentioned this pull request Sep 26, 2018

Import promoCode from browser-laptop when upgrading to brave-core brave/brave-browser#1294

Closed

bbondy added this to the 0.55.x - Release milestone Jan 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for referral promos #428

Add support for referral promos #428

emerick commented Sep 10, 2018 •

edited

Loading

emerick Sep 10, 2018

bbondy commented Sep 22, 2018 •

edited

Loading

diracdeltas left a comment

diracdeltas Sep 24, 2018

emerick Sep 25, 2018

emerick Sep 25, 2018

diracdeltas Sep 24, 2018

emerick Sep 25, 2018

bsclifton Sep 25, 2018 •

edited

Loading

diracdeltas Sep 24, 2018 •

edited

Loading

emerick Sep 25, 2018

emerick Sep 25, 2018 •

edited

Loading

bsclifton left a comment

bsclifton commented Sep 26, 2018 •

edited

Loading

bbondy commented Sep 26, 2018

diracdeltas commented Sep 26, 2018

emerick commented Sep 26, 2018

bsclifton commented Sep 26, 2018

Add support for referral promos #428

Add support for referral promos #428

Conversation

emerick commented Sep 10, 2018 • edited Loading

Submitter Checklist:

Automated Test Plan

Test Plan:

Reviewer Checklist:

emerick Sep 10, 2018

Choose a reason for hiding this comment

bbondy commented Sep 22, 2018 • edited Loading

diracdeltas left a comment

Choose a reason for hiding this comment

diracdeltas Sep 24, 2018

Choose a reason for hiding this comment

emerick Sep 25, 2018

Choose a reason for hiding this comment

emerick Sep 25, 2018

Choose a reason for hiding this comment

diracdeltas Sep 24, 2018

Choose a reason for hiding this comment

emerick Sep 25, 2018

Choose a reason for hiding this comment

bsclifton Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

diracdeltas Sep 24, 2018 • edited Loading

Choose a reason for hiding this comment

emerick Sep 25, 2018

Choose a reason for hiding this comment

emerick Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

bsclifton left a comment

Choose a reason for hiding this comment

bsclifton commented Sep 26, 2018 • edited Loading

bbondy commented Sep 26, 2018

diracdeltas commented Sep 26, 2018

emerick commented Sep 26, 2018

bsclifton commented Sep 26, 2018

emerick commented Sep 10, 2018 •

edited

Loading

bbondy commented Sep 22, 2018 •

edited

Loading

bsclifton Sep 25, 2018 •

edited

Loading

diracdeltas Sep 24, 2018 •

edited

Loading

emerick Sep 25, 2018 •

edited

Loading

bsclifton commented Sep 26, 2018 •

edited

Loading