Merge pull request #3671 from openSUSE/drop_ldap_user_management

Drop ldap user management

src/api/app/controllers/person_controller.rb

@@ -51,7 +51,7 @@ def post_userinfo
     if params[:cmd] == "change_password"
       login ||= @http_user.login
       password = request.raw_post.to_s.chomp
-      if login != @http_user.login && !@http_user.is_admin?
+      if (login != User.current.login && !User.current.is_admin?) || !::Configuration.passwords_changable?
         render_error status: 403, errorcode: "change_password_no_permission",
                      message: "No permission to change password for user #{login}"
         return
@@ -87,6 +87,12 @@ def put_userinfo
     login = params[:login]
     user = User.find_by_login(login) if login
 
+    unless ::Configuration.accounts_editable?
+      render_error(status: 403, errorcode: 'change_userinfo_no_permission',
+                   message: "no permission to change userinfo for user #{user.login}")
+      return
+    end
+
     if user
       unless user.login == User.current.login || User.current.is_admin?
         logger.debug "User has no permission to change userinfo"
@@ -149,6 +155,15 @@ class NoPermission < APIException
   end
 
   def internal_register
+    if ::Configuration.ldap_enabled?
+      render_error(
+        status: 403,
+        errorcode: 'permission_denied',
+        message: "User accounts can not be registered via OBS when in LDAP mode. Please refer to your LDAP server to create new users."
+      )
+      return
+    end
+
     xml = REXML::Document.new( request.raw_post )
 
     logger.debug( "register XML: #{request.raw_post}" )

src/api/app/controllers/webui/user_controller.rb

@@ -60,6 +60,7 @@ def show
     if User.current == @displayed_user
       @reviews = @displayed_user.involved_reviews.exists?
       @patchinfos = @displayed_user.involved_patchinfos
+      @account_edit_link = CONFIG['proxy_auth_account_page']
     end
   end
 
@@ -114,14 +115,17 @@ def save
     @displayed_user = User.find_by_login(params[:user][:login])
 
     unless User.current.is_admin?
-      if User.current != @displayed_user
+      if User.current != @displayed_user || !@configuration.accounts_editable?
         flash[:error] = "Can't edit #{@displayed_user.login}"
         redirect_back(fallback_location: root_path)
         return
       end
     end
 
-    @displayed_user.assign_attributes(params[:user].slice(:realname, :email).permit!)
+    if @configuration.accounts_editable?
+      @displayed_user.assign_attributes(params[:user].slice(:realname, :email).permit!)
+    end
+
     if User.current.is_admin?
       @displayed_user.assign_attributes(params[:user].slice(:state).permit!)
       @displayed_user.update_globalroles(Role.global.where(id: params[:user][:role_ids])) unless params[:user][:role_ids].nil?
@@ -226,6 +230,12 @@ def password_dialog
   end
 
   def change_password
+    unless @configuration.passwords_changable?
+      flash[:error] = "You're not authorized to change your password."
+      redirect_back fallback_location: root_path
+      return
+    end
+
     # check the valid of the params
     unless User.current.password_equals?(params[:password])
       errmsg = 'The value of current password does not match your current password. Please enter the password and try again.'

src/api/app/models/configuration.rb

@@ -72,6 +72,19 @@ def ldapgroup_enabled?
   end
   # End of class methods
 
+  def ldap_enabled?
+    CONFIG['ldap_mode'] == :on
+  end
+
+  def passwords_changable?
+    change_password && CONFIG['proxy_auth_mode'] != :on && CONFIG['ldap_mode'] != :on
+  end
+
+  def accounts_editable?
+    (CONFIG['proxy_auth_mode'] != :on || (CONFIG['proxy_auth_mode'] == :on && !CONFIG['proxy_auth_account_page'].blank?)) && \
+      CONFIG['ldap_mode'] != :on
+  end
+
   def update_from_options_yml
     # strip the not set ones
     attribs = ::Configuration::OPTIONS_YML.clone

src/api/app/views/webui/user/edit.html.haml

@@ -12,11 +12,11 @@
   %p
     = f.label(:realname, "Name:")
     %br
-    = f.text_field(:realname)
+    = f.text_field(:realname, disabled: @configuration.ldap_enabled?)
   %p
     = f.label(:email, "e-Mail:")
     %br
-    = f.text_field(:email, required: true, email: true)
+    = f.text_field(:email, required: true, email: true, disabled: @configuration.ldap_enabled?)
   %p
     = f.collection_check_boxes(:role_ids, Role.global, :id, :title)
   %p

src/api/app/views/webui/user/show.html.erb

@@ -46,14 +46,14 @@
 <!-- edit -->
     <% if @displayed_user == User.current %>
       <p>
