Some verified publishers no longer remove green tick at sublevels - Fixes issue #10484 #13675

jasonrsadler · 2018-03-31T02:26:39Z

Fixes #10484

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. (Ask a Brave employee to help if you cannot access this document.)

Test Plan:

Reviewer Checklist:

Request a security/privacy review as needed if one was not already requested.

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

I could only get this to repro on the brianbondy site. All other verified pubs didn't have this problem.

Regardless the addition allows the brianbondy sub levels to show green tick

Review Requested @bsclifton
Security Review Requested @diracdeltas

codecov-io · 2018-03-31T02:47:30Z

Codecov Report

Merging #13675 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #13675      +/-   ##
==========================================
- Coverage   56.45%   56.44%   -0.01%     
==========================================
  Files         283      283              
  Lines       28967    28982      +15     
  Branches     4803     4807       +4     
==========================================
+ Hits        16353    16360       +7     
- Misses      12614    12622       +8

Flag	Coverage Δ
#unittest	`56.44% <100%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
app/common/state/ledgerState.js	`85.53% <100%> (+0.8%)`	⬆️
.../renderer/components/navigation/publisherToggle.js	`88.4% <100%> (ø)`	⬆️
js/stores/appStoreRenderer.js	`91.66% <0%> (-8.34%)`	⬇️
app/renderer/components/reduxComponent.js	`57.75% <0%> (-3.45%)`	⬇️
js/stores/windowStore.js	`28.46% <0%> (-0.3%)`	⬇️

jasonrsadler · 2018-04-03T01:09:14Z

Recommend leaving this until after #12272

NejcZdovc · 2018-04-04T10:38:40Z

@jasonrsadler can you please add test for this one?

jasonrsadler · 2018-04-04T19:27:28Z

@NejcZdovc I've never been able to get the tests to run successfully. Can you tell me what I'm doing wrong. This is on a win10 box. I do npm run watch-test and then npm run test -- --grep="verified". Here's the test:

edited for brevity. webdriver tests not required

NejcZdovc · 2018-04-04T20:19:42Z

@jasonrsadler I was thinking about unit test for function getLocationProp. You can run unit test with npm run unittest. We don't need webdriver test for this one

jasonrsadler · 2018-04-04T20:53:10Z

Thanks for clarifying.

jasonrsadler · 2018-04-05T02:21:05Z

@NejcZdovc, can you tell me what I'm doing wrong?:

it('verified publisher', function () {
      const url = 'https://www.duckduckgo.com'
      const prop = 'publisher'
      const state = defaultState.setIn(['ledger', 'locations', url, prop], 'duckduckgo.com')
      const result = ledgerState.getLocationProp(state, url, prop)
      assert.equal(result, 'duckduckgo.com')
    })

result is always coming back undefined

NejcZdovc · 2018-04-05T04:03:37Z

@jasonrsadler you are getting undefined, because url.substring(0, url.indexOf('/', 8) + 1) is returning empty string and because of that getIn is not returning anything. If I understand code correctly what you want to achieve is to change long url to domain only url. Is that correct?

jasonrsadler · 2018-04-05T14:33:07Z

smacks forehead
Thanks

jasonrsadler · 2018-04-05T17:05:14Z

@NejcZdovc, would you let me know if this provides adequate coverage? Codecov doesn't seem to be running.

Thanks.

NejcZdovc · 2018-04-06T05:58:12Z

@jasonrsadler can you please add tests for shortenUrlToDomainTLD function as well? Thank you

jasonrsadler · 2018-04-06T11:02:28Z

@NejcZdovc, How is this?

NejcZdovc · 2018-04-06T12:01:52Z

js/lib/urlutil.js

