extension: better handling of missing permissions + updated/docs for publishing to AMO

doc/DEVELOPMENT.org

@@ -0,0 +1,17 @@
+* Releasing extension
+** AMO (addons.mozilla.org)
+: cd extension
+: npm install # (if necessary)
+: ./build --firefox --v2 --lint --release --publish
+
+Note that this command will go in a busy loop waiting for the extension to be approved on AMO.
+(you will see it on https://addons.mozilla.org/en-US/developers/addon/grasp/versions)
+
+- for now =v2= version of manifest is used with Firefox
+- you can use =--publish=unlisted= if you want to test extension yourself first
+
+
+Currently seems like it's not possible to upload the source code and notes for reviewers automatically:
+
+- https://github.com/mozilla/web-ext/issues/3137
+- https://github.com/mozilla/addons/issues/6158

extension/build

@@ -85,25 +85,26 @@ def main() -> None:
     if args.release:
         assert args.lint # TODO not sure..
 
-    def firefox_release_args():
+    def firefox_publish_args():
         from firefox_dev_secrets import API_KEY, API_SECRET
         return [
             '--artifacts-dir', str(artifacts_dir),
             '--api-key'      , API_KEY,
             '--api-secret'   , API_SECRET,
-            '--id'           , IDS[target],
+            # seems like webext sign requires addon id to be in manifest now
         ]
 
     if args.publish is not None:
         assert args.lint
         assert args.release
         if 'firefox' in target:
             check_call([
-                npm, 'run', 'release:amo',
+                npm, 'run', 'web-ext',
                 '--',
+                'sign', '--use-submission-api',
                 '--channel', args.publish,
                 '--source-dir', str(ext_dir),
-                *firefox_release_args(),
+                *firefox_publish_args(),
             ])
         elif target == 'chrome':
             assert args.publish == 'listed'  # no notion of unlisted on chrome store?

extension/generate_manifest.js

@@ -155,19 +155,23 @@ export function generateManifest({
 
     if (v3) {
         if (target === T.FIREFOX) {
-            // firefox doesn't support optional host permissions
+            // firefox doesn't support optional_host_permissions yet
+            // see https://bugzilla.mozilla.org/show_bug.cgi?id=1766026
+            // and https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/optional_permissions#host_permissions
             // note that these will still have to be granted by user (unlike in chrome)
-            manifest['host_permissions'] = [...host_permissions, ...optional_host_permissions]
+            manifest.host_permissions = host_permissions
+            manifest.optional_permissions.push(...optional_host_permissions)
         } else {
-            manifest['host_permissions'] = host_permissions
-            manifest['optional_host_permissions'] = optional_host_permissions
+            manifest.host_permissions = host_permissions
+            manifest.optional_host_permissions = optional_host_permissions
         }
     } else {
         manifest.permissions.push(...host_permissions)
         manifest.optional_permissions.push(...optional_host_permissions)
     }
 
-    if (v3) {
+    if (target === T.FIREFOX || v3) {
+        // for firefox, this is required during publishing?
         // this isn't really required in chrome, but without it, webext lint fails for chrome addon
         const gecko_id = target === T.FIREFOX ? ext_id : '{00000000-0000-0000-0000-000000000000}'
         manifest['browser_specific_settings'] = {

extension/rollup.config.js

@@ -56,7 +56,7 @@ function generateManifestPlugin() {
             const manifest = generateManifest({
                 target: target,
                 version: manifest_version,
-                releas: release,
+                release: release,
                 ext_id: ext_id,
             })
             const mjs = JSON.stringify(manifest, null, 2)

extension/src/background.ts

@@ -59,10 +59,13 @@ async function makeCaptureRequest(
     // We can't call ensurePermissions here, getting "permissions.request may only be called from a user input handler" error
     // even though it's called from the keyboard shortcut???
     // so the best we can do is try to check and at least show a more helful error
+    // also relevant https://bugzilla.mozilla.org/show_bug.cgi?id=1811608
 
     const has = await hasPermissions(endpoint)
     if (!has) {
-        throw new Error(`${endpoint}: no permissions detected! Go to extension settings and approve them.`)
+        // kinda awkward to open options page here etc, but fine for now
+        browser.tabs.create({url: 'options.html'})
+        throw new Error(`${endpoint}: no permissions detected!\nApprove the endpoint in extension settings, and repeat capture after that.`)
     }
 
     const data = JSON.stringify(params)
@@ -96,14 +99,16 @@ async function makeCaptureRequest(
 }
 
 
-// TODO ugh. need defensive error handling on the very top...
-async function capture(comment: string | null = null, tag_str: string | null = null) {
+async function capture(comment: string | null = null, tag_str: string | null = null): Promise<boolean> {
+    /**
+     * Returns whether capture has succeeded
+     */
     const tabs = await browser.tabs.query({currentWindow: true, active: true})
     const tab = tabs[0]
     if (tab.url == null) {
-        // todo await?
+        // todo when does this happen?
         showNotification('ERROR: trying to capture null')
-        return
+        return false
     }
     const url: string = tab.url
     const title: string | null = tab.title || null
@@ -126,8 +131,6 @@ async function capture(comment: string | null = null, tag_str: string | null = n
     let selection
     if (has_scripting) {
         const tab_id = tab.id as number
-        // TODO switch to polyfill and add flow types
-        // scripting is already promise based so it should be oly change to types
         const results = await browser.scripting.executeScript({
             target: {tabId: tab_id},
             func: () => window.getSelection()!.toString()
@@ -143,35 +146,47 @@ async function capture(comment: string | null = null, tag_str: string | null = n
 
     try {
         await makeCaptureRequest(payload(selection), opts)
+        return true
     } catch (err) {
         console.error(err)
         // todo make sure we catch errors from then clause too?
         const error_message = `ERROR: ${(err as Error).toString()}`
         console.error(error_message)
-        showNotification(error_message, 1)
-        // TODO crap, doesn't really seem to respect urgency...
+        showNotification(error_message, 1)  // todo crap, doesn't really seem to respect urgency...
+        return false
     }
 }
 
 
-browser.commands.onCommand.addListener(command => {
+browser.commands.onCommand.addListener((command: string) => {
     if (command === COMMAND_CAPTURE_SIMPLE) {
-        // todo await
-        capture(null, null)
+        // not checking return value here, can't really do much?
+        capture(null, null)  // todo await?
     }
 })
 
 
-browser.runtime.onMessage.addListener((message: any, sender: any, sendResponse: () => void) => {
+// ok so sadly it seems like async listener doesn't really work in chrome due to a bug
+// https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/runtime/onMessage#sending_an_asynchronous_response_using_a_promise
+// also see https://stackoverflow.com/questions/44056271/chrome-runtime-onmessage-response-with-async-await
+browser.runtime.onMessage.addListener((message: any, sender: any, sendResponse: (_arg: any) => void) => {
     if (message.method === 'logging') {
         console.error("[%s] %o", message.source, message.data)
     }
     if (message.method === METHOD_CAPTURE_WITH_EXTRAS) {
         const comment = message.comment
         const tag_str = message.tag_str
-        // todo await
-        capture(comment, tag_str)
-        sendResponse()
+
+        // NOTE: calling async directly doesn't work here in firefox
+        // (getting "Could not establish connection. Receiving end does not exist" error)
+        // I guess has something to do with micro (async) vs macro (setTimeout) tasks
+        // although if anything I'd expect macro tasks to work worse :shrug:
+        setTimeout(async () => {
+            // this will be handled in the popup
+            const success = await capture(comment, tag_str)
+            sendResponse({success: success})
+        })
+        return true  // means 'async response'
     }
     if (message.method == 'DARK_MODE') {
         const icon_path = message.hasDarkMode ? 'img/unicorn_dark.png' : 'img/unicorn.png'
@@ -181,6 +196,3 @@ browser.runtime.onMessage.addListener((message: any, sender: any, sendResponse:
         action.setIcon({path: icon_path})
     }
 })
-
-// TODO handle cannot access chrome:// url??
-

extension/src/options.html

@@ -10,9 +10,10 @@
   </style>
 </head>
 <body>
+    <h1>Grasp settings</h1>
     <fieldset>
         <legend>Endpoint</legend>
-        <input type='URL' id='endpoint_id'/>
+        <input type='URL' size='40' id='endpoint_id'/>
         <div id='has_permission_id' style="display: none; color: red">
             No permission to access the endpoint. Will request when you press "Save".
         </div>

extension/src/popup.ts

@@ -84,24 +84,24 @@ async function restoreState(state: State | null) {
 }
 
 
-function submitCapture () {
+async function submitCapture () {
     const state = getUiState()
 
     const message = {
         'method': METHOD_CAPTURE_WITH_EXTRAS,
         ...state,
     }
 
-    browser.runtime.sendMessage(message).then((_: any) => {
+    const result = await browser.runtime.sendMessage(message)
+    if (result.success) {
         // @ts-expect-error
         window.justSubmitted = true
         clearState()
-
-        // TODO not sure about this?
-        window.close()
         console.log("[popup] captured!")
-    })
-
+    } else {
+        // if capture wasn't successful, keep the state intact
+    }
+    window.close()
 }
 
 

tests/addon.py

@@ -38,7 +38,29 @@ class OptionsPage:
     def open(self) -> None:
         self.helper.open_page(self.helper.options_page_name)
 
+    def check_opened(self) -> None:
+        current_url = self.helper.driver.current_url
+        assert current_url.endswith(self.helper.options_page_name), current_url  #  just in case
+
+    def save(self, *, wait_for_permissions: bool = False) -> None:
+        self.check_opened()
+
+        driver = self.helper.driver
+
+        se = driver.find_element('id', 'save_id')
+        se.click()
+
+        if wait_for_permissions:
+            # we can't accept this alert via webdriver, it's a native chrome alert, not DOM
+            click.confirm(click.style('You should see prompt for permissions. Accept them', blink=True, fg='yellow'), abort=True)
+
+        alert = driver.switch_to.alert
+        assert alert.text == 'Saved!', alert.text  # just in case
+        alert.accept()
+
     def change_endpoint(self, endpoint: str, *, wait_for_permissions: bool = False) -> None:
+        self.check_opened()
+
         driver = self.helper.driver
 
         current_url = driver.current_url
@@ -52,16 +74,7 @@ def change_endpoint(self, endpoint: str, *, wait_for_permissions: bool = False)
         ep.clear()
         ep.send_keys(endpoint)
 
-        se = driver.find_element('id', 'save_id')
-        se.click()
-
-        if wait_for_permissions:
-            # we can't accept this alert via webdriver, it's a native chrome alert, not DOM
-            click.confirm(click.style('You should see prompt for permissions. Accept them', blink=True, fg='yellow'), abort=True)
-
-        alert = driver.switch_to.alert
-        assert alert.text == 'Saved!', alert.text  # just in case
-        alert.accept()
+        self.save(wait_for_permissions=wait_for_permissions)
 
 
 @dataclass
@@ -74,7 +87,12 @@ def open(self) -> None:
     def enter_data(self, *, comment: str, tags: str) -> None:
         helper = self.addon.helper
 
-        helper.gui_hotkey('tab')  # give focus to the input
+        if helper.driver.name == 'firefox':
+            # for some reason in firefox under geckodriver it woudn't focus comment input field??
+            # tried both regular and dev edition firefox with latest geckodriver
+            # works fine when extension is loaded in firefox manually or in chrome with chromedriver..
+            # TODO file a bug??
+            helper.gui_hotkey('tab')  # give focus to the input
 
         helper.gui_write(comment)
 

tests/test_end2end.py

@@ -80,7 +80,7 @@ def confirm(what: str) -> None:
 
 # chrome  v3 works
 # firefox v2 works
-# firefox v3 BROKEN -- needs the user to approve the localhost access
+# firefox v3 works (although a little more elaborate due to additional approvals)
 def test_capture_no_configuration(addon: Addon) -> None:
     """
     This checks that capture works with default hostname/port without opening settings first
@@ -96,21 +96,36 @@ def test_capture_no_configuration(addon: Addon) -> None:
 
     addon.quick_capture()
 
+    if addon.helper.driver.name == 'firefox' and addon.helper.manifest_version == 3:
+        # Seems like if v3 firefox, localhost permissions aren't granted by default
+        # (despite being declared in host_permissions manifest)
+        # so the above will result in error + opening options page so the user can approve
+        time.sleep(0.5)  # meh. give the options page time to open
+        [orig_page, options_page] = driver.window_handles
+        driver.switch_to.window(options_page)  # meh. is there a better way??
+        addon.options_page.save(wait_for_permissions=True)
+        driver.close()  # close settings
+        driver.switch_to.window(orig_page)  # meh. is there a better way??
+
+        addon.quick_capture()  # retry capture
+
     confirm('Should show a successful capture notification, and the link should be in your default capture file')
 
 
 # chrome  v3 works
 # firefox v2 works
-# firefox v3 BROKEN (sort of)
-#            seems like manifest v3 is prompting for permission even if we only change port
+# firefox v3 works
 def test_capture_bad_port(addon: Addon) -> None:
     """
     Check that we get error notification instead of silently failing if the endpoint is wrong
     """
     driver = addon.helper.driver
 
     addon.options_page.open()
-    addon.options_page.change_endpoint(endpoint='http://localhost:12345/capture')
+
+    # seems like manifest v3 in firefox is prompting for permission even if we only change port
+    wait_for_permissions = addon.helper.driver.name == 'firefox' and addon.helper.manifest_version == 3
+    addon.options_page.change_endpoint(endpoint='http://localhost:12345/capture', wait_for_permissions=wait_for_permissions)
 
     driver.get('https://example.com')
 
@@ -200,3 +215,4 @@ def test_capture_with_extra_data(addon: Addon, server: Server) -> None:
 note
 '''
     )
+    confirm("Popup should be closed")