Closed fl0p3k closed 4 years ago
Hi @fl0p3k, thanks very much for this!
To help create a high quality resource that follows the patterns used in this module, can you fill out the proposed properties in this issue description, and detail what Cmdlets the resource calls. We can then discuss here whether any changes are needed before you finalise your PR.
The property DisplayName
is set to type qualifier Write
and not Required
, what is your thought here, that DisplayName
is set to the Name if not provided in a configuration?
The property ComplexityEnabled
says in the description ...is enabled for the default password policy
, what default password policy is that? Is the description meant to say that it is enabling this make use of the default password policy? 🤔
Would it be possible to skip the Credential
parameter and instead make use of the built-in PsDscRunAsCredential
parameter? Less logic to test and maintain. 🤔
Hi @fl0p3k , I've put the properties in a table to be easier to read.
Initial comments:
Ensure
property, with allowed values of Present
or Absent
. It will therefore need to additionally use the Remove-ADFineGrainedPasswordPolicy
cmdlet.Add-ADFineGrainedPasswordPolicySubject
and Remove-ADFineGrainedPasswordPolicySubject
cmdlets. I would suggest the following additional properties for this: Subjects
, SubjectsToInclude
and SubjectsToExclude
. Subjects
should enforce the policy to only apply to the specified list of identities. SubjectsToInclude
should apply the policy to the specified identities, but not remove any additional identities that the policy applies to. SubjectsToExclude
should remove the policy from applying to the specified identities, but not change any other identities the policy applies to. This follows the model used in the ADGroup
resource.LockoutDuration
, LockoutObservationWindow
, MaxPasswordAge
and MinPasswordAge
cmdlet parameters are actually TimeSpan
objects. Can we use the model used in xWebAdministration where String
representations of these are used, rather than using Uint32
?Description
parameter should be added to the resource.The property
DisplayName
is set to type qualifierWrite
and notRequired
, what is your thought here, thatDisplayName
is set to the Name if not provided in a configuration?The property
ComplexityEnabled
says in the description...is enabled for the default password policy
, what default password policy is that? Is the description meant to say that it is enabling this make use of the default password policy? 🤔Would it be possible to skip the
Credential
parameter and instead make use of the built-inPsDscRunAsCredential
parameter? Less logic to test and maintain. 🤔
I am not sure DisplayName is required since the underlying ActiveDirectory cmdlet does not require it.
As for the complexity enabled option I simply had a typo in the context. Corrected.
Regarding the credentials, when I look at the syntax of the module, I do see it as PsDscRunAsCredential:
ADFineGrainedPasswordPolicy [String] #ResourceName { Name = [string] [ComplexityEnabled = [bool]] [Credential = [PSCredential]] [DependsOn = [string[]]] [DisplayName = [string]] [DomainController = [string]] [LockoutDuration = [UInt32]] [LockoutObservationWindow = [UInt32]] [LockoutThreshold = [UInt32]] [MaxPasswordAge = [UInt32]] [MinPasswordAge = [UInt32]] [MinPasswordLength = [UInt32]] [PasswordHistoryCount = [UInt32]] [Precedence = [UInt32]] [ProtectedFromAccidentalDeletion = [bool]] [PsDscRunAsCredential = [PSCredential]] [ReversibleEncryptionEnabled = [bool]] }
Hi @fl0p3k , I've put the properties in a table to be easier to read.
Initial comments:
- As this resource can be added and deleted, the resource needs to have an
Ensure
property, with allowed values ofPresent
orAbsent
. It will therefore need to additionally use theRemove-ADFineGrainedPasswordPolicy
cmdlet.- The resource should manage subjects that the policy applies to using the
Add-ADFineGrainedPasswordPolicySubject
andRemove-ADFineGrainedPasswordPolicySubject
cmdlets. I would suggest the following additional properties for this:Subjects
,SubjectsToInclude
andSubjectsToExclude
.Subjects
should enforce the policy to only apply to the specified list of identities.SubjectsToInclude
should apply the policy to the specified identities, but not remove any additional identities that the policy applies to.SubjectsToExclude
should remove the policy from applying to the specified identities, but not change any other identities the policy applies to. This follows the model used in theADGroup
resource.- The
LockoutDuration
,LockoutObservationWindow
,MaxPasswordAge
andMinPasswordAge
cmdlet parameters are actuallyTimeSpan
objects. Can we use the model used in xWebAdministration whereString
representations of these are used, rather than usingUint32
?- The
Description
parameter should be added to the resource.- Can the property descriptions be based on the text from the New-ADFineGrainedPasswordPolicy Cmdlet documentation.
Thanks for the feedback and comments. As for the time span parameters, this module was closely based off the existing ADDomainDefaultPasswordPolicy DscResource in this module. I'm not sure changing the parameter type would be good unless we change both to ensure consistency for the operator experience, plus any existing configuration dependence would break if we did.
ADDomainDefaultPasswordPolicy [String] #ResourceName
{
DomainName = [string]
[ComplexityEnabled = [bool]]
[Credential = [PSCredential]]
[DependsOn = [string[]]]
[DomainController = [string]]
[LockoutDuration = [UInt32]]
[LockoutObservationWindow = [UInt32]]
[LockoutThreshold = [UInt32]]
[MaxPasswordAge = [UInt32]]
[MinPasswordAge = [UInt32]]
[MinPasswordLength = [UInt32]]
[PasswordHistoryCount = [UInt32]]
[PsDscRunAsCredential = [PSCredential]]
[ReversibleEncryptionEnabled = [bool]]
}
ADFineGrainedPasswordPolicy [String] #ResourceName
{
Name = [string]
[ComplexityEnabled = [bool]]
[Credential = [PSCredential]]
[DependsOn = [string[]]]
[DisplayName = [string]]
[DomainController = [string]]
[LockoutDuration = [UInt32]]
[LockoutObservationWindow = [UInt32]]
[LockoutThreshold = [UInt32]]
[MaxPasswordAge = [UInt32]]
[MinPasswordAge = [UInt32]]
[MinPasswordLength = [UInt32]]
[PasswordHistoryCount = [UInt32]]
[Precedence = [UInt32]]
[ProtectedFromAccidentalDeletion = [bool]]
[PsDscRunAsCredential = [PSCredential]]
[ReversibleEncryptionEnabled = [bool]]
}
The ADDomainDefaultPasswordPolicy
was written four years ago, and is on my list of resources needing a refactor.
I've discussed the TimeSpan
properties with @johlju, and we would prefer for them to be TimeSpan
string representations, as implemented by xWebAdministration. We will raise a future breaking change PR to update the ADDomainDefaultPasswordPolicy
resource to use the same type.
@fl0p3k, can you please update the initial issue comment with the final list of cmdlets and properties.
@fl0p3k, can you please update the initial issue comment with the final list of cmdlets and properties.
Issue updated
Can you add Remove-ADFineGrainedPasswordPolicy
to the cmdlet list and the Description
parameter.
Please also update the property descriptions where applicable from the New-ADFineGrainedPasswordPolicy cmdlet documentation.
Can you add
Remove-ADFineGrainedPasswordPolicy
to the cmdlet list and theDescription
parameter.Please also update the property descriptions where applicable from the New-ADFineGrainedPasswordPolicy cmdlet documentation.
Thanks added. I also updated the descriptions
Regarding the descriptions, can you check again? Lost of them still aren't correct.
Subjects
, properties, base the description on the Subjects
property in Add-ADFineGrainedPasswordPolicySubject.ComplexityEnabled
description isn't correct.LockoutObservationWindow
description isn't correct.MinPasswordAge
description isn't correct.MinPasswordLength
description isn't correct.
etc.@fl0p3k, can you please add the Description
property to the proposal, and your PR. The spelling of ADPrincipal
is also not correct, so can you fix that too. Thanks.
Description
New resource to support fine grained password policies for AD Principals.
Proposed properties
The new resource will be using the Active Directory module commands below:
The resource will contain the following properties:
Special considerations or limitations