Merge remote-tracking branch 'origin/dev' into feature/grid_my_page

Gemfile

@@ -128,6 +128,9 @@ gem 'okcomputer', '~> 1.17.3'
 
 gem 'gon', '~> 6.2.1'
 
+# Lograge to provide sane and non-verbose logging
+gem 'lograge', '~> 0.10.0'
+
 # catch exceptions and send them to any airbrake compatible backend
 # don't require by default, instead load on-demand when actually configured
 gem 'airbrake', '~> 7.4.0', require: false

Gemfile.lock

@@ -506,6 +506,11 @@ GEM
       omniauth (~> 1.1)
       omniauth-openid-connect (>= 0.2.1)
       rails (>= 3.2.21)
+    lograge (0.10.0)
+      actionpack (>= 4)
+      activesupport (>= 4)
+      railties (>= 4)
+      request_store (~> 1.0)
     loofah (2.2.3)
       crass (~> 1.0.2)
       nokogiri (>= 1.5.9)
@@ -903,6 +908,7 @@ DEPENDENCIES
   letter_opener
   listen (~> 3.1)
   livingstyleguide (~> 2.0.1)
+  lograge (~> 0.10.0)
   meta-tags (~> 2.6.0)
   multi_json (~> 1.13.1)
   mysql2 (~> 0.5.0)

app/controllers/application_controller.rb

@@ -44,6 +44,7 @@ class ApplicationController < ActionController::Base
   include HookHelper
   include ::OpenProject::Authentication::SessionExpiry
   include AdditionalUrlHelpers
+  include OpenProjectErrorHelper
 
   layout 'base'
 
@@ -100,6 +101,9 @@ class ApplicationController < ActionController::Base
       # greeted with the CSRF error upon login.
       message = I18n.t(:error_token_authenticity)
       message << ' ' + I18n.t(:error_cookie_missing) if openproject_cookie_missing?
+
+      log_csrf_failure
+
       render_error status: 422, message: message
     end
   end
@@ -218,6 +222,15 @@ class ApplicationController < ActionController::Base
   end
   helper_method :openproject_cookie_missing?
 
+  ##
+  # Create CSRF issue
+  def log_csrf_failure
+    message = 'CSRF validation error'
+    message << ' (No session cookie present)' if openproject_cookie_missing?
+
+    op_handle_error message, reference: :csrf_validation_failed
+  end
+
   def log_requesting_user
     return unless Setting.log_requesting_user?
     login_and_mail = " (#{escape_for_logging(User.current.login)} ID: #{User.current.id} " \
@@ -500,6 +513,8 @@ class ApplicationController < ActionController::Base
     @message = l(@message) if @message.is_a?(Symbol)
     @status = arg[:status] || 500
 
+    op_handle_error "[Error #@status] #@message"
+
     respond_to do |format|
       format.html do
         render template: 'common/error', layout: use_layout, status: @status

app/controllers/users_controller.rb

@@ -268,7 +268,7 @@ class UsersController < ApplicationController
     # true if the user deletes him/herself
     self_delete = (@user == User.current)
 
-    DeleteUserService.new(@user, User.current).call
+    Users::DeleteService.new(@user, User.current).call
 
     flash[:notice] = l('account.deleted')
 
@@ -313,7 +313,7 @@ class UsersController < ApplicationController
   end
 
   def check_if_deletion_allowed
-    render_404 unless DeleteUserService.deletion_allowed? @user, User.current
+    render_404 unless Users::DeleteService.deletion_allowed? @user, User.current
   end
 
   def my_or_admin_layout

app/helpers/open_project_error_helper.rb

