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

Support permission lifetime options for "Block" decision. #8693

Merged
merged 1 commit into from
May 6, 2021

Conversation

goodov
Copy link
Member

@goodov goodov commented May 4, 2021

Extend permission lifetime logic to support lifetime options for "Block" decision.

Resolves brave/brave-browser#15603

image

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:


void PermissionDialogDelegate::Accept(JNIEnv* env,
const JavaParamRef<jobject>& obj) {
+ BRAVE_PERMISSION_DIALOG_DELEGATE_ACCEPT
permission_prompt_->Accept();
}

void PermissionDialogDelegate::Cancel(JNIEnv* env,
const JavaParamRef<jobject>& obj) {
+ BRAVE_PERMISSION_DIALOG_DELEGATE_CANCEL
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think with Cancel you can redefine the method in chromium_src for .h and .cc. It wasn't possible for Accept since it also calls Accept internally, but Cancel calls Deny, so maybe we can avoid this part of the patch?

Copy link
Member Author

Choose a reason for hiding this comment

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

#define Cancel                                                        \
  Cancel_ChromiumImpl(JNIEnv* env, const JavaParamRef<jobject>& obj); \
  void Cancel(JNIEnv* env, const JavaParamRef<jobject>& obj);

this doesn't work because Cancel is widely used everywhere and I see some weird errors:

In file included from ../../mojo/public/cpp/bindings/lib/interface_ptr_state.h:31:
In file included from ../../mojo/public/cpp/bindings/lib/multiplex_router.h:29:
In file included from ../../mojo/public/cpp/bindings/connector.h:24:
In file included from ../../mojo/public/cpp/system/handle_signal_tracker.h:12:
../../mojo/public/cpp/system/simple_watcher.h:143:8: error: no template named 'JavaParamRef'; did you mean 'base::android::JavaParamRef'?
  void Cancel();

not sure how it can be done the other way, because it's a JNI method and JNI-generated code expects exactly PermissionDialogDelegate::Cancel method. I don't see an option to make it virtual with a define because it's also widely used.

are there some other options I don't see?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, if it messes up a lot of other files, let's just go with a patch. Thanks for trying.

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.

chromium_src and patches changes LGTM

@goodov goodov merged commit 364e1a9 into master May 6, 2021
@goodov goodov deleted the issues/15603 branch May 6, 2021 18:27
@goodov goodov added this to the 1.26.x - Nightly milestone May 6, 2021
brave-builds pushed a commit that referenced this pull request May 6, 2021
@stephendonner
Copy link
Contributor

Verified FIXED on

Brave 1.26.16 Chromium: 91.0.4472.38 (Official Build) nightly (64-bit)
Revision 8155e7afab5d695cf0e028f4d77203287523cda9-refs/branch-heads/4472_35@{#6}
OS Windows 10 OS Version 2009 (Build 21376.1)

Cases

20 seconds, Allow permission (camera + microphone):

  1. launch Brave using --permission-lifetime-test-seconds=20
  2. load https://permission.site
  3. click on Camera + Microphone
  4. give permission for 20 seconds, click Allow
  5. click on the lock icon in the URL bar
  6. confirm the popup shows Camera - Allow and Microphone - Allow
  7. wait the remainder of the 20 seconds
  8. click on the lock icon again
  9. confirm the popup no longer has Camera nor Microphone with any values present
example example example
cam-and-mic cam-and-mic-popup cam-and-mic-popup-perms-gone

Origin-based behavior, multiple tabs (location):

  1. load hulu.com
  2. log in
  3. give permission until I close this site
  4. click Allow
  5. confirm brave://settings/content/location shows Allow under Location
  6. click Allow when prompted to install Widevine
  7. play a video
  8. open hulu.com in another tab
  9. play a video
  10. close either of the two open tabs
  11. confirm brave://settings/content/location shows Allow under Location
  12. close the remaining tab
  13. confirm brave://settings/content/location now shows Ask before accessing (recommended) and has nothing under Block and Allow
example example example example example
hulu-allow hulu-settings hulu-two-tabs hulu-one-tabs hulu-no-tabs

23-hours positive test (microphone):

  1. load https://permission.site/
  2. click on Microphone
  3. give permission for 24 hours
  4. click Allow
  5. confirm brave://settings/content shows Allow for Microphone
  6. shut down Brave
  7. set system clock forward by 23 hours
  8. launch Brave
  9. confirm brave://settings/content shows Allow for Microphone
  10. load https://permission.site/
  11. confirm you do NOT get re-prompted for microphone permissions when clicking on Microphone
example example example example
microphone mic-settings mic-permissions-2 mic-site

Consecutive-revocation permissions (location)

  1. launch Brave using --permission-lifetime-test-seconds=20
  2. load https://permission.site
  3. click on Location
  4. give permission for 20 seconds
  5. click Allow
  6. click on the lock icon next to permission.site in the URL bar
  7. confirm Location is set to Allow
  8. wait 10 seconds
  9. open https://browserleaks.com/geo
  10. give permission for 20 seconds
  11. return to permissions.site
  12. click on the lock icon; confirm you no longer see Location with any value
  13. return to browserleaks.com/geo
  14. click on the lock icon; confirm you no longer see Location with any value
example example
consecutive-dialog consecutive-allow

Block forever (notifications):

  1. load https://permission.site/
  2. click on Notifications
  3. choose forever from the Remember my decision dropdown, then click on Block
  4. confirm brave://settings/content shows Block for Notifications
  5. shut down Brave
  6. set system clock forward by 3 months
  7. confirm brave://settings/content shows Block for Notifications
  8. load https://permission.site/
  9. confirm you get do NOT get reprompted for Notifications
example example
notifications-site notifications-perms

kylehickinson pushed a commit that referenced this pull request Jan 29, 2024
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.

Change permissions lifetime UI to make relationship between "block" and the dropdown clearer
4 participants