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
Restrict adding or editing custom attributes via API by section #13360
Conversation
@miq-bot add_label api |
a90961b
to
bd5fabc
Compare
@enoodle @simon3z @abellotti please review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
@@ -179,15 +179,24 @@ | |||
expect_bad_request("Must specify a name") | |||
end | |||
|
|||
it "add custom attribute to a provider with forbidden section" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: change name to "blocks adding ..." or "prevents .."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thanks!
@@ -91,7 +91,12 @@ def format_provider_custom_attributes(attribute) | |||
attribute["value"] = attribute.delete("field_type").safe_constantize.parse(attribute["value"]) | |||
end | |||
attribute["section"] ||= "metadata" unless @req.action == "edit" | |||
if attribute["section"].present? && !(CustomAttribute::ALLOWED_API_SECTIONS.include? attribute["section"]) | |||
raise ForbiddenError, "Unauthorized attribute section specified: #{attribute["section"]}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be a BadRequestError, ForbiddenError is meant for HTTP 403 (i.e. resource RBAC) maybe change to:
raise "Invalid attribute section specified: #{attribute["section"]}"
and remove the rescue ForbiddenErrror below, it will be picked up by the rescue and raise in lines 100-101.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abellotti Actually raising ForbiddenError
was my intention, since we don't want to allow adding or editing cas with unauthorized sections.
@simon3z what is your opinion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we'd still not be allowing it with the BadRequestError (400), 403's definition is at higher level than a custom attribute section of a resource.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abellotti Done, thanks.
bd5fabc
to
e6ccdc3
Compare
e6ccdc3
to
3f5a7b4
Compare
The only supported sections are: “metadata” and “cluster_settings”
3f5a7b4
to
0631d36
Compare
Checked commit dkorn@0631d36 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1 |
LGTM!! |
@miq-bot add_label euwe/yes |
@simaishi PTAL |
Restrict adding or editing custom attributes via API by section (cherry picked from commit aa69a4f) https://bugzilla.redhat.com/show_bug.cgi?id=1414451
Euwe backport details:
|
The only supported sections are: “metadata” and “cluster_settings”