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

Activity

1. bmillwood commented on Sep 28, 2025

   

   bmillwood

   on Sep 28, 2025

   Last edited by bmillwood

   ContributorAuthor

   More actions

   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.
2. isovector commented on Apr 22, 2026

   

   isovector

   on Apr 22, 2026

   Contributor

   More actions

   @bmillwood I'm currently working on swapping out acid-store for a real relational database, which in turn means I'm thinking about data models. In your opinion, what's the best path forward here? Key groups by uid instead of username? Actually /delete/ deleted users? Something else entirely?
3. bmillwood commented on Apr 26, 2026

   

   bmillwood

   on Apr 26, 2026

   Last edited by bmillwood

   ContributorAuthor

   More actions

   @bmillwood I'm currently working on swapping out acid-store for a real relational database, which in turn means I'm thinking about data models. In your opinion, what's the best path forward here? Key groups by uid instead of username? Actually /delete/ deleted users? Something else entirely?

   I have to say whatever I learned about the data model in the course of investigating this months ago has left my brain again 😅

   I think deleting users should actually delete them unless there's a strong reason not to, but I imagine there could well be a strong reason not to. In that case I'd say deleting a user should strip them of all their groups, but I'm not particularly opinionated about how that should happen.

   Keying groups by uid instead of username sounds like the natural way to do things, but again, I have forgotten whatever I once knew about which is the primary key of the users collection.

   Happy to see people working on this, btw :)