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.

Sign up for GitHub

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

Jump to bottom

Add origin-based permission lifetime support #8378

Merged
merged 9 commits into from
Apr 13, 2021
Merged

Add origin-based permission lifetime support #8378

merged 9 commits into from
Apr 13, 2021

Conversation

goodov
Copy link
Member

@goodov goodov commented Mar 29, 2021 •
edited
Loading

Added an integration of the ephemeral origin lifetime tracking with the permission revoke mechanism.
This is a second PR, should be merged on top of the first one.

Resolves brave/brave-browser#14126

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • 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)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • 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:

@goodov goodov requested a review from iefremov March 29, 2021 14:06
@goodov goodov requested review from bridiver and a team as code owners March 29, 2021 14:06
@goodov goodov force-pushed the issues/14126-2 branch 2 times, most recently from 86faf7c to 1eab63a Compare March 30, 2021 07:39
@goodov goodov force-pushed the issues/14126-2 branch 3 times, most recently from 5f1b47a to eb6200a Compare April 5, 2021 09:51
bridiver
bridiver reviewed Apr 8, 2021
View reviewed changes
components/permissions/permission_lifetime_manager.h
@@ -35,7 +37,9 @@ class PermissionLifetimeManager : public KeyedService,
static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);

PermissionLifetimeManager(HostContentSettingsMap* host_content_settings_map,
PrefService* prefs);
PrefService* prefs,
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing either an include or forward decl for PrefService

Copy link
Member Author

@goodov goodov Apr 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

it's right here:

brave-core/components/permissions/permission_lifetime_manager.h

Line 28 in ced3795

class PrefService;

bridiver
bridiver reviewed Apr 8, 2021
View reviewed changes
chromium_src/content/browser/tld_ephemeral_lifetime.cc Outdated
auto it = active_tld_storage_areas().find(key);
DCHECK(it == active_tld_storage_areas().end() || it->second.get());
return it != active_tld_storage_areas().end() ? it->second.get() : nullptr;
const TLDEphemeralLifetimeKey key(browser_context, std::move(storage_domain));
Copy link
Collaborator

Choose a reason for hiding this comment

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

same, const& instead of std::move and you still only have one copy operation inside TLDEphemeralLifetimeKey

bridiver
bridiver reviewed Apr 12, 2021
View reviewed changes
.github/CODEOWNERS Outdated
@@ -69,6 +69,10 @@ components/weekly_storage/ @iefremov
# Perf predictor
components/brave_perf_predictor/ @iefremov

# Permissions
browser/permissions/ @akhoroshilov
Copy link
Collaborator

Choose a reason for hiding this comment

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

we really need to avoid having entire components blocked on one person, can you restrict this to just critical parts of the code and/or add more people?

@goodov goodov merged commit 72c9338 into master Apr 13, 2021
@goodov goodov deleted the issues/14126-2 branch April 13, 2021 13:25
@stephendonner
Copy link
Contributor

As discussed in Slack, the origin-based permission lifetime (until I close this page) requires uplift to the 1.24.x branch.

@goodov when you next get online, mind please getting that started? So as not to block that effort, I started testing on nightly; notes here.

/cc @kjozwiak

Verified the following 3 cases with this nightly build; @pes10k @goodov let me know ASAP, please, if there are additional cases to cover for the uplift. The full verification will be done over in brave/brave-browser#14126

Brave 1.25.25 Chromium: 90.0.4430.72 (Official Build) nightly (x86_64)
Revision b6172ef8d07ef486489a4b11b66b2eaeed50d132-refs/branch-heads/4430@{#1233}
OS macOS Version 11.2.3 (Build 20D91)

Case 1: Origin-based behavior, multiple tabs (location):

  1. load brave://flags and enable the Permission Lifetime flag
  2. restart Brave
  3. load hulu.com
  4. log in, install Widevine
  5. Give permission until I close this page
  6. click Allow
  7. confirm brave://settings/content shows Allow for Location
  8. play a video
  9. open hulu.com in another tab
  10. play a video
  11. close either of the two open tabs
  12. confirm brave://settings/content shows Allow for Location
  13. close the remaining tab
  14. confirm brave://settings/content now shows Ask (default) for Location

Case 2: Origin-based behavior, leave tab open, restart (MIDI):

  1. load brave://flags and enable the Permission Lifetime flag
  2. restart Brave
  3. load https://permission.site
  4. click on MIDI
  5. Give permission until I close this page
  6. click Allow
  7. confirm brave://settings/content shows Allow for MIDI devices
  8. leave tab open; shut down Brave
  9. confirm brave://settings/content shows Ask (default) for MIDI devices
  10. load https://permission.site
  11. click on MIDI
  12. confirm you get prompted for permission

Case 3: Origin-based behavior, close tab, open new tab (MIDI):

  1. load brave://flags and enable the Permission Lifetime flag
  2. restart Brave
  3. load https://permission.site
  4. click on MIDI
  5. Give permission until I close this page
  6. click Allow
  7. confirm brave://settings/content shows Allow for MIDI devices
  8. close permission.site tab
  9. open https://permission.site in a new tab
  10. confirm brave://settings/content shows Ask (default) for MIDI devices
  11. click on MIDI
  12. confirm you get prompted for permission
example example example example example
Screen Shot 2021-04-20 at 10 18 40 AM Screen Shot 2021-04-20 at 10 44 12 AM two-hulu-tabs Screen Shot 2021-04-20 at 10 16 07 AM Screen Shot 2021-04-20 at 10 16 56 AM

@stephendonner stephendonner mentioned this pull request Apr 20, 2021
Closed
brave-builds pushed a commit that referenced this pull request Apr 21, 2021
@brave-browser-releases
Uplift of #8378 (squashed) to beta
4f22090
@brave-builds brave-builds mentioned this pull request Apr 21, 2021
Merged
7 tasks
kylehickinson pushed a commit that referenced this pull request Jan 4, 2024
@iccub
Ref #8297: Update brave-core to 1.60.114 (#8378)
d23bcf4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iefremov iefremov iefremov approved these changes

@bridiver bridiver bridiver approved these changes

Assignees

@goodov goodov

Labels
None yet
Projects
None yet
Milestone
1.25.x - Release
Development

Successfully merging this pull request may close these issues.

Add support for time limits for Web API permissions in brave-core
4 participants
@goodov @stephendonner @bridiver @iefremov