Admin can change other users settings without user password #2646

app/assets/stylesheets/shared/general-bootstrap.sass

@@ -65,3 +65,6 @@
 .required:after
   content: "*"
   margin: 0 0 0 1px
+
+.not-required:after
+  content: ''

app/assets/stylesheets/shared/general.sass

@@ -57,3 +57,5 @@ body > .container
 
 .text-grey
   color: grey
+
+

app/controllers/admin/admin_users_controller.rb

@@ -11,6 +11,10 @@ class Admin::AdminUsersController < AdminController
     @admin_user = AdminUser.new
   end
 
+  def show; end
+
+  def edit; end
+
   def create
     @admin_user = AdminUser.new(admin_user_params)
 
@@ -23,12 +27,11 @@ class Admin::AdminUsersController < AdminController
     end
   end
 
-  def show; end
-
-  def edit; end
-
   def update
-    if @admin_user.update(admin_user_params)
+    params[:admin_user].delete(:password) if params[:admin_user][:password].blank?
+    params[:admin_user].delete(:password_confirmation) if params[:admin_user][:password_confirmation].blank?
+
+    if @admin_user.update_attributes(admin_user_params)
       flash[:notice] = I18n.t('record_updated')
       redirect_to [:admin, @admin_user]
     else

app/controllers/admin/api_users_controller.rb

@@ -29,6 +29,7 @@ class Admin::ApiUsersController < AdminController
   def edit; end
 
   def update
+    params[:api_user].delete(:password) if params[:api_user][:password].blank?
     if @api_user.update(api_user_params)
       flash[:notice] = I18n.t('record_updated')
       redirect_to [:admin, @api_user]

app/models/admin_user.rb

@@ -1,9 +1,10 @@
 class AdminUser < User
-  validates :username, :password, :country_code, :roles, presence: true
+  validates :username, :country_code, :roles, presence: true
   validates :identity_code, uniqueness: true, allow_blank: true
   validates :identity_code, presence: true, if: -> { country_code == 'EE' }
   validates :email, presence: true 
-
+  validates :password, :password_confirmation, presence: true, if: :new_record?
+  validates :password_confirmation, presence: true, if: :encrypted_password_changed?
   validate :validate_identity_code, if: -> { country_code == 'EE' }
 
   ROLES = %w(user customer_service admin) # should not match to api_users roles

app/views/admin/admin_users/_form.haml

@@ -11,14 +11,15 @@
       - if @admin_user.new_record? || can?(:update, AdminUser)
         .form-group
           .col-md-4.control-label
-            = f.label :password
+            - not_required = @admin_user.new_record? ? '' : 'not-required'
+            = f.label :password, class: not_required
           .col-md-8
-            = f.text_field(:password, class: 'form-control')
+            = f.password_field(:password, class: 'form-control')
         .form-group
           .col-md-4.control-label
-            = f.label :password_confirmation
+            = f.label :password_confirmation, class: not_required
           .col-md-8
-            = f.text_field(:password_confirmation, class: 'form-control')
+            = f.password_field(:password_confirmation, class: 'form-control')
 
       %hr
       .form-group
@@ -48,3 +49,8 @@
   .row
     .col-md-8.text-right
       = button_tag(t(:save), class: 'btn btn-primary')
+
+:coffee
+  $("#admin_user_password").removeAttr('required')
+  $("#admin_user_password_confirmation").removeAttr('required')
+

app/views/admin/api_users/_form.haml

@@ -1,4 +1,5 @@
-= form_for([:admin, @api_user], multipart: true, html: {class: 'form-horizontal'}) do |f|
+= form_for([:admin, @api_user], multipart: true, 
+  html: {class: 'form-horizontal', autocomplete: 'off'}) do |f|
   = render 'shared/full_errors', object: @api_user
 
   .row
@@ -10,9 +11,11 @@
           = f.text_field(:username, class: 'form-control')
       .form-group
         .col-md-4.control-label
-          = f.label :password
+          - not_required = @api_user.new_record? ? '' : 'not-required'
+          = f.label :password, class: not_required
         .col-md-7
-          = f.text_field(:password, class: 'form-control')
+          = f.text_field :password, class: 'form-control', autocomplete: 'off'
+
       .form-group
         .col-md-4.control-label
           = f.label :identity_code
@@ -48,4 +51,5 @@
       = button_tag(t(:save), class: 'btn btn-primary')
 
 :coffee
-  Autocomplete.bindAdminRegistrarSearch();
+  Autocomplete.bindAdminRegistrarSearch()
+  $("#api_user_password").removeAttr('required')

spec/fabricators/admin_user_fabricator.rb

@@ -4,7 +4,7 @@ Fabricator(:admin_user) do
   username 'gitlab'
   password 'ghyt9e4fu'
   password_confirmation 'ghyt9e4fu'
-  email 'info@gitlab.eu'
+  email { sequence(:email) { |i| "info#{i}@example.com" } }
   country_code 'FI'
   roles ['admin']
 end

spec/models/admin_user_spec.rb

@@ -4,95 +4,61 @@ require 'cancan/matchers'
 describe AdminUser do
   context 'with invalid attribute' do
     before :all do
-      @user = AdminUser.new
+      @admin_user = AdminUser.new
     end
 
     it 'should not be valid' do
-      @user.valid?
-      @user.errors.full_messages.should match_array([
+      @admin_user.valid?
+      @admin_user.errors.full_messages.should match_array([
         "Country code is missing",
         "Email Email is missing",
         "Email Email is missing",
         "Password Password is missing",
         "Password Password is missing",
+        "Password confirmation is missing",
         "Roles is missing",
         "Username Username is missing"
       ])
     end
 
     it 'should not have any versions' do
-      @user.versions.should == []
+      @admin_user.versions.should == []
     end
   end
 
   context 'with valid attributes' do
     before :all do
-      @user = Fabricate(:admin_user)
+      @admin_user = Fabricate(:admin_user)
     end
 
     it 'should be valid' do
-      @user.valid?
-      @user.errors.full_messages.should match_array([])
+      @admin_user.valid?
+      @admin_user.errors.full_messages.should match_array([])
     end
 
-    # it 'should be valid twice' do
-      # @user = Fabricate(:admin_user)
-      # @user.valid?
-      # @user.errors.full_messages.should match_array([])
-    # end
+    it 'should be valid twice' do
+      @admin_user = Fabricate(:admin_user)
+      @admin_user.valid?
+      @admin_user.errors.full_messages.should match_array([])
+    end
 
-    # it 'should have one version' do
-      # with_versioning do
-        # @user.versions.should == []
-        # @user.zip = 'New zip'
-        # @user.save
-        # @user.errors.full_messages.should match_array([])
-        # @user.versions.size.should == 1
-      # end
-    # end
+    it 'should have one version' do
+      with_versioning do
+        @admin_user.versions.should == []
+        @admin_user.updated_at = Time.zone.now
+        @admin_user.save
+        @admin_user.errors.full_messages.should match_array([])
+        @admin_user.versions.size.should == 1
+      end
+    end
+
+    it 'should require password confirmation when changing password' do
+      @admin_user.valid?.should == true
+      @admin_user.password = 'not confirmed'
+      @admin_user.valid?
+      @admin_user.errors.full_messages.should match_array([
+        "Password confirmation doesn't match Password"
+      ])
+    end
   end
-
-  # describe 'abilities' do
-    # subject(:ability) { Ability.new(user) }
-    # let(:user) { nil }
-
-    # context 'when user is admin' do
-      # let(:user) { Fabricate(:admin_user) }
-
-      # it { should be_able_to(:manage, Domain.new) }
-      # it { should be_able_to(:manage, Contact.new) }
-      # it { should be_able_to(:manage, Registrar.new) }
-      # it { should be_able_to(:manage, Setting.new) }
-      # it { should be_able_to(:manage, ZonefileSetting.new) }
-      # it { should be_able_to(:manage, DomainVersion.new) }
-      # it { should be_able_to(:manage, User.new) }
-      # it { should be_able_to(:manage, ApiUser.new) }
-      # it { should be_able_to(:manage, Keyrelay.new) }
-      # it { should be_able_to(:manage, LegalDocument.new) }
-      # it { should be_able_to(:read, ApiLog::EppLog.new) }
-      # it { should be_able_to(:read, ApiLog::ReppLog.new) }
-      # it { should be_able_to(:index, :delayed_job) }
-      # it { should be_able_to(:create, :zonefile) }
-      # it { should be_able_to(:access, :settings_menu) }
-    # end
-
-    # context 'when user is customer service' do
-      # let(:user) { Fabricate(:user, roles: ['customer_service']) }
-
-      # it { should be_able_to(:manage, Domain.new) }
-      # it { should be_able_to(:manage, Contact.new) }
-      # it { should be_able_to(:manage, Registrar.new) }
-      # it { should_not be_able_to(:manage, Setting.new) }
-      # it { should_not be_able_to(:manage, ZonefileSetting.new) }
-      # it { should_not be_able_to(:manage, DomainVersion.new) }
-      # it { should_not be_able_to(:manage, User.new) }
-      # it { should_not be_able_to(:manage, ApiUser.new) }
-      # it { should_not be_able_to(:manage, LegalDocument.new) }
-      # it { should_not be_able_to(:read, ApiLog::EppLog.new) }
-      # it { should_not be_able_to(:read, ApiLog::ReppLog.new) }
-      # it { should_not be_able_to(:index, :delayed_job) }
-      # it { should_not be_able_to(:create, :zonefile) }
-      # it { should_not be_able_to(:access, :settings_menu) }
-    # end
-  # end
 end