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

SSO: rely on Connection methods instead of relying on Jetpack. Part 2 #36989

Merged
merged 19 commits into from
Apr 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions projects/packages/connection/.phan/baseline.php
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,11 @@
// PhanTypeArraySuspiciousNullable : 5 occurrences
// PhanTypeMismatchDefault : 5 occurrences
// PhanTypeMismatchArgumentInternal : 3 occurrences
// PhanTypeMismatchArgumentNullable : 3 occurrences
// PhanTypeObjectUnsetDeclaredProperty : 3 occurrences
// PhanNonClassMethodCall : 2 occurrences
// PhanPluginUnreachableCode : 2 occurrences
// PhanPossiblyUndeclaredVariable : 2 occurrences
// PhanTypeMismatchArgumentNullable : 2 occurrences
// PhanTypeMismatchPropertyDefault : 2 occurrences
// PhanTypeMismatchReturnNullable : 2 occurrences
// PhanTypePossiblyInvalidDimOffset : 2 occurrences
Expand Down Expand Up @@ -57,7 +57,7 @@
'src/class-partner.php' => ['PhanTypeMismatchPropertyProbablyReal'],
'src/class-plugin-storage.php' => ['PhanUndeclaredClassMethod'],
'src/class-rest-authentication.php' => ['PhanTypeMismatchPropertyDefault', 'PhanTypeMismatchPropertyProbablyReal'],
'src/class-rest-connector.php' => ['PhanParamTooMany', 'PhanTypeMismatchArgument', 'PhanTypeMismatchArgumentProbablyReal', 'PhanTypeMismatchReturnProbablyReal', 'PhanUndeclaredClassMethod'],
'src/class-rest-connector.php' => ['PhanParamTooMany', 'PhanTypeMismatchArgument', 'PhanTypeMismatchArgumentProbablyReal', 'PhanTypeMismatchReturnProbablyReal'],
'src/class-secrets.php' => ['PhanNonClassMethodCall', 'PhanTypeMismatchArgument'],
'src/class-server-sandbox.php' => ['PhanPluginDuplicateConditionalNullCoalescing', 'PhanTypeMismatchArgument'],
'src/class-tokens.php' => ['PhanImpossibleTypeComparison', 'PhanTypeMismatchArgumentInternal', 'PhanTypeMismatchReturn', 'PhanTypeMismatchReturnProbablyReal'],
Expand All @@ -66,7 +66,7 @@
'src/sso/class-helpers.php' => ['PhanTypeMismatchArgumentProbablyReal'],
'src/sso/class-sso.php' => ['PhanNoopNew', 'PhanRedundantCondition', 'PhanTypeMismatchArgument', 'PhanTypeMismatchArgumentProbablyReal', 'PhanUndeclaredClassMethod'],
'src/sso/class-user-admin.php' => ['PhanPluginUnreachableCode', 'PhanTypeMismatchArgument'],
'src/webhooks/class-authorize-redirect.php' => ['PhanTypeMismatchArgumentNullable', 'PhanUndeclaredClassMethod', 'PhanUndeclaredClassReference'],
'src/webhooks/class-authorize-redirect.php' => ['PhanUndeclaredClassMethod', 'PhanUndeclaredClassReference'],
'tests/php/test-class-nonce-handler.php' => ['PhanPluginDuplicateAdjacentStatement', 'PhanTypeMismatchArgument'],
'tests/php/test-class-webhooks.php' => ['PhanDeprecatedFunction'],
'tests/php/test-partner-coupon.php' => ['PhanDeprecatedFunction'],
Expand Down
4 changes: 4 additions & 0 deletions projects/packages/connection/changelog/update-sso-connect
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: patch
Type: changed

