Skip to content

Implement RoleDefinitions RoleCapabilityFiles keyword#3067

Merged
daxian-dbw merged 5 commits into
PowerShell:masterfrom
PaulHigin:RoleCapFile
Feb 14, 2017
Merged

Implement RoleDefinitions RoleCapabilityFiles keyword#3067
daxian-dbw merged 5 commits into
PowerShell:masterfrom
PaulHigin:RoleCapFile

Conversation

@PaulHigin
Copy link
Copy Markdown
Contributor

This change adds the ability to specify RoleDefinition RoleCapability with a file path name. Previously a RoleCapability would be defined by a name and then a RoleCapability file with that name and .psrc extension would have to be placed within a $PSModulePath module under a RoleCapabilities directory.

Now a role capability can also be specified by providing a single file path name using the new RoleCapabilityFiles keyword in a RoleDefinition. Both the previous RoleCapabilities keyword and the new RoleCapabilityFiles keyword are now supported for specifying role capabilities.

Example

New-PSSessionConfigurationFile -Path .\MyConfig.pssc -RoleDefinitions @{
    Administrators = @{ RoleCapabilityFiles = "C:\AdminCaps.psrc" }
}

$iss = [initialsessionstate]::CreateFromSessionConfigurationFile($PSSessionConfigFile, { $true })
throw 'Should have thrown CouldNotFindRoleCapabilityFile exception'
}
catch [System.Management.Automation.MethodInvocationException]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. We should follow our guidance
throw "No Exception!"
  1. We should not filter exceptions with catch [System.Management.Automation.MethodInvocationException]
    We should catch all exceptions and compare them with the expected one.

$iss = [initialsessionstate]::CreateFromSessionConfigurationFile($PSSessionConfigFile, { $true })
throw 'Should have thrown InvalidRoleCapabilityFileExtension exception'
}
catch [System.Management.Automation.MethodInvocationException]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same.

$result = $ps.AddCommand('Get-Process').AddParameter('Name', 'PowerShell*').Invoke()

$result.Count | Should Not Be 0
$ps.Dispose()
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fail test case the Invoke() will be raise CommandNotFoundException exception.
Also the test check nothing because Get-Process is always present by default.
With changing the limit to three cmdlets:

New-PSRoleCapabilityFile -Path $GoodRoleCapFile -VisibleCmdlets 'Get-Command','Get-Process','Clear-Host'

we could do something like:

    It "Verifies good role capability file" {

        try {
            New-PSSessionConfigurationFile -Path $PSSessionConfigFile -RoleDefinitions @{
                Administrators = @{ RoleCapabilityFiles = "$GoodRoleCapFile" }
            }

            $iss = [initialsessionstate]::CreateFromSessionConfigurationFile($PSSessionConfigFile, { $true })
            $ps = [powershell]::Create($iss)
            $result1 = $ps.AddCommand('Get-Command').Invoke()
            $result2 = $ps.AddCommand('Get-Command').AddParameter('Name', 'Clear-Host').Invoke()
            $result3 = $ps.AddCommand('Get-Command').AddParameter('Name', 'Get-Process').Invoke()
        } finally {
            if ($ps) { $ps.Dispose() }
        }
        $result1.Count | Should Be 3
        $result2.Name | Should Be "Clear-Host"
        $result3.Name | Should Be "Get-Process"
    }