-      <% if CONFIG['proxy_auth_mode'] == :on %>
-        <% unless CONFIG['proxy_auth_account_page'].blank? %>
-          <%= link_to sprited_text('user_edit', 'Edit your account'), "#{CONFIG['proxy_auth_account_page']}" %><br />
+      <% if @configuration.accounts_editable? %>
+        <% if @account_edit_link.present? %>
+          <%= link_to sprited_text('user_edit', 'Edit your account'), @account_edit_link %><br />
+        <% else %>
+          <%= link_to(sprited_text('user_edit', 'Edit your account'), { :controller => 'user', :action => 'save_dialog', :user => User.current }, {:id => 'save_dialog', :remote => true}) %><br />
         <% end %>
-      <% else %>
-        <%= link_to(sprited_text('user_edit', 'Edit your account'), { :controller => 'user', :action => 'save_dialog', :user => User.current }, {:id => 'save_dialog', :remote => true}) %><br />
       <% end %>
-      <% if @configuration['change_password'] && CONFIG['proxy_auth_mode'] != :on %>
+      <% if @configuration.passwords_changable? %>
         <%= link_to(sprited_text('key', 'Change your password'), { :controller => 'user', :action => 'password_dialog', :user => User.current }, {:id => 'password_dialog', :remote => true}) %><br />
       <% end %>
       <%= link_to(sprited_text('email', 'Change your notifications'), user_notifications_path) %><br />

src/api/spec/controllers/person_controller_spec.rb

@@ -0,0 +1,105 @@
+require 'rails_helper'
+# WARNING: If you change tests make sure you uncomment this line
+# and start a test backend. Some of the actions
+# require real backend answers for projects/packages.
+# CONFIG['global_write_through'] = true
+
+RSpec.describe PersonController, vcr: false do
+  let(:user) { create(:confirmed_user) }
+  let(:admin_user) { create(:admin_user) }
+
+  shared_examples "not allowed to change user details" do
+    it 'sets an error code' do
+      subject
+      expect(response.header['X-Opensuse-Errorcode']).to eq('change_userinfo_no_permission')
+    end
+
+    it 'does not change users real name' do
+      expect { subject }.not_to(change { user.realname })
+    end
+
+    it 'does not change users email address' do
+      expect { subject }.not_to(change { user.email })
+    end
+  end
+
+  describe 'POST #post_userinfo' do
+    context 'when in LDAP mode' do
+      before do
+        login user
+        stub_const('CONFIG', CONFIG.merge({ 'ldap_mode' => :on }))
+        post :post_userinfo, params: { login: user.login, cmd: 'change_password', format: :xml }
+      end
+
+      it 'user is not allowed to change their password' do
+        expect(response.header['X-Opensuse-Errorcode']).to eq('change_password_no_permission')
+      end
+    end
+  end
+
+  describe 'PUT #put_userinfo' do
+    let(:xml) {
+      <<-XML_DATA
+        <userinfo>
+          <realname>test name</realname>
+          <email>test@test.de</email>
+        </userinfo>
+      XML_DATA
+    }
+
+    context 'when in LDAP mode' do
+      before do
+        stub_const('CONFIG', CONFIG.merge({ 'ldap_mode' => :on }))
+        request.env["RAW_POST_DATA"] = xml
+      end
+
+      subject { put :put_userinfo, params: { login: user.login, format: :xml } }
+
+      context 'as an admin' do
+        before do
+          login admin_user
+        end
+
+        it_should_behave_like "not allowed to change user details"
+      end
+
+      context 'as a user' do
+        before do
+          login user
+        end
+
+        it_should_behave_like "not allowed to change user details"
+      end
+    end
+  end
+
+  describe 'POST #register' do
+    context 'when in LDAP mode' do
+      before do
+        stub_const('CONFIG', CONFIG.merge({ 'ldap_mode' => :on }))
+      end
+
+      subject! { post :register }
+
+      it 'sets an error code' do
+        expect(response.header['X-Opensuse-Errorcode']).to eq('permission_denied')
+      end
+    end
+  end
+
+  describe 'POST #command' do
+    context 'with param cmd = register' do
+      context 'when in LDAP mode' do
+        before do
+          stub_const('CONFIG', CONFIG.merge({ 'ldap_mode' => :on }))
+        end
+
+        subject! { post :command, params: { cmd: 'register' } }
+
+        it 'sets an error code' do
+          expect(response.header['X-Opensuse-Errorcode']).to eq('permission_denied')
+        end
+      end
+    end
+  end
+end

src/api/spec/controllers/webui/user_controller_spec.rb

@@ -229,6 +229,43 @@
         expect(user.roles).to match_array old_global_role
       end
     end
+
+    context "when LDAP mode is enabled" do
+      let!(:old_realname) { user.realname }
+      let!(:old_email) { user.email }
+      let(:http_request) {
+        post :save, params: { user: { login: user.login, realname: 'another real name', email: 'new_valid@email.es' } }
+      }
+
+      before do
+        stub_const('CONFIG', CONFIG.merge({ 'ldap_mode' => :on }))
+      end
+
+      describe "as an admin user" do
+        before do
+          login admin_user
+
+          http_request
+          user.reload
+        end
+
+        it { expect(user.realname).to eq(old_realname) }
+        it { expect(user.email).to eq(old_email) }
+      end
+
+      describe "as a user" do
+        before do
+          login user
+
+          http_request
+          user.reload
+        end
+
+        it { expect(controller).to set_flash[:error] }
+        it { expect(user.realname).to eq(old_realname) }
+        it { expect(user.email).to eq(old_email) }
+      end
+    end
   end
 
   describe "GET #delete" do
@@ -301,8 +338,17 @@
     skip
   end
 
-  describe "GET #change_password" do
-    skip
+  describe "POST #change_password" do
+    before do
+      login non_admin_user
+
+      stub_const('CONFIG', CONFIG.merge({ 'ldap_mode' => :on }))
+      post :change_password
+    end
+
+    it 'shows an error message when in LDAP mode' do
+      expect(controller).to set_flash[:error]
+    end
   end
 
   describe "GET #autocomplete" do

src/api/spec/models/configuration_spec.rb

@@ -18,4 +18,74 @@
     Configuration.title
     expect(Configuration.count).to eq(1)
   end
+
+  describe '#ldap_enabled?' do
+    let(:config) { Configuration.first }
+
+    it 'returns true if config option `ldap_mode` is set to :on' do
+      stub_const('CONFIG', CONFIG.merge({ 'ldap_mode' => :on }))
+      expect(config.ldap_enabled?).to eq(true)
+    end
+
+    it 'returns false if config option `ldap_mode` is not set to :on' do
+      stub_const('CONFIG', CONFIG.merge({ 'ldap_mode' => :off }))
+      expect(config.ldap_enabled?).to eq(false)
+    end
+  end
+
+  describe '#passwords_changable?' do
+    let(:config) { Configuration.first }
+
+    it 'returns false if config option `change_password` is set to false' do
+      allow(config).to receive(:change_password).and_return(false)
+      expect(config.passwords_changable?).to eq(false)
+    end
+
+    context 'external authentication services' do
+      it 'returns false if config option `proxy_auth_mode` is set to :on' do
+        stub_const('CONFIG', CONFIG.merge({ 'proxy_auth_mode' => :on }))
+        expect(config.passwords_changable?).to eq(false)
+      end
+
+      it 'returns false if config option `ldap_mode` is set to :on' do
+        stub_const('CONFIG', CONFIG.merge({ 'ldap_mode' => :on }))
+        expect(config.passwords_changable?).to eq(false)
+      end
+    end
+
+    context 'without external authentication services' do
+      it 'returns true' do
+        expect(config.passwords_changable?).to eq(true)
+      end
+    end
+  end
+
+  describe '#accounts_editable?' do
+    let(:config) { Configuration.first }
+
+    context 'proxy_auth_mode is enabled' do
+      before do
+        stub_const('CONFIG', CONFIG.merge({ 'proxy_auth_mode' => :on }))
+      end
+
+      it 'returns false if proxy_auth_account_page is not present' do
+        expect(config.accounts_editable?).to eq(false)
+      end
+
+      it 'returns true if proxy_auth_account_page is present' do
+        stub_const('CONFIG', CONFIG.merge({ 'proxy_auth_account_page' => 'https://opensuse.org' }))
+        expect(config.accounts_editable?).to eq(true)
+      end
+    end
+
+    context 'ldap_mode is enabled' do
+      before do
+        stub_const('CONFIG', CONFIG.merge({ 'ldap_mode' => :on }))
+      end
+
+      it 'returns false' do
+        expect(config.accounts_editable?).to eq(false)
+      end
+    end
+  end
 end

src/api/test/unit/code_quality_test.rb

@@ -82,7 +82,7 @@ def setup
       'ConfigurationsController#update'                                         => 82.1,
       'IssueTrackersController#update'                                          => 100.78,
       'MaintenanceHelper#instantiate_container'                                 => 163.57,
-      'PersonController#internal_register'                                      => 112.01,
+      'PersonController#internal_register'                                      => 115.01,
       'Package#find_changed_issues'                                             => 93.74,
       'Package#close_requests'                                                  => 84.82,
       'Flag#compute_status'                                                     => 145.55,