riboseinc / attr_masker Goto Github PK

The latest Rails 6.1 is two major versions ahead Rails 4.2. Upcoming API changes will be a good occasion to drop support for the latter. Also, we should drop support for some older Rubies, I haven't decided yet which ones.

@skalee since there have been many changes (positive changes) it's probably time to make a release.

Let me know when you're going to bump version, to make a release.

Then we'll change our code to adopt the latest version 👍

When the gem is used with another attribute modifier, such as attr_encryptor like below:

attr_encryptor :test_field, key: proc { |k| k.key }
attr_masker :test_field, column_name: :encrypted_test_field

1. This configuration is highly dependent on the order of definition.
2. The masker (and the encryptor) will produce an error because the "masked" field is not a valid value to the encryptor (and vice versa)

Is there a way we can avoid this problem and have it compatible?

Thanks!

We should create full RubyDocs documentation for the gem, and show it via GitHub Pages.

• A lot of warnings and weird output produced by specs.
• Cannot reliably test without Mongoid or ActiveRecord — WITHOUT_MONGOID etc. stopped working.

GitHub Actions and my own workstation are affected. Surprisingly, all works in Travis CI we are migrating away from.

• Builds on master against 2.7 output lots of deprecation warnings.
• Builds on master against Ruby HEAD do fail.

@ronaldtse any comments? Especially on this issue's importance?

After setting attr_mask, the original value cannot be returned.

@ronaldtse, I've just released v0.2.1 which contains some important bugfixes. Please push it to Rubygems.

Tests are run twice in each pull request, for example both following builds were triggered for a single push in #12 pull request.

• https://travis-ci.org/riboseinc/attr_masker/builds/256405677
• https://travis-ci.org/riboseinc/attr_masker/builds/256405673

@ronaldtse @abunashir can we do something with that? Not a serious problem though.

Currently, the default redaction action is to slap a "(redacted)" on the original String.

This could be made more flexible. But by default it should also be internationalized (i18n).

Target these languages first:

• en (default)
• zh_CN
• zh_TW
• ja
• ko
• fr
• de
• es
• pt

#56

In order to resolve the conflict of the relevant PR
I tried pushing, but I could not do it

• Proc as parameter
• create some default scrambling algorithms
• structured-text-preserving algorithms
  • e.g., keeping an HTML snippet valid HTML, but with masked inner text
• structured Object preserving algorithms
  • i.e. generalization of the above HTML scenario

At the moment attr_masker doesn't work with Rails 5 due to breaking changes in ActiveRecord.

Currently, there's basically zero tests.

Write tests to cover all configurations.

• Write specs for the default (optionless) usage
• Write specs for using custom maskers
• Write specs for using if/unless filters
• Write specs for using the column_name option
• Write specs for masking the marshalled data
• Write specs for the existing simplistic masker
• Write specs which prove that data is safe in the production environment

First, Thank you for this gem!

So, I definitely see an argument for putting the attr_masker calls inside the models as its currently implemented. But there is also an argument for a centralized global configuration so all the masker calls are together and managed independently of the ORM (ActiveRecord) models. Further, this enables common conditions and custom maskers to be reused across models.

Thanks to the beauty of ruby, this is easy to do. Here is a sample from our app

# config/initializers/attr_masker.rb
require 'faker' # Faker gem for simple realistic data masking
leave_alone_domains = ["primarydomain.com", "secondarydomain.io"]

Company.class_eval do
  condition = ->(company) { !company.domain.in?(leave_alone_domains) }
  domain_mask = ->(**) { "#{Faker::Internet.unique.domain_word}.fake.#{Faker::Internet.domain_suffix}" }

  attr_masker :domain, if: condition, masker: domain_mask
end

User.class_eval do
  condition = ->(user) { user.network.in?(leave_alone_domains) }
  email_mask = ->(model:, **) { "user#{model.id}@example.com" }

  attr_masker :email, unless: condition, masker: email_mask
  attr_masker :name, unless: condition
end

The above works beautifully but obviously is a bit cumbersome syntax. We can do better if this gem supported a higher level dsl. Here is one suggestion:

# config/initializers/attr_masker.rb

condition = ->(company) { !company.domain.in?(leave_alone_domains) }
domain_mask = ->(**) { "#{Faker::Internet.unique.domain_word}.fake.#{Faker::Internet.domain_suffix}" }
email_mask = ->(model:, **) { "user#{model.id}@example.com" }

attr_masker Company, :domain, if: condition, masker: domain_mask
attr_masker User, :email, if: condition, masker: email_mask

# or alternately do it in a block

attr_masker do
  mask Company, :domain, if: condition, masker: domain_mask
  mask User, :email, if: condition, masker: email_mask
end

I think something like this would be fairly simple to implement. I could potentially submit a PR if you think this is useful. Let me know! Thanks again!

No record should become invalid in the result of masking. The Rake task should loudly fail in the attempt of invalid record creation. Or, if it's difficult, we should at least add a clear warning to the README file.

@ronaldtse's comment:

