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

PADV-1394: Improve LTI profile user #39

Merged
merged 1 commit into from
Jun 21, 2024
Merged

PADV-1394: Improve LTI profile user #39

merged 1 commit into from
Jun 21, 2024

Conversation

kuipumu
Copy link
Contributor

@kuipumu kuipumu commented Jun 13, 2024

Tickets

Description

This PR improves the generation of the LtiProfile user, more specifically the LtiProfile user username and email structure, additionally the LtiProfile name property was modified and the name used on the user profile was truncated to 255 characters in case a very large name is used to avoid error with the field max_length.

Type of Change

  • Added shortuuid base requirement.
  • Added LtiProfile short_uuid property.
  • Added LtiProfile given_name property.
  • Added LtiProfile middle_name property.
  • Added LtiProfile family_name property.
  • Added LtiProfile user_collision method.
  • Added LtiProfile configure_user method.
  • Modify LtiProfile name property.
  • Modify LtiProfile save method.
  • Add or modify any unit test for all related code.

Testing:

  1. Run the LMS: make dev.up.lms.
  2. Run the learning MFE on the host machine: npm run start.
  3. Install openedx_lti_tool_plugin on the LMS.
  4. Run ngrok or any other similar service on LMS: ngrok http 18000
  5. Run ngrok or any other similar service on learning MFE: ngrok http 2000
  6. Add required settings to LMS.
# Enable LTI Tool Provider Plugin.
OLTITP_ENABLE_LTI_TOOL = True

# Cookies settings.
SHARED_COOKIE_DOMAIN = ".ngrok.io"  # Replace .ngrok.io with the domain of the service used to expose the LMS and learning MFE if using another service other than ngrok.
# MFE config API settings.
ENABLE_MFE_CONFIG_API = True
MFE_CONFIG_API_CACHE_TIMEOUT = 0
  1. Create a site for your external LMS address: http://localhost:18000/admin/sites/site/add/.
  2. Create a site configuration for your site: http://localhost:18000/admin/site_configuration/siteconfiguration/add/
  {
      "SESSION_COOKIE_DOMAIN": ".ngrok.io", # This should be the same value has SHARED_COOKIE_DOMAIN setting.
      "LEARNING_MICROFRONTEND_URL": "https://learning-mfe.ngrok.io",
      "MFE_CONFIG": {
          "LMS_BASE_URL": "https://lms.ngrok.io",
          "LOGIN_URL": "https://lms.ngrok.io/login",
          "LOGOUT_URL": "https://lms.ngrok.io/logout",
          "MARKETING_SITE_BASE_URL": "https://lms.ngrok.io",
          "BASE_URL": "lms.ngrok.io",
          "LEARNING_BASE_URL": "https://learning-mfe.ngrok.io,
          "REFRESH_ACCESS_TOKEN_ENDPOINT": "https://lms.ngrok.io/login_refresh"
      }
  }
  1. Restart the LMS: make dev.restart-container.lms.
  2. Modify the learning MFE webpack.dev.config.js file (create one if not file exists.):
const path = require('path');
const { createConfig } = require('@edx/frontend-build');
const config = createConfig('webpack-dev', {
    devServer: {
      allowedHosts: 'all',
    },
});
config.resolve.modules = [
  path.resolve(__dirname, './src'),
  'node_modules',
];
module.exports = config;
  1. Modify the learning MFE .env.development file and add these variables:
APP_ID='learning'
MFE_CONFIG_API_URL='https://lms.ngrok.io/api/mfe_config/v1'
  1. Create a new LTI 1.3 tool key config: http://localhost:18000/admin/lti1p3_tool_config/ltitoolkey/add/ (You can use IMS RI to create a key pair: https://lti-ri.imsglobal.org/keygen/index)
  2. Create a new LTI 1.3 tool config: http://localhost:18000/admin/lti1p3_tool_config/ltitool/add/
Issuer: https://saltire.lti.app/platform
Client id: saltire.lti.app
Auth login: url: https://saltire.lti.app/platform/auth
Auth token url: https://saltire.lti.app/platform/token/sda42bb0cf2352259a367a404d48d54e8
Key set url: https://saltire.lti.app/platform/jwks/sda42bb0cf2352259a367a404d48d54e8
Deployment ids: ["cLWwj9cbmkSrCNsckEFBmA"]
  1. Go to the saLTIre platform https://saltire.lti.app/platform
  2. Go to "Security Model" on the left sidebar and set these settings:
