Rails 4.0 api_key implementation

Question

I have a concept of the user having many API keys, but the API key belongs to the user, so I did the following:

config/routes.rb

  resources :users do
    resources :api_keys, path: '/developmentcenter'
  end

app/models/api_key.rb

class ApiKey < ActiveRecord::Base
  belongs_to :user
end

app/controllers/api_keys.rb

Note: I have separate some of the logic up because I don't like messy methods, so I split them into lib/aisis_planner/api_keys_controller_mixins.rb.

class ApiKeysController < ApplicationController
  before_action :authenticate_user!

  def index
    @user = User.find(params[:user_id])
    @api_key = User.apikeys
  end

  def create
    @user = User.find(params[:user_id])
    @api_key = ApiKey.new(create_new_api_key)
    create_api_key(@api_key, @user)
    end

  def destroy
    @user = User.find(params[:user_id])
    destroy_api_key(@user)
  end

  private

  def create_new_api_key
    params.permit(:api_key, user_attributes: [:id])
  end
end

lib/aisis_planner/api_keys_controller_mixins.rb

module AisisPlanner
  module ApiKeysControllerMixins
    def create_api_key(api_key, user)
      if api_key.save
        flash[:notice] = 'Created a new API Key'
        redirect_to user_path(user.id)
      else
        flash[:alert] = 'Could not create API Key please try again'
        redirect_to user_path(user.id)
      end
    end

    def destroy_api_key(user)
      ApiKey.find(params[:id]).destroy
      flash[:notice] = 'Api Key has been deleted.'
      redirect_to user_path(user.id)
    end
  end
end

Now for the tests. There are no integration tests at this time (I use capybara).

require 'spec_helper'

describe ApiKeysController do
  let(:user) { FactoryGirl.create(:user) }

  describe "#create" do
    it "should not create an api key for those not logged in" do
      post :create, :user_id => '', :id => ''
      expect(response).to redirect_to login_path
    end

    it "should create an api key" do
      sign_in user
      cookies[:auth_token] = user.auth_token
      post :create, :user_id => user.id.to_param
      expect(response).to redirect_to user_path(user.id)
    end
  end

  describe "#destroy" do
    it "should not delete an api for a user not logged in" do
      post :destroy, :user_id => '', :id => ''
      expect(response).to redirect_to login_path
    end

    it "should delete an api key for a logged in user" do
      sign_in user
      cookies[:auth_token] = user.auth_token
      key = FactoryGirl.create(:api_key, user_id: user.id)
      post :destroy, :user_id => user.id.to_param, :id => key.id.to_param
    end
  end
end

What are your thoughts?

Answer 1

Quite a few things sprang to mind immediately, but as I delved deeper I realized that, well, there are a lot of issues. So here goes.

Outright bugs, I think

User.apikeys - what? Shouldn't that be @user.api_keys? But I notice you don't test the index action, which would have caught that.

Security Issues

You're not really using params filtering correctly. You're permitting (rather than requiring) some top-level params, but never actually filter the "sub-params". The point is usually to filter the nested params, e.g. params.require(:api_key).permit(:foo, :bar).
But judging by your tests, it seems that creating an API key doesn't need any params at all, which makes things simpler.

Secondly, create new API key records through the has_many association on User (which I assume you have); don't set the user_id directly. I.e. @user.api_keys.new(...). But read on first.

If the idea is that a user can only create API keys for himself/herself (which the tests seem to indicate), then don't find the user by params[:user_id]. Right now, it's very easy to craft a request with a different user_id param, and destroy API keys that belong to other users in the system.

So if you only want the current user to create keys for himself/herself, the route doesn't need to be nested at all, since you only ever need the currently logged in user. I'm assuming you're using Devise, so the current user is accessible via the aptly named current_user helper method. (You can also dispense with the nested routing, if you only want the current user).

Even so, any user would still be able to delete other people's keys, because you don't check whether a given id param actually belongs to the current user (or the @user for that matter). Basically, if someone's logged in, they can just call the destroy action with completely arbitrary numbers and destroy everything.

