Skip to content

Add listacl listpolicy#1421

Merged
fviard merged 8 commits intos3tools:masterfrom
marsteg:ADD_listacl_listpolicy
Oct 22, 2025
Merged

Add listacl listpolicy#1421
fviard merged 8 commits intos3tools:masterfrom
marsteg:ADD_listacl_listpolicy

Conversation

@marsteg
Copy link
Contributor

@marsteg marsteg commented Apr 5, 2025

I added some very simple commands to list the Policy and ACL. It seems like it's almost a design decission that this wasn't there... but since I missed it, i thought maybe others find it useful, too.

Example Usage:
python3 s3cmd -c ceph.cfg listacl s3://test3
python3 s3cmd -c ceph.cfg listacl s3://test3/test.txt
python3 s3cmd -c ceph.cfg listpolicy s3://test3

I was only able to test this on ceph unfortunately.

Copy link
Contributor

@mludvig mludvig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fviard
Copy link
Contributor

fviard commented Oct 16, 2025

Thanks for your contribution, and sorry for the delay of my reply.
There is no dedicated command for list acl and list policy because today you will get both these info from the s3cmd info command.

I don't know if you have a case where you would really need a dedicated command for them?

For your info, there was 2 reasons for why there was not a dedicated command so far:

  • s3cmd have already a lot of command, that makes it easily confusing, so avoiding adding much more than necessary is always better I think
  • Especially for policy, a big question for adding a dedicated command is what format should be used for the output? Something formatted that could be convenient to understand by the user; the xml in output directly; an export to a file that could be used directly by the setpolicy, ...

Copy link
Contributor

@fviard fviard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't put individual review comments as I don't think that the PR would be necessary as we have the "info" command with that,
but in the case that you would have a good argument for the dedicated commands, here are the issues I see with your PR:

  • "list_policy" is not needed because "get_policy" already does that.
  • Only bucket have policies, not the individual objects
  • Your 2 new commands only output the input uri and nothing else...
    (In fact, I discovered today that if you return a string to sys.exit, it will print it, so I see how you saw your acl, but we are not supposed to do that in the code, just return an error code at the end of the function)

@marsteg
Copy link
Contributor Author

marsteg commented Oct 16, 2025

I've updated the PR. To me, the explicit commands were useful. I cannot say how it is for others, but i think it's more confusing to expect the ACLs in an "info" command rather than having explicit one (i didn't find the info command 6 months ago, when I used this). But obviously this is your call :)

Copy link
Contributor

@fviard fviard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% convinced that the dedicated commands are needed, but I'm not against adding them.
But there are a few more changes to be done that you will find with this review.

@marsteg
Copy link
Contributor Author

marsteg commented Oct 17, 2025

I'm not 100% convinced that the dedicated commands are needed, but I'm not against adding them. But there are a few more changes to be done that you will find with this review.

Thanks a lot for the hints! To make both our efforts not worthless, i've updated everything based on your feedback but kept it to a minimum (didn't add a parameter for the raw acl output). Thanks a lot for your suggestions!

Copy link
Contributor

@fviard fviard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for my long delay to reply sometimes and thank you for your own reactivity.
It is almost good, but it just remains a few small things and then we will be good to go :-)

btw I tested that it works ok for the nick with aws. In fact, for the moment, we will have the nick of the owner just for the bucket and not for buckets, so your code is working well in both cases.

@marsteg marsteg requested a review from fviard October 22, 2025 06:49
@marsteg
Copy link
Contributor Author

marsteg commented Oct 22, 2025

Hey, No worries - happy that we move forward :)

@fviard fviard merged commit cee84f9 into s3tools:master Oct 22, 2025
8 checks passed
@fviard
Copy link
Contributor

fviard commented Oct 22, 2025

All good now and merged, thank you!

@fviard fviard added this to the 2.4.1 milestone Oct 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants