add ability to remove users from groups

[rplevy]

Presently you can add a user to a group, but there is no way to delete them.

[technomancy]

This opens a pretty big can of worms since pretty clearly we don't want to have a case where you add someone and they turn around and remove you.

There are two ways we could go here.

0. Introduce "levels" of membership. If you add someone as an "admin", watch out; they can do anything. If you add someone as a "member" then all they can do is push; they can't even add new members.

1. Only allow you to remove people you've added (either directly or transitively). We already track who's added whom, so this wouldn't be terribly tricky to implement. But if we do this deletion must be implemented behind the scenes as database insertion, otherwise you could remove someone and then be unable to remove people who they added.

[dexterous]

👍

[mynomoto]

I started working on this using the approach 1) on https://github.com/mynomoto/clojars-web/tree/delete-users-from-groups

[danielcompton]

I think I would prefer to go with technomancy's admin/member option, as this is typically how most web apps manage these kinds of permissions, and matches to my mental model of how maintainers would want to manage permissions.

[mynomoto]

Ok, I will change it. I have a couple of questions:

• How current members should be migrated?
• Should delete still be additive, or can it simply delete the row (breaking the added_by chain)?

[danielcompton]

How current members should be migrated?

Probably safest to make all current members admins, as if we only chose the first pusher, they may be people who are no longer involved in the Clojure community and it may be hard to reach them if maintainers want to add new people.

Should delete still be additive, or can it simply delete the row (breaking the added_by chain)?

Not too sure. Probably better to keep it additive, by making their admin privileges inactive from a flag. If they are re-added (reactivated) a second time, then the activating user would be the new added_by user.

[mynomoto]

Probably safest to make all current members admins, as if we only chose the first pusher, they may be people who are no longer involved in the Clojure community and it may be hard to reach them if maintainers want to add new people.

I agree but then we will need to communicate that clearly when this goes live. It's a big change and someone could remove other members from groups.

Not too sure. Probably better to keep it additive, by making their admin privileges inactive from a flag. If they are re-added (reactivated) a second time, then the activating user would be the new added_by user.

I'm not sure if I follow. Would delete remove the user from the group or only remove admin privileges? Also changing the added_by_user may cause discontinuous graphs of who added who, not sure if continuous graphs are important though.

[danielcompton]

I agree but then we will need to communicate that clearly when this goes live. It's a big change and someone could remove other members from groups.

Yeah, perhaps we start off with only the first pusher as admin, give them a month to audit/set permissions, then set everyone else as admins? Not too sure really...

I'm not sure if I follow. Would delete remove the user from the group or only remove admin privileges?

It would remove any privileges, but keep the deleted record for auditing ability.

Also changing the added_by_user may cause discontinuous graphs of who added who, not sure if continuous graphs are important though.

Don't think this would be a problem?

[mynomoto]

Don't think this would be a problem?

So there are scenarios where that could present a problem considering rogue users. If someone remove everyone else from a group, re-adds them and remove them again you would need some place other than the current db to check who had permissions at the beginning, assuming that you would consider a request of the former owner in this situation. As this code is in the open, malicious users could use this info.

That's part of the reason why I had picked the option 1) first. Less worms on that can 😉

Let me know if I should move forward with 0) and what to do when someone is re-added.

[tobias]

I prefer option 0 as well, and think you should move forward with it if you are still willing.

With regards to the issues above:

1. Who is an admin initially? - I'd like to make just the primary the user the admin, then let a user that wants to petition for admin rights to file an issue, hopefully including the current admin in the discussion, asking for a transfer of rights.

2. How to handle removal/added-by history in the db - I think it would work to add admin, inactive, and inactivated_by columns to the groups table. When removing a user, we set the inactive column to true and store the current user in inactivated_by. If that user is re-added, we create a new record, with a new added_by, and we ensure that only one active record exists for a user/group pair. If a user's role changes (member -> admin or admin -> member), we inactivate the current record and add a new one. This would make a role change look like a deletion and re-add, but would make the group history immutable, and conceptually we don't care if a user was actually removed and re-added with a different role, or the role was just changed.

How do those solutions sound? Any issues you can see?

[mynomoto]

Those sound fine, thanks. I hope to find time to do this soon.

[mynomoto]

Started a new branch with this approach: https://github.com/mynomoto/clojars-web/tree/remove-users-from-groups

[tobias]

Great! Feel free to PR when you are ready for feedback.