From 03e44f93001db97953917e0a100c627e189e2be6 Mon Sep 17 00:00:00 2001 From: Kasper Timm Hansen Date: Sun, 4 Aug 2019 01:32:41 +0200 Subject: Revise credentials diffing flow to use a separate diff command Didn't like the complicated stuff that happened on credentials:edit. It would append to .gitattributes multiple times. Though I see why it was written that way. I'm cutting off for now, but since this new flow would require each developer to run --enable perhaps this should really be: 1. Developer enrolls Rails app by running `credentials:diff --enable` 2. credentials:edit checks .gitattributes for `diff=rails_credentials` and if the current file is covered by that. 3. If so, set up the "rails_credentials" driver automatically. --- railties/lib/rails/commands/credentials/USAGE | 14 +++ .../commands/credentials/credentials_command.rb | 19 ++-- .../credentials/credentials_command/diffing.rb | 43 +++------ railties/test/commands/credentials_test.rb | 104 ++++++++------------- railties/test/isolation/abstract_unit.rb | 4 +- 5 files changed, 79 insertions(+), 105 deletions(-) diff --git a/railties/lib/rails/commands/credentials/USAGE b/railties/lib/rails/commands/credentials/USAGE index c8d3fb9eda..0396fcb403 100644 --- a/railties/lib/rails/commands/credentials/USAGE +++ b/railties/lib/rails/commands/credentials/USAGE @@ -30,6 +30,20 @@ You could prepend that to your server's start command like this: RAILS_MASTER_KEY="very-secret-and-secure" server.start +=== Set up Git to Diff Credentials + +Rails provides `rails credentials:diff --enable` to instruct Git to call `rails credentials:diff` +when `git diff` is run on a credentials file. + +Any credentials files are set to use the "rails_credentials" diff driver in .gitattributes. +Since Git requires the diff driver to be set up in a config file, the command uses +the project local .git/config. Since that config isn't stored in Git each team member +must enable separately. + +Or set up the "rails_credentials" diff driver globally with: + + git config --global diff.rails_credentials.textconv "bin/rails credentials:diff" + === Editing Credentials This will open a temporary file in `$EDITOR` with the decrypted contents to edit diff --git a/railties/lib/rails/commands/credentials/credentials_command.rb b/railties/lib/rails/commands/credentials/credentials_command.rb index d1054f8b63..9b3a7dbb86 100644 --- a/railties/lib/rails/commands/credentials/credentials_command.rb +++ b/railties/lib/rails/commands/credentials/credentials_command.rb @@ -38,7 +38,6 @@ module Rails end say "File encrypted and saved." - enable_credentials_diffing rescue ActiveSupport::MessageEncryptor::InvalidMessage say "Couldn't decrypt #{content_path}. Perhaps you passed the wrong key?" end @@ -50,14 +49,20 @@ module Rails say credentials.read.presence || missing_credentials_message end - def diff(content_path) - @content_path = content_path + option :enable, type: :boolean, default: false, + desc: "Pass `--enable` to make credential files diffable with `git diff`" - extract_environment_option_from_argument(default_environment: extract_environment_from_path(content_path)) - require_application! + def diff(content_path = nil) + if @content_path = content_path + extract_environment_option_from_argument(default_environment: extract_environment_from_path(content_path)) + require_application! - say credentials.read.presence || credentials.content_path.read - rescue + say credentials.read.presence || credentials.content_path.read + else + require_application! + enable_diffing if options[:enable] + end + rescue ActiveSupport::MessageEncryptor::InvalidMessage say credentials.content_path.read end diff --git a/railties/lib/rails/commands/credentials/credentials_command/diffing.rb b/railties/lib/rails/commands/credentials/credentials_command/diffing.rb index 1598ecaa8d..b7330178a3 100644 --- a/railties/lib/rails/commands/credentials/credentials_command/diffing.rb +++ b/railties/lib/rails/commands/credentials/credentials_command/diffing.rb @@ -3,45 +3,28 @@ module Rails::Command::CredentialsCommand::Diffing # :nodoc: class Error < StandardError; end - def enable_credentials_diffing - unless already_answered? || enabled? - answer = yes?("Would you like to make the credentials diff from git more readable in the future? [Y/n]") + def enable_diffing + if enabled? + say "Already enabled!" + else + enable + say "Diffing enabled! Editing a credentials file will display a diff of what actually changed." end - - enable if answer - FileUtils.touch(tracker) unless answer.nil? rescue Error - say "Couldn't setup git to enable credentials diffing" + say "Couldn't setup Git to enable credentials diffing." end private - def already_answered? - tracker.exist? - end - def enabled? - system_call("git config --get 'diff.rails_credentials.textconv'", accepted_codes: [0, 1]) + system "git config --get diff.rails_credentials.textconv", out: File::NULL end def enable - system_call("git config diff.rails_credentials.textconv 'bin/rails credentials:diff'", accepted_codes: [0]) - - git_attributes = Rails.root.join(".gitattributes") - File.open(git_attributes, "a+") do |file| - file.write(<<~EOM) - config/credentials/*.yml.enc diff=rails_credentials - config/credentials.yml.enc diff=rails_credentials - EOM - end - end - - def tracker - Rails.root.join("tmp", "rails_pretty_credentials") - end + raise Error unless system("git config diff.rails_credentials.textconv 'bin/rails credentials:diff'") - def system_call(command_line, accepted_codes:) - result = system(command_line) - raise(Error) if accepted_codes.exclude?($?.exitstatus) - result + Rails.root.join(".gitattributes").write(<<~end_of_template, mode: "a") + config/credentials/*.yml.enc diff=rails_credentials + config/credentials.yml.enc diff=rails_credentials + end_of_template end end diff --git a/railties/test/commands/credentials_test.rb b/railties/test/commands/credentials_test.rb index ec8fa4365e..e5397523e1 100644 --- a/railties/test/commands/credentials_test.rb +++ b/railties/test/commands/credentials_test.rb @@ -89,62 +89,60 @@ class Rails::Command::CredentialsCommandTest < ActiveSupport::TestCase assert_match(/secret_key_base/, output) end - test "edit ask the user to opt in to pretty credentials" do - assert_match(/Would you like to make the credentials diff from git/, run_edit_command) + + test "show credentials" do + assert_match(/access_key_id: 123/, run_show_command) end - test "edit doesn't ask the user to opt in to pretty credentials when alreasy asked" do - app_file("tmp/rails_pretty_credentials", "") + test "show command raises error when require_master_key is specified and key does not exist" do + remove_file "config/master.key" + add_to_config "config.require_master_key = true" - assert_no_match(/Would you like to make the credentials diff from git/, run_edit_command) + assert_match(/Missing encryption key to decrypt file with/, run_show_command(allow_failure: true)) end - test "edit doesn't ask the user to opt in when user already opted in" do - content = <<~EOM - [diff "rails_credentials"] - textconv = bin/rails credentials:show - EOM - app_file(".git/config", content) + test "show command does not raise error when require_master_key is false and master key does not exist" do + remove_file "config/master.key" + add_to_config "config.require_master_key = false" - assert_no_match(/Would you like to make the credentials diff from git/, run_edit_command) + assert_match(/Missing 'config\/master\.key' to decrypt credentials/, run_show_command) end - test "edit ask the user to opt in to pretty credentials, user accepts" do - file = Tempfile.open("credentials_test") - file.write("y") - file.rewind + test "show command displays content specified by environment option" do + run_edit_command(environment: "production") - run_edit_command(stdin: file.path) + assert_match(/access_key_id: 123/, run_show_command(environment: "production")) + end + + test "show command properly expands environment option" do + run_edit_command(environment: "production") - git_attributes = app_path(".gitattributes") - expected = <<~EOM + output = run_show_command(environment: "prod") + assert_match(/access_key_id: 123/, output) + assert_no_match(/secret_key_base/, output) + end + + + test "diff enable diffing" do + run_diff_command(enable: true) + + assert_equal <<~EOM, File.read(app_path(".gitattributes")) config/credentials/*.yml.enc diff=rails_credentials config/credentials.yml.enc diff=rails_credentials EOM - assert(File.exist?(git_attributes)) - assert_equal(expected, File.read(git_attributes)) + Dir.chdir(app_path) do assert_equal "bin/rails credentials:diff", `git config --get 'diff.rails_credentials.textconv'`.strip end - ensure - file.close! end - test "edit ask the user to opt in to pretty credentials, user refuses" do - file = Tempfile.open("credentials_test") - file.write("n") - file.rewind - - run_edit_command(stdin: file.path) - - git_attributes = app_path(".gitattributes") - assert_not(File.exist?(git_attributes)) - ensure - file.close! - end + test "diff won't enable again if already enabled" do + app_file(".git/config", <<~EOM) + [diff "rails_credentials"] + textconv = bin/rails credentials:diff + EOM - test "show credentials" do - assert_match(/access_key_id: 123/, run_show_command) + assert_no_match(/Diffing enabled/, run_diff_command(enable: true)) end test "diff from git diff left file" do @@ -184,33 +182,6 @@ class Rails::Command::CredentialsCommandTest < ActiveSupport::TestCase assert_match(encrypted_content, run_diff_command(content_path)) end - test "show command raises error when require_master_key is specified and key does not exist" do - remove_file "config/master.key" - add_to_config "config.require_master_key = true" - - assert_match(/Missing encryption key to decrypt file with/, run_show_command(allow_failure: true)) - end - - test "show command does not raise error when require_master_key is false and master key does not exist" do - remove_file "config/master.key" - add_to_config "config.require_master_key = false" - - assert_match(/Missing 'config\/master\.key' to decrypt credentials/, run_show_command) - end - - test "show command displays content specified by environment option" do - run_edit_command(environment: "production") - - assert_match(/access_key_id: 123/, run_show_command(environment: "production")) - end - - test "show command properly expands environment option" do - run_edit_command(environment: "production") - - output = run_show_command(environment: "prod") - assert_match(/access_key_id: 123/, output) - assert_no_match(/secret_key_base/, output) - end private def run_edit_command(editor: "cat", environment: nil, **options) @@ -225,7 +196,8 @@ class Rails::Command::CredentialsCommandTest < ActiveSupport::TestCase rails "credentials:show", args, **options end - def run_diff_command(path, **options) - rails "credentials:diff", path, **options + def run_diff_command(path = nil, enable: nil, **options) + args = enable ? ["--enable"] : [path] + rails "credentials:diff", args, **options end end diff --git a/railties/test/isolation/abstract_unit.rb b/railties/test/isolation/abstract_unit.rb index 6077ba3ee7..0fe62df8ba 100644 --- a/railties/test/isolation/abstract_unit.rb +++ b/railties/test/isolation/abstract_unit.rb @@ -301,7 +301,7 @@ module TestHelpers # stderr:: true to pass STDERR output straight to the "real" STDERR. # By default, the STDERR and STDOUT of the process will be # combined in the returned string. - def rails(*args, allow_failure: false, stderr: false, stdin: File::NULL) + def rails(*args, allow_failure: false, stderr: false) args = args.flatten fork = true @@ -328,7 +328,7 @@ module TestHelpers out_read.close err_read.close if err_read - $stdin.reopen(stdin, "r") + $stdin.reopen(File::NULL, "r") $stdout.reopen(out_write) $stderr.reopen(err_write) -- cgit v1.2.3