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

Customize Auth flow for Blaze Pro #91095

Merged
merged 7 commits into from
May 30, 2024
Merged

Conversation

timur987
Copy link
Contributor

@timur987 timur987 commented May 24, 2024

Related to A8C-DSP's issue 1439.

Proposed Changes

  • Adjust common styles for the case of BlazePro (base on WooCommerce's Auth flow implementation).

Why are these changes being made?

  • We need to customize WPCOM Authentication flow for BlazePro (in the same way it's implemented for WooCommerce)

Designs in Figma.

Testing Instructions

Despite the PR 1520 from A8C-DSP repo is the starting point for running Blaze Pro, we can test changes in Calypso with the use of the following script that generates links for Login and Sign Up (it should be run with node).

function generateState() {
  const crypto = require('crypto');
  const hmac = crypto.createHmac('sha256', 'secret' );
  const data = hmac.update(new Date().getTime().toString());
  const state = data.digest('hex');
  return state;
}

const redirectUri = 'http://blaze.pro:3005/auth/connected&blog_id=0&wpcom_connect=1&calypso_env=development&scope=global&from-calypso=1';
const getLoginLink = () =>
	`https://public-api.wordpress.com/oauth2/authorize?response_type=code&client_id=92099&state=${generateState()}&redirect_uri=${encodeURIComponent(redirectUri)}`;

const signInLink = `http://calypso.localhost:3000/log-in?client_id=92099&redirect_to=${encodeURIComponent(getLoginLink())}`;

const signUpLink = `http://calypso.localhost:3000/start/wpcc/oauth2-user?oauth2_client_id=92099&oauth2_redirect=${encodeURIComponent(getLoginLink())}`;

console.log('Login URL:');
console.log(signInLink);
console.log('');
console.log('Sign Up URL:');
console.log(signUpLink);

Caveat 1: I noticed that for Google Chrome uses "DNS over HTTPS", so for me it's impossible to use it for sandboxing. So, I used Firefox for testing.

Caveat 2: I was not able to have authentication to be working locally. I assume that as long as it will be merged it should work properly due to having wordpress.com instead of calypso.localhost:3000.

Login Flow

  • Sandbox WPCOM,
  • Apply D149804-code,
  • Use the script to generate a URL,
  • Run Calypso locally,
  • Open a URL in your browser (like https://public-api.wordpress.com/oauth2/authorize?response_type=code&client_id=92099&...),
  • Ensure you see Login page similar to image below:
image
  • Use your WPCOM credentials to continue
  • Due to inability to authenticate locally, as a sign of being authenticated in WPCOM we should see a screen of already authenticated on WPCOM's side:
image

Short demo:

Screen.Recording.2024-05-24.at.14.03.40.mov

Sign Up Flow

(Continue testing)

  • Clear browser cookies,
  • Adjust code (source) in client/signup/main.jsx:850 in order to show Processing Screen from:
{ this.state.shouldShowLoadingScreen ? (

to:

{ true || this.state.shouldShowLoadingScreen ? (
  • Get a URL for Sign Up from the script,
  • Open a URL in your browser (like http://calypso.localhost:3000/start/wpcc/oauth2-user?oauth2_client_id=92099&...),

processing-screen

  • Remove code adjustment you've done before
  • Refresh the page and make sure you see Sign Up form
image

Short demo:

Screen.Recording.2024-05-24.at.14.27.51.mov

Pre-merge Checklist

  • Has the general commit checklist been followed? (PCYsg-hS-p2)
  • Have you written new tests for your changes?
  • Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
  • Have you checked for TypeScript, React or other console errors?
  • Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
  • Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
  • For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-aUh-p2)?

@matticbot
Copy link
Contributor

matticbot commented May 24, 2024

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

App Entrypoints (~638 bytes added 📈 [gzipped])

name                 parsed_size           gzip_size
entry-login              +3720 B  (+0.3%)     +564 B  (+0.1%)
entry-stepper            +1557 B  (+0.1%)     +308 B  (+0.1%)
entry-main               +1557 B  (+0.1%)     +308 B  (+0.1%)
entry-subscriptions       +205 B  (+0.0%)      +64 B  (+0.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Sections (~534 bytes added 📈 [gzipped])

name             parsed_size           gzip_size
jetpack-connect      +2408 B  (+0.2%)     +307 B  (+0.1%)
accept-invite        +1166 B  (+0.6%)     +143 B  (+0.3%)
signup                +565 B  (+0.2%)     +117 B  (+0.2%)

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

Async-loaded Components (~410 bytes added 📈 [gzipped])

name                          parsed_size           gzip_size
async-load-design-blocks          +2056 B  (+0.1%)     +208 B  (+0.0%)
async-load-signup-steps-user      +1502 B  (+0.8%)     +202 B  (+0.4%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

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.

@timur987 timur987 requested a review from chihsuan May 24, 2024 09:14
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label May 24, 2024
@timur987 timur987 requested a review from ilyasfoo May 24, 2024 09:14
@timur987 timur987 marked this pull request as draft May 24, 2024 09:15
@timur987 timur987 self-assigned this May 24, 2024
@timur987 timur987 marked this pull request as ready for review May 24, 2024 11:37
@timur987 timur987 force-pushed the add/blaze-pro-authentication-flow branch from 16585fd to b00e08c Compare May 24, 2024 11:43
@matticbot
Copy link
Contributor

matticbot commented May 24, 2024

This PR modifies the release build for the following Calypso Apps:

For info about this notification, see here: PCYsg-OT6-p2

  • blaze-dashboard
  • odyssey-stats

To test WordPress.com changes, run install-plugin.sh $pluginSlug add/blaze-pro-authentication-flow on your sandbox.

Copy link
Contributor

@therocket-gr therocket-gr left a comment

Choose a reason for hiding this comment

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

tested locally
works as described. code changes LGTM

Copy link
Member

@chihsuan chihsuan left a comment

Choose a reason for hiding this comment

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

Hey @timur987, thanks for adding us as reviewers! Nice work here. 👍

First, I think code overall looks good and I confirmed that WooCommerce flow styles are applied correctly. However, I have a few comments:

  1. I see a couple of UI issues. The UI is not fully consistent with the designs. For example, the font size of footer text is a bit bigger than in the designs and padding between the heading and subheading is smaller. I'd suggest to make sure that the UI is consistent with the designs. But I'll leave it up to you to decide if you want to make these changes or not.
Screenshot 2024-05-27 at 12 24 39
  1. The title flashing on the screen for a second before the page is fully loaded. I think there might some heading mismatch in the code.
Screen.Recording.2024-05-27.at.12.35.51.mov
  1. When I clicked on Log in here link, I was redirected to a blank page. I think it should redirect to the login page.
Screen.Recording.2024-05-27.at.12.27.55.mov
  1. I tried to sign up with an new email, but I got an error Invalid OAuth2 redirect url.
Screenshot 2024-05-27 at 12 31 07
  1. 2FA screens are WP.com branded. Looks like design is missing for these screens.
Screenshot 2024-05-27 at 12 33 48
  1. We're using passwordless signup for WooCommerce. I'd suggest to use the same for BlazePro as well.
Screenshot 2024-05-27 at 12 57 40

Due to inability to authenticate locally, as a sign of being authenticated in WPCOM we should see a screen of already authenticated on WPCOM's side:

Caveat 1: I noticed that for Google Chrome uses "DNS over HTTPS", so for me it's impossible to use it for sandboxing. So, I used Firefox for testing.

Caveat 2: I was not able to have authentication to be working locally. I assume that as long as it will be merged it should work properly due to having wordpress.com instead of calypso.localhost:3000.

To test sign/login flow locally, you can set up a local wordpress.com or wpcalypso.wordpress.com environment by following the instructions in this field guide (PCYsg-5YE-p2 > Running Calypso locally under wordpress.com or wpcalypso.wordpress.com)

My team also created some scripts that use docker to set up a local environment. It's in a private repo. If you want, I can share it with you.

@timur987
Copy link
Contributor Author

Thanks for such a thorough review @chihsuan!

I addressed your comments (118c6ba) and will refer to them in order not to miss anything.

UI Issues

  1. I see a couple of UI issues. The UI is not fully consistent with the designs. For example, the font size of footer text is a bit bigger than in the designs and padding between the heading and subheading is smaller. I'd suggest to make sure that the UI is consistent with the designs.

Thanks for matching the design. I changed icon to be 32x32px, adjusted padding between the heading and subheading. The only issue I can't find the way to overcome is to use fonts as in design. In particular, Calypso asks to use rem as a unit an has limited set of allowed values (style lint settings). For this case I needed to use 18px which is 1.125rem but Calypso allows only either 1rem or 1.25rem, so I decided that bigger is better. Maybe I'm wrong. Will ask our designer, @bartoszbak.

Title flashing

  1. The title flashing on the screen for a second before the page is fully loaded. I think there might some heading mismatch in the code.

Indeed there was the discrepancy between the OAuth App name in Calypso's code and in WPCOM Apps. Fixed by changing name in App's settings (here).

URL issues

The following two (3 and 4) are similar:

  1. When I clicked on Log in here link, I was redirected to a blank page. I think it should redirect to the login page.
  2. I tried to sign up with an new email, but I got an error Invalid OAuth2 redirect url.

Sorry for that. I made a mistake (that caused improper URI encoding of redirect URLs) in script for link generation in testing instructions. Updated the script, so now it should work properly.

2FA

  1. 2FA screens are WP.com branded. Looks like design is missing for these screens.

Thanks for pointing me, indeed. Addresses it as well.

Screen.Recording.2024-05-28.at.11.54.42.mov

Passwordless signup

  1. We're using passwordless signup for WooCommerce. I'd suggest to use the same for BlazePro as well.

Thanks for letting me know. I'll discuss this opportunity with my team.

Local setup

To test sign/login flow locally, you can set up a local wordpress.com or wpcalypso.wordpress.com environment by following the instructions in this field guide (PCYsg-5YE-p2 > Running Calypso locally under wordpress.com or wpcalypso.wordpress.com)

My team also created some scripts that use docker to set up a local environment. It's in a private repo. If you want, I can share it with you.

Thanks for letting me know. I thing for now the right front-end design is what we need. I think that in case of issues I'll definitely ask you for access to the mentioned private repo.

@timur987 timur987 requested a review from chihsuan May 28, 2024 09:04
@chihsuan
Copy link
Member

@timur987 Thanks for addressing feedback.

For this case I needed to use 18px which is 1.125rem but Calypso allows only either 1rem or 1.25rem, so I decided that bigger is better. Maybe I'm wrong. Will ask our designer,

You can use rem(18px) to get 1.125rem in Calypso. It's a helper function that converts pixels to rems and not limited by stylelint settings.

2FA
Thanks for pointing me, indeed. Addresses it as well.

I noticed that on the video recording, when you hovered over the button, the background color changed to WP blue. I think it would be great to fix that.

Copy link
Member

@chihsuan chihsuan left a comment

Choose a reason for hiding this comment

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

I tested again and functionality works as expected. 👍

Just left a few comments for UI improvements. Since this doesn't affect the WooCommerce flows and Blaze Pro is not public yet, I'm going to approve this PR and let your team decide on the final design. 🙂

@timur987
Copy link
Contributor Author

Thanks, @chihsuan for priceless suggestions and TIL moment regarding to using rem function. I'll wait a bit for @bartoszbak's opinion and will deploy it.

@timur987 timur987 merged commit 933a574 into trunk May 30, 2024
11 checks passed
@timur987 timur987 deleted the add/blaze-pro-authentication-flow branch May 30, 2024 12:16
@github-actions github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label May 30, 2024
timur987 added a commit that referenced this pull request May 30, 2024
timur987 added a commit that referenced this pull request May 30, 2024
timur987 added a commit that referenced this pull request May 30, 2024
timur987 added a commit that referenced this pull request May 30, 2024
timur987 added a commit that referenced this pull request May 30, 2024
timur987 added a commit that referenced this pull request May 30, 2024
timur987 added a commit that referenced this pull request May 30, 2024
* Revert "Revert "2nd attempt to Customize Auth flow for Blaze Pro (#91095)" (#…"

This reverts commit 8224f2f.

* Restore correct showing of username input
@timur987
Copy link
Contributor Author

I have several attempts to deliver this PR but it failed Pre-release tests. Here's the last PR I tried to deliver to production: #91304.

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

4 participants