Ensure @params in shadow resource always has a valid value.

[miah]

Add tests for method chained shadow resource with readable and unreadable shadow files.

Ensure @params always has a safe value, otherwise we may stacktrace when
unable to read /etc/shadow and invoked with method chaining.

My only complaint with this is that when the /etc/shadow file is unreadable, you won't see the error when chaining; you'll need to test the file or a specific user.

Signed-off-by: Miah Johnson [email protected]

Fixes #2913

[jerryaldrichiii]

Looks good to me! Thanks @miah!

[TrevorBramble]

You asked for it! ;^)

[TrevorBramble]

I'd really rather see this assignment preserved, rather than treating the setup as a side-effect of calling a method.

def initialize
  @params = set_params(@lines)
end

def set_params(lines)
  lines.nil? ? [] : lines.map { |l| parse_shadow_line(1) }
end

[TrevorBramble]

While we're there, @raw_content and @lines are only used in the initializer (if you pass @lines to the params method) so why not make them local variables instead? You could even inline the lines parsing because there's no conditional logic involved.

def initialize(path = '/etc/shadow', opts = nil)
  # ...
  params(raw_content.to_s.split("\n"))
end

...And while we're there

def initialize(path = '/etc/shadow', opts = nil)
  opts ||= {}
  # ...
end

This makes me so sad. Would you mind?

def initialize(path = '/etc/shadow', opts = {})
  # ...
end

[miah]

I like this @params = params(@raw_content.to_s.split("\n")) but when I set up our params this way, when filter is called I'm still getting a nil value.

NoMethodError: undefined method `find_all' for nil:NilClass
  /Users/miah/projects/github/tmp/inspec/lib/resources/shadow.rb:80:in `block in filter'

So what I've done is preserved @lines and reference that from set_params.

[TrevorBramble]

Ah, I missed def lines elsewhere. Still, I'd pass it into the new method that makes your params and use it as a local variable there.

[TrevorBramble]

Couple of directions this could go to make the point more obvious.

You could separate the guard statement:

def params(lines)
  return [] if lines.nil?

  lines.map { |l| parse_shadow_line(1) }
end

And that maps directly to your logic in the ternary. But what I think you're actually concerned about is calling map on nil? So this would actually be more direct and clear:

def params(lines)
  Array(lines).map { |l| parse_shadow_line(1) }
end

Array(nil) will produce the empty array, and pass through the map call to return what you expect, just that empty array. So there's one path through the code and all the needed safety.

[TrevorBramble]

Consider using collect here instead of map to make it clear that you're collecting a new set of values from the input data. (Same for the parsed shadow lines in the params method.)

[TrevorBramble]

Separate sections with newlines please. =^)

[TrevorBramble]

Thank you!

[chris-rock]

I am wondering if we could check if the file exist and throw an exception here? Otherwise we absorb the error.

[miah]

Definitely can do this, but the file resource we use for reading and content gathering also does this; but we do absorb the error when method chained.

[chris-rock]

I think it would be beneficial to move the read logic out of the initialize function and do this during runtime not object creation time. This would allow us to do less during inspec check runs.

[jquick]

Thanks @miah