feat: add CoursewareViewStarted, CourseStartDateValidationFailed, and CoursewareAccessChecksRequested filters#336
Conversation
7e9d307 to
75be6aa
Compare
75be6aa to
39c6daf
Compare
f11df07 to
b80e6ce
Compare
ca033ae to
ed2af98
Compare
… CoursewareAccessChecksRequested filters Plugins can now supply a courseware view redirect URL, inject specific error payloads when course start-date validation fails, or deny courseware access with a priority (non-bypassable) error. ENT-11544
ed2af98 to
a78948c
Compare
|
@felipemontoya this is now ready for your review. For reference, the proposed call sites for all 3 added filters can be found in the draft platform PR. Also, other unrelated repo cleanup stuff:
|
|
Reviewing now. For future reference @pwnage101, whenever you need to do a lot of cosmetic changes its better to keep those in a separate PR. Those I can merge easily and that makes reviewing the actual change in the filters easier. |
felipemontoya
left a comment
There was a problem hiding this comment.
Overall the 3 filters make are well defined, correctly documented and are new places in the platform cycle not covered by previous filters. I'd like to hear from the RBAC devs before merging but otherwise I approve.
| return data["context"], data["user_id"], data["course_id"] | ||
|
|
||
|
|
||
| class CoursewareViewStarted(OpenEdxPublicFilter): |
There was a problem hiding this comment.
This filter makes a lot of sense to me.
If I understand correctly this goes first in the stack, right?
1. CoursewareViewStarted
2. VerticalBlockChildRenderStarted
3. Loop over RenderXBlockStarted
4. VerticalBlockRenderCompleted
There was a problem hiding this comment.
To be completely honest, I don't 100% know. My goal with this ticket is a 1:1 replacement, so I'm pretty laser focused on replacing the exact call sites. That said, filter naming is important, so it is a good idea to understand the code path to come up with a self-documenting name.
The best I can tell, there are 2 code paths that can trigger a DSC redirect when a learner views courseware (legacy vs. MFE), and they use different filters:
Legacy (server-rendered) courseware views:
- CoursewareViewStarted (view decorator / WikiAccessMiddleware)
- If DSC redirect is needed, returns an HTTP redirect here.
- loop over VerticalBlockChildRenderStarted
- VerticalBlockRenderCompleted
Learning MFE — two separate requests:
- Request A — course_home_api/course_metadata BFF call:
- CoursewareAccessChecksRequested (from check_course_access)
- If DSC redirect is needed, returns the error_code =
data_sharing_access_requiredand developer_message =<consent URL>in API response. - MFE reads developerMessage as the consent redirect URL, performs redirect via javascript.
- If DSC redirect is needed, returns the error_code =
- CoursewareAccessChecksRequested (from check_course_access)
- Request B — render_xblock:
- RenderXBlockStarted
- loop over VerticalBlockChildRenderStarted
- VerticalBlockRenderCompleted
|
|
||
| Trigger: | ||
| - Repository: openedx/openedx-platform | ||
| - Path: lms/djangoapps/courseware/access_utils.py |
There was a problem hiding this comment.
Do you think this must connect with the RBAC project in any meaningful way?
@mariajgrimaldi or @BryanttV do you see any cause for concern with a filter like this?
There was a problem hiding this comment.
I'm not sure, this is the first I've heard of the "RBAC project" (openedx/openedx-authz), having only contributed to openedx/edx-rbac which we use extensively to manage role-based authorization for all of our enterprise IDAs.
|
|
||
| class CoursewareAccessChecksRequested(OpenEdxPublicFilter): | ||
| """ | ||
| Filter triggered during courseware access checks so plugins can deny access. |
There was a problem hiding this comment.
This filter also makes a lot of sense to me. I'd also like to see how this interacts with RBAC for future-proofing purposes.
Plugins can now supply a courseware view redirect URL, inject specific error
payloads when course start-date validation fails, or deny courseware access
with a priority (non-bypassable) error.
ENT-11544
Blocks: