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

Check if Deployment ID claim is missing and return an error even if validation if disabled #1396

Closed
jonespm opened this issue Aug 19, 2022 · 5 comments · Fixed by #1401
Closed
Assignees

Comments

@jonespm
Copy link
Member

jonespm commented Aug 19, 2022

We failed the certification because we allowed missing deployment ids. We could probably turn this on and validate it but if it's missing completely this should have an error and fail.

It looks like from the spec that this is required to be present but it doesn't necessarily need to be validated. https://www.imsglobal.org/spec/lti/v1p3#lti-deployment-id-claim

if settings.LTI_CONFIG_DISABLE_DEPLOYMENT_ID_VALIDATION:

@jonespm jonespm transferred this issue from tl-its-umich-edu/canvas-app-explorer Aug 19, 2022
@jonespm jonespm self-assigned this Aug 19, 2022
@jonespm jonespm added this to To do in MyLA-2022.02.01 via automation Aug 19, 2022
@jonespm jonespm moved this from To do to In progress in MyLA-2022.02.01 Aug 25, 2022
jonespm added a commit to jonespm/student-dashboard-django that referenced this issue Aug 25, 2022
… and return an

error even if validation is disabled
MyLA-2022.02.01 automation moved this from In progress to Review/QA Aug 30, 2022
jonespm added a commit that referenced this issue Aug 30, 2022
@jonespm jonespm moved this from Review/QA to Review/QA - DEV in MyLA-2022.02.01 Aug 30, 2022
@pushyamig
Copy link
Contributor

I will QA this

@pushyamig
Copy link
Contributor

pushyamig commented Sep 15, 2022

@jonespm, So the LTI launch variable are sending 'https://purl.imsglobal.org/spec/lti/claim/deployment_id = ''`

now with the check _get_deployment_id() it will throw a deployment_id is not set in jwt body is that what is expected and regardless what the chosen value for LTI_CONFIG_DISABLE_DEPLOYMENT_ID_VALIDATION?

@jonespm
Copy link
Member Author

jonespm commented Sep 15, 2022

Right, the spec says that this has to be set, so even if validation is disabled it should still fail if it's empty. The value can be set to anything though with validation disabled and it will pass, as long as it's set to something.

@pushyamig
Copy link
Contributor

Test passes.

I couldn't set a null to deployment id since canvas always sending it and we cannot control that from Canvas instance. I think it will be a overkill to use the 1Edtech LTI test suit for resolving this issue

So What I did is I throw an error when _get_deployment_id() is called and that's a way I am mimicking it.

@pushyamig pushyamig moved this from Review/QA - DEV to Done in MyLA-2022.02.01 Sep 15, 2022
@jonespm
Copy link
Member Author

jonespm commented Sep 15, 2022

That sounds good! I plan to run the 1EdTech test probably next week, hopefully on Monday so we'll see if it passes that or not. But looks it should be fine.

@jennlove-um jennlove-um removed this from Done in MyLA-2022.02.01 Sep 20, 2022
@jennlove-um jennlove-um added this to To do in MyLA-2022.01.03 via automation Sep 20, 2022
@pushyamig pushyamig moved this from To do to Done in MyLA-2022.01.03 Sep 20, 2022
jonespm added a commit to jonespm/student-dashboard-django that referenced this issue Sep 20, 2022
… and return an error even if validation is disabled (tl-its-umich-edu#1401)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
2 participants