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

Development: Fix LTI on multi node systems #9085

Merged
merged 33 commits into from
Aug 4, 2024
Merged

Conversation

Strohgelaender
Copy link
Contributor

@Strohgelaender Strohgelaender commented Jul 18, 2024

Checklist

General

Server

  • Important: I implemented the changes with a very good performance and prevented too many (unnecessary) database calls.
  • I strictly followed the server coding and design guidelines.
  • I added multiple integration tests (Spring) related to the features (with a high test coverage).
  • I added pre-authorization annotations according to the guidelines and checked the course groups for all new REST Calls (security).
  • I documented the Java code using JavaDoc style.

Motivation and Context

LTI does not work on multinode setups.
Fixes #9017.

Description

I copied the code of the default StateAuthorizationRequestRepository (which sadly is a final class) into a new DistributedStateAuthorizationRequestRepository that now uses Hazelcast to share the content between nodes.

Steps for Testing

Use a multinode testserver like TS3 (see https://confluence.ase.in.tum.de/pages/viewpage.action?pageId=25252245).

  1. An Admin must connect the testserver with moodle (https://docs.artemis.cit.tum.de/user/lti.html).
  2. Activate "Online Course" for your course (Course Management -> Edit -> Checkbox at the bottom).
  3. Link exercises to moodle (follow the documentation on https://docs.artemis.cit.tum.de/user/lti.html#linking-artemis-exercises)

Testserver States

Note

These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.







Review Progress

Performance Review

  • I (as a reviewer) confirm that the server changes (in particular related to database calls) are implemented with a very good performance

Code Review

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

Test Coverage

Summary by CodeRabbit

  • New Features

    • Introduced enhanced security configurations with distributed data management using Hazelcast.
    • Added a new repository for managing OAuth2 authorization requests in a distributed environment.
    • Improved LTI 1.3 security filter configurations and logging capabilities for better traceability.
  • Improvements

    • Refined error handling in the LTI 1.3 launch filter for better user experience.
    • Clarified naming conventions for JWT lifetime constants to improve code readability.
    • Enhanced the scalability and robustness of state management in security configurations.
  • Bug Fixes

    • Corrected documentation in the login cookie method for better clarity.

@github-actions github-actions bot added the server Pull requests that update Java code. (Added Automatically!) label Jul 18, 2024
Copy link
Contributor

@JohannesStoehr JohannesStoehr left a comment

Choose a reason for hiding this comment

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

Code

@krusche krusche modified the milestones: 7.4.6, 7.5.0 Jul 28, 2024
Copy link
Collaborator

@MaximilianAnzinger MaximilianAnzinger left a comment

Choose a reason for hiding this comment

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

In general I like the logging you did here. However, I'm not sure if all of it should be logged as info?

@Strohgelaender
Copy link
Contributor Author

In general I like the logging you did here. However, I'm not sure if all of it should be logged as info?

I had it at debug it first, but Stephan requested it to be in info level. LTI is not used often (which makes it a prime candidate for some regressions to sneak in) and in these cases gaining information about the LTI process should be as simple as possible.

Copy link
Member

@maximiliansoelch maximiliansoelch left a comment

Choose a reason for hiding this comment

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

Didn't look at the code, but tested the PR on the staging server and it now works as expected 👍

@krusche krusche added ready to merge maintainer-approved The feature maintainer has approved the PR labels Aug 4, 2024
@krusche krusche changed the title General: Fix LTI on multinode systems Development: Fix LTI on multi node systems Aug 4, 2024
@krusche krusche merged commit 8cc18bb into develop Aug 4, 2024
65 of 73 checks passed
@krusche krusche deleted the bugfix/lit-multinode branch August 4, 2024 07:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintainer-approved The feature maintainer has approved the PR ready for review ready to merge server Pull requests that update Java code. (Added Automatically!)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Error when linking exercises via LTI
6 participants