[Request] user_role module should take a list of roles, and (optionally) remove users from the roles not in the list

[harCamConsulting]

Is your feature request related to a problem? Please describe.
Using user_role module could lead to permissions build up over time if users not removed from roles more easily.
Currently when I need to change the roles for a user, I would add another whole play and edit it. If I need to remove a user from having access to a database, I need to know the roles they were added to (which isn't necessarily always known)
This enhancement would make it easier to remove users from roles they don't need while leaving in ones that they do. It would also make the playbooks read more idempotently

Describe the solution you'd like
The parameter "role" should take a list of strings. A string could be converted into a list with one item in it if needed. The plugin would then cycle over each of the roles the user already has. If the users current roles has extra than the list desired, they would be removed. If the user is missing roles that are desired, they would be added.

The powershell command something like this could be used to get the list of current roles the user has into an array:
$currentroles = get-dbadbrolemember -database $databasename -sqlinstance $sqlinstance -sqlcredential $cred | where-object {$_.username -eq $username} | select -expandproperty role

The list from that could then be used in a compare-object command against the list passed from ansible to find the changes needed (if any), e.g.:

$diffs = Compare-Object $currentroles $desired
$to_add = $diffs | Where-Object {$_.sideindicator -eq '=>'} | select -ExpandProperty inputobject
$to_del = $diffs | Where-Object {$_.sideindicator -eq '<='} | select -ExpandProperty inputobject

Describe alternatives you've considered
Considered doing in the playbook using either custom powershell scripts or SQL scripts, but this looks messier and is harder to maintain

Additional context

[briantist]

Consider the way that the microsoft.ad collection handles multi-value settings. All such options allow setting one (or more) of three subkeys: add, remove, and set so that you may choose whether to add or remove a given value, or to idempotently control the entire list (set).

• https://docs.ansible.com/ansible/latest/collections/microsoft/ad/docsite/guide_list_values.html

This may be a good pattern to adopt for values like this, and there's plenty of sample code on how to handle that in the AD collection's modules.

That being said, this would be a breaking change for this module/collection.

[harCamConsulting]

That being said, this would be a breaking change for this module/collection.

It could be made a non-breaking change by making a variable that sets the module to optionally do it the current way or the proposed way and make the default to do it the current way. Either:
a) make another variable, something like "remove_unlisted_roles", with a default of false
b) add another option to state. "absent" removes the role(s), "present" adds the listed role(s), and a new option "set" which would both add the listed roles, and remove any not listed.

The next major version would then change the default to the proposed way.

I think option a) reads easier, but either way works I guess

[harCamConsulting]

I also had a much longer read through the code at microsoft.ad - that seems to rely on get- and set- commands with a substitutable noun for it's actual work on ad objects - I don't think much of it is re-usable here, where the dbatools module uses get-, add-, and remove- verbs for updating role memberships.

[briantist]

It is not necessary to do implementation the same way as microsoft.ad, I'm suggesting that the pattern (from the user-facing side) is what's valuable. If someone wanted to (partially?) re-use the implementation though and the issue were as small as command verbs, it is quite easy enough to create aliases.

But I will leave such details to a PR author or the collection maintainer :)

[lowlydba]

@harCamConsulting thanks for opening the issue!

This has indeed been a weak spot of this collection for a long time now, and I agree that it would be a great quality of life enhancement for far more succinct use.

I likely won't be able to dedicate time on it myself, but am happy to accept a pull request for it based on the patterns discussed above - it should be relatively straight forward it seems.

If the breaking change is avoidable, that's great too, but not a hard requirement.

[harCamConsulting]

OK I have made a PR with some powershell updates that seem to work on my systems - link - but this is kinda my first PR to a public project ever. I'm not sure how to set up the full testing environment, and while I've documented what I can find, I'm sure I've probably missed some things . I've tried my best though.

[harCamConsulting]

oh - for the moment I have not used the "add/remove/set" dict pattern - I haven't been able to understand the code in the referenced modules that process those sort of dicts (I think they call them "list of dict"s?"). For now I've gone with what I've proposed originally, but hopefully I've written the loops in such a way that it should be relatively easy to move around to support using the add/remove/set mechanic by someone who knows better