Merge pull request #7074 from donny-wong/version_2.4.10

Version 2.4.10

.pre-commit-config.yaml

@@ -42,7 +42,8 @@ repos:
           (?x)^(
             db/migrate/.*|
             db/schema.rb|
-            lib/repo/test/.*
+            lib/repo/test/.*|
+            Vagrantfile
           )$
         additional_dependencies:
           - rubocop-rails:2.19.1

.rubocop.yml

@@ -7,6 +7,7 @@ AllCops:
     - 'db/migrate/.*'
     - 'db/schema.rb'
     - 'lib/repo/test/.*'
+    - 'Vagrantfile'
   NewCops: enable
   SuggestExtensions: false
   TargetRubyVersion: 2.7

Changelog.md

@@ -1,5 +1,11 @@
 # Changelog
 
+## [v2.4.10]
+
+### ✨ New features and improvements
+
+- Allow restricting IP addresses for remote logins (#7072)
+
 ## [v2.4.9]
 - Peer review table bug fix to display total marks (#7034)
 - Fix bug preventing deletion of autotest files (#7033)

app/MARKUS_VERSION

@@ -1 +1 @@
-VERSION=v2.4.9,PATCH_LEVEL=DEV
+VERSION=v2.4.10,PATCH_LEVEL=DEV

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

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

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)

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.

spec/controllers/main_controller_spec.rb

@@ -67,6 +67,30 @@
         get :login
         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 }
         it 'should respond with redirect' do

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

spec/models/user_spec.rb

@@ -57,22 +57,22 @@
   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 username' do
-        expect(User.authenticate('ab', "12\na3")).to eq User::AUTHENTICATE_BAD_CHAR
+      it 'should not allow a newline in the password' do
+        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
     context 'without a custom exit status messages' do
@@ -82,12 +82,35 @@
       end
       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
@@ -99,21 +122,21 @@
       end
       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