diff --git a/.reek.yml b/.reek.yml index aa0ba8b4ed..df0c9d3bdc 100644 --- a/.reek.yml +++ b/.reek.yml @@ -42,3 +42,7 @@ directories: "test": InstanceVariableAssumption: enabled: false + DuplicateMethodCall: + enabled: false + UncommunicativeVariableName: + enabled: false diff --git a/.rubocop.yml b/.rubocop.yml index 1cc9becde0..e72507bc79 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -8,6 +8,13 @@ Rails: Rails/RequestReferer: EnforcedStyle: referrer +Rails/ActionOrder: + Enabled: false + +Rails/SkipsModelValidations: + Exclude: + - 'test/**/*_test.rb' + Lint: Enabled: true @@ -20,6 +27,7 @@ Metrics: Metrics/BlockLength: Exclude: - 'test/**/*_test.rb' + - 'spec/**/*_spec.rb' Lint/AssignmentInCondition: AllowSafeAssignment: true @@ -75,12 +83,16 @@ Layout/LineLength: Metrics/AbcSize: Max: 20 + Exclude: + - 'test/**/*_test.rb' Metrics/MethodLength: Max: 20 Metrics/ClassLength: Max: 200 + Exclude: + - 'test/**/*_test.rb' AllCops: NewCops: enable diff --git a/app/controllers/admin/api/accounts_controller.rb b/app/controllers/admin/api/accounts_controller.rb index 6393002400..64963a71f1 100644 --- a/app/controllers/admin/api/accounts_controller.rb +++ b/app/controllers/admin/api/accounts_controller.rb @@ -43,9 +43,8 @@ def update preload_to_present(buyer_account) - buyer_account.vat_rate = params[:vat_rate].to_f if params[:vat_rate] buyer_account.settings.attributes = billing_params - buyer_account.update_with_flattened_attributes(flat_params) + buyer_account.update(account_params) respond_with(buyer_account) end @@ -134,7 +133,7 @@ def account_plan end def billing_params - @billing_params ||= params.permit(:monthly_billing_enabled, :monthly_charging_enabled) + params.permit(:monthly_billing_enabled, :monthly_charging_enabled) end private @@ -156,7 +155,20 @@ def find_buyer_account end end - def flat_params - super.except(:vat_rate, :id) + def account_params + defined_fields_names = buyer_account.defined_fields_names + allowed_attrs = defined_fields_names + %w[name] + nested_params = { + extra_fields: buyer_account.defined_extra_fields_names, + annotations: {} + } + + if defined_fields_names.include?('billing_address') + allowed_attrs += %w[billing_address_name billing_address_address1 billing_address_address2 billing_address_city + billing_address_country billing_address_state billing_address_zip billing_address_phone] + nested_params[:billing_address] = %i[name address1 address2 city country state zip phone] + end + + params.permit(*allowed_attrs, **nested_params) end end diff --git a/app/controllers/admin/api/buyers_application_plans_controller.rb b/app/controllers/admin/api/buyers_application_plans_controller.rb index 6b47082c20..22b8bef5eb 100644 --- a/app/controllers/admin/api/buyers_application_plans_controller.rb +++ b/app/controllers/admin/api/buyers_application_plans_controller.rb @@ -25,31 +25,11 @@ def index respond_with(bought_application_plans) end - # swagger - ## e = sapi.apis.add - ## e.path = "/admin/api/accounts/{account_id}/application_plans/{id}/buy.xml" - ## e.responseClass = "application" - ## @desc = "Creates an application for a partner on a given application plan (deprecated)." - ## e.description = @desc - # - ## op = e.operations.add - ## op.deprecated = true - ## op.nickname = "buyer_app_plan_buy" - ## op.httpMethod = "POST" - ## op.summary = @desc - # - ## @id = { :name => "id", :description => "ID of the application plan.", :dataType => "int", :allowMultiple => false, :required => true, :paramType => "path" } - # - ## op.parameters.add @access_token - ## op.parameters.add @account_id - ## op.parameters.add @id - ## op.parameters.add :name => "name", :description => "Name of the application to be created.", :dataType => "string", :allowMultiple => false, :required => true, :paramType => "query" - ## op.parameters.add :name => "description", :description => "Description of the application to be created.", :dataType => "string", :allowMultiple => false, :required => true, :paramType => "query" - # - + # Creates an application for a partner on a given application plan (deprecated). + # POST /admin/api/accounts/{account_id}/application_plans/{id}/buy.xml #TODO: deprecate this, beware there are clients using it def buy - application = buyer.buy(application_plan, application_plan_params) + application = buyer.buy(application_plan, contract_params) respond_with(application, serialize: application_plan) end @@ -63,8 +43,9 @@ def application_plan @application_plan ||= accessible_application_plans.stock.find(params[:id]) end - def application_plan_params - attributes = current_account.fields.for(Cinstance) - params.slice(*attributes) + def contract_params + allowed_attrs = current_account.defined_fields_names_for(Cinstance) + + %w[user_key application_id redirect_url first_traffic_at first_daily_traffic_at] + params.permit(*allowed_attrs) end end diff --git a/app/controllers/admin/api/buyers_applications_controller.rb b/app/controllers/admin/api/buyers_applications_controller.rb index 58d4b5eb39..f34c8b062b 100644 --- a/app/controllers/admin/api/buyers_applications_controller.rb +++ b/app/controllers/admin/api/buyers_applications_controller.rb @@ -2,6 +2,10 @@ class Admin::Api::BuyersApplicationsController < Admin::Api::BuyersBaseControlle representer Cinstance before_action :find_or_create_service_contract, :only => :create + before_action :find_application, except: %i[create index find] + before_action :build_new_application, only: %i[create] + + attr_reader :application # Application List # GET /admin/api/accounts/{account_id}/applications.xml @@ -12,10 +16,7 @@ def index # Application Create # POST /admin/api/accounts/{account_id}/applications.xml def create - application = applications.new(user_account: buyer, plan: application_plan, create_origin: "api") - application.unflattened_attributes = application_params - application.user_key = params[:user_key] if params[:user_key] - application.application_id = params[:application_id] if params[:application_id] + application.assign_attributes(application_params) Array(params[:application_key]).each do |key| application.application_keys.build(value: key) @@ -35,10 +36,7 @@ def show # Application Update # PUT /admin/api/accounts/{account_id}/applications/{id}.xml def update - application.unflattened_attributes = flat_params - application.user_key = params[:user_key] if params[:user_key] - - application.save + application.update(application_update_params) respond_with application end @@ -115,8 +113,12 @@ def applications @applications ||= accessible_bought_cinstances.includes(:user_account, :plan, :service) end - def application - @application ||= applications.find params[:id] + def build_new_application + @application = applications.new(user_account: buyer, plan: application_plan, create_origin: "api") + end + + def find_application + @application = applications.find params[:id] end def application_plan @@ -124,15 +126,14 @@ def application_plan end def application_params - flat_params.slice(*application_attributes) - end - - def application_attributes - current_account.fields.for(Cinstance) + %w|user_key application_id| + @application_params ||= begin + allowed_attrs = application.defined_fields_names + %w[user_key application_id redirect_url first_traffic_at first_daily_traffic_at] + params.permit(*allowed_attrs) + end end - def flat_params - super.except(:account_id) + def application_update_params + application_params.except(:application_id) end def find_or_create_service_contract diff --git a/app/controllers/admin/api/buyers_users_controller.rb b/app/controllers/admin/api/buyers_users_controller.rb index 208841aa97..614334c91b 100644 --- a/app/controllers/admin/api/buyers_users_controller.rb +++ b/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,14 +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.signup_type = :api - - user.save + user.update(user_params.merge(signup_type: :api)) respond_with(user) end @@ -37,7 +37,7 @@ def show def update authorize! :update, user - user.update_with_flattened_attributes(flat_params) + user.update(user_params) respond_with(user) end @@ -108,10 +108,6 @@ 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) @@ -119,8 +115,17 @@ def users 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 + params.permit(*user.defined_fields_names, :password, :password_confirmation) + end end diff --git a/app/controllers/admin/api/providers_controller.rb b/app/controllers/admin/api/providers_controller.rb index 3ee5b88ef7..8fcc92e48b 100644 --- a/app/controllers/admin/api/providers_controller.rb +++ b/app/controllers/admin/api/providers_controller.rb @@ -13,7 +13,7 @@ def show # Provider Account Update # PUT /admin/api/provider.xml def update - current_account.update(provider_params, without_protection: true) + current_account.update(provider_params) respond_with current_account end diff --git a/app/controllers/admin/api/signups_controller.rb b/app/controllers/admin/api/signups_controller.rb index bbb9f70861..1832846582 100644 --- a/app/controllers/admin/api/signups_controller.rb +++ b/app/controllers/admin/api/signups_controller.rb @@ -7,7 +7,9 @@ class Admin::Api::SignupsController < Admin::Api::BaseController def create authorize!(:create, Account) if current_user - @signup_result = Signup::DeveloperAccountManager.new(current_account).create(signup_params) + @signup_result = account_manager.create(plans: plans, defaults: defaults, + account_params: account_params, + user_params: user_params.merge(signup_type: :minimal)) check_creation_errors respond_with(@signup_result.account, user_options: { with_apps: true }) @@ -15,8 +17,17 @@ def create private + def account_manager + @account_manager ||= Signup::DeveloperAccountManager.new(current_account) + end + def user_params - flat_params.merge({signup_type: :minimal}) + params.permit(*account_manager.user.defined_fields_names, :password) + end + + def account_params + allowed_attrs = account_manager.account.defined_fields_names - %w[billing_address] + params.permit(*allowed_attrs, annotations: {}) end def check_creation_errors @@ -30,10 +41,6 @@ def check_creation_errors end end - def signup_params - Signup::SignupParams.new(plans: plans, user_attributes: user_params, account_attributes: flat_params, defaults: defaults) - end - def defaults { ApplicationPlan => { :name => 'API signup', :description => 'API signup', :create_origin => 'api' } } end diff --git a/app/controllers/admin/api/users_controller.rb b/app/controllers/admin/api/users_controller.rb index 0689d10222..9fb589a54e 100644 --- a/app/controllers/admin/api/users_controller.rb +++ b/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,14 +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.signup_type = :api - - user.save + user.update(user_params.merge(signup_type: :api)) respond_with(user) end @@ -39,7 +38,7 @@ def show def update authorize! :update, user - user.update_with_flattened_attributes(flat_params, as: current_user.try(:role)) + user.update(user_params) respond_with(user) end @@ -110,10 +109,6 @@ 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) @@ -121,8 +116,12 @@ def users 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 @@ -131,7 +130,10 @@ def can_create private - def flat_params - super.except(:id) + def user_params + permission_attrs = [:member_permission_service_ids, { member_permission_service_ids: [], member_permission_ids: [] }] + allowed_attrs = user.defined_fields_names + %w[password password_confirmation cas_identifier] + allowed_attrs += permission_attrs if provider_key.present? || current_user.admin? + params.permit(*allowed_attrs) end end diff --git a/app/controllers/api/applications_controller.rb b/app/controllers/api/applications_controller.rb index fe84ab7dd9..2ae79ea039 100644 --- a/app/controllers/api/applications_controller.rb +++ b/app/controllers/api/applications_controller.rb @@ -20,6 +20,7 @@ class Api::ApplicationsController < FrontendController def new; end def create + @cinstance.assign_attributes(application_params) if @cinstance.save redirect_to provider_admin_application_path(@cinstance), success: t('.success') else diff --git a/app/controllers/buyers/accounts_controller.rb b/app/controllers/buyers/accounts_controller.rb index 6d18bca33d..e8a1fb5661 100644 --- a/app/controllers/buyers/accounts_controller.rb +++ b/app/controllers/buyers/accounts_controller.rb @@ -9,6 +9,7 @@ class Buyers::AccountsController < Buyers::BaseController before_action :set_plans, :only => %i[new create] before_action :find_account, except: %i[index new create] + before_action :init_signup_account_manager, only: %i[create] activate_menu :buyers, :accounts, :listing @@ -30,10 +31,7 @@ def new end def update - vat = account_params[:vat_rate] - account.vat_rate = vat if vat # vat_rate is protected attribute - - if account.update(account_params.except(:vat_rate)) + if account.update(account_params) redirect_to admin_buyers_account_path(account), success: t('.success') else render :edit @@ -41,14 +39,11 @@ def update end def create - signup_result = Signup::DeveloperAccountManager.new(current_account).create(signup_params) + signup_result = @account_manager.create(**signup_params) @buyer = signup_result.account if signup_result.persisted? - unless signup_result.user_active? - signup_result.account_approve! - signup_result.user_activate! - end + approve_account_user(signup_result) redirect_to admin_buyers_account_path(@buyer), success: t('.success') else @user = signup_result.user @@ -84,7 +79,7 @@ def show protected - attr_reader :account + attr_reader :account, :user def find_account with_deleted = %w[show resume].include?(action_name) @@ -105,23 +100,27 @@ def change_state redirect_to admin_buyers_account_path(account) end - def signup_params - Signup::SignupParams.new(plans: [], user_attributes: user_params.merge(signup_type: :created_by_provider), account_attributes: account_params, validate_fields: false) - end - - # TODO: using `permit` later def account_params - @account_params ||= params.require(:account).except(:user) + allowed_attrs = account.defined_builtin_fields_names - %w[billing_address country] + %w[country_id] + params.require(:account).permit(*allowed_attrs, extra_fields: account.defined_extra_fields_names) end def user_params - params.require(:account).fetch(:user, {}) + allowed_attrs = user.defined_builtin_fields_names + %w[password] + params.require(:account).fetch(:user, {}).permit(*allowed_attrs, extra_fields: user.defined_extra_fields_names) + end + + def signup_params + { + plans: [], + validate_fields: false, + account_params: account_params, + user_params: user_params.merge(signup_type: :created_by_provider) + } end def set_plans - unless current_account.create_buyer_possible? - redirect_to admin_buyers_account_plans_path, danger: t('buyers.accounts.set_plans_error') - end + redirect_to admin_buyers_account_plans_path, danger: t('buyers.accounts.set_plans_error') unless current_account.create_buyer_possible? @plans = [] # this is here only to make new_signups/form happy end @@ -131,4 +130,17 @@ def presenter user: current_user, params: params) end + + def init_signup_account_manager + @account_manager = Signup::DeveloperAccountManager.new(current_account) + @account = @account_manager.account + @user = @account_manager.user + end + + def approve_account_user(signup_result) + return if signup_result.user_active? + + signup_result.account_approve! + signup_result.user_activate! + end end diff --git a/app/controllers/buyers/applications_controller.rb b/app/controllers/buyers/applications_controller.rb index d74b5a40b7..cdcbc17c69 100644 --- a/app/controllers/buyers/applications_controller.rb +++ b/app/controllers/buyers/applications_controller.rb @@ -23,6 +23,8 @@ def new end def create + @cinstance.assign_attributes(application_params) + if @cinstance.save redirect_to provider_admin_application_path(@cinstance), success: t('.success') else diff --git a/app/controllers/buyers/groups_controller.rb b/app/controllers/buyers/groups_controller.rb index 0ced9bed07..1255729c91 100644 --- a/app/controllers/buyers/groups_controller.rb +++ b/app/controllers/buyers/groups_controller.rb @@ -10,7 +10,7 @@ class Buyers::GroupsController < Buyers::BaseController def show; end def update - flash[:success] = t('.success') if @account.update(params[:account]) + flash[:success] = t('.success') if @account.update(groups_params) redirect_to action: :show, id: @account.id end @@ -28,4 +28,8 @@ def authorize_groups def find_account @account = current_account.buyer_accounts.find(params[:account_id]) end + + def groups_params + params.require(:account).permit(group_ids: []) + end end diff --git a/app/controllers/buyers/users_controller.rb b/app/controllers/buyers/users_controller.rb index 172b97b3bf..8a93daf86e 100644 --- a/app/controllers/buyers/users_controller.rb +++ b/app/controllers/buyers/users_controller.rb @@ -22,10 +22,7 @@ def show def edit; end 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.assign_attributes(permitted_user_params) user.role = user_params.fetch(:role, user.role) if can?(:update_role, user) if user.save @@ -86,10 +83,13 @@ 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 + user_params.permit(*user.defined_builtin_fields_names, :password, :password_confirmation, + extra_fields: user.defined_extra_fields_names) end def redirect_back_or_show_detail(**opts) diff --git a/app/controllers/master/api/providers_controller.rb b/app/controllers/master/api/providers_controller.rb index cad60a29a3..7d9abbbe24 100644 --- a/app/controllers/master/api/providers_controller.rb +++ b/app/controllers/master/api/providers_controller.rb @@ -8,7 +8,9 @@ class Master::Api::ProvidersController < Master::Api::BaseController authenticate_access_token plain: 'unauthorized' self.access_token_scopes = :account_management + before_action :ensure_master_with_plans, only: :create + before_action :init_signup_account_manager, only: :create before_action :find_plan_for_upgrade, only: :plan_upgrade @@ -35,7 +37,7 @@ def change_partner # Tenant Create # POST /master/api/providers.xml def create - signup_result = Signup::ProviderAccountManager.new(current_account).create(create_params, ::Signup::ResultWithAccessToken) + signup_result = @account_manager.create(**create_params, signup_result_class: ::Signup::ResultWithAccessToken) if signup_result.persisted? signup_result.account_approve! unless signup_result.account_approval_required? @@ -48,9 +50,7 @@ def create # Tenant Update # PUT /master/api/providers/{id}.xml def update - provider_account.assign_attributes(update_params, without_protection: true) - provider_account.assign_unflattened_attributes(params.require(:account)) - provider_account.save + provider_account.update(update_account_params) respond_with signup_result_with_nil_token end @@ -91,6 +91,12 @@ def provider_account @provider_account ||= current_account.providers.without_deleted(!action_includes_deleted_providers?).find(params[:id]) end + def init_signup_account_manager + @account_manager = Signup::ProviderAccountManager.new(current_account) + @provider_account = @account_manager.account + @user = @account_manager.user + end + def signup_result_with_nil_token signup_result = Signup::ResultWithAccessToken.new(account: provider_account, user: provider_account.admin_users.first) @@ -117,14 +123,26 @@ def find_plan_for_upgrade render_error "Plan with ID #{plan_id.presence} not found", status: :not_found end - def update_params - permitted_params = provider_account.scheduled_for_deletion? ? %i[state_event] : UPDATE_PARAMS - params.require(:account).permit(permitted_params) - end - def create_params defaults = { ApplicationPlan => { :name => 'API signup', :description => 'API signup', :create_origin => 'api' } } - Signup::SignupParams.new(plans: plans, user_attributes: flat_params.merge(signup_type: :created_by_provider), account_attributes: flat_params, defaults: defaults) + { plans: plans, user_params: user_params.merge(signup_type: :created_by_provider), account_params: create_account_params, defaults: defaults } + end + + def user_params + params.permit(*@user.defined_fields_names, :password) + end + + def permitted_account_attrs + @provider_account.defined_fields_names | UPDATE_PARAMS + end + + def create_account_params + params.permit(*permitted_account_attrs) + end + + def update_account_params + allowed_attrs = provider_account.scheduled_for_deletion? ? %i[state_event] : permitted_account_attrs + params.require(:account).permit(*allowed_attrs) end def plans diff --git a/app/controllers/partners/providers_controller.rb b/app/controllers/partners/providers_controller.rb index 74b3bbad5a..f22a2d668f 100644 --- a/app/controllers/partners/providers_controller.rb +++ b/app/controllers/partners/providers_controller.rb @@ -1,27 +1,20 @@ +# frozen_string_literal: true + class Partners::ProvidersController < Partners::BaseController - before_action :find_account, only: [:destroy, :update] + before_action :find_account, only: %i[destroy update] def create - partner_system_name = partner.system_name - - signup_result = Signup::ProviderAccountManager.new(Account.master).create(signup_params) do |result| - account = result.account - account.signup_mode! - account.subdomain = "#{permitted_params[:subdomain]}-#{partner_system_name}" - account.generate_domains! - account.partner = partner - account.extra_fields['partner'] = partner_system_name - account.settings.monthly_charging_enabled = false + account_manager = Signup::ProviderAccountManager.new(Account.master) + signup_result = account_manager.create(**signup_params) do |result| + assign_account_attributes(result) end @account = signup_result.account @user = signup_result.user if signup_result.persisted? signup_result.user_activate! - tracking = ThreeScale::Analytics.user_tracking(@user) - tracking.identify({}) - tracking.track('Signup', {}) + track_user render json: {id: @account.id, provider_key: @account.api_key, end_point: @account.external_admin_domain, success: true} else render json: {errors: {user: @user.errors, account: @account.errors}, success: false}, status: :unprocessable_entity @@ -41,7 +34,9 @@ def destroy private def signup_params - Signup::SignupParams.new(plans: [selected_plan], user_attributes: user_params, account_attributes: account_params, validate_fields: true) + { + plans: [selected_plan], user_params: user_params, account_params: account_params, validate_fields: true + } end def account_params @@ -86,4 +81,21 @@ def selected_plan def permitted_params params.permit(%i[application_plan open_id last_name first_name email password id subdomain org_name]) end + + def assign_account_attributes(result) + partner_system_name = partner.system_name + account = result.account + account.signup_mode! + account.subdomain = "#{permitted_params[:subdomain]}-#{partner_system_name}" + account.generate_domains! + account.partner = partner + account.extra_fields['partner'] = partner_system_name + account.settings.monthly_charging_enabled = false + end + + def track_user + tracking = ThreeScale::Analytics.user_tracking(@user) + tracking.identify({}) + tracking.track('Signup', {}) + end end diff --git a/app/controllers/partners/users_controller.rb b/app/controllers/partners/users_controller.rb index c140736e89..5660981d67 100644 --- a/app/controllers/partners/users_controller.rb +++ b/app/controllers/partners/users_controller.rb @@ -20,13 +20,7 @@ def destroy end def create - @user = @account.users.build - @user.email = params[:email] - @user.password = params[:password].presence - @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 = @account.users.build(user_params) @user.signup_type = :partner @user.role = :admin @user.activate! @@ -42,4 +36,9 @@ def create def find_account @account = @partner.providers.find(params[:provider_id]) end + + def user_params + allowed_attrs = %i[email password first_name last_name open_id username] + params.permit(*allowed_attrs) + end end diff --git a/app/controllers/provider/admin/account/users_controller.rb b/app/controllers/provider/admin/account/users_controller.rb index 702515ae28..d9e19ce54f 100644 --- a/app/controllers/provider/admin/account/users_controller.rb +++ b/app/controllers/provider/admin/account/users_controller.rb @@ -25,7 +25,6 @@ def update @user.validate_fields! @user.assign_attributes(user_params) - @user.role = user_params.fetch(:role, @user.role) if @user.save redirect_to provider_admin_account_users_path, success: t('.success') @@ -57,7 +56,7 @@ def users end def user_params - allowed_attrs = @user.defined_builtin_fields.map(&:name) + @user.special_fields + allowed_attrs = @user.defined_builtin_fields_names + %w[password password_confirmation] if can?(:update_role, @user) allowed_attrs += [:role, { member_permission_ids: [] }] diff --git a/app/controllers/provider/admin/accounts_controller.rb b/app/controllers/provider/admin/accounts_controller.rb index bcacf5fba5..c7fc28782c 100644 --- a/app/controllers/provider/admin/accounts_controller.rb +++ b/app/controllers/provider/admin/accounts_controller.rb @@ -8,6 +8,7 @@ class Provider::Admin::AccountsController < Provider::Admin::Account::BaseContro before_action :deny_unless_can_update, :only => [:update, :edit] before_action :check_provider_signup_possible, :only => %i[new create] before_action :disable_client_cache + before_action :init_signup_account_manager, only: %i[create] def new activate_menu :buyers, :accounts, :listing @@ -17,7 +18,9 @@ def new end def create - signup_result = Signup::ProviderAccountManager.new(current_account).create(signup_params) + signup_result = @account_manager.create(plans: [], validate_fields: false, + account_params: account_params, + user_params: user_params.merge(signup_type: :created_by_provider)) @provider = signup_result.account @user = signup_result.user @@ -42,7 +45,7 @@ def update # rubocop:disable Metrics/AbcSize check_require_billing_information respond_to do |format| # FIXME: Always false if account does not have billing address. Billing address is set in the next page. - if @account.update(account_params) + if @account.update(update_account_params) format.html { redirect_to_success } format.js do flash.now[:success] = t('.success') @@ -58,18 +61,27 @@ def update # rubocop:disable Metrics/AbcSize private - def signup_params - params_required = params.require(:account) - account_params = params_required.except(:user) - user_params = params_required.fetch(:user, {}).merge(signup_type: :created_by_provider) - Signup::SignupParams.new(plans: [], user_attributes: user_params, account_attributes: account_params, validate_fields: false) + def init_signup_account_manager + @account_manager = Signup::ProviderAccountManager.new(current_account) + @account = @account_manager.account + @user = @account_manager.user + end + + def account_params + allowed_attrs = @account.defined_builtin_fields_names - %i[billing_address country] + %i[country_id] + params.require(:account).permit(*allowed_attrs, extra_fields: @account.defined_extra_fields_names) + end + + def user_params + allowed_attrs = @user.defined_builtin_fields_names + %w[password password_confirmation] + params.require(:account).fetch(:user, {}).permit(*allowed_attrs, extra_fields: @user.defined_extra_fields_names) end def check_provider_signup_possible redirect_to admin_buyers_accounts_path, info: t('.not_possible') unless current_account.signup_provider_possible? end - def account_params + def update_account_params allowed_fields = current_account.editable_defined_fields_for(current_user).map do |field| field_name = field.name field_name == 'country' ? 'country_id' : field_name diff --git a/app/controllers/provider/admin/applications_controller.rb b/app/controllers/provider/admin/applications_controller.rb index f5a5a88b06..68a7316f7b 100644 --- a/app/controllers/provider/admin/applications_controller.rb +++ b/app/controllers/provider/admin/applications_controller.rb @@ -29,6 +29,8 @@ def show def new; end def create + @cinstance.assign_attributes(application_params) + if @cinstance.save redirect_to provider_admin_application_path(@cinstance), success: t('.success') else @@ -43,7 +45,7 @@ def edit; end def update # TODO: this is not needed if this controller is used only by providers @cinstance.validate_human_edition! - @cinstance.attributes = params[:cinstance] + @cinstance.assign_attributes(application_params) respond_to do |format| json = @cinstance.to_json(only: %i[id name], methods: %i[errors]) diff --git a/app/controllers/provider/admin/user/personal_details_controller.rb b/app/controllers/provider/admin/user/personal_details_controller.rb index c3b8080db2..3b5180c9a5 100644 --- a/app/controllers/provider/admin/user/personal_details_controller.rb +++ b/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,12 @@ def redirect_path end def user_params - params.require(:user) + @user_params ||= params.require(:user) + end + + def permitted_user_params + allowed_attrs = current_user.defined_builtin_fields_names + %w[password] + user_params.permit(*allowed_attrs, extra_fields: current_user.defined_extra_fields_names) end def current_password_verification diff --git a/app/controllers/provider/invitee_signups_controller.rb b/app/controllers/provider/invitee_signups_controller.rb index 2e81400e58..3dd389a9a5 100644 --- a/app/controllers/provider/invitee_signups_controller.rb +++ b/app/controllers/provider/invitee_signups_controller.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + class Provider::InviteeSignupsController < FrontendController skip_before_action :login_required @@ -5,15 +7,15 @@ class Provider::InviteeSignupsController < FrontendController before_action :ask_for_upgrade before_action :find_invitation - before_action :build_user + before_action :build_new_user before_action :instantiate_sessions_presenter layout 'provider/login' - def show - end + def show; end def create + @user.assign_attributes(user_params) @user.admin_sections = domain_account.provider_can_use?(:service_permissions) ? [] : ['monitoring'] if can_create? && @user.save @@ -23,11 +25,7 @@ def create redirect_to provider_login_path, success: t('.success') else - errors = @user.errors.full_messages.reduce do |result, error| - "#{result}\n#{error}" - end - - flash.now[:danger] = t('.error', errors: errors) + flash.now[:danger] = t('.error', errors: user_errors) render 'show' end end @@ -52,12 +50,12 @@ def can_create? account.provider_constraints.can_create_user? end - def build_user - @user = @invitation.make_user(params[:user] || {}) + def build_new_user + @user = @invitation.make_user + end - # This is just a sanity guard added when splitting invitation - # controllers. Remove when SURE. - raise 'Developer invitation used and worked on provider side!' unless @user.account.provider? + def user_params + params.require(:user).permit(@user.defined_fields_names, :password, :password_confirmation) end def invitation_token @@ -67,4 +65,12 @@ def invitation_token def instantiate_sessions_presenter @presenter = Provider::SessionsPresenter.new(domain_account) end + + def user_errors + return "" unless @user + + @user.errors.full_messages.reduce do |result, error| + "#{result}\n#{error}" + end + end end diff --git a/app/controllers/provider/signups_controller.rb b/app/controllers/provider/signups_controller.rb index d305e2f8a6..cb32ae87e9 100644 --- a/app/controllers/provider/signups_controller.rb +++ b/app/controllers/provider/signups_controller.rb @@ -7,40 +7,34 @@ class Provider::SignupsController < Provider::BaseController before_action :ensure_signup_possible skip_before_action :login_required - skip_before_action :enable_analytics, only: :test before_action :cors public :cors before_action :set_analytics_page before_action :handle_cache_response, only: :show + before_action :init_account_manager, only: :create layout 'provider/iframe' self.layoutless_rendering = false - def show # original iframe form + def show @provider = master.providers.build @user = @provider.users.build_with_fields - @plan = plan - @signup_origin = default_params[:origin] || default_params[:signup_origin] - @fields = Fields::SignupForm.new(@provider, @user, default_params[:fields]) + @signup_origin = params[:origin] || params[:signup_origin] + @fields = Fields::SignupForm.new(@provider, @user, params[:fields]) end def create - @plan = plan - provider_account_manager = Signup::ProviderAccountManager.new(master) - signup_result = provider_account_manager.create(signup_params, &method(:build_signup_result_custom_fields)) - @fields = Fields::SignupForm.new(@provider, @user, default_params[:fields]) + signup_result = @account_manager.create(**signup_params) { |result| build_signup_result_custom_fields(result) } + @fields = Fields::SignupForm.new(@provider, @user, params[:fields]) return render :show unless signup_result.persisted? session[:success_data] = { first_name: @user.first_name, email: @user.email } - tracking = ThreeScale::Analytics.user_tracking(@user) - traits = tracking.identify(analytics_session.traits) - tracking.track('Signup', signup_options) - analytics_session.delayed.identify(@user.id, traits) + track_user if request.xhr? render json: { redirect: success_provider_signup_url, success: true } @@ -52,20 +46,27 @@ def create protected def signup_params - Signup::SignupParams.new(plans: [plan], user_attributes: user_params, account_attributes: account_params, validate_fields: true) + { + plans: [plan], + validate_fields: true, + account_params: account_params.merge(sample_data: true), + user_params: user_params.merge(signup_type: :new_signup, username: :admin) + } end def account_params - params.require(:account).except(:user).merge(sample_data: true) + account = @account_manager.account + allowed_attrs = account.defined_builtin_fields_names + %w[name subdomain self_subdomain] + params.require(:account).permit(*allowed_attrs, extra_fields: account.defined_extra_fields_names) end def user_params - params.require(:account).fetch(:user, {}).merge(signup_type: :new_signup, username: :admin) + params.require(:account).fetch(:user, {}).permit(:first_name, :last_name, :email, :password) end def handle_cache_response expires_in 1.hour, public: true - fresh_when etag: default_params, last_modified: System::Application.config.boot_time + fresh_when etag: params.permit!.to_h, last_modified: System::Application.config.boot_time end def set_analytics_page @@ -76,10 +77,6 @@ def current_user nil # no one can't be logged in! end - def signup_options - { mkt_cookie: cookies[:_mkto_trk], analytics: analytics_session.traits } - end - def master site_account end @@ -101,12 +98,21 @@ def build_signup_result_custom_fields(result) end def plan - plan_ids = default_params[:plan_id].presence - master.accessible_services.default.application_plans.published.find(plan_ids) if plan_ids + @plan ||= begin + plan_ids = params[:plan_id].presence + master.accessible_services.default.application_plans.published.find(plan_ids) if plan_ids + end end - def default_params - # permit all since it can have multiple different and dynamic data - params.permit!.to_h + def init_account_manager + @account_manager = Signup::ProviderAccountManager.new(master) + end + + def track_user + tracking = ThreeScale::Analytics.user_tracking(@user) + traits = tracking.identify(analytics_session.traits) + signup_options = { mkt_cookie: cookies[:_mkto_trk], analytics: analytics_session.traits } + tracking.track('Signup', signup_options) + analytics_session.delayed.identify(@user.id, traits) end end diff --git a/app/controllers/sites/dns_controller.rb b/app/controllers/sites/dns_controller.rb index 8229c2e1d6..8883b64d91 100644 --- a/app/controllers/sites/dns_controller.rb +++ b/app/controllers/sites/dns_controller.rb @@ -8,7 +8,7 @@ def show end def update - if @account.update(site_params, without_protection: true) + if @account.update(site_params) redirect_to admin_site_dns_url, success: t('.success') else flash.now[:danger] = @account.errors.full_messages.join(' ') diff --git a/app/lib/applications_controller_methods.rb b/app/lib/applications_controller_methods.rb index 04ba83af1e..54da7b3ee0 100644 --- a/app/lib/applications_controller_methods.rb +++ b/app/lib/applications_controller_methods.rb @@ -23,7 +23,7 @@ def index # TODO: this should be done by buy! method def initialize_cinstance - @cinstance = current_account.provider_builds_application_for(@account, @application_plan, params[:cinstance], @service_plan) + @cinstance = current_account.provider_builds_application_for(@account, @application_plan, @service_plan) @cinstance.validate_human_edition! end @@ -74,15 +74,24 @@ def find_application_plan def find_service_plan service_plans = @service.service_plans - @service_plan = if (service_plan_id = params[:cinstance].delete(:service_plan_id)) + @service_plan = if (service_plan_id = cinstance_params[:service_plan_id]) service_plans.find(service_plan_id) else @service.default_service_plan || service_plans.first end end + def cinstance_params + @cinstance_params ||= params.require(:cinstance) + end + + def application_params + allowed_attrs = @cinstance.defined_builtin_fields_names + cinstance_params.permit(*allowed_attrs, extra_fields: @cinstance.defined_extra_fields_names) + end + def plan_id - @plan_id ||= params.require(:cinstance).permit(:plan_id).tap { |plan_params| plan_params.require(:plan_id) }[:plan_id] + @plan_id ||= cinstance_params.require(:plan_id) end def accessible_services diff --git a/app/lib/authentication/by_password.rb b/app/lib/authentication/by_password.rb index 1abbdef91f..b5485eff5d 100644 --- a/app/lib/authentication/by_password.rb +++ b/app/lib/authentication/by_password.rb @@ -20,8 +20,6 @@ module ByPassword validates :lost_password_token, :password_digest, length: { maximum: 255 } - attr_accessible :password, :password_confirmation, as: %i[default member admin] - scope :with_valid_password_token, -> { where { lost_password_token_generated_at >= 24.hours.ago } } alias_method :authenticate_without_normalization, :authenticate @@ -90,10 +88,6 @@ def can_set_password? account.password_login_allowed? && !already_using_password? end - def special_fields - %i[password password_confirmation] - end - def reset_lost_password_token self.lost_password_token = nil end diff --git a/app/lib/authentication/strategy/oauth2.rb b/app/lib/authentication/strategy/oauth2.rb index 27a58fdee1..f6489f0644 100644 --- a/app/lib/authentication/strategy/oauth2.rb +++ b/app/lib/authentication/strategy/oauth2.rb @@ -23,7 +23,8 @@ def create_account(session) signup_result = new_user_created = nil Account.transaction do - signup_result = SignupService.create(**signup_service_params(session)) + signup_service = SignupService.new(provider: site_account, plans: [], session: session) + signup_result = signup_service.create(account_params: { org_name: user_data.org_name }, user_params: user_data.to_hash) if new_user_created = signup_result.persisted? if authentication_provider.automatically_approve_accounts? && !signup_result.account_approved? signup_result.account_approve! @@ -38,11 +39,6 @@ def create_account(session) signup_result.user if new_user_created end - def signup_service_params(session) - { provider: site_account, plans: [], session: session, - account_params: { org_name: user_data.org_name }, user_params: user_data.to_hash } - end - def session @session ||= build_session end diff --git a/app/lib/backend/model_extensions/service.rb b/app/lib/backend/model_extensions/service.rb index 73547244ae..e83bf9c9c8 100644 --- a/app/lib/backend/model_extensions/service.rb +++ b/app/lib/backend/model_extensions/service.rb @@ -45,7 +45,7 @@ def delete_backend_service def make_default_backend_service ThreeScale::Core::Service.make_default(backend_id) - account.update!({default_service_id: id}, without_protection: true) + account.update!({default_service_id: id}) end end end diff --git a/app/lib/fields/extra_fields.rb b/app/lib/fields/extra_fields.rb index 332764b7e4..48069142c9 100644 --- a/app/lib/fields/extra_fields.rb +++ b/app/lib/fields/extra_fields.rb @@ -70,19 +70,6 @@ def extra_fields_to_xml(xml) end end - def update_with_flattened_attributes(flattened_attrs, options = {}) - assign_unflattened_attributes(flattened_attrs, options) - save - end - - def assign_unflattened_attributes(attributes, options = {}) - assign_attributes(nest_extra_fields(attributes), options) - end - - def unflattened_attributes=(flattened_attrs) - self.attributes = nest_extra_fields(flattened_attrs) - end - protected def encode_string_extra_field(value) @@ -110,26 +97,4 @@ def encode_extra_field(value) value end end - - def nest_extra_fields(flattened_attrs) - attrs = { } - flattened_attrs.each_pair do |key, value| - # we don't mind too much about this loose condition, that leads to push into - # attrs[:extra_fields] many undesired things, since extra_fields= method will take - # care of not allowing those to get in into the db - if extra_fields_attribute?(key) || special_field?(key) - attrs[key] = value - else - attrs[:extra_fields] ||= { } - attrs[:extra_fields][key] = encode_extra_field(value) - end - end - - attrs - end - - def extra_fields_attribute?(key) - respond_to?("#{key}=") && - !(self.class.reflect_on_aggregation(key.to_sym) || self.class.reflect_on_association(key.to_sym)) - end end diff --git a/app/lib/fields/fields.rb b/app/lib/fields/fields.rb index f244ce6fb5..790c0d1df6 100644 --- a/app/lib/fields/fields.rb +++ b/app/lib/fields/fields.rb @@ -97,14 +97,6 @@ def has_fields? true end - # these fields are here to be able to recognize fields that are not in the - # db but still can be updated and such. This is a need given the flatten - # params on user-management-api, if that gets removed this won't be needed - # e.g. password and password_confirmation in User - def special_fields - [] - end - end # ClassMethods module ActiverecordOverrides @@ -166,11 +158,6 @@ def internal_fields self.class.internal_fields end - def special_fields - self.class.special_fields - end - - # TODO: implementation of field_definitions_object, fields_definitions_source_root_object, validate_fields? # is 3scale specific and should be extracted to separate module # and here should be just non implemented methods @@ -275,6 +262,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 @@ -323,10 +318,6 @@ def internal_field?(name) field(name) && internal_fields.include?(name.to_s) end - def special_field?(key) - special_fields.include?(key.to_sym) - end - def field(name) defined_fields_hash[name.to_sym] end diff --git a/app/lib/fields/provider.rb b/app/lib/fields/provider.rb index cba41e14c9..9b00cb5fff 100644 --- a/app/lib/fields/provider.rb +++ b/app/lib/fields/provider.rb @@ -23,6 +23,10 @@ def fields_definitions end end + def defined_fields_names_for(klass) + fields_definitions.by_target(klass.name.underscore).map(&:name) + end + def fields @provider_fields ||= ProviderFields.new(self) end diff --git a/app/lib/logic/contracting.rb b/app/lib/logic/contracting.rb index ca4d640f65..07e74fcf2a 100644 --- a/app/lib/logic/contracting.rb +++ b/app/lib/logic/contracting.rb @@ -28,7 +28,7 @@ def can_create_service_contract? end module Provider - def provider_builds_application_for(buyer, application_plan, application_attrs = {}, service_plan = nil) + def provider_builds_application_for(buyer, application_plan, service_plan = nil, application_attrs: {}) service_contracted = buyer.bought_service_contracts.map(&:service).include?(application_plan.service) service_contract = unless service_contracted diff --git a/app/lib/logic/provider_signup.rb b/app/lib/logic/provider_signup.rb index a7e5e6e906..b3df81749c 100644 --- a/app/lib/logic/provider_signup.rb +++ b/app/lib/logic/provider_signup.rb @@ -71,10 +71,8 @@ def ensure_users(count) def signup_user email_part = email.split('@') password = Logic::SampleDeveloperPassword.for(@provider) - user_attributes = { email: "#{email_part[0]}+test@#{email_part[1]}", **User::JOHN_DOE_ATTRS, - password: password, password_confirmation: password, signup_type: :minimal } - signup_params = ::Signup::SignupParams.new(plans: [], user_attributes: user_attributes, account_attributes: { org_name: User::JOHN_DOE_ORG_NAME }) - ::Signup::DeveloperAccountManager.new(@provider).create(signup_params) + user_params = { email: "#{email_part[0]}+test@#{email_part[1]}", **User::JOHN_DOE_ATTRS, password: password, password_confirmation: password, signup_type: :minimal} + ::Signup::DeveloperAccountManager.new(@provider).create(plans: [], user_params:, account_params: { org_name: User::JOHN_DOE_ORG_NAME }) end def basic_features diff --git a/app/lib/signup/account_manager.rb b/app/lib/signup/account_manager.rb index fe561a7a48..808ab9122b 100644 --- a/app/lib/signup/account_manager.rb +++ b/app/lib/signup/account_manager.rb @@ -4,15 +4,19 @@ module Signup class AccountManager def initialize(manager_account) @manager_account = manager_account + @account = manager_account.buyers.new + @user = @account.users.new end - attr_reader :manager_account + attr_reader :manager_account, :account, :user - def create(signup_params, signup_result_class = ::Signup::Result) + def create(user_params: {}, account_params: {}, plans: [], defaults: {}, validate_fields: true, signup_result_class: ::Signup::Result) transaction do - signup_result = build_signup_result(signup_params, signup_result_class) + assign_attributes_for_account(account_params, validate_fields) + assign_attributes_for_user(user_params, validate_fields) + signup_result = build_signup_result(signup_result_class) yield(signup_result) if block_given? - save_result_with_plans(signup_result, signup_params) if signup_result.valid? + save_result_with_plans(signup_result, plans, defaults) if signup_result.valid? signup_result end end @@ -24,12 +28,13 @@ def create(signup_params, signup_result_class = ::Signup::Result) class_attribute :account_builder, default: proc {} # This method smells of :reek:TooManyStatements and :reek:DuplicateMethodCall - def save_result_with_plans(result, params) - plans = plans_with_defaults(result, params.plans) - return if plans.errors.any? + def save_result_with_plans(result, plans, defaults) + signup_plans = plans_with_defaults(result, plans) + return if signup_plans.errors.any? + transaction do begin - persist!(result, plans, params.defaults) + persist!(result, signup_plans, defaults) publish_related_event(result) rescue ActiveRecord::RecordInvalid raise ActiveRecord::Rollback @@ -44,21 +49,21 @@ def persist!(*) raise NotImplementedError, 'persist! should be implemented in subclasses' end - def build_signup_result(signup_params, signup_result_class) - account = build_account(signup_params) - signup_result_class.new(user: build_user(signup_params, account), account: account) + def build_signup_result(signup_result_class) + signup_result_class.new(user: user, account: account) end - def build_user(signup_params, account) - user = signup_params.build_user_with_attributes_for_account(account) - user.role = :admin - user + def assign_attributes_for_account(account_params, validate_fields) + account.validate_fields! if validate_fields + account_params.delete(:name) if account_params[:org_name].present? + account.assign_attributes(account_params) + account_builder.call(account) end - def build_account(signup_params) - account = signup_params.build_account_with_attributes_for_provider_account(manager_account) - account_builder.call(account) - account + def assign_attributes_for_user(user_params, validate_fields) + user.validate_fields! if validate_fields + user.assign_attributes(user_params) + user.role = :admin end def create_contract_plans_for_account!(account, plans, defaults) diff --git a/app/lib/signup/signup_params.rb b/app/lib/signup/signup_params.rb deleted file mode 100644 index 1fb6ef7bb7..0000000000 --- a/app/lib/signup/signup_params.rb +++ /dev/null @@ -1,48 +0,0 @@ -# frozen_string_literal: true - -module Signup - class SignupParams - def initialize(user_attributes: {}, account_attributes: {}, plans: [], defaults: {}, validate_fields: true) - @attributes = {user: user_attributes, account: prepare_account_attributes(account_attributes)} - @plans = plans - @defaults = defaults - @validate_fields = validate_fields - end - attr_reader :plans, :defaults - - def build_user_with_attributes_for_account(account) - user = account.users.new - user.validate_fields! if validate_fields - user.unflattened_attributes = user_attributes.except(:signup_type) - user.signup_type = user_attributes[:signup_type] - user - end - - def build_account_with_attributes_for_provider_account(provider_account) - account = provider_account.buyers.new - account.validate_fields! if validate_fields - account.unflattened_attributes = account_attributes - vat_rate = account_attributes[:vat_rate] - account.vat_rate = vat_rate.to_f if vat_rate - account - end - - private - - attr_reader :attributes, :validate_fields - - def account_attributes - attributes[:account] - end - - def user_attributes - attributes[:user] - end - - def prepare_account_attributes(attributes) - attributes ||= {} - attributes.delete('name') if attributes['org_name'].present? - attributes - end - end -end diff --git a/app/models/account.rb b/app/models/account.rb index 3e05ff3ab3..22e9e12cd6 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -107,11 +107,6 @@ class Account < ApplicationRecord before_validation(on: :create, if: :provider?) { generate_domains } before_create :generate_site_access_code - attr_protected :master, :provider, :buyer, :from_email, :vat_rate, :sample_data, :default_service_id, :s3_prefix, - :provider_account_id, :paid_at, :paid, :signs_legal_terms, :tenant_id, :default_account_plan_id, - :default_service_id, :domain, :subdomain, :self_subdomain, :self_domain,:audit_ids, :partner, - :hosted_proxy_deployed_at - belongs_to :partner has_many :users, inverse_of: :account, dependent: :destroy has_many :admin_users, -> { admins }, class_name: 'User', inverse_of: :account @@ -337,10 +332,6 @@ def country=(country_name) end end - def special_fields - %i[country annotations] - end - # Returns the id corresponding to an account with given api key. This function avoids # database lookup if possible (uses cache), so it is super fast. def self.id_from_api_key(api_key) diff --git a/app/models/contract.rb b/app/models/contract.rb index 62ba38a387..308eb9441b 100644 --- a/app/models/contract.rb +++ b/app/models/contract.rb @@ -48,8 +48,6 @@ class Contract < ApplicationRecord # TODO: remove with Rails 3 attr_reader :old_plan, :accepted_on_create - attr_protected :plan_id, :state, :provider_public_key, :paid_until, :trial_period_expires_at, :setup_fee, :type, :variable_cost_paid_until, :application_id, :user_key, :user_account_id, :tenant_id, :audit_ids - # TODO: unit test this scope def self.provided_by(account) where.has do diff --git a/app/models/contract/trial.rb b/app/models/contract/trial.rb index 56d1c1ddfd..f1bb6447c1 100644 --- a/app/models/contract/trial.rb +++ b/app/models/contract/trial.rb @@ -6,8 +6,6 @@ module Contract::Trial included do before_create :set_trial_period_expires_at, :set_setup_fee - attr_protected :trial_period_expires_at - sifter :trial_period_expires_on do |date| sift(:date, trial_period_expires_at) == sift(:to_date, quoted(date.to_date)) end diff --git a/app/models/invitation.rb b/app/models/invitation.rb index bd5698338d..348fb37b68 100644 --- a/app/models/invitation.rb +++ b/app/models/invitation.rb @@ -24,7 +24,7 @@ class Invitation < ApplicationRecord # Build new user on information in this invitation. def make_user(params = {}) - self.user= account.users.build_with_fields params.reverse_merge(:email => email, :invitation => self) + self.user = account.users.build_with_fields params.reverse_merge(email: email, invitation: self) end def accepted? diff --git a/app/models/user.rb b/app/models/user.rb index c17f5a61e3..adb77fb3e3 100644 --- a/app/models/user.rb +++ b/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 diff --git a/app/models/user/invitations.rb b/app/models/user/invitations.rb index 224e4cfe87..53a51611ee 100644 --- a/app/models/user/invitations.rb +++ b/app/models/user/invitations.rb @@ -5,12 +5,11 @@ 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 - before_destroy :destroy_invitation + before_destroy :destroy_invitation end def accept_invitation diff --git a/app/models/user/permissions.rb b/app/models/user/permissions.rb index 2f70bdd3d2..e2ec004f7a 100644 --- a/app/models/user/permissions.rb +++ b/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 diff --git a/app/queries/usage_limit_violations_query.rb b/app/queries/usage_limit_violations_query.rb index 977c1eff12..4f70a69ca9 100644 --- a/app/queries/usage_limit_violations_query.rb +++ b/app/queries/usage_limit_violations_query.rb @@ -30,7 +30,7 @@ def initialize(account_id:, account_name:, alerts_count:) end def account - ::Account.new { |a| a.assign_attributes(account_attributes, without_protection: true) } + ::Account.new { |a| a.assign_attributes(account_attributes) } end protected diff --git a/app/services/signup_service.rb b/app/services/signup_service.rb index 8d8c888586..cb0eb58159 100644 --- a/app/services/signup_service.rb +++ b/app/services/signup_service.rb @@ -2,19 +2,18 @@ class SignupService - attr_reader :provider, :plans, :session, :account_params, :user_params, :authentication_provider + attr_reader :provider, :plans, :session, :authentication_provider, :account_manager - def initialize(provider:, plans:, session:, account_params:, user_params:, authentication_provider: nil) + def initialize(provider:, plans:, session:, authentication_provider: nil) @provider = provider @plans = plans @session = session - @account_params = account_params - @user_params = user_params.merge(signup_type: :new_signup) @authentication_provider = authentication_provider + @account_manager = Signup::DeveloperAccountManager.new(provider) end - def create - signup_result = Signup::DeveloperAccountManager.new(provider).create(signup_params) do |signup| + def create(account_params: {}, user_params: {}) + signup_result = account_manager.create(plans: plans, user_params: user_params.merge(signup_type: :new_signup), account_params: account_params) do |signup| strategy.on_new_user(signup.user, session) yield(signup) if block_given? end @@ -40,10 +39,6 @@ def account_should_be_approved?(signup_result) authentication_provider.automatically_approve_accounts? && !signup_result.account_approved? end - def signup_params - Signup::SignupParams.new(plans: plans, user_attributes: user_params, account_attributes: account_params) - end - def push_webhooks(user) webhook_name = 'created' buyer_account = user.account diff --git a/features/step_definitions/account_steps.rb b/features/step_definitions/account_steps.rb index b8bb1ee185..dc6df436f0 100644 --- a/features/step_definitions/account_steps.rb +++ b/features/step_definitions/account_steps.rb @@ -37,14 +37,19 @@ end Then "new accounts with {plan} will be pending for approval" do |plan| - params = Signup::SignupParams.new(plans: [plan], user_attributes: { - password: 'superSecret1234#', - username: 'pepe', - email: 'pepe@example.com' - }, account_attributes: { - org_name: 'Banana API' - }, defaults: {}) - signup = Signup::DeveloperAccountManager.new(current_account).create(params) + params = { + plans: [plan], + user_params: { + password: 'superSecret1234#', + username: 'pepe', + email: 'pepe@example.com' + }, + account_params: { + org_name: 'Banana API' + }, + defaults: {} + } + signup = Signup::DeveloperAccountManager.new(current_account).create(**params) assert signup.account_approval_required? end diff --git a/lib/developer_portal/app/controllers/developer_portal/accounts/invitee_signups_controller.rb b/lib/developer_portal/app/controllers/developer_portal/accounts/invitee_signups_controller.rb index 23d97821ce..1d47e48060 100644 --- a/lib/developer_portal/app/controllers/developer_portal/accounts/invitee_signups_controller.rb +++ b/lib/developer_portal/app/controllers/developer_portal/accounts/invitee_signups_controller.rb @@ -4,7 +4,7 @@ class DeveloperPortal::Accounts::InviteeSignupsController < DeveloperPortal::Bas before_action :redirect_if_logged_in before_action :find_invitation before_action :check_invitation! - before_action :build_user + before_action :build_new_user liquify prefix: 'accounts/invitee_signups'.freeze @@ -47,7 +47,7 @@ def sso_create def create build_sso_authorization - if @user.save + if @user.update(user_params) # we are activating users directly on signup so no activation email @user.activate! session.delete(:invitation_sso_uid) @@ -67,12 +67,6 @@ def sso_attributes_provided? sso_attributes.all? { |_key, value| value.present? } end - def create_sso_authorization - return unless sso_attributes_provided? - build_sso_authorization - @user.save - end - def authentication_provider site_account.authentication_providers .find_by(system_name: session[:invitation_sso_system_name]) @@ -118,8 +112,13 @@ def check_invitation! end end - def build_user - @user = @invitation.make_user(filter_readonly_params(params[:user], User)) + def build_new_user + @user = @invitation.make_user + end + + def user_params + allowed_attrs = @user.defined_fields_names + %w[password password_confirmation] + filter_readonly_params(params[:user], User).permit(*allowed_attrs) end def invitation_token diff --git a/lib/developer_portal/app/controllers/developer_portal/admin/account/passwords_controller.rb b/lib/developer_portal/app/controllers/developer_portal/admin/account/passwords_controller.rb index 7d74be3fb1..7ad2facfee 100644 --- a/lib/developer_portal/app/controllers/developer_portal/admin/account/passwords_controller.rb +++ b/lib/developer_portal/app/controllers/developer_portal/admin/account/passwords_controller.rb @@ -45,21 +45,10 @@ def redirect_to_request_password(error_message) redirect_to new_admin_account_password_url(request_password_reset: true) end - def buyer - @buyer ||= @provider.buyers.build do |account| - # We need to get all the account params to run the spam check - account.unflattened_attributes = account_params - end - end - def password_params params.require(:user).permit(:password, :password_confirmation) end - def account_params - params.fetch(:account, {}) - end - def find_user @token = params[:password_reset_token] @user = @provider.buyer_users.find_with_valid_password_token(@token) diff --git a/lib/developer_portal/app/controllers/developer_portal/admin/account/personal_details_controller.rb b/lib/developer_portal/app/controllers/developer_portal/admin/account/personal_details_controller.rb index 928ab5c41a..36b7651cf6 100644 --- a/lib/developer_portal/app/controllers/developer_portal/admin/account/personal_details_controller.rb +++ b/lib/developer_portal/app/controllers/developer_portal/admin/account/personal_details_controller.rb @@ -14,7 +14,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.' @@ -40,9 +40,11 @@ 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 + filter_readonly_params(user_params, User).permit(*resource.defined_fields_names, :password, :password_confirmation) end def verify_current_password diff --git a/lib/developer_portal/app/controllers/developer_portal/admin/account/users_controller.rb b/lib/developer_portal/app/controllers/developer_portal/admin/account/users_controller.rb index 7126f951d2..dbd2594d10 100644 --- a/lib/developer_portal/app/controllers/developer_portal/admin/account/users_controller.rb +++ b/lib/developer_portal/app/controllers/developer_portal/admin/account/users_controller.rb @@ -57,6 +57,6 @@ def assign_liquid_drops end def user_params - params.require(:user).permit(*@user.defined_fields.map(&:name), *@user.special_fields) + params.require(:user).permit(*@user.defined_fields_names, :password, :password_confirmation) end end diff --git a/lib/developer_portal/app/controllers/developer_portal/admin/accounts_controller.rb b/lib/developer_portal/app/controllers/developer_portal/admin/accounts_controller.rb index 65d40c4a3f..d81a5b7ba0 100644 --- a/lib/developer_portal/app/controllers/developer_portal/admin/accounts_controller.rb +++ b/lib/developer_portal/app/controllers/developer_portal/admin/accounts_controller.rb @@ -21,7 +21,7 @@ def update current_account.validate_fields! respond_to do |format| - if current_account.update_with_flattened_attributes(account_params) + if current_account.update(account_params) flash[:notice] = 'The account information was updated.'.freeze format.html { redirect_to admin_account_url } format.js { render js: "jQuery.flash.notice('#{flash[:notice]}')" } @@ -35,7 +35,8 @@ def update private def account_params - filter_readonly_params(params.require(:account), Account) + allowed_attrs = current_account.defined_fields_names - %w[country vat_rate] + %w[country_id] + filter_readonly_params(params.require(:account), Account).permit(*allowed_attrs) end def find_countries diff --git a/lib/developer_portal/app/controllers/developer_portal/admin/applications_controller.rb b/lib/developer_portal/app/controllers/developer_portal/admin/applications_controller.rb index ed4b76a06e..e38ed2022d 100644 --- a/lib/developer_portal/app/controllers/developer_portal/admin/applications_controller.rb +++ b/lib/developer_portal/app/controllers/developer_portal/admin/applications_controller.rb @@ -63,7 +63,7 @@ def update application.validate_fields! end - application.update(application_params) + application.update(permitted_application_params) assign_drops application: application # make sure to prevent xss on js rendering if this notice is changed to include e.g. application name if application.valid? @@ -127,7 +127,7 @@ def application end def new_application - @cinstance ||= applications.build_with_fields(application_params) do |application| + @cinstance ||= applications.build_with_fields(permitted_application_params) do |application| application.plan = application.can_change_plan?(service) ? plan : default_plan end end @@ -148,10 +148,6 @@ def plan end end - def application_params - @application_params ||= accepted_application_params - end - def authorize_new_app if service authorize! :create_application, service @@ -168,13 +164,14 @@ def authorize_update_app authorize! :update, application end - def accepted_application_params + def application_params # cinstance[*] naming is present for legacy reasons - application_attributes = params[:application] || params[:cinstance] - return {} unless application_attributes + @application_params ||= params[:application] || params.fetch(:cinstance, {}) + end - permitted_params = fields_definitions + %i[plan_id redirect_url] - application_attributes.permit(*permitted_params) + def permitted_application_params + permitted_params = fields_definitions + %i[redirect_url] + application_params.permit(*permitted_params) end def fields_definitions diff --git a/lib/developer_portal/app/controllers/developer_portal/base_controller.rb b/lib/developer_portal/app/controllers/developer_portal/base_controller.rb index ba9885bfed..54aa514400 100644 --- a/lib/developer_portal/app/controllers/developer_portal/base_controller.rb +++ b/lib/developer_portal/app/controllers/developer_portal/base_controller.rb @@ -27,9 +27,10 @@ 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) + read_only_fields += %w[country_id] if read_only_fields.include?('country') params.except(*read_only_fields) end end diff --git a/lib/developer_portal/app/controllers/developer_portal/signup_controller.rb b/lib/developer_portal/app/controllers/developer_portal/signup_controller.rb index 1ab0f04475..29b730e3a5 100644 --- a/lib/developer_portal/app/controllers/developer_portal/signup_controller.rb +++ b/lib/developer_portal/app/controllers/developer_portal/signup_controller.rb @@ -11,6 +11,7 @@ class SignupController < DeveloperPortal::BaseController before_action :deny_if_signup_disabled before_action :find_plans, :except => :success before_action :set_strategy, :only => %i[show create] + before_action :init_signup_service, only: %i[create] skip_before_action :finish_signup_for_paid_plan self.builtin_template_scope = 'signup' @@ -30,10 +31,7 @@ def show end def create - account_params = filter_readonly_params(params[:account], Account) - user_params = filter_readonly_params(account_params.try(:delete, :user), User) - - if signup_user!(account_params, user_params) + if signup_user! if @user.can_login? self.current_user = @user create_user_session! @@ -67,9 +65,9 @@ def authentication_provider .find_by(system_name: session[:authentication_provider]) end - def signup_user!(account_params, user_params) + def signup_user! Account.transaction do - SignupService.create(**signup_service_params(account_params, user_params)) do |signup_result| + @signup_service.create(account_params:, user_params: user_params.merge(signup_type: :minimal)) do |signup_result| @signup_result = signup_result @user = signup_result.user @buyer = signup_result.account @@ -80,16 +78,6 @@ def signup_user!(account_params, user_params) @signup_result.persisted? end - def signup_service_params(account_params, user_params) - { provider: @provider, - plans: @plans, - session: session, - account_params: account_params, - user_params: user_params, - authentication_provider: authentication_provider - } - end - def redirect_if_logged_in redirect_to admin_dashboard_path if logged_in? end @@ -134,5 +122,27 @@ def convert_legacy_params params[:plans] << params[type] if params[type].present? end end + + def init_signup_service + signup_service_params = { + provider: @provider, + plans: @plans, + session: session, + authentication_provider: authentication_provider + } + @signup_service = SignupService.new(**signup_service_params) + end + + def account_params + account = @signup_service.account_manager.account + allowed_attrs = account.defined_fields_names - %w[country vat_rate] + %w[country_id] + filter_readonly_params(params.fetch(:account, {}), Account).permit(*allowed_attrs) + end + + def user_params + user = @signup_service.account_manager.user + allowed_attrs = user.defined_fields_names + %w[password] + filter_readonly_params(params.fetch(:account, {})[:user], User).permit(*allowed_attrs) + end end end diff --git a/spec/acceptance/api/account_spec.rb b/spec/acceptance/api/account_spec.rb index 6d4c0dd95c..acedf8938f 100644 --- a/spec/acceptance/api/account_spec.rb +++ b/spec/acceptance/api/account_spec.rb @@ -8,11 +8,7 @@ let(:account) { FactoryBot.build(:buyer_account, provider_account: provider) } let(:payment_detail) { FactoryBot.create(:payment_detail, account: account) } - let(:resource) do - FieldsDefinition.create_defaults!(master) - provider.reload - account - end + let(:resource) { account } let(:expected_provider_fields) { %w[admin_domain domain admin_base_url base_url from_email support_email finance_support_email site_access_code] } diff --git a/spec/acceptance/api/account_user_spec.rb b/spec/acceptance/api/account_user_spec.rb index 7366b598ba..9f55454f94 100644 --- a/spec/acceptance/api/account_user_spec.rb +++ b/spec/acceptance/api/account_user_spec.rb @@ -7,11 +7,7 @@ let(:buyer) { FactoryBot.create(:buyer_account, provider_account: provider) } let(:user) { FactoryBot.build(:user, account: buyer) } - let(:resource) do - FieldsDefinition.create_defaults!(master) - provider.reload - user - end + let(:resource) { user } let(:account_id) { buyer.id } diff --git a/spec/acceptance/api/user_spec.rb b/spec/acceptance/api/user_spec.rb index 8d0299e76d..bba1622e5c 100644 --- a/spec/acceptance/api/user_spec.rb +++ b/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) } diff --git a/spec/support/api/context.rb b/spec/support/api/context.rb index 25fc20dbe1..c6989e0a69 100644 --- a/spec/support/api/context.rb +++ b/spec/support/api/context.rb @@ -22,7 +22,11 @@ shared_context "provider api", provider: true do let(:master) { provider && master_account } - let(:provider) { FactoryBot.create(:provider_account, self_domain: 'example.org') } + let(:provider) do + provider_acc = FactoryBot.create(:provider_account, self_domain: 'example.org') + FieldsDefinition.create_defaults!(provider_acc.provider_account) + provider_acc + end let(:provider_key) { provider.provider_key } parameter :provider_key, 'Provider Key' diff --git a/test/functional/developer_portal/admin/account/users_controller_test.rb b/test/functional/developer_portal/admin/account/users_controller_test.rb new file mode 100644 index 0000000000..26dee3d353 --- /dev/null +++ b/test/functional/developer_portal/admin/account/users_controller_test.rb @@ -0,0 +1,111 @@ +# frozen_string_literal: true + +require "test_helper" + +class DeveloperPortal::Admin::Account::UsersControllerTest < DeveloperPortal::ActionController::TestCase + include FieldsDefinitionsHelpers + + def setup + super + @provider = FactoryBot.create(:provider_account) + @buyer = FactoryBot.create(:buyer_account, provider_account: @provider) + @admin_user = @buyer.admins.first + @member_user = FactoryBot.create(:active_user, account: @buyer) + + host! @provider.external_domain + login_as @admin_user + end + + test "index shows users list" do + get :index + assert_response :success + + assigned_drops = assigns(:_assigned_drops) + + assert assigned_drops['users'].is_a?(Liquid::Drops::Collection) + assert assigned_drops['pagination'].is_a?(Liquid::Drops::Pagination) + end + + test "edit shows user form" do + get :edit, params: { id: @member_user.id } + assert_response :success + + user_drop = assigns(:_assigned_drops)['user'] + + assert user_drop.is_a?(Liquid::Drops::User) + assert_equal @member_user.username, user_drop.username + end + + test "update with valid params redirects to users list" do + put :update, params: { + id: @member_user.id, + user: { username: "new_username", email: "newemail@example.com" } + } + + assert_redirected_to admin_account_users_path + assert_equal "User was successfully updated.", flash[:notice] + + @member_user.reload + assert_equal "new_username", @member_user.username + assert_equal "newemail@example.com", @member_user.email + end + + test "update with invalid params renders edit" do + put :update, params: { + id: @member_user.id, + user: { email: "invalid-email" } + } + + assert_response :success + assert_template :edit + end + + test "update can change role when authorized" do + put :update, params: { + id: @member_user.id, + user: { role: "admin" } + } + + @member_user.reload + assert_equal :admin, @member_user.role + end + + test "member cannot change its own role" do + login_as @member_user + + put :update, params: { + id: @member_user.id, + user: { role: "admin" } + } + + @member_user.reload + assert_equal :member, @member_user.role + end + + test "update custom fields that are not read-only" do + field_defined(@provider, { target: "User", name: "custom_field" }) + field_defined(@provider, { target: "User", name: "readonly_field", read_only: true }) + @member_user.reload + + @member_user.update("custom_field" => "custom", "readonly_field" => "readonly") + + put :update, params: { + id: @member_user.id, + user: { custom_field: "new custom", readonly_field: "new readonly" } + } + + @member_user.reload + assert_equal "new custom", @member_user.extra_fields["custom_field"] + assert_equal "readonly", @member_user.extra_fields["readonly_field"] + end + + test "destroy removes user" do + user_to_delete = FactoryBot.create(:active_user, account: @buyer) + + assert_difference "@buyer.users.count", -1 do + delete :destroy, params: { id: user_to_delete.id } + end + + assert_redirected_to admin_account_users_path + end +end diff --git a/test/functional/developer_portal/admin/accounts_controller_test.rb b/test/functional/developer_portal/admin/accounts_controller_test.rb index 75d8274af2..7d2dbb0278 100644 --- a/test/functional/developer_portal/admin/accounts_controller_test.rb +++ b/test/functional/developer_portal/admin/accounts_controller_test.rb @@ -1,14 +1,60 @@ require 'test_helper' class DeveloperPortal::Admin::AccountsControllerTest < DeveloperPortal::ActionController::TestCase + include FieldsDefinitionsHelpers - test "update without account params should respond with 400" do - provider = FactoryBot.create(:simple_provider) - buyer = FactoryBot.create(:buyer_account, :provider_account => provider) + setup do + @provider = FactoryBot.create(:simple_provider) + @buyer = FactoryBot.create(:buyer_account, provider_account: @provider) + FieldsDefinition.create_defaults!(@provider) + + host! @provider.internal_domain + login_as @buyer.first_admin + end - host! provider.internal_domain - login_as buyer.first_admin + test "update without account params should respond with 400" do put :update assert_response 400 end + + test "extra fields can be updated, except vat_rate" do + field_defined(@provider, { target: "Account", name: "custom_field" }) + field_defined(@provider, { target: "Account", name: "vat_rate" }) + + assert_nil @buyer.vat_rate + + put :update, params: { + account: { + org_name: 'New name', + custom_field: 'test', + vat_rate: 10 + } + } + + assert_redirected_to '/admin/account' + @buyer.reload + + assert_equal 'New name', @buyer.org_name + assert_equal 'test', @buyer.extra_fields['custom_field'] + assert_nil @buyer.vat_rate + end + + test 'update country only when not read-only' do + country_field = field_defined(@provider, { target: "Account", name: "country" }) + + original_country = @buyer.country + assert_not_nil original_country + + country = FactoryBot.create(:country, code: "SL", name: "Superland") + + put :update, params: { account: { country_id: country.id } } + + assert_equal 'Superland', @buyer.reload.country.name + + country_field.update(read_only: true) + + put :update, params: { account: { country_id: original_country.id } } + + assert_equal 'Superland', @buyer.reload.country.name + end end diff --git a/test/functional/provider/admin/user/personal_details_controller_test.rb b/test/functional/provider/admin/user/personal_details_controller_test.rb index c6a122a5b0..683ae890da 100644 --- a/test/functional/provider/admin/user/personal_details_controller_test.rb +++ b/test/functional/provider/admin/user/personal_details_controller_test.rb @@ -3,11 +3,15 @@ require 'test_helper' class Provider::Admin::User::PersonalDetailsControllerTest < ActionController::TestCase + include FieldsDefinitionsHelpers def setup @provider = FactoryBot.create(:provider_account) + @master = @provider.provider_account + FieldsDefinition.create_defaults!(@master) host! @provider.external_admin_domain - login_as @provider.admins.first + @user = @provider.admins.first + login_as @user end @@ -43,11 +47,69 @@ def setup put :update, params: { user: {current_password: 'wrong_password', password: 'new_password', password_confirmation: 'new_password'} } end + test 'user can update permitted builtin and custom fields' do + field_defined(@master, { target: 'User', name: 'first_name' }) + field_defined(@master, { target: 'User', name: 'last_name' }) + field_defined(@master, { target: 'User', name: 'title' }) + field_defined(@master, { target: 'User', name: 'job_role' }) + field_defined(@master, { target: 'User', name: 'custom' }) + + put :update, params: { user: { + current_password: 'superSecret1234#', + username: 'newusername', + email: 'newemail@example.com', + first_name: 'NewFirstName', + last_name: 'NewLastName', + title: 'NewTitle', + job_role: 'NewJobRole', + extra_fields: { custom: 'custom value' } + } } + + assert_redirected_to edit_provider_admin_user_personal_details_path + @user.reload + assert_equal 'newusername', @user.username + assert_equal 'newemail@example.com', @user.email + assert_equal 'NewFirstName', @user.first_name + assert_equal 'NewLastName', @user.last_name + assert_equal 'NewTitle', @user.title + assert_equal 'NewJobRole', @user.job_role + assert_equal 'custom value', @user.extra_fields['custom'] + end + + test 'user cannot update role attribute' do + original_role = @user.role + + put :update, params: { user: { + current_password: 'superSecret1234#', + username: 'newusername', + role: 'member' + } } + + assert_redirected_to edit_provider_admin_user_personal_details_path + @user.reload + assert_equal 'newusername', @user.username + assert_equal original_role, @user.role, 'Role should not be updated' + end + + test 'user cannot update builtin and custom fields not defined explicitly' do + put :update, params: { user: { + current_password: 'superSecret1234#', + first_name: 'New Name', + extra_fields: { custom: 'some_value' } + } } + + assert_redirected_to edit_provider_admin_user_personal_details_path + @user.reload + assert_nil @user.first_name, 'Undefined builtin field should not be set' + assert_nil @user.extra_fields['custom'], 'Undefined custom field should not be set' + end + class SSOUserWithoutPasswordTest < ActionController::TestCase tests Provider::Admin::User::PersonalDetailsController def setup @provider = FactoryBot.create(:provider_account) + FieldsDefinition.create_defaults!(@provider.provider_account) @user = @provider.admins.first # Simulate an SSO user: no password and has authentication_id (makes oauth2? return true) @user.update_columns(password_digest: nil, authentication_id: 'sso-user-id') @@ -89,6 +151,7 @@ class UserWithPasswordTest < ActionController::TestCase def setup @provider = FactoryBot.create(:provider_account) + FieldsDefinition.create_defaults!(@provider.provider_account) @user = @provider.admins.first host! @provider.external_admin_domain login_as @user diff --git a/test/integration/admin/api/accounts_controller_test.rb b/test/integration/admin/api/accounts_controller_test.rb index eb6b6c8092..ac65a690b8 100644 --- a/test/integration/admin/api/accounts_controller_test.rb +++ b/test/integration/admin/api/accounts_controller_test.rb @@ -56,6 +56,7 @@ def setup test '#update from a member with service_permissions is updated correctly' do rolling_updates_on rolling_update(:service_permissions, enabled: true) + assert_not buyer.settings.monthly_billing_enabled put admin_api_account_path(buyer, format: :xml), params: update_params assert_response :ok @@ -70,10 +71,17 @@ def setup FactoryBot.create(:fields_definition, account: @provider, target: 'Account', name: 'my_field') - put admin_api_account_path(buyer, format: :xml), params: update_params.merge({ extra_fields: { my_field: 4 } }), as: :json + put admin_api_account_path(buyer, format: :xml), params: update_params.merge(extra_fields: { my_field: 4 }), as: :json assert_response :ok - assert buyer.reload.extra_fields['my_field'].is_a?(String) + my_field = buyer.reload.extra_fields['my_field'] + assert my_field.is_a?(String) + assert_equal "4", my_field + + put admin_api_account_path(buyer, format: :xml), params: update_params.merge(my_field: 'another value'), as: :json + assert_response :ok + + assert_equal 'another value', buyer.reload.extra_fields['my_field'] end test '#update from a member without service_permissions returns error message in xml' do diff --git a/test/integration/admin/api/backend_apis_controller_test.rb b/test/integration/admin/api/backend_apis_controller_test.rb index a4a3e331bb..0611660894 100644 --- a/test/integration/admin/api/backend_apis_controller_test.rb +++ b/test/integration/admin/api/backend_apis_controller_test.rb @@ -41,6 +41,22 @@ def setup assert_persists_right_params end + test 'update with annotations' do + assert_empty backend_api.annotations + + put admin_api_backend_api_path(backend_api), params: { + access_token: access_token_plaintext_value, + annotations: { managed_by: 'operator' } + } + + assert_response :success + + assert_equal({ 'managed_by' => 'operator'}, response.parsed_body[:backend_api][:annotations]) + + assert_equal 1, backend_api.reload.annotations.count + assert_equal 'operator', backend_api.annotations.where(name: 'managed_by').first.value + end + test 'update with errors in the model' do put admin_api_backend_api_path(backend_api), params: { access_token: access_token_plaintext_value, private_endpoint: '' } assert_response :unprocessable_entity @@ -56,6 +72,26 @@ def setup assert_persists_right_params end + test 'create with annotations' do + assert_difference(BackendApi.method(:count)) do + post admin_api_backend_apis_path, params: { + access_token: access_token_plaintext_value, + name: 'the-name', + description: 'New description.', + private_endpoint: 'http://custom-api.example.org:80', + annotations: { managed_by: 'operator' } + } + assert_response :created + end + + backend_api = response.parsed_body[:backend_api] + assert_equal({ 'managed_by' => 'operator'}, backend_api[:annotations]) + + persisted_backend_api = BackendApi.find(backend_api[:id]) + assert_equal 1, persisted_backend_api.annotations.count + assert_equal 'operator', persisted_backend_api.annotations.where(name: 'managed_by').first.value + end + test 'create with errors in the model' do post admin_api_backend_apis_path, params: { access_token: access_token_plaintext_value, private_endpoint: '' } assert_response :unprocessable_entity diff --git a/test/integration/admin/api/buyers_users_controller_test.rb b/test/integration/admin/api/buyers_users_controller_test.rb index c66c4da507..37e6ee608a 100644 --- a/test/integration/admin/api/buyers_users_controller_test.rb +++ b/test/integration/admin/api/buyers_users_controller_test.rb @@ -3,6 +3,8 @@ require 'test_helper' class Admin::Api::BuyersUsersControllerTest < ActionDispatch::IntegrationTest + include FieldsDefinitionsHelpers + def setup @provider = FactoryBot.create(:provider_account) @buyer = FactoryBot.create(:simple_buyer, provider_account: provider) @@ -13,6 +15,9 @@ def setup attr_reader :provider, :buyer test 'creates a user for its buyer' do + field_defined(provider, { target: 'User', name: 'first_name' }) + field_defined(provider, { target: 'User', name: 'last_name' }) + assert_difference(buyer.users.method(:count)) do post admin_api_account_users_path(buyer), params: params end @@ -22,6 +27,7 @@ def setup assert_equal expected_value, user.public_send(attr_name), "#{attr_name} expected to be #{expected_value} but is #{user.public_send(attr_name)}" end + assert_equal :api, user.signup_type end test 'updates a buyer user with password' do diff --git a/test/integration/api/applications_controller_test.rb b/test/integration/api/applications_controller_test.rb index 4ca90736ea..bb5e9425de 100644 --- a/test/integration/api/applications_controller_test.rb +++ b/test/integration/api/applications_controller_test.rb @@ -82,14 +82,14 @@ def setup attr_reader :service_plan, :buyer, :application_plan, :service - test 'crate application redirects to the provider admin index page' do + test 'create application redirects to the provider admin index page' do post admin_service_applications_path(service), params: { account_id: buyer.id, cinstance: { service_plan_id: service_plan.id, plan_id: application_plan.id, name: 'My Application' } } assert_redirected_to provider_admin_application_path(Cinstance.last) end - test 'crate application with no service plan selected' do + test 'create application with no service plan selected' do post admin_service_applications_path(service), params: { account_id: buyer.id, cinstance: { plan_id: application_plan.id, name: 'My Application' } } @@ -97,7 +97,7 @@ def setup assert_redirected_to provider_admin_application_path(application) end - test 'crate application with no service plan selected and a default service plan' do + test 'create application with no service plan selected and a default service plan' do default_service_plan = FactoryBot.create(:service_plan, service: service) service.update(default_service_plan: default_service_plan) post admin_service_applications_path(service), params: { account_id: buyer.id, @@ -107,7 +107,7 @@ def setup assert_equal default_service_plan, buyer.bought_service_contracts.first.service_plan end - test 'crate application with no service plan selected and no default service plan' do + test 'create application with no service plan selected and no default service plan' do other_service_plan = FactoryBot.create(:service_plan, service: service) service.update(default_service_plan: nil) post admin_service_applications_path(service), params: { account_id: buyer.id, @@ -117,7 +117,7 @@ def setup assert_not_equal other_service_plan, buyer.bought_service_contracts.first.service_plan end - test 'crate application with no service plan selected and a subscription' do + test 'create application with no service plan selected and a subscription' do subscribed_service_plan = FactoryBot.create(:service_plan, service: service) buyer.bought_service_contracts.create(plan: subscribed_service_plan) post admin_service_applications_path(service), params: { account_id: buyer.id, diff --git a/test/integration/audited_hacks_async_test.rb b/test/integration/audited_hacks_async_test.rb index 2a521ef68d..266deb84c1 100644 --- a/test/integration/audited_hacks_async_test.rb +++ b/test/integration/audited_hacks_async_test.rb @@ -14,6 +14,7 @@ class AuditedHacksAsyncTest < ActionDispatch::IntegrationTest Settings::Switch.any_instance.stubs(:allowed?).returns(true) @provider = FactoryBot.create(:simple_provider) + FieldsDefinition.create_defaults!(@provider.provider_account) @provider.create_settings # prevent creating settings lazily, as this triggers an Audit creation @admin = FactoryBot.create :simple_user, account: @provider, role: 'admin' User.current = @admin diff --git a/test/integration/buyers/accounts_controller_test.rb b/test/integration/buyers/accounts_controller_test.rb index 121e61ec96..f8a6790a4c 100644 --- a/test/integration/buyers/accounts_controller_test.rb +++ b/test/integration/buyers/accounts_controller_test.rb @@ -32,9 +32,9 @@ def setup assert_equal 'hi', user.extra_fields['created_by'] end - test 'billing address extra field and webhooks' do + test 'legal address extra field and webhooks' do FactoryBot.create(:fields_definition, account: @provider, - target: 'Account', name: 'billing_address', read_only: true) + target: 'Account', name: 'org_legaladdress') @provider.settings.allow_web_hooks! WebHook.delete_all @@ -273,6 +273,19 @@ def setup end end + test 'update account, including optional built-in and custom fields' do + FactoryBot.create(:fields_definition, account: @provider, target: 'Account', name: 'custom_account_field') + FactoryBot.create(:fields_definition, account: @provider, target: 'Account', name: 'vat_rate') + + put admin_buyers_account_path(@buyer), params: { account: { org_name: 'new name', vat_rate: '33', extra_fields: { custom_account_field: 'custom value' } } } + + assert_redirected_to admin_buyers_account_path(@buyer) + @buyer.reload + assert_equal 'new name', @buyer.name + assert_equal 'custom value', @buyer.extra_fields['custom_account_field'] + assert_equal 33.0, @buyer.vat_rate.to_f + end + test "can't manage buyer's of other providers" do another_provider = FactoryBot.create(:provider_account) login! another_provider diff --git a/test/integration/buyers/groups_controller_test.rb b/test/integration/buyers/groups_controller_test.rb new file mode 100644 index 0000000000..3ad7e20978 --- /dev/null +++ b/test/integration/buyers/groups_controller_test.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +require 'test_helper' + +class Buyers::GroupsControllerTest < ActionDispatch::IntegrationTest + setup do + @provider = FactoryBot.create(:provider_account) + @buyer = FactoryBot.create(:buyer_account, provider_account: @provider) + @group1 = FactoryBot.create(:cms_group, provider: @provider, name: "group1") + @group2 = FactoryBot.create(:cms_group, provider: @provider, name: "group2") + + @provider.settings.allow_groups! + login! @provider + end + + test 'show displays groups for buyer account' do + get admin_buyers_account_groups_path(@buyer) + + assert_response :success + + page = Nokogiri::HTML4::Document.parse(response.body) + checkboxes = page.xpath("//input[@name='account[group_ids][]'][@type='checkbox'][not(@hidden)]") + labels = checkboxes.map { |cb| cb.xpath("following-sibling::label").text } + + assert_same_elements %w[group1 group2], labels + assert checkboxes.all? { |cb| cb['checked'].nil? }, "Expected all checkboxes to be unchecked" + end + + test 'update sets correctly the groups' do + put admin_buyers_account_groups_path(@buyer), params: { + account: { + group_ids: [@group2.id] + } + } + + assert_redirected_to admin_buyers_account_groups_path(@buyer, id: @buyer.id) + assert_equal 'Account updated', flash[:success] + assert_equal [@group2.id], @buyer.reload.group_ids + + put admin_buyers_account_groups_path(@buyer), params: { + account: { + group_ids: [@group1.id, @group2.id] + } + } + assert_response :redirect + assert_same_elements [@group1.id, @group2.id], @buyer.reload.group_ids + + put admin_buyers_account_groups_path(@buyer), params: { + account: { + group_ids: [""] + } + } + assert_response :redirect + assert_empty @buyer.reload.group_ids + end +end diff --git a/test/integration/buyers/users_controller_integration_test.rb b/test/integration/buyers/users_controller_integration_test.rb index 40f87514a4..20fe9b6cbc 100644 --- a/test/integration/buyers/users_controller_integration_test.rb +++ b/test/integration/buyers/users_controller_integration_test.rb @@ -3,6 +3,8 @@ require 'test_helper' class Buyers::UsersControllerIntegrationTest < ActionDispatch::IntegrationTest + include FieldsDefinitionsHelpers + def setup @provider = FactoryBot.create(:provider_account) login! provider @@ -69,6 +71,31 @@ def setup assert_equal 'Japan', user.field_value('country') end + test 'only update optional fields that are enabled through fields definitions' do + field_defined(provider, { target: 'User', name: 'first_name' }) + field_defined(provider, { target: 'User', name: 'last_name' }) + provider.reload + user = FactoryBot.create(:member, account: buyer) + assert_same_elements %w[username email first_name last_name], user.defined_fields_names + + optional_fields = { + title: 'Ms.', first_name: 'fn', last_name: 'ln', job_role: 'manager' + } + + optional_fields.each_key do |field| + assert_nil user.public_send(field), "Field '#{field}' should not be set" + end + + put admin_buyers_account_user_path(account_id: buyer.id, id: user.id), params: { user: optional_fields } + + user.reload + + assert_equal 'fn', user.first_name + assert_equal 'ln', user.last_name + assert_nil user.job_role + assert_nil user.title + end + test "#activate" do user = FactoryBot.create(:pending_user, account: buyer) assert_not user.active? @@ -79,4 +106,64 @@ def setup assert_response :ok assert user.reload.active? end + + test "update password" do + user = buyer.admin_user + new_password = SecureRandom.hex(8) + assert_not user.authenticated?(new_password) + + put admin_buyers_account_user_path(account_id: buyer.id, id: user.id), params: { + user: { + password: new_password, + password_confirmation: new_password + } + } + + user.reload + assert user.reload.authenticated?(new_password) + end + + test "do not update password in does not match password confirmation" do + user = buyer.admin_user + new_password = SecureRandom.hex(8) + assert_not user.authenticated?(new_password) + + put admin_buyers_account_user_path(account_id: buyer.id, id: user.id), params: { + user: { + password: new_password, + password_confirmation: new_password.reverse + } + } + + assert_not user.reload.authenticated?(new_password) + + page = Nokogiri::HTML::Document.parse(response.body) + password_confirmation_error = page.at_xpath("//input[@id='user_password_confirmation']/following-sibling::p[@class='inline-errors']") + assert password_confirmation_error + assert_equal "doesn't match Password", password_confirmation_error.text + end + + test "update user role" do + member = FactoryBot.create(:member, account: buyer) + assert member.member? + + put admin_buyers_account_user_path(account_id: buyer.id, id: member.id), params: { + user: { + role: "admin" + } + } + + assert member.reload.admin? + end + + test "suspend and unsuspend user" do + user = buyer.admin_user + assert user.active? + + post suspend_admin_buyers_account_user_path(buyer, user) + assert user.reload.suspended? + + post unsuspend_admin_buyers_account_user_path(buyer, user) + assert user.reload.active? + end end diff --git a/test/integration/developer_portal/accounts/invitee_signups_controller_test.rb b/test/integration/developer_portal/accounts/invitee_signups_controller_test.rb index 57e37c5a24..03c6a4fe71 100644 --- a/test/integration/developer_portal/accounts/invitee_signups_controller_test.rb +++ b/test/integration/developer_portal/accounts/invitee_signups_controller_test.rb @@ -4,6 +4,7 @@ class DeveloperPortal::Accounts::InviteeSignupsControllerTest < ActionDispatch::IntegrationTest include System::UrlHelpers.cms_url_helpers + include FieldsDefinitionsHelpers def setup @buyer = FactoryBot.create(:buyer_account) @@ -65,6 +66,44 @@ def setup assert_redirected_to login_path end + test 'create sets custom fields that are not read-only' do + field_defined(buyer.provider_account, { target: 'User', name: 'department' }) + field_defined(buyer.provider_account, { target: 'User', name: 'employee_id', read_only: true }) + + invitation = FactoryBot.create(:invitation, account: buyer) + + assert_difference(buyer.users.method(:count), +1) do + post invitee_signup_path(invitation_token: invitation.token, user: user_params.merge( + department: 'Engineering', + employee_id: 'EMP-12345' + )) + end + + assert_equal I18n.t('developer_portal.accounts.invitee_signups.create.success'), flash[:notice] + assert_redirected_to login_path + + created_user = invitation.reload.user + assert_equal 'Engineering', created_user.extra_fields['department'] + assert_nil created_user.extra_fields['employee_id'] + end + + test 'do not set unpermitted attributes' do + invitation = FactoryBot.create(:invitation, account: buyer) + + assert_difference(buyer.users.method(:count), +1) do + post invitee_signup_path(invitation_token: invitation.token, user: user_params.merge( + role: 'superadmin' + )) + end + + assert_equal I18n.t('developer_portal.accounts.invitee_signups.create.success'), flash[:notice] + assert_redirected_to login_path + + created_user = invitation.reload.user + assert_equal :member, created_user.role + assert_equal 'admin', created_user.username + end + private def user_params diff --git a/test/integration/developer_portal/edit_account_test.rb b/test/integration/developer_portal/edit_account_test.rb index 7eeeaf98f8..69fe4ff9ce 100644 --- a/test/integration/developer_portal/edit_account_test.rb +++ b/test/integration/developer_portal/edit_account_test.rb @@ -6,6 +6,7 @@ class DeveloperPortal::EditAccountTest < ActionDispatch::IntegrationTest def setup @provider = FactoryBot.create(:simple_provider) @buyer = FactoryBot.create(:buyer_account, provider_account: @provider, org_name: 'ontheroad') + FieldsDefinition.create_defaults!(@provider) login_buyer @buyer diff --git a/test/integration/developer_portal/invitation_signup_test.rb b/test/integration/developer_portal/invitation_signup_test.rb index 030f5e8322..22024ce7c1 100644 --- a/test/integration/developer_portal/invitation_signup_test.rb +++ b/test/integration/developer_portal/invitation_signup_test.rb @@ -4,6 +4,7 @@ class DeveloperPortal::InvitationSignupTest < ActionDispatch::IntegrationTest include System::UrlHelpers.cms_url_helpers OAuth2 = Authentication::Strategy::OAuth2 + UserData = ThreeScale::OAuth2::UserData def setup @provider = FactoryBot.create(:simple_provider) @@ -24,7 +25,7 @@ def test_show # sso attributes do exist, sso authorization object should be built # and therefore, user password should not be required OAuth2.any_instance.stubs(:authentication_provider).returns(@auth_provider) - OAuth2.any_instance.expects(:user_data).returns({ uid: '12345' }) + OAuth2.any_instance.expects(:user_data).returns(UserData.new({ uid: '12345' })) get "/auth/invitations/#{@invitation.token}/github/callback" assert_response :success get invitee_signup_path(invitation_token: @invitation.token) @@ -77,7 +78,7 @@ def test_sso_create assert session[:invitation_sso_uid].blank? refute assigns(:user).valid? - OAuth2.any_instance.expects(:user_data).returns({ uid: '12345' }) + OAuth2.any_instance.expects(:user_data).returns(UserData.new({ uid: '12345' })) get "/auth/invitations/#{@invitation.token}/github/callback" assert_response :success assert_equal '12345', session[:invitation_sso_uid] @@ -103,6 +104,7 @@ def test_error_sso_create end def test_create + FieldsDefinition.create_defaults!(@provider) OAuth2.any_instance.stubs(:authentication_provider).returns(@auth_provider) assert_difference '@buyer.users.count' do diff --git a/test/integration/master/api/providers_controller_integration_test.rb b/test/integration/master/api/providers_controller_integration_test.rb index 630f18fe5a..656021a30f 100644 --- a/test/integration/master/api/providers_controller_integration_test.rb +++ b/test/integration/master/api/providers_controller_integration_test.rb @@ -10,6 +10,7 @@ def setup @service_plan = master_account.default_service_plans.first @application_plan = master_account.default_application_plans.first + FieldsDefinition.create_defaults!(master_account) FactoryBot.create(:fields_definition, account: master_account, target: 'Account', name: 'account_extra_field') FactoryBot.create(:fields_definition, account: master_account, target: 'User', name: 'user_extra_field') @@ -169,6 +170,15 @@ def setup assert_response :unprocessable_entity end + test '#create with annotations' do + post master_api_providers_path(format: :json), params: signup_params.merge({ annotations: { managed_by: 'operator' }}) + assert_response :created + + new_tenant = response.parsed_body[:signup][:account] + assert_equal({ 'managed_by' => 'operator'}, new_tenant[:annotations]) + assert_equal 'operator', Account.find(new_tenant[:id]).annotations.where(name: 'managed_by').first.value + end + test '#update' do provider = FactoryBot.create(:provider_account, provider_account: master_account) user = FactoryBot.create(:member, account: master_account, admin_sections: ['partners']) @@ -197,14 +207,35 @@ def setup token = FactoryBot.create(:access_token, owner: user, scopes: 'account_management') provider.schedule_for_deletion! - update_params = { account: { from_email: 'from@email.com', state_event: 'resume'}, + update_params = { account: { from_email: 'from@email.com', state_event: 'resume', org_name: 'new-org-name' }, access_token: token.plaintext_value, format: :json } put master_api_provider_path(provider, update_params) assert_response :ok provider.reload + assert_equal 'approved', provider.state assert_not_equal update_params[:account][:from_email], provider.from_email - assert_equal 'approved', provider.state + assert_not_equal 'new-org-name', provider.org_name + end + + test '#update with annotations' do + provider = FactoryBot.create(:provider_account, provider_account: master_account) + user = FactoryBot.create(:member, account: master_account, admin_sections: ['partners']) + token = FactoryBot.create(:access_token, owner: user, scopes: 'account_management') + + assert_empty provider.annotations + + put master_api_provider_path(provider, format: :json), params: { + annotations: { managed_by: 'operator' }, + access_token: token.plaintext_value + } + assert_response :ok + + updated_account = response.parsed_body[:signup][:account] + assert_equal({ 'managed_by' => 'operator'}, updated_account[:annotations]) + + assert_equal 1, provider.reload.annotations.count + assert_equal 'operator', provider.annotations.where(name: 'managed_by').first.value end test '#destroy' do @@ -260,7 +291,9 @@ def signup_params(different_params = {}) email: 'person@example.com', password: 'superSecret1234#', user_extra_field: 'hi-user', - account_extra_field: 'hi-account' + account_extra_field: 'hi-account', + from_email: 'from@example.com', + support_email: 'support@example.com' }.merge(different_params) end diff --git a/test/integration/multitenant_enforcement_test.rb b/test/integration/multitenant_enforcement_test.rb index 5576d785aa..8376828cc7 100644 --- a/test/integration/multitenant_enforcement_test.rb +++ b/test/integration/multitenant_enforcement_test.rb @@ -5,6 +5,7 @@ class MultitenantEnforcementTest < ActionDispatch::IntegrationTest setup do @provider = FactoryBot.create(:provider_account) + FieldsDefinition.create_defaults!(@provider.provider_account) end test "forbid retrieving objects from multiple tenants" do diff --git a/test/integration/provider/admin/account/users_controller_test.rb b/test/integration/provider/admin/account/users_controller_test.rb index c7c414da99..050f25b439 100644 --- a/test/integration/provider/admin/account/users_controller_test.rb +++ b/test/integration/provider/admin/account/users_controller_test.rb @@ -3,6 +3,7 @@ require 'test_helper' class Provider::Admin::Account::UsersControllerTest < ActionDispatch::IntegrationTest + include FieldsDefinitionsHelpers def setup @provider = FactoryBot.create :provider_account @@ -11,6 +12,7 @@ def setup @default_service_ids = @services.first(2).map(&:id) @user = FactoryBot.create :simple_user, account: @provider, member_permission_ids: @default_ids, member_permission_service_ids: @default_service_ids + FieldsDefinition.create_defaults!(@provider.provider_account) login! @provider end @@ -157,4 +159,39 @@ def setup assert_select 'input[name="user[password]"]' assert_select 'input[name="user[password_confirmation]"]' end + + test 'update user fields including optional and custom ones' do + field_defined(@provider.provider_account, { target: 'User', name: 'job_role' }) + field_defined(@provider.provider_account, { target: 'User', name: 'custom_field' }) + + put provider_admin_account_user_path(@user), params: + { + user: { + username: 'new_username', + job_role: 'developer', + extra_fields: { + custom_field: 'custom value' + } + } + } + + @user.reload + + assert_equal 'new_username', @user.username + assert_equal 'developer', @user.job_role + assert_equal 'custom value', @user.extra_fields['custom_field'] + end + + test 'update password' do + put provider_admin_account_user_path(@user), params: { user: { password: 'superSecret1234#', password_confirmation: 'not-matching' } } + + assert_template :edit + assert_select 'p.inline-errors', text: "doesn't match Password" + + put provider_admin_account_user_path(@user), params: { user: { password: 'superSecret1234#', password_confirmation: 'superSecret1234#' } } + + assert_redirected_to provider_admin_account_users_path + assert_equal "User was successfully updated", flash[:success] + assert @user.reload.authenticated?('superSecret1234#') + end end diff --git a/test/integration/provider/admin/accounts_controller_test.rb b/test/integration/provider/admin/accounts_controller_test.rb index d2db2717f5..846e4136a8 100644 --- a/test/integration/provider/admin/accounts_controller_test.rb +++ b/test/integration/provider/admin/accounts_controller_test.rb @@ -11,6 +11,7 @@ def setup @service_plan = master.default_service_plans.first @application_plan = master.default_application_plans.first @application_plan.update(system_name: 'enterprise') # for the switches tested later + FieldsDefinition.create_defaults!(@master) FactoryBot.create(:fields_definition, account: @master, target: 'User', name: 'created_by') end diff --git a/test/integration/provider/invitee_signups_controller_integration_test.rb b/test/integration/provider/invitee_signups_controller_integration_test.rb index 80d5c7c648..b854f336ef 100644 --- a/test/integration/provider/invitee_signups_controller_integration_test.rb +++ b/test/integration/provider/invitee_signups_controller_integration_test.rb @@ -19,6 +19,7 @@ def setup end test 'create' do + FieldsDefinition.create_defaults!(provider.provider_account) assert_difference(provider.users.method(:count)) do post provider_invitee_signup_path(invitation_token: invitation.token, user: user_params) end @@ -27,6 +28,25 @@ def setup assert_redirected_to provider_login_path end + test 'do not set unpermitted attributes' do + FieldsDefinition.create_defaults!(provider.provider_account) + invitation = FactoryBot.create(:invitation, account: provider) + + assert_difference(provider.users.method(:count), +1) do + post provider_invitee_signup_path(invitation_token: invitation.token, user: user_params.merge( + role: 'superadmin' + )) + end + + assert_equal I18n.t('provider.invitee_signups.create.success'), flash[:success] + assert_redirected_to provider_login_path + + created_user = invitation.reload.user + assert_equal :member, created_user.role + assert_equal 'admin', created_user.username + end + + test 'get asks for upgrade' do provider.create_provider_constraints!(max_users: 0) diff --git a/test/integration/provider/signups_controller_integration_test.rb b/test/integration/provider/signups_controller_integration_test.rb index 92f0844334..c275d2185b 100644 --- a/test/integration/provider/signups_controller_integration_test.rb +++ b/test/integration/provider/signups_controller_integration_test.rb @@ -5,6 +5,7 @@ class Provider::SignupsControllerIntegrationTest < ActionDispatch::IntegrationTest def setup login! master_account + FieldsDefinition.create_defaults!(master_account) end test 'disables x_frame_options header' do diff --git a/test/integration/user-management-api/accounts_test.rb b/test/integration/user-management-api/accounts_test.rb index 170346a342..93679ebe59 100644 --- a/test/integration/user-management-api/accounts_test.rb +++ b/test/integration/user-management-api/accounts_test.rb @@ -62,12 +62,31 @@ def setup end test 'update billing_address' do + field_defined(@provider, { target: "Account", name: "billing_address", read_only: true }) + put admin_api_account_path(@buyer, format: :xml), params: params.merge({ org_name: 'alaska', billing_address: 'Calle Napoles 187, Barcelona. Spain' }) assert_response :unprocessable_entity - billing_address = { name: '3scale', address1: 'Calle Napoles 187', city: 'Barcelona', country: 'Spain' }.transform_keys { |k| "billing_address[#{k}]" } - put admin_api_account_path(@buyer, format: :xml), params: params.merge(billing_address).merge({ org_name: 'alaska' }) + billing_address = { name: '3scale', address1: 'Calle Napoles 187', city: 'Barcelona', country: 'Spain' } + + billing_address_params_nested = billing_address.transform_keys { |k| "billing_address[#{k}]" } + put admin_api_account_path(@buyer, format: :xml), params: params.merge(billing_address_params_nested) assert_response :success + + @buyer.reload + assert_equal 'Barcelona', @buyer.billing_address_city + assert_equal 'Calle Napoles 187', @buyer.billing_address_address1 + + # reset to empty to test with another format + @buyer.update(billing_address_name: '', billing_address_address1: '', billing_address_city: '', billing_address_country: '') + + billing_address_params_strings = billing_address.transform_keys { |k| "billing_address_#{k}" } + put admin_api_account_path(@buyer, format: :xml), params: params.merge(billing_address_params_strings) + assert_response :success + + @buyer.reload + assert_equal 'Barcelona', @buyer.billing_address_city + assert_equal 'Calle Napoles 187', @buyer.billing_address_address1 end test '#update' do @@ -606,6 +625,7 @@ class ProviderKeyTest < Admin::Api::AccountsTest test 'update with extra fields' do field_defined(@provider, { target: "Account", name: "some_extra_field" }) + field_defined(@provider, { target: "Account", name: "vat_rate" }) put admin_api_account_path(@buyer, format: :xml), params: params.merge({ some_extra_field: "stuff", vat_rate: 33 }) @@ -614,7 +634,18 @@ class ProviderKeyTest < Admin::Api::AccountsTest @buyer.reload assert_equal "stuff", @buyer.extra_fields["some_extra_field"] - assert_equal 33, @buyer.vat_rate + assert_equal 33.0, @buyer.vat_rate.to_f + end + + test 'account update with annotations' do + put admin_api_account_path(@buyer, format: :json), params: params.merge({ annotations: { managed_by: 'operator' } }) + + assert_response :success + + assert_equal({ 'managed_by' => 'operator'}, response.parsed_body[:account][:annotations]) + + assert_equal 1, @buyer.reload.annotations.count + assert_equal 'operator', @buyer.annotations.where(name: 'managed_by').first.value end test 'destroy' do diff --git a/test/integration/user-management-api/buyers_users_test.rb b/test/integration/user-management-api/buyers_users_test.rb index 103d4ee4d3..8ebb17b919 100644 --- a/test/integration/user-management-api/buyers_users_test.rb +++ b/test/integration/user-management-api/buyers_users_test.rb @@ -9,6 +9,7 @@ class Admin::Api::BuyerUsersTest < ActionDispatch::IntegrationTest def setup @provider = FactoryBot.create(:provider_account, domain: 'provider.example.com') @provider.default_account_plan = @provider.account_plans.first + @token = FactoryBot.create(:access_token, owner: @provider.first_admin, scopes: ['account_management']) @buyer = FactoryBot.create(:buyer_account, provider_account: @provider) @buyer.buy! @provider.default_account_plan @@ -22,7 +23,7 @@ def setup class AccessTokenWithCallbacks < ActionDispatch::IntegrationTest def setup - @provider = FactoryBot.create(:simple_provider) + @provider = FactoryBot.create(:provider_account) @buyer = FactoryBot.create(:simple_buyer, provider_account: @provider) host! @provider.external_admin_domain @@ -531,6 +532,33 @@ def setup assert chuck.active? end + test 'only update optional fields that are enabled through fields definitions' do + field_defined(@provider, { target: 'User', name: 'first_name' }) + field_defined(@provider, { target: 'User', name: 'last_name' }) + assert_same_elements %w[username email first_name last_name], @member.reload.defined_fields_names + + optional_fields = { + title: 'Ms.', first_name: 'fn', last_name: 'ln', job_role: 'manager' + } + + optional_fields.each_key do |field| + assert_nil @member.public_send(field), "Field '#{field}' should not be set" + end + + put admin_api_account_user_path(account_id: @buyer.id, format: :xml, id: @member.id), params: { + access_token: @token.plaintext_value, + **optional_fields + } + assert_response :success + + @member.reload + + assert_equal 'fn', @member.first_name + assert_equal 'ln', @member.last_name + assert_nil @member.job_role + assert_nil @member.title + end + protected def active_user diff --git a/test/integration/user-management-api/services_test.rb b/test/integration/user-management-api/services_test.rb index 0d5f7346d4..18a286ec46 100644 --- a/test/integration/user-management-api/services_test.rb +++ b/test/integration/user-management-api/services_test.rb @@ -57,6 +57,26 @@ def setup assert @provider.services.find_by(name: 'service foo') end + test 'create with annotations' do + Settings::Switch.any_instance.stubs(:allowed?).returns(true) + + assert_difference(Service.method(:count)) do + post admin_api_services_path(format: :json), params: { + provider_key: @provider.api_key, + name: 'annotated service', + annotations: { managed_by: 'operator' } + } + assert_response :created + end + + service = response.parsed_body[:service] + assert_equal({ 'managed_by' => 'operator'}, service[:annotations]) + + persisted_service = Service.find(service[:id]) + assert_equal 1, persisted_service.annotations.count + assert_equal 'operator', persisted_service.annotations.where(name: 'managed_by').first.value + end + test 'create with json body parameters' do @provider.settings.allow_multiple_services! @provider.provider_constraints.update!(max_services: 5) @@ -93,6 +113,21 @@ def setup assert_equal 'supp@topo.com', @service.support_email end + test 'update with annotations' do + put admin_api_service_path(@service, format: :json), params: { + provider_key: @provider.api_key, + annotations: { managed_by: 'operator' } + } + + assert_response :success + + service = response.parsed_body[:service] + assert_equal({ 'managed_by' => 'operator'}, service[:annotations]) + + assert_equal 1, @service.reload.annotations.count + assert_equal 'operator', @service.annotations.where(name: 'managed_by').first.value + end + pending_test 'update with wrong id' do end diff --git a/test/integration/user-management-api/signup_test.rb b/test/integration/user-management-api/signup_test.rb index 1c4b496d06..ef7c5217ca 100644 --- a/test/integration/user-management-api/signup_test.rb +++ b/test/integration/user-management-api/signup_test.rb @@ -71,6 +71,8 @@ def setup end test 'successful api signup with country' do + field_defined(@provider, { target: 'Account', name: 'country' }) + post admin_api_signup_path, params: { format: :xml, provider_key: @provider.api_key, org_name: 'fiona', username: 'fiona', country: 'Spain' } assert_response :created @@ -113,6 +115,8 @@ def setup UserMailer.expects(:deliver_signup_notification).never field_defined(@provider, { target: 'Account', name: 'account_extra_field' }) + field_defined(@provider, { target: 'Account', name: 'org_legaladdress' }) + field_defined(@provider, { target: 'Account', name: 'vat_rate' }) field_defined(@provider, { target: 'User', name: 'user_extra_field' }) post admin_api_signup_path, params: { format: :xml, @@ -325,4 +329,19 @@ def setup assert xml.xpath('.//account/applications/application').present? end + + test 'account signup with annotations' do + post admin_api_signup_path(format: :json), params: { + provider_key: @provider.api_key, + org_name: 'annotated account', + username: 'annotated', + annotations: { managed_by: 'operator' } + } + + assert_response :success + + new_account = response.parsed_body[:account] + assert_equal({ 'managed_by' => 'operator'}, new_account[:annotations]) + assert_equal 'operator', Account.find(new_account[:id]).annotations.where(name: 'managed_by').first.value + end end diff --git a/test/integration/user-management-api/users_test.rb b/test/integration/user-management-api/users_test.rb index 762ea10266..3382ab957a 100644 --- a/test/integration/user-management-api/users_test.rb +++ b/test/integration/user-management-api/users_test.rb @@ -160,6 +160,18 @@ def setup assert_equal admin_sections, @member.admin_sections end + test 'set member permissions with provider key' do + service = @provider.services.default + + put admin_api_user_path(format: :xml, id: @member.id), params: { member_permission_service_ids: [service.id], member_permission_ids: %w[monitoring services], provider_key: @provider.api_key } + + assert_response :success + + assert @member.reload + assert_equal [service.id], @member.member_permission_service_ids + assert_equal Set[ :services, :monitoring ], @member.admin_sections + end + test 'suspend/unsuspend with access token as a member' do token = FactoryBot.create(:access_token, owner: @member, scopes: ['account_management']) diff --git a/test/unit/account/provider_domains_test.rb b/test/unit/account/provider_domains_test.rb index 8aba633231..8d0f095175 100644 --- a/test/unit/account/provider_domains_test.rb +++ b/test/unit/account/provider_domains_test.rb @@ -41,11 +41,6 @@ class Account::ProviderDomainsTest < ActiveSupport::TestCase assert_nil account.dedicated_domain end - test 'domain is excluded from mass assignment ' do - account = Account.new(:domain => 'api.example.net') - assert_nil account.dedicated_domain - end - test '#superdomain returns the domain without www striped' do account = Account.new diff --git a/test/unit/account_test.rb b/test/unit/account_test.rb index 3c8b2982a5..05849840d7 100644 --- a/test/unit/account_test.rb +++ b/test/unit/account_test.rb @@ -183,6 +183,26 @@ def setup assert_equal 2.34, @account.reload.vat_rate end + test 'vat_rate value is converted automatically on update' do + assert_nil @account.vat_rate + + @account.update(vat_rate: "23.45") + assert_instance_of BigDecimal, @account.vat_rate + assert_equal 23.45, @account.vat_rate + + @account.update(vat_rate: Float(45.23)) + assert_instance_of BigDecimal, @account.vat_rate + assert_equal 45.23, @account.vat_rate + + @account.update(vat_rate: Integer(30)) + assert_instance_of BigDecimal, @account.vat_rate + assert_equal 30, @account.vat_rate + + @account.update(vat_rate: 'random-string') + assert_instance_of BigDecimal, @account.vat_rate + assert_equal 0, @account.vat_rate + end + test 'without :timezone set return UTC as default :timezone' do assert_equal 'UTC', @account.timezone end diff --git a/test/unit/developer_portal/base_controller_test.rb b/test/unit/developer_portal/base_controller_test.rb index 23c15577a9..c6319076a5 100644 --- a/test/unit/developer_portal/base_controller_test.rb +++ b/test/unit/developer_portal/base_controller_test.rb @@ -14,9 +14,9 @@ class FilterReadOnlyParamsTest < BaseControllerTest params = account.fields_definitions.each_with_object({}) { |fd, p| p[fd.name]=SecureRandom.hex } TestController.any_instance.expects(:site_account).returns(account) - result = TestController.new.send(:filter_readonly_params, params, User) + result = TestController.new.send(:filter_readonly_params, ::ActionController::Parameters.new(params), User) - assert_equal 3, result.size + assert_equal 3, result.keys.size assert_not_includes result, ro_fields.map(&:name) end diff --git a/test/unit/fields/fields_test.rb b/test/unit/fields/fields_test.rb index c8a4d8ea6c..e21c505867 100644 --- a/test/unit/fields/fields_test.rb +++ b/test/unit/fields/fields_test.rb @@ -61,4 +61,40 @@ def test_fields_definitions_source! model.fields_definitions_source! end end + + def test_defined_fields_names + field1 = mock('field', name: 'field1') + field2 = mock('field', name: 'field2') + model = Model.new + model.stubs(:defined_fields).returns([field1, field2]) + assert_same_elements %w[field1 field2], model.defined_fields_names + end + + def test_defined_extra_fields_names + extra_field = mock('field', name: 'extra') + builtin_field = mock('field') + model = Model.new + model.stubs(:defined_fields).returns([extra_field, builtin_field]) + model.stubs(:defined_builtin_fields).returns([builtin_field]) + assert_equal %w[extra], model.defined_extra_fields_names + end + + def test_defined_builtin_fields_names + field1 = mock('field', name: 'field1') + field2 = mock('field', name: 'field2') + model = Model.new + model.stubs(:defined_builtin_fields).returns([field1, field2]) + assert_same_elements %w[field1 field2], model.defined_builtin_fields_names + end + + def test_provider_fields + provider = FactoryBot.create :provider_account + FactoryBot.create(:fields_definition, account: provider, target: 'Account', name: 'custom_account') + FactoryBot.create(:fields_definition, account: provider, target: 'User', name: 'custom_user') + FactoryBot.create(:fields_definition, account: provider, target: 'Cinstance', name: 'custom_application') + + assert_same_elements %w[org_name custom_account], provider.defined_fields_names_for(Account) + assert_same_elements %w[username email custom_user], provider.defined_fields_names_for(User) + assert_same_elements %w[name description custom_application], provider.defined_fields_names_for(Cinstance) + end end diff --git a/test/unit/services/signup_service_test.rb b/test/unit/services/signup_service_test.rb index f38d3d18d0..9faf5ed2e6 100644 --- a/test/unit/services/signup_service_test.rb +++ b/test/unit/services/signup_service_test.rb @@ -19,34 +19,32 @@ def test_create assert_not user.persisted? end - service = new_instance_signup_service(valid_account_params, valid_user_params) + service = new_instance_signup_service assert_difference(User.method(:count), +1) do - user = service.create + user = service.create(account_params: valid_account_params, user_params: valid_user_params) assert user.persisted? end - service = new_instance_signup_service(valid_account_params, valid_user_params) + service = new_instance_signup_service assert_difference(User.method(:count), +1) do - signup = service.create { |_signup| [] } + signup = service.create(account_params: valid_account_params, user_params: valid_user_params) { |_signup| [] } assert signup.persisted? end - service = new_instance_signup_service(valid_account_params, valid_user_params) + service = new_instance_signup_service assert_difference(User.method(:count), 0) do - service.create { |signup| @signup = signup; break } + service.create(account_params: valid_account_params, user_params: valid_user_params) { |signup| @signup = signup; break } refute @signup.persisted? end end private - def new_instance_signup_service(account_params = {}, user_params = {}) + def new_instance_signup_service SignupService.new( provider: @provider, plans: [], - session: @session, - account_params: account_params, - user_params: user_params + session: @session ) end diff --git a/test/unit/signup/account_manager_test.rb b/test/unit/signup/account_manager_test.rb index a18efc1778..34127a5fcc 100644 --- a/test/unit/signup/account_manager_test.rb +++ b/test/unit/signup/account_manager_test.rb @@ -8,9 +8,28 @@ class AccountManagerTest < ActiveSupport::TestCase include TestHelpers::Events class ProviderAccountManagerTest < Signup::AccountManagerTest + + setup do + FieldsDefinition.create_defaults!(manager_account) + end + + test 'validate_fields parameter controls field validation' do + # With validate_fields: true (default), fields are validated + manager_with_validation = signup_account_manager + manager_with_validation.account.expects(:validate_fields!).once + manager_with_validation.user.expects(:validate_fields!).once + manager_with_validation.create(**signup_params, validate_fields: true) + + # With validate_fields: false, fields are not validated + manager_without_validation = signup_account_manager + manager_without_validation.account.expects(:validate_fields!).never + manager_without_validation.user.expects(:validate_fields!).never + manager_without_validation.create(**signup_params, validate_fields: false) + end + test 'create provider with right params' do org_name_param = 'Alaska' - signup_result = signup_account_manager.create(signup_params(different_account_params: {org_name: org_name_param})) + signup_result = signup_account_manager.create(**signup_params(different_account_params: {org_name: org_name_param})) account = signup_result.account user = signup_result.user imp_config = ThreeScale.config.impersonation_admin @@ -71,7 +90,7 @@ class ProviderAccountManagerTest < Signup::AccountManagerTest end test 'enqueues signup job' do - signup_account_manager.create(signup_params) do |signup_result| + signup_account_manager.create(**signup_params) do |signup_result| SignupWorker.expects(:enqueue).with(signup_result.account) end end @@ -84,7 +103,7 @@ class ProviderAccountManagerTest < Signup::AccountManagerTest # On-prem ThreeScale.config.stubs(onpremises: true) - signup_result = signup_account_manager.create(signup_params) + signup_result = signup_account_manager.create(**signup_params) account = signup_result.account # account has the right plans assert_equal account_plan, account.bought_account_plan @@ -95,7 +114,7 @@ class ProviderAccountManagerTest < Signup::AccountManagerTest # Saas ThreeScale.config.stubs(onpremises: false) - signup_result = signup_account_manager.create(signup_params) + signup_result = signup_account_manager.create(**signup_params) account = signup_result.account # account has the right plans assert_equal account_plan, account.bought_account_plan @@ -106,7 +125,7 @@ class ProviderAccountManagerTest < Signup::AccountManagerTest end test 'create provider with wrong params does not create correctly without org_name' do - signup_result = signup_account_manager.create(signup_params(different_account_params: { org_name: '' })) + signup_result = signup_account_manager.create(**signup_params(different_account_params: { org_name: '' })) refute signup_result.valid? refute signup_result.persisted? @@ -119,14 +138,14 @@ class ProviderAccountManagerTest < Signup::AccountManagerTest test 'creating a provider will create contract only 1 contract with the default application plan' do enterprise_plan = manager_account.default_application_plans.first! enterprise_plan.update_attribute(:system_name, 'enterprise') - account = signup_account_manager.create(signup_params).account + account = signup_account_manager.create(**signup_params).account assert account.signup? # assert signup_mode instance variable to true, which skips the application_plan callback assert_equal [enterprise_plan], account.bought_application_plans assert_equal 'API', account.first_service!.name end test 'first service has a complete backend api' do - account = signup_account_manager.create(signup_params).account + account = signup_account_manager.create(**signup_params).account assert_equal 1, account.backend_apis.count assert (service = account.default_service) assert_equal 1, service.backend_apis.count @@ -138,6 +157,28 @@ class ProviderAccountManagerTest < Signup::AccountManagerTest assert_equal service.account_id, backend_api.account_id end + test "'name' is an alias for 'org_name', but the 'org_name' has priority" do + user_params = { user_params: valid_user_params } + + # only name provided + params = user_params.merge({ account_params: ActionController::Parameters.new({ name: 'name' }).permit!}) + signup_result = signup_account_manager.create(**params) + account = signup_result.account + + assert signup_result.valid? + assert signup_result.persisted? + assert_equal 'name', account.name + + # both org_name and name (in this order) provided + params = user_params.merge({ account_params: ActionController::Parameters.new({ org_name: 'org_name', name: 'name' }).permit!}) + signup_result = signup_account_manager.create(**params) + account = signup_result.account + + assert signup_result.valid? + assert signup_result.persisted? + assert_equal 'org_name', account.name + end + private def manager_account @@ -166,7 +207,7 @@ class DeveloperAccountManagerTest < Signup::AccountManagerTest signup = [] 2.times do - signup << signup_account_manager.create(signup_params) + signup << signup_account_manager.create(**signup_params) end assert signup[0].persisted? @@ -181,14 +222,14 @@ class DeveloperAccountManagerTest < Signup::AccountManagerTest end test 'create does not create correctly without username' do - signup_result = signup_account_manager.create(signup_params(different_user_params: {username: ''})) + signup_result = signup_account_manager.create(**signup_params(different_user_params: {username: ''})) refute signup_result.valid? refute signup_result.persisted? assert_match /User Username is too short/, signup_result.errors.full_messages.to_sentence end test 'create developer with the right params' do - signup_result = signup_account_manager.create(signup_params) + signup_result = signup_account_manager.create(**signup_params) account = signup_result.account # persisted correctly @@ -226,7 +267,7 @@ class DeveloperAccountManagerTest < Signup::AccountManagerTest test 'create developer without account plan approval required and minimal signup' do @account_plan.update_attribute(:approval_required, false) - signup_result = signup_account_manager.create(signup_params(different_user_params: {signup_type: :minimal})) + signup_result = signup_account_manager.create(**signup_params(different_user_params: {signup_type: :minimal})) # the user is active and the account is approved assert signup_result.persisted? @@ -237,7 +278,7 @@ class DeveloperAccountManagerTest < Signup::AccountManagerTest test 'create developer with account plan approval required and minimal signup' do # The only difference is result.user_activate_on_minimal? should return false, and it only happens if the contract_plans goes before this @account_plan.update_attribute(:approval_required, true) - signup_result = signup_account_manager.create(signup_params(different_user_params: {signup_type: :minimal})) + signup_result = signup_account_manager.create(**signup_params(different_user_params: {signup_type: :minimal})) account = signup_result.account # the user is pending and the account is created @@ -246,6 +287,48 @@ class DeveloperAccountManagerTest < Signup::AccountManagerTest assert_equal 'created', account.state end + test 'plan validation errors are added to signup result' do + # Remove default plans to trigger validation errors + manager_account.update_attribute(:default_account_plan, nil) + + signup_result = signup_account_manager.create(**signup_params) + + refute signup_result.persisted? + refute signup_result.valid? + assert_includes signup_result.errors.full_messages.join, 'Account plan is required' + end + + test 'custom signup_result_class can be used' do + custom_result_class = Class.new(Signup::Result) do + def custom_method + 'custom' + end + end + + signup_result = signup_account_manager.create(**signup_params, signup_result_class: custom_result_class) + + assert_instance_of custom_result_class, signup_result + assert_equal 'custom', signup_result.custom_method + assert signup_result.persisted? + end + + test 'transaction rollback prevents partial saves on record invalid' do + # Test that when save! raises RecordInvalid, the transaction rolls back + # and nothing is persisted + manager = signup_account_manager + result_double = manager.instance_variable_get(:@account).users.first || manager.user + + # Stub the result.save! to raise RecordInvalid + Signup::Result.any_instance.stubs(:save!).raises(ActiveRecord::RecordInvalid.new(result_double)) + + signup_result = manager.create(**signup_params) + + # Neither account nor user should be persisted due to rollback + assert_not signup_result.persisted? + assert signup_result.account.new_record? + assert signup_result.user.new_record? + end + private def manager_account @@ -263,10 +346,10 @@ class DeadlockTest < ActiveSupport::TestCase setup do @provider_account = FactoryBot.create(:provider_account) - buyer_params = { org_name: 'My company' } - user_params = { username: 'new_user', email: 'new.user@company.com', signup_type: :minimal } + buyer_params = ActionController::Parameters.new({ org_name: 'My company' }).permit! + user_params = ActionController::Parameters.new({ username: 'new_user', email: 'new.user@company.com', signup_type: :minimal }).permit! plan_defaults = { ApplicationPlan => { :name => 'API signup', :description => 'API signup', :create_origin => 'api' } } - @signup_params = Signup::SignupParams.new(account_attributes: buyer_params, user_attributes: user_params, defaults: plan_defaults) + @signup_params = { account_params: buyer_params, user_params: user_params, defaults: plan_defaults} end test 'records are correctly saved after deadlock retry' do @@ -276,7 +359,7 @@ class DeadlockTest < ActiveSupport::TestCase .raises(ActiveRecord::StatementInvalid, 'Deadlock found when trying to get lock') .then.returns(true) - result = manager.create(@signup_params) + result = manager.create(**@signup_params) assert result.persisted? @@ -292,7 +375,10 @@ class DeadlockTest < ActiveSupport::TestCase private def signup_params(different_account_params: {}, different_user_params: {}) - Signup::SignupParams.new(user_attributes: valid_user_params(different_user_params: different_user_params), account_attributes: valid_account_params(different_account_params: different_account_params)) + { + user_params: valid_user_params(different_user_params: different_user_params), + account_params: valid_account_params(different_account_params: different_account_params) + } end def valid_account_params(different_account_params: {}) diff --git a/test/unit/signup/signup_params_test.rb b/test/unit/signup/signup_params_test.rb deleted file mode 100644 index 33d6bb8998..0000000000 --- a/test/unit/signup/signup_params_test.rb +++ /dev/null @@ -1,83 +0,0 @@ -# frozen_string_literal: true - -require 'test_helper' - -module Signup - class SignupParamsTest < ActiveSupport::TestCase - test '#build_account_with_attributes_for_provider_account returns the account with the right params' do - account = signup_params.build_account_with_attributes_for_provider_account(provider_account) - assert_equal account_params[:org_name], account.org_name - assert_equal account_params[:vat_rate].to_f, account.vat_rate - assert_equal provider_account, account.provider_account - end - - test '#build_user_with_attributes_for_account returns the account with the right params' do - user = signup_params.build_user_with_attributes_for_account(account) - assert_equal user_params[:email], user.email - assert_equal user_params[:username], user.username - assert_equal user_params[:first_name], user.first_name - assert_equal user_params[:last_name], user.last_name - assert_equal user_params[:password], user.password - assert_equal user_params[:signup_type], user.signup_type - assert_equal account, user.account - end - - test '#plans returns the plans param' do - assert_equal signup_params_hash[:plans], signup_params.plans - end - - test '#defaults return the defaults param' do - assert_equal signup_params_hash[:defaults], signup_params.defaults - end - - test 'set validate_fields for account and user' do - account = signup_params.build_account_with_attributes_for_provider_account(provider_account) - user = signup_params.build_user_with_attributes_for_account(account) - assert account.fields_validations? - assert user.fields_validations? - end - - test 'extra fields are accepted' do - FactoryBot.create(:fields_definition, account: provider_account, target: 'User', name: 'created_by') - FactoryBot.create(:fields_definition, account: provider_account, target: 'Account', name: 'extra_for_account') - - user_attributes = { email: 'emailTest@email.com', username: 'john', first_name: 'John', last_name: 'Doe', - password: 'superSecret1234#', password_confirmation: 'superSecret1234#', signup_type: :minimal, 'created_by': 'hi' } - account_attributes = { org_name: 'Developer', vat_rate: 33, extra_fields: { extra_for_account: 'itWorks' } } - signup_params = Signup::SignupParams.new(user_attributes: user_attributes, account_attributes: account_attributes, plans: [], defaults: {}) - - account = signup_params.build_account_with_attributes_for_provider_account(provider_account) - user = signup_params.build_user_with_attributes_for_account(account) - - assert_equal 'hi', user.extra_fields[:created_by] - assert_equal 'itWorks', account.extra_fields[:extra_for_account] - end - - private - - def signup_params - @signup_params ||= Signup::SignupParams.new(user_attributes: user_params, account_attributes: account_params, plans: [], defaults: {}) - end - - def signup_params_hash - { user_attributes: user_params, account_attributes: account_params, plans: [], defaults: {} } - end - - def account - @account ||= Account.new(account_params) - end - - def provider_account - @provider_account ||= FactoryBot.create(:provider_account) - end - - def user_params - { email: 'emailTest@email.com', username: 'john', first_name: 'John', last_name: 'Doe', - password: 'superSecret1234#', password_confirmation: 'superSecret1234#', signup_type: :minimal } - end - - def account_params - { org_name: 'Developer', vat_rate: 33 } - end - end -end diff --git a/test/unit/user/roles_test.rb b/test/unit/user/roles_test.rb index 85c0b9bd2a..f06b9dc14d 100644 --- a/test/unit/user/roles_test.rb +++ b/test/unit/user/roles_test.rb @@ -95,14 +95,6 @@ class User::RolesTest < ActiveSupport::TestCase assert_not account.admins.first.provider_admin? end - test 'role is not mass assignable' do - user = FactoryBot.create(:user) - - assert_no_change :of => -> { user.role } do - user.update(role: :admin) - end - end - test 'role accepts also string' do user = FactoryBot.create(:user) user.role = "admin"