Skip to content

Commit 0f788ab

Browse files
bert-ebert-e
committed
Merge branch 'improvement/CLDSRV-621/support_bucket_owner_id' into q/9.0
2 parents 2369327 + 3893ec2 commit 0f788ab

File tree

11 files changed

+318
-4
lines changed

11 files changed

+318
-4
lines changed

‎constants.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -222,6 +222,7 @@ const constants = {
222222
'isNull',
223223
'isDeleteMarker',
224224
'x-amz-meta-scal-version-id',
225+
'bucketOwnerId',
225226
],
226227
unsupportedSignatureChecksums: new Set([
227228
'STREAMING-AWS4-HMAC-SHA256-PAYLOAD-TRAILER',

‎lib/api/apiUtils/object/createAndStoreObject.js

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -194,6 +194,10 @@ function createAndStoreObject(bucketName, bucketMD, objectKey, objMD, authInfo,
194194
metadataStoreParams.oldReplayId = objMD.uploadId;
195195
}
196196

197+
if (authInfo.getCanonicalID() != bucketMD.getOwner()) {
198+
metadataStoreParams.bucketOwnerId = bucketMD.getOwner();
199+
}
200+
197201
if (isPutVersion && location === bucketMD.getLocationConstraint() && bucketMD.isIngestionBucket()) {
198202
// When restoring to OOB bucket, we cannot force the versionId of the object written to the
199203
// backend, and it is thus not match the versionId of the ingested object. Thus we add extra

‎lib/api/completeMultipartUpload.js

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -379,6 +379,11 @@ function completeMultipartUpload(authInfo, request, log, callback) {
379379
masterKeyId: destBucket.getSseMasterKeyId(),
380380
};
381381
}
382+
383+
if (authInfo.getCanonicalID() !== destBucket.getOwner()) {
384+
metaStoreParams.bucketOwnerId = destBucket.getOwner();
385+
}
386+
382387
// if x-scal-s3-version-id header is specified, we overwrite the object/version metadata.
383388
if (isPutVersion) {
384389
const options = overwritingVersioning(objMD, metaStoreParams);

‎lib/api/objectCopy.js

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -189,6 +189,11 @@ function _prepMetadata(request, sourceObjMD, headers, sourceIsDestination,
189189
if (!storeMetadataParams.contentType) {
190190
storeMetadataParams.contentType = sourceObjMD['content-type'];
191191
}
192+
193+
if (authInfo.getCanonicalID() !== destBucketMD.getOwner()) {
194+
storeMetadataParams.bucketOwnerId = destBucketMD.getOwner();
195+
}
196+
192197
return { storeMetadataParams, sourceLocationConstraintName,
193198
backendInfoDest: backendInfoObjDest.backendInfo };
194199
}

‎lib/api/objectPutCopyPart.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -342,6 +342,7 @@ function objectPutCopyPart(authInfo, request, sourceBucket,
342342
splitter: constants.splitter,
343343
lastModified,
344344
overheadField: constants.overheadField,
345+
ownerId: destBucketMD.getOwner(),
345346
};
346347
return services.metadataStorePart(mpuBucketName,
347348
locations, metaStoreParams, log, err => {

‎lib/services.js

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,7 @@ const services = {
109109
dataStoreName, creationTime, retentionMode, retentionDate,
110110
legalHold, originOp, updateMicroVersionId, archive, oldReplayId,
111111
deleteNullKey, amzStorageClass, overheadField, needOplogUpdate,
112-
restoredEtag } = params;
112+
restoredEtag, bucketOwnerId } = params;
113113
log.trace('storing object in metadata');
114114
assert.strictEqual(typeof bucketName, 'string');
115115
const md = new ObjectMD();
@@ -274,6 +274,10 @@ const services = {
274274
md.setAcl(params.acl);
275275
}
276276

277+
if (bucketOwnerId) {
278+
md.setBucketOwnerId(bucketOwnerId);
279+
}
280+
277281
log.trace('object metadata', { omVal: md.getValue() });
278282
// If this is not the completion of a multipart upload or
279283
// the creation of a delete marker, parse the headers to
@@ -763,7 +767,7 @@ const services = {
763767
metadataStorePart(mpuBucketName, partLocations,
764768
metaStoreParams, log, cb) {
765769
assert.strictEqual(typeof mpuBucketName, 'string');
766-
const { partNumber, contentMD5, size, uploadId, lastModified, splitter, overheadField }
770+
const { partNumber, contentMD5, size, uploadId, lastModified, splitter, overheadField, ownerId }
767771
= metaStoreParams;
768772
const dateModified = typeof lastModified === 'string' ?
769773
lastModified : new Date().toJSON();
@@ -778,6 +782,7 @@ const services = {
778782
'last-modified': dateModified,
779783
'content-md5': contentMD5,
780784
'content-length': size,
785+
'owner-id': ownerId,
781786
};
782787

783788
const params = {};

‎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
});

0 commit comments

Comments
 (0)