From 2d1e3be142e2ef65cf6a94e50337dd4be5565c55 Mon Sep 17 00:00:00 2001 From: Evan Rolfe Date: Thu, 24 Aug 2017 15:14:26 +0100 Subject: [PATCH 1/5] [webui] Drop editing LDAP user accounts --- src/api/app/controllers/person_controller.rb | 2 +- .../app/controllers/webui/user_controller.rb | 14 +++- src/api/app/models/configuration.rb | 13 ++++ src/api/app/views/webui/user/edit.html.haml | 17 ++--- src/api/app/views/webui/user/show.html.erb | 12 ++-- .../controllers/person_controller_spec.rb | 23 ++++++ .../controllers/webui/user_controller_spec.rb | 50 ++++++++++++- src/api/spec/models/configuration_spec.rb | 70 +++++++++++++++++++ 8 files changed, 182 insertions(+), 19 deletions(-) create mode 100644 src/api/spec/controllers/person_controller_spec.rb diff --git a/src/api/app/controllers/person_controller.rb b/src/api/app/controllers/person_controller.rb index 16d1b832b72..b07faebfaa5 100644 --- a/src/api/app/controllers/person_controller.rb +++ b/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 diff --git a/src/api/app/controllers/webui/user_controller.rb b/src/api/app/controllers/webui/user_controller.rb index 57aadd11112..29fb0735cf3 100644 --- a/src/api/app/controllers/webui/user_controller.rb +++ b/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.' diff --git a/src/api/app/models/configuration.rb b/src/api/app/models/configuration.rb index 0a7478db960..d53f6610398 100644 --- a/src/api/app/models/configuration.rb +++ b/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 diff --git a/src/api/app/views/webui/user/edit.html.haml b/src/api/app/views/webui/user/edit.html.haml index c78a3673be9..6ef5864084d 100644 --- a/src/api/app/views/webui/user/edit.html.haml +++ b/src/api/app/views/webui/user/edit.html.haml @@ -9,14 +9,15 @@ = form_for(@displayed_user, url: user_save_path, method: 'post') do |f| = f.hidden_field(:login) - %p - = f.label(:realname, "Name:") - %br - = f.text_field(:realname) - %p - = f.label(:email, "e-Mail:") - %br - = f.text_field(:email, required: true, email: true) + - unless @configuration.ldap_enabled? + %p + = f.label(:realname, "Name:") + %br + = f.text_field(:realname) + %p + = f.label(:email, "e-Mail:") + %br + = f.text_field(:email, required: true, email: true) %p = f.collection_check_boxes(:role_ids, Role.global, :id, :title) %p diff --git a/src/api/app/views/webui/user/show.html.erb b/src/api/app/views/webui/user/show.html.erb index 9a23958ac73..1780f396980 100644 --- a/src/api/app/views/webui/user/show.html.erb +++ b/src/api/app/views/webui/user/show.html.erb @@ -46,14 +46,14 @@ <% if @displayed_user == User.current %>

