Fix named instance review findings

Author: jnunemaker

README.md

@@ -123,6 +123,10 @@ through `FLIPPER_CLOUD_CROSS_APP_TOKEN` and
 `FLIPPER_CLOUD_CROSS_APP_SYNC_SECRET`. Named instances never inherit the default
 `FLIPPER_CLOUD_TOKEN` or `FLIPPER_CLOUD_SYNC_SECRET`.
 
+Applications sharing a Cloud project must register the same named group predicates
+and return the same `flipper_id` for shared actors. Cloud synchronizes group names and
+actor identifiers, not Ruby group definitions or application identity mappings.
+
 ## Flipper Cloud
 
 Like Flipper and want more? Check out [Flipper Cloud](https://www.flippercloud.io?utm_source=oss&utm_medium=readme&utm_campaign=check_out), which comes with:

lib/flipper.rb

@@ -340,7 +340,8 @@ def refresh_named_instance_accessors
         next if named_instance_accessor_names.include?(name)
 
         validate_named_instance_name!(name)
-        define_singleton_method(name) { named(name) }
+        proxy = named(name)
+        define_singleton_method(name) { proxy }
         named_instance_accessor_names.add(name)
         named_instance_accessor_methods[name] = method(name)
       end

lib/flipper/cloud/routes.rb

@@ -1,5 +1,7 @@
 # Default routes loaded by Flipper::Cloud::Engine
 Rails.application.routes.draw do
+  cloud_mounts = []
+
   if ENV["FLIPPER_CLOUD_TOKEN"] && !ENV.fetch("FLIPPER_CLOUD_SYNC_SECRET", "").empty?
     require 'flipper/cloud'
     config = Rails.application.config.flipper
@@ -9,24 +11,31 @@
       memoizer_options: { preload: config.preload }
     )
 
-    mount cloud_app, at: config.cloud_path
+    cloud_mounts << [config.cloud_path, cloud_app]
   end
 
-  if Flipper.configuration.respond_to?(:named_instance_names)
+  if !Rails.application.config.flipper.test_help && Flipper.configuration.respond_to?(:named_instance_names)
     Flipper.configuration.named_instance_names.each do |name|
       named = Flipper.configuration.named_configuration(name)
       next unless named.cloud? && named.cloud_path
 
       cloud_options = named.resolve_cloud_credentials
       sync_secret = cloud_options[:sync_secret]
-      next if sync_secret.nil? || sync_secret.empty?
+      next if sync_secret.nil? || sync_secret == false || sync_secret.empty?
 
       require "flipper/cloud"
       cloud_app = Flipper::Cloud.app(Flipper.named(name),
         env_key: named.env_key,
         memoizer_options: { preload: named.preload }
       )
-      mount cloud_app, at: named.cloud_path
+      cloud_mounts << [named.cloud_path, cloud_app]
     end
   end
+
+  cloud_mounts.sort_by do |path, _|
+    normalized_path = path.to_s.sub(%r{\A/+}, "").sub(%r{/+\z}, "")
+    -normalized_path.length
+  end.each do |path, cloud_app|
+    mount cloud_app, at: path
+  end
 end

lib/flipper/engine.rb

@@ -74,11 +74,16 @@ def self.default_strict_value
         config.use Flipper::Adapters::ActorLimit, flipper.actor_limit if flipper.actor_limit
       end
 
+      if flipper.test_help
+        require "flipper/test_help"
+        Flipper::TestHelp.flipper_configure_named_instances
+      end
+
       if Flipper.configuration.respond_to?(:named_instance_names)
         Flipper.configuration.named_instance_names.each do |name|
           named = Flipper.configuration.named_configuration(name)
           named.inherit_rails_configuration(flipper)
-          if named.cloud?
+          if named.cloud? && !flipper.test_help
             named.resolve_cloud_credentials({
               token: app.credentials.dig(:flipper, name, :cloud_token),
               sync_secret: app.credentials.dig(:flipper, name, :cloud_sync_secret),
@@ -127,6 +132,7 @@ def self.default_strict_value
     end
 
     initializer "flipper.named_cloud_paths", after: :load_config_initializers do |app|
+      next if app.config.flipper.test_help
       next unless Flipper.configuration.respond_to?(:named_instance_names)
 
       named_paths = Flipper.configuration.named_instance_names.map do |name|

lib/flipper/test_help.rb

@@ -10,15 +10,18 @@ def flipper_configure
       Flipper.configure do |config|
         config.adapter { adapter }
         config.default { Flipper.new(config.adapter) }
+        flipper_configure_named_instances(config)
+      end
+    end
 
-        if config.respond_to?(:named_instance_names)
-          config.named_instance_names.each do |name|
-            named = config.named_configuration(name)
-            named_adapter = Flipper::Adapters::Memory.new
-            named.adapter { named_adapter }
-            named.default { Flipper.new(named.adapter) }
-          end
-        end
+    def flipper_configure_named_instances(config = Flipper.configuration)
+      return unless config.respond_to?(:named_instance_names)
+
+      config.named_instance_names.each do |name|
+        named = config.named_configuration(name)
+        named_adapter = Flipper::Adapters::Memory.new
+        named.adapter { named_adapter }
+        named.default { Flipper.new(named.adapter) }
       end
     end
 
@@ -29,11 +32,9 @@ def flipper_reset
       if Flipper.configuration.respond_to?(:named_instance_names)
         Flipper.configuration.named_instance_names.each do |name|
           named = Flipper.configuration.named_configuration(name)
-          if named.cloud?
-            named_adapter = Flipper::Adapters::Memory.new
-            named.adapter { named_adapter }
-            named.default { Flipper.new(named.adapter) }
-          end
+          named_adapter = Flipper::Adapters::Memory.new
+          named.adapter { named_adapter }
+          named.default { Flipper.new(named.adapter) }
           Flipper.named(name).features.each(&:remove) rescue nil
         end
       end

spec/flipper/engine_spec.rb

@@ -315,10 +315,26 @@ def get_all
         to raise_error(Flipper::InvalidConfigurationValue, /environment keys must be unique/)
     end
 
+    it "boots test apps with named Cloud configured and no credentials" do
+      Rails.env = "test"
+      initializer do
+        Flipper.configure do |flipper_config|
+          flipper_config.named(:cross_app) do |named|
+            named.cloud(path: "_cross_app")
+          end
+        end
+      end
+
+      expect { subject }.not_to raise_error
+      expect(Flipper.cross_app.enabled?(:chat)).to be(false)
+      expect(a_request(:any, /flippercloud/)).not_to have_been_made
+    end
+
     context "with named Cloud" do
       let(:app) { application.routes }
       let(:named_cloud_path) { "_cross_app" }
       let(:named_sync_secret) { "named-secret" }
+      let(:named_cloud_options) { {path: named_cloud_path} }
       let(:request_body) do
         JSON.generate({
           "environment_id" => 1,
@@ -341,7 +357,7 @@ def get_all
         initializer do
           Flipper.configure do |flipper_config|
             flipper_config.named(:cross_app) do |named|
-              named.cloud(path: named_cloud_path)
+              named.cloud(named_cloud_options)
               named.register(:cross_app_group) { true }
             end
           end
@@ -372,6 +388,34 @@ def get_all
         expect(Flipper.cross_app.instance).to be_a(Flipper::Cloud::DSL)
       end
 
+      context "when nested under the default Cloud path" do
+        let(:named_cloud_path) { "_flipper/cross_app" }
+
+        before do
+          ENV["FLIPPER_CLOUD_TOKEN"] = "default-token"
+          ENV["FLIPPER_CLOUD_SYNC_SECRET"] = "default-secret"
+        end
+
+        after do
+          ENV.delete("FLIPPER_CLOUD_TOKEN")
+          ENV.delete("FLIPPER_CLOUD_SYNC_SECRET")
+        end
+
+        it "routes the more specific named webhook first" do
+          silence { application.initialize! }
+          stub = stub_request(:get, /features\?_cb=\d+&exclude_gate_names=true/).with({
+            headers: { "flipper-cloud-token" => "named-token" },
+          }).to_return(status: 200, body: JSON.generate({features: {}}), headers: {})
+
+          post "/_flipper/cross_app", request_body, {
+            "HTTP_FLIPPER_CLOUD_SIGNATURE" => signature_header_value,
+          }
+
+          expect(last_response.status).to eq(200)
+          expect(stub).to have_been_requested
+        end
+      end
+
       context "when the path matches the default" do
         let(:named_cloud_path) { "/_flipper/" }
 
@@ -400,6 +444,20 @@ def get_all
           expect(last_response.status).to eq(404)
         end
       end
+
+      context "with a false sync secret" do
+        let(:named_cloud_options) { {path: named_cloud_path, sync_secret: false} }
+
+        it "does not mount a webhook" do
+          silence { application.initialize! }
+
+          post "/_cross_app", request_body, {
+            "HTTP_FLIPPER_CLOUD_SIGNATURE" => signature_header_value,
+          }
+
+          expect(last_response.status).to eq(404)
+        end
+      end
     end
 
     it "loads named Cloud credentials from the matching Rails credential scope" do

spec/flipper/test_help_spec.rb

@@ -40,6 +40,16 @@
     expect(a_request(:any, /flippercloud/)).not_to have_been_made
   end
 
+  it "shares Memory for a non-Cloud named instance added after test setup" do
+    described_class.flipper_configure
+    Flipper.configure { |config| config.named(:cross_app) }
+
+    described_class.flipper_reset
+    Flipper.cross_app.enable(:chat)
+
+    expect(Thread.new { Flipper.cross_app.enabled?(:chat) }.value).to be(true)
+  end
+
   it "clears named features while preserving registered groups" do
     Flipper.configure do |config|
       config.named(:cross_app)