Add ability to use the new kauth helper in file ioslave
ClosedPublic

Authored by chinmoyr on Jul 22 2017, 11:34 AM.

Details

Summary

This patch adds a method execWithElevatedPrivilege() which can be used by other methods of FileProtocol class when there occurs an error due to insufficient privileges.

It takes the following arguments,

  1. int error : to check if error == EACCES. otherwise don't continue.
  2. ActionType : the c-library function to execute
  3. three QVariants: the arguments to the function

This method makes use of the signal execPrivilegeData to check if the job requires privileges to be elevated and to ask the job to show a warning/confirmation dialog.

Depends on D6197

Diff Detail

Branch
temp
Lint
No Linters Available
Unit
No Unit Test Coverage
chinmoyr created this revision.Jul 22 2017, 11:34 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptJul 22 2017, 11:34 AM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
chinmoyr edited the summary of this revision. (Show Details)Jul 22 2017, 2:43 PM
chinmoyr added reviewers: dfaure, Frameworks.
dfaure requested changes to this revision.Jul 24 2017, 6:12 PM

Good work, just some small things, and a suggested simplification to the overall slave<->job protocol.

src/core/slavebase.h
941

It's not similar to data(), it doesn't send data from the file being read ;-)

Please remove that sentence from the docu, and rename the signal (see D6832).

src/core/slaveinterface.cpp
323

Do the bytearray -> enum conversion here, so that everyone else than slavebase and slaveinterface (i.e. both the job and the slave) only see the enum.

QDataStream can be used to stream an int (the enum value) in and out, no need to use "strings" in the bytearray. Alternatively, QByteArray::number() can do the job as long as it's really just the one enum value that's needed here (and we know it will never need to be more...).

src/core/slaveinterface.h
89

Move the new value above the "add new ones here" comment, otherwise the next person will break BC.

src/ioslaves/file/file_unix.cpp
670

Wait, why the two step roundtrip here?
Instead of "can you become root?" / "yes I can" / "ok please ask the user" / "OK, the user said yes", the protocol here could really be simplified to
"request to elevate privilege" "OK (it's enabled, and the user said yes)".

Which even removes the need for any enum, it's really just one request and one reply which is true or false.

691

use C++11 initializer to avoid reallocations, i.e. QStringList testData{execAction.name(), ..., ..., ...};

Ah but then you just use a join(","), so the most efficient thing to do here would be to drop the QStringList and do

const QString metaData = execAction.name() + ',' + QString::number... + ',' + etc.
This revision now requires changes to proceed.Jul 24 2017, 6:12 PM
chinmoyr updated this revision to Diff 17202.Jul 26 2017, 7:52 AM
chinmoyr edited edge metadata.
chinmoyr marked 5 inline comments as done.
This comment was removed by chinmoyr.
chinmoyr updated this revision to Diff 17203.Jul 26 2017, 8:06 AM

Updated doc
Renamed signal to privilegeOperationRequested().
Merged request(signal) and reply into one method, isPrivilegeOperationAllowed(). Now there is no round trip.
Eliminated QStringList.

dfaure added inline comments.Jul 27 2017, 7:56 AM
src/core/slavebase.cpp
1464

Better in one line: const QByteArray buffer = "0";

src/ioslaves/file/file.h
115

Why virtual? Nothing inherits from this class, right?

dfaure added inline comments.Aug 4 2017, 8:50 PM
src/ioslaves/file/file_unix.cpp
659

with EPERM we should try as root too, no?

(e.g. for unlink, rename, chown...)

chinmoyr updated this revision to Diff 17778.Aug 6 2017, 2:35 PM
This comment was removed by chinmoyr.
chinmoyr updated this revision to Diff 17779.Aug 6 2017, 2:42 PM
  • removed virtual keyword
  • added a private header
  • minor fixes
dfaure accepted this revision.Aug 6 2017, 3:30 PM
dfaure added inline comments.
src/ioslaves/file/file_unix.cpp
677

