PoC tenant integrity checking task

app/models/account/provider_methods.rb

@@ -67,6 +67,7 @@ def build_kind(kind:, **attributes)
       end
     end
 
+    # has_many :buyer_accounts, -> { not_master }, class_name: 'Account', foreign_key: 'provider_account_id', dependent: :destroy, inverse_of: :provider_account do
     has_many :buyer_accounts, ->(provider) { where.not(id: provider.id) }, class_name: 'Account', foreign_key: 'provider_account_id', dependent: :destroy, inverse_of: :provider_account do
       def latest
         order(created_at: :desc).includes([:admin_users]).limit(5)

app/models/oidc_configuration.rb

@@ -55,7 +55,7 @@ def attributes
     end
   end
 
-  belongs_to :oidc_configurable, polymorphic: true
+  belongs_to :oidc_configurable, polymorphic: true, inverse_of: :oidc_configuration
   serialize :config, OIDCConfiguration::Config
 
   delegate(*OIDCConfiguration::Config::ATTRIBUTES, to: :config)

config/abilities/provider_member.rb

@@ -26,9 +26,11 @@
     can :create, Account
     can :update, Account if account.provider_can_use?(:service_permissions)
 
-    # Using historical optimized way and leave canonical way commented out below
+    # Using historical optimized permission and leave canonical way commented out below
     # The resulting hash presently is something like {"type"=>"Cinstance", "service_id"=>[ids..]}
-    # Seems like canonically though that we are moving towards accessing service through plan
+    # Seems like in general we are moving towards accessing service through plan.
+    # Also this doesn't check account of the service but `member_permission_service_ids` will not
+    # return foreign IDs. Also tenant_id checking should not allow foreign objects access.
     can %i[read show edit update], Cinstance, Cinstance.permitted_for(user).where_values_hash
     # can %i[read show edit update], Cinstance, user.accessible_cinstances do |cinstance|
     #   cinstance.plan&.issuer_type == "Service" && cinstance.plan.issuer.account == user.account &&

lib/tasks/multitenant/tenants.rake

@@ -68,6 +68,82 @@ namespace :multitenant do
       update_tenant_ids(proc { |object| object.account.tenant_id }, proc { account }, proc { tenant_id == nil }, args.to_hash.merge({ table_name: 'Alert' }))
     end
 
+    desc 'validate tenant_id integrity'
+    task :integrity => :environment do
+      processed = []
+      inconsistent = []
+      models = leaf_models
+
+      models.each do |model|
+        Rails.logger.info "Tenant integrity of #{model}"
+        inconsistent.concat inconsistent_tenants(model, processed: processed)
+      end
+
+      Rails.logger.error "Inconsistent tenant_ids for:\n#{inconsistent.map {|ma, m1,m2| "#{m1.class}:#{m1.id} <#{ma.name}> #{m2.first.class}:#{m2.map {_1.send _1.class.primary_key}}" }.join("\n")}"
+    end
+
+    def inconsistent_tenants(model, processed: [])
+      # we can ignore these as they can't be automatically excluded but are redundant for the check anyway
+      ignored = {
+        Service => %i[all_metrics], # all metrics of service and APIs used by service so is redundant
+        Account => %i[provider_accounts], # only master has this and it is normal that all will mismatch
+      }
+
+      model.reflect_on_all_associations.inject([]) do |bad, association|
+        next bad if ignored[model]&.include?(association.name)
+
+        # we live in a perfect world where all associations have an inverse so we can skip polymorphic ones
+        next bad if association.polymorphic?
+
+        # arity can be one when association has a scope defined with a proc taking current object as argument
+        # We can't handle such associations but we can ignore them if the inverse one we can handle
+        if association.scope&.arity&.public_send(:>, 0)
+          next bad unless association.inverse_of.polymorphic? || association.inverse_of.scope&.arity&.public_send(:>, 0)
+          raise "we can't handle #{association.name} of #{model}"
+        end
+
+        next bad unless association.klass.attribute_names.include?("tenant_id")
+
+        # skip indirect associations where the "through association" has tenant_id, because we will check that
+        #   indirect association through the "through association" later (or we did already)
+        next bad if association.through_reflection&.try(:klass)&.attribute_names&.include?("tenant_id")
+
+        next bad if processed.any? do
+          _1 == association || _1 == association.inverse_of
+        end
+
+        processed << association
+
+        table_alias = last_table_alias_from_sql(model.joins(association.name).to_sql)
+        found = model.joins(association.name).where.not("#{table_alias}.tenant_id = #{model.arel_table.name}.tenant_id")
+        found = found.where("(accounts.provider = FALSE OR accounts.provider IS NULL)") if model == Account && association.name == :provider_account
+        bad.concat found.map{ [association, _1, ar_representation(_1, association.name)] }
+      end
+    end
+
+    def ar_representation(model, association_name)
+      items = model.public_send(association_name)
+      items = items.to_a if items.is_a?(ActiveRecord::Relation)
+      Array(items)
+    end
+
+    def last_table_alias_from_sql(sql)
+      matcher = /.*INNER JOIN [`'"]([\S]+)[`'"] (?:[`'"]([\S]+)[`'"] )?ON/i.match(sql)
+      matcher[2] || matcher[1]
+    end
+
+    def leaf_models
+      all_models = ApplicationRecord.descendants.select(&:arel_table).reject(&:abstract_class?)
+      all_models.select! { _1.attribute_names.include? "tenant_id"}
+      # we only want child STI classes, not the parent
+      # all_models.reject do |model|
+      #   base_class = model.base_class
+      #   base_class == model && all_models.any? do |potential_child|
+      #     potential_child.base_class == base_class
+      #   end
+      # end
+    end
+
     def update_tenant_ids(tenant_id_block, association_block, condition, **args)
       query = args[:table_name].constantize.joining(&association_block).where.has(&condition)
       puts "------ Updating #{args[:table_name]} ------"

test/unit/tasks/multitenant/tenants_integrity_test.rb

@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+require 'test_helper'
+
+module Tasks
+  module Multitenant
+    class TenantsIntegrityTest < ActiveSupport::TestCase
+      test "reports tenant lack of integrity with has_many associations" do
+        provider = FactoryBot.create(:simple_provider)
+        buyers_to_update = FactoryBot.create_list(:simple_buyer, 2, provider_account: provider, tenant_id: provider.id + 1)
+        buyer_not_to_update = FactoryBot.create(:simple_buyer, provider_account: provider, tenant_id: provider.id)
+
+        execute_rake_task 'multitenant/tenants.rake', 'multitenant:tenants:integrity'
+      end
+    end
+  end
+end