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

Enable security components #7521

Merged
merged 3 commits into from
Jan 13, 2021
Merged

Enable security components #7521

merged 3 commits into from
Jan 13, 2021

Conversation

fmarier
Copy link
Member

@fmarier fmarier commented Jan 5, 2021

Resolves brave/brave-browser#12999 and brave/brave-browser#13010

Submitter Checklist:

  • There is a ticket for my issue.
  • Used Github auto-closing keywords in the commit message.
  • 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).
  • Requested a security/privacy review as needed (not needed: no extra network requests other than already-proxied component updater).

Reviewer Checklist:

  • 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:

  1. Start Brave with a new profile.
  2. Go into chrome://components.
  3. Check that Safety Tips has a version number greater than 0 (e.g. 2533).
  4. Check that Certificate Error Assistant has a version number greater than 0 (e.g. 7).
  5. Visit https://badssl.com/test/safety-tips/ and confirm that it triggers a notification:

Screenshot from 2021-01-04 18-42-40

@fmarier fmarier requested a review from a team as a code owner January 5, 2021 02:44
@fmarier fmarier self-assigned this Jan 5, 2021
@fmarier fmarier added this to the 1.20.x - Nightly milestone Jan 5, 2021
@mihaiplesa mihaiplesa requested a review from mherrmann January 5, 2021 05:21
@simonhong
Copy link
Member

@mherrmann if your fix(#7164) is merged, maybe this PR is not needed?

@mherrmann
Copy link
Collaborator

Do I understand correctly that the goal of this PR is to get Certificate Error Assistant and Safety Tips installed in Brave? If yes, then @simonhong correctly points out that #7164 has a connection to this PR. Specifically, if #7164 is merged before this one, then it will be enough to remove the two components from disallowed_components. The forceful call to BraveOnDemandUpdate will no longer be necessary. There is one caveat: BraveOnDemandUpdate leads to the component being installed immediately on browser start. Without this, the component will be installed on the first automatic update check, which happens roughly one minute (iirc) into the browser running.

If this delay is acceptable, then I think it would be cleaner to merge #7164 first, then simplify this present PR.

@simonhong
Copy link
Member

Yes, it seems this PR want to enable these two components. and avoiding install during the startup would be nice.

@fmarier
Copy link
Member Author

fmarier commented Jan 5, 2021

If this delay is acceptable, then I think it would be cleaner to merge #7164 first, then simplify this present PR.

Let's do that. I'll wait for that PR to merge and then rebase this one.

@fmarier fmarier marked this pull request as draft January 5, 2021 19:19
@simonhong
Copy link
Member

@fmarier fyi #7164 is merged.
I confirmed that only removing them from disallowed list works well.

@fmarier fmarier force-pushed the enable-security-components branch from 3912528 to 8aaea4f Compare January 9, 2021 02:35
@fmarier fmarier marked this pull request as ready for review January 9, 2021 02:36
@fmarier
Copy link
Member Author

fmarier commented Jan 9, 2021

Thanks @simonhong . I have updated the patch with all that's needed now.

@fmarier fmarier force-pushed the enable-security-components branch 2 times, most recently from 4424731 to 9facc4d Compare January 11, 2021 23:23
@fmarier fmarier force-pushed the enable-security-components branch from 9facc4d to 20f1e92 Compare January 12, 2021 05:01
Copy link
Collaborator

@mherrmann mherrmann left a comment

Choose a reason for hiding this comment

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

lgtm

@mihaiplesa mihaiplesa removed the request for review from mkarolin January 12, 2021 11:48
@fmarier fmarier force-pushed the enable-security-components branch from 20f1e92 to db31b77 Compare January 12, 2021 19:07
Copy link
Collaborator

@mkarolin mkarolin left a comment

Choose a reason for hiding this comment

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

++

@fmarier fmarier merged commit ffc31d7 into master Jan 13, 2021
@fmarier fmarier deleted the enable-security-components branch January 13, 2021 17:28
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.

[Security] Enable Safety Tips
5 participants