@@ -0,0 +1,36 @@
+##
+# Logging helper to forward to the OpenProject log delegator
+# which will log and report errors appropriately.
+module OpenProjectErrorHelper
+  def op_logger
+    ::OpenProject.logger
+  end
+
+  def op_handle_error(message_or_exception, context = {})
+    ::OpenProject.logger.error message_or_exception, context.merge(op_logging_context)
+  end
+
+  def op_handle_warning(message_or_exception, context = {})
+    ::OpenProject.logger.warn message_or_exception, context.merge(op_logging_context)
+  end
+
+  def op_handle_info(message_or_exception, context = {})
+    ::OpenProject.logger.info message_or_exception, context.merge(op_logging_context)
+  end
+
+  def op_handle_debug(message_or_exception, context = {})
+    ::OpenProject.logger.debug message_or_exception, context.merge(op_logging_context)
+  end
+
+  private
+
+  def op_logging_context
+    {
+      current_user: current_user,
+      params: params,
+      request: try(:request),
+      session: try(:session),
+      env: try(:env),
+    }
+  end
+end

app/seeders/root_seeder.rb

@@ -65,7 +65,11 @@ class RootSeeder < Seeder
         puts '*** Seeding development data'
         require 'factory_bot'
         # Load FactoryBot factories
-        ::FactoryBot.find_definitions
+        begin
+          ::FactoryBot.find_definitions
+        rescue => e
+          raise e unless e.message.downcase.include? "factory already registered"
+        end
 
         DevelopmentDataSeeder.new.seed!
       end

app/services/users/delete_service.rb

@@ -1,4 +1,5 @@
 #-- encoding: UTF-8
+
 #-- copyright
 # OpenProject is a project management system.
 # Copyright (C) 2012-2018 the OpenProject Foundation (OPF)
@@ -29,56 +30,59 @@
 
 ##
 # Implements the deletion of a user.
-class DeleteUserService
-  attr_reader :user, :actor
+module Users
+  class DeleteService
+    attr_reader :user, :actor
 
-  def initialize(user, actor)
-    @user = user
-    @actor = actor
-  end
-  ##
-  # Deletes the given user if allowed.
-  #
-  # @return True if the user deletion has been initiated, false otherwise.
-  def call
-    if deletion_allowed?
-      # as destroying users is a lengthy process we handle it in the background
-      # and lock the account now so that no action can be performed with it
-      user.lock!
-      Delayed::Job.enqueue DeleteUserJob.new(user.id), priority: ::ApplicationJob.priority_number(:low)
+    def initialize(user, actor)
+      @user = user
+      @actor = actor
+    end
 
-      logout! if self_delete?
+    ##
+    # Deletes the given user if allowed.
+    #
+    # @return True if the user deletion has been initiated, false otherwise.
+    def call
+      if deletion_allowed?
+        # as destroying users is a lengthy process we handle it in the background
+        # and lock the account now so that no action can be performed with it
+        user.lock!
+        Delayed::Job.enqueue DeleteUserJob.new(user.id), priority: ::ApplicationJob.priority_number(:low)
 
-      true
-    else
-      false
+        logout! if self_delete?
+
+        true
+      else
+        false
+      end
     end
-  end
 
-  ##
-  # Checks if a given user may be deleted by another one.
-  #
-  # @param user [User] User to be deleted.
-  # @param actor [User] User who wants to delete the given user.
-  def self.deletion_allowed?(user, actor)
-    if actor == user
-      Setting.users_deletable_by_self?
-    else
-      actor.admin && Setting.users_deletable_by_admins?
+    ##
+    # Checks if a given user may be deleted by another one.
+    #
+    # @param user [User] User to be deleted.
+    # @param actor [User] User who wants to delete the given user.
+    def self.deletion_allowed?(user, actor)
+      if actor == user
+        Setting.users_deletable_by_self?
+      else
+        actor.admin? && actor.active? && Setting.users_deletable_by_admins?
+      end
     end
-  end
 
-  private
+    private
 
-  def deletion_allowed?
-    self.class.deletion_allowed? user, actor
-  end
+    def deletion_allowed?
+      self.class.deletion_allowed? user, actor
+    end
 
-  def self_delete?
-    user == actor
-  end
+    def self_delete?
+      user == actor
+    end
 
-  def logout!
-    User.current = nil
+    def logout!
+      User.current = nil
+    end
   end
 end

config/initializers/lograge.rb

@@ -0,0 +1,9 @@
+Rails.application.configure do
+  config.lograge.enabled = true
+  config.lograge.base_controller_class = %w[ActionController::Base]
+
+  # Add custom data to event payload
+  config.lograge.custom_payload do |controller|
+    ::OpenProject::Logging::LogDelegator.controller_payload_hash controller
+  end
+end

