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.

Sign up for GitHub

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

Update validation for analytics shared requests #5727

Merged
merged 9 commits into from
Aug 24, 2022
Merged

Update validation for analytics shared requests #5727

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7339288
Add exception classes for invalid metrics and dimensions.
nfmohit Aug 23, 2022
ed28b0e
Update validation methods to use new exception classes.
nfmohit Aug 23, 2022
e1d9a2b
Change shared credentials usage property only for the current request.
nfmohit Aug 23, 2022
f0d5cd0
Update comment to be more useful.
nfmohit Aug 23, 2022
97474ec
Change namespaces for the new exception classes.
nfmohit Aug 23, 2022
d935f65
Update params to expect an array of the appropriate instance.
nfmohit Aug 23, 2022
89771bf
Refactor validation exceptions.
aaemnnosttv Aug 24, 2022
a89ee09
Fix dimensions referenced in exception.
aaemnnosttv Aug 24, 2022
dbd3d9e
Fix docblocks for exceptions.
aaemnnosttv Aug 24, 2022
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
3 changes: 3 additions & 0 deletions includes/Core/Modules/Module.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,9 @@ final protected function execute_data_request( Data_Request $data ) {
$datapoint = $this->get_datapoint_definition( "{$data->method}:{$data->datapoint}" );
$oauth_client = $this->get_oauth_client_for_datapoint( $datapoint );

// Set request as using shared credentials to false.
aaemnnosttv marked this conversation as resolved.
Show resolved Hide resolved
$this->is_using_shared_credentials = false;

$this->validate_datapoint_scopes( $datapoint, $oauth_client );
$this->validate_base_scopes( $oauth_client );

Expand Down
75 changes: 75 additions & 0 deletions includes/Core/REST_API/Exception/Invalid_Dimensions_Exception.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
<?php
/**
* Class Google\Site_Kit\Core\REST_API\Exception\Invalid_Dimensions_Exception
*
* @package Google\Site_Kit\Core\REST_API\Exception
* @copyright 2022 Google LLC
* @license https://www.apache.org/licenses/LICENSE-2.0 Apache License 2.0
* @link https://sitekit.withgoogle.com
*/

namespace Google\Site_Kit\Core\REST_API\Exception;
aaemnnosttv marked this conversation as resolved.
Show resolved Hide resolved

use Google\Site_Kit\Core\Contracts\WP_Errorable;
use Exception;
use WP_Error;

/**
* Exception thrown when dimensions are invalid for a report request.
*
* @since n.e.x.t
* @access private
* @ignore
*/
class Invalid_Dimensions_Exception extends Exception implements WP_Errorable {
aaemnnosttv marked this conversation as resolved.
Show resolved Hide resolved

const WP_ERROR_CODE = 'invalid_dimensions';

/**
* Constructor.
*
* @since n.e.x.t
*
* @param string $message Optional. Exception message.
* @param int $code Optional. Exception code.
* @param Throwable $previous Optional. Previous exception used for chaining.
* @param array $dimensions Optional. Dimensions that are invalid.
*/
public function __construct( $message = '', $code = 0, $previous = null, $dimensions = array() ) {
if ( ! isset( $message ) || empty( $message ) ) {
$message = sprintf(
/* translators: %s is replaced with a comma separated list of the invalid dimensions. */
_n(
'Unsupported dimension requested: %s',
'Unsupported dimensions requested: %s',
count( $dimensions ),
'google-site-kit'
),
join(
/* translators: used between list items, there is a space after the comma. */
__( ', ', 'google-site-kit' ),
$dimensions
)
);
}

parent::__construct( $message, $code, $previous );
}

/**
* Gets the WP_Error representation of this exception.
*
* @since n.e.x.t
*
* @return WP_Error
*/
public function to_wp_error() {
return new WP_Error(
static::WP_ERROR_CODE,
$this->getMessage(),
array(
'status' => 400, // Bad request.
)
);
}
}
75 changes: 75 additions & 0 deletions includes/Core/REST_API/Exception/Invalid_Metrics_Exception.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
<?php
/**
* Class Google\Site_Kit\Core\REST_API\Exception\Invalid_Metrics_Exception
*
* @package Google\Site_Kit\Core\REST_API\Exception
* @copyright 2022 Google LLC
* @license https://www.apache.org/licenses/LICENSE-2.0 Apache License 2.0
* @link https://sitekit.withgoogle.com
*/

namespace Google\Site_Kit\Core\REST_API\Exception;

use Google\Site_Kit\Core\Contracts\WP_Errorable;
use Exception;
use WP_Error;

/**
* Exception thrown when metrics are invalid for a report request.
*
* @since n.e.x.t
* @access private
* @ignore
*/
class Invalid_Metrics_Exception extends Exception implements WP_Errorable {

const WP_ERROR_CODE = 'invalid_metrics';

/**
* Constructor.
*
* @since n.e.x.t
*
* @param string $message Optional. Exception message.
* @param int $code Optional. Exception code.
* @param Throwable $previous Optional. Previous exception used for chaining.
* @param array $metrics Optional. Metrics that are invalid.
*/
public function __construct( $message = '', $code = 0, $previous = null, $metrics = array() ) {
if ( ! isset( $message ) || empty( $message ) ) {
$message = sprintf(
/* translators: %s is replaced with a comma separated list of the invalid metrics. */
_n(
'Unsupported metric requested: %s',
'Unsupported metrics requested: %s',
count( $metrics ),
'google-site-kit'
),
join(
/* translators: used between list items, there is a space after the comma. */
__( ', ', 'google-site-kit' ),
$metrics
)
);
}

parent::__construct( $message, $code, $previous );
}

/**
* Gets the WP_Error representation of this exception.
*
* @since n.e.x.t
*
* @return WP_Error
*/
public function to_wp_error() {
return new WP_Error(
static::WP_ERROR_CODE,
$this->getMessage(),
array(
'status' => 400, // Bad request.
)
);
}
}
86 changes: 26 additions & 60 deletions includes/Modules/Analytics.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
use Google\Site_Kit\Core\Modules\Module_With_Owner;
use Google\Site_Kit\Core\Modules\Module_With_Owner_Trait;
use Google\Site_Kit\Core\REST_API\Exception\Invalid_Datapoint_Exception;
use Google\Site_Kit\Core\REST_API\Exception\Invalid_Metrics_Exception;
use Google\Site_Kit\Core\REST_API\Exception\Invalid_Dimensions_Exception;
use Google\Site_Kit\Core\Authentication\Google_Proxy;
use Google\Site_Kit\Core\Assets\Asset;
use Google\Site_Kit\Core\Assets\Script;
Expand Down Expand Up @@ -543,22 +545,7 @@ function ( $dimension_def ) {
);

if ( ! empty( $dimensions ) ) {
$invalid_dimensions_error_message = $this->validate_report_dimensions(
array_map(
function ( $dimension_def ) {
return $dimension_def->getName();
},
$dimensions
)
);

if ( isset( $invalid_dimensions_error_message ) ) {
return new WP_Error(
'invalid_dimensions',
$invalid_dimensions_error_message,
array( 'status' => 400 )
Copy link
Collaborator

Choose a reason for hiding this comment

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

As mentioned before, 400 isn't really the proper status here. We can keep this as the default 500 as this is more of an "internal server" error than something wrong with the request itself, even though it is directly related to the requested data. The only reason it's an error is due to the internal server state related to sharing.

);
}
$this->validate_report_dimensions( $dimensions );
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this was mostly correct before. The only change to really make is adding a try/catch around the validation and returning the appropriate WP_Error.


$request_args['dimensions'] = $dimensions;
}
Expand Down Expand Up @@ -658,22 +645,7 @@ function ( $metric_def ) {
);

if ( ! empty( $metrics ) ) {
$invalid_metrics_error_message = $this->validate_report_metrics(
array_map(
function ( $metric_def ) {
return $metric_def->getExpression();
},
$metrics
)
);

if ( isset( $invalid_metrics_error_message ) ) {
return new WP_Error(
'invalid_metrics',
$invalid_metrics_error_message,
array( 'status' => 400 )
);
}
$this->validate_report_metrics( $metrics );

$request->setMetrics( $metrics );
}
Expand Down Expand Up @@ -1372,7 +1344,7 @@ public function check_service_entity_access() {
*
* @param array $metrics The metrics to validate.
aaemnnosttv marked this conversation as resolved.
Show resolved Hide resolved
*
* @return null|WP_Error NULL if metrics are valid, otherwise WP_Error.
* @throws Invalid_Metrics_Exception Thrown if the metrics are invalid.
*/
protected function validate_report_metrics( $metrics ) {
if ( false === $this->is_using_shared_credentials ) {
Expand All @@ -1395,22 +1367,19 @@ protected function validate_report_metrics( $metrics ) {
)
);

$invalid_metrics = array_diff( $metrics, $valid_metrics );
$invalid_metrics = array_diff(
array_map(
function ( $metric ) {
return $metric->getExpression();
},
$metrics
),
$valid_metrics
);

if ( count( $invalid_metrics ) > 0 ) {
return sprintf(
/* translators: %s is replaced with a comma separated list of the invalid metrics. */
_n(
'Unsupported metric requested: %s',
'Unsupported metrics requested: %s',
count( $invalid_metrics ),
'google-site-kit'
),
implode( ', ', $invalid_metrics )
);
throw new Invalid_Metrics_Exception( '', 0, null, $invalid_metrics );
Copy link
Collaborator

Choose a reason for hiding this comment

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

As mentioned before, let's give the message to the exception rather than have it generate its own. This makes the instantiation much simpler as well as we don't need to provide it with the metrics (although maybe this would make sense to do in the future), it's not needed yet.

}

return null;
}

/**
Expand All @@ -1420,7 +1389,7 @@ protected function validate_report_metrics( $metrics ) {
*
* @param array $dimensions The dimensions to validate.
aaemnnosttv marked this conversation as resolved.
Show resolved Hide resolved
*
* @return null|WP_Error NULL if dimensions are valid, otherwise WP_Error.
* @throws Invalid_Dimensions_Exception Thrown if the dimensions are invalid.
*/
protected function validate_report_dimensions( $dimensions ) {
if ( false === $this->is_using_shared_credentials ) {
Expand All @@ -1440,22 +1409,19 @@ protected function validate_report_dimensions( $dimensions ) {
)
);

$invalid_dimensions = array_diff( $dimensions, $valid_dimensions );
$invalid_dimensions = array_diff(
array_map(
function ( $dimension ) {
return $dimension->getName();
},
$dimensions
),
$valid_dimensions
);

if ( count( $invalid_dimensions ) > 0 ) {
return sprintf(
/* translators: %s is replaced with a comma separated list of the invalid dimensions. */
_n(
'Unsupported dimension requested: %s',
'Unsupported dimensions requested: %s',
count( $invalid_dimensions ),
'google-site-kit'
),
implode( ', ', $invalid_dimensions )
);
throw new Invalid_Dimensions_Exception( '', 0, null, $invalid_dimensions );
}

return null;
}

}