From 21b33b7aa168aae5e035750bb281c78632b68d5c Mon Sep 17 00:00:00 2001 From: Manish Rathi Date: Fri, 11 Jun 2021 23:27:16 +0200 Subject: [PATCH 1/8] [regression][pilot] Fix upload using `api_key_path + apple_id` CLI options --- pilot/lib/pilot/build_manager.rb | 9 ++-- pilot/spec/build_manager_spec.rb | 83 ++++++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+), 3 deletions(-) diff --git a/pilot/lib/pilot/build_manager.rb b/pilot/lib/pilot/build_manager.rb index 22a4e06d856..57d54cd25f1 100644 --- a/pilot/lib/pilot/build_manager.rb +++ b/pilot/lib/pilot/build_manager.rb @@ -14,6 +14,12 @@ def upload(options) # Only need to login before upload if no apple_id was given # 'login' will be deferred until before waiting for build processing should_login_in_start = options[:apple_id].nil? + + # see: https://github.com/fastlane/fastlane/issues/18767 + # We will do App Store Connect API login, if needed on start and set the token to use JWT auth in the future. + # 'login' will happen only if token is not set and api-key or key-path is given + should_login_in_start = Spaceship::ConnectAPI.token.nil? && (options[:api_key_path] || options[:api_key]) + start(options, should_login: should_login_in_start) UI.user_error!("No ipa file given") unless config[:ipa] @@ -366,9 +372,6 @@ def expire_previous_builds(build) # If there are multiple teams, infer the provider from the selected team name. # If there are fewer than two teams, don't infer the provider. def transporter_for_selected_team(options) - # Ensure that user is authenticated - start(options) - # Use JWT auth api_token = Spaceship::ConnectAPI.token unless api_token.nil? diff --git a/pilot/spec/build_manager_spec.rb b/pilot/spec/build_manager_spec.rb index 197f91c4cf5..9a681b99136 100644 --- a/pilot/spec/build_manager_spec.rb +++ b/pilot/spec/build_manager_spec.rb @@ -534,6 +534,89 @@ fake_build_manager.upload(upload_options) end + + shared_examples "performing the spaceship login for JWT auth by pilot" do + before(:each) do + expect(fake_build_manager).to(receive(:login)) + end + + it "performs the login using Manager.login" do + fake_build_manager.upload(upload_options) + end + end + + shared_examples "skipping the spaceship login for JWT auth by pilot" do + before(:each) do + expect(fake_build_manager).not_to(receive(:login)) + end + + it "skips the login using Manager.login" do + fake_build_manager.upload(upload_options) + end + end + + describe "what happens when spaceship token is nil" do + before(:each) do + allow(Spaceship::ConnectAPI).to receive(:token).and_return(nil) + end + + context "when input options has NO api_key or api_key_path input" do + before(:each) do + upload_options[:api_key] = nil + upload_options[:api_key_path] = nil + end + + it_behaves_like "skipping the spaceship login for JWT auth by pilot" + end + + context "when input options has api_key input" do + before(:each) do + upload_options[:api_key] = "fake api key" + end + + it_behaves_like "performing the spaceship login for JWT auth by pilot" + end + + context "when input options has api_key_path input" do + before(:each) do + upload_options[:api_key_path] = "./spaceship/spec/connect_api/fixtures/asc_key.json" + end + + it_behaves_like "performing the spaceship login for JWT auth by pilot" + end + end + + describe "what happens when spaceship token is already set" do + before(:each) do + fake_api_key_json_path = "./spaceship/spec/connect_api/fixtures/asc_key.json" + allow(Spaceship::ConnectAPI).to receive(:token).and_return(Spaceship::ConnectAPI::Token.from(filepath: fake_api_key_json_path)) + end + + context "when input options has NO api_key or api_key_path input" do + before(:each) do + upload_options[:api_key] = nil + upload_options[:api_key_path] = nil + end + + it_behaves_like "skipping the spaceship login for JWT auth by pilot" + end + + context "when input options has api_key input" do + before(:each) do + upload_options[:api_key] = "fake api key" + end + + it_behaves_like "skipping the spaceship login for JWT auth by pilot" + end + + context "when input options has api_key_path input" do + before(:each) do + upload_options[:api_key_path] = "./spaceship/spec/connect_api/fixtures/asc_key.json" + end + + it_behaves_like "skipping the spaceship login for JWT auth by pilot" + end + end end end From e0204514cd52369ea2b8780545a8b09c5af7c7d9 Mon Sep 17 00:00:00 2001 From: Manish Rathi Date: Fri, 11 Jun 2021 23:37:37 +0200 Subject: [PATCH 2/8] clean up --- pilot/lib/pilot/build_manager.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pilot/lib/pilot/build_manager.rb b/pilot/lib/pilot/build_manager.rb index 57d54cd25f1..ab90cb19f6a 100644 --- a/pilot/lib/pilot/build_manager.rb +++ b/pilot/lib/pilot/build_manager.rb @@ -18,7 +18,7 @@ def upload(options) # see: https://github.com/fastlane/fastlane/issues/18767 # We will do App Store Connect API login, if needed on start and set the token to use JWT auth in the future. # 'login' will happen only if token is not set and api-key or key-path is given - should_login_in_start = Spaceship::ConnectAPI.token.nil? && (options[:api_key_path] || options[:api_key]) + should_login_in_start = true if Spaceship::ConnectAPI.token.nil? && (options[:api_key_path] || options[:api_key]) start(options, should_login: should_login_in_start) From ddbcf70038096543f9f4919f2648de7504bc9ccf Mon Sep 17 00:00:00 2001 From: Manish Rathi Date: Sat, 12 Jun 2021 00:13:28 +0200 Subject: [PATCH 3/8] reverted changes.... --- pilot/lib/pilot/build_manager.rb | 9 ++-- pilot/spec/build_manager_spec.rb | 83 -------------------------------- 2 files changed, 3 insertions(+), 89 deletions(-) diff --git a/pilot/lib/pilot/build_manager.rb b/pilot/lib/pilot/build_manager.rb index ab90cb19f6a..22a4e06d856 100644 --- a/pilot/lib/pilot/build_manager.rb +++ b/pilot/lib/pilot/build_manager.rb @@ -14,12 +14,6 @@ def upload(options) # Only need to login before upload if no apple_id was given # 'login' will be deferred until before waiting for build processing should_login_in_start = options[:apple_id].nil? - - # see: https://github.com/fastlane/fastlane/issues/18767 - # We will do App Store Connect API login, if needed on start and set the token to use JWT auth in the future. - # 'login' will happen only if token is not set and api-key or key-path is given - should_login_in_start = true if Spaceship::ConnectAPI.token.nil? && (options[:api_key_path] || options[:api_key]) - start(options, should_login: should_login_in_start) UI.user_error!("No ipa file given") unless config[:ipa] @@ -372,6 +366,9 @@ def expire_previous_builds(build) # If there are multiple teams, infer the provider from the selected team name. # If there are fewer than two teams, don't infer the provider. def transporter_for_selected_team(options) + # Ensure that user is authenticated + start(options) + # Use JWT auth api_token = Spaceship::ConnectAPI.token unless api_token.nil? diff --git a/pilot/spec/build_manager_spec.rb b/pilot/spec/build_manager_spec.rb index 9a681b99136..197f91c4cf5 100644 --- a/pilot/spec/build_manager_spec.rb +++ b/pilot/spec/build_manager_spec.rb @@ -534,89 +534,6 @@ fake_build_manager.upload(upload_options) end - - shared_examples "performing the spaceship login for JWT auth by pilot" do - before(:each) do - expect(fake_build_manager).to(receive(:login)) - end - - it "performs the login using Manager.login" do - fake_build_manager.upload(upload_options) - end - end - - shared_examples "skipping the spaceship login for JWT auth by pilot" do - before(:each) do - expect(fake_build_manager).not_to(receive(:login)) - end - - it "skips the login using Manager.login" do - fake_build_manager.upload(upload_options) - end - end - - describe "what happens when spaceship token is nil" do - before(:each) do - allow(Spaceship::ConnectAPI).to receive(:token).and_return(nil) - end - - context "when input options has NO api_key or api_key_path input" do - before(:each) do - upload_options[:api_key] = nil - upload_options[:api_key_path] = nil - end - - it_behaves_like "skipping the spaceship login for JWT auth by pilot" - end - - context "when input options has api_key input" do - before(:each) do - upload_options[:api_key] = "fake api key" - end - - it_behaves_like "performing the spaceship login for JWT auth by pilot" - end - - context "when input options has api_key_path input" do - before(:each) do - upload_options[:api_key_path] = "./spaceship/spec/connect_api/fixtures/asc_key.json" - end - - it_behaves_like "performing the spaceship login for JWT auth by pilot" - end - end - - describe "what happens when spaceship token is already set" do - before(:each) do - fake_api_key_json_path = "./spaceship/spec/connect_api/fixtures/asc_key.json" - allow(Spaceship::ConnectAPI).to receive(:token).and_return(Spaceship::ConnectAPI::Token.from(filepath: fake_api_key_json_path)) - end - - context "when input options has NO api_key or api_key_path input" do - before(:each) do - upload_options[:api_key] = nil - upload_options[:api_key_path] = nil - end - - it_behaves_like "skipping the spaceship login for JWT auth by pilot" - end - - context "when input options has api_key input" do - before(:each) do - upload_options[:api_key] = "fake api key" - end - - it_behaves_like "skipping the spaceship login for JWT auth by pilot" - end - - context "when input options has api_key_path input" do - before(:each) do - upload_options[:api_key_path] = "./spaceship/spec/connect_api/fixtures/asc_key.json" - end - - it_behaves_like "skipping the spaceship login for JWT auth by pilot" - end - end end end From 9fff5853aa32314bca8600f535829e29a1d4fa2b Mon Sep 17 00:00:00 2001 From: Manish Rathi Date: Sat, 12 Jun 2021 01:00:14 +0200 Subject: [PATCH 4/8] Start Pilot with App Store Connect API login if possible --- pilot/lib/pilot/build_manager.rb | 3 --- pilot/lib/pilot/manager.rb | 5 ++++- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pilot/lib/pilot/build_manager.rb b/pilot/lib/pilot/build_manager.rb index 22a4e06d856..454b98520bc 100644 --- a/pilot/lib/pilot/build_manager.rb +++ b/pilot/lib/pilot/build_manager.rb @@ -366,9 +366,6 @@ def expire_previous_builds(build) # If there are multiple teams, infer the provider from the selected team name. # If there are fewer than two teams, don't infer the provider. def transporter_for_selected_team(options) - # Ensure that user is authenticated - start(options) - # Use JWT auth api_token = Spaceship::ConnectAPI.token unless api_token.nil? diff --git a/pilot/lib/pilot/manager.rb b/pilot/lib/pilot/manager.rb index 8624863dc89..f2dc0e5bf6d 100644 --- a/pilot/lib/pilot/manager.rb +++ b/pilot/lib/pilot/manager.rb @@ -13,7 +13,10 @@ class Manager def start(options, should_login: true) return if @config # to not login multiple times @config = options - login if should_login + + # we will always start with App Store Connect API login if possible + # else fallback to 'should_login' param for 'apple_id' login + login if (config[:api_key] || config[:api_key_path]) || should_login end def login From b70f3dce8235901c1ec8926650e40f3e2023b5ab Mon Sep 17 00:00:00 2001 From: Manish Rathi Date: Sat, 12 Jun 2021 01:08:51 +0200 Subject: [PATCH 5/8] start with App Store Connect API login if required and possible --- pilot/lib/pilot/manager.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pilot/lib/pilot/manager.rb b/pilot/lib/pilot/manager.rb index f2dc0e5bf6d..49f063376d6 100644 --- a/pilot/lib/pilot/manager.rb +++ b/pilot/lib/pilot/manager.rb @@ -14,9 +14,10 @@ def start(options, should_login: true) return if @config # to not login multiple times @config = options - # we will always start with App Store Connect API login if possible + # we will always start with App Store Connect API login if required and possible # else fallback to 'should_login' param for 'apple_id' login - login if (config[:api_key] || config[:api_key_path]) || should_login + should_login_for_appstore_connect_api = Spaceship::ConnectAPI.token.nil? && (options[:api_key_path] || options[:api_key]) + login if should_login_for_appstore_connect_api || should_login end def login From 0f0b946cea3d07b9dad464baa7c131540175ac96 Mon Sep 17 00:00:00 2001 From: Manish Rathi Date: Mon, 14 Jun 2021 14:25:24 +0200 Subject: [PATCH 6/8] PR review fix --- pilot/lib/pilot/manager.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pilot/lib/pilot/manager.rb b/pilot/lib/pilot/manager.rb index 49f063376d6..63e14685382 100644 --- a/pilot/lib/pilot/manager.rb +++ b/pilot/lib/pilot/manager.rb @@ -14,9 +14,9 @@ def start(options, should_login: true) return if @config # to not login multiple times @config = options - # we will always start with App Store Connect API login if required and possible + # we will always start with App Store Connect API login 'if possible' # else fallback to 'should_login' param for 'apple_id' login - should_login_for_appstore_connect_api = Spaceship::ConnectAPI.token.nil? && (options[:api_key_path] || options[:api_key]) + should_login_for_appstore_connect_api = (options[:api_key_path] || options[:api_key]) login if should_login_for_appstore_connect_api || should_login end From 3e7d7a37e836ca23924b73894011da948e5418b6 Mon Sep 17 00:00:00 2001 From: Manish Rathi Date: Mon, 14 Jun 2021 18:27:43 +0200 Subject: [PATCH 7/8] clean-up --- pilot/lib/pilot/manager.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pilot/lib/pilot/manager.rb b/pilot/lib/pilot/manager.rb index 63e14685382..707b2002dc0 100644 --- a/pilot/lib/pilot/manager.rb +++ b/pilot/lib/pilot/manager.rb @@ -16,8 +16,7 @@ def start(options, should_login: true) # we will always start with App Store Connect API login 'if possible' # else fallback to 'should_login' param for 'apple_id' login - should_login_for_appstore_connect_api = (options[:api_key_path] || options[:api_key]) - login if should_login_for_appstore_connect_api || should_login + login if config[:api_key_path] || config[:api_key] || should_login end def login From c3a43a65ea14051ebafd355d5a4dd4d176bb6ed7 Mon Sep 17 00:00:00 2001 From: Manish Rathi Date: Mon, 14 Jun 2021 21:20:43 +0200 Subject: [PATCH 8/8] Added unit tests --- pilot/lib/pilot/manager.rb | 2 +- pilot/spec/manager_spec.rb | 40 ++++++++++++++++++++++++++++++++------ 2 files changed, 35 insertions(+), 7 deletions(-) diff --git a/pilot/lib/pilot/manager.rb b/pilot/lib/pilot/manager.rb index 707b2002dc0..d4bb43ab749 100644 --- a/pilot/lib/pilot/manager.rb +++ b/pilot/lib/pilot/manager.rb @@ -16,7 +16,7 @@ def start(options, should_login: true) # we will always start with App Store Connect API login 'if possible' # else fallback to 'should_login' param for 'apple_id' login - login if config[:api_key_path] || config[:api_key] || should_login + login if options[:api_key_path] || options[:api_key] || should_login end def login diff --git a/pilot/spec/manager_spec.rb b/pilot/spec/manager_spec.rb index acbb604c7f4..b672c8c9b7f 100644 --- a/pilot/spec/manager_spec.rb +++ b/pilot/spec/manager_spec.rb @@ -58,15 +58,43 @@ end context "when passing 'should_login' value as FALSE" do - before(:each) do - expect(fake_manager).not_to receive(:login) + context "when input options has no 'api_key' or 'api_key_path' param" do + before(:each) do + expect(fake_manager).not_to receive(:login) + end + + it "sets the 'config' variable value and doesn't call login" do + options = {} + fake_manager.start(options, should_login: false) + + expect(fake_manager.instance_variable_get(:@config)).to eq(options) + end end - it "sets the 'config' variable value and doesn't call login" do - options = {} - fake_manager.start(options, should_login: false) + context "when input options has 'api_key' param" do + before(:each) do + expect(fake_manager).to receive(:login) + end - expect(fake_manager.instance_variable_get(:@config)).to eq(options) + it "sets the 'config' variable value and calls login for appstore connect api token" do + options = { api_key: "fake_api_key" } + fake_manager.start(options, should_login: false) + + expect(fake_manager.instance_variable_get(:@config)).to eq(options) + end + end + + context "when input options has 'api_key_path' param" do + before(:each) do + expect(fake_manager).to receive(:login) + end + + it "sets the 'config' variable value and calls login for appstore connect api token" do + options = { api_key_path: "fake api_key_path" } + fake_manager.start(options, should_login: false) + + expect(fake_manager.instance_variable_get(:@config)).to eq(options) + end end end end