Skip to content
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

Cannot change superuser email when using custom flag field and createsuperuser #144

Open
Neraste opened this issue May 26, 2021 · 3 comments · May be fixed by #145
Open

Cannot change superuser email when using custom flag field and createsuperuser #144

Neraste opened this issue May 26, 2021 · 3 comments · May be fixed by #145
Labels

Comments

@Neraste
Copy link

Neraste commented May 26, 2021

Describe the bug

I changed the default flag field with USER_VERIFICATION_FLAG_FIELD and created a superuser with the Django command createsuperuser. Changing this superuser's email results in an error when verifying the email.

Expected behavior

Superuser email is changed.

Actual behavior

Error 400 with the error message: "User not found."

Steps to reproduce

  1. Create a Boolean field in user model and set USER_VERIFICATION_FLAG_FIELD to this field;
  2. Create a superuser with Django admin command createsuperuser;
  3. Register a new mail for this superuser;
  4. Validate the new email.

Possible explanation

This is due to the fact createsuperuser will not enable the custom flag field (as it sets username, email and password by default), hence this superuser is not considered as valid and cannot be retrieved.

In rest_registration.api.views.register_email.process_verify_email_data, get_user_by_verification_id is called with argument require_verified True by default.

Associated PR

See #145.

@Neraste Neraste linked a pull request May 26, 2021 that will close this issue
@apragacz
Copy link
Owner

apragacz commented Jun 1, 2021

Hi @Neraste ,
thanks for a very detailed bug report.

I'm still unsure if that's an actual bug; I'm trying to understand your use case.

Does that mean that you want your users to be able to log in when they are still unverified? and after they log in, they could theoretically change their email to another one? that use case may not work as expected, as using different field from is_active for a verification is not yet explored well.

The current assumption was to use only verified user data (with some exceptions, like reset password) to avoid security issues. But it looks that we could loosen up that requirement, as register_email endpoint requires authentication anyway. Perhaps it should be an opt-in setting, and at some point it could become the default one after some time? Do you think this is the right way to go?

@apragacz apragacz added the state:needs-answer Needs additional information from the issue creator label Jun 3, 2021
@Neraste
Copy link
Author

Neraste commented Jun 3, 2021

I could see from the tests that using a different field than is_active is not deeply explored for now. My initial reason to use a such a field was because the Django documentation considers it's not designed for that use:

This [field] doesn’t necessarily control whether or not the user can log in. Authentication backends aren’t required to check for the is_active flag but the default backend (ModelBackend) and the RemoteUserBackend do. You can use AllowAllUsersModelBackend or AllowAllUsersRemoteUserBackend if you want to allow inactive users to login.

This also means that a using the AllowAllUsers*Backends would allow un-validated users to log in.

I think using is_active blurs the line between "this user is active" (which doesn't automatically mean they can log in) and "this user is allowed to log in."

A superuser created with the createsuperuser command is automatically active and, with the default field, their email address is also automatically valid, even if no verification occurs. They can log in because they are active/their email is valid. But for me, a superuser should log in whatever their email is because they are superuser and they are active, not because their email is valid, automatically or not.

Using a custom field, the superuser has not their email address validated by default. Their ability to log in depends on the authentication backend, but they cannot change their email with this library, which sounds unexpected and is the reason of this issue.

On the other hand, using is_active is convenient when using this library, you don't have to create a new field in your user model, and you don't have to write a custom authentication backend neither. So I also understand this choice.

@no-response no-response bot removed the state:needs-answer Needs additional information from the issue creator label Jun 3, 2021
@Neraste
Copy link
Author

Neraste commented Jun 3, 2021

I know this is a very edgy case, as you don't create tons of superusers from the command line.

On my side, I considered a superuser without a valid email would be acceptable in my authentication backend, so I was surprised that my superuser couldn't change their email. I imagined that they could pass trough the email check, as implemented in the PR. Eventually, I solved the problem in my project by forbidding users with un-validated email address to change their email address on the front (only such superusers should fall in this case).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants