fix: #14385 (direct login infinite redirect loop)

app/controllers/account_controller.rb

@@ -40,14 +40,27 @@ class AccountController < ApplicationController
 
   # Login request and validation
   def login
-    if User.current.logged?
+    user = User.current
+
+    if user.logged?
       redirect_to home_url
     elsif Concerns::OmniauthLogin.direct_login?
-      ps = {}.tap do |p|
-        p[:origin] = params[:back_url] if params[:back_url]
-      end
+      if flash.empty?
+        ps = {}.tap do |p|
+          p[:origin] = params[:back_url] if params[:back_url]
+        end
 
-      redirect_to Concerns::OmniauthLogin.direct_login_provider_url(ps)
+        redirect_to Concerns::OmniauthLogin.direct_login_provider_url(ps)
+      else
+        instructions =
+          if user.active?
+            I18n.t :instructions_after_error, signin: translation_signin_link
+          else
+            I18n.t :instructions_after_registration, signin: translation_signin_link
+          end
+
+        render :exit, locals: { instructions: instructions }
+      end
     elsif request.post?
       authenticate_user
     end
@@ -56,7 +69,15 @@ class AccountController < ApplicationController
   # Log out current user and redirect to welcome page
   def logout
     logout_user
-    redirect_to home_url
+    if Concerns::OmniauthLogin.direct_login?
+      flash.now[:notice] = I18n.t :notice_logged_out
+      render :exit,
+             locals: {
+               instructions: I18n.t(:instructions_after_logout, signin: translation_signin_link)
+             }
+    else
+      redirect_to home_url
+    end
   end
 
   # Enable user to choose a new password
@@ -443,4 +464,8 @@ class AccountController < ApplicationController
       redirect_back_or_default :controller => '/my', :action => 'page'
     end
   end
+
+  def translation_signin_link
+    view_context.link_to(I18n.t('label_here'), view_context.signin_path)
+  end
 end

app/views/account/exit.html.erb

@@ -0,0 +1,45 @@
+<%#-- copyright
+OpenProject is a project management system.
+Copyright (C) 2012-2014 the OpenProject Foundation (OPF)
+
+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 doc/COPYRIGHT.rdoc for more details.
+
+++#%>
+
+<%#
+LOCALS
+
+  instructions: String
+#%>
+
+<% disable_accessibility_css! %>
+
+<% breadcrumb_paths(l(:label_login)) %>
+<%= call_hook :view_account_login_top %>
+<div id="login-form" class="form">
+  <h1><%= I18n.t(:label_login) %></h1>
+  <hr class="form_separator">
+  <p><%= instructions.html_safe %></p>
+</div>
+<%= call_hook :view_account_login_bottom %>

config/locales/de.yml

@@ -579,6 +579,10 @@ de:
   gui_validation_error: "1 Fehler"
   gui_validation_error_plural: "%{count} Fehler"
 
+  instructions_after_registration: "Sie können sich %{signin} einloggen sobald Ihr Konto aktiviert wurde."
+  instructions_after_logout: "Sie können sich %{signin} wieder einloggen."
+  instructions_after_error: "Sie können noch mal versuchen sich einzuloggen, indem Sie %{signin} klicken. Wenn der Fehler weiterhin auftritt, wenden Sie sich bitte an den Ihren Admin."
+
   label_account: "Konto"
   label_activity: "Aktivität"
   label_add_another_file: "Eine weitere Datei hinzufügen"
@@ -712,6 +716,7 @@ de:
   label_group_new: "Neue Gruppe"
   label_group_plural: "Gruppen"
   label_help: "Hilfe"
+  label_here: hier
   label_hide: "Verbergen"
   label_history: "Historie"
   label_home: "Hauptseite"
@@ -1090,6 +1095,7 @@ de:
   notice_unable_delete_time_entry: "Der Zeiterfassungseintrag konnte nicht gelöscht werden."
   notice_unable_delete_version: "Die Version konnte nicht gelöscht werden."
   notice_automatic_set_of_standard_type: "Der Standard-Typ wurde automatisch gesetzt."
+  notice_logged_out: "Sie wurden ausgeloggt."
   error_types_in_use_by_work_packages: "Die folgenden Typen werden von Arbeitspaketen referenziert: %{types}"
 
  # Default format for numbers

config/locales/en.yml

@@ -576,6 +576,10 @@ en:
   gui_validation_error: "1 error"
   gui_validation_error_plural: "%{count} errors"
 
+  instructions_after_registration: "You can sign in as soon as your account has been activated by clicking %{signin}."
+  instructions_after_logout: "You can sign in again by clicking %{signin}."
+  instructions_after_error: "You can try to sign in again by clicking %{signin}. If the error persists, ask your admin for help."
+
   label_account: "Account"
   label_activity: "Activity"
   label_add_another_file: "Add another file"
@@ -709,6 +713,7 @@ en:
   label_group_new: "New group"
   label_group_plural: "Groups"
   label_help: "Help"
+  label_here: here
   label_hide: "Hide"
   label_history: "History"
   label_home: "Home"
@@ -1088,6 +1093,7 @@ en:
   notice_unable_delete_time_entry: "Unable to delete time log entry."
   notice_unable_delete_version: "Unable to delete version."
   notice_automatic_set_of_standard_type: "Set standard type automatically."
+  notice_logged_out: "You have been logged out."
   error_types_in_use_by_work_packages: "The following types are still referenced by work packages: %{types}"
 
   # Default format for numbers

config/routes.rb

@@ -57,6 +57,7 @@ OpenProject::Application.routes.draw do
 
     match '/login', :action => 'login',  :as => 'signin', :via => [:get, :post]
     get '/logout', :action => 'logout', :as => 'signout'
+    get '/exit', :action => 'exit'
   end
 
   namespace :api do

spec/features/omniauth/omniauth_spec.rb

@@ -101,6 +101,29 @@ describe 'Omniauth authentication' do
     end
   end
 
+  describe 'sign out a user with direct login' do
+    let!(:user) do
+      FactoryGirl.create(:user,
+                         force_password_change: false,
+                         identity_url: 'developer:omnibob@example.com',
+                         login: 'omnibob',
+                         mail: 'omnibob@example.com',
+                         firstname: 'omni',
+                         lastname: 'bob'
+                        )
+    end
+
+    before do
+      Concerns::OmniauthLogin.stub(:direct_login_provider).and_return('developer')
+    end
+
+    it 'shows a notice that the user has been logged out' do
+      visit signout_path
+
+      expect(page).to have_content(I18n.t(:notice_logged_out))
+    end
+  end
+
   shared_examples 'omniauth user registration' do
     it 'should register new user' do
       visit '/auth/developer'
@@ -166,15 +189,69 @@ describe 'Omniauth authentication' do
     end
   end
 
+  context 'registration by email' do
+    before do
+      allow(Setting).to receive(:self_registration?).and_return(true)
+      allow(Setting).to receive(:self_registration).and_return('1')
+    end
+
+    shared_examples 'registration with registration by email' do
+      it 'shows a note explaining that the account has to be activated' do
+        visit login_path
+
+        # login form developer strategy
+        fill_in 'first_name', with: 'Ifor'
+        fill_in 'last_name',  with: 'McAlistar'
+        fill_in 'email',      with: 'i.mcalistar@example.com'
+
+        click_link_or_button 'Sign In'
+
+        expect(page).to have_content(I18n.t(:notice_account_register_done))
+      end
+    end
+
+    it_behaves_like 'registration with registration by email' do
+      let(:login_path) { '/auth/developer' }
+    end
+
+    context 'with direct login enabled' do
+      before do
+        Concerns::OmniauthLogin.stub(:direct_login_provider).and_return('developer')
+      end
+
+      it_behaves_like 'registration with registration by email' do
+        # i.e. it still shows a notice
+        # instead of redirecting straight back to the omniauth login provider
+        let(:login_path) { signin_path }
+      end
+    end
+  end
+
   context 'error occurs' do
-    it 'should fail with generic error message' do
-      # set omniauth to test mode will redirect all calls to omniauth
-      # directly to the callback and by setting the mock_auth provider
-      # to a symbol will force omniauth to fail /auth/failure
-      OmniAuth.config.test_mode = true
-      OmniAuth.config.mock_auth[:developer] = :invalid_credentials
-      visit '/auth/developer'
-      expect(page).to have_content(I18n.t(:error_external_authentication_failed))
+    shared_examples 'omniauth signin error' do
+      it 'should fail with generic error message' do
+        # set omniauth to test mode will redirect all calls to omniauth
+        # directly to the callback and by setting the mock_auth provider
+        # to a symbol will force omniauth to fail /auth/failure
+        OmniAuth.config.test_mode = true
+        OmniAuth.config.mock_auth[:developer] = :invalid_credentials
+        visit login_path
+        expect(page).to have_content(I18n.t(:error_external_authentication_failed))
+      end
+    end
+
+    it_behaves_like 'omniauth signin error' do
+      let(:login_path) { '/auth/developer' }
+    end
+
+    context 'with direct login' do
+      before do
+        Concerns::OmniauthLogin.stub(:direct_login_provider).and_return('developer')
+      end
+
+      it_behaves_like 'omniauth signin error' do
+        let(:login_path) { signin_path }
+      end
     end
   end
 end