Issue with Strong Parameters and prep_signup_view

[davidmccormick]

Hi Daudi - many thanks for all of the work you have put into your milia gem! I have been trying to integrate 1.0.0-beta3 into my own project and hit an awkward issue that had me scratching my head for a couple of days and I think has highlighted an issue in the milia registrations controller and prep_signup_view code.

I was getting an ActiveModel::ForbiddenAttributesError error on my sign up form if hit submit whilst leaving the "Organisation Name" field blank (leaving the user fields blank was fine). When I ran your sample application I noticed that you don't have this issue (although your coupon field value becomes a hash for some reason - i'm leaving the coupon out so ignored this).

To cut a long painful story short, I had added a validation to my tenant model for the 'name' field: -

class Tenant < ActiveRecord::Base
    validates :name, presence: true, length: { maximum: 80 }, uniqueness: { case_sensitive: false }

I think it reasonable to want to add some validation on the tenant to ensure uniqueness, presence and length etc. This leads to a problem in the milia registrations controller. If the tenant does not save properly then the form is re-displayed by your registration controller: -

Tenant.transaction  do 
      @tenant = Tenant.create_new_tenant(sign_up_params_tenant, sign_up_params_coupon)
      if @tenant.errors.empty?   # tenant created

        initiate_tenant( @tenant )    # first time stuff for new tenant

        devise_create   # devise resource(user) creation; sets resource

        if resource.errors.empty?   #  SUCCESS!

            # do any needed tenant initial setup
          Tenant.tenant_signup(resource, @tenant, params[:coupon])

        else  # user creation failed; force tenant rollback
          raise ActiveRecord::Rollback   # force the tenant transaction to be rolled back  
        end  # if..then..else for valid user creation

      else
        prep_signup_view( @tenant, params[:user] , params[:coupon]) # PROBLEM
        render :new
      end # if .. then .. else no tenant errors

By adding a validation on the tenant I caused the tenant save to fail we get sent through prep_signup_view before the form is re-rendered. You are passing the tenant object instance variable but passing the params[:user] hash. The params hash does not have any strong parameter whitelisting (permits etc.) on it and so causes the ForbiddenAttributesError when klass_option_obj calls 'new' on User using this hash.

Further down the controller you call prep_signup_view like this: -

prep_signup_view( sign_up_params_tenant, sign_up_params, sign_up_params_coupon )

This is great because it preps the form by using the permitted strong parameter versions of the parameters but has the downside that all error messages are lost. The errors are lost because prep_signup_view recreates the @user, @Tenant and @coupon instance variables as new from the params hashes.

I have tested this modification to the registration create method which fixes the strong parameter issue and makes sure that ALL validation errors are available to the form: -

def create

  sign_out_session!
  build_resource(sign_up_params) # added by dave
  prep_signup_view( sign_up_params_tenant, sign_up_params, sign_up_params_coupon ) # added by dave

     # validate recaptcha first unless not enabled
  if !::Milia.use_recaptcha  ||  verify_recaptcha

    Tenant.transaction  do 
      @tenant = Tenant.create_new_tenant(sign_up_params_tenant, sign_up_params_coupon)
      if @tenant.errors.empty?   # tenant created

        initiate_tenant( @tenant )    # first time stuff for new tenant

        devise_create   # devise resource(user) creation; sets resource

        if resource.errors.empty?   #  SUCCESS!

            # do any needed tenant initial setup
          Tenant.tenant_signup(resource, @tenant, params[:coupon])

        else  # user creation failed; force tenant rollback
          raise ActiveRecord::Rollback   # force the tenant transaction to be rolled back  
        end  # if..then..else for valid user creation

      else
    resource.valid?
        #prep_signup_view( @tenant, resource, sign_up_params_coupon ) # removed by dave
        render :new
      end # if .. then .. else no tenant errors

    end  #  wrap tenant/user creation in a transaction

  else
    flash[:error] = "Recaptcha codes didn't match; please try again"
    resource.valid?
    @tenant.valid?
    #prep_signup_view( @tenant, resource, sign_up_params_coupon ) # removed by dave
    render :new
  end

end   # def create

By placing prep_signup_view at the top - it makes sure that all of the instance variables are created unless they already exist.

I needed to call devises build_resource function and the calls to valid? so that all form objects get validated, even if there are recaptcha errors or tenant problems. The form in your sample app doesn't appear to display any error messages.

One last change, it would be good to remember the tenant name even if there are errors with the user object validations, e.g. password_confirmation does not match. Instead of creating a new tenant form entry each time, you could use the existing one if already defined: -

      .group
        = fields_for( :tenant, @tenant || Tenant.new ) do |w|
          = w.label( :name, 'Organization', :class => "label" ) 
          = w.text_field( :name, :class => "text_field")
          %span.description unique name for your group or organization for the new account

This is a little ugly so it might be better to just use fields_for( @Tenant ) and add your prep_signup_view function to the devise controllers #new method (otherwise @Tenant will by nil the first time the form is called).

Thanks for having a look through my issue - I hope you will consider some of my suggestions! I do think that, as it stands, the sign_up form is broken if the tenant does not save properly (if someone innocently adds some simple validations to it!). I'm sure that a lot of people will be basing new applications on your sample application, so it would be very helpful if it it's able to display validation errors and remember the tenant and coupon fields successfully.

regards

Dave

[davidmccormick]

Oops - apologies, I had altered the prep_signup_view call when the re-captcha was not entered correctly; the version in your beta3 branch passes the non-whitelisted params hashes.

[dsaronin]

I believe these issues have been fixed in 1.0.0-beta-5