Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove XBlock location dependencies from LTI 1.3 launches #273

Open
giovannicimolin opened this issue Aug 12, 2022 · 14 comments
Open

Remove XBlock location dependencies from LTI 1.3 launches #273

giovannicimolin opened this issue Aug 12, 2022 · 14 comments

Comments

@giovannicimolin
Copy link
Contributor

giovannicimolin commented Aug 12, 2022

In order to make LTI 1.3 re-usable, we need to remove any dependencies on location from the LTI configuration model and from the launch flow. We currently use location to get contextual information and determine user permissions before an LTI launch.

This is being discussed in a few different places, so I wanted to link them all together here:

We need to:

  • Remove any hard dependencies on location for LTI launches.
  • Add some way for whatever is launching the LTI tool to pass the current context.
  • Implement some form of permission checking for each context/launch type.

The outcomes of this issue

  • An ADR should be written to cover technical details about permission checking in different contexts and how the context will be passed to LtiConfiguration.
  • LTI Consumer should be able to launch LTI content in the given contexts: XBlock, re-usable XBlock configuration, and directly through the Django APIs.
  • ???

Use cases

  • Do content launches through XBlocks
  • Re-using configurations and enabling LTI 1.3 launches from multiple blocks (and keeping context/grades separate)
  • Do content launches outside a block's context (e.g.: LTI course tabs, LTI 1.3 embedded discussions)
  • Do content launches on separate a IDA (e.g.: proctoring)

Notes:
I don't think we need to get everything working and fixed in one PR, this can be divided and worked on in smaller steps.


@tecoholic @Zacharis278 @MichaelRoytman @schenedx @ormsbee

@Zacharis278
Copy link
Contributor

