Tidying up password storage in Amarok

[Andrzej J. R. Hunt]

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.

A rough outline of what PasswordManager would look like to the world:

PasswordManager(QString aPluginName, bool migratePlaintext=false);
// 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