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

@@ -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

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

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"

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 }