Bug 351056 - useless migration wizard
Summary: useless migration wizard
Status: VERIFIED FIXED
Alias: None
Product: frameworks-kwallet
Classification: Frameworks and Libraries
Component: general (show other bugs)
Version: unspecified
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: Valentin Rusu
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-08-07 11:15 UTC by Harald Sitter
Modified: 2015-08-24 08:44 UTC (History)
3 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Harald Sitter 2015-08-07 11:15:11 UTC
kwalletd5 presents a migration wizard when it detects kwalletd4 running.
this wizard appears not very useful for two reasons:

a) a user by default would always prefer not to loose their passwords, so migration is the default
b) not migrating is functionally equivalent to migrating automatically and the user then deleting the migrated wallet

It is therefore my believe that it would be very good if kwallet didn't interrupt the user with useless migration wizards but instead migrated by default and if the user were to want an empty wallet they can delete and create a fresh one.

Additionally this becomes an even greater problem with the fact that it always wants to migrate right now [1] regardless of whether there actually is content in kwalletd4 to migrate, which in of itself is pretty bad.

[1] https://bugs.launchpad.net/ubuntu/+source/kwallet-kf5/+bug/1434052

Reproducible: Always
Comment 1 Martin Klapetek 2015-08-07 11:25:53 UTC
I agree with Harald that migration should happen automatically, behind the scenes, so that everything seems very seamless (wrt switching from kdelibs app to kf5 app) and the user is happy by not having to do anything while everything just works(tm).
Comment 2 Valentin Rusu 2015-08-07 14:33:08 UTC
Could be done. However, wouldn't it be nice for the user to display some message box after migrating just to keep him informed?

Oh, and I see another problem. Even if the migration starts in the background, the new wallet creation request will prompt the user for a password. So, without the migration wizard context, that dialog would pop-up out of the blue. The user may get even more confused in that case, IMHO.

Waiting for your replies, I'll check the code to see if the pam module could help preventing that new wallet password prompt.
Comment 3 Harald Sitter 2015-08-07 15:19:48 UTC
Regarding the message box, I think even a notification is not necessary for the migration. It should  "just work". The user doesn't really need to know about this if everything goes according to plan and the migration works. If the migration fails however it would absolutely make sense to show a notification that allows the user to inspect the problem (look at the log view?) and/or file a bug etc.

kwallet-pam solves the password dialog as the user should not get any dialog if pam can unlock the wallet, so that should not be a problem for 99% of mainstream users.
In instances without kwallet-pam I do not think this would be the only case where kwallet throws arbitrary password requests without context (plasma-nm wifi for example?). It's really more of a general design flaw than a problem limited to the this particular instance unfortunately.

Also I just noticed that this wizard essentially renders kwallet-pam user experience broken on systems with both kwallet4 and kwallet5.
In order to allow existing wallets to migrate I understand that kwallet5 must be started after kwallet4, so kwallet-pam systems would *always* run into the launchpad bug I mentioned above where kwalletd5 wants to migrate an empty wallet for new users.
With kwallet-pam being part of Plasma 5.4 we really need to get this fixed as soon as possible to not have problems with the user experience for too long :/
Comment 4 Valentin Rusu 2015-08-07 16:18:11 UTC
(In reply to Harald Sitter from comment #3)
> Regarding the message box, I think even a notification is not necessary for
> the migration. It should  "just work". The user doesn't really need to know
> about this if everything goes according to plan and the migration works. If
> the migration fails however it would absolutely make sense to show a
> notification that allows the user to inspect the problem (look at the log
> view?) and/or file a bug etc.

Ok for that, makes sense.

> Also I just noticed that this wizard essentially renders kwallet-pam user
> experience broken on systems with both kwallet4 and kwallet5.
> In order to allow existing wallets to migrate I understand that kwallet5
> must be started after kwallet4, 

That's not exact. The migration wizard actually handles kwalletd4 start via QDBusConnection::startService()

> so kwallet-pam systems would *always* run
> into the launchpad bug I mentioned above where kwalletd5 wants to migrate an
> empty wallet for new users.

That's not exact because of what I pointed-out just before.

> With kwallet-pam being part of Plasma 5.4 we really need to get this fixed
> as soon as possible to not have problems with the user experience for too
> long :/

User experience is important, that's true. However, kwallet-pam have some design flaws that creates some problems here. For instance, it assumes user only has one wallet, always named "kdewallet". See BUG:334373 for more information. 

On the other hand, the migration wizard gracefully enumerates old wallets and attempt to re-create them with the same names. Users may be quite happy with that, IMHO.

As a matter of fact, this request will lead to some hacky code in the migration agent in order to adjust for the hard-coded wallet created by the pam module. 

Finally, I won't remove the migration wizard code. I'll introduce a configuration flag allowing it to be brought back by the users.
Comment 5 Valentin Rusu 2015-08-07 17:04:00 UTC
Git commit 127efedd1668b546d0ac8c83655a2056d0439f29 by Valentin Rusu.
Committed on 07/08/2015 at 16:59.
Pushed by vrusu into branch 'master'.

Stop showing the migration wizard by default

If the migration wizard is needed, then add this to kwalletrc
[Migration]
showMigrationWizard=true

On systems having kwallet-pam the migration agent would also merge all the old
wallets into the default LocalWallet, as a side effect. This would avoid
wallet creation prompts, though.

M  +1    -1    src/runtime/kwalletd/main.cpp
M  +34   -8    src/runtime/kwalletd/migrationagent.cpp
M  +3    -2    src/runtime/kwalletd/migrationagent.h
M  +1    -1    src/runtime/kwalletd/migrationwizard.cpp

http://commits.kde.org/kwallet-framework/127efedd1668b546d0ac8c83655a2056d0439f29
Comment 6 Valentin Rusu 2015-08-07 17:05:07 UTC
This should now work as expected. However, could you please test and return here if further work should be done.
Comment 7 Harald Sitter 2015-08-24 08:44:23 UTC
Seems to work splendidly. Thanks Valentin!