Tidying up password storage in Amarok

[Bart Cerneels]

On Mon, Apr 9, 2012 at 22:50, Andrzej J. R. Hunt <andrzej at ahunt.org> wrote:
> Hi,
>
> I'd like to propose some changes to password storage in amarok, in
> particular the way KWallet is dealt with, and what is done if not available.
>
> I noticed that there seem to be a number of different ways password storage
> is done in amarok, with each plugin having some unique way of doing things
> -- Bart made me aware of the fact that some might be buggy when KWallet
> isn't available (I originally submitted a patch copying the LastFM password
> storage to Magnatune).
>
> Most plugins use KWallet, but some resort to plaintext if KWallet isn't
> available (and some ask the user to allow this) -- meaning similar code is
> replicated over many plugins [some plugins only use plaintext currently]. I
> propose to write a wrapper class ("PasswordManager" ?), which uses KWallet
> if available, but if KWallet turns out not to be available then the user is
> asked once whether to use plaintext storage, with this setting being
> remembered across all plugins.  The end result is that a plugin only needs
> to instantiate PasswordManager with the plugin name, and then all the
> dealing with KWallet, or if necessary amarokrc, is done in once central
> place.
>
> I would also add the option to PasswordManager to check for existing
> plaintext passwords, importing them to KWallet as necessary, to ease
> migration from older to newer versions of amarok (I could also add a panel
> to the amarok config allowing configuration of the password settings, i.e.
> to allow migration from plaintext to KWallet in case KWallet wasn't initally
> available, but becomes available; or the reverse -- that would be a later
> stage).
>
> Another issue is that some plugins set a variable to store the fact that a
> password has been stored (in order to load KWallet on startup) but don't
> remove this variable if the login details are removed, meaning KWallet is
> always opened if a login has ever been set, even if no longer necessary. To
> solve such issues, nternally I would store a isPasswordStored variable in
> amarokrc for each plugin, which is set depending on whether
> username/password are empty or not. This would probably be in addition to
> any isMember variables stored by the plugin, since that way you could still
> store passwords but disable a login as desired.

Sounds sane. I'm also wondering about supporting other
keyring/password storage frameworks this way (gnome keyring, OSX,
windows, ...).

>
> A rough outline of what PasswordManager would look like to the world:
>
> PasswordManager(QString aPluginName, bool migratePlaintext=false);

Don't put the migration in the constructor, it's only supposed to be
used once. Make it a separate function or even class.
How can PluginManager know where to find the plain text password if
there is no common way to do it yet? Shouldn't the plugins themselves
migrate? Or perhaps write a one-time script that transfers them all.

> // aPluginName has to be the same as that used when getting the config group
> for the plugin
> QString username(); //Returns empty string if isPasswordStored == false,
> otherwise deals with KWallet.
> QString password(); //as above
> void setUsername(QString username);
> void setPassword(QString password);
>
> In amarokrc it would store:
> General: disableKWallet
> <Per plugin>: isPasswordStored
> [If ignoreWallet] <Per Plugin>: username and password.
>
>
> Before I actually start writing such a patch: does this all look sensible
> and useful?
>
> Cheers,
> Andrzej Hunt

Overall a good idea, I'll be happy to see patches.