Improve create picture validation #13697

jntullo · 2017-01-30T19:39:51Z

This PR improves validation for creating pictures via the api by:

validating the presence of content
validating that the content is in the expected format (base64) via Base64.strict_decode64

@miq-bot assign @abellotti
@miq-bot add_label enhancement, api

imtayadeway · 2017-01-30T19:47:43Z

app/controllers/api/pictures_controller.rb

@@ -1,11 +1,26 @@
 module Api
  class PicturesController < BaseController
    def create_resource(_type, _id, data)
+      validate_picture_data(data)


Did you consider putting this validation in the Picture model?

@imtayadeway I did, actually. I actually looked into validating the actual content type to ensure it is a picture via Paperclip (they have built in validation for attachments that I thought I could leverage since we already use it) and some other methods. It wasn't that straight forward though, so I felt like I was going down a rabbit hole. However if you think that's the way I should go, I can spend more time on it.

@jntullo I'm guessing that if there was (or ever is) a way to great a bogus picture outside of this controller, and that's not wanted for the same reason, then it may belong in the model. I am assuming here that this is the case, but I don't know if that is so. But if it is, is there a way to push what logic you have here down into the model and then maybe iterate on it to see if you can delegate to Paperclip as a follow up?

abellotti · 2017-02-01T19:10:28Z

I'm 👍 with this. @imtayadeway ?

jntullo · 2017-02-01T19:23:26Z

@abellotti will have some updates up shortly 😄

use strict_decode to validate base64 encryption

jntullo · 2017-02-01T19:54:42Z

app/controllers/api/pictures_controller.rb

-      picture
+      raise 'requires content' unless data['content']
+      Picture.new(data.except('content')).tap do |picture|
+        picture.content = Base64.strict_decode64(data['content'])


@imtayadeway strict_decode64 still doesn't like nils, so I had to keep in the requires content validation

@imtayadeway no it doesn't, so I raise requires content if it's not present

imtayadeway · 2017-02-01T20:14:34Z

spec/models/picture_spec.rb

@@ -1,15 +1,25 @@
 describe Picture do
  subject { FactoryGirl.build :picture }

+  before do


Did you consider doing subject { FactoryGirl.build(:picture, :content => "foo") } above?

imtayadeway · 2017-02-01T20:56:22Z

spec/models/picture_spec.rb

@@ -90,6 +95,28 @@
    end
  end

+  context '#create_from_base64' do


Nit, but this should be .create_from_base64

imtayadeway · 2017-02-01T21:03:30Z

spec/requests/api/service_templates_spec.rb

@@ -13,7 +13,7 @@
  let(:ra1)        { FactoryGirl.create(:resource_action, :action => "Provision", :dialog => dialog1) }
  let(:ra2)        { FactoryGirl.create(:resource_action, :action => "Retirement", :dialog => dialog2) }

-  let(:picture)    { FactoryGirl.create(:picture, :extension => "jpg") }
+  let(:picture)    { FactoryGirl.create(:picture, :extension => "jpg", :content => 'foo') }


I'm assuming that this needed to be added because it fails to create now that :content is required. I think it would be a good idea to add this to the factory, and then this doesn't need to be changed (assuming that it doesn't test any behavior that's specific to the content)

imtayadeway · 2017-02-01T21:09:49Z

spec/requests/api/picture_spec.rb

  let(:dialog1)  { FactoryGirl.create(:dialog, :label => "ServiceDialog1") }
  let(:ra1)      { FactoryGirl.create(:resource_action, :action => "Provision", :dialog => dialog1) }
-  let(:picture)  { FactoryGirl.create(:picture, :extension => "jpg") }
+  let(:picture)  { FactoryGirl.create(:picture, :extension => "jpg", :content => content) }


Doesn't this give the picture a with un-decoded content? Which I know probably doesn't matter, but it does help the next person looking at this 😄

imtayadeway · 2017-02-01T21:13:09Z

spec/requests/api/picture_spec.rb

@@ -63,26 +67,10 @@ def expect_result_to_include_picture_href(source_id)
  end

  describe 'POST /api/pictures' do
-    # one pixel png image encoded in Base64


Even though this looks like gibberish, I sort of liked this because it was a technically a valid image, giving anyone looking for documentation in these specs some idea of what to expect. The shorter, valid base 64 code above may suffice though. @abellotti any thoughts?

@imtayadeway strict_decode64 had some issues with all of the newlines. Apparently it might not have been base64 😳

Ah, that makes sense. So you'd need a really really really long line 😝

Or, you could use this pattern:

"foo"\ "bar"\ "baz" # => "foobarbaz"

Or, you could store the pixel as a fixture on the disk? I don't know.....could all be OTP

imtayadeway · 2017-02-02T14:20:54Z

spec/factories/picture.rb

@@ -1,4 +1,7 @@
 FactoryGirl.define do
  factory :picture do
+    after(:build) do |x|


Does content "foo" not work here?

@imtayadeway no

That seems like a bug to me. It should use the custom setter internally

imtayadeway · 2017-02-02T14:22:48Z

spec/models/picture_spec.rb

@@ -1,15 +1,21 @@
 describe Picture do
-  subject { FactoryGirl.build :picture }
+  subject { FactoryGirl.build(:picture, :content => 'foo') }


Is this change still needed, since you updated the factory?

@imtayadeway nope. meant to remove this

imtayadeway · 2017-02-02T14:23:44Z

@jntullo this is looking great. Couple of nits above, but 👍

abellotti · 2017-02-02T20:01:28Z

In the latest commits, I didn't see the API tests, even though we're leveraging the model for picture validation, the API should have the 3 tests, missing extension, missing content and invalid picture (model validation failure caught by API and returning the BadRequestError).

abellotti · 2017-02-02T20:07:03Z

We should also have the model check for the extension existence and validity (png, jpg, svg).

adding content to picture factory update specs

miq-bot · 2017-02-03T13:32:26Z

Checked commits jntullo/manageiq@2a47718~...cb92bf1 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
5 files checked, 0 offenses detected
Everything looks good. 🍰

abellotti · 2017-02-03T14:40:59Z

spec/requests/api/picture_spec.rb

+      "MS4wLyI+CiAgICAgICAgIDx0aWZmOk9yaWVudGF0aW9uPjE8L3RpZmY6T3Jp"\
+      "ZW50YXRpb24+CiAgICAgIDwvcmRmOkRlc2NyaXB0aW9uPgogICA8L3JkZjpS"\
+      "REY+CjwveDp4bXBtZXRhPgpMwidZAAAADUlEQVQIHWNgYGCwBQAAQgA+3N0+"\
+      "xQAAAABJRU5ErkJggg=="


Thanks for the updates @jntullo, question about above change that is driven by using strict_decode64. does this mean we just broke compatibility with v2.3.0 or the old usage with \n would never really occur ? How does the current service-ui generate the content in these payloads ? Thanks.

@abellotti the old usage with \n would never occur. When thinking about it, not really sure why I put that there in the first place, it's a mistake on my part. The current service-ui isn't uploading pictures via the pictures API - yet

abellotti · 2017-02-03T15:24:06Z

Thanks @jntullo 👍

himdel · 2017-02-03T18:58:57Z

When adding euwe/yes or euwe/no, please add the same to ManageIQ/manageiq-ui-classic#299.

because of new validations added to Picture in ManageIQ/manageiq#13697, a Picture model always requires content and extension, and can't be created using argumentless Picture.new anymore. Updating CatalogController to either create the model directly, or update the existing one.

improve validation of pictures API

2a47718

miq-bot assigned abellotti Jan 30, 2017

miq-bot added api enhancement labels Jan 30, 2017

imtayadeway reviewed Jan 30, 2017

View reviewed changes

add content validation on the model

022eed7

use strict_decode to validate base64 encryption

jntullo commented Feb 1, 2017

View reviewed changes

imtayadeway reviewed Feb 1, 2017

View reviewed changes

imtayadeway reviewed Feb 2, 2017

View reviewed changes

create_from_base64

cb92bf1

adding content to picture factory update specs

abellotti reviewed Feb 3, 2017

View reviewed changes

abellotti added this to the Sprint 54 Ending Feb 13, 2017 milestone Feb 3, 2017

abellotti merged commit 31d19f2 into ManageIQ:master Feb 3, 2017

jntullo deleted the enhancement/pictures_api_validation branch February 3, 2017 15:26

himdel mentioned this pull request Feb 3, 2017

CatalogController: Create Picture with content & extension ManageIQ/manageiq-ui-classic#299

Merged

abellotti mentioned this pull request Feb 14, 2017

creating pictures should do better validation #12533

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve create picture validation #13697

Improve create picture validation #13697

jntullo commented Jan 30, 2017 •

edited by miq-bot

Loading

imtayadeway Jan 30, 2017

jntullo Jan 30, 2017

imtayadeway Jan 30, 2017

abellotti commented Feb 1, 2017

jntullo commented Feb 1, 2017

jntullo Feb 1, 2017

jntullo Feb 1, 2017

imtayadeway Feb 1, 2017

imtayadeway Feb 1, 2017

imtayadeway Feb 1, 2017

imtayadeway Feb 1, 2017

imtayadeway Feb 1, 2017

jntullo Feb 1, 2017

imtayadeway Feb 1, 2017

imtayadeway Feb 1, 2017

imtayadeway Feb 2, 2017

jntullo Feb 2, 2017

imtayadeway Feb 2, 2017

imtayadeway Feb 2, 2017

jntullo Feb 2, 2017

imtayadeway commented Feb 2, 2017

abellotti commented Feb 2, 2017

abellotti commented Feb 2, 2017

miq-bot commented Feb 3, 2017

abellotti Feb 3, 2017

jntullo Feb 3, 2017

abellotti commented Feb 3, 2017

himdel commented Feb 3, 2017

Improve create picture validation #13697

Improve create picture validation #13697

Conversation

jntullo commented Jan 30, 2017 • edited by miq-bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abellotti commented Feb 1, 2017

jntullo commented Feb 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imtayadeway commented Feb 2, 2017

abellotti commented Feb 2, 2017

abellotti commented Feb 2, 2017

miq-bot commented Feb 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abellotti commented Feb 3, 2017

himdel commented Feb 3, 2017

jntullo commented Jan 30, 2017 •

edited by miq-bot

Loading