-
Notifications
You must be signed in to change notification settings - Fork 0
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
Conversation
|
||
if given_name and family_name: | ||
return f'{given_name} {family_name}' | ||
def user_collision(self) -> bool: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexjmpb For various reasons:
- 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.
- 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
return self.given_name | ||
|
||
@property | ||
def username(self) -> str: |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
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
shortuuid
base requirement.LtiProfile
short_uuid
property.LtiProfile
given_name
property.LtiProfile
middle_name
property.LtiProfile
family_name
property.LtiProfile
user_collision
method.LtiProfile
configure_user
method.LtiProfile
name
property.LtiProfile
save method.Testing:
make dev.up.lms
.npm run start
.openedx_lti_tool_plugin
on the LMS.ngrok http 18000
ngrok http 2000
make dev.restart-container.lms
.webpack.dev.config.js
file (create one if not file exists.):.env.development
file and add these variables: