From b72b88006e9f0027336d94ac2d5adcbaa21e4dd9 Mon Sep 17 00:00:00 2001 From: Robb Kidd Date: Thu, 18 Jan 2018 12:03:13 -0500 Subject: [PATCH] validate a cookbook version against chef's required format A version for a cookbook must be either X.Y.Z or X.Y.[1] Up until this change, the validation for a CookbookVersion's version was against SemVer rules. SemVer allows for release labels or build info to appear after the version number itself. Chef does not! Here, we add a new ChefVersionValidator that reuses the existing version validation logic of the Chef gem in ::Chef::Version. Supermarket is already relying on the Chef gem to validate version constraints set for cookbook dependencies and supported platform. Fixes #1714 to return an error to any client if a cookbook is uploaded with an invalid version. [1] https://docs.chef.io/cookbook_versions.html#constraints Signed-off-by: Robb Kidd --- .../validations/chef_version_validator.rb | 43 +++++++++++++ .../app/models/cookbook_version.rb | 19 +----- .../spec/models/cookbook_upload_spec.rb | 13 ++++ .../spec/models/cookbook_version_spec.rb | 64 +++++++++++-------- 4 files changed, 97 insertions(+), 42 deletions(-) create mode 100644 src/supermarket/app/lib/active_model/validations/chef_version_validator.rb diff --git a/src/supermarket/app/lib/active_model/validations/chef_version_validator.rb b/src/supermarket/app/lib/active_model/validations/chef_version_validator.rb new file mode 100644 index 0000000000..2e80494f6f --- /dev/null +++ b/src/supermarket/app/lib/active_model/validations/chef_version_validator.rb @@ -0,0 +1,43 @@ +require 'active_model' +require 'chef/version_class' +require 'chef/exceptions' + +module ActiveModel + module Validations + # + # Validates that strings are formatted as Chef version constraints. + # + class ChefVersionValidator < ActiveModel::EachValidator + # + # Create a new validator. Called implicitly when + # a +chef_version+ validation is added to an attribute. + # + # @param options [Hash] validation options + # @option options [String] :message + # ('is not a valid Chef version') a custom validation message + # + def initialize(options) + options.fetch(:message) do + options.store(:message, 'is not a valid Chef version') + end + + super(options) + end + + # + # Add an error to +attribute+ of +record+ if the given +value+ is not + # a valid Chef version constraint + # + # @param record [ActiveModel::Model] + # @param attribute [Symbol] + # @param value + # + def validate_each(record, attribute, value) + Chef::Version.new(value) + rescue Chef::Exceptions::InvalidCookbookVersion => e + msg = "#{options.fetch(:message)}. #{e.class}: #{e.message}" + record.errors.add(attribute, msg, value: value) + end + end + end +end diff --git a/src/supermarket/app/models/cookbook_version.rb b/src/supermarket/app/models/cookbook_version.rb index ab7ea71d6d..602a1dc412 100644 --- a/src/supermarket/app/models/cookbook_version.rb +++ b/src/supermarket/app/models/cookbook_version.rb @@ -1,3 +1,5 @@ +require 'active_model/validations/chef_version_validator' + class CookbookVersion < ApplicationRecord include SeriousErrors @@ -20,8 +22,7 @@ class CookbookVersion < ApplicationRecord validates :license, presence: true, length: { maximum: 255 } validates :description, presence: true validates :readme, presence: true - validates :version, presence: true, uniqueness: { scope: :cookbook } - validate :semantic_version + validates :version, presence: true, uniqueness: { scope: :cookbook }, chef_version: true validates_attachment( :tarball, presence: true, @@ -95,18 +96,4 @@ def metric_result_pass_rate '-' end end - - private - - # - # Ensure that the version string we have been given conforms to semantic - # versioning at http://semver.org - # - def semantic_version - begin - Semverse::Version.new(version) - rescue Semverse::InvalidVersionFormat - errors.add(:version, 'is formatted incorrectly') - end - end end diff --git a/src/supermarket/spec/models/cookbook_upload_spec.rb b/src/supermarket/spec/models/cookbook_upload_spec.rb index 4b10257439..c266ab01ed 100644 --- a/src/supermarket/spec/models/cookbook_upload_spec.rb +++ b/src/supermarket/spec/models/cookbook_upload_spec.rb @@ -122,6 +122,19 @@ expect(version).to be_present end + it 'yields an error if the version number is not a valid Chef version' do + tarball = build_cookbook_tarball('invalid_version') do |tar| + tar.file('metadata.json') { JSON.dump(name: 'invalid_version', version: '1.2.3-rc4') } + tar.file('README.md') { "# Check for a bad version" } + end + + upload = CookbookUpload.new(user, cookbook: '{}' , tarball: tarball) + errors = upload.finish { |e, _| e } + + expect(errors.full_messages). + to include( a_string_matching('not a valid Chef version') ) + end + it 'yields an error if the cookbook is not valid JSON' do upload = CookbookUpload.new(user, cookbook: 'ack!', tarball: 'tarball') errors = upload.finish { |e, _| e } diff --git a/src/supermarket/spec/models/cookbook_version_spec.rb b/src/supermarket/spec/models/cookbook_version_spec.rb index 74528d8f67..6f38335e6f 100644 --- a/src/supermarket/spec/models/cookbook_version_spec.rb +++ b/src/supermarket/spec/models/cookbook_version_spec.rb @@ -11,37 +11,49 @@ it { should validate_presence_of(:description) } it { should validate_presence_of(:version) } it { should validate_presence_of(:readme) } - it "validates uniqueness of version for a cookbook" do - cookbook = create(:cookbook) - create(:cookbook_version, cookbook: cookbook) - should validate_uniqueness_of(:version).scoped_to(:cookbook_id) - end it { should validate_length_of(:license).is_at_most(255) } - it 'seriously validates the uniqueness of cookbook version numbers' do - cookbook = create(:cookbook) - cookbook_version = create(:cookbook_version, cookbook: cookbook) + context 'for version number' do + let(:cookbook) { create(:cookbook) } + let(:cookbook_version) { create(:cookbook_version, cookbook: cookbook) } - duplicate_version = CookbookVersion.new( - cookbook: cookbook, - license: cookbook_version.license, - tarball: cookbook_version.tarball, - version: cookbook_version.version - ) + it 'passes for a valid version number format x.y.z' do + cookbook_version.version = '1.2.3' + expect(cookbook_version).to be_valid + expect(cookbook_version.errors[:version]).to be_empty + end - expect do - duplicate_version.save(validate: false) - end.to raise_error(ActiveRecord::RecordNotUnique) - end + it "validates uniqueness of version for a cookbook" do + cookbook_version + should validate_uniqueness_of(:version).scoped_to(:cookbook_id) + end - it 'validates that the version number is semantically correct' do - cookbook = create(:cookbook) - cookbook_version = build(:cookbook_version, cookbook: cookbook, version: 'hahano') - expect(cookbook_version).to_not be_valid - expect(cookbook_version.errors[:version]).to_not be_empty - cookbook_version.version = '8.7.6' - expect(cookbook_version).to be_valid - expect(cookbook_version.errors[:version]).to be_empty + it 'seriously validates the uniqueness of cookbook version numbers' do + duplicate_version = CookbookVersion.new( + cookbook: cookbook, + license: cookbook_version.license, + tarball: cookbook_version.tarball, + version: cookbook_version.version + ) + + expect do + duplicate_version.save(validate: false) + end.to raise_error(ActiveRecord::RecordNotUnique) + end + + context 'if not a valid chef version' do + it 'fails on just a string of silliness' do + cookbook_version.version = 'hahahano' + expect(cookbook_version).to_not be_valid + expect(cookbook_version.errors[:version]).to include( a_string_matching('not a valid Chef version') ) + end + + it 'fails on a dashed extra info like RC or pre' do + cookbook_version.version = '1.2.3-RC1' + expect(cookbook_version).to_not be_valid + expect(cookbook_version.errors[:version]).to include( a_string_matching('not a valid Chef version') ) + end + end end context 'when something that is not a tarball is given' do