User group lists still include deleted users (and they can't be removed)

[bmillwood]

Once a user is marked as deleted, their username becomes free for reuse. However, they still show up in user group listings, and their name links to the user page currently with that username, which could by now be someone else.

Moreover, the group membership editing functions are keyed by username, and that username no longer resolves to their user ID, so we can't remove the group membership via the API anymore. (And user undeletion is not implemented yet, so I can't fix this!)

I think likely deleting a user should remove all group memberships. If we have some way to reliably ensure groups never contain deleted members, so much the better, but if not we may also want to modify the groups logic to not show deleted users, or to flag that the user is deleted.

[bmillwood]

I came up with this test:

diff --git a/tests/HighLevelTest.hs b/tests/HighLevelTest.hs
index e5e1a5ab..0461ed9a 100644
--- a/tests/HighLevelTest.hs
+++ b/tests/HighLevelTest.hs
@@ -126,6 +126,13 @@ runUserTests = do
     do info "Checking password has changed"
        void $ validate (Auth "HackageTestUser2" "newtestpass2") "/user/HackageTestUser2/password"
        checkIsUnauthorized (Auth "HackageTestUser2" "testpass2") "/user/HackageTestUser2/password"
+    do info "Adding HackageTestUser2 to uploaders"
+       post (Auth "admin" "admin") "/packages/uploaders/" [
+           ("user", "HackageTestUser2")
+         ]
+       xs <- getGroup "/packages/uploaders/.json"
+       unless ("HackageTestUser2" `elem` map userName (groupMembers xs)) $
+           die ("Expected HackageTestUser2 to be an uploader: " ++ show xs)
     do info "Trying to delete HackageTestUser2 as HackageTestUser2"
        delete isForbidden (Auth "HackageTestUser2" "newtestpass2") "/user/HackageTestUser2"
        xs <- fmap (map userName) getUsers
@@ -136,6 +143,10 @@ runUserTests = do
        xs <- fmap (map userName) getUsers
        unless (xs == ["admin","HackageTestUser1"]) $
            die ("Bad user list: " ++ show xs)
+    do info "Checking HackageTestUser2 is no longer in uploaders listing"
+       xs <- getGroup "/packages/uploaders/.json"
+       unless ("HackageTestUser2" `notElem` map userName (groupMembers xs)) $
+           die ("Expected HackageTestUser2 to no longer be an uploader: " ++ show xs)
     do info "Getting user info for HackageTestUser1"
        xs <- validate NoAuth "/user/HackageTestUser1"
        --TODO: set the user's real name, and then look for that here

I could get the test to pass by just dropping deleted people from group listings:

diff --git a/src/Distribution/Server/Features/Users.hs b/src/Distribution/Server/Features/Users.hs
index 69b215b3..b621cef3 100644
--- a/src/Distribution/Server/Features/Users.hs
+++ b/src/Distribution/Server/Features/Users.hs
@@ -896,7 +896,11 @@ userFeature templates usersState adminsState
                                  ui_username = Users.userIdToName userDb uid,
                                  ui_userid   = uid
                                }
-                             | uid <- Group.toList userlist ]
+                             | uid <- Group.toList userlist
+                             , Just UserInfo{ userStatus = status }
+                                 <- pure $ Users.lookupUserId uid userDb
+                             , status /= AccountDeleted
+                             ]
           }
 
     --TODO: add serveUserGroupUserPost for the sake of the html frontend

but I think really we should be removing deleted users from groups, hiding them seems like a pain, so I haven't submitted this as a PR yet.