docker/entrypoint-all-in-one.sh

@@ -6,6 +6,7 @@ set -o pipefail
 PGDATA=${PGDATA:=/var/lib/postgresql/9.6/main}
 PGUSER=${PGUSER:=postgres}
 PGPASSWORD=${PGPASSWORD:=postgres}
+PG_STARTUP_WAIT_TIME=${PG_STARTUP_WAIT_TIME:=10}
 PGBIN="/usr/lib/postgresql/9.6/bin"
 
 if [ ! -z "$ATTACHMENTS_STORAGE_PATH" ]; then
@@ -52,6 +53,7 @@ install_plugins() {
 }
 
 migrate() {
+	wait_for_postgres
 	pushd /usr/src/app
 	/etc/init.d/memcached start
 	bundle exec rake db:migrate db:seed db:structure:dump
@@ -60,6 +62,17 @@ migrate() {
 	popd
 }
 
+wait_for_postgres() {
+	retries=${PG_STARTUP_WAIT_TIME}
+
+	echo "Trying to contact PostgreSQL server instance or waiting for it to come online."
+
+	until su postgres -c "$PGBIN/psql $DATABASE_URL -c 'select 1;' > /dev/null 2>&1" || [ $retries -eq 0 ]; do
+		echo "Waiting for postgres server, $((retries--)) remaining attempts..."
+		sleep 3
+	done
+}
+
 if [ "$dbhost" = "127.0.0.1" ]; then
 	# initialize cluster if it does not exist yet
 	if [ -f "$PGDATA/PG_VERSION" ]; then
@@ -88,3 +101,4 @@ echo "       On first installation, the default admin credentials are login: adm
 
 echo "-----> Launching supervisord..."
 exec /usr/bin/supervisord
+

frontend/src/app/components/op-context-menu/wp-context-menu/wp-single-context-menu.ts

@@ -119,7 +119,7 @@ export class WorkPackageSingleContextMenuDirective extends OpContextMenuTrigger
         {
           href: configureFormLink.href,
           icon: 'icon-settings3',
-          linkText: configureFormLink.name,
+          linkText: I18n.t('js.button_configure-form'),
           onClick: () => false
         }
       );

lib/api/root.rb

@@ -177,6 +177,14 @@ module API
         authorize_by_with_raise(authorized, &block)
       end
 
+      def authorize_admin
+        authorize_by_with_raise(current_user.admin? && (current_user.active? || current_user.is_a?(SystemUser)))
+      end
+
+      def authorize_logged_in
+        authorize_by_with_raise(current_user.logged? && current_user.active? || current_user.is_a?(SystemUser))
+      end
+
       def raise_invalid_query_on_service_failure
         service = yield
 

lib/api/v3/users/user_representer.rb

@@ -46,7 +46,6 @@ module API
           new(user, current_user: current_user)
         end
 
-
         def initialize(user, current_user:)
           super(user, current_user: current_user)
         end
@@ -213,7 +212,7 @@ module API
         end
 
         def current_user_can_delete_represented?
-          current_user && DeleteUserService.deletion_allowed?(represented, current_user)
+          current_user && ::Users::DeleteService.deletion_allowed?(represented, current_user)
         end
       end
     end

lib/api/v3/users/users_api.rb

@@ -47,32 +47,18 @@ module API
               fail ::API::Errors::InvalidUserStatusTransition
             end
           end
-
-          def current_user_if_logged
-            if User.current.logged?
-              User.current
-            else
-              fail ::API::Errors::Unauthorized
-            end
-          end
-
-          def allow_only_admin
-            unless current_user.admin?
-              fail ::API::Errors::Unauthorized
-            end
-          end
         end
 
         resources :users do
           helpers ::API::V3::Users::CreateUser
 
           post do
-            allow_only_admin
+            authorize_admin
             create_user(request_body, current_user)
           end
 
           get do
-            allow_only_admin
+            authorize_admin
 
             query = ParamsToQueryService.new(User, current_user).call(params)
 