Thanks for centralizing this. We have an internal 2U ticket to work out a solution and propose an ADR. We plan to get started on that right away since it blocks launching exams (they don't have a specific xblock). Scheduling wise I hope we can have something proposed or functional by September.

@Zacharis278
Copy link
Contributor

Wanted the throw out a solution that came up when looking at the proctoring PR. Can we solve this by building a context object that includes location and storing that is part of the lti_message_hint (serialized as a JWT). That way when we do the actual launch we don't have to reach out to another part of the system to get course context, role, etc.

It isn't totally clear to me if that's the intention of this parameter based on the IMS spec but I'm leaning that way.

@MichaelRoytman
Copy link
Contributor

MichaelRoytman commented Aug 25, 2022

I like that idea, actually. We're using the login_hint for a similar function in the LTI 1.3 launches by storing the XBlock usage_key, and lti_message_hint is definitely more appropriate for that than login_hint.

The specification seems pretty generic: the lti_message_hint is there "to carry information about the actual LTI message that is being launched". I think it's a completely apt use of the parameter. In my proctoring implementation, I am using it to store the type of the LTI launch message that will be sent once the authentication request is received from the tool; this would just expand it to include more data.

I'm going to see whether this would work for our existing LTI 1.3 launches and report back.

@giovannicimolin
Copy link
Contributor Author

giovannicimolin commented Aug 25, 2022

@MichaelRoytman @Zacharis278

The specification seems pretty generic: the lti_message_hint is there "to carry information about the actual LTI message that is being launched". I think it's a completely apt use of the parameter. In my proctoring implementation, I am using it to store the type of the LTI launch message that will be sent once the authentication request is received from the tool; this would just expand it to include more data.

Yes, that's the idea behind those two parameters (login_hint and lti_message_hint). It is mandatory for tools to send back the exact values on these variables without applying any transformations. So yes, we can use this to pass data around and locate the LTI configurations.

Currently, we're passing the block_id (code here) in the hint, with two different purposes:

  1. Locate and identify the LTI configuration (code here to retrieve configuration and set up for launch)
  2. Using it to check permissions against that specific course by retrieving the user role (code here).

Part of making LTI configurations re-usable is separating those two usages into different identifiers.
Since we want to remove the dependency from the block, we could use login_hint kinda like this: lti_configuration_uuid : type_of_context : context_identifier (as a single string).

  • lti_configuration_uuid would be the LtiConfiguration identifier, just used as a reference for the configuration and consumer setup
  • type_of_context would be a pointer to tell the LtiConfiguration how to get contextual information (permissions, user role, etc)
  • context_identifier would be where that context information is coming from.

Example

Login hint: b6d9bf7f-26b1-4480-b98d-d956f1f88ac3:xblock:block-v1:edX+DemoX.1+2014+type@lti@block_id

  • Config id: b6d9bf7f-26b1-4480-b98d-d956f1f88ac3
  • Context type: XBlock.
  • Context identifier: block-v1:edX+DemoX.1+2014+type@lti@block_id.

A different block using the same configuration would have a similar message, but with a different context identifier:

b6d9bf7f-26b1-4480-b98d-d956f1f88ac3:xblock:block-v1:edX+DemoX.1+2014+type@lti@a_different_block_id

We can also imagine the same configuration being used as a discussion provider: in that case, the type_of_context value would also be different so that the consumer would know where to find the context and permissions.

b6d9bf7f-26b1-4480-b98d-d956f1f88ac3:course_wide_lti_integration:block-v1:edX+DemoX.1+2014+type@lti@a_different_block_id

I'm not sure I'm covering all bases with this suggestion, but I think it's a good starting point. What do you say?

Final notes

Note that the lti_message_hint is already being used in some places (see this section of the code) to identify what kind of launch is being done (normal, deep linking setup, deep linking content, etc).

We should definitely modify the current usages to improve consistency in the usage of login_hint and lti_message_hint along with these changes.


EDIT: ⚠️
I actually missed this comment about serializing the context as a JWT, this is a great idea too!

Wanted the throw out a solution that came up when looking at the proctoring PR. Can we solve this by building a context object that includes location and storing that is part of the lti_message_hint (serialized as a JWT). That way when we do the actual launch we don't have to reach out to another part of the system to get course context, role, etc.

@MichaelRoytman
Copy link
Contributor

Thanks for the additional context, @giovannicimolin. That's very helpful.

I have a number of questions. I'd like to propose answers to some of these problems, but I'm not sure whether they are problems that we're seeking to solve here.

Contextual Information in login_hint

How are you anticipating using the login_hint to get the contextual information? From the launch_gate_endpoint, would we have something like this? This is just rough code.

login_hint = request.GET.get("login_hint")
config_uuid = login_hint.split("-")[0]
context_type = login_hint.split("-")[1]
context_identifier = login_hint.split("-")[2]

lti_config = LtiConfiguration.get(config_id=config_uuid)
lti_consumer = lti_config.get_lti_consumer()

...

if context_type == constants.XBLOCK_CONTEXT_TYPE:
    usage_key = UsageKey.from_string(context_identifier)
    course_key = usage_key.course_key
    course = compat.get_course_by_id(course_key)
    user_role = compat.get_user_role(request.user, course_key)
    external_user_id = compat.get_external_id_for_user(request.user)
elif context_type == constants.<OTHER_CONTEXT_TYPE>:
    ...

This does decouple this launch view from the location when determining the contextual information, but it brings up a few other questions.

  1. What other context types exist? Is there a discrete set of context types?
  2. We are still coupled to the LMS here if the context type is XBlock both through the block itself and through the compatibility layer. Are we going to have a branch in the code for each context type? I think that could get pretty complex as we add context types. I think Zach's suggestion could push the LMS coupling into the XBlock, which I describe below.
  3. I think we need a solution for how to get this contextual data from the context. It's clear how to do that in the XBlock context, but what about the case where this library is installed in a IDA? I don't think it's sustainable to have a compatibility layer for each context type unless it's very well defined and scoped to the platform, which would exclude microservices like edx-exams.
  4. The login_hint has to be sent with the OIDC third-party login initiation request, which means the login_hint would be created from get_lti_1p3_content_url method. Doesn't this mean that each context would have to be self-aware? The library needs to know what context get_lti_1p3_content_url is being called from, so the XBlock would have to pass "XBlock" as a parameter to get_lti_1p3_content_url. Am I understanding that correctly?

Contextual Information in lti_message_hint

If we go with this approach, we can encode all contextual information in the lti_message_hint, including role and permissions information, instead of information for how to get it later. And because this hint is first sent in the OIDC third-party login initiation request, it can be determined earlier in the flow, instead of in the authentication request handler. The contextual environment can call the get_lti_1p3_content_url method with kwargs that define this contextual information. For example, the XBlock can call get_lti_1p3_content_url from _get_lti_block_launch_handler. edx-exams can call get_lti_1p3_content_url from edx-exams. This way, the contextual information is determined in the contextual environment and fed through the LTI launch flow. Therefore, the compatibility layer is moved into the contextual environment; XBlock imports from edx-platform; edx-exams imports from edx-exams, etc. What do you think?

My only concern is that the consuming context has to concern itself with the various LTI claims in order to determine what context to send. For example, the Proctoring flow has a lot of optional claims.

I'm also not sure how this would apply to Advantage services. I'm not as familiar with them. Are we sticking just to the LTI 1.3 launch here? Skimming some of the Advantage service views, the following questions come to mind.

  1. I see that we refer to either the LtiConfiguration.location or LtiConfiguration.block when determining access in some of the deep linking endpoints. Are we seeking to decouple these?
  2. If this library will be used in contexts besides the platform, this makes the use of CourseWaffleFlag more challenging. How will that impact things like the CourseAllowPIISharingInLTIFlag?

Decouplinglocation from LtiConfiguration

In the issue, you say, "we need to remove any dependencies on location from the LTI configuration model and from the launch flow". Are you saying that you would like to see the location field removed from the LtiConfiguration model? If so, that's a problem that will need its own unique solution. If we remove location from LtiConfiguration, the LtiConfiguration will have no way to read data from the XBlock when creating the LTI consumer class. If you would like to see this happen, there are some follow up questions.

  1. Are we planning to maintain the CONFIG_ON_XBLOCK config_type long term? Will the modulestore be the source of truth for this data? Reading https:/openedx/xblock-lti-consumer/blob/master/docs/decisions/0001-lti-extensions-plugin.rst, it sounds like the future vision is to deprecate CONFIG_ON_XBLOCK, but recent conversations suggest that this has changed.
  2. I vaguely recall conversations about splitting this library into two - the XBlock and the LTI consumer library. If so, won't we need to remove the location field?

Is this a goal of ours? I ask because it seems to be implied in this issue, but I'm not sure.

@MichaelRoytman
Copy link
Contributor

Hello,

I think there are a few steps here. I've summarized them below and then go into further detail about my proposal for solving them later on.

Let me know what you think. I can draft up an ADR once we align on an approach. I'm also happy to do some work on a rough POC to demonstrate how this would work.

Steps


  1. We need decouple the launch_gate_endpoint view from the XBlock by removing references to the LtiConfiguration.location field.
  2. We need to decouple the launch_gate_endpoint view from the LMS by removing references to LMS functions via the compatibility layer.
  3. We need to provide mechanisms for users of the xblock-lti-consumer library to pass contextual information to the library that was originally fetched from the LMS.
  4. We need provide mechanisms for users of the xblock-lti-consumer library to determine contextual information that is currently stored in the LMS. I think this is outside the scope of this issue.
  5. We need to remove the location field of the LtiConfiguration model. I am not entirely sure that we want this. I'd appreciate some thoughts on the longer term vision of the library here.

1. Decouple the launch_gate_endpoint From the XBlock


I like the idea of using the lti_message_hint query parameter for this.

I propose the following uses of login_hint and lti_message_hint.

  1. Use the login_hint to store the requesting user's identifier. In our case, this would be the ExternalId associated with that learner. This is in closer alignment to the LTI specification, I believe; although, it is still fairly vague. We are meant to compare the login_hint echoed back to us by the Tool to the currently authenticated user.
  2. Use the lti_message_hint to store a JWT that encodes contextual information provided by the user of the library and that is necessary for the LTI launch.

The purpose of using the lti_message_hint is that it allows the preflight request (generated via the get_lti_1p3_content_url function) to pass this information to the launch_gate_endpoint view via the tool.

There is a lot of contextual information that can be sent this way, and much of it is optional.

For now, I propose that we include the following information, which is the information included in the current LTI launch.

Base LTI 1.3 Parameters

  • config_id
  • user_id
  • role
  • resource_link_id
  • context_id
  • context_title
  • context_label
  • message_type

Proctoring Parameters

  • attempt_number

LTI Advantage Parameters

  • I haven't looked through this in detail yet.

The advantage of this approach is that it moves the responsibility of defining the contextual information to the code that handles the OIDC third-party authentication initiation. This is much closer to the origination of the LTI request, and so it makes it easier to decouple the launch from the LMS in a later step and makes it easier for other users of the library to provide this contextual information.

2. Decouple the OIDC Third-Party Authentication Initiation From the LMS


The above strategy moves the responsibility of defining the contextual information to the code that handles the OIDC third-party authentication initiation. Currently, that's the the get_lti_1p3_content_url function.

Currently, the function has the following signature.

get_lti_1p3_content_url(config_id=None, block=None, hint="")

I propose that we modify the signature to include a number of keyword arguments for contextual information that are necessary for an LTI launch. These data will map directly to parameters I proposed that we include in the lti_message_hint above. The parameters will be encoded into a JWT, and the JWT will be sent in the lti_message_hint parameter in the OIDC third-party authentication initiation request.

For example, the function could have the following signature.

get_lti_1p3_content_url(
	config_id=None,
	block=None,
	user_id,
	role,
	resource_link_id,
	context_id=None,
	...
)

There are a lot of claims in LTI. Some are required. Some are optional. Some are specification specific. This has the potential to balloon the signature of this function.

3. Provide Mechanisms for Users of Library to Pass Contextual Information


There are a few ways to accomplish this.

XBlock Context

The XBlock does not have to change much. We can push the use of the compatibility layer into the XBlock.

Here is a quick example of what I am thinking. I've removed some of the irrelevant parts related to LTI 1.1 launches.

    def _get_lti_block_launch_handler(self):
        """
        Return the LTI block launch handler.
        """
        ....
        # LTI 1.3 launch
        else:
            # Runtime import since this will only run in the Open edX LMS/Studio environments.
            from lti_consumer.api import get_lti_1p3_content_url  # pylint: disable=import-outside-toplevel

            course_key = self.block.location.course_key
    		course = compat.get_course_by_id(course_key)
    		user_role = compat.get_user_role(request.user, course_key)
    		external_user_id = compat.get_external_id_for_user(request.user)

    		context_title = " - ".join([
            	    course.display_name_with_default,
            	    course.display_org_with_default
        	])

            # Retrieve and set LTI 1.3 Launch start URL
            lti_block_launch_handler = get_lti_1p3_content_url(
                block=self,
                user_id=external_user_id,
                role=user_role,
                resource_link_id=self.location,
                context_id=course_key,
                ...,
            )

        return lti_block_launch_handler

Other Contexts Via lti_embed Function

We can mirror the lti_embed function and expose a similar function that returns an auto-submitting form that kicks off the LTI 1.3 launch. The set of parameters for this function would be very similar to that of the get_lti_1p3_content_url function.

Other Contexts Via Python API

For other application contexts where it's preferable for, say, an MFE to have control over the frontend experience, or where we want more control over when the request is made, we can use the get_lti_1p3_content_url function of the Python API to get a preflight URL and then do a simple redirect within the application. This assumes that the size of the query parameters, including the JWT, will be under the maximum character limit for a GET request.

4. Provide Mechanisms To Get LMS Contextual Information From Outside the Library


I think this is outside the purview of this issue or this library, but I want to surface it as a problem that will have to be solved.

Both the user_id and the user_role that are currently retrieved from the LMS via the compatibility layer will have to be accessible from contexts outside the LMS (like edx-exams). I was hoping that contexts outside the LMS could leverage the data included in the authentication JWT, but the data in the JWT is not sufficient. I brainstormed a few options, but I don't want to clutter this issue with them. @Zacharis278 I can create a follow up discovery story to outline how we want to handle this; what do you think?

5. Remove the Location Field From the LtiConfiguration Model


I'm still not sure whether we want to do this. It looks like the remaining places where we reference the location is in a number of LTI Advantage features.

@ashultz0
Copy link
Contributor

What does encoding things as a JWT get us? Is it just a convenient way to get a string that can pass through all the various layers?

@MichaelRoytman
Copy link
Contributor

@ashultz0 Yes, essentially. We can encode it however we want; it just has to be a string. It does not have to be a JWT.

@ashultz0
Copy link
Contributor

for 2 I would suggest not ballooning the signature and instead making an actual object type to wrangle all the lti fields together that callers can build

@Zacharis278
Copy link
Contributor

Zacharis278 commented Aug 31, 2022

Initial impression, this all looks great!

To build on Andy's suggestion. We could build out a 'claims' object (whatever we choose to name it) using a series of setter functions for logical groups of claims. That way we might have some control over 'if this field is set that field is also required' and other validations. Maybe simply allowing additional_claims at a certain point for niche cases to provide flexibility?

As far as 4 and 5. I think these are both getting into a long vision for the library where the actual XBlock code is split out of this. I do think that should be our aim but IMO we can approach this incrementally by focusing on 1-3 now. My recommendation is to simply open it as an issue on library so our intent is documented but my understanding (correct me if I'm wrong) is we don't actually need to solve this right away.

@ashultz0
Copy link
Contributor

ashultz0 commented Sep 1, 2022

I was actually thinking about how weird it is that the xblock is in here while considering how do we turn off features we're not using. Maybe the correct layout would be lti library with most of this stuff, maybe in multiple apps that you could turn on and off, and separate lti xblock library that depends on it.

@giovannicimolin
Copy link
Contributor Author

giovannicimolin commented Sep 6, 2022

@MichaelRoytman

What other context types exist? Is there a discrete set of context types?

I can think of XBlock, Course, and Django so far - what I had in mind was to add support as we go, but we might be better served by flipping this around and enabling some form of pluggability as suggested on your post.

We are still coupled to the LMS here if the context type is XBlock both through the block itself and through the compatibility layer. Are we going to have a branch in the code for each context type? I think that could get pretty complex as we add context types. I think Zach's suggestion could push the LMS coupling into the XBlock, which I describe below.

Yup, that was one of the options. Pushing it to the context using it is definitely the better option, but we need mechanisms to pass and retrieve context information that is not dependent on the launch itself (for LTI advantage services, for example).

The login_hint has to be sent with the OIDC third-party login initiation request, which means the login_hint would be created from get_lti_1p3_content_url method. Doesn't this mean that each context would have to be self-aware? The library needs to know what context get_lti_1p3_content_url is being called from, so the XBlock would have to pass "XBlock" as a parameter to get_lti_1p3_content_url. Am I understanding that correctly?

Yes, that's it. 👍

My only concern is that the consuming context has to concern itself with the various LTI claims in order to determine what context to send. For example, the Proctoring flow has a lot of optional claims.

Each context should know what variables it needs to send to make an LTI launch! But as a safeguard we can add validation on the library to the parameters sent.

I'm also not sure how this would apply to Advantage services. I'm not as familiar with them. Are we sticking just to the LTI 1.3 launch here? Skimming some of the Advantage service views, the following questions come to mind.

Each advantage service has it's own set of challenges, and in order to get Open edX LTI certified we need to fully support all three on at least one setting.

  1. LTI-NRPS: It's just an endpoint that allows retrieving user information. The challenge here is limiting the amount of information that is sent back to the tool (this is a server-to-server API, so we don't have access to the context variables to limit the data in which the tool can access - the solution here is to track the contexts in which a tool was used and limit access to user information from those tracked contexts).

  2. LTI-AGS: The tools can either have full control of grades (programmatic access) or partial control (declarative), we need to account for both use cases (my recommendation would be to support the programmatic access only since it's easier to handle permissions that way). The second challenge is to send grades/information back to the context, which is easily solvable by (1) a signal emitted by the grades service when a grade is posted and (2) something on the context side that catches that signal, verifies if it matches the context and stores the grade.

  3. LTI-DL: This is the trickier one, because it allows a kind of custom LTI launch for each placement. To make it re-usable, we will need to replace the location with a context identifier that will be used by contexts to display LTI content. This identifier needs to be unique, and be sent from each context trying to do a launch. Example: consider two blocks using the same LTI integration, but configured differently through LTI-DL. Each placement needs to have a unique identifier so that the LTI consumer knows which Deep Linking "configuration" to use when presenting content to end users.

Edit: an extra note: it doesn't make sense to have all Advantage services working everywhere. For XBlocks it sure makes sense, but whats the use of grades outside a learning context, or access to a student list (NRPS) from a proctoring service? We can star by only worrying about actual use cases for each service to avoid the extra complications that come with making them globally available.

I see that we refer to either the LtiConfiguration.location or LtiConfiguration.block when determining access in some of the deep linking endpoints. Are we seeking to decouple these?

Initially no, but to get LTI certified we need to decouple those as well.

If this library will be used in contexts besides the platform, this makes the use of CourseWaffleFlag more challenging. How will that impact things like the CourseAllowPIISharingInLTIFlag?

I don't think we should use CourseWaffleFlag for enabling and disabling features - each context should display/hide/enable/disable the features it doesn't want to use from the consumer.
The problem lies specifically on CourseAllowPIISharingInLTIFlag - how do we pass the context to server2server calls? See my remarks on the LTI-NRPS item above.

In the issue, you say, "we need to remove any dependencies on location from the LTI configuration model and from the launch flow". Are you saying that you would like to see the location field removed from the LtiConfiguration model? If so, that's a problem that will need its own unique solution. If we remove location from LtiConfiguration, the LtiConfiguration will have no way to read data from the XBlock when creating the LTI consumer class. If you would like to see this happen, there are some follow up questions.

Yup, that's the idea, but only once we get the LTI 1.3 re-usable working. Currently we are relying on this field for limiting which data LTI Advantage services have access to.

Are we planning to maintain the CONFIG_ON_XBLOCK config_type long term? Will the modulestore be the source of truth for this data? Reading https:/openedx/xblock-lti-consumer/blob/master/docs/decisions/0001-lti-extensions-plugin.rst, it sounds like the future vision is to deprecate CONFIG_ON_XBLOCK, but recent conversations suggest that this has changed.

If the goal is to make this library completely independent and push the configuration responsibility to the context, then no, we should plan to deprecate CONFIG_ON_XBLOCK.


LTI Advantage Parameters
I haven't looked through this in detail yet.

Come to think of this, we can pass parameters when calling the API methods that define the behavior and limitations of the LTI Advantage services and store those settings somewhere on the DB. I haven't thought this one through yet though...

@MichaelRoytman
Copy link
Contributor

I apologize for forgetting to post this here, but I have a draft ADR up that addresses many concerns in this issue. Please review, if you are able to! #281

@giovannicimolin
Copy link
Contributor Author

@MichaelRoytman Thanks! I'll do a review pass today!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Enhancements and Fixes - In Progress
Development

No branches or pull requests

4 participants