Masking MUST make sure the record is still valid. However, it might be up to the user of the attr_masker gem to get this right, i.e. it's not the burden of attr_masker to ensure validity. I don't think it is trivial at all to make attr_masker reuse the validation rules (which can be very arbitrary) for its attribute to generate random but valid values.

In the case of masking causing bad records, it is up to the user to have done DB backups and to restore it to its valid state, and to fix up any bad masking settings before re-running db:mask.

Maybe we should make this point clear in the README, etc.

@ronaldtse any comments? Especially on this issue's importance?

ActiveRecord feature specs fail in GitHub Actions for Ruby 2.5.7, Rails 6.0.2, and WITHOUT="mongoid". Failure has been replicated by re-running it a couple of times.

This is really odd, especially that the same test on the same Ruby and Rails but WITHOUT="" succeeds. Looks like some race condition, or that examples interfere with each other.

https://github.com/riboseinc/attr_masker/runs/417065415?check_suite_focus=true

As reported in #109 (comment)

I'm working on a fix #110 , but stuck on the part where the gems database_cleaner-* with versions 2.x get installed when it shouldn't. These newer versions removed the called_externally? method from DatabaseCleaner, which caused the errors seen in https://github.com/riboseinc/attr_masker/runs/2021552398?check_suite_focus=true#step:8:14

Then there's this seemingly unrelated error reported by CodeCov:

https://github.com/riboseinc/attr_masker/runs/2021552207?check_suite_focus=true#step:8:32

The questions remain:

1. How to reliably pin the versions for database_cleaner-*?
2. What is happening with CodeCov? Is this something caused by something on our end?

When calling ::ActiveRecord::Base.descendants in all_models, some models cannot be loaded.

May consider to use Rails.application.eager_load! to load all models.

May be a bug in Ruby or Bundler. For instance, see:

• https://github.com/riboseinc/attr_masker/runs/1128318091
• https://github.com/riboseinc/attr_masker/runs/1128318030
• https://github.com/riboseinc/attr_masker/runs/1128092437

Database Cleaner 2.0 is out, get ready for that.

Currently tests are failing, e.g. in #97 with these:

Bundler could not find compatible versions for gem "activerecord":
In Rails-4.2.gemfile:
  activerecord (~> 4.2.0)

  attr_masker was resolved to 0.2.1, which depends on
    rails (>= 4.0.0, < 7) was resolved to 5.0.5.rc1, which depends on
      activerecord (= 5.0.5.rc1)

They just went modular, which will complicate our dependency set a bit. (IMO controversial move for a rather small project which doesn't pull runtime dependencies, but whatever.)

Anyway, we probably want to reorganize our dependencies. For now, < 1.8 version constraint has been added.

It has disappeared when we migrated to GHA, and must be reconfigured.

@ronaldtse Please add a coverage upload token to this repo. Precisely:

1. Find "Repository Upload Token" on this page: https://codecov.io/gh/riboseinc/attr_masker/settings
2. Add it to this repo as a secret variable named CODECOV_TOKEN.

Currently, rake db:mask only runs when Rails.env.production? is false.

Make this more flexible, maybe via a config somewhere, e.g. by passing ENV vars.

Already started, but there are some dependency issues, probably due to missing dependencies, which cause build failures.

This question is somewhat a continuation of my discussion with @ronaldtse in the #5, which consisted of following two comments.

skalee: I wonder what the :column_name option is for. Apparently it overrides the column name when it's different from the attribute name. However, I cannot think of a good reason to support it at the moment. It isn't present in the original gem, and has been added in commit 82cab53, so I bet it is used by one of your projects. Therefore, I ask you for some concrete example or green light on its removal.

ronaldtse: @skalee I suspect it was used to support attributes that uses different column names like attr_encrypted does, but it might no longer be used. @ribose-jeffreylau should be able to clarify this with authority since he wrote it.

However, after some thinking I am coming to conclusion that :column_name option is too limited to be useful. In most cases, the attribute name is the same as the column name. I can imagine rare cases when this assumption is false, like attachment attribute handled by Paperclip or Carrierwave gem, or some serialized data stored in a column which name is suffixed. However in some cases things tend to go even more complex, for example Paperclip may use up to 4 columns. Passing single column name to that parameter is not enough.

I suggest renaming :column_name to :column_names and allow passing an array of column names. For example,

attr_masker :photo, column_names: %i[photo_file_name photo_content_type]

would update two columns.

I am going to implement it this way unless someone opposes.

#96 removes the unnecessarily polluting executables. Need to make a new release.

Ping @skalee

CodeCov incorrectly reports that test coverage is about 60%. We need to investigate issue and reconfigure the tool.

Builds on master fail for Rails HEAD.

We are not testing this gem against Rails 5.2. Compatibility is unknown.

1. Test against Ruby 3 in CI.
2. Make sure all tests do pass.

Possibly relaxing the dependency constraint is enough.

If config/attr_masker.rb exists, then it should be loaded from a Rake task.

Extracted from #109.

Convert all hashes to the 1.9 key: value syntax.

Document all configuration options.