Remove the ability to look up another user's last.fm

This is a privacy issue that should not have been implemented.

Author: linuxdaemon

CHANGELOG.md

@@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Changed
 - Refactor tests to remove dependency on mock library
 - Change link_announcer.py to only warn on connection errors
+- Change user lookup logic in last.fm plugin
 ### Fixed
 - Fix matching exception in horoscope test
 - Fix youtube.py ISO time parse

plugins/lastfm.py

@@ -22,6 +22,10 @@
 
 
 def format_user(user):
+    """
+    >>> format_user('someuser')
+    's\u200bomeuser'
+    """
     return '\u200B'.join((user[:1], user[1:]))
 
 
@@ -32,6 +36,10 @@ def filter_tags(tags, artist, limit=4):
      * All lowercase
      * Artist name removed
      * Blacklist of words removed
+
+    >>> filter_tags(['sometag', 'artist', 'seen live', 'Some RaNDoM tAG',
+    ... 'tag5', 'tag6', 'tag7'], 'artist')
+    ['sometag', 'some random tag', 'tag5', 'tag6']
     """
 
     # We never want to see these specific tags
@@ -49,15 +57,15 @@ def filter_tags(tags, artist, limit=4):
     artist = artist.translate(translator).lower()
 
     # Perform the actual filtering, stop when we reach the desired number of tags
-    count = 0
     filtered_tags = []
     for tag in tags:
-        if not tag == artist:
-            if tag not in blacklist:
-                filtered_tags.append(tag)
-        count += 1
-        if count == limit:
-            return filtered_tags
+        if not tag == artist and tag not in blacklist:
+            filtered_tags.append(tag)
+
+        if len(filtered_tags) >= limit:
+            break
+
+    return filtered_tags
 
 
 last_cache = {}
@@ -116,7 +124,7 @@ def get_tags(method, artist, **params):
         for item in tags['toptags']['tag']:
             tag_list.append(item['name'])
 
-    tag_list = filter_tags(tag_list, artist, limit=4)
+    tag_list = filter_tags(tag_list, artist)
 
     return ', '.join(tag_list) if tag_list else 'no tags'
 
@@ -165,27 +173,21 @@ def getartistinfo(artist, user=''):
     return artist
 
 
-def check_key_and_user(nick, text, lookup=False):
+def check_key_and_user(nick, text):
     """
     Verify an API key is set and perform basic user lookups
 
     Used as a prerequisite for multiple API commands
+
     :param nick: The nick of the calling user
     :param text: The text passed to the command, possibly a different username to use
