-
Notifications
You must be signed in to change notification settings - Fork 500
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
Shibboleth users who predate Shibboleth are assumed to be email-verified but aren't #5663
Comments
I think the logic snag is that when the |
@pameyer seems better to make current Shib logins check for emailconfirmed and set if necessary — we have a bunch of users matching unc.edu who may or may not sign in going forward. My inner Security Chicken Little would prefer to leave them NULL. |
@donsizemore That seems reasonable to me; but means it would take code changes rather than database cleanup. |
I'm pretty sure I'm affected by this personally in Harvard Dataverse (I had an old account in the DVN 3.x days and I'm using Shib to log in) and the user experience is pretty terrible. Here are some screenshots from 5.10: I noticed "Not Verified" and clicked "Verify Email".I get a "success, email sent" messageThe email I received is nonsensical (no link, for example, no content, really)Knowing what I know from hacking on pull request #8579 recently, I'm aware that the user experience is even further confusing because prior to that pull request if you go back to Account Information and click "Verify Email" a pop up will show (screenshots below). What this popup is TRYING to say is that an email has already been sent. In the that pull request we decided to remove the popup. Here's how it looks in 5.10: I'm back on "Account Information" (within 24 hours so the link/token is still present in the database)Confusing popup that's trying to say an email has already been sentGiven all this, what's the plan? Some thoughts:
Update: Examples of the mostly empty nonsensical email above: |
@pdurbin I love the idea of populating |
@donsizemore great. Update at each login. Sounds like a plan. OAuth and OIDC users would not be affected by any change I'm planning for this issue. For OIDC, please note that there's an open issue about email verification: Meanwhile I did some more Shib testing locally and wanted to share some screenshots and a bug I'd like to fix. When you create a Shib account, the process is fine (email shows as verified) except that the in-app notification says, "check for your welcome email to verify your address." This part of the message shouldn't be shown to Shib users. The email itself is fine. That is, the email correctly does not include a link or any text about verifying your email. Subject: Root: Your account has been created Hello, You may contact us for support at support@mailinator.com. Thank you, I have the change queued up locally to update the in-app notification for newly created Shib users. |
More screenshots. I also looked at converting accounts from builtin to Shib. The bug here is that the email is not automatically set to verified. Another potential bug is that a "confirm email" token is in the database for this user. Presumably this token was created back when I created the builtin user. Perhaps the conversion process to Shib should clear out any "confirm email" tokens. In practice, this is not a big problem because this token will simply expire in 24 hours (by default). |
…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).
…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).
I put fixes in this pull request: |
This issue is related to #1401 and possibly supersedes #3300. Per Phil and Pete in IRC Shibboleth users are now assumed to have their e-mail addresses verified as trusted / provided by the IdP. For new users this happens. Existing users such as myself and our archivists remain unverified. I can sign in with my Shibboleth account to our test server running 4.11 and don't get an "emailconfirmed" timestamp:
I could start wedging in timestamps for institutional users but would love to find the logic snag.
The text was updated successfully, but these errors were encountered: