Group add member methods should prevent last owner from receiving lower access level
Follow-up to !93310 (merged) and #367953 (closed).
Group#add_member
and related Group#add_<role>
methods use the Members::CreatorService
to add members. These methods will also update an existing user's access level. When passing a current_user
to the service (the person making the change to the member) we conduct a proper policy check to ensure the current_user
has permission to modify the user. This policy also includes a check to ensure the group's last owner can't be modified. If the user is the last owner the can_update_existing_member
method returns false
.
However, when there's no current_user
passed, such as when these methods are used in Sidekiq jobs or other processes where we don't need to check the current user's permissions, these methods bypass the last owner check.
To ensure groups are not left without an owner we should introduce this last owner check before the early return for skipped authorization. This will bring more integrity to our member management and prevents the need for numerous last owner checks sprinkled through the code base.
The problem is this will require many spec updates. The group_member factory defaults the access level to owner. Any use of this factory that doesn't otherwise specify an access level or role trait gets owner by default. It's then very common for specs to use methods like Group#add_maintainer
to change levels throughout the tests. All of these tests now fail if we add the last owner condition.
In any case we should:
- Implement the last owner check in the creator service.
- Modify the factory to not default to owner access level.
Potential solutions
- Update all of these specs and enforce that the last owner can't be changed.
- Pros: Arguably the most 'right' solution. This is probably the way things would be if we had this last owner behavior from the beginning.
- Cons: Lots of touch points. Anytime we're touching lots of specs the possibility of messing up the spec are high (will it properly fail in the future if something isn't modified just right?)
- Add some special method/argument that allows forcing the change?
- Pros: Still requires lots of touch points, but less chance of causing unexpected problems.
- Cons: Creating special methods/arguments just to satisfy specs seems like an anti-pattern.
- Add a superfluous group owner to the group factory so our groups always have at least one owner. Then specs don't really need to do anything special with the members they create - they can make the secondary user an owner and still change them to a lower level.
- Pros: Least touch points.
- Cons: We're creating otherwise unnecessary records in our test runs. It could also produce unexpected behavior if specs are specifically testing last owner, or setting expectations on the number of members in their group.
- Variation on the last option: Default the factory not to create this first owner but allow it as a trait/attribute. This is sort of in the same style as the second potential solution except instead of creating a method/arg within our regular code we're creating an option within our specs/factories instead.
- Pros: It's less of an anti-pattern.
- Cons: Still has lots of touch points.