Exam markus auth #7072

pretendWhale · 2024-05-08T18:42:59Z

Proposed Changes

(Describe your changes here. Also describe the motivation for your changes: what problem do they solve, or how do they improve the application or codebase? If this pull request fixes an open issue, use a keyword to link this pull request to the issue.)

This PR adds a Settings.remote_validate_file option. If set to a file location, anytime a user logs in with remote auth (e.g. UTORauth), the program at Settings.remote_validate_file will be run. Login will be rejected if the program's exit status is anything other than 0. This was accomplished by modifying the User.authenticate method.

Tested on a staging server with utorauth https://ben.teach.cs.toronto.edu/canvas-test/ (by directly modifying remote_validate_file to exit with a desired code).

Screenshots of your changes (if applicable)

Associated [documentation repository](https://github.com/MarkUsProject/Wiki) pull request (if applicable)

https://github.com/MarkUsProject/Wiki/pull/213

Type of Change

(Write an X or a brief description next to the type or types that best describe your changes.)

Type	Applies?
🚨 Breaking change (fix or feature that would cause existing functionality to change)
✨ New feature (non-breaking change that adds functionality)	x
🐛 Bug fix (non-breaking change that fixes an issue)
🎨 User interface change (change to user interface; provide screenshots)
♻️ Refactoring (internal change to codebase, without changing functionality)
🚦 Test update (change that only adds or modifies tests)
📦 Dependency update (change that updates a dependency)
🔧 Internal (change that only affects developers or continuous integration)

Checklist

(Complete each of the following items for your pull request. Indicate that you have completed an item by changing the [ ] into a [x] in the raw text, or by clicking on the checkbox in the rendered description on GitHub.)

Before opening your pull request:

I have performed a self-review of my changes.
- Check that all changed files included in this pull request are intentional changes.
- Check that all changes are relevant to the purpose of this pull request, as described above.
I have added tests for my changes, if applicable.
- This is required for all bug fixes and new features.
I have updated the project documentation, if applicable.
- This is required for new features.
If this is my first contribution, I have added myself to the list of contributors.

After opening your pull request:

I have updated the project Changelog (this is required for all changes).
I have verified that the pre-commit.ci checks have passed.
I have verified that the CI tests have passed.
I have reviewed the test coverage changes reported by Coveralls.
I have requested a review from a project maintainer.

Questions and Comments

(Include any questions or comments you have regarding your changes.)
I took the approach of modifying User.authenticate, but maybe it would be cleaner as a separate method?

coveralls · 2024-05-08T19:52:51Z

Pull Request Test Coverage Report for Build 9026043649

Details

47 of 48 (97.92%) changed or added relevant lines in 5 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.008%) to 91.378%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
config/initializers/config.rb	0	1	0.0%

Totals
Change from base Build 8995247780:	0.008%
Covered Lines:	39991
Relevant Lines:	43080

💛 - Coveralls

david-yz-liu

@pretendWhale good work. I left a few comments. Also whenever a new setting is added it should be reflected in the schema in config.rb.

david-yz-liu · 2024-05-09T14:31:56Z

app/models/user.rb

@@ -37,10 +37,12 @@ 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
  # through a script specified by Settings.validate_file


Please update the function documentation

david-yz-liu · 2024-05-09T14:32:55Z

app/controllers/main_controller.rb

-  def validate_login(user_name, password)
-    if user_name.blank? || password.blank?
+  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


This comment also needs to be updated

@pretendWhale it doesn't seem like you changed this comment. My point here is that the comment is now misleading because the check below isn't just checking for "no validate file".

david-yz-liu · 2024-05-09T14:33:40Z

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 this MarkUs is restricted.


remove the word "this"

david-yz-liu · 2024-05-09T14:34:00Z

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


always use correct capitalization when referring to "MarkUs"

pretendWhale · 2024-05-09T15:42:59Z

Thanks @david-yz-liu ! Addressed the comments and added remote_validate_file to config.rb.

david-yz-liu

I left one more comment. By the way, I think the overall approach of changing validate_login was totally fine 👍

david-yz-liu · 2024-05-10T00:11:14Z

app/controllers/main_controller.rb

-  def validate_login(user_name, password)
-    if user_name.blank? || password.blank?
+  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


@pretendWhale it doesn't seem like you changed this comment. My point here is that the comment is now misleading because the check below isn't just checking for "no validate file".

pretendWhale added 3 commits May 7, 2024 13:59

allow validation script for remote logins

fb97301

check for remote validation if logged in

ecd0623

add tests

36bb69e

donny-wong added this to the v2.4.10 milestone May 8, 2024

pretendWhale added 2 commits May 8, 2024 14:51

add missing validation script

2f430db

normalize i18n keys

f317cfa

Changelog: update changelog [ci skip]

7fac7b1

pretendWhale requested a review from david-yz-liu May 8, 2024 19:56

david-yz-liu reviewed May 9, 2024

View reviewed changes

update comments and respond to PR feedback

76da95e

pretendWhale requested a review from david-yz-liu May 9, 2024 15:43

david-yz-liu reviewed May 10, 2024

View reviewed changes

update misleading comment

20ffb45

pretendWhale requested a review from david-yz-liu May 10, 2024 01:51

david-yz-liu approved these changes May 10, 2024

View reviewed changes

david-yz-liu merged commit 0db831b into MarkUsProject:master May 10, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exam markus auth #7072

Exam markus auth #7072

pretendWhale commented May 8, 2024 •

edited by david-yz-liu

Loading

coveralls commented May 8, 2024 •

edited

Loading

david-yz-liu left a comment

david-yz-liu May 9, 2024

david-yz-liu May 9, 2024

david-yz-liu May 10, 2024

david-yz-liu May 9, 2024

david-yz-liu May 9, 2024

pretendWhale commented May 9, 2024

david-yz-liu left a comment

david-yz-liu May 10, 2024

Exam markus auth #7072

Exam markus auth #7072

Conversation

pretendWhale commented May 8, 2024 • edited by david-yz-liu Loading

Proposed Changes

Type of Change

Checklist

Questions and Comments

coveralls commented May 8, 2024 • edited Loading

Pull Request Test Coverage Report for Build 9026043649

Details

💛 - Coveralls

david-yz-liu left a comment

Choose a reason for hiding this comment

david-yz-liu May 9, 2024

Choose a reason for hiding this comment

david-yz-liu May 9, 2024

Choose a reason for hiding this comment

david-yz-liu May 10, 2024

Choose a reason for hiding this comment

david-yz-liu May 9, 2024

Choose a reason for hiding this comment

david-yz-liu May 9, 2024

Choose a reason for hiding this comment

pretendWhale commented May 9, 2024

david-yz-liu left a comment

Choose a reason for hiding this comment

david-yz-liu May 10, 2024

Choose a reason for hiding this comment

pretendWhale commented May 8, 2024 •

edited by david-yz-liu

Loading

coveralls commented May 8, 2024 •

edited

Loading