Adds media publishers support #11727

NejcZdovc · 2017-10-31T07:59:52Z

Fix #11851

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.

Test Plan:

Reviewer Checklist:

Tests

Adequate test coverage exists to prevent regressions
Tests should be independent and work correctly when run individually or as a suite ref
New files have MPL2 license header

bsclifton · 2017-11-03T18:44:37Z

app/browser/api/ledger.js

@@ -44,6 +44,7 @@ const request = require('../../../js/lib/request')
 const ledgerUtil = require('../../common/lib/ledgerUtil')
 const tabState = require('../../common/state/tabState')
 const pageDataUtil = require('../../common/lib/pageDataUtil')
+const ledgerVideoCahce = require('../../common/cache/ledgerVideoCache')


s/ledgerVideoCahce/ledgerVideoCache/g

bsclifton · 2017-11-03T18:47:01Z

app/browser/api/ledger.js

-  // still generate publisherURL.
-  data.publisherURL = (result.protocol || 'http:') + '//' + result.publisherKey
+
+  data.publisherURL = result.publisherURL || (result.protocol || 'http:') + '//' + result.publisherKey


would be nice to use parens to make this clear- I think you're meaning it to be:
result.publisherURL || ((result.protocol || 'http:') + '//' + result.publisherKey)

bsclifton · 2017-11-03T20:41:29Z

while testing, I got this crash. I was about 30 seconds into a video. The crash caused the browser to stop responding (I couldn't switch tabs, etc)... but the current tab behaved properly

An uncaught exception occurred in the main process Uncaught Exception:
AssertionError: state must contain ledgerVideos as Immutable.Map
    at validateState (/Users/clifton/Documents/browser-laptop/app/common/cache/ledgerVideoCache.js:12:10)
    at Object.getDataByVideoId (/Users/clifton/Documents/browser-laptop/app/common/cache/ledgerVideoCache.js:17:11)
    at Object.onMediaRequest (/Users/clifton/Documents/browser-laptop/app/browser/api/ledger.js:2495:34)
    at ledgerReducer (/Users/clifton/Documents/browser-laptop/app/browser/reducers/ledgerReducer.js:410:27)
    at reducers.reduce (/Users/clifton/Documents/browser-laptop/js/stores/appStore.js:169:24)
    at Array.reduce (<anonymous>)
    at applyReducers (/Users/clifton/Documents/browser-laptop/js/stores/appStore.js:167:68)
    at handleAppAction (/Users/clifton/Documents/browser-laptop/js/stores/appStore.js:231:16)
    at callbacks.forEach (/Users/clifton/Documents/browser-laptop/js/dispatcher/appDispatcher.js:107:7)
    at Array.forEach (<anonymous>)
    at AppDispatcher.dispatchToOwnRegisteredCallbacks (/Users/clifton/Documents/browser-laptop/js/dispatcher/appDispatcher.js:106:20)
    at AppDispatcher.dispatchInternal (/Users/clifton/Documents/browser-laptop/js/dispatcher/appDispatcher.js:132:10)
    at runCallback (timers.js:672:20)
    at tryOnImmediate (timers.js:645:5)
    at processImmediate [as _immediateCallback] (timers.js:617:5)

NejcZdovc · 2017-11-03T20:43:47Z

@bsclifton this is a new thing that I added for tests, apparently I broke something 😄

bsclifton · 2017-11-03T20:52:39Z

Interestingly enough, when that error happens (and likely the case with other events we're processing), the video stutters. We already have this captured with #5557 and #5504

NejcZdovc · 2017-11-06T09:47:47Z

@bsclifton ok tests are fixed and functionality should be working again. Please re-test

bsclifton · 2017-11-06T19:55:00Z

Happy path is working great! This is awesome

As shared on Slack, with an upgraded profile (upgraded from Bitcoin => BAT) I got the following:

AssertionError: state must contain ledgerVideos as Immutable.Map
    at validateState (/Users/clifton/Documents/browser-laptop/app/common/cache/ledgerVideoCache.js:12:10)
    at Object.getDataByVideoId (/Users/clifton/Documents/browser-laptop/app/common/cache/ledgerVideoCache.js:17:11)
    at Object.onMediaRequest (/Users/clifton/Documents/browser-laptop/app/browser/api/ledger.js:2500:34)
    at ledgerReducer (/Users/clifton/Documents/browser-laptop/app/browser/reducers/ledgerReducer.js:410:27)
    at reducers.reduce (/Users/clifton/Documents/browser-laptop/js/stores/appStore.js:169:24)
    at Array.reduce (<anonymous>)
    at applyReducers (/Users/clifton/Documents/browser-laptop/js/stores/appStore.js:167:68)
    at handleAppAction (/Users/clifton/Documents/browser-laptop/js/stores/appStore.js:231:16)
    at callbacks.forEach (/Users/clifton/Documents/browser-laptop/js/dispatcher/appDispatcher.js:107:7)
    at Array.forEach (<anonymous>)
    at AppDispatcher.dispatchToOwnRegisteredCallbacks (/Users/clifton/Documents/browser-laptop/js/dispatcher/appDispatcher.js:106:20)
    at AppDispatcher.dispatchInternal (/Users/clifton/Documents/browser-laptop/js/dispatcher/appDispatcher.js:132:10)
    at runCallback (timers.js:672:20)
    at tryOnImmediate (timers.js:645:5)
    at processImmediate [as _immediateCallback] (timers.js:617:5)

bsclifton · 2017-11-06T20:18:32Z

OK noticed a few bad ones here- it seems that:

pinning channels doesn't work
enabling / disabling channels affects all channels at once
deleting channels (trash can icon) deletes all the channels (and origin site)

bsclifton

see comments (still reviewing)

bsclifton · 2017-11-06T20:31:20Z

app/common/constants/ledgerMediaProviders.js

+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+const mapValuesByKeys = require('../../../js/lib/functional').mapValuesByKeys


I believe there is a performance impact by using this approach (using mapValuesByKeys)... Going forward, I think it might be better to just assign a value directly. For example:

const providers = { YOUTUBE: 'youtube.com', TWITCH: 'twitch.tv' //... etc ... }

cc: @bbondy

I used what we have now for other constants, but I can change it

replaced with string

bsclifton · 2017-11-06T20:33:58Z

app/common/lib/ledgerUtil.js

+  return parseInt(time)
+}
+
+const isMediaProvider = (url) => {


Would this be better named as getMediaProvider?

bsclifton · 2017-11-06T20:34:34Z

app/common/lib/ledgerUtil.js

+  return duration
+}
+
+const getYoutubeDuration = (data) => {


nit: to respect the brand name, it should be YouTube (ex: getYouTubeDuration)

bsclifton · 2017-11-06T20:50:40Z

If I play a few videos at once, the time for each is recorded (even if it's not the active tab). I think this is OK, however the ones which are not active have their view count incremented each time the heartbeat comes in

NejcZdovc · 2017-11-07T04:47:31Z

app/sessionStore.js

@@ -926,7 +926,8 @@ module.exports.defaultAppState = () => {
    },
    cache: {
      bookmarkLocation: undefined,
-      bookmarkOrder: {}
+      bookmarkOrder: {},
+      ledgerVideos: {}


We need to add deep merge for state migration. Suggestion by @bsclifton https://github.com/KyleAMathews/deepmerge

will add it after PR will be confirmed

diracdeltas · 2017-11-09T18:03:44Z

This should not be merged without security review, so blocked on https://github.com/brave/internal/issues/141

diracdeltas · 2017-11-09T22:24:24Z

app/browser/api/ledger.js

+  // media publisher
+  if (result.faviconName) {
+    const on = locale.translation('on').toLowerCase()
+    data.siteName = `${result.faviconName} ${on} ${result.providerName}`


[minor] this might cause the wrong word order in some languages. i think we support l10n parameters to solve this problem, but maybe not in the locale module

diracdeltas · 2017-11-09T22:43:37Z

app/browser/api/ledger.js

-  // still generate publisherURL.
-  data.publisherURL = (result.protocol || 'http:') + '//' + result.publisherKey
+
+  data.publisherURL = result.publisherURL || ((result.protocol || 'http:') + '//' + result.publisherKey)


should probably use https instead of http by default since that is what https://github.com/brave-intl/bat-publisher/blob/master/index.js#L100 does

diracdeltas · 2017-11-09T22:49:18Z

app/browser/api/ledger.js

@@ -853,7 +860,7 @@ const excludeP = (publisherKey, callback) => {
      return done(err, result)
    }

-    let props = ledgerPublisher.getPublisherProps('https://' + publisherKey)
+    let props = ledgerPublisher.getPublisherProps(publisherKey)


where is publisherKey ultimately generated? just making sure it has some kind of input validation

we get it from bat-publisher library https://github.com/brave/browser-laptop/pull/11727/files#diff-3cdfdb124f718787d584e9e2b82fcedcR2505, with ledgerPublisher.getMedia.getPublisherFromMediaProps

in https://github.com/brave-intl/bat-publisher/blob/d896a93465df57b3cca227b90530f0b40044d5fc/getMedia.js#L91, where does author_url come from?

In https://github.com/brave-intl/bat-publisher/blob/d896a93465df57b3cca227b90530f0b40044d5fc/getMedia.js#L77 we call publisher oembed, which is defined here https://github.com/brave-intl/bat-publisher/blob/d896a93465df57b3cca227b90530f0b40044d5fc/media/providers.json#L7 (for youtube is https://www.youtube.com/oembed?url=https://www.youtube.com/watch?v=kLiLOkzLetE) and we get back something like this

{"html":"\u003ciframe width=\"480\" height=\"270\" src=\"https:\/\/www.youtube.com\/embed\/kLiLOkzLetE?feature=oembed\" frameborder=\"0\" gesture=\"media\" allowfullscreen\u003e\u003c\/iframe\u003e","title":"This Is Brave","thumbnail_height":360,"version":"1.0","provider_name":"YouTube","thumbnail_width":480,"width":480,"author_name":"Brave","author_url":"https:\/\/www.youtube.com\/channel\/UCFNTTISby1c_H-rm5Ww5rZg","height":270,"thumbnail_url":"https:\/\/i.ytimg.com\/vi\/kLiLOkzLetE\/hqdefault.jpg","provider_url":"https:\/\/www.youtube.com\/","type":"video"}

which contains author_url

i think https://github.com/brave-intl/bat-publisher/blob/d896a93465df57b3cca227b90530f0b40044d5fc/getMedia.js#L91 needs to validate that author_url is a valid URL. this is easy because if not then parts will be null

https://github.com/brave-intl/bat-publisher/blob/d896a93465df57b3cca227b90530f0b40044d5fc/index.js#L101 should also return immediately if props is null

these last two are fixed in bat-publisher@1.2.1 and bat-client@1.4.2

versions updated in this PR

diracdeltas · 2017-11-09T22:58:03Z

app/browser/api/ledger.js

-      ctx.RLD = tldjs.getSubdomain(ctx.host)
-      ctx.QLD = ctx.RLD ? underscore.last(ctx.RLD.split('.')) : ''
+      if (publisher.publisherURL) ctx.URL = publisher.publisherURL
+      if (!ctx.URL) ctx.URL = (publisher.get('protocol') || 'http:') + '//' + publisherKey


same as above for 'https' vs 'http'

diracdeltas · 2017-11-09T23:18:32Z

app/browser/api/ledger.js

+    providerName: type
+  }
+
+  ledgerPublisher.getMedia.getPublisherFromMediaProps(mediaProps, options, (error, response) => {


bat-publisher should throw an error if the media URL protocol is not "https:"

it appears bat-publisher is making a call to a youtube.com page from the browser main process, scraping and parsing the page HTML, and extracting page data from it. from a security perspective, it would be much better to do this in a page content script that runs when the user visits a youtube page (ex: how we currently extract favicons from publisher pages). https://github.com/brave-intl/bat-publisher/blob/d896a93465df57b3cca227b90530f0b40044d5fc/getMedia.js#L103

cc @mrose17

bat-publisher is being passed a URL that the browser is playing. whether it is http? is immaterial. the goal is to look at the URL string and figure out where to look for the oembed data.

i think that this is a worthy future, but not essential at this point. it is certainly good to have routines that parse data run in a separate context. it's not clear to me why that is "more necessary" in this case. we can open up an issue for this, and when we do, it would be worthwhile to see what other cases might exist!

is an easy change and should be done for paranoia (AKA security in depth) in case there's ever code that looks at the URL and fetches it.

I've opened a follow up issue for 2: #11889

@mrose17 I think comment 1 is still unaddressed? I'd also strongly prefer we address 2 before moving on. The follow up issue seems to be to address the general case, but no reason for us to not adhere to the correct practices now.

@mrose17 I'm going to hold off on merge until @evq 's concern can be addressed (and we have a sign off in https://github.com/brave/internal/issues/141). Let us know if we need to update libs after that is addressed

is addressed. we aren't opening a URL, we are parsing it in order to find an oembed provider. if the browser decides to play it and then ask for information, then the module will provide the information. however, in the interest of moving things along, i have pushed bat-publisher@1.2.3 that makes this change.

it is not a simple thing to make this change (it has to be made in the browser). nor am i convinced that it makes sense to require this code be held to a standard higher than any other code in the browser. as i noted in my comment.

everything has a cost. i suppose we can address 1, because it's a small change, even if i do view it as unnecessary. in addressing 2, it has a large cost (i already researched it with @NejcZdovc) and addresses a theoretical concern. there is significant cost to that we will not be able to merge this upcoming week. i don't have an issue with the browser folks reviewing the issue that @diracdeltas created after this merge, and having the same standard applied to the entire browser. i most certainly have an issue having this new standard being applied in this context and in this timeline.

diracdeltas · 2017-11-09T23:25:47Z

package.json

    "bignumber.js": "^4.0.4",
    "bloodhound-js": "brave/bloodhound",
    "clipboard-copy": "^1.0.0",
    "compare-versions": "^3.0.1",
+    "deepmerge": "^2.0.1",


i don't see anything obviously wrong with deepmerge but fwiw we use lodash.merge in sync. i wonder if @ayumi knows why?

no particular reason. lodash seemed well maintained.

we can switch to loadash as well

either way is fine

diracdeltas · 2017-11-10T04:45:29Z

There's a security issue in https://github.com/brave-intl/bat-publisher/blob/master/getMedia.js#L55

if (!schemes.length) return (mediaURL.indexOf(rule.domain) !== -1) ...

say that rule.domain is youtube.com, then youtube.com.evil.com would match for it

Resolves brave#11851 Auditors: Test Plan:

NejcZdovc · 2017-11-10T08:25:41Z

app/browser/api/ledger.js

@@ -107,6 +110,38 @@ underscore.keys(fileTypes).forEach((fileType) => {
 })
 signatureMax = Math.ceil(signatureMax * 1.5)

+if (ipc) {


Accidentally removed in #11882

@diracdeltas

good catch from @diracdeltas on brave/browser-laptop#11727 (review)

@bbondy

Auditors: @bbondy @bsclifton

@mrose17

Addressing feedback found in security review (thanks, @mrose17) brave#11727 (comment) Auditors: @evq, @NejcZdovc

Adds media publishers support

bsclifton · 2017-11-12T06:08:14Z

master fafa483
0.21.x 466cc7f
0.20.x 41e4327
0.19.x @NejcZdovc I need your help merging this one 😄

Adds media publishers support

NejcZdovc · 2017-11-12T12:05:58Z

@bsclifton merged in 0.19 2d55627

NejcZdovc added feature/rewards PR/work-in-progress ⚒ labels Oct 31, 2017

NejcZdovc added this to the 0.19.x Hotfix 4 (Release channel) milestone Oct 31, 2017

NejcZdovc self-assigned this Oct 31, 2017

NejcZdovc force-pushed the feature/youtube-ledger branch 5 times, most recently from 9dd73af to 298c7b5 Compare November 3, 2017 05:11

bsclifton reviewed Nov 3, 2017

View reviewed changes

NejcZdovc force-pushed the feature/youtube-ledger branch from 6fc982e to efa3dd0 Compare November 3, 2017 19:20

NejcZdovc removed the PR/work-in-progress ⚒ label Nov 3, 2017

NejcZdovc added PR/work-in-progress ⚒ and removed PR/work-in-progress ⚒ labels Nov 4, 2017

NejcZdovc force-pushed the feature/youtube-ledger branch from efa3dd0 to d3fb149 Compare November 6, 2017 09:48

NejcZdovc added the PR/pending-review label Nov 6, 2017

bsclifton suggested changes Nov 6, 2017

View reviewed changes

bsclifton reviewed Nov 6, 2017

View reviewed changes

NejcZdovc commented Nov 7, 2017

View reviewed changes

NejcZdovc force-pushed the feature/youtube-ledger branch from d3fb149 to 6ccd7fb Compare November 7, 2017 04:58

srirambv mentioned this pull request Nov 9, 2017

Shields menu cover entire browser view when clicked on view count #11878

Closed

diracdeltas added the security label Nov 9, 2017

NejcZdovc added the PR/blocked label Nov 9, 2017

diracdeltas reviewed Nov 9, 2017

View reviewed changes

diracdeltas mentioned this pull request Nov 9, 2017

restrict origins that brave app proccess can connect to #11889

Closed

diracdeltas reviewed Nov 9, 2017

View reviewed changes

NejcZdovc requested a review from diracdeltas November 10, 2017 04:39

NejcZdovc force-pushed the feature/youtube-ledger branch from 6594a55 to 7eae0b7 Compare November 10, 2017 04:40

Adds media publishers support

476b2f7

Resolves brave#11851 Auditors: Test Plan:

NejcZdovc force-pushed the feature/youtube-ledger branch from 7eae0b7 to 7e7ca0f Compare November 10, 2017 08:24

NejcZdovc commented Nov 10, 2017

View reviewed changes

mrose17 added a commit to brave-intl/bat-publisher that referenced this pull request Nov 10, 2017

1.2.2: better test for domain matching for providers ...

dc8f2b6

good catch from @diracdeltas on brave/browser-laptop#11727 (review)

Adds deep merge for state migration

cf2e3b2

Auditors: @bbondy @bsclifton

NejcZdovc force-pushed the feature/youtube-ledger branch from 7e7ca0f to cf2e3b2 Compare November 11, 2017 07:47

Update bat-publisher to 1.2.3

0f82df2

Addressing feedback found in security review (thanks, @mrose17) brave#11727 (comment) Auditors: @evq, @NejcZdovc

bsclifton merged commit fafa483 into brave:master Nov 12, 2017

bsclifton removed PR/blocked PR/pending-review labels Nov 12, 2017

bsclifton added a commit that referenced this pull request Nov 12, 2017

Merge pull request #11727 from NejcZdovc/feature/youtube-ledger

466cc7f

Adds media publishers support

bsclifton added a commit that referenced this pull request Nov 12, 2017

Merge pull request #11727 from NejcZdovc/feature/youtube-ledger

41e4327

Adds media publishers support

NejcZdovc pushed a commit that referenced this pull request Nov 12, 2017

Merge pull request #11727 from NejcZdovc/feature/youtube-ledger

2d55627

Adds media publishers support

tildelowengrimm mentioned this pull request Apr 17, 2018

Whitelist origins accessed from main processes brave/brave-browser#163

Closed

Adds media publishers support #11727

Adds media publishers support #11727

Conversation

NejcZdovc commented Oct 31, 2017 • edited by bsclifton Loading

bsclifton Nov 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsclifton commented Nov 3, 2017 • edited Loading

NejcZdovc commented Nov 3, 2017 • edited Loading

bsclifton commented Nov 3, 2017 • edited Loading

NejcZdovc commented Nov 6, 2017

bsclifton commented Nov 6, 2017 • edited by NejcZdovc Loading

bsclifton commented Nov 6, 2017 • edited by NejcZdovc Loading

bsclifton left a comment

Choose a reason for hiding this comment

bsclifton Nov 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsclifton commented Nov 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diracdeltas commented Nov 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NejcZdovc Nov 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diracdeltas Nov 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diracdeltas commented Nov 10, 2017

Choose a reason for hiding this comment

bsclifton commented Nov 12, 2017

NejcZdovc commented Nov 12, 2017

NejcZdovc commented Oct 31, 2017 •

edited by bsclifton

Loading

bsclifton Nov 3, 2017 •

edited

Loading

bsclifton commented Nov 3, 2017 •

edited

Loading

NejcZdovc commented Nov 3, 2017 •

edited

Loading

bsclifton commented Nov 3, 2017 •

edited

Loading

bsclifton commented Nov 6, 2017 •

edited by NejcZdovc

Loading

bsclifton commented Nov 6, 2017 •

edited by NejcZdovc

Loading

bsclifton Nov 6, 2017 •

edited

Loading

diracdeltas commented Nov 9, 2017 •

edited

Loading

NejcZdovc Nov 10, 2017 •

edited

Loading

diracdeltas Nov 9, 2017 •

edited

Loading