Closed nh2 closed 4 years ago
Merging #499 into master will increase coverage by
<.01%
. The diff coverage is0%
.
@@ Coverage Diff @@
## master #499 +/- ##
=========================================
+ Coverage 9.36% 9.37% +<.01%
=========================================
Files 46 46
Lines 2925 2924 -1
=========================================
Hits 274 274
+ Misses 2651 2650 -1
Impacted Files | Coverage Δ | |
---|---|---|
src/configdialog.cpp | 0% <0%> (ø) |
:arrow_up: |
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 b0cdc00...2ca9f0e. Read the comment docs.
I think the pass OTP extension path should simply be made (compile-time) configurable, so distributions can set what path they install this extension at. OTOH, if you compile QtPass yourself you can set it as you like, even to /bin/true.
OTP module autodetection is useful, please don't remove this functionality.
I think the pass OTP extension path should simply be made (compile-time) configurable, so distributions can set what path they install this extension at.
That'd do as well.
I didn't do it here because it's more sophisticated and I won't have time to do it.
While I'd be happy with that, I also am not fully convinced that that's how pass
envisioned it to be used; from man pass
you can set PASSWORD_STORE_EXTENSIONS_DIR
to anything you like at run time, thus also allowing user-provided extensions (for example in ~/.pass/.extensions
) that are not available at compile time. That's why I think that checking e.g. pass-otp --version
may be even better than your proposed /bin/true
solution.
OTP module autodetection is useful, please don't remove this functionality.
Personally I think it's a good approach to make a quick fix available immediately and work on an improvement afterwards (especially for a convenience feature like force-disabling a check box), versus having it broken until the improved solution is available.
We must remember that the only thing this check does is to prevent the user to turn on an off-by-default checkbox by hand (or at least it seemed to be off-by-default on my system), which I'd consider not really that useful compared to not being able to use it at all.
Personally I think it's a good approach to make a quick fix available immediately and work on an improvement afterwards (especially for a convenience feature like force-disabling a check box), versus having it broken until the improved solution is available.
I agree with this . .
/usr/lib
does not exist in various Linux distributions, nor does it work if you have installed your qtpass to a custom prefix such as/usr/local
or into your home directory.Disable this check for now, so that the OTP functionality can be enabled in such cases.
As a result,
ConfigDialog::isPassOtpAvailable()
now only has the functionality to returnfalse
on platforms where the OTP functionality is certainly not supported.CC @FiloSpaTeam as the author of https://github.com/IJHack/QtPass/commit/5fc6c652af6b6cc949b571c090d5e6e65228b135 (via #407); also mentioning #394 and #327 so that this PR shows up there for others that, like me, searched for this problem.
A better future detection logic might be to run
pass otp --version
and check if that succeeds, but I'm not doing that here because it's more sophisticated and I'd like qtpass to work well withpass-otp
for all users as soon as possible.