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

Main window entry details improvements #477

Merged
merged 4 commits into from Sep 28, 2019
Merged

Conversation

@maciejsszmigiero
Copy link
Contributor

maciejsszmigiero commented Sep 27, 2019

This PR contains two main window entry details improvements:

  1. Make password entry non-template details actually display upon selecting the entry in the main
    QtPass window (fixes a regression introduced by commit 3cb140c),

  2. Don't show a TOTP secret when selecting a password entry since it isn't a proper way to utilize a
    OTP and also it isn't safe.

Also included here is a commit that removes an unused signal and an unused function from
MainWindow class.

… MainWindow
QtPass::connectPassSignalHandlers() will be called twice, the first time
for the real pass and the second time for a pass imitator.
This means that we can't connect MainWindow::passShowHandlerFinished signal
in this function or it will be connected (and so then invoked) twice.

Connect it in the QtPass::connectPassSignalHandlers() single caller
instead.
Commit 3cb140c ("Trying to use QtPass as process handler and connector for Pass")
removed a DisplayInTextBrowser() call from MainWindow::passShowHandler()
and added a similar QtPass::showInTextBrowser() function that is invoked by
QtPass::passShowHandlerFinished() slot.

This slot is in turn connected to MainWindow::passShowHandlerFinished
signal which wasn't emitted anywhere in the code.

Emit it where the old DisplayInTextBrowser() call was so password entry
non-template details are actually displayed upon selecting it in the main
QtPass window.
…window

Knowing the TOTP secret for a password entry allows somebody to recreate
the whole OTP sequence so it definitely shouldn't be displayed in the
clear.

In fact, it shouldn't be displayed at all in the main window since the
proper way to utilize a TOTP entry is to click the "OTP" button to generate
a new OTP (rather than to copy the secret to the clipboard like it was a
password).

The password edit dialog isn't affected by this change and will still show
the whole entry, including its TOTP secret if present.
@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 27, 2019

Coverage Status

Coverage increased (+0.1%) to 7.049% when pulling 0498dd6 on maciejsszmigiero:main-window-entry-details-improvements into df43cec on IJHack:master.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Sep 27, 2019

Codecov Report

Merging #477 into master will increase coverage by 0.16%.
The diff coverage is 57.14%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #477      +/-   ##
=========================================
+ Coverage     7.3%   7.46%   +0.16%     
=========================================
  Files          44      44              
  Lines        2806    2812       +6     
=========================================
+ Hits          205     210       +5     
- Misses       2601    2602       +1
Impacted Files Coverage Δ
src/mainwindow.h 0% <ø> (ø) ⬆️
src/filecontent.h 100% <ø> (ø) ⬆️
src/mainwindow.cpp 0% <0%> (ø) ⬆️
src/qtpass.cpp 0% <0%> (ø) ⬆️
src/filecontent.cpp 94.44% <80%> (-5.56%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df43cec...0498dd6. Read the comment docs.

@annejan annejan merged commit 37472bc into IJHack:master Sep 28, 2019
5 checks passed
5 checks passed
CodeFactor No issues found.
Details
codecov/patch 57.14% of diff hit (target 7.3%)
Details
codecov/project 7.46% (+0.16%) compared to df43cec
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.