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

Create right-click menu #284

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
8 changes: 8 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ In order to use Browserpass you must also install a [companion native messaging
- [Organizing password store](#organizing-password-store)
- [First steps in browser extension](#first-steps-in-browser-extension)
- [Available keyboard shortcuts](#available-keyboard-shortcuts)
- [Usage via right-click menu](#usage-via-right-click-menu)
- [Password matching and sorting](#password-matching-and-sorting)
- [Searching password entries](#searching-password-entries)
- [OpenID authentication](#openid-authentication)
Expand Down Expand Up @@ -160,6 +161,12 @@ Note: If the cursor is located in the search input field, every shortcut that wo
| <kbd>Ctrl+Shift+G</kbd> | Open URL in the new tab |
| <kbd>Backspace</kbd> (with no search text entered) | Search passwords in the entire password store |

### Usage via right-click menu

You can right-click anywhere a visited website and there will appear a menu with an option `Browserpass - <n> entries`, where `n` is the number of entries that match the host of the visited website. When you select an entry, that one gets automatically filled in, equivalent to the behavior when an entry is selected from the Browserpass popup. This can be helpful if you want to fill credentials in a browser popup window without extension buttons. Selecting single form fields and choosing values to fill in is currently not supported

![The right-click menu of browserpass](https://user-images.githubusercontent.com/15818773/153720814-66b99653-3b8a-456d-a55a-8dd208e66028.gif)

### Password matching and sorting

When you first open the Browserpass popup, you will see a badge with the current domain name in the search input field:
Expand Down Expand Up @@ -300,6 +307,7 @@ Browserpass extension requests the following permissions:
| `tabs` | To get URL of a given tab, used for example to set count of the matching passwords for a given tab |
| `clipboardRead` | To ensure only copied credentials and not other content is cleared from the clipboard after 60 seconds |
| `clipboardWrite` | For "Copy password" and "Copy username" functionality |
| `contextMenus` | To create a context menu, also called right-click menu |
| `nativeMessaging` | To allow communication with the native app |
| `notifications` | To show browser notifications on install or update |
| `webRequest` | For modal HTTP authentication |
Expand Down
64 changes: 64 additions & 0 deletions src/background.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ chrome.browserAction.setBadgeBackgroundColor({
chrome.tabs.onUpdated.addListener((tabId, info) => {
// unregister any auth listeners for this tab
if (info.status === "complete") {
createContextMenu();
Copy link
Member

Choose a reason for hiding this comment

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

While testing I managed to get some errors in the console when I was opening a new tab, which is because of concurrent attempts to recreate context menu. Here's what happens:

createContextMenu is async, so by calling it here we don't really block browser, there's no way of knowing when the function completes. In the beginning of createContextMenu we first remove the context menu and then try to recreate it. At the same time, chrome.tabs.onActivated will get called, in turn calling createContextMenu in parallel. The previous run has just created the parent menu, this run removes it, and then the previous run tries to create children to a now gone parent, and crashes.

Of course one way to reduce the chances of a crash is to move chrome.contextMenus.removeAll() from the beginning of the function to as much later as possible - but this will not entirely eliminate the problem, but rather just reduce its chances.

Another problem is that here you actually call createContextMenu only on complete event - I understand why you did it, but it presents a problem on slow websites. I open github.com and my context menu is filled with entries. I then go in this tab to some very slow website, which loads a ton of images, and so complete event will only fire in say 5 seconds. During the first 5 seconds if I right click, I will still see entries for github.

I propose to repeat what updateMatchingPasswordsCount function does:

  • be called on all statuses, not just on complete
  • use some kind of analogue to isRefreshing variable to prevent concurrent runs
  • probably you won't need chrome.tabs.onActivated below, because badge seems to not need it...

Let me know if it makes sense and if you have any questions

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the thorough review, I can reproduce but haven't figured out how to solve it completely yet. I'll keep you posted.

Copy link
Author

Choose a reason for hiding this comment

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

I think I figured it out 🎉. I call createContextMenu now on every status, but I need chrome.tabs.onActivated to "kill" callbacks that are being called from an "old" tab.

It's a bit hard to formulate, so here's an example: I'm in tab A with a slow page, I switch to tab B before tab A finished loading, when tab A finally finished loading I don't want it to mess up the context menu.

Copy link
Collaborator

@erayd erayd Feb 21, 2022

Choose a reason for hiding this comment

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

@fkneist Would simply including the origin in the callback be sufficient? That way you can just discard any calls that have a different origin than the currently active one.


if (authListeners[tabId]) {
chrome.webRequest.onAuthRequired.removeListener(authListeners[tabId]);
delete authListeners[tabId];
Expand All @@ -51,6 +53,10 @@ chrome.tabs.onUpdated.addListener((tabId, info) => {
updateMatchingPasswordsCount(tabId);
});

chrome.tabs.onActivated.addListener(() => {
createContextMenu();
});

// handle incoming messages
chrome.runtime.onMessage.addListener(function (message, sender, sendResponse) {
receiveMessage(message, sender, sendResponse);
Expand Down Expand Up @@ -1156,3 +1162,61 @@ function onExtensionInstalled(details) {
});
}
}

/**
* Create a context menu, also called right-click menu
*
* @since 3.8.0
*
* @return void
*/
async function createContextMenu() {
await chrome.contextMenus.removeAll();

const menuEntryProps = {
contexts: ["all"],
type: "normal",
};
const menuEntryId = "menuEntry";

const settings = await getFullSettings();
const response = await hostAction(settings, "list");

if (response.status != "ok") {
throw new Error(JSON.stringify(response));
}
const files = helpers.ignoreFiles(response.data.files, settings);
const logins = helpers.prepareLogins(files, settings);
const loginsForThisHost = helpers.filterSortLogins(logins, "", true);
maximbaz marked this conversation as resolved.
Show resolved Hide resolved
const numberOfLoginsForThisHost = loginsForThisHost.length;
const singularOrPlural = numberOfLoginsForThisHost === 1 ? 'entry' : 'entries'

await chrome.contextMenus.create({
...menuEntryProps,
title: `Browserpass - ${numberOfLoginsForThisHost} ${singularOrPlural}`,
Copy link
Member

Choose a reason for hiding this comment

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

Although it's cool to be able to have a dynamic title, honestly I don't see how it is useful - except to indicate that there are 0 entries, in which case I think it's better to not take up space in context menu at all... I'd just prefer a static minimalistic "Browserpass" title...

Copy link
Author

@fkneist fkneist Feb 12, 2022

Choose a reason for hiding this comment

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

I can live with that :) In combination with the not showing if 0 matches, the presence of the menu entry now indicates that there is at least one match

Copy link
Member

Choose a reason for hiding this comment

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

Cool :) Would that be too much to ask to rerecord the gif? Not important as you'll change UI in the next PR anyway but if it's super quick for you...

Copy link
Author

Choose a reason for hiding this comment

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

here we go:
browserpass-right-click-2022-02-21

id: menuEntryId,
});

for (let i = 0; i < numberOfLoginsForThisHost; i++) {
await chrome.contextMenus.create({
...menuEntryProps,
parentId: menuEntryId,
id: "login" + i,
title: loginsForThisHost[i].login,
onclick: () => clickMenuEntry(settings, loginsForThisHost[i]),
});
}
}

/**
* Handle the click of a context menu item
*
* @since 3.8.0
*
* @param object settings Full settings object
* @param array login Filtered and sorted list of logins
Copy link
Member

Choose a reason for hiding this comment

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

I believe this comment is not true, it's not a list of logins right?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, you're right. Btw that reminds me of an idea that I had: what would you think about porting the project to TypeScript, then part of the documentation would become unnecessary and wouldn't need to be manually maintained + it would maybe be a bit easier to understand input/output parameters quickly.

Let me know what you think about that, I'd be interested to look into it.

Copy link
Collaborator

@erayd erayd Feb 21, 2022

Choose a reason for hiding this comment

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

what would you think about porting the project to TypeScript

I'd prefer not to do that at this point. It's a discussion for another time perhaps, but in the more immediate term I'd rather get the current backlog of stuff cleared and ready for release.

* @return void
*/
async function clickMenuEntry(settings, login) {
await handleMessage(settings, { action: "fill", login }, () => {});
Copy link
Member

Choose a reason for hiding this comment

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

The last parameter to handleMessage currently does nothing but should at least log some error in background page's console. Above, on line 1186, you throw an error and it does end up in background page's console (easy to test by swapping the condition to == 'ok', it's something to test here as well, perhaps just throwing an error would be sufficient?

}
3 changes: 2 additions & 1 deletion src/manifest-chromium.json
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@
"webRequest",
"webRequestBlocking",
"http://*/*",
"https://*/*"
"https://*/*",
"contextMenus"
],
"content_security_policy": "default-src 'none'; font-src 'self'; img-src 'self' data:; script-src 'self'; style-src 'self'",
"commands": {
Expand Down
3 changes: 2 additions & 1 deletion src/manifest-firefox.json
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@
"webRequest",
"webRequestBlocking",
"http://*/*",
"https://*/*"
"https://*/*",
"contextMenus"
],
"content_security_policy": "default-src 'none'; font-src 'self'; img-src 'self' data:; script-src 'self'; style-src 'self'",
"applications": {
Expand Down