-
Notifications
You must be signed in to change notification settings - Fork 50
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
Add test case to validate if anonymous delete request returns 200 #116
Conversation
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.
Overall changes look good to me. Couple of minor comments.
run/core/aws-sdk-php/quick-tests.php
Outdated
]); | ||
// Response code should be 200 | ||
if (getstatuscode($result) != HTTP_OK) | ||
throw new Exception('deleteObjects failed for ' . |
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.
could we change the exception message to,
"deleteObjects returned incorrect response " . getStatusCode($result);
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.
yes that is better.
run/core/aws-sdk-php/quick-tests.php
Outdated
if (getstatuscode($result) != HTTP_OK) | ||
throw new Exception('deleteObjects api failed for ' . | ||
$bucket); | ||
} |
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.
could we check if errors for each object is empty in the authenticated client test?
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 test case validates the case where the requesting user does not have access to the bucket they are attempting to delete objects from and should receive an 'Access Denied' error for each object they are attempting to delete rather than the generic 403 AccessDenied error response.
05aa395
to
e9d1425
Compare
Done @krisis |
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.
LGTM
The test case validates the case where the requesting user does not have access to the bucket they are attempting to delete objects from and should receive a response 200 and an 'Access Denied' error for each object they are attempting to delete rather than the generic 403 AccessDenied error response.
AWS S3 reference doc: http://docs.aws.amazon.com/AmazonS3/latest/API/multiobjectdeleteapi.html#multiobjectdeleteapi-examples
Fixes: #115