-
Notifications
You must be signed in to change notification settings - Fork 3
feat: add webhook endpoint for Open edX course enrollment #3372
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
0949f1c
4bc17d6
ec5e184
cb534e8
982afd4
c280bc6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, I don't think these changes are related. Could you open a separate PR for these if needed? |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1255,6 +1255,12 @@ | |
| description="Timeout (in seconds) for requests made via the edX API client", | ||
| ) | ||
|
|
||
| OPENEDX_WEBHOOK_KEY = get_string( | ||
| name="OPENEDX_WEBHOOK_KEY", | ||
| default=None, | ||
| description="Shared secret token used to authenticate incoming webhook requests from Open edX", | ||
| ) | ||
|
|
||
|
Comment on lines
+1258
to
+1263
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rhysyngsun what's your opinion on this? Do you think a pre-generated config-based string bearer token is fine here, or should we rather go with a staff OAuth token with expiry, or HMAC maybe (Is it worth it)? |
||
| # django debug toolbar only in debug mode | ||
| if DEBUG: | ||
| INSTALLED_APPS += ("debug_toolbar",) | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,10 +1,171 @@ | ||||||
| """Views for openedx""" | ||||||
|
|
||||||
| import logging | ||||||
|
|
||||||
| from django.conf import settings | ||||||
| from django.http import HttpResponse | ||||||
| from drf_spectacular.utils import extend_schema | ||||||
| from rest_framework import status | ||||||
| from rest_framework.decorators import api_view, permission_classes | ||||||
| from rest_framework.permissions import AllowAny | ||||||
| from rest_framework.response import Response | ||||||
|
|
||||||
| from courses.api import create_run_enrollments | ||||||
| from courses.models import CourseRun | ||||||
| from users.models import User | ||||||
|
|
||||||
| log = logging.getLogger(__name__) | ||||||
|
|
||||||
|
|
||||||
| def openedx_private_auth_complete(request): # noqa: ARG001 | ||||||
| """Responds with a simple HTTP_200_OK""" | ||||||
| # NOTE: this is only meant as a landing endpoint for api.create_edx_auth_token() flow | ||||||
| return HttpResponse(status=status.HTTP_200_OK) | ||||||
|
|
||||||
|
|
||||||
| @extend_schema(exclude=True) | ||||||
| @api_view(["POST"]) | ||||||
| @permission_classes([AllowAny]) | ||||||
| def edx_enrollment_webhook(request): # noqa: PLR0911, C901 | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe not right now, but we may want to move it to ol-django at some point to make it common between other applications that might want to use this API endpoint. I'll re-think more on this. |
||||||
| """ | ||||||
| Webhook endpoint that receives enrollment notifications from Open edX. | ||||||
|
|
||||||
| When a user needs to be enrolled in a course (e.g., staff/instructor role added), | ||||||
| the Open edX plugin POSTs to this endpoint so MITx Online can enroll them as an | ||||||
| auditor in the corresponding course run. | ||||||
|
|
||||||
| Expected payload: | ||||||
| { | ||||||
| "email": "instructor@example.com", | ||||||
| "course_id": "course-v1:MITx+1.001x+2025_T1", | ||||||
| "role": "instructor" | ||||||
| } | ||||||
| """ | ||||||
| # --- Authenticate via Bearer token --- | ||||||
| webhook_key = getattr(settings, "OPENEDX_WEBHOOK_KEY", None) | ||||||
| if not webhook_key: | ||||||
|
Comment on lines
+45
to
+46
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This logic may change as per my comment above for the token mechanism. |
||||||
| log.error("OPENEDX_WEBHOOK_KEY is not configured") | ||||||
| return Response( | ||||||
| {"error": "Webhook is not configured"}, | ||||||
| status=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't be a 500. It should be a 400 instead. |
||||||
| ) | ||||||
|
|
||||||
| auth_header = request.META.get("HTTP_AUTHORIZATION", "") | ||||||
| if not auth_header.startswith("Bearer "): | ||||||
| return Response( | ||||||
| {"error": "Missing or invalid Authorization header"}, | ||||||
| status=status.HTTP_401_UNAUTHORIZED, | ||||||
| ) | ||||||
|
|
||||||
| token = auth_header[len("Bearer ") :] | ||||||
| if token != webhook_key: | ||||||
| return Response( | ||||||
| {"error": "Invalid webhook token"}, | ||||||
| status=status.HTTP_403_FORBIDDEN, | ||||||
| ) | ||||||
|
|
||||||
| # --- Validate payload --- | ||||||
| email = request.data.get("email") | ||||||
| course_id = request.data.get("course_id") | ||||||
| role = request.data.get("role", "") | ||||||
|
|
||||||
| if not email or not course_id: | ||||||
| return Response( | ||||||
| {"error": "Missing required fields: email and course_id"}, | ||||||
| status=status.HTTP_400_BAD_REQUEST, | ||||||
| ) | ||||||
|
|
||||||
| # --- Look up user --- | ||||||
| try: | ||||||
| user = User.objects.get(email=email) | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we don't allow case-sensitive emails.
Suggested change
|
||||||
| except User.DoesNotExist: | ||||||
| log.warning( | ||||||
| "Webhook: No user found with email %s for course %s (role: %s)", | ||||||
| email, | ||||||
| course_id, | ||||||
| role, | ||||||
| ) | ||||||
| return Response( | ||||||
| {"error": f"User with email {email} not found"}, | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. PII caring
Suggested change
|
||||||
| status=status.HTTP_404_NOT_FOUND, | ||||||
| ) | ||||||
| except User.MultipleObjectsReturned: | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this ever going to happen? Can there be multiple users in the Users table with same email? |
||||||
| log.warning( | ||||||
| "Webhook: Multiple users found with email %s for course %s (role: %s)", | ||||||
| email, | ||||||
| course_id, | ||||||
| role, | ||||||
| ) | ||||||
| return Response( | ||||||
| {"error": f"Multiple users found with email {email}"}, | ||||||
| status=status.HTTP_409_CONFLICT, | ||||||
|
Comment on lines
+99
to
+101
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could be a 400 as well. |
||||||
| ) | ||||||
|
|
||||||
| # --- Look up course run --- | ||||||
| try: | ||||||
| course_run = CourseRun.objects.get(courseware_id=course_id) | ||||||
| except CourseRun.DoesNotExist: | ||||||
| log.warning( | ||||||
| "Webhook: No course run found with courseware_id %s (user: %s, role: %s)", | ||||||
| course_id, | ||||||
| email, | ||||||
| role, | ||||||
| ) | ||||||
| return Response( | ||||||
| {"error": f"Course run with id {course_id} not found"}, | ||||||
| status=status.HTTP_404_NOT_FOUND, | ||||||
| ) | ||||||
|
|
||||||
| # --- Enroll user as auditor --- | ||||||
| try: | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we do an early return at this point or maybe at the start of the API to make it idempotent? The thing we should ideally do is to check if the enrollment exists in the system, before doing anything else, and if the enrollment does exist already, we may return 409 conflict in that case actually otherwise proceed with whatever needs to happen. |
||||||
| enrollments, edx_request_success = create_run_enrollments( | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. create_run_enrollments is for creating enrollments for edX as well. In this case, the API request itself is coming from enrollment in edX, so should we call this method? Or create a new one so that we just create a local enrollment upon the Webhook call? Apparently, from the method docstr this is all this method eventually does: So the point is, do we want to go this route? cc: @pdpinch |
||||||
| user, | ||||||
| [course_run], | ||||||
| keep_failed_enrollments=True, | ||||||
| ) | ||||||
| except Exception: | ||||||
| log.exception( | ||||||
| "Webhook: Error creating enrollment for user %s in course run %s", | ||||||
| email, | ||||||
| course_id, | ||||||
| ) | ||||||
| return Response( | ||||||
| {"error": "Failed to create enrollment"}, | ||||||
| status=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 400 |
||||||
| ) | ||||||
|
|
||||||
| if enrollments: | ||||||
| enrollment = enrollments[0] | ||||||
| if not edx_request_success: | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What will this status be? A false? Because the user would already be enrolled in the course in edX |
||||||
| log.warning( | ||||||
| "Webhook: Local enrollment created but edX API call failed for user %s in course run %s", | ||||||
| email, | ||||||
| course_id, | ||||||
| ) | ||||||
| log.info( | ||||||
| "Webhook: Successfully enrolled user %s in course run %s as auditor (role: %s, active: %s, edx_synced: %s)", | ||||||
| email, | ||||||
| course_id, | ||||||
| role, | ||||||
| enrollment.active, | ||||||
| edx_request_success, | ||||||
| ) | ||||||
| return Response( | ||||||
| { | ||||||
| "message": "Enrollment successful", | ||||||
| "enrollment_id": enrollment.id, | ||||||
| "active": enrollment.active, | ||||||
| "edx_enrolled": enrollment.edx_enrolled, | ||||||
| }, | ||||||
| status=status.HTTP_200_OK, | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could be 201 since this is a creation response. |
||||||
| ) | ||||||
| else: | ||||||
| log.error( | ||||||
| "Webhook: Enrollment creation returned empty for user %s in course run %s", | ||||||
| email, | ||||||
| course_id, | ||||||
| ) | ||||||
| return Response( | ||||||
| {"error": "Enrollment creation failed"}, | ||||||
| status=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||||||
| ) | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this change for? At best, Its is not related to this PR.