handle wrong password when using sudo which asks for another password
BUG: 389049
Details
Details
build and run with sudo and su options, test password and no, test diff languages
Diff Detail
Diff Detail
- Repository
- R299 KDESu
- Branch
- master
- Lint
No Linters Available - Unit
No Unit Test Coverage
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
As mentioned on IRC I think this would benefit greatly from some unit testing as that line check is dangerously close to requiring mental gymnastics to read. AT a glance, all that's needed is writing stub sudo/su helpers and a test asserting outcome of SuProcess('us0r', 'PATH/fakesudo').converseSU('passwd') in the given scenarios (password good su&sudo, password bad su&sudo).
Comment Actions
Add test. This uses the real sudo and su on your system, I fear that creating a stub replacement wouldn't be necessarily a reliable recreation. It does mean adding your own password.h with #defines for your user and root password
Comment Actions
I am not sure what you mean. If you create a stub from what you observe right now then that is a reliable recreation of how sudo 1.89 / su 4.2 behave, is it not?
Comment Actions
- in stub su and sudo use passed argument to find kdesu_stub and in suprocess pass local kdesu_stub for test mode
| src/suprocess.cpp | ||
|---|---|---|
| 120 ↗ | (On Diff #27691) | This means the path to the build directory is hardcoded in the executable. That needs to be avoided to make builds fully reproducible (https://reproducible-builds.org/). |
Comment Actions
- write out path to kdesu_stub and su command from the tests to a config rather than setting it within the suprocess class, allows for other testing possibilities and binary relateability
Comment Actions
What you want is QStandardPaths::setTestModeEnabled(true);.
Some more general comments:
- Autotests need to be conditional based on BUILD_TESTING
- Is it necessary to write the mock su/sudo in python? That introduces a big and mostly unnecessary dependency on python.
- By adding a new constructor to SuCommand which allows to specify the full path to su/sudo, testing would be much easier. It might be usable outside of the tests as well.
- The testcases would be simpler if it used QTESTDATA and rows for the su/sudo and correct/incorrect password cases
| autotests/kdesutest.cpp | ||
|---|---|---|
| 3 ↗ | (On Diff #49175) | Try to make #include "suprocess.h" work instead. |
| 13 ↗ | (On Diff #49175) | Should be above the defines. |
| 45 ↗ | (On Diff #49175) | The config modification should be split into a separate method. |
| src/suprocess.cpp | ||
| 34 ↗ | (On Diff #27691) | If autotests aren't built, this isn't available. AFAICT it's not necessary anyway. |
| 42 ↗ | (On Diff #27691) | Not used anymore. |
Comment Actions
Is it necessary to write the mock su/sudo in python? That introduces a big and mostly unnecessary dependency on python.
Nope but that's what was easiest for me and I'm out of energy for this
By adding a new constructor to SuCommand which allows to specify the full path to su/sudo, testing would be much easier. It might be usable outside of the tests as well.
I looked at that but I'd be scared of introducing binary incompatiblity. This approach covers the current needs.
The testcases would be simpler if it used QTESTDATA and rows for the su/sudo and correct/incorrect password cases
Something else I need to learn about.
Comment Actions
- remove unused variable
- remove unused include
- use QStandardPaths::setTestModeEnabled
- tidy up includes and defines
- refactor out config setting to remove duplication
| autotests/su | ||
|---|---|---|
| 28 ↗ | (On Diff #49091) | Not sure what you mean by won't work anywhere else, it'll work using the kdesu_stub which is built by the local compile. It doens't need kdesu_stub to be installed. |
Comment Actions
Ok, those aren't major issues anyway.
By adding a new constructor to SuCommand which allows to specify the full path to su/sudo, testing would be much easier. It might be usable outside of the tests as well.
I looked at that but I'd be scared of introducing binary incompatiblity. This approach covers the current needs.
I don't really like that there's now a config entry only used for and during testing, maybe an environment variable would work?
Other than that, this is still missing:
- Autotests need to be conditional based on BUILD_TESTING
It would be nice to have a check for python3 as well, but IMO the message on failure should be obvious enough.
| autotests/su | ||
|---|---|---|
| 28 ↗ | (On Diff #49091) | Now it doesn't - the comment was referring to the hardcoded path to kdesu_stub, which is now gone. To avoid such misleading comments in the future, you can mark a comment as done to prevent it from reappearing out of context. |
| src/suprocess.cpp | ||
| 122 ↗ | (On Diff #27691) | Use QStringLiteral instead. |
| src/suprocess.h | ||
| 75 ↗ | (On Diff #49244) | Still necessary? |
Comment Actions
BUILD_TESTING easy fix: https://phabricator.kde.org/source/knotifyconfig/browse/master/CMakeLists.txt$60
| src/suprocess.h | ||
|---|---|---|
| 75 ↗ | (On Diff #49244) | yes |
| src/suprocess.h | ||
|---|---|---|
| 75 ↗ | (On Diff #49244) | Just use 0 instead - the enum isn't meant for that apparently. |
Comment Actions
- make string const, follow normal code style of pointer * being with variable name not the type
| src/suprocess.h | ||
|---|---|---|
| 75 ↗ | (On Diff #49244) | This is still there, so not done yet. The enum SuErrors is (apparently) not meant to be used by callers of exec as they have different accessibility. |