From 27f5063dae4a9d90e187a0c1c1ff0130e07bc065 Mon Sep 17 00:00:00 2001 From: x4d3 Date: Wed, 1 Nov 2017 17:50:45 +0000 Subject: [PATCH] [MNOE-688] Generate relation_id getter and setter for has_one relationship - Introduce HasOneExtension, generating getter and setter for has_one relations, making them saved in relationships, and allowing their retrieving from a relations cache - Remove useless timestamp method - Rplace is_integer by is_id that is more semantic - Try to not rely on parent_organization --- .../jpi/v1/base_resource_controller.rb | 85 +++++++++++-------- .../jpi/v1/app_instances_controller.rb | 12 +-- .../jpi/v1/app_instances_controller_spec.rb | 51 +++++------ .../models/mno_enterprise/base_resource.rb | 5 ++ .../models/mno_enterprise/orga_relation.rb | 7 +- core/app/models/mno_enterprise/user.rb | 2 +- .../has_one_extension.rb | 30 +++++++ .../mno_enterprise/concerns/models/ability.rb | 14 +-- .../concerns/models/app_instance.rb | 6 +- .../concerns/models/organization.rb | 5 -- .../factories/app_instances.rb | 1 - 11 files changed, 127 insertions(+), 91 deletions(-) create mode 100644 core/lib/json_api_client_extension/has_one_extension.rb diff --git a/api/app/controllers/mno_enterprise/jpi/v1/base_resource_controller.rb b/api/app/controllers/mno_enterprise/jpi/v1/base_resource_controller.rb index 26a5ef0ea..7a2669798 100644 --- a/api/app/controllers/mno_enterprise/jpi/v1/base_resource_controller.rb +++ b/api/app/controllers/mno_enterprise/jpi/v1/base_resource_controller.rb @@ -3,50 +3,61 @@ class Jpi::V1::BaseResourceController < ApplicationController before_filter :check_authorization protected - - def timestamp - @timestamp ||= (params[:timestamp] || 0).to_i + def is_id?(string) + # we consider that it is an id, if it's + string.to_i.to_s == string + end + + def orga_relation + @orga_relation ||= begin + id_or_uid = params[:organization_id] + organization_field = is_id?(id_or_uid) ? 'id' : 'uid' + MnoEnterprise::OrgaRelation.where('user.id' => current_user.id, "organization.#{organization_field}" => id_or_uid).first end - - def is_integer?(string) - string.to_i.to_s == string + end + + def parent_organization_id + id_or_uid = params[:organization_id] + if is_id?(id_or_uid) + id_or_uid + else + parent_organization.id end + end - def parent_organization - @parent_organization ||= begin - id_or_uid = params[:organization_id] - query = is_integer?(id_or_uid) ? id_or_uid : {uid: id_or_uid} - o = MnoEnterprise::Organization.includes(:orga_relations, :users).find(query).first - ## check that user is in the organization - o if o && o.orga_relation(current_user) - end + def parent_organization + @parent_organization ||= begin + id_or_uid = params[:organization_id] + query = is_id?(id_or_uid) ? id_or_uid : { uid: id_or_uid } + MnoEnterprise::Organization.find(query).first end - - # Check current user is logged in - # Check organization is valid if specified - def check_authorization - unless current_user - render nothing: true, status: :unauthorized - return false - end - if params[:organization_id] && !parent_organization - render nothing: true, status: :forbidden - return false - end - true + end + + # Check current user is logged in + # Check organization is valid if specified + def check_authorization + unless current_user + render nothing: true, status: :unauthorized + return false end - - def render_not_found(resource, id = params[:id]) - render json: { errors: {message: "#{resource.titleize} not found (id=#{id})", code: 404, params: params} }, status: :not_found + if params[:organization_id] && !orga_relation + render nothing: true, status: :forbidden + return false end + true + end - def render_bad_request(attempted_action, issue) - issue = issue.full_messages if issue.respond_to?(:full_messages) - render json: { errors: {message: "Error while trying to #{attempted_action}: #{issue}", code: 400, params: params} }, status: :bad_request - end + def render_not_found(resource, id = params[:id]) + render json: { errors: { message: "#{resource.titleize} not found (id=#{id})", code: 404, params: params } }, status: :not_found + end - def render_forbidden_request(attempted_action) - render json: { errors: {message: "Error while trying to #{attempted_action}: you do not have permission", code: 403} }, status: :forbidden - end + def render_bad_request(attempted_action, issue) + issue = issue.full_messages if issue.respond_to?(:full_messages) + render json: { errors: { message: "Error while trying to #{attempted_action}: #{issue}", code: 400, params: params } }, status: :bad_request + end + + def render_forbidden_request(attempted_action) + render json: { errors: { message: "Error while trying to #{attempted_action}: you do not have permission", code: 403 } }, status: :forbidden + end end end diff --git a/api/lib/mno_enterprise/concerns/controllers/jpi/v1/app_instances_controller.rb b/api/lib/mno_enterprise/concerns/controllers/jpi/v1/app_instances_controller.rb index b74982003..93e69cc1d 100644 --- a/api/lib/mno_enterprise/concerns/controllers/jpi/v1/app_instances_controller.rb +++ b/api/lib/mno_enterprise/concerns/controllers/jpi/v1/app_instances_controller.rb @@ -16,25 +16,25 @@ module MnoEnterprise::Concerns::Controllers::Jpi::V1::AppInstancesController # GET /mnoe/jpi/v1/organization/1/app_instances def index statuses = MnoEnterprise::AppInstance::ACTIVE_STATUSES.join(',') - @app_instances = MnoEnterprise::AppInstance.includes(:app).where(owner_id: parent_organization.id, 'status.in': statuses, 'fulfilled_only': true).to_a.select do |i| + @app_instances = MnoEnterprise::AppInstance.includes(:app).where('owner.id': parent_organization.id, 'status.in': statuses, 'fulfilled_only': true).to_a.select do |i| can?(:access,i) end end # POST /mnoe/jpi/v1/organization/1/app_instances def create - authorize! :manage_app_instances, parent_organization - app_instance = parent_organization.provision_app_instance!(params[:nid]) + authorize! :manage_app_instances, orga_relation + input = { data: { attributes: { app_nid: params[:nid], owner_id: parent_organization_id, owner_type: 'Organization' } } } + app_instance = MnoEnterprise::AppInstance.provision!(input) MnoEnterprise::EventLogger.info('app_add', current_user.id, 'App added', app_instance) head :created end # DELETE /mnoe/jpi/v1/app_instances/1 def destroy - @app_instance = MnoEnterprise::AppInstance.find_one(params[:id]) + @app_instance = MnoEnterprise::AppInstance.find_one(params[:id], :owner) if @app_instance - organization = MnoEnterprise::Organization.find_one(@app_instance.owner_id) - authorize! :manage_app_instances, organization + authorize! :manage_app_instances, current_user.orga_relation(@app_instance.owner) MnoEnterprise::EventLogger.info('app_destroy', current_user.id, 'App destroyed', @app_instance) @app_instance = @app_instance.terminate! end diff --git a/api/spec/controllers/mno_enterprise/jpi/v1/app_instances_controller_spec.rb b/api/spec/controllers/mno_enterprise/jpi/v1/app_instances_controller_spec.rb index 3eaaee8e6..27cedfb72 100644 --- a/api/spec/controllers/mno_enterprise/jpi/v1/app_instances_controller_spec.rb +++ b/api/spec/controllers/mno_enterprise/jpi/v1/app_instances_controller_spec.rb @@ -14,69 +14,62 @@ module MnoEnterprise # Stub user and user call let(:user) { build(:user) } + let(:organization) { build(:organization) } + let(:orga_relation) { build(:orga_relation) } let!(:current_user_stub) { stub_user(user) } - # Stub organization and association - let!(:organization) { - o = build(:organization, orga_relations: []) - o.orga_relations << build(:orga_relation, user_id: user.id, organization_id: o.id, role: 'Super Admin') - o - } - before { stub_api_v2(:get, "/organizations/#{organization.id}", organization, %i(orga_relations users)) } + before { stub_api_v2(:get, '/orga_relations', orga_relation, [], { filter: { 'user.id': user.id, 'organization.id': organization.id }, page: { number: 1, size: 1 } }) } describe 'GET #index' do - - let(:app_instance) { build(:app_instance, status: 'running' , under_free_trial: false) } - - before { stub_api_v2(:get, '/app_instances', [app_instance], [:app], {filter: {owner_id: organization.id, 'status.in': MnoEnterprise::AppInstance::ACTIVE_STATUSES.join(','), 'fulfilled_only': true }}) } - + let(:app_instance) { build(:app_instance, status: 'running', under_free_trial: false) } + let!(:stub) { stub_api_v2(:get, '/app_instances', [app_instance], [:app], { filter: { 'owner.id': organization.id, 'status.in': MnoEnterprise::AppInstance::ACTIVE_STATUSES.join(','), fulfilled_only: true } }) } before { sign_in user } subject { get :index, organization_id: organization.id } it_behaves_like 'jpi v1 protected action' describe 'all' do - it { + it do subject - # TODO: Test that the rendered json is the expected one - # expect(assigns(:app_instances)).to eq([app_instance]) - assert_requested(:get, api_v2_url('/app_instances', [:app], {_locale: I18n.locale, filter: {owner_id: organization.id, 'status.in': MnoEnterprise::AppInstance::ACTIVE_STATUSES.join(','), 'fulfilled_only': true }})) - } + expect(subject).to be_successful + expect(stub).to have_been_requested + end end context 'without access to the app_instance' do before { allow(ability).to receive(:can?).with(any_args).and_return(false) } - it { + it do subject expect(assigns(:app_instances)).to be_empty - } + end end end describe 'POST #create' do before { stub_audit_events } let(:app) { build(:app, nid: 'my-app') } - let(:app_instance) { build(:app_instance, app: app, owner: organization, owner_id: organization.id) } + let(:app_instance) { build(:app_instance, app: app, owner: organization) } subject { post :create, organization_id: organization.id, nid: 'my-app' } it_behaves_like 'jpi v1 protected action' + let!(:stub) { stub_api_v2(:post, '/app_instances/provision', app_instance) } before do - stub_api_v2(:post, '/app_instances/provision', app_instance) sign_in user end - it { + it do expect(subject).to be_successful - assert_requested_api_v2(:post, '/app_instances/provision') - } + expect(stub).to have_been_requested + end end describe 'DELETE #destroy' do before { stub_audit_events } - let(:app_instance) { build(:app_instance) } + let(:app_instance) { build(:app_instance, owner: organization) } let(:terminated_app_instance) { build(:app_instance, id: app_instance.id, status: 'terminated') } - before { stub_api_v2(:get, "/app_instances/#{app_instance.id}", app_instance)} - before { stub_api_v2(:delete, "/app_instances/#{app_instance.id}/terminate", terminated_app_instance)} - before { stub_api_v2(:get, "/organizations/#{app_instance.owner_id}")} + before { stub_api_v2(:get, "/app_instances/#{app_instance.id}", app_instance, [:owner]) } + let!(:stub) { stub_api_v2(:delete, "/app_instances/#{app_instance.id}/terminate", terminated_app_instance) } + + before { sign_in user } subject { delete :destroy, id: app_instance.id } @@ -84,7 +77,7 @@ module MnoEnterprise it { subject - assert_requested_api_v2(:delete, "/app_instances/#{app_instance.id}/terminate") + expect(stub).to have_been_requested expect(assigns(:app_instance).status).to eq('terminated') } end diff --git a/core/app/models/mno_enterprise/base_resource.rb b/core/app/models/mno_enterprise/base_resource.rb index 7b1a22770..fa51b6f70 100644 --- a/core/app/models/mno_enterprise/base_resource.rb +++ b/core/app/models/mno_enterprise/base_resource.rb @@ -3,6 +3,7 @@ module MnoEnterprise class BaseResource < ::JsonApiClient::Resource include ActiveModel::Callbacks + include JsonApiClientExtension::HasOneExtension self.site = URI.join(MnoEnterprise.api_host, MnoEnterprise.mno_api_v2_root_path).to_s self.parser = JsonApiClientExtension::CustomParser @@ -75,6 +76,10 @@ def self.process_custom_result(result) end # == Instance Methods ======================================================== + # cache of the loaded relations, used in JsonApiClientExtension::HasOneExtension + def relations + @relations ||= ActiveSupport::HashWithIndifferentAccess.new + end def process_custom_result(result) collect_errors(result.errors) diff --git a/core/app/models/mno_enterprise/orga_relation.rb b/core/app/models/mno_enterprise/orga_relation.rb index 4ee508e1e..de5deefa1 100644 --- a/core/app/models/mno_enterprise/orga_relation.rb +++ b/core/app/models/mno_enterprise/orga_relation.rb @@ -4,8 +4,11 @@ class OrgaRelation < BaseResource property :created_at, type: :time property :updated_at, type: :time # json_api_client map all primary id as string - property :organization_id, type: :string - property :user_id, type: :string + property :role, type: :string + + has_one :organization + has_one :user + end end diff --git a/core/app/models/mno_enterprise/user.rb b/core/app/models/mno_enterprise/user.rb index 2825054f0..0e49144de 100644 --- a/core/app/models/mno_enterprise/user.rb +++ b/core/app/models/mno_enterprise/user.rb @@ -124,7 +124,7 @@ def role_from_id(organization_id) end def orga_relation_from_id(organization_id) - self.orga_relations.find { |r| r.organization_id == organization_id } + MnoEnterprise::OrgaRelation.where('user.id': id, 'organization.id': organization_id).first end def create_deletion_request! diff --git a/core/lib/json_api_client_extension/has_one_extension.rb b/core/lib/json_api_client_extension/has_one_extension.rb new file mode 100644 index 000000000..491179a5a --- /dev/null +++ b/core/lib/json_api_client_extension/has_one_extension.rb @@ -0,0 +1,30 @@ +module JsonApiClientExtension::HasOneExtension + extend ActiveSupport::Concern + + class_methods do + def has_one(attr_name, options = {}) + class_eval <<-CODE + def #{attr_name}_id=(id) + ActiveSupport::Deprecation.warn(self.class.name + ".#{attr_name}_id Use relationships instead") + association = id ? {data: {type: "#{attr_name.to_s.pluralize}", id: id.to_s}} : nil + self.relationships.#{attr_name} = association + end + def #{attr_name}_id + # First we try in the relationship + relationship_definitions = self.relationships.try(:#{attr_name}) + return nil unless relationship_definitions + relationship_definitions.dig(:data, :id) + end + def #{attr_name}=(relation) + self.relationships.#{attr_name} = relation + relations[:#{attr_name}] = relation + end + def #{attr_name} + relations[:#{attr_name}] ||= super + end + + CODE + super + end + end +end diff --git a/core/lib/mno_enterprise/concerns/models/ability.rb b/core/lib/mno_enterprise/concerns/models/ability.rb index efb9d5809..cfdbaa3d0 100644 --- a/core/lib/mno_enterprise/concerns/models/ability.rb +++ b/core/lib/mno_enterprise/concerns/models/ability.rb @@ -43,20 +43,20 @@ def initialize(user) :invite_member, :administrate, :manage_app_instances, - :manage_teams], MnoEnterprise::Organization do |organization| - ['Super Admin','Admin'].include? user.role(organization) + :manage_teams], MnoEnterprise::OrgaRelation do |orga_relation| + ['Super Admin','Admin'].include? orga_relation.role end # To be updated # TODO: replace by organization_id, no need to load a full organization - can :sync_apps, MnoEnterprise::Organization do |organization| - user.role(organization) + can :sync_apps, MnoEnterprise::OrgaRelation |orga_relation| + !!orga_relation end # To be updated # TODO: replace by organization_id, no need to load a full organization - can :check_apps_sync, MnoEnterprise::Organization do |organization| - user.role(organization) + can :check_apps_sync, MnoEnterprise::OrgaRelation do |orga_relation| + !!orga_relation end #=================================================== @@ -151,7 +151,7 @@ def impac_abilities(user) # Abilities for admin user def admin_abilities(user) if user.admin_role.to_s.casecmp('admin').zero? || user.admin_role.to_s.casecmp('staff').zero? - can :manage_app_instances, MnoEnterprise::Organization + can :manage_app_instances, MnoEnterprise::OrgaRelation can :manage_sub_tenant, MnoEnterprise::SubTenant end end diff --git a/core/lib/mno_enterprise/concerns/models/app_instance.rb b/core/lib/mno_enterprise/concerns/models/app_instance.rb index 1fc766f91..64c6417c2 100644 --- a/core/lib/mno_enterprise/concerns/models/app_instance.rb +++ b/core/lib/mno_enterprise/concerns/models/app_instance.rb @@ -10,8 +10,6 @@ module MnoEnterprise::Concerns::Models::AppInstance property :created_at, type: :time property :updated_at, type: :time - property :owner_id, type: :string - # delete /app_instances/:id/terminate custom_endpoint :terminate, on: :member, request_method: :delete custom_endpoint :provision, on: :collection, request_method: :post @@ -21,6 +19,8 @@ module MnoEnterprise::Concerns::Models::AppInstance #============================================================== ACTIVE_STATUSES = [:running, :stopped, :staged, :provisioning, :starting, :stopping, :updating] TERMINATION_STATUSES = [:terminating, :terminated] + + has_one :owner end #================================================================== @@ -36,12 +36,12 @@ def provision!(input) def active? status.to_sym.in? ACTIVE_STATUSES end + #================================================================== # Instance methods #================================================================== - def to_audit_event { id: id, diff --git a/core/lib/mno_enterprise/concerns/models/organization.rb b/core/lib/mno_enterprise/concerns/models/organization.rb index 5f35d843d..c4fb2f0aa 100644 --- a/core/lib/mno_enterprise/concerns/models/organization.rb +++ b/core/lib/mno_enterprise/concerns/models/organization.rb @@ -94,11 +94,6 @@ def add_user!(user, role = 'Member') MnoEnterprise::OrgaRelation.create!(organization_id: self.id, user_id: user.id, role: role) end - def provision_app_instance!(app_nid) - input = { data: { attributes: { app_nid: app_nid, owner_id: id, owner_type: 'Organization' } } } - MnoEnterprise::AppInstance.provision!(input) - end - def new_credit_card MnoEnterprise::CreditCard.new(owner_id: id, owner_type: 'Organization') end diff --git a/core/lib/mno_enterprise/testing_support/factories/app_instances.rb b/core/lib/mno_enterprise/testing_support/factories/app_instances.rb index 353f6a3fa..4389b2295 100644 --- a/core/lib/mno_enterprise/testing_support/factories/app_instances.rb +++ b/core/lib/mno_enterprise/testing_support/factories/app_instances.rb @@ -25,7 +25,6 @@ per_user_licence 1 app { build(:app, nid: app_nid) } sequence(:owner) { |n| build(:organization, id: n.to_s) } - sequence(:owner_id, &:to_s) end end end