-
Notifications
You must be signed in to change notification settings - Fork 39
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
Added option to allow use browser's built-in password manager #139
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this. I've just created some further guidelines for creating pull requests which will help you and others save time in future: https://github.com/kee-org/browser-addon/blob/master/PULL_REQUEST_TEMPLATE.md
It's just a first step but will hopefully be useful.
To summarise the general points in there which apply to this PR, please can you not change the messages.json files in other _locales (just the en source file) and also please adjust the indentation of the modified lines in KF.ts to be 4 spaces (some you've touched are 2). I can't necessarily see from GitHub whether things like line endings are consistent with the rules in the .editorconfig file so if possible, try using a code editor which supports .editorconfig files to be sure that is all correct (most text editors either support this already or have plugins that add support).
Other than those issues which are entirely down to me not having got around to writing down these guidelines anywhere yet, the main thing that would hold this back from approval is that it appears that the change will only take effect once the browser has restarted, which is less than ideal and confusing for users so I think it's important that we call browser.privacy.services.passwordSavingEnabled.set when the setting gets changed, not just during the init function.
Finally, I think the log message lines should be tweaked slightly - I've added specific comments to the relevant lines.
Thanks again and let me know if any of the above is unclear.
background/KF.ts
Outdated
{ | ||
browser.privacy.services.passwordSavingEnabled.set({ value: false }, function () { | ||
if (chrome.runtime.lastError != null) { | ||
KeeLog.warn("KeeFox was unable to disable built-in password manager saving - confusion may ensue! " + chrome.runtime.lastError); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we're here, please can you change "KeeFox" to "Kee".
background/KF.ts
Outdated
} | ||
else | ||
{ | ||
KeeLog.warn("we didn't try to disable built-in password manager saving"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please can we use this instead:
KeeLog.debug("Kee didn't try to disable built-in password manager saving");
The indentation should match the original files, I had to move some part that was wrapped in new if(){}. and may have missed a tab somewhere (I use tabs as 2 spaces (saves on file sizes)), will double check. File endings should also match the original, I'll double check. Good thing about the languages! As of setting not being applied until browser restart, I wasn't sure if this should be done or not, not a big deal to implement. |
Also, perhaps Kee should not have any routine to change the signon.rememberSignons preference on startup at all. Let the user control it via browser's settings. To make it less confusing, the new option should remain and changing it, should immediately change the system pref, but that's all. I'd be more confused if I change it in browser's settings and it changes back by itself, then I had to go through disabling every extension to find which one does it (it's faster then going through each extension's options) [EDIT] another thing, do we really need an internal preference? I mean couldn't we just pull the value directly from signon.rememberSignons pref instead? IMO this would be the best approach. |
Any reasons why this requires some npm wizardry and couldn't simply be as a from of final extension? Right now we have to edit .ts files, run
It fails on this, which works fine in addon:
I'm about to through everything out and simply provide finished addon files instead...this is madness! |
Nice one - thanks.
The init function is also invoked when the addon gets enabled or installed.
Assuming we do (and can) split the enable/install routine from typical browser startup, we would then have to either interrupt the user immediately upon first install/enable to ask them what to do, or set signon.rememberSignons pref to false every time. I don't think that's consistent with the usual behaviour of addon settings persisting during disable/enable periods and disappearing during uninstall/reinstall events so is liable to confuse users more and is liable to be rejected by the AMO review team.
Having it as just the raw extension JavaScript files would mean that we are unable to write it in Typescript. I'm not prepared to lose the benefits to productivity and code quality that using TS over JS brings so we need some way to compile the TS to JS. Sure, we could try something other than gulp but I don't think that's the true source of your frustration anyway? There's also the potential future benefits of automated testing and the existing (and likely increasing) benefits of easy building for multiple browsers).
Interesting. I think I must have run
None of this is necessary. I guess I should elaborate a bit on point 4ii in the README build instructions - it's no different to any WebExtensions addon in this respect but that's still new or unknown to many people so it's definitely on my radar to provide more detailed steps to help people get started more quickly.
I'm not certain but I think the error you are seeing is a result of the buggy browser polyfill Mozilla offer in order to aid with cross-browser WebExtensions support. I have seen a lot of errors relating to promises vs callbacks - sometimes they occur like this at compile time and sometimes at runtime (usually in Chrome rather than Firefox). There may well be an updated version of the polyfill which I hope to look into in a month or two in case it can help us to consolidate on either the So for this, see if using the I'm in two minds whether to document this or not since I don't fully understand it and it should really "just work" if the MDN docs are to be believed so I'm torn between working on half-baked documentation or working on polyfill updates or PRs to fix bugs in that. It might sometimes be an issue with the TS type definition files. Let's see what we can learn from fixing this specific compilation error and maybe a small warning note can come out of it to go into the guidelines that are currently in the PR template. Your feedback has been really useful; thanks for persevering while we get the docs up to scratch! By the way, I'm especially not happy with the structure of the gulp file build process at the moment - getting gulp to execute steps in order rather than concurrently was a right pain and I think I've only just got the hang of it. I've therefore now got a variety of changes I'm going to attempt in order to provide some improvements in development experience. It might not become faster than the XUL addon days of hacking in a JS file and restarting the browser but it shouldn't be much slower than that and will still give us all the TypeScript benefits of type checking, Intellisense, etc. I'll have at least 4 hours free to look into these build issues over the next week or so but can't promise exactly when so apologies in advance if it takes me a few days to follow up in some cases. |
#140 would be of interest. I'll leave Travis to try a build overnight and come back to it with a fresh pair of eyes sometime in the next couple of days before merging to master. |
I think you overthinking this whole situation... First of all, the addon doesn't work upon initial install without user's interaction, so this whole "interrupting user" is happening regardless. By opening options page upon first install should be enough for the user to decide if they want this setting still be enabled or not. Since it will be user's decision about that setting, the addon won't need to worry about what to do with it upon enable/disable. |
It's true that I am thinking more widely than just the specifics of this PR; I feel that have to do this in order to ensure that any specific PR improves Kee without risking the introduction of new problems down the line, potentially in seemingly unrelated aspects of the code or user experience. It sounds like you're putting forward a similar argument that I've heard in other cases: "Getting Kee to work in the first place is confusing so there is a certain required level of technical knowledge, experience or intelligence in order to use it anyway, therefore we don't need to worry too much about introducing new questions/settings/steps/behaviours that only a subset of people are able to understand". Have I understood you correctly? If that's the case, then I'm afraid I don't really feel the same way. For example, the ability required to set up Kee is only a fraction above that required to operate a web browser on a desktop computer; I don't think that the need to follow some instructions to perform typical desktop PC operations such as installing a program and moving files around is justification for an unlimited number of further cognitive barriers being put in place. Further, there are some people who use Kee for years without really using KeePass beyond an interface to enter a master password. Yes, the KeePass application as a whole can be very complex for some people to understand but I don't think most people need to learn of those complexities, at least not when first installing Kee. If there is no alternative to introducing extra steps at install time, we can weigh up whether the benefits of such steps outweigh the drawbacks but in the vast majority of cases there is an alternative. In this specific case I think the alternative is to add a little more complexity to the implementation (in the form of maintaining this separate Kee-specifc record of the user's preference as per the original plan on #55) - I prefer that to leaving the end user to face even a small amount of increased complexity. Do you agree? |
@vanowm , I've now merged #140 into master so if you rebase (or perhaps do a fresh checkout into a new folder and then reapply your code changes?) you should find that the build/development guidelines on README.md are now more reliable and detailed. NB: You shouldn't need to directly invoke gulp anymore so whether it is installed globally or not should not make a difference. Let me know how you get on since I'm keen to improve the experience for an initial clone+build if it's still not good enough. |
The new built is great, very easy to use. Still gives me same error as above, but I found a work around. Before I update my commit, I couldn't find if Kee has ability track if it was just installed (for the first time)? At the moment I've added new setting What I'm planning to do is disable |
Glad to hear the new build is good. Thanks for the feedback. Firefox does not allow an add-on to detect if it is the first time it was installed. All add-on configuration and settings are removed when the user uninstalls an add-on. Checking the value of the new That won't let the user change the setting via about:config or the main browser preferences screen but I think that's the correct behaviour - users need to enable the ability to end up in this advanced configuration position only after seeing and understanding that it is controlled via Kee and that it is not recommended for typical use cases - that's only possible if they go via the Kee settings/options page. |
Since WE doesn't have ability alter GUI of the browser and doesn't allow remove that setting from browser's options, this behaviour is not welcome, it's more confusing then two totally different, yet self explanatory popups... |
When is this feature to become available? |
Sounds like the owner of this PR isn't going to continue work on it and it's got stale so I'm closing it. Please feel free to create a new one per the discussion and spec in #55 |
Added option to allow use browser's built-in password manager (issue #55)