CLDSRV-774: Add aclRequired field to server access logs by dvasilas · Pull Request #6142 · scality/cloudserver

dvasilas · 2026-04-07T14:49:19Z

Add the aclRequired field to access logs.

From https://docs.aws.amazon.com/AmazonS3/latest/userguide/LogFormat.html#log-record-fields

indicates whether the request required an access control list (ACL) for authorization. If the request required an ACL for authorization, the string is Yes. If no ACLs were required, the string is -.

bert-e · 2026-04-07T14:49:24Z

Hello dvasilas,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Available options

name	description	privileged	authored
`/after_pull_request`	Wait for the given pull request id to be merged before continuing with the current one.
`/bypass_author_approval`	Bypass the pull request author's approval	⭐
`/bypass_build_status`	Bypass the build and test status	⭐
`/bypass_commit_size`	Bypass the check on the size of the changeset `TBA`	⭐
`/bypass_incompatible_branch`	Bypass the check on the source branch prefix	⭐
`/bypass_jira_check`	Bypass the Jira issue check	⭐
`/bypass_peer_approval`	Bypass the pull request peers' approval	⭐
`/bypass_leader_approval`	Bypass the pull request leaders' approval	⭐
`/approve`	Instruct Bert-E that the author has approved the pull request.		✍️
`/create_pull_requests`	Allow the creation of integration pull requests.
`/create_integration_branches`	Allow the creation of integration branches.
`/no_octopus`	Prevent Wall-E from doing any octopus merge and use multiple consecutive merge instead
`/unanimity`	Change review acceptance criteria from `one reviewer at least` to `all reviewers`
`/wait`	Instruct Bert-E not to run until further notice.

Available commands

name	description	privileged
`/help`	Print Bert-E's manual in the pull request.
`/status`	Print Bert-E's current status in the pull request `TBA`
`/clear`	Remove all comments from Bert-E from the history `TBA`
`/retry`	Re-start a fresh build `TBA`
`/build`	Re-start a fresh build `TBA`
`/force_reset`	Delete integration branches & pull requests, and restart merge process from the beginning.
`/reset`	Try to remove integration branches unless there are commits on them which do not appear on the source branch.

Status report is not available.

bert-e · 2026-04-07T14:49:34Z

Incorrect fix version

The Fix Version/s in issue CLDSRV-774 contains:

9.2.34

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

9.2.34
9.3.7
9.4.0

Please check the Fix Version/s of CLDSRV-774, or the target
branch of this pull request.

claude · 2026-04-07T14:50:59Z

LGTM

Clean implementation. The save/restore pattern in metadataUtils.js for copy-source aclRequired is well thought out, and the processBucketPolicy return-value change is properly propagated across all three callers (isBucketAuthorized, isObjAuthorized, evaluateBucketPolicyWithIAM). Tests cover the key authorization paths.

Review by Claude Code

bert-e · 2026-04-07T14:51:09Z

Request integration branches

Waiting for integration branch creation to be requested by the user.

To request integration branches, please comment on this pull request with the following command:

/create_integration_branches

Alternatively, the /approve and /create_pull_requests commands will automatically
create the integration branches.

BourgoisMickael · 2026-04-07T16:12:31Z

lib/api/apiUtils/authorization/permissionChecks.js

        }
    }
-    return processedResult;
+    return { allowed: processedResult, aclRequired };


you could update the request.serverAccessLog.aclRequired immediately in that function instead of returning the value

I didn't do it like that because there is 1 caller of this function that never uses ACLs, and we should skip aclRequired there.

BourgoisMickael · 2026-04-07T16:40:47Z

