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

Add a checkmark and the ability to toggle the permission #8

Merged
merged 12 commits into from
Aug 7, 2019

Conversation

fregante
Copy link
Owner

@fregante fregante commented Feb 13, 2019

Fixes #4
Fixes refined-github/refined-github#1509

Full rewrite sadly necessary to enable that little checkmark:

checkbox

I looked into a few simpler methods but they didn't work:

  • contextMenus lets you pick which origins to show the item on, but not where to show/hide its checkbox. Also no way to exclude pages
  • declarativeContent can only toggle the whole pageAction icon or load files; nothing else, no custom functions
  • the new user controls on Chrome 70+ can't be used as a whitelist... yet

Also I couldn't find anything simpler than these to pick which domain to enable it on:

https://github.com/bfred-it/webext-domain-permission-toggle/blob/325b814979d0e803532ead364848f5ac8a823152/webext-domain-permission-toggle.js#L98-L103

https://github.com/bfred-it/webext-domain-permission-toggle/blob/325b814979d0e803532ead364848f5ac8a823152/webext-domain-permission-toggle.js#L45-L56

@fregante
Copy link
Owner Author

fregante commented Feb 13, 2019

Things to look forward: https://blog.chromium.org/2018/10/trustworthy-chrome-extensions-by-default.html

In 2019 we will introduce the next extensions manifest version .... Some key goals of manifest v3 include: More narrowly-scoped and declarative APIs, to decrease the need for overly-broad access and enable more performant implementation by the browser, while preserving important functionality

@sindresorhus
Copy link
Collaborator

How can I test this without having access to GitHub Enterprise?

@fregante
Copy link
Owner Author

fregante commented Feb 14, 2019

  1. Visit google.com
  2. “Enable Refined GitHub…”
  3. See obvious errors in console

The injection part hasn’t changed, but the permission management has

@busches
Copy link

busches commented Feb 14, 2019

Testing this via refined-github/refined-github#1774

If I'm on GH, I see it checked, I remove it, RGH is disabled, but if I reenable it on GH, it doesn't turn on, but the checkmark says it's enabled.

Testing on GHE, for a domain that was previously added to permissions, first load RGH is working. Disable it, RGH is disabled. Also, I noticed if I disable it, it's coming back as checked on the next page load, but RGH is disabled. The extension tells me I need to refresh the page, even though I did. I used the extension to refresh, still shows as checked, but RGH is not working. It shows a weird state:
image
I'm now in a state when I uncheck the domain and it thinks it's loaded for the domain. If I uncheck it (even though it's already removed the permission), then recheck it, it prompts for permission. If I grant it, RGH is still not working.

I can see the permissions exist for GH and GHE but it's not working on either:
image

@fregante
Copy link
Owner Author

That’s… not right. This module only handles the permissions; if you can see your domain as officially allowed in the chrome page then it really is, and the content scripts should be automatically injected by the other module (unrelated, untouched by this PR)

@fregante
Copy link
Owner Author

fregante commented Feb 14, 2019

Can you check again? The github.com permission cannot be programmatically revoked and there’s nothing we can do to prevent its injection, so if you don’t see RGH there there’s something bigger at play (Chrome bug or you’re testing it wrong) 😅

Alternatively you might have disabled it from the native “Page access” menu, hence the red X on the icon.

@busches
Copy link

busches commented Feb 15, 2019

Retested. It's again working when I enable it, not sure what happened.

I can still "disable" it on GitHub, it doesn't actually disabled RGH, but the checkbox is missing.

With that said, when it's not enabled on the domain and we click the icon, we get "Refresh Required" message and I find it confusing. Refreshing the page doesn't matter if I haven't enabled it for the domain yet. Can we tie the "Ok, Refresh" button to also asking for the domain permission if it's not there?

@fregante
Copy link
Owner Author

The check is already there. In my case I never see the red X on the icon and that might be throwing off the system. At every step I verify that we have permanent permission to the domain. If not, the check will be hidden.

In my case, I receive an Alert saying that I can’t disable it on GitHub.com

What Chrome version are you on? Can you make sure no flags are enabled in chrome://flags?

@busches
Copy link

busches commented Feb 15, 2019

The issue is likely Chrome versions, I'm locked to 64 currently when using GHE. This requires a change to the manifest and to update the catch{} blocks in two places to get it to run. I have not tested it at home, since it would be the same testing you're doing. :)

@fregante
Copy link
Owner Author

fregante commented Feb 15, 2019

Alright, there are indeed some yucky Chrome bugs:

  • chrome.permissions.contains will report false if a domain was removed, but
  • chrome://extensions will still list the domain in the "Allow on all websites" list

Also after clicking that context menu item we will still have access to the current tab until we navigate away from the current domain (thanks to activeTab) — regardless of the permissions we actually have.

This might have caused some issues to your testing and it means I'll have to rewrite https://github.com/bfred-it/webext-dynamic-content-scripts to have the browser inject the scripts where they're permanently allowed, and not on activeTab

Closing for now until fregante/webext-dynamic-content-scripts#4 is done.

@fregante fregante closed this Feb 15, 2019
@fregante
Copy link
Owner Author

fregante commented Feb 15, 2019

Beautiful Chrome extensions bug:

screenshot 2019-02-16 at 00 03 48

chrome.permissions.getAll will report origins from content_scripts (manifest.json) but chrome.permissions.contains only looks in the permissions field


Edit: note to future self: this can probably be fixed by checking whether a domain is declared in the manifest, via chrome.runtime.getManifest().content_scripts

Also that webext-dynamic-content-scripts activeTab issue can be fixed by using the same mechanism to detect whether the current origin is allowed.

fregante added a commit to refined-github/refined-github that referenced this pull request Jun 2, 2019
@fregante fregante reopened this Aug 6, 2019
@fregante
Copy link
Owner Author

fregante commented Aug 6, 2019

Will test version v1.0.0-2

chrome.runtime.onInstalled isn't called when it's installed locally
@fregante
Copy link
Owner Author

fregante commented Aug 7, 2019

This is now completely disabled on domains that can't be removed:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants