search

cfangmeier / tuijam

A fancy TUI client for Google Play Music
MIT License
129 stars 9 forks source link

Last.fm scrobbling is broken #42

Closed t1meshift closed 5 years ago

t1meshift commented 5 years ago

I guess this is related with OAuth login. Also, I should notice that a non-encrypted token could easily be stolen. Of course, that's way better than a password leak, but that's still an issue.

Maybe we need to find a way to extract a secret key from config. I don't think that every single user would use their own script to decrypt a config :)

cfangmeier commented 5 years ago

LastFM scrobbling is still working for me. Can you check that you've ran 

tuijam configure_last_fm

and have lastfm_sk in your config file?

t1meshift commented 5 years ago

I have lastfm_sk in my config file, but the thing is I have it encrypted since PR #32 :) It has been decrypting every time I enter my config password, but now the player does not prompt password (https://github.com/cfangmeier/tuijam/commit/393ec82336ac27fff5e411c37789b74450886306#diff-9ba3d0b631977fd8339d9e7909c66211L130-L137).

In my opinion, we need to make config transition from old versions (not mandatory so a user could reject it, though.) If there is encrypted field in a config, then lastfm_sk should be decrypted or removed. If removed, then show an alert to a user that they need to re-run configure_last_fm. device_id, email and password fields should be removed as obsolete regardless of encryption status.

Despite it's my whim, it could help old users :)

Another solution is just to keep things as they are. At first, this issue can be easily found and token regeneration is not rocket science. At second, it would be less headache even for me.

cfangmeier commented 5 years ago

ok, I understand the issues. I have two thoughts:

  1. I don't see having the LastFM secret key in plain text in the config file as a big issue. As you said, when we had username/password combos, a leak of this could give an attacker access to a gmail/hangouts/play store/etc which is obviously terrible. In this case, the attacker would have access to... Scrobbling? Maybe I'm being naive, but I'm not worried about this.

  2. I'm not too excited about the prospect of maintaining migrations between different versions of config files. TUIJam, by virtue of being a terminal app, will mostly be used by tech-literate folks, so as long as the README is accurate users should be able to figure out problems with the config. Also, the config is super simple, only ~5 or so fields for people to worry about.