[BD-13] [BB-6144] deprecate hostname attribute from xmodule ModuleSystem

[tecoholic]

Description

The hostname attribute was needed for OAuth in the LTI Module and was
being passed via the x_module.ModuleSystem class as a mixin. This attribute has been removed.
It has been replaced in the xmodule.lti_module with get_current_request_hostname utility function.

This change affects only the module_renderer module of the LMS which is also updated to initialize the XBlocks without passing the hostname attribute anymore.

This shouldn't cause any changes for any users and should continue to work just as usual.

Supporting information

• Related OpenCraft Issue - https://tasks.opencraft.com/browse/BB-6144

Testing instructions

1. Set your dev environment to master branch.
2. Add "lti" to the Advanced Module List in the Advanced Settings. (NOT lti_consumer which is an independent xblock, this PR deals with the old lti module)
3. Scroll down to LTI Passports and add a new entry "saltire:consumer_key:secret". We will be using the Saltire Tool Emulator to test the changes
4. Save Changes
5. Add a new new unit to your test course and select Advanced -> LTI
6. Click Edit and enter the following details and save the block
   • LTI ID: saltire
   • LTI URL: https://saltire.lti.app/tool
   • Open in New Page: False
7. Once Saltire loads in the block, make sure Verification is show as Passed
8. Save and publish the unit.
9. Open the unit in LMS and expand the Message Parameters in the Saltire Block. Note the value of resource_link_id. It should be something like {hostname}-{random-hash-value}. Eg., when testing from local devstack localhost%3A18000-71e809b7a55442afaa590a477145bb6c
10. Now checkout the edx-platform to the PR branch open-craft:tecoholic/BB-6144-deprecate-hostname-in-ModuleSystem
11. Restart your studio & lms services to make sure they are loading the changes from PR.
12. Refresh the LMS page with Saltire and expand the Message Parameters once again
13. Compare the resource_link_id show with the old value. Both should have the same {hostname} part.
14. You can create a new Unit + LTI Block after the PR branch has been checked out to ensure hostname value is populated as expected.

[openedx-webhooks]

Thanks for the pull request, @tecoholic! I've created BLENDED-1215 to keep track of it in Jira. More details are on the BD-13 project page.

When this pull request is ready, tag your edX technical lead.

[Agrendalath]

@tecoholic, this PR:

Changes the existing behavior of the LTI XBlock in Studio and Preview

Assumptions:

1. LMS URL is $DOMAIN.
2. Studio URL is studio.$DOMAIN.
3. Preview URL is preview.$DOMAIN.

The original LTIBlock provides the following Resource Link IDs:

• LMS: $DOMAIN:$PORT-$HASH,
• Studio: -$HASH,
• Preview: $DOMAIN:$PORT-$HASH (same as LMS).

The version from this PR provides the following values:

• LMS: $DOMAIN:$PORT-$HASH,
• Studio: studio.$DOMAIN:$PORT-$HASH,
• Preview: preview.$DOMAIN:$PORT-$HASH.

@giovannicimolin, would you mind commenting on this?

Breaks the LTI Consumer XBlock

Using the LTI Consumer XBlock (which is installed in the edx-platform by default) throws the AttributeError with the following message: 'CachingDescriptorSystem' object has no attribute 'hostname'
This XBlock is still using runtime.hostname for some reason. We should replace this call by creating a new service that will allow XBlocks to retrieve the hostname.

[tecoholic]

@Agrendalath I was thinking using the get_current_request_hostname was probably not the best solution here after working on #30320. I think that reflects in your observations about the hostname changing when the view changes. I will try to see if there is a service already defined around the Django settings and use it to get the SITE_NAME just like the original code or create a new one to do that.

[tecoholic]

@Agrendalath I reworked the solution to use a new service called hostname which just returns the value of django.conf.settings.SITE_NAME.

1. This is passed independently to the LMS module system and the CMS module system, which means, we will see different values for the resource id in studio, preview & LMS. I am not sure how to get around this.
2. The service resolves to a string - which works fine for the requirement. However it is not a "service" in the sense as it is not a custom class with other methods.

