PowerShell / SecretManagement

PowerShell module to consistent usage of secrets through different extension vaults
MIT License
317 stars 46 forks source link

Error text and README are not that clear on which functions are optional and which are required #137

Open fsackur opened 3 years ago

fsackur commented 3 years ago

EDIT: I forked in order to PR a README change, and spotted what I'd missed - d'oh! The correct commands are called out in README.md#L157 FunctionsToExport = @('Set-Secret','Get-Secret','Remove-Secret','Get-SecretInfo','Test-SecretVault') and with #Optional comments above the optional functions. I missed that :-( However, I still think this issue points to a possible improvement.

Following the help, I implemented an Extension script module with the following functions:

However, vault registration failed:

Register-SecretVault -Name MyVault -ModuleName PasswordSafe -VaultParameters @{Id = 32048} -Verbose
VERBOSE: Performing the operation "Register module as a SecretManagement extension vault for current user" on target "MyVault".
Register-SecretVault: Could not find a SecretManagement extension implementing script module.

I inspected the source and found that Set-SecretInfo is not a required command, but Test-SecretVault is: https://github.com/PowerShell/SecretManagement/blob/b1fbbd0/src/code/SecretManagement.cs#L495-L511

The UX would be better if this were made clearer in the README, and/or the exact error message were propagated upwards. With NormalView, the error seen is:

Register-SecretVault : Could not find a SecretManagement extension implementing script module.
At line:1 char:1
+ Register-SecretVault -Name wham5-dev -ModuleName PasswordSafe -VaultP …
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo          : ObjectNotFound: (Microsoft.PowerShel…rSecretVaultCommand:RegisterSecretVaultCommand) [Register-SecretVault], PSInvalidOperationException
+ FullyQualifiedErrorId : RegisterSecretVaultCantFindImplementingScriptModule,Microsoft.PowerShell.SecretManagement.RegisterSecretVaultCommand

That could potentially tell us exactly what command or parameter is missing...?

fsackur commented 3 years ago

Possibly something like this for the error at https://github.com/PowerShell/SecretManagement/blob/b1fbbd0/src/code/SecretManagement.cs#L288 ?

var invalidException = new PSInvalidOperationException(
    message: string.Format(CultureInfo.InvariantCulture,
    "{0} is not a valid SecretManagement extension: {1}", moduleInfo.Name, error.Message
)
innerException: error);

The user would see:

PasswordSafe.Extension is not a valid SecretManagement extension: Test-SecretVault function not found.
PaulHigin commented 3 years ago

From the SecretManagement README.md file:

This module script implements the five functions, as cmdlets, required by SecretManagement, plus one optional function.
It also implements an optional `Unregister-SecretVault` function that is called during vault extension un-registration.
It also implements an optional function `Set-SecretInfo` function that sets secret metadata to a specific secret in the vault.  

So this can be made more clear.

Will update the error message and bubble up inner exception message, but in the mean time you can look at the inner exception for more error details (Get-Error command will dump all exception information).

PS> Get-Error
...
    Message        : Could not find a SecretManagement extension implementing script module.
    InnerException :
        Type        : System.Management.Automation.ItemNotFoundException
        ErrorRecord :
            Exception             :
                Type    : System.Management.Automation.ParentContainsErrorRecordException
                Message : Test-SecretVault function not found.
                HResult : -2146233087
            CategoryInfo          : InvalidArgument: (:String) [], ParentContainsErrorRecordException
            FullyQualifiedErrorId : SessionStateException
        **Message     : Test-SecretVault function not found.**
...
fsackur commented 3 years ago

Yes, and I do see that the info was there if I'd looked harder!