Open giovannicimolin opened 2 years ago
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.
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.
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.
@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:
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 setuptype_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.Login hint: b6d9bf7f-26b1-4480-b98d-d956f1f88ac3:xblock:block-v1:edX+DemoX.1+2014+type@lti@block_id
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?
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: :warning: 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.
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.
edx-exams
.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.
LtiConfiguration.location
or LtiConfiguration.block
when determining access in some of the deep linking endpoints. Are we seeking to decouple these?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.
CONFIG_ON_XBLOCK
config_type
long term? Will the modulestore be the source of truth for this data? Reading https://github.com/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.location
field?Is this a goal of ours? I ask because it seems to be implied in this issue, but I'm not sure.
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.
launch_gate_endpoint
view from the XBlock by removing references to the LtiConfiguration.location
field.launch_gate_endpoint
view from the LMS by removing references to LMS functions via the compatibility layer.xblock-lti-consumer
library to pass contextual information to the library that was originally fetched from the LMS.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.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.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
.
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.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
Proctoring Parameters
LTI Advantage Parameters
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.
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.
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.
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?
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.
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?
@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.
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
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.
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.
@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. :+1:
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.
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).
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.
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://github.com/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...
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! https://github.com/openedx/xblock-lti-consumer/pull/281
@MichaelRoytman Thanks! I'll do a review pass today!
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 uselocation
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:
location
for LTI launches.The outcomes of this issue
Use cases
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