lib/metadata/metadataUtils.js

        },
    ], (err, bucket, objMD, raftSessionId) => {
        storeServerAccessLogInfo(request, bucket, raftSessionId, params.serverAccessLogOptions);
+        if (isCopySource && request?.sourceServerAccessLog) {


Should this logic rather be in storeServerAccessLogInfo ?

Yes, that's a bit better, done, thanks

codecov · 2026-04-08T08:02:22Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.33%. Comparing base (c09dc78) to head (9d6061a).
⚠️ Report is 3 commits behind head on development/9.2.
✅ All tests successful. No failed tests found.

Additional details and impacted files

Files with missing lines	Coverage Δ
lib/api/apiUtils/authorization/permissionChecks.js	`97.26% <100.00%> (+2.90%)`	⬆️
lib/metadata/metadataUtils.js	`90.55% <100.00%> (+0.21%)`	⬆️
lib/utilities/serverAccessLogger.js	`81.85% <ø> (ø)`

@@                 Coverage Diff                 @@
##           development/9.2    #6142      +/-   ##
===================================================
+ Coverage            84.25%   84.33%   +0.08%     
===================================================
  Files                  204      204              
  Lines                13126    13140      +14     
===================================================
+ Hits                 11059    11082      +23     
+ Misses                2067     2058       -9

Flag	Coverage Δ
file-ft-tests	`67.69% <88.88%> (+0.02%)`	⬆️
kmip-ft-tests	`28.19% <22.22%> (+<0.01%)`	⬆️
mongo-v0-ft-tests	`68.88% <88.88%> (-0.03%)`	⬇️
mongo-v1-ft-tests	`68.92% <88.88%> (+0.02%)`	⬆️
multiple-backend	`35.25% <44.44%> (+<0.01%)`	⬆️
sur-tests	`35.71% <22.22%> (-0.01%)`	⬇️
sur-tests-inflights	`37.59% <22.22%> (+0.02%)`	⬆️
unit	`69.90% <100.00%> (+0.15%)`	⬆️
utapi-v2-tests	`34.49% <22.22%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

claude · 2026-04-08T08:02:55Z

LGTM

Review by Claude Code

aclRequired indicates whether the request required an ACL for authorization. It is "Yes" when no bucket policy exists or when the bucket policy returns DEFAULT_DENY, falling back to ACL evaluation. It is "-" for owner requests, service accounts, and requests decided by IAM or bucket policy alone. For copy operations, the source bucket auth runs on the same request object and would overwrite the destination's aclRequired. The value is saved before source auth, then moved to sourceServerAccessLog and the destination's value is restored.

dvasilas · 2026-04-09T09:30:40Z

/create_integration_branches

bert-e · 2026-04-09T09:30:52Z

Conflict

A conflict has been raised during the creation of
integration branch w/9.3/improvement/CLDSRV-774 with contents from improvement/CLDSRV-774
and development/9.3.

I have not created the integration branch.

Here are the steps to resolve this conflict:

 git fetch
 git checkout -B w/9.3/improvement/CLDSRV-774 origin/development/9.3
 git merge origin/improvement/CLDSRV-774
 # <intense conflict resolution>
 git commit
 git push -u origin w/9.3/improvement/CLDSRV-774

The following options are set: create_integration_branches

dvasilas · 2026-04-09T09:32:27Z

ping

bert-e · 2026-04-09T09:32:39Z

Integration data created

I have created the integration data for the additional destination branches.

this pull request will merge improvement/CLDSRV-774 into
development/9.2
w/9.3/improvement/CLDSRV-774 will be merged into development/9.3
w/9.4/improvement/CLDSRV-774 will be merged into development/9.4

The following branches will NOT be impacted:

development/7.10
development/7.4
development/7.70
development/8.8
development/9.0
development/9.1

You can set option create_pull_requests if you need me to create
integration pull requests in addition to integration branches, with:

@bert-e create_pull_requests

The following options are set: create_integration_branches

bert-e · 2026-04-09T09:32:40Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
2 peers

The following options are set: create_integration_branches

dvasilas · 2026-04-09T09:32:53Z

/approve

claude · 2026-04-09T09:33:20Z

LGTM

Review by Claude Code

bert-e · 2026-04-09T09:58:36Z

I have successfully merged the changeset of this pull request
into targetted development branches:

✔️ development/9.2
✔️ development/9.3
✔️ development/9.4

The following branches have NOT changed:

development/7.10
development/7.4
development/7.70
development/8.8
development/9.0
development/9.1

Please check the status of the associated issue CLDSRV-774.

Goodbye dvasilas.

The following options are set: approve, create_integration_branches

leif-scality approved these changes Apr 7, 2026

View reviewed changes

BourgoisMickael reviewed Apr 7, 2026

View reviewed changes

BourgoisMickael approved these changes Apr 7, 2026

View reviewed changes

dvasilas mentioned this pull request Apr 8, 2026

LOGC-57: Add e2e tests for aclRequired field scality/log-courier#143

Open

dvasilas added 3 commits April 9, 2026 12:29

CLDSRV-774: Add aclRequired field tests

cdfe3cc

CLDSRV-774: Bump version to 9.2.34

9d6061a

dvasilas force-pushed the improvement/CLDSRV-774 branch from a4027ab to 9d6061a Compare April 9, 2026 09:30

bert-e merged commit 9d6061a into development/9.2 Apr 9, 2026
30 checks passed

bert-e deleted the improvement/CLDSRV-774 branch April 9, 2026 09:58

Conversation

dvasilas commented Apr 7, 2026

Uh oh!

bert-e commented Apr 7, 2026

Hello dvasilas,

Uh oh!

bert-e commented Apr 7, 2026

Incorrect fix version

Uh oh!

claude bot commented Apr 7, 2026

Uh oh!

bert-e commented Apr 7, 2026

Request integration branches

Uh oh!

BourgoisMickael Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

dvasilas Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

BourgoisMickael Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dvasilas Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

claude bot commented Apr 8, 2026

Uh oh!

dvasilas commented Apr 9, 2026

Uh oh!

bert-e commented Apr 9, 2026

Conflict

Uh oh!

dvasilas commented Apr 9, 2026

Uh oh!

bert-e commented Apr 9, 2026

Integration data created

Uh oh!

bert-e commented Apr 9, 2026

Waiting for approval

Uh oh!

dvasilas commented Apr 9, 2026

Uh oh!

claude bot commented Apr 9, 2026

Uh oh!

bert-e commented Apr 9, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

BourgoisMickael Apr 7, 2026 •

edited

Loading

codecov bot commented Apr 8, 2026 •

edited

Loading