-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Add VerificationAttempt model to verify_student application. #35304
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
0001. Extending Identity Verification | ||
##################################### | ||
|
||
Status | ||
****** | ||
|
||
**Accepted** *2024-08-26* | ||
|
||
Context | ||
******* | ||
|
||
The backend implementation of identity verification (IDV) is in the `verify_student Django application`_. The | ||
`verify_student Django application`_ also contains a frontend user experience for performing photo IDV via an | ||
integration with Software Secure. There is also a `React-based implementation of this flow`_ in the | ||
`frontend-app-account MFE`_, so the frontend user experience stored in the `verify_student Django application`_ is often | ||
called the "legacy flow". | ||
|
||
The current architecture of the `verify_student Django application`_ requires that any additional implementations of IDV | ||
are stored in the application. For example, the Software Secure integration is stored in this application even though | ||
it is a custom integration that the Open edX community does not use. | ||
|
||
Different Open edX operators have different IDV needs. There is currently no way to add additional IDV implementations | ||
to the platform without committing them to the core. The `verify_student Django application`_ needs enhanced | ||
extensibility mechanisms to enable per-deployment integration of IDV implementations without modifying the core. | ||
|
||
Decision | ||
******** | ||
|
||
* We will support the integration of additional implementations of IDV through the use of Python plugins into the | ||
platform. | ||
* We will add a ``VerificationAttempt`` model, which will store generic, implementation-agnostic information about an | ||
IDV attempt. | ||
* We will expose a simple Python API to write and update instances of the ``VerificationAttempt`` model. This will | ||
enable plugins to publish information about their IDV attempts to the platform. | ||
* The ``VerificationAttempt`` model will be integrated into the `verify_student Django application`_, particularly into | ||
the `IDVerificationService`_. | ||
* We will emit Open edX events for each status change of a ``VerificationAttempt``. | ||
* We will add an Open edX filter hook to change the URL of the photo IDV frontend. | ||
|
||
Consequences | ||
************ | ||
|
||
* It will become possible for Open edX operators to implement and integrate any additional forms of IDV necessary for | ||
their deployment. | ||
* The `verify_student Django application`_ will contain both concrete implementations of forms of IDV (i.e. manual, SSO, | ||
Software Secure, etc.) and a generic, extensible implementation. The work to deprecate and remove the Software Secure | ||
integration and to transition the other existing forms of IDV (i.e. manual and SSO) to Django plugins will occur | ||
independently of the improvements to extensibility described in this decision. | ||
|
||
Rejected Alternatives | ||
********************* | ||
|
||
We considered introducing a ``fetch_verification_attempts`` filter hook to allow plugins to expose additional | ||
``VerificationAttempts`` to the platform in lieu of an additional model. However, doing database queries via filter | ||
hooks can cause unpredictable performance problems, and this has been a pain point for Open edX. | ||
|
||
References | ||
********** | ||
`[Proposal] Add Extensibility Mechanisms to IDV to Enable Integration of New IDV Vendor Persona <https://openedx.atlassian.net/wiki/spaces/OEPM/pages/4307386369/Proposal+Add+Extensibility+Mechanisms+to+IDV+to+Enable+Integration+of+New+IDV+Vendor+Persona>`_ | ||
`Add Extensibility Mechanisms to IDV to Enable Integration of New IDV Vendor Persona <https://github.com/openedx/platform-roadmap/issues/367>`_ | ||
|
||
.. _frontend-app-account MFE: https://github.com/openedx/frontend-app-account | ||
.. _IDVerificationService: https://github.com/openedx/edx-platform/blob/master/lms/djangoapps/verify_student/services.py#L55 | ||
.. _React-based implementation of this flow: https://github.com/openedx/frontend-app-account/tree/master/src/id-verification | ||
.. _verify_student Django application: https://github.com/openedx/edx-platform/tree/master/lms/djangoapps/verify_student |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
# Generated by Django 4.2.15 on 2024-08-26 14:05 | ||
|
||
from django.conf import settings | ||
from django.db import migrations, models | ||
import django.db.models.deletion | ||
import django.utils.timezone | ||
import model_utils.fields | ||
|
||
|
||
class Migration(migrations.Migration): | ||
|
||
dependencies = [ | ||
migrations.swappable_dependency(settings.AUTH_USER_MODEL), | ||
('verify_student', '0014_remove_softwaresecurephotoverification_expiry_date'), | ||
] | ||
|
||
operations = [ | ||
migrations.CreateModel( | ||
name='VerificationAttempt', | ||
fields=[ | ||
('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), | ||
('created', model_utils.fields.AutoCreatedField(default=django.utils.timezone.now, editable=False, verbose_name='created')), | ||
('modified', model_utils.fields.AutoLastModifiedField(default=django.utils.timezone.now, editable=False, verbose_name='modified')), | ||
('name', models.CharField(blank=True, max_length=255)), | ||
('status', models.CharField(choices=[('created', 'created'), ('pending', 'pending'), ('approved', 'approved'), ('denied', 'denied')], max_length=64)), | ||
('expiration_datetime', models.DateTimeField(blank=True, null=True)), | ||
('user', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to=settings.AUTH_USER_MODEL)), | ||
], | ||
options={ | ||
'abstract': False, | ||
}, | ||
), | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,6 +31,7 @@ | |
from django.utils.translation import gettext_lazy | ||
from model_utils import Choices | ||
from model_utils.models import StatusModel, TimeStampedModel | ||
from lms.djangoapps.verify_student.statuses import VerificationAttemptStatus | ||
from opaque_keys.edx.django.models import CourseKeyField | ||
|
||
from lms.djangoapps.verify_student.ssencrypt import ( | ||
|
@@ -1189,3 +1190,27 @@ class Meta: | |
|
||
def __str__(self): | ||
return str(self.arguments) | ||
|
||
|
||
class VerificationAttempt(TimeStampedModel): | ||
""" | ||
The model represents impelementation-agnostic information about identity verification (IDV) attempts. | ||
|
||
Plugins that implement forms of IDV can store information about IDV attempts in this model for use across | ||
the platform. | ||
""" | ||
user = models.ForeignKey(User, db_index=True, on_delete=models.CASCADE) | ||
name = models.CharField(blank=True, max_length=255) | ||
|
||
STATUS_CHOICES = [ | ||
VerificationAttemptStatus.created, | ||
VerificationAttemptStatus.pending, | ||
VerificationAttemptStatus.approved, | ||
VerificationAttemptStatus.denied, | ||
] | ||
status = models.CharField(max_length=64, choices=[(status, status) for status in STATUS_CHOICES]) | ||
|
||
expiration_datetime = models.DateTimeField( | ||
null=True, | ||
blank=True, | ||
) | ||
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. If your most common query is something like, "does this user have an unexpired verification?", you might want to explicitly make a composite index for that ( 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 took a look at the the query patterns on the IDV tables, and they're almost always querying on the I considered adding a composite index on If you have any thoughts about an index like this, let me know. I'll merge this PR as-is, but I'm happy to put in a follow up PR to adjust the index. Thanks! |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
""" | ||
Status enums for verify_student. | ||
""" | ||
|
||
|
||
class VerificationAttemptStatus: | ||
"""This class describes valid statuses for a verification attempt to be in.""" | ||
|
||
# This is the initial state of a verification attempt, before a learner has started IDV. | ||
created = "created" | ||
|
||
# A verification attempt is pending when it has been started but has not yet been completed. | ||
pending = "pending" | ||
|
||
# A verification attempt is approved when it has been approved by some mechanism (e.g. automatic review, manual | ||
# review, etc). | ||
approved = "approved" | ||
|
||
# A verification attempt is denied when it has been denied by some mechanism (e.g. automatic review, manual review, | ||
# etc). | ||
denied = "denied" |
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.
Is a blank name really ever okay?
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.
It depends. Based on the current Software Secure flow, it is possible but is difficult and likely occurs seldomly. You'd have to really try to get around providing a name, likely by hitting the legacy IDV flow somehow.
The SoftwareSecurePhotoVerification instance can be created without a name if the full name isn't provided during IDV - this is the legacy view but is also true of the MFE based flow. The name is added in a subsequent mark_ready step, but
profile_name
is an optional field of theUserProfile
model, so it can be empty.I don't know the full history of the choice for
blank=True
, but my guess is that before we added the Name Affirmation feature, we only cared that the user was ID verified, and the name was secondary to that fact and was only used optionally for the purposes of a name change - it wasn't necessary to supply a name to become IDV verified. Now, you need to supply a name before IDV because of the Name Affirmation feature; although, there are ways around it.Also, an
SSOVerification
can be created without a name becauseprofile_name
is an optional field. If we adopt the position that IDV is used to verify names and identities, then we could only createSSOVerifications
only when the learner has a name to verify and that has been synced from the third party provider.I don't think
ManualVerification
or our Persona integration will make sense without a name, but I tried to mimic the schema of theIDVerificationAttempt
model here. I'm comfortable making it required based on those two verification methods, but I'm unsure aboutSSOVerification
, and I don't know if additional implementations of IDV may require an IDV attempt without name.If that's something we'd like to enforce, then I can make it
required
, but it's going to require some help from product re:SSOVerifications
and will complicate process of deprecating the Software Secure integration. When deprecating theSoftwareSecurePhotoVerification
model and migrating the data intoVerificationAttempt
, we'll have to drop any instances without aname
value.Do you have any thoughts?
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.
I asked mostly just to check that it wasn't a simple oversight. If this were something being built from scratch, I might push to disallow blank entries here. But I'm not looking to do anything that would complicate the migration process, so
blank=True
it is.