MGMT-132: Supporting migrations #99

dbhynds · 2023-09-06T17:09:02Z

Summary of Changes

Background

This conversation began in this pull request: Allow LtiMessgageLaunch validation steps to be modified by tool providers #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

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

…and it has lots of problems

dbhynds · 2023-09-06T17:10:54Z

@erikdonohoo Tagging you for your feedback.

erikdonohoo · 2023-09-06T22:24:11Z

src/LtiMessageLaunch.php

+
+    private function shouldMigrate(): bool
+    {
+        return $this->hasMigrationStrategy()


Where is hasMigrationStrategy() defined?

It's not and I wasn't sure where it should be. It depends on how we end up defining the migration process.

erikdonohoo · 2023-09-06T22:25:19Z

src/LtiMessageLaunch.php

+        return $this->hasMigrationStrategy()
+            // I don't like this here. Move it later
+            && isset($this->jwt['body'][LtiConstants::DEPLOYMENT_ID])
+            && $this->db->hasMatchingLti11Key($this->getLaunchData()['oauth_consumer_key_sign']);


In order to verify this, you need to be passed both oauth_consumer_key_sign and oauth_consumer_key. These properties will be under the 1.1 migration claim, and not on the top level of the launch data.

Its also possible to not send these values, so you can't assume they are there (referring to the key on the array if its not there will blow up).

I threw this together from memory without looking to see where these actually come from or putting in any safeguards. (This definitely isn't meant to be production-ready). Both of your suggestions here sound right to me.

erikdonohoo · 2023-09-06T22:26:27Z

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

dbhynds · 2023-09-06T22:35:30Z

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

dbhynds · 2023-09-07T14:18:28Z

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

erikdonohoo · 2023-09-07T16:12:54Z

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

…_sign

…to Lti1p1Install

erikdonohoo · 2023-09-13T20:23:05Z

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 · 2023-09-13T20:26:17Z

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

dbhynds · 2023-09-13T20:38:09Z

.vscode/launch.json

+      }
+    }
+  ]
+}


Let's add this to the .gitignore

dbhynds · 2023-09-13T20:51:09Z

@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.

dbhynds · 2023-09-13T20:54:59Z

src/LtiMessageLaunch.php