Message URL: https://lms.ngrok.io/openedx_lti_tool_plugin/1.3/launch/{course_id}
Initiate login URL: https://lms.ngrok.io/openedx_lti_tool_plugin/1.3/login
Redirection URI(s): https://lms.ngrok.io/openedx_lti_tool_plugin/1.3/launch/{course_id}
Public keyset URL: https://lms.ngrok.io/openedx_lti_tool_plugin/1.3/pub/jwks
  1. Go to "User" on the left sidebar and set these settings:
  2. Check the checkbox on "Given name", and "Family name" fields.
  3. Click on the "Save" button on the top navbar.
  4. Click on the dropdown next to the "Connect" button on the top navbar.
  5. Click on "Open in iframe" or "Open in new window".
  6. The launch should redirect you to the requested course on the learning MFE.
  7. Go to https://lms.ngrok.io/account/settings
  8. The username should show like "givennamefamilynameinitial.short_uuid"
  9. The email should show like "uuid@openedx_lti_tool_plugin"
  10. Repeat steps 16 - 22.
  11. The username should show like "short_uuid"
  12. The email should show like "uuid@openedx_lti_tool_plugin"

@kuipumu kuipumu self-assigned this Jun 13, 2024

if given_name and family_name:
return f'{given_name} {family_name}'
def user_collision(self) -> bool:
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think of searching if there is already an LTI profile with the same uuid? I would say it's very unlikely for a regular user, that is not associated with an LTI profile, to have that specific username and email.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexjmpb There is already a unique=True parameter set on the LtiProfile uuid field, and the short uuid is derived from such value so there is no way the uuids are duplicated.

The need for the user_collision method is for the even more unlikely scenario that the username (which contains a short uuid with a bigger collision chance than a regular uuid) already exists, if this verification wasn't added the user could just simply retry the launch.

I would think making sure there will be no issues is better, and the performance impact will be minimal since this will only be executed when the LtiProfile has no user assigned.

What do you think?, I'm missing something?, do you think there could be another alternative apart from just letting it fail on the rare occurrence of a username collision?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree.

return self.given_name

@property
def username(self) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

If the preferred_username PII data is specified, wouldn't we want to use it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexjmpb For various reasons:

  1. The OpenID Connect Core specification states that the preferred_username claim is not a stable claim and should not be used has an unique identifier for the End-User.
  2. The preferred_username claim could be exploited by the LTI platform user, if they create another account with a username that matches a privileged account on our LMS, they could execute a LTI launch and escalate privilege on our LMS. Also they could use it to access to an user with grades that should not be accessible on the context of the LTI launch.

There are some other points I'm might be missing, this was analyzed previously on a discovery related to using the preffered_username or email claim to reuse existing accounts. A solution to this could be to create some mechanism to make the LTI platform user authenticate on the LMS with a known account once the LTI launch is executed, however it's still not clear how this could be achieved

Here is the URL to the discovery document:
https://docs.google.com/document/d/18CPyVm8MBqAhROCghjZMV3gOwK-chguKCFpfxX0zmjE/edit

alexjmpb
alexjmpb previously approved these changes Jun 20, 2024
return self.given_name

@property
def username(self) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you planning to perform a migration of the users currently existing? With this new property wouldn't be a discrepancy between the existing user models username and the username generated by this property?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexjmpb There will be a discrepancy with the structure of the username of the older LtiProfiles with the new LtiProfiles.

Right now there is no requirement to update the username of the older LtiProfiles, if this is required in the future we could execute a backfill to fix this discrepancy.

Copy link
Contributor

Choose a reason for hiding this comment

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

What if the username property returns the username of the platform user model if exists?

alexjmpb
alexjmpb previously approved these changes Jun 20, 2024
@kuipumu kuipumu removed the request for review from anfbermudezme June 21, 2024 15:12
@kuipumu kuipumu merged commit f980ede into main Jun 21, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants