20220112 email verified

[peterthomassen]

[peterthomassen]

The PR improves (hopefully) on the way authenticated actions are processed. User.is_active is now nullable, with the null value representing pending activation. Activated users have is_active = True, and locked ones have is_active = False, as before. Users which have not logged in and previously had is_active = False are pending activation (in the old scheme) and are migrated to "null".

Authenticated actions now only can be performed by active users (except account activation, which only requires that is_active != False, so null is fine). Locked users cannot perform any authenticated actions any longer.

The PR also adds User.email_verified which is set any time an auth action code is presented and passes the serializer validation. This means that the code is structurally valid and not expired (this includes cases where the action is not executed because the user is locked, but the code was otherwise valid).

We started verifying email addresses on Oct 23, 2019. Users which signed up on or after this date and have is_active = True or have logged in at least once have email_verified set to created. (The last case covers locked users, for which we set is_active = False manually at some point.)

[peterthomassen]

rebased on top of #592, i.e. first commit doesn't actually belong to this PR

[peterthomassen]

I've read everything before "feat(api): add User.outreach_preference" and left questions and comments on the code. Rest will follow later.

Addressed all your comments. The confirmation email sending is refactored in 65ed7a9d9d6e5b651e6d95bb801ce75e11360909. I have a few thoughts on that:

• confirm-account and change-outreach-preference confirmations still need to have their templates adapted
• In conventional DRF use, save() would call update() when self.instance is present (as is the case here), which requires having called .is_valid(); however, .is_valid() is not applicable here (as we don't initialize the serializer with a serialized instance). I still used save() because it makes clear that some kind of write operation happens. I'm happy to rename the method to something else though, if we don't want to break DRF style.
• Sending a confirmation email from a view now requires 1.) instantiating the action, 2.) passing it to the serializer, 3.) saving the serializer. Steps 2 and 3 are pure boilerplate, so we could give the action seralizer a class method that takes the arguments for 1.) and also executes 2.) and 3.). What do you think?
• I introduced the action_user property on the action serializers so that the save() method can be written identically both for individual and for list serializers, so that it could get outsourced to AuthenticatedBasicUserActionMixin.
  • Is that overengineered?
  • I called the property user initially, but that collided with the case where user is also the name of an action parameter (which is always the case at the moment). I can imagine similar problems to happen again in the future, without getting caught, with unintended side effects. -- Thus: Should we prefix attributes that are not action parameters with an underscore (_reason, _validity_period, _user (instead of action_user))? -- Those would then be accessed freely across the AuthenticatedActionSerializer inheritance hierarchy.
  • I also investigated moving the email target user to context, but it looked like that would cause more problems than it would solve.

feat(api): introduce auth action to verify email for legacy accounts This commit can be reverted once all email addresses have been verified.

I believe the migration file must be retained and reverted by another migration. Maybe instead implement (now or later) an action for re-verification of accounts that can be triggered if needed.

Good point. Regarding your suggestion, isn't that what this commit implements? (The only change being that we wouldn't revert it.)

[peterthomassen]

rebased on top of main

[peterthomassen]

Also, 67ee0d8a5d0237f491bcd56382af5d779df68f52 essentially makes 6719dc5d5ce851372967ae5d4cd07b4db07d5bd2 obsolete (both are in this PR). But I suspect it would be very messy to get rid of the latter ...

[nils-wisiol]

* In conventional DRF use, `save()` would call `update()` when `self.instance` is present (as is the case here), which requires having called `.is_valid()`; however, `.is_valid()` is not applicable here (as we don't initialize the serializer with a serialized instance). I still used `save()` because it makes clear that some kind of write operation happens. I'm happy to rename the method to something else though, if we don't want to break DRF style.

I'm fine with save.

* Sending a confirmation email from a view now requires 1.) instantiating the action, 2.) passing it to the serializer, 3.) saving the serializer. Steps 2 and 3 are pure boilerplate, so we could give the action seralizer a class method that takes the arguments for 1.) and also executes 2.) and 3.). What do you think?

I like that.

* I introduced the `action_user` property on the action serializers so that the `save()` method can be written identically both for individual and for list serializers, so that it could get outsourced to `AuthenticatedBasicUserActionMixin`.
  
  * Is that overengineered?
  * I called the property `user` initially, but that collided with the case where `user` is also the name of an action parameter (which is always the case at the moment). I can imagine similar problems to happen again in the future, without getting caught, with unintended side effects. -- Thus: Should we prefix attributes that are not action parameters with an underscore (`_reason`, `_validity_period`, `_user` (instead of `action_user`))? -- Those would then be accessed freely across the `AuthenticatedActionSerializer` inheritance hierarchy.

I think action_user is fine.

feat(api): introduce auth action to verify email for legacy accounts This commit can be reverted once all email addresses have been verified.

I believe the migration file must be retained and reverted by another migration. Maybe instead implement (now or later) an action for re-verification of accounts that can be triggered if needed.

Good point. Regarding your suggestion, isn't that what this commit implements? (The only change being that we wouldn't revert it.)

:+1:

[nils-wisiol]

Let's merge everything including up to bd84f7e0e153e3e7e4220fe36ad40012cab6a52f and move the rest to a new PR

[peterthomassen]

I believe all comments have been addressed! The outreach management command is now there and covers all cases we need (I think; let's talk about how it does that). I think it's ready!

[peterthomassen]

Alright, this time ready for real. Did some minor tweaks here and there, added some unrelated bug fixes, and put all the commits in right order.

[nils-wisiol]

Note the one comment above. While at it, maybe add an example for how to use this to README?