-    :param lookup: Whether to look up `text` as another user's nick in the user table
     :return: The parsed username and any error message that occurred
     """
     api_key = bot.config.get_api_key("lastfm")
     if not api_key:
         return None, "Error: No API key set."
 
-    if text:
-        if lookup:
-            username = get_account(text, text)
-        else:
-            username = text
-    else:
-        username = get_account(nick)
+    username = text or get_account(nick)
 
     if not username:
         return None, "No last.fm username specified and no last.fm username is set in the database."
@@ -194,7 +196,7 @@ def check_key_and_user(nick, text, lookup=False):
 
 
 def _topartists(text, nick, period=None, limit=10):
-    username, err = check_key_and_user(nick, text, True)
+    username, err = check_key_and_user(nick, text)
     if err:
         return err
 
@@ -422,7 +424,7 @@ def lastfmcompare(text, nick):
 @hook.command("ltop", "ltt", autohelp=False)
 def toptrack(text, nick):
     """[username] - Grabs a list of the top tracks for a last.fm username"""
-    username, err = check_key_and_user(nick, text, True)
+    username, err = check_key_and_user(nick, text)
     if err:
         return err
 

tests/plugin_tests/test_lastfm.py

@@ -7,6 +7,7 @@
 
 from cloudbot.bot import bot
 from cloudbot.config import Config
+from plugins import lastfm
 from plugins.lastfm import api_request
 
 
@@ -20,37 +21,220 @@ def __init__(self, config):
         self.config = MockConfig(self, config)
 
 
+def test_get_account(mock_db):
+    lastfm.table.create(mock_db.engine)
+    mock_db.add_row(lastfm.table, nick="foo", acc="bar")
+    lastfm.load_cache(mock_db.session())
+
+    assert lastfm.get_account("foo", "baz") == "bar"
+    assert lastfm.get_account("FOO", "baz") == "bar"
+    assert lastfm.get_account("foo1", "baz") == "baz"
+    assert lastfm.get_account("foo1", "baa") == "baa"
+
+
 def test_api(unset_bot):
     bot.set(MockBot({"api_keys": {"lastfm": "hunter20"}}))
 
     with RequestsMock() as reqs:
         with pytest.raises(requests.ConnectionError):
-            api_request('track.getTopTags')
+            api_request("track.getTopTags")
 
-        reqs.add(
-            reqs.GET, "http://ws.audioscrobbler.com/2.0/",
-            json={"data": "thing"}
-        )
+        reqs.add(reqs.GET, "http://ws.audioscrobbler.com/2.0/", json={"data": "thing"})
 
-        res, _ = api_request('track.getTopTags')
+        res, _ = api_request("track.getTopTags")
 
         assert res["data"] == "thing"
 
     with RequestsMock() as reqs:
-        reqs.add(
-            reqs.GET, "http://ws.audioscrobbler.com/2.0/",
-            body="<html></html>"
-        )
+        reqs.add(reqs.GET, "http://ws.audioscrobbler.com/2.0/", body="<html></html>")
 
         with pytest.raises(JSONDecodeError):
             api_request("track.getTopTags")
 
     with RequestsMock() as reqs:
         reqs.add(
-            reqs.GET, "http://ws.audioscrobbler.com/2.0/",
+            reqs.GET,
+            "http://ws.audioscrobbler.com/2.0/",
             body="<html></html>",
-            status=403
+            status=403,
         )
 
         with pytest.raises(HTTPError):
             api_request("track.getTopTags")
+
+
+def test_api_error_message(mock_requests, mock_api_keys):
+    mock_requests.add(
+        "GET",
+        "http://ws.audioscrobbler.com/2.0/",
+        json={"error": 10, "message": "Invalid API Key",},
+    )
+
+    _, error = api_request("track.getTopTags")
+
+    assert error == "Error: Invalid API Key."
+
+
+def test_getartisttags(mock_requests, mock_api_keys):
+    url = "http://ws.audioscrobbler.com/2.0/?format=json&artist=foobar&autocorrect=1&method=artist.getTopTags&api_key=APIKEY"
+    mock_requests.add(
+        "GET", url, json={"toptags": {},}, match_querystring=True,
+    )
+    res = lastfm.getartisttags("foobar")
+    assert res == "no tags"
+
+
+class TestGetArtistTags:
+    url = "http://ws.audioscrobbler.com/2.0/?format=json&artist=foobar&autocorrect=1&method=artist.getTopTags&api_key=APIKEY"
+
+    def get_tags(self):
+        return lastfm.getartisttags("foobar")
+
+    def test_missing_tags(self, mock_requests, mock_api_keys):
+        mock_requests.add(
+            "GET", self.url, json={"toptags": {},}, match_querystring=True,
+        )
+        res = self.get_tags()
+        assert res == "no tags"
+
+    def test_no_tags(self, mock_requests, mock_api_keys):
+        mock_requests.add(
+            "GET", self.url, json={"toptags": {"tags": []},}, match_querystring=True,
+        )
+        res = self.get_tags()
+        assert res == "no tags"
+
+    def test_non_existent_artist(self, mock_requests, mock_api_keys):
+        mock_requests.add(
+            "GET",
+            self.url,
+            json={"error": 6, "message": "Missing artist."},
+            match_querystring=True,
+        )
+        res = self.get_tags()
+        assert res == "no tags"
+
+    def test_tags(self, mock_requests, mock_api_keys):
+        mock_requests.add(
+            "GET",
+            self.url,
+            json={
+                "toptags": {
+                    "tag": [
+                        {"name": name}
+                        for name in [
+                            "foobar",
+                            "tag2",
+                            "seen live",
+                            "tag4",
+                            "tag5",
+                            "tag6",
+                            "tag7",
+                        ]
+                    ]
+                },
+            },
+            match_querystring=True,
+        )
+        res = self.get_tags()
+        assert res == "tag2, tag4, tag5, tag6"
+
+
+def test_gettracktags(mock_requests, mock_api_keys):
+    url = "http://ws.audioscrobbler.com/2.0/?format=json&artist=foobar&autocorrect=1&track=foobaz&method=track.getTopTags&api_key=APIKEY"
+    mock_requests.add(
+        "GET", url, json={"toptags": {}}, match_querystring=True,
+    )
+    res = lastfm.gettracktags("foobar", "foobaz")
+    assert res == "no tags"
+
+
+class TestCheckKeyAndUser:
+    def test_text(self, mock_api_keys, mock_requests, mock_db):
+        lastfm.table.create(mock_db.engine)
+        mock_db.add_row(lastfm.table, nick="foo", acc="bar")
+        lastfm.load_cache(mock_db.session())
+
+        res, err = lastfm.check_key_and_user("foo", "baz")
+        assert err is None
+        assert res == "baz"
+
+    def test_db_lookup(self, mock_api_keys, mock_requests, mock_db):
+        lastfm.table.create(mock_db.engine)
+        mock_db.add_row(lastfm.table, nick="foo", acc="bar")
+        lastfm.load_cache(mock_db.session())
+
+        res, err = lastfm.check_key_and_user("foo", "")
+        assert err is None
+        assert res == "bar"
+
+    def test_missing_user(self, mock_api_keys, mock_requests, mock_db):
+        lastfm.table.create(mock_db.engine)
+        mock_db.add_row(lastfm.table, nick="foo", acc="bar")
+        lastfm.load_cache(mock_db.session())
+
+        res, err = lastfm.check_key_and_user("foo1", "")
+        assert res is None
+        expected = "No last.fm username specified and no last.fm username is set in the database."
+        assert err == expected
+
+    def test_no_key(self, mock_api_keys, mock_requests, mock_db):
+        bot.config.get_api_key.return_value = None
+        res, err = lastfm.check_key_and_user("foo", "baz")
+        assert res is None
+        assert err == "Error: No API key set."
+
+
+class TestTopArtists:
+    def test_topweek_self(self, mock_api_keys, mock_requests, mock_db):
+        lastfm.table.create(mock_db.engine)
+        mock_db.add_row(lastfm.table, nick="foo", acc="bar")
+        lastfm.load_cache(mock_db.session())
+
+        mock_requests.add(
+            "GET",
+            "http://ws.audioscrobbler.com/2.0/?format=json&user=bar&limit=10&period=7day&method=user.gettopartists&api_key=APIKEY",
+            match_querystring=True,
+            json={
+                "topartists": {
+                    "artist": [
+                        {"name": "foo", "playcount": 5},
+                        {"name": "bar", "playcount": 2},
+                    ]
+                }
+            },
+        )
+
+        out = lastfm.topweek("", "foo")
+
+        assert out == "b\u200bar's favorite artists: foo [5] bar [2] "
+
+
+class TestTopTrack:
+    def test_toptrack_self(self, mock_api_keys, mock_requests, mock_db):
+        lastfm.table.create(mock_db.engine)
+        mock_db.add_row(lastfm.table, nick="foo", acc="bar")
+        lastfm.load_cache(mock_db.session())
+
+        mock_requests.add(
+            "GET",
+            "http://ws.audioscrobbler.com/2.0/?format=json&user=bar&limit=5&method=user.gettoptracks&api_key=APIKEY",
+            match_querystring=True,
+            json={
+                "toptracks": {
+                    "track": [
+                        {
+                            "name": "some song",
+                            "artist": {"name": "some artist"},
+                            "playcount": 10,
+                        }
+                    ]
+                }
+            },
+        )
+
+        out = lastfm.toptrack("", "foo")
+
+        expected = "b\u200bar's favorite songs: some song by some artist listened to 10 times. "
+
+        assert out == expected