A report was made where an LTI tool unenrolled all users when a malformed Names and Roles Provisioning Service response was received, requiring restoration from backups to get the school back in business. The specifics of this case were responses with 200 status codes but empty response bodies caused by some sort of temporary unexpected downtime.
In LTIServiceConnector, no Guzzle ClientException was thrown in MakeServiceRequest, so the blank body was passed to getResultsFromResponse, and the library returned an empty array that the application using the library interpreted by unenrolling the students, which is what it indeed should have done if the NRPS call received a well-formed response with an empty members array. There is no way to differentiate between those two situations from the empty array return value from getAll.
If the maintainers agree this should be made more robust, there are at least two ways to fix it:
One is to validate that the result of json_decode in getResponseBody contains data, by testing gettype(json_decode_output) == 'object' and raising an exception on false.
Another is to validate that, if $key is provided to the getResultsFromResponse function, the key exists in the decoded json and raise an exception otherwise.
I would think those exceptions should be consumed by the application using the library rather than trying again when $shouldRetry.
I'd be happy to create a PR for this if you have an opinion on what the correct behavior should be.
A report was made where an LTI tool unenrolled all users when a malformed Names and Roles Provisioning Service response was received, requiring restoration from backups to get the school back in business. The specifics of this case were responses with 200 status codes but empty response bodies caused by some sort of temporary unexpected downtime.
In LTIServiceConnector, no Guzzle ClientException was thrown in MakeServiceRequest, so the blank body was passed to getResultsFromResponse, and the library returned an empty array that the application using the library interpreted by unenrolling the students, which is what it indeed should have done if the NRPS call received a well-formed response with an empty members array. There is no way to differentiate between those two situations from the empty array return value from getAll.
If the maintainers agree this should be made more robust, there are at least two ways to fix it:
One is to validate that the result of json_decode in getResponseBody contains data, by testing gettype(json_decode_output) == 'object' and raising an exception on false.
Another is to validate that, if $key is provided to the getResultsFromResponse function, the key exists in the decoded json and raise an exception otherwise.
I would think those exceptions should be consumed by the application using the library rather than trying again when $shouldRetry.
I'd be happy to create a PR for this if you have an opinion on what the correct behavior should be.