[squash] fix tests 3

lib/api/apiUtils/authorization/permissionChecks.js

@@ -229,11 +229,17 @@ function checkBucketPolicy(policy, requestType, canonicalID, arn, log) {
     return permission;
 }
 
-function isBucketAuthorized(bucket, requestType, canonicalID, arn, log) {
+function isBucketAuthorized(bucket, requestType, canonicalID, authInfo, log) {
     // Check to see if user is authorized to perform a
     // particular action on bucket based on ACLs.
     // TODO: Add IAM checks
-    if (bucket.getOwner() === canonicalID) {
+    let requesterIsNotUser = true;
+    let arn = null;
+    if (authInfo) {
+        requesterIsNotUser = !authInfo.isRequesterAnIAMUser();
+        arn = authInfo.getArn();
+    }
+    if ((bucket.getOwner() === canonicalID) && requesterIsNotUser) {
         return true;
     }
     const aclPermission = checkBucketAcls(bucket, requestType, canonicalID);
@@ -252,19 +258,28 @@ function isBucketAuthorized(bucket, requestType, canonicalID, arn, log) {
     return (aclPermission || (bucketPolicyPermission === 'allow'));
 }
 
-function isObjAuthorized(bucket, objectMD, requestType, canonicalID, arn, log) {
+function isObjAuthorized(bucket, objectMD, requestType, canonicalID, authInfo,
+log) {
     const bucketOwner = bucket.getOwner();
     if (!objectMD) {
         return false;
     }
-    if (objectMD['owner-id'] === canonicalID) {
+    let requesterIsNotUser = true;
+    let arn = null;
+    if (authInfo) {
+        requesterIsNotUser = !authInfo.isRequesterAnIAMUser();
+        arn = authInfo.getArn();
+    }
+    if (objectMD['owner-id'] === canonicalID && requesterIsNotUser) {
         return true;
     }
     // account is authorized if:
     // - requesttype is included in bucketOwnerActions and
     // - account is the bucket owner
+    // - requester is account, not user
     if (constants.bucketOwnerActions.includes(requestType)
-    && bucketOwner === canonicalID) {
+    && bucketOwner === canonicalID
+    && requesterIsNotUser) {
         return true;
     }
     const aclPermission = checkObjectAcls(bucket, objectMD, requestType,

lib/api/bucketDeleteCors.js

@@ -20,7 +20,6 @@ const requestType = 'bucketDeleteCors';
 function bucketDeleteCors(authInfo, request, log, callback) {
     const bucketName = request.bucketName;
     const canonicalID = authInfo.getCanonicalID();
-    const requestArn = authInfo.getArn();
 
     return metadata.getBucket(bucketName, log, (err, bucket) => {
         const corsHeaders = collectCorsHeaders(request.headers.origin,
@@ -34,7 +33,7 @@ function bucketDeleteCors(authInfo, request, log, callback) {
         }
         log.trace('found bucket in metadata');
 
-        if (!isBucketAuthorized(bucket, requestType, canonicalID, requestArn,
+        if (!isBucketAuthorized(bucket, requestType, canonicalID, authInfo,
         log)) {
             log.debug('access denied for user on bucket', {
                 requestType,

lib/api/bucketDeleteWebsite.js

@@ -12,7 +12,6 @@ const requestType = 'bucketDeleteWebsite';
 function bucketDeleteWebsite(authInfo, request, log, callback) {
     const bucketName = request.bucketName;
     const canonicalID = authInfo.getCanonicalID();
-    const requestArn = authInfo.getArn();
 
     return metadata.getBucket(bucketName, log, (err, bucket) => {
         const corsHeaders = collectCorsHeaders(request.headers.origin,
@@ -26,7 +25,7 @@ function bucketDeleteWebsite(authInfo, request, log, callback) {
         }
         log.trace('found bucket in metadata');
 
-        if (!isBucketAuthorized(bucket, requestType, canonicalID, requestArn,
+        if (!isBucketAuthorized(bucket, requestType, canonicalID, authInfo,
         log)) {
             log.debug('access denied for user on bucket', {
                 requestType,

lib/api/bucketGetCors.js

@@ -21,7 +21,6 @@ const requestType = 'bucketGetCors';
 function bucketGetCors(authInfo, request, log, callback) {
     const bucketName = request.bucketName;
     const canonicalID = authInfo.getCanonicalID();
-    const requestArn = authInfo.getArn();
 
     metadata.getBucket(bucketName, log, (err, bucket) => {
         if (err) {
@@ -35,7 +34,7 @@ function bucketGetCors(authInfo, request, log, callback) {
         const corsHeaders = collectCorsHeaders(request.headers.origin,
             request.method, bucket);
 
-        if (!isBucketAuthorized(bucket, requestType, canonicalID, requestArn,
+        if (!isBucketAuthorized(bucket, requestType, canonicalID, authInfo,
         log)) {
             log.debug('access denied for user on bucket', {
                 requestType,

lib/api/bucketGetLocation.js

@@ -22,7 +22,6 @@ const requestType = 'bucketGetLocation';
 function bucketGetLocation(authInfo, request, log, callback) {
     const bucketName = request.bucketName;
     const canonicalID = authInfo.getCanonicalID();
-    const requestArn = authInfo.getArn();
 
     return metadata.getBucket(bucketName, log, (err, bucket) => {
         if (err) {
@@ -37,7 +36,7 @@ function bucketGetLocation(authInfo, request, log, callback) {
         const corsHeaders = collectCorsHeaders(request.headers.origin,
             request.method, bucket);
 
-        if (!isBucketAuthorized(bucket, requestType, canonicalID, requestArn,
+        if (!isBucketAuthorized(bucket, requestType, canonicalID, authInfo,
         log)) {
             log.debug('access denied for account on bucket', {
                 requestType,

lib/api/bucketGetWebsite.js

@@ -21,7 +21,6 @@ const requestType = 'bucketGetWebsite';
 function bucketGetWebsite(authInfo, request, log, callback) {
     const bucketName = request.bucketName;
     const canonicalID = authInfo.getCanonicalID();
-    const requestArn = authInfo.getArn();
 
     metadata.getBucket(bucketName, log, (err, bucket) => {
         if (err) {
@@ -35,7 +34,7 @@ function bucketGetWebsite(authInfo, request, log, callback) {
 
         const corsHeaders = collectCorsHeaders(request.headers.origin,
             request.method, bucket);
-        if (!isBucketAuthorized(bucket, requestType, canonicalID, requestArn,
+        if (!isBucketAuthorized(bucket, requestType, canonicalID, authInfo,
         log)) {
             log.debug('access denied for user on bucket', {
                 requestType,

lib/api/bucketPutCors.js

@@ -24,7 +24,6 @@ function bucketPutCors(authInfo, request, log, callback) {
     log.debug('processing request', { method: 'bucketPutCors' });
     const bucketName = request.bucketName;
     const canonicalID = authInfo.getCanonicalID();
-    const requestArn = authInfo.getArn();
 
     if (!request.post) {
         log.debug('CORS xml body is missing',
@@ -67,8 +66,8 @@ function bucketPutCors(authInfo, request, log, callback) {
             });
         },
         function validateBucketAuthorization(bucket, rules, corsHeaders, next) {
-            if (!isBucketAuthorized(bucket, requestType, canonicalID,
-            requestArn, log)) {
+            if (!isBucketAuthorized(bucket, requestType, canonicalID, authInfo,
+            log)) {
                 log.debug('access denied for account on bucket', {
                     requestType,
                 });

lib/api/bucketPutWebsite.js

@@ -23,7 +23,6 @@ function bucketPutWebsite(authInfo, request, log, callback) {
     log.debug('processing request', { method: 'bucketPutWebsite' });
     const bucketName = request.bucketName;
     const canonicalID = authInfo.getCanonicalID();
-    const requestArn = authInfo.getArn();
 
     if (!request.post) {
         return callback(errors.MissingRequestBodyError);
@@ -47,8 +46,8 @@ function bucketPutWebsite(authInfo, request, log, callback) {
             });
         },
         function validateBucketAuthorization(bucket, config, next) {
-            if (!isBucketAuthorized(bucket, requestType, canonicalID,
-            requestArn, log)) {
+            if (!isBucketAuthorized(bucket, requestType, canonicalID, authInfo,
+            log)) {
                 log.debug('access denied for user on bucket', {
                     requestType,
                     method: 'bucketPutWebsite',

lib/api/multiObjectDelete.js

@@ -317,7 +317,6 @@ function multiObjectDelete(authInfo, request, log, callback) {
 
     const bucketName = request.bucketName;
     const canonicalID = authInfo.getCanonicalID();
-    const requestArn = authInfo.getArn();
 
     return async.waterfall([
         function parseXML(next) {
@@ -449,8 +448,8 @@ function multiObjectDelete(authInfo, request, log, callback) {
                     return next(null, quietSetting, errorResults, inPlay,
                         bucketMD);
                 }
-                if (!isBucketAuthorized(bucketMD, 'objectDelete',
-                    canonicalID, requestArn, log)) {
+                if (!isBucketAuthorized(bucketMD, 'objectDelete', canonicalID,
+                authInfo, log)) {
                     log.trace("access denied due to bucket acl's");
                     // if access denied at the bucket level, no access for
                     // any of the objects so all results will be error results

lib/api/objectPutPart.js

@@ -83,7 +83,6 @@ function objectPutPart(authInfo, request, streamingV4Params, log,
     log.trace('owner canonicalid to send to data', {
         canonicalID: authInfo.getCanonicalID,
     });
-    const requestArn = authInfo.getArn();
     // Note that keys in the query object retain their case, so
     // `request.query.uploadId` must be called with that exact capitalization.
     const uploadId = request.query.uploadId;
@@ -111,8 +110,8 @@ function objectPutPart(authInfo, request, streamingV4Params, log,
             // For validating the request at the destinationBucket level the
             // `requestType` is the general 'objectPut'.
             const requestType = 'objectPut';
-            if (!isBucketAuthorized(destinationBucket, requestType,
-                canonicalID, requestArn, log)) {
+            if (!isBucketAuthorized(destinationBucket, requestType, canonicalID,
+                authInfo, log)) {
                 log.debug('access denied for user on bucket', { requestType });
                 return next(errors.AccessDenied, destinationBucket);
             }

lib/metadata/metadataUtils.js

@@ -176,7 +176,6 @@ function metadataGetObject(bucketName, objectKey, versionId, log, cb) {
 function metadataValidateBucketAndObj(params, log, callback) {
     const { authInfo, bucketName, objectKey, versionId, requestType } = params;
     const canonicalID = authInfo.getCanonicalID();
-    const requestArn = authInfo.getArn();
     async.waterfall([
         function getBucketAndObjectMD(next) {
             return metadataGetBucketAndObject(requestType, bucketName,
@@ -184,7 +183,7 @@ function metadataValidateBucketAndObj(params, log, callback) {
         },
         function checkBucketAuth(bucket, objMD, next) {
             if (!isBucketAuthorized(bucket, requestType, canonicalID,
-            requestArn, log)) {
+            authInfo, log)) {
                 log.debug('access denied for user on bucket', { requestType });
                 return next(errors.AccessDenied, bucket);
             }
@@ -202,7 +201,7 @@ function metadataValidateBucketAndObj(params, log, callback) {
                 return next(null, bucket);
             }
             if (!isObjAuthorized(bucket, objMD, requestType, canonicalID,
-            requestArn, log)) {
+            authInfo, log)) {
                 log.debug('access denied for user on object', { requestType });
                 return next(errors.AccessDenied, bucket);
             }
@@ -256,13 +255,12 @@ function metadataGetBucket(requestType, bucketName, log, cb) {
 function metadataValidateBucket(params, log, callback) {
     const { authInfo, bucketName, requestType } = params;
     const canonicalID = authInfo.getCanonicalID();
-    const requestArn = authInfo.getArn();
     return metadataGetBucket(requestType, bucketName, log, (err, bucket) => {
         if (err) {
             return callback(err);
         }
         // still return bucket for cors headers
-        if (!isBucketAuthorized(bucket, requestType, canonicalID, requestArn,
+        if (!isBucketAuthorized(bucket, requestType, canonicalID, authInfo,
         log)) {
             log.debug('access denied for user on bucket', { requestType });
             return callback(errors.AccessDenied, bucket);

tests/unit/api/bucketPolicyAuth.js

@@ -3,20 +3,23 @@ const { BucketInfo, BucketPolicy } = require('arsenal').models;
 const constants = require('../../../constants');
 const { isBucketAuthorized, isObjAuthorized, validatePolicyResource }
     = require('../../../lib/api/apiUtils/authorization/permissionChecks');
-const { DummyRequestLogger } = require('../helpers');
+const { DummyRequestLogger, makeAuthInfo } = require('../helpers');
 
-const bucketOwnerCanonicalId = 'bucketOwnerCanonicalId';
+const accessKey = 'accessKey1';
+const altAccessKey = 'accessKey2';
+const authInfo = makeAuthInfo(accessKey);
+const userAuthInfo = makeAuthInfo(accessKey, 'user');
+const altUserAuthInfo = makeAuthInfo(accessKey, 'other');
+const altAcctAuthInfo = makeAuthInfo(altAccessKey);
+const altAcctUserAuthInfo = makeAuthInfo(altAccessKey, 'altUser');
+const bucketOwnerCanonicalId = authInfo.getCanonicalID();
+const objectOwnerCanonicalId = userAuthInfo.getCanonicalID();
+const canonicalIdToVet = altAcctAuthInfo.getCanonicalID();
 const creationDate = new Date().toJSON();
 const bucket = new BucketInfo('policyBucketAuthTester', bucketOwnerCanonicalId,
-    'iAmTheOwnerDisplayName', creationDate);
-const objectOwnerCanonicalId = 'objectOwnerCanonicalId';
+    authInfo.getAccountDisplayName(), creationDate);
 const object = { 'owner-id': objectOwnerCanonicalId };
-const canonicalIdToVet = 'canonicalIdToVet';
-const userArn = 'arn:aws:iam::123456789012:user/user';
-const otherUserArn = 'arn:aws:iam::123456789012:user/other';
-const otherAccountUserArn = 'arn:aws:iam:987654321098:user/other';
-const accountArn = 'arn:aws:iam::123456789012:root';
-const accountId = '123456789012';
+const accountId = authInfo.getShortid();
 const bucAction = 'bucketPut';
 const objAction = 'objectPut';
 const basePolicyObj = {
@@ -35,7 +38,9 @@ const authTests = [
     {
         name: 'should allow access if canonical user principal matches non-',
         bucketId: canonicalIdToVet,
+        bucketAuthInfo: altAcctAuthInfo,
         objectId: canonicalIdToVet,
+        objectAuthInfo: altAcctAuthInfo,
         keyToChange: 'Principal',
         bucketValue: { CanonicalUser: [canonicalIdToVet] },
         objectValue: { CanonicalUser: [canonicalIdToVet] },
@@ -43,26 +48,32 @@ const authTests = [
     },
     {
         name: 'should allow access if user arn principal matches non-',
-        bucketId: userArn,
-        objectId: userArn,
+        bucketId: objectOwnerCanonicalId,
+        bucketAuthInfo: userAuthInfo,
+        objectId: objectOwnerCanonicalId,
+        objectAuthInfo: userAuthInfo,
         keyToChange: 'Principal',
-        bucketValue: { AWS: userArn },
-        objectValue: { AWS: userArn },
+        bucketValue: { AWS: userAuthInfo.getArn() },
+        objectValue: { AWS: userAuthInfo.getArn() },
         expected: true,
     },
     {
         name: 'should allow access if account arn principal matches non-',
-        bucketId: accountArn,
-        objectId: accountArn,
+        bucketId: bucketOwnerCanonicalId,
+        bucketAuthInfo: authInfo,
+        objectId: bucketOwnerCanonicalId,
+        objectAuthInfo: authInfo,
         keyToChange: 'Principal',
-        bucketValue: { AWS: accountArn },
-        objectValue: { AWS: accountArn },
+        bucketValue: { AWS: authInfo.getArn() },
+        objectValue: { AWS: authInfo.getArn() },
         expected: true,
     },
     {
         name: 'should allow access if account id principal matches non-',
         bucketId: accountId,
+        bucketAuthInfo: authInfo,
         objectId: accountId,
+        objectAuthInfo: authInfo,
         keyToChange: 'Principal',
         bucketValue: { AWS: accountId },
         objectValue: { AWS: accountId },
@@ -71,8 +82,10 @@ const authTests = [
     {
         name: 'should allow access if account id principal is contained in ' +
             'user arn of non-',
-        bucketId: userArn,
-        objectId: userArn,
+        bucketId: objectOwnerCanonicalId,
+        bucketAuthInfo: userAuthInfo.getArn(),
+        objectId: objectOwnerCanonicalId,
+        objectAuthInfo: userAuthInfo.getArn(),
         keyToChange: 'Principal',
         bucketValue: { AWS: accountId },
         objectValue: { AWS: accountId },
@@ -81,8 +94,10 @@ const authTests = [
     {
         name: 'should allow access if account id principal is contained in ' +
             'account arn of non-',
-        bucketId: accountArn,
-        objectId: accountArn,
+        bucketId: bucketOwnerCanonicalId,
+        bucketAuthInfo: authInfo,
+        objectId: bucketOwnerCanonicalId,
+        objectAuthInfo: authInfo,
         keyToChange: 'Principal',
         bucketValue: { AWS: accountId },
         objectValue: { AWS: accountId },
@@ -91,37 +106,45 @@ const authTests = [
     {
         name: 'should allow access if account arn principal is contained in ' +
             'user arn of non-',
-        bucketId: userArn,
-        objectId: userArn,
+        bucketId: objectOwnerCanonicalId,
+        bucketAuthInfo: userAuthInfo,
+        objectId: objectOwnerCanonicalId,
+        objectAuthInfo: userAuthInfo,
         keyToChange: 'Principal',
-        bucketValue: { AWS: accountArn },
-        objectValue: { AWS: accountArn },
+        bucketValue: { AWS: authInfo.getArn() },
+        objectValue: { AWS: authInfo.getArn() },
         expected: true,
     },
     {
         name: 'should deny access if account arn principal doesn\'t match ' +
             'user arn of non-',
-        bucketId: otherAccountUserArn,
-        objectId: otherAccountUserArn,
+        bucketId: altAcctUserAuthInfo.getCanonicalID(),
+        bucketAuthInfo: altAcctUserAuthInfo,
+        objectId: altAcctUserAuthInfo.getCanonicalID(),
+        objectAuthInfo: altAcctUserAuthInfo,
         keyToChange: 'Principal',
-        bucketValue: { AWS: accountArn },
-        objectValue: { AWS: accountArn },
+        bucketValue: { AWS: authInfo.getArn() },
+        objectValue: { AWS: authInfo.getArn() },
         expected: false,
     },
     {
         name: 'should deny access if user arn principal doesn\'t match ' +
             'user arn of non-',
-        bucketId: userArn,
-        objectId: userArn,
+        bucketId: objectOwnerCanonicalId,
+        bucketAuthInfo: userAuthInfo,
+        objectId: objectOwnerCanonicalId,
+        objectAuthInfo: userAuthInfo,
         keyToChange: 'Principal',
-        bucketValue: { AWS: otherUserArn },
-        objectValue: { AWS: otherUserArn },
+        bucketValue: { AWS: altUserAuthInfo.getArn() },
+        objectValue: { AWS: altUserAuthInfo.getArn() },
         expected: false,
     },
     {
         name: 'should deny access if principal doesn\'t match non-',
         bucketId: canonicalIdToVet,
+        bucketAuthInfo: altAcctAuthInfo,
         objectId: canonicalIdToVet,
+        objectAuthInfo: altAcctAuthInfo,
         keyToChange: 'Principal',
         bucketValue: { CanonicalUser: [bucketOwnerCanonicalId] },
         objectValue: { CanonicalUser: [objectOwnerCanonicalId] },
@@ -131,7 +154,9 @@ const authTests = [
         name: 'should allow access if principal and action match policy for ' +
             'non-',
         bucketId: canonicalIdToVet,
+        bucketAuthInfo: altAcctAuthInfo,
         objectId: canonicalIdToVet,
+        objectAuthInfo: altAcctAuthInfo,
         keyToChange: 'Action',
         bucketValue: ['s3:CreateBucket'],
         objectValue: ['s3:PutObject'],
@@ -141,7 +166,9 @@ const authTests = [
         name: 'should deny access if principal matches but action does not ' +
             'match policy for non-',
         bucketId: canonicalIdToVet,
+        bucketAuthInfo: altAcctAuthInfo,
         objectId: canonicalIdToVet,
+        objectAuthInfo: altAcctAuthInfo,
         keyToChange: 'Action',
         bucketValue: ['s3:GetBucketLocation'],
         objectValue: ['s3:GetObject'],
@@ -150,7 +177,9 @@ const authTests = [
     {
         name: 'should allow access even if bucket policy denies for ',
         bucketId: bucketOwnerCanonicalId,
+        bucketAuthInfo: authInfo,
         objectId: objectOwnerCanonicalId,
+        objectAuthInfo: userAuthInfo,
         keyToChange: 'Effect',
         bucketValue: 'Deny',
         objectValue: 'Deny',
@@ -187,7 +216,7 @@ const resourceTests = [
     },
 ];
 
-describe('bucket policy authorization', () => {
+describe.only('bucket policy authorization', () => {
     describe('isBucketAuthorized with no policy set', () => {
         it('should allow access to bucket owner', done => {
             const allowed = isBucketAuthorized(bucket, 'bucketPut',
@@ -234,7 +263,7 @@ describe('bucket policy authorization', () => {
                 newPolicy.Statement[0][t.keyToChange] = t.bucketValue;
                 bucket.setBucketPolicy(newPolicy);
                 const allowed = isBucketAuthorized(bucket, bucAction,
-                    t.bucketId, t.bucketId, log);
+                    t.bucketId, t.bucketAuthInfo, log);
                 assert.equal(allowed, t.expected);
                 done();
             });
@@ -310,7 +339,7 @@ describe('bucket policy authorization', () => {
                 newPolicy.Statement[0][t.keyToChange] = t.objectValue;
                 bucket.setBucketPolicy(newPolicy);
                 const allowed = isObjAuthorized(bucket, object, objAction,
-                    t.objectId, t.objectId, log);
+                    t.objectId, t.objectAuthInfo, log);
                 assert.equal(allowed, t.expected);
                 done();
             });

tests/unit/helpers.js

@@ -63,7 +63,7 @@ function timeDiff(startTime) {
     return milliseconds;
 }
 
-function makeAuthInfo(accessKey) {
+function makeAuthInfo(accessKey, userName) {
     const canIdMap = {
         accessKey1: '79a59df900b949e55d96a1e698fbacedfd6e09d98eacf8f8d5218e7'
             + 'cd47ef2be',
@@ -73,12 +73,18 @@ function makeAuthInfo(accessKey) {
     };
     canIdMap[constants.publicId] = constants.publicId;
 
-    return new AuthInfo({
+    const params = {
         canonicalID: canIdMap[accessKey] || canIdMap.default,
         shortid: 'shortid',
         email: `${accessKey}@l.com`,
         accountDisplayName: `${accessKey}displayName`,
-    });
+        arn: 'arn:aws:iam::shortid:root',
+    };
+    if (userName) {
+        params.IAMdisplayName = `${accessKey}-${userName}-UserDisplayName`;
+        params.arn = `arn:aws:iam::shortid:user/${userName}`;
+    }
+    return new AuthInfo(params);
 }
 
 class WebsiteConfig {