@@ -95,9 +81,11 @@ module API
             helpers ::API::V3::Users::UpdateUser
 
             before do
+              authorize_logged_in
+
               @user =
                 if params[:id] == 'me'
-                  current_user_if_logged
+                  User.current
                 else
                   User.find_by_unique!(params[:id])
                 end
@@ -108,12 +96,12 @@ module API
             end
 
             patch do
-              allow_only_admin
+              authorize_admin
               update_user(request_body, current_user)
             end
 
             delete do
-              if DeleteUserService.new(@user, current_user).call
+              if ::Users::DeleteService.new(@user, current_user).call
                 status 202
               else
                 fail ::API::Errors::Unauthorized
@@ -123,7 +111,7 @@ module API
             namespace :lock do
               # Authenticate lock transitions
               before do
-                allow_only_admin
+                authorize_admin
               end
 
               desc 'Set lock on user account'

lib/open_project.rb

@@ -32,6 +32,7 @@ require 'redmine/menu_manager'
 require 'redmine/activity'
 require 'redmine/search'
 require 'open_project/custom_field_format'
+require 'open_project/logging/log_delegator'
 require 'redmine/mime_type'
 require 'redmine/core_ext'
 require 'open_project/design'
@@ -41,3 +42,13 @@ require 'redmine/plugin'
 require 'redmine/notifiable'
 
 require 'csv'
+
+module OpenProject
+
+  ##
+  # Shortcut to the OpenProject log delegator, which extends
+  # default Rails error handling with other error handlers such as sentry.
+  def self.logger
+    ::OpenProject::Logging::LogDelegator
+  end
+end

lib/open_project/logging/log_delegator.rb

@@ -0,0 +1,102 @@
+module OpenProject
+  module Logging
+    class LogDelegator
+      class << self
+
+        ##
+        # Consume a message and let it be handled
+        # by all handlers
+        def log(exception, context = {})
+          message =
+            if exception.is_a? Exception
+              context[:exception] = exception
+              context[:backtrace] = clean_backtrace(exception)
+              "#{exception}: #{exception.message}"
+            else
+              exception.to_s
+            end
+
+          # Set current contexts
+          context[:level] ||= context[:exception] ? :error : :info
+          context[:current_user] ||= User.current
+
+          registered_handlers.values.each do |handler|
+            handler.call message, context
+          end
+
+          nil
+        end
+
+        %i(debug info warn error fatal unknown).each do |level|
+          define_method(level) do |*args|
+            message = args.shift
+            context = args.shift || {}
+
+            log(message, context.merge(level: level))
+          end
+        end
+
+        ##
+        # Get a clean backtrace
+        def clean_backtrace(exception)
+          return nil unless exception&.backtrace
+          Rails.backtrace_cleaner.clean exception.backtrace
+        end
+
+        ##
+        # The active set of error handlers
+        def registered_handlers
+          @handlers ||= default_handlers
+        end
+
+        ##
+        # Register a new handler
+        def register(key, handler)
+          raise "#{key} already registered" if registered_handlers.key?(key)
+          raise "handler must respond_to #call" unless handler.respond_to?(:call)
+
+          @handlers[key] = handler
+        end
+
+        ##
+        # Create a payload for lograge from a controller request line
+        def controller_payload_hash(controller)
+          {
+            user: User.current.try(:id)
+          }
+        end
+
+        private
+
+        def default_handlers
+          { rails_logger: method(:rails_logger_handler) }
+        end
+
+        ##
+        # A lambda handler for logging the error
+        # to rails.
+        def rails_logger_handler(message, context = {})
+          Rails.logger.public_send(
+            context[:level],
+            "#{context_string(context)} #{message}"
+          )
+        end
+
+        ##
+        # Create a context string
+        def context_string(context)
+          %i[current_user project reference]
+            .map do |key|
+              value = context[key]
+
+              if value
+                "[#{key}=#{value}]"
+              end
+            end
+            .compact
+            .join(' ')
+        end
+      end
+    end
+  end
+end

modules/pdf_export/app/views/export_card_configurations/_rows_format_help.html.erb

