Revert "Do not ask for root permissions when it's unnecessary"
ClosedPublic

Authored by sitter on Dec 6 2016, 10:54 AM.

Details

Summary

This reverts commit a666712102be7ef4dd48202cc2411921fc4d392b.

This broke adding new users when not setting realname or adminflag (i.e.
at present there is no way to create a !admin user at all).

Distributions, as we are still 3 weeks away from 5.8.5 I'd advise patching
this to restore working behavior for the time being.

The problem in particular is that the model gobbles up setData requests to
new rows and forwards them to newUserSetData which in turn caches them
until username&realname&admin are present and only then forwards the call
to accountsservice. By calling setData on-demand the three fields are not
set unless they in fact all where "toggled" from their default.

I suggest that the noop decision be moved into the setData itself. In there
it should be possible to accurately decide whether or not the data
actually changed and accountsservice needs to be called.

(Ideally though IMO the collection in newUserSetData should be gotten
rid of. I haven't had a close look, but creating the user with random
data for everything but username and then manipulating it on the
subsequent setData calls should be a more future-proof and reliable
approach)

BUG: 373276
CCMAIL: kde-distro-packagers@kde.org
CCMAIL: larrosa@kde.org
PHAB: https://phabricator.kde.org/D3102

Diff Detail

Repository
R128 User Manager
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
sitter updated this revision to Diff 8801.Dec 6 2016, 10:54 AM
sitter retitled this revision from to Revert "Do not ask for root permissions when it's unnecessary".
sitter updated this object.
sitter edited the test plan for this revision. (Show Details)
sitter added reviewers: davidedmundson, mart.
sitter added a subscriber: antlarr.
Restricted Application added a project: Plasma. · View Herald TranscriptDec 6 2016, 10:54 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
davidedmundson accepted this revision.Dec 9 2016, 11:22 AM
davidedmundson edited edge metadata.

I'd prefer it if you actually fixed it, instead of opting for the simplest solution. Otherwise we may go back to only having a smaller bug, but it would be better to have none.

You've already done all the research which is 90% of the work, and now it's "just" a case of making sure m_infoToSave is always set for new users right?

This revision is now accepted and ready to land.Dec 9 2016, 11:22 AM

You've already done all the research which is 90% of the work, and now it's "just" a case of making sure m_infoToSave is always set for new users right?

As I've mentioned I think this has to be solved by doing a complete 180 on the original change and make the noop decision inside the model rather than the kcm. The time hog is the testing more than the code I should think though.

This revision was automatically updated to reflect the committed changes.