From 55f17c2ad95fa8826dd872a1f0ae111d916e2140 Mon Sep 17 00:00:00 2001 From: David Spencer Date: Wed, 8 Oct 2025 09:05:46 -0400 Subject: [PATCH 1/5] Support custom fallback strategies These come after the custom and default strategies, and can be used by the application to optionally create new ApplicationInstance records on launch, or do something else in the case where we weren't able to find the ApplicationInstance record for the LTI launch. --- lib/atomic_tenant.rb | 1 + .../current_application_instance_middleware.rb | 8 +++++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/lib/atomic_tenant.rb b/lib/atomic_tenant.rb index 1c81a7f..8c348f3 100644 --- a/lib/atomic_tenant.rb +++ b/lib/atomic_tenant.rb @@ -12,6 +12,7 @@ module AtomicTenant mattr_accessor :custom_strategies + mattr_accessor :custom_fallback_strategies mattr_accessor :jwt_secret mattr_accessor :jwt_aud diff --git a/lib/atomic_tenant/current_application_instance_middleware.rb b/lib/atomic_tenant/current_application_instance_middleware.rb index 0e0ddc6..722ea5f 100644 --- a/lib/atomic_tenant/current_application_instance_middleware.rb +++ b/lib/atomic_tenant/current_application_instance_middleware.rb @@ -21,12 +21,18 @@ def call(env) elsif env['atomic.validated.id_token'].present? custom_strategies = AtomicTenant.custom_strategies || [] + custom_fallback_strategies = AtomicTenant.custom_fallback_strategies || [] default_strategies = [ AtomicTenant::DeploymentManager::PlatformGuidStrategy.new, AtomicTenant::DeploymentManager::ClientIdStrategy.new ] - deployment_manager = AtomicTenant::DeploymentManager::DeploymentManager.new(custom_strategies.concat(default_strategies)) + deployment_manager = AtomicTenant::DeploymentManager::DeploymentManager.new([ + *custom_strategies, + *default_strategies, + *custom_fallback_strategies + ]) + decoded_token = env['atomic.validated.decoded_id_token'] iss = env['atomic.validated.decoded_id_token']['iss'] deployment_id = env['atomic.validated.decoded_id_token'][AtomicLti::Definitions::DEPLOYMENT_ID] From 04a79217fc84089eace388f7c21325c9a6ae7f83 Mon Sep 17 00:00:00 2001 From: David Spencer Date: Wed, 8 Oct 2025 16:12:07 -0400 Subject: [PATCH 2/5] Move most auto creating new app instance logic into gem The application will still have to provide a custom strategy, but at least most of the logic is contained in the gem now. --- lib/atomic_tenant.rb | 4 + ...ract_auto_create_platform_guid_strategy.rb | 150 ++++++++++++++++++ lib/atomic_tenant/exceptions.rb | 1 + 3 files changed, 155 insertions(+) create mode 100644 lib/atomic_tenant/deployment_manager/abstract_auto_create_platform_guid_strategy.rb diff --git a/lib/atomic_tenant.rb b/lib/atomic_tenant.rb index 8c348f3..0e95dc4 100644 --- a/lib/atomic_tenant.rb +++ b/lib/atomic_tenant.rb @@ -3,6 +3,7 @@ require 'atomic_tenant/deployment_manager/platform_guid_strategy' require 'atomic_tenant/deployment_manager/client_id_strategy' require 'atomic_tenant/deployment_manager/deployment_manager_strategy' +require 'atomic_tenant/deployment_manager/abstract_auto_create_platform_guid_strategy' require 'atomic_tenant/engine' require 'atomic_tenant/current_application_instance_middleware' require 'atomic_tenant/tenant_switching' @@ -14,6 +15,9 @@ module AtomicTenant mattr_accessor :custom_strategies mattr_accessor :custom_fallback_strategies + mattr_accessor :untrusted_iss_tenant_limit, default: 100 + mattr_accessor :tenant_uuid_namespace + mattr_accessor :jwt_secret mattr_accessor :jwt_aud diff --git a/lib/atomic_tenant/deployment_manager/abstract_auto_create_platform_guid_strategy.rb b/lib/atomic_tenant/deployment_manager/abstract_auto_create_platform_guid_strategy.rb new file mode 100644 index 0000000..fbf6444 --- /dev/null +++ b/lib/atomic_tenant/deployment_manager/abstract_auto_create_platform_guid_strategy.rb @@ -0,0 +1,150 @@ +require 'uri' + +module AtomicTenant + module DeploymentManager + class AbstractAutoCreatePlatformGuidStrategy < DeploymentManagerStrategy + TRUSTED_ISSUERS = [ + %r|^https://canvas\.instructure\.com$|, + %r|^https://[a-z0-9.-]+\.brightspace\.com$|, + %r|^https://blackboard\.com$|, + ].freeze + + def name + raise NotImplementedError, "Subclasses must implement #name" + end + + def call(decoded_id_token:) + issuer = decoded_id_token["iss"] + platform_guid = decoded_id_token.dig(AtomicLti::Definitions::TOOL_PLATFORM_CLAIM, "guid") + target_link_uri = decoded_id_token[AtomicLti::Definitions::TARGET_LINK_URI_CLAIM] + + if !platform_guid.present? || !target_link_uri.present? + return AtomicTenant::DeploymentManager::DeploymentStrategyResult.new() + end + + uri = URI.parse(target_link_uri) + application_key = uri.host&.split('.')&.first + return AtomicTenant::DeploymentManager::DeploymentStrategyResult.new() if !application_key.present? + + app = Application.find_by(key: application_key) + return AtomicTenant::DeploymentManager::DeploymentStrategyResult.new() if app.nil? + + if !TRUSTED_ISSUERS.any? { |pattern| issuer.match?(pattern) } + existing_app_instance_count = AtomicTenant::LtiDeployment + .joins(:application_instance) + .where( + iss: issuer, + application_instances: { application_id: app.id }, + ).distinct.count(:application_instance_id) + + if existing_app_instance_count >= AtomicTenant.untrusted_iss_tenant_limit + raise AtomicTenant::Exceptions::OnboardingException, "The issuer #{issuer} has reached the limit of #{AtomicTenant.untrusted_iss_tenant_limit} unique tenants for the application #{application_key}." + end + end + + site_url = extract_site_url(decoded_id_token) + + app_inst = find_existing(app, site_url, issuer, platform_guid) + app_inst ||= maybe_create_new_instance(app, site_url, issuer, platform_guid) + pin = pin_platform_guid(issuer, platform_guid, app.id, app_inst.id) + AtomicTenant::DeploymentManager::DeploymentStrategyResult.new(application_instance_id: pin.application_instance_id) + end + + private + + def create_new_instance(app, site_url, issuer, platform_guid) + raise NotImplementedError, "Subclasses must implement #create_new_instance" + end + + def maybe_create_new_instance(app, site_url, issuer, platform_guid) + ActiveRecord::Base.transaction do + create_new_instance(app, site_url, issuer, platform_guid) + rescue ActiveRecord::RecordNotUnique + # If we get a RecordNotUnique error, it means another process created the instance concurrently. + find_existing(app, site_url, issuer, platform_guid) + end + end + + def find_existing(current_application, site_url, issuer, platform_guid) + # This won't work if they uninstall in a vanity/non-vanity domain and then + # reinstall in the other. But that is probably a rare case and we can deal + # with it manually in the admin interface if needed. + site_key = URI.parse(site_url).hostname + site_key = site_key. + gsub(".instructure.com", ""). + gsub(/[^\w]+/, "_") # Replace non alphanumeric characters with _ + + old_tenant = "#{site_key}-#{current_application.key}" + app_instance = ApplicationInstance.find_by(tenant: old_tenant, application: current_application) + + tenant = tenant_name(issuer, platform_guid, current_application.key) + app_instance ||= ApplicationInstance.find_by(tenant:, application: current_application) + + return app_instance + end + + # Pin platform guid, handling concurrent launches both trying to pin the same + # platform guid at the same time. + def pin_platform_guid(iss, platform_guid, application_id, application_instance_id) + begin + AtomicTenant::PinnedPlatformGuid.create!( + iss:, + platform_guid:, + application_id:, + application_instance_id:, + ) + rescue ActiveRecord::RecordNotUnique + AtomicTenant::PinnedPlatformGuid.find_by!( + iss:, + platform_guid:, + application_id:, + application_instance_id:, + ) + end + end + + def tenant_uuid(issuer, platform_guid) + Digest::UUID.uuid_v5(AtomicTenant.tenant_uuid_namespace, "#{issuer}|#{platform_guid}") + end + + # For new tenants use UUIDv5 based on iss+platform_guid. This is more reliable + # than the site url which can change if they switch from vanity to non-vanity + # domain or vice versa. We use a UUID because iss+platform_guid can be long and + # postgres truncates schema names over 63 characters. + def tenant_name(issuer, platform_guid, application_key) + "#{tenant_uuid(issuer, platform_guid)}-#{application_key}" + end + + def extract_site_url(decoded_id_token) + platform_claim = decoded_id_token[AtomicLti::Definitions::TOOL_PLATFORM_CLAIM] + product_family_code = platform_claim["product_family_code"] + + if product_family_code == "canvas" + canvas_domain = decoded_id_token.dig(AtomicLti::Definitions::CUSTOM_CLAIM, "canvas_api_domain") + if canvas_domain.blank? + raise AtomicTenant::Exceptions::OnboardingException, "Missing canvas_api_domain claim from canvas launch" + end + + ensure_https(canvas_domain) + elsif product_family_code == "BlackboardLearn" + blackboard_url = platform_claim["url"] + + if blackboard_url.blank? + raise AtomicTenant::Exceptions::OnboardingException, "Missing url in platform claim from blackboard launch" + end + + ensure_https(blackboard_url) + else + decoded_id_token["iss"] + end + end + + def ensure_https(url) + return nil if url.blank? + + url = "https://#{url}" unless url.start_with?("http") + url.gsub("http://", "https://") + end + end + end +end diff --git a/lib/atomic_tenant/exceptions.rb b/lib/atomic_tenant/exceptions.rb index 6ae95dc..d5a5ef2 100644 --- a/lib/atomic_tenant/exceptions.rb +++ b/lib/atomic_tenant/exceptions.rb @@ -10,5 +10,6 @@ class NoAdminApp < StandardError; end class InvalidTenantKeyError < StandardError; end class TenantNotFoundError < StandardError; end class TenantNotSet < StandardError; end + class OnboardingException < StandardError; end end end From a5d937c3e8b10c1dbda74ee631fec465c02af24b Mon Sep 17 00:00:00 2001 From: David Spencer Date: Wed, 8 Oct 2025 17:25:29 -0400 Subject: [PATCH 3/5] Add tests for abstract strategy class Also moves more to be implemented by the application. --- lib/atomic_tenant.rb | 1 - ...ract_auto_create_platform_guid_strategy.rb | 34 +- ...auto_create_platform_guid_strategy_spec.rb | 407 ++++++++++++++++++ 3 files changed, 411 insertions(+), 31 deletions(-) create mode 100644 spec/lib/atomic_tenant/deployment_manager/abstract_auto_create_platform_guid_strategy_spec.rb diff --git a/lib/atomic_tenant.rb b/lib/atomic_tenant.rb index 0e95dc4..15481c7 100644 --- a/lib/atomic_tenant.rb +++ b/lib/atomic_tenant.rb @@ -16,7 +16,6 @@ module AtomicTenant mattr_accessor :custom_fallback_strategies mattr_accessor :untrusted_iss_tenant_limit, default: 100 - mattr_accessor :tenant_uuid_namespace mattr_accessor :jwt_secret mattr_accessor :jwt_aud diff --git a/lib/atomic_tenant/deployment_manager/abstract_auto_create_platform_guid_strategy.rb b/lib/atomic_tenant/deployment_manager/abstract_auto_create_platform_guid_strategy.rb index fbf6444..c3a31a3 100644 --- a/lib/atomic_tenant/deployment_manager/abstract_auto_create_platform_guid_strategy.rb +++ b/lib/atomic_tenant/deployment_manager/abstract_auto_create_platform_guid_strategy.rb @@ -52,6 +52,10 @@ def call(decoded_id_token:) private + def find_existing(current_application, site_url, issuer, platform_guid) + raise NotImplementedError, "Subclasses must implement #find_existing" + end + def create_new_instance(app, site_url, issuer, platform_guid) raise NotImplementedError, "Subclasses must implement #create_new_instance" end @@ -65,24 +69,6 @@ def maybe_create_new_instance(app, site_url, issuer, platform_guid) end end - def find_existing(current_application, site_url, issuer, platform_guid) - # This won't work if they uninstall in a vanity/non-vanity domain and then - # reinstall in the other. But that is probably a rare case and we can deal - # with it manually in the admin interface if needed. - site_key = URI.parse(site_url).hostname - site_key = site_key. - gsub(".instructure.com", ""). - gsub(/[^\w]+/, "_") # Replace non alphanumeric characters with _ - - old_tenant = "#{site_key}-#{current_application.key}" - app_instance = ApplicationInstance.find_by(tenant: old_tenant, application: current_application) - - tenant = tenant_name(issuer, platform_guid, current_application.key) - app_instance ||= ApplicationInstance.find_by(tenant:, application: current_application) - - return app_instance - end - # Pin platform guid, handling concurrent launches both trying to pin the same # platform guid at the same time. def pin_platform_guid(iss, platform_guid, application_id, application_instance_id) @@ -103,18 +89,6 @@ def pin_platform_guid(iss, platform_guid, application_id, application_instance_i end end - def tenant_uuid(issuer, platform_guid) - Digest::UUID.uuid_v5(AtomicTenant.tenant_uuid_namespace, "#{issuer}|#{platform_guid}") - end - - # For new tenants use UUIDv5 based on iss+platform_guid. This is more reliable - # than the site url which can change if they switch from vanity to non-vanity - # domain or vice versa. We use a UUID because iss+platform_guid can be long and - # postgres truncates schema names over 63 characters. - def tenant_name(issuer, platform_guid, application_key) - "#{tenant_uuid(issuer, platform_guid)}-#{application_key}" - end - def extract_site_url(decoded_id_token) platform_claim = decoded_id_token[AtomicLti::Definitions::TOOL_PLATFORM_CLAIM] product_family_code = platform_claim["product_family_code"] diff --git a/spec/lib/atomic_tenant/deployment_manager/abstract_auto_create_platform_guid_strategy_spec.rb b/spec/lib/atomic_tenant/deployment_manager/abstract_auto_create_platform_guid_strategy_spec.rb new file mode 100644 index 0000000..6ec8621 --- /dev/null +++ b/spec/lib/atomic_tenant/deployment_manager/abstract_auto_create_platform_guid_strategy_spec.rb @@ -0,0 +1,407 @@ +require "rails_helper" + +# Test implementation of the abstract class +class TestAutoCreateStrategy < AtomicTenant::DeploymentManager::AbstractAutoCreatePlatformGuidStrategy + def name + "TestAutoCreateStrategy" + end + + private + + def create_new_instance(app, site_url, issuer, platform_guid) + # Simple test implementation that creates an ApplicationInstance + # In a real implementation, this would also create the tenant/schema + ApplicationInstance.create!( + application: app, + lti_key: "test-lti-key-#{site_url}" + ) + end + + # Implement find_existing to work with the dummy app structure + # In real apps, this would query by tenant field/relationship + def find_existing(current_application, site_url, issuer, platform_guid) + ApplicationInstance.find_by( + application: current_application, + lti_key: "test-lti-key-#{site_url}" + ) + end +end + +RSpec.describe AtomicTenant::DeploymentManager::AbstractAutoCreatePlatformGuidStrategy do + let(:strategy) { TestAutoCreateStrategy.new } + let!(:application) { create(:application, key: "lti-example") } + let(:base_decoded_token) do + { + "iss" => "https://canvas.instructure.com", + "https://purl.imsglobal.org/spec/lti/claim/tool_platform" => { + "guid" => "test-platform-guid", + "product_family_code" => "canvas" + }, + "https://purl.imsglobal.org/spec/lti/claim/target_link_uri" => "https://lti-example.example.com/lti_launches", + "https://purl.imsglobal.org/spec/lti/claim/custom" => { + "canvas_api_domain" => "atomicjolt.instructure.com" + } + } + end + + before do + # Configure the gem settings + AtomicTenant.tenant_uuid_namespace = "6ba7b810-9dad-11d1-80b4-00c04fd430c8" + AtomicTenant.untrusted_iss_tenant_limit = 100 + end + + describe "#name" do + it "raises NotImplementedError for abstract class" do + abstract_strategy = AtomicTenant::DeploymentManager::AbstractAutoCreatePlatformGuidStrategy.new + expect { abstract_strategy.name }.to raise_error(NotImplementedError, "Subclasses must implement #name") + end + + it "returns the strategy name for concrete implementation" do + expect(strategy.name).to eq("TestAutoCreateStrategy") + end + end + + describe "#call" do + context "with missing platform_guid" do + let(:decoded_token) { base_decoded_token.tap { |t| t["https://purl.imsglobal.org/spec/lti/claim/tool_platform"].delete("guid") } } + + it "returns empty result" do + result = strategy.call(decoded_id_token: decoded_token) + expect(result).to be_a(AtomicTenant::DeploymentManager::DeploymentStrategyResult) + expect(result.application_instance_id).to be_nil + end + end + + context "with missing target_link_uri" do + let(:decoded_token) { base_decoded_token.tap { |t| t.delete("https://purl.imsglobal.org/spec/lti/claim/target_link_uri") } } + + it "returns empty result" do + result = strategy.call(decoded_id_token: decoded_token) + expect(result).to be_a(AtomicTenant::DeploymentManager::DeploymentStrategyResult) + expect(result.application_instance_id).to be_nil + end + end + + context "with missing application_key from target_link_uri" do + let(:decoded_token) { base_decoded_token.tap { |t| t["https://purl.imsglobal.org/spec/lti/claim/target_link_uri"] = "https://" } } + + it "returns empty result" do + result = strategy.call(decoded_id_token: decoded_token) + expect(result).to be_a(AtomicTenant::DeploymentManager::DeploymentStrategyResult) + expect(result.application_instance_id).to be_nil + end + end + + context "with invalid application_key from target_link_uri" do + let(:decoded_token) { base_decoded_token.tap { |t| t["https://purl.imsglobal.org/spec/lti/claim/target_link_uri"] = "https://invalid-application.example.com" } } + + it "returns empty result" do + result = strategy.call(decoded_id_token: decoded_token) + expect(result).to be_a(AtomicTenant::DeploymentManager::DeploymentStrategyResult) + expect(result.application_instance_id).to be_nil + end + end + + context "with trusted issuer creating new instance" do + let(:decoded_token) { base_decoded_token } + + it "creates application instance with pinned platform guid" do + result = strategy.call(decoded_id_token: decoded_token) + expect(result).to be_a(AtomicTenant::DeploymentManager::DeploymentStrategyResult) + expect(result.application_instance_id).to be_present + + # Verify the application instance was created + app_instance = ApplicationInstance.find(result.application_instance_id) + expect(app_instance.application).to eq(application) + + # Verify pinned platform guid was created + pinned_guid = AtomicTenant::PinnedPlatformGuid.find_by( + iss: "https://canvas.instructure.com", + platform_guid: "test-platform-guid", + application_id: application.id, + application_instance_id: result.application_instance_id + ) + expect(pinned_guid).to be_present + end + + it "allows creating unlimited instances for trusted issuers" do + # Set a low limit for testing + AtomicTenant.untrusted_iss_tenant_limit = 1 + + # Create an existing instance from the same trusted issuer + existing = create(:application_instance, application: application) + AtomicTenant::LtiDeployment.create!( + iss: "https://canvas.instructure.com", + application_instance: existing, + deployment_id: "existing-deployment" + ) + + # Should still allow creating a new instance because issuer is trusted + expect do + result = strategy.call(decoded_id_token: decoded_token) + expect(result.application_instance_id).not_to eq(existing.id) + expect(result.application_instance_id).to be_present + end.not_to raise_error + end + end + + context "with existing application instance" do + let(:decoded_token) { base_decoded_token } + let!(:existing_app_instance) do + create(:application_instance, + application: application, + lti_key: "test-lti-key-https://atomicjolt.instructure.com" + ) + end + + it "finds existing application instance" do + result = strategy.call(decoded_id_token: decoded_token) + expect(result.application_instance_id).to eq(existing_app_instance.id) + end + end + + context "with untrusted issuer" do + let(:decoded_token) { base_decoded_token.tap { |t| t["iss"] = "https://untrusted.example.com" } } + + before do + AtomicTenant.untrusted_iss_tenant_limit = 5 + end + + context "when under tenant limit" do + before do + # Create fewer than the limit + 3.times do |i| + app_inst = create(:application_instance, application: application) + AtomicTenant::LtiDeployment.create!( + iss: "https://untrusted.example.com", + application_instance: app_inst, + deployment_id: "deployment-#{i}" + ) + end + end + + it "allows creation of new instance" do + expect { strategy.call(decoded_id_token: decoded_token) }.not_to raise_error + end + end + + context "when at tenant limit" do + before do + # Create exactly the limit + 5.times do |i| + app_inst = create(:application_instance, application: application) + AtomicTenant::LtiDeployment.create!( + iss: "https://untrusted.example.com", + application_instance: app_inst, + deployment_id: "deployment-#{i}" + ) + end + end + + it "raises OnboardingException" do + expect { + strategy.call(decoded_id_token: decoded_token) + }.to raise_error(AtomicTenant::Exceptions::OnboardingException, /has reached the limit/) + end + end + end + end + + describe "platform-specific tenant creation" do + context "with Canvas platform" do + let(:decoded_token) do + base_decoded_token.merge( + "iss" => "https://canvas.instructure.com", + "https://purl.imsglobal.org/spec/lti/claim/tool_platform" => { + "guid" => "canvas-guid-123", + "product_family_code" => "canvas" + }, + "https://purl.imsglobal.org/spec/lti/claim/custom" => { + "canvas_api_domain" => "custom.canvas.com" + } + ) + end + + it "calls create_new_instance with correct site_url from canvas_api_domain" do + mock_app_instance = create(:application_instance, application: application) + + expect(strategy).to receive(:find_existing) + .with(application, "https://custom.canvas.com", "https://canvas.instructure.com", "canvas-guid-123") + .and_return(nil) + + expect(strategy).to receive(:create_new_instance) + .with(application, "https://custom.canvas.com", "https://canvas.instructure.com", "canvas-guid-123") + .and_return(mock_app_instance) + + result = strategy.call(decoded_id_token: decoded_token) + expect(result.application_instance_id).to eq(mock_app_instance.id) + end + + it "calls find_existing with correct site_url from canvas_api_domain" do + mock_app_instance = create(:application_instance, application: application) + + expect(strategy).to receive(:find_existing) + .with(application, "https://custom.canvas.com", "https://canvas.instructure.com", "canvas-guid-123") + .and_return(mock_app_instance) + + result = strategy.call(decoded_id_token: decoded_token) + expect(result.application_instance_id).to eq(mock_app_instance.id) + end + + it "raises OnboardingException when canvas_api_domain is missing" do + invalid_token = decoded_token.tap { |t| t["https://purl.imsglobal.org/spec/lti/claim/custom"] = {} } + + expect { + strategy.call(decoded_id_token: invalid_token) + }.to raise_error(AtomicTenant::Exceptions::OnboardingException, /Missing canvas_api_domain/) + end + end + + context "with Blackboard platform" do + let(:decoded_token) do + base_decoded_token.merge( + "iss" => "https://blackboard.com", + "https://purl.imsglobal.org/spec/lti/claim/tool_platform" => { + "guid" => "bb-guid-456", + "product_family_code" => "BlackboardLearn", + "url" => "blackboard.example.com" + } + ) + end + + it "calls create_new_instance with correct site_url from platform url" do + mock_app_instance = create(:application_instance, application: application) + + expect(strategy).to receive(:find_existing) + .with(application, "https://blackboard.example.com", "https://blackboard.com", "bb-guid-456") + .and_return(nil) + + expect(strategy).to receive(:create_new_instance) + .with(application, "https://blackboard.example.com", "https://blackboard.com", "bb-guid-456") + .and_return(mock_app_instance) + + result = strategy.call(decoded_id_token: decoded_token) + expect(result.application_instance_id).to eq(mock_app_instance.id) + end + + it "calls find_existing with correct site_url from platform url" do + mock_app_instance = create(:application_instance, application: application) + + expect(strategy).to receive(:find_existing) + .with(application, "https://blackboard.example.com", "https://blackboard.com", "bb-guid-456") + .and_return(mock_app_instance) + + result = strategy.call(decoded_id_token: decoded_token) + expect(result.application_instance_id).to eq(mock_app_instance.id) + end + + it "raises OnboardingException when platform url is missing" do + invalid_token = decoded_token.tap do |t| + t["https://purl.imsglobal.org/spec/lti/claim/tool_platform"].delete("url") + end + + expect { + strategy.call(decoded_id_token: invalid_token) + }.to raise_error(AtomicTenant::Exceptions::OnboardingException, /Missing url in platform claim/) + end + end + + context "with D2L platform" do + let(:decoded_token) do + base_decoded_token.merge( + "iss" => "https://atomicjolt.brightspace.com", + "https://purl.imsglobal.org/spec/lti/claim/tool_platform" => { + "guid" => "d2l-guid-789", + "product_family_code" => "desire2learn" + } + ) + end + + it "calls create_new_instance with correct site_url from issuer" do + mock_app_instance = create(:application_instance, application: application) + + expect(strategy).to receive(:find_existing) + .with(application, "https://atomicjolt.brightspace.com", "https://atomicjolt.brightspace.com", "d2l-guid-789") + .and_return(nil) + + expect(strategy).to receive(:create_new_instance) + .with(application, "https://atomicjolt.brightspace.com", "https://atomicjolt.brightspace.com", "d2l-guid-789") + .and_return(mock_app_instance) + + result = strategy.call(decoded_id_token: decoded_token) + expect(result.application_instance_id).to eq(mock_app_instance.id) + end + + it "calls find_existing with correct site_url from issuer" do + mock_app_instance = create(:application_instance, application: application) + + expect(strategy).to receive(:find_existing) + .with(application, "https://atomicjolt.brightspace.com", "https://atomicjolt.brightspace.com", "d2l-guid-789") + .and_return(mock_app_instance) + + result = strategy.call(decoded_id_token: decoded_token) + expect(result.application_instance_id).to eq(mock_app_instance.id) + end + end + end + + describe "concurrent platform guid creation" do + let(:decoded_token) do + base_decoded_token.merge( + "iss" => "https://existing.platform.com", + "https://purl.imsglobal.org/spec/lti/claim/tool_platform" => { + "guid" => "existing-platform-guid", + "product_family_code" => "moodle" + } + ) + end + let(:existing_app_instance) do + create(:application_instance, + application: application, + lti_key: "test-lti-key-https://existing.platform.com" + ) + end + let!(:existing_pinned_guid) do + AtomicTenant::PinnedPlatformGuid.create!( + iss: "https://existing.platform.com", + platform_guid: "existing-platform-guid", + application_id: application.id, + application_instance_id: existing_app_instance.id + ) + end + + it "returns existing application instance when found" do + result = strategy.call(decoded_id_token: decoded_token) + + expect(result.application_instance_id).to eq(existing_app_instance.id) + + pinned_guids = AtomicTenant::PinnedPlatformGuid.where( + iss: "https://existing.platform.com", + platform_guid: "existing-platform-guid", + application_id: application.id + ) + + expect(pinned_guids.count).to eq(1) + expect(pinned_guids.first).to eq(existing_pinned_guid) + end + end + + describe "concurrent instance creation handling" do + let(:decoded_token) { base_decoded_token } + + context "when RecordNotUnique is raised during creation" do + it "calls find_existing twice when creation fails" do + mock_app_instance = create(:application_instance, application: application) + + # Simulate concurrent creation by making create_new_instance raise RecordNotUnique + allow_any_instance_of(TestAutoCreateStrategy).to receive(:create_new_instance).and_raise(ActiveRecord::RecordNotUnique) + + # Expect find_existing to be called twice - first returns nil, second returns the instance + expect_any_instance_of(TestAutoCreateStrategy).to receive(:find_existing) + .and_return(nil, mock_app_instance) + + strategy.call(decoded_id_token: decoded_token) + end + end + end +end From 6ad63fc8eb6d0c6edc285df955747e750cbc22d2 Mon Sep 17 00:00:00 2001 From: David Spencer Date: Wed, 8 Oct 2025 17:35:14 -0400 Subject: [PATCH 4/5] Make create/find named consistent --- ...ract_auto_create_platform_guid_strategy.rb | 8 +++---- ...auto_create_platform_guid_strategy_spec.rb | 22 +++++++------------ 2 files changed, 12 insertions(+), 18 deletions(-) diff --git a/lib/atomic_tenant/deployment_manager/abstract_auto_create_platform_guid_strategy.rb b/lib/atomic_tenant/deployment_manager/abstract_auto_create_platform_guid_strategy.rb index c3a31a3..f573dde 100644 --- a/lib/atomic_tenant/deployment_manager/abstract_auto_create_platform_guid_strategy.rb +++ b/lib/atomic_tenant/deployment_manager/abstract_auto_create_platform_guid_strategy.rb @@ -44,7 +44,7 @@ def call(decoded_id_token:) site_url = extract_site_url(decoded_id_token) - app_inst = find_existing(app, site_url, issuer, platform_guid) + app_inst = find_existing_instance(app, site_url, issuer, platform_guid) app_inst ||= maybe_create_new_instance(app, site_url, issuer, platform_guid) pin = pin_platform_guid(issuer, platform_guid, app.id, app_inst.id) AtomicTenant::DeploymentManager::DeploymentStrategyResult.new(application_instance_id: pin.application_instance_id) @@ -52,8 +52,8 @@ def call(decoded_id_token:) private - def find_existing(current_application, site_url, issuer, platform_guid) - raise NotImplementedError, "Subclasses must implement #find_existing" + def find_existing_instance(current_application, site_url, issuer, platform_guid) + raise NotImplementedError, "Subclasses must implement #find_existing_instance" end def create_new_instance(app, site_url, issuer, platform_guid) @@ -65,7 +65,7 @@ def maybe_create_new_instance(app, site_url, issuer, platform_guid) create_new_instance(app, site_url, issuer, platform_guid) rescue ActiveRecord::RecordNotUnique # If we get a RecordNotUnique error, it means another process created the instance concurrently. - find_existing(app, site_url, issuer, platform_guid) + find_existing_instance(app, site_url, issuer, platform_guid) end end diff --git a/spec/lib/atomic_tenant/deployment_manager/abstract_auto_create_platform_guid_strategy_spec.rb b/spec/lib/atomic_tenant/deployment_manager/abstract_auto_create_platform_guid_strategy_spec.rb index 6ec8621..4da9cd3 100644 --- a/spec/lib/atomic_tenant/deployment_manager/abstract_auto_create_platform_guid_strategy_spec.rb +++ b/spec/lib/atomic_tenant/deployment_manager/abstract_auto_create_platform_guid_strategy_spec.rb @@ -19,7 +19,7 @@ def create_new_instance(app, site_url, issuer, platform_guid) # Implement find_existing to work with the dummy app structure # In real apps, this would query by tenant field/relationship - def find_existing(current_application, site_url, issuer, platform_guid) + def find_existing_instance(current_application, site_url, issuer, platform_guid) ApplicationInstance.find_by( application: current_application, lti_key: "test-lti-key-#{site_url}" @@ -44,12 +44,6 @@ def find_existing(current_application, site_url, issuer, platform_guid) } end - before do - # Configure the gem settings - AtomicTenant.tenant_uuid_namespace = "6ba7b810-9dad-11d1-80b4-00c04fd430c8" - AtomicTenant.untrusted_iss_tenant_limit = 100 - end - describe "#name" do it "raises NotImplementedError for abstract class" do abstract_strategy = AtomicTenant::DeploymentManager::AbstractAutoCreatePlatformGuidStrategy.new @@ -225,7 +219,7 @@ def find_existing(current_application, site_url, issuer, platform_guid) it "calls create_new_instance with correct site_url from canvas_api_domain" do mock_app_instance = create(:application_instance, application: application) - expect(strategy).to receive(:find_existing) + expect(strategy).to receive(:find_existing_instance) .with(application, "https://custom.canvas.com", "https://canvas.instructure.com", "canvas-guid-123") .and_return(nil) @@ -240,7 +234,7 @@ def find_existing(current_application, site_url, issuer, platform_guid) it "calls find_existing with correct site_url from canvas_api_domain" do mock_app_instance = create(:application_instance, application: application) - expect(strategy).to receive(:find_existing) + expect(strategy).to receive(:find_existing_instance) .with(application, "https://custom.canvas.com", "https://canvas.instructure.com", "canvas-guid-123") .and_return(mock_app_instance) @@ -272,7 +266,7 @@ def find_existing(current_application, site_url, issuer, platform_guid) it "calls create_new_instance with correct site_url from platform url" do mock_app_instance = create(:application_instance, application: application) - expect(strategy).to receive(:find_existing) + expect(strategy).to receive(:find_existing_instance) .with(application, "https://blackboard.example.com", "https://blackboard.com", "bb-guid-456") .and_return(nil) @@ -287,7 +281,7 @@ def find_existing(current_application, site_url, issuer, platform_guid) it "calls find_existing with correct site_url from platform url" do mock_app_instance = create(:application_instance, application: application) - expect(strategy).to receive(:find_existing) + expect(strategy).to receive(:find_existing_instance) .with(application, "https://blackboard.example.com", "https://blackboard.com", "bb-guid-456") .and_return(mock_app_instance) @@ -320,7 +314,7 @@ def find_existing(current_application, site_url, issuer, platform_guid) it "calls create_new_instance with correct site_url from issuer" do mock_app_instance = create(:application_instance, application: application) - expect(strategy).to receive(:find_existing) + expect(strategy).to receive(:find_existing_instance) .with(application, "https://atomicjolt.brightspace.com", "https://atomicjolt.brightspace.com", "d2l-guid-789") .and_return(nil) @@ -335,7 +329,7 @@ def find_existing(current_application, site_url, issuer, platform_guid) it "calls find_existing with correct site_url from issuer" do mock_app_instance = create(:application_instance, application: application) - expect(strategy).to receive(:find_existing) + expect(strategy).to receive(:find_existing_instance) .with(application, "https://atomicjolt.brightspace.com", "https://atomicjolt.brightspace.com", "d2l-guid-789") .and_return(mock_app_instance) @@ -397,7 +391,7 @@ def find_existing(current_application, site_url, issuer, platform_guid) allow_any_instance_of(TestAutoCreateStrategy).to receive(:create_new_instance).and_raise(ActiveRecord::RecordNotUnique) # Expect find_existing to be called twice - first returns nil, second returns the instance - expect_any_instance_of(TestAutoCreateStrategy).to receive(:find_existing) + expect_any_instance_of(TestAutoCreateStrategy).to receive(:find_existing_instance) .and_return(nil, mock_app_instance) strategy.call(decoded_id_token: decoded_token) From db4cdcf720e9abe9a4ba40f17f5abe5b8bfe2d72 Mon Sep 17 00:00:00 2001 From: David Spencer Date: Thu, 9 Oct 2025 09:32:56 -0400 Subject: [PATCH 5/5] Make find/create names more descriptive find_existing is redundant, so is create_new. Instead call it exactly what we're finding/creating --- ...ract_auto_create_platform_guid_strategy.rb | 18 ++++---- ...auto_create_platform_guid_strategy_spec.rb | 46 +++++++++---------- 2 files changed, 32 insertions(+), 32 deletions(-) diff --git a/lib/atomic_tenant/deployment_manager/abstract_auto_create_platform_guid_strategy.rb b/lib/atomic_tenant/deployment_manager/abstract_auto_create_platform_guid_strategy.rb index f573dde..e2cc411 100644 --- a/lib/atomic_tenant/deployment_manager/abstract_auto_create_platform_guid_strategy.rb +++ b/lib/atomic_tenant/deployment_manager/abstract_auto_create_platform_guid_strategy.rb @@ -44,28 +44,28 @@ def call(decoded_id_token:) site_url = extract_site_url(decoded_id_token) - app_inst = find_existing_instance(app, site_url, issuer, platform_guid) - app_inst ||= maybe_create_new_instance(app, site_url, issuer, platform_guid) + app_inst = find_application_instance(app, site_url, issuer, platform_guid) + app_inst ||= maybe_create_application_instance(app, site_url, issuer, platform_guid) pin = pin_platform_guid(issuer, platform_guid, app.id, app_inst.id) AtomicTenant::DeploymentManager::DeploymentStrategyResult.new(application_instance_id: pin.application_instance_id) end private - def find_existing_instance(current_application, site_url, issuer, platform_guid) - raise NotImplementedError, "Subclasses must implement #find_existing_instance" + def find_application_instance(current_application, site_url, issuer, platform_guid) + raise NotImplementedError, "Subclasses must implement #find_application_instance" end - def create_new_instance(app, site_url, issuer, platform_guid) - raise NotImplementedError, "Subclasses must implement #create_new_instance" + def create_application_instance(app, site_url, issuer, platform_guid) + raise NotImplementedError, "Subclasses must implement #create_application_instance" end - def maybe_create_new_instance(app, site_url, issuer, platform_guid) + def maybe_create_application_instance(app, site_url, issuer, platform_guid) ActiveRecord::Base.transaction do - create_new_instance(app, site_url, issuer, platform_guid) + create_application_instance(app, site_url, issuer, platform_guid) rescue ActiveRecord::RecordNotUnique # If we get a RecordNotUnique error, it means another process created the instance concurrently. - find_existing_instance(app, site_url, issuer, platform_guid) + find_application_instance(app, site_url, issuer, platform_guid) end end diff --git a/spec/lib/atomic_tenant/deployment_manager/abstract_auto_create_platform_guid_strategy_spec.rb b/spec/lib/atomic_tenant/deployment_manager/abstract_auto_create_platform_guid_strategy_spec.rb index 4da9cd3..f71a869 100644 --- a/spec/lib/atomic_tenant/deployment_manager/abstract_auto_create_platform_guid_strategy_spec.rb +++ b/spec/lib/atomic_tenant/deployment_manager/abstract_auto_create_platform_guid_strategy_spec.rb @@ -8,7 +8,7 @@ def name private - def create_new_instance(app, site_url, issuer, platform_guid) + def create_application_instance(app, site_url, issuer, platform_guid) # Simple test implementation that creates an ApplicationInstance # In a real implementation, this would also create the tenant/schema ApplicationInstance.create!( @@ -17,9 +17,9 @@ def create_new_instance(app, site_url, issuer, platform_guid) ) end - # Implement find_existing to work with the dummy app structure + # Implement find_application_instance to work with the dummy app structure # In real apps, this would query by tenant field/relationship - def find_existing_instance(current_application, site_url, issuer, platform_guid) + def find_application_instance(current_application, site_url, issuer, platform_guid) ApplicationInstance.find_by( application: current_application, lti_key: "test-lti-key-#{site_url}" @@ -216,14 +216,14 @@ def find_existing_instance(current_application, site_url, issuer, platform_guid) ) end - it "calls create_new_instance with correct site_url from canvas_api_domain" do + it "calls create_application_instance with correct site_url from canvas_api_domain" do mock_app_instance = create(:application_instance, application: application) - expect(strategy).to receive(:find_existing_instance) + expect(strategy).to receive(:find_application_instance) .with(application, "https://custom.canvas.com", "https://canvas.instructure.com", "canvas-guid-123") .and_return(nil) - expect(strategy).to receive(:create_new_instance) + expect(strategy).to receive(:create_application_instance) .with(application, "https://custom.canvas.com", "https://canvas.instructure.com", "canvas-guid-123") .and_return(mock_app_instance) @@ -231,10 +231,10 @@ def find_existing_instance(current_application, site_url, issuer, platform_guid) expect(result.application_instance_id).to eq(mock_app_instance.id) end - it "calls find_existing with correct site_url from canvas_api_domain" do + it "calls find_application_instance with correct site_url from canvas_api_domain" do mock_app_instance = create(:application_instance, application: application) - expect(strategy).to receive(:find_existing_instance) + expect(strategy).to receive(:find_application_instance) .with(application, "https://custom.canvas.com", "https://canvas.instructure.com", "canvas-guid-123") .and_return(mock_app_instance) @@ -263,14 +263,14 @@ def find_existing_instance(current_application, site_url, issuer, platform_guid) ) end - it "calls create_new_instance with correct site_url from platform url" do + it "calls create_application_instance with correct site_url from platform url" do mock_app_instance = create(:application_instance, application: application) - expect(strategy).to receive(:find_existing_instance) + expect(strategy).to receive(:find_application_instance) .with(application, "https://blackboard.example.com", "https://blackboard.com", "bb-guid-456") .and_return(nil) - expect(strategy).to receive(:create_new_instance) + expect(strategy).to receive(:create_application_instance) .with(application, "https://blackboard.example.com", "https://blackboard.com", "bb-guid-456") .and_return(mock_app_instance) @@ -278,10 +278,10 @@ def find_existing_instance(current_application, site_url, issuer, platform_guid) expect(result.application_instance_id).to eq(mock_app_instance.id) end - it "calls find_existing with correct site_url from platform url" do + it "calls find_application_instance with correct site_url from platform url" do mock_app_instance = create(:application_instance, application: application) - expect(strategy).to receive(:find_existing_instance) + expect(strategy).to receive(:find_application_instance) .with(application, "https://blackboard.example.com", "https://blackboard.com", "bb-guid-456") .and_return(mock_app_instance) @@ -311,14 +311,14 @@ def find_existing_instance(current_application, site_url, issuer, platform_guid) ) end - it "calls create_new_instance with correct site_url from issuer" do + it "calls create_application_instance with correct site_url from issuer" do mock_app_instance = create(:application_instance, application: application) - expect(strategy).to receive(:find_existing_instance) + expect(strategy).to receive(:find_application_instance) .with(application, "https://atomicjolt.brightspace.com", "https://atomicjolt.brightspace.com", "d2l-guid-789") .and_return(nil) - expect(strategy).to receive(:create_new_instance) + expect(strategy).to receive(:create_application_instance) .with(application, "https://atomicjolt.brightspace.com", "https://atomicjolt.brightspace.com", "d2l-guid-789") .and_return(mock_app_instance) @@ -326,10 +326,10 @@ def find_existing_instance(current_application, site_url, issuer, platform_guid) expect(result.application_instance_id).to eq(mock_app_instance.id) end - it "calls find_existing with correct site_url from issuer" do + it "calls find_application_instance with correct site_url from issuer" do mock_app_instance = create(:application_instance, application: application) - expect(strategy).to receive(:find_existing_instance) + expect(strategy).to receive(:find_application_instance) .with(application, "https://atomicjolt.brightspace.com", "https://atomicjolt.brightspace.com", "d2l-guid-789") .and_return(mock_app_instance) @@ -384,14 +384,14 @@ def find_existing_instance(current_application, site_url, issuer, platform_guid) let(:decoded_token) { base_decoded_token } context "when RecordNotUnique is raised during creation" do - it "calls find_existing twice when creation fails" do + it "calls find_application_instance twice when creation fails" do mock_app_instance = create(:application_instance, application: application) - # Simulate concurrent creation by making create_new_instance raise RecordNotUnique - allow_any_instance_of(TestAutoCreateStrategy).to receive(:create_new_instance).and_raise(ActiveRecord::RecordNotUnique) + # Simulate concurrent creation by making create_application_instance raise RecordNotUnique + allow_any_instance_of(TestAutoCreateStrategy).to receive(:create_application_instance).and_raise(ActiveRecord::RecordNotUnique) - # Expect find_existing to be called twice - first returns nil, second returns the instance - expect_any_instance_of(TestAutoCreateStrategy).to receive(:find_existing_instance) + # Expect find_application_instance to be called twice - first returns nil, second returns the instance + expect_any_instance_of(TestAutoCreateStrategy).to receive(:find_application_instance) .and_return(nil, mock_app_instance) strategy.call(decoded_id_token: decoded_token)