From 8b3e84ad53be361f860e3be9b9509d4567a2dc80 Mon Sep 17 00:00:00 2001 From: Dante Soares Date: Thu, 24 Oct 2024 11:16:25 -0500 Subject: [PATCH] Fix admin users controller errors (#1270) * Fix error when admin attempts to add themselves to an app they are creating * Fix application_user edits not being saved; Wrap the entire user update action into a transaction --- app/controllers/admin/users_controller.rb | 51 ++++++++++--------- .../oauth/applications_controller.rb | 41 ++++++++------- .../oauth/applications_controller_spec.rb | 8 +-- spec/factories/user.rb | 5 +- 4 files changed, 58 insertions(+), 47 deletions(-) diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index acc3ba1669..49bf5ef623 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -19,25 +19,27 @@ def search end def update - was_administrator = @user.is_administrator + User.transaction do + was_administrator = @user.is_administrator - respond_to do |format| - if change_user_password && add_email_to_user && change_salesforce_contact && update_user - security_log :user_updated_by_admin, user_id: params[:id], username: @user.username, - user_params: request.filtered_parameters['user'] + respond_to do |format| + if change_user_password && add_email_to_user && change_salesforce_contact && update_user + security_log :user_updated_by_admin, user_id: params[:id], username: @user.username, + user_params: request.filtered_parameters['user'] - security_log :admin_created, user_id: params[:id], username: @user.username \ - if @user.is_administrator && !was_administrator - security_log :admin_deleted, user_id: params[:id], username: @user.username \ - if !@user.is_administrator && was_administrator + security_log :admin_created, user_id: params[:id], username: @user.username \ + if @user.is_administrator && !was_administrator + security_log :admin_deleted, user_id: params[:id], username: @user.username \ + if !@user.is_administrator && was_administrator - security_log :trusted_launch_removed, user_id: params[:id], username: @user.username \ - if params[:user][:keep_external_uuids] == '0' + security_log :trusted_launch_removed, user_id: params[:id], username: @user.username \ + if params[:user][:keep_external_uuids] == '0' - format.html { redirect_to edit_admin_user_path(@user), - notice: 'User profile was successfully updated.' } - else - format.html { render action: "edit" } + format.html { redirect_to edit_admin_user_path(@user), + notice: 'User profile was successfully updated.' } + else + format.html { render action: "edit" } + end end end end @@ -134,18 +136,17 @@ def update_user end user_params = params.require(:user).permit(:first_name, :last_name, :username) - User.transaction do - @user.application_users = (params[:application_users]&.permit!&.to_h || {}).map do |_, au| - application_user = @user.application_users.to_a.find do |a_user| - a_user.application_id == au[:application_id].to_i - end - application_user = @user.application_users.new(application_id: au[:application_id].to_i)\ - if application_user.nil? - application_user.roles = au[:roles].split(',').map(&:strip) - application_user + @user.application_users = (params[:application_users]&.permit!&.to_h || {}).map do |_, au| + application_user = @user.application_users.to_a.find do |a_user| + a_user.application_id == au[:application_id].to_i end - @user.update! user_params + application_user = @user.application_users.new(application_id: au[:application_id].to_i)\ + if application_user.nil? + application_user.roles = au[:roles].split(',').map(&:strip) + application_user.save! + application_user end + @user.update! user_params end end end diff --git a/app/controllers/oauth/applications_controller.rb b/app/controllers/oauth/applications_controller.rb index abe9ad3671..61d8cf0697 100644 --- a/app/controllers/oauth/applications_controller.rb +++ b/app/controllers/oauth/applications_controller.rb @@ -33,31 +33,36 @@ def new def create @application = Doorkeeper::Application.new(app_params) @application.owner = Group.new - @application.owner.add_member(current_user) @application.owner.add_owner(current_user) OSU::AccessPolicy.require_action_allowed!(:create, @user, @application) Doorkeeper::Application.transaction do - if add_application_owners && @application.save - security_log :application_created, application_id: @application.id, - application_name: @application.name - flash[:notice] = I18n.t( - :notice, scope: %i[doorkeeper flash applications create] - ) - respond_to do |format| - format.html { redirect_to oauth_application_url(@application) } - format.json { render json: @application } - end - else - respond_to do |format| - format.html { render :new } - format.json do - render json: { errors: @application.errors.full_messages }, - status: :unprocessable_entity + if add_application_owners + # This has to happen after add_application_owners + # to avoid conflicts in case an admin adds themselves + @application.owner.add_member(current_user) + + if @application.save + security_log :application_created, application_id: @application.id, + application_name: @application.name + flash[:notice] = I18n.t( + :notice, scope: %i[doorkeeper flash applications create] + ) + respond_to do |format| + format.html { redirect_to oauth_application_url(@application) } + format.json { render json: @application } + end + else + respond_to do |format| + format.html { render :new } + format.json do + render json: { errors: @application.errors.full_messages }, + status: :unprocessable_entity + end end + raise ActiveRecord::Rollback end - raise ActiveRecord::Rollback end end end diff --git a/spec/controllers/oauth/applications_controller_spec.rb b/spec/controllers/oauth/applications_controller_spec.rb index df37ccc319..9b15305d26 100644 --- a/spec/controllers/oauth/applications_controller_spec.rb +++ b/spec/controllers/oauth/applications_controller_spec.rb @@ -79,7 +79,8 @@ module Oauth name: 'Some app', redirect_uri: 'https://www.example.com', can_message_users: true - } + }, + member_ids: admin.id.to_s } ) @@ -89,6 +90,7 @@ module Oauth expect(assigns(:application).name).to eq('Some app') expect(assigns(:application).redirect_uri).to eq('https://www.example.com') expect(assigns(:application).can_message_users).to eq(true) + expect(assigns(:application).owner.has_member?(admin)).to eq(true) end it "should let an admin edit someone else's application" do @@ -330,7 +332,7 @@ module Oauth member_ids: user2.id.to_s + " uteq" } ) - + id = assigns(:application).id expect(id).not_to be_nil expect(response.body).to include "Member ids must be a space separated list of integers" @@ -349,7 +351,7 @@ module Oauth member_ids: user2.id.to_s + "12345" } ) - + id = assigns(:application).id expect(id).not_to be_nil expect(response.body).to include "12345 is not a valid user id" diff --git a/spec/factories/user.rb b/spec/factories/user.rb index 21eed19623..4376283a1a 100644 --- a/spec/factories/user.rb +++ b/spec/factories/user.rb @@ -7,7 +7,10 @@ uuid {Faker::Alphanumeric.alphanumeric(number: 10, min_alpha: 3, min_numeric: 3)} role { User::STUDENT_ROLE } school { FactoryBot.build(:school) } - consent_preferences { JSON.generate({'accepted': ['functional', 'analytical'], 'rejected': ['essential', 'personalization']}) } + consent_preferences { JSON.generate({ + accepted: ['necessary', 'analytics', 'functional'], + rejected: ['advertisement', 'performance'] + }) } is_profile_complete { true }