MGMT-132: Supporting migrations

[dbhynds]

Summary of Changes

Background

• This conversation began in this pull request: https://github.com/packbackbooks/lti-1-3-php-library/pull/98
• IMSGlobal's description of how this should function can be found here: https://www.imsglobal.org/spec/lti/v1p3/migr

Summary of the problem

While validating an LTI Message Lauch, the library throws an exception if it can't find a deployment. However, when migrating, if the launch contains a oauth_consumer_key_sign, the tool may try to upgrade the launch from 1.1 to 1.3. In that case, an deployment should

We're essentially muddying the waters in terms of separation of concerns: validation and migration. We need to validate the LTI launch message before we migrate, but we also need a legitimate path for migration, while still also ultimately validating the deployment provided by the launch message.

How this would be used

A tool provider would implement their own migration code in the database service. Then in the launch you would simply do:

$message->initialize($request->all());

Testing

• [x] I have added automated tests for my changes
• [x] I ran composer test before opening this PR
• [x] I ran composer lint-fix before opening this PR

[dbhynds]

@erikdonohoo Tagging you for your feedback.

[erikdonohoo]

Left a few notes on this. Anyway I could take a stab at modifying some of this?

[dbhynds]

Yep! I'll look into getting you access tomorrow.

[dbhynds]

@erikdonohoo Added you as a contributor. Feel free to push to this branch

[erikdonohoo]

Thanks @dbhynds. I will spend some time on this and then you can let me know what you think.

[erikdonohoo]

Hey there @dbhynds . I think I found a nice way to handle this while fully preserving backwards compatibility. Let me know what you think of these changes.

[erikdonohoo]

I see @Watercycle is an assigned reviewer of this. Pinging as well.

[dbhynds]

@erikdonohoo have you tested this out IRL yet? At the very least, it looks like we're mixing types for $this->db on the LtiMessageLauch, which I suspect will cause issues.

Also, could you give me a rough idea of what your implementation of the IMigrationDatabase looks like? I don't need to see anything proprietary, but just enough to give me a sense for the main things it's doing.

[erikdonohoo]

@erikdonohoo have you tested this out IRL yet? At the very least, it looks like we're mixing types for $this->db on the LtiMessageLauch, which I suspect will cause issues.

Also, could you give me a rough idea of what your implementation of the IMigrationDatabase looks like? I don't need to see anything proprietary, but just enough to give me a sense for the main things it's doing.

Sure thing. Yes this does work in real life, and the unit tests prove that. We aren't mixing types, as they are both interfaces. You can check to see if the actual real life DB class implements either class. We are only checking for migrations if the new interface was actually implemented by the db class. This makes it completely backwards compatible. People who do not implement the new interface will see no change in behavior.

If this library locked support to only php > 8.0, then we could add the union type to the DB class of both iMigrationDb and IDatabase. But its fine to still expect it to be an IDatabase. The other interface is entirely optional, which is why we check in the shouldMigrate method. But its possible to be both at the same time, and in the case you support migrations, it would be both.

As far as how those methods would be implemented, it depends on how each tool stored data about 1.1 installations in their system. But the idea is for getMatchingLti1p1Install you look at all the data coming in from the launch and see if you have a matching 1.1 install based on the 1.1 claim or other claims in the launch. It will all be up to the tool to figure that out. In our case, we used things like the tool_consumer_instance_guid, combined with some other properties to uniqely identify the "organizations" representing a 1.1 launch.

For migrateFromLti1p1, the goal is to add a LtiDeployment into your system automatically. BY the time this method is called, a passed oauth consumer key sign has already been automatically verified, so it is safe to "auto provision" the 1.3 deployment. This code will create that deployment, as well as do any other rebinding or associating needed to the 1.1 installation. Again, this is all going to be dependent on how an individual tool stored that data in their own system.

@dbhynds Let me know what other questions you have on this

[dbhynds]

Ah, I misunderstood your approach with the interfaces, but I get it now. I like what you did.

[dbhynds]

@erikdonohoo I think this is ready for your eyes again.

[dbhynds]

@Watercycle this is ready for you when you get back

[dbhynds]

Here are these changes in our CI pipeline: https://gitlab.com/packback/questions/-/pipelines/1039934019

[dbhynds]

@snake @erikdonohoo I still have a few questions that will just be quicker to talk through live. Send me an email at davo@packback.co (easier than github comments) and let's find a time to chat. I'm thinking the best time to meet will be:

• 8 am (snake)
• 6 pm (erik)
• 7 pm (me)

Next week I'm free Wed and Thurs evening (i.e. Thurs or Fri morning for snake)

[dbhynds]

@Watercycle here's a pipeline with my latest changes: https://gitlab.com/packback/questions/-/pipelines/1086844076

[dbhynds]

@Watercycle I've made a few changes. I'm also going to go back and update documentation in the wiki after this merges.