From 2a47718353fddf2d934d882ffdf7b40502a907a8 Mon Sep 17 00:00:00 2001 From: Jillian Tullo Date: Mon, 30 Jan 2017 12:33:20 -0500 Subject: [PATCH 1/3] improve validation of pictures API --- app/controllers/api/pictures_controller.rb | 23 +++++++++-- spec/requests/api/picture_spec.rb | 45 ++++++++++++++++++++++ 2 files changed, 64 insertions(+), 4 deletions(-) diff --git a/app/controllers/api/pictures_controller.rb b/app/controllers/api/pictures_controller.rb index 532252f30d3..deb31961ebe 100644 --- a/app/controllers/api/pictures_controller.rb +++ b/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) data['content'] = Base64.decode64(data['content']) - picture = Picture.create(data) - raise BadRequestError, - "Failed to create Picture - #{picture.errors.full_messages.join(', ')}" unless picture.valid? - picture + Picture.create(data) + rescue => err + raise BadRequestError, "Failed to create Picture - #{err}" + end + + private + + def validate_picture_data(data) + raise 'requires an extension' unless data['extension'] + if data['content'] + raise 'content is not base64' unless base64_content?(data['content']) + else + raise 'requires content' + end + end + + def base64_content?(content) + (content.length % 4).zero? && (content =~ %r{^[A-Za-z0-9+\/=]+\Z}) end end end diff --git a/spec/requests/api/picture_spec.rb b/spec/requests/api/picture_spec.rb index 47e0cc832ce..a2f00fe3514 100644 --- a/spec/requests/api/picture_spec.rb +++ b/spec/requests/api/picture_spec.rb @@ -117,5 +117,50 @@ def expect_result_to_include_picture_href(source_id) expect(response.parsed_body).to include(expected) expect(response).to have_http_status(:ok) end + + it 'rejects a picture that is not Base64 encoded' do + api_basic_authorize collection_action_identifier(:pictures, :create) + + run_post pictures_url, :extension => 'png', :content => 'bogus' + + expected = { + 'error' => a_hash_including( + 'kind' => 'bad_request', + 'message' => a_string_matching(/content is not base64/), + ) + } + expect(response.parsed_body).to include(expected) + expect(response).to have_http_status(:bad_request) + end + + it 'requires an extension' do + api_basic_authorize collection_action_identifier(:pictures, :create) + + run_post pictures_url, :content => content + + expected = { + 'error' => a_hash_including( + 'kind' => 'bad_request', + 'message' => a_string_matching(/requires an extension/), + ) + } + expect(response.parsed_body).to include(expected) + expect(response).to have_http_status(:bad_request) + end + + it 'requires content' do + api_basic_authorize collection_action_identifier(:pictures, :create) + + run_post pictures_url, :extension => 'png' + + expected = { + 'error' => a_hash_including( + 'kind' => 'bad_request', + 'message' => a_string_matching(/requires content/), + ) + } + expect(response.parsed_body).to include(expected) + expect(response).to have_http_status(:bad_request) + end end end From 022eed7a0f0468fc1d0d31a0368582349c2cb625 Mon Sep 17 00:00:00 2001 From: Jillian Tullo Date: Wed, 1 Feb 2017 11:51:38 -0500 Subject: [PATCH 2/3] add content validation on the model use strict_decode to validate base64 encryption --- app/controllers/api/pictures_controller.rb | 23 +++-------- app/models/picture.rb | 1 + spec/models/picture_spec.rb | 23 +++++++---- spec/requests/api/collections_spec.rb | 2 +- spec/requests/api/picture_spec.rb | 43 ++++----------------- spec/requests/api/service_templates_spec.rb | 2 +- 6 files changed, 32 insertions(+), 62 deletions(-) diff --git a/app/controllers/api/pictures_controller.rb b/app/controllers/api/pictures_controller.rb index deb31961ebe..029a150158e 100644 --- a/app/controllers/api/pictures_controller.rb +++ b/app/controllers/api/pictures_controller.rb @@ -1,26 +1,13 @@ module Api class PicturesController < BaseController def create_resource(_type, _id, data) - validate_picture_data(data) - data['content'] = Base64.decode64(data['content']) - Picture.create(data) + raise 'requires content' unless data['content'] + Picture.new(data.except('content')).tap do |picture| + picture.content = Base64.strict_decode64(data['content']) + picture.save! + end rescue => err raise BadRequestError, "Failed to create Picture - #{err}" end - - private - - def validate_picture_data(data) - raise 'requires an extension' unless data['extension'] - if data['content'] - raise 'content is not base64' unless base64_content?(data['content']) - else - raise 'requires content' - end - end - - def base64_content?(content) - (content.length % 4).zero? && (content =~ %r{^[A-Za-z0-9+\/=]+\Z}) - end end end diff --git a/app/models/picture.rb b/app/models/picture.rb index ffb4d4f388c..f9694454f50 100644 --- a/app/models/picture.rb +++ b/app/models/picture.rb @@ -4,6 +4,7 @@ class Picture < ApplicationRecord validates :extension, :inclusion => { :in => %w(png jpg svg), :message => 'must be a png, jpg, or svg' }, :if => :extension + validates :content, :presence => true virtual_has_one :image_href, :class_name => "String" diff --git a/spec/models/picture_spec.rb b/spec/models/picture_spec.rb index c45c4bad4bb..cb5ae61489a 100644 --- a/spec/models/picture_spec.rb +++ b/spec/models/picture_spec.rb @@ -1,15 +1,25 @@ describe Picture do subject { FactoryGirl.build :picture } + before do + subject.content = 'foo' + end + it "auto-creates needed directory" do expect(File.directory?(described_class.directory)).to be_truthy end - it "#content" do - expect(subject.content).to be_nil - expected = "FOOBAR" - subject.content = expected.dup - expect(subject.content).to eq(expected) + context "#content" do + it 'returns expected content' do + expected = "FOOBAR" + subject.content = expected.dup + expect(subject.content).to eq(expected) + end + + it 'requires content' do + subject.content = '' + expect(subject.valid?).to be_falsey + end end context "#extension" do @@ -65,9 +75,8 @@ end it "#size" do - expect(subject.size).to eq(0) expected = "FOOBAR" - subject.content = expected.dup + subject.content = expected.dup expect(subject.size).to eq(expected.length) end diff --git a/spec/requests/api/collections_spec.rb b/spec/requests/api/collections_spec.rb index 0a6d226e415..54b531348f5 100644 --- a/spec/requests/api/collections_spec.rb +++ b/spec/requests/api/collections_spec.rb @@ -129,7 +129,7 @@ def test_collection_bulk_query(collection, collection_url, klass, id = nil) end it "query Pictures" do - FactoryGirl.create(:picture) + FactoryGirl.create(:picture, :content => 'foo') test_collection_query(:pictures, pictures_url, Picture) end diff --git a/spec/requests/api/picture_spec.rb b/spec/requests/api/picture_spec.rb index a2f00fe3514..636d7007b08 100644 --- a/spec/requests/api/picture_spec.rb +++ b/spec/requests/api/picture_spec.rb @@ -6,9 +6,13 @@ # - Query picture and image_href of service_requests /api/service_requests/:id?attributes=picture,picture.image_href # describe "Pictures" do + # Valid base64 + let(:content) do + "aW1hZ2U=" + end 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) } let(:template) do FactoryGirl.create(:service_template, :name => "ServiceTemplate", @@ -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 - let(:content) do - "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAABGdBTUEAALGP\n"\ - "C/xhBQAAACBjSFJNAAB6JgAAgIQAAPoAAACA6AAAdTAAAOpgAAA6mAAAF3Cc\n"\ - "ulE8AAAACXBIWXMAAAsTAAALEwEAmpwYAAABWWlUWHRYTUw6Y29tLmFkb2Jl\n"\ - "LnhtcAAAAAAAPHg6eG1wbWV0YSB4bWxuczp4PSJhZG9iZTpuczptZXRhLyIg\n"\ - "eDp4bXB0az0iWE1QIENvcmUgNS40LjAiPgogICA8cmRmOlJERiB4bWxuczpy\n"\ - "ZGY9Imh0dHA6Ly93d3cudzMub3JnLzE5OTkvMDIvMjItcmRmLXN5bnRheC1u\n"\ - "cyMiPgogICAgICA8cmRmOkRlc2NyaXB0aW9uIHJkZjphYm91dD0iIgogICAg\n"\ - "ICAgICAgICB4bWxuczp0aWZmPSJodHRwOi8vbnMuYWRvYmUuY29tL3RpZmYv\n"\ - "MS4wLyI+CiAgICAgICAgIDx0aWZmOk9yaWVudGF0aW9uPjE8L3RpZmY6T3Jp\n"\ - "ZW50YXRpb24+CiAgICAgIDwvcmRmOkRlc2NyaXB0aW9uPgogICA8L3JkZjpS\n"\ - "REY+CjwveDp4bXBtZXRhPgpMwidZAAAADUlEQVQIHWNgYGCwBQAAQgA+3N0+\n"\ - "xQAAAABJRU5ErkJggg==\n" - end - it 'rejects create without an appropriate role' do api_basic_authorize - run_post pictures_url, :extension => 'png', :content => content + run_post pictures_url, :content => content expect(response).to have_http_status(:forbidden) end @@ -118,7 +106,7 @@ def expect_result_to_include_picture_href(source_id) expect(response).to have_http_status(:ok) end - it 'rejects a picture that is not Base64 encoded' do + it 'rejects a bad picture' do api_basic_authorize collection_action_identifier(:pictures, :create) run_post pictures_url, :extension => 'png', :content => 'bogus' @@ -126,22 +114,7 @@ def expect_result_to_include_picture_href(source_id) expected = { 'error' => a_hash_including( 'kind' => 'bad_request', - 'message' => a_string_matching(/content is not base64/), - ) - } - expect(response.parsed_body).to include(expected) - expect(response).to have_http_status(:bad_request) - end - - it 'requires an extension' do - api_basic_authorize collection_action_identifier(:pictures, :create) - - run_post pictures_url, :content => content - - expected = { - 'error' => a_hash_including( - 'kind' => 'bad_request', - 'message' => a_string_matching(/requires an extension/), + 'message' => a_string_matching(/invalid base64/), ) } expect(response.parsed_body).to include(expected) diff --git a/spec/requests/api/service_templates_spec.rb b/spec/requests/api/service_templates_spec.rb index 3f258894b7a..45cc4b8111e 100644 --- a/spec/requests/api/service_templates_spec.rb +++ b/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') } let(:template) { FactoryGirl.create(:service_template, :name => "ServiceTemplate") } describe "Service Templates query" do From cb92bf1a08bc88cd9786cb9195fec129127b2467 Mon Sep 17 00:00:00 2001 From: Jillian Tullo Date: Wed, 1 Feb 2017 15:51:01 -0500 Subject: [PATCH 3/3] create_from_base64 adding content to picture factory update specs --- app/controllers/api/pictures_controller.rb | 6 +-- app/models/picture.rb | 12 ++++- spec/factories/picture.rb | 4 ++ spec/models/picture_spec.rb | 44 ++++++++++++------ spec/requests/api/collections_spec.rb | 2 +- spec/requests/api/picture_spec.rb | 50 +++++++++++++++------ spec/requests/api/service_templates_spec.rb | 2 +- 7 files changed, 84 insertions(+), 36 deletions(-) diff --git a/app/controllers/api/pictures_controller.rb b/app/controllers/api/pictures_controller.rb index 029a150158e..08f2c43d42d 100644 --- a/app/controllers/api/pictures_controller.rb +++ b/app/controllers/api/pictures_controller.rb @@ -1,11 +1,7 @@ module Api class PicturesController < BaseController def create_resource(_type, _id, data) - raise 'requires content' unless data['content'] - Picture.new(data.except('content')).tap do |picture| - picture.content = Base64.strict_decode64(data['content']) - picture.save! - end + Picture.create_from_base64(data) rescue => err raise BadRequestError, "Failed to create Picture - #{err}" end diff --git a/app/models/picture.rb b/app/models/picture.rb index f9694454f50..8073fcd78fd 100644 --- a/app/models/picture.rb +++ b/app/models/picture.rb @@ -2,8 +2,8 @@ class Picture < ApplicationRecord has_one :binary_blob, :as => :resource, :dependent => :destroy, :autosave => true validates :extension, - :inclusion => { :in => %w(png jpg svg), :message => 'must be a png, jpg, or svg' }, - :if => :extension + :presence => true, + :inclusion => { :in => %w(png jpg svg), :message => 'must be a png, jpg, or svg' } validates :content, :presence => true virtual_has_one :image_href, :class_name => "String" @@ -33,6 +33,14 @@ def self.url_path(filename, basepath = URL_ROOT) end end + def self.create_from_base64(attributes = {}) + attributes = attributes.with_indifferent_access + new(attributes.except(:content)).tap do |picture| + picture.content = Base64.strict_decode64(attributes[:content].to_s) + picture.save! + end + end + def content binary_blob.try(:binary) end diff --git a/spec/factories/picture.rb b/spec/factories/picture.rb index 9d16dca418f..c5b834ddbc2 100644 --- a/spec/factories/picture.rb +++ b/spec/factories/picture.rb @@ -1,4 +1,8 @@ FactoryGirl.define do factory :picture do + extension 'png' + after(:build) do |x| + x.content = 'foo' + end end end diff --git a/spec/models/picture_spec.rb b/spec/models/picture_spec.rb index cb5ae61489a..1453d9c6ee2 100644 --- a/spec/models/picture_spec.rb +++ b/spec/models/picture_spec.rb @@ -1,9 +1,5 @@ describe Picture do - subject { FactoryGirl.build :picture } - - before do - subject.content = 'foo' - end + subject { FactoryGirl.build(:picture) } it "auto-creates needed directory" do expect(File.directory?(described_class.directory)).to be_truthy @@ -23,8 +19,13 @@ end context "#extension" do + it 'is required' do + subject.extension = nil + expect(subject.valid?).to be_falsey + expect(subject.errors.messages).to eq(:extension => ["can't be blank", "must be a png, jpg, or svg"]) + end + it "accepts only png, jpg, or svg" do - expect(subject.extension).to be_nil subject.extension = "foo" expect(subject.valid?).to be_falsey @@ -41,7 +42,6 @@ end it "on new record" do - expect(subject.extension).to be_nil ext = "png" subject.extension = ext.dup expect(subject.extension).to eq(ext) @@ -57,9 +57,8 @@ it "on existing record" do subject.save - subject.reload - expect(subject.extension).to be_nil + ext = "jpg" subject.extension = ext.dup expect(subject.extension).to eq(ext) @@ -86,11 +85,6 @@ end context "works when record is saved" do - it "without extension" do - subject.save - expect(subject.basename).to eq("#{subject.compressed_id}.") - end - it "with extension" do subject.extension = "png" subject.save @@ -99,6 +93,28 @@ end end + context '.create_from_base64' do + let(:attributes) do + { + :extension => 'png', + :content => 'aW1hZ2U=' + } + end + + it 'creates a picture' do + expect do + Picture.create_from_base64(attributes) + end.to change(Picture, :count).by(1) + end + + it 'requires valid base64' do + attributes[:content] = 'bogus' + expect do + Picture.create_from_base64(attributes) + end.to raise_error(StandardError, 'invalid base64') + end + end + it "#filename" do basename = "foo.bar" allow(subject).to receive(:basename).and_return(basename) diff --git a/spec/requests/api/collections_spec.rb b/spec/requests/api/collections_spec.rb index 54b531348f5..0a6d226e415 100644 --- a/spec/requests/api/collections_spec.rb +++ b/spec/requests/api/collections_spec.rb @@ -129,7 +129,7 @@ def test_collection_bulk_query(collection, collection_url, klass, id = nil) end it "query Pictures" do - FactoryGirl.create(:picture, :content => 'foo') + FactoryGirl.create(:picture) test_collection_query(:pictures, pictures_url, Picture) end diff --git a/spec/requests/api/picture_spec.rb b/spec/requests/api/picture_spec.rb index 636d7007b08..64e6c4e527c 100644 --- a/spec/requests/api/picture_spec.rb +++ b/spec/requests/api/picture_spec.rb @@ -6,13 +6,9 @@ # - Query picture and image_href of service_requests /api/service_requests/:id?attributes=picture,picture.image_href # describe "Pictures" do - # Valid base64 - let(:content) do - "aW1hZ2U=" - end let(:dialog1) { FactoryGirl.create(:dialog, :label => "ServiceDialog1") } let(:ra1) { FactoryGirl.create(:resource_action, :action => "Provision", :dialog => dialog1) } - let(:picture) { FactoryGirl.create(:picture, :extension => "jpg", :content => content) } + let(:picture) { FactoryGirl.create(:picture, :extension => "jpg") } let(:template) do FactoryGirl.create(:service_template, :name => "ServiceTemplate", @@ -67,10 +63,26 @@ def expect_result_to_include_picture_href(source_id) end describe 'POST /api/pictures' do + # Valid base64 image + let(:content) do + "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAABGdBTUEAALGP"\ + "C/xhBQAAACBjSFJNAAB6JgAAgIQAAPoAAACA6AAAdTAAAOpgAAA6mAAAF3Cc"\ + "ulE8AAAACXBIWXMAAAsTAAALEwEAmpwYAAABWWlUWHRYTUw6Y29tLmFkb2Jl"\ + "LnhtcAAAAAAAPHg6eG1wbWV0YSB4bWxuczp4PSJhZG9iZTpuczptZXRhLyIg"\ + "eDp4bXB0az0iWE1QIENvcmUgNS40LjAiPgogICA8cmRmOlJERiB4bWxuczpy"\ + "ZGY9Imh0dHA6Ly93d3cudzMub3JnLzE5OTkvMDIvMjItcmRmLXN5bnRheC1u"\ + "cyMiPgogICAgICA8cmRmOkRlc2NyaXB0aW9uIHJkZjphYm91dD0iIgogICAg"\ + "ICAgICAgICB4bWxuczp0aWZmPSJodHRwOi8vbnMuYWRvYmUuY29tL3RpZmYv"\ + "MS4wLyI+CiAgICAgICAgIDx0aWZmOk9yaWVudGF0aW9uPjE8L3RpZmY6T3Jp"\ + "ZW50YXRpb24+CiAgICAgIDwvcmRmOkRlc2NyaXB0aW9uPgogICA8L3JkZjpS"\ + "REY+CjwveDp4bXBtZXRhPgpMwidZAAAADUlEQVQIHWNgYGCwBQAAQgA+3N0+"\ + "xQAAAABJRU5ErkJggg==" + end + it 'rejects create without an appropriate role' do api_basic_authorize - run_post pictures_url, :content => content + run_post pictures_url, :extension => 'png', :content => content expect(response).to have_http_status(:forbidden) end @@ -106,19 +118,18 @@ def expect_result_to_include_picture_href(source_id) expect(response).to have_http_status(:ok) end - it 'rejects a bad picture' do + it 'requires an extension' do api_basic_authorize collection_action_identifier(:pictures, :create) - run_post pictures_url, :extension => 'png', :content => 'bogus' + run_post pictures_url, :content => content expected = { 'error' => a_hash_including( - 'kind' => 'bad_request', - 'message' => a_string_matching(/invalid base64/), + 'message' => a_string_including("Extension can't be blank") ) } - expect(response.parsed_body).to include(expected) expect(response).to have_http_status(:bad_request) + expect(response.parsed_body).to include(expected) end it 'requires content' do @@ -128,12 +139,25 @@ def expect_result_to_include_picture_href(source_id) expected = { 'error' => a_hash_including( - 'kind' => 'bad_request', - 'message' => a_string_matching(/requires content/), + 'message' => a_string_including("Content can't be blank") ) } + expect(response).to have_http_status(:bad_request) expect(response.parsed_body).to include(expected) + end + + it 'requires content with valid base64' do + api_basic_authorize collection_action_identifier(:pictures, :create) + + run_post pictures_url, :content => 'not base64', :extension => 'png' + + expected = { + 'error' => a_hash_including( + 'message' => a_string_including('invalid base64') + ) + } expect(response).to have_http_status(:bad_request) + expect(response.parsed_body).to include(expected) end end end diff --git a/spec/requests/api/service_templates_spec.rb b/spec/requests/api/service_templates_spec.rb index 45cc4b8111e..3f258894b7a 100644 --- a/spec/requests/api/service_templates_spec.rb +++ b/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", :content => 'foo') } + let(:picture) { FactoryGirl.create(:picture, :extension => "jpg") } let(:template) { FactoryGirl.create(:service_template, :name => "ServiceTemplate") } describe "Service Templates query" do