LTI 1.3 #1023

mpiraux · 2024-10-15T14:08:39Z

This PR updates the LTI support of INGInious to LTI 1.3, along with a couple of internal changes.

The support of LTI 1.1 is dropped by these changes so all integrations must be updated when integrating this code into running instances.

I'll detail the contributions as they are integrated into the PR.

7a3a4d6 has been tested with a test instance of the syllabus and INGInious with the existing LTI code. I tried submitting code to a task and browsing submissions.

9b1d6e5 updates the basic features (i.e. not the Outcome service) to LTI 1.3. This has been tested with an ongoing patch to the syllabus and saLTIre.

11ebad4 implements the LTI Assignment and Grade service to report grades back to the LTI Platform as a modern replacement to the Outcome service. This has been tested with the test tools of IMS Global as I couldn't configure saLTIre properly so the message carrying the grade could be authenticated by the platform. I suppose IMS Global checks this properly.

262f9fd gives a try at making a clear separation between regular Flask sessions and LTI sessions. Both are still backed by the database. The UserManager becomes the access point to the session now. Tested with the syllabus, saLTIre and IMS Global.

mpiraux · 2024-10-15T14:09:55Z

inginious/frontend/flask/mongo_sessions.py

@@ -68,7 +68,7 @@ def _get_signer(self, app):

    def open_session(self, app, request):
        # Check for cookieless session in the path
-        path_session = re.match(r"(/@)([a-f0-9A-F_]*)(@)", request.path)


I tried to look into the code for other instances of this kind of check by searching for "/@" . I think I found all of them.

inginious/frontend/pages/utils.py

inginious/frontend/app.py

mpiraux · 2024-10-16T08:47:28Z

Looks like the CI is broken btw.

anthonygego · 2024-10-16T09:33:37Z

Looks like the CI is broken btw.

Yep, we still have to fix that 😞

inginious/frontend/app.py

mpiraux · 2024-10-18T09:35:56Z

inginious/frontend/courses.py

-        """ {name: key} for the LTI customers """
-        return self._lti_keys if self._is_lti else {}
+    def lti_config(self):
+        """ LTI Tool config dictionary. Specs are at https://github.com/dmitry-viskov/pylti1.3/blob/master/README.rst?plain=1#L70-L98 """


There will be documentation part of this PR on this.

setup.py

mpiraux · 2024-10-21T12:38:19Z

inginious/frontend/lti_grade_manager.py

+from pylti1p3.launch_data_storage.base import LaunchDataStorage
+
+
+class MongoLTILaunchDataStorage(LaunchDataStorage):


This could be used to store only the LTI Launch ID in the user session and retrieve the rest from the database.

anthonygego

Globally this LGTM. The comments are for me, except if you want to address them in the meantime.

I won't merge it now as I'd rather have this merged in a clean main branch with tasksets removed, but it will as soon as this new branch is ready. I should also validate this on several platforms before being sure nothing has been missed.

As we retire LTI1.1 in the same operation, we'll probably also have to produce migration scripts for LTI configurations. That could include automated key generation.

anthonygego · 2025-02-25T08:51:44Z

inginious/frontend/plugin_manager.py

@@ -93,7 +93,7 @@ def add_page(self, pattern, classname_or_viewfunc):
        if not self._loaded:
            raise PluginManagerNotLoadedException()

-        self._flask_app.add_url_rule("/<cookieless:sessionid>" + pattern[1:], view_func=classname_or_viewfunc)
+        self._flask_app.add_url_rule("/" + pattern[1:], view_func=classname_or_viewfunc)


Suggested change

self._flask_app.add_url_rule("/" + pattern[1:], view_func=classname_or_viewfunc)

self._flask_app.add_url_rule(pattern, view_func=classname_or_viewfunc)

anthonygego · 2025-03-03T10:01:26Z

inginious/frontend/user_manager.py

+                flask.g.lti_session = self._database.lti_sessions.find_one({'session_id': flask.request.args['session_id']})
+        if 'lti_session' not in flask.g or not flask.g.lti_session:
+            flask.g.lti_session = {}
+        return flask.g.lti_session


We should document somewhere that flask.g has the same lifetime than a request and that it should therefore be safe to use it this way in a multi-process, multi-user production deployment.

anthonygego · 2025-03-03T11:10:30Z

inginious/frontend/user_manager.py

+        if app.user_manager.session_is_lti():
+            lti_session_id = flask.request.args.get('session_id', flask.g.get('lti_session_id'))
+            app.user_manager._database.lti_sessions.find_one_and_update({'session_id': lti_session_id}, {'$set': flask.g.lti_session}, upsert=True)
+        # TODO(mp): Find whether the session should be dropped instead?


Based on what is done in mong_sessions.py, the session should be dropped at the opening stage, according to the expiration time.

anthonygego · 2025-03-04T07:34:39Z

inginious/frontend/courses.py

+                    yield '/'.join([iss, client_config['client_id'], deployment_id])
+
+    def lti_keyset_hash(self, issuer: str, client_id: str) -> str:
+        return hashlib.md5((issuer + client_id).encode('utf-8')).digest().hex()


This is not probable but maybe we should add a delimiter char to avoid conflicts with different sets of issuer and client ids.

anthonygego · 2025-03-04T07:42:11Z

inginious/frontend/flask/mongo_sessions.py

-            cookieless = False
-            sid = request.cookies.get(self.get_cookie_name(app))
+        if lti_session or is_lti_launch:
+            return None


My understanding here is that, before, we returned the result of self.session_class(sid=None) while here we return directly None. Is that equivalent ?

anthonygego · 2025-03-04T07:45:56Z

inginious/frontend/pages/utils.py

