Added pxe server request validation.

Author: Hyperkid123

app/controllers/api/pxe_servers_controller.rb

@@ -1,21 +1,40 @@
 module Api
   class PxeServersController < BaseController
-    INVALID_PXE_SERVER_ATTRS = %w[id href].freeze # Cannot update or create these
-
     include Subcollections::PxeImages
     include Subcollections::PxeMenus
+    INVALID_ATTRIBUTES = {
+      "PxeServer" => %w[id href], # Cannot update or create these
+    }.freeze
+    PXE_ATTRIBUTES = {
+      "PxeServer"      => %w[name uri],
+      "PxeMenu"        => %w[file_name],
+      "Authentication" => %w[userid password]
+    }.freeze
 
     def create_resource(_type, _id, data = {})
-      validate_pxe_server_create_data(data)
+      authentication = data.delete('authentication')
       menus = data.delete('pxe_menus')
+      validate_data_for('PxeServer', data)
 
-      server = collection_class(:pxe_servers).create(data)
-      if server.invalid?
-        raise BadRequestError, "Failed to add a pxe server - #{server.errors.full_messages.join(', ')}"
-      end
+      PxeServer.transaction do
+        server = collection_class(:pxe_servers).new(data)
+        # generates uir_prefix which checks if server needs authentication or not
+        server.verify_uri_prefix_before_save
+
+        if server.requires_credentials? && server.missing_credentials?
+          validate_data_for('Authentication', authentication || {})
+          server.update_authentication({:default => authentication.compact}, {:save => true})
+        end
+
+        server.pxe_menus = create_pxe_menus(menus) if menus
+
+        if server.invalid?
+          raise BadRequestError, "Failed to add a pxe server - #{server.errors.full_messages.join(', ')}"
+        end
 
-      server.pxe_menus = create_pxe_menus(menus) if menus
-      server
+        server.save
+        server
+      end
     end
 
     def delete_resource(_type, id = nil, data = nil)
@@ -27,34 +46,33 @@ def delete_resource(_type, id = nil, data = nil)
     def edit_resource(type, id, data)
       server = resource_search(id, type, collection_class(:pxe_servers))
       menus = data.delete('pxe_menus')
-      if menus
-        server.pxe_menus.clear
-        data['pxe_menus'] = create_pxe_menus(menus)
+      authentication = data.delete('authentication')
+      PxeServer.transaction do
+        if menus
+          server.pxe_menus.clear
+          data['pxe_menus'] = create_pxe_menus(menus)
+        end
+        server.update!(data)
+        server.update_authentication({:default => authentication.transform_keys(&:to_sym)}, {:save => true}) if authentication && server.requires_credentials?
+        server
       end
-
-      server.update!(data)
-      server
     end
 
     private
 
     def create_pxe_menus(menus)
+      menus.each do |menu|
+        validate_data_for('PxeMenu', menu)
+      end
       menus.map do |menu|
         collection_class(:pxe_menus).create(menu)
       end
     end
 
-    def validate_pxe_server_data(data)
-      bad_attrs = data.keys.select { |k| INVALID_PXE_SERVER_ATTRS.include?(k) }.compact.join(", ")
-      raise BadRequestError, "Invalid attribute(s) #{bad_attrs} specified for a pxe server" if bad_attrs.present?
-    end
-
-    def validate_pxe_server_create_data(data)
-      validate_pxe_server_data(data)
-      req_attrs = %w[name uri]
+    def validate_data_for(klass, data)
       bad_attrs = []
-      req_attrs.each { |attr| bad_attrs << attr if data[attr].blank? }
-      raise BadRequestError, "Missing attribute(s) #{bad_attrs.join(', ')} for creating a pxe server" if bad_attrs.present?
+      PXE_ATTRIBUTES[klass].each { |attr| bad_attrs << attr if data[attr].blank? }
+      raise BadRequestError, "Missing attribute(s) #{bad_attrs.join(', ')} for creating a #{klass}" if bad_attrs.present?
     end
   end
 end

config/api.yml

@@ -2420,6 +2420,8 @@
       :post:
       - :name: create
         :identifier: pxe_server_create
+      - :name: edit
+        :identifier: pxe_server_edit
       :patch:
       - :name: edit
         :identifier: pxe_server_edit
@@ -2433,6 +2435,17 @@
       :get:
       - :name: read
         :identifier: pxe_server_view
+      :patch:
+      - :name: edit
+        :identifier: pxe_server_edit
+      :post:
+      - :name: create
+        :identifier: pxe_server_create
+      - :name: edit
+        :identifier: pxe_server_edit
+      :delete:
+      - :name: delete
+        :identifier: pxe_server_delete
   :quotas:
     :description: TenantQuotas
     :options:

spec/requests/pxe_servers_spec.rb

@@ -1,5 +1,5 @@
 RSpec.describe 'PxeServers API' do
