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

Add support for browsers integration: a first step #175

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@shaform

shaform commented Aug 7, 2016

It appears that #110, #111 are not being merged due to multiple concerns:

  • Too much unrelated commits to be cleaned up.
  • Copyright concerns.
  • Unit tests.
  • Coding style.
  • One class per file.

Therefore, this pull request attempts to resolve this issue by only introducing a very early implementation (Typz/keepassx@ea992bc), which has already been released under GPL (Typz#1). I also made some modifications (remove qjson & qhttpserver dependency and use libevent instead) to make it compatible with 2.0.

Once the change is merged, it would be easier to introduce further incremental changes to support full feature set.

As the change is small, it could also be potentially easier for me to resolve coding style and unit test issues while continuously aligning the code with the current master branch.

So the only potential blocker may be the plugin architecture:

debfx commented on Oct 16, 2015
KeePassX needs to gain a proper plugin architecture. I plan to work on this once 2.0 is released.

But perhaps we could merge it first and refactor again when the plugin architecture comes. What do you think?

  • One class per file
  • Qt 5
  • Coding Style
  • Unit tests
    • HttpServer
    • Request
    • Response
    • Server
    • Service
@fcore117

This comment has been minimized.

Show comment
Hide comment
@fcore117

fcore117 Aug 13, 2016

Wonderful, thank you and i really hope this will be merged so i can quit KeePass and abandon firefox and can use chromium. I plan to nuke .net framework out of windows with winreducer and keepass is last obstacle(currently i have to use keepass with ChromeIPass addon).

fcore117 commented Aug 13, 2016

Wonderful, thank you and i really hope this will be merged so i can quit KeePass and abandon firefox and can use chromium. I plan to nuke .net framework out of windows with winreducer and keepass is last obstacle(currently i have to use keepass with ChromeIPass addon).

@shaform shaform changed the title from [WIP] Add support for browsers integration: a first step to Add support for browsers integration: a first step Aug 14, 2016

@shaform

This comment has been minimized.

Show comment
Hide comment
@shaform

shaform Aug 15, 2016

@fcore117
Thanks for the support. I am also a KeePass user looking to switch to KeePassX!

I've finally completed basic unit tests for each component. So this PR should be ready for review now.

shaform commented Aug 15, 2016

@fcore117
Thanks for the support. I am also a KeePass user looking to switch to KeePassX!

I've finally completed basic unit tests for each component. So this PR should be ready for review now.

@bradst

This comment has been minimized.

Show comment
Hide comment
@bradst

bradst Aug 15, 2016

Thanks for working on this! Hopefully this PR will have an easier time being merged. It's a shame that so many people can't switch to KeePassX because it lacks browser integration.

bradst commented Aug 15, 2016

Thanks for working on this! Hopefully this PR will have an easier time being merged. It's a shame that so many people can't switch to KeePassX because it lacks browser integration.

@rockihack

This comment has been minimized.

Show comment
Hide comment
@rockihack

rockihack Aug 15, 2016

Contributor

A lot of people demand browser integration, but personally I do not want any kind of networking code in my password manager. We need a plugin system for this kind of features.

Contributor

rockihack commented Aug 15, 2016

A lot of people demand browser integration, but personally I do not want any kind of networking code in my password manager. We need a plugin system for this kind of features.

@fcore117

This comment has been minimized.

Show comment
Hide comment
@fcore117

fcore117 Aug 17, 2016

plugins or not but as of now it is urgent need, rockihack: you could just disable it if you do not want and anyway it is local lan only.

fcore117 commented Aug 17, 2016

plugins or not but as of now it is urgent need, rockihack: you could just disable it if you do not want and anyway it is local lan only.

@rockihack

This comment has been minimized.

Show comment
Hide comment
@rockihack

rockihack Aug 18, 2016

Contributor

@fcore117 I know it listens on the loopback interface per default,
but I don't want any networking code in my password manager at all.
It intodruces new kinds of security issues (e.g. API to receive passwords)
and if the code is included it can be enabled and abused.

Contributor

rockihack commented Aug 18, 2016

@fcore117 I know it listens on the loopback interface per default,
but I don't want any networking code in my password manager at all.
It intodruces new kinds of security issues (e.g. API to receive passwords)
and if the code is included it can be enabled and abused.

@jvoisin

This comment has been minimized.

Show comment
Hide comment
@jvoisin

jvoisin Sep 22, 2016

I agree with @rockihack. There has been a lot of vulnerabilities recently with password-manager implementing browser integration.

I also don't think that "it is urgent need" is a wise argument for merging something ; I would really prefer to have this in a plugin that has to be installed separately, instead of Keepassx core.

jvoisin commented Sep 22, 2016

I agree with @rockihack. There has been a lot of vulnerabilities recently with password-manager implementing browser integration.

I also don't think that "it is urgent need" is a wise argument for merging something ; I would really prefer to have this in a plugin that has to be installed separately, instead of Keepassx core.

@mossholderm

This comment has been minimized.

Show comment
Hide comment
@mossholderm

mossholderm Sep 22, 2016

On Sep 22, 2016 3:05 AM, "jvoisin" notifications@github.com wrote:

I agree with @rockihack. There has been a lot of vulnerabilities recently
with password-manager implementing browser integration.

I also don't think that "it is urgent need" is a wise argument for
merging something ; I would really prefer to have this in a plugin that has
to be installed separately, instead of Keepassx core.

There are a lot of people who either want or don't want this, so a binary
decision is probably not the correct approach. Why not put it behind a
compile-time flag?

A plugin at a later date would be better, but there is no such
infrastructure today. We have the code, so why not implement it for those
who need it, but make it easily disabled?

mossholderm commented Sep 22, 2016

On Sep 22, 2016 3:05 AM, "jvoisin" notifications@github.com wrote:

I agree with @rockihack. There has been a lot of vulnerabilities recently
with password-manager implementing browser integration.

I also don't think that "it is urgent need" is a wise argument for
merging something ; I would really prefer to have this in a plugin that has
to be installed separately, instead of Keepassx core.

There are a lot of people who either want or don't want this, so a binary
decision is probably not the correct approach. Why not put it behind a
compile-time flag?

A plugin at a later date would be better, but there is no such
infrastructure today. We have the code, so why not implement it for those
who need it, but make it easily disabled?

@fcore117

This comment has been minimized.

Show comment
Hide comment
@fcore117

fcore117 Sep 22, 2016

If google had integrated ultrastrong master password system then chromium could work without external password managers.

fcore117 commented Sep 22, 2016

If google had integrated ultrastrong master password system then chromium could work without external password managers.

@shaform

This comment has been minimized.

Show comment
Hide comment
@shaform

shaform Apr 29, 2017

Closing this issues since we have https://github.com/keepassxreboot/keepassxc, which already supports browser integration, and it looks like this PR is unlikely to be merged.

shaform commented Apr 29, 2017

Closing this issues since we have https://github.com/keepassxreboot/keepassxc, which already supports browser integration, and it looks like this PR is unlikely to be merged.

@shaform shaform closed this Apr 29, 2017

@fcore117

This comment has been minimized.

Show comment
Hide comment
@fcore117

fcore117 Apr 29, 2017

yep, using KeepassXC over 2 months now and works perfect like it should be with Chromium. Finally i have dropped that .net based keepass which was much slower.

fcore117 commented Apr 29, 2017

yep, using KeepassXC over 2 months now and works perfect like it should be with Chromium. Finally i have dropped that .net based keepass which was much slower.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment