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

Change how authentication with unverified email addresses is implemented #128

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

armellarcier
Copy link

No description provided.

ocram and others added 2 commits October 12, 2018 16:19
…opers should implement warnings about verification themselves then.

Last parameter of constructor : `new Auth($databaseConnection, $ipAddress, $dbTablePrefix, $throttling, $sessionResyncInterval, $dbSchema, $loginUnverified)`
@ocram
Copy link
Contributor

ocram commented Oct 14, 2018

Thanks!

Glad that this works for you, but I’m not sure if we really want it in this repository here.

There’s also a way to disable email verification and thus let users sign in immediately, as explained in the README:

If you don’t want to perform email verification, just omit the last parameter to Auth#register. The new user will be active immediately, then.

So I guess we don’t need another way to make this work. Granted, with your solution, you decide on login, and with our current solution, you decide on registration. But I think that’s sufficient in most cases and often what you actually want.

That’s also why the README says:

All contributions are welcome! If you wish to contribute, please create an issue first so that your feature, problem or question can be discussed.

Thanks for your contribution, nevertheless.

@ocram ocram closed this Oct 14, 2018
@armellarcier
Copy link
Author

I just don’t think the user should be marked as verified in the database if they’re not.
Plus We should be able to authorize login without verification while restricting some features to verified users only. I don’t really get your point. All PRs are closed without being merged in this repo. How comes ?

@ocram
Copy link
Contributor

ocram commented Oct 15, 2018

I just don’t think the user should be marked as verified in the database if they’re not.

That is certainly a good point. Thank you! You could later reverse your decision and need information on the authenticity of the email address. If it’s immediately marked as verified although it’s actually not, that’s a problem. These “allegedly confirmed” email addresses could even be mixed with addresses that have in fact been confirmed.

We could solve that problem simply by introducing a third state, i.e. in addition to 0 and 1, we could introduce -1 or 2. That would be the change requiring the fewest code changes. Though your implementation probably makes more sense and is easier to read. So perhaps we should reconsider it.

We should be able to authorize login without verification while restricting some features to verified users only

Well, it’s not only about verifying the client as a human. Even more important is verifying the email address as not belonging to some stranger. So if you use email for identification and authentication, it’s really important to know that it’s actually owned by the user.

It’s not as easy as just merging your pull request, in any case. So that’s one reason why pull requests are not always merged. If you can sign in with an unverified email address, this needs to be taken into account in other areas as well.

Some pull requests are also just bad – that’s another reason. Some are too extensive and complex, and change the whole library with hundreds or even thousands of lines in a very opinionated way. That’s something that should go into a fork, in most cases.

Then, as we have explained in the README, and as I have quoted for you again, we explicitly state that we very much like to see and discuss contributions, but please let’s talk about these ideas and possible implementations first. Sometimes, the contributor just got something wrong and didn’t have a perfect understanding of the existing code yet, which is absolutely comprehensible.

Finally, we do merge some pull requests, just not with GitHub’s tools but instead by applying Git’s raw patches. That’s why they’re not marked as merged on GitHub.

@ocram ocram reopened this Oct 15, 2018
@ocram ocram changed the title Feat/optional verification Change how authentication with unverified email addresses is implemented Oct 15, 2018
@armellarcier
Copy link
Author

armellarcier commented Oct 15, 2018

Thanks for your kind and thorough reply! I’ll be happy to discuss this more extensively later on this week.
Just to clarify, stating the obvious here, the email address verification is needed to make sure that :

  • the address does exist
  • the owner of the address agrees that an account has been created with it

So next question is, in what use case will I need to allow the unverified user to login but also need email verification at one point?
My use case is e-mailing. I should not be sending commercial e-mails to unverified addresses. I need an explicit consent from the owner.
For that use case, an extra state sounds ok. -1 doesn’t seem so bad. But I still have a hard time accepting the idea of saving this new « login without verification » clearance for every user in the very column we’re using to save verification state instead of setting this as a configuration ( that could change with an update of the app without the need of updating the database, changing -1 into 0 ).

@ocram
Copy link
Contributor

ocram commented Oct 15, 2018

Totally agree with all of what you said here!

And as I said before, your solution seems to be the one that is conceptually clearer.

If you allow logins without a verified email address, though, you have to guard every usage of the email address against the possibility of that email address not being verified.

Further:

  • Can users change their email address if the old address still hasn’t been verified?
  • Can users request to reset their password if their email address hasn’t been verified yet?
  • Can administrators sign in as users with an unverified email address? Maybe if and only if users themselves can sign in to their account when the address is still unverified?

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

Successfully merging this pull request may close these issues.

2 participants