-  let!(:pxe_server) { FactoryBot.create(:pxe_server) }
+  let(:pxe_server) { FactoryBot.build(:pxe_server).tap { |x| x.update_authentication(:default => {:userid => 'foo', :password => 'bar'}) } }
   let!(:pxe_image_1) { FactoryBot.create(:pxe_image, :pxe_server => pxe_server) }
   let!(:pxe_image_2) { FactoryBot.create(:pxe_image, :pxe_server => pxe_server) }
   let!(:pxe_menu_1) { FactoryBot.create(:pxe_menu, :pxe_server => pxe_server) }
@@ -142,31 +142,73 @@
 
     it 'create new pxe server' do
       api_basic_authorize collection_action_identifier(:pxe_servers, :create, :post)
-      post(url, :params => {:name => 'foo', :uri => 'bar/quax'})
+      post(
+        url,
+        :params => {
+          :name           => 'test server',
+          :uri            => 'bar://quax',
+          :authentication => {
+            :userid   => 'foo',
+            :password => 'bar'
+          }
+        }
+      )
+      expect(response).to have_http_status(:ok)
+      expect(response.parsed_body['results'].first['name']).to eq('test server')
+      expect(response.parsed_body['results'].first['uri']).to eq('bar://quax')
+      expect(response.parsed_body['results'].first['uri_prefix']).to eq('bar')
+    end
+
+    it 'create new nfs pxe server without authentication' do
+      api_basic_authorize collection_action_identifier(:pxe_servers, :create, :post)
+      post(url, :params => {:name => 'test server', :uri => 'nfs://quax'})
       expect(response).to have_http_status(:ok)
-      expect(response.parsed_body['results'].first['name']).to eq('foo')
-      expect(response.parsed_body['results'].first['uri']).to eq('bar/quax')
+      expect(response.parsed_body['results'].first['name']).to eq('test server')
+      expect(response.parsed_body['results'].first['uri']).to eq('nfs://quax')
+      expect(response.parsed_body['results'].first['uri_prefix']).to eq('nfs')
     end
 
     it 'create new pxe server with pxe menu' do
       api_basic_authorize collection_action_identifier(:pxe_servers, :create, :post)
-      post(url, :params => {:name => 'foo', :uri => 'bar/quax', :pxe_menus => [{:file_name => 'menu_1'}]})
+      post(url, :params => {:name => 'foo', :uri => 'bar://quax',
+        :pxe_menus      => [{:file_name => 'menu_1'}, {:file_name => 'menu_2'}],
+        :authentication => {:userid => 'foo', :password => 'bar' }})
       expect(response).to have_http_status(:ok)
       expect(PxeServer.find(response.parsed_body['results'].first['id']).pxe_menus.first[:file_name]).to eq('menu_1')
     end
+
+    it 'fail to create new non nfs pxe server without correct authentication' do
+      api_basic_authorize collection_action_identifier(:pxe_servers, :create, :post)
+      post(url, :params => {:name => 'test server', :uri => 'smb://quax', :authentication => {:userid => 'user'}})
+      expect(response).to have_http_status(:bad_request)
+    end
+
+    it 'will forbid create action withouth an appropriate authorization' do
+      api_basic_authorize
+      post(url, :params => {:name => 'foo', :uri => 'bar/quax',
+                            :pxe_menus      => [{:file_name => 'menu_1'}, {:file_name => 'menu_2'}],
+                            :authentication => {:userid => 'foo', :password => 'bar' }})
+      expect(response).to have_http_status(:forbidden)
+    end
   end
 
-  describe 'patch /api/pxe_servers/:id' do
+  describe 'update /api/pxe_servers/:id' do
     let(:url) { "/api/pxe_servers/#{pxe_server.id}" }
 
     it 'update pxe server' do
       api_basic_authorize collection_action_identifier(:pxe_servers, :edit, :patch)
-      patch(url, :params => {:name => 'updated name', :uri => 'updated/url', :pxe_menus => [{:file_name => 'updated menu'}]})
+      patch(url, :params => {:name => 'updated name', :uri => 'updated://url', :pxe_menus => [{:file_name => 'updated menu'}]})
       expect(response).to have_http_status(:ok)
       expect(response.parsed_body['name']).to eq('updated name')
-      expect(response.parsed_body['uri']).to eq('updated/url')
+      expect(response.parsed_body['uri']).to eq('updated://url')
       expect(PxeServer.find(response.parsed_body['id']).pxe_menus.first[:file_name]).to eq('updated menu')
     end
+
+    it 'will forbid update action withouth an appropriate authorization' do
+      api_basic_authorize
+      patch(url, :params => {:name => 'updated name', :uri => 'updated://url', :pxe_menus => [{:file_name => 'updated menu'}]})
+      expect(response).to have_http_status(:forbidden)
+    end
   end
 
   describe 'delete /api/pxe_servers/:id' do
@@ -178,5 +220,11 @@
       expect(response).to have_http_status(:no_content)
       expect(PxeServer.exists?(pxe_server.id)).to be_falsey
     end
+
+    it 'will forbid delete action withouth an appropriate authorization' do
+      api_basic_authorize
+      delete(url)
+      expect(response).to have_http_status(:forbidden)
+    end
   end
 end