CODEWITHSUNDEEP
ruby-on-rails
Robert Travis Pierce
Robert Travis Pierce

Reputation: 94

Is This a Safe Way to Permit Params in Rails on the Back End?

In one of my Rails (6.1.0) models I have a few columns that need to be protected from CRUD actions based on user role. I have implemented frontend form input disabling but not sure if I am protecting the backend correctly/safely. I experimented with a few ways and settled on this conditional concatenation on the params object, but since it's not something I have seen out there explicitly, I'm not confident it's best practice and/or fully safe. Is there a better way?

Given some user permission/access levels:

class User < ApplicationRecord
...

enum role: {guest: 0, employee: 1, manager: 2, executive: 3, administrator: 4, superadmin: 5, developer: 6}

#TODO: Extract & expand this authorization scheme (Pundit?) 
def has_employee_rights?
  self.employee? || self.manager? || self.executive? || self.administrator? || self.superadmin? || self.developer?
end

def has_manager_rights?
  self.manager? || self.executive? || self.administrator? || self.superadmin? || self.developer?
end

def has_executive_rights?
  self.executive? || self.administrator? || self.superadmin? || self.developer?
end

def has_administrator_rights?
  self.administrator? || self.superadmin? ||  self.developer?
end

def has_superadmin_rights?
  self.superadmin? || self.developer?
end

def has_developer_rights?
  self.developer?
end

...
end

I have handled the front end form by setting the 'readonly' attribute (Haml w/ classes removed for clarity):

  # Normal form field

  = form.label :prov_dist_code, "Provisional Dist. Code"
  = form.text_field :prov_dist_code, value: @project.prov_dist_code

  # "Protected" form fields

  = form.label :dist_code, "Final Distribution Code", readonly: !current_user.has_manager_rights?
  = form.text_field :dist_code, value: @project.dist_code

  = form.label :pcode, "PCode"
  = form.text_field :pcode, readonly: !current_user.has_administrator_rights?, value: @project.pcode

Then in the controller I further protect the column:

class ProjectsController < ApplicationController
  ...

  def create
    @project = Project.new(project_params)

    if @project.save
      redirect_to @project, notice: "Project was successfully created."
    else
      render :new
    end
  end

  ...

  private

    #TODO: Is this conditional concatenation safe/adequate?
    def allowed_params
      ap = [:network_id,
            :dist_title,
            :dist_description,
            :prov_dist_code,
            :name,
            :hosts,
            :guests...]
      ap << :pcode if current_user.has_administrator_rights?
      ap << :dist_code if current_user.has_manager_rights?
    end

    def project_params
      params.require(:project).permit(*allowed_params)
    end
end

Upvotes: 0

Views: 294

Answers (1)

max
max

Reputation: 102001

What you're doing is really just creating an array of arguments programatically and using the splat operator to apply the array as a list of arguments. There is nothing inherantly unsafe about that. The only real security concern would be if you're taking keys for the whitelist from the user.

From a code review standpoint through it does place a lot of logic in the controller layer where its hard to test and adds even more responsibilities to your model. Since you mentioned Pundit thats actually an excellent choice for where to consolidate your authorization logic as its easy to test in isolation.

class ApplicationPolicy < Pundit::Policy

  # ...

  private

  def employee?
    user.employee? || user.manager? || user.executive? || user.administrator? || user.superadmin? || user.developer?
  end
  
  def manager?
    user.manager? || user.executive? || user.administrator? || user.superadmin? || user.developer?
  end
  
  def executive?
    user.executive? || user.administrator? || user.superadmin? || user.developer?
  end
  
  def administrator?
    user.administrator? || user.superadmin? ||  user.developer?
  end
  
  def superadmin?
    user.superadmin? || user.developer?
  end
  
  def developer?
    user.developer?
  end
end

class ProjectPolicy < ApplicationPolicy

  BASE_ATTRIBUTES = [
    :network_id,
    :dist_title,
    :dist_description,
    :prov_dist_code,
    :name,
    :hosts,
    :guests
  ].freeze
  # ...

  def permitted_attributes
    BASE_ATTRIBUTES.dup.then do |attrs|
      attrs << :pcode if administrator?
      attrs << :dist_code if manager?
    end
  end
end

You can then use your policy to whitelist the parameters:

class ProjectsController < ApplicationController
  ...

  def create
    @project = Project.new(permitted_attributes(@project))

    if @project.save
      redirect_to @project, notice: "Project was successfully created."
    else
      render :new
    end
  end
end

Upvotes: 1

Related Questions