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

Email verification link #7117

Merged
merged 9 commits into from
Jun 27, 2024
Merged

Email verification link #7117

merged 9 commits into from
Jun 27, 2024

Conversation

shaunanoordin
Copy link
Member

@shaunanoordin shaunanoordin commented Jun 1, 2024

PR Overview

Staging branch URL: https://pr-7117.pfe-preview.zooniverse.org/settings/email
Follows #7083, towards #7032

This PR adds a link to the Email Settings page (under User -> Settings -> Email), which allows users to request a confirmation email from Panoptes.

image

New behaviour:

  • This link only appears if your account's email is unverified.
  • Clicking on the link will send a request to Panoptes, to send the confirmation email.
  • Status messages:
    • Idle: "Resend confirmation email" (link)
    • Requesting: "Requesting..."
    • Success: "Confirmation email requested, please check your inbox"
    • Error: "ERROR: could not request confirmation email"

Testing

  • Login using a test account with an unverified email.
  • Go to the Email Settings page.
  • Click on the "Resend confirmation email" link.

Status

⚠️ WIP - this PR is not yet functional.

As of commit aeaeca2, there's a problem where sending the request results in a 406 error, at least for staging.

@coveralls
Copy link

coveralls commented Jun 1, 2024

Coverage Status

coverage: 56.94% (-0.03%) from 56.969%
when pulling 14eb6ec on email-verification-link
into 5150a5e on master.

</a>
*/}
{(this.state.requestConfirmationEmailStatus === 'idle') && (
<a href="#" onClick={this.requestConfirmationEmail}>
Copy link
Contributor

@eatyourgreens eatyourgreens Jun 1, 2024

Choose a reason for hiding this comment

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

This link links back to the current page. I think you meant to use a button here. In a screen reader, or even as a sighted user, I would expect the browser to navigate to a new location after clicking a link. I think the screen reader is more confusing, as this will be announced as a link, but the browser doesn't navigate anywhere after I click it (very confusing if I can't see the page.)

Copy link
Contributor

Choose a reason for hiding this comment

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

See the WebAIM accessibility cheat sheet for more info about accessible links, but the code here would fail an accessibility audit.

@eatyourgreens
Copy link
Contributor

eatyourgreens commented Jun 1, 2024

By the way, 406: Not Acceptable is the response when content negotiation fails, so my first thought would be to look at the Accept headers.

However, when the 406 Not Acceptable response is returned, the message body will contain a list of available representations that the client can choose from.

https://http.dev/406

@eatyourgreens
Copy link
Contributor

eatyourgreens commented Jun 1, 2024

If I go to https://pr-7117.pfe-preview.zooniverse.org/settings/email, then change my email address to a totally new email address, the page still says my email is verified. Is that a bug that I can exploit in order to get around spam blocks?

I tested this by switching between an address on a personal domain that I own (which is verified), and my work address on the ox.ac.uk domain (which is not verified.)

EDIT: when I changed my email address, the old address didn't get an email to say 'hey, someone changed your email address to a new address.' That's possibly an exploitable security hole too. Or maybe the notification emails are being a bit slow today.

EDIT THE EDIT: I've now changed my email address a few times, swapping between personal and work domains. I haven't had any email notifications to say that someone else may have changed my email address, or that there's potentially suspicious activity on my account.

@eatyourgreens
Copy link
Contributor

eatyourgreens commented Jun 3, 2024

Here's a weird bug (particularly if I'm using a screen reader, or colour blind, so don't see the change in colour of the input label.) When I delete my email address completely, I'm told the empty box is both valid and verified. When you're using colour like this, it's good practice to test your component in a greyscale mode, to catch accessibility failures where colour is being used to convey important information.

There's also no button to submit changes. That's another potential accessibility failure. I guess that if I'm on a phone or tablet, I can discover by trial-and-error that tapping outside the box will save changes, but the UX is less than ideal.

Screen.Recording.2024-06-03.at.09.52.23.mov

@shaunanoordin shaunanoordin requested a review from a team June 17, 2024 18:51
@shaunanoordin
Copy link
Member Author

PR Update

Update: the resend confirmation email link should now be fully functional!

  • 406 error has been resolved by actually sending the correct request format.
    • To be specific, the solution was: 1. POST to the correct URL, 2. header should only include Content-Type and charset, 3. body/data should NOT be in JSON format, 4. NOTHING ELSE
  • Reviewing Zach's example, I realised I was overthinking what actually needed to be sent. As a result, all code require getBearerToken() has been stripped out, and all attempts to make the POST request resemble a PJC JSON API client request have been removed.

Status

This PR is ready for review.

A follow up is required to address the accessibility feedback.

@lcjohnso
Copy link
Member

lcjohnso commented Jun 17, 2024

Great work, Shaun! I can confirm this solution works when tested for an unconfirmed user account.

I agree with Jim's comment regarding link vs. button. When "Resend Confirmation Email" appears as a clickable link -- as Sean and I recommend -- it is a bit odd to see https://pr-7117.pfe-preview.zooniverse.org/settings/email# as the link destination. Now that I've used the feature, I'd prefer a button so I don't expect to navigate away to a different page.

Sorry for the indecision! @seanmiller26 -- are you OK with a button instead of a link?

@seanmiller26
Copy link

Sure, that sounds good. Let's use this style of button design - #ffffff fill with #005d69 outline, 4pt rounded corners, 16pt font.

@shaunanoordin
Copy link
Member Author

PR Update

Thanks for the feedback, everyone!

  • Accessibility updates:
    • Status messages are now inside <output>
    • "Resend email" link replaced with button. (@seanmiller26 see design notes below)

Design note 1: button looks like this. Font size is kept consistent with rest of text, which is... uh... 13.3333px? PFE is weird like that.

image
(If you're curious to see what the button looks like with font-size: 16px, click here) image

Design note 2: hover & click has a different background.

image

Design note 3: status text

image

For further discussion, but won't be addressed in this PR:

  • ❓ can an email be un-verified by changing it? Does changing an email mean it's still verified?

@lcjohnso
Copy link
Member

@shaunanoordin -- I can answer this one:

  • ❓ can an email be un-verified by changing it? Does changing an email mean it's still verified?

No. Once verified, the account is always considered verified, which is more about confirming the validity of the account rather than the email. See zooniverse/panoptes#4268 (comment).

@shaunanoordin shaunanoordin force-pushed the email-verification-link branch 2 times, most recently from c2e095e to b4296ef Compare June 18, 2024 21:15
@eatyourgreens
Copy link
Contributor

Yep,once I've verified an account, I can change the email address to any valid email address at all, without the owner of that address being notified that their address is being used.

A verified Zooniverse account for cliff+random@zooniverse.org

@lcjohnso
Copy link
Member

re: email notifications -- I've opened zooniverse/panoptes#4351; that is out of scope for this PR and will be resolved separately.

@kieftrav kieftrav requested review from kieftrav and removed request for a team June 26, 2024 18:02
Copy link
Contributor

@kieftrav kieftrav left a comment

Choose a reason for hiding this comment

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

Works as expected and code looks good!

'posting': <Translate content="emailSettings.general.emailUnverifiedRequesting" />,
'success': <Translate content="emailSettings.general.emailUnverifiedSuccess" />,
'error': <Translate content="emailSettings.general.emailUnverifiedError" />,
}[this.state.requestConfirmationEmailStatus] || ''}
Copy link
Contributor

Choose a reason for hiding this comment

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

Clever!

@shaunanoordin shaunanoordin enabled auto-merge (squash) June 27, 2024 14:39
@shaunanoordin shaunanoordin merged commit d7f9954 into master Jun 27, 2024
5 checks passed
@shaunanoordin shaunanoordin deleted the email-verification-link branch June 27, 2024 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants