edx-platform
edx-platform copied to clipboard
openedx
feat: author permission studio xblock handler
Description
Conversation around a hasty fix for security reasons led to a fix-forward improvement. You can find that conversation here. https://github.com/openedx/edx-platform/pull/31221 .
This fix has some open questions:
- are we failing loudly in the correct way? running the handler, but not making any changes to course content, then raising permission denied? If things can't be persisted, that is ok, but doesn't seem super clean still.
Describe what this pull request changes, and why. Include implications for people using this change. I'll add more to this description as we flesh these things out.
Implications for this change seem to be ok, as long as users aren't required to run xblock handlers from studio. I haven't tested every handler, but I have tested CRUD operations as well as publish for the following xblocks:
- LTI Consumer
- Text
- Video
- Problem
- ORA
Useful information to include:
- Which edX user roles will this change impact?: "Learner", "Course Author",
Supporting information
Testing instructions
Deadline
"None" if there's no rush, or provide a specific date or event (and reason) if there is one.
Other information
are we failing loudly in the correct way? running the handler, but not making any changes to course content, then raising permission denied?
Do we have read-only permissions in Studio? (I don't think so?) Because if we do, there is definitely a need for users with read-only permissions to be able to call handlers within Studio, to preview the interactions with XBlocks in the preview view.
But if not, then I think the only users who will see the XBlock at all in Studio are course authors, so it seems reasonable to block the handlers completely and throw an exception if users don't have author permission.
Drag and drop v2:
- example handler used during course authoring
- example handler used during testing/previewing the XBlock in Studio or learning in the LMS
For what it's worth, here's how I implemented this in the Blockstore XBlock runtime, which is newer than this one:
- Whenever any handler runs, the base XBlock Runtime class always calls
.save()afterward. XBlock.save()will in turn callruntime.save_block()- The Blockstore runtime's save_block first checks if any changes have been made to "authored data" - if there are no changes, it just returns immediately (succeeds with no changes)
- Otherwise, it enforces permissions and fails loudly if the user is not authorized to update the authored data.
In my opinion this is the best way to go, but I'm not sure if it's possible in the "regular" XBlock runtime. And even answering that question (is it possible) may be tricky due to the many layers of persistence mechanisms involved.
Hey @bradenmacdonald thanks for the excellent feedback. Team priorities have kept my eyes off this for a bit, but I think I have a solution.
Do we have read-only permissions in Studio? (I don't think so?)
I can say, with some certainty that we do -- in the case of OrgLibraryUserRole course authors who have permission to view content from a library block should not be able to alter that content-- right? We specifically set up parameters for studio read only, so we have to respect that possibility.
therefore, we need to allow users with read-only permissions to be able to call handlers within Studio, which forces our hand into the way things a presently: gating on the module store write operation. This solves our security goals and, as you say:
This is a case where there is no official spec or guideliness for how this part of the XBlock API should handle authentication so we just have to pick what seems most sensible and is compatible with most existing xblocks.
in 99% of cases we expect users to have author access, so this shouldn't matter too much, but adding this check will prevent malicious activity.
One follow up: what might be the most gracious way to fail quietly? Not raise an error? send an internal logging message? still send an error?
One follow up: what might be the most gracious way to fail quietly? Not raise an error? send an internal logging message? still send an error?
Yeah, if we have to support read-only users and we can't check if the user actually tried to make changes before saving, I think just ignoring the write and logging an error makes sense.
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.
EdX Release Notice: This PR has been deployed to the production environment.