Skip to content

Commit 861a69b

Browse files
tmacroBourgoisMickael
tmacro
authored and
BourgoisMickael
committed
impr(CLDSRV-621): Add tests for bucketOwnerId
(cherry picked from commit 3893ec2)
1 parent 3ea120e commit 861a69b

File tree

5 files changed

+294
-1
lines changed

5 files changed

+294
-1
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
@@ -37,6 +37,7 @@ const log = new DummyRequestLogger();
3737
const splitter = constants.splitter;
3838
const canonicalID = 'accessKey1';
3939
const authInfo = makeAuthInfo(canonicalID);
40+
const authInfoOtherAcc = makeAuthInfo('accessKey2');
4041
const namespace = 'default';
4142
const bucketName = 'bucketname';
4243
const lockedBucket = 'objectlockenabledbucket';
@@ -2309,9 +2310,14 @@ describe('complete mpu with bucket policy', () => {
23092310

23102311
beforeEach(done => {
23112312
cleanup();
2313+
sinon.spy(metadataswitch, 'putObjectMD');
23122314
bucketPut(authInfo, bucketPutRequest, log, done);
23132315
});
23142316

2317+
afterEach(() => {
2318+
sinon.restore();
2319+
});
2320+
23152321
it('should complete with a deny on unrelated object as non root', done => {
23162322
const bucketPutPolicyRequest = getPolicyRequest({
23172323
Version: '2012-10-17',
@@ -2378,4 +2384,94 @@ describe('complete mpu with bucket policy', () => {
23782384
done();
23792385
});
23802386
});
2387+
2388+
it('should set bucketOwnerId if requester is not destination bucket owner', done => {
2389+
const partBody = Buffer.from('I am a part\n', 'utf8');
2390+
const bucketPutPolicyRequest = getPolicyRequest({
2391+
Version: '2012-10-17',
2392+
Statement: [
2393+
{
2394+
Effect: 'Allow',
2395+
Principal: { AWS: `arn:aws:iam::${authInfoOtherAcc.shortid}:root` },
2396+
Action: ['s3:*'],
2397+
Resource: `arn:aws:s3:::${bucketName}/*`,
2398+
},
2399+
],
2400+
});
2401+
async.waterfall([
2402+
next => bucketPutPolicy(authInfo, bucketPutPolicyRequest, log, next),
2403+
(corsHeaders, next) => initiateMultipartUpload(authInfoOtherAcc,
2404+
initiateRequest, log, next),
2405+
(result, corsHeaders, next) => parseString(result, next),
2406+
],
2407+
(err, json) => {
2408+
// Need to build request in here since do not have uploadId
2409+
// until here
2410+
assert.ifError(err);
2411+
const testUploadId =
2412+
json.InitiateMultipartUploadResult.UploadId[0];
2413+
const md5Hash = crypto.createHash('md5').update(partBody);
2414+
const calculatedHash = md5Hash.digest('hex');
2415+
const partRequest = new DummyRequest({
2416+
bucketName,
2417+
namespace,
2418+
objectKey,
2419+
headers: { host: `${bucketName}.s3.amazonaws.com` },
2420+
url: `/${objectKey}?partNumber=1&uploadId=${testUploadId}`,
2421+
query: {
2422+
partNumber: '1',
2423+
uploadId: testUploadId,
2424+
},
2425+
// Note that the body of the post set in the request here does
2426+
// not really matter in this test.
2427+
// The put is not going through the route so the md5 is being
2428+
// calculated above and manually being set in the request below.
2429+
// What is being tested is that the calculatedHash being sent
2430+
// to the API for the part is stored and then used to
2431+
// calculate the final ETag upon completion
2432+
// of the multipart upload.
2433+
calculatedHash,
2434+
socket: {
2435+
remoteAddress: '1.1.1.1',
2436+
},
2437+
}, partBody);
2438+
objectPutPart(authInfoOtherAcc, partRequest, undefined, log, err => {
2439+
assert.ifError(err);
2440+
const completeBody = '<CompleteMultipartUpload>' +
2441+
'<Part>' +
2442+
'<PartNumber>1</PartNumber>' +
2443+
`<ETag>"${calculatedHash}"</ETag>` +
2444+
'</Part>' +
2445+
'</CompleteMultipartUpload>';
2446+
const completeRequest = {
2447+
bucketName,
2448+
namespace,
2449+
objectKey,
2450+
parsedHost: 's3.amazonaws.com',
2451+
url: `/${objectKey}?uploadId=${testUploadId}`,
2452+
headers: { host: `${bucketName}.s3.amazonaws.com` },
2453+
query: { uploadId: testUploadId },
2454+
post: completeBody,
2455+
actionImplicitDenies: false,
2456+
socket: {
2457+
remoteAddress: '1.1.1.1',
2458+
},
2459+
};
2460+
completeMultipartUpload(authInfoOtherAcc,
2461+
completeRequest, log, err => {
2462+
assert.ifError(err);
2463+
sinon.assert.calledWith(
2464+
metadataswitch.putObjectMD.lastCall,
2465+
bucketName,
2466+
objectKey,
2467+
sinon.match({ bucketOwnerId: authInfo.canonicalId }),
2468+
sinon.match.any,
2469+
sinon.match.any,
2470+
sinon.match.any
2471+
);
2472+
done();
2473+
});
2474+
});
2475+
});
2476+
});
23812477
});

‎tests/unit/api/objectCopy.js‎

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

55
const { bucketPut } = require('../../../lib/api/bucketPut');
66
const bucketPutVersioning = require('../../../lib/api/bucketPutVersioning');
7+
const bucketPutPolicy = require('../../../lib/api/bucketPutPolicy');
78
const objectPut = require('../../../lib/api/objectPut');
89
const objectCopy = require('../../../lib/api/objectCopy');
910
const { ds } = require('arsenal').storage.data.inMemory.datastore;
@@ -40,6 +41,7 @@ function _createObjectCopyRequest(destBucketName) {
4041
objectKey,
4142
headers: {},
4243
url: `/${destBucketName}/${objectKey}`,
44+
socket: {},
4345
};
4446
return new DummyRequest(params);
4547
}
@@ -63,6 +65,7 @@ describe('objectCopy with versioning', () => {
6365

6466
before(done => {
6567
cleanup();
68+
sinon.spy(metadata, 'putObjectMD');
6669
async.series([
6770
callback => bucketPut(authInfo, putDestBucketRequest, log,
6871
callback),
@@ -91,7 +94,10 @@ describe('objectCopy with versioning', () => {
9194
});
9295
});
9396

94-
after(() => cleanup());
97+
after(() => {
98+
metadata.putObjectMD.restore();
99+
cleanup();
100+
});
95101

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

113207
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: 81 additions & 0 deletions
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');
@@ -634,6 +635,86 @@ describe('objectPut API', () => {
634635
});
635636
});
636637
});
638+
639+
it('should not set bucketOwnerId if requester owns the bucket', done => {
640+
const testPutObjectRequest = new DummyRequest({
641+
bucketName,
642+
namespace,
643+
objectKey: objectName,
644+
headers: {},
645+
url: `/${bucketName}/${objectName}`,
646+
calculatedHash: 'vnR+tLdVF79rPPfF+7YvOg==',
647+
}, postBody);
648+
649+
bucketPut(authInfo, testPutBucketRequest, log, () => {
650+
objectPut(authInfo, testPutObjectRequest, undefined, log,
651+
err => {
652+
assert.ifError(err);
653+
sinon.assert.calledWith(metadata.putObjectMD.lastCall,
654+
bucketName,
655+
objectName,
656+
sinon.match({ bucketOwnerId: sinon.match.typeOf('undefined') }),
657+
any,
658+
any,
659+
any
660+
);
661+
done();
662+
}
663+
);
664+
});
665+
});
666+
667+
it('should set bucketOwnerId if requester does not own the bucket', done => {
668+
const authInfo2 = makeAuthInfo('accessKey2');
669+
670+
const testPutObjectRequest = new DummyRequest({
671+
bucketName,
672+
namespace,
673+
objectKey: objectName,
674+
headers: {},
675+
url: `/${bucketName}/${objectName}`,
676+
calculatedHash: 'vnR+tLdVF79rPPfF+7YvOg==',
677+
}, postBody);
678+
679+
const testPutPolicyRequest = new DummyRequest({
680+
bucketName,
681+
namespace,
682+
headers: { host: `${bucketName}.s3.amazonaws.com` },
683+
url: '/',
684+
post: JSON.stringify({
685+
Version: '2012-10-17',
686+
Statement: [
687+
{
688+
Sid: 'AllowCrossAccountReadWrite',
689+
Effect: 'Allow',
690+
Principal: { AWS: `arn:aws:iam::${authInfo2.shortid}:root` },
691+
Action: ['s3:PutObject'],
692+
Resource: [`arn:aws:s3:::${bucketName}/*`],
693+
},
694+
],
695+
}),
696+
});
697+
698+
bucketPut(authInfo, testPutBucketRequest, log, () => {
699+
bucketPutPolicy(authInfo, testPutPolicyRequest, log, err => {
700+
assert.ifError(err);
701+
objectPut(authInfo, testPutObjectRequest, undefined, log,
702+
err => {
703+
assert.ifError(err);
704+
sinon.assert.calledWith(metadata.putObjectMD.lastCall,
705+
bucketName,
706+
objectName,
707+
sinon.match({ bucketOwnerId: authInfo.canonicalId }),
708+
any,
709+
any,
710+
any
711+
);
712+
done();
713+
}
714+
);
715+
});
716+
});
717+
});
637718
});
638719

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

0 commit comments

Comments
 (0)