Avoid ActiveRecord queries starting with model class

22 Nov 2022

When writing a Rails resource controller code it’s quite common to start with querying from model class itself: Model.find, Model.find_by, Model.where, etc.. After all for simplicity sake it is what’s given in some Rails guides examples:

class ArticlesController < ApplicationController
  def index
    @articles = Article.all
  end

  def show
    @article = Article.find(params[:id])
  end

  def new
    @article = Article.new
  end

  def create
    @article = Article.new(article_params)

    if @article.save
      redirect_to @article
    else
      render :new, status: :unprocessable_entity
    end
  end

  def edit
    @article = Article.find(params[:id])
  end

  def update
    @article = Article.find(params[:id])

    if @article.update(article_params)
      redirect_to @article
    else
      render :edit, status: :unprocessable_entity
    end
  end

  def destroy
    @article = Article.find(params[:id])
    @article.destroy

    redirect_to root_path, status: :see_other
  end

  private
    def article_params
      params.require(:article).permit(:title, :body)
    end
end

What is not shown in these examples is user authorisation to view/create/update/delete the resource. As soon as we add Devise to our app it is tempting to implement article access authorization like this:

class ArticlesController < ApplicationController
  def index
    @articles = Article.where(owner: current_user)
  end

  def show
    @article = Article.find_by(id: params[:id], owner: current_user)
  end

  def new
    @article = Article.new
  end

  def create
    @article = Article.new(article_params)
    @article.owner = current_user

    if @article.save
      redirect_to @article
    else
      render :new, status: :unprocessable_entity
    end
  end

  def edit
    @article = Article.find_by(id: params[:id], owner: current_user)
  end

  def update
    @article = Article.find_by(id: params[:id], owner: current_user)

    if @article.update(article_params)
      redirect_to @article
    else
      render :edit, status: :unprocessable_entity
    end
  end

  def destroy
    @article = Article.find(params[:id])
    @article.destroy

    redirect_to root_path, status: :see_other
  end

  private
    def article_params
      params.require(:article).permit(:title, :body)
    end
end

Works fine until one forgets to add owner: current_user in one of queries. Which is quite easy to miss. And as an example I made described error in the #destroy method. As a result one system user can delete articles owned by another user by “feeding” the method some random identifier.

It should be caught in code review. But can we reduce likelihood of an error in this case by changing the way we write resource queries?

My rule of thumb for this: avoid queries starting with model class and if possible load it through what is known/loaded in current context.

In our case we have Devise loaded current user instance. All we need to do is “walk” resource relationship graph until we reach the relevant resource:

class ArticlesController < ApplicationController
  def index
    @articles = current_user.articles
  end

  def show
    @article = current_user.articles.find(params[:id])
  end

  def new
    @article = current_user.articles.build
  end

  def create
    @article = current_user.articles.build(article_params)

    if @article.save
      redirect_to @article
    else
      render :new, status: :unprocessable_entity
    end
  end

  def edit
    @article = current_user.articles.find(params[:id])
  end

  def update
    @article = current_user.articles.find(params[:id])

    if @article.update(article_params)
      redirect_to @article
    else
      render :edit, status: :unprocessable_entity
    end
  end

  def destroy
    @article = current_user.articles.find(params[:id])
    @article.destroy

    redirect_to root_path, status: :see_other
  end

  private
    def article_params
      params.require(:article).permit(:title, :body)
    end
end

This way looking up something without scoping to current user is pretty much impossible.

Note that it also applies to initializing/creating resource records as well. ActiveRecord relationships provide methods #build and #create which sets up relevant owner record attribute for you.