edly-io / h5pxblock

Xblock which provides ability to play H5P content in open edX
MIT License
13 stars 23 forks source link

feat: allow specifying an alternative storage through django settings #19

Closed Cup0fCoffee closed 1 year ago

Cup0fCoffee commented 1 year ago

Description

This PR adds a way to specify via Django settings storage for storing content different to the default_storage.

By default the XBlock will still use the default_storage. However, now an alternative storage can be specified via H5PXLBOCK_STORAGE settings, which allows to specify storage_class and a dictionary of settings. If storage_class is not specified, the implementation relies on get_storage_class from django.core.files.storage to get the default storage class. If settings are not specified, whatever mechanism there is for initializing the default settings will work, e.g. for S3Boto3Storage would pull them from django.conf.settings - ref. What keys to pass to the storage class can be figured out by reading it's source code. For example, for S3Boto3Storage, looking at these line of code in it's constructor, we can tell that to overwrite a settings, we should find it's name here, e.g. bucket_name, querystring_auth, access_key, secret_key, e.t.c., would be used as keys in the settings dictionary.

Our need for this change comes from the fact that we want to keep the default storage, which is an S3 bucket, private. Presigning urls doesn't work in this case, because of how h5pxblock and h5p-standalone work. So instead of using the default storage, what we want to be able to do, is to have a separate public bucket, and pass it to the h5pxblock to use instead of the default one. We also want to be able to pass overwrites for certain settings.

Examples

For the use case described above

Supporting information

OpenCraft private JIRA Ticket - BB-7138

Testing instructions

  1. Setup master or nutmeg.master devstack.
  2. Clone h5pxblock to <devstack parent>/src.
  3. make dev.up.lms+studio+frontend-app-learning+frontend-app-course-authoring.
  4. Devstack by default uses local storage to store content, but via this feature we can pass an S3 bucket to the h5pxblock. Create a public S3 bucket and make sure to add the following to bucket's Permissions -> CORS (without that devstack LMS will have CORS issues loading content from the bucket):
    [
        {
            "AllowedHeaders": [
                "*"
            ],
            "AllowedMethods": [
                "GET",
                "PUT",
                "POST",
                "HEAD",
                "DELETE"
            ],
            "AllowedOrigins": [
                "*"
            ],
            "ExposeHeaders": [],
            "MaxAgeSeconds": 3000
        }
    ]
  5. Add the necessary settings to <devstack parent>/edx-platform/{lms,cms}/envs/private.py. Your settings in private.py should look something like this:
    H5PXBLOCK_STORAGE = {
        'storage_class': 'storages.backends.s3boto3.S3Boto3Storage',
        'settings': {
            'access_key': '<aws-access-key-id>',
            'secret_key': '<aws-secret-access-key>',
            'bucket_name': 'non-default-bucket-name',
            'querystring_auth': False,
            # on devstack it has a default value which has to be changed; we can set it to None or an empty string
            'custom_domain': None  # or ''
        },
    }
  6. make {lms,studio}-shell and inside both of the run pip install -e /edx/src/h5pxblock. make {lms,studio}-restart to restart both.
  7. Create new course or use the demo one on the devstack.
  8. Add h5pxblock in the advanced settings of the course in studio.
  9. Add a new unit and h5pxblock in it, upload some h5p content by editing the XBlock. (Example content can be take from here).
  10. Verify that:
    1. Content got uploaded to the bucket you've specified.
    2. That when visiting the course in the LMS, the content is correctly loaded from the bucket.
navinkarkera commented 1 year ago

@Cup0fCoffee The upload worked fine, the content was uploaded to the non-default bucket. But while loading this block in LMS, I am facing below error:

Uncaught (in promise) TypeError: Window.fetch: https://set-me-please/ (ex. bucket-name.s3.amazonaws.com)/h5pxblockmedia/edX/DemoX/da1f99fbfad44789a57a71f5ac41c1d0/h5p.json is not a valid URL.

The url seems to be using AWS_S3_CUSTOM_DOMAIN setting for building the link. Am I missing something here?

Cup0fCoffee commented 1 year ago

@Cup0fCoffee The upload worked fine, the content was uploaded to the non-default bucket. But while loading this block in LMS, I am facing below error:

Uncaught (in promise) TypeError: Window.fetch: https://set-me-please/ (ex. bucket-name.s3.amazonaws.com)/h5pxblockmedia/edX/DemoX/da1f99fbfad44789a57a71f5ac41c1d0/h5p.json is not a valid URL.

The url seems to be using AWS_S3_CUSTOM_DOMAIN setting for building the link. Am I missing something here?

@navinkarkera Ah, I forgot to add this to the testing instructions. You can basically set it to an empty string, or as the value says - <bucket-name>.s3.amazonaws.com. Just remember to modify it either in the /edx/etc/{lms,studio}.yml inside of both of the container's, or add it to private.yml and do make {lms,studio}-{down,up}.

It's just that devstack has a default non empty value for it, so this code is executed. So you either need to set the valid domain or empty string.

Once you've changed it, you should be able to reupload h5p content through the XBlock and the URL will be updated to the correct value. (Otherwise, you can just edit the URL manually in studio and test that it works in LMS :stuck_out_tongue_winking_eye:).

Cup0fCoffee commented 1 year ago

@navinkarkera I've improved the description and fixed testing instructions. You should be able to finish your testing once you fix the CORS issue.

Cup0fCoffee commented 1 year ago

@ziafazal Hi, we would like this feature to be merged from our fork into the upstream repo. Would you kindly review this PR, and let us know if you have any feedback? Cheers.

ziafazal commented 1 year ago

@Cup0fCoffee @navinkarkera I'm unable to understand the purpose of this PR. H5P xblock relies on django's default storage API and if a consumer of H5P xblock wants to use different storage it should change default storage settings. For example to use S3 you can have these settings

AWS_ACCESS_KEY_ID = '{your key id}'
AWS_SECRET_ACCESS_KEY = '{your secret key}'

AWS_STORAGE_BUCKET_NAME = '{S3 bucket}'

AWS_QUERYSTRING_AUTH = False

AWS_S3_CUSTOM_DOMAIN = '{bucket-name}.s3.amazonaws.com'
DEFAULT_FILE_STORAGE = 'storages.backends.s3boto3.S3Boto3Storage'
Cup0fCoffee commented 1 year ago

@ziafazal TLDR: in our case, we need the django's default storage and storage used by the H5P XBlock to be different.

The issue is that the default django storage in out project uses settings that are incompatible with the H5P XBlock. We are using an S3 bucket as our default storage, however, it is set to be private. To access any of it's content by an unauthorized client requires the URLs to the files to be pre-signed (this behavior is controlled by the AWS_QUERYSTRING_AUTH). Pre-signed URLs don't work with the h5p-standalone which is used by this XBlock on the front-end.

We don't want to make the default storage public, because it contains PII. The solution we came up with is to allow specifying an alternative to the default storage for this XBlock. That way we can create a separate public S3 bucket, and tell the H5P XBlock to use it instead of the default storage. This PR implements the mechanism for this feature.

I hope this clears things up, but please let me know if there are any more questions.

ziafazal commented 1 year ago

@Cup0fCoffee these changes are released in v0.2.4.

Cup0fCoffee commented 1 year ago

@ziafazal Thank you!