Skip to content

Commit b207c84

Browse files
bert-ebert-e
committed
Merge branch 'improvement/CLDSRV-621/support_bucket_owner_id' into tmp/octopus/w/9.1/improvement/CLDSRV-621/support_bucket_owner_id
2 parents 63fce68 + 915946a commit b207c84

File tree

9 files changed

+217
-4
lines changed

9 files changed

+217
-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/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)