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

rgw: Return Error if Bucket Policy Contians Undefined Action #17433

Merged
merged 2 commits into from Sep 26, 2017
Merged

rgw: Return Error if Bucket Policy Contians Undefined Action #17433

merged 2 commits into from Sep 26, 2017

Conversation

C2python
Copy link

@C2python C2python commented Sep 3, 2017

Signed-off-by: Wen Zhang zhangwen1@unionpay.com

Signed-off-by: Wen Zhang zhangwen1@unionpay.com
@@ -792,6 +792,8 @@ static optional<Principal> parse_principal(CephContext* cct, TokenID t,
bool ParseState::do_string(CephContext* cct, const char* s, size_t l) {
auto k = pp->tokens.lookup(s, l);
Policy& p = pp->policy;
bool isAction = false;

Choose a reason for hiding this comment

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

Is there a real need to take isAction variable?
Because whenever it goes inside case ((w->id == TokenID::Action) || (w->id == TokenID::NotAction)) it will loop through the for() .
And if it does not go inside ((w->id == TokenID::Action) || (w->id == TokenID::NotAction)) isValidAction is always false

Copy link
Author

Choose a reason for hiding this comment

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

If we do not mark isAction flag, will return false when w->id is another value. For example, w->id == TokenID::Effect, isVaildAction is false,. At finally, "if(!isValidAction)return false" will be executed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, looks reasonable,

Copy link
Contributor

Choose a reason for hiding this comment

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

nit, might be consistent with the naming convention: we don't use camelCase in Ceph, so i'd suggest rename this variable to is_action instead.

@C2python
Copy link
Author

C2python commented Sep 5, 2017

@adamemerson Could you review this? Tks~

@@ -792,6 +792,8 @@ static optional<Principal> parse_principal(CephContext* cct, TokenID t,
bool ParseState::do_string(CephContext* cct, const char* s, size_t l) {
auto k = pp->tokens.lookup(s, l);
Policy& p = pp->policy;
bool isAction = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, looks reasonable,

Signed-off-by: Wen Zhang zhangwen1@unionpay.com
@C2python
Copy link
Author

@tchaikov I just rename the variables. Could you review it? Tks~

@tchaikov
Copy link
Contributor

thanks, looks better =)

@C2python
Copy link
Author

@joscollin Could you merge it? Tks~

@joscollin
Copy link
Member

joscollin commented Sep 21, 2017

@C2python This PR is waiting for testing. See the needs-qa label.
Also, I usually refrain from merging (except doc PRs) and let the Team Leads to merge the PRs.

@mattbenjamin mattbenjamin merged commit fcc014c into ceph:master Sep 26, 2017
@mattbenjamin
Copy link
Contributor

@joscollin thanks for test runs, looks good

@joscollin
Copy link
Member

@mattbenjamin no problem :-)

@C2python C2python deleted the check_action branch September 27, 2017 04:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants