[WIP] [PoC] Migrate from protected attributes to strong parameters

app/controllers/admin/api/buyers_users_controller.rb

@@ -1,6 +1,11 @@
 class Admin::Api::BuyersUsersController < Admin::Api::BuyersBaseController
   representer User
 
+  before_action :find_user, except: %i[create index]
+  before_action :build_new_user, only: %i[create]
+
+  attr_reader :user
+
   # User List
   # GET /admin/api/accounts/{account_id}/users.xml
   def index
@@ -12,11 +17,9 @@ def index
   # User Create
   # POST /admin/api/accounts/{account_id}/users.xml
   def create
-    user = new_user
-
     authorize! :create, user
 
-    user.unflattened_attributes = flat_params
+    user.unflattened_attributes = user_params
     user.signup_type = :api
 
     user.save
@@ -37,7 +40,7 @@ def show
   def update
     authorize! :update, user
 
-    user.update_with_flattened_attributes(flat_params)
+    user.update_with_flattened_attributes(user_params)
 
     respond_with(user)
   end
@@ -108,19 +111,30 @@ def authorize!(*args)
     current_user ? super : logged_in?
   end
 
-  def new_user
-    @new_user ||= buyer.users.new
-  end
-
   def users
     @users ||= begin
       conditions = params.slice(:state, :role)
       buyer.users.where(conditions)
     end
   end
 
-  def user
-    @user ||= buyer.users.find(params[:id])
+  private
+
+  def build_new_user
+    @user = buyer.users.new
+  end
+
+  def find_user
+    @user = buyer.users.find(params[:id])
+  end
+
+  def user_params
+    @user_params ||= begin
+                       allowed_attrs = user.defined_fields_names | %i(password password_confirmation)
+                       # TODO: are these parameters needed?
+                       # allowed_attrs |= %i(conditions cas_identifier open_id service_conditions)
+                       flat_params.permit(*allowed_attrs)
+                     end
   end
 
 end

app/controllers/admin/api/users_controller.rb

@@ -2,6 +2,10 @@ class Admin::Api::UsersController < Admin::Api::BaseController
   representer User
 
   before_action :can_create, only: :create
+  before_action :build_new_user, only: %i[create]
+  before_action :find_user, except: %i[create index]
+
+  attr_reader :user
 
   # User List (provider account)
   # GET /admin/api/users.xml
@@ -14,11 +18,9 @@ def index
   # User Create (provider account)
   # POST /admin/api/users.xml
   def create
-    user = new_user
-
     authorize! :create, user
 
-    user.unflattened_attributes = flat_params
+    user.unflattened_attributes = user_params
     user.signup_type = :api
 
     user.save
@@ -39,7 +41,7 @@ def show
   def update
     authorize! :update, user
 
-    user.update_with_flattened_attributes(flat_params, as: current_user.try(:role))
+    user.update_with_flattened_attributes(user_params)
 
     respond_with(user)
   end
@@ -110,19 +112,19 @@ def authorize!(*args)
     current_user ? super : logged_in?
   end
 
-  def new_user
-    @new_user ||= current_account.users.new
-  end
-
   def users
     @users ||= begin
       conditions = params.slice(:state, :role)
       current_account.users.but_impersonation_admin.where(conditions)
     end
   end
 
-  def user
-    @user ||= current_account.users.but_impersonation_admin.find(params[:id])
+  def build_new_user
+    @user = current_account.users.new
+  end
+
+  def find_user
+    @user = current_account.users.but_impersonation_admin.find(params[:id])
   end
 
   def can_create
@@ -134,4 +136,14 @@ def can_create
   def flat_params
     super.except(:id)
   end
+
+  def user_params
+    @user_params ||= begin
+                       allowed_attrs = user.defined_fields_names | %i(password password_confirmation cas_identifier)
+                       allowed_attrs |= [member_permission_service_ids: [], member_permission_ids: [], allowed_sections: [], allowed_service_ids: []] if (provider_key.present? || current_user.admin?)
+                       # TODO: are these parameters needed?
+                       # allowed_attrs |= %i(conditions open_id service_conditions)
+                       flat_params.permit(*allowed_attrs)
+                     end
+  end
 end

app/controllers/buyers/users_controller.rb

@@ -15,7 +15,7 @@ def update
     # TODO: I think this controller is used only on provider side
     user.validate_fields! if current_account.buyer?
 
-    user.attributes = user_params
+    user.attributes = permitted_user_params
     user.role = user_params.fetch(:role, user.role) if can?(:update_role, user)
 
     if user.save
@@ -80,10 +80,17 @@ def find_user
     @user = @account.users.find(params[:id]).decorate
   end
 
-  DEFAULT_PARAMS = %i[username email password password_confirmation role].freeze
-
   def user_params
-    @user_params ||= params.require(:user).permit(*DEFAULT_PARAMS, extra_fields: [*user.defined_extra_fields_names])
+    @user_params ||= params.require(:user)
+  end
+
+  def permitted_user_params
+    @permitted_user_params ||= begin
+                                 allowed_attrs = user.defined_builtin_fields_names | %i(password password_confirmation)
+                                 # TODO: are these parameters needed?
+                                 # allowed_attrs |= %i(conditions cas_identifier open_id service_conditions)
+                                 user_params.permit(*allowed_attrs, extra_fields: user.defined_extra_fields_names)
+                               end
   end
 
   def redirect_back_or_show_detail(**opts)

app/controllers/partners/users_controller.rb

@@ -20,13 +20,8 @@ def destroy
   end
 
   def create
-    @user = @account.users.build
-    @user.email = params[:email]
+    @user = @account.users.build(user_params)
     @user.password = SecureRandom.hex
-    @user.first_name = params[:first_name].presence
-    @user.last_name = params[:last_name].presence
-    @user.open_id = params[:open_id].presence
-    @user.username = params[:username]
     @user.signup_type = :partner
     @user.role = :admin
     @user.activate!
@@ -42,4 +37,10 @@ def create
   def find_account
     @account = @partner.providers.find(params[:provider_id])
   end
+
+  def user_params
+    # Partners controller only allows a subset of common attributes
+    allowed = [:email, :first_name, :last_name, :open_id, :username]
+    params.permit(*allowed)
+  end
 end

app/controllers/provider/admin/account/users_controller.rb

@@ -66,7 +66,7 @@ def update_resource(user, attributes)
 
     user.class.transaction do
       user.assign_attributes(attributes)
-      user.assign_attributes(protected_attributes, without_protection: can?(:update_role, user))
+      user.assign_attributes(protected_attributes) if can?(:update_role, user)
 
       user.save
     end

app/controllers/provider/admin/user/personal_details_controller.rb

@@ -5,7 +5,7 @@ class Provider::Admin::User::PersonalDetailsController < Provider::Admin::User::
   def edit; end
 
   def update
-    if current_user.update(user_params.except(:current_password))
+    if current_user.update(permitted_user_params)
       if current_user.just_changed_password?
         current_user.kill_user_sessions(user_session)
       end
@@ -26,7 +26,16 @@ def redirect_path
   end
 
   def user_params
-    params.require(:user)
+    @user_params ||= params.require(:user)
+  end
+
+  def permitted_user_params
+    @permitted_user_params ||= begin
+                                 allowed_attrs = current_user.defined_builtin_fields_names | %i(password)
+                                 # TODO: are these parameters needed?
+                                 # allowed_attrs |= %i(conditions cas_identifier open_id service_conditions)
+                                 user_params.permit(*allowed_attrs, extra_fields: current_user.defined_extra_fields_names)
+                               end
   end
 
   def current_password_verification

app/controllers/provider/invitee_signups_controller.rb

@@ -53,7 +53,9 @@ def can_create?
   end
 
   def build_user
-    @user = @invitation.make_user(params[:user] || {})
+    allowed_attrs = @invitation.account.users.new.defined_fields_names | %i(password password_confirmation)
+    user_params = params.fetch(:user, ActionController::Parameters.new).permit(*allowed_attrs)
+    @user = @invitation.make_user(user_params)
 
     # This is just a sanity guard added when splitting invitation
     # controllers. Remove when SURE.

app/lib/authentication/by_password.rb

@@ -32,8 +32,6 @@ module ByPassword
 
       validates :lost_password_token, :password_digest, length: { maximum: 255 }
 
-      attr_accessible :password, :password_confirmation
-
       scope :with_valid_password_token, -> { where { lost_password_token_generated_at >= 24.hours.ago } }
 
       alias_method :authenticated?, :authenticate

app/lib/fields/fields.rb

@@ -275,6 +275,14 @@ def defined_extra_fields_names
     defined_extra_fields.map(&:name)
   end
 
+  def defined_fields_names
+    defined_fields.map(&:name)
+  end
+
+  def defined_builtin_fields_names
+    defined_builtin_fields.map(&:name)
+  end
+
   def defined_fields_hash
     @defined_fields_hash ||= Hash[defined_fields.map { |f| [f.name.to_sym, f]}]
   end

app/models/user.rb

@@ -109,14 +109,6 @@ def moderatable
   validate :username_is_unique
   validates :open_id, uniqueness: { case_sensitive: true }, allow_nil: true
 
-  attr_accessible :title, :username, :email, :first_name, :last_name,
-                  :conditions, :cas_identifier, :open_id, :service_conditions,
-                  :job_role, :extra_fields, as: %i[default member admin]
-
-  attr_accessible :member_permission_service_ids, :member_permission_ids, as: %i[admin]
-
-
-
   def self.search_states
     %w(pending active)
   end

