Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhancement: Brand new DSL #109

Open
synth opened this issue Feb 28, 2021 · 11 comments
Open

Enhancement: Brand new DSL #109

synth opened this issue Feb 28, 2021 · 11 comments
Assignees

Comments

@synth
Copy link

synth commented Feb 28, 2021

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!

@synth synth changed the title Global configuration DSL Enhancement: Global configuration DSL Feb 28, 2021
@ronaldtse
Copy link
Contributor

Hi @synth , thank you for the suggestion! Agree this is a good addition and any PR would be very welcome. 👍

cc @ribose-jeffreylau @skalee

@ribose-jeffreylau
Copy link
Contributor

@synth Looks good! PR is very welcome.

@synth
Copy link
Author

synth commented Mar 2, 2021

Cool. So, I forked this repo and ran rspec spec and got this error:

An error occurred in a `before(:suite)` hook.
Failure/Error: DatabaseCleaner[:mongoid].strategy = :truncation, { only: "users" }

DatabaseCleaner::UnknownStrategySpecified:
  The 'truncation' strategy does not exist for the mongoid ORM!  Available strategies: deletion
# ./spec/support/db_cleaner.rb:15:in `block (2 levels) in <top (required)>'
# ------------------
# --- Caused by: ---
# NameError:
#   uninitialized constant DatabaseCleaner::Mongoid::Truncation
#   ./spec/support/db_cleaner.rb:15:in `block (2 levels) in <top (required)>'

Any ideas?

@skalee
Copy link
Contributor

skalee commented Mar 3, 2021

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.

I remember that some time ago I was thinking about some kind of global masker configuration too. Actually, I liked the idea: everything is in one place, models are not polluted with attr_masker calls… But then I realized that if someone wants it this way, he will be better off implementing his own script. It's really simple like that:

User.unscoped.each do |usr|
  # do something
  usr.save!
end

But if you want to do it with attr_masker (maybe because you like that declarative syntax or maybe because you get a progress bar), then how re-opening your models and calling attr_masker in a Rake task like that:

namespace :db do
  task :setup_maskers => :environment do
    Rails.application.eager_load!

    class User
      attr_masker :name
    end

    # or

    User.class_exec do
      attr_masker :name
    end
  end
end

# see: https://stackoverflow.com/a/47784838/304175
Rake::Task["db:mask"].enhance(["db:setup_maskers"])

At the moment I don't have a good Rails application to test that, so consider it a hint rather than a proven solution, sorry! Oh, and I'm not sure if it works well with tools like Spring or Zeus.

I guess we should mention that in documentation.

Also, if you just want re-using, then remember that anything what responds to #call can be a masker. So you can define your custom masker, for example in Rails initializer:

MY_MASKER = lambda do |**args|
  # do sth
end

# or

class MyMasker
  def call(**args)
    # do sth
  end
end

and use it:

class User
  attr_masker :name, masker: MY_MASKER
  # or
  attr_masker :name, masker: MyMasker.new
end

@synth
Copy link
Author

synth commented Mar 3, 2021

Indeed, doing a User.class_eval initializer vs opening up the class in the rake task is probably a matter of preference. The former seems nicer syntactically but also permanently pollutes the class hierarchy which isn't cool - but that's also the main suggestion of this gem - eg attr_masker calls inside the main model. The rake task isn't as nice syntax, but makes more sense as the configuration really is only needed at Rake execution time.

Our initializer/Class.eval approach is working really well for us and in fact, we'll wrap the whole initializer in a Rails.env.production? check so its not executed in prod environments. Its just not very DRY for our particular use case.

Imagine something like this:

global_condition = ->(model: ,**) { model.company_id.in?(valid_company_ids) }
email_masker = ->(**) { Faker::Name.email }

attr_masker User do
  condition global_condition
  mask :email, masker: email_masker
  nullify :ip_address, :crypted_password
end

Some notes about the above potential syntax:

  • By having a global config and an attr_masker block syntax, you can create a really nice expressive DSL (condition, mask, nullify expressive high level specifications)
  • For instance, condition is a condition or scope to applied to all the masks
  • nullify: I've found that in a lot of cases, I don't really want to mask the data but I simply want to nullify it, so why not have a nice high level dsl for it.

