Feature/issue 253 amazonmq broker user password #301
Conversation
…User Password property rules
| 'F52' | ||
| end | ||
|
|
||
| def resource_type |
There was a problem hiding this comment.
this method is unnecessary since this class doesn't derive from PasswordBaseRule
|
|
||
| violating_resources = resources.select do |resource| | ||
| violating_users = resource.users.select do |user| | ||
| insecure_parameter?(cfn_model, user['Password']) || \ |
There was a problem hiding this comment.
-
break this logic out into a predicate. the backslashes give me the willies, but aside from that a complex boolean like this deserves its own named method to lower CCM and improve readability
-
checking the nil.... i'm hemming and hawing over that. cfn-model is the underlying gem that is supposed to parse and prepare things so that we don't have to do a lot of this checking in nag rules. since this is a nested data structure though (User) we are stuck doing some of that work here. i guess i'd suggest checking it's there before trying to reference it... but then if it's not there, just do nothing.
| end | ||
| end | ||
|
|
||
| context 'AmazonMQBroker has password from Not Secure String in Systems Manager' do |
There was a problem hiding this comment.
missing password is one of the contexts to capture for coverage
| violating_users = resource.users.select do |user| | ||
| insecure_parameter?(cfn_model, user['Password']) || \ | ||
| insecure_string_or_dynamic_reference?(cfn_model, user['Password']) || \ | ||
| user['Password'].nil? |
There was a problem hiding this comment.
actually.... can't you derive form PasswordBaseRule and use the subproperty behavior? i'm not for sure on that
…t defined in AWS::AmazonMQ::Broker resource
| end | ||
| end | ||
|
|
||
| def get_violating_users(cfn_model, resource) |
There was a problem hiding this comment.
call this violating_users? name the resource argument as"mq_broker
| end | ||
| end | ||
|
|
||
| def check_user_property(user_property) |
There was a problem hiding this comment.
i'd nuke this method. the return values could be true or nil which is possibly confusing - but you can check the nil? inline where this is invoked imho
| 'F52' | ||
| end | ||
|
|
||
| def check_password_property(cfn_model, user) |
There was a problem hiding this comment.
'check' is a bit opaque as a term. while i would never say comments are never necessary, imho writing them usually means something isn't obvious from the code itself.
user_has_insecure_password? is probably a better name
|
@erickascic Made some additional refactors based on your comments:
|
ghost
merged commit 
Adding custom rules and tests for issue #253 - AWS::AmazonMQ::Broker User Password property rules.