Show dialog when Brave tries to import Safari data w/o disk permission #4530

simonhong · 2020-02-06T09:25:18Z

Show tab modal dialog when Brave tries to import Safari data w/o disk permission

fix brave/brave-browser#2710

Submitter Checklist:

Test Plan:

Check Brave doesn't have full disk access permission
Launch Brave and tries to import safari data
Check above full disk access tab modal dialog is shown after import dialog is closed
Check how to url is loaded next to the current settings tab after clicking learn link

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

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on.
All relevant documentation has been updated.

simonhong · 2020-02-10T03:05:53Z

patches/chrome-browser-ui-webui-settings-settings_import_data_handler.h.patch

+   void OnJavascriptDisallowed() override;
+
+  private:
+  friend class BraveImportDataHandler;


Needed to call ImportDataHandler::StartImport().

simonhong · 2020-02-10T03:10:54Z

@rebron Can you check the text in the dialog? I referred edge's sentence.

patches/chrome-browser-ui-webui-settings-settings_import_data_handler.cc.patch

rebron · 2020-02-12T02:29:07Z

cc: @tomlowenthal cc: @Brave-Matt on text.

Original:
Full Disk Access required
Brave needs Full Disk Access to import your Bookmarks and History from Safari.
Learn to how to grant Full Disk Access from your System Preferences.

Edited per comment below.
Should read:
Full Disk Access required
Brave needs Full Disk Access to import your Bookmarks and History from Safari.
Learn how to grant Full Disk Access from your System Preferences.

simonhong · 2020-02-12T09:01:11Z

Text updated.

tildelowengrimm · 2020-02-13T02:00:58Z

Text seems fine. It's not very explanatory, but maybe it doesn't need to be.

simonhong · 2020-02-14T11:53:34Z

Kindly ping to reviewers :)

Brave-Matt · 2020-02-14T18:41:08Z

Use this help link should point to the Safari section on the support page directly: https://support.brave.com/hc/en-us/articles/360019782291#safari

bsclifton · 2020-02-14T21:04:31Z

Verified this shows dialog as expected and the help page has the info needed

Since I'm running locally though, I tried to add src/out/Component/Brave Browser Development.App to this list and it didn't work. Going to debug a bit more. Maybe I should try with stand-alone executable after create_dist

simonhong · 2020-02-15T04:18:11Z

@Brave-Matt URL is updated. thanks for checking!
@bsclifton Yes, you can see when you install from dmg.

bsclifton · 2020-02-17T05:38:35Z

Built locally, verified the modal shows. Also grabbed test DMG from here:
https://bravesoftware.slack.com/archives/CMH8DU4TF/p1581715873050300

Works great! 😄

bsclifton

New code and code changes look great to me! Nice patch cleanup and follows patch guidelines. As noted, tested and it works great 😄 Nice job!

bsclifton · 2020-02-17T05:55:41Z

cc: @bridiver for patch review

bsclifton · 2020-02-18T05:32:44Z

@simonhong is it possible to open the panel directly? If so, that would be great 😄 I think the guide link would still be good - as a way to explain what to do once you get there

simonhong · 2020-02-18T05:51:04Z

@bsclifton We can. @bridiver shared the api :)

#import <AppKit/AppKit.h>

[[NSWorkspace sharedWorkspace] openURL:[NSURL URLWithString:@"x-apple.systempreferences:com.apple.preference.security?Privacy_AllFiles"]];

simonhong · 2020-02-18T08:40:04Z

Tested with two buttons like below.

fix brave/brave-browser#2710

rebron · 2020-02-19T23:40:05Z

That looks great.

kjozwiak · 2020-02-27T07:20:03Z

Verification PASSED on macOS 10.15.3 x64 using the following build:

Brave	1.7.22 Chromium: 80.0.3987.122 (Official Build) nightly (64-bit)
Revision	cf72c4c4f7db75bc3da689cd76513962d31c7b52-refs/branch-heads/3987@{#943}
OS	macOS Version 10.15.3 (Build 19D76)

ensured that Open System Preferences opened Security & Privacy and Full Disk Access was selected
ensured that "Cancel" cancelled the import operation and didn't import any bookmarks from Safari
ensured that the Learn how to grant Full Disk Access from your System Preferences text opened https://support.brave.com/hc/en-us/articles/360019782291#safari in a new tab
ensured importing worked once Brave received Full Disk Access via Security & Privacy when Safari is closed
ensured importing worked once Brave received Full Disk Access via Security & Privacy when Safari is opened

Examples of the above PR working:

kjozwiak · 2020-02-27T07:22:50Z

@rebron @tomlowenthal Learn to how to grant Full Disk Access from your System Preferences. sounds weird, specifically the Learn to how to part. Should it be the following instead?

Learn how to grant Full Disk Access from your System Preferences.

Either way, just double checking if we're good with Learn to how to grant Full

rebron · 2020-02-27T16:02:49Z

@simonhong I made typo error.
Dialog should read

Learn how to grant Full Disk Access from your System Preferences.

I added an extra to by mistake.

kjozwiak · 2020-02-28T20:58:03Z

Verification PASSED on macOS 10.15.3 x64 (Catalina) using the following build:

Brave	1.7.28 Chromium: 80.0.3987.122 (Official Build) nightly (64-bit)
Revision	cf72c4c4f7db75bc3da689cd76513962d31c7b52-refs/branch-heads/3987@{#943}
OS	macOS Version 10.15.3 (Build 19D76)

Text looks much better 👍

kjozwiak · 2020-02-29T00:22:01Z

Looks like we're going to need to edit the text once again. We're currently not importing History from Safari so the above should be using either the following:

Bookmarks and Favorites
Favorites and Bookmarks

CCing @rebron to create the follow up issue so we can change the text and decide on the order.

simonhong · 2020-02-29T00:34:42Z

@kjozwiak Not sure why current safari importer can't import history.
Upstream has that code. maybe upstream bug or blocked by other places.. not sure need to investigate. https://source.chromium.org/chromium/chromium/src/+/master:chrome/common/importer/safari_importer_utils.mm;l=25

kjozwiak · 2020-03-04T04:32:07Z

Verification PASSED on macOS 10.15.3 x64 using the following build:

Brave | 1.7.37 Chromium: 80.0.3987.122 (Official Build) nightly (64-bit)
-- | --
Revision | cf72c4c4f7db75bc3da689cd76513962d31c7b52-refs/branch-heads/3987@{#943}
OS | macOS Version 10.15.3 (Build 19D76)

@simonhong the issue with History not being imported into Brave from Safari will be addressed via brave/brave-browser#8515.

simonhong · 2020-03-04T04:46:10Z

@kjozwiak Ok thanks!

simonhong added this to the 1.6.x - Nightly milestone Feb 6, 2020

simonhong self-assigned this Feb 6, 2020

simonhong force-pushed the issue_2710 branch 2 times, most recently from 69a146b to 6a43b73 Compare February 10, 2020 02:52

simonhong commented Feb 10, 2020

View reviewed changes

simonhong marked this pull request as ready for review February 10, 2020 03:06

simonhong requested a review from bridiver as a code owner February 10, 2020 03:06

simonhong requested review from fmarier, bsclifton, bbondy and petemill February 10, 2020 03:06

simonhong force-pushed the issue_2710 branch from 6a43b73 to a00a26e Compare February 10, 2020 03:29

simonhong commented Feb 10, 2020

View reviewed changes

patches/chrome-browser-ui-webui-settings-settings_import_data_handler.cc.patch Outdated Show resolved Hide resolved

simonhong force-pushed the issue_2710 branch from a00a26e to 42b3bae Compare February 10, 2020 05:09

fmarier removed their request for review February 11, 2020 20:00

simonhong force-pushed the issue_2710 branch from 42b3bae to 219206f Compare February 12, 2020 02:44

bsclifton force-pushed the issue_2710 branch from 219206f to 817cd84 Compare February 14, 2020 17:09

simonhong force-pushed the issue_2710 branch from 817cd84 to 4943336 Compare February 15, 2020 04:16

simonhong force-pushed the issue_2710 branch from 4943336 to 0c895c6 Compare February 17, 2020 00:59

bsclifton approved these changes Feb 17, 2020

View reviewed changes

simonhong force-pushed the issue_2710 branch 2 times, most recently from 46e84d8 to 24c20e4 Compare February 18, 2020 02:13

simonhong force-pushed the issue_2710 branch from 24c20e4 to 3a8eac9 Compare February 18, 2020 08:38

Show dialog when Brave tries to import Safari data w/o disk permission

225d5b6

fix brave/brave-browser#2710

simonhong force-pushed the issue_2710 branch from 3a8eac9 to 225d5b6 Compare February 18, 2020 23:42

simonhong modified the milestones: 1.6.x - Dev, 1.7.x - Nightly Feb 20, 2020

bridiver approved these changes Feb 20, 2020

View reviewed changes

simonhong merged commit aeb928a into master Feb 20, 2020

simonhong deleted the issue_2710 branch February 20, 2020 22:29

simonhong mentioned this pull request Feb 21, 2020

[Dektop] Add Brave app automatically to Full Disk Access panel for macOS brave/brave-browser#8343

Closed

emerick mentioned this pull request Feb 21, 2020

Don't build brave_settings_import_data_handler on Android #4711

Merged

32 tasks

brave-builds pushed a commit that referenced this pull request Feb 25, 2020

Uplift of #4530 (squashed) to dev

a7daa85

brave-builds pushed a commit that referenced this pull request Feb 25, 2020

Uplift of #4530 (squashed) to beta

d8fbfee

This was referenced Feb 25, 2020

Show dialog when Brave tries to import Safari data w/o disk permission (uplift to 1.6.x) #4730

Merged

Show dialog when Brave tries to import Safari data w/o disk permission (uplift to 1.5.x) #4731

Merged

This was referenced Mar 4, 2020

importing bookmarks from Safari appears successful but fails - follow up to 2710 brave/brave-browser#8537

Closed

Importing bookmarks from Safari fails brave/brave-browser#2710

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show dialog when Brave tries to import Safari data w/o disk permission #4530

Show dialog when Brave tries to import Safari data w/o disk permission #4530

simonhong commented Feb 6, 2020 •

edited

Loading

simonhong Feb 10, 2020 •

edited

Loading

simonhong commented Feb 10, 2020 •

edited

Loading

rebron commented Feb 12, 2020 •

edited

Loading

simonhong commented Feb 12, 2020

tildelowengrimm commented Feb 13, 2020

simonhong commented Feb 14, 2020

Brave-Matt commented Feb 14, 2020

bsclifton commented Feb 14, 2020

simonhong commented Feb 15, 2020

bsclifton commented Feb 17, 2020

bsclifton left a comment

bsclifton commented Feb 17, 2020

bsclifton commented Feb 18, 2020

simonhong commented Feb 18, 2020

simonhong commented Feb 18, 2020

rebron commented Feb 19, 2020

kjozwiak commented Feb 27, 2020

kjozwiak commented Feb 27, 2020 •

edited

Loading

rebron commented Feb 27, 2020

kjozwiak commented Feb 28, 2020 •

edited

Loading

kjozwiak commented Feb 29, 2020 •

edited

Loading

simonhong commented Feb 29, 2020

kjozwiak commented Mar 4, 2020

simonhong commented Mar 4, 2020

Show dialog when Brave tries to import Safari data w/o disk permission #4530

Show dialog when Brave tries to import Safari data w/o disk permission #4530

Conversation

simonhong commented Feb 6, 2020 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

After-merge Checklist:

simonhong Feb 10, 2020 • edited Loading

Choose a reason for hiding this comment

simonhong commented Feb 10, 2020 • edited Loading

rebron commented Feb 12, 2020 • edited Loading

simonhong commented Feb 12, 2020

tildelowengrimm commented Feb 13, 2020

simonhong commented Feb 14, 2020

Brave-Matt commented Feb 14, 2020

bsclifton commented Feb 14, 2020

simonhong commented Feb 15, 2020

bsclifton commented Feb 17, 2020

bsclifton left a comment

Choose a reason for hiding this comment

bsclifton commented Feb 17, 2020

bsclifton commented Feb 18, 2020

simonhong commented Feb 18, 2020

simonhong commented Feb 18, 2020

rebron commented Feb 19, 2020

kjozwiak commented Feb 27, 2020

kjozwiak commented Feb 27, 2020 • edited Loading

rebron commented Feb 27, 2020

kjozwiak commented Feb 28, 2020 • edited Loading

kjozwiak commented Feb 29, 2020 • edited Loading

simonhong commented Feb 29, 2020

kjozwiak commented Mar 4, 2020

simonhong commented Mar 4, 2020

simonhong commented Feb 6, 2020 •

edited

Loading

simonhong Feb 10, 2020 •

edited

Loading

simonhong commented Feb 10, 2020 •

edited

Loading

rebron commented Feb 12, 2020 •

edited

Loading

kjozwiak commented Feb 27, 2020 •

edited

Loading

kjozwiak commented Feb 28, 2020 •

edited

Loading

kjozwiak commented Feb 29, 2020 •

edited

Loading