Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ToolProvider: Fix for issue 74 #75

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

rghostin
Copy link

@rghostin rghostin commented Sep 5, 2020

[c.f issue #74]


In some cases, the ToolProvider.consumer_secret is not set. This prevents the signing of outcome requests' XML.

Code snippets below help reproduce the events.

  • In a launch view:
def lti_launch(request):
    if request.method == "POST":
        tool_provider = DjangoToolProvider.from_django_request(request=request)
        # tool_provider.consumer_key was set from the oauth_consumer_key post parameter; consumer_secret is still None
        oauth_validator = SigOnlyRequestValidator()
        is_valid_oauth = tool_provider.is_valid_request(oauth_validator)    
        [...]
  • In the method ToolProvider.is_valid_request:
validator = ProxyValidator(validator)
valid, request = endpoint.validate_request([...])
# At this point the proxy contains the secret in validator.secret, though the tool_provider.consumer_secret is still None
if valid and not self.consumer_key and not self.consumer_secret:     # ! Potentially faulty line ! Condition is False, so tool_provider.consumer_secret stays to None
    self.consumer_key = self.launch_params['oauth_consumer_key']
    self.consumer_secret = validator.secret
return valid
  • Problems arise later on during the usage of the ToolProvider instance:
tool_provider.post_replace_result(score=1)    # unable to sign the request since consumer_secret is None (OutcomeRequest.has_required_attributes returns False)

Solution:
In the method ToolProvider.is_valid_request, the condition should be formulated as:

if valid:
    # Gather the key and secret
    if not self.consumer_key: 
        self.consumer_key = self.launch_params['oauth_consumer_key']
    if not self.consumer_secret:
        self.consumer_secret = validator.secret

@codecov
Copy link

codecov bot commented Sep 5, 2020

Codecov Report

Merging #75 into master will decrease coverage by 1.15%.
The diff coverage is 80.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #75      +/-   ##
==========================================
- Coverage   86.05%   84.90%   -1.16%     
==========================================
  Files          16       16              
  Lines         667      669       +2     
  Branches      115      117       +2     
==========================================
- Hits          574      568       -6     
- Misses         74       81       +7     
- Partials       19       20       +1     
Impacted Files Coverage Δ
src/lti/tool_provider.py 85.54% <80.00%> (-4.59%) ⬇️
src/lti/utils.py 70.00% <0.00%> (-10.00%) ⬇️
src/lti/launch_params.py 88.00% <0.00%> (-2.67%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f7ca2af...a1ab615. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant