Add basic KAuth support to file ioslave
Needs ReviewPublic

Authored by chinmoyr on Mon, Jun 12, 4:18 PM.

Details

Reviewers
elvisangelaccio
Group Reviewers
Frameworks
Summary

This patch adds the relevant KAuth code to file ioslave that can be used to perform various file management operations with escalated privilege.
execWithRoot() : This method performs the specified operation as root
warningMessage(): Decides upon the warning message. As of yet it only has warning for the delete operation.
showWarning(): Shows the warning.

meta-data "UnitTesting" will be used in unit test for this code.

Test Plan

use D6199 to test the changes

Diff Detail

Repository
R241 KIO
Lint
Lint Skipped
Unit
Unit Tests Skipped
chinmoyr created this revision.Mon, Jun 12, 4:18 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMon, Jun 12, 4:18 PM
chinmoyr edited the test plan for this revision. (Show Details)Mon, Jun 12, 5:08 PM
elvisangelaccio requested changes to this revision.Sat, Jun 17, 8:52 AM

If I don't enter the authentication password, after ~20 seconds I get the "Access denied to <PATH>" error. Is this some timeout in the ioslave?

src/ioslaves/file/file.cpp
1382

Can't this go in file_unix.cpp, without ifdefs?

1387

Prefer QLatin1String if you want to concatenate. Or you could use QStringLiteral("org.kde.kio.file.%1").arg(action).

1443

I didn't get this messagebox after deleting something when I was already authenticated. What did I do wrong?

src/ioslaves/file/file.h
108

endProvidiledgeOperation() is more descriptive, no?

This revision now requires changes to proceed.Sat, Jun 17, 8:52 AM
chinmoyr updated this revision to Diff 15515.Sat, Jun 17, 11:12 AM
aacid added a subscriber: aacid.Sat, Jun 17, 4:43 PM
aacid added inline comments.
src/ioslaves/file/file.cpp
1382

Why is there an ifdef anyway? KAuth has at least a mac backend (no idea how much it works) but adding an ifdef at this level seems the wrong thing to do.

eliasp added a subscriber: eliasp.Mon, Jun 19, 8:59 AM
eliasp added inline comments.
src/ioslaves/file/file.h
107

I find execWithRoot() to be a bit misleading, as the goal shouldn't be to always elevate to root's privileges but to only what's required to execute the specific operation (e.g. browse /home/someotheruser doesn't need root's privileges but only someotheruser's privileges).

Always elevating to root is IMHO by far too permissive.

chinmoyr updated this revision to Diff 15621.Tue, Jun 20, 5:23 AM
chinmoyr marked 6 inline comments as done.
chinmoyr added a reviewer: dfaure.
chinmoyr added inline comments.
src/ioslaves/file/file.cpp
1382

This method will be used by FileProtocol::mkdir and FileProtocol::chmod which are not virtual method. So when called in windows this must return false for the time being. So IMO ifdef or something similar is necessary. What do you suggest?

src/ioslaves/file/file.h
107

I agree with you on the part that execWithRoot() is slightly misleading. So I changed it to execWithElevatedPrivilege(). And acquiring somotheruser's privileges still requires elevated privilege does it not?

chinmoyr edited the summary of this revision. (Show Details)Tue, Jun 20, 5:29 AM
aacid added inline comments.Tue, Jun 20, 10:23 PM
src/ioslaves/file/file.cpp
1382

Well, just use KAuth, since there's no backend in windows it will return false, why do you need an ifdef?

chinmoyr updated this revision to Diff 15702.Wed, Jun 21, 5:40 PM
chinmoyr marked 2 inline comments as done.

Removed the ifdef. Moved the execWithElevatedPrivilege() method to file_unix.cpp .

Looks good to me now, just minor issues.

src/ioslaves/file/file.cpp
1407–1408

Missing i18n() here

1409

Use ButtonCode::Cancel rather than hardcoding 2

chinmoyr updated this revision to Diff 15729.Thu, Jun 22, 8:35 AM
chinmoyr marked 2 inline comments as done.
chinmoyr removed a reviewer: dfaure.

QStringLiteral -> i18n