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

basic-lti-launch-request needs to be LTI 1.0 version ( Learning Tools Interoperability® Certification Suite Test 1.6) #5

Open
jozefbriss opened this issue May 23, 2018 · 6 comments

Comments

@jozefbriss
Copy link

$this->ok = isset($_POST['resource_link_id']) && (strlen(trim($_POST['resource_link_id'])) > 0);

Should be changed to something like:

            $this->ok = $_POST['lti_version'] == self::LTI_VERSION1;
            if (!$this->ok) {
                $this->reason = 'Wrong lti_version parameter.';
            } else {
                $this->ok = isset($_POST['resource_link_id']) && (strlen(trim($_POST['resource_link_id'])) > 0);
                if (!$this->ok) {
                    $this->reason = 'Missing resource link ID.';
                }
            }
@Izumi-kun
Copy link
Owner

Izumi-kun commented May 23, 2018

https://www.imsglobal.org/wiki/step-1-lti-launch-request

It should include a POST parameter named lti_version with a value of LTI-1p0 ( or LTI-2p0 for an LTI 2 tool provider)

So LTI-2p0 is valid value also.

$this->ok = isset($_POST['lti_version']) && in_array($_POST['lti_version'], self::$LTI_VERSIONS);
if (!$this->ok) {
$this->reason = 'Invalid or missing lti_version parameter.';
}

@jozefbriss
Copy link
Author

I am running their certification 'Test 1.6 Wrong LTI version'
Expected result: Return user to the Tool Consumer with an error message.

They are sending lti_version=LTI-2p0 in this test and thus this test does not produce the error message.

I have work around in onLaunch for now.

@Izumi-kun
Copy link
Owner

Checking for version in your app seems normal since lti provider library does not known about what LTI version actually used in app.

What do you think?

@jozefbriss
Copy link
Author

You are right.

@dac514
Copy link
Collaborator

dac514 commented Jun 5, 2018

I looked into this today.

The test Test 1.6 Wrong LTI version assumes a correct configuration with Consumer key and secret,. It sends lti_version: LTI-2p0 over $_POST.

So somewhere in between:

$this->ok = isset($_POST['lti_version']) && in_array($_POST['lti_version'], self::$LTI_VERSIONS);
if (!$this->ok) {
    $this->reason = 'Invalid or missing lti_version parameter.';
}

And.

if ($this->consumer->ltiVersion !== $_POST['lti_version']) {
  $this->consumer->ltiVersion = $_POST['lti_version'];
  $doSaveConsumer = true;
}

Source

There should be something like:

if ( ! empty( $this->consumer->ltiVersion ) && $this->consumer->ltiVersion !== $_POST['lti_version'] ) {
  $this->reason = 'Wrong LTI version';
  $this->ok = false;
}

Because this LTI provider library does know the version. It's stored in the database under lti2_consumer.lti_version

Best regards,

@Izumi-kun Izumi-kun reopened this Jun 6, 2018
@Izumi-kun
Copy link
Owner

LTI 1.1.1

lti_version=LTI-1p0
This indicates which version of the specification is being used for this particular message. Since launches for version 1.1 are upwards compatible with 1.0 launches, this value is not advanced for LTI 1.1. This parameter is required.

LTI 2.0

lti_version=LTI-2p0 (Required)
This indicates which version of the specification is being used for this particular message. This parameter is required in all messages.

  • In both versions lti_version parameter indicates which version of the specification is being used for this particular message. This is means that version of messages from Tool Consumer can be various? If yes, why Test 1.6 requires exactly LTI-1p0 value?

  • \IMSGlobal\LTI\ToolProvider\ToolConsumer::$ltiVersion property contains LTI version (as reported by last tool consumer connection). This property used only for signing outcomes. This is means that version is fixed for specific ToolConsumer? If yes, why this prop not configurable and updates on every message from Consumer?

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

No branches or pull requests

3 participants