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

Provide email address verification feature #2481

Merged
merged 24 commits into from Aug 29, 2019

Conversation

@marienfressinaud
Copy link
Member

marienfressinaud commented Aug 6, 2019

This PR adds the support for email address verification.

It is based on #2476 (so this one needs to be merged first) and better reviewed commit by commit.

Closes #2469

Nice to have

These are improvements that are not "necessary" but could be nice to have. I added the reason why I didn't implement these in parenthesis:

  • give easy access to validation url in development mode, when SMTP isn't configured (useful?)
  • rename mail_login into email so it's more intuitive (need to migrate current configuration)
  • add force_email_validation to the CLI (useful?)
  • change profile layout when email is not validated (more work than expected)

Depending on your feedback, I can implement them in this PR, in other PRs, or not at all ;)

Basic workflow

An email field is added to the profile page.

If the force_email_validation boolean is set to true:

  • the field also appears during registration

Capture d’écran de 2019-08-06 14-33-52

  • an email to validate the user is sent when the email address changes (including registration)

Capture d’écran de 2019-08-06 14-35-21

  • FreshRSS is blocked on a page which asks the user to validate its email address

Capture d’écran - 2019-08-06 à 14 34 39

  • the user can ask to resend the email or access the "profile" page (the other pages are blocked)

Capture d’écran - 2019-08-06 à 14 34 53

  • once the user clicks on the link, its account is enabled

Capture d’écran - 2019-08-06 à 14 35 44

Copy link
Member

Frenzie left a comment

Offer to resend as well?

Edit: d'oh, next commit.

@marienfressinaud marienfressinaud force-pushed the flusio:add/add-email-to-users branch 4 times, most recently from fbf4975 to 7fe6119 Aug 6, 2019
@marienfressinaud

This comment has been minimized.

Copy link
Member Author

marienfressinaud commented Aug 6, 2019

I made a bunch of improvements and added a bit of documentation. I think it's now ready to be reviewed, but it's still blocking by #2476 (from which the two first commits come). Also added the "nice to have" improvements that I didn't implement in the first message.

@marienfressinaud marienfressinaud force-pushed the flusio:add/add-email-to-users branch from 7fe6119 to b548a77 Aug 7, 2019
@jkinable jkinable mentioned this pull request Aug 14, 2019
10 of 10 tasks complete
@Alkarex Alkarex changed the title [wip] add: Provide email address verification feature Provide email address verification feature Aug 15, 2019
@Alkarex Alkarex added this to the 1.15.0 milestone Aug 15, 2019
$userConfig = get_user_configuration($user);
if ($userConfig === null) {
return false;
}
if ($email !== null && $userConfig->mail_login !== $email) {

This comment has been minimized.

Copy link
@Alkarex

Alkarex Aug 15, 2019

Member

$email != '' would be safer here, especially since it might come from Minz_Request::param('email', '');

This comment has been minimized.

Copy link
@marienfressinaud

marienfressinaud Aug 15, 2019

Author Member

That was on purpose since the email should not be required when force_email_validation is false. But I realize that I should enforce the value if it's true.

$userConfig = get_user_configuration($user);
if ($userConfig === null) {
return false;
}
if ($email !== null && $userConfig->mail_login !== $email) {
$userConfig->mail_login = $email;

This comment has been minimized.

Copy link
@Alkarex

Alkarex Aug 15, 2019

Member

Somewhere we need to sanitize this user input. Either here or in profileAction().
I suggest looking at https://php.net/filter.filters.validate FILTER_VALIDATE_EMAIL / FILTER_SANITIZE_EMAIL

This comment has been minimized.

Copy link
@marienfressinaud

marienfressinaud Aug 15, 2019

Author Member

I would simply check that the email contains a @. Maybe we can strip the string too. But emails are hard to validate and it's almost always badly done (more here). And I'm not comfortable with that:

In general, this validates e-mail addresses against the syntax in RFC 822, with the exceptions that comments and whitespace folding and dotless domain names are not supported.

I don't understand why they would respect the RFC, but in fact not…

The feature itself is about validating that the address is valid by sending an email so I don't think we have to be too strict.

This comment has been minimized.

Copy link
@Frenzie

Frenzie Aug 15, 2019

Member

That's definitely very odd about dotless domains. The whitespace & comments things is more common. For example, the W3C recommends this for input type=email:

This requirement is a willful violation of RFC 5322, which defines a syntax for e-mail addresses that is simultaneously too strict (before the "@" character), too vague (after the "@" character), and too lax (allowing comments, whitespace characters, and quoted strings in manners unfamiliar to most users) to be of practical use here.

/^[a-zA-Z0-9.!#$%&'*+\/=?^_`{|}~-]+@[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(?:\.[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*$/

This comment has been minimized.

Copy link
@Alkarex

Alkarex Aug 15, 2019

Member

There are still some classes of characters, which should be discarded, and potentially also a min and max length. I think it is dangerous to pass some user input to the backend without any form of sanitization. If you do not like the PHP built-in validation (which I personally think would be reasonable - I an not even sure the rest of the back-end is able to handle what the PHP validation rejects anyway), then at least the sanitize filter FILTER_SANITIZE_EMAIL https://php.net/manual/filter.filters.sanitize

This comment has been minimized.

Copy link
@marienfressinaud

marienfressinaud Aug 16, 2019

Author Member

Ok, I'll take a look to clean at least a bit this param!

This comment has been minimized.

Copy link
@Alkarex

Alkarex Aug 17, 2019

Member

Ah, I did not realise that the HTML5 version does not allow UTF-8 before the @ 😢

This comment has been minimized.

Copy link
@Alkarex

Alkarex Aug 17, 2019

Member

So, it seems that the conclusion is to use the PHPMailer's pcre8 option as @Frenzie suggested higher up

This comment has been minimized.

Copy link
@marienfressinaud

marienfressinaud Aug 20, 2019

Author Member

Thanks for digging into the problem! (I was finishing my holidays :)) I didn't thought that PHPMailer could have a verification function (sounds obvious now!) I'll try to finish this tomorrow 👍

This comment has been minimized.

Copy link
@marienfressinaud

marienfressinaud Aug 21, 2019

Author Member

It looks like none of the patterns allow internationalized email addresses (not even pcre8). I'm also very surprised that Firefox or Thunderbird don't allow UTF-8 before the @ either… Is there any server or client that fully support UTF-8 addresses?!

I found that PHPMailer provides a punyencodeAddress function which convert only the domain, that's a start. So I'll convert the address to punycode first and validate the address against this value. Then I can either store the value as punycode (but I'll have to transform it again when displaying the address) or store the utf-8 version (easier, and it should be ok since PHPMailer transforms all the addresses before sending the email)

This comment has been minimized.

Copy link
@Frenzie

Frenzie Aug 21, 2019

Member

Is there any server or client that fully support UTF-8 addresses?!

Sure, Microsoft has announced support, this one does http://www.postfix.org/SMTPUTF8_README.html

There's an incomplete list on Wikipedia:
https://en.wikipedia.org/wiki/Extended_SMTP#SMTPUTF8

Edit: to be clear, that list is from like 4 years ago and even then I doubt it was complete.

I reuse the `mail_login` from the configuration. I'm not sure if it's
useful today (I would say it was used when Persona login was available).

A good improvement would be to rename `mail_login` into `email` so it
would be more intuitive to use.
This commit only adds a configuration item.
@marienfressinaud marienfressinaud force-pushed the flusio:add/add-email-to-users branch from ded76ef to d037d9a Aug 15, 2019
@marienfressinaud

This comment has been minimized.

Copy link
Member Author

marienfressinaud commented Aug 15, 2019

Just to let you know @Alkarex, I saw you merged dev into my branch but I prefer to rebase my branches to keep the history clean (easier to read, easier to find a bug in a specific commit)

@Alkarex

This comment has been minimized.

Copy link
Member

Alkarex commented Aug 15, 2019

@marienfressinaud I just used GitHub's built-in tool to resolve the conflict :-)
In general, I think forced pushes and other forms of history rewriting should be kept to rare occasions to fix errors. They make collaboration quite difficult. For instance, if I merge this branch in my own Alkarex/dev branch to test it (where I also simultaneously test other PRs), and your branch history is rewritten, then I cannot pull your new changes anymore and there is a big work to do to get a working branch again (potentially having to reset my test branch and re-merge all the ones I want to test at the same time). Even when testing a single branch, a rewritten history is annoying as it breaks simple pulls to get newer updates.
It also breaks reviews, make some commits disappear (e.g. my conflict resolution is gone), make some comments on old commits disappear, breaks the ability to see the changes since the last review, etc.
Finally, I do not think the clean history is an issue at all: all git clients that I know distinguish commits made in the active branch from commits made in other branches (e.g. merged), and we can then squash & merge when the PR is ready anyway :-)

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Aug 15, 2019

Btw, perhaps you already know but I recently discovered that on GitHub you can do this:

# assuming your own fork is origin and the one here is upstream
git fetch upstream pull/2481/head
git checkout FETCH_HEAD
@marienfressinaud

This comment has been minimized.

Copy link
Member Author

marienfressinaud commented Aug 16, 2019

That's interesting and I strongly agree with some of your arguments. In the mean time, I consider my branches as my personal workspaces and nobody should base their work on it nor modify them. In this context, force-push should not be a problem. I would not recommend to test different work-in-progress branches at the same time neither, but I can understand that can be useful.

I'm not a huge fan of squash because you lose how the feature has been constructed. I always try to build my PRs logically and they are reviewable commit by commit. I personally prefer a merge with no fast-forward to keep the history. It can also help a lot to understand why and how a bug got introduced.

In the mean time, GitHub is definitely not adapted to this workflow: as you said, comments are lost, it's difficult to see the last changes, commits are not ordered correctly in the "Commits" tab, etc.

Considering that, I'll try an "in-between": I'll commit my fixes with commit --fixup=<commit to fix> and don't rebase until the PR is approved. I'll have to refrain myself because I worked this way for the three last years ^^ I found it powerful and it helped me to understand better how Git works and how to fix an error… but it's definitely advanced usage of Git and collaboration is more important than some technical tricks :)

@Alkarex

This comment has been minimized.

Copy link
Member

Alkarex commented Aug 16, 2019

It is not a big deal in any case :-) A bit more feedbacks:
Regarding the squash & merge, the history is kept in the PR itself, together with the discussions, reviews, etc.
Rewriting history makes a branch more difficult to test (pull is not working anymore), even when you are the only one making changes.
Rewriting history also makes the exact construction progress disappear (e.g. missing commits, out of context commits).
By the way, when you submit the PR, you can decide whether you allow changes to your branch by others or not.

@marienfressinaud

This comment has been minimized.

Copy link
Member Author

marienfressinaud commented Aug 28, 2019

I think it's all good now!

@Alkarex

This comment has been minimized.

Copy link
Member

Alkarex commented Aug 28, 2019

And still have the following error, preventing any e-mail from being sent:

[error] --- Minz_Mailer cannot send a message: Invalid address:  (From): root@localhost
AH01071: Got error 'PHP message: Invalid address:  (From): root@localhost\n'

P.S.: No difference between root and noreply. But changing for postmaster@[.....].fr worked

@Alkarex

This comment has been minimized.

Copy link
Member

Alkarex commented Aug 28, 2019

It is because PHPMailer calls PHP's FILTER_VALIDATE_EMAIL which does not accept @dotless

@Alkarex

This comment has been minimized.

Copy link
Member

Alkarex commented Aug 28, 2019

@Alkarex

This comment has been minimized.

Copy link
Member

Alkarex commented Aug 28, 2019

With the syntax fix, and the validator fix, it should be good for me.

marienfressinaud and others added 3 commits Aug 29, 2019
Co-Authored-By: Alexandre Alapetite <alexandre@alapetite.fr>
@marienfressinaud

This comment has been minimized.

Copy link
Member Author

marienfressinaud commented Aug 29, 2019

I used html5 validator, like in the rest of the code. It should work now!

@Alkarex Alkarex merged commit 75632e7 into FreshRSS:dev Aug 29, 2019
4 checks passed
4 checks passed
ci/dockercloud (/Docker/Dockerfile) Your tests passed in Docker Cloud
Details
ci/dockercloud (/Docker/Dockerfile-Alpine) Your tests passed in Docker Cloud
Details
ci/dockercloud (/Docker/Dockerfile-QEMU-ARM) Your tests passed in Docker Cloud
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@marienfressinaud marienfressinaud deleted the flusio:add/add-email-to-users branch Aug 29, 2019
@Alkarex Alkarex mentioned this pull request Oct 26, 2019
@Purexo

This comment has been minimized.

Copy link
Contributor

Purexo commented Nov 22, 2019

Uh,

I see on fediverse that FreshRSS has been updated, so i the update from web admin interface has alway !

  1. after update (complete I hope) redirect to https://<my instance>/i/?c=error
  2. on network navigator debugger many https://<my instance>/i/?c=user&a=validateEmail
  3. I try to log with my email instead username, classic error login page
  4. I connect to my VPS, configs seems OK
  5. Little search on github issue / pr I fall here
  6. Inspect some code and found this line : https://github.com/flusio/FreshRSS/blob/a3031d21bf7ffbeae10683ad333dc5c355fa09bb/app/Controllers/userController.php#L128
  7. check my account config file : no key email_validation_token in this file (not found in example user config file too but i'm on a terminal maybe i don't see it)
  8. So i add this key with '' value
  9. Yeah i'm able to connect

I recommend you to use php empty function instead strongly typed equality :

$email_not_verified = !empty(FreshRSS_Context::$user_conf->email_validation_token);

Check https://www.php.net/manual/en/function.empty.php
maybe if you support very old php version you should store FreshRSS_Context::$user_conf->email_validation_token in a variable and use this var in empty

More safe in context of checking config that can be from very old version of app my instance running without update issue from many years (4-5years I think)

At this time, I'm happy to be a Developper or I will not hase solve my issue without help ^^.

Best regards

@Alkarex

This comment has been minimized.

Copy link
Member

Alkarex commented Nov 22, 2019

@Purexo Yes, this line is indeed bad, especially the strict inequality. Could you please send a pull request to fix it? I have not checked much the e-mail flow ( ping @marienfressinaud ), but couldn't you log with your normal username? Please open another issue if that was not the case.
Even when the parameters are not in your own account config file, default parameters are supposed to be loaded from https://github.com/FreshRSS/FreshRSS/blob/dev/config-user.default.php so I am not entirely sure of what the exact problem was.

@marienfressinaud

This comment has been minimized.

Copy link
Member Author

marienfressinaud commented Nov 23, 2019

I confirm that the email_validation_token should be taken from the default user config if not set in your own config file. I also checked that config-user.default.php is updated during a ZIP update (it should but maybe it failed?) Could you post the content of this file to check if it's actually up-to-date?

@marienfressinaud

This comment has been minimized.

Copy link
Member Author

marienfressinaud commented Nov 23, 2019

Side note about empty function: it's probably what should be used here in PHP, but I'm really annoyed with this kind of solution since it basically says: I don't really know what's the value of this variable. Here, it would even have hidden a real bug.

@Alkarex

This comment has been minimized.

Copy link
Member

Alkarex commented Nov 23, 2019

When some data comes from somewhere that is not fully controlled, the code should allow for some typical variations. In this case, I do not think it makes sense to strictly test for an empty string and fail for the other nullable values. In this case, I would argue for != ''. Not empty() though, because the value comes back from a wrapper (which might return null):

/**
* Return the value of the given param.
*
* @param $key the name of the param.
* @param $default default value to return if key does not exist.
* @return the value corresponding to the key.
* @throws Minz_ConfigurationParamException if the param does not exist
*/
public function param($key, $default = null) {
if (isset($this->data[$key])) {
return $this->data[$key];
} elseif (!is_null($default)) {
return $default;
} else {
Minz_Log::warning($key . ' does not exist in configuration');
return null;
}
}
/**
* A wrapper for param().
*/
public function __get($key) {
return $this->param($key);
}

(Which I think we should remove at some point in favour of native code, but that is another story :-))

@Purexo

This comment has been minimized.

Copy link
Contributor

Purexo commented Nov 23, 2019

but couldn't you log with your normal username? Please open another issue if that was not the case.

I was not able to connect with my username nor email.
Since I've add email_validation_token in my user config file, I am able to connect with my username. Not with my email but I don't care.
I think when tried to login with username before modify the config, the form works well, and I was connected but the system was checking validateEmail rule, the rules did not passed so redirect (badly infinite, Firefox was detect the redirect loop and abort it.)
Should I create an issue ?

Even when the parameters are not in your own account config file, default parameters are supposed to be loaded from https://github.com/FreshRSS/FreshRSS/blob/dev/config-user.default.php

I've looked into the config-user.default.php file (on my server): no email_validation_token key in.

Yes I've update with zip (available in web-admin interface), it work well from many years and really convenient.

I can make a fast PR from github if you wan't. Should I checkout from dev branch ?

@Alkarex

This comment has been minimized.

Copy link
Member

Alkarex commented Nov 23, 2019

@Purexo Yes please, /dev branch 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.