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

Don't base pass-otp availability decision on hardcoded /usr/lib #499

Merged
merged 1 commit into from
Jan 7, 2020

Conversation

nh2
Copy link
Contributor

@nh2 nh2 commented Dec 20, 2019

/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 return false on platforms where the OTP functionality is certainly not supported.

CC @FiloSpaTeam as the author of 5fc6c65 (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 with pass-otp for all users as soon as possible.

/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.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 7.105% when pulling 2ca9f0e on nh2:patch-1 into b0cdc00 on IJHack:master.

@codecov
Copy link

codecov bot commented Dec 20, 2019

Codecov Report

Merging #499 into master will increase coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            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%> (ø) ⬆️

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.

@maciejsszmigiero
Copy link
Contributor

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.

@nh2
Copy link
Contributor Author

nh2 commented Dec 24, 2019

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.

@annejan
Copy link
Member

annejan commented Jan 7, 2020

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 . .

@annejan annejan merged commit 1c327f4 into IJHack:master Jan 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants