Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AADUser: Add support for MemberOf #3825

Merged
merged 35 commits into from
Nov 30, 2023
Merged

Conversation

salbeck-sit
Copy link
Contributor

Pull Request (PR) description

AADUser: Adds support for property MemberOf

This Pull Request (PR) fixes the following issues

Fixes #3820

@NikCharlebois
Copy link
Collaborator

Thanks for the PR. I am trying to understand the scenario here and why this wouldn't already be covered by the Members property of the AADGroup resource. I am concerned that we could run into cases where membership is defined both at the user and group levels and if these differ, we will be running into endless configuration loops.

e.g.,

AADUser John
{
UserPrincipalName = "john.smith'
MemberOf('GroupA', 'GroupC')
}

AADGroup GroupA
{
DisplayName = 'GroupA'
Members = @('notJohn')
}

AADGroup GroupB
{
DisplayName = 'GroupB'
Members = @('john.smith')
}

@salbeck-sit
Copy link
Contributor Author

Basically it's a matter of choice. We're mostly using group-based licensing and recently I've begun preparing for 'pre-staging' resource-accounts for Teams Rooms and here I want to ensure that the account is made a member of the security-group that provides the correct license without at the same time maintaining a list of which accounts are members of the specific group.

Perhaps the examples should be clear in that regard, for instance by saying that use of AADUSer.MemberOf may collide with use of AADGroup.Member - something that the examples for AADGroup should also mention.
I've updated the readme for AADUser and AAGroup to explain the potential conflict

@salbeck-sit
Copy link
Contributor Author

I've updated the changelog but now there's a conflict that I just can't see.

@salbeck-sit
Copy link
Contributor Author

Phew. Some ugly trial-error rebasing seems to have solved the issue

@salbeck-sit
Copy link
Contributor Author

PS You have the same 'conflict' within AADGroup where it's possible to specify Members AND MemberOf. When a group that is included elsewhere in another Group's MemberOf is configured with Members and the list doesn't contain that group - what then?
The same issue has existed for ages in dear old Group Policy Restricted Groups where you also find both Member and MemberOf, they also happily trip each other up.

My take is that if you want to control total membership of a group you use AADGroup -> Members.
If, on the other hand, you want to ensure (sic) membership of some group without wanting to control that group's membership, then use AADGroup -> MemberOf or AADUser -> MemberOf

@salbeck-sit
Copy link
Contributor Author

PS I haven't updated the changelog - yet. When you're ready to approve, let me know and I'll include it in the PR

@salbeck-sit
Copy link
Contributor Author

Ah. I accidentally closed the PR. That wasn't my intention

@salbeck-sit salbeck-sit reopened this Nov 14, 2023
@NikCharlebois NikCharlebois merged commit 8abd8c9 into microsoft:Dev Nov 30, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AADUser: Add support for MemberOf
2 participants