@@ -211,7 +220,7 @@ def POST(self, *args, **kwargs):
            if not self.user_manager.session_username() and not self.__class__.__name__ == "ProfilePage":
                return redirect("/preferences/profile")

-            if not self.is_lti_page and self.user_manager.session_lti_info() is not None:  # lti session
+            if not self.is_lti_page and self.user_manager.session_lti_info() is not None:  # lti session, TODO(mp): Not sure whether it is still needed


Suggested change

if not self.is_lti_page and self.user_manager.session_lti_info() is not None: # lti session, TODO(mp): Not sure whether it is still needed

if not self.is_lti_page and self.user_manager.session_lti_info() is not None: # lti session

anthonygego · 2025-03-04T08:26:59Z

inginious/frontend/pages/lti.py

-                                               data=None, error=_("Missing LTI session id"))
+    def _get_lti_session_data(self):
+        if not self.user_manager.session_is_lti():
+            return self.template_helper.render("lti_bind.html", success=False, data=None, error=_("Missing LTI session id"))


The method returns a tuple below, not here. Besides, I would avoid returning rendered templates directly and try to gather these calls to render in one unique method, even if this is not the case for most of the existing code.

anthonygego · 2025-03-04T08:32:34Z

inginious/frontend/lti_grade_manager.py

+        super().__init__(*args, **kwargs)
+
+    def can_set_keys_expiration(self) -> bool:
+        return False  # TODO(mp): I think it's reasonable to clean LTI Launch messages further than a week away tho


anthonygego · 2025-03-04T08:39:58Z

inginious/frontend/pages/lti.py

+        can_report_grades = message_launch.has_ags() and tool_conf.get_iss_config(iss=message_launch.get_iss(),
+                                                                                  client_id=message_launch.get_client_id()).get('auth_token_url')
+
+        session_id = hashlib.sha256(launch_id.encode('utf-8')).digest().hex()  # TODO(mp): Make this more secure


This depends on what the launch id is based on. However, as it is probably shared externally, it would indeed be more secure to add some private hint in the session id generation to avoid session hijacking. Maybe based on the flask session secret parameter.

anthonygego · 2025-03-04T08:52:19Z

inginious/frontend/lti_grade_manager.py

+
+                    if ags.can_put_grade():
+                        sc = Grade()
+                        # TODO(mp): Is there a better timestamp to set with the score? Submission time? Grading time?


Maybe it's better to always set the score as the latest for the platform (and therefore keep the datetime.now()). This way we ensure the score shown on INGInious is the one that is stored by the platform.

mpiraux commented Oct 15, 2024

View reviewed changes

inginious/frontend/pages/utils.py Outdated Show resolved Hide resolved

mpiraux commented Oct 15, 2024

View reviewed changes

inginious/frontend/app.py Show resolved Hide resolved

mpiraux force-pushed the lti1.3 branch 4 times, most recently from 3591e25 to 609a552 Compare October 15, 2024 14:40

Reworks the cookieless session id from path to parameters

29067a6

mpiraux force-pushed the lti1.3 branch from 609a552 to 29067a6 Compare October 15, 2024 14:44

Implements basic LTI 1.3

9b1d6e5

mpiraux force-pushed the lti1.3 branch from e32e666 to 9b1d6e5 Compare October 18, 2024 09:29

mpiraux commented Oct 18, 2024

View reviewed changes

inginious/frontend/app.py Outdated Show resolved Hide resolved

mpiraux commented Oct 18, 2024

View reviewed changes

setup.py Show resolved Hide resolved

Implements LTI AGS to report grade back to the LTI Platform

11ebad4

mpiraux commented Oct 21, 2024

View reviewed changes

Separates LTI sessions from regular sessions

262f9fd

mpiraux force-pushed the lti1.3 branch from ec714b8 to b07487f Compare October 22, 2024 13:09

Misc

e2200fd

mpiraux force-pushed the lti1.3 branch from b07487f to e2200fd Compare October 22, 2024 14:45

nrybowski force-pushed the master branch 2 times, most recently from e53b0e2 to 9a6d192 Compare December 21, 2024 13:51

anthonygego marked this pull request as ready for review February 25, 2025 07:17

anthonygego approved these changes Mar 4, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LTI 1.3 #1023

LTI 1.3 #1023

mpiraux commented Oct 15, 2024 •

edited

Loading

mpiraux Oct 15, 2024

mpiraux commented Oct 16, 2024

anthonygego commented Oct 16, 2024

mpiraux Oct 18, 2024

mpiraux Oct 21, 2024

anthonygego left a comment

anthonygego Feb 25, 2025

anthonygego Mar 3, 2025

anthonygego Mar 3, 2025

anthonygego Mar 4, 2025

anthonygego Mar 4, 2025

anthonygego Mar 4, 2025

anthonygego Mar 4, 2025

anthonygego Mar 4, 2025

anthonygego Mar 4, 2025

anthonygego Mar 4, 2025

		from pylti1p3.launch_data_storage.base import LaunchDataStorage


		class MongoLTILaunchDataStorage(LaunchDataStorage):

	self._flask_app.add_url_rule("/" + pattern[1:], view_func=classname_or_viewfunc)
	self._flask_app.add_url_rule(pattern, view_func=classname_or_viewfunc)

	if not self.is_lti_page and self.user_manager.session_lti_info() is not None: # lti session, TODO(mp): Not sure whether it is still needed
	if not self.is_lti_page and self.user_manager.session_lti_info() is not None: # lti session

LTI 1.3 #1023

Are you sure you want to change the base?

LTI 1.3 #1023

Conversation

mpiraux commented Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

mpiraux commented Oct 16, 2024

anthonygego commented Oct 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anthonygego left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpiraux commented Oct 15, 2024 •

edited

Loading