openedx / public-engineering

General public issue repository for the Open edX engineering community
4 stars 2 forks source link

[DEPR]: User email activation check in user.is_active #165

Open robrap opened 2 years ago

robrap commented 2 years ago

Proposal Date

2022-12-08

Target Ticket Acceptance Date

2022-12-23

Earliest Open edX Named Release Without This Functionality

Palm - 2023-04 (Q+ more likely)

Rationale

In LMS/Studio, we currently have a non-standard use of Django's user.is_active which often adds confusion, and cause incompatibilities with third party libraries that expect a the more typical use of user.is_active. Our non-standard muddies is_active by also possibly meaning whether or not the user's email has been activated. See the following history for more details.

Historical context:

  1. In the early days, there was a business rule that a user could not log-in (except initial registration login) without having verified email. The implementation was to override Django's is_active to include also looking at user's email activation status. This is done in edx-platform, and affects LMS/Studio. The default implementation of DRF's SessionAuthentication checks is_active, so APIs could also not be called by users without email activation. This implementation often confuses engineers, because it is an unexpected behavior for is_active.
  2. Mobile was added with new business requirements to be less strict about email activation. SessionAuthenticationAllowInactiveUser was introduced, which ignores is_active, and was added to a variety of endpoints to enable mobile to bypass the check for email activation. Since the is_active check was being bypassed, it also meant that we could not use is_active to disable user accounts. In LMS/Studio, there has been a switch to using has_usable_password to determine if an account has been disabled, which makes the platform more complicated to understand, and doesn't work across services.
  3. MFEs were introduced, making API calls on services protected by a newer JwtAuthentication. To simplify things, our JwtAuthentication implementation drops the is_active check, and we don't have a version of it that checks is_active (or email activation status). Many endpoints are now protected by JwtAuthentication and SessionAuthentication in this order. See https://github.com/openedx/edx-platform/search?q=SessionAuthentication. This means email activation will not be checked if the user authenticates with a JWT, but it will be checked if they have a session, but not a JWT. This adds to the confusion. See https://github.com/openedx/edx-platform/pull/31267 for a sample fix for this issue to a single endpoint.
  4. (This needs to be confirmed by the Product-WG.) The original business rule was relaxed, and now login is allowed on certain deployments, even without email activation. However, API endpoints using SessionAuthentication continue to block access.

Removal

Replacement

Deprecation

The main function affected, is_active, will not actually be deprecated, but will be changing behavior.

Some classes like SessionAuthenticationAllowInactiveUser would be deprecated, and could be marked as such.

Migration

Needs planning:

  1. Can we use activation_timestamp as-is, or do we need to back-fill?
  2. Detailed plan would be needed for how and when is_active will be updated, and when and if to sync with has_unusable_password and other disabling capabilities. In theory, everyone could temporarily be set to is_active = True once we have replacement explicit checks for email/account activation that is separate.

Additional Info

Once is_active is no longer tied to email/account activation, this would open up the following possibilities:

  1. Restore is_active to the original Django meaning of this field, where it can be set to false to disable (soft delete) user accounts. This might require changes where has_usable_password was being used for disabled accounts.
  2. Add the is_active check to JwtAuthentication, which would now be limited to checking if an account were disabled.

Other Thoughts:

robrap commented 2 years ago

Note: I was wondering if the relaxing of business requirements might make this problem more tractable at this time, but not sure. I'm sure there are lots of additional things to look at, like:

  1. Commerce is_account_activation_requirement_disabled
  2. Social auth clean-up
kdmccormick commented 1 year ago

Thanks for the detailed writeup. It is confusing that email activation is sometimes required and sometimes not, and that it may depend on undocumented settings.

I support the change in behavior.

If email activation is a community-desired feature, I'd rather it be re-implemented either via a hooks plugin, or in edx-platform using a more explicit field, something like UserProfile.is_email_activated.

kdmccormick commented 1 year ago

Regarding:

(This needs to be confirmed.) The original business rule was relaxed, and now login is allowed on certain deployments, even without email activation. However, API endpoints using SessionAuthentication continue to block access.

Unconfirmed, but I assume SKIP_EMAIL_VALIDATION is how certain deployments relax this business rule?

robrap commented 1 year ago

Thanks for the thoughts @kdmccormick.

Unconfirmed, but I assume SKIP_EMAIL_VALIDATION is how certain deployments relax this business rule?

At edX.org this is using the default, which is False, so not sure. I'm also not certain about the relaxed business rules, other than knowing about the Mobile vs Web split. I plan to add some text to make this potentially easier to digest by Product, and to ensure that the Product WG gets to see this to help weigh-in on the business rules.

As you noted, no matter what the rules, there should be another way to implement.

bradenmacdonald commented 1 year ago

I am a big fan of this change. One point which I don't think was fully captured here: the way that the platform currently uses is_active in a non-standard way causes issues when integrating third party django apps that expect it to work the way it does in every other django service. DRF is mentioned as one example, but python-social-auth is another and I believe there have been more. So extra work is required to modify those apps to work with edx-platform instead of just being able to use them.

robrap commented 1 year ago

@feanil: FYI: I added a roadmap issue to get Product input on requirements: https://github.com/openedx/platform-roadmap/issues/266. Feel free to update the roadmap issue as you see fit.

robrap commented 1 year ago

[inform] I just found the decorator view_auth_classes which is used in many place in edx-platform, and buries behind it use of SessionAuthenticationAllowInactiveUser.