Provide email address verification feature by marienfressinaud · Pull Request #2481 · FreshRSS/FreshRSS

marienfressinaud · 2019-08-06T12:52:05Z

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.

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

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

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

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

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

Frenzie

Offer to resend as well?

Edit: d'oh, next commit.

marienfressinaud · 2019-08-06T15:28:07Z

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.

Alkarex · 2019-08-15T15:27:18Z

app/Controllers/userController.php

 			return false;
 		}

+		if ($email !== null && $userConfig->mail_login !== $email) {


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

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.

Alkarex · 2019-08-15T15:30:02Z

app/Controllers/userController.php

 		}

+		if ($email !== null && $userConfig->mail_login !== $email) {
+			$userConfig->mail_login = $email;


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

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.

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])?)*$/

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

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

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

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

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 👍

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)

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 · 2019-08-15T15:49:05Z

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 · 2019-08-15T17:30:15Z

@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 · 2019-08-15T18:00:11Z

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 · 2019-08-16T07:01:33Z

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 · 2019-08-16T10:46:53Z

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 · 2019-08-28T14:48:51Z

I think it's all good now!

app/Controllers/userController.php

Alkarex · 2019-08-28T18:50:31Z

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 · 2019-08-28T19:02:04Z

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

Alkarex · 2019-08-28T19:25:57Z

I suggest adding:

PHPMailer::$validator = 'pcre8';

Just after
https://github.com/flusio/FreshRSS/blob/2cde74db503adf581ece09a17924da378edcb660/lib/Minz/Mailer.php#L77

Alkarex · 2019-08-28T19:28:52Z

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

Co-Authored-By: Alexandre Alapetite <[email protected]>

marienfressinaud · 2019-08-29T09:04:22Z

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

Purexo · 2019-11-22T18:32:07Z

Uh,

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

after update (complete I hope) redirect to https://<my instance>/i/?c=error
on network navigator debugger many https://<my instance>/i/?c=user&a=validateEmail
I try to log with my email instead username, classic error login page
I connect to my VPS, configs seems OK
Little search on github issue / pr I fall here
Inspect some code and found this line : https://github.com/flusio/FreshRSS/blob/a3031d21bf7ffbeae10683ad333dc5c355fa09bb/app/Controllers/userController.php#L128
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)
So i add this key with '' value
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 · 2019-11-22T21:41:47Z

@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 · 2019-11-23T11:27:40Z

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 · 2019-11-23T11:30:28Z

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 · 2019-11-23T11:39:08Z

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):

FreshRSS/lib/Minz/Configuration.php

