Skip to content

Commit 3893ec2

Browse files
tmacrotmacro
committed
impr(CLDSRV-621): Add tests for bucketOwnerId
1 parent fe77871 commit 3893ec2

File tree

5 files changed

+295
-2
lines changed

5 files changed

+295
-2
lines changed

‎tests/unit/DummyRequest.js‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,11 @@ class DummyRequest extends http.IncomingMessage {
2626
}
2727
this.push(null);
2828
}
29+
30+
_destroy(err, cb) {
31+
// this is a no-op
32+
cb();
33+
}
2934
}
3035

3136
module.exports = DummyRequest;

‎tests/unit/api/multipartUpload.js‎

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ const log = new DummyRequestLogger();
3838
const splitter = constants.splitter;
3939
const canonicalID = 'accessKey1';
4040
const authInfo = makeAuthInfo(canonicalID);
41+
const authInfoOtherAcc = makeAuthInfo('accessKey2');
4142
const namespace = 'default';
4243
const bucketName = 'bucketname';
4344
const lockedBucket = 'objectlockenabledbucket';
@@ -2661,9 +2662,14 @@ describe('complete mpu with bucket policy', () => {
26612662

26622663
beforeEach(done => {
26632664
cleanup();
2665+
sinon.spy(metadataswitch, 'putObjectMD');
26642666
bucketPut(authInfo, bucketPutRequest, log, done);
26652667
});
26662668

2669+
afterEach(() => {
2670+
sinon.restore();
2671+
});
2672+
26672673
it('should complete with a deny on unrelated object as non root', done => {
26682674
const bucketPutPolicyRequest = getPolicyRequest({
26692675
Version: '2012-10-17',
@@ -2730,6 +2736,96 @@ describe('complete mpu with bucket policy', () => {
27302736
done();
27312737
});
27322738
});
2739+
2740+
it('should set bucketOwnerId if requester is not destination bucket owner', done => {
2741+
const partBody = Buffer.from('I am a part\n', 'utf8');
2742+
const bucketPutPolicyRequest = getPolicyRequest({
2743+
Version: '2012-10-17',
2744+
Statement: [
2745+
{
2746+
Effect: 'Allow',
2747+
Principal: { AWS: `arn:aws:iam::${authInfoOtherAcc.shortid}:root` },
2748+
Action: ['s3:*'],
2749+
Resource: `arn:aws:s3:::${bucketName}/*`,
2750+
},
2751+
],
2752+
});
2753+
async.waterfall([
2754+
next => bucketPutPolicy(authInfo, bucketPutPolicyRequest, log, next),
2755+
(corsHeaders, next) => initiateMultipartUpload(authInfoOtherAcc,
2756+
initiateRequest, log, next),
2757+
(result, corsHeaders, next) => parseString(result, next),
2758+
],
2759+
(err, json) => {
2760+
// Need to build request in here since do not have uploadId
2761+
// until here
2762+
assert.ifError(err);
2763+
const testUploadId =
2764+
json.InitiateMultipartUploadResult.UploadId[0];
2765+
const md5Hash = crypto.createHash('md5').update(partBody);
2766+
const calculatedHash = md5Hash.digest('hex');
2767+
const partRequest = new DummyRequest({
2768+
bucketName,
2769+
namespace,
2770+
objectKey,
2771+
headers: { host: `${bucketName}.s3.amazonaws.com` },
2772+
url: `/${objectKey}?partNumber=1&uploadId=${testUploadId}`,
2773+
query: {
2774+
partNumber: '1',
2775+
uploadId: testUploadId,
2776+
},
2777+
// Note that the body of the post set in the request here does
2778+
// not really matter in this test.
2779+
// The put is not going through the route so the md5 is being
2780+
// calculated above and manually being set in the request below.
2781+
// What is being tested is that the calculatedHash being sent
2782+
// to the API for the part is stored and then used to
2783+
// calculate the final ETag upon completion
2784+
// of the multipart upload.
2785+
calculatedHash,
2786+
socket: {
2787+
remoteAddress: '1.1.1.1',
2788+
},
2789+
}, partBody);
2790+
objectPutPart(authInfoOtherAcc, partRequest, undefined, log, err => {
2791+
assert.ifError(err);
2792+
const completeBody = '<CompleteMultipartUpload>' +
2793+
'<Part>' +
2794+
'<PartNumber>1</PartNumber>' +
2795+
`<ETag>"${calculatedHash}"</ETag>` +
2796+
'</Part>' +
2797+
'</CompleteMultipartUpload>';
2798+
const completeRequest = {
2799+
bucketName,
2800+
namespace,
2801+
objectKey,
2802+
parsedHost: 's3.amazonaws.com',
2803+
url: `/${objectKey}?uploadId=${testUploadId}`,
2804+
headers: { host: `${bucketName}.s3.amazonaws.com` },
2805+
query: { uploadId: testUploadId },
2806+
post: completeBody,
2807+
actionImplicitDenies: false,
2808+
socket: {
2809+
remoteAddress: '1.1.1.1',
2810+
},
2811+
};
2812+
completeMultipartUpload(authInfoOtherAcc,
2813+
completeRequest, log, err => {
2814+
assert.ifError(err);
2815+
sinon.assert.calledWith(
2816+
metadataswitch.putObjectMD.lastCall,
2817+
bucketName,
2818+
objectKey,
2819+
sinon.match({ bucketOwnerId: authInfo.canonicalId }),
2820+
sinon.match.any,
2821+
sinon.match.any,
2822+
sinon.match.any
2823+
);
2824+
done();
2825+
});
2826+
});
2827+
});
2828+
});
27332829
});
27342830

27352831
describe('multipart upload in ingestion bucket', () => {

‎tests/unit/api/objectCopy.js‎

Lines changed: 95 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ const sinon = require('sinon');
55

66
const { bucketPut } = require('../../../lib/api/bucketPut');
77
const bucketPutVersioning = require('../../../lib/api/bucketPutVersioning');
8+
const bucketPutPolicy = require('../../../lib/api/bucketPutPolicy');
89
const objectPut = require('../../../lib/api/objectPut');
910
const objectCopy = require('../../../lib/api/objectCopy');
1011
const DummyRequest = require('../DummyRequest');
@@ -45,6 +46,7 @@ function _createObjectCopyRequest(destBucketName) {
4546
objectKey,
4647
headers: {},
4748
url: `/${destBucketName}/${objectKey}`,
49+
socket: {},
4850
};
4951
return new DummyRequest(params);
5052
}
@@ -68,6 +70,7 @@ describe('objectCopy with versioning', () => {
6870

6971
before(done => {
7072
cleanup();
73+
sinon.spy(metadata, 'putObjectMD');
7174
async.series([
7275
callback => bucketPut(authInfo, putDestBucketRequest, log,
7376
callback),
@@ -96,7 +99,10 @@ describe('objectCopy with versioning', () => {
9699
});
97100
});
98101

99-
after(() => cleanup());
102+
after(() => {
103+
metadata.putObjectMD.restore();
104+
cleanup();
105+
});
100106

101107
it('should delete null version when creating new null version, ' +
102108
'even when null version is not the latest version', done => {
@@ -125,6 +131,94 @@ describe('objectCopy with versioning', () => {
125131
});
126132
});
127133
});
134+
135+
it('should not set bucketOwnerId if requesting account owns dest bucket', done => {
136+
const testObjectCopyRequest = _createObjectCopyRequest(destBucketName);
137+
objectCopy(authInfo, testObjectCopyRequest, sourceBucketName, objectKey,
138+
undefined, log, err => {
139+
assert.ifError(err);
140+
sinon.assert.calledWith(
141+
metadata.putObjectMD.lastCall,
142+
destBucketName,
143+
objectKey,
144+
sinon.match({ _data: { bucketOwnerId: sinon.match.typeOf('undefined') }}),
145+
sinon.match.any,
146+
sinon.match.any,
147+
sinon.match.any
148+
);
149+
done();
150+
});
151+
});
152+
153+
// TODO: S3C-9965
154+
// Skipped because the policy is not checked correctly
155+
// When source bucket policy is checked destination arn is used
156+
it.skip('should set bucketOwnerId if requesting account differs from dest bucket owner', done => {
157+
const authInfo2 = makeAuthInfo('accessKey2');
158+
const testObjectCopyRequest = _createObjectCopyRequest(destBucketName);
159+
const testPutSrcPolicyRequest = new DummyRequest({
160+
bucketName: sourceBucketName,
161+
namespace,
162+
headers: { host: `${sourceBucketName}.s3.amazonaws.com` },
163+
url: '/',
164+
socket: {},
165+
post: JSON.stringify({
166+
Version: '2012-10-17',
167+
Statement: [
168+
{
169+
Sid: 'AllowCrossAccountRead',
170+
Effect: 'Allow',
171+
Principal: { AWS: `arn:aws:iam::${authInfo2.shortid}:root` },
172+
Action: ['s3:GetObject'],
173+
Resource: [
174+
`arn:aws:s3:::${sourceBucketName}/*`
175+
],
176+
},
177+
],
178+
}),
179+
});
180+
const testPutDestPolicyRequest = new DummyRequest({
181+
bucketName: destBucketName,
182+
namespace,
183+
headers: { host: `${destBucketName}.s3.amazonaws.com` },
184+
url: '/',
185+
socket: {},
186+
post: JSON.stringify({
187+
Version: '2012-10-17',
188+
Statement: [
189+
{
190+
Sid: 'AllowCrossAccountWrite',
191+
Effect: 'Allow',
192+
Principal: { AWS: `arn:aws:iam::${authInfo2.shortid}:root` },
193+
Action: ['s3:PutObject'],
194+
Resource: [
195+
`arn:aws:s3:::${destBucketName}/*`
196+
],
197+
},
198+
],
199+
}),
200+
});
201+
bucketPutPolicy(authInfo, testPutSrcPolicyRequest, log, err => {
202+
assert.ifError(err);
203+
bucketPutPolicy(authInfo, testPutDestPolicyRequest, log, err => {
204+
assert.ifError(err);
205+
objectCopy(authInfo2, testObjectCopyRequest, sourceBucketName, objectKey,
206+
undefined, log, err => {
207+
sinon.assert.calledWith(
208+
metadata.putObjectMD.lastCall,
209+
destBucketName,
210+
objectKey,
211+
sinon.match({_data: { bucketOwnerId: authInfo.canonicalID }}),
212+
sinon.match.any,
213+
sinon.match.any,
214+
sinon.match.any
215+
);
216+
assert.ifError(err);
217+
done();
218+
});
219+
});
220+
});
221+
});
128222
});
129223

130224
describe('non-versioned objectCopy', () => {

‎tests/unit/api/objectCopyPart.js‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,4 +137,21 @@ describe('objectCopyPart', () => {
137137
done();
138138
});
139139
});
140+
141+
it('should set owner-id to the canonicalId of the dest bucket owner', done => {
142+
const testObjectCopyRequest = _createObjectCopyPartRequest(destBucketName, uploadId);
143+
objectPutCopyPart(authInfo, testObjectCopyRequest, sourceBucketName, objectKey, undefined, log, err => {
144+
assert.ifError(err);
145+
sinon.assert.calledWith(
146+
metadataswitch.putObjectMD.lastCall,
147+
sinon.match.string, // MPU shadow bucket
148+
`${uploadId}..|..00001`,
149+
sinon.match({ 'owner-id': authInfo.canonicalID }),
150+
sinon.match.any,
151+
sinon.match.any,
152+
sinon.match.any
153+
);
154+
done();
155+
});
156+
});
140157
});

‎tests/unit/api/objectPut.js‎

Lines changed: 82 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ const { bucketPut } = require('../../../lib/api/bucketPut');
88
const bucketPutObjectLock = require('../../../lib/api/bucketPutObjectLock');
99
const bucketPutACL = require('../../../lib/api/bucketPutACL');
1010
const bucketPutVersioning = require('../../../lib/api/bucketPutVersioning');
11+
const bucketPutPolicy = require('../../../lib/api/bucketPutPolicy');
1112
const { parseTagFromQuery } = s3middleware.tagging;
1213
const { cleanup, DummyRequestLogger, makeAuthInfo, versioningTestUtils }
1314
= require('../helpers');
@@ -582,7 +583,7 @@ describe('objectPut API', () => {
582583
data.implName = 'multipleBackends';
583584

584585
const originalPut = data.client.put;
585-
data.client.put = (hashedStream, valueSize, keyContext, backendInfo, log, cb) =>
586+
data.client.put = (hashedStream, valueSize, keyContext, backendInfo, log, cb) =>
586587
cb({ httpCode: 408 });
587588
bucketPut(authInfo, testPutBucketRequest, log, () => {
588589
objectPut(authInfo, testPutObjectRequest, undefined, log,
@@ -762,6 +763,86 @@ describe('objectPut API', () => {
762763
},
763764
], done);
764765
});
766+
767+
it('should not set bucketOwnerId if requester owns the bucket', done => {
768+
const testPutObjectRequest = new DummyRequest({
769+
bucketName,
770+
namespace,
771+
objectKey: objectName,
772+
headers: {},
773+
url: `/${bucketName}/${objectName}`,
774+
calculatedHash: 'vnR+tLdVF79rPPfF+7YvOg==',
775+
}, postBody);
776+
777+
bucketPut(authInfo, testPutBucketRequest, log, () => {
778+
objectPut(authInfo, testPutObjectRequest, undefined, log,
779+
err => {
780+
assert.ifError(err);
781+
sinon.assert.calledWith(metadata.putObjectMD.lastCall,
782+
bucketName,
783+
objectName,
784+
sinon.match({ bucketOwnerId: sinon.match.typeOf('undefined') }),
785+
any,
786+
any,
787+
any
788+
);
789+
done();
790+
}
791+
);
792+
});
793+
});
794+
795+
it('should set bucketOwnerId if requester does not own the bucket', done => {
796+
const authInfo2 = makeAuthInfo('accessKey2');
797+
798+
const testPutObjectRequest = new DummyRequest({
799+
bucketName,
800+
namespace,
801+
objectKey: objectName,
802+
headers: {},
803+
url: `/${bucketName}/${objectName}`,
804+
calculatedHash: 'vnR+tLdVF79rPPfF+7YvOg==',
805+
}, postBody);
806+
807+
const testPutPolicyRequest = new DummyRequest({
808+
bucketName,
809+
namespace,
810+
headers: { host: `${bucketName}.s3.amazonaws.com` },
811+
url: '/',
812+
post: JSON.stringify({
813+
Version: '2012-10-17',
814+
Statement: [
815+
{
816+
Sid: 'AllowCrossAccountReadWrite',
817+
Effect: 'Allow',
818+
Principal: { AWS: `arn:aws:iam::${authInfo2.shortid}:root` },
819+
Action: ['s3:PutObject'],
820+
Resource: [`arn:aws:s3:::${bucketName}/*`],
821+
},
822+
],
823+
}),
824+
});
825+
826+
bucketPut(authInfo, testPutBucketRequest, log, () => {
827+
bucketPutPolicy(authInfo, testPutPolicyRequest, log, err => {
828+
assert.ifError(err);
829+
objectPut(authInfo, testPutObjectRequest, undefined, log,
830+
err => {
831+
assert.ifError(err);
832+
sinon.assert.calledWith(metadata.putObjectMD.lastCall,
833+
bucketName,
834+
objectName,
835+
sinon.match({ bucketOwnerId: authInfo.canonicalId }),
836+
any,
837+
any,
838+
any
839+
);
840+
done();
841+
}
842+
);
843+
});
844+
});
845+
});
765846
});
766847

767848
describe('objectPut API with versioning', () => {

0 commit comments

Comments
 (0)