Learnosity / learnosity-sdk-python

Learnosity SDK for Python
Apache License 2.0
7 stars 10 forks source link

Do not enfore ASCII encoding for request payload #50

Closed andreyfedoseev closed 1 year ago

andreyfedoseev commented 4 years ago

Otherwise, non-ASCII characters are escaped and it causes "Signature mismatch" error if the payload contains non-ASCII characters.

Checklist

Ninavdk commented 4 years ago

Hi @andreyfedoseev ,

Thanks for submitting the PR. I've added it to our backlog for the team to review.

Cheers, Nina

Ninavdk commented 4 years ago

Hi @andreyfedoseev,

Our team were triaging the issue and are unable to reproduce an issue for the fix you've proposed.

Could you share steps to reproduce the signature mismatch? Specifically would be good to note what non-ASCII characters are causing the issue.

Cheers, Nina

andreyfedoseev commented 4 years ago

@Ninavdk

Here's a sample payload that's causing the problem:

{'rendering_type': 'assess', 'type': 'submit_practice', 'name': 'тест', 'user_id': '200200213', 'session_id': 'b20a1a71-2ead-497b-8cf0-cad881784731', 'activity_id': '6c0e7b30-0455-45cf-916f-14716b433e7a', 'items': ['VH953846'], 'config': {'labelBundle': {'fontSizeInstructions': 'Adjust the font size in the exam player.', 'paletteInstructions': 'Change the color scheme of the exam player.'}, 'navigation': {'scroll_to_test': False, 'scroll_to_top': False, 'show_intro': False, 'show_outro': False, 'auto_save': {'save_interval_duration': 1, 'ui': True}}, 'configuration': {'responsive_regions': False, 'renderSaveButton': True, 'lazyload': True, 'events': True}, 'annotations_api_init_options': {'modules': {'texthighlight': True}}, 'region_overrides': {'slider_element': {'scrollable_option': True}, 'items.progress_element': False, 'right.expand_menu': False, 'right.reviewscreen_button': False, 'top-left.masking_button': False, 'top-left.title_element': False, 'right.accessibility_button': {'zoom_option': False}}, 'regions': {'top-left': [{'type': 'title_element', 'position': 'left'}], 'top-right': [{'type': 'previous_button', 'position': 'right'}, {'type': 'itemcount_element', 'position': 'right'}, {'type': 'next_button', 'position': 'right', 'hide_label_option': True, 'enable_basic_next_option': False}], 'right': [{'type': 'calculator_button'}, {'type': 'accessibility_button'}], 'items': [{'type': 'previous_button', 'position': 'left'}, {'type': 'next_button', 'position': 'right', 'hide_label_option': True, 'enable_basic_next_option': False}]}, 'events': True, 'title': 'тест', 'questions_api_init_options': {'render_with_captured_questions': True}}, 'retrieve_tags': True, 'session_metadata': {'session_tags': [{'type': 'attempt_number', 'name': '1'}]}}

(this is a Python dictionary before it's serialized to JSON)

The non-ASCII characters are in in name field.

When I sign this request and pass to it Items API I get "Signature mismatch error" in the browser.

If I apply the proposed fix everything works correctly.

Note how the non-ASCII characters are treated when ensure_ascii is set to True or False.

Ninavdk commented 4 years ago

Thanks for the additional info @andreyfedoseev , we'll take a look.

Cheers, Nina

Ninavdk commented 4 years ago

Thanks @andreyfedoseev , we've been able to reproduce the issue now.

Am I correct in thinking you're using your fix locally, and aren't blocked from using the Learnosity APIs whilst waiting on a merge from us?

andreyfedoseev commented 4 years ago

Am I correct in thinking you're using your fix locally, and aren't blocked from using the Learnosity APIs whilst waiting on a merge from us?

Yes, we created a sub-class based on Init and added the fix there.

Ninavdk commented 4 years ago

Ok cool. Now we can reproduce the issue on our side, this ticket will go into the backlog, can be pointed and fit into an upcoming sprint accordingly.

We'll update here when things progress.

Much appreciated once again for your submission.

Cheers, Nina

shtrom commented 1 year ago

No issue from my side. This looks pretty straightforward.

Before mergeng, we should add a test case based on the example provided by @andreyfedoseev. We should also update the ChangeLog.md.