@PowerShellTeam PowerShellTeam added the Review - Needed The PR is being reviewed label Jan 30, 2017
}
else
{
throw 'Unexpected Exception'
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not again raise an exception. It is enough to check the current exception. In other words the L53 is sufficient.

try  {  
           $iss = [initialsessionstate]::CreateFromSessionConfigurationFile($PSSessionConfigFile, { $true })  
           throw 'No Exception!'  
 }  
 catch  
 {  
     $_.Exception.InnerException.ErrorRecord.FullyQualifiedErrorId | Should Be 'CouldNotFindRoleCapabilityFile'  
 }  

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not sufficient because L53 is invoked only if the correct exception is found. If it is not the expected exception then the test must fail.

I have to say that not filtering on the desired exception feels like an arbitrary and unneeded change. It did exactly what I intended and with less code.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, maybe I don't see nuances in this test.
My understanding is:

  1. The tested code should call a particular exception CouldNotFindRoleCapabilityFile
  2. We need to catch all exceptions, because we do not know whether this code caused the correct or incorrect exception.
  3. We need to check that the exception is correct or incorrect - all by one Should Be:
$_.Exception.InnerException.ErrorRecord.FullyQualifiedErrorId | Should Be 'CouldNotFindRoleCapabilityFile'

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see. Yes this will work. But if an incorrect exception is thrown then we will likely get a "null reference" exception when the test fails instead of seeing the actual thrown exception. But I guess this is Ok.

}
catch
{
([System.Management.Automation.PSInvalidOperationException] ($_.Exception).InnerException).ErrorRecord.FullyQualifiedErrorId | Should Be 'CouldNotFindRoleCapabilityFile'
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cast may throw 😖 Maybe use the pattern as in the last test with Gettype()?

}
catch
{
([System.Management.Automation.PSInvalidOperationException] ($_.Exception).InnerException).ErrorRecord.FullyQualifiedErrorId | Should Be 'InvalidRoleCapabilityFileExtension'
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same.

}
catch
{
($_.Exception).InnerException.GetType().FullName | Should Be 'System.Management.Automation.CommandNotFoundException'
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pattern we can use for all test above too.
Only here GetType() call may raise a "null reference" exception.

        try
        {
            $ps.Invoke()
            throw 'No Exception!'
        }
        catch
        {
            if ($_.Exception.InnerException) {
                $exc_name = $_.Exception.InnerException.GetType().FullName 
            }
            $exc_name | Should Be 'System.Management.Automation.CommandNotFoundException'
        }

@iSazonov
Copy link
Copy Markdown
Collaborator

iSazonov commented Feb 3, 2017

:shipit: for tests.

{
result.Append(SessionConfigurationUtils.ConfigFragment(ConfigFileConstants.RoleDefinitions, RemotingErrorIdStrings.DISCRoleDefinitionsComment,
"@{ 'CONTOSO\\SqlAdmins' = @{ RoleCapabilities = 'SqlAdministration' }; 'CONTOSO\\ServerMonitors' = @{ VisibleCmdlets = 'Get-Process' } } ", streamWriter, true));
"@{ 'CONTOSO\\SqlAdmins' = @{ RoleCapabilities = 'SqlAdministration' }; 'CONTOSO\\SqlManaged' = @{ RoleCapabilityFiles = 'C:\\RoleCapability\\SqlManaged.psrc' }; 'CONTOSO\\ServerMonitors' = @{ VisibleCmdlets = 'Get-Process' } } ", streamWriter, true));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I notice windows specific paths (C:\\) are used in the code, and I assume the related functionalities are not supported on unix plats, but what would be the user experience? New-PSSessionConfigurationFile is exposed on unix plats, so will it throw a friendly error when running the following?

New-PSSessionConfigurationFile -Path $PSSessionConfigFile -RoleDefinitions @{
           Administrators = @{ RoleCapabilityFiles = "$RoleCapDirectory\NoFile.psrc" }
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remoting endpoint configuration is supported on Windows only, at least for now, hence the Windows only test restriction. This change follows the existing code as originally ported.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean this cmdlet shouldn't be exposed on Linux/OSX at all? I opened #3147 to track this usability issue.
The code change LGTM.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, none of our remoting endpoint configuration works on Linux since it is specific to Windows/WinRM, and is not functional with OMI/WinRM AFAIK. I hope to create an RFC in the not too distant future that proposes how this might work on Linux, including JEA and hosting model.

Can you please make this issue more general since it involves more than just one cmdlet and is really about endpoint configuration on Linux.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification. I updated the issue to make it more general. Feel free to update further to make it more clear.

Copy link
Copy Markdown
Member

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

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.

5 participants