@@ -6,7 +6,7 @@
 </div>
 <div class="form--field-instructions">
   <span class="help">
-    <a href="https://github.com/finnlabs/openproject-pdf_export#usage"
+    <a href="https://github.com/opf/openproject/tree/dev/modules/pdf_export#usage"
        target="_blank"
        class="icon icon-help1">
       <%= l('help_link_rows_format') %>

spec/lib/api/v3/queries/query_representer_generation_spec.rb

@@ -33,7 +33,7 @@ describe ::API::V3::Queries::QueryRepresenter do
 
   let(:query) { FactoryBot.build_stubbed(:query, project: project) }
   let(:project) { FactoryBot.build_stubbed(:project) }
-  let(:user) { double('current_user', allowed_to?: true, admin: true, admin?: true) }
+  let(:user) { double('current_user', allowed_to?: true, admin: true, admin?: true, active?: true) }
   let(:embed_links) { true }
   let(:representer) do
     described_class.new(query, current_user: user, embed_links: embed_links)

spec/lib/api/v3/users/user_representer_spec.rb

@@ -152,7 +152,7 @@ describe ::API::V3::Users::UserRepresenter do
 
       context 'when deletion is allowed' do
         before do
-          allow(DeleteUserService).to receive(:deletion_allowed?)
+          allow(Users::DeleteService).to receive(:deletion_allowed?)
             .with(user, current_user)
             .and_return(true)
         end
@@ -164,7 +164,7 @@ describe ::API::V3::Users::UserRepresenter do
 
       context 'when deletion is not allowed' do
         before do
-          allow(DeleteUserService).to receive(:deletion_allowed?)
+          allow(Users::DeleteService).to receive(:deletion_allowed?)
             .with(user, current_user)
             .and_return(false)
         end
@@ -179,8 +179,8 @@ describe ::API::V3::Users::UserRepresenter do
       it 'is based on the representer\'s cache_key' do
         expect(OpenProject::Cache)
           .to receive(:fetch)
-                .with(representer.json_cache_key)
-                .and_call_original
+          .with(representer.json_cache_key)
+          .and_call_original
 
         representer.to_json
       end

spec/requests/api/v3/authentication_spec.rb

@@ -31,7 +31,7 @@ require 'spec_helper'
 describe API::V3, type: :request do
   describe 'basic auth' do
     let(:user)     { FactoryBot.create :user }
