Challenges in Validating LtiConfiguration Data

[michaelroytman]

Context:

In #260, support for database configuration for LTI 1.3 launches was added; these changes were put behind a CourseWaffleFlag. The CourseWaffleFlag is referenced in the LtiConsumerXBlock and in the LtiConfiguration classes. In the LtiConfiguration's clean method, the waffle flag is used to validate or clean incoming data to prevent the creation of an LtiConfiguration with a config_store value of CONFIG_ON_DB if the waffle flag is not enabled.

The problem is that the CourseWaffleFlag.is_enabled(<course_key>) method can only be called with a course_key. For LtiConfigurations with a None value for the location field, this raises an exception, because the location is used to load in the XBlock from the modulestore. The course_key is then retrieved from the XBlock. Without a location, there is no course_key, and CourseWaffleFlag.is_enabled(<course_key>) fails. This is only observed in the Django admin, because Model.full_clean() and Model.clean() are not called after Model.save().

I tried to use a WaffleFlag instead, thinking that I would sacrifice the added customizability of the CourseWaffleFlag in exchange for the ability of the flag to work outside the XBlock context. However, WaffleFlag requires there to be an available request object, and there isn't one when executing a model's clean method.

Problem:

Regardless, I believe that this kind of validation does not belong in the model clean method. This kind of business logic validation should occur before the database layer (e.g. in a Form class or in a DRF serializer). But we are not making use of Form classes or DRF serializers here. I would prefer that this check be in a centralized place within the library; however, I cannot easily determine where this should be because of the various ways LtiConfigurations can be created. The two questions I would like to answer are.

1. How can we use feature flags throughout the codebase in a way that does not assume an XBlock runtime context?

2. How can we ensure that the same kind of validation is applied in the three different ways LtiConfigurations can be created?

Requirements:

We want to ensure we do not save invalid data to LtiConfiguration in three places:

1. in the XBlock edit menu in Studio
Currently, this is handled by the xblock_studio_view.js Javascript. There is no backend enforcement.
We could add this waffle flag to validate_field_data.

2. in the Django admin
Currently, whatever is in the LtiConfiguration.clean() method will be run.

3. in any other places where we create LtiConfiguration instances, like the Python API
Currently, there is no validation of the data used to create LtiConfiguration instances when the Python API is used. This is because calling Model.clean() does not call Model.full_clean() and Model.clean().

It may happen that we decide to release "CONFIG_ON_DB" platform wide and remove the feature flag before edx-exams even needs it, in which case the only remaining issue is #2 under "Problems".

Other Notes:

1. In the Python API, there is no clear place to validate the incoming data. Because of the way that data is synced from the XBlock to the LtiConfiguration in _get_lti_config and _get_or_create_local_lti_config, any time a caller intends to fetch an LtiConfiguration, it may be updated as part of the fetch to sync it with the modulestore. It's unexpected for a function like get_lti_1p3_launch_info to have to handle a ValidationError if we were to add validation to _get_or_create_local_lti_config.

2. Currently, LtiConfiguration instances are created via the _get_or_create_local_lti_config API method, which is used throughout the codebase. Note that Model.full_clean() and Model.clean() are not actually called when Model.save() is called, so the model clean method only runs when making changes through the Django admin. With support for CONFIG_ON_DB, we need to support creating and editing LtiConfigurations from the Django admin while also being able to validate incoming data. It's possible to customize the model admin form to store the request and use a WaffleFlag, but it's hacky.

[ashultz0]

LTI configurations are not super high traffic or made by random people who don't understand anything

I think we could allow LTI config on DB to just gate the creation of config on DB in the UI but if someone gets around that via the admin that's not a big deal. Maybe we can just waffle flag that interface.

I agree that we absolutely cannot waffle flag the model or the model validation, that's a disaster. This data is allowed to be on the model, period. We can waffle flag users creating it, and if we're really worried we can also waffle flag using it so that even if you have it set you can't use it except in a course that enabled it. But we probably don't need the second one.

[zacharis278]

I think you bring up some good points on how these fields are validated but I haven't totally wrapped my head around it so I'll come back to that.

As far as the waffle flag. Now that you point out these issues I don't think this toggle should apply to every use of this library. We really just want to gate this in studio (having this available and studio was just for testing anyway, we're not sure if the UX is something we even want to release widely) so the toggle should really only apply in that context. Forcing another application that uses this library to implement a similar toggle seems wrong.

[michaelroytman]

I agree with Zach and Andy about the toggle. I can remove the toggle from LtiConfiguration.clean. It is already being used in the Studio UI to only display the database configuration as an option when the flag is enabled.

[michaelroytman]

The second part of the issue was that _get_or_create_local_lti_config doesn't call full_clean to validate the input data. Is there a reason we do not do this? I believe this is the only place in the codebase where we create and save LtiConfigurations (besides Django admin). Shouldn't we validate the data before saving it?

The thing that gives me pause is that _get_or_create_local_lti_config is called by _get_lti_config which is called from numerous places. One of which is get_lti_1p3_launch_info, which is used to display LTI configuration in the XBlock in Studio. If we were to call full_clean before save, and it were to raise a ValidationError It feels a little odd to handle that in a method whose role was to get some LTI data from the database. Why should the data be validated both on writing the data and on reading the data?

Actually, perhaps this is moot. Studio has good enough validation in the validate_field_data XBlock method. This covers the Studio case. For the CONFIG_ON_DB and CONFIG_EXTERNAL cases, are we going to trust users of the library to know to create valid LtiConfigurations?