+    {
+        // Check to see if we have params to calculate oauth_consumer_key_sign
+        if (!isset($launchData[LtiConstants::LTI1P1]) || !isset($launchData[LtiConstants::LTI1P1]['oauth_consumer_key_sign'])) {
+            throw new LtiException(static::ERR_MISSING_OAUTH_CONSUMER_KEY_SIGN);


Does IMSGlobal provide guidance on how failures should be handled? As this is currently implemented, a mismatch would throw an exception which would be returned to the user. This means all LTI launches would fail until it was corrected. That seems bad.

This will not fail all LTI launches, only LTI launches for a tool who has a matching 1.1 install, but where the deployment is not already registered in their system.

Not trying to be pedantic, but I keep going back to the failure states here. I want to make sure we never fail an otherwise valid launch just because something related to the migration is incorrect. Consider:

A 1.3 integration has been successfully set up with several valid, existing deployments. A school admin sets things up to do a migration from 1.1 and messes something up with the oauth key/secret. If a user from an existing deployment launches, they shouldn't see an error message. We'd only want to serve an error for launches to deployments we don't recognize.

If you already have a working 1.3 integration, then migration is no longer an option. Otherwise, the tool would need to support rebinding all the data and things created in the 1.3 migration to work again in the 1.1 integration. The point is if you are a tool supporting this, you block them before they start using it. Not that you block them when they are already using it. Even if they did do something wrong with the keys/secrets however, this STILL wouldn't get triggered, as it only gets triggered if the deployment_id in the launch isn't already found in the system. Migration can only occur if that deployment is not already registered in the tool. Does that make sense @dbhynds ? The validateDeployment() method only tries to do this code that can throw this error if it didn't find the deployment in the first attempt.

Understood. Thanks for the clarification

We support all major LMSs, and even homegrown LMSs. Basically anything that can do an LTI 1.1 launch, we work with. So we have a wide variety of different types of data in our system tied to the LTI spec. Our issue is ultimately this:

We have a tool that allows students to submit videos and get feedback from teachers on those videos. This comes with all kinds of other features and history that a user has access to. We need to ensure that all our existing customers who move to 1.3, have their 1.1 users bound to the new 1.3 users we make so they are in fact one and the same. So they maintain access to the content they have been creating for years with us using 1.1. If that re-binding does not take place at the beginning, then those users begin 1.3 with a new "user" in our system that is essentially invisible to them. Our tool doesn't require users to login to an account to begin with, as that account is strictly tied to the information coming in via the 1.1 launch. Being tied to a specific key/secret is where the security of the launches come in, not from them authorizing through some separate login on our side.

With that in mind, this is why on initial launch it is so important for us to handle that rebinding. The launch itself provisions entities on our side, and if we don't know what to do with the IDs, new things will be created in our DB that are not associated with the original 1.1 user that person had.

With 1.3, we intend to block all new launches with an unknown deployment as well. And I believe that is the correct way to do things according to the IMS global spec. We will have an internal tool that saves all the info that we could have used to auto-provision that deployment, and allow our support teams to verify the identify of the launcher and get that deployment_id in our system that way. After that, launches will work successfully. It is also at that moment where we will determine if they are an existing 1.1 customer or not and "migrate" them by mapping their existing 1.1 organization to the new params coming in for 1.3 launch. That way, they can launch 1.1 or 1.3 into the same content and as long as the ids they are passing are the same or mapped in the migration claim, it will always work.

As you know, almost no LMSs support the migration claim. IMS global documented the state of it here. https://www.imsglobal.org/lti-platform-migration-specifics They also produced a video with details on each LMSs support plans for migrations here https://www.youtube.com/watch?v=sg0dhf5j2xs

The hope was if the oauth_consumer_key_sign was passed, that this could be a short cut for customers who want to move to 1.3 if their LMS supports that param. It would allow us to "verify their identity" in the launch automatically and do the "re-binding" that IMS global speaks about in the video securely. None of this matters if your tool doesn't persist things between launches for your users and make things accessible between launches for them based on the user_id in the launch params (sub). But for us, we do that. And the only way to securely move them over with no intervention, would be this claim. Any migrations without this claim are technically a security risk vulnerable to social engineering. We are working with Blackboard and Canvas on this and have open security issues with them on the lack of having this for 1.1 migrations.

Hopefully this didn't sound like just straight on blabbering, but we've definitely been thinking about and working on this for a while and want to get it right for our customers when we turn on 1.3 support.

If we think it would be valuable, maybe we could schedule a group call between some of the interested parties here to make sure whatever is added into packback for migration supports everyones needs. We certainly don't want to cause issues for others that are ahead of us on 1.3 at this point. @snake @dbhynds

@dbhynds I just looked over your most recent changes, particularly to shouldMigrate by adding it to the DB. I like this as it allows the tool providers more control over when to do the migration. I am satisfied with this and think this would work fine for our needs. I would be interested in hearing what @snake has to say on this as well.

I think hopping on a call might be helpful. Thanks for suggesting! I'm assuming everyone is located in the US, but correct me if I'm wrong. Here's a doodle for us to coordinate a time to talk. https://doodle.com/meeting/participate/id/bq2LGDGd

@snake @erikdonohoo

@dbhynds @erikdonohoo: that recent change also suits our (albeit probably obscure) use case. As long as we can make that decision about whether to continue with the migration, that ticks the box for us.

Happy to jump on a call but I'm GMT+08, so it's usually an early morning 8am or 8pm affair for meetings with the states.

@snake @dbhynds I've updated my github notification settings but for whatever reason I'm still not getting notified when we reply to each other either. I'm still available for that call if we feel its necessary, but I agree with @snake the most recent chagnes made will most likely satisfy everyone's needs. (At least as far as I can see)

erikdonohoo · 2023-10-17T16:00:58Z

src/LtiMessageLaunch.php

@@ -147,6 +173,32 @@ public function validate(array $request = null)
            ->cacheLaunchData();
    }

+    public function migrate()
+    {
+        if (isset($this->deployment) || !$this->canMigrate()) {


Let me describe a situation here. A user used 1.1 and wants to migrate. Their LMS however does not support the 1p1 migrate claim or the oauth_consumer_key_sign. Despite this however, using things like the toolPlatform->guid claim, we can reasonably infer that this user NEEDS to migrate, even though they didn't pass the oauth_consumer_key. In that case, I, the tool provider, need a different error out of packback. They "can't" migrate, but they "should" have migrated. This helps me in the messaging I will show them, as well as handling manually creating the deployment, to ensure the re-binding can occur manually on my side. This migrate function should change slightly.

The canMigrate function should be modified and check if the tool provider implemented code for migration, and if findLti1p1Keys returns any keys. If the deployment is set, or they didn't implement migration, or there is no key to migrate, then just return, as migration either isn't necessary, or isn't supported.

If we found a key, but the system didn't pass the key_sign or the consumer_key, then we shuld throw an error saying the KEY_SIGN or KEY is missing.

If we have the keys, but the signatures don't match, we should throw a KEY_SIGN_MISMATCH error.

Otherwise, we should do the migration.

Let me know if that makes sense @dbhynds or if you have any concerns about this

Let me make sure I understand the scenario you described in the first paragraph:

A user performs an LTI 1.3 launch.

The LMS does not support migrations, and therefor the oauth_consumer_key is not included in the launch.

Despite this, there is a matching LTI 1.1 key that could be migrated (i.e., calling findMatchingLti1p1Keys() returns several results), but cannot be automatically due to the missing claims.

In that case, how would you know whether a user has already migrated or not? What in the system indicates "the user should migrate" vs "the user already migrated"?

The LMS does not support the "migration claim" but the users of that LMS still need to properly "migrate".

There IS a matching LTI 1.1 Key in our system that was found using findMatchingLti1p1Keys() even though they didn't pass the key. The reason being, each LMS can still be uniquely identified by the tool_consumer_instance_guid combined with some of the other LTI 1.1 params. Those params are not re-named for most LMSs in the move from 1.1 to 1.3. For example, blackboard passes the same ids whether you are doing a 1.1 launch or 1.3 launch. Other LMSs are the same. So even though they don't pass the oauth_consumer_key_sign for us to do a secure migration automatically, the user STILL NEEDS TO BE MIGRATED as you detected in your system that you have seen them before.

I would knwo they already migrated or not because in my system I can see if they have a binding to a 1.3 lti deployment or not. It is up to me, the tool provider to be able to know if a rebinding has taken place yet or not. @dbhynds

dbhynds · 2023-10-17T16:03:47Z

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

dbhynds · 2023-10-27T19:14:12Z

@snake @erikdonohoo I still have a few questions that will just be quicker to talk through live. Send me an email at [email protected] (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 · 2023-11-17T17:57:24Z

src/LtiMessageLaunch.php


-        if (empty($deployment)) {
-            // deployment not recognized.
+        if (!isset($this->deployment) && !$this->canMigrate()) {


@snake @erikdonohoo before I merge this, I just want to verify a few things:

Is this logic canMigrate() accurate here? Or should it be shouldMigrate()?

I've currently got matchingLti1p1KeyExists() marked private. Should it be public? I wasn't sure if you would need it as part of checking $this->db->shouldMigrate()

For our case, shouldMigrate() is really most useful when we've already passed validation of the launch data (i.e. deployment is found and valid). At this point, shouldMigrate() allows check whether we've migrated already, and also would allow us to quickly skip migration again on subsequent launches (because, of course, migrate() is still being called on every launch). So, for us, what you've got works fine (as would shouldMigrate()) since we're dealing with the case where we DO have a known deployment id. I think it's Erik's scenario that's more prevalent though and which really hits this conditional. In that case, if you don't have a deployment mapped and are unable to migrate (because of an incompatible DB type), you'd want to fail here.

I can't think of what calling shouldMigrate() would add. It seems it might support the following:

I don't know this deployment id

I am equipped to migrate (i.e. I have an IMigrationDatabase)

but I want to invalidate the launch before attempting migration

I can't think of a situation like that, personally. Maybe others can?

I think we would probably use the database method findLti1p1Keys() if we needed to do that, so no problem leaving it as is. As long as we have the launch data, and an opportunity to control whether migration happens, we're happy.

Same here. What you have I think handles all the edges. And we have confirmed this logic does indeed work. D2L is passing correctly signed payloads on migrated resource links today. So the migration claim is useful here. Canvas also recently re-enabled their code that passes the migration claim with the key sign, however, they have a bug and are doing their signature wrong, making the signature not trustworthy. But again, with the code you have here, that can be handled as well since we have the launch data and can see if its coming from a canvas provider.

dbhynds · 2023-11-27T20:06:18Z

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

Watercycle

Had a few inconsequential code & documentation comments. Otherwise, no concerns here.

As noted in a comment, please let me know if there's any explicit QA you'd like performed. We're not currently doing LTI 1.1 migrations like this in our application; but, based on what I'm seeing here, this is a flexible architecture to do so that provides reasonable scaffolding instead of having folks roll their own solutions.

P.S. Great discussions and awesome work on this PR!

Watercycle · 2023-11-30T09:35:54Z

src/ImsStorage/ImsCache.php

+/**
+ * @todo Deprecate this in the next major version
+ */


Pedantic, but I'd suggest using @deprecated since most IDEs will warn users. Ideally, stuff shouldn't be removed until it has been sitting deprecated for a major version.

Not that we have to do that here - just something I try to encourage for the future.

I've got a separate PR open where I'm going to go through and do all of that.

Watercycle · 2023-11-30T09:37:07Z

src/Interfaces/IMigrationDatabase.php

+ * To use this, just have whatever class you create that implements IDatabase
+ * also implement this interface.
+ */
+interface IMigrationDatabase


It's probably worth also having a "Migrating from LTI 1.1" page at https://github.com/packbackbooks/lti-1-3-php-library/wiki/ for more visibility.

Should this extend IDatabase?

Watercycle · 2023-11-30T09:42:46Z

src/Interfaces/IMigrationDatabase.php

+    /**
+     * Using the LtiMessageLaunch return an array of matching LTI 1.1 keys
+     */
+    public function findLti1p1Keys(LtiMessageLaunch $launch): array;


To be super clear, it's probably worth using a bit of PHPDoc here. In particular, @return Array<\Packback\Lti1p3\Lti1p1Key>. Plus, maybe a @see IMigrationDatabase on Lti1p1Key.

Watercycle · 2023-11-30T09:45:44Z

src/Lti1p1Key.php

+        $baseString = implode('&', $signatureComponents);
+        $utf8String = mb_convert_encoding($baseString, 'utf8', mb_detect_encoding($baseString));
+        $hash = hash_hmac('sha256', $utf8String, $this->getSecret(), true);


This goes over my head a little. In general, I'd encourage doing an @see [link to relevant part of spec]. Basically, if it's worth telling a reviewer in PR, it's probably worth noting for the next person poking around in the implementation.

Watercycle · 2023-11-30T09:50:12Z

src/LtiMessageLaunch.php

+        return $this;
+    }
+
+    public function initialize(array $request)


Is there any README.md or pages that need to be updated as part of this change?

Watercycle · 2023-11-30T09:56:15Z

src/LtiMessageLaunch.php

+        // @TODO: Add this in v6.0
+        // ->cacheLaunchData();


I know this is a little tedious to ask on a large, technical PR; but, could you clarify why we're adding cacheLaunchData here and deprecating stuff like ImsCache? For general traceability, a quick note should generally be added in the TODO, in the relevant commit, or on the PR.

For reference, here's the PR documenting what changes we'll be making in v6 and why: #105

Watercycle · 2023-11-30T09:57:58Z

src/LtiMessageLaunch.php

            ->cacheLaunchData();
    }

+    public function migrate()


I generally read over the relevant bits of the IMS spec and have read up and down the PR a few times. The test coverage also looks good. Is there any other particular QA you would like me to perform or things you would like me to check?

dbhynds · 2023-12-01T19:45:19Z

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

Watercycle

Thank you! No concerns here, then!

MGMT-132: Initial structure for supporting migrations. Nothing works …

2cf8037

…and it has lots of problems

dbhynds requested a review from Watercycle September 6, 2023 17:10

MGMT-132: Whitespace

99c825e

erikdonohoo reviewed Sep 6, 2023

View reviewed changes

erikdonohoo added 13 commits September 11, 2023 11:03

MGMT-132: Add methods for validating and computing oauth_consumer_key…

ed910da

…_sign

MGMT-132: Fix some linting errors, move logic for key sign compute in…

153a52b

…to Lti1p1Install

Keep validation chain intact while handling migration

f40657c

Fix linting errors

562a835

Remove trailing whitespace

9b4e267

Fix typings on test class

a3cdf9b

Remove static return from initialize

30f6ab9

Revert typings of IDatabase for backwards compatability

9e057cd

Add convenience file for debugging in VSCode

fd9ad00

Support key rotation in 1.1 registrations

ff3ed91

Finalize code and testing

077d763

Fix remaining lint errors

c059716

Ensure php7.4 support

7a91c9e

erikdonohoo force-pushed the mgmt-132-migrations branch from e23fbef to 7a91c9e Compare September 13, 2023 20:22

dbhynds commented Sep 13, 2023

View reviewed changes

.vscode/launch.json Outdated

}

}

]

}

