dmitry-viskov / pylti1.3

LTI 1.3 Advantage Tool
MIT License
111 stars 60 forks source link

only decode service data as JSON if content-type says so #93

Closed hmoffatt closed 1 year ago

hmoffatt commented 1 year ago

The ServiceConnector should check the returned content-type before decoding the body as JSON. LTI 1.3 allows the 1.0/1.1 basic outcomes service to be used as a 1.3 service, but it returns XML.

dmitry-viskov commented 1 year ago

Hey @hmoffatt . Thanks for you contribution but I don't think that this improvement has any sense. Well, in theory probably you can use LTI 1.3 together with outcomes service from LTI 1.1 specification but on practice it looks weird. Anyway ServiceConnector (that is used as request library for advantage services) is not compatible with LTI 1.1 outcomes service. Differences are not only in Content-Type. For example LTI 1.1 is based on OAuth authorization and you needn't to make additional query to get access token (as we do in LTI 1.3). The goal of this library is to support LTI 1.3 only and all related Advantage services like Deep Linking and etc. LTI 1.1 is the separate story and out of scope of this library. If somebody wants to use basic outcomes service together with LTI 1.3 i don't think that ServiceConnector somehow helps him

hmoffatt commented 1 year ago

Hi @dmitry-viskov . Moodle has implemented Basic Outcomes as a 1.3 service. It behaves exactly as a 1.3 service using oauth etc, but the payload is the LTI 1.1 Basic Outcomes XML. If users do not turn on the Assessment and Grades Service then Moodle offers Basic Outcomes instead.

I have implemented this in my tool, all that is needed is this small change to the generic service connector not to decode JSON if the content-type is not JSON.

dmitry-viskov commented 1 year ago

@hmoffatt it looks like custom Moodle's feature and it is 100% contradicts with documentation. Response may not have "Content-Type" header but anyway must be considered as JSON. I've updated code. So you may redefine ServiceConnector and create your own implementation: https://github.com/dmitry-viskov/pylti1.3/blob/master/pylti1p3/message_launch.py#L344

hmoffatt commented 1 year ago

Basic outcomes on 1.3 is documented in the specification here: https://www.imsglobal.org/spec/lti-bo/v1p1#integration-with-lti-1-3

There's a php library for it too: https://github.com/oat-sa/lib-lti1p3-basic-outcome/blob/master/README.md

I can subclass Service connector and change the logic there instead, but I have to copy a lot of code to make this simple change.

dmitry-viskov commented 1 year ago

From documentation:

The claim to include basic outcome parameters in LTI 1.3 messages is: https://purl.imsglobal.org/spec/lti-bo/claim/basicoutcome.

It contains two properties: lis_outcome_service_url and lis_result_sourcedid as defined above.

{
"lis_result_sourcedid": "82098-21uhd",
"lis_outcome_service_url": "https://www.myuniv.example.com/2344"
}

Library doesn't support this service and consequently it makes no sense to modify ServiceConnector. BO will not work even if Content-Type will be changed

hmoffatt commented 1 year ago

Sorry change was wrong anyway because some services return special lti...+json content-type, not application/json. I have deviced a modified ServiceConnector class for my Basic Outcomes service instead.