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

Canary hasn't updated since 10/24 #1591

Closed
ferBonnin opened this issue Oct 31, 2019 · 2 comments · Fixed by #1717
Closed

Canary hasn't updated since 10/24 #1591

ferBonnin opened this issue Oct 31, 2019 · 2 comments · Fixed by #1717
Labels
bug Something isn't working status: resolved This issue has been merged into main and deployed to canary.

Comments

@ferBonnin
Copy link
Contributor

Describe the bug

Our canary releases are "in pending review" by the Chrome web store.
Canary is stuck since 10/24, push to insider took 1.5 days and Playground is also not in review.

Let's review Google's guidance and check if there is any permissions we can remove/edit to avoid having a compliance review for each release.

Expected behavior

release should be available as fast as it was before (<1 hour)

Additional context

This is blocking our ability to do fast development

@ferBonnin ferBonnin added the bug Something isn't working label Oct 31, 2019
@msft-github-bot msft-github-bot added the status: new This issue is new and requires triage by DRI. label Oct 31, 2019
@smoralesd
Copy link
Contributor

google developer's transition guide around permissions: https://developer.chrome.com/extensions/runtime_host_permissions

@smoralesd smoralesd added status: active This issue is currently being worked on by someone. and removed status: new This issue is new and requires triage by DRI. labels Nov 1, 2019
@smoralesd
Copy link
Contributor

In order to improve our chances to not get flag with Pending review (which takes 5~7 bussiness days acording to google developer support) we need to change our current permissions. Basically, remove "https://*/*", "http://*/*", "file://*/*" and add "activeTab".
"activeTab" gives us permission to inject javascript and css to the target page in a tab-by-tab basis and only when the user activate our extension in the context of a tab (thus, we get the permissions for said tab, not all the tabs). The user can activate an extension by at least a couple of means:

  • clicking on the extension icon on the extensions bar righ by the address bar)
  • using the extension shortcut to activate it (provided the extension declares a "_execute_browser_action" on the manifest)

This permissions are granted for as long the tab lives, and it's not revoked when the tab goes "not active" (meaning, when a different tab gets active), so we can still try to scan a page (from a details view associated with it) when its tab is not active.

The downside of using "activeTab" permissions is: this breaks our e2e tests. This comes from a couple of details:

  1. We use puppeteer for our e2e test and as of today, puppeteer does not provides a way to activate the extension icon as mentioned here.
  2. Because of this, we create tabs and navigate to chrome-extension urls for our extension to open the popup, details view and guidance content. Doing this does not grant us "activeTab" permissions as we are not activating the extension from a tab context.

We have a couple of approaches we need to try:

  1. Try to send the extension shortcut ("Shift + Ctrl + K") so we can activate the extension from a tab.
    1. We may try to use puppeteer keyboard api to do so
    2. We may try a different approach where we try to do this at the OS level
  2. We have a different manifest for our extension while running 2e2 tests, where we have access to known urls we are trying to scan during the tests (worst case scenario: we add the '<all_urls>' permission.

smoralesd pushed a commit that referenced this issue Dec 2, 2019
This purpose of this PR is to resolve #1591 by replacing our `manifest.json` permissions for `https://*/*", "http://*/*", "file://*/*` with the `activeTab` permission.

To achieve this, it updates the way we track creation and update events for tabs such that we now:
* treat opening the popup view (which gives our extension permissions for the currently-active tab for as long as it stays on the same origin) as "opening" that tab
* treat tab updates/navigation events that change the origin of a tab as having "closed" that tab

This PR only slightly changes the `target-page-controller` implementation, but its tests were a real challenge to follow originally, so the PR completely rewrites `target-page-controller.test.ts` (don't bother comparing them side-by-side, treat it as a new test file for the class).

* refactor: rename Tab.Update and Tab.Change msgs to be distinguishable
* consider origin change events to be like page close events
* prettier format for manifest.json
* Update popup-action-message-creator unit tests per changes
* Update tab store onExistingTabUpdated tests to account for new branch
* Update onNewTabCreated tests to account for new cases
* Add PopupActionCreator tests
* Rewrite target page controller tests (in-progress)
* Refactor simualted browser adapter to separate file, finish test impl
* Fix lint errors
* use isFunction check on setup for addListener mocks
* add tabToContextMap behavior to test corpus
* adjust permission on runtime for e2e to be able to inject js/css into the target page
* Fix merge conflict resolutions
* Fix lint/format issues from merge conflicts
* Add prerequisite "launch popup" step to details view e2e tests
* Update adhoc-panel tests to simulate activeTab permissions
* Fix issue where screenshotOnError would time out and break error stacks if evaluate took too long
* Hacky workaround for adhoc-panel.test timeouts
* Add puppeteer tracing around failing adhoc panel tests
* Avoid issue with page mutation between different parts of clickSelector
* Revert withTracing changes that were extracted to #1751
@DaveTryon DaveTryon added status: resolved This issue has been merged into main and deployed to canary. and removed status: active This issue is currently being worked on by someone. labels Apr 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working status: resolved This issue has been merged into main and deployed to canary.
Projects
None yet
4 participants