Add CyberSource Export Compliance screening to enrollment - #3706
Conversation
OpenAPI ChangesShow/hide ## Changes for v0.yaml:Unexpected changes? Ensure your branch is up-to-date with |
92b21c2 to
c0d8121
Compare
e6d3938 to
a029c40
Compare
rhysyngsun
left a comment
There was a problem hiding this comment.
Good start, but I think we need to address how code like views and any tasks that call this code handle it. So there should also be accompanying tests that verify the API responses when a user is not permitted so the frontend can display an appropriate message to the user.
There should also be a feature flag for turning this on.
| try: | ||
| from CyberSource.api.verification_api import VerificationApi | ||
| from CyberSource.models.riskv1exportcomplianceinquiries_order_information import ( | ||
| Riskv1exportcomplianceinquiriesOrderInformation, | ||
| ) | ||
| from CyberSource.models.riskv1exportcomplianceinquiries_order_information_bill_to import ( | ||
| Riskv1exportcomplianceinquiriesOrderInformationBillTo, | ||
| ) | ||
| from CyberSource.models.riskv1liststypeentries_client_reference_information import ( | ||
| Riskv1liststypeentriesClientReferenceInformation, | ||
| ) | ||
| from CyberSource.models.validate_export_compliance_request import ( | ||
| ValidateExportComplianceRequest, | ||
| ) | ||
| except ModuleNotFoundError as exc: | ||
| VerificationApi = None | ||
| Riskv1exportcomplianceinquiriesOrderInformation = None | ||
| Riskv1exportcomplianceinquiriesOrderInformationBillTo = None | ||
| Riskv1liststypeentriesClientReferenceInformation = None | ||
| ValidateExportComplianceRequest = None | ||
| _CYBERSOURCE_IMPORT_ERROR = exc | ||
| else: | ||
| _CYBERSOURCE_IMPORT_ERROR = None |
There was a problem hiding this comment.
Under what scenarios would an import fail, shouldn't the library just be installed?
| try: | ||
| legal_address = user.legal_address | ||
| except ObjectDoesNotExist: | ||
| legal_address = None |
There was a problem hiding this comment.
In this case if we have no legal address we should fail the operation and this should get bubbled up to the user as an error to either fix the data or contact support.
There was a problem hiding this comment.
With descriptive message on what is missing?
| f"decision={result.decision!r}, reason_code={result.reason_code!r}" | ||
| ) | ||
| log.warning(message) | ||
| raise ValidationError(message) |
There was a problem hiding this comment.
I think this should be raising a custom error type that higher level code (views, tasks, etc) catch and log / return errors to the user.
4c17268 to
6008eb6
Compare
804c79b to
02a1ff6
Compare
bcf6a7e to
981b860
Compare
5075c2f to
49a74fc
Compare
for more information, see https://pre-commit.ci
| created in mitxonline, paired with a boolean indicating whether or not the edX enrollment API call was successful | ||
| for all of the given course runs | ||
| """ | ||
| _verify_exports_compliance_for_enrollment(user) | ||
|
|
||
| if keep_failed_enrollments is None: | ||
| keep_failed_enrollments = settings.FEATURES.get( | ||
| features.IGNORE_EDX_FAILURES, False |
There was a problem hiding this comment.
Bug: Administrative operations like downgrade_learner will crash due to unhandled ExportComplianceError or ExportComplianceDataError exceptions if the user fails a compliance check.
Severity: MEDIUM
Suggested Fix
In administrative functions like downgrade_learner, wrap the call to create_run_enrollments in a try/except block to catch and log compliance-related exceptions without halting the downgrade process. Alternatively, add a parameter to create_run_enrollments to conditionally skip the compliance check for internal, administrative operations.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: courses/api.py#L204-L211
Potential issue: Administrative operations like downgrading a learner's enrollment via
`downgrade_learner` or processing a refund via the `perform_downgrade_from_order` task
now unconditionally trigger an export compliance check. If this check fails by raising
an `ExportComplianceError` or `ExportComplianceDataError` (e.g., due to a user's
incomplete profile data), the exception is unhandled. This will crash the administrative
task, preventing the downgrade and leaving the user's enrollment in an inconsistent
state (e.g., still verified when it should be audit). This bug will only manifest if the
`EXPORT_COMPLIANCE_CHECK_ENABLED` feature flag is active.
Also affects:
courses/api.py:402~426
Did we get this right? 👍 / 👎 to inform future reviews.
| if enrollment.enrollment_mode == EDX_ENROLLMENT_VERIFIED_MODE | ||
| ] | ||
|
|
||
| if len(verified_program_enrollments) == 0: | ||
| # No verified enrollments, so it doesn't matter - the user will get an | ||
| # audit one. (But make the audit enrollment to not confuse the course run | ||
| # process later.) | ||
| create_program_enrollments( | ||
| request.user, programs, enrollment_mode=EDX_ENROLLMENT_AUDIT_MODE | ||
| ) | ||
| elif ( | ||
| len(verified_program_enrollments) == 1 | ||
| and verified_program_enrollments[0].program == root_program | ||
| ): | ||
| # The verified enrollment that's here is for the root program, so we can | ||
| # create a verified enrollment for the other program. | ||
| create_program_enrollments( | ||
| request.user, programs, enrollment_mode=EDX_ENROLLMENT_VERIFIED_MODE | ||
| ) | ||
| elif ( | ||
| len(verified_program_enrollments) == 1 | ||
| and verified_program_enrollments[0].program != root_program | ||
| ): | ||
| # The verified enrollment that's here is _not_ for the root program, so | ||
| # we should stop. | ||
| log.error( | ||
| "add_verified_program_course_enrollment: user %s enrolling in %s has no verified enrollment in %s", | ||
| request.user, | ||
| courserun_id, | ||
| root_program, | ||
| ) | ||
| return Response(status=status.HTTP_400_BAD_REQUEST) | ||
| error_response = _reconcile_verified_program_enrollments( | ||
| request, courserun_id, root_program, verified_program_enrollments, programs | ||
| ) | ||
| if error_response is not None: | ||
| return error_response | ||
|
|
||
| # If we fell out the bottom, we have all verified enrollments, or we've made | ||
| # sufficient enrollments to fill the gaps. |
There was a problem hiding this comment.
Bug: A double export compliance check occurs during program enrollment, creating inefficiency and a risk of inconsistent state if the second check fails.
Severity: LOW
Suggested Fix
Refactor the enrollment logic to ensure the export compliance check is only performed once for the user at the beginning of the enrollment flow. The result of this single check should be passed down and reused for all subsequent enrollment creation steps within the same user operation.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: courses/views/v2/__init__.py#L959-L969
Potential issue: When enrolling in a program, a compliance check is performed by
`_reconcile_verified_program_enrollments`. If this succeeds and the process continues to
create an associated audit enrollment for a course run, a second compliance check is
performed inside `create_run_enrollments`. This double check is inefficient. More
importantly, it creates a risk of an inconsistent state where the program enrollment is
successfully created but the subsequent course run enrollment fails on the second check,
leaving the user partially enrolled.
Did we get this right? 👍 / 👎 to inform future reviews.
What are the relevant tickets?
Closes https://github.com/mitodl/hq/issues/11991
Description (What does it do?)
Add CyberSource Export Compliance screening to course enrollment.
Required fields https://developer.cybersource.com/library/documentation/dev_guides/Verification_Svcs_SO_API/html/Topics/Request_Fields.htm
How can this be tested?