I think this functionality was accidentally disabled in
65b044d7a8 . When a user changes their
email, an email is sent to the new email to ensure the user actually
owns that new email and isn't sending emails to people that aren't them. A secret token is
sent in that email and the user is only allowed to change their email if
they can prove they know the secret that was only sent to that email.
This scheme breaks down if the secret is shared with multiple emails.
Not that this is likely, but a user *could* change their email once to
an email they control, and get a secret to confirm that email. If that
user then changed their email again to a target email they want to spam,
and they knew that the same confirmation secret was used to confirm that
second new email, they could craft and visit a link that confirmed it
without ever having recieved the second confirmation email. That would
allow them to spam the target email if the application sends lots of
emails.
Long story short, the app used to this, and while its a bit farfetched
it may as well continue to do this.
This commit uncomments the line and adds a test.
With NO_EMAILS set (say in a dev environment), some pieces of the User update mutation are skipped. Running the tests for Users::Update on a fresh clone of master with NO_EMAILS set had a failing test:
```
Failures:
1) Users::Update changes email addresses
Failure/Error: expect(u.unconfirmed_email?).to be true
expected true
got false
# ./spec/mutations/users/update_spec.rb:31:in `block (2 levels) in <top (required)>'
# /Users/airhorns/.rbenv/versions/2.5.1/bin/rspec:23:in `load'
# /Users/airhorns/.rbenv/versions/2.5.1/bin/rspec:23:in `<top (required)>'
# /Users/airhorns/.rbenv/versions/2.5.1/bin/bundle:23:in `load'
# /Users/airhorns/.rbenv/versions/2.5.1/bin/bundle:23:in `<main>'
```
This test failed because it was expecting against the `unconfirmed_email` attribute, which in the NO_EMAILS case, isn't used at all. So, the test now checks if email verification is on, and expects against `unconfirmed_email` if so, and the actual `email` attribute otherwise.
Turns out, that test failed too! The actual code in `Users::Update` figured out that direct email mutations should be allowed when NO_EMAILS is set, but didn't pass that down to the `#update_attributes` call. I think this was just a small mistake because the code builds a list of attribute exclusions conditional on NO_EMAILS already, and just never uses it. This code was added in 65b044d7a8 which looks like it might not have been intended to hit master. No matter, easy fix!
With NO_EMAILS set (say in a dev environment), some pieces of the User update mutation are skipped. Running the tests for Users::Update on a fresh clone of master with NO_EMAILS set had a failing test:
```
Failures:
1) Users::Update changes email addresses
Failure/Error: expect(u.unconfirmed_email?).to be true
expected true
got false
# ./spec/mutations/users/update_spec.rb:31:in `block (2 levels) in <top (required)>'
# /Users/airhorns/.rbenv/versions/2.5.1/bin/rspec:23:in `load'
# /Users/airhorns/.rbenv/versions/2.5.1/bin/rspec:23:in `<top (required)>'
# /Users/airhorns/.rbenv/versions/2.5.1/bin/bundle:23:in `load'
# /Users/airhorns/.rbenv/versions/2.5.1/bin/bundle:23:in `<main>'
```
This test failed because it was expecting against the `unconfirmed_email` attribute, which in the NO_EMAILS case, isn't used at all. So, the test now checks if email verification is on, and expects against `unconfirmed_email` if so, and the actual `email` attribute otherwise.
Turns out, that test failed too! The actual code in `Users::Update` figured out that direct email mutations should be allowed when NO_EMAILS is set, but didn't pass that down to the `#update_attributes` call. I think this was just a small mistake because the code builds a list of attribute exclusions conditional on NO_EMAILS already, and just never uses it. This code was added in 65b044d7a8 which looks like it might not have been intended to hit master. No matter, easy fix!
Rick Carlino