Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

set-json to fail on custom policies without .json #4745

Merged
merged 8 commits into from
Nov 27, 2023

Conversation

r-scheele
Copy link
Contributor

@r-scheele r-scheele commented Nov 4, 2023

Fix set-json command to fail on custom policies without .json

This commit addresses the bug where the set-json command incorrectly
succeeds for custom policies even when the policy file doesn't have a .json
extension. The fix ensures that the command fails as expected, prompting the user to use the correct file.

./mc anonymous set-json myminio/mydata ~/test/anonymous.json

Now, set-json will return an error message if the provided policy file is not a .json, aligning the behavior with the documented functionality.

Related to the discussion with @kannappanr

Copy link
Contributor

@klauspost klauspost left a comment

Choose a reason for hiding this comment

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

File extension shouldn't really matter IMO.

@r-scheele
Copy link
Contributor Author

File extension shouldn't really matter IMO.

The previous code only validates the file ext, so i think only .json policies are valid, i've added more validations asides the file ext and better error handling that prompted the discussion with @kannappanr. PTAL @klauspost @harshavardhana

File extension shouldn't really matter IMO.

@@ -28,12 +33,50 @@ func (b accessPerms) isValidAccessPERM() bool {
return false
}

type PolicyDocument struct {
Copy link
Member

Choose a reason for hiding this comment

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

why a new struct - can you not use pkg/policy ?

@harshavardhana
Copy link
Member

File extension shouldn't really matter IMO.

The previous code only validates the file ext, so i think only .json policies are valid, i've added more validations asides the file ext and better error handling that prompted the discussion with @kannappanr. PTAL @klauspost @harshavardhana

File extension shouldn't really matter IMO.

the file extension check can be relaxed though.

@r-scheele
Copy link
Contributor Author

File extension shouldn't really matter IMO.

The previous code only validates the file ext, so i think only .json policies are valid, i've added more validations asides the file ext and better error handling that prompted the discussion with @kannappanr. PTAL @klauspost @harshavardhana

File extension shouldn't really matter IMO.

the file extension check can be relaxed though.

Do you mean I should remove it?

@harshavardhana
Copy link
Member

File extension shouldn't really matter IMO.

The previous code only validates the file ext, so i think only .json policies are valid, i've added more validations asides the file ext and better error handling that prompted the discussion with @kannappanr. PTAL @klauspost @harshavardhana

File extension shouldn't really matter IMO.

the file extension check can be relaxed though.

Do you mean I should remove it?

Yes @r-scheele

@@ -52,7 +52,7 @@ var anonymousCmd = cli.Command{

USAGE:
{{.HelpName}} [FLAGS] set PERMISSION TARGET
{{.HelpName}} [FLAGS] set-json TARGET FILE
{{.HelpName}} [FLAGS] set-json FILE TARGET
Copy link
Member

Choose a reason for hiding this comment

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

Can you update the example below as well ?

cmd/access-perms.go Show resolved Hide resolved
@harshavardhana harshavardhana merged commit 5e6ae21 into minio:master Nov 27, 2023
5 checks passed
@subnix subnix mentioned this pull request Dec 14, 2023
8 tasks
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.

None yet

4 participants