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

improve email verification, no popup, auto-verify Shib users #8579

Merged
merged 8 commits into from
Apr 22, 2022

Conversation

pdurbin
Copy link
Member

@pdurbin pdurbin commented Apr 5, 2022

What this PR does / why we need it:

Users are confused the "Verify Email" button. Sometimes it shows a popup. Sometimes it shows a green message at the top. It's unclear when an email is sent.

In this pull request, we eliminate the popup and always send a fresh email when you click the button, which has been renamed to "Send Email Verification".

In addition, issues related to email verification of Shibboleth users were fixed. The guides had long suggested that the emailconfirmed timestamp is being set for Shibboleth users on login. It turns out this was only true for new Shibboleth users on their first login. If you converted your account from builtin to Shibboleth, the timestamp was not being set. Now we update the timestamp on every Shib login.

I'm a Shib user of Harvard Dataverse and I saw a "Verify Email" button. Clicking it resulted in an email to me. We don't want Shib users to see that button or get these emails so I added a check to prevent the email from being sent.

In terms of why I was seeing the "Verify Email" button as Shib user, I think the hasNoStaleVerificationTokens check added in PR #6974 has been causing problems. I eliminated this check which should mean that the presence of the emailconfirmed timestamp is enough to know if an email has been verified or not. This was the original design in PR #3299. Instead of a boolean, we store a timestamp so we have more information. Any tokens in the database shouldn't matter.