- <% 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']}" %>
+ <% if @configuration.accounts_editable? %> + <% if @account_edit_link.present? %> + <%= link_to sprited_text('user_edit', 'Edit your account'), @account_edit_link %>
+ <% else %> + <%= link_to(sprited_text('user_edit', 'Edit your account'), { :controller => 'user', :action => 'save_dialog', :user => User.current }, {:id => 'save_dialog', :remote => true}) %>
<% end %> - <% else %> - <%= link_to(sprited_text('user_edit', 'Edit your account'), { :controller => 'user', :action => 'save_dialog', :user => User.current }, {:id => 'save_dialog', :remote => true}) %>
<% 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}) %>
<% end %> <%= link_to(sprited_text('email', 'Change your notifications'), user_notifications_path) %>
diff --git a/src/api/spec/controllers/person_controller_spec.rb b/src/api/spec/controllers/person_controller_spec.rb new file mode 100644 index 00000000000..96cb68bf661 --- /dev/null +++ b/src/api/spec/controllers/person_controller_spec.rb @@ -0,0 +1,23 @@ +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 + describe 'POST #post_userinfo' do + let(:user) { create(:confirmed_user) } + + 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 +end diff --git a/src/api/spec/controllers/webui/user_controller_spec.rb b/src/api/spec/controllers/webui/user_controller_spec.rb index 54ab2d24da9..147fbf77c71 100644 --- a/src/api/spec/controllers/webui/user_controller_spec.rb +++ b/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 diff --git a/src/api/spec/models/configuration_spec.rb b/src/api/spec/models/configuration_spec.rb index 9e17f5de9e5..5d2610d868c 100644 --- a/src/api/spec/models/configuration_spec.rb +++ b/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 From fd2d8b7d2b279466e6e5e5d7a70c08e8dedfdbd8 Mon Sep 17 00:00:00 2001 From: Manuel Schnitzer Date: Tue, 22 Aug 2017 10:34:50 +0200 Subject: [PATCH 2/5] [webui] Allow admins to see realnames and email addresses in LDAP mode --- src/api/app/views/webui/user/edit.html.haml | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/api/app/views/webui/user/edit.html.haml b/src/api/app/views/webui/user/edit.html.haml index 6ef5864084d..c5a637f93ba 100644 --- a/src/api/app/views/webui/user/edit.html.haml +++ b/src/api/app/views/webui/user/edit.html.haml @@ -9,15 +9,14 @@ = form_for(@displayed_user, url: user_save_path, method: 'post') do |f| = f.hidden_field(:login) - - unless @configuration.ldap_enabled? - %p - = f.label(:realname, "Name:") - %br - = f.text_field(:realname) - %p - = f.label(:email, "e-Mail:") - %br - = f.text_field(:email, required: true, email: true) + %p + = f.label(:realname, "Name:") + %br + = f.text_field(:realname, disabled: @configuration.ldap_enabled?) + %p + = f.label(:email, "e-Mail:") + %br + = f.text_field(:email, required: true, email: true, disabled: @configuration.ldap_enabled?) %p = f.collection_check_boxes(:role_ids, Role.global, :id, :title) %p From 733defa59daa0cb5f88d41390f67b28818ca78af Mon Sep 17 00:00:00 2001 From: Manuel Schnitzer Date: Mon, 21 Aug 2017 12:14:29 +0200 Subject: [PATCH 3/5] [api] Updated controller to forbid ldap users from registering. Since d1c5b6cdd3d42e30f207040b88e829a919607ebb we shall no longer allow to edit or create users via the API. --- src/api/app/controllers/person_controller.rb | 6 ++ .../controllers/person_controller_spec.rb | 62 ++++++++++++++++++- 2 files changed, 66 insertions(+), 2 deletions(-) diff --git a/src/api/app/controllers/person_controller.rb b/src/api/app/controllers/person_controller.rb index b07faebfaa5..4ad0ab0a30c 100644 --- a/src/api/app/controllers/person_controller.rb +++ b/src/api/app/controllers/person_controller.rb @@ -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" diff --git a/src/api/spec/controllers/person_controller_spec.rb b/src/api/spec/controllers/person_controller_spec.rb index 96cb68bf661..7b4516258da 100644 --- a/src/api/spec/controllers/person_controller_spec.rb +++ b/src/api/spec/controllers/person_controller_spec.rb @@ -5,9 +5,27 @@ # CONFIG['global_write_through'] = true RSpec.describe PersonController, vcr: false do - describe 'POST #post_userinfo' do - let(:user) { create(:confirmed_user) } + let(:user) { create(:confirmed_user) } + let(:admin_user) { create(:admin_user) } + + let!(:old_realname) { user.realname } + let!(:old_email) { user.email } + + shared_examples "not allowed to change user details" do + it 'sets an error code' do + expect(response.header['X-Opensuse-Errorcode']).to eq('change_userinfo_no_permission') + end + it 'does not change users real name' do + expect(user.realname).to eq(old_realname) + end + + it 'does not change users email address' do + expect(user.email).to eq(old_email) + end + end + + describe 'POST #post_userinfo' do context 'when in LDAP mode' do before do login user @@ -20,4 +38,44 @@ end end end + + describe 'PUT #put_userinfo' do + let(:xml) { + <<-XML_DATA + + test name + test@test.de + + 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 + + context 'as an admin' do + before do + login admin_user + + put :put_userinfo, params: { login: user.login, format: :xml } + user.reload + end + + it_should_behave_like "not allowed to change user details" + end + + context 'as a user' do + before do + login user + + put :put_userinfo, params: { login: user.login, format: :xml } + user.reload + end + + it_should_behave_like "not allowed to change user details" + end + end + end end From 80343b1248d6b25e52213c9cea761e223e03a58e Mon Sep 17 00:00:00 2001 From: Evan Rolfe Date: Wed, 23 Aug 2017 13:43:49 +0100 Subject: [PATCH 4/5] [ci] Refactored spec to use rspec change operator. --- .../spec/controllers/person_controller_spec.rb | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/src/api/spec/controllers/person_controller_spec.rb b/src/api/spec/controllers/person_controller_spec.rb index 7b4516258da..5a463632f42 100644 --- a/src/api/spec/controllers/person_controller_spec.rb +++ b/src/api/spec/controllers/person_controller_spec.rb @@ -8,20 +8,18 @@ let(:user) { create(:confirmed_user) } let(:admin_user) { create(:admin_user) } - let!(:old_realname) { user.realname } - let!(:old_email) { user.email } - 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(user.realname).to eq(old_realname) + expect { subject }.not_to(change { user.realname }) end it 'does not change users email address' do - expect(user.email).to eq(old_email) + expect { subject }.not_to(change { user.email }) end end @@ -55,12 +53,11 @@ 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 - - put :put_userinfo, params: { login: user.login, format: :xml } - user.reload end it_should_behave_like "not allowed to change user details" @@ -69,9 +66,6 @@ context 'as a user' do before do login user - - put :put_userinfo, params: { login: user.login, format: :xml } - user.reload end it_should_behave_like "not allowed to change user details" From e2a445eda5d109d5264fc07bf865634e95eed5c2 Mon Sep 17 00:00:00 2001 From: Evan Rolfe Date: Wed, 23 Aug 2017 14:52:56 +0100 Subject: [PATCH 5/5] [ci][api] Disallow user password change in LDAP mode. --- src/api/app/controllers/person_controller.rb | 9 ++++++ .../controllers/person_controller_spec.rb | 30 +++++++++++++++++++ src/api/test/unit/code_quality_test.rb | 2 +- 3 files changed, 40 insertions(+), 1 deletion(-) diff --git a/src/api/app/controllers/person_controller.rb b/src/api/app/controllers/person_controller.rb index 4ad0ab0a30c..00bb89bf7f5 100644 --- a/src/api/app/controllers/person_controller.rb +++ b/src/api/app/controllers/person_controller.rb @@ -155,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}" ) diff --git a/src/api/spec/controllers/person_controller_spec.rb b/src/api/spec/controllers/person_controller_spec.rb index 5a463632f42..5b5535403c2 100644 --- a/src/api/spec/controllers/person_controller_spec.rb +++ b/src/api/spec/controllers/person_controller_spec.rb @@ -72,4 +72,34 @@ 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 diff --git a/src/api/test/unit/code_quality_test.rb b/src/api/test/unit/code_quality_test.rb index 57d1d81aa88..b5ed8dfeadd 100644 --- a/src/api/test/unit/code_quality_test.rb +++ b/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,