Skip to content

Commit

Permalink
Merge pull request #602 from ubc/fix-unicode-auth-issue
Browse files Browse the repository at this point in the history 
Handle unicode in cas and lti membership
  • Loading branch information
@xcompass
xcompass authored Sep 26, 2017
2 parents 5589eb3 + d97495c commit 37da211
Show file tree
Hide file tree
Showing 3 changed files with 53 additions and 36 deletions.
21 changes: 18 additions & 3 deletions compair/cas.py
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# -*- coding: utf-8 -*-
from __future__ import unicode_literals
from flask import current_app, url_for
from caslib import SAMLClient, CASClient
from caslib import SAMLClient, CASClient, CASResponse
from xml.dom.minidom import parseString
import requests

Expand All @@ -18,7 +20,7 @@ def _get_client():
auth_prefix=auth_prefix
)
else:
return CASClient(
return CustomCASClient(
server_url=server_url,
service_url=service_url,
auth_prefix=auth_prefix
Expand All @@ -36,14 +38,27 @@ def get_cas_logout_url():
return _get_client()._logout_url(logout_service_url)


class CustomCASClient(CASClient):
def get_cas_response(self, url):
try:
# overwritten to allow development environment to use self signed certificates
verify = current_app.config.get('ENFORCE_SSL', True)
response = requests.get(url, verify=verify)
response_text = response.text.encode('utf-8') if response.text else None
return CASResponse(response_text)
except Exception:
current_app.logging.exception("CASLIB: Error retrieving a response")
return None


class CustomSAMLClient(SAMLClient):
def get_saml_response(self, url, envelope):
try:
# overwritten to allow development environment to use self signed certificates
verify = current_app.config.get('ENFORCE_SSL', True)
response = requests.post(url, data=envelope, verify=verify)
return CustomSAMLResponse(response.text)
response_text = response.text.encode('utf-8') if response.text else None
return CustomSAMLResponse(response_text)
except Exception:
current_app.logger.error("SAML: Error retrieving a response")
raise
Expand Down
2 changes: 1 addition & 1 deletion compair/models/lti_models/lti_membership.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,7 @@ def _get_membership_ext(cls, lti_context):
params = parse_qs(signed_request.body.decode('utf-8'))

data = LTIMembership._post_membership_request(memberships_url, params)
root = ElementTree.fromstring(data)
root = ElementTree.fromstring(data.encode('utf-8'))

codemajor = root.find('statusinfo/codemajor')
if codemajor is not None and codemajor.text in ['Failure', 'Unsupported']:
Expand Down
66 changes: 34 additions & 32 deletions compair/tests/api/test_lti_launch.py
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
# -*- coding: utf-8 -*-
from __future__ import unicode_literals
import json
import mock

Expand Down Expand Up @@ -698,7 +700,7 @@ def test_lti_course_link_with_membership_ext(self, mocked_post_membership_reques
<roles>Learner</roles>
</member>
<member>
<user_id>compair_student_3</user_id>
<user_id>compair_student_3è</user_id>
<roles>TeachingAssistant</roles>
</member>
<member>
Expand Down Expand Up @@ -731,7 +733,7 @@ def test_lti_course_link_with_membership_ext(self, mocked_post_membership_reques
self.assertEqual(len(lti_memberships), 5)
for lti_membership in lti_memberships:
self.assertIn(lti_membership.lti_user.user_id, [lti_user.user_id, "compair_student_1", "compair_student_2",
"compair_student_3", "compair_instructor_2"])
"compair_student_3è", "compair_instructor_2"])

# test successful membership response (with user_id_override)
with self.lti_launch(lti_consumer, lti_resource_link.resource_link_id,
Expand Down Expand Up @@ -765,9 +767,9 @@ def test_lti_course_link_with_membership_ext(self, mocked_post_membership_reques
<custom_puid>compair_student_2</custom_puid>
</member>
<member>
<user_id>ignore_4</user_id>
<user_id>ignore_4è</user_id>
<roles>TeachingAssistant</roles>
<custom_puid>compair_student_3</custom_puid>
<custom_puid>compair_student_3è</custom_puid>
</member>
<member>
<user_id>ignore_5</user_id>
Expand Down Expand Up @@ -800,7 +802,7 @@ def test_lti_course_link_with_membership_ext(self, mocked_post_membership_reques
self.assertEqual(len(lti_memberships), 5)
for lti_membership in lti_memberships:
self.assertIn(lti_membership.lti_user.user_id, [lti_user.user_id, "compair_student_1", "compair_student_2",
"compair_student_3", "compair_instructor_2"])
"compair_student_3è", "compair_instructor_2"])


@mock.patch('compair.models.lti_models.lti_membership.LTIMembership._get_membership_request')
Expand Down Expand Up @@ -903,7 +905,7 @@ def minimal_membership_requests(memberships_url, headers=None):
"urn:lti:role:ims/lis/TeachingAssistant"
],
"member":{
"userId":"compair_student_3"
"userId":"compair_student_3è"
}
},
{
Expand Down Expand Up @@ -995,11 +997,11 @@ def minimal_membership_requests(memberships_url, headers=None):
"urn:lti:role:ims/lis/TeachingAssistant"
],
"member":{
"userId":"compair_student_3"
"userId":"compair_student_3è"
},
"message": [{
"message_type": "basic-lti-launch-request",
"lis_result_sourcedid": "lis_result_sourcedid_compair_student_3"
"lis_result_sourcedid": "lis_result_sourcedid_compair_student_3è"
},{
"message_type": "other-message-type"
}]
Expand Down Expand Up @@ -1076,7 +1078,7 @@ def minimal_membership_requests(memberships_url, headers=None):
self.assertEqual(len(lti_memberships), 5)
for lti_membership in lti_memberships:
self.assertIn(lti_membership.lti_user.user_id, [lti_user.user_id, "compair_student_1", "compair_student_2",
"compair_student_3", "compair_instructor_2"])
"compair_student_3è", "compair_instructor_2"])

