-
Notifications
You must be signed in to change notification settings - Fork 2
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
fix: invalid certificate uuid should raise 404 #2990
Conversation
cms/models_test.py
Outdated
def test_course_run_certificate_get(user_client): | ||
"""Test that course run certificate get is successful for a valid UUID and raises 404 for invalid UUID""" | ||
certificate = CourseRunCertificateFactory.create() | ||
resp = user_client.get(f"/certificate/{certificate.uuid}/") | ||
assert resp.status_code == 200 | ||
|
||
invalid_uuid = str(certificate.uuid)[0:-12] | ||
resp = user_client.get(f"/certificate/{invalid_uuid}/") | ||
assert resp.status_code == 404 | ||
|
||
|
||
def test_program_certificate_get(user_client): | ||
"""Test that program certificate get is successful for a valid UUID and raises 404 for invalid UUID""" | ||
certificate = ProgramCertificateFactory.create() | ||
resp = user_client.get(f"/certificate/program/{certificate.uuid}/") | ||
assert resp.status_code == 200 | ||
|
||
invalid_uuid = str(certificate.uuid)[0:-12] | ||
resp = user_client.get(f"/certificate/program/{invalid_uuid}/") | ||
assert resp.status_code == 404 |
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.
nit: We can refactor this in this way:
- A test only to check the valid certificate for both course and program certificate
- A parametrized test to check invalid certificates for both course & program certificate
We should check on multiple invalid UUIDs. Ideally making these tests parameterized for invalid e.g.
- Short string
- Empty String
- String longer than the length of UUID
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.
There were no existing tests so I just tried to put in some basic ones quickly but sure, I will add some more as you suggested.
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.
Looks good 👍
Before merging this I would recommend:
- Checking the existing lengths of the certificate UUIDs on Prod
- Checking the existing lengths of the certificate UUIDs on RC
This way we'll make sure that this won't break on existing certificates.
What are the relevant tickets?
Fixes Sentry error
Description (What does it do?)
Using an invalid UUID to get a course run or program certificate results in a 500 server error. It should return 404.
Steps to reproduce:
/certificate/f5ab714b-a7ee-498f-bbec-/
How can this be tested?
/certificate/<uuid>/
and the program certificate at/certificate/program/<uuid>/
. You should see the valid certificates.f5ab714b-a7ee-498f-bbec-
. You should see a 404 page.