While testing, I noticed a weird empty "Account Information" dropdown for Shib users (#8223). I went ahead and removed this as well.

I noticed that when testing with two browsers, the browser where I did not click the verification link does not show "verified" until I log out and back in.

Which issue(s) this PR closes:

Special notes for your reviewer:

None.

Suggestions on how to test this:

Popup-related changes (#8227)

  • make sure the popup is gone
  • a fresh email should be sent each time the "Send Verification Email" button is clicked
  • a "success" message should display on the page each time a fresh email is sent.
  • when a fresh email is sent, any old links should no longer work.

two browsers (login/logout behavior)

  • If you are logged in with one browser and click the verification email link in another browser, the first browser should reflect that your email is now verified without you having to log out and back in.

Shib-related changes (#5663)

  • emailconfirmed (authenticateduser table) should be updated on each Shib login. You can inspect this via API ( http://localhost:8080/api/admin/authenticatedUsers/jharvard ).
  • Shib users should never receive an email about email verification (a message will be logged instead).
  • If you create a builtin user and don't click the link to verify the email (so the token is still present in the database) and then convert that user to Shib, the email should show as verified. (This has to do with removing the hasNoStaleVerificationTokens check.)
  • In the welcome in-app notification (from creating their account) Shib users will no longer see "Also, check for your welcome email to verify your address."

Weird empty "Account Information" dropdown for Shib users (#8223)

  • ensure that the weird empty "Account Information" dropdown is gone.

Changes to error handling

  • The error message users see on the Dataverse side will now vary based on what went wrong. Most commonly you will see "Invalid token" but it can also show "User deactivated".
  • For errors when clicking the email validation link, you should see more messages in server.log

Reverifying email on email change

  • I didn't touch the code having to to with blanking out emailconfirmed and sending a new verification email when you change your email but it would be good to test this.

Does this PR introduce a user interface change? If mockups are available, please link/include them here:

Yes. The bulk of the screenshots below are for possible path through the verify email workflow. Then screenshots show some fixes specific to Shib users.

Verify email workflows

A few workflows are possible:

  • 1. User clicks link in "user created" email
  • 2. User clicks "Send Notification Email" button
  • 3. User clicks an expired link

1. User clicks link in "user created" email

1a. creates account and reads email


Subject: Root: Your account has been created

Hello,
Welcome to Root! Get started by adding or finding data. Have questions? Check out the User Guide at https://guides.dataverse.org/en/5.10/user or contact Root Support Team at support@mailinator.com for assistance.

Please verify your email address at http://localhost:8080/confirmemail.xhtml?token=688597d1-1b41-4ae7-9950-a45d3f855a38 . Note, the verify link will expire after 24 hours. Send another verification email by visiting your account page.

You may contact us for support at support@mailinator.com.

Thank you,
Root Support Team


1b. clicks link and sees success message

Screen Shot 2022-04-05 at 10 20 43 AM

1c. visits account page and sees that email shows as verified

(In the screenshot, I'm also showing the dropdown for how to get to "Account Information".)

Screen Shot 2022-04-05 at 10 21 16 AM

2. User clicks "Send Notification Email" button

The user would click the "Send Notification Email" button for a few reasons:

  • They never got the first email.
  • The link is expired

2a. Clicking the button and seeing the success message (email sent)

Screen Shot 2022-04-05 at 10 24 02 AM

2b. A new link is sent via email


Subject: Root: Verify your email address

Hello, e7a744.

Please verify your email address at http://localhost:8080/confirmemail.xhtml?token=bea7e595-3022-455e-bfeb-72de253bf3b1 . Note, the verify link will expire after 24 hours. Send another verification email by visiting your account page.

Please contact us if you did not intend this change or if you need assistance.

You may contact us for support at support@mailinator.com.

Thank you,
Root Support Team


2c. Go to success message above

Clicking the link before it has expired results in the same success message and "verified" state as above.

3. User clicks an expired link

(See also "error handling" below.)

3a. message for expired link

Screen Shot 2022-04-11 at 4 08 14 PM

Shib in-app welcome (new account) message

In the welcome in-app notification (from creating their account) Shib users will no longer see "Also, check for your welcome email to verify your address." (reported in #5663). Instead, it should look like this:

Screen Shot 2022-04-08 at 3 59 40 PM

Weird empty "Account Information" dropdown removed for Shib

This pull request also includes a fix to remove the weird empty "Account Information" dropdown for Shib users reported in #8223.

before (weird dropdown present)

Screen Shot 2022-04-08 at 3 23 46 PM

after (weird dropdown absent)

Screen Shot 2022-04-08 at 3 13 29 PM

Error handling

The highlighted area in the screenshot below can vary. It will most often say "Invalid token" for non-existent or expired tokens. As shown below, it can also say "Deactivated user".

Screen Shot 2022-04-11 at 4 00 03 PM

Is there a release notes update needed for this change?:

Included.

Additional documentation:

The Admin guide has been updated. It incorrectly stated that the email address is re-verified on every login. This is now actually true. I also noted that the welcome email does not contain a URL to verify one's email.

pdurbin added 2 commits April 8, 2022 11:50
…okens #5663

For Shib users we now set the emailconfirmed timestamp on login.
(The guides say we do this already but are wrong. It was only
being set on account creation.)

For Shib users, I also prevent "check for your welcome email to
verify your address" from being shown in the in-app
welcome/new account notification.

I put in a check to make sure Shib users never get a "verify your
email address" email notification.

Finally, I removed the hasNoStaleVerificationTokens check from the
hasVerifiedEmail method. We've never worried about if there are
stale verification tokens in the database or not and this check
was preventing "Verified" from being shown, even when the user has
a timestamp (the timestamp being the way we know if an email is
verified or not).
@pdurbin pdurbin force-pushed the 8227-verify-email branch from 8b1500c to 25dc681 Compare April 8, 2022 15:51
@pdurbin pdurbin changed the title rename button to "Send Verification Email", remove popup #8227 improve email verification, no popup, auto-verify Shib users Apr 8, 2022
@coveralls
Copy link

coveralls commented Apr 8, 2022

Coverage Status

Coverage decreased (-0.003%) to 18.969% when pulling 9783504 on 8227-verify-email into b864d1a on develop.

@pdurbin pdurbin marked this pull request as ready for review April 8, 2022 19:26
@pdurbin pdurbin removed their assignment Apr 8, 2022
@pdurbin pdurbin self-assigned this Apr 11, 2022
Copy link
Contributor

@sekmiller sekmiller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable. passing to QA

@sekmiller sekmiller removed their assignment Apr 15, 2022
@kcondon kcondon self-assigned this Apr 19, 2022
@kcondon
Copy link
Contributor

kcondon commented Apr 19, 2022

Issues:

  1. Verify email button still says verify email and not send email verification.
  2. If edit account information to change an existing email, two issues are apparent: a. it returns you to my data tab and not the account info tab you were just on/editing. b. when you click on any tab other than my data, without refreshing the page, you get a blank tab and a server log error: [2022-04-19T19:31:54.383+0000] [Payara 5.2021.5] [SEVERE] [] [javax.enterprise.resource.webcontainer.jsf.context] [tid: _ThreadID=95 _ThreadName=http-thread-pool::jk-connector(3)] [timeMillis: 1650396714383] [levelValue: 1000] [[
    java.lang.IndexOutOfBoundsException: Index 2 out of bounds for length 2
    at java.base/jdk.internal.util.Preconditions.outOfBounds(Preconditions.java:64)

@pdurbin
Copy link
Member Author

pdurbin commented Apr 22, 2022

It turns out the "change email" bug exists in 5.10.1 (and who knows how many releases). I just wrote it up:

@kcondon kcondon merged commit 2c805a7 into develop Apr 22, 2022
@kcondon kcondon deleted the 8227-verify-email branch April 22, 2022 18:00
@pdurbin pdurbin added this to the 5.11 milestone May 4, 2022
@pdurbin pdurbin mentioned this pull request Jun 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment