-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Include calypso_env in the send-login-email payload #94692
Include calypso_env in the send-login-email payload #94692
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~34 bytes added 📈 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~30 bytes added 📈 [gzipped])
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 (~30 bytes added 📈 [gzipped])
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. Generated by performance advisor bot at iscalypsofastyet.com. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR works as described, but I think more context about what this is intended to do would be useful as it relates to the login system and should be scrutinised closely.
The passwordless login works correctly without this PR but redirects to wordpress.com, so I'm inferring that the intent is to allow the changing of the redirect_to on the wpcom login API? I think it would be safer to switch that based on a different client oauth ID so that you can't (for example) login using dev credentials and redirect to prod. I'm not really sure why calypso does not have a dev / staging client oauth ID unlike all the other envs, though
D161820-code should better reflect the intention of the change.
It's currently sending all logins to https://wordpress.com/, regardless of the environment you're in (It's already doing what you've described). I don't think it can be exploited just by changing the redirection URL with the current configuration, but I agree that we should restrict the redirection URLs. I think it needs further investigation though. |
Yep ok thanks that PR gives all the necessary information! I think it would be good to include that in the PR description for future passerbys |
👍 Done! Sorry for the confusion 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good! 🚢
… us to use the correct login URL when mocking wordpress with start-mock-wordpress-com command or manually configuring the host via host file
81adc85
to
137d9d0
Compare
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
This PR adds
calypso_env
in thesend-login-email
request payload. This allow us to have a magic link in different calypso environments. For more information how the backend handlescalypso_env
, please see D161820-codeProposed Changes
calypso_env
in thesend-login-email
request payloadTesting Instructions
config/development.json
"woocommerce/core-profiler-passwordless-auth"
feature flag.yarn start
Continue
send-login-email
request and confirm the payload containscalypso_env
Pre-merge Checklist