app/models/user/invitations.rb

@@ -5,7 +5,6 @@ module User::Invitations
     after_commit :accept_invitation, :on => :create
 
     attr_accessor :invitation
-    attr_accessible :invitation
 
     # TODO: refactor to make this work removing above attribute.
     # has_one :invitation

app/models/user/permissions.rb

@@ -8,8 +8,6 @@ module User::Permissions
   included do
     has_many :member_permissions, dependent: :destroy, autosave: true
 
-    attr_accessible :member_permission_service_ids, :member_permission_ids, :allowed_sections, :allowed_service_ids
-
     alias_method :allowed_sections, :member_permission_ids
     alias_method :allowed_sections=, :member_permission_ids=
     alias_method :allowed_service_ids, :member_permission_service_ids

lib/developer_portal/app/controllers/developer_portal/accounts/invitee_signups_controller.rb

@@ -33,7 +33,9 @@ def sso_create
       redirect_to strategy.redirect_to_on_successful_login
     else
       user_data = strategy.user_data || {}
-      @user.assign_attributes(user_data.to_hash.compact)
+      # TODO: verify that this is the right thing to do
+      user_attributes = user_data.to_hash.compact.slice(:username, :email)
+      @user.assign_attributes(user_attributes)
       session[:invitation_sso_uid] = user_data[:uid]
       session[:invitation_sso_system_name] = strategy.authentication_provider.system_name
       build_sso_authorization
@@ -119,7 +121,8 @@ def check_invitation!
   end
 
   def build_user
-    @user = @invitation.make_user(filter_readonly_params(params[:user], User))
+    allowed_attrs = @invitation.account.users.new.defined_fields_names | %i(password password_confirmation)
+    @user = @invitation.make_user(filter_readonly_params(params[:user], User).permit(*allowed_attrs))
   end
 
   def invitation_token

lib/developer_portal/app/controllers/developer_portal/admin/account/personal_details_controller.rb

@@ -18,7 +18,7 @@ def show
 
   def update
     resource.validate_fields!
-    if resource.errors.empty? && resource.update(user_params)
+    if resource.errors.empty? && resource.update(permitted_user_params)
       resource.kill_user_sessions(user_session) if resource.just_changed_password?
 
       redirect_to admin_account_users_path, notice: 'User was successfully updated.'
@@ -44,9 +44,12 @@ def deny_unless_can_update
   end
 
   def user_params
-    filter_readonly_params(params.require(:user), User).permit([:current_password] +
-                                   resource.special_fields +
-                                   resource.defined_fields.map(&:name))
+    @user_params ||= params.require(:user)
+  end
+
+  def permitted_user_params
+    @permitted_user_params ||= filter_readonly_params(user_params, User).permit(resource.special_fields +
+                                                                                resource.defined_fields.map(&:name))
   end
 
   def verify_current_password

lib/developer_portal/app/controllers/developer_portal/admin/account/users_controller.rb

@@ -53,7 +53,7 @@ def collection
 
   def update_resource(user, attributes)
     attributes.each do |attrs|
-      user.attributes = filter_readonly_params(attrs, User)
+      user.attributes = filter_readonly_params(attrs, User).permit!
       user.role = attrs[:role] if can? :update_role, user
     end
     user.save

lib/developer_portal/app/controllers/developer_portal/base_controller.rb

@@ -27,7 +27,7 @@ def finish_signup_for_paid_plan
   end
 
   def filter_readonly_params(params, resource_class)
-    return {} unless params
+    return ActionController::Parameters.new unless params
 
     read_only_fields = FieldsDefinition.by_provider(site_account).by_target(resource_class.name).read_only.pluck(:name)
     params.except(*read_only_fields)

spec/acceptance/api/user_spec.rb

@@ -75,13 +75,9 @@
 
     let(:account_id) { buyer.id }
 
-    let (:user) { FactoryBot.build(:user, account: buyer) }
+    let(:user) { FactoryBot.build(:user, account: buyer) }
 
-    let(:resource) do
-      FieldsDefinition.create_defaults!(master)
-      provider.reload
-      user
-    end
+    let(:resource) { user }
 
     get '/admin/api/accounts/:account_id/users/:id.:format', action: :show
     delete '/admin/api/accounts/:account_id/users/:id', action: :destroy
@@ -147,12 +143,7 @@
 
     let(:user) { FactoryBot.create(:user, account: provider) }
 
-    # creating new db records for fields that are in db is pathetic as it can get
-    let(:resource) do
-      FieldsDefinition.create_defaults!(master)
-      provider.reload
-      user
-    end
+    let(:resource) { user }
 
     it { should include('id' => user.id, 'state' => user.state, 'role' => user.role.to_s) }
     it { should include('email' => user.email, 'username' => user.username) }