[BD-13] refactor: deprecates ModuleSystem properties related to code sandboxing #29260

pomegranited · 2021-11-05T07:24:40Z

Description

Builds on the shim added by https://github.com/edx/edx-platform/pull/29190 to deprecate the following ModuleSystem attributes which are only used by CAPA problems:

cache: deprecated in favour of the added CacheService
can_execute_unsafe_code, get_python_lib_zip: deprecated in favour of the added SandboxService

Also fixes the safe_exec.tests.TestSafeOrNot.test_cant_do_something_forbidden test , which was failing on my devstack with latest master. But since Jenkins and the github CI don't run in a virtualenv, this test is skipped, so this issue had gone unnoticed.

This change is only a refactoring, and should not affect Learners, Course Authors, or anyone else using edx-platform.

Supporting information

Content Core Platform Simplication project
BLENDED-113 (edX internal ticket)
FAL-2209 (OpenCraft internal ticket)

Testing instructions

LMS:

Login as a demo learner (honor@example.com / edx)
Enrol in the BD-13 test course
Navigate to the Complex Blocks > Custom python problem. Ensure that you can answer the question correctly (this tests the python code sandboxing).
Switch to the Legacy Course Experience and ensure you can still answer this problem.

Regression testing:

Browse around the Demo course, paying particular attention to the CAPA problems.
Check that you can submit answers to problems, and save changes to blocks. (This tests that these changes haven't broken anything else with CAPA problems.)
Switch to the Legacy Course Experience and ensure you can submit answers to problems there too.

Studio:

Login as a demo staff (staff@example.com / edx)
Navigate to the Demo course and browse through the blocks, paying particular attention to the CAPA problem blocks. Ensure they render and edit as expected.
Check that you can submit answers to problems, and save changes to blocks.
Preview unpublished changes and ensure they render as expected.
Publish changes, and ensure they render in the LMS as expected.

safe_exec:

In your devstack's lms-shell, run the following command, and ensure all tests pass.

pytest common/lib/capa/capa/safe_exec/tests/test_safe_exec.py

Deadline

None

openedx-webhooks · 2021-11-05T07:24:45Z

Thanks for the pull request, @pomegranited! I've created BLENDED-1005 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.

pomegranited · 2021-11-08T07:32:36Z

jenkins run python

pomegranited · 2021-11-09T10:29:07Z

jenkins run python

common/lib/xmodule/xmodule/services.py

common/lib/xmodule/xmodule/tests/__init__.py

common/lib/xmodule/xmodule/tests/test_services.py

lms/djangoapps/courseware/tests/test_module_render.py

pomegranited · 2021-12-02T05:37:38Z

jenkins run python

pomegranited · 2021-12-06T02:44:03Z

Hi @ormsbee , are you able to review and merge this OSPR? CC @symbolist

The test failure looks unrelated (and intermittant):

ERROR: Could not find a version that satisfies the requirement django-ratelimit-backend (unavailable)
ERROR: No matching distribution found for django-ratelimit-backend (unavailable)

but I don't know how to trigger a re-run of these unit tests?

ormsbee · 2021-12-06T17:28:53Z

@pomegranited: I'm re-running the tests. I might not get to review this until tomorrow though. Is this blocking other BD-13 work?

pomegranited · 2021-12-07T00:39:00Z

Thank you @ormsbee ! Nope, this isn't blocking other BD-13 work, so tomorrow is perfectly fine.

ormsbee

The code looks fine to me. I think this mostly comes down to how we want to test it before trying to merge it in.

@pdpinch: Any thoughts on this? This is refactoring work so it's not supposed to result in user-facing changes, but it's code sandboxing, which means that if there is a regression, it's very likely to hit advanced MIT content first. 😛

FYI @connorhaugh, @kenclary, @jristau1984

pomegranited · 2021-12-09T02:44:04Z

@ormsbee @pdpinch You're welcome to use the sandbox server for testing, it's running the current version of this branch:

It has the usual demo users on it, so you can use staff@example.com / edx to create or modify courses.

There's a working custom python problem, and one that demonstrates a PermissionDenied error, but you're welcome to add more courses/units and test however you need.

CC @connorhaugh @kenclary @jristau1984

pdpinch · 2021-12-13T01:50:57Z

Would it be possible to enable the python_lib.zip feature? I thought that the most thorough way to test this would be to run the demo course in https://github.com/mitodl/mitx-grading-library but it looks like the course can't find the python library (for example).

edx-status-bot · 2021-12-17T07:25:12Z

Your PR has finished running tests. There were no failures.

pomegranited · 2022-01-05T03:41:37Z

@pdpinch CC @ormsbee @Agrendalath

I've resolved conflicts created when the other BD-13 PRs merged, and deployed a new sandbox server using the latest code. Could you please verify that your MITx Grading Library problems are working correctly, and let me know if anything needs to be fixed before this can be merged?

pomegranited · 2022-01-12T01:13:00Z

Nudge @pdpinch CC @ormsbee @Agrendalath :)

