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

Changing email at social signup keeps original verified one in place #1361

Closed
softzer0 opened this issue Apr 29, 2016 · 11 comments
Closed

Changing email at social signup keeps original verified one in place #1361

softzer0 opened this issue Apr 29, 2016 · 11 comments

Comments

@softzer0
Copy link

softzer0 commented Apr 29, 2016

I have a concern regarding email verification, because if user tries to signup via some social account, in the finishing signup form a field for email address is enabled and it can be altered to the incorrect/invalidated one. The problem is that it would pass and successfully register without any further verification. And I still can't manage to create a custom (social account signup) form which will disable email field and also avoid POST hacks in order to prevent this from happening.

@softzer0 softzer0 changed the title [Security issue] Email can be changed when signing up via social account [Security issue] Email can be invalidated when signing up via social account Apr 29, 2016
@softzer0 softzer0 changed the title [Security issue] Email can be invalidated when signing up via social account [Security issue] Email can be changed when signing up via social account Apr 29, 2016
@pennersr
Copy link
Owner

"successfully register without any further verification" -- this is not true, email address validation (if enabled) will kick in.

Let's take the security part of this discussion offline -- if you really do think there is an issue, please provide me with an exact scenario to reproduce using just vanilla allauth: raymond.penners@intenct.nl

@softzer0
Copy link
Author

I've forgot to mention that in settings.py I have this: ACCOUNT_EMAIL_VERIFICATION = "mandatory"
However, I'm able to register with invalid email address via social signup after changing the valid one I got from my social account.

@pennersr
Copy link
Owner

As for the changing of the email, please provide me some insights with the issue you are trying to tackle. A few thoughts:

  • Not all social providers provide an email address (e.g. Twitter does not), so providing an email address during signup is essential.
  • Assuming no security issue exists, could you elaborate what issue you see with people being able to alter their email? Even if you were to prevent inputting the email address during signup, people can eaily alter it just after signup, with the same result: a different email address is attached to the account compared to the one registered with their social provider.

@pennersr
Copy link
Owner

I guess the issue you are seeing is that after signup two email addresses are attached to the account. The original one, marked as verified because it was handed over like that by the social provider, and another one, the one you inputted manually. But, the latter one is not marked as verified. The result would be the same if you signed up without being able to alter the email address and add it manually later on. So that is not a security issue. Can you confirm this is what you are seeing?

@softzer0
Copy link
Author

Email verification is required when the address is entered manually, because I set that setting to "mandatory" so it doesn't matter if user puts it on signup or changes it afterwards - it must require verification in both cases if it's done by the user. This works well on standard signup but on social signup there's no check if the verified email address (if there's one) is altered, and if it is to proceed with the verification process.
I will report you later what I've further investigated.

@pennersr
Copy link
Owner

In this case it is concluded that the user did provide a verified email address during signup, namely the original one from the social provider. The one manually inputted is clearly marked as not verified. This is why email verification allows this user to pass: during signup the user handed over a verified email address (in this case, even two).

I can imagine this can be confusing, and this behavior is debatable. But, can we first come to the conclusion that there is no security issue here? You configured allauth in such a way that the users need to hand over a verified email address to be allowed in, which did happen.

@pennersr
Copy link
Owner

I just added explicit test scenario's for this. See: c2545be

@softzer0
Copy link
Author

softzer0 commented Apr 29, 2016

Thanks for this, I will run your tests. What happens if I want only one email address to be set for each user? Is the default behavior that each user can have as many emails as he wants? What I want to achieve in my case is that user must have no more than one.

@pennersr
Copy link
Owner

Out of the box there is no setting that would limit users to just having one email address. I am wondering why you would want that anyway. At the very least, users should be able to have (temporarily) two email addresses so that they can change the address themselves: 1) the current primary/verified one, and 2) a new one that they just added that needs to be verified.

If you insist, you should be able to build this on top of allauth by hooking into the various signals, but as I do not see any good use case for this I don't want to support such a feature out of the box.

Now, coming back to the original issue, I do agree that the current behavior is rather unexpected. If you change your email address during signup, you would not expect the original email to be still used as the primary/verified one. I will keep this ticket open to change this behavior. Though I want to stress that there is currently no known security issue on this point ...

@pennersr pennersr changed the title [Security issue] Email can be changed when signing up via social account Changing email at social signup keeps original verified one in place Apr 30, 2016
@pennersr pennersr added TODO and removed Unconfirmed labels Apr 30, 2016
@derek-adair
Copy link

Worth closing. Another corner case request that is not worth supporting directly.

@pennersr pennersr removed the TODO label Aug 17, 2023
@pennersr
Copy link
Owner

Agreed.

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

No branches or pull requests

3 participants