celtic-project / wordpress-lti

LTI Connector for WordPress
GNU General Public License v3.0
23 stars 11 forks source link

Incorrect max length on `resource_link_id` #24

Closed Kuret closed 5 months ago

Kuret commented 1 year ago

A client encountered this error while trying to integrate with our platform: [ERROR] Request failed with reason: 'Invalid parameter(s): resource_link_id (too long)

https://github.com/celtic-project/wordpress-lti/blob/master/includes/WPTool.php#L59 It appears that there is a max length of 40 on the resource_link_id but I cannot find anywhere in the spec where this is a requirement. For LTI 1.1 I cannot find anything about a max length at all, and for LTI 1.3 it appears to be varchar(255).

In other celtic libraries or just general LTI libraries I never encountered this restriction either.

It looks like the max length of 40 is incorrectly applied here, and should either not be enforced at all or at least be set to 255.

spvickers commented 1 year ago

In this instance, I suspect the reason for limiting the length of the resource link ID is to prevent usernames from being longer than accepted by WordPress when the resource format is being used for usernames. How long are your resource link ID values?

Kuret commented 1 year ago

I am not sure what you mean? This is not about usernames right? It should be an opaque value unique for the LMS:

resource_link_id=88391-e1919-bb3456 This is an opaque unique identifier that the TC guarantees will be unique within the TC for every placement of the link. If the tool / activity is placed multiple times in the same context, each of those placements will be distinct. This value will also change if the item is exported from one system or context and imported into another system or context. This parameter is required.

How long the resource link ID values are, should also be irrelevant according to the spec. (I'll have to look it up but they are definitely well below 100 chars)

spvickers commented 1 year ago

When resource level format is selected for usernames, the resource link ID from the platform is used when constructing a username for a user. Hence the concern about this not being too long as WordPress places a limit on the length of a username.

Kuret commented 1 year ago

This is obviously a deficit in the datamodel and/or implementation then, the Resource Link ID should not be limited by this but should just follow the spec. Other platforms and libraries cannot account for the quirks of individual platforms and libraries, that is precisely the reason a universal agreed upon specification exists.

spvickers commented 1 year ago

Have your read the documentation about the username format setting? All usernames created for WordPress users have to be within the length permitted by WordPress. The constraint on the length of a resource link ID value could be removed but it will increase the chance that a WordPress username is generated which exceeds the permitted length (when the resource level username format is being used).

Kuret commented 1 year ago

I am honestly a bit stumped by your reply. This has nothing to do with WordPress right? The issue is that a value which is valid according to the LTI spec does NOT get accepted. The reason why does not really matter, since the LTI spec is not bound by any specific platform and has no knowledge of these quirks (nor should it account for every single language/library/platform in existence). Again, OTHER platforms that integrate with this solution might end up with a faulty implementation while doing nothing wrong on their end because they correctly followed the spec. This means a platform should keep a list of EVERY potential Tool and their quirks to account for them, which is exactly what a specification like LTI is trying to avoid.

spvickers commented 1 year ago

It is related to WordPress. WordPress imposes a limit on the length of usernames, which this plugin has to comply with when creating users. One of the options in the plugin for generating usernames is to include the resource link ID in the username, hence, when this option has been selected, there is a connection between the WordPress limit and the usernames generated by this plugin. Is the plugin being used in a multisite instance of WordPress?

Kuret commented 1 year ago

One of the options in the plugin for generating usernames is to include the resource link ID in the username

So this was always a faulty option because the resource_link_id is allowed to be more than 40 characters. This option could've never conformed to the spec so shouldn't have existed in this form. At least put a warning/caveat message somehwere easily seen which explains this library does not conform exactly 100% to the spec because of this reason. The fact that this is a paid product makes it worse.

This is actually an issue between our customer and their supplier (who uses this solution), I just noticed this max length error and in the spirit of open source might as well be helpful and make this issue.

Since your replies are all irrelevant to the core of the issue (the non-conformity to the official specification) this is as much time away from my actual work as I can responsibly spend on this.

I thank you for your prompt responses and hope you keep this issue in mind, I can't believe we are the only platform ever to use >40 length ID's.

spvickers commented 1 year ago

Please note that this is NOT a "paid product"; it is open source and free to use.

spvickers commented 1 year ago

By way of background to the resource-level username format option, this plugin was originally written for a multisite instance of WordPress to allow blogs (sites) to be created for use within an LMS course. The resource-level username option provided a mechanism by which a user was limited to accessing the single blog which was created for each resource link; to access any other blog the user is forced to follow the related link from within the LMS. In that way the authorization of users to access each blog is completely controlled by the LMS and removing a user's access to a link within the course will ensure that they are unable to access the blog associated with it. Other options for username formats are also supported when this level of access control is not deemed to be necessary.

If you let me know the length of the resource link ID values causing the issue, then I will explore how best to accommodate them. It is certainly the case that, with non-multisite instances of WordPress (as the plugin now supports), the constraint is less relevant as all users will be accessing the same blog.

dkhgh commented 1 year ago

Hi @spvickers, I'm a colleague of @Kuret.

Please note that this is NOT a "paid product"; it is open source and free to use.

Fair enough, Stephen, appreciate that.

One of the options in the plugin for generating usernames is to include the resource link ID in the username, hence, when this option has been selected, there is a connection between the WordPress limit and the usernames generated by this plugin.

Thanks, this might be interesting for our customer's supplier, we'll share it with them. They're experienced with Wordpress, but new to LTI. They might not need this option at all.

If you let me know the length of the resource link ID values causing the issue

I think we can't fully predict the length, in this case they are around 50 characters.

Would it be feasible to uncouple the resource_link_id from the username? Maybe we could store the resource_link_id separately and then persist a truncated value in the username field. Would something like that solve the issue here?

It's been a long time since I worked with Wordpress, and I am unfamiliar with PHP, so take my thoughts with a grain of salt. If it helps, I'd be happy to help come up with a solution, sett up a test environment, or do some QA.

Of course, I completely respect if this isn't a priority for you at the moment.

spvickers commented 1 year ago

The resource-level username option ensures that a unique user account is created for each blog - the resource link ID value is used to make sure the username is unique. This option, therefore, requires the two to be coupled.

Perhaps you can pass on details of your use case - e.g. which username format option is being used, is a multisite instance of WordPress being used? I will then update the code to ensure this case is covered. In the meantime, try removing (or commenting out) the constraint to workaround the issue.

dkhgh commented 1 year ago

Thanks for your response @spvickers, We're still waiting to hear back from our customer's supplier, so Im not sure yet if they have a multisite configuration. I'll keep the option of commenting out the constraint in mind.

The resource-level username option ensures that a unique user account is created for each blog - the resource link ID value is used to make sure the username is unique. This option, therefore, requires the two to be coupled.

Would it be an option to calculate a (max 40 character) hash that uses the resource link ID as input, and use that as the Wordpress username, while persisting the original, full-length, resource_link_id?

If I am not mistaken, this could preserve both a unique Wordpress username and the original resource_link_id.

Again, I completely understand if this isn't a priority at the moment.

spvickers commented 1 year ago

Yes, a hash could be possible, but is not guaranteed to be as unique as the resource link ID itself and could cause issues for existing integrations. This plugin has been in use for over 10 years with this constraint and has not been a problem until now. I have yet to come across an LTI platform which uses such a long ID - do you know which platform is being used here? I suspect there will be a simple solution which does not require any changes to the existing usernames; it probably just needs to be more flexible about when to enforce the constraint.

dkhgh commented 1 year ago

The LTI platform used is our own, and the resource_link_id includes a UUID which is 36 chars long.