SSO: Show wp-admin login form if site has local users #39139
Conversation
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Follow this PR Review Process:
Still unsure? Reach out in #jetpack-developers for guidance! Wpcomsh plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
github-actions
bot
added
the
[Status] Needs Author Reply
mmtr
added
[Status] Needs Team Review
and removed
[Status] Needs Author Reply
| $connection_manager = new \Automattic\Jetpack\Connection\Manager( 'jetpack' ); | ||
| $users = get_users( array( 'fields' => array( 'ID' ) ) ); | ||
| foreach ( $users as $user ) { | ||
| if ( ! $connection_manager->is_user_connected( $user->ID ) ) { |
There was a problem hiding this comment.
How does this perform for large sites? > 10k users?
There was a problem hiding this comment.
Good question! I have no idea 😅 I'll see if there is an easy way to populate a test site with 10k fake users and check
There was a problem hiding this comment.
I'd say though that if a site has 10k users, it's very likely that most of them will be non-connected, and we'll stop the loop when the first one is found.
There was a problem hiding this comment.
Used FakerPress to create 5k users (not connected to WP.com) on a site:
It seems that the performance is not largely affected in this scenario.
Before, it took 1.34s to show the Jetpack SSO login. Now, it takes 1.53s
| Before | After |
|---|---|
 |
 |
There was a problem hiding this comment.
Imo, that's just one scenario. If 4.9k users are connected there would be a higher performance penalty. This has an O(n) complexity.
Stops enforcing the WP.com login for Atomic sites with local users. Previously, we were disabling the enforced WP.com login on sites with the classic interface (except for users coming from Calypso), and kept it on sites with the default interface. However, sites with the default interface can have local users as well (users not connected to WP.com) who are unable to use their wp-admin credentials to log in into wp-admin. This commit fixes that by changing who is enforced to log in with a WP.com account: - Sites without local users: - WP.com login, always. - Sites with local users: - If user comes from Calypso: WP.com login - Otherwise: Jetpack SSO login, so they can decide whether to use a WP.com account or a local account.
Fixes #39037
Proposed changes:
Stops enforcing the WP.com login for Atomic sites with local users.
Previously, we were disabling the enforced WP.com login on sites with the classic interface (except for users coming from Calypso), and kept it on sites with the default interface.
However, sites with the default interface can have local users as well (users not connected to WP.com) who are unable to use their wp-admin credentials to log in into wp-admin.
This PR fixes that by changing who is enforced to log in with a WP.com account:
Other information:
Jetpack product discussion
pc4f5j-4Ky-p2
Does this pull request change what data or activity we track or use?
No
Testing instructions:
wpcomsh)./wp-admin/users.php/wp-admin/wp-admin