-    let(:resource) { "/api/v3/users/#{user.id}" }
+    let(:resource) { "/api/v3/projects" }
 
     let(:response_401) do
       {

spec/requests/api/v3/user/user_resource_spec.rb

@@ -35,20 +35,25 @@ describe 'API v3 User resource', type: :request, content_type: :json do
 
   let(:current_user) { FactoryBot.create(:user) }
   let(:user) { FactoryBot.create(:user) }
+  let(:admin) { FactoryBot.create(:admin) }
+  let(:locked_admin) { FactoryBot.create :admin, status: Principal::STATUSES[:locked] }
 
   subject(:response) { last_response }
 
+  before do
+    allow(User).to receive(:current).and_return current_user
+  end
+
   describe '#index' do
     let(:get_path) { api_v3_paths.users }
 
     before do
       user
-      allow(User).to receive(:current).and_return current_user
       get get_path
     end
 
     context 'admin user' do
-      let(:current_user) { FactoryBot.create(:admin) }
+      let(:current_user) { admin }
 
       it 'should respond with 200' do
         expect(subject.status).to eq(200)
@@ -58,13 +63,13 @@ describe 'API v3 User resource', type: :request, content_type: :json do
       # meaning the order in which they where saved
       it 'contains the user in the response' do
         expect(subject.body)
-          .to be_json_eql(user.name.to_json)
+          .to be_json_eql(current_user.name.to_json)
           .at_path('_embedded/elements/0/name')
       end
 
       it 'contains the current user in the response' do
         expect(subject.body)
-          .to be_json_eql(current_user.name.to_json)
+          .to be_json_eql(user.name.to_json)
           .at_path('_embedded/elements/1/name')
       end
 
@@ -79,7 +84,7 @@ describe 'API v3 User resource', type: :request, content_type: :json do
 
         it 'contains the current user in the response' do
           expect(subject.body)
-            .to be_json_eql(current_user.name.to_json)
+            .to be_json_eql(user.name.to_json)
             .at_path('_embedded/elements/0/name')
         end
       end
@@ -147,21 +152,25 @@ describe 'API v3 User resource', type: :request, content_type: :json do
       end
     end
 
+    context 'locked admin' do
+      let(:current_user) { locked_admin }
+
+      it_behaves_like 'unauthorized access'
+    end
+
     context 'other user' do
-      it 'should respond with 403' do
-        expect(subject.status).to eq(403)
-      end
+      it_behaves_like 'unauthorized access'
     end
   end
 
   describe '#get' do
-    context 'logged in user' do
-      let(:get_path) { api_v3_paths.user user.id }
-      before do
-        allow(User).to receive(:current).and_return current_user
-        get get_path
-      end
+    let(:get_path) { api_v3_paths.user user.id }
+
+    before do
+      get get_path
+    end
 
+    context 'logged in user' do
       it 'should respond with 200' do
         expect(subject.status).to eq(200)
       end
@@ -191,10 +200,6 @@ describe 'API v3 User resource', type: :request, content_type: :json do
 
     context 'get with login' do
       let(:get_path) { api_v3_paths.user user.login }
-      before do
-        allow(User).to receive(:current).and_return current_user
-        get get_path
-      end
 
       it 'should respond with 200' do
         expect(subject.status).to eq(200)
@@ -208,6 +213,12 @@ describe 'API v3 User resource', type: :request, content_type: :json do
     it_behaves_like 'handling anonymous user' do
       let(:path) { api_v3_paths.user user.id }
     end
+
+    context 'locked admin' do
+      let(:current_user) { locked_admin }
+
+      it_behaves_like 'unauthorized access'
+    end
   end
 
   describe '#delete' do
@@ -216,15 +227,13 @@ describe 'API v3 User resource', type: :request, content_type: :json do
     let(:self_delete) { true }
 
     before do
-      allow(User).to receive(:current).and_return current_user
-
       allow(Setting).to receive(:users_deletable_by_admins?).and_return(admin_delete)
       allow(Setting).to receive(:users_deletable_by_self?).and_return(self_delete)
 
       delete path
     end
 
-    shared_examples 'deletion through allowed user' do
+    shared_examples 'deletion allowed' do
       it 'should respond with 202' do
         expect(subject.status).to eq 202
       end
@@ -241,14 +250,6 @@ describe 'API v3 User resource', type: :request, content_type: :json do
           let(:type) { 'User' }
         end
       end
-
-      context 'with non-admin user' do
-        let(:current_user) { FactoryBot.create :user, admin: false }
-
-        it 'responds with 403' do
-          expect(subject.status).to eq 403
-        end
-      end
     end
 
     shared_examples 'deletion is not allowed' do
@@ -262,12 +263,12 @@ describe 'API v3 User resource', type: :request, content_type: :json do
     end
 
     context 'as admin' do
-      let(:current_user) { FactoryBot.create :admin }
+      let(:current_user) { admin }
 
       context 'with users deletable by admins' do
         let(:admin_delete) { true }
 
-        it_behaves_like 'deletion through allowed user'
+        it_behaves_like 'deletion allowed'
       end
 
       context 'with users not deletable by admins' do
@@ -277,6 +278,12 @@ describe 'API v3 User resource', type: :request, content_type: :json do
       end
     end
 
+    context 'as locked admin' do
+      let(:current_user) { locked_admin }
+
+      it_behaves_like 'deletion is not allowed'
+    end
+
     context 'as non-admin' do
       let(:current_user) { FactoryBot.create :user, admin: false }
 
@@ -289,7 +296,7 @@ describe 'API v3 User resource', type: :request, content_type: :json do
       context 'with self-deletion allowed' do
         let(:self_delete) { true }
 
-        it_behaves_like 'deletion through allowed user'
+        it_behaves_like 'deletion allowed'
       end
 
       context 'with self-deletion not allowed' do