Deleted user not removed from group

mikemets · November 23, 2022, 9:12am

Hi, I have a problem when I add a user to a group, then delete that user from the control panel and then recreate it, the user pops up in the group (see test below). I see in test_portalmembership that deleting a user is non trivial and although I remove the member from membership and I delete it, it still appears in the group. Is this a bug?

    def testDeleteMember(self):
        self.createMember(
            "barney", "Barney Rubble", "barney@bedrock.com", ["Member"], "2002-01-01"
        )
        self.portal.manage_role("Member", [Permissions.manage_users])
        g = self.groups.getGroupById("foo")
        g.addMember("barney")
        self.assertEqual(len(g.getGroupMembers()), 1)
        self.membership.deleteMembers(["barney"])
        barney = self.membership.getMemberById("barney")
        del barney
        self.assertEqual(len(g.getGroupMembers()), 0)
        self.createMember(
            "barney", "Barney Rubble", "barney@bedrock.com", ["Member"], "2002-01-01"
        )
        self.assertEqual(len(g.getGroupMembers()), 0)

dieter · November 23, 2022, 10:03am

Mike Metcalfe via Plone Community wrote at 2022-11-23 09:22 +0000:

... delete user does not remove the user from groups ...
Is this a bug?

Not necessarily:
Products.PluggableAuthService (underlying Products.PlonePAS) has
been designed to be modular. Its plugins try by purpose to be independent
of one another and each focus on very few methods.
Unfortunately, this brings the risk of less integration
and therefore more work for the administrator.

In your case: deleting a user is indeed complex.
Not only, you must delete the user but also:

remove him from any group he belongs to
remove all "local role"s associated with this user
handle creator references to this user in all content objects
potentially (if the user had sufficient priviledges)
fix "executable ownership" for all executable objects
(scripts, templates) created by that user.

The easiest thing is to avoid deleting a user
but instead disable him (such that he can no longer log in).
If you must delete data associated with the user for legal reasons,
delete the data but keep the user object (appropriately disabled).

jensens · November 23, 2022, 10:10am

This is known and historically somehow by intend. Also ownership and local roles on items will be "transferred" to the new user. To avoid this behavior it is recommended to use UUIDs as userids internally:

yurj · November 23, 2022, 10:28am

github.com

zopefoundation/Products.PluggableAuthService/blob/82038544ee31975a648a4f6e468fb2c280faca56/src/Products/PluggableAuthService/plugins/ZODBGroupManager.py#L272


      
          
          
    if not already:
                  new = current + (group_id,)
                  self._principal_groups[principal_id] = new
                  self._invalidatePrincipalCache(principal_id)
                  notify(PrincipalAddedToGroup(principal_id, group_id))
          
          
    return not already
          
          
@security.private
          def removePrincipalFromGroup(self, principal_id, group_id):
              """ Remove a prinicpal from from a group.
          
          
    o Return a boolean indicating whether the principal was already
                a member of the group.
          
          
    o Raise KeyError if 'group_id' is unknown.
          
          
    o Ignore requests to remove a principal if not already a member
                of the group.
              """

github.com

zopefoundation/Products.PluggableAuthService/blob/82038544ee31975a648a4f6e468fb2c280faca56/src/Products/PluggableAuthService/plugins/ZODBGroupManager.py#L249


      
                      info = parent.searchPrincipals(id=k, exact_match=True)
                      if len(info) == 0:
                          title = '<%s: not found>' % k
                      else:
                          # always use the title of the first principal found
                          title = info[0].get('title', k)
                      result.append((k, title))
          
          
    return result
          
          
@security.private
          def addPrincipalToGroup(self, principal_id, group_id):
              """ Add a principal to a group.
          
          
    o Return a boolean indicating whether a new assignment was created.
          
          
    o Raise KeyError if 'group_id' is unknown.
              """
              # raise KeyError if unknown!
              group_info = self._groups[group_id]  # noqa

github.com

zopefoundation/Products.CMFCore/blob/361a30e0c72a15a21f88433b8d5fc49331f36728/src/Products/CMFCore/MembershipTool.py#L490


      
              already been performed.  Called by portal_registration.
              """
              self.acl_users._doAddUser(id, password, roles, domains)
          
          
    if properties is not None:
                  member = self.getMemberById(id)
                  member.setMemberProperties(properties)
          
          
@security.protected(ManageUsers)
          @postonly
          def deleteMembers(self, member_ids, delete_memberareas=1,
                            delete_localroles=1, REQUEST=None):
              """ Delete members specified by member_ids.
              """
              # Delete members in acl_users.
              acl_users = self.acl_users
              if _checkPermission(ManageUsers, acl_users):
                  if isinstance(member_ids, six.string_types):
                      member_ids = (member_ids,)
                  member_ids = list(member_ids)
                  for member_id in member_ids[:]:

seems all ok, it is a btree and things are removed. deleteMembers just remove the user but not the user from the group.

You've to explicitly remove it form the group before deleting the user.

mikemets · November 23, 2022, 10:55am

Thanks guys, I think I'll go with Dieter and instruct the administrators to disable rather than delete users.

mauritsvanrees · November 24, 2022, 11:47pm

How would you disable a user?

If users are content (dexterity.membrane) you could do this with workflow. But how do you do this for normal users?

One sneaky way I can think of:

Set their email to nobody@example.org.
Reset their password.

mikemets · November 25, 2022, 5:19am

I was thinking of just removing the Member role.

mauritsvanrees · November 25, 2022, 11:53am

As non-Member they might still have access to some areas with local roles. And they might have access simply because they are authenticated. But it helps.

It could be a nice addition for Plone to have an extra user property active. Site Administrators could uncheck this and the user would no longer be able to login: the authentication plugins would check for this property and deny authentication. Is there no add-on that does this?

dieter · November 25, 2022, 12:33pm

Maurits van Rees via Plone Community wrote at 2022-11-25 12:03 +0000:

As non-Member they might still have access to some areas with local roles. And they might have access simply because they are authenticated. But it helps.

It could be a nice addition for Plone to have an extra user property active. Site Administrators could uncheck this and the user would no longer be able to login: the authentication plugins would check for this property and deny authentication. Is there no add-on that does this?

Products.PlonePAS notifies the UserLoggedInEvent event;
one could implement denying logic in a corresponding subscriber.

jensens · November 25, 2022, 1:08pm

For pas.plugins.ldap, the underlying node.ext.ldap UGM implementation supports expiring accounts and so pas.plugins.ldap does. Expired accounts are prevented to authenticate.

This can be enabled in the settings by a checkbox. Once enabled two values are expected: The unit, to check if its days or seconds since epoch and the attribute to check for the timestamp in the given unit. On each authenticate the values are checked and if the current timestamp is larger than the stored authentication is prevented.

As it is LDAP there are different attributes used. AD uses AccountExpires while edirectory uses LoginExpirationTime and there is also PwdEndTime somewhere used.

Overall I like the idea to add such a date based feature to Plone internal users itself. This is more flexible than a boolean and by setting the date to now an immediate lock would work too.

Esoth · January 25, 2024, 7:15pm

I found this while looking into whether or not users not removed from groups was a "bug", but there is some interesting discussion on deactivating users here. My team is actually planning on this for unrelated reasons. My plan was to add a member field 'active' and then use our existing SSO plugin's authenticateCredentials method to check that property's value. If they don't have it checked, they aren't authenticated.

What looks more complicated is adapting the Users Overview interface UX. I can add another column for active status, no problem. But I can see an admin wanting to search for just active (or inactive) users and I don't see how you add more options to that search form without creating a mess.