Differentiate error handling for storages

Introducing an explicit difference between users that were never linked
and those that have a link (i.e. a remote identity) but receive an authentication
error anyways. There is also differentiation between SSO users and non-SSO users.
When authenticating to the storage via SSO, it wouldn't make sense to show a login
button to users.

config/locales/en.yml

@@ -4577,6 +4577,7 @@ en:
       connected: "Connected"
       error: "Error"
       failed_authorization: "Authorization failed"
+      not_connected: "Not connected"
     labels:
       label_oauth_integration: "OAuth2 integration"
       label_redirect_uri: "Redirect URI"

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

@@ -40,6 +40,7 @@ import {
   fileLinkStatusError,
   storageAuthorizationError,
   storageConnected,
+  storageNotConnected,
   storageFailedAuthorization,
 } from 'core-app/shared/components/storages/storages-constants.const';
 
@@ -48,11 +49,15 @@ export class StorageInformationService {
   private text = {
     fileLinkErrorHeader: this.i18n.t('js.storages.information.live_data_error'),
     fileLinkErrorContent: (storageType:string):string => this.i18n.t('js.storages.information.live_data_error_description', { storageType }),
+    authenticationErrorHeader: (storageType:string):string => this.i18n.t('js.storages.authentication_error', { storageType }),
+    authenticationErrorContent: (storageType:string):string => this.i18n.t('js.storages.information.authentication_error', { storageType }),
     connectionErrorHeader: (storageType:string):string => this.i18n.t('js.storages.no_connection', { storageType }),
     connectionErrorContent: (storageType:string):string => this.i18n.t('js.storages.information.connection_error', { storageType }),
-    authorizationFailureHeader: (storageType:string):string => this.i18n.t('js.storages.login_to', { storageType }),
-    authorizationFailureContent: (storageType:string):string => this.i18n.t('js.storages.information.not_logged_in', { storageType }),
+    notConnectedHeader: (storageType:string):string => this.i18n.t('js.storages.login_to', { storageType }),
+    notConnectedContent: (storageType:string):string => this.i18n.t('js.storages.information.not_logged_in', { storageType }),
     loginButton: (storageType:string):string => this.i18n.t('js.storages.login', { storageType }),
+    suggestLogout: this.i18n.t('js.storages.information.suggest_logout'),
+    suggestRelink: this.i18n.t('js.storages.information.suggest_relink'),
   };
 
   constructor(
@@ -73,6 +78,8 @@ export class StorageInformationService {
           switch (storage._links.authorizationState.href) {
             case storageFailedAuthorization:
               return [this.failedAuthorizationInformation(storage, storageType)];
+            case storageNotConnected:
+              return [this.notConnectedInformation(storage, storageType)];
             case storageAuthorizationError:
               return [this.authorizationErrorInformation(storageType)];
             case storageConnected:
@@ -87,15 +94,16 @@ export class StorageInformationService {
       );
   }
 
-  private failedAuthorizationInformation(storage:IStorage, storageType:string):StorageInformationBox {
+  private notConnectedInformation(storage:IStorage, storageType:string):StorageInformationBox {
     if (!storage._links.authorize) {
-      throw new Error('Authorize link is missing!');
+      // user should authenticate through SSO, but is not yet linked, that's an error
+      return this.failedAuthorizationInformation(storage, storageType);
     }
 
     return new StorageInformationBox(
       'import',
-      this.text.authorizationFailureHeader(storageType),
-      this.text.authorizationFailureContent(storageType),
+      this.text.notConnectedHeader(storageType),
+      this.text.notConnectedContent(storageType),
       {
         storageId: storage.id,
         storageType: storage._links.type.href,
@@ -104,6 +112,20 @@ export class StorageInformationService {
     );
   }
 
+  private failedAuthorizationInformation(storage:IStorage, storageType:string):StorageInformationBox {
+    const suggestion = storage._links.authorize ? this.text.suggestRelink : this.text.suggestLogout;
+    return new StorageInformationBox(
+      'error',
+      this.text.authenticationErrorHeader(storageType),
+      `${this.text.authenticationErrorContent(storageType)} ${suggestion}`,
+      storage._links.authorize && {
+        storageId: storage.id,
+        storageType: storage._links.type.href,
+        authorizationLink: storage._links.authorize,
+      },
+    );
+  }
+
   private authorizationErrorInformation(storageType:string):StorageInformationBox {
     return new StorageInformationBox(
       'remove-link',

frontend/src/app/shared/components/storages/storages-constants.const.ts

@@ -4,6 +4,7 @@ export const oneDrive = 'urn:openproject-org:api:v3:storages:OneDrive';
 
 // Storage authorization state
 export const storageConnected = 'urn:openproject-org:api:v3:storages:authorization:Connected';
+export const storageNotConnected = 'urn:openproject-org:api:v3:storages:authorization:NotConnected';
 export const storageFailedAuthorization = 'urn:openproject-org:api:v3:storages:authorization:FailedAuthorization';
 export const storageAuthorizationError = 'urn:openproject-org:api:v3:storages:authorization:Error';
 

modules/storages/app/common/storages/peripherals/storage_interaction/authentication.rb

@@ -57,10 +57,14 @@ module Storages
 
         # Checks for the current authorization state of a user on a specific file storage.
         # Returns one of three results:
-        # - :connected If a valid user token is available
-        # - :failed_authorization If a user token is available, which is invalid and not refreshable
+        # - :connected If requests can be made to the storage in the user's name
+        # - :failed_authorization If the token to request the storage in the user's name was rejected
+        # - :not_connected if the user still needs to establish a connection to the storage
         # - :error If an unexpected error occurred
         def self.authorization_state(storage:, user:)
+          selector = AuthenticationMethodSelector.new(storage:, user:)
+          return :not_connected if RemoteIdentity.where(integration: storage, user:).none? && selector.storage_oauth?
+
           auth_strategy = Registry.resolve("#{storage}.authentication.user_bound").call(user:, storage:)
 
           Registry

modules/storages/app/common/storages/peripherals/storage_interaction/authentication_method_selector.rb

@@ -0,0 +1,68 @@
+# frozen_string_literal:true
+
+#-- copyright
+# OpenProject is an open source project management software.
+# Copyright (C) the OpenProject GmbH
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2013 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See COPYRIGHT and LICENSE files for more details.
+#++
+
+module Storages
+  module Peripherals
+    module StorageInteraction
+      class AuthenticationMethodSelector
+        attr_reader :storage, :user
+
+        def initialize(storage:, user:)
+          @storage = storage
+          @user = user
+        end
+
+        def authentication_method
+          sso_preferred = storage.authenticate_via_idp? && oidc_provider_for(user)
+
+          if sso_preferred
+            :sso
+          elsif storage.authenticate_via_storage?
+            :storage_oauth
+          end
+        end
+
+        def sso?
+          authentication_method == :sso
+        end
+
+        def storage_oauth?
+          authentication_method == :storage_oauth
+        end
+
+        private
+
+        def oidc_provider_for(user)
+          user.authentication_provider.is_a?(OpenIDConnect::Provider)
+        end
+      end
+    end
+  end
+end

modules/storages/app/common/storages/peripherals/storage_interaction/authentication_strategies/nextcloud_strategies.rb

@@ -47,13 +47,14 @@ module Storages
 
               def call(user:, storage:)
                 with_tagged_logger do
-                  sso_preferred = storage.authenticate_via_idp? && oidc_provider_for(user)
+                  selector = AuthenticationMethodSelector.new(user:, storage:)
 
-                  if sso_preferred
+                  case selector.authentication_method
+                  when :sso
                     ::Storages::Peripherals::StorageInteraction::AuthenticationStrategies::SsoUserToken
                       .strategy
                       .with_user(user)
-                  elsif storage.authenticate_via_storage?
+                  when :storage_oauth
                     ::Storages::Peripherals::StorageInteraction::AuthenticationStrategies::OAuthUserToken
                       .strategy
                       .with_user(user)

modules/storages/config/locales/js-en.yml

@@ -2,6 +2,7 @@
 en:
   js:
     storages:
+      authentication_error: "Authentication with %{storageType} failed"
       link_files_in_storage: "Link files in %{storageType}"
       link_existing_files: "Link existing files"
       upload_files: "Upload files"
@@ -21,6 +22,7 @@ en:
         default: "Storage"
 
       information:
+        authentication_error: "The request to %{storageType} could not be authenticated, that's an error."
         connection_error: >
           Some %{storageType} settings are not working. Please contact your %{storageType} administrator.
         live_data_error: "Error fetching file details"
@@ -30,6 +32,8 @@ en:
         no_file_links: "In order to link files to this work package please do it via %{storageType}."
         not_logged_in: >
           To add a link, see or upload files related to this work package, please login to %{storageType}.
+        suggest_logout: You can try whether logging out and back in fixes this problem.
+        suggest_relink: You can try whether re-linking your account via the login button below fixes this problem.
 
       files:
         already_existing_header: "This file already exists"

modules/storages/lib/api/v3/storages/storage_representer.rb

@@ -34,6 +34,7 @@
 # Reference: Roar-Rails integration: https://github.com/apotonick/roar-rails
 module API::V3::Storages
   URN_CONNECTION_CONNECTED = "#{::API::V3::URN_PREFIX}storages:authorization:Connected".freeze
+  URN_CONNECTION_NOT_CONNECTED = "#{::API::V3::URN_PREFIX}storages:authorization:NotConnected".freeze
   URN_CONNECTION_AUTH_FAILED = "#{::API::V3::URN_PREFIX}storages:authorization:FailedAuthorization".freeze
   URN_CONNECTION_ERROR = "#{::API::V3::URN_PREFIX}storages:authorization:Error".freeze
 
@@ -166,6 +167,8 @@ module API::V3::Storages
       urn = case auth_state
             when :connected
               URN_CONNECTION_CONNECTED
+            when :not_connected
+              URN_CONNECTION_NOT_CONNECTED
             when :failed_authorization
               URN_CONNECTION_AUTH_FAILED
             else
@@ -177,7 +180,7 @@ module API::V3::Storages
     end
 
     link :authorize do
-      next if hide_authorize_link?
+      next unless show_authorize_link?
 
       { href: represented.oauth_configuration.authorization_uri, title: "Authorize" }
     end
@@ -227,8 +230,14 @@ module API::V3::Storages
       current_user.admin? && represented.provider_type_nextcloud?
     end
 
-    def hide_authorize_link?
-      represented.oauth_client.blank? || authorization_state != :failed_authorization
+    def show_authorize_link?
+      selector = Storages::Peripherals::StorageInteraction::AuthenticationMethodSelector.new(
+        user: current_user, storage: represented
+      )
+
+      selector.storage_oauth? &&
+        represented.oauth_client.present? &&
+        authorization_state.in?(%i[not_connected failed_authorization])
     end
 
     def storage_projects(storage)

modules/storages/spec/common/storages/peripherals/storage_interaction/authentication_method_selector_spec.rb

@@ -0,0 +1,122 @@
+# frozen_string_literal: true
+
+#-- copyright
+# OpenProject is an open source project management software.
+# Copyright (C) the OpenProject GmbH
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2013 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See COPYRIGHT and LICENSE files for more details.
+#++
+
+require "spec_helper"
+require_module_spec_helper
+
+RSpec.describe Storages::Peripherals::StorageInteraction::AuthenticationMethodSelector do
+  subject { described_class.new(storage:, user:) }
+
+  context "if user is provisioned by an IDP" do
+    let(:provider) { create(:oidc_provider) }
+    let(:user) { create(:user, identity_url: "#{provider.slug}:me") }
+
+    context "if file storage is configured for sso only" do
+      let(:storage) { create(:nextcloud_storage, :oidc_sso_enabled) }
+
+      it { is_expected.to be_sso }
+      it { is_expected.not_to be_storage_oauth }
+
+      it "indicates an authentication method of :sso" do
+        expect(subject.authentication_method).to eq(:sso)
+      end
+    end
+
+    context "if file storage is configured for sso and oauth" do
+      let(:storage) { create(:nextcloud_storage_configured, :oidc_sso_with_fallback) }
+
+      it { is_expected.to be_sso }
+      it { is_expected.not_to be_storage_oauth }
+
+      it "indicates an authentication method of :sso" do
+        expect(subject.authentication_method).to eq(:sso)
+      end
+    end
+
+    context "if file storage is configured for oauth only" do
+      let(:storage) { create(:nextcloud_storage_configured) }
+
+      it { is_expected.not_to be_sso }
+      it { is_expected.to be_storage_oauth }
+
+      it "indicates an authentication method of :storage_oauth" do
+        expect(subject.authentication_method).to eq(:storage_oauth)
+      end
+    end
+  end
+
+  context "if user is local" do
+    let(:user) { create(:user) }
+
+    context "if file storage is configured for sso only" do
+      let(:storage) { create(:nextcloud_storage, :oidc_sso_enabled) }
+
+      it { is_expected.not_to be_sso }
+      it { is_expected.not_to be_storage_oauth }
+
+      it "indicates an authentication method of :sso" do
+        expect(subject.authentication_method).to be_nil
+      end
+    end
+
+    context "if file storage is configured for sso and oauth" do
+      let(:storage) { create(:nextcloud_storage_configured, :oidc_sso_with_fallback) }
+
+      it { is_expected.not_to be_sso }
+      it { is_expected.to be_storage_oauth }
+
+      it "indicates an authentication method of :storage_oauth" do
+        expect(subject.authentication_method).to eq(:storage_oauth)
+      end
+    end
+
+    context "if file storage is configured for oauth only" do
+      let(:storage) { create(:nextcloud_storage_configured) }
+
+      it { is_expected.not_to be_sso }
+      it { is_expected.to be_storage_oauth }
+
+      it "indicates an authentication method of :storage_oauth" do
+        expect(subject.authentication_method).to eq(:storage_oauth)
+      end
+    end
+
+    context "if file storage is configured for oauth only, but client and app not fully configured" do
+      let(:storage) { create(:nextcloud_storage) }
+
+      it { is_expected.not_to be_sso }
+      it { is_expected.to be_storage_oauth }
+
+      it "indicates an authentication method of :storage_oauth" do
+        expect(subject.authentication_method).to eq(:storage_oauth)
+      end
+    end
+  end
+end

modules/storages/spec/features/show_file_links_spec.rb

@@ -46,6 +46,7 @@ RSpec.describe "Showing of file links in work package", :js do
 
   let(:sync_service) { instance_double(Storages::FileLinkSyncService) }
   let(:authorization_state) { ServiceResult.success }
+  let(:remote_identity) { create(:remote_identity, user: current_user, integration: storage) }
 
   before do
     Storages::Peripherals::Registry.stub(
@@ -62,6 +63,7 @@ RSpec.describe "Showing of file links in work package", :js do
 
     project_storage
     file_link
+    remote_identity
 
     login_as current_user
     wp_page.visit_tab! :files
@@ -93,13 +95,25 @@ RSpec.describe "Showing of file links in work package", :js do
     end
   end
 
+  context "if user is not connected to Nextcloud" do
+    let(:remote_identity) { nil }
+
+    it "must show storage information box with login button" do
+      within_test_selector("op-tab-content--tab-section", text: "MY STORAGE", wait: 25) do
+        expect(page).to have_button("Nextcloud login")
+        expect(page).to have_text("Login to Nextcloud")
+        expect(page).to have_list_item(text: "logo.png")
+      end
+    end
+  end
+
   context "if user is not authorized in Nextcloud" do
     let(:authorization_state) { ServiceResult.failure(errors: Storages::StorageError.new(code: :unauthorized)) }
 
     it "must show storage information box with login button" do
       within_test_selector("op-tab-content--tab-section", text: "MY STORAGE", wait: 25) do
         expect(page).to have_button("Nextcloud login")
-        expect(page).to have_text("Login to Nextcloud")
+        expect(page).to have_text("Authentication with Nextcloud failed")
         expect(page).to have_list_item(text: "logo.png")
       end
     end

modules/storages/spec/lib/api/v3/storages/storages_representer_rendering_spec.rb

@@ -33,7 +33,7 @@ require_module_spec_helper
 
 RSpec.describe API::V3::Storages::StorageRepresenter, "rendering" do
   let(:oauth_client_credentials) { build_stubbed(:oauth_client) }
-  let(:user) { build_stubbed(:user) }
+  let(:user) { create(:user) }
   let(:auth_check_result) { ServiceResult.success }
   let(:representer) { described_class.new(storage, current_user: user, embed_links: true) }
 
@@ -73,6 +73,12 @@ RSpec.describe API::V3::Storages::StorageRepresenter, "rendering" do
   end
 
   shared_examples_for "common file storage links" do
+    let(:remote_identity) { create :remote_identity, user:, integration: storage }
+
+    before do
+      remote_identity
+    end
+
     describe "self" do
       it_behaves_like "has a titled link" do
         let(:link) { "self" }
@@ -107,6 +113,16 @@ RSpec.describe API::V3::Storages::StorageRepresenter, "rendering" do
           let(:title) { "Error" }
         end
       end
+
+      context "if there is no remote identity for the user at the storage" do
+        let(:remote_identity) { nil }
+
+        it_behaves_like "has a titled link" do
+          let(:link) { "authorizationState" }
+          let(:href) { "urn:openproject-org:api:v3:storages:authorization:NotConnected" }
+          let(:title) { "Not connected" }
+        end
+      end
     end
 
     describe "prepareUpload" do
@@ -173,7 +189,7 @@ RSpec.describe API::V3::Storages::StorageRepresenter, "rendering" do
       end
 
       context "as admin" do
-        let(:user) { build_stubbed(:admin) }
+        let(:user) { create(:admin) }
 
         it_behaves_like "has an untitled link" do
           let(:link) { "oauthClientCredentials" }
@@ -182,7 +198,7 @@ RSpec.describe API::V3::Storages::StorageRepresenter, "rendering" do
       end
 
       context "as admin without oauth client credentials set" do
-        let(:user) { build_stubbed(:admin) }
+        let(:user) { create(:admin) }
         let(:oauth_client_credentials) { nil }
 
         it_behaves_like "has an untitled link" do
@@ -198,13 +214,13 @@ RSpec.describe API::V3::Storages::StorageRepresenter, "rendering" do
       it { is_expected.not_to have_json_path("_embedded/oauthClientCredentials") }
 
       context "as admin" do
-        let(:user) { build_stubbed(:admin) }
+        let(:user) { create(:admin) }
 
         it { is_expected.to be_json_eql(oauth_client_credentials.id).at_path("_embedded/oauthClientCredentials/id") }
       end
 
       context "as admin without oauth client credentials set" do
-        let(:user) { build_stubbed(:admin) }
+        let(:user) { create(:admin) }
         let(:oauth_client_credentials) { nil }
 
         it { is_expected.not_to have_json_path("_embedded/oauthClientCredentials") }
@@ -260,7 +276,7 @@ RSpec.describe API::V3::Storages::StorageRepresenter, "rendering" do
         end
 
         context "as admin" do
-          let(:user) { build_stubbed(:admin) }
+          let(:user) { create(:admin) }
 
           it_behaves_like "has a titled link" do
             let(:link) { "oauthApplication" }
@@ -287,7 +303,7 @@ RSpec.describe API::V3::Storages::StorageRepresenter, "rendering" do
         it { is_expected.not_to have_json_path("_embedded/oauthApplication") }
 
         context "as admin" do
-          let(:user) { build_stubbed(:admin) }
+          let(:user) { create(:admin) }
 
           it { is_expected.to be_json_eql(oauth_application.id).at_path("_embedded/oauthApplication/id") }
         end

modules/storages/spec/requests/api/v3/storages/storages_api_spec.rb

@@ -244,29 +244,41 @@ RSpec.describe "API v3 storages resource", :storage_server_helpers, :webmock, co
         end
       end
 
-      context "when authorization succeeds and storage is connected" do
-        let(:auth_check_result) { ServiceResult.success }
+      context "when user has a remote identity for the storage" do
+        before do
+          create :remote_identity, user: current_user, integration: storage
+        end
 
-        include_examples "a storage authorization result",
-                         expected: API::V3::Storages::URN_CONNECTION_CONNECTED,
-                         has_authorize_link: false
+        context "when authorization succeeds and storage is connected" do
+          let(:auth_check_result) { ServiceResult.success }
+
+          include_examples "a storage authorization result",
+                           expected: API::V3::Storages::URN_CONNECTION_CONNECTED,
+                           has_authorize_link: false
+        end
+
+        context "when authorization fails" do
+          let(:auth_check_result) { ServiceResult.failure(errors: Storages::StorageError.new(code: :unauthorized)) }
+
+          include_examples "a storage authorization result",
+                           expected: API::V3::Storages::URN_CONNECTION_AUTH_FAILED,
+                           has_authorize_link: true
+        end
+
+        context "when authorization fails with an error" do
+          let(:auth_check_result) { ServiceResult.failure(errors: Storages::StorageError.new(code: :error)) }
+
+          include_examples "a storage authorization result",
+                           expected: API::V3::Storages::URN_CONNECTION_ERROR,
+                           has_authorize_link: false
+        end
       end
 
-      context "when authorization fails" do
-        let(:auth_check_result) { ServiceResult.failure(errors: Storages::StorageError.new(code: :unauthorized)) }
-
+      context "when user has no remote identity for the storage" do
         include_examples "a storage authorization result",
-                         expected: API::V3::Storages::URN_CONNECTION_AUTH_FAILED,
+                         expected: API::V3::Storages::URN_CONNECTION_NOT_CONNECTED,
                          has_authorize_link: true
       end
-
-      context "when authorization fails with an error" do
-        let(:auth_check_result) { ServiceResult.failure(errors: Storages::StorageError.new(code: :error)) }
-
-        include_examples "a storage authorization result",
-                         expected: API::V3::Storages::URN_CONNECTION_ERROR,
-                         has_authorize_link: false
-      end
     end
   end
 

modules/storages/spec/requests/storages/project_settings/oauth_access_grant_flow_spec.rb

@@ -97,6 +97,7 @@ RSpec.describe "GET /projects/:project_id/settings/project_storages/:id/oauth_ac
 
       before do
         Storages::Peripherals::Registry.stub("nextcloud.queries.user", ->(_) { ServiceResult.success })
+        create(:remote_identity, user:, integration: storage)
       end
 
       it "redirects to destination_url" do

spec/requests/oauth_clients/ensure_connection_flow_spec.rb

@@ -139,6 +139,7 @@ RSpec.describe "/oauth_clients/:oauth_client_id/ensure_connection endpoint", :we
           let!(:oauth_client_token) do
             create(:oauth_client_token, oauth_client:, user:)
           end
+          let!(:remote_identity) { create(:remote_identity, user:, integration: storage) }
 
           before do
             stub_request(:get, "#{storage.host}ocs/v1.php/cloud/user")