# ensure the lti_user_resource_link is generated and stores the lis_result_sourcedid
lti_user_resource_links = [lti_user_resource_link \
Expand Down Expand Up @@ -1180,10 +1182,10 @@ def user_id_override_membership_requests(memberships_url, headers=None):
"message_type": "basic-lti-launch-request",
"lis_result_sourcedid": "1234567890-3",
"custom" : {
"puid": "compair_student_3_puid"
"puid": "compair_student_3è_puid"
},
"ext" : {
"user_username": "compair_student_3_username",
"user_username": "compair_student_3è_username",
}
}
]
Expand Down Expand Up @@ -1324,12 +1326,12 @@ def user_id_override_membership_requests(memberships_url, headers=None):
"message" : [
{
"message_type": "basic-lti-launch-request",
"lis_result_sourcedid": "lis_result_sourcedid_compair_student_3",
"lis_result_sourcedid": "lis_result_sourcedid_compair_student_3è",
"custom" : {
"puid": "compair_student_3_puid"
"puid": "compair_student_3è_puid"
},
"ext" : {
"user_username": "compair_student_3_username",
"user_username": "compair_student_3è_username",
}
}
]
Expand Down Expand Up @@ -1420,10 +1422,10 @@ def user_id_override_membership_requests(memberships_url, headers=None):
self.assertIn(lti_membership.lti_user.user_id, [lti_user.user_id, "userId_2", "userId_3", "userId_4", "userId_5"])
elif user_id_override == "custom_puid":
self.assertIn(lti_membership.lti_user.user_id, [lti_user.user_id, "compair_student_1_puid", "compair_student_2_puid",
"compair_student_3_puid", "compair_instructor_2_puid"])
"compair_student_3è_puid", "compair_instructor_2_puid"])
elif user_id_override == "ext_user_username":
self.assertIn(lti_membership.lti_user.user_id, [lti_user.user_id, "compair_student_1_username", "compair_student_2_username",
"compair_student_3_username", "compair_instructor_2_username"])
"compair_student_3è_username", "compair_instructor_2_username"])
elif user_id_override == "lis_result_sourcedid":
self.assertIn(lti_membership.lti_user.user_id, [lti_user.user_id, "1234567890-1", "1234567890-2",
"1234567890-3", "1234567890-4"])
Expand Down Expand Up @@ -1503,13 +1505,13 @@ def paginated_membership_requests(memberships_url, headers=None):
elif memberships_url == "https://mockmembershipurl.com?page=4&per_page=1":
result['nextPage'] = "https://mockmembershipurl.com?page=5&per_page=1"
result['pageOf']['membershipSubject']['membership'][0]['role'] = ["urn:lti:role:ims/lis/TeachingAssistant"]
result['pageOf']['membershipSubject']['membership'][0]['member']['userId'] = "compair_student_3"
result['pageOf']['membershipSubject']['membership'][0]['member']['userId'] = "compair_student_3è"

elif memberships_url == "https://mockmembershipurl.com?rlid="+rlid+"&page=4&per_page=1":
result['nextPage'] = "https://mockmembershipurl.com?rlid="+rlid+"&page=5&per_page=1"
result['pageOf']['membershipSubject']['membership'][0]['role'] = ["urn:lti:role:ims/lis/TeachingAssistant"]
result['pageOf']['membershipSubject']['membership'][0]['member']['userId'] = "compair_student_3"
result_launch_message['lis_result_sourcedid'] = "lis_result_sourcedid_compair_student_3"
result['pageOf']['membershipSubject']['membership'][0]['member']['userId'] = "compair_student_3è"
result_launch_message['lis_result_sourcedid'] = "lis_result_sourcedid_compair_student_3è"
result['pageOf']['membershipSubject']['membership'][0]['message'] = [result_launch_message]