pdpinch · 2022-01-12T20:02:20Z

So sorry that I lost track of this.

I'm finding that the first problem in the MITx Grading Library test course isn't behaving as advertised.

I'm going to continue testing today/tomorrow to try to come up with an explanation, and also review other problems, but I didn't want to hold up this key bit of feedback any longer.

pdpinch · 2022-01-12T20:34:31Z

I tested up through formula grader, but I need to take a break. I'm really not sure what's up with the first problem. Most everything else is working, except:

responses messages with a single apostrophe ' are getting escaped. I think this is pre-existing issue.
The 2nd sample answer to "Sibling Variables" in this unit is returning an incorrect response when it should be correct.

I'll test these on edge as a comparison point.

pomegranited · 2022-01-13T02:18:18Z

Hi @pdpinch , thanks for testing your custom problems on edge too. I don't think this change would have caused the kind of issues you're seeing -- basically, either codejail would work or it wouldn't work -- but we need to be sure before merging.

pdpinch · 2022-01-18T13:24:16Z

@pomegranited sorry again for being slow to follow up. I've gone through the entire MITx Graders course now.

I reviewed the first problem again and now it seems to be working. The most likely explanation is I misunderstood the directions.

The other issues I noted are still there:

responses messages with a single apostrophe ' are getting escaped.

I confirmed this is a pre-existing issue by testing on edge.edx.org

The 2nd sample answer to "Sibling Variables" in this unit is returning an incorrect response when it should be correct.

The behavior here is different on the sandbox from edX. Since this a problem that uses sampling, I imagine it might be some subtle difference between the sandbox and edge. I'll consult with someone who knows more about the inner workings, but I don't think this, by itself, should stop you from proceeding with this PR.

ormsbee · 2022-01-18T15:04:53Z

I think that means we're good to go, but please don't merge until tomorrow (Jan 19). It's my understanding that edX is being extra careful about release today. FYI @jristau1984, @doctoryes, @connorhaugh, @kenclary

pdpinch · 2022-01-18T19:05:56Z

Out of an abundance of caution, I also tested to make sure that python_lib.zip is not available via an unauthorized or unauthenticated browser request. I'm relieved to say that it is not and the following URL returned a 404 as expected:

https://pr29260.sandbox.opencraft.hosting/asset-v1:MITx+demo+3T2021+type@asset+block@python_lib.zip

pomegranited · 2022-01-19T00:14:15Z

Brilliant, thank you for your thorough testing @pdpinch !
And thanks for providing a merge timeline @ormsbee . Are you able to merge this tomorrow @pdpinch ?

pdpinch · 2022-01-21T14:08:25Z

Looks like there are some pylint failures. I'll try running the tests again.

pomegranited · 2022-01-24T01:13:57Z

@pdpinch

Looks like there are some pylint failures. I'll try running the tests again.

Looks like pylint has changed its mind about some import ordering.. Hopefully this fix will sort it.

pomegranited · 2022-01-24T07:53:49Z

... and it has!
@pdpinch would you be willing to work with edX to merge this on Tuesday? CC @ormsbee @Agrendalath

pdpinch · 2022-01-24T13:36:33Z

@pomegranited this passed tests. Do you want to squash it?

…cache * Deprecates ModuleSystem can_execute_unsafe_code, get_python_lib_zip and cache properties * Adds a new CacheService and SandboxService to provide the deprecated property * Adds tests for the added CacheService and SandboxService * Updates the ModuleSystemShim tests in Lms and Studio

so that both tests succeed on local devstack and in CI.

pomegranited · 2022-01-25T04:21:14Z

Thanks @pdpinch -- squashed and rebased.

pdpinch · 2022-01-25T16:38:04Z

@pomegranited since this touches a sensitive area of the code base on edx.org, I'm making sure that we have someone monitoring the deployment and available to do a rollback if necessary.

Question for you: are there any concerns about rolling this back, if it is necessary? I don't see anything myself, but is there anything we should know, perhaps about caching?

pomegranited · 2022-01-26T02:58:32Z

@pdpinch Nope, it should roll back cleanly if there are any issues in production.
It's just a code refactoring -- no changes to where or what gets cached, and hopefully it won't cause any errors.
But good plan to deploy it under supervision -- could you ping people on the openedx-cc-edx-platform Slack room according to the Changes to Committer “Champion” Program?

openedx-webhooks · 2022-01-26T14:57:47Z

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

pdpinch · 2022-01-26T14:57:58Z

Thanks @pomegranited!

edx-pipeline-bot · 2022-01-26T16:27:38Z

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

edx-pipeline-bot · 2022-01-26T17:05:14Z

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

pomegranited · 2022-01-27T03:13:58Z

Woo hoo!! Thank you so much for testing this thorougly @pdpinch , and for getting it merged!

openedx-webhooks added blended PR is managed through 2U's blended developmnt program waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Nov 5, 2021

pomegranited force-pushed the jill/BD-13-sandbox branch 2 times, most recently from 3871b27 to 6fc4322 Compare November 8, 2021 06:52

pomegranited marked this pull request as ready for review November 8, 2021 08:53

openedx-webhooks added needs triage and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Nov 8, 2021

pomegranited marked this pull request as draft November 8, 2021 08:56

pomegranited force-pushed the jill/BD-13-sandbox branch from 7cf3455 to c82aab0 Compare November 9, 2021 08:51

pomegranited force-pushed the jill/BD-13-sandbox branch 2 times, most recently from 7cb3749 to 914b809 Compare November 10, 2021 07:02

pomegranited marked this pull request as ready for review November 10, 2021 07:53

pomegranited force-pushed the jill/BD-13-sandbox branch 2 times, most recently from d3757da to 3352024 Compare November 11, 2021 01:40

pomegranited commented Nov 15, 2021

View reviewed changes

common/lib/xmodule/xmodule/services.py Outdated Show resolved Hide resolved

symbolist suggested changes Nov 22, 2021

View reviewed changes

common/lib/xmodule/xmodule/tests/__init__.py Outdated Show resolved Hide resolved

common/lib/xmodule/xmodule/tests/test_services.py Outdated Show resolved Hide resolved

lms/djangoapps/courseware/tests/test_module_render.py Show resolved Hide resolved

pomegranited mentioned this pull request Nov 24, 2021

[BD-13] deprecates runtime.xqueue in favor of the XQueueService #29312

Merged

pomegranited force-pushed the jill/BD-13-sandbox branch from de362ec to d30e673 Compare December 2, 2021 02:10

pomegranited force-pushed the jill/BD-13-sandbox branch from 9e51a7e to d8e32e5 Compare December 6, 2021 01:48

ormsbee self-assigned this Dec 8, 2021

ormsbee approved these changes Dec 9, 2021

View reviewed changes

pomegranited added 3 commits January 25, 2022 14:42

refactor: removes calls to deprecated properties from source and tests.

57b390f

test: fixes safe_exec test TestSafeOrNot

23a0d9f

so that both tests succeed on local devstack and in CI.

pomegranited force-pushed the jill/BD-13-sandbox branch from b8e8f8a to 23a0d9f Compare January 25, 2022 04:19

pdpinch merged commit 4f58ed4 into openedx:master Jan 26, 2022

openedx-webhooks added merged and removed needs triage labels Jan 26, 2022

Agrendalath deleted the jill/BD-13-sandbox branch January 26, 2022 23:44

[BD-13] refactor: deprecates ModuleSystem properties related to code sandboxing #29260

[BD-13] refactor: deprecates ModuleSystem properties related to code sandboxing #29260

Uh oh!

Conversation

pomegranited commented Nov 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Supporting information

Testing instructions

Deadline

Uh oh!

openedx-webhooks commented Nov 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pomegranited commented Nov 8, 2021

Uh oh!

pomegranited commented Nov 9, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pomegranited commented Dec 2, 2021

Uh oh!

pomegranited commented Dec 6, 2021

Uh oh!

ormsbee commented Dec 6, 2021

Uh oh!

pomegranited commented Dec 7, 2021

Uh oh!

ormsbee left a comment

Choose a reason for hiding this comment

Uh oh!

pomegranited commented Dec 9, 2021

Uh oh!

pdpinch commented Dec 13, 2021

Uh oh!

edx-status-bot commented Dec 17, 2021

Uh oh!

pomegranited commented Jan 5, 2022

Uh oh!

pomegranited commented Jan 12, 2022

Uh oh!

pdpinch commented Jan 12, 2022

Uh oh!

pdpinch commented Jan 12, 2022

Uh oh!

pomegranited commented Jan 13, 2022

Uh oh!

pdpinch commented Jan 18, 2022

Uh oh!

ormsbee commented Jan 18, 2022

Uh oh!

pdpinch commented Jan 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pomegranited commented Jan 19, 2022

Uh oh!

pdpinch commented Jan 21, 2022

Uh oh!

pomegranited commented Jan 24, 2022

Uh oh!

pomegranited commented Jan 24, 2022

Uh oh!

pdpinch commented Jan 24, 2022

Uh oh!

pomegranited commented Jan 25, 2022

Uh oh!

pdpinch commented Jan 25, 2022

Uh oh!

pomegranited commented Jan 26, 2022

Uh oh!

openedx-webhooks commented Jan 26, 2022

Uh oh!

pdpinch commented Jan 26, 2022

Uh oh!

edx-pipeline-bot commented Jan 26, 2022

Uh oh!

edx-pipeline-bot commented Jan 26, 2022

Uh oh!

pomegranited commented Nov 5, 2021 •

edited

Loading

openedx-webhooks commented Nov 5, 2021 •

edited

Loading

pdpinch commented Jan 18, 2022 •

edited

Loading