Indeed, I concede that all of the above is possible without the DSL as you have described @skalee. In fact, we've met our somewhat advanced case as is by using the initializer/Class.eval approach and so we're super happy as is. But as rubyists, we strive for expressiveness, no? In any case, I'm blocked from proceeding to develop a PR due to the aforementioned issue just trying to run the tests. Happy to move forward with a potential global config DSL, if I can get unblocked. Otherwise, happy to leave as is and drop this idea as well. Cheers! :)

@skalee
Copy link
Contributor

skalee commented Mar 4, 2021

Indeed, doing a User.class_eval initializer vs opening up the class in the rake task is probably a matter of preference. The former seems nicer syntactically but also permanently pollutes the class hierarchy which isn't cool - but that's also the main suggestion of this gem - eg attr_masker calls inside the main model.

This is more a legacy thing rather than a deliberate decision – this gem is forked from attr_encrypted.

I really like the idea of moving configuration out of models to one place. Also, I am already convinced that the Rakefile trick is not obvious enough. IMO config/attr_masker.rb would be the best place for configuration. Adding support for that file with current API is as easy as adding one conditional require call. Let's do that.

Furthermore, I agree that API should be updated too, as dedicated configuration file opens new possibilities. However, I need to think the details through, sorry. One reason is that we want to support more ORMs eventually, Sequel for instance. It was difficult to do that with current API and I want the new API to be elastic enough. Regarding DSL and block syntax, I hope that a good set of keyword arguments will be a better option, but I'll think about that too. For sure the new API should not involve class re-opening.

So, I'll think on that and post updates here.

@synth
Copy link
Author

synth commented Mar 4, 2021

Sounds good. So, to provide some more information about our specific cases that we discovered implementing this gem, here you go:

  1. Simple attribute masking (this is what is very well covered now)
  2. Class level query conditions to filter rows from being masked across all attributes.
  3. Ability to nullify a set of attributes
  4. Ability to order class processing. And ability to order mask commands inside of a class.
  5. Ability for class level sql updates. There are times (like when nullifying) that instead of doing row level processing, you can get away with an update_all on the model which is obviously way faster than doing row level processing.

So, here's a contrived example that illustrates all of the above:

attr_masker Company do # This class runs first
  global_condition ->{ where.not(id: 1) } # sets global query scope
  mask :domain, -> { Faker::Internet.domain_name }
end

attr_masker User do # This class runs second
  global_condition ->{ where.not(company_id: 1) } # sets global query scope

  nullify :crypted_password, :last_login_ip # runs first clearing out all the crud we don't need
  mask :first_name, -> { Faker::Name.first_name }  # runs second
  mask :last_name, -> { Faker::Name.last_name }  # runs third
  mask :email, ->(model: ,**) { "#{model.first_name}.#{model.last_name}@#{model.company.domain}") # User runs after Company so we can guarantee the domain has been masked. Same goes for first/last name.
  update_all "global_unsubscribe = true" # finally, do a final update_all on any other mass assignments like making sure everyone is globally unsubscribed to make sure they dont accidentally get emails (even if we've masked them).
end

I know I'm being cheeky by getting rid of the keyword argument masker in the block syntax...It just feels nicer to me :)

Anyway, let me know what you think when you get to it! This is just how we're using it and I think could definitely add value to others. Cheers!

@ribose-jeffreylau
Copy link
Contributor

Thanks @synth for the good points, especially 5) since this is something we'd like to have as well. We hope to be able support most if not all of these use cases in the next API update.

@ronaldtse
Copy link
Contributor

ronaldtse commented Mar 5, 2021

Thank you @synth for the great insights — the proposed DSL is thoughtful and demonstrates good use. Agree with @ribose-jeffreylau .

@skalee skalee changed the title Enhancement: Global configuration DSL Enhancement: Brand new DSL Mar 5, 2021
@skalee
Copy link
Contributor

skalee commented Mar 5, 2021

Feature request for global configuration file (without API changes) has been extracted to #118. I prefer to keep this discussion open, so I've renamed the issue.

@skalee
Copy link
Contributor

skalee commented Mar 5, 2021

Cool. So, I forked this repo and ran rspec spec and got this error.

For reference, this error is now fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants