How to condense this PowerShell script?

Question

I am creating a code to open a website if a user is in a certain AD group and when connected to a certain network. This is what I have got so far:

$user = $env:username
$group1 = "examplegroup1"
$group2 = "examplegroup2"

if (Test-Connection "examplenetwork" -Quiet)
{      
$members1 = Get-ADGroupMember -Identity $group1 | Select -ExpandProperty 
SamAccountName

$members2 = Get-ADGroupMember -Identity $group2 | Select -ExpandProperty 
SamAccountName

If ($members1 -contains $user -or $members2 -contains $user) {Start-Process 
"examplewebsite"}
}

It works as it should, opening the website if the user is in the correct group and on the network, however I was just wondering if there was a way to condense the code?

It seems a waste to have to create 2 '$groups' and then repeat the Get-ADGroupMember aswell. I have played around with 'ForEach' but haven't managed to get it to work.

Any ideas on how to condense this? Preferably using the ForEach cmdlet.

Answer 1

Although my other answer is 'don't do that', if you do want your code, condensed:

$groups = 'group1', 'group2'

if ((Test-Connection -ComputerName "examplenetwork" -Quiet) -and 
    ($env:USERNAME -in ($groups | Get-ADGroupMember -Recursive).SamAccountName))
{
    Start-Process "www.example.com"
}

You really don't need either foreach.

Answer 2

If (("examplegroup1", "examplegroup2" | % {Get-ADGroupMember -Identity $_} | Select -ExpandProperty SamAccountName) -Contains $env:username) {Start-Process "examplewebsite"} 

Answer 3

I would probably do it backward:

$user = $env:username
$groups = "examplegroup1", "examplegroup1"

$CheckMembership = Get-ADUser -Identity $user -Property MemberOf | Select-Object -ExpandProperty MemberOf | Where-Object { $_ -in $groups }

if ($CheckMembership) {
   Start-Process "http://www.example.com"
}

You'd want to make sure your list of groups was a list of distinguished names, but other than that this reduces the number of AD queries to 1.

Answer 4

Anything which relies on Get-AD___ needs the RSAT tools to get the ActiveDirectory module, that's an unlikely assumption for end user workstations as @Rohin Sidharth comments.

@James C.'s currently accepted answer will not handle recursive group membership (would need the -Recursive parameter), but also involves listing all the members of both groups - imagine if that was a billion members for each group - and it has the poor habit of array addition.

@Bacon Bits answer gets the user's group membership which is better for 'getting less data' but still won't handle recursive group membership and still relies on the ActiveDirectory module.

To avoid RSAT, something like ADSI could be used - which is wrapped by System.DirectoryServices.AccountManagement. Discussed here by Richard Siddaway.

That has a nice method to list the group members for a user, which appears to be broken - pinching from Terry Tsay's C# answer on a similar question here, I port his code to this but I've focused on the current user and included distribution groups by default:

Add-Type -AssemblyName System.DirectoryServices.AccountManagement

Function IsUserInGroup([string] $groupName)
{
    # Remove DOMAIN\ from the start of the groupName.
    $groupName = $groupName -replace '^.*\\'


    # Get an AD context for the current user's domain
    $context = New-Object -TypeName System.DirectoryServices.AccountManagement.PrincipalContext -ArgumentList 'Domain', $ENV:USERDOMAIN


    # Find the current user account in AD, and refresh the security and distribution groups 
    $user = [System.DirectoryServices.AccountManagement.UserPrincipal]::FindByIdentity($context, 'SAMAccountName', $env:USERNAME)
    $userEntry = [System.DirectoryServices.DirectoryEntry] $user.GetUnderlyingObject()
    $userEntry.RefreshCache(@('tokenGroupsGlobalAndUniversal'))


    # Get all the security and distribution groups the user belongs to, including nested memberships
    $usersGroupSIDs = foreach ($sid in $userEntry.Properties.tokenGroupsGlobalAndUniversal.Value)
    {
        New-Object System.Security.Principal.SecurityIdentifier -ArgumentList $sid, 0   
    }


    # Get the AD details for the group to test, and test membership
    $group = [System.DirectoryServices.AccountManagement.GroupPrincipal]::FindByIdentity($context, 'SamAccountName', $groupName)

    $usersGroupSIDs.Contains($group.Sid)
}

e.g.

PS C:\> IsUserInGroup 'parent-nested-group-here'
True

Which isn't condensed or simpler, but it should handle more conditions with less AD connecting overhead especially as member count of groups increases, and less need for extra modules, just using the .Net framework.

Then you could modify that to do

$group2 = [System.DirectoryServices.AccountManagement.GroupPrincipal]::FindByIdentity($context, 'SamAccountName', $group2Name)

$usersGroupSIDs.Contains($group.Sid) -or $usersGroupSIDs.Contains($group2.Sid)

Answer 5

You could add a foreach loop, but in would complicate what is already a very small and simple script.

The most I'd do is add the memberships from both groups together...

$user = $env:username
$group1 = "examplegroup1"
$group2 = "examplegroup2"

if (Test-Connection "examplenetwork" -Quiet)
{      
    $members = Get-ADGroupMember -Identity $group1 | Select -ExpandProperty SamAccountName

    $members += Get-ADGroupMember -Identity $group2 | Select -ExpandPropertySamAccountName

    If ($members -contains $user) {Start-Process "http://www.example.com"}
}