+
+  shortenUrlToDomainTLD: (url) => {
+    if (url !== null) {
+      var hasProtocol = url.includes('://')


please replace this with const

NejcZdovc · 2018-04-06T12:02:12Z

@jasonrsadler really good tests, great job

NejcZdovc · 2018-04-06T12:02:51Z

js/lib/urlutil.js

+  },
+
+  shortenUrlToDomainTLD: (url) => {
+    if (url !== null) {


url !== null let's change this to url != null. This way we are checking null and undefined

NejcZdovc · 2018-04-06T14:30:30Z

@jasonrsadler please squash commits and then we are good to go with the code

jasonrsadler · 2018-04-06T14:31:07Z

@NejcZdovc Done. Thanks for the assist and let me know if everything else looks good.

NejcZdovc

++ thank you for good test coverage

NejcZdovc · 2018-04-11T06:29:54Z

test/unit/lib/urlutilTest.js

+
+    it('normal url', function () {
+      const result = urlUtil.shortenUrlToDomainTLD('https://brave.com')
+      assert.equal(result, 'https://brave.com/')


@jasonrsadler this test should fail. We should not apply / at the end if / was not there from the gecko

other unit tests are failing because of that

Agree. But something else is appending the trailing '/' at the end of the url during normal browsing.
Example: using latest release type in brianbondy.com and press enter. The trailing '/' gets appended at the end.

As for the tests, will fix.

Let me know what your opinion is regarding the current navigation behavior.

diracdeltas · 2018-04-11T16:44:11Z

js/lib/urlutil.js

@@ -508,6 +508,14 @@ const UrlUtil = {
    return ip.isPrivate(hostname) || hostname === 'localhost' || whitelistSuffixes.some((suffix) => {
      return hostname && hostname.endsWith(`.${suffix}`)
    })
+  },
+
+  shortenUrlToDomainTLD: (url) => {


can you just use getBaseDomain from js/lib/baseDomain.js ?

nvm, i guess what you want here is the full URL without the path?

this seems like it would fail on URLs that don't include slashes in the protocol, like mailto:

you could do parsed = urlParse(url); return parsed.path ? parsed.split(parsed.path)[0] : url or something like that

^ though i guess that doesn't work with URLs that don't have a protocol

since publishers only works on http or https sites, you could fix by

if (!url.startsWith('http://') && !url.startsWith('https://')) { url = `https://${url}` }

@diracdeltas That works well. Thanks!

@diracdeltas I'm going to go with a combo and also to make sure another protocol isn't passed in.
if (!url.startsWith('http://') && !url.startsWith('https://') && !url.includes('://')) { url = `https://${url}` }

jasonrsadler · 2018-04-16T12:56:33Z

The STR I had I could only reproduce on brianbondy.com. That site seems to be down ATM. Need other sites with this issue to validate.

diracdeltas · 2018-04-17T20:49:26Z

js/lib/urlutil.js

+  },
+
+  shortenUrlToDomainTLD: (url) => {
+    if (url != null && (!url.startsWith('http://') && !url.startsWith('https://')) ? `https://${url}` : url) {


not sure I understand this if condition. the part after the first && returns a truthy value, but it doesn't set the URL

i think you mean:

if (url != null) { url = (!url.startsWith('http://') && !url.startsWith('https://')) ? `https://${url}` : url return url.substring(0, url.indexOf('/', url.indexOf('://') + 3)) }

Yes thank you.

diracdeltas · 2018-04-17T20:52:02Z

test/unit/lib/urlutilTest.js

@@ -562,4 +562,51 @@ describe('urlutil', function () {
      assert.equal(result, 'http://www.test.com/test.pdf')
    })
  })
+


tests below should be uncommented?

Which I guess is why the function passed.

Thanks for catching.

diracdeltas · 2018-04-18T03:17:25Z

test/unit/app/common/state/ledgerStateTest.js

@@ -622,4 +623,32 @@ describe('ledgerState unit test', function () {
      assert.equal(result, 'corrupted')
    })
  })
+
+  describe('getVerifiedPublisherLocationProp', function () {


minor: it would be good to have a test case for a URL without a protocol, ex: brianbondy.com

This got me thinking: From everything I've seen in this so far. URLs already have the protocol resolved before they get to this point. @diracdeltas or @NejcZdovc, do you know of any exceptions where URLs don't have a protocol already before getting to the publisherToggle renderer? If not, then I might have been overthinking shortenUrlToDomainTLD logic

@jasonrsadler I don't know of any examples but I also am not familiar with this code path at all

not that I am aware

diracdeltas

security review passes. i did not do a functional test because my ledger client seems to be broken locally

NejcZdovc · 2018-04-18T08:55:48Z

app/common/state/ledgerState.js

@@ -67,12 +67,25 @@ const ledgerState = {
    return state.setIn(['ledger', 'locations', url, prop], value)
  },

-  getLocationProp: (state, url, prop) => {
+  getVerifiedPublisherLocationProp: (state, url, prop) => {


let's remove prop, because we changed this to specific publisher function, so it's not needed anymore

NejcZdovc · 2018-04-18T08:59:38Z

app/common/state/ledgerState.js

+    if (prop === 'publisher') {
+      url = urlUtil.shortenUrlToDomainTLD(url)
+    }
+    const urlLocations = state.getIn(['ledger', 'locations']).toJS()


I think this one can be problematic, perf wise. getVerifiedPublisherLocationProp is called from mergeProps, which is called a lot of times. ['ledger', 'locations'] can be a really big list which we convert from immutable to regular (expensive) and then filter that potential big list.

I didn't run the code yet, but can't we just do state.getIn(['ledger', 'locations', url, prop]) where url is created on line 76?

The problem I was running into is that sometimes the url is formatted inconsistently. Sometimes, there's a '/' at the end, sometimes it's left off which makes it difficult to get an exact match on the key.

I agree ['ledger', 'locations'] will get too big to pass back and forth. I'll need to rethink this.

if you need someone to brainstorm with just let me know

NejcZdovc

looks good to me. The only thing that we need to improve is that filter step

diracdeltas · 2018-04-18T19:04:31Z

app/common/state/ledgerState.js

+
+    if (!publisherKey) {
+      const parsedUrl = urlParse(url)
+      if (parsedUrl) {


IIRC this will always be truthy because urlParse returns {} for non-valid URLs

we can do const parsedUrl = urlParse(url) || {} just to be safe and then we can remove that if check

diracdeltas · 2018-04-18T19:08:18Z

app/common/state/ledgerState.js

+    if (!publisherKey) {
+      const parsedUrl = urlParse(url)
+      if (parsedUrl) {
+        publisherKey = parsedUrl.hostname.replace('www', '')


parsedUrl.hostname may be null

also this would turn somesitewww.com into somesite.com. i think you want to either omit the replace or use a regex (/^www\./)

diracdeltas

lgtm security-wise

Changes were made

ryanml

I don't think it's related to this PR, but I am no longer seeing a checkmark for verified publishers.

Looks like a publisher verification issue:

Error verifying publisher brianbondy.com: Error: HTTP response 429 for GET /v3/publisher/identity?publisher=brianbondy.com

NejcZdovc · 2018-04-20T08:06:45Z

app/common/state/ledgerState.js

@@ -65,12 +68,28 @@ const ledgerState = {
    return state.setIn(['ledger', 'locations', url, prop], value)
  },

+  getVerifiedPublisherLocationProp: (state, url) => {


getVerifiedPublisherLocationProp -> getVerifiedPublisherLocation

Let's rename it, because we don't have prop anymore

NejcZdovc · 2018-04-20T08:07:07Z

app/common/state/ledgerState.js

@@ -18,6 +18,9 @@ const settings = require('../../../js/constants/settings')
 const getSetting = require('../../../js/settings').getSetting
 const {makeImmutable, isMap} = require('../../common/state/immutableUtil')



Please remove this empty line

WIP -- unit test Moved url stripping logic to urlutil. Implemented unit tests Removed logging debug statements shortenUrlToDomainTLD now more utility than ledger based. corrected lint corrected types and checks merge lint wip -- refactor shortenUrlToDomainTLD wip -- fixup unit tests wip -- moved logic to own function to get publisher key Removed file (wrong branch) Updated url shortener and moved getter to separate getter. linting Corrected URL parsing logic and tests Update verified publisher method and updated tests corrected parsing for publisher keys check for null parsedUrl.hostname Renamed function to reflect params

NejcZdovc

++ code and tests are looking good

Some verified publishers no longer remove green tick at sublevels - Fixes issue #10484

NejcZdovc · 2018-04-20T11:41:28Z

master fcf1667
0.23 7ca75e8
0.22 e1113e3

NejcZdovc self-requested a review March 31, 2018 05:20

NejcZdovc assigned jasonrsadler Mar 31, 2018

NejcZdovc added the feature/rewards label Mar 31, 2018

NejcZdovc reviewed Apr 6, 2018

View reviewed changes

NejcZdovc self-requested a review April 6, 2018 14:30

NejcZdovc approved these changes Apr 6, 2018

View reviewed changes

NejcZdovc modified the milestones: Completed work, 0.22.x Release 2, 0.22.x Release 3 (Beta channel) Apr 6, 2018

NejcZdovc previously requested changes Apr 11, 2018

View reviewed changes

diracdeltas reviewed Apr 11, 2018

View reviewed changes

diracdeltas reviewed Apr 17, 2018

View reviewed changes

jasonrsadler requested a review from diracdeltas April 18, 2018 01:58

diracdeltas reviewed Apr 18, 2018

View reviewed changes

diracdeltas previously approved these changes Apr 18, 2018

View reviewed changes

NejcZdovc reviewed Apr 18, 2018

View reviewed changes

NejcZdovc previously requested changes Apr 18, 2018

View reviewed changes

jasonrsadler dismissed diracdeltas’s stale review via ee75fb0 April 18, 2018 17:35

jasonrsadler requested review from diracdeltas and NejcZdovc April 18, 2018 17:36

diracdeltas reviewed Apr 18, 2018

View reviewed changes

NejcZdovc requested a review from ryanml April 19, 2018 08:12

jasonrsadler requested a review from diracdeltas April 19, 2018 15:16

diracdeltas previously approved these changes Apr 20, 2018

View reviewed changes

ryanml reviewed Apr 20, 2018

View reviewed changes

NejcZdovc reviewed Apr 20, 2018

View reviewed changes

NejcZdovc self-requested a review April 20, 2018 08:08

jasonrsadler dismissed diracdeltas’s stale review via d070cbd April 20, 2018 10:08

NejcZdovc approved these changes Apr 20, 2018

View reviewed changes

NejcZdovc merged commit fcf1667 into brave:master Apr 20, 2018

NejcZdovc added a commit that referenced this pull request Apr 20, 2018

Merge pull request #13675 from jasonrsadler/working-dir5

7ca75e8

Some verified publishers no longer remove green tick at sublevels - Fixes issue #10484

NejcZdovc added a commit that referenced this pull request Apr 20, 2018

Merge pull request #13675 from jasonrsadler/working-dir5

e1113e3

Some verified publishers no longer remove green tick at sublevels - Fixes issue #10484

jasonrsadler deleted the working-dir5 branch April 20, 2018 13:05

jasonrsadler removed the PR/pending-review label Apr 20, 2018

		@@ -18,6 +18,9 @@ const settings = require('../../../js/constants/settings')
		const getSetting = require('../../../js/settings').getSetting
		const {makeImmutable, isMap} = require('../../common/state/immutableUtil')

Some verified publishers no longer remove green tick at sublevels - Fixes issue #10484 #13675

Some verified publishers no longer remove green tick at sublevels - Fixes issue #10484 #13675

Conversation

jasonrsadler commented Mar 31, 2018 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

codecov-io commented Mar 31, 2018 • edited Loading

Codecov Report

jasonrsadler commented Apr 3, 2018

NejcZdovc commented Apr 4, 2018

jasonrsadler commented Apr 4, 2018 • edited Loading

NejcZdovc commented Apr 4, 2018

jasonrsadler commented Apr 4, 2018

jasonrsadler commented Apr 5, 2018

NejcZdovc commented Apr 5, 2018 • edited Loading

jasonrsadler commented Apr 5, 2018

jasonrsadler commented Apr 5, 2018 • edited Loading

NejcZdovc commented Apr 6, 2018

jasonrsadler commented Apr 6, 2018

Choose a reason for hiding this comment

NejcZdovc commented Apr 6, 2018

Choose a reason for hiding this comment

NejcZdovc commented Apr 6, 2018

jasonrsadler commented Apr 6, 2018

NejcZdovc left a 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 Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonrsadler commented Apr 16, 2018

diracdeltas Apr 17, 2018 • 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

diracdeltas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonrsadler Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonrsadler Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NejcZdovc left a 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 left a comment

Choose a reason for hiding this comment

ryanml left a comment • 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

NejcZdovc left a comment

Choose a reason for hiding this comment

NejcZdovc commented Apr 20, 2018

jasonrsadler commented Mar 31, 2018 •

edited

Loading

codecov-io commented Mar 31, 2018 •

edited

Loading

jasonrsadler commented Apr 4, 2018 •

edited

Loading

NejcZdovc commented Apr 5, 2018 •

edited

Loading

jasonrsadler commented Apr 5, 2018 •

edited

Loading

diracdeltas Apr 11, 2018 •

edited

Loading

diracdeltas Apr 17, 2018 •

edited

Loading

jasonrsadler Apr 18, 2018 •

edited

Loading

jasonrsadler Apr 18, 2018 •

edited

Loading

ryanml left a comment •

edited

Loading