Copy link

Member Author

dbhynds Sep 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add this to the .gitignore

dbhynds commented Sep 13, 2023

View reviewed changes

Abstract the code

2a34774

dbhynds assigned Watercycle Oct 6, 2023

erikdonohoo reviewed Oct 17, 2023

View reviewed changes

dbhynds added 6 commits October 20, 2023 15:50

Add shouldMigrate to migration db interface

2201519

Merge branch 'master' into mgmt-132-migrations

8aafc02

Rework some logic

661438d

Call migrate from validate so that we ensure the deployment exists

4b22395

Reorder Validation

a7d2446

JK

7feabf0

dbhynds added 2 commits October 27, 2023 15:18

Merge branch 'master' into mgmt-132-migrations

08dc5b5

Make a few adjustments

d2a22d7

dbhynds commented Nov 17, 2023

View reviewed changes

dbhynds requested review from erikdonohoo and snake November 17, 2023 17:57

dbhynds added 4 commits November 17, 2023 11:57

Lint

69e5836

Merge branch 'master' into mgmt-132-migrations

36db980

Abstract deployment check

8b1e590

Add tests

4729f38

Watercycle previously approved these changes Nov 30, 2023

View reviewed changes

PR feedback

bd917dc

dbhynds dismissed Watercycle’s stale review via bd917dc December 1, 2023 17:51

