From 0c0569635d095b8af74962403c28de148cad73ed Mon Sep 17 00:00:00 2001 From: karlicoss Date: Sun, 26 May 2024 21:47:42 +0100 Subject: [PATCH] extension: better handling of missing permissions - open extension settings on error - preserve popup state on unsuccessful capture - workaround for manifest v3 stuff in end2end tests --- extension/generate_manifest.js | 11 +++++--- extension/src/background.ts | 50 +++++++++++++++++++++------------- extension/src/options.html | 3 +- extension/src/popup.ts | 14 +++++----- tests/addon.py | 40 +++++++++++++++++++-------- tests/test_end2end.py | 24 +++++++++++++--- 6 files changed, 96 insertions(+), 46 deletions(-) diff --git a/extension/generate_manifest.js b/extension/generate_manifest.js index 4442816..2e49924 100644 --- a/extension/generate_manifest.js +++ b/extension/generate_manifest.js @@ -155,12 +155,15 @@ 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) diff --git a/extension/src/background.ts b/extension/src/background.ts index 896fbe0..e48ab8a 100644 --- a/extension/src/background.ts +++ b/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 { + /** + * 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?? - diff --git a/extension/src/options.html b/extension/src/options.html index 3b46a07..e7e5e22 100644 --- a/extension/src/options.html +++ b/extension/src/options.html @@ -10,9 +10,10 @@ +

Grasp settings

Endpoint - + diff --git a/extension/src/popup.ts b/extension/src/popup.ts index d96beb9..522afe4 100644 --- a/extension/src/popup.ts +++ b/extension/src/popup.ts @@ -84,7 +84,7 @@ async function restoreState(state: State | null) { } -function submitCapture () { +async function submitCapture () { const state = getUiState() const message = { @@ -92,16 +92,16 @@ function submitCapture () { ...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() } diff --git a/tests/addon.py b/tests/addon.py index 24d15d3..4ee54d7 100644 --- a/tests/addon.py +++ b/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) diff --git a/tests/test_end2end.py b/tests/test_end2end.py index bf352f6..d2b9144 100644 --- a/tests/test_end2end.py +++ b/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,13 +96,25 @@ 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 @@ -110,7 +122,10 @@ def test_capture_bad_port(addon: Addon) -> None: 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")