Heck, you can do that from the JS console in a browser. Try typing $.ajax({url: "/users/x/developmentcenter/y", type: "DELETE"}) where x is your own user ID (which you can see in a URL somewhere) and y is any number between 1 and whatever the maximum for your database's ID column is. If y happens to a key id - any key - it'll get destroyed.

The mixins could also be a source of security issues. See the section below about that.

Consistency and Naming

In your index action you have @api_key = User.apikeys.
Why is one singular, and the other one plural? @api_key should be called @api_keys (since it's a collection), and the has_many relation on User should be api_keys with an underscore (since that's how you write it everywhere else). Again, I'm thinking that last part is a straight-up bug.

Your create_new_api_key doesn't do what it says; it doesn't create anything. Call it api_key_params instead, since that's what it returns. Besides, it's pretty confusing to have two methods called create_new_api_key and create_api_key... which one do you think actually creates an API key? But again, judging by your tests, you don't need create_new_api_key at all, since no params filtering should be needed.

Pointless Mixins

I understand that you don't like long methods, but making a mixin module is excessive. Do you intend to create/destroy API keys (and redirect) in other controllers? I'm guessing that the answer is "no" (if it's "yes", you've got other problems). So your mixins can only be mixed into 1 specific class, which completely negates the point of having "mixins".
The only thing you're "gaining" is that you've split your code into two files. Now you've got two places to look whenever you have to work with the code. Personally, I had to look back and forth quite a few times to figure stuff out. Since this is supposed to be a very straight-forward controller, that's not a good sign.

Besides, responsibility is all over the map now. In create_api_key you pass in both the user and the new API key instance. So there, your controller code handles params, and object instantiation. But in destroy_api_key you only pass only the user, and let the mixin method mess around with params to find the API key to destroy.

Optimally, a mixin has zero dependencies on its use context (your code depends on at least params, redirect_to which are only relevant for controllers), and is of general use (your code simply isn't). What you're doing is moving some lines of code and making things worse.

I can't actually see where you include the module... if you're doing it in ApplicationController you're polluting every controller with completely irrelevant methods.

And all those methods become public methods on every controller they're included in, meaning they function as actions. So worst case scenario, if you've got a too-liberal routing scheme, all of those methods can be called as actions simply by putting their names in the URL.

Odds and Ends

Don't pass the id attribute to path/url helpers; pass the record itself. If you ever want to override to_param in your models to make nicer URLs, it won't have any effect if all your code explicitly passes only the id attribute. I.e. redirect_to user_path(user.id) → redirect_to user_path(user)

Furthermore, for link_to and redirect_to you don't even need to use a path/url helper if it's a resourceful route; you can just pass the record. Rails will figure the rest out. I.e. So redirect_to user_path(user) → redirect_to user

If you've only got index, create and destroy actions, then make that explicit in the routes. And there's no need for a slash in the path argument.
resources :api_keys, only: [:index, :create, :destroy], path: 'developmentcenter'

Your last test is missing an expectation. It just calls some stuff, but doesn't actually test anything. I'd suggest at least an expect {...}.to change(ApiKeys, :count).by(-1).

More tests in general - for instance, tests of all the security stuff mentioned above.

Proof-read your flash messages (and test 'em while you're at it).

---

Here's how I'd write it. I'm assuming you only want users to create/delete their own keys and no params are required to do so

class ApiKeysController < ApplicationController
  before_action :authenticate_user!

  def index; end # just access current_user.api_keys directly in the view

  def create
    api_key = current_user.api_keys.new
    if api_key.save
      redirect_to current_user, notice: 'Created a new API key'
    else
      redirect_to current_user, alert: 'Could not create a new API key. Please try again'
    end
  end

  def destroy
    api_key = current_user.api_keys.find params[:id]
    api_key.destroy
    redirect_to current_user, notice: 'API key has been deleted.'
  end
end

No mixins, yet shorter than the original, and a lot more secure