Revamped + Enhanced Shibboleth support #842
Conversation
| @@ -75,7 +80,7 @@ class ShibSPTest(ModuleStoreTestCase): | |||
| def setUp(self): | |||
| self.store = editable_modulestore() | |||
|
|
|||
| @unittest.skipUnless(settings.MITX_FEATURES.get('AUTH_USE_SHIB'), True) | |||
| @unittest.skipUnless(settings.MITX_FEATURES.get('AUTH_USE_SHIB'), "AUTH_USE_SHIB not set") | |||
There was a problem hiding this comment.
Heh. The 2nd argument of skipUnless is supposed to be a reason string. Lucky happenstance that this worked before.
|
@brianhw can you take a look at this PR also? |
| self.assertIsNone(get_course_enrollment_domain(self.course.id)) | ||
| self.assertEqual(self.shib_course.enrollment_domain, get_course_enrollment_domain(self.shib_course.id)) | ||
|
|
||
| @unittest.skipUnless(settings.MITX_FEATURES.get('AUTH_USE_SHIB'), "AUTH_USE_SHIB not set") |
There was a problem hiding this comment.
Do any of these tests work if SHIB is not enabled? (The difference is CMS versus LMS, right?)
There was a problem hiding this comment.
No they don't. These tests are assuming LMS (which also has external_auth as an INSTALLED_APP), mainly b/c they use the LMS URL schema. They don't run under CMS because external_auth is not an INSTALLED_APP.
Actually the implicit assumption is that SHIB is only used on the LMS side, even though it's in common, again because the login/reg handler functions in external_auth/views.py assume redirects to LMS student views.
I should put a comment somewhere. Probably in envs/common.py for both LMS and CMS.
Actually a broader question. Should external_auth be in common anymore? I can see student being in common for the models, even though the views are all LMS specific, but I don't know if the same can be said for external_auth.
There was a problem hiding this comment.
and also, the settings.MITX_FEATURES.get('AUTH_USE_SHIB') serves as a proxy for LMS vs. CMS. If there were another condition that was more directly determining LMS vs CMS tests, I might and that condition with this one.
|
I've made a pass, @jbau. Lots of great stuff here. |
|
thx for your comments @brianhw |
| """ | ||
| method_calls = mock_audit_log.method_calls | ||
| self.assertEquals(len(method_calls), 1) |
There was a problem hiding this comment.
These changes are because I added the one log item to AUDIT_LOG in student.views.login_user. Changes this to check only the last call.
* If a shib users type in their email on the regular login page, redirects them to /shib-login/ * Modify student.views.accounts_login to handle redirects generated by @login_required for courses that use shib for access control. Redirect those logins to /shib-login/?next=
* Fix open redirect vulnerability * Add Logging To AUDIT_LOG : Note I had to change existing tests that mocked AUDIT_LOG with this * Use external_auth.views.SHIBBOLETH_DOMAIN_PREFIX in student.views * Add a bunch of documentation * PEP8 / Pylint
| """ | ||
| Tests util functions in shib module | ||
| """ | ||
| def test__flatten_to_ascii(self): |
There was a problem hiding this comment.
added test for flattening to ascii.
|
@ormsbee also, any blockers remaining? I'd like merge instead of leaving this hanging so I can move on to other priorities. (cf. our previous discussion of using the regexp versus the return value of django.urlresolvers.resolve(), where I was that the return values of resolve() didn't reveal enough about whether a URL is in a "course" context without also relying on code conventions (like course_id being a view kwarg), to the point of testing with regxep being more reliable.) |
|
re-ran the tests and passed, so merging. |
Revamped + Enhanced Shibboleth support
Upgrading rocket chat version to 0.2.16
Repatriate test that was erroneously deleted.
redirects them to /shib-login/
generated by @login_required for courses that use shib for
access control.
Redirect those logins to /shib-login/?next=
@ormsbee @brianhw (whom I understand may be on vacation)