diff --git a/changelog/62502.fixed b/changelog/62502.fixed new file mode 100644 index 000000000000..5de34ad71be4 --- /dev/null +++ b/changelog/62502.fixed @@ -0,0 +1 @@ +Fix user.present to allow removing groups using optional_groups parameter and enforcing idempotent group membership. diff --git a/salt/states/user.py b/salt/states/user.py index 294145110632..8415c58e5128 100644 --- a/salt/states/user.py +++ b/salt/states/user.py @@ -98,7 +98,7 @@ def _changes( return False change = {} - if groups is None: + if not remove_groups and groups is None: groups = lusr["groups"] wanted_groups = sorted(set((groups or []) + (optional_groups or []))) if uid and lusr["uid"] != uid: diff --git a/tests/pytests/functional/states/test_user.py b/tests/pytests/functional/states/test_user.py index df7001221f0f..a360f9e40add 100644 --- a/tests/pytests/functional/states/test_user.py +++ b/tests/pytests/functional/states/test_user.py @@ -363,3 +363,51 @@ def test_user_present_existing(states, username): assert ret.changes["profile"] == win_profile assert "description" in ret.changes assert ret.changes["description"] == win_description + + +def test_user_present_change_groups(modules, states, username, group_1, group_2): + ret = states.user.present( + name=username, + groups=[group_1.name, group_2.name], + ) + assert ret.result is True + + user_info = modules.user.info(username) + assert user_info + assert user_info["groups"] == [group_1.name, group_2.name] + + # run again and remove group_2 + ret = states.user.present( + name=username, + groups=[group_1.name], + ) + assert ret.result is True + + user_info = modules.user.info(username) + assert user_info + assert user_info["groups"] == [group_1.name] + + +def test_user_present_change_optional_groups( + modules, states, username, group_1, group_2 +): + ret = states.user.present( + name=username, + optional_groups=[group_1.name, group_2.name], + ) + assert ret.result is True + + user_info = modules.user.info(username) + assert user_info + assert user_info["optional_groups"] == [group_1.name, group_2.name] + + # run again and remove group_2 + ret = states.user.present( + name=username, + optional_groups=[group_1.name], + ) + assert ret.result is True + + user_info = modules.user.info(username) + assert user_info + assert user_info["optional_groups"] == [group_1.name] diff --git a/tests/pytests/unit/states/test_user.py b/tests/pytests/unit/states/test_user.py index d17f36dd5017..50dee9d959e2 100644 --- a/tests/pytests/unit/states/test_user.py +++ b/tests/pytests/unit/states/test_user.py @@ -178,8 +178,10 @@ def test_present_uid_gid_change(): # get the before/after for the changes dict, and one last time to # confirm that no changes still need to be made. mock_info = MagicMock(side_effect=[before, before, after, after]) - mock_group_to_gid = MagicMock(side_effect=["foo", "othergroup"]) - mock_gid_to_group = MagicMock(side_effect=[5000, 5000, 5001, 5001]) + mock_group_to_gid = MagicMock(side_effect=[5000, 5001]) + mock_gid_to_group = MagicMock( + side_effect=["othergroup", "foo", "othergroup", "othergroup"] + ) dunder_salt = { "user.info": mock_info, "user.chuid": Mock(), @@ -197,7 +199,7 @@ def test_present_uid_gid_change(): ) assert ret == { "comment": "Updated user foo", - "changes": {"gid": 5001, "uid": 5001, "groups": ["othergroup"]}, + "changes": {"gid": 5001, "uid": 5001, "groups": []}, "name": "foo", "result": True, }