From 9cda1493225ae4c97acd43160aa373e4f35aea88 Mon Sep 17 00:00:00 2001 From: Siddarth R Date: Sat, 19 May 2018 10:14:38 +0700 Subject: [PATCH 1/3] Fixing issue with 500 thrown on nss/host - Added name constraint on hostmnachine and group name, and unuathenticated token access returning appropriate json response --- app/controllers/application_controller.rb | 7 +++++- app/controllers/nss_controller.rb | 24 +++++++++++++-------- app/models/group.rb | 1 + app/models/host_machine.rb | 9 +++++++- spec/controllers/nss_controller_spec.rb | 6 ++---- spec/models/host_machine_spec.rb | 26 +++++++++++++++++++++++ 6 files changed, 58 insertions(+), 15 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index bf815109..3cfba29c 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -16,6 +16,11 @@ def render_404 end def authenticate_access_token! - head :unauthorized if !(AccessToken.valid_token params[:token]) + unless AccessToken.valid_token(params[:token]) + render json: { + success: false, + errors: ['Unauthorized'] + }, status: 403 + end end end diff --git a/app/controllers/nss_controller.rb b/app/controllers/nss_controller.rb index 7baa2f70..27e7abb0 100755 --- a/app/controllers/nss_controller.rb +++ b/app/controllers/nss_controller.rb @@ -1,5 +1,6 @@ class NssController < ApplicationController skip_before_filter :verify_authenticity_token, only: [ :add_host, :add_user_to_group ] + before_filter :authenticate_access_token!, only: %i[add_host] def host token = AccessToken.valid_token params[:token] @@ -22,16 +23,21 @@ def host end def add_host - token = AccessToken.valid_token params[:token] - if token - @response = HostMachine.find_or_create_by(name: params[:name]) if params[:name].present? - @group = Group.find_or_create_by(name: (params[:name] + "_host_group").downcase.squish ) if params[:group_name].present? - @response.groups << @group if @response.present? and @group.present? and @response.groups.find_by_id(@group.id).blank? - @group = Group.find_or_create_by(name: params[:group_name] ) if params[:group_name].present? - @response.groups << @group if @response.present? and @group.present? and @response.groups.find_by_id(@group.id).blank? - @response.save! + begin + host = HostMachine.find_or_create_by(name: params[:name]) + host.add_groups(params[:name], params[:group_name]) + render json: { + success: true, + access_key: host.access_key, + host: host.name, + groups: host.groups.map(&:name) + } + rescue ActiveRecord::RecordInvalid + render json: { + success: false, + errors: host.errors + } end - render json: @response end def group diff --git a/app/models/group.rb b/app/models/group.rb index 0988a773..d9e0338c 100755 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -13,6 +13,7 @@ class Group < ActiveRecord::Base belongs_to :vpn validates_uniqueness_of :name, case_sensitive: false + validates :name, presence: true before_create :set_lower_case_name acts_as_paranoid diff --git a/app/models/host_machine.rb b/app/models/host_machine.rb index b348f9ab..e3850515 100755 --- a/app/models/host_machine.rb +++ b/app/models/host_machine.rb @@ -4,13 +4,14 @@ class HostMachine < ActiveRecord::Base has_many :host_access_groups has_many :groups, through: :host_access_groups validates_uniqueness_of :name, case_sensitive: false + validates :name, presence: true before_create :set_lower_case_name before_save :set_host_access_key before_create :set_host_access_key def set_host_access_key - self.access_key = ROTP::Base32.random_base32 + self.access_key = ROTP::Base32.random_base32 end def set_lower_case_name @@ -34,4 +35,10 @@ def sysadmins users.uniq end + def add_groups(name, group_name) + host_group_name = name.blank? ? '' : "#{name}_host_group".downcase.squish + self.groups << Group.find_or_create_by(name: host_group_name) + self.groups << Group.find_or_create_by(name: group_name) + self.save + end end diff --git a/spec/controllers/nss_controller_spec.rb b/spec/controllers/nss_controller_spec.rb index 6ec5f089..bf2ec07e 100755 --- a/spec/controllers/nss_controller_spec.rb +++ b/spec/controllers/nss_controller_spec.rb @@ -30,11 +30,9 @@ expect(data["success"]).to eq(true) end - it "should return sysadmins for that host" do - + it "should return sysadmins for that host" do sign_in user access_token = create(:access_token) - json = { token: access_token.token, name: "random_host_01" } post "add_host", { token: access_token.token, name: "random_host_01", group_name: "random_group_01", format: :json} body = response.body @@ -151,7 +149,7 @@ group.burst_host_cache cache_count_aft = REDIS_CACHE.keys("*").count - expect(cache_count_aft).to eq cache_count_bfr + expect(cache_count_aft).to eq cache_count_bfr end diff --git a/spec/models/host_machine_spec.rb b/spec/models/host_machine_spec.rb index 28fb7150..ab28342b 100755 --- a/spec/models/host_machine_spec.rb +++ b/spec/models/host_machine_spec.rb @@ -79,4 +79,30 @@ end end + + context 'add_group' do + let(:host_machine) { HostMachine.find_or_create_by(name: 'machine') } + it 'should create groups given valid name and group_name' do + name = host_machine.name; group_name = "#{name}_group" + host_machine.add_groups(name, group_name) + groups = host_machine.groups.map(&:name) + expect(groups.include?("#{name}_host_group")).to eq(true) + expect(groups.include?(group_name)).to eq(true) + expect(host_machine.errors.blank?).to eq(true) + end + + it 'shouldn\'t create group if name is blank' do + name = ''; group_name = "#{host_machine.name}_group" + expect { + host_machine.add_groups(name, group_name) + }.to raise_error { ActiveRecord::RecordInvalid } + end + + it 'shouldn\'t create group if group name is blank' do + name = host_machine.name; group_name = '' + expect { + host_machine.add_groups(name, group_name) + }.to raise_error { ActiveRecord::RecordInvalid } + end + end end From fe206375ed24611a307297d0834c2c24fca67919 Mon Sep 17 00:00:00 2001 From: Siddarth R Date: Sat, 19 May 2018 10:14:38 +0700 Subject: [PATCH 2/3] Fixing issue with 500 thrown on nss/host - Added name constraint on hostmnachine and group name, and unuathenticated token access returning appropriate json response --- app/controllers/application_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 3cfba29c..89234abc 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -20,7 +20,7 @@ def authenticate_access_token! render json: { success: false, errors: ['Unauthorized'] - }, status: 403 + }, status: :unauthorized end end end From 3cf4c3d3b76089523c01a6f637a5d35305c865ec Mon Sep 17 00:00:00 2001 From: Siddarth R Date: Mon, 21 May 2018 14:45:46 +0800 Subject: [PATCH 3/3] Removed exception based flows for nss/host and seperated the logic to add a group and add a host group for a hostmachine --- app/controllers/application_controller.rb | 10 +++-- app/controllers/nss_controller.rb | 23 +++++------ app/models/host_machine.rb | 17 +++++--- app/views/common/errors.json.jbuilder | 2 + app/views/nss/add_host.json.jbuilder | 4 ++ spec/controllers/nss_controller_spec.rb | 7 ++++ spec/models/host_machine_spec.rb | 50 ++++++++++++++++------- spec/rails_helper.rb | 1 + 8 files changed, 77 insertions(+), 37 deletions(-) create mode 100644 app/views/common/errors.json.jbuilder create mode 100644 app/views/nss/add_host.json.jbuilder diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 89234abc..585c6334 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -17,10 +17,12 @@ def render_404 def authenticate_access_token! unless AccessToken.valid_token(params[:token]) - render json: { - success: false, - errors: ['Unauthorized'] - }, status: :unauthorized + render_error(['Unauthorized'], :unauthorized) end end + + def render_error(errors, status = 400) + render 'common/errors', locals: { errors: errors }, status: status + return + end end diff --git a/app/controllers/nss_controller.rb b/app/controllers/nss_controller.rb index 27e7abb0..824d5028 100755 --- a/app/controllers/nss_controller.rb +++ b/app/controllers/nss_controller.rb @@ -23,20 +23,17 @@ def host end def add_host - begin + if params[:name].present? host = HostMachine.find_or_create_by(name: params[:name]) - host.add_groups(params[:name], params[:group_name]) - render json: { - success: true, - access_key: host.access_key, - host: host.name, - groups: host.groups.map(&:name) - } - rescue ActiveRecord::RecordInvalid - render json: { - success: false, - errors: host.errors - } + host.add_host_group(params[:name]) + host.add_group(params[:group_name]) + render 'add_host', locals: { host: host }, format: :json + else + errors = ['Name can\'t be blank'] + if params.key?(:group_name) && params[:group_name].blank? + errors << 'Group Name can\'t be blank' + end + render_error(errors) end end diff --git a/app/models/host_machine.rb b/app/models/host_machine.rb index e3850515..e4eb229c 100755 --- a/app/models/host_machine.rb +++ b/app/models/host_machine.rb @@ -35,10 +35,17 @@ def sysadmins users.uniq end - def add_groups(name, group_name) - host_group_name = name.blank? ? '' : "#{name}_host_group".downcase.squish - self.groups << Group.find_or_create_by(name: host_group_name) - self.groups << Group.find_or_create_by(name: group_name) - self.save + def add_host_group(name) + if name.squish.present? + name = "#{name.squish}_host_group" + self.add_group(name.squish.downcase) + end + end + + def add_group(name) + if name.squish.present? + self.groups << Group.find_or_initialize_by(name: name.squish.downcase) + self.save + end end end diff --git a/app/views/common/errors.json.jbuilder b/app/views/common/errors.json.jbuilder new file mode 100644 index 00000000..8cf928e7 --- /dev/null +++ b/app/views/common/errors.json.jbuilder @@ -0,0 +1,2 @@ +json.success false +json.errors errors diff --git a/app/views/nss/add_host.json.jbuilder b/app/views/nss/add_host.json.jbuilder new file mode 100644 index 00000000..b1ae82df --- /dev/null +++ b/app/views/nss/add_host.json.jbuilder @@ -0,0 +1,4 @@ +json.success true +json.access_key host.access_key +json.host host.name +json.groups host.groups.map(&:name) diff --git a/spec/controllers/nss_controller_spec.rb b/spec/controllers/nss_controller_spec.rb index bf2ec07e..b00e98c2 100755 --- a/spec/controllers/nss_controller_spec.rb +++ b/spec/controllers/nss_controller_spec.rb @@ -30,6 +30,13 @@ expect(data["success"]).to eq(true) end + it 'it shouldn\'t return sysadmins for invalid token' do + json = { token: '', name: 'random_host', group_name: '', format: :json } + post 'add_host', json + body = response.body + expect(JSON.parse(body)['success']).to eq(false) + end + it "should return sysadmins for that host" do sign_in user access_token = create(:access_token) diff --git a/spec/models/host_machine_spec.rb b/spec/models/host_machine_spec.rb index ab28342b..fb1e1f02 100755 --- a/spec/models/host_machine_spec.rb +++ b/spec/models/host_machine_spec.rb @@ -80,29 +80,49 @@ end end + context 'add_host_group' do + let(:host_machine) { HostMachine.find_or_create_by(name: 'machine') } + it 'should create host group given valid name' do + host_machine.add_host_group(host_machine.name) + groups = host_machine.groups.map(&:name) + expect(groups.include?("#{host_machine.name}_host_group")).to eq(true) + expect(host_machine.valid?).to eq(true) + end + + it 'should create the group with all downcase' do + host_machine.add_host_group(host_machine.name.upcase) + groups = host_machine.groups.map(&:name) + expect(groups.include?("#{host_machine.name.downcase}_host_group")).to eq(true) + end + + it 'shouldn\'t add the group if the name is invalid' do + host_machine.add_host_group('') + groups = host_machine.groups.map(&:name) + expect(groups.include?("")).to eq(false) + expect(groups.include?("_host_group")).to eq(false) + end + end + context 'add_group' do let(:host_machine) { HostMachine.find_or_create_by(name: 'machine') } - it 'should create groups given valid name and group_name' do - name = host_machine.name; group_name = "#{name}_group" - host_machine.add_groups(name, group_name) + let(:group_name) { 'machine_group' } + it 'should create host group given valid name' do + host_machine.add_group(group_name) groups = host_machine.groups.map(&:name) - expect(groups.include?("#{name}_host_group")).to eq(true) expect(groups.include?(group_name)).to eq(true) - expect(host_machine.errors.blank?).to eq(true) + expect(host_machine.valid?).to eq(true) end - it 'shouldn\'t create group if name is blank' do - name = ''; group_name = "#{host_machine.name}_group" - expect { - host_machine.add_groups(name, group_name) - }.to raise_error { ActiveRecord::RecordInvalid } + it 'should create the group with all downcase' do + host_machine.add_group(group_name.upcase) + groups = host_machine.groups.map(&:name) + expect(groups.include?(group_name.downcase)).to eq(true) end - it 'shouldn\'t create group if group name is blank' do - name = host_machine.name; group_name = '' - expect { - host_machine.add_groups(name, group_name) - }.to raise_error { ActiveRecord::RecordInvalid } + it 'shouldn\'t add the group if the name is invalid' do + host_machine.add_group('') + groups = host_machine.groups.map(&:name) + expect(groups.include?("")).to eq(false) end end end diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index e57d4216..35f94daf 100755 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -35,6 +35,7 @@ config.include Devise::Test::ControllerHelpers, type: :controller config.include Devise::Test::ControllerHelpers, type: :view config.include ControllerHelpers, type: :controller + config.render_views = true # Remove this line if you're not using ActiveRecord or ActiveRecord fixtures config.fixture_path = "#{::Rails.root}/spec/fixtures"