From e38019b4234d248da688a08abcff3d752e71b718 Mon Sep 17 00:00:00 2001 From: Eugene Gavrilov Date: Mon, 29 Oct 2018 00:17:43 +0500 Subject: [PATCH 1/5] update rails version in dummy app to 5.2, run migrations in environment --- spec/dummy/config/application.rb | 1 + spec/dummy/config/environments/development.rb | 4 ++++ .../migrate/20150428220101_create_people.rb | 4 ++++ spec/integration/rails_spec.rb | 19 +++++++++++++++++++ spec/spec_helper.rb | 8 ++++---- vault.gemspec | 2 +- 6 files changed, 33 insertions(+), 5 deletions(-) diff --git a/spec/dummy/config/application.rb b/spec/dummy/config/application.rb index 1e7e54e8..7f6c1a9c 100644 --- a/spec/dummy/config/application.rb +++ b/spec/dummy/config/application.rb @@ -13,6 +13,7 @@ module Dummy class Application < Rails::Application + config.load_defaults 5.2 # Settings in config/environments/* take precedence over those specified here. # Application configuration should go into files in config/initializers # -- all .rb files in that directory are automatically loaded. diff --git a/spec/dummy/config/environments/development.rb b/spec/dummy/config/environments/development.rb index 71e0a329..7a92d4f7 100644 --- a/spec/dummy/config/environments/development.rb +++ b/spec/dummy/config/environments/development.rb @@ -36,4 +36,8 @@ # Raises error for missing translations # config.action_view.raise_on_missing_translations = true + + config.after_initialize do + ActiveRecord::MigrationContext.new(Rails.root.join('db/migrate')).migrate + end end diff --git a/spec/dummy/db/migrate/20150428220101_create_people.rb b/spec/dummy/db/migrate/20150428220101_create_people.rb index 344c262f..c6e5fd1a 100644 --- a/spec/dummy/db/migrate/20150428220101_create_people.rb +++ b/spec/dummy/db/migrate/20150428220101_create_people.rb @@ -1,4 +1,8 @@ +<<<<<<< 132e351f918e3a32e2c8c730b186b88104cedfb3 class CreatePeople < ActiveRecord::Migration[4.2] +======= +class CreatePeople < ActiveRecord::Migration[5.2] +>>>>>>> update rails version in dummy app to 5.2, run migrations in environment def change create_table :people do |t| t.string :name diff --git a/spec/integration/rails_spec.rb b/spec/integration/rails_spec.rb index 54344995..70955255 100644 --- a/spec/integration/rails_spec.rb +++ b/spec/integration/rails_spec.rb @@ -226,6 +226,25 @@ expect(person.credit_card_was).to eq("1234567890111213") end + it "tracks dirty attributes init with empty field" do + person = Person.new(credit_card: '1234') + person.save! + expect(person.cc_encrypted).to eql('12312321') + + # expect(person.credit_card_changed?).to be(false) + # expect(person.credit_card_change).to eq(nil) + # expect(person.credit_card_was).to eq("1234567890111213") + + # person.credit_card = "123456789010" + + # expect(person.credit_card_changed?).to be(true) + # expect(person.credit_card_change).to eq(["1234567890111213", "123456789010"]) + # expect(person.credit_card_was).to eq("1234567890111213") + + # person.save! + # expect(person.credit_card).to eq("") + end + it "allows attributes to be unset" do person = Person.create!(credit_card: "1234567890111213") person.update_attributes!(credit_card: nil) diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 4dcf41fc..3f356c79 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -1,7 +1,7 @@ -$LOAD_PATH.unshift File.expand_path("../../lib", __FILE__) -require "vault/rails" +$LOAD_PATH.unshift File.expand_path('../../lib', __FILE__) +require 'vault/rails' -require "rspec" +require 'rspec' RSpec.configure do |config| # Prohibit using the should syntax @@ -21,4 +21,4 @@ config.order = 'random' end -require File.expand_path("../dummy/config/environment.rb", __FILE__) +require File.expand_path('../dummy/config/environment.rb', __FILE__) diff --git a/vault.gemspec b/vault.gemspec index adc27add..77ac8efd 100644 --- a/vault.gemspec +++ b/vault.gemspec @@ -17,7 +17,7 @@ Gem::Specification.new do |s| s.files = Dir["{app,config,db,lib}/**/*", "LICENSE", "Rakefile", "README.md"] s.test_files = Dir["spec/**/*"] - s.add_dependency "rails", [">= 4.1"] + s.add_dependency "rails", [">= 5.2"] s.add_dependency "vault", "~> 0.5" s.add_development_dependency "appraisal", "~> 2.1" From 05c9ea89683d992642832449389c0d3dd839367e Mon Sep 17 00:00:00 2001 From: Eugene Gavrilov Date: Sat, 4 May 2019 23:32:05 +0500 Subject: [PATCH 2/5] fix for dirty changed attributes --- lib/vault/encrypted_model.rb | 97 +++++++++++-------- spec/dummy/app/models/person.rb | 6 +- .../migrate/20150428220101_create_people.rb | 4 - spec/integration/rails_spec.rb | 19 ---- 4 files changed, 59 insertions(+), 67 deletions(-) diff --git a/lib/vault/encrypted_model.rb b/lib/vault/encrypted_model.rb index 01c553a0..b24b2ea4 100644 --- a/lib/vault/encrypted_model.rb +++ b/lib/vault/encrypted_model.rb @@ -41,10 +41,16 @@ module ClassMethods # a proc to encode the value with # @option options [Proc] :decode # a proc to decode the value with - def vault_attribute(attribute, options = {}) - encrypted_column = options[:encrypted_column] || "#{attribute}_encrypted" + def vault_attribute(vault_attr, options = {}) + # https://github.com/rails/rails/issues/33753#issuecomment-417503496 + # You cannot call attribute_will_change! on something not registered + # with the attributes API -- There must be attribute :user_ids if you + # want that to be managed by Active Record. + attribute(vault_attr) if defined? attributes + + encrypted_column = options[:encrypted_column] || "#{vault_attr}_encrypted" path = options[:path] || "transit" - key = options[:key] || "#{Vault::Rails.application}_#{table_name}_#{attribute}" + key = options[:key] || "#{Vault::Rails.application}_#{table_name}_#{vault_attr}" context = options[:context] default = options[:default] @@ -68,53 +74,56 @@ def vault_attribute(attribute, options = {}) end # Getter - define_method("#{attribute}") do - self.__vault_load_attributes! unless @__vault_loaded - instance_variable_get("@#{attribute}") + define_method(vault_attr.to_s) do + __vault_load_attributes! unless @__vault_loaded + instance_variable_get("@#{vault_attr}") end # Setter - define_method("#{attribute}=") do |value| - self.__vault_load_attributes! unless @__vault_loaded + define_method("#{vault_attr}=") do |value| + __vault_load_attributes! unless @__vault_loaded # We always set it as changed without comparing with the current value # because we allow our held values to be mutated, so we need to assume # that if you call attr=, you want it send back regardless. + attribute_will_change!(vault_attr.to_s) + instance_variable_set("@#{vault_attr}", value) - attribute_will_change!("#{attribute}") - instance_variable_set("@#{attribute}", value) - - # Return the value to be consistent with other AR methods. - value + # Call super method or return value to be consistent with other AR methods. + if defined?(super) + super(value) + else + value + end end # Checker - define_method("#{attribute}?") do + define_method("#{vault_attr}?") do self.__vault_load_attributes! unless @__vault_loaded - instance_variable_get("@#{attribute}").present? + instance_variable_get("@#{vault_attr}").present? end # Dirty method - define_method("#{attribute}_change") do - changes["#{attribute}"] + define_method("#{vault_attr}_change") do + changes["#{vault_attr}"] end # Dirty method - define_method("#{attribute}_changed?") do - changed.include?("#{attribute}") + define_method("#{vault_attr}_changed?") do + changed.include?("#{vault_attr}") end # Dirty method - define_method("#{attribute}_was") do - if changes["#{attribute}"] - changes["#{attribute}"][0] + define_method("#{vault_attr}_was") do + if changes["#{vault_attr}"] + changes["#{vault_attr}"][0] else - public_send("#{attribute}") + public_send("#{vault_attr}") end end # Make a note of this attribute so we can use it in the future (maybe). - __vault_attributes[attribute.to_sym] = { + __vault_attributes[vault_attr.to_sym] = { context: context, default: default, encrypted_column: encrypted_column, @@ -175,6 +184,9 @@ def vault_lazy_decrypt! # Vault and decrypt any attributes unless vault_lazy_decrypt is set. after_initialize :__vault_initialize_attributes! + # Before save we keep changed attributes to variable + before_save :__vault_keep_changed_attributes! + # After we save the record, persist all the values to Vault and reload # them attributes from Vault to ensure we have the proper attributes set. # The reason we use `after_save` here is because a `before_save` could @@ -251,20 +263,21 @@ def __vault_load_attribute!(attribute, options) def __vault_persist_attributes! changes = {} - self.class.__vault_attributes.each do |attribute, options| - if c = self.__vault_persist_attribute!(attribute, options) - changes.merge!(c) - end + changed_attributes = instance_variable_get('@vault_changed_attributes') + # Only persist changed attributes to minimize requests - this helps + # minimize the number of requests to Vault. + changed_attributes.each do |attribute| + options = self.class.__vault_attributes[attribute] + persist_attribute = self.__vault_persist_attribute!(attribute, options) + changes.merge!(persist_attribute) if persist_attribute end # If there are any changes to the model, update them all at once, # skipping any callbacks and validation. This is okay, because we are # already in a transaction due to the callback. - if !changes.empty? - self.update_columns(changes) - end + self.update_columns(changes) unless changes.empty? - return true + true end # Encrypt a single attribute using Vault and persist back onto the @@ -276,19 +289,11 @@ def __vault_persist_attribute!(attribute, options) column = options[:encrypted_column] context = options[:context] - # Only persist changed attributes to minimize requests - this helps - # minimize the number of requests to Vault. - if !changed.include?("#{attribute}") - return - end - # Get the current value of the plaintext attribute plaintext = instance_variable_get("@#{attribute}") # Apply the serialize to the plaintext value, if one exists - if serializer - plaintext = serializer.encode(plaintext) - end + plaintext = serializer.encode(plaintext) if serializer # Generate context if needed generated_context = __vault_generate_context(context) @@ -321,6 +326,16 @@ def __vault_generate_context(context) end end + # Keep changed attributes + def __vault_keep_changed_attributes! + instance_variable_set( + '@vault_changed_attributes', + self.class.__vault_attributes.keys & changed.map(&:to_sym) + ) + + true + end + # Override the reload method to reload the Vault attributes. This will # ensure that we always have the most recent data from Vault when we # reload a record from the database. diff --git a/spec/dummy/app/models/person.rb b/spec/dummy/app/models/person.rb index fc6514a7..2f0cb4d7 100644 --- a/spec/dummy/app/models/person.rb +++ b/spec/dummy/app/models/person.rb @@ -1,4 +1,4 @@ -require "binary_serializer" +require 'binary_serializer' class Person < ActiveRecord::Base include Vault::EncryptedModel @@ -7,8 +7,8 @@ class Person < ActiveRecord::Base vault_attribute :credit_card, encrypted_column: :cc_encrypted, - path: "credit-secrets", - key: "people_credit_cards" + path: 'credit-secrets', + key: 'people_credit_cards' vault_attribute :details, serialize: :json diff --git a/spec/dummy/db/migrate/20150428220101_create_people.rb b/spec/dummy/db/migrate/20150428220101_create_people.rb index c6e5fd1a..344c262f 100644 --- a/spec/dummy/db/migrate/20150428220101_create_people.rb +++ b/spec/dummy/db/migrate/20150428220101_create_people.rb @@ -1,8 +1,4 @@ -<<<<<<< 132e351f918e3a32e2c8c730b186b88104cedfb3 class CreatePeople < ActiveRecord::Migration[4.2] -======= -class CreatePeople < ActiveRecord::Migration[5.2] ->>>>>>> update rails version in dummy app to 5.2, run migrations in environment def change create_table :people do |t| t.string :name diff --git a/spec/integration/rails_spec.rb b/spec/integration/rails_spec.rb index 70955255..54344995 100644 --- a/spec/integration/rails_spec.rb +++ b/spec/integration/rails_spec.rb @@ -226,25 +226,6 @@ expect(person.credit_card_was).to eq("1234567890111213") end - it "tracks dirty attributes init with empty field" do - person = Person.new(credit_card: '1234') - person.save! - expect(person.cc_encrypted).to eql('12312321') - - # expect(person.credit_card_changed?).to be(false) - # expect(person.credit_card_change).to eq(nil) - # expect(person.credit_card_was).to eq("1234567890111213") - - # person.credit_card = "123456789010" - - # expect(person.credit_card_changed?).to be(true) - # expect(person.credit_card_change).to eq(["1234567890111213", "123456789010"]) - # expect(person.credit_card_was).to eq("1234567890111213") - - # person.save! - # expect(person.credit_card).to eq("") - end - it "allows attributes to be unset" do person = Person.create!(credit_card: "1234567890111213") person.update_attributes!(credit_card: nil) From 42e3bac102c7124c0b2d2e15318e836ac75d0ef5 Mon Sep 17 00:00:00 2001 From: Eugene Gavrilov Date: Sun, 5 May 2019 00:46:21 +0500 Subject: [PATCH 3/5] test fixes --- Appraisals | 4 ++++ lib/vault/encrypted_model.rb | 2 +- spec/dummy/config/application.rb | 1 - spec/dummy/config/environments/development.rb | 4 ---- vault.gemspec | 2 +- 5 files changed, 6 insertions(+), 7 deletions(-) diff --git a/Appraisals b/Appraisals index 6b02f1a9..609041a8 100644 --- a/Appraisals +++ b/Appraisals @@ -9,3 +9,7 @@ end appraise "rails-5.1" do gem "rails", "~> 5.1.0" end + +appraise "rails-5.2" do + gem "rails", "~> 5.2.0" +end diff --git a/lib/vault/encrypted_model.rb b/lib/vault/encrypted_model.rb index b24b2ea4..6e8d6002 100644 --- a/lib/vault/encrypted_model.rb +++ b/lib/vault/encrypted_model.rb @@ -46,7 +46,7 @@ def vault_attribute(vault_attr, options = {}) # You cannot call attribute_will_change! on something not registered # with the attributes API -- There must be attribute :user_ids if you # want that to be managed by Active Record. - attribute(vault_attr) if defined? attributes + attribute(vault_attr, ActiveRecord::Type::Value.new) encrypted_column = options[:encrypted_column] || "#{vault_attr}_encrypted" path = options[:path] || "transit" diff --git a/spec/dummy/config/application.rb b/spec/dummy/config/application.rb index 7f6c1a9c..1e7e54e8 100644 --- a/spec/dummy/config/application.rb +++ b/spec/dummy/config/application.rb @@ -13,7 +13,6 @@ module Dummy class Application < Rails::Application - config.load_defaults 5.2 # Settings in config/environments/* take precedence over those specified here. # Application configuration should go into files in config/initializers # -- all .rb files in that directory are automatically loaded. diff --git a/spec/dummy/config/environments/development.rb b/spec/dummy/config/environments/development.rb index 7a92d4f7..71e0a329 100644 --- a/spec/dummy/config/environments/development.rb +++ b/spec/dummy/config/environments/development.rb @@ -36,8 +36,4 @@ # Raises error for missing translations # config.action_view.raise_on_missing_translations = true - - config.after_initialize do - ActiveRecord::MigrationContext.new(Rails.root.join('db/migrate')).migrate - end end diff --git a/vault.gemspec b/vault.gemspec index 77ac8efd..adc27add 100644 --- a/vault.gemspec +++ b/vault.gemspec @@ -17,7 +17,7 @@ Gem::Specification.new do |s| s.files = Dir["{app,config,db,lib}/**/*", "LICENSE", "Rakefile", "README.md"] s.test_files = Dir["spec/**/*"] - s.add_dependency "rails", [">= 5.2"] + s.add_dependency "rails", [">= 4.1"] s.add_dependency "vault", "~> 0.5" s.add_development_dependency "appraisal", "~> 2.1" From d64b594a2ad48d9e9c5c9814c19b4f3bd8f1566b Mon Sep 17 00:00:00 2001 From: Eugene Gavrilov Date: Sun, 5 May 2019 01:19:53 +0500 Subject: [PATCH 4/5] fix for rails 4.1 --- gemfiles/.bundle/config | 2 ++ lib/vault/encrypted_model.rb | 4 +++- 2 files changed, 5 insertions(+), 1 deletion(-) create mode 100644 gemfiles/.bundle/config diff --git a/gemfiles/.bundle/config b/gemfiles/.bundle/config new file mode 100644 index 00000000..c127f802 --- /dev/null +++ b/gemfiles/.bundle/config @@ -0,0 +1,2 @@ +--- +BUNDLE_RETRY: "1" diff --git a/lib/vault/encrypted_model.rb b/lib/vault/encrypted_model.rb index 6e8d6002..97d57c2a 100644 --- a/lib/vault/encrypted_model.rb +++ b/lib/vault/encrypted_model.rb @@ -46,7 +46,9 @@ def vault_attribute(vault_attr, options = {}) # You cannot call attribute_will_change! on something not registered # with the attributes API -- There must be attribute :user_ids if you # want that to be managed by Active Record. - attribute(vault_attr, ActiveRecord::Type::Value.new) + if defined? ActiveRecord::Type # for rails versions > 4.1 + attribute(vault_attr, ActiveRecord::Type::Value.new) + end encrypted_column = options[:encrypted_column] || "#{vault_attr}_encrypted" path = options[:path] || "transit" From 99233c7cc01f92aaa83f3057cac1ebb848b3ac9a Mon Sep 17 00:00:00 2001 From: Eugene Gavrilov Date: Thu, 16 May 2019 00:15:57 +0500 Subject: [PATCH 5/5] clear pull request --- gemfiles/.bundle/config | 2 -- lib/vault/encrypted_model.rb | 14 +++++++++----- spec/dummy/app/models/person.rb | 7 ++++--- spec/spec_helper.rb | 8 ++++---- 4 files changed, 17 insertions(+), 14 deletions(-) delete mode 100644 gemfiles/.bundle/config diff --git a/gemfiles/.bundle/config b/gemfiles/.bundle/config deleted file mode 100644 index c127f802..00000000 --- a/gemfiles/.bundle/config +++ /dev/null @@ -1,2 +0,0 @@ ---- -BUNDLE_RETRY: "1" diff --git a/lib/vault/encrypted_model.rb b/lib/vault/encrypted_model.rb index 97d57c2a..c9cbd0ff 100644 --- a/lib/vault/encrypted_model.rb +++ b/lib/vault/encrypted_model.rb @@ -77,13 +77,13 @@ def vault_attribute(vault_attr, options = {}) # Getter define_method(vault_attr.to_s) do - __vault_load_attributes! unless @__vault_loaded + self.__vault_load_attributes! unless @__vault_loaded instance_variable_get("@#{vault_attr}") end # Setter define_method("#{vault_attr}=") do |value| - __vault_load_attributes! unless @__vault_loaded + self.__vault_load_attributes! unless @__vault_loaded # We always set it as changed without comparing with the current value # because we allow our held values to be mutated, so we need to assume @@ -277,9 +277,11 @@ def __vault_persist_attributes! # If there are any changes to the model, update them all at once, # skipping any callbacks and validation. This is okay, because we are # already in a transaction due to the callback. - self.update_columns(changes) unless changes.empty? + if !changes.empty? + self.update_columns(changes) + end - true + return true end # Encrypt a single attribute using Vault and persist back onto the @@ -295,7 +297,9 @@ def __vault_persist_attribute!(attribute, options) plaintext = instance_variable_get("@#{attribute}") # Apply the serialize to the plaintext value, if one exists - plaintext = serializer.encode(plaintext) if serializer + if serializer + plaintext = serializer.encode(plaintext) + end # Generate context if needed generated_context = __vault_generate_context(context) diff --git a/spec/dummy/app/models/person.rb b/spec/dummy/app/models/person.rb index 2f0cb4d7..f459ab17 100644 --- a/spec/dummy/app/models/person.rb +++ b/spec/dummy/app/models/person.rb @@ -1,4 +1,4 @@ -require 'binary_serializer' +require "binary_serializer" class Person < ActiveRecord::Base include Vault::EncryptedModel @@ -7,8 +7,8 @@ class Person < ActiveRecord::Base vault_attribute :credit_card, encrypted_column: :cc_encrypted, - path: 'credit-secrets', - key: 'people_credit_cards' + path: "credit-secrets", + key: "people_credit_cards" vault_attribute :details, serialize: :json @@ -42,3 +42,4 @@ def encryption_context "user_#{id}" end end + diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 3f356c79..4dcf41fc 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -1,7 +1,7 @@ -$LOAD_PATH.unshift File.expand_path('../../lib', __FILE__) -require 'vault/rails' +$LOAD_PATH.unshift File.expand_path("../../lib", __FILE__) +require "vault/rails" -require 'rspec' +require "rspec" RSpec.configure do |config| # Prohibit using the should syntax @@ -21,4 +21,4 @@ config.order = 'random' end -require File.expand_path('../dummy/config/environment.rb', __FILE__) +require File.expand_path("../dummy/config/environment.rb", __FILE__)