[BD-32] feat: add first batch of Open edX Filters

[mariajgrimaldi]

Description

This PR adds the 1st Open edX Filters batch as part of the implementation plan of Hooks Extension Framework:

• PreEnrollmentFilter
• PreRegisterFilter
• PreLoginFilter

Supporting information

• Hooks Extension Framework OEP-50
  ADR(s) on:
• Open edX Filters naming and versioning: about how to identify filters and manage its versions
• Open edX Filters configuration: how to configure filters
• Open edX Filters tooling: what to use to run filters
• Open edX Filters payload: input arguments for filters

Testing instructions

1. Install openedx-filters library:

pip install openedx-filters==0.4.3

2. Implement your pipeline steps in your favorite plugin. We created some as illustration in openedx-filters-samples. We'll be using those in this example.
3. Install openedx-filters-samples

pip install git+https://github.com/eduNEXT/openedx-filters-samples.git@master#egg=openedx_filters_samples

4. Configure your filters:
   With this configuration, you won't be able to enroll, register or login. To change the behavior you can replace Stop<process> with NoopFilter or remove it completely.

OPEN_EDX_FILTERS_CONFIG = {
    "org.openedx.learning.course.enrollment.started.v1": {
        "fail_silently": False,
        "pipeline": [
            "openedx_filters_samples.samples.pipeline.ModifyModeBeforeEnrollment",
            "openedx_filters_samples.samples.pipeline.StopEnrollment"
        ]
    },
    "org.openedx.learning.student.login.requested.v1": {
        "fail_silently": False,
        "pipeline": [
            "openedx_filters_samples.samples.pipeline.ModifyUserProfileBeforeLogin",
            "openedx_filters_samples.samples.pipeline.StopLogin"
        ]
    },
    "org.openedx.learning.student.registration.requested.v1": {
        "fail_silently": False,
        "pipeline": [
            "openedx_filters_samples.samples.pipeline.ModifyUsernameBeforeRegistration",
            "openedx_filters_samples.samples.pipeline.StopRegister"
        ]
    }
}

Notes about the filters steps:

• ModifyModeBeforeEnrollment: changes enrollment mode to honor
• ModifyUserProfileBeforeLogin: adds previous_login field to the user's profile
• ModifyUsernameBeforeRegistration: changes the user's username before registration, appending -modified

Simple and straightforward implementations as a way of illustrating how they work. More complex steps are coming.

Deadline

None

Other information

Concerns

• The PreRegisterFilter uses the request.data containing the registration form information, including the user's password, which may result in a security leak, right? What can we do to avoid sending the password? maybe removing it from form_data?

[openedx-webhooks]

Thanks for the pull request, @mariajgrimaldi! I've created BLENDED-1027 to keep track of it in Jira. More details are on the BD-32 project page.

When this pull request is ready, tag your edX technical lead.

[mariajgrimaldi]

Hi there! I hope you're all well! I'm tagging you here since this PR is part of the implementation plan of BD-32 or Hooks Extensions Framework.

Please, let us know what you think. @felipemontoya @feanil @ormsbee @jmbowman

[felipemontoya]

To the comment about removing the password.
I can see reasons to leave the password in, for example auditing the security by calling an API at have i been pwned, but that is a small possibility/feature at the cost of opening up a big hole in the security since other plugins could use the password for the wrong reasons.

In any case it would be easy to copy the data dict and remove anything in the list of censored strings (https://github.com/edx/edx-platform/blob/4f4be6538ae008a6d65cd7cc49d776aebfa9e40a/common/djangoapps/track/middleware.py#L64)

[ormsbee]

I'm a little ambivalent on this. Anything running with this level of permission can already do anything they want, security wise. So this is to prevent people accidentally doing horribly unsecure things. That being said, I can think of other fields that edX itself would also consider extremely sensitive, and not want filters to operate on (like certain demographics information).

In any case, I think it's fine to block these off for now, and then we can always add them back in later. I'm imagining that the LMS would always pass through the entire form data, and the PreRegisterFilter implementation would decide how much to pass on to the individual filters or not? And then if we really want to get fancy down the road, we could make that configurable via filter-level configuration?

[mariajgrimaldi]

That was my concern, what developers could do with the form_data -mainly unintentionally-. I took your advice and implemented within the PreEnrollmentFilter class a way of extracting sensitive data -there's a lot of room for improvement-. Here it is: link to Open edX Filter PR #20

We could add more sensitive_form_data in a Django setting to make it more extensible. What do you think? Either way, I like this approach a lot.

[ormsbee]

A bunch of questions, and a few requests for additional tests.

[ormsbee]

Does it make sense to log some of these parameters (user/course_key/mode) before running the filter? The reason I ask is that there are places where CourseEnrollmentException is caught and the message is never used or inspected. From what I can tell, the pipeline will log which step failed and echo out the exception, but that won't necessarily have the inputs that caused it to fail. I guess the main thing I want to understand is: when one of these filters does something wrong with a really unhelpful message like NoneType is not iterable, will we have enough information in the logs to understand how to reproduce the issue?

FWIW, I think that making EnrollmentNotAllowed a subclass of CourseEnrollmentException, and then doing this bit of except/raise here is a great idea. It maintains backwards compatibility in a lightweight and very self-contained way. Kudos.

[mariajgrimaldi]

From what I can tell, the pipeline will log which step failed and echo out the exception, but that won't necessarily have the inputs that caused it to fail.

You're right. Let's test those scenarios:

Case 1. The developer raises PreventEnrollment from its filter step:
With this config:

OPEN_EDX_FILTERS_CONFIG = {
    "org.openedx.learning.student.login.requested.v1": {
        "fail_silently": False,
        "pipeline": [
            "openedx_filters_samples.samples.pipeline.ModifyUserProfileBeforeLogin",
            "openedx_filters_samples.samples.pipeline.StopLogin"
        ]
    },
}

When trying to login:

Case 2. The filter step fails and raises NoneType error and fail_silently is False

What we see first

If we scroll up we'll find:

It seems like too little and scarce information, that's why we've been working on a better way for logging this type of errors. For example:

Using this configuration:

OPEN_EDX_FILTERS_CONFIG = {
    "org.openedx.learning.student.login.requested.v1": {
        "fail_silently": False,
        "pipeline": [
            "openedx_filters_samples.samples.pipeline.ModifyUserProfileBeforeLogin",
            "openedx_filters_samples.samples.pipeline.StopLogin"
        ],
        "log_level": "info"/"debug"
    },
}

Case 1.

INFO log level:

While DEBUG log level:

Case 2.

INFO log level:

While DEBUG log level:

I've been implementing this logging strategy for a later release in this PR: https://github.com/eduNEXT/openedx-filters/pull/17

[ormsbee]

It's great to see that you folks are already tackling this issue. 😄 Thanks for pointing me to that PR!

[ormsbee]

AuthFailedError defines a number of fields. Is constructing it like this sufficient?

[ormsbee]

I'm also not clear why we're using the pattern of using str(exc) as the message for these exceptions. Is the idea that we'd lose the original source exception information otherwise? Or do we expect that the exception message might actually be bubbled up to users directly?

[mariajgrimaldi]

1. I added more info! Now, the PreLoginException is looking like this and our filter step (in our samples plugin), like this

2. We -as it is right now, this can change if necessary- expect the developer to describe the error in a friendly and helpful way in the exception message to show it to the end-user. This way, we define just one helpful message, and the error information is logged in for the developer to see. What do you think it's best? Maybe we could use a developer_message and user_message 🤔

[ormsbee]

Does it do the right thing when this configuration is specified but the pipeline is empty?

[ormsbee]

Also, should we have a test with more than one step?

[mariajgrimaldi]

1. Yes, it behaves like a noop. I'll attach some tests:

• Unit-test from openedx-filters: test_run_empty_pipeline
• On my dev environment:
  Using this configuration ->

OPEN_EDX_FILTERS_CONFIG = {
    "org.openedx.learning.student.login.requested.v1": {
        "fail_silently": False,
        "pipeline": [],
        "log_level": "debug",
    },
}

2. Of course! I'll try that

[ormsbee]

Could you please put in a test that returns this successfully when the filter is not running? That way, we can be absolutely sure that it's the filter causing the 400 error, and not that data is somehow being malformed in the test itself.

[ormsbee]

I'm a little ambivalent on this. Anything running with this level of permission can already do anything they want, security wise. So this is to prevent people accidentally doing horribly unsecure things. That being said, I can think of other fields that edX itself would also consider extremely sensitive, and not want filters to operate on (like certain demographics information).

In any case, I think it's fine to block these off for now, and then we can always add them back in later. I'm imagining that the LMS would always pass through the entire form data, and the PreRegisterFilter implementation would decide how much to pass on to the individual filters or not? And then if we really want to get fancy down the road, we could make that configurable via filter-level configuration?

[ormsbee]

I'm a little concerned about how we maintain the contract here with form_data, since it seems like the LMS is free to remove a field at any time without the filter needing to be updated, which would potentially cause filters to break downstream. I'm not sure how to address that given the optional nature of a lot of form data. I'm not going to block this PR on it, since I think it's more important to get this out for people to try, and it's better than what exists now. But I am curious what your thoughts on it are these days.

[mariajgrimaldi]

Formally, we haven't had any discussion about this specific case. But what I think is, why don't we take a screenshot of the form_data contents as they are right now? And use that as v1. Then, if the form's contents change, we update the filter.

What do you think? @ormsbee @felipemontoya

[xitij2000]

In this scenario, i.e. if you bump up the version of the filter from v1 to v2, how do those two interoperate? Do they interoperate?

What I mean is, if you bump up a filter to v2, will support be immediately dropped for v1? If not, then how do you work in a mixed environment of some v1 filters and some v2 filters.

[mariajgrimaldi]

Following the Open edX filters naming and versioning ADR, they must interoperate:

• Open edX core developers wanting to deprecate and eventually remove filters.

This is what I imagine it'd be:

# data.keys -> ["username", "email", "name", "password", "mailing_address"]
data = PreRegisterFilter.run(form_data=data)  # Filter expects data.keys -> ["username", "email", "name"]
data = PreRegisterFilterV2.run(form_data=data) # Filter expects data.keys -> ["username", "email"]
data = PreRegisterFilterV3.run(form_data=data) # Filter expects data.keys -> ["username"]

For each filter, a different configuration:

OPEN_EDX_FILTERS_CONFIG = {
    "org.openedx.learning.course.enrollment.started.v1": {
        "fail_silently": False,
        "pipeline": [
            "openedx_filters_samples.samples.pipeline.ModifyModeBeforeEnrollment",
            "openedx_filters_samples.samples.pipeline.StopEnrollment"
        ]
    },
    "org.openedx.learning.course.enrollment.started.v2": {
        "fail_silently": False,
        "pipeline": [
            "openedx_filters_samples.samples.pipeline.ModifyModeBeforeEnrollmentV2",
            "openedx_filters_samples.samples.pipeline.StopEnrollmentV2"
        ]
    },
    "org.openedx.learning.course.enrollment.started.v3": {
        "fail_silently": False,
        "pipeline": [
            "openedx_filters_samples.samples.pipeline.ModifyModeBeforeEnrollmentV3",
            "openedx_filters_samples.samples.pipeline.StopEnrollmentV3"
        ]
    },
}

Filters, by definition, must return what they receive (the whole data.keys), but they'll be able to use just the subset that the filter defines. I imagine this evolution happening because we decide that the filter can't modify some fields 🤔

This is just me throwing some ideas. I'm thrilled to hear what you think

[ormsbee]

@mariajgrimaldi:

Formally, we haven't had any discussion about this specific case. But what I think is, why don't we take a screenshot of the form_data contents as they are right now? And use that as v1. Then, if the form's contents change, we update the filter.

So we make the current fields a part of the filter's contract, and if edx-platform changes the field names for some reason, we do the translation at the filter level so that any pipeline steps that have been written are unaffected? That sounds good to me. The only question I'd have then is how we detect a break in the contract from edx-platform testing–i.e. if I change a field name right now, will it automatically get caught by a filter-related test breaking in CI?

[felipemontoya]

I agree with the sentiment that we should not block the PR on the issue of maintaining the contract that form_data provides. I'm actually going to propose the counter argument. Should we at all try lock in the keys in form_data dict and maintain them? I think not.

There is always going to be some degree of flexibility due to the optional fields which will make this very difficult if at all possible. From my perspective, the Filter at the student registration is the key piece of extension that allows any sort of registration flow to be handled. A developer could now remove or dramatically alter the form at the registration page and send any sort of info, which the filter will receive and "normalize" into something that the core platform can use to create the final user.

E.g:

• a custom MFE for registration gathers names in spanish customs (2 names, two last names), national id, and phone (I've seen this kind of requirement many times).
• the filter processes this dict and creates a completely new form_data that is consistent with the openedx user models and views. Fullname, meta fields for ID and all that. Creates a password since it was not included and emails it to the user (horrible experience, but I have also have had this requested from us).

Filters, specially one like the filter before a student registers, are going to have some risk of breaking since we are passing in memory objects to them. The same would be true for a filter that receives a user object or enrollment object. Eventually it will have to deal with certain methods of the object changing. I think we have to embrace that and learn to write filters that are delicate in the way the handle the live data that is passed to them.

[giovannicimolin]

I think I'm on the side of not enforcing the form_data contract, it's up to filter developers to make sure their filters return sane data to the platform (it's similar to Django plugin apps - we have more freedom, but a plugin can easily break the LMS if implemented wrong).

[felipemontoya]

@ormsbee has any of this conversation swayed you to not fixing in place the content of form_data?
This is probably the last outstanding request to change the current form of this PR. If the issue is not blocking for you, would you agree that we go ahead with this?

[ormsbee]

@felipemontoya: Yeah, it's not a blocker.

[mariajgrimaldi]

Hi there @ormsbee!

Sorry for the delayed response, I was finishing some sprint tasks 😃 I'll be answering all your questions today! Thanks for the patience 🤘

[felipemontoya]

fyi @nizarmah @giovannicimolin

[giovannicimolin]

@felipemontoya I'd like to review this, but I'll only be available early next year due to end of year holidays. Don't block merging this on me.

[mariajgrimaldi]

Hi there! I have some updates:

We now have a beta release of Open edX Filters (v0.4.2), equipped with login, registration, and enrollment filters ready to use, as we see in this PR 😄 and in our samples repository.

Now, this is what's left to address within this integration (non-blocking):

• Form data contract as mentioned in this thread
• Logging strategy, coming in a later release. We'll update openedx-filters version when it's ready to use.

As I see it, we don't have any other blocking issue to solve. Do you think of any? What do you think is the status of this PR?

@felipemontoya @feanil @giovannicimolin @ormsbee @xitij2000

[felipemontoya]

I spent a good while testing this and trying to make all sort behavior changes using filters.

At this moment I'm very happy with the result. Thanks @mariajgrimaldi great work.

After fixing tests and probably squashing the ~22 commits I'd say we are ready to merge. However, and this is not blocking for me, we could make the filters respond a little better after the filter is run and the platform code takes back control.

1. StudentLoginRequested: we could connect the redirect_url value to the UI. Should the filter return that value via the exceptions or regular operation. Currently the AJAX caller just ignores the returned value.
2. CourseEnrollmentStarted: we could pipe the exception message to the UI. Currently it will always say "Could not enroll" since it's fixed at: https://github.com/edx/edx-platform/blob/master/common/djangoapps/student/views/management.py#L386

In the case of StudentRegistrationRequested I think it is already working quite nicely, but I would still like to hear some opinion from others about making the contract of the form_data fixed or not.

My greatest priority right now is to get this out and start gathering feedback so again, I would not block on any of the above.

[edx-status-bot]

Your PR has finished running tests. There were no failures.

[MaferMazu]

The filters worked for me.
Tested: ModifyUsernameBeforeRegistration, ModifyUserProfileBeforeLogin, StopLogin, StopRegister and StopEnrollment.

[giovannicimolin]

@mariajgrimaldi @felipemontoya Awesome work on this! I'm looking forward to seeing this in a named release in the near future. 🚀

👍
I've tested the filters with using the examples provided and they worked perfectly.

I have one question/suggestion:
Since filters will be central to the platform (and possibly widely used by it) does it make sense to move the filter pipeline implementation into the core instead of having it in an external plugin? That would make maintenance easier by not having to deal pinning versions and forking the plugin repo when working on the filter pipeline on named releases.

[felipemontoya]

Hi @giovannicimolin, thanks a lot for the review.

To the question about the filter pipeline implementation being in the external plugin.

One of the key aspects of the filters architecture is that as a developer, you are easily capable of testing the code you are writting. This force-pushes the definition of the filters to the external library so that it is easy to import in your third party code to test. We thought we could extend this to the pipeline implementation as well, since this would allow us to have it available both in core-platform tests and plugin tests.
For maintenance we will indeed need to be pinning the library, but that was true either way because of the definition of the filters themselves. This will not necessarily prevent errors, but it will make it easy to catch them during testing.

[shadinaif]

Thank you @mariajgrimaldi @felipemontoya for this effort. I've tried it and it works like described. Very nice. I have a couple of questions though:

1- fail_silently is very handy. Can it be per-filter instead of per-pipeline?

2- The pipeline fails silently if something is not right with configuration. For example: I forgot to install my pre-registration filter package! the pipeline fails with a ModuleNotFoundError exception logged and the registration is completed normally after that. Knowing that fail_silently is set to False. Wouldn't be better if it fails similar to failed filters to avoid having useless pipelines in my configuration?

[mariajgrimaldi]

Hi there @shadinaif. Thank you so much for the review and the fresh insight!

1. A filter is a list of functions executed as a pipeline, then, at the end of the day is the filter that fails silently. Now, we could have a fail_silently per pipeline step. Do you think of a use case? I imagine we have a few steps that perform some read operations and a step that performs a write operation, then, I care about what this step has to say, but that could work as it is right now.
2. You're right! Just logs the exception reporting the import error, maybe crashing would be better? What do you think @shadinaif @felipemontoya?

[felipemontoya]

2. You're right! Just logs the exception reporting the import error, maybe crashing would be better? What do you think @shadinaif @felipemontoya?

For consistency I think if fail_silently is false in this scenario then it should definitely let the error pass and probably this means it would crash. However if fail_silently is set to true, then the crashing should stop and it is back to logging.

[shadinaif]

Thank you @mariajgrimaldi and @felipemontoya for the quick response!
I see we're on the same page regarding the second point, cool 😄

For the first point;
My use case is having critical and non-critical filters in the same pipeline. For example: a package containing many filters that only read data to track them (maybe do db inserts in another tracking database). I can afford to have this filter fail silently, but I can't let another critical verification filter fail unnoticed.

I think the above scenario can be solved by having the critical filter run before the non-critical one. But this might not always solve the problem (order wise)

If the above makes any sense to you; then I have a suggestion:
Remove the fail-silently flag from the pipeline and add some kind of a method in PipelineStep class that we can inherit:

    @staticmethod
    def get_fail_silently():
        return False

This way, the default is raising exceptions; if the developer sees the filter as safe-to-fail, then an override of the method is needed. The inherited method can also read from customized configs; which is awesome

Again; the above is just a suggestion it can be a future enhancement upon your vision (if it makes sense)

In the end; this PR is good to go 🚀 and many thanks for your efforts 👍🏼

[mariajgrimaldi]

Hi there @shadinaif! I just published a new openedx-filters version to Pypi that works as you suggested. Can you help me test it? 😄

@ormsbee can you help us with a review so we can merge? 🥇

[shadinaif]

Hi @mariajgrimaldi , it's awesome, works like a charm. Thank you!

[openedx-webhooks]

@mariajgrimaldi 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

[felipemontoya]

Thanks a lot to everyone for all the reviews, comments and all the work poured into this initiative. Thanks to @mariajgrimaldi for all the revisions, updates and rebases.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the production environment.

[ormsbee]

@mariajgrimaldi, @felipemontoya: Congrats folks!

Would one of you be up for posting a quick update about the recent Hooks work in the forums? I think that a lot of folks would be interested in filters being added and the openedx-events + message bus discussions in https://github.com/eduNEXT/openedx-events/issues/38 and https://github.com/eduNEXT/openedx-events/issues/39. I can do it if you don't have time, but I feel like you folks should take your bows for this.

Please also put a mention of it in the release page for Nutmeg.

Thank you!

[mariajgrimaldi]

Thank you so much for the help @ormsbee! Here is the post I've created 🥳