diff --git a/Changelog.md b/Changelog.md index 1b07344db9..f353952e93 100644 --- a/Changelog.md +++ b/Changelog.md @@ -15,6 +15,7 @@ - Display error message for student-run tests when no test groups are runnable (#7003) - Added a confirmation check while renaming a file with a different extension (#7024) - Ensure user params are passed as keyword arguments to database queries (#7040) +- Allow restricting IP addresses for remote logins (#7072) ### 🐛 Bug fixes diff --git a/app/controllers/main_controller.rb b/app/controllers/main_controller.rb index 6a352f0423..cd2ce16f7d 100644 --- a/app/controllers/main_controller.rb +++ b/app/controllers/main_controller.rb @@ -23,6 +23,14 @@ class MainController < ApplicationController def login # redirect to main page if user is already logged in. if logged_in? && !request.post? + if remote_auth? && Settings.remote_validate_file && !validate_login(request.env['HTTP_X_FORWARDED_USER'], '', + auth_type: User::AUTHENTICATE_REMOTE) + logout + flash_message(:error, I18n.t('main.external_authentication_bad_ip', + name: Settings.remote_auth_login_name || + I18n.t('main.external_authentication_default_name'))) + return + end if cookies.encrypted[:lti_data].present? lti_data = JSON.parse(cookies.encrypted[:lti_data]).symbolize_keys redirect_url = lti_data.fetch(:lti_redirect, root_url) @@ -129,18 +137,21 @@ def refresh_session # with user name "real_user" is authenticated. Effective and real users might be the # same for regular logins and are different on an assume role call. # If the login keyword is true then this method also authenticates the real_user - # - def validate_login(user_name, password) - if user_name.blank? || password.blank? + # If auth_type == User::AUTHENTICATE_LOCAL, the real_user will be authenticated against their password + # If auth_type == User::AUTHENTICATE_REMOTE, the real_user will be authenticated against their + # user_name. if Settings.validate_ip is true, the user's ip address will also be validated + def validate_login(user_name, password, auth_type: User::AUTHENTICATE_LOCAL) + if user_name.blank? || (password.blank? && auth_type == User::AUTHENTICATE_LOCAL) flash_now(:error, get_blank_message(user_name, password)) return false end - # No validate file means only remote authentication is allowed - return false unless Settings.validate_file + # Validate locally or by user_name for remote authentication. + # If there is no validate_file, only remote authentication is allowed + return false unless Settings.validate_file || Settings.remote_validate_file ip = Settings.validate_ip ? request.remote_ip : nil - authenticate_response = User.authenticate(user_name, password, ip: ip) + authenticate_response = User.authenticate(user_name, password: password, ip: ip, auth_type: auth_type) custom_status = Settings.validate_custom_status_message[authenticate_response] if authenticate_response == User::AUTHENTICATE_BAD_PLATFORM diff --git a/app/models/user.rb b/app/models/user.rb index 17fa9127b4..835924c324 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -37,10 +37,14 @@ class User < ApplicationRecord AUTHENTICATE_ERROR = 'error'.freeze AUTHENTICATE_BAD_PLATFORM = 'bad_platform'.freeze AUTHENTICATE_BAD_CHAR = 'bad_char'.freeze + AUTHENTICATE_LOCAL = 'local'.freeze + AUTHENTICATE_REMOTE = 'remote'.freeze - # Authenticates login against its password + # If auth_type == AUTHENTICATE_LOCAL: Authenticates login against its password # through a script specified by Settings.validate_file - def self.authenticate(login, password, ip: nil) + # if auth_type == AUTHENTICATE_REMOTE: Authenticates user name + # through a script specified by Settings.remote_validate_file + def self.authenticate(login, password: nil, ip: nil, auth_type: AUTHENTICATE_LOCAL) # Do not allow the following characters in usernames/passwords # Right now, this is \n and \0 only, since username and password # are delimited by \n and C programs use \0 to terminate strings @@ -62,7 +66,8 @@ def self.authenticate(login, password, ip: nil) # In general, the external password validation program should exit with 0 for success # and exit with any other integer for failure. - pipe = IO.popen("'#{Settings.validate_file}'", 'w+') # quotes to avoid choking on spaces + validate_script = auth_type == AUTHENTICATE_LOCAL ? Settings.validate_file : Settings.remote_validate_file + pipe = IO.popen("'#{validate_script}'", 'w+') # quotes to avoid choking on spaces to_stdin = [login, password, ip].compact.join("\n") pipe.puts(to_stdin) # write to stdin of Settings.validate_file pipe.close diff --git a/config/initializers/config.rb b/config/initializers/config.rb index 628c139575..f0b5b69bcb 100644 --- a/config/initializers/config.rb +++ b/config/initializers/config.rb @@ -125,6 +125,7 @@ end optional(:resque_scheduler).hash optional(:validate_file).filled(:string) + optional(:remote_validate_file).filled(:string) optional(:validate_ip).filled(:bool) required(:validate_custom_status_message).hash required(:validate_user_not_allowed_message).maybe(:string) diff --git a/config/locales/views/main/en.yml b/config/locales/views/main/en.yml index d3a4220539..2e30247fd9 100644 --- a/config/locales/views/main/en.yml +++ b/config/locales/views/main/en.yml @@ -4,6 +4,7 @@ en: cannot_role_switch: You do not have permission to role switch to this account. cannot_role_switch_to_self: You cannot role switch to your own account. create_marking_scheme: Create a Marking Scheme to display course summary graph. + external_authentication_bad_ip: Authentication with %{name} was successful, but access to MarkUs is restricted. external_authentication_default_name: remote authentication external_authentication_not_supported: External authentication not supported on your platform! external_authentication_user_not_found: Authentication with %{name} was successful but no corresponding user was found in the MarkUs database. diff --git a/spec/controllers/main_controller_spec.rb b/spec/controllers/main_controller_spec.rb index ad80f5cafc..607b5d9c66 100644 --- a/spec/controllers/main_controller_spec.rb +++ b/spec/controllers/main_controller_spec.rb @@ -83,6 +83,31 @@ expect(response).to redirect_to action: 'index', controller: 'courses' end + context 'when MarkUs is in restricted mode' do + before do + allow(Settings).to receive_messages(remote_validate_file: Rails.root + .join('spec/fixtures/files/dummy_remote_validate.sh'), + validate_ip: true) + allow_any_instance_of(ActionDispatch::Request).to receive(:remote_ip).and_return('192.168.0.1') + end + + it 'should return an error if user ip is not in the approved ip range' do + get :login + expect(flash[:error].size).to be 1 + end + + context 'with an allowed ip' do + before do + allow_any_instance_of(ActionDispatch::Request).to receive(:remote_ip).and_return('0.0.0.0') + end + + it 'should redirect to the courses index route' do + get :login + expect(response).to redirect_to action: 'index', controller: 'courses' + end + end + end + context 'going to a page that requires authentication' do before { post :logout } diff --git a/spec/fixtures/files/dummy_remote_validate.sh b/spec/fixtures/files/dummy_remote_validate.sh new file mode 100755 index 0000000000..9ee50c7501 --- /dev/null +++ b/spec/fixtures/files/dummy_remote_validate.sh @@ -0,0 +1,10 @@ +#!/bin/bash + +read user +read password +read ip + +if [ ! "$ip" == "0.0.0.0" ]; then + exit 1 + fi +exit 0 diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index fcd8768765..5545d1cbbb 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -60,26 +60,26 @@ describe '.authenticate' do context 'bad character' do it 'should not allow a null char in the username' do - expect(User.authenticate("a\0b", '123')).to eq User::AUTHENTICATE_BAD_CHAR + expect(User.authenticate("a\0b", password: '123')).to eq User::AUTHENTICATE_BAD_CHAR end it 'should not allow a null char in the password' do - expect(User.authenticate('ab', "12\0a3")).to eq User::AUTHENTICATE_BAD_CHAR + expect(User.authenticate('ab', password: "12\0a3")).to eq User::AUTHENTICATE_BAD_CHAR end it 'should not allow a newline in the username' do - expect(User.authenticate("a\nb", '123')).to eq User::AUTHENTICATE_BAD_CHAR + expect(User.authenticate("a\nb", password: '123')).to eq User::AUTHENTICATE_BAD_CHAR end it 'should not allow a newline in the password' do - expect(User.authenticate('ab', "12\na3")).to eq User::AUTHENTICATE_BAD_CHAR + expect(User.authenticate('ab', password: "12\na3")).to eq User::AUTHENTICATE_BAD_CHAR end end context 'bad platform' do it 'should not allow validation if the server OS is windows' do stub_const('RUBY_PLATFORM', 'mswin') - expect(User.authenticate('ab', '123')).to eq User::AUTHENTICATE_BAD_PLATFORM + expect(User.authenticate('ab', password: '123')).to eq User::AUTHENTICATE_BAD_PLATFORM end end @@ -91,13 +91,36 @@ context 'a successful login' do it 'should return a success message' do - expect(User.authenticate('ab', '123')).to eq User::AUTHENTICATE_SUCCESS + expect(User.authenticate('ab', password: '123')).to eq User::AUTHENTICATE_SUCCESS end end context 'an unsuccessful login' do it 'should return a failure message' do - expect(User.authenticate('exit3', '123')).to eq User::AUTHENTICATE_ERROR + expect(User.authenticate('exit3', password: '123')).to eq User::AUTHENTICATE_ERROR + end + end + + context 'with a remote validation file' do + before do + allow(Settings).to receive_messages(remote_validate_file: Rails.root + .join('spec/fixtures/files/dummy_remote_validate.sh'), + validate_ip: true) + end + + it 'should return a failure with no ip' do + expect(User.authenticate('exit3', password: '123', + auth_type: User::AUTHENTICATE_REMOTE)).to eq User::AUTHENTICATE_ERROR + end + + it 'should return a failure with a disallowed ip' do + expect(User.authenticate('exit3', password: '123', ip: '192.168.0.1', + auth_type: User::AUTHENTICATE_REMOTE)).to eq User::AUTHENTICATE_ERROR + end + + it 'should return a success with an allowed ip' do + expect(User.authenticate('exit3', password: '123', ip: '0.0.0.0', + auth_type: User::AUTHENTICATE_REMOTE)).to eq User::AUTHENTICATE_SUCCESS end end end @@ -112,25 +135,25 @@ context 'a successful login' do it 'should return a success message' do - expect(User.authenticate('ab', '123')).to eq User::AUTHENTICATE_SUCCESS + expect(User.authenticate('ab', password: '123')).to eq User::AUTHENTICATE_SUCCESS end end context 'an unsuccessful login' do it 'should return a failure message with a 1' do - expect(User.authenticate('exit1', '123')).to eq User::AUTHENTICATE_ERROR + expect(User.authenticate('exit1', password: '123')).to eq User::AUTHENTICATE_ERROR end it 'should return a failure message with a 4' do - expect(User.authenticate('exit4', '123')).to eq User::AUTHENTICATE_ERROR + expect(User.authenticate('exit4', password: '123')).to eq User::AUTHENTICATE_ERROR end it 'should return a custom message with a 2' do - expect(User.authenticate('exit2', '123')).to eq '2' + expect(User.authenticate('exit2', password: '123')).to eq '2' end it 'should return a custom message with a 3' do - expect(User.authenticate('exit3nomsg', '123')).to eq '3' + expect(User.authenticate('exit3nomsg', password: '123')).to eq '3' end end end