diff --git a/NTFSSecurity/ParameterConversion.cs b/NTFSSecurity/ParameterConversion.cs index 738bf21..11ee2fa 100644 --- a/NTFSSecurity/ParameterConversion.cs +++ b/NTFSSecurity/ParameterConversion.cs @@ -17,10 +17,10 @@ namespace NTFSSecurity /// /// Converts the raw value of a parameter to T, the way the PowerShell binder converts an argument for a /// parameter of that type: names in any case, a comma-separated list for a flags enumeration, a number, or - /// an enumeration instance. An enumeration that is not a flags enumeration takes one value: PowerShell would - /// add up the numbers of a list for it, so that one of the values wins silently or a value that the caller - /// did not name is chosen, so a list is rejected for it, in whatever kind of collection it comes. A - /// collection of one element converts as that element. + /// an enumeration instance. An enumeration that is not a flags enumeration takes one value: PowerShell + /// combines the numbers of a list for it bit by bit, so that one of the values wins silently or a value that + /// the caller did not name is chosen, so a list is rejected for it, in whatever kind of collection it comes. + /// A collection of one element converts as that element. /// /// /// The raw value of the parameter. Not null: the caller keeps the default of the parameter for null instead @@ -40,13 +40,13 @@ namespace NTFSSecurity if (!typeof(T).IsDefined(typeof(FlagsAttribute), false)) { - string list; - convertible = TakeOneValue(value, out list); - if (list != null) + bool isList; + convertible = TakeOneValue(value, out isList); + if (isList) { throw new ArgumentException(string.Format( "The value '{0}' for parameter -{1} is not valid: the parameter takes one value, not a list. {2}", - list, parameterName, DescribeValidValues(typeof(T)))); + Describe(value), parameterName, DescribeValidValues(typeof(T)))); } } @@ -64,7 +64,7 @@ namespace NTFSSecurity catch (Exception ex) { throw new ArgumentException( - string.Format("The value '{0}' for parameter -{1} is not valid. {2}", value, parameterName, DescribeValidValues(typeof(T))), + string.Format("The value '{0}' for parameter -{1} is not valid. {2}", Describe(value), parameterName, DescribeValidValues(typeof(T))), ex); } } @@ -82,30 +82,31 @@ namespace NTFSSecurity return string.Format("The value for parameter -{0} is null. {1}", parameterName, DescribeValidValues(typeof(T))); } + // How many collections that hold one collection each are opened before the argument counts as a list: enough + // for any real argument, and a stop for a collection that holds itself. + private const int MaxNesting = 32; + // The one value that the conversion takes from the argument of an enumeration that is not a flags enumeration: // the argument itself, or the only element of a collection of any kind (a wrapped PSObject counts as what it - // wraps, and a collection that holds a collection is opened too). When the argument holds more than one - // value, list is its text for the message: a string that holds a comma, a collection of several elements, or - // a collection of one element that holds such a list; the returned value is then not used. A dictionary, an - // empty collection, or a collection that cannot be enumerated is returned as it is, for the conversion to - // reject or convert with its own error. - private static object TakeOneValue(object value, out string list) + // wraps, and a collection that holds a collection is opened too). isList is true when the argument holds more + // than one value: a string that holds a comma, a collection of several elements (two are enough to know, so + // a long or endless sequence is not read to its end), a collection of one element that holds such a list, or + // collections nested deeper than MaxNesting; the returned value is then not used. A dictionary, an empty + // collection, or a collection that cannot be enumerated is returned as it is, for the conversion to reject or + // convert with its own error. + private static object TakeOneValue(object value, out bool isList) { - list = null; + isList = false; var current = value; - while (true) + for (var depth = 0; depth < MaxNesting; depth++) { var unwrapped = current is PSObject ? ((PSObject)current).BaseObject : current; var text = unwrapped as string; if (text != null) { - if (text.IndexOf(',') >= 0) - { - list = text; - } - + isList = text.IndexOf(',') >= 0; return unwrapped; } @@ -115,12 +116,19 @@ namespace NTFSSecurity return unwrapped; } - var items = new System.Collections.Generic.List(); + var count = 0; + object only = null; try { foreach (var item in collection) { - items.Add(item); + count++; + if (count > 1) + { + break; + } + + only = item; } } catch (Exception) @@ -128,19 +136,71 @@ namespace NTFSSecurity return value; } - if (items.Count == 1) + if (count == 1) { - current = items[0]; + current = only; continue; } - if (items.Count > 1) + isList = count > 1; + return value; + } + + isList = true; + return value; + } + + // The value as a message names it: a collection of one element by that element (also when it is a collection + // itself), a collection of several elements by its first ten, anything else by its text. + private static string Describe(object value) + { + var current = value; + + for (var depth = 0; depth < MaxNesting; depth++) + { + var unwrapped = current is PSObject ? ((PSObject)current).BaseObject : current; + var collection = unwrapped as IEnumerable; + + if (unwrapped is string || collection == null || unwrapped is IDictionary) { - list = string.Join(", ", items.ConvertAll(item => string.Format("{0}", item))); + return string.Format("{0}", current); } - return value; + var shown = new System.Collections.Generic.List(); + var more = false; + try + { + foreach (var item in collection) + { + if (shown.Count == 10) + { + more = true; + break; + } + + shown.Add(item); + } + + if (shown.Count == 1 && !more) + { + current = shown[0]; + continue; + } + + if (shown.Count == 0) + { + return string.Format("{0}", current); + } + + return string.Join(", ", shown.ConvertAll(item => string.Format("{0}", item))) + (more ? ", ..." : string.Empty); + } + catch (Exception) + { + return string.Format("{0}", value); + } } + + return string.Format("{0}", value); } // The valid names of the enumeration, in declaration order, with a note for a flags enumeration that a diff --git a/Tests/ParameterSets.Access.Tests.ps1 b/Tests/ParameterSets.Access.Tests.ps1 index 6bdbb64..4a0a2c6 100644 --- a/Tests/ParameterSets.Access.Tests.ps1 +++ b/Tests/ParameterSets.Access.Tests.ps1 @@ -1337,12 +1337,22 @@ Describe 'Get-NTFSEffectiveAccess' { # piped value the binder cannot convert to the enumeration is an error instead of a silently kept default. Describe 'Invalid and valid values of AccessType, InheritanceFlags, PropagationFlags, and AppliesTo' { BeforeDiscovery { + # A collection nested 40 levels deep is a list too: the conversion opens at most 32 levels, which also ends a + # collection that holds itself (a test with such a collection would not end if the limit were lost). + $accessDeepAccessType = 'Deny' + $accessDeepScope = 'FilesOnly' + foreach ($level in 1..40) { + $accessDeepAccessType = , $accessDeepAccessType + $accessDeepScope = , $accessDeepScope + } + $accessBadShapes = @( @{ Shape = 'a misspelled name'; Value = 'Typoo'; ValueText = 'Typoo' } @{ Shape = 'a blank string'; Value = ''; ValueText = $null } @{ Shape = 'a white-space string'; Value = ' '; ValueText = $null } @{ Shape = 'an undefined number'; Value = 99; ValueText = '99' } @{ Shape = 'a hashtable'; Value = @{ Unrelated = 1 }; ValueText = $null } + @{ Shape = 'an array of one misspelled name'; Value = @('Typoo'); ValueText = 'Typoo' } ) $accessTypeBadShapes = @( @{ Shape = 'a misspelled name'; Value = 'Denyy'; ValueText = 'Denyy' } @@ -1350,26 +1360,29 @@ Describe 'Invalid and valid values of AccessType, InheritanceFlags, PropagationF @{ Shape = 'a white-space string'; Value = ' '; ValueText = $null } @{ Shape = 'an undefined number'; Value = 99; ValueText = '99' } @{ Shape = 'a hashtable'; Value = @{ Unrelated = 1 }; ValueText = $null } + @{ Shape = 'an array of one misspelled name'; Value = @('Denyy'); ValueText = 'Denyy' } @{ Shape = 'a comma list of two names'; Value = 'Allow,Deny'; ValueText = 'Allow,Deny' } @{ Shape = 'an array of two names'; Value = @('Allow', 'Deny'); ValueText = 'Allow, Deny' } @{ Shape = 'an array of one comma list'; Value = @('Allow,Deny'); ValueText = 'Allow,Deny' } @{ Shape = 'an array that holds an array of two names'; Value = , @('Allow', 'Deny'); ValueText = 'Allow, Deny' } @{ Shape = 'a hash set of two names'; Value = [System.Collections.Generic.HashSet[string]]::new([string[]]@('Allow', 'Deny')); ValueText = $null } @{ Shape = 'the keys of a hashtable'; Value = (@{ Allow = 1; Deny = 2 }).Keys; ValueText = $null } + @{ Shape = 'an array nested 40 levels deep'; Value = $accessDeepAccessType; ValueText = $null } ) - # AccessType and AppliesTo name one value each, not a combination: PowerShell would add the numbers of a list, so that - # one of the values wins silently or, for AppliesTo, a scope that the caller did not name is chosen. Every kind of - # collection counts, also a collection of one element that holds a list. + # AccessType and AppliesTo name one value each, not a combination: PowerShell combines the numbers of a list bit by + # bit, so that one of the values wins silently or, for AppliesTo, a scope that the caller did not name is chosen. + # Every kind of collection counts, also a collection of one element that holds a list. $accessAppliesToListShapes = @( @{ Shape = 'a comma list of two names'; Value = 'ThisFolderAndSubfolders,ThisFolderAndFiles'; ValueText = 'ThisFolderAndSubfolders,ThisFolderAndFiles' } @{ Shape = 'an array of two names'; Value = @('ThisFolderAndSubfolders', 'ThisFolderAndFiles'); ValueText = 'ThisFolderAndSubfolders, ThisFolderAndFiles' } @{ Shape = 'an array of one comma list'; Value = @('ThisFolderAndSubfolders,ThisFolderAndFiles'); ValueText = 'ThisFolderAndSubfolders,ThisFolderAndFiles' } @{ Shape = 'a hash set of two names'; Value = [System.Collections.Generic.HashSet[string]]::new([string[]]@('ThisFolderAndSubfolders', 'ThisFolderAndFiles')); ValueText = $null } @{ Shape = 'the keys of a hashtable'; Value = (@{ ThisFolderAndSubfolders = 1; ThisFolderAndFiles = 2 }).Keys; ValueText = $null } + @{ Shape = 'an array nested 40 levels deep'; Value = $accessDeepScope; ValueText = $null } ) - # The cross product of the two cmdlets that share these parameters and every bad shape: 2 * (11 + 3 * 5 + 5) = 62 rows. + # The cross product of the two cmdlets that share these parameters and every bad shape: 2 * (13 + 3 * 6 + 6) = 74 rows. $accessInvalidParameterCases = foreach ($cmdlet in 'Add-NTFSAccess', 'Remove-NTFSAccess') { foreach ($shapeCase in $accessTypeBadShapes) { @{ Cmdlet = $cmdlet; Parameter = 'AccessType'; Shape = $shapeCase.Shape; Value = $shapeCase.Value; ValueText = $shapeCase.ValueText } @@ -1409,6 +1422,8 @@ Describe 'Invalid and valid values of AccessType, InheritanceFlags, PropagationF @{ Cmdlet = $cmdlet; Parameter = 'AppliesTo'; Value = @('ThisFolderAndSubfolders', 'ThisFolderAndFiles') } @{ Cmdlet = $cmdlet; Parameter = 'AppliesTo'; Value = @('ThisFolderAndSubfolders,ThisFolderAndFiles') } @{ Cmdlet = $cmdlet; Parameter = 'AppliesTo'; Value = [System.Collections.Generic.HashSet[string]]::new([string[]]@('ThisFolderAndSubfolders', 'ThisFolderAndFiles')) } + @{ Cmdlet = $cmdlet; Parameter = 'AccessType'; Value = $accessDeepAccessType } + @{ Cmdlet = $cmdlet; Parameter = 'AppliesTo'; Value = $accessDeepScope } } # A named $null (a literal $null, a variable, an alias, splatting, or $PSDefaultParameterValues) must @@ -1429,6 +1444,7 @@ Describe 'Invalid and valid values of AccessType, InheritanceFlags, PropagationF @{ Description = 'an array of one name'; Value = @('Deny') } @{ Description = 'a list of one name'; Value = [System.Collections.Generic.List[string]]::new([string[]]@('Deny')) } @{ Description = 'a hash set of one name'; Value = [System.Collections.Generic.HashSet[string]]::new([string[]]@('Deny')) } + @{ Description = 'an array that holds an array of one name'; Value = , @('Deny') } ) $accessValidFlagsShapeCases = @( @@ -1527,7 +1543,7 @@ Describe 'Invalid and valid values of AccessType, InheritanceFlags, PropagationF $pipelineErrors[0].TargetObject | Should -Be $path $pipelineErrors[0].Exception.Message | Should -Match ([regex]::Escape($Parameter)) if ($ValueText) { - $pipelineErrors[0].Exception.Message | Should -Match ([regex]::Escape($ValueText)) + $pipelineErrors[0].Exception.Message | Should -Match ("The value '" + [regex]::Escape($ValueText) + "'") } (Get-Acl -LiteralPath $path).Sddl | Should -BeExactly $before $result | Should -Not -BeNullOrEmpty diff --git a/Tests/ParameterSets.Audit.Tests.ps1 b/Tests/ParameterSets.Audit.Tests.ps1 index 6361a2a..5207d18 100644 --- a/Tests/ParameterSets.Audit.Tests.ps1 +++ b/Tests/ParameterSets.Audit.Tests.ps1 @@ -1004,12 +1004,20 @@ Describe 'Get-NTFSOrphanedAudit' { # privilege, unlike most of the tests above. Describe 'Invalid and valid values of AuditFlags, InheritanceFlags, PropagationFlags, and AppliesTo' { BeforeDiscovery { + # A collection nested 40 levels deep is a list too: the conversion opens at most 32 levels, which also ends a + # collection that holds itself. + $auditDeepScope = 'FilesOnly' + foreach ($level in 1..40) { + $auditDeepScope = , $auditDeepScope + } + $auditBadShapes = @( @{ Shape = 'a misspelled name'; Value = 'Typoo'; ValueText = 'Typoo' } @{ Shape = 'a blank string'; Value = ''; ValueText = $null } @{ Shape = 'a white-space string'; Value = ' '; ValueText = $null } @{ Shape = 'an undefined number'; Value = 99; ValueText = '99' } @{ Shape = 'a hashtable'; Value = @{ Unrelated = 1 }; ValueText = $null } + @{ Shape = 'an array of one misspelled name'; Value = @('Typoo'); ValueText = 'Typoo' } ) $auditFlagsBadShapes = @( @{ Shape = 'a misspelled name'; Value = 'Failurre'; ValueText = 'Failurre' } @@ -1017,18 +1025,20 @@ Describe 'Invalid and valid values of AuditFlags, InheritanceFlags, PropagationF @{ Shape = 'a white-space string'; Value = ' '; ValueText = $null } @{ Shape = 'an undefined number'; Value = 99; ValueText = '99' } @{ Shape = 'a hashtable'; Value = @{ Unrelated = 1 }; ValueText = $null } + @{ Shape = 'an array of one misspelled name'; Value = @('Failurre'); ValueText = 'Failurre' } ) - # The cross product of the two cmdlets that share these parameters and every bad shape: 2 * (5 + 3 * 5 + 5) = 50 rows. - # AppliesTo names one value, not a combination: PowerShell would add the numbers of a list, so that one of the - # values wins silently or a scope that the caller did not name is chosen. AuditFlags and the two flags parameters - # take a list. Every kind of collection counts, also a collection of one element that holds a list. + # The cross product of the two cmdlets that share these parameters and every bad shape: 2 * (6 + 3 * 6 + 6) = 60 rows. + # AppliesTo names one value, not a combination: PowerShell combines the numbers of a list bit by bit, so that one + # of the values wins silently or a scope that the caller did not name is chosen. AuditFlags and the two flags + # parameters take a list. Every kind of collection counts, also a collection of one element that holds a list. $auditAppliesToListShapes = @( @{ Shape = 'a comma list of two names'; Value = 'ThisFolderAndSubfolders,ThisFolderAndFiles'; ValueText = 'ThisFolderAndSubfolders,ThisFolderAndFiles' } @{ Shape = 'an array of two names'; Value = @('ThisFolderAndSubfolders', 'ThisFolderAndFiles'); ValueText = 'ThisFolderAndSubfolders, ThisFolderAndFiles' } @{ Shape = 'an array of one comma list'; Value = @('ThisFolderAndSubfolders,ThisFolderAndFiles'); ValueText = 'ThisFolderAndSubfolders,ThisFolderAndFiles' } @{ Shape = 'a hash set of two names'; Value = [System.Collections.Generic.HashSet[string]]::new([string[]]@('ThisFolderAndSubfolders', 'ThisFolderAndFiles')); ValueText = $null } @{ Shape = 'the keys of a hashtable'; Value = (@{ ThisFolderAndSubfolders = 1; ThisFolderAndFiles = 2 }).Keys; ValueText = $null } + @{ Shape = 'an array nested 40 levels deep'; Value = $auditDeepScope; ValueText = $null } ) $auditInvalidParameterCases = foreach ($cmdlet in 'Add-NTFSAudit', 'Remove-NTFSAudit') { @@ -1066,6 +1076,7 @@ Describe 'Invalid and valid values of AuditFlags, InheritanceFlags, PropagationF @{ Cmdlet = $cmdlet; Parameter = 'AppliesTo'; Value = @('ThisFolderAndSubfolders', 'ThisFolderAndFiles') } @{ Cmdlet = $cmdlet; Parameter = 'AppliesTo'; Value = @('ThisFolderAndSubfolders,ThisFolderAndFiles') } @{ Cmdlet = $cmdlet; Parameter = 'AppliesTo'; Value = [System.Collections.Generic.HashSet[string]]::new([string[]]@('ThisFolderAndSubfolders', 'ThisFolderAndFiles')) } + @{ Cmdlet = $cmdlet; Parameter = 'AppliesTo'; Value = $auditDeepScope } } # A named $null (a literal $null, a variable, splatting, or $PSDefaultParameterValues) must stay a @@ -1181,7 +1192,7 @@ Describe 'Invalid and valid values of AuditFlags, InheritanceFlags, PropagationF $invalidValueErrors[0].CategoryInfo.Category | Should -Be 'InvalidArgument' $invalidValueErrors[0].Exception.Message | Should -Match ([regex]::Escape($Parameter)) if ($ValueText) { - $invalidValueErrors[0].Exception.Message | Should -Match ([regex]::Escape($ValueText)) + $invalidValueErrors[0].Exception.Message | Should -Match ("The value '" + [regex]::Escape($ValueText) + "'") } if ($canReadAudit) { Get-TestAuditSddl -Path $path | Should -BeExactly $before