Lines 139 to 163 in 0320877

    
           	/** 
        
           	 * 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 · 2019-11-23T17:51:42Z

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 · 2019-11-23T17:59:48Z

@Purexo Yes please, /dev branch 👍

Frenzie reviewed Aug 6, 2019

View reviewed changes

marienfressinaud force-pushed the add/add-email-to-users branch 4 times, most recently from fbf4975 to 7fe6119 Compare August 6, 2019 15:25

marienfressinaud force-pushed the add/add-email-to-users branch from 7fe6119 to b548a77 Compare August 7, 2019 09:04

jkinable mentioned this pull request Aug 14, 2019

Adding email verification support #2469

Closed

10 tasks

Alkarex added Work in progress 🚧 Wait before merging and removed Work in progress 🚧 Wait before merging labels Aug 15, 2019

Alkarex changed the title ~~[wip] add: Provide email address verification feature~~ Provide email address verification feature Aug 15, 2019

Alkarex added this to the 1.15.0 milestone Aug 15, 2019

Alkarex reviewed Aug 15, 2019

View reviewed changes

marienfressinaud added 9 commits August 15, 2019 17:30

Add an email field to the profile page

b289f09

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.

Add boolean to the conf to force email validation

73de0d2

This commit only adds a configuration item.

Add email during registration if email must be validated

80483a7

Set email token to validate when email changes

66fe611

Block access to FreshRSS if email is not validated

fd26629

Send email when address is changed

1df3f6e

Allow to resend the validation email

1a77a64

Allow the user to change its email while blocked

48e073b

Document the email validation feature

d037d9a

marienfressinaud force-pushed the add/add-email-to-users branch from ded76ef to d037d9a Compare August 15, 2019 15:32

fixup! Allow the user to change its email while blocked

4682c2b

marienfressinaud added 5 commits August 28, 2019 16:22

Complete all the locales with default english

bc80eca

Hide sidebar (profile page) if email is not validated

326c26f

Don't check email format if value is empty

986d04f

Check email requirements on registration

2aa28f5

Allow admin to specify email when creating users

2cde74d

Alkarex reviewed Aug 28, 2019

View reviewed changes

app/Controllers/userController.php Outdated Show resolved Hide resolved

marienfressinaud and others added 3 commits August 29, 2019 10:20

Remove trailing comma in userController

0e34cd2

Co-Authored-By: Alexandre Alapetite <[email protected]>

Set PHPMailer validator to html5 before sending email

4a6b37a

fixup! Remove trailing comma in userController

a3031d2

Alkarex approved these changes Aug 29, 2019

View reviewed changes

Alkarex merged commit 75632e7 into FreshRSS:dev Aug 29, 2019

Alkarex removed the Work in progress 🚧 Wait before merging label Aug 29, 2019

marienfressinaud deleted the add/add-email-to-users branch August 29, 2019 10:31

marienfressinaud mentioned this pull request Aug 29, 2019

fix: Fix "validate email"-related issues #2512

Merged

Frenzie mentioned this pull request Sep 5, 2019

Option to show horizontal line separator in footer koreader/koreader#5309

Merged

Alkarex mentioned this pull request Oct 26, 2019

FreshRSS 1.15 #2599

Merged

Purexo mentioned this pull request Nov 25, 2019

FIX email token check if not in user-config #2686

Merged

Uh oh!

Conversation

marienfressinaud commented Aug 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Nice to have

Basic workflow

Uh oh!

Frenzie left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marienfressinaud commented Aug 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Alkarex Aug 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Alkarex Aug 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Frenzie Aug 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marienfressinaud commented Aug 15, 2019

Uh oh!

Alkarex commented Aug 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Frenzie commented Aug 15, 2019

Uh oh!

marienfressinaud commented Aug 16, 2019

Uh oh!

Alkarex commented Aug 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marienfressinaud commented Aug 28, 2019

Uh oh!

Uh oh!

Alkarex commented Aug 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Alkarex commented Aug 28, 2019

Uh oh!

Alkarex commented Aug 28, 2019

Uh oh!

Alkarex commented Aug 28, 2019

Uh oh!

marienfressinaud commented Aug 29, 2019

Uh oh!

Purexo commented Nov 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Alkarex commented Nov 22, 2019

Uh oh!

marienfressinaud commented Nov 23, 2019

Uh oh!

marienfressinaud commented Nov 23, 2019

Uh oh!

Alkarex commented Nov 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

marienfressinaud commented Aug 6, 2019 •

edited

Loading

Frenzie left a comment •

edited

Loading

marienfressinaud commented Aug 6, 2019 •

edited

Loading

Alkarex Aug 15, 2019 •

edited

Loading

Alkarex Aug 17, 2019 •

edited

Loading

Frenzie Aug 21, 2019 •

edited

Loading

Alkarex commented Aug 15, 2019 •

edited

Loading

Alkarex commented Aug 16, 2019 •

edited

Loading

Alkarex commented Aug 28, 2019 •

edited

Loading

Purexo commented Nov 22, 2019 •

edited

Loading

Alkarex commented Nov 23, 2019 •

edited

Loading

Purexo commented Nov 23, 2019 •

edited

Loading