elif memberships_url == "https://mockmembershipurl.com?page=5&per_page=1":
Expand Down Expand Up @@ -1573,7 +1575,7 @@ def paginated_membership_requests(memberships_url, headers=None):
self.assertEqual(len(lti_memberships), 5)
for lti_membership in lti_memberships:
self.assertIn(lti_membership.lti_user.user_id, [lti_user.user_id, "compair_student_1", "compair_student_2",
"compair_student_3", "compair_instructor_2"])
"compair_student_3è", "compair_instructor_2"])

@mock.patch('compair.models.lti_models.lti_membership.LTIMembership._post_membership_request')
def test_lti_membership_for_consumer_with_membership_ext(self, mocked_post_membership_request):
Expand Down Expand Up @@ -1694,11 +1696,11 @@ def test_lti_membership_for_consumer_with_membership_ext(self, mocked_post_membe
<lis_result_sourcedid>:_676_1::compai:compair_student_2</lis_result_sourcedid>
</member>
<member>
<user_id>compair_student_3</user_id>
<user_id>compair_student_3è</user_id>
<user_image>http://www.gravatar.com/avatar/4</user_image>
<roles>TeachingAssistant</roles>
<person_sourcedid>compair_student_3</person_sourcedid>
<person_contact_email_primary>compair_student_3@test.com</person_contact_email_primary>
<person_sourcedid>compair_student_3è</person_sourcedid>
<person_contact_email_primary>compair_student_3è@test.com</person_contact_email_primary>
<person_name_given>Student</person_name_given>
<person_name_family>Six</person_name_family>
<person_name_full>Student Six</person_name_full>
Expand Down Expand Up @@ -1739,7 +1741,7 @@ def test_lti_membership_for_consumer_with_membership_ext(self, mocked_post_membe
self.assertEqual(len(lti_memberships), 5)
for lti_membership in lti_memberships:
self.assertIn(lti_membership.lti_user.user_id, [lti_user_instructor.user_id, lti_user_student_1.user_id,
"compair_student_2", "compair_student_3", "compair_instructor_2"])
"compair_student_2", "compair_student_3è", "compair_instructor_2"])

# test minimual membership response
mocked_post_membership_request.return_value = """
Expand All @@ -1765,7 +1767,7 @@ def test_lti_membership_for_consumer_with_membership_ext(self, mocked_post_membe
<roles>Learner</roles>
</member>
<member>
<user_id>compair_student_3</user_id>
<user_id>compair_student_3è</user_id>
<roles>TeachingAssistant</roles>
</member>
<member>
Expand Down Expand Up @@ -1798,7 +1800,7 @@ def test_lti_membership_for_consumer_with_membership_ext(self, mocked_post_membe
self.assertEqual(len(lti_memberships), 5)
for lti_membership in lti_memberships:
self.assertIn(lti_membership.lti_user.user_id, [lti_user_instructor.user_id, lti_user_student_1.user_id,
"compair_student_2", "compair_student_3", "compair_instructor_2"])
"compair_student_2", "compair_student_3è", "compair_instructor_2"])


# test ensure current user is not unenrolled from course on membership fetch
Expand Down Expand Up @@ -1993,12 +1995,12 @@ def test_lti_membership_for_consumer_with_membership_service(self, mocked_get_me
"@id":None,
"name":"Student Six",
"img":"http://www.gravatar.com/avatar/4",
"email":"compair_student_3@test.com",
"email":"compair_student_3è@test.com",
"familyName":"Six",
"givenName":"Student",
"resultSourcedId":None,
"sourcedId":"compair_student_3",
"userId":"compair_student_3"
"sourcedId":"compair_student_3è",
"userId":"compair_student_3è"
}
},
{
Expand Down Expand Up @@ -2067,7 +2069,7 @@ def test_lti_membership_for_consumer_with_membership_service(self, mocked_get_me
self.assertEqual(len(lti_memberships), 5)
for lti_membership in lti_memberships:
self.assertIn(lti_membership.lti_user.user_id, [lti_user_instructor.user_id, lti_user_student_1.user_id,
"compair_student_2", "compair_student_3", "compair_instructor_2", "compair_student_100"])
"compair_student_2", "compair_student_3è", "compair_instructor_2", "compair_student_100"])

# test minimual membership response
mocked_get_membership_request.return_value = {
Expand Down Expand Up @@ -2116,7 +2118,7 @@ def test_lti_membership_for_consumer_with_membership_service(self, mocked_get_me
"urn:lti:role:ims/lis/TeachingAssistant"
],
"member":{
"userId":"compair_student_3"
"userId":"compair_student_3è"
}
},
{
Expand Down Expand Up @@ -2166,7 +2168,7 @@ def test_lti_membership_for_consumer_with_membership_service(self, mocked_get_me
self.assertEqual(len(lti_memberships), 5)
for lti_membership in lti_memberships:
self.assertIn(lti_membership.lti_user.user_id, [lti_user_instructor.user_id, lti_user_student_1.user_id,
"compair_student_2", "compair_student_3", "compair_instructor_2"])
"compair_student_2", "compair_student_3è", "compair_instructor_2"])


# test ensure current user is not unenrolled from course on membership fetch
Expand Down

0 comments on commit 37da211

Please sign in to comment.