archive_file: better handle mode property and deprecate Integer valuesfix_file_mode

FileUtils.mkdir_p expects the mode to be in octal form. We accept Integers, but Ruby doesn't have a way to tell if what the user passed was octal or not. The safest thing to do here is to only accept mode as a String and then convert it to octal form with .to_i(8). This adds a new deprecation and correctly sets the file mode value as octal. Right now this feature works like this:

String mode: applies wrong permission
Base 10 mode: applies wrong permissions
Octal mode: Works

New behavior:

String mode: Works
Base 10 mode: Works but throws a deprecation warning
Octal mode: applies the wrong permissions and throws a deprecation warning

Signed-off-by: Tim Smith <tsmith@chef.io>

3 files changed, 24 insertions, 4 deletions

diff --git a/lib/chef/deprecated.rb b/lib/chef/deprecated.rb
index 9ac08f43ee..d67cee8153 100644
--- a/lib/chef/deprecated.rb
+++ b/lib/chef/deprecated.rb
@@ -237,6 +237,10 @@ class Chef
       target 29
     end
 
+    class ArchiveFileIntegerFileMode < Base
+      target 30
+    end
+
     class Generic < Base
       def url
         "https://docs.chef.io/chef_deprecations_client/"
diff --git a/lib/chef/resource/archive_file.rb b/lib/chef/resource/archive_file.rb
index 58e106d9bc..c02d160729 100644
--- a/lib/chef/resource/archive_file.rb
+++ b/lib/chef/resource/archive_file.rb
@@ -53,7 +53,7 @@ class Chef
         description: "The group of the extracted files."
 
       property :mode, [String, Integer],
-        description: "The mode of the extracted files.",
+        description: "The mode of the extracted files. Integer values are deprecated as octal strings (ex. 0755) would not be interpreted correctly.",
         default: "755"
 
       property :destination, String,
@@ -85,7 +85,8 @@ class Chef
           Chef::Log.trace("File or directory does not exist at destination path: #{new_resource.destination}")
 
           converge_by("create directory #{new_resource.destination}") do
-            FileUtils.mkdir_p(new_resource.destination, mode: new_resource.mode.to_i)
+            # @todo when we remove the ability for mode to be an int we can remove the .to_s below
+            FileUtils.mkdir_p(new_resource.destination, mode: new_resource.mode.to_s.to_i(8))
           end
 
           extract(new_resource.path, new_resource.destination, Array(new_resource.options))
@@ -113,6 +114,12 @@ class Chef
       end
 
       action_class do
+        def define_resource_requirements
+          if new_resource.mode.is_a?(Integer)
+            Chef.deprecated(:archive_file_integer_file_mode, "The mode property should be passed to archive_file resources as a String and not an Integer to ensure the value is properly interpreted.")
+          end
+        end
+
         # This can't be a constant since we might not have required 'ffi-libarchive' yet.
         def extract_option_map
           {
diff --git a/spec/unit/resource/archive_file_spec.rb b/spec/unit/resource/archive_file_spec.rb
index 5e44284b66..6effe550db 100644
--- a/spec/unit/resource/archive_file_spec.rb
+++ b/spec/unit/resource/archive_file_spec.rb
@@ -18,8 +18,11 @@
 require "spec_helper"
 
 describe Chef::Resource::ArchiveFile do
-
-  let(:resource) { Chef::Resource::ArchiveFile.new("foo") }
+  let(:node) { Chef::Node.new }
+  let(:events) { Chef::EventDispatch::Dispatcher.new }
+  let(:run_context) { Chef::RunContext.new(node, {}, events) }
+  let(:resource) { Chef::Resource::ArchiveFile.new("foo", run_context) }
+  let(:provider) { resource.provider_for_action(:extract) }
 
   it "has a resource name of :archive_file" do
     expect(resource.resource_name).to eql(:archive_file)
@@ -41,6 +44,12 @@ describe Chef::Resource::ArchiveFile do
     expect(resource.mode).to eql("755")
   end
 
+  it "mode property throws a deprecation warning if Integers are passed" do
+    expect(Chef::Log).to receive(:deprecation)
+    resource.mode 755
+    provider.define_resource_requirements
+  end
+
   it "options property defaults to [:time]" do
     expect(resource.options).to eql([:time])
   end