Code Review

[andrew-gardener]

Just going to list issues bellow (sorry couldn't make inline arbitrary inline comments on a branch in github like I thought). I'll ping @ionparticle when I'm done.

• [x] It might be a good to add function to check that there aren't unexpected params (check that there are only required+optional). https://github.com/ubc/lti-shim/blob/5dd0cb27cf11f95817e0a57df6762fac3452571a/lti/Specs/ParamChecker.php#L19
• [ ] I think that you might need to trying re-fetching the jwt keys at least once on failure (JwksUpdater). I didn't notice and handling of key rotation long term (after all the initial set of keys are expired). https://github.com/ubc/lti-shim/blob/15a6de90c80ef65df85166f3047746254449dcd0/lti/Specs/Launch/ToolLaunch.php#L176
• [x] I think it would be cleaner to store deployment_id and login_hint in separate columns instead of mixing it in with the launch params. https://github.com/ubc/lti-shim/blob/15a6de90c80ef65df85166f3047746254449dcd0/lti/Specs/Launch/ToolLaunch.php#L132
• [x] LtiUser fake_email should probably be unique in db (and retry saving with a different one if unique constraint fails for it).
• [x] For the Launch params: https://github.com/ubc/lti-shim/blob/15a6de90c80ef65df85166f3047746254449dcd0/lti/Specs/Launch/PlatformLaunch.php#L120
  • [x] resource link is still fake
  • [x] services aren't added yet but you could still probably build out fake claims for them if they are passed in the launch
  • [x] launch_presentation might be something that you still want to pass through (pass document_target, height, and width. proxy or exclude return_url. exclude locale)
• [x] add real nonces (prevent replay) https://github.com/ubc/lti-shim/blob/15a6de90c80ef65df85166f3047746254449dcd0/lti/Specs/Launch/ToolLaunch.php#L105
• [ ] Just wondering why there is a second apt-get install near the end of the Dockerfile (usually its good to have it before you copy files so rebuilding is faster+can be less steps). Can it only happen after the composer files are included? https://github.com/ubc/lti-shim/blob/5dd0cb27cf11f95817e0a57df6762fac3452571a/Dockerfile#L21
• [ ] delete the lti_session record when finished https://github.com/ubc/lti-shim/blob/15a6de90c80ef65df85166f3047746254449dcd0/lti/Specs/Launch/PlatformLaunch.php#L155

[andrew-gardener]

@ionparticle Looks pretty good overall. looking forward to when this is further along!

[ionparticle]

Clarified and expanded some tasks after discussion with Andrew:

• Optional parameter validation probably isn't needed.
• [ ] Will need to handle expired keys in JWKS, probably a scheduled task to remove keys that no longer shows up in the JWKS.
• [x] LtiUser probably should be split into two tables, LtiRealUser and LtiFakeUser, in order to reduce data duplication.
• [x] Will need to generate a fake resource link ID.