Since there is already a SettingsService for fetching XBlock settings, I didn't want to create another one on top of the full "django.conf.settings". Also, it might be a security loophole for external XBlocks to simply import it. However if you think we should have something like

class HostnameService:
    def get_hostname(self):
        return settings.SITE_NAME

let me know. I will make the changes and update the PR.

[Agrendalath]

@tecoholic,

This is passed independently to the LMS module system and the CMS module system, which means, we will see different values for the resource id in studio, preview & LMS. I am not sure how to get around this.

I am not sure how this affects the LTI functionality. Maybe it doesn't have any impact here. We should check this with @giovannicimolin.

The service resolves to a string - which works fine for the requirement. However it is not a "service" in the sense as it is not a custom class with other methods.

While the approach seems to be fine for a one-liner, I haven't found a service that would not return an instance of its class. I believe we should follow the official API here, and create the HostnameService(Service) instead. We should also make sure it can be accessed by pure XBlocks and add it here.

[tecoholic]

@Agrendalath Agreed. I will update the PR with a proper service class.

[tecoholic]

@Agrendalath I have replaced the one-liner with HostnameService in xmodule.services with tests and also have included it in the XBlock Runtime.

[giovannicimolin]

@Agrendalath

The original LTIBlock provides the following Resource Link IDs:
LMS: $DOMAIN:$PORT-$HASH,
Studio: -$HASH,
Preview: $DOMAIN:$PORT-$HASH (same as LMS).
The version from this PR provides the following values:
LMS: $DOMAIN:$PORT-$HASH,
Studio: studio.$DOMAIN:$PORT-$HASH,
Preview: preview.$DOMAIN:$PORT-$HASH.
giovannicimolin, would you mind commenting on this?

The LTI spec, under section 3 mentions:

resource_link_id 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.

So, this implementation is more consistent and correct than the previous implementation, but there's a gotcha still: ideally, they should all be the same value since they represent the same placement in a course.

Studio and Preview values are not that important to keep the same, just make sure the LMS value doesn't change, otherwise students will lose their progress in the LTI tool.

[giovannicimolin]

@schenedx Some changes are incoming to LTI consumer - should not impact end users though. See my comment above ^

[giovannicimolin]

For reference: there's a thread open on the discussion forums about this parameter mismatch.

[schenedx]

@giovannicimolin Thank you for the heads up. I read the comment above your comment. The consistency make sense to me because, through my own testing, I also noticed the value on the resource_link_id on Studio preview is missing hostname. I am supportive of this change.
I also want to make you and maybe others who are reviewing this change aware, that I have a sister PR that would make the attribute user_id on LTI to have the unique anonymous_user_id value on Studio Preview.

[Agrendalath]

@ormsbee, this one is almost ready - we just need to replace one variable. mentioned here. Would you like to take a look at this before we merge it?

We'll also open a PR to the LTI Consumer XBlock, but it is going to be as small as openedx/xblock-lti-consumer#249.

[Agrendalath]

@tecoholic, please look into the following failing tests:

• common/lib/xmodule/xmodule/tests/test_lti_unit.py::LTIBlockTest::test_lis_result_sourcedid
• common/lib/xmodule/xmodule/tests/test_lti_unit.py::LTIBlockTest::test_successful_verify_oauth_body_sign

[tecoholic]

@Agrendalath Talk about unintended consequences. The default SITE_NAME is just localhost but the LMS_BASE is localhost:18000. This changed the hash of the mock request body in the tests and a resource id to be URL encoded twice.

The tests are fixed now and pushed them as a separate commit.

[tecoholic]

An unrelated test FAILED lms/djangoapps/discussion/django_comment_client/base/tests.py::TeamsPermissionsTestCase::test_update_thread_08___group_moderator____cohorted____course_commentable_id___200___cohort__ seems to be failing.

Maybe an effect of old code, should we rebase this onto master?

[tecoholic]

Moving this to draft as we found that this approach is not a good one.

[Agrendalath]

@ormsbee, would you like to take a look at this before we merge it?

[ormsbee]

@ormsbee, would you like to take a look at this before we merge it?

You can merge without me, just please make sure @jristau1984 is aware of when it's happening. Thank you.

[schenedx]

👍

[openedx-webhooks]

@tecoholic 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the production environment.