Skip to content

Commit cdfe3cc

Browse files
dvasilasdvasilas
committed
CLDSRV-774: Add aclRequired field tests
1 parent 9478786 commit cdfe3cc

4 files changed

Lines changed: 290 additions & 2 deletions

File tree

‎tests/functional/aws-node-sdk/test/serverAccessLogs/testServerAccessLogFile.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,7 @@ describe('Server Access Logs - File Output', async () => {
143143
'authenticationType': 'AuthHeader', // STATIC
144144
// 'hostHeader': '', // UNKNOWN
145145
'tlsVersion': null, // TODO: Add https tests.
146-
'aclRequired': null, // TODO: Add https tests.
146+
'aclRequired': null, // DYNAMIC (absent for owner, "Yes" when ACL is consulted)
147147
'bucketOwner': '79a59df900b949e55d96a1e698fbacedfd6e09d98eacf8f8d5218e7cd47ef2be', // DYNAMIC
148148
bucketName, // DYNAMIC
149149
// 'req_id': '', // UNKNOWN

‎tests/unit/api/apiUtils/permissionChecks.js‎

Lines changed: 216 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,15 @@
11
const assert = require('assert');
22

3-
const { isLifecycleSession, checkBucketPolicyResult, checkBucketPolicy } =
3+
const { isLifecycleSession, checkBucketPolicyResult, checkBucketPolicy,
4+
isBucketAuthorized, isObjAuthorized, evaluateBucketPolicyWithIAM } =
45
require('../../../../lib/api/apiUtils/authorization/permissionChecks.js');
56
const { DummyRequestLogger } = require('../../helpers');
67

78
const stubLog = new DummyRequestLogger();
89

10+
const ownerCanonicalId = '79a59df900b949e55d96a1e698fbacedfd6e09d98eacf8f8d5218e7cd47ef2be';
11+
const otherCanonicalId = 'aaaaaaaabbbbbbbbccccccccddddddddeeeeeeeeffffffffgggggggghhhhhhh1';
12+
913
describe('authInfoHelper', () => {
1014
const tests = [
1115
{
@@ -708,3 +712,214 @@ describe('checkBucketPolicy Principal logic', () => {
708712
});
709713
});
710714
});
715+
716+
describe('aclRequired field in isBucketAuthorized', () => {
717+
function makeBucket(owner, acl, bucketPolicy) {
718+
return {
719+
getOwner: () => owner,
720+
getAcl: () => ({
721+
Canned: acl?.Canned || '',
722+
FULL_CONTROL: acl?.FULL_CONTROL || [],
723+
READ: acl?.READ || [],
724+
READ_ACP: acl?.READ_ACP || [],
725+
WRITE: acl?.WRITE || [],
726+
WRITE_ACP: acl?.WRITE_ACP || [],
727+
}),
728+
getBucketPolicy: () => bucketPolicy || null,
729+
};
730+
}
731+
732+
function makeRequest() {
733+
return { serverAccessLog: {}, socket: { remoteAddress: '127.0.0.1' }, headers: {} };
734+
}
735+
736+
function makeAuthInfo(canonicalID, arn, isIAMUser = false) {
737+
return {
738+
getCanonicalID: () => canonicalID,
739+
getArn: () => arn,
740+
isRequesterAnIAMUser: () => isIAMUser,
741+
};
742+
}
743+
744+
it('should not set aclRequired when requester is bucket owner', () => {
745+
const request = makeRequest();
746+
const bucket = makeBucket(ownerCanonicalId);
747+
const authInfo = makeAuthInfo(ownerCanonicalId, 'arn:aws:iam::123456789012:/owner/');
748+
isBucketAuthorized(bucket, 'bucketGet', ownerCanonicalId, authInfo, stubLog,
749+
request, { bucketGet: false });
750+
assert.strictEqual(request.serverAccessLog.aclRequired, undefined);
751+
});
752+
753+
it('should set aclRequired to Yes for non-owner with no bucket policy', () => {
754+
const request = makeRequest();
755+
const bucket = makeBucket(ownerCanonicalId, { READ: [otherCanonicalId] });
756+
const authInfo = makeAuthInfo(otherCanonicalId, 'arn:aws:iam::999999999999:user/other');
757+
isBucketAuthorized(bucket, 'bucketGet', otherCanonicalId, authInfo, stubLog,
758+
request, { bucketGet: false });
759+
assert.strictEqual(request.serverAccessLog.aclRequired, 'Yes');
760+
});
761+
762+
it('should not set aclRequired when bucket policy explicitly allows', () => {
763+
const request = makeRequest();
764+
// Use an IAM user in the bucket owner's account so principal match is OK (not CROSS_ACCOUNT)
765+
// and the owner early-return doesn't fire (requester is IAM user, not account root)
766+
const iamUserCanonicalId = ownerCanonicalId;
767+
const bucket = makeBucket(ownerCanonicalId, {}, {
768+
Statement: [{
769+
Effect: 'Allow',
770+
Principal: { AWS: 'arn:aws:iam::123456789012:user/iamuser' },
771+
Action: ['s3:ListBucket'],
772+
Resource: ['arn:aws:s3:::test-bucket'],
773+
}],
774+
});
775+
const authInfo = makeAuthInfo(iamUserCanonicalId, 'arn:aws:iam::123456789012:user/iamuser', true);
776+
isBucketAuthorized(bucket, 'bucketGet', iamUserCanonicalId, authInfo, stubLog,
777+
request, { bucketGet: false });
778+
assert.strictEqual(request.serverAccessLog.aclRequired, undefined);
779+
});
780+
781+
it('should not set aclRequired when bucket policy explicitly denies', () => {
782+
const request = makeRequest();
783+
const iamUserCanonicalId = ownerCanonicalId;
784+
const bucket = makeBucket(ownerCanonicalId, { READ: [iamUserCanonicalId] }, {
785+
Statement: [{
786+
Effect: 'Deny',
787+
Principal: { AWS: 'arn:aws:iam::123456789012:user/iamuser' },
788+
Action: ['s3:ListBucket'],
789+
Resource: ['arn:aws:s3:::test-bucket'],
790+
}],
791+
});
792+
const authInfo = makeAuthInfo(iamUserCanonicalId, 'arn:aws:iam::123456789012:user/iamuser', true);
793+
isBucketAuthorized(bucket, 'bucketGet', iamUserCanonicalId, authInfo, stubLog,
794+
request, { bucketGet: false });
795+
assert.strictEqual(request.serverAccessLog.aclRequired, undefined);
796+
});
797+
798+
it('should set aclRequired to Yes when bucket policy returns DEFAULT_DENY', () => {
799+
const request = makeRequest();
800+
// Policy grants PutObject to a different principal — nothing matches
801+
// the bucketGet request from otherCanonicalId, so checkBucketPolicy
802+
// returns DEFAULT_DENY and falls back to ACL evaluation.
803+
const bucket = makeBucket(ownerCanonicalId, { READ: [otherCanonicalId] }, {
804+
Statement: [{
805+
Effect: 'Allow',
806+
Principal: { AWS: 'arn:aws:iam::111111111111:root' },
807+
Action: ['s3:PutObject'],
808+
Resource: ['arn:aws:s3:::test-bucket/*'],
809+
}],
810+
});
811+
const authInfo = makeAuthInfo(otherCanonicalId, 'arn:aws:iam::999999999999:user/other');
812+
isBucketAuthorized(bucket, 'bucketGet', otherCanonicalId, authInfo, stubLog,
813+
request, { bucketGet: false });
814+
assert.strictEqual(request.serverAccessLog.aclRequired, 'Yes');
815+
});
816+
817+
it('should handle missing serverAccessLog gracefully', () => {
818+
const request = {};
819+
const bucket = makeBucket(ownerCanonicalId, { READ: [otherCanonicalId] });
820+
const authInfo = makeAuthInfo(otherCanonicalId, 'arn:aws:iam::999999999999:user/other');
821+
assert.doesNotThrow(() => {
822+
isBucketAuthorized(bucket, 'bucketGet', otherCanonicalId, authInfo, stubLog,
823+
request, { bucketGet: false });
824+
});
825+
});
826+
});
827+
828+
describe('aclRequired field in isObjAuthorized', () => {
829+
function makeBucket(owner, acl, bucketPolicy) {
830+
return {
831+
getOwner: () => owner,
832+
getName: () => 'test-bucket',
833+
getAcl: () => ({
834+
Canned: acl?.Canned || '',
835+
FULL_CONTROL: acl?.FULL_CONTROL || [],
836+
READ: acl?.READ || [],
837+
READ_ACP: acl?.READ_ACP || [],
838+
WRITE: acl?.WRITE || [],
839+
WRITE_ACP: acl?.WRITE_ACP || [],
840+
}),
841+
getBucketPolicy: () => bucketPolicy || null,
842+
};
843+
}
844+
845+
function makeRequest() {
846+
return { serverAccessLog: {} };
847+
}
848+
849+
function makeAuthInfo(canonicalID, arn) {
850+
return {
851+
getCanonicalID: () => canonicalID,
852+
getArn: () => arn,
853+
isRequesterAnIAMUser: () => false,
854+
};
855+
}
856+
857+
function makeObjectMD(ownerId) {
858+
return {
859+
'owner-id': ownerId,
860+
acl: {
861+
Canned: '',
862+
FULL_CONTROL: [],
863+
READ: [],
864+
READ_ACP: [],
865+
WRITE: [],
866+
WRITE_ACP: [],
867+
},
868+
};
869+
}
870+
871+
it('should not set aclRequired when requester is object owner', () => {
872+
const request = makeRequest();
873+
const bucket = makeBucket(ownerCanonicalId);
874+
const objectMD = makeObjectMD(otherCanonicalId);
875+
const authInfo = makeAuthInfo(otherCanonicalId, 'arn:aws:iam::999999999999:/account/');
876+
isObjAuthorized(bucket, objectMD, 'objectGet', otherCanonicalId, authInfo, stubLog,
877+
request, { objectGet: false });
878+
assert.strictEqual(request.serverAccessLog.aclRequired, undefined);
879+
});
880+
881+
it('should set aclRequired to Yes for non-owner with no bucket policy', () => {
882+
const request = makeRequest();
883+
const bucket = makeBucket(ownerCanonicalId);
884+
const objectMD = makeObjectMD(ownerCanonicalId);
885+
objectMD.acl.READ = [otherCanonicalId];
886+
const authInfo = makeAuthInfo(otherCanonicalId, 'arn:aws:iam::999999999999:user/other');
887+
isObjAuthorized(bucket, objectMD, 'objectGet', otherCanonicalId, authInfo, stubLog,
888+
request, { objectGet: false });
889+
assert.strictEqual(request.serverAccessLog.aclRequired, 'Yes');
890+
});
891+
});
892+
893+
describe('aclRequired field in evaluateBucketPolicyWithIAM', () => {
894+
function makeBucket(owner, bucketPolicy) {
895+
return {
896+
getOwner: () => owner,
897+
getAcl: () => ({
898+
Canned: '',
899+
FULL_CONTROL: [],
900+
READ: [],
901+
READ_ACP: [],
902+
WRITE: [],
903+
WRITE_ACP: [],
904+
}),
905+
getBucketPolicy: () => bucketPolicy || null,
906+
};
907+
}
908+
909+
function makeAuthInfo(canonicalID, arn) {
910+
return {
911+
getCanonicalID: () => canonicalID,
912+
getArn: () => arn,
913+
isRequesterAnIAMUser: () => false,
914+
};
915+
}
916+
917+
it('should not set aclRequired (ACLs are not actually consulted)', () => {
918+
const request = { serverAccessLog: {} };
919+
const bucket = makeBucket(ownerCanonicalId);
920+
const authInfo = makeAuthInfo(otherCanonicalId, 'arn:aws:iam::999999999999:user/other');
921+
evaluateBucketPolicyWithIAM(bucket, 'objectDelete', otherCanonicalId, authInfo,
922+
{ objectDelete: false }, stubLog, request);
923+
assert.strictEqual(request.serverAccessLog.aclRequired, undefined);
924+
});
925+
});

‎tests/unit/metadata/metadataUtils.spec.js‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ const {
1818
validateBucket,
1919
metadataGetObjects,
2020
metadataGetObject,
21+
storeServerAccessLogInfo,
2122
} = require('../../../lib/metadata/metadataUtils');
2223
const metadata = require('../../../lib/metadata/wrapper');
2324

@@ -150,3 +151,35 @@ describe('metadataGetObject', () => {
150151
});
151152
});
152153
});
154+
155+
describe('storeServerAccessLogInfo - copySource aclRequired', () => {
156+
it('should move source aclRequired to sourceServerAccessLog and restore destination value', () => {
157+
// Destination auth set aclRequired='Yes', then source auth ran on the
158+
// same request object and did not set aclRequired (owner on source).
159+
const request = {
160+
serverAccessLog: {},
161+
};
162+
const options = {
163+
copySource: true,
164+
savedAclRequired: 'Yes',
165+
};
166+
storeServerAccessLogInfo(request, null, null, options);
167+
assert.strictEqual(request.sourceServerAccessLog.aclRequired, undefined);
168+
assert.strictEqual(request.serverAccessLog.aclRequired, 'Yes');
169+
});
170+
171+
it('should swap aclRequired when source auth also required ACL check', () => {
172+
// Destination auth did not set aclRequired (owner on dest), then
173+
// source auth set aclRequired='Yes' on the same request object.
174+
const request = {
175+
serverAccessLog: { aclRequired: 'Yes' },
176+
};
177+
const options = {
178+
copySource: true,
179+
savedAclRequired: undefined,
180+
};
181+
storeServerAccessLogInfo(request, null, null, options);
182+
assert.strictEqual(request.sourceServerAccessLog.aclRequired, 'Yes');
183+
assert.strictEqual(request.serverAccessLog.aclRequired, undefined);
184+
});
185+
});