SSO: rely on Connection methods instead of relying on methods from the Jetpack plugin.
35 changes: 30 additions & 5 deletions projects/packages/connection/src/class-manager.php
Original file line number Diff line number Diff line change
Expand Up @@ -1878,11 +1878,16 @@ public function get_token( $data ) {
/**
* Builds a URL to the Jetpack connection auth page.
*
* @param WP_User $user (optional) defaults to the current logged in user.
* @param String $redirect (optional) a redirect URL to use instead of the default.
* @since $$next-version$$ Added optional $from and $raw parameters.
*
* @param WP_User $user (optional) defaults to the current logged in user.
* @param string $redirect (optional) a redirect URL to use instead of the default.
* @param bool|string $from If not false, adds 'from=$from' param to the connect URL.
* @param bool $raw If true, URL will not be escaped.
*
* @return string Connect URL.
*/
public function get_authorization_url( $user = null, $redirect = null ) {
public function get_authorization_url( $user = null, $redirect = null, $from = false, $raw = false ) {
if ( empty( $user ) ) {
$user = wp_get_current_user();
}
Expand Down Expand Up @@ -1975,8 +1980,28 @@ public function get_authorization_url( $user = null, $redirect = null ) {

$url = add_query_arg( $body, $api_url );

/** This filter is documented in plugins/jetpack/class-jetpack.php */
return apply_filters( 'jetpack_build_authorize_url', $url );
if ( is_network_admin() ) {
$url = add_query_arg( 'is_multisite', network_admin_url( 'admin.php?page=jetpack-settings' ), $url );
}

if ( $from ) {
$url = add_query_arg( 'from', $from, $url );
}

if ( $raw ) {
$url = esc_url_raw( $url );
sergeymitr marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Filter the URL used when connecting a user to a WordPress.com account.
*
* @since 2.0.0
* @since $$next-version$$ Added $raw parameter.
*
* @param string $url Connection URL.
* @param bool $raw If true, URL will not be escaped.
*/
return apply_filters( 'jetpack_build_authorize_url', $url, $raw );
}

/**
Expand Down
7 changes: 2 additions & 5 deletions projects/packages/connection/src/class-rest-connector.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

namespace Automattic\Jetpack\Connection;

use Automattic\Jetpack\Connection\Webhooks\Authorize_Redirect;
use Automattic\Jetpack\Constants;
use Automattic\Jetpack\Redirect;
use Automattic\Jetpack\Status;
Expand Down Expand Up @@ -814,11 +815,7 @@ public function connection_register( $request ) {

$redirect_uri = $request->get_param( 'redirect_uri' ) ? admin_url( $request->get_param( 'redirect_uri' ) ) : null;

if ( class_exists( 'Jetpack' ) ) {
$authorize_url = \Jetpack::build_authorize_url( $redirect_uri );
} else {
$authorize_url = $this->connection->get_authorization_url( null, $redirect_uri );
}
$authorize_url = ( new Authorize_Redirect( $this->connection ) )->build_authorize_url( $redirect_uri );

/**
* Filters the response of jetpack/v4/connection/register endpoint
Expand Down
7 changes: 4 additions & 3 deletions projects/packages/connection/src/sso/class-sso.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
use Automattic\Jetpack\Connection\SSO\Helpers;
use Automattic\Jetpack\Connection\SSO\Notices;
use Automattic\Jetpack\Connection\SSO\User_Admin;
use Automattic\Jetpack\Connection\Webhooks\Authorize_Redirect;
use Automattic\Jetpack\Roles;
use Automattic\Jetpack\Status;
use Automattic\Jetpack\Status\Host;
Expand Down Expand Up @@ -1168,8 +1169,6 @@ public static function get_user_by_wpcom_id( $wpcom_user_id ) {
* calls menu_page_url() which doesn't work properly until admin menus are registered.
*/
public function maybe_authorize_user_after_sso() {
$jetpack = Jetpack::init();

if ( empty( $_GET['jetpack-sso-auth-redirect'] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended
return;
}
Expand All @@ -1194,7 +1193,9 @@ public function maybe_authorize_user_after_sso() {
*/
remove_all_filters( 'jetpack_use_iframe_authorization_flow' );
Copy link
Contributor

Choose a reason for hiding this comment

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

It's been a while since we removed the iframe flow, and looks like the filter is no longer there.
We probably could remove these hook calls as well.

add_filter( 'jetpack_use_iframe_authorization_flow', '__return_false' );
$connect_url = $jetpack->build_connect_url( true, $redirect_after_auth, 'sso' );

$connection = new Manager( 'jetpack-connection' );
$connect_url = ( new Authorize_Redirect( $connection ) )->build_authorize_url( $redirect_after_auth, 'sso', true );

add_filter( 'allowed_redirect_hosts', array( Helpers::class, 'allowed_redirect_hosts' ) );
wp_safe_redirect( $connect_url );
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
use Automattic\Jetpack\Admin_UI\Admin_Menu;
use Automattic\Jetpack\Constants;
use Automattic\Jetpack\Licensing;
use Automattic\Jetpack\Status\Host;
use Automattic\Jetpack\Tracking;
use GP_Locales;
use Jetpack_Network;
Expand Down Expand Up @@ -97,44 +98,57 @@ function ( $domains ) {
}

/**
* Create the Jetpack authorization URL. Copied from Jetpack class.
* Create the Jetpack authorization URL.
*
* @since $$next-version$$ Added optional $from and $raw parameters.
*
* @param bool|string $redirect URL to redirect to.
* @param bool|string $from If not false, adds 'from=$from' param to the connect URL.
* @param bool $raw If true, URL will not be escaped.
*
* @todo Update default value for redirect since the called function expects a string.
*
* @return mixed|void
*/
public function build_authorize_url( $redirect = false ) {
public function build_authorize_url( $redirect = false, $from = false, $raw = false ) {
Copy link
Member Author

Choose a reason for hiding this comment

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

At this point I'm wondering if build_authorize_url shouldn't be merged into get_authorization_url, but I'm not familiar enough with this to be confident that change can be done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Those filters have been there for a while, and I do wonder if we actually need them. And if we do, why we use them in some use cases, but not in others.
So I guess we could mark this as tech debt and return to this later.


add_filter( 'jetpack_connect_request_body', array( __CLASS__, 'filter_connect_request_body' ) );
add_filter( 'jetpack_connect_redirect_url', array( __CLASS__, 'filter_connect_redirect_url' ) );

$url = $this->connection->get_authorization_url( wp_get_current_user(), $redirect );
$url = $this->connection->get_authorization_url( wp_get_current_user(), $redirect, $from, $raw );

remove_filter( 'jetpack_connect_request_body', array( __CLASS__, 'filter_connect_request_body' ) );
remove_filter( 'jetpack_connect_redirect_url', array( __CLASS__, 'filter_connect_redirect_url' ) );

/** This filter is documented in plugins/jetpack/class-jetpack.php */
return apply_filters( 'jetpack_build_authorize_url', $url );
/**
* Filter the URL used when authorizing a user to a WordPress.com account.
*
* @since jetpack-8.9.0
* @since $$next-version$$ Added $raw parameter.
*
* @param string $url Connection URL.
* @param bool $raw If true, URL will not be escaped.
*/
return apply_filters( 'jetpack_build_authorize_url', $url, $raw );
}

/**
* Filters the redirection URL that is used for connect requests. The redirect
* URL should return the user back to the Jetpack console.
* Copied from Jetpack class.
* URL should return the user back to the My Jetpack page.
*
* @param String $redirect the default redirect URL used by the package.
* @return String the modified URL.
* @param string $redirect the default redirect URL used by the package.
* @return string the modified URL.
*/
public static function filter_connect_redirect_url( $redirect ) {
$jetpack_admin_page = esc_url_raw( admin_url( 'admin.php?page=jetpack' ) );
$jetpack_admin_page = esc_url_raw( admin_url( 'admin.php?page=my-jetpack' ) );
jeherve marked this conversation as resolved.
Show resolved Hide resolved
$redirect = $redirect
? wp_validate_redirect( esc_url_raw( $redirect ), $jetpack_admin_page )
: $jetpack_admin_page;

// phpcs:ignore WordPress.Security.NonceVerification.Recommended
if ( isset( $_REQUEST['is_multisite'] ) ) {
if (
class_exists( 'Jetpack_Network' )
&& isset( $_REQUEST['is_multisite'] ) // phpcs:ignore WordPress.Security.NonceVerification.Recommended
) {
$redirect = Jetpack_Network::init()->get_url( 'network_admin_page' );
}

Expand All @@ -143,7 +157,6 @@ public static function filter_connect_redirect_url( $redirect ) {

/**
* Filters the connection URL parameter array.
* Copied from Jetpack class.
*
* @param array $args default URL parameters used by the package.
* @return array the modified URL arguments array.
Expand All @@ -170,7 +183,7 @@ public static function filter_connect_request_body( $args ) {
)
);

$calypso_env = self::get_calypso_env();
$calypso_env = ( new Host() )->get_calypso_env();

if ( ! empty( $calypso_env ) ) {
$args['calypso_env'] = $calypso_env;
Expand All @@ -184,25 +197,15 @@ public static function filter_connect_request_body( $args ) {
* it with different Calypso enrionments, such as localhost.
* Copied from Jetpack class.
*
* @deprecated $$next-version$$
*
* @since 1.37.1
*
* @return string Calypso environment
*/
public static function get_calypso_env() {
// phpcs:ignore WordPress.Security.NonceVerification.Recommended
if ( isset( $_GET['calypso_env'] ) ) {
// phpcs:ignore WordPress.Security.NonceVerification.Recommended
return sanitize_key( $_GET['calypso_env'] );
}

if ( getenv( 'CALYPSO_ENV' ) ) {
return sanitize_key( getenv( 'CALYPSO_ENV' ) );
}

if ( defined( 'CALYPSO_ENV' ) && CALYPSO_ENV ) {
return sanitize_key( CALYPSO_ENV );
}
_deprecated_function( __METHOD__, '$$next-version$$', 'Automattic\\Jetpack\\Status\\Host::get_calypso_env' );

return '';
return ( new Host() )->get_calypso_env();
}
}
5 changes: 5 additions & 0 deletions projects/packages/jitm/changelog/update-sso-connect
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Significance: patch
Type: changed
Comment: Phan config update


2 changes: 1 addition & 1 deletion projects/packages/jitm/src/class-jitm.php
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
*/
class JITM {

const PACKAGE_VERSION = '3.1.9';
const PACKAGE_VERSION = '3.1.10-alpha';

/**
* The configuration method that is called from the jetpack-config package.
Expand Down
4 changes: 4 additions & 0 deletions projects/plugins/jetpack/changelog/update-sso-connect
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: patch
Type: other

Janitorial: deprecate methods in Jetpack class in favor of methods from the Connection package.
Loading
Loading