Skip to content

Commit

Permalink
feat(s3): throw ValidationError instead of untyped errors (#33031)
Browse files Browse the repository at this point in the history 
### Issue 

`aws-s3` for #32569 

### Description of changes

Added an `UnscopedValidationError` for situations where now scope is
available. This is to be used sparsely as it's less useful for users.

### Describe any new or updated permissions being added

n/a

### Description of how you validated changes

Existing tests. Exemptions granted as this is basically a refactor of
existing code.

### Checklist
- [x] My code adheres to the [CONTRIBUTING
GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and
[DESIGN
GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache-2.0 license*
  • Loading branch information
@mrgrain
mrgrain authored Jan 21, 2025
1 parent fa6f604 commit 61e876b
Show file tree
Hide file tree
Showing 6 changed files with 95 additions and 42 deletions.
9 changes: 9 additions & 0 deletions packages/aws-cdk-lib/.eslintrc.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,13 @@ baseConfig.rules['import/no-extraneous-dependencies'] = [
}
];


// no-throw-default-error
const modules = ['aws-s3'];
baseConfig.overrides.push({
files: modules.map(m => `./${m}/lib/**`),
rules: { "@cdklabs/no-throw-default-error": ['error'] },
});


module.exports = baseConfig;
55 changes: 28 additions & 27 deletions packages/aws-cdk-lib/aws-s3/lib/bucket.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
Tokenization,
Annotations,
} from '../../core';
import { UnscopedValidationError, ValidationError } from '../../core/lib/errors';
import { CfnReference } from '../../core/lib/private/cfn-reference';
import { AutoDeleteObjectsProvider } from '../../custom-resource-handlers/dist/aws-s3/auto-delete-objects-provider.generated';
import * as cxapi from '../../cx-api';
Expand Down Expand Up @@ -869,7 +870,7 @@ export abstract class BucketBase extends Resource implements IBucket {
*/
public grantPublicAccess(keyPrefix = '*', ...allowedActions: string[]) {
if (this.disallowPublicAccess) {
throw new Error("Cannot grant public access when 'blockPublicPolicy' is enabled");
throw new ValidationError("Cannot grant public access when 'blockPublicPolicy' is enabled", this);
}

allowedActions = allowedActions.length > 0 ? allowedActions : ['s3:GetObject'];
Expand Down Expand Up @@ -982,7 +983,7 @@ export abstract class BucketBase extends Resource implements IBucket {
})).statementAdded);
if (accessControlTransition) {
if (!account) {
throw new Error('account must be specified to override ownership access control transition');
throw new ValidationError('account must be specified to override ownership access control transition', this);
}
results.push(this.addToResourcePolicy(new iam.PolicyStatement({
actions: ['s3:ObjectOwnerOverrideToBucketOwner'],
Expand Down Expand Up @@ -1987,7 +1988,7 @@ export class Bucket extends BucketBase {

const bucketName = parseBucketName(scope, attrs);
if (!bucketName) {
throw new Error('Bucket name is required');
throw new ValidationError('Bucket name is required', scope);
}
Bucket.validateBucketName(bucketName, true);

Expand Down Expand Up @@ -2151,7 +2152,7 @@ export class Bucket extends BucketBase {
}

if (errors.length > 0) {
throw new Error(`Invalid S3 bucket name (value: ${bucketName})${EOL}${errors.join(EOL)}`);
throw new UnscopedValidationError(`Invalid S3 bucket name (value: ${bucketName})${EOL}${errors.join(EOL)}`);
}
}

Expand Down Expand Up @@ -2247,7 +2248,7 @@ export class Bucket extends BucketBase {
this.enforceSSLStatement();
this.minimumTLSVersionStatement(props.minimumTLSVersion);
} else if (props.minimumTLSVersion) {
throw new Error('\'enforceSSL\' must be enabled for \'minimumTLSVersion\' to be applied');
throw new ValidationError('\'enforceSSL\' must be enabled for \'minimumTLSVersion\' to be applied', this);
}

if (props.serverAccessLogsBucket instanceof Bucket) {
Expand Down Expand Up @@ -2281,15 +2282,15 @@ export class Bucket extends BucketBase {

if (props.publicReadAccess) {
if (props.blockPublicAccess === undefined) {
throw new Error('Cannot use \'publicReadAccess\' property on a bucket without allowing bucket-level public access through \'blockPublicAccess\' property.');
throw new ValidationError('Cannot use \'publicReadAccess\' property on a bucket without allowing bucket-level public access through \'blockPublicAccess\' property.', this);
}

this.grantPublicAccess();
}

if (props.autoDeleteObjects) {
if (props.removalPolicy !== RemovalPolicy.DESTROY) {
throw new Error('Cannot use \'autoDeleteObjects\' property on a bucket without setting removal policy to \'DESTROY\'.');
throw new ValidationError('Cannot use \'autoDeleteObjects\' property on a bucket without setting removal policy to \'DESTROY\'.', this);
}

this.enableAutoDeleteObjects();
Expand Down Expand Up @@ -2409,12 +2410,12 @@ export class Bucket extends BucketBase {

// if encryption key is set, encryption must be set to KMS or DSSE.
if (encryptionType !== BucketEncryption.DSSE && encryptionType !== BucketEncryption.KMS && props.encryptionKey) {
throw new Error(`encryptionKey is specified, so 'encryption' must be set to KMS or DSSE (value: ${encryptionType})`);
throw new ValidationError(`encryptionKey is specified, so 'encryption' must be set to KMS or DSSE (value: ${encryptionType})`, this);
}

// if bucketKeyEnabled is set, encryption can not be BucketEncryption.UNENCRYPTED
if (props.bucketKeyEnabled && encryptionType === BucketEncryption.UNENCRYPTED) {
throw new Error(`bucketKeyEnabled is specified, so 'encryption' must be set to KMS, DSSE or S3 (value: ${encryptionType})`);
throw new ValidationError(`bucketKeyEnabled is specified, so 'encryption' must be set to KMS, DSSE or S3 (value: ${encryptionType})`, this);
}

if (encryptionType === BucketEncryption.UNENCRYPTED) {
Expand Down Expand Up @@ -2497,7 +2498,7 @@ export class Bucket extends BucketBase {
return { bucketEncryption };
}

throw new Error(`Unexpected 'encryptionType': ${encryptionType}`);
throw new ValidationError(`Unexpected 'encryptionType': ${encryptionType}`, this);
}

/**
Expand All @@ -2521,7 +2522,7 @@ export class Bucket extends BucketBase {
if ((rule.expiredObjectDeleteMarker)
&& (rule.expiration || rule.expirationDate || self.parseTagFilters(rule.tagFilters))) {
// ExpiredObjectDeleteMarker cannot be specified with ExpirationInDays, ExpirationDate, or TagFilters.
throw new Error('ExpiredObjectDeleteMarker cannot be specified with expiration, ExpirationDate, or TagFilters.');
throw new ValidationError('ExpiredObjectDeleteMarker cannot be specified with expiration, ExpirationDate, or TagFilters.', self);
}

if (
Expand All @@ -2534,7 +2535,7 @@ export class Bucket extends BucketBase {
rule.noncurrentVersionTransitions === undefined &&
rule.transitions === undefined
) {
throw new Error('All rules for `lifecycleRules` must have at least one of the following properties: `abortIncompleteMultipartUploadAfter`, `expiration`, `expirationDate`, `expiredObjectDeleteMarker`, `noncurrentVersionExpiration`, `noncurrentVersionsToRetain`, `noncurrentVersionTransitions`, or `transitions`');
throw new ValidationError('All rules for `lifecycleRules` must have at least one of the following properties: `abortIncompleteMultipartUploadAfter`, `expiration`, `expirationDate`, `expiredObjectDeleteMarker`, `noncurrentVersionExpiration`, `noncurrentVersionsToRetain`, `noncurrentVersionTransitions`, or `transitions`', self);
}

const x: CfnBucket.RuleProperty = {
Expand Down Expand Up @@ -2580,7 +2581,7 @@ export class Bucket extends BucketBase {
props.encryption &&
[BucketEncryption.KMS_MANAGED, BucketEncryption.DSSE_MANAGED].includes(props.encryption)
) {
throw new Error('Default bucket encryption with KMS managed or DSSE managed key is not supported for Server Access Logging target buckets');
throw new ValidationError('Default bucket encryption with KMS managed or DSSE managed key is not supported for Server Access Logging target buckets', this);
}

// When there is an encryption key exists for the server access logs bucket, grant permission to the S3 logging SP.
Expand Down Expand Up @@ -2657,7 +2658,7 @@ export class Bucket extends BucketBase {
}

if (accessControlRequiresObjectOwnership && this.objectOwnership === ObjectOwnership.BUCKET_OWNER_ENFORCED) {
throw new Error (`objectOwnership must be set to "${ObjectOwnership.OBJECT_WRITER}" when accessControl is "${this.accessControl}"`);
throw new ValidationError (`objectOwnership must be set to "${ObjectOwnership.OBJECT_WRITER}" when accessControl is "${this.accessControl}"`, this);
}

return {
Expand Down Expand Up @@ -2703,7 +2704,7 @@ export class Bucket extends BucketBase {
return undefined;
}
if (objectLockEnabled === false && objectLockDefaultRetention) {
throw new Error('Object Lock must be enabled to configure default retention settings');
throw new ValidationError('Object Lock must be enabled to configure default retention settings', this);
}

return {
Expand All @@ -2723,16 +2724,16 @@ export class Bucket extends BucketBase {
}

if (props.websiteErrorDocument && !props.websiteIndexDocument) {
throw new Error('"websiteIndexDocument" is required if "websiteErrorDocument" is set');
throw new ValidationError('"websiteIndexDocument" is required if "websiteErrorDocument" is set', this);
}

if (props.websiteRedirect && (props.websiteErrorDocument || props.websiteIndexDocument || props.websiteRoutingRules)) {
throw new Error('"websiteIndexDocument", "websiteErrorDocument" and, "websiteRoutingRules" cannot be set if "websiteRedirect" is used');
throw new ValidationError('"websiteIndexDocument", "websiteErrorDocument" and, "websiteRoutingRules" cannot be set if "websiteRedirect" is used', this);
}

const routingRules = props.websiteRoutingRules ? props.websiteRoutingRules.map<CfnBucket.RoutingRuleProperty>((rule) => {
if (rule.condition && rule.condition.httpErrorCodeReturnedEquals == null && rule.condition.keyPrefixEquals == null) {
throw new Error('The condition property cannot be an empty object');
throw new ValidationError('The condition property cannot be an empty object', this);
}

return {
Expand Down Expand Up @@ -2762,17 +2763,17 @@ export class Bucket extends BucketBase {
}

if (!props.versioned) {
throw new Error('Replication rules require versioning to be enabled on the bucket');
throw new ValidationError('Replication rules require versioning to be enabled on the bucket', this);
}
if (props.replicationRules.length > 1 && props.replicationRules.some(rule => rule.priority === undefined)) {
throw new Error('\'priority\' must be specified for all replication rules when there are multiple rules');
throw new ValidationError('\'priority\' must be specified for all replication rules when there are multiple rules', this);
}
props.replicationRules.forEach(rule => {
if (rule.replicationTimeControl && !rule.metrics) {
throw new Error('\'replicationTimeControlMetrics\' must be enabled when \'replicationTimeControl\' is enabled.');
throw new ValidationError('\'replicationTimeControlMetrics\' must be enabled when \'replicationTimeControl\' is enabled.', this);
}
if (rule.deleteMarkerReplication && rule.filter?.tags) {
throw new Error('tag filter cannot be specified when \'deleteMarkerReplication\' is enabled.');
throw new ValidationError('tag filter cannot be specified when \'deleteMarkerReplication\' is enabled.', this);
}
});

Expand Down Expand Up @@ -2846,7 +2847,7 @@ export class Bucket extends BucketBase {
if (isCrossAccount) {
Annotations.of(this).addInfo('For Cross-account S3 replication, ensure to set up permissions on source bucket using method addReplicationPolicy() ');
} else if (rule.accessControlTransition) {
throw new Error('accessControlTranslation is only supported for cross-account replication');
throw new ValidationError('accessControlTranslation is only supported for cross-account replication', this);
}

return {
Expand Down Expand Up @@ -2921,7 +2922,7 @@ export class Bucket extends BucketBase {
conditions: conditions,
}));
} else if (this.accessControl && this.accessControl !== BucketAccessControl.LOG_DELIVERY_WRITE) {
throw new Error("Cannot enable log delivery to this bucket because the bucket's ACL has been set and can't be changed");
throw new ValidationError("Cannot enable log delivery to this bucket because the bucket's ACL has been set and can't be changed", this);
} else {
this.accessControl = BucketAccessControl.LOG_DELIVERY_WRITE;
}
Expand All @@ -2937,7 +2938,7 @@ export class Bucket extends BucketBase {
const format = inventory.format ?? InventoryFormat.CSV;
const frequency = inventory.frequency ?? InventoryFrequency.WEEKLY;
if (inventory.inventoryId !== undefined && (inventory.inventoryId.length > 64 || inventoryIdValidationRegex.test(inventory.inventoryId))) {
throw new Error(`inventoryId should not exceed 64 characters and should not contain special characters except . and -, got ${inventory.inventoryId}`);
throw new ValidationError(`inventoryId should not exceed 64 characters and should not contain special characters except . and -, got ${inventory.inventoryId}`, this);
}
const id = inventory.inventoryId ?? `${this.node.id}Inventory${index}`.replace(inventoryIdValidationRegex, '').slice(-64);

Expand Down Expand Up @@ -3523,10 +3524,10 @@ export class ObjectLockRetention {
private constructor(mode: ObjectLockMode, duration: Duration) {
// https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-lock-managing.html#object-lock-managing-retention-limits
if (duration.toDays() > 365 * 100) {
throw new Error('Object Lock retention duration must be less than 100 years');
throw new UnscopedValidationError('Object Lock retention duration must be less than 100 years');
}
if (duration.toDays() < 1) {
throw new Error('Object Lock retention duration must be at least 1 day');
throw new UnscopedValidationError('Object Lock retention duration must be at least 1 day');
}

this.mode = mode;
Expand Down
13 changes: 7 additions & 6 deletions packages/aws-cdk-lib/aws-s3/lib/notifications-resource/notifications-resource.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { Construct, IConstruct } from 'constructs';
import { NotificationsResourceHandler } from './notifications-resource-handler';
import * as iam from '../../../aws-iam';
import * as cdk from '../../../core';
import { ValidationError } from '../../../core/lib/errors';
import * as cxapi from '../../../cx-api';
import { Bucket, IBucket, EventType, NotificationKeyFilter } from '../bucket';
import { BucketNotificationDestinationType, IBucketNotificationDestination } from '../destination';
Expand Down Expand Up @@ -71,7 +72,7 @@ export class BucketNotifications extends Construct {
const targetProps = target.bind(this, this.bucket);
const commonConfig: CommonConfiguration = {
Events: [event],
Filter: renderFilters(filters),
Filter: renderFilters(filters, this),
};

// if the target specifies any dependencies, add them to the custom resource.
Expand All @@ -96,7 +97,7 @@ export class BucketNotifications extends Construct {
break;

default:
throw new Error('Unsupported notification target type:' + BucketNotificationDestinationType[targetProps.type]);
throw new ValidationError('Unsupported notification target type:' + BucketNotificationDestinationType[targetProps.type], this);
}
}

Expand Down Expand Up @@ -171,7 +172,7 @@ export class BucketNotifications extends Construct {
}
}

function renderFilters(filters?: NotificationKeyFilter[]): Filter | undefined {
function renderFilters(filters: NotificationKeyFilter[], scope: BucketNotifications): Filter | undefined {
if (!filters || filters.length === 0) {
return undefined;
}
Expand All @@ -182,20 +183,20 @@ function renderFilters(filters?: NotificationKeyFilter[]): Filter | undefined {

for (const rule of filters) {
if (!rule.suffix && !rule.prefix) {
throw new Error('NotificationKeyFilter must specify `prefix` and/or `suffix`');
throw new ValidationError('NotificationKeyFilter must specify `prefix` and/or `suffix`', scope);
}

if (rule.suffix) {
if (hasSuffix) {
throw new Error('Cannot specify more than one suffix rule in a filter.');
throw new ValidationError('Cannot specify more than one suffix rule in a filter.', scope);
}
renderedRules.push({ Name: 'suffix', Value: rule.suffix });
hasSuffix = true;
}

if (rule.prefix) {
if (hasPrefix) {
throw new Error('Cannot specify more than one prefix rule in a filter.');
throw new ValidationError('Cannot specify more than one prefix rule in a filter.', scope);
}
renderedRules.push({ Name: 'prefix', Value: rule.prefix });
hasPrefix = true;
Expand Down
3 changes: 2 additions & 1 deletion packages/aws-cdk-lib/aws-s3/lib/util.ts
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { IConstruct } from 'constructs';
import { BucketAttributes } from './bucket';
import * as cdk from '../../core';
import { ValidationError } from '../../core/lib/errors';

export function parseBucketArn(construct: IConstruct, props: BucketAttributes): string {

Expand All @@ -20,7 +21,7 @@ export function parseBucketArn(construct: IConstruct, props: BucketAttributes):
});
}

throw new Error('Cannot determine bucket ARN. At least `bucketArn` or `bucketName` is needed');
throw new ValidationError('Cannot determine bucket ARN. At least `bucketArn` or `bucketName` is needed', construct);
}

export function parseBucketName(construct: IConstruct, props: BucketAttributes): string | undefined {
Expand Down
Loading

0 comments on commit 61e876b

Please sign in to comment.