Merge pull request #14087 from opf/bug/50871-adding-a-second-one-driv…

…e-storage-with-same-azure-app-breaks-second-storage

[#50871] added storage id to oauth state cookie

app/controllers/oauth_clients_controller.rb

@@ -132,8 +132,20 @@ def set_connection_manager
     )
   end
 
+  # rubocop:disable Metrics/AbcSize
   def find_oauth_client
-    @oauth_client = OAuthClient.find_by(client_id: params[:oauth_client_id])
+    oauth_client_from_cookie = -> do
+      cookie = cookies["oauth_state_#{@oauth_state}"]
+      return nil if cookie.blank?
+
+      # FIXME: This is a hack, fetching additional information of the storage to identify the oauth client.
+      # This must be fixed in #50872.
+      state_value = MultiJson.load(cookie, symbolize_keys: true)
+      @oauth_client = OAuthClient.find_by(client_id: params[:oauth_client_id],
+                                          integration_id: state_value[:storageId])
+    end
+
+    @oauth_client = oauth_client_from_cookie.call
     if @oauth_client.nil?
       # oauth_client can be nil if OAuthClient was not found.
       # This happens during admin setup if the user forgot to update the return_uri
@@ -149,6 +161,8 @@ def find_oauth_client
     end
   end
 
+  # rubocop:enable Metrics/AbcSize
+
   def redirect_user_or_admin(redirect_uri = nil)
     # This needs to be modified as soon as we support more integration types.
     if User.current.admin && redirect_uri && (nextcloud? || one_drive?)

app/services/oauth_clients/redirect_uri_from_state_service.rb

@@ -51,7 +51,11 @@ def call
     def oauth_state_cookie
       return nil if @state.blank?
 
-      @cookies["oauth_state_#{@state}"]
+      state_cookie = @cookies["oauth_state_#{@state}"]
+      return nil if state_cookie.blank?
+
+      state_value = MultiJson.load(@cookies["oauth_state_#{@state}"], symbolize_keys: true)
+      state_value[:href]
     end
   end
 end

frontend/src/app/shared/components/storages/storage-information/storage-information.service.ts

@@ -97,6 +97,7 @@ export class StorageInformationService {
       this.text.authorizationFailureHeader(storageType),
       this.text.authorizationFailureContent(storageType),
       {
+        storageId: storage.id,
         storageType: storage._links.type.href,
         authorizationLink: storage._links.authorize,
       },

frontend/src/app/shared/components/storages/storage-login-button/storage-login-button.component.ts

@@ -73,9 +73,11 @@ export class StorageLoginButtonComponent implements OnInit {
   }
 
   private setAuthorizationCallbackCookie(nonce:string):void {
-    this.cookieService.set(`oauth_state_${nonce}`, window.location.href, {
-      path: '/',
-    });
+    this.cookieService.set(
+      `oauth_state_${nonce}`,
+      JSON.stringify({ href: window.location.href, storageId: this.input.storageId }),
+      { path: '/' },
+    );
   }
 
   private static authorizationFailureActionUrl(baseUrl:string, nonce:string):string {

frontend/src/app/shared/components/storages/storage-login-button/storage-login-input.ts

@@ -26,9 +26,12 @@
 // See COPYRIGHT and LICENSE files for more details.
 //++
 
+import { ID } from '@datorama/akita';
+
 import { IHalResourceLink } from 'core-app/core/state/hal-resource';
 
 export interface IStorageLoginInput {
+  storageId:ID;
   storageType:string;
   authorizationLink:IHalResourceLink;
 }

modules/storages/app/helpers/storage_login_helper.rb

@@ -36,6 +36,7 @@ def storage_login_input(storage)
                            .new(user: current_user, configuration: storage.oauth_configuration)
 
     {
+      storageId: storage.id,
       storageType: API::V3::Storages::STORAGE_TYPE_URN_MAP[storage.provider_type],
       authorizationLink: {
         href: connection_manager.get_authorization_uri

spec/requests/oauth_clients/callback_flow_spec.rb

@@ -51,12 +51,8 @@
            client_id: 'kETWr2XsjPxhVbN7Q5jmPq83xribuUTRzgfXthpYT0vSqyJWm4dOnivKzHiZasf0',
            client_secret: 'J1sg4L5PYbM2RZL3pUyxTnamvfpcP5eUcCPmeCQHJO60Gy6CJIdDaF4yXOeC8BPS')
   end
-  let(:rack_oauth2_client) do
-    instance_double(Rack::OAuth2::Client)
-  end
-  let(:connection_manager) do
-    instance_double(OAuthClients::ConnectionManager)
-  end
+  let(:rack_oauth2_client) { instance_double(Rack::OAuth2::Client) }
+  let(:connection_manager) { instance_double(OAuthClients::ConnectionManager) }
   let(:uri) { URI(File.join('oauth_clients', oauth_client.client_id, 'callback')) }
 
   subject(:response) { last_response }
@@ -66,13 +62,13 @@
 
     allow(Rack::OAuth2::Client).to receive(:new).and_return(rack_oauth2_client)
     allow(rack_oauth2_client)
-      .to receive(:access_token!).with(:body)
-            .and_return(
-              Rack::OAuth2::AccessToken::Bearer.new(access_token: 'xyzaccesstoken',
-                                                    refresh_token: 'xyzrefreshtoken')
-            )
+      .to receive(:access_token!)
+            .with(:body)
+            .and_return(Rack::OAuth2::AccessToken::Bearer.new(access_token: 'xyzaccesstoken',
+                                                              refresh_token: 'xyzrefreshtoken'))
     allow(rack_oauth2_client).to receive(:authorization_code=)
-    set_cookie "oauth_state_asdf1234=#{redirect_uri}"
+    state_cookie = CGI.escape({ href: redirect_uri, storageId: oauth_client.integration_id }.to_json)
+    set_cookie "oauth_state_asdf1234=#{state_cookie}"
   end
 
   # rubocop:disable RSpec/Rails/HaveHttpStatus

spec/services/oauth_clients/redirect_uri_from_state_service_spec.rb

@@ -32,7 +32,7 @@
 RSpec.describe OAuthClients::RedirectUriFromStateService, type: :model do
   let(:state) { 'asdf123425' }
   let(:redirect_uri) { File.join(API::V3::Utilities::PathHelper::ApiV3Path::root_url, 'foo/bar') }
-  let(:cookies) { { "oauth_state_#{state}": redirect_uri }.with_indifferent_access }
+  let(:cookies) { { "oauth_state_#{state}": { href: redirect_uri }.to_json }.with_indifferent_access }
   let(:instance) { described_class.new(state:, cookies:) }
 
   describe '#call' do