single-quotes for the commas (','), like in my earlier recommendation.

This revision is now accepted and ready to land.Aug 6 2017, 3:30 PM
chinmoyr updated this revision to Diff 17781.Aug 6 2017, 4:34 PM
  • used single quotes
  • check for 'UnitTesting' meta-data before showing a prompt.
  • removed private header. it was supposed to appear in another commit.
dfaure added inline comments.Aug 6 2017, 6:42 PM
src/core/slavebase.h
939

missing @since 5.38, BTW

chinmoyr updated this revision to Diff 18014.Aug 11 2017, 6:14 PM
  • added @since 5.38
  • rearranged code-blocks in execWithElevatedPrivilege() method. Now the prompt is also tested in unit test mode.\ D6832 will add relevant code for testing confirmation.
dfaure accepted this revision.Aug 11 2017, 8:00 PM
chinmoyr updated this revision to Diff 18204.Aug 15 2017, 6:25 PM
  • Added enum to denote file operation state
  • Added new error code
chinmoyr updated this revision to Diff 18205.Aug 15 2017, 6:36 PM
  • Fix spelling
chinmoyr updated this revision to Diff 18206.Aug 15 2017, 6:37 PM
  • Fix spelling
dfaure requested changes to this revision.Aug 15 2017, 8:47 PM
dfaure added inline comments.
src/core/global.h
270 ↗(On Diff #18205)

Canceled and cancelled as both correct English (maybe one is UK and one is US, I don't know).

In KIO we used Canceled everywhere, though. I would prefer if this stayed consistent with the rest of KIO..

src/core/slavebase.h
944

If it returns an int, it can't really be named "isSomething" anymore, which is for boolean methods.

"Is this allowed?" "2" doesn't really work.

Can't this return an enum type rather than an int?
Also, the method should be const...

src/ioslaves/file/file_p.h
24 ↗(On Diff #18205)

#define is for 1990, these days we have a proper C++ language where preprocessor hacks are less and less needed.

An enum value is probably the cleanest way here (to avoid the whole issue of "in which .cpp file to implement it", if it was an actual int variable).

The naming EFOO is very libc-like, I wouldn't use this here.
In fact, why not use KIO::ERR_USER_CANCELED? (note that it has a value of 1, don't use 1 for something else ;)

src/ioslaves/file/file_unix.cpp
666

Who's going to read that value? The caller? Abusing errno to ship a return value via global state reads horrible to me.... we're not a libc function....

This revision now requires changes to proceed.Aug 15 2017, 8:47 PM
dfaure added inline comments.Aug 15 2017, 9:02 PM
src/core/global.h
265 ↗(On Diff #18204)

@since 5.38

267 ↗(On Diff #18204)

I suggest naming it PrivilegedOperationStatus

(Better avoid abbreviations, and "File" is kind of obvious in KIO.)

chinmoyr updated this revision to Diff 18242.Aug 16 2017, 3:59 PM
chinmoyr edited edge metadata.
  • Added variable (mUserCanceled) to denote if user canceled operation
  • Return PrivilegeOperationStatusin SlaveBase's method. Went with switch instead static_cast. Cannot make it const.
  • Cancelled -> Canceled
dfaure requested changes to this revision.Aug 16 2017, 11:02 PM
dfaure added inline comments.
src/core/slavebase.cpp
1461

Oh, indeed, can't be const, because it's not a getter, it's a method that asks for the status via communication with the app.
That means the naming could be improved.

Maybe something like queryPrivilegeOperationStatus() or requestPrivilegeOperation().

1468

Not needed, just return KIO::PrivilegeOperationStatus(buffer.toInt()) (constructor-like syntax - or static_cast if that doesn't work).

If you think casting is evil, hardcoding 1/2/3 is even more evil ;-)

And in this case casting is not evil at all, we simply serialize/deserialize the enum using its integral value.

src/core/slavebase.h
940

Four? I count only three. No need to duplicate the full enum docu anyway, it's bound to go out-of-date at some point, just refer to the enum.

src/ioslaves/file/file_unix.cpp
666

No chance to return an enum instead of a bool here, to avoid modifying state?

The problem with mUserCanceled is that it's easy to get it wrong. Like in this patch, which never resets it to false for the next operation after one canceled operation. It's just cleaner to return an enum instead of modifying a member variable temporarily, although the question is, in both cases, what the caller code will look like...

My fear with a member var is that too many callers will forget to check it.
Let's approach this from the viewpoint of the calling code.

Before:

if (!dir.rmdir(itemPath)) {
    error(KIO::ERR_CANNOT_DELETE, itemPath);
    return false;
}

After:

if (!dir.rmdir(itemPath)) {
    if (auto ret = execWithElevatedPrivilege(RMDIR, itemPath)) { // for this to work, success must be 0 (or the returned type can be a class with operator bool()...)
        if (ret == KIO::OperationCanceled) { // or a different enum
            error(KIO::ERR_USER_CANCELED, itemPath);
        } else {
            error(KIO::ERR_CANNOT_DELETE, itemPath);
        }
        return false;
    }
}

Alternatively, since we know cancelling will always need to error(KIO::ERR_USER_CANCELED) (and the QString argument doesn't matter), we could call error from within execWithElevatedPrivilege. But we still need the enum ret val:

if (!dir.rmdir(itemPath)) {
    if (auto ret = execWithElevatedPrivilege(RMDIR, itemPath)) { // see above
        if (ret != KIO::OperationCanceled) { // or a different enum
            error(KIO::ERR_CANNOT_DELETE, itemPath);
        }
        return false;
    }
}

I actually like the idea of returning a small class. Then it could even have a wasCanceled() method... we don't need an enum, we need a value that can convert to bool (for the if) and additionally let us know if cancelation happened. But all this in a temporary value, not in a member var of slavebase.

To flesh it out in more details:

class ExecWithElevatedPrivilegeReturnValue
{
public:
    static ExecWithElevatedPrivilegeReturnValue success() { return ExecWithElevatedPrivilegeReturnValue{true, false}; }
    static ExecWithElevatedPrivilegeReturnValue failure() { return ExecWithElevatedPrivilegeReturnValue{true, false}; }
    static ExecWithElevatedPrivilegeReturnValue canceled() { return ExecWithElevatedPrivilegeReturnValue{true, true}; }
    operator bool() const { return m_success; }
    bool wasCanceled() const { return m_canceled; }
private:
    ExecWithElevatedPrivilegeReturnValue(bool success, bool canceled) : m_success(success), m_canceled(canceled) {}
    const bool m_success;
    const bool m_canceled;
};

(no setters, all of the slave code shouldn't be able to change this object, it's created immediately with the right values)

With this, the calling code can be simply

if (!dir.rmdir(itemPath)) {
    if (auto ret = execWithElevatedPrivilege(RMDIR, itemPath)) {
        if (!ret.wasCanceled()) {
            error(KIO::ERR_CANNOT_DELETE, itemPath);
        }
        return false;
    }
}

and the code in this review (execWithElevatedPrivilege) can do things like return ExecWithElevatedPrivilegeReturnValue::canceled(); etc.

How does that sound?

This revision now requires changes to proceed.Aug 16 2017, 11:02 PM
chinmoyr updated this revision to Diff 18320.Aug 18 2017, 4:48 AM
chinmoyr edited edge metadata.
  • Added class PrivilegeOperationReturnValue
  • Fixed doc
  • Removed hard-coded switch..case
  • privilegeOperationStatus() -> requestPrivilegeOperation()

Returning a small class instead of enum doesn't really reduce the number of nested if's (which was why I didn't used an enum in first place). But it offers bit more clarity and readability in if..else statements. The rest depends on me.

I want to confirm if these should be the return values or not
For success : m_success=true m_canceled=false
For failure : m_success = m_failure=false
For canceled: m_success=false m_canceled=true

chinmoyr updated this revision to Diff 18508.Aug 21 2017, 5:57 PM
  • Added new variable m_failure to PrivilegeOperationReturnValue so that we can directly check for failed state rather than depending upon fallacy of other variables.
  • Added check for chmod, chown and utime operation in execWithElevatedPrivilege method
  • Added code to serialize FdReceiver's (socket) path if the requested action is either OPEN or OPENDIR.

Urgh you don't need a new variable for failure(), just do

bool failed() const {  return !m_success && !m_canceled; }

Actually, I like this construct quite a bit.

if (!dir.rmdir(itemPath)) {
    if (auto ret = execWithElevatedPrivilege(RMDIR, itemPath)) {
        if (!ret.wasCanceled()) {
            error(KIO::ERR_CANNOT_DELETE, itemPath);
        }
        return false;
    }
}

But for this to work the class should be something like this

class ExecWithElevatedPrivilegeReturnValue
{
public:
    static ExecWithElevatedPrivilegeReturnValue success() { return ExecWithElevatedPrivilegeReturnValue{false, false}; }
    static ExecWithElevatedPrivilegeReturnValue failure() { return ExecWithElevatedPrivilegeReturnValue{true, false}; }
    static ExecWithElevatedPrivilegeReturnValue canceled() { return ExecWithElevatedPrivilegeReturnValue{true, true}; }
    operator bool() const { return m_failed; }
    bool wasCanceled() const { return m_canceled; }
private:
    ExecWithElevatedPrivilegeReturnValue(bool failed, bool canceled) : m_failed(failed), m_canceled(canceled) {}
    const bool m_failed;
    const bool m_canceled;
};

am I correct?

chinmoyr updated this revision to Diff 18693.Aug 24 2017, 5:01 PM
  • Changed back to the old version of PrivilegeOperationReturnValue with slight modification. See my previous comment.
dfaure accepted this revision.Aug 24 2017, 5:15 PM

Oh I see, sorry for the bug in operator bool() in my suggested code. It reads strange that operator bool() returns true on failure, but .... yeah it's the most common use case for this class. Make sure to add a comment in that class docu, it's surely going to surprise someone some day, just like it just surprised me despite me writing the pseudo-code for it ;)
Something like this...

/**
 * PrivilegeOperationReturnValue encapsulates the return value from execWithElevatedPrivilege() in a convenient way.
 * Warning, this class will cast to a bool that is false on success and true on failure. This unusual solution allows to write
 * kioslave code like this:

if (!dir.rmdir(itemPath)) {
    if (auto ret = execWithElevatedPrivilege(RMDIR, itemPath)) {
        if (!ret.wasCanceled()) {
            error(KIO::ERR_CANNOT_DELETE, itemPath);
        }
        return false;
    }
}
// directory successfully removed, continue with the next operation
*/
This revision is now accepted and ready to land.Aug 24 2017, 5:15 PM
chinmoyr updated this revision to Diff 20311.Oct 3 2017, 5:12 PM

copied the exact comment :)

dfaure accepted this revision.Oct 7 2017, 7:02 PM

I like this comment :-)

This revision was automatically updated to reflect the committed changes.
chinmoyr reopened this revision.Oct 29 2017, 4:11 PM
This revision is now accepted and ready to land.Oct 29 2017, 4:11 PM
chinmoyr updated this revision to Diff 24578.Jan 2 2018, 11:52 AM

1.Added method privilegeOperationUnitTestMode. This replaces the previous block of code which
tested the validity of the KAuth action by looking up the action in *.policy file. I think instead of looking
for some external files its better to test if the job has the flag for privilege operation set and has a
parent job as well (in case of subjob). The method requestPrivilegeOperation is there for this exact purpose.
So for the purpose of unit test checking return value of requestPrivilegeOperation should be
sufficient.

  1. Fixed compilation issue on windows. (There was a mismatch in signature of execWithElavatedPrivileges)
  2. Changed API version to 5.43
This revision was automatically updated to reflect the committed changes.