Lint

186e1f5

dbhynds requested a review from Watercycle December 1, 2023 19:44

dbhynds mentioned this pull request Dec 4, 2023

Initial preparation for v6.0 #105

Merged

3 tasks

Watercycle approved these changes Dec 4, 2023

View reviewed changes

dbhynds merged commit 1860139 into master Dec 4, 2023
3 checks passed

dbhynds deleted the mgmt-132-migrations branch December 4, 2023 19:08

MGMT-132: Supporting migrations #99

MGMT-132: Supporting migrations #99

Conversation

dbhynds commented Sep 6, 2023 • edited by erikdonohoo Loading

Summary of Changes

Testing

dbhynds commented Sep 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikdonohoo commented Sep 6, 2023

dbhynds commented Sep 6, 2023

dbhynds commented Sep 7, 2023 • edited Loading

erikdonohoo commented Sep 7, 2023

erikdonohoo commented Sep 13, 2023

erikdonohoo commented Sep 13, 2023

Choose a reason for hiding this comment

dbhynds commented Sep 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikdonohoo Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikdonohoo Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbhynds commented Oct 17, 2023

dbhynds commented Oct 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbhynds commented Nov 27, 2023

Watercycle left a comment

Choose a reason for hiding this comment

Watercycle Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbhynds commented Dec 1, 2023

Watercycle left a comment

Choose a reason for hiding this comment

dbhynds commented Sep 6, 2023 •

edited by erikdonohoo

Loading

dbhynds commented Sep 7, 2023 •

edited

Loading

erikdonohoo Oct 24, 2023 •

edited

Loading

erikdonohoo Oct 17, 2023 •

edited

Loading

Watercycle Nov 30, 2023 •

edited

Loading