Initial work on a new Security Checkup feature. #43101

daledupreez · 2020-06-09T14:05:09Z

Changes proposed in this Pull Request

This PR sets the groundwork for a new Security Checkup feature (described here), which is currently set up behind the security/security-checkup feature flag, which is limited to dev environments for now.

The goal is to show a quick overview of a user's security and account recovery settings. For now, this initial PR does not represent a wrapped product, but a basic skeleton to support further iteration.

At present, this PR includes the following items:

Create SecurityCheckupComponent with basic styling and wire it into the security nav
Wire in the following links and basic assessments:
- Account email
- 2FA configuration
- Recovery email
- Recovery phone/SMS number
- Connected apps

Open issues

I'll address these in subsequent iterations/PRs.

The security nav is now using a dropdown instead of a tab-like view due to the additional menu item
The wording almost certainly needs additional love/attention
Maybe add in a row for password details -- not quite sure what ought to go in here
Add in a row for social logins
Consider using different/better icons, possibly with colours - I just picked some existing icons as a starting point
Clean up the use of userSettings and the 2FA logic, including expanding the assessment to include backup codes
General event tracking/logging
Most bugs! 😁 I suspect there are some with the 2FA handling, as that seemed the most likely to be buggy.

Side note: A huge 🎩 to @fditrapani! I cribbed his designs for the new domain settings UI for the basic layout, as the core UX in the two screens is very similar.

Testing instructions

Verify that the feature flag is working and does not expose the new UI when not in a dev environment.
With the feature flag on, verify that the following work:
- All of the rows render correct information and an appropriate icon
- Clicking on each row navigates to the correct target UI

Relates to #26874, though it doesn't directly address all of the items in the issue yet.

Work items include: * Set up the `security/security-checkup` feature flag and limit the feature to dev environments * Create `SecurityCheckupComponent` with basic styling and wire it into the security nav * Wire in the following links and basic assessments: - Account email - 2FA configuration - Recovery email - Recovery phone/SMS number - Connected apps

matticbot · 2020-06-09T14:05:11Z

Test live: https://calypso.live/?branch=add/security-checkup

matticbot · 2020-06-09T14:05:43Z

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Sections (~1366 bytes added 📈 [gzipped])

name      parsed_size           gzip_size
security      +8293 B  (+1.9%)    +1366 B  (+1.2%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

a8ci18n · 2020-06-09T14:33:51Z

client/me/security-checkup/index.jsx

+			<SecurityCheckupNavigationItem
+				path={ '/me/security/account-recovery' }
+				materialIcon={ icon }
+				text={ translate( 'Recovery SMS Number' ) }


ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 42 times:
translate( 'Recovery SMS number' ) ES Score: 15
See 1 additional suggestions in the PR translation status page

fditrapani · 2020-06-09T16:59:31Z

Nice, the screenshot and options look good! Just curious about the "Security checkup" dropdown and where this screen fits in to the security section of /me?

daledupreez · 2020-06-09T17:16:41Z

Thanks for taking a look, @fditrapani!

The dropdown seems to be a "feature" of the SectionNav component when the text exceeds the available width of the current screen. In this particular case, the low-level .main style has max-width set to 720px, so we exceed the limit and shift to the dropdown layout. 😬

This is definitely on my to-fix list!

daledupreez · 2020-06-09T17:47:13Z

OK, so the dropdown/width solution is going to be messy... each component in /me/security pulls in Main directly, so "just" enabling the wideLayout option is going to end up touching a number of files and affecting some existing UX, etc, etc. Definitely going to take that on separately.

klimeryk

Tested - already looking really promising! Verified that without the feature flag there does not appear to be any regressions as well 👍

klimeryk · 2020-06-09T17:51:02Z

client/me/security-checkup/index.jsx

+	renderVerticalNav() {
+		return (
+			<VerticalNav>
+				{ this.renderAccountEmail() }


It might be already on your radar, but I'd suggest splitting each of these rows/render* calls into separate components. On their own, they are not that complicated, but even this "skeleton" version of the component is already over 400 lines long and I imagine it will get longer as more logic is added.

That was definitely starting to become more apparent as I added new rows -- the code and the dependencies started to become unwieldy. So I'll shift the new rows to components tomorrow and then loop back to the items I've already implemented.

wensco · 2020-06-09T19:15:29Z

The wording almost certainly needs additional love/attention

I actually think it's pretty good as-is. You could simplify the first item by omitting "set to" so it just reads "Your account email is [email]" but that's fairly nitpicky.

For the recovery options (email, SMS), I'm wondering if users will understand what this is. We could link them to relevant support docs, which might actually not be a bad idea across the board.

Consider using different/better icons, possibly with colours - I just picked some existing icons as a starting point

Agree, this looks like a nice starting point though would be nice to use colors/icons so that users can grok at a glance which items need attention.

Maybe add in a row for password details -- not quite sure what ought to go in here

Not sure we want to go down the path of indicating good/strong PWs here, especially since we already have something that checks that upon account creation. Perhaps an indicator of when the last time was that the PW was reset, eg "Your password was last changed on [date]". We could then show [date] in yellow if it's been more than (let's say) a year with a tool tip suggestion to consider updating it.

a8ci18n · 2020-06-10T09:16:23Z

This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/3792313

Thank you @daledupreez for including a screenshot in the description! This is really helpful for our translators.

fditrapani · 2020-06-10T12:43:18Z

OK, so the dropdown/width solution is going to be messy... each component in /me/security pulls in Main directly, so "just" enabling the wideLayout option is going to end up touching a number of files and affecting some existing UX, etc, etc. Definitely going to take that on separately.

Good call. It's a tricky problem to work around but here's one potential solution:

We can make this new screen the landing page for Security where it can double as a status screen and navigation which will allow us to trim down the section navigation.

daledupreez · 2020-06-10T14:08:12Z

Thanks for working on this, @fditrapani! I think it would be cleaner if we shift the conversation to #43144 so we can clearly focus on this specific problem.

a8ci18n · 2020-06-17T12:28:31Z

Translation for this Pull Request has now been finished.

daledupreez added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Security labels Jun 9, 2020

daledupreez requested a review from a team June 9, 2020 14:05

daledupreez self-assigned this Jun 9, 2020

a8ci18n reviewed Jun 9, 2020

View reviewed changes

klimeryk approved these changes Jun 9, 2020

View reviewed changes

klimeryk added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jun 9, 2020

daledupreez added the [Status] Needs e2e Testing label Jun 9, 2020

daledupreez merged commit ccc2982 into master Jun 10, 2020

daledupreez deleted the add/security-checkup branch June 10, 2020 07:19

matticbot removed the [Status] Ready to Merge label Jun 10, 2020

This was referenced Jun 10, 2020

Refactor code to manage Security Checkup items in dedicated components #43142

Merged

Improve navigation for Security section as a result of Security Checkup #43144

Closed

This was referenced Jun 12, 2020

Add social login information to security checkup #43226

Merged

Experiment: Restructure /me/security #43234

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial work on a new Security Checkup feature. #43101

Initial work on a new Security Checkup feature. #43101

daledupreez commented Jun 9, 2020

matticbot commented Jun 9, 2020

matticbot commented Jun 9, 2020

a8ci18n Jun 9, 2020

fditrapani commented Jun 9, 2020

daledupreez commented Jun 9, 2020

daledupreez commented Jun 9, 2020

klimeryk left a comment

klimeryk Jun 9, 2020

daledupreez Jun 9, 2020

wensco commented Jun 9, 2020

a8ci18n commented Jun 10, 2020

fditrapani commented Jun 10, 2020 •

edited

Loading

daledupreez commented Jun 10, 2020

a8ci18n commented Jun 17, 2020

Initial work on a new Security Checkup feature. #43101

Initial work on a new Security Checkup feature. #43101

Conversation

daledupreez commented Jun 9, 2020

Changes proposed in this Pull Request

Open issues

Testing instructions

matticbot commented Jun 9, 2020

matticbot commented Jun 9, 2020

a8ci18n Jun 9, 2020

Choose a reason for hiding this comment

fditrapani commented Jun 9, 2020

daledupreez commented Jun 9, 2020

daledupreez commented Jun 9, 2020

klimeryk left a comment

Choose a reason for hiding this comment

klimeryk Jun 9, 2020

Choose a reason for hiding this comment

daledupreez Jun 9, 2020

Choose a reason for hiding this comment

wensco commented Jun 9, 2020

a8ci18n commented Jun 10, 2020

fditrapani commented Jun 10, 2020 • edited Loading

daledupreez commented Jun 10, 2020

a8ci18n commented Jun 17, 2020

fditrapani commented Jun 10, 2020 •

edited

Loading