‎tests/unit/utils/serverAccessLogger.js‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1231,6 +1231,46 @@ describe('serverAccessLogger utility functions', () => {
12311231
assert.strictEqual('loggingEnabled' in loggedData, true);
12321232
});
12331233

1234+
it('should log aclRequired as Yes when set on request', () => {
1235+
setServerAccessLogger(mockLogger);
1236+
const req = {
1237+
serverAccessLog: {
1238+
aclRequired: 'Yes',
1239+
},
1240+
headers: {},
1241+
socket: {},
1242+
};
1243+
const res = {
1244+
serverAccessLog: {},
1245+
getHeader: () => null,
1246+
};
1247+
1248+
logServerAccess(req, res);
1249+
1250+
assert.strictEqual(mockLogger.write.callCount, 1);
1251+
const loggedData = JSON.parse(mockLogger.write.firstCall.args[0].trim());
1252+
assert.strictEqual(loggedData.aclRequired, 'Yes');
1253+
});
1254+
1255+
it('should omit aclRequired when not set on request', () => {
1256+
setServerAccessLogger(mockLogger);
1257+
const req = {
1258+
serverAccessLog: {},
1259+
headers: {},
1260+
socket: {},
1261+
};
1262+
const res = {
1263+
serverAccessLog: {},
1264+
getHeader: () => null,
1265+
};
1266+
1267+
logServerAccess(req, res);
1268+
1269+
assert.strictEqual(mockLogger.write.callCount, 1);
1270+
const loggedData = JSON.parse(mockLogger.write.firstCall.args[0].trim());
1271+
assert.strictEqual('aclRequired' in loggedData, false);
1272+
});
1273+
12341274
it('should include error code when present', () => {
12351275
setServerAccessLogger(mockLogger);
12361276
const req = {

0 commit comments

Comments
 (0)