nested_attributes_for does not work with STI

[pwnall]

When using the *_attributes= method generated by nested_attributes_for to create new models the newly created models always have the base class. The type attribute is ignored.

Failing test case against master:
pwnall@344cddb

Branch containing the failing test case:
https://github.com/pwnall/rails/tree/nested_attrs_sti

I'll try to work on a fix. In the meantime, I hope this can help others experiencing this problem.

The bug seems to be well-known, but I couldn't find any proposed fixes.
http://grosser.it/2010/06/04/fixing-rails-nested-attributes-on-collections-with-sti/
http://stackoverflow.com/questions/2553931/can-nested-attributes-be-used-in-combination-with-inheritance
http://coderrr.wordpress.com/2008/04/22/building-the-right-class-with-sti-in-rails/

[pwnall]

First attempt at fixing the bug here.

pwnall@2a8774b

[pixeltrix]

This isn't just an issue with nested_attributes - creating a model via an association directly will fail also. The reason is you don't want arbitrary data going into the type column as it could create a security hole:

class User < ActiveRecord::Base; end
class Administrator < User; end

class ApplicationController < ActionController::Base
  before_filter :load_site

  def load_site
    @site = Site.find_by_url!(request.host)
  end
end

class UsersController < ApplicationController
  respond_to :html, :json

  def create
    @user = @site.users.build(params[:user])
    flash[:notice] = 'You have successfully signed up to our service' if @user.save
    respond_with(@user)
  end
end

If Rails didn't protect both the foreign key and the type column this code would be open to all sorts of abuse. If you want to enable it on a class by class basis then the generic module below will allow you to pass a #{klass}_type attribute with the underscore version of a class name:

module Morphing
  extend ActiveSupport::Concern

  module ClassMethods
    def new(attributes = {}, options = {}, &block)
      prefix = base_class.name.demodulize.underscore

      klass_type = attributes.delete("#{prefix}_type".to_sym)
      return super if klass_type.blank?

      klass_name = "#{klass_type.to_s.camelize}"
      if parent.const_defined?(klass_name)
        klass = parent.const_get(klass_name)
      elsif parent != Object && Object.const_defined?(klass_name)
        klass = Object.const_get(klass_name)
      else
        begin
          klass = parent.const_missing(klass_name)
        rescue NameError => e
          if e.instance_of?(NameError)
            raise ArgumentError, "Unknown #{prefix} type: #{klass_type}"
          else
            raise e
          end
        end
      end

      klass.new(attributes, options, &block)
    end
  end
end

This then allows you to do the following:

admin = User.new(:user_type => 'administrator')

There has been a pull request for this feature before: #1686

[pwnall]

Thank you for your comments!

I agree that allowing the type column to participate in mass assignment is a security hole, but I disagree with your conclusion. I think that attr_accessible and attr_protected are available and well-known, and can be used to handle the security issue.

Let's look at the User / Administrator example that you gave, because I think it is very useful. If the distinction between users and admins is implemented by a is_admin boolean field or by a role text field, then the code needs to use attr_protected / attr_accessible, to avoid the security hole that you've shown. Why should type be any different?

Furthermore, I think that not having uniform behavior is more dangerous, because people will Google for the issue, land on one of the blog posts that I mentioned in my bug report, and apply the fix, without thinking of security. If we have a simple, uniform conceptual model, we have more time and energy to think about security.

Thank you for your time and consideration!