Use audit logger for logging of logins in external_auth... #511
Conversation
|
@jarv You also asked to review this. |
| @@ -93,6 +93,13 @@ def test_exception_shib_login(self): | |||
| self.assertEqual(no_idp_response.status_code, 403) | |||
| self.assertIn("identity server did not return your ID information", no_idp_response.content) | |||
|
|
|||
| def _assert_shib_login_is_logged(self, audit_log_call, remote_user): | |||
| """Asserts that shibboleth login attempt is being logged""" | |||
| name, args, _kwargs = audit_log_call | |||
There was a problem hiding this comment.
perhaps call this fn_name rather than name, just to make it clearer? I found the assertion confusing.
|
Only other thing I can think of is to check w/ Studio (@cahrens?) about logging of login attempts via the django admin interface. |
|
👍 |
|
For now, I have added handlers for the user_logged_in and user_logged_out signals. This covers the case of successful login/logout through the admin console (in studio), as well as the regular LMS calls to login and logout. |
…uth and student apps. Move test_login to student app. Improve conditional tests for Shibboleth login logic. (Does not include reconfiguring log settings.)
|
@jarv This no longer includes the audit log definition. That has been separated out into PR 539 so that this can be merged to master without losing any logging. The configuration can then be put in place for the separate "audit" log file by devops, and then PR 539 can be committed. |
|
@dianakhuang Can you please review this as well. |
| @@ -1087,7 +1098,7 @@ def change_email_request(request): | |||
| subject = ''.join(subject.splitlines()) | |||
| message = render_to_string('emails/email_change.txt', d) | |||
|
|
|||
| res = send_mail(subject, message, settings.DEFAULT_FROM_EMAIL, [pec.new_email]) | |||
| _res = send_mail(subject, message, settings.DEFAULT_FROM_EMAIL, [pec.new_email]) | |||
There was a problem hiding this comment.
Since it doesn't seem like we're using these variables anywhere, was there a reason why you just added underscores as opposed to removing them?
|
That was my only comment. Otherwise, 👍 If you want to merge this first, I can handle the merge conflicts on my branch. |
Use audit logger for logging of logins in external_auth...
Forum refactoring/fixes
…visioning Automatic account provisioning
…cms-env-for-load-tab Mod cms env for loading InstructorDashboardTab openedx#434
…ent-ids-studio Display unobtrusive component locations in studio
Part 2 of REV-2130
... and student apps. Move test_login to student app. Improve conditional tests for Shibboleth login logic.
@cpennington
@sefk , can someone from edX West review this in @jbau 's absence? The logging calls themselves should have little effect, but this PR also includes changes to the Shibboleth-specific conditional logic (to test the external domain rather than just relying on a setting). Jason acknowledged one of these changes, but there are a few others. It would be good to get a manual test of the branch to confirm that the Shibboleth code paths still work.