Best way to implement user-dependent validations

rubyonrails-talk
,
1

Say the users of my app are admins of email domains and each of them is allowed to create mailboxes for one or multiple domains by typing out the email address for that mailbox. Currently, the Mailbox-model has an unpersisted attribute that acts as a whitelist for domains.

Is there an alternative/better way to implement something like this? I most likely want to implement this as a model validation to display a nice message to the user. I do not want to discard invalid email attributes nor do I want to have a view-only solution (something like using a <select> for the domain).

class Mailbox < ApplicationRecord
  attribute :_allowed_domains, array: true

  validates :email, presence: true, format: URI::MailTo::EMAIL_REGEXP
  validate :email_domain_allowed, if: :email

  private
  def email_domain_allowed
    raise "_allowed_domains is not populated, cannot validate" if self._allowed_domains.nil?

    self.errors.add(:email, :domain_not_allowed) if self._allowed_domains.exclude?(self.email.split("@", 2).last)
  end
end

class MailboxesController < ApplicationController
  def create
    @mailbox = Mailbox.new(mailbox_params)
    respond_to do |format|
      if @mailbox.save
        format.html { redirect_to(@mailbox) }
        format.json { head(:created) }
      else
        format.html { render("new", status: :unprocessable_content) }
        format.json { render(json: { error: @mailbox.errors.full_messages.to_sentence }, status: :unprocessable_content) }
      end
    end
  end

  private
  def mailbox_params
    parameters = params.expect(mailbox: [:email])
    parameters[:_allowed_domains] = current_login.user.allowed_domains
    return parameters
  end
end
2

I think overall you have a good general direction although there are some details I would change if it were me.

I wouldn’t make it prefixed with an _. That usually implies some sort of private or internal value but since you are setting it in another object (your controller) it’s obviously not internal. I would make allowed_domains as an non-persisted attribute like you have it but without the _ prefix.

Next, I would not inject the value into the params like you are doing. That seems to be mixing different goals into the same function. I would keep mailbox_params just focused on filtering the user submitted params. For assigning the allowed_domains I would probably just put that in the action after the initialization of the mailbox. So:

@mailbox = Mailbox.new(mailbox_params)
@mailbox.allowed_domains = current_login.user.allowed_domains

Finally I would probably not use a custom validation. I might make a getter called domain that returns the domain part of the email:

def domain = email.split("@", 2).last

Then you can use the normal exclusion validation:

validates :domain, exclusion: { in: allowed_domains }, allow_nil: true

The only thing that is missing your verification that the allowed domains was configured but I’m not sure that is even necessary. If you define allowed_domains with a default:

attribute :allowed_domains, array: true, default: []

Since you have a default of an empty array, if an email is provided then it will always fail. This is a bit different as it gives an error to the user instead of throwing an error for the developer to catch but might be fine enough.

If you really want to throw an error then I would probably just use a before_save callback:

before_save { raise "...." if allowed_domains.empty? }
3

Thanks for your feedback, much appreciated.

Yeah, tampering with the params seems wrong, but on the other hand, it is more “reusable”, f.e. I could just call it in create and update without having to remember to also explicitly set allowed_domains. Maybe the method could be renamed to mailbox_params_with_allowed_domains to be more explicit that it’s not “only” user-provided params.

One note, though, mutable objects should not be used as default in the attribute API, so the call must be attribute :allowed_domains, array: true, default: -> { [] }.

Since I’m kind of paranoid, I prefer raising in case allowed_domains was not set.