diff options
53 files changed, 1509 insertions, 886 deletions
diff --git a/CHANGELOG b/CHANGELOG index 3e41526181e..d9c8d38991d 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -4,6 +4,7 @@ v 7.11.0 (unreleased) - Don't show duplicate deploy keys - Fix commit time being displayed in the wrong timezone in some cases (Hannes Rosenögger) - Make the first branch pushed to an empty repository the default HEAD (Stan Hu) + - Fix broken view when using a tag to display a tree that contains git submodules (Stan Hu) - Make Reply-To config apply to change e-mail confirmation and other Devise notifications (Stan Hu) - Add application setting to restrict user signups to e-mail domains (Stan Hu) - Don't allow a merge request to be merged when its title starts with "WIP". @@ -26,7 +27,7 @@ v 7.11.0 (unreleased) - When use change branches link at MR form - save source branch selection instead of target one - Improve handling of large diffs - Added GitLab Event header for project hooks - - + - Add Two-factor authentication (2FA) for GitLab logins - Show Atom feed buttons everywhere where applicable. - Add project activity atom feed. - Don't crash when an MR from a fork has a cross-reference comment from the target project on one of its commits. @@ -52,6 +53,7 @@ v 7.11.0 (unreleased) - Add current_sign_in_at to UserFull REST api. - Make Sidekiq MemoryKiller shutdown signal configurable - Add "Create Merge Request" buttons to commits and branches pages and push event. + - Show user roles by comments. - Fix automatic blocking of auto-created users from Active Directory. v 7.10.2 @@ -34,12 +34,17 @@ gem 'omniauth-bitbucket' gem 'doorkeeper', '2.1.3' gem "rack-oauth2", "~> 1.0.5" +# Two-factor authentication +gem 'devise-two-factor' +gem 'rqrcode-rails3' +gem 'attr_encrypted', '1.3.4' + # Browser detection gem "browser" # Extracting information from a git repository # Provide access to Gitlab::Git library -gem "gitlab_git", '~> 7.1.10' +gem "gitlab_git", '~> 7.1.11' # Ruby/Rack Git Smart-HTTP Server Handler gem 'gitlab-grack', '~> 2.0.2', require: 'grack' diff --git a/Gemfile.lock b/Gemfile.lock index 6f58c4f4fda..9940ab15242 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -46,6 +46,8 @@ GEM ast (2.0.0) astrolabe (1.3.0) parser (>= 2.2.0.pre.3, < 3.0) + attr_encrypted (1.3.4) + encryptor (>= 1.3.0) attr_required (1.0.0) autoprefixer-rails (5.1.11) execjs @@ -136,6 +138,13 @@ GEM warden (~> 1.2.3) devise-async (0.9.0) devise (~> 3.2) + devise-two-factor (1.0.1) + activemodel + activesupport + attr_encrypted (~> 1.3.2) + devise (~> 3.2.4) + rails + rotp (~> 1.6.1) diff-lcs (1.2.5) diffy (3.0.3) docile (1.1.5) @@ -147,6 +156,7 @@ GEM email_spec (1.5.0) launchy (~> 2.1) mail (~> 2.2) + encryptor (1.3.0) enumerize (0.7.0) activesupport (>= 3.2) equalizer (0.0.8) @@ -215,7 +225,7 @@ GEM mime-types (~> 1.19) gitlab_emoji (0.1.0) gemojione (~> 2.0) - gitlab_git (7.1.10) + gitlab_git (7.1.11) activesupport (~> 4.0) charlock_holmes (~> 0.6) gitlab-linguist (~> 3.0) @@ -482,7 +492,11 @@ GEM rest-client (1.6.7) mime-types (>= 1.16) rinku (1.7.3) + rotp (1.6.1) rouge (1.7.7) + rqrcode (0.4.2) + rqrcode-rails3 (0.1.7) + rqrcode (>= 0.4.2) rspec (2.99.0) rspec-core (~> 2.99.0) rspec-expectations (~> 2.99.0) @@ -670,6 +684,7 @@ DEPENDENCIES annotate (~> 2.6.0.beta2) asana (~> 0.0.6) asciidoctor (= 0.1.4) + attr_encrypted (= 1.3.4) awesome_print better_errors binding_of_caller @@ -691,6 +706,7 @@ DEPENDENCIES default_value_for (~> 3.0.0) devise (= 3.2.4) devise-async (= 0.9.0) + devise-two-factor diffy (~> 3.0.3) doorkeeper (= 2.1.3) dropzonejs-rails @@ -707,7 +723,7 @@ DEPENDENCIES gitlab-grack (~> 2.0.2) gitlab-linguist (~> 3.0.1) gitlab_emoji (~> 0.1) - gitlab_git (~> 7.1.10) + gitlab_git (~> 7.1.11) gitlab_meta (= 7.0) gitlab_omniauth-ldap (= 1.2.1) gollum-lib (~> 4.0.2) @@ -762,6 +778,7 @@ DEPENDENCIES redis-rails request_store rerun (~> 0.10.0) + rqrcode-rails3 rspec-rails (= 2.99) rubocop (= 0.28.0) rugments diff --git a/app/assets/stylesheets/pages/commits.scss b/app/assets/stylesheets/pages/commits.scss index 84361e15481..359f4073e87 100644 --- a/app/assets/stylesheets/pages/commits.scss +++ b/app/assets/stylesheets/pages/commits.scss @@ -29,10 +29,6 @@ .commits-feed-holder { float: right; - - .btn { - padding: 4px 12px; - } } li.commit { diff --git a/app/assets/stylesheets/pages/notes.scss b/app/assets/stylesheets/pages/notes.scss index 589a43c4264..61b907e39be 100644 --- a/app/assets/stylesheets/pages/notes.scss +++ b/app/assets/stylesheets/pages/notes.scss @@ -44,6 +44,14 @@ ul.notes { } .author-username { } + + .note-role { + float: right; + margin-top: 2px; + border: 1px solid #bbb; + background-color: transparent; + color: #999; + } } .discussion { @@ -136,6 +144,7 @@ ul.notes { .note { &.note:hover { .note-actions { display: block; } + .note-actions + .note-role { display: none; } } .discussion-header:hover { .discussion-actions { display: block; } @@ -153,6 +162,8 @@ ul.notes { } a { + margin-left: 5px; + @extend .cgray; &:hover { diff --git a/app/assets/stylesheets/pages/projects.scss b/app/assets/stylesheets/pages/projects.scss index 5a8d4665294..224aea2db59 100644 --- a/app/assets/stylesheets/pages/projects.scss +++ b/app/assets/stylesheets/pages/projects.scss @@ -249,7 +249,8 @@ ul.nav.nav-projects-tabs { } .breadcrumb.repo-breadcrumb { - padding: 2px 0; + padding: 0; + line-height: 34px; background: white; border: none; font-size: 16px; diff --git a/app/assets/stylesheets/pages/tree.scss b/app/assets/stylesheets/pages/tree.scss index 57f63b52aa1..34ee4d7b31e 100644 --- a/app/assets/stylesheets/pages/tree.scss +++ b/app/assets/stylesheets/pages/tree.scss @@ -106,17 +106,9 @@ } } -.tree-download-holder .btn { - padding: 4px 12px; -} - .tree-ref-holder { float: left; margin-right: 15px; - - .select2-container .select2-choice, .select2-container.select2-drop-above .select2-choice { - padding: 4px 12px; - } } .readme-holder { diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index eee10d6c22a..8ce881c7414 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -252,7 +252,7 @@ class ApplicationController < ActionController::Base end def configure_permitted_parameters - devise_parameter_sanitizer.sanitize(:sign_in) { |u| u.permit(:username, :email, :password, :login, :remember_me) } + devise_parameter_sanitizer.for(:sign_in) { |u| u.permit(:username, :email, :password, :login, :remember_me, :otp_attempt) } end def hexdigest(string) diff --git a/app/controllers/passwords_controller.rb b/app/controllers/passwords_controller.rb index dcbbe5baa4b..88459d4080a 100644 --- a/app/controllers/passwords_controller.rb +++ b/app/controllers/passwords_controller.rb @@ -15,4 +15,25 @@ class PasswordsController < Devise::PasswordsController respond_with(resource) end end + + # After a user resets their password, prompt for 2FA code if enabled instead + # of signing in automatically + # + # See http://git.io/vURrI + def update + super do |resource| + # TODO (rspeicher): In Devise master (> 3.4.1), we can set + # `Devise.sign_in_after_reset_password = false` and avoid this mess. + if resource.errors.empty? && resource.try(:otp_required_for_login?) + resource.unlock_access! if unlockable?(resource) + + # Since we are not signing this user in, we use the :updated_not_active + # message which only contains "Your password was changed successfully." + set_flash_message(:notice, :updated_not_active) if is_flashing_format? + + # Redirect to sign in so they can enter 2FA code + respond_with(resource, location: new_session_path(resource)) and return + end + end + end end diff --git a/app/controllers/profiles/two_factor_auths_controller.rb b/app/controllers/profiles/two_factor_auths_controller.rb new file mode 100644 index 00000000000..30ee6891733 --- /dev/null +++ b/app/controllers/profiles/two_factor_auths_controller.rb @@ -0,0 +1,49 @@ +class Profiles::TwoFactorAuthsController < Profiles::ApplicationController + def new + unless current_user.otp_secret + current_user.otp_secret = User.generate_otp_secret + current_user.save! + end + + @qr_code = build_qr_code + end + + def create + if current_user.valid_otp?(params[:pin_code]) + current_user.otp_required_for_login = true + @codes = current_user.generate_otp_backup_codes! + current_user.save! + + render 'create' + else + @error = 'Invalid pin code' + @qr_code = build_qr_code + render 'new' + end + end + + def codes + @codes = current_user.generate_otp_backup_codes! + current_user.save! + end + + def destroy + current_user.update_attributes({ + otp_required_for_login: false, + encrypted_otp_secret: nil, + encrypted_otp_secret_iv: nil, + encrypted_otp_secret_salt: nil, + otp_backup_codes: nil + }) + + redirect_to profile_account_path + end + + private + + def build_qr_code + issuer = "GitLab | #{current_user.email}" + uri = current_user.otp_provisioning_uri(current_user.email, issuer: issuer) + RQRCode::render_qrcode(uri, :svg, level: :m, unit: 3) + end +end diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 3f11d7afe6f..d4ff0d97561 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -1,4 +1,12 @@ class SessionsController < Devise::SessionsController + prepend_before_action :authenticate_with_two_factor, only: [:create] + + # This action comes from DeviseController, but because we call `sign_in` + # manually inside `authenticate_with_two_factor`, not skipping this action + # would cause a "You are already signed in." error message to be shown upon + # successful login. + skip_before_action :require_no_authentication, only: [:create] + def new redirect_path = if request.referer.present? && (params['redirect_to_referer'] == 'yes') @@ -14,7 +22,7 @@ class SessionsController < Devise::SessionsController # Prevent a 'you are already signed in' message directly after signing: # we should never redirect to '/users/sign_in' after signing in successfully. - unless redirect_path == '/users/sign_in' + unless redirect_path == new_user_session_path store_location_for(:redirect, redirect_path) end @@ -27,11 +35,54 @@ class SessionsController < Devise::SessionsController def create super do |resource| - # User has successfully signed in, so clear any unused reset tokens + # User has successfully signed in, so clear any unused reset token if resource.reset_password_token.present? resource.update_attributes(reset_password_token: nil, reset_password_sent_at: nil) end end end + + private + + def user_params + params.require(:user).permit(:login, :password, :remember_me, :otp_attempt) + end + + def find_user + if user_params[:login] + User.by_login(user_params[:login]) + elsif user_params[:otp_attempt] && session[:otp_user_id] + User.find(session[:otp_user_id]) + end + end + + def authenticate_with_two_factor + user = self.resource = find_user + + return unless user && user.otp_required_for_login + + if user_params[:otp_attempt].present? && session[:otp_user_id] + if valid_otp_attempt?(user) + # Remove any lingering user data from login + session.delete(:otp_user_id) + + sign_in(user) and return + else + flash.now[:alert] = 'Invalid two-factor code.' + render :two_factor and return + end + else + if user && user.valid_password?(user_params[:password]) + # Save the user's ID to session so we can ask for a one-time password + session[:otp_user_id] = user.id + render :two_factor and return + end + end + end + + def valid_otp_attempt?(user) + user.valid_otp?(user_params[:otp_attempt]) || + user.invalidate_otp_backup_code!(user_params[:otp_attempt]) + end end diff --git a/app/helpers/icons_helper.rb b/app/helpers/icons_helper.rb index a9030729b48..a730684f8f3 100644 --- a/app/helpers/icons_helper.rb +++ b/app/helpers/icons_helper.rb @@ -26,15 +26,15 @@ module IconsHelper end def public_icon - icon('globe') + icon('globe fw') end def internal_icon - icon('shield') + icon('shield fw') end def private_icon - icon('lock') + icon('lock fw') end def file_type_icon_class(type, mode, name) diff --git a/app/models/concerns/mentionable.rb b/app/models/concerns/mentionable.rb index 3ef3e8b67d8..a5957391bb7 100644 --- a/app/models/concerns/mentionable.rb +++ b/app/models/concerns/mentionable.rb @@ -39,7 +39,7 @@ module Mentionable # Determine whether or not a cross-reference Note has already been created between this Mentionable and # the specified target. def has_mentioned?(target) - Note.cross_reference_exists?(target, local_reference) + SystemNoteService.cross_reference_exists?(target, local_reference) end def mentioned_users(current_user = nil, p = project) diff --git a/app/models/note.rb b/app/models/note.rb index cbce6786683..6939a7e73a0 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -63,143 +63,9 @@ class Note < ActiveRecord::Base after_update :set_references class << self - def create_status_change_note(noteable, project, author, status, source) - body = "Status changed to #{status}#{' by ' + source.gfm_reference if source}" - - create( - noteable: noteable, - project: project, - author: author, - note: body, - system: true - ) - end - - # +noteable+ was referenced from +mentioner+, by including GFM in either - # +mentioner+'s description or an associated Note. - # Create a system Note associated with +noteable+ with a GFM back-reference - # to +mentioner+. - def create_cross_reference_note(noteable, mentioner, author) - gfm_reference = mentioner_gfm_ref(noteable, mentioner) - - note_options = { - project: noteable.project, - author: author, - note: cross_reference_note_content(gfm_reference), - system: true - } - - if noteable.kind_of?(Commit) - note_options.merge!(noteable_type: 'Commit', commit_id: noteable.id) - else - note_options.merge!(noteable: noteable) - end - - create(note_options) unless cross_reference_disallowed?(noteable, mentioner) - end - - def create_milestone_change_note(noteable, project, author, milestone) - body = if milestone.nil? - 'Milestone removed' - else - "Milestone changed to #{milestone.title}" - end - - create( - noteable: noteable, - project: project, - author: author, - note: body, - system: true - ) - end - - def create_assignee_change_note(noteable, project, author, assignee) - body = assignee.nil? ? 'Assignee removed' : "Reassigned to @#{assignee.username}" - - create({ - noteable: noteable, - project: project, - author: author, - note: body, - system: true - }) - end - - def create_labels_change_note(noteable, project, author, added_labels, removed_labels) - labels_count = added_labels.count + removed_labels.count - added_labels = added_labels.map{ |label| "~#{label.id}" }.join(' ') - removed_labels = removed_labels.map{ |label| "~#{label.id}" }.join(' ') - message = '' - - if added_labels.present? - message << "added #{added_labels}" - end - - if added_labels.present? && removed_labels.present? - message << ' and ' - end - - if removed_labels.present? - message << "removed #{removed_labels}" - end - - message << ' ' << 'label'.pluralize(labels_count) - body = "#{message.capitalize}" - - create( - noteable: noteable, - project: project, - author: author, - note: body, - system: true - ) - end - - def create_new_commits_note(merge_request, project, author, new_commits, existing_commits = [], oldrev = nil) - total_count = new_commits.length + existing_commits.length - commits_text = ActionController::Base.helpers.pluralize(total_count, 'commit') - body = "Added #{commits_text}:\n\n" - - if existing_commits.length > 0 - commit_ids = - if existing_commits.length == 1 - existing_commits.first.short_id - else - if oldrev - "#{Commit.truncate_sha(oldrev)}...#{existing_commits.last.short_id}" - else - "#{existing_commits.first.short_id}..#{existing_commits.last.short_id}" - end - end - - commits_text = ActionController::Base.helpers.pluralize(existing_commits.length, 'commit') - - branch = - if merge_request.for_fork? - "#{merge_request.target_project_namespace}:#{merge_request.target_branch}" - else - merge_request.target_branch - end - - message = "* #{commit_ids} - #{commits_text} from branch `#{branch}`" - body << message - body << "\n" - end - - new_commits.each do |commit| - message = "* #{commit.short_id} - #{commit.title}" - body << message - body << "\n" - end - - create( - noteable: merge_request, - project: project, - author: author, - note: body, - system: true - ) + # TODO (rspeicher): Update usages + def create_cross_reference_note(*args) + SystemNoteService.cross_reference(*args) end def discussions_from_notes(notes) @@ -227,88 +93,19 @@ class Note < ActiveRecord::Base [:discussion, type.try(:underscore), id, line_code].join("-").to_sym end - # Determine if cross reference note should be created. - # eg. mentioning a commit in MR comments which exists inside a MR - # should not create "mentioned in" note. - def cross_reference_disallowed?(noteable, mentioner) - if mentioner.kind_of?(MergeRequest) - mentioner.commits.map(&:id).include? noteable.id - end - end - - # Determine whether or not a cross-reference note already exists. - def cross_reference_exists?(noteable, mentioner) - gfm_reference = mentioner_gfm_ref(noteable, mentioner, true) - notes = if noteable.is_a?(Commit) - where(commit_id: noteable.id, noteable_type: 'Commit') - else - where(noteable_id: noteable.id, noteable_type: noteable.class) - end - - notes.where('note like ?', cross_reference_note_pattern(gfm_reference)). - system.any? - end - def search(query) where("note like :query", query: "%#{query}%") end + end - def cross_reference_note_prefix - 'mentioned in ' - end - - private - - def cross_reference_note_content(gfm_reference) - cross_reference_note_prefix + "#{gfm_reference}" - end - - def cross_reference_note_pattern(gfm_reference) - # Older cross reference notes contained underscores for emphasis - "%" + cross_reference_note_content(gfm_reference) + "%" - end - - # Prepend the mentioner's namespaced project path to the GFM reference for - # cross-project references. For same-project references, return the - # unmodified GFM reference. - def mentioner_gfm_ref(noteable, mentioner, cross_reference = false) - if mentioner.is_a?(Commit) && cross_reference - return mentioner.gfm_reference.sub('commit ', 'commit %') - end - - full_gfm_reference(mentioner.project, noteable.project, mentioner) - end - - # Return the +mentioner+ GFM reference. If the mentioner and noteable - # projects are not the same, add the mentioning project's path to the - # returned value. - def full_gfm_reference(mentioning_project, noteable_project, mentioner) - if mentioning_project == noteable_project - mentioner.gfm_reference - else - if mentioner.is_a?(Commit) - mentioner.gfm_reference.sub( - /(commit )/, - "\\1#{mentioning_project.path_with_namespace}@" - ) - else - mentioner.gfm_reference.sub( - /(issue |merge request )/, - "\\1#{mentioning_project.path_with_namespace}" - ) - end - end - end + def cross_reference? + system && SystemNoteService.cross_reference?(note) end def max_attachment_size current_application_settings.max_attachment_size.megabytes.to_i end - def cross_reference? - note.start_with?(self.class.cross_reference_note_prefix) - end - def find_diff return nil unless noteable && noteable.diffs.present? @@ -449,16 +246,6 @@ class Note < ActiveRecord::Base @discussion_id ||= Note.build_discussion_id(noteable_type, noteable_id || commit_id, line_code) end - # Returns true if this is a downvote note, - # otherwise false is returned - def downvote? - votable? && (note.start_with?('-1') || - note.start_with?(':-1:') || - note.start_with?(':thumbsdown:') || - note.start_with?(':thumbs_down_sign:') - ) - end - def for_commit? noteable_type == "Commit" end @@ -500,14 +287,18 @@ class Note < ActiveRecord::Base nil end - # Returns true if this is an upvote note, - # otherwise false is returned + DOWNVOTES = %w(-1 :-1: :thumbsdown: :thumbs_down_sign:) + + # Check if the note is a downvote + def downvote? + votable? && note.start_with?(*DOWNVOTES) + end + + UPVOTES = %w(+1 :+1: :thumbsup: :thumbs_up_sign:) + + # Check if the note is an upvote def upvote? - votable? && (note.start_with?('+1') || - note.start_with?(':+1:') || - note.start_with?(':thumbsup:') || - note.start_with?(':thumbs_up_sign:') - ) + votable? && note.start_with?(*UPVOTES) end def superceded?(notes) diff --git a/app/models/user.rb b/app/models/user.rb index a70cbaa518b..d088d2d8630 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -50,6 +50,11 @@ # bitbucket_access_token :string(255) # bitbucket_access_token_secret :string(255) # location :string(255) +# encrypted_otp_secret :string(255) +# encrypted_otp_secret_iv :string(255) +# encrypted_otp_secret_salt :string(255) +# otp_required_for_login :boolean +# otp_backup_codes :text # public_email :string(255) default(""), not null # @@ -70,8 +75,14 @@ class User < ActiveRecord::Base default_value_for :hide_no_password, false default_value_for :theme_id, gitlab_config.default_theme - devise :database_authenticatable, :lockable, :async, - :recoverable, :rememberable, :trackable, :validatable, :omniauthable, :confirmable, :registerable + devise :two_factor_authenticatable, + otp_secret_encryption_key: File.read(Rails.root.join('.secret')).chomp + + devise :two_factor_backupable, otp_number_of_backup_codes: 10 + serialize :otp_backup_codes, JSON + + devise :lockable, :async, :recoverable, :rememberable, :trackable, + :validatable, :omniauthable, :confirmable, :registerable attr_accessor :force_random_password diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 5e1906ad2ae..8960235b093 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -2,17 +2,17 @@ class IssuableBaseService < BaseService private def create_assignee_note(issuable) - Note.create_assignee_change_note( + SystemNoteService.change_assignee( issuable, issuable.project, current_user, issuable.assignee) end def create_milestone_note(issuable) - Note.create_milestone_change_note( + SystemNoteService.change_milestone( issuable, issuable.project, current_user, issuable.milestone) end def create_labels_note(issuable, added_labels, removed_labels) - Note.create_labels_change_note( + SystemNoteService.change_label( issuable, issuable.project, current_user, added_labels, removed_labels) end end diff --git a/app/services/issues/close_service.rb b/app/services/issues/close_service.rb index f670019cc63..138465859ce 100644 --- a/app/services/issues/close_service.rb +++ b/app/services/issues/close_service.rb @@ -14,7 +14,7 @@ module Issues private def create_note(issue, current_commit) - Note.create_status_change_note(issue, issue.project, current_user, issue.state, current_commit) + SystemNoteService.change_status(issue, issue.project, current_user, issue.state, current_commit) end end end diff --git a/app/services/issues/reopen_service.rb b/app/services/issues/reopen_service.rb index 1e5c398516d..e48ca359f4f 100644 --- a/app/services/issues/reopen_service.rb +++ b/app/services/issues/reopen_service.rb @@ -14,7 +14,7 @@ module Issues private def create_note(issue) - Note.create_status_change_note(issue, issue.project, current_user, issue.state, nil) + SystemNoteService.change_status(issue, issue.project, current_user, issue.state, nil) end end end diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb index f6e1ae6f283..e455fe95791 100644 --- a/app/services/merge_requests/base_service.rb +++ b/app/services/merge_requests/base_service.rb @@ -2,7 +2,7 @@ module MergeRequests class BaseService < ::IssuableBaseService def create_note(merge_request) - Note.create_status_change_note(merge_request, merge_request.target_project, current_user, merge_request.state, nil) + SystemNoteService.change_status(merge_request, merge_request.target_project, current_user, merge_request.state, nil) end def hook_data(merge_request, action) diff --git a/app/services/merge_requests/refresh_service.rb b/app/services/merge_requests/refresh_service.rb index e9b526d1fb7..66610a08a44 100644 --- a/app/services/merge_requests/refresh_service.rb +++ b/app/services/merge_requests/refresh_service.rb @@ -82,8 +82,9 @@ module MergeRequests mr_commit_ids.include?(commit.id) end - Note.create_new_commits_note(merge_request, merge_request.project, - @current_user, new_commits, existing_commits, @oldrev) + SystemNoteService.add_commits(merge_request, merge_request.project, + @current_user, new_commits, + existing_commits, @oldrev) end end diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb new file mode 100644 index 00000000000..0614f8689a4 --- /dev/null +++ b/app/services/system_note_service.rb @@ -0,0 +1,303 @@ +# SystemNoteService +# +# Used for creating system notes (e.g., when a user references a merge request +# from an issue, an issue's assignee changes, an issue is closed, etc.) +class SystemNoteService + # Called when commits are added to a Merge Request + # + # noteable - Noteable object + # project - Project owning noteable + # author - User performing the change + # new_commits - Array of Commits added since last push + # existing_commits - Array of Commits added in a previous push + # oldrev - TODO (rspeicher): I have no idea what this actually does + # + # See new_commit_summary and existing_commit_summary. + # + # Returns the created Note object + def self.add_commits(noteable, project, author, new_commits, existing_commits = [], oldrev = nil) + total_count = new_commits.length + existing_commits.length + commits_text = "#{total_count} commit".pluralize(total_count) + + body = "Added #{commits_text}:\n\n" + body << existing_commit_summary(noteable, existing_commits, oldrev) + body << new_commit_summary(new_commits).join("\n") + + create_note(noteable: noteable, project: project, author: author, note: body) + end + + # Called when the assignee of a Noteable is changed or removed + # + # noteable - Noteable object + # project - Project owning noteable + # author - User performing the change + # assignee - User being assigned, or nil + # + # Example Note text: + # + # "Assignee removed" + # + # "Reassigned to @rspeicher" + # + # Returns the created Note object + def self.change_assignee(noteable, project, author, assignee) + body = assignee.nil? ? 'Assignee removed' : "Reassigned to @#{assignee.username}" + + create_note(noteable: noteable, project: project, author: author, note: body) + end + + # Called when one or more labels on a Noteable are added and/or removed + # + # noteable - Noteable object + # project - Project owning noteable + # author - User performing the change + # added_labels - Array of Labels added + # removed_labels - Array of Labels removed + # + # Example Note text: + # + # "Added ~1 and removed ~2 ~3 labels" + # + # "Added ~4 label" + # + # "Removed ~5 label" + # + # Returns the created Note object + def self.change_label(noteable, project, author, added_labels, removed_labels) + labels_count = added_labels.count + removed_labels.count + + references = ->(label) { "~#{label.id}" } + added_labels = added_labels.map(&references).join(' ') + removed_labels = removed_labels.map(&references).join(' ') + + body = '' + + if added_labels.present? + body << "added #{added_labels}" + body << ' and ' if removed_labels.present? + end + + if removed_labels.present? + body << "removed #{removed_labels}" + end + + body << ' ' << 'label'.pluralize(labels_count) + body = "#{body.capitalize}" + + create_note(noteable: noteable, project: project, author: author, note: body) + end + + # Called when the milestone of a Noteable is changed + # + # noteable - Noteable object + # project - Project owning noteable + # author - User performing the change + # milestone - Milestone being assigned, or nil + # + # Example Note text: + # + # "Milestone removed" + # + # "Miletone changed to 7.11" + # + # Returns the created Note object + def self.change_milestone(noteable, project, author, milestone) + body = 'Milestone ' + body += milestone.nil? ? 'removed' : "changed to #{milestone.title}" + + create_note(noteable: noteable, project: project, author: author, note: body) + end + + # Called when the status of a Noteable is changed + # + # noteable - Noteable object + # project - Project owning noteable + # author - User performing the change + # status - String status + # source - Mentionable performing the change, or nil + # + # Example Note text: + # + # "Status changed to merged" + # + # "Status changed to closed by bc17db76" + # + # Returns the created Note object + def self.change_status(noteable, project, author, status, source) + body = "Status changed to #{status}" + body += " by #{source.gfm_reference}" if source + + create_note(noteable: noteable, project: project, author: author, note: body) + end + + # Called when a Mentionable references a Noteable + # + # noteable - Noteable object being referenced + # mentioner - Mentionable object + # author - User performing the reference + # + # Example Note text: + # + # "Mentioned in #1" + # + # "Mentioned in !2" + # + # "Mentioned in 54f7727c" + # + # See cross_reference_note_content. + # + # Returns the created Note object + def self.cross_reference(noteable, mentioner, author) + return if cross_reference_disallowed?(noteable, mentioner) + + gfm_reference = mentioner_gfm_ref(noteable, mentioner) + + note_options = { + project: noteable.project, + author: author, + note: cross_reference_note_content(gfm_reference) + } + + if noteable.kind_of?(Commit) + note_options.merge!(noteable_type: 'Commit', commit_id: noteable.id) + else + note_options.merge!(noteable: noteable) + end + + create_note(note_options) + end + + def self.cross_reference?(note_text) + note_text.start_with?(cross_reference_note_prefix) + end + + # Check if a cross-reference is disallowed + # + # This method prevents adding a "mentioned in !1" note on every single commit + # in a merge request. + # + # noteable - Noteable object being referenced + # mentioner - Mentionable object + # + # Returns Boolean + def self.cross_reference_disallowed?(noteable, mentioner) + return false unless MergeRequest === mentioner + return false unless Commit === noteable + + mentioner.commits.include?(noteable) + end + + def self.cross_reference_exists?(noteable, mentioner) + # Initial scope should be system notes of this noteable type + notes = Note.system.where(noteable_type: noteable.class) + + if noteable.is_a?(Commit) + # Commits have non-integer IDs, so they're stored in `commit_id` + notes = notes.where(commit_id: noteable.id) + else + notes = notes.where(noteable_id: noteable.id) + end + + gfm_reference = mentioner_gfm_ref(noteable, mentioner, true) + notes = notes.where(note: cross_reference_note_content(gfm_reference)) + + notes.count > 0 + end + + private + + def self.create_note(args = {}) + Note.create(args.merge(system: true)) + end + + # Prepend the mentioner's namespaced project path to the GFM reference for + # cross-project references. For same-project references, return the + # unmodified GFM reference. + def self.mentioner_gfm_ref(noteable, mentioner, cross_reference = false) + # FIXME (rspeicher): This was breaking things. + # if mentioner.is_a?(Commit) && cross_reference + # return mentioner.gfm_reference.sub('commit ', 'commit %') + # end + + full_gfm_reference(mentioner.project, noteable.project, mentioner) + end + + # Return the +mentioner+ GFM reference. If the mentioner and noteable + # projects are not the same, add the mentioning project's path to the + # returned value. + def self.full_gfm_reference(mentioning_project, noteable_project, mentioner) + if mentioning_project == noteable_project + mentioner.gfm_reference + else + if mentioner.is_a?(Commit) + mentioner.gfm_reference.sub( + /(commit )/, + "\\1#{mentioning_project.path_with_namespace}@" + ) + else + mentioner.gfm_reference.sub( + /(issue |merge request )/, + "\\1#{mentioning_project.path_with_namespace}" + ) + end + end + end + + def self.cross_reference_note_prefix + 'mentioned in ' + end + + def self.cross_reference_note_content(gfm_reference) + "#{cross_reference_note_prefix}#{gfm_reference}" + end + + # Build an Array of lines detailing each commit added in a merge request + # + # new_commits - Array of new Commit objects + # + # Returns an Array of Strings + def self.new_commit_summary(new_commits) + new_commits.collect do |commit| + "* #{commit.short_id} - #{commit.title}" + end + end + + # Build a single line summarizing existing commits being added in a merge + # request + # + # noteable - MergeRequest object + # existing_commits - Array of existing Commit objects + # oldrev - Optional String SHA of ... TODO (rspeicher): I have no idea what this actually does. + # + # Examples: + # + # "* ea0f8418...2f4426b7 - 24 commits from branch `master`" + # + # "* ea0f8418..4188f0ea - 15 commits from branch `fork:master`" + # + # "* ea0f8418 - 1 commit from branch `feature`" + # + # Returns a newline-terminated String + def self.existing_commit_summary(noteable, existing_commits, oldrev = nil) + return '' if existing_commits.empty? + + count = existing_commits.size + + commit_ids = if count == 1 + existing_commits.first.short_id + else + if oldrev + "#{Commit.truncate_sha(oldrev)}...#{existing_commits.last.short_id}" + else + "#{existing_commits.first.short_id}..#{existing_commits.last.short_id}" + end + end + + commits_text = "#{count} commit".pluralize(count) + + branch = noteable.target_branch + branch = "#{noteable.target_project_namespace}:#{branch}" if noteable.for_fork? + + "* #{commit_ids} - #{commits_text} from branch `#{branch}`\n" + end +end diff --git a/app/views/devise/sessions/two_factor.html.haml b/app/views/devise/sessions/two_factor.html.haml new file mode 100644 index 00000000000..22b2c1a186b --- /dev/null +++ b/app/views/devise/sessions/two_factor.html.haml @@ -0,0 +1,10 @@ +%div + .login-box + .login-heading + %h3 Two-factor Authentication + .login-body + = form_for(resource, as: resource_name, url: session_path(resource_name), method: :post) do |f| + = f.text_field :otp_attempt, class: 'form-control', placeholder: 'Two-factor authentication code', required: true, autofocus: true + %p.help-block.hint If you've lost your phone, you may enter one of your recovery codes. + .prepend-top-20 + = f.submit "Verify code", class: "btn btn-save" diff --git a/app/views/layouts/nav/_profile.html.haml b/app/views/layouts/nav/_profile.html.haml index 31d8ed3ed86..ac37fd4c1c1 100644 --- a/app/views/layouts/nav/_profile.html.haml +++ b/app/views/layouts/nav/_profile.html.haml @@ -4,7 +4,7 @@ = icon('user fw') %span Profile - = nav_link(controller: :accounts) do + = nav_link(controller: [:accounts, :two_factor_auths]) do = link_to profile_account_path, title: 'Account', data: {placement: 'right'} do = icon('gear fw') %span diff --git a/app/views/profiles/accounts/show.html.haml b/app/views/profiles/accounts/show.html.haml index 1c3a3d68aca..6ac60b01f85 100644 --- a/app/views/profiles/accounts/show.html.haml +++ b/app/views/profiles/accounts/show.html.haml @@ -26,6 +26,33 @@ %span You don`t have one yet. Click generate to fix it. = f.submit 'Generate', class: "btn success btn-build-token" + - unless current_user.ldap_user? + %fieldset + - if current_user.otp_required_for_login + %legend.text-success + = icon('check') + Two-factor Authentication enabled + %div + .pull-right + = link_to 'Disable Two-factor Authentication', profile_two_factor_auth_path, method: :delete, class: 'btn btn-close btn-sm', + data: { confirm: 'Are you sure?' } + %p + If you lose your recovery codes you can + %strong + = succeed ',' do + = link_to 'generate new ones', codes_profile_two_factor_auth_path, method: :post, data: { confirm: 'Are you sure?' } + invalidating all previous codes. + + - else + %legend Two-factor Authentication + %div + %p + Increase your account's security by enabling two-factor authentication (2FA). + %p + Each time you log in you’ll be required to provide your username and + password as usual, plus a randomly-generated code from your phone. + %div + = link_to 'Enable Two-factor Authentication', new_profile_two_factor_auth_path, class: 'btn btn-success' - if show_profile_social_tab? %fieldset @@ -38,7 +65,7 @@ class: "btn btn-lg #{'active' if oauth_active?(provider)}" - if oauth_active?(provider) = link_to unlink_profile_account_path(provider: provider), method: :delete, class: 'btn btn-lg' do - %i.fa.fa-close + = icon('close') - if show_profile_username_tab? %fieldset.update-username @@ -52,7 +79,7 @@ .loading-gif.hide %p - %i.fa.fa-spinner.fa-spin + = icon('spinner spin') Saving new username %p.light = user_url(@user) diff --git a/app/views/profiles/two_factor_auths/_codes.html.haml b/app/views/profiles/two_factor_auths/_codes.html.haml new file mode 100644 index 00000000000..1b1395eaa17 --- /dev/null +++ b/app/views/profiles/two_factor_auths/_codes.html.haml @@ -0,0 +1,11 @@ +%p.slead + Should you ever lose your phone, each of these recovery codes can be used one + time each to regain access to your account. Please save them in a safe place. + +.codes.well + %ul + - @codes.each do |code| + %li + %span.monospace= code + += link_to 'Proceed', profile_account_path, class: 'btn btn-success' diff --git a/app/views/profiles/two_factor_auths/codes.html.haml b/app/views/profiles/two_factor_auths/codes.html.haml new file mode 100644 index 00000000000..addf356697a --- /dev/null +++ b/app/views/profiles/two_factor_auths/codes.html.haml @@ -0,0 +1,5 @@ +- page_title 'Recovery Codes', 'Two-factor Authentication' + +%h3.page-title Two-factor Authentication Recovery codes +%hr += render 'codes' diff --git a/app/views/profiles/two_factor_auths/create.html.haml b/app/views/profiles/two_factor_auths/create.html.haml new file mode 100644 index 00000000000..e330aadac13 --- /dev/null +++ b/app/views/profiles/two_factor_auths/create.html.haml @@ -0,0 +1,6 @@ +- page_title 'Two-factor Authentication', 'Account' + +.alert.alert-success + Congratulations! You have enabled Two-factor Authentication! + += render 'codes' diff --git a/app/views/profiles/two_factor_auths/new.html.haml b/app/views/profiles/two_factor_auths/new.html.haml new file mode 100644 index 00000000000..fe03a259a12 --- /dev/null +++ b/app/views/profiles/two_factor_auths/new.html.haml @@ -0,0 +1,23 @@ +- page_title 'Two-factor Authentication', 'Account' + +%h2.page-title Two-Factor Authentication (2FA) +%p + Download the Google Authenticator application from App Store for iOS or + Google Play for Android and scan this code. + +%hr + += form_tag profile_two_factor_auth_path, method: :post, class: 'form-horizontal' do |f| + - if @error + .alert.alert-danger + = @error + .form-group + .col-sm-2 + .col-sm-10 + = raw @qr_code + .form-group + = label_tag :pin_code, nil, class: "control-label" + .col-sm-10 + = text_field_tag :pin_code, nil, class: "form-control", required: true, autofocus: true + .form-actions + = submit_tag 'Submit', class: 'btn btn-success' diff --git a/app/views/projects/_aside.html.haml b/app/views/projects/_aside.html.haml index 1241b51f9ac..c2f56996ba8 100644 --- a/app/views/projects/_aside.html.haml +++ b/app/views/projects/_aside.html.haml @@ -1,85 +1,102 @@ .clearfix - .append-bottom-20 - = render "shared/clone_panel" - - unless @project.empty_repo? .well %h4.visibility-level-label = visibility_level_icon(@project.visibility_level) = "#{visibility_level_label(@project.visibility_level).capitalize} project" - %ul.nav.nav-pills - %li= link_to pluralize(number_with_delimiter(@repository.commit_count), 'commit'), namespace_project_commits_path(@project.namespace, @project, @ref || @repository.root_ref) - %li= link_to pluralize(number_with_delimiter(@repository.branch_names.count), 'branch'), namespace_project_branches_path(@project.namespace, @project) - %li= link_to pluralize(number_with_delimiter(@repository.tag_names.count), 'tag'), namespace_project_tags_path(@project.namespace, @project) - .actions - = link_to namespace_project_compare_index_path(@project.namespace, @project, from: @repository.root_ref, to: @ref || @repository.root_ref), class: 'btn btn-sm' do - %i.fa.fa-exchange - Compare code + - if @repository.changelog || @repository.license || @repository.contribution_guide + %ul.nav.nav-pills + - if @repository.changelog + %li.hidden-xs + = link_to changelog_url(@project) do + = icon("list-alt fw") + Changelog + - if @repository.license + %li + = link_to license_url(@project) do + = icon("check-circle-o fw") + License + - if @repository.contribution_guide + %li + = link_to contribution_guide_url(@project) do + = icon("info-circle fw") + Contribution guide - - if can?(current_user, :download_code, @project) - - = render 'projects/repositories/download_archive', split_button: true, btn_class: 'btn-group-sm' - - - unless @project.empty_repo? - .well - %h4 Contribute - %ul.nav.nav-pills - - if @repository.changelog - %li.hidden-xs - = link_to changelog_url(@project) do - Changelog - - if @repository.contribution_guide - %li.hidden-xs - = link_to contribution_guide_url(@project) do - Contribution guide - - if @repository.license - %li - = link_to license_url(@project) do - License .actions - = link_to url_for_new_issue(@project, only_path: true), title: "New Issue", class: 'btn btn-sm' do - %i.fa.fa-fw.fa-exclamation-circle - New issue + - if can? current_user, :write_issue, @project + = link_to url_for_new_issue(@project, only_path: true), title: "New Issue", class: 'btn btn-sm append-right-10' do + = icon("exclamation-circle fw") + New Issue + - if can? current_user, :write_merge_request, @project - = link_to new_namespace_project_merge_request_path(@project.namespace, @project), class: "btn btn-sm", title: "New Merge Request" do - %i.fa.fa-plus + = icon("plus fw") New Merge Request - - - - if @project.archived? - .alert.alert-warning - %h4 - %i.fa.fa-exclamation-triangle - Archived project! - %p Repository is read-only - - - if @project.forked_from_project + - if forked_from_project = @project.forked_from_project .well %h4 + = icon("code-fork fw") Forked from .pull-right - = link_to @project.forked_from_project.namespace.try(:name), project_path(@project.forked_from_project) + = link_to forked_from_project.namespace.try(:name), project_path(forked_from_project) + - if version = @repository.version + .well + %h4 + = icon("clock-o fw") + Version + .pull-right + = link_to version_url(@project) do + = @repository.blob_by_oid(version.id).data -- if version = @repository.version - .well - %h4 - Version - .pull-right - = link_to version_url(@project) do - = @repository.blob_by_oid(version.id).data + - @project.ci_services.each do |ci_service| + - if ci_service.active? && ci_service.respond_to?(:builds_path) + .well + %h4 + = icon("check fw") + = ci_service.title + .pull-right + - if ci_service.respond_to?(:status_img_path) + = link_to ci_service.builds_path, :'data-no-turbolink' => 'data-no-turbolink' do + = image_tag ci_service.status_img_path, alt: "build status" + - else + = link_to 'view builds', ci_service.builds_path, :'data-no-turbolink' => 'data-no-turbolink' -- @project.ci_services.each do |ci_service| - - if ci_service.active? && ci_service.respond_to?(:builds_path) + - unless @project.empty_repo? .well %h4 - = ci_service.title - .pull-right - - if ci_service.respond_to?(:status_img_path) - = link_to ci_service.builds_path, :'data-no-turbolink' => 'data-no-turbolink' do - = image_tag ci_service.status_img_path, alt: "build status" - - else - = link_to 'view builds', ci_service.builds_path, :'data-no-turbolink' => 'data-no-turbolink' + = icon("archive fw") + Repository + + %ul.nav.nav-pills + %li + = link_to namespace_project_commits_path(@project.namespace, @project, @ref || @repository.root_ref) do + = icon("history fw") + = pluralize(number_with_delimiter(@repository.commit_count), 'commit') + %li + = link_to namespace_project_branches_path(@project.namespace, @project) do + = icon("code-fork fw") + = pluralize(number_with_delimiter(@repository.branch_names.count), 'branch') + %li + = link_to namespace_project_tags_path(@project.namespace, @project) do + = icon("tags fw") + = pluralize(number_with_delimiter(@repository.tag_names.count), 'tag') + + .actions + = link_to namespace_project_compare_index_path(@project.namespace, @project, from: @repository.root_ref, to: @ref || @repository.root_ref), class: 'btn btn-sm append-right-10' do + %i.fa.fa-exchange + Compare code + + - if can?(current_user, :download_code, @project) + = render 'projects/repositories/download_archive', split_button: true, btn_class: 'btn-group-sm' + + = render "shared/clone_panel" + + - if @project.archived? + .alert.alert-warning + %h4 + = icon("exclamation-triangle fw") + Archived project! + %p Repository is read-only diff --git a/app/views/projects/_section.html.haml b/app/views/projects/_section.html.haml index 0b7f4cb780a..f4f876f3809 100644 --- a/app/views/projects/_section.html.haml +++ b/app/views/projects/_section.html.haml @@ -1,10 +1,12 @@ %ul.nav.nav-tabs %li.active = link_to '#tab-activity', 'data-toggle' => 'tab' do + = icon("tachometer") Activity - if @repository.readme %li = link_to '#tab-readme', 'data-toggle' => 'tab' do + = icon("file-text-o") Readme .tab-content .tab-pane.active#tab-activity diff --git a/app/views/projects/commits/_head.html.haml b/app/views/projects/commits/_head.html.haml index 66101f3f0da..66261c7336d 100644 --- a/app/views/projects/commits/_head.html.haml +++ b/app/views/projects/commits/_head.html.haml @@ -1,17 +1,22 @@ %ul.nav.nav-tabs = nav_link(controller: [:commit, :commits]) do = link_to namespace_project_commits_path(@project.namespace, @project, @ref || @repository.root_ref) do + = icon("history") Commits %span.badge= number_with_precision(@repository.commit_count, precision: 0, delimiter: ',') = nav_link(controller: :compare) do - = link_to 'Compare', namespace_project_compare_index_path(@project.namespace, @project, from: @repository.root_ref, to: @ref || @repository.root_ref) + = link_to namespace_project_compare_index_path(@project.namespace, @project, from: @repository.root_ref, to: @ref || @repository.root_ref) do + = icon("exchange") + Compare = nav_link(html_options: {class: branches_tab_class}) do = link_to namespace_project_branches_path(@project.namespace, @project) do + = icon("code-fork") Branches %span.badge.js-totalbranch-count= @repository.branches.size = nav_link(controller: :tags) do = link_to namespace_project_tags_path(@project.namespace, @project) do + = icon("tags") Tags %span.badge.js-totaltags-count= @repository.tags.length diff --git a/app/views/projects/notes/_note.html.haml b/app/views/projects/notes/_note.html.haml index 4d26b52df01..2f3c407d6e9 100644 --- a/app/views/projects/notes/_note.html.haml +++ b/app/views/projects/notes/_note.html.haml @@ -9,22 +9,30 @@ .timeline-content .note-header .note-actions - = link_to "##{dom_id(note)}", name: dom_id(note) do - = icon('link') + = link_to "##{dom_id(note)}", name: dom_id(note), title: "Link here" do + = icon('link fw') Link here - + - if note_editable?(note) = link_to '#', title: 'Edit comment', class: 'js-note-edit' do - = icon('pencil-square-o') + = icon('pencil-square-o fw') Edit - - = link_to namespace_project_note_path(@project.namespace, @project, note), title: 'Remove comment', method: :delete, data: { confirm: 'Are you sure you want to remove this comment?' }, remote: true, class: 'danger js-note-delete' do - = icon('trash-o', class: 'cred') + + = link_to namespace_project_note_path(note.project.namespace, note.project, note), title: 'Remove comment', method: :delete, data: { confirm: 'Are you sure you want to remove this comment?' }, remote: true, class: 'danger js-note-delete' do + = icon('trash-o fw', class: 'cred') Remove + + - unless note.system + - member = note.project.team.find_member(note.author.id) + - if member + %span.note-role.label + = member.human_access + - if note.system = link_to user_path(note.author) do = image_tag avatar_icon(note.author_email), class: 'avatar s16', alt: '' - = link_to_member(@project, note.author, avatar: false) + + = link_to_member(note.project, note.author, avatar: false) %span.author-username = '@' + note.author.username %span.note-last-update @@ -65,7 +73,7 @@ = link_to note.attachment.url, target: '_blank' do = icon('paperclip') = note.attachment_identifier - = link_to delete_attachment_namespace_project_note_path(@project.namespace, @project, note), + = link_to delete_attachment_namespace_project_note_path(note.project.namespace, note.project, note), title: 'Delete this attachment', method: :delete, remote: true, data: { confirm: 'Are you sure you want to remove the attachment?' }, class: 'danger js-note-attachment-delete' do = icon('trash-o', class: 'cred') .clear diff --git a/app/views/projects/tree/show.html.haml b/app/views/projects/tree/show.html.haml index 72916cad182..04590f65b27 100644 --- a/app/views/projects/tree/show.html.haml +++ b/app/views/projects/tree/show.html.haml @@ -8,7 +8,7 @@ - if can? current_user, :download_code, @project .tree-download-holder - = render 'projects/repositories/download_archive', ref: @ref, btn_class: 'btn-group-sm pull-right hidden-xs hidden-sm', split_button: true + = render 'projects/repositories/download_archive', ref: @ref, btn_class: 'btn-group pull-right hidden-xs hidden-sm', split_button: true #tree-holder.tree-holder.clearfix = render "tree", tree: @tree diff --git a/config/application.rb b/config/application.rb index fa399533e52..7e899cc3b5b 100644 --- a/config/application.rb +++ b/config/application.rb @@ -31,7 +31,7 @@ module Gitlab config.encoding = "utf-8" # Configure sensitive parameters which will be filtered from the log file. - config.filter_parameters.push(:password, :password_confirmation, :private_token) + config.filter_parameters.push(:password, :password_confirmation, :private_token, :otp_attempt) # Enable escaping HTML in JSON. config.active_support.escape_html_entities_in_json = true diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index 8f8c4169740..091548348b1 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -1,6 +1,11 @@ # Use this hook to configure devise mailer, warden hooks and so forth. The first # four configuration values can also be set straight in your models. Devise.setup do |config| + config.warden do |manager| + manager.default_strategies(scope: :user).unshift :two_factor_authenticatable + manager.default_strategies(scope: :user).unshift :two_factor_backupable + end + # ==> Mailer Configuration # Configure the class responsible to send e-mails. config.mailer = "DeviseMailer" diff --git a/config/routes.rb b/config/routes.rb index 4b38dede7b4..bf2cb6421c5 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -226,6 +226,11 @@ Gitlab::Application.routes.draw do resources :keys resources :emails, only: [:index, :create, :destroy] resource :avatar, only: [:destroy] + resource :two_factor_auth, only: [:new, :create, :destroy] do + member do + post :codes + end + end end end diff --git a/db/migrate/20150327223628_add_devise_two_factor_to_users.rb b/db/migrate/20150327223628_add_devise_two_factor_to_users.rb new file mode 100644 index 00000000000..11b026ee8f3 --- /dev/null +++ b/db/migrate/20150327223628_add_devise_two_factor_to_users.rb @@ -0,0 +1,8 @@ +class AddDeviseTwoFactorToUsers < ActiveRecord::Migration + def change + add_column :users, :encrypted_otp_secret, :string + add_column :users, :encrypted_otp_secret_iv, :string + add_column :users, :encrypted_otp_secret_salt, :string + add_column :users, :otp_required_for_login, :boolean + end +end diff --git a/db/migrate/20150331183602_add_devise_two_factor_backupable_to_users.rb b/db/migrate/20150331183602_add_devise_two_factor_backupable_to_users.rb new file mode 100644 index 00000000000..913958db7c5 --- /dev/null +++ b/db/migrate/20150331183602_add_devise_two_factor_backupable_to_users.rb @@ -0,0 +1,5 @@ +class AddDeviseTwoFactorBackupableToUsers < ActiveRecord::Migration + def change + add_column :users, :otp_backup_codes, :text + end +end diff --git a/db/migrate/20150423033240_add_default_project_visibililty_to_application_settings.rb b/db/migrate/20150423033240_add_default_project_visibililty_to_application_settings.rb index 9b0f13f3fa7..50a9b2439e0 100644 --- a/db/migrate/20150423033240_add_default_project_visibililty_to_application_settings.rb +++ b/db/migrate/20150423033240_add_default_project_visibililty_to_application_settings.rb @@ -1,7 +1,11 @@ class AddDefaultProjectVisibililtyToApplicationSettings < ActiveRecord::Migration - def change + def up add_column :application_settings, :default_project_visibility, :integer visibility = Settings.gitlab.default_projects_features['visibility_level'] execute("update application_settings set default_project_visibility = #{visibility}") end + + def down + remove_column :application_settings, :default_project_visibility + end end diff --git a/db/migrate/20150429002313_remove_abandoned_group_members_records.rb b/db/migrate/20150429002313_remove_abandoned_group_members_records.rb index 6013605bb35..244637e1c4a 100644 --- a/db/migrate/20150429002313_remove_abandoned_group_members_records.rb +++ b/db/migrate/20150429002313_remove_abandoned_group_members_records.rb @@ -1,6 +1,9 @@ class RemoveAbandonedGroupMembersRecords < ActiveRecord::Migration - def change + def up execute("DELETE FROM members WHERE type = 'GroupMember' AND source_id NOT IN(\ SELECT id FROM namespaces WHERE type='Group')") end + + def down + end end diff --git a/db/migrate/20150509180749_convert_legacy_reference_notes.rb b/db/migrate/20150509180749_convert_legacy_reference_notes.rb new file mode 100644 index 00000000000..b02605489be --- /dev/null +++ b/db/migrate/20150509180749_convert_legacy_reference_notes.rb @@ -0,0 +1,16 @@ +# Convert legacy Markdown-emphasized notes to the current, non-emphasized format +# +# _mentioned in 54f7727c850972f0401c1312a7c4a6a380de5666_ +# +# becomes +# +# mentioned in 54f7727c850972f0401c1312a7c4a6a380de5666 +class ConvertLegacyReferenceNotes < ActiveRecord::Migration + def up + execute %q{UPDATE notes SET note = trim(both '_' from note) WHERE system = true AND note LIKE '\_%\_'} + end + + def down + # noop + end +end diff --git a/db/schema.rb b/db/schema.rb index 04abf9bb9a6..c89d22d241c 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20150502064022) do +ActiveRecord::Schema.define(version: 20150509180749) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -493,6 +493,11 @@ ActiveRecord::Schema.define(version: 20150502064022) do t.string "bitbucket_access_token_secret" t.string "location" t.string "public_email", default: "", null: false + t.string "encrypted_otp_secret" + t.string "encrypted_otp_secret_iv" + t.string "encrypted_otp_secret_salt" + t.boolean "otp_required_for_login" + t.text "otp_backup_codes" end add_index "users", ["admin"], name: "index_users_on_admin", using: :btree diff --git a/doc/development/migration_style_guide.md b/doc/development/migration_style_guide.md index a102d7987de..4fa1961fde9 100644 --- a/doc/development/migration_style_guide.md +++ b/doc/development/migration_style_guide.md @@ -39,4 +39,50 @@ Your migration should be reversible. This is very important, as it should be possible to downgrade in case of a vulnerability or bugs. In your migration, add a comment describing how the reversibility of the -migration was tested.
\ No newline at end of file +migration was tested. + + +## Removing indices + +If you need to remove index, please add a condition like in following example: + +``` +remove_index :namespaces, column: :name if index_exists?(:namespaces, :name) +``` + +## Adding indices + +If you need to add an unique index please keep in mind there is possibility of existing duplicates. If it is possible write a separate migration for handling this situation. It can be just removing or removing with overwriting all references to these duplicates depend on situation. + +## Testing + +Make sure that your migration works with MySQL and PostgreSQL with data. An empty database does not guarantee that your migration is correct. + +Make sure your migration can be reversed. + +## Data migration + +Please prefer Arel and plain SQL over usual ActiveRecord syntax. In case of using plain SQL you need to quote all input manually with `quote_string` helper. + +Example with Arel: + +``` +users = Arel::Table.new(:users) +users.group(users[:user_id]).having(users[:id].count.gt(5)) + +#updtae other tables with this results +``` + +Example with plain SQL and `quote_string` helper: + +``` +select_all("SELECT name, COUNT(id) as cnt FROM tags GROUP BY name HAVING COUNT(id) > 1").each do |tag| + tag_name = quote_string(tag["name"]) + duplicate_ids = select_all("SELECT id FROM tags WHERE name = '#{tag_name}'").map{|tag| tag["id"]} + origin_tag_id = duplicate_ids.first + duplicate_ids.delete origin_tag_id + + execute("UPDATE taggings SET tag_id = #{origin_tag_id} WHERE tag_id IN(#{duplicate_ids.join(",")})") + execute("DELETE FROM tags WHERE id IN(#{duplicate_ids.join(",")})") +end +``` diff --git a/doc/update/mysql_to_postgresql.md b/doc/update/mysql_to_postgresql.md index 50941db25f6..2c43cf59c1f 100644 --- a/doc/update/mysql_to_postgresql.md +++ b/doc/update/mysql_to_postgresql.md @@ -1,7 +1,7 @@ # Migrating GitLab from MySQL to Postgres *Make sure you view this [guide from the `master` branch](../../../master/doc/update/mysql_to_postgresql.md) for the most up to date instructions.* -If you are replacing MySQL with Postgres while keeping GitLab on the same server all you need to do is to export from MySQL, import into Postgres and rebuild the indexes as described below. If you are also moving GitLab to another server, or if you are switching to omnibus-gitlab, you may want to use a GitLab backup file. The second part of this documents explains the procedure to do this. +If you are replacing MySQL with Postgres while keeping GitLab on the same server all you need to do is to export from MySQL, convert the resulting SQL file, and import it into Postgres. If you are also moving GitLab to another server, or if you are switching to omnibus-gitlab, you may want to use a GitLab backup file. The second part of this documents explains the procedure to do this. ## Export from MySQL and import into Postgres @@ -14,13 +14,11 @@ sudo service gitlab stop git clone https://github.com/gitlabhq/mysql-postgresql-converter.git -b gitlab cd mysql-postgresql-converter -mysqldump --compatible=postgresql --default-character-set=utf8 -r databasename.mysql -u root gitlabhq_production -p -python db_converter.py databasename.mysql databasename.psql +mysqldump --compatible=postgresql --default-character-set=utf8 -r gitlabhq_production.mysql -u root gitlabhq_production -p +python db_converter.py gitlabhq_production.mysql gitlabhq_production.psql # Import the database dump as the application database user -sudo -u git psql -f databasename.psql -d gitlabhq_production - -# Rebuild indexes (see below) +sudo -u git psql -f gitlabhq_production.psql -d gitlabhq_production # Install gems for PostgreSQL (note: the line below states '--without ... mysql') sudo -u git -H bundle install --without development test mysql --deployment @@ -28,51 +26,6 @@ sudo -u git -H bundle install --without development test mysql --deployment sudo service gitlab start ``` -## Rebuild indexes - -The lanyrd database converter script does not preserve all indexes, so we have to recreate them ourselves after migrating from MySQL. It is not necessary to shut down GitLab for this process. - -### For non-omnibus installations - -On non-omnibus installations (distributed using Git) we retrieve the index declarations from version control using `git stash`. - -``` -# Clone the database converter on your Postgres-backed GitLab server -cd /tmp -git clone https://github.com/gitlabhq/mysql-postgresql-converter.git -b gitlab - -cd /home/git/gitlab - -# Stash changes to db/schema.rb to make sure we can find the right index statements -sudo -u git -H git stash - -# Generate add_index.rb -ruby /tmp/mysql-postgresql-converter/add_index_statements.rb db/schema.rb > /tmp/mysql-postgresql-converter/add_index.rb - -# Create the indexes -sudo -u git -H bundle exec rails runner -e production 'eval $stdin.read' < /tmp/mysql-postgresql-converter/add_index.rb -``` - -### For omnibus-gitlab installations - -On omnibus-gitlab we need to get the index declarations from a file called `schema.rb.bundled`. For versions older than 6.9, we need to download the file. - -``` -# Clone the database converter on your Postgres-backed GitLab server -cd /tmp -/opt/gitlab/embedded/bin/git clone https://github.com/gitlabhq/mysql-postgresql-converter.git -b gitlab -cd /tmp/mysql-postgresql-converter - -# Download schema.rb.bundled if necessary -test -e /opt/gitlab/embedded/service/gitlab-rails/db/schema.rb.bundled || sudo /opt/gitlab/embedded/bin/curl -o /opt/gitlab/embedded/service/gitlab-rails/db/schema.rb.bundled https://gitlab.com/gitlab-org/gitlab-ce/raw/v6.9.1/db/schema.rb - -# Generate add_index.rb -/opt/gitlab/embedded/bin/ruby add_index_statements.rb /opt/gitlab/embedded/service/gitlab-rails/db/schema.rb.bundled > add_index.rb - -# Create the indexes -/opt/gitlab/bin/gitlab-rails runner 'eval $stdin.read' < add_index.rb -``` - ## Converting a GitLab backup file from MySQL to Postgres **Note:** Please make sure to have Python 2.7.x (or higher) installed. diff --git a/lib/tasks/brakeman.rake b/lib/tasks/brakeman.rake index 52a9b017e79..5d4e0740373 100644 --- a/lib/tasks/brakeman.rake +++ b/lib/tasks/brakeman.rake @@ -1,5 +1,7 @@ desc 'Security check via brakeman' task :brakeman do + # We get 0 warnings at level 'w3' but we would like to reach 'w2'. Merge + # requests are welcome! if system(*%W(brakeman --skip-files lib/backup/repository.rb -w3 -z)) puts 'Security check succeed' else diff --git a/spec/controllers/profiles/two_factor_auths_controller_spec.rb b/spec/controllers/profiles/two_factor_auths_controller_spec.rb new file mode 100644 index 00000000000..f05d1f5fbe1 --- /dev/null +++ b/spec/controllers/profiles/two_factor_auths_controller_spec.rb @@ -0,0 +1,126 @@ +require 'spec_helper' + +describe Profiles::TwoFactorAuthsController do + before do + # `user` should be defined within the action-specific describe blocks + sign_in(user) + + allow(subject).to receive(:current_user).and_return(user) + end + + describe 'GET new' do + let(:user) { create(:user) } + + it 'generates otp_secret' do + expect { get :new }.to change { user.otp_secret } + end + + it 'assigns qr_code' do + code = double('qr code') + expect(subject).to receive(:build_qr_code).and_return(code) + + get :new + expect(assigns[:qr_code]).to eq code + end + end + + describe 'POST create' do + let(:user) { create(:user) } + let(:pin) { 'pin-code' } + + def go + post :create, pin_code: pin + end + + context 'with valid pin' do + before do + expect(user).to receive(:valid_otp?).with(pin).and_return(true) + end + + it 'sets otp_required_for_login' do + go + + user.reload + expect(user.otp_required_for_login).to eq true + end + + it 'presents plaintext codes for the user to save' do + expect(user).to receive(:generate_otp_backup_codes!).and_return(%w(a b c)) + + go + + expect(assigns[:codes]).to match_array %w(a b c) + end + + it 'renders create' do + go + expect(response).to render_template(:create) + end + end + + context 'with invalid pin' do + before do + expect(user).to receive(:valid_otp?).with(pin).and_return(false) + end + + it 'assigns error' do + go + expect(assigns[:error]).to eq 'Invalid pin code' + end + + it 'assigns qr_code' do + code = double('qr code') + expect(subject).to receive(:build_qr_code).and_return(code) + + go + expect(assigns[:qr_code]).to eq code + end + + it 'renders new' do + go + expect(response).to render_template(:new) + end + end + end + + describe 'POST codes' do + let(:user) { create(:user, :two_factor) } + + it 'presents plaintext codes for the user to save' do + expect(user).to receive(:generate_otp_backup_codes!).and_return(%w(a b c)) + + post :codes + expect(assigns[:codes]).to match_array %w(a b c) + end + + it 'persists the generated codes' do + post :codes + + user.reload + expect(user.otp_backup_codes).not_to be_empty + end + end + + describe 'DELETE destroy' do + let(:user) { create(:user, :two_factor) } + let!(:codes) { user.generate_otp_backup_codes! } + + it 'clears all 2FA-related fields' do + expect(user.otp_required_for_login).to eq true + expect(user.otp_backup_codes).not_to be_nil + expect(user.encrypted_otp_secret).not_to be_nil + + delete :destroy + + expect(user.otp_required_for_login).to eq false + expect(user.otp_backup_codes).to be_nil + expect(user.encrypted_otp_secret).to be_nil + end + + it 'redirects to profile_account_path' do + delete :destroy + + expect(response).to redirect_to(profile_account_path) + end + end +end diff --git a/spec/factories.rb b/spec/factories.rb index 19f2935f30e..26e8a795fa4 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -28,6 +28,13 @@ FactoryGirl.define do admin true end + trait :two_factor do + before(:create) do |user| + user.otp_required_for_login = true + user.otp_secret = User.generate_otp_secret + end + end + factory :omniauth_user do ignore do extern_uid '123456' diff --git a/spec/factories/notes.rb b/spec/factories/notes.rb index f1c33461b55..e1009d5916e 100644 --- a/spec/factories/notes.rb +++ b/spec/factories/notes.rb @@ -25,15 +25,16 @@ FactoryGirl.define do note "Note" author - factory :note_on_commit, traits: [:on_commit] - factory :note_on_commit_diff, traits: [:on_commit, :on_diff] - factory :note_on_issue, traits: [:on_issue], aliases: [:votable_note] - factory :note_on_merge_request, traits: [:on_merge_request] + factory :note_on_commit, traits: [:on_commit] + factory :note_on_commit_diff, traits: [:on_commit, :on_diff] + factory :note_on_issue, traits: [:on_issue], aliases: [:votable_note] + factory :note_on_merge_request, traits: [:on_merge_request] factory :note_on_merge_request_diff, traits: [:on_merge_request, :on_diff] - factory :note_on_project_snippet, traits: [:on_project_snippet] + factory :note_on_project_snippet, traits: [:on_project_snippet] + factory :system_note, traits: [:system] trait :on_commit do - project factory: :project + project commit_id RepoHelpers.sample_commit.id noteable_type "Commit" end @@ -43,7 +44,7 @@ FactoryGirl.define do end trait :on_merge_request do - project factory: :project + project noteable_id 1 noteable_type "MergeRequest" end @@ -58,6 +59,10 @@ FactoryGirl.define do noteable_type "Snippet" end + trait :system do + system true + end + trait :with_attachment do attachment { fixture_file_upload(Rails.root + "spec/fixtures/dk.png", "`/png") } end diff --git a/spec/features/login_spec.rb b/spec/features/login_spec.rb new file mode 100644 index 00000000000..046a9f6191d --- /dev/null +++ b/spec/features/login_spec.rb @@ -0,0 +1,101 @@ +require 'spec_helper' + +feature 'Login' do + describe 'with two-factor authentication' do + context 'with valid username/password' do + let(:user) { create(:user, :two_factor) } + + before do + login_with(user) + expect(page).to have_content('Two-factor Authentication') + end + + def enter_code(code) + fill_in 'Two-factor authentication code', with: code + click_button 'Verify code' + end + + it 'does not show a "You are already signed in." error message' do + enter_code(user.current_otp) + expect(page).not_to have_content('You are already signed in.') + end + + context 'using one-time code' do + it 'allows login with valid code' do + enter_code(user.current_otp) + expect(current_path).to eq root_path + end + + it 'blocks login with invalid code' do + enter_code('foo') + expect(page).to have_content('Invalid two-factor code') + end + + it 'allows login with invalid code, then valid code' do + enter_code('foo') + expect(page).to have_content('Invalid two-factor code') + + enter_code(user.current_otp) + expect(current_path).to eq root_path + end + end + + context 'using backup code' do + let(:codes) { user.generate_otp_backup_codes! } + + before do + expect(codes.size).to eq 10 + + # Ensure the generated codes get saved + user.save + end + + context 'with valid code' do + it 'allows login' do + enter_code(codes.sample) + expect(current_path).to eq root_path + end + + it 'invalidates the used code' do + expect { enter_code(codes.sample) }. + to change { user.reload.otp_backup_codes.size }.by(-1) + end + end + + context 'with invalid code' do + it 'blocks login' do + code = codes.sample + expect(user.invalidate_otp_backup_code!(code)).to eq true + + user.save! + expect(user.reload.otp_backup_codes.size).to eq 9 + + enter_code(code) + expect(page).to have_content('Invalid two-factor code.') + end + end + end + end + end + + describe 'without two-factor authentication' do + let(:user) { create(:user) } + + it 'allows basic login' do + login_with(user) + expect(current_path).to eq root_path + end + + it 'does not show a "You are already signed in." error message' do + login_with(user) + expect(page).not_to have_content('You are already signed in.') + end + + it 'blocks invalid login' do + user = create(:user, password: 'not-the-default') + + login_with(user) + expect(page).to have_content('Invalid email or password.') + end + end +end diff --git a/spec/features/password_reset_spec.rb b/spec/features/password_reset_spec.rb new file mode 100644 index 00000000000..a34efce09ef --- /dev/null +++ b/spec/features/password_reset_spec.rb @@ -0,0 +1,53 @@ +require 'spec_helper' + +feature 'Password reset' do + def forgot_password + click_on 'Forgot your password?' + fill_in 'Email', with: user.email + click_button 'Reset password' + user.reload + end + + def get_reset_token + mail = ActionMailer::Base.deliveries.last + body = mail.body.encoded + body.scan(/reset_password_token=(.+)\"/).flatten.first + end + + def reset_password(password = 'password') + visit edit_user_password_path(reset_password_token: get_reset_token) + + fill_in 'New password', with: password + fill_in 'Confirm new password', with: password + click_button 'Change your password' + end + + describe 'with two-factor authentication' do + let(:user) { create(:user, :two_factor) } + + it 'requires login after password reset' do + visit root_path + + forgot_password + reset_password + + expect(page).to have_content("Your password was changed successfully.") + expect(page).not_to have_content("You are now signed in.") + expect(current_path).to eq new_user_session_path + end + end + + describe 'without two-factor authentication' do + let(:user) { create(:user) } + + it 'automatically logs in after password reset' do + visit root_path + + forgot_password + reset_password + + expect(current_path).to eq root_path + expect(page).to have_content("Your password was changed successfully. You are now signed in.") + end + end +end diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index 4a6bfdb2910..ddacba58261 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -20,68 +20,88 @@ require 'spec_helper' describe Note do - describe "Associations" do + describe 'associations' do it { is_expected.to belong_to(:project) } it { is_expected.to belong_to(:noteable) } it { is_expected.to belong_to(:author).class_name('User') } end - describe "Mass assignment" do - end - - describe "Validation" do + describe 'validation' do it { is_expected.to validate_presence_of(:note) } it { is_expected.to validate_presence_of(:project) } end - describe "Voting score" do - let(:project) { create(:project) } + describe '#votable?' do + it 'is true for issue notes' do + note = build(:note_on_issue) + expect(note).to be_votable + end + + it 'is true for merge request notes' do + note = build(:note_on_merge_request) + expect(note).to be_votable + end + + it 'is false for merge request diff notes' do + note = build(:note_on_merge_request_diff) + expect(note).not_to be_votable + end + + it 'is false for commit notes' do + note = build(:note_on_commit) + expect(note).not_to be_votable + end - it "recognizes a neutral note" do - note = create(:votable_note, note: "This is not a +1 note") + it 'is false for commit diff notes' do + note = build(:note_on_commit_diff) + expect(note).not_to be_votable + end + end + + describe 'voting score' do + it 'recognizes a neutral note' do + note = build(:votable_note, note: 'This is not a +1 note') expect(note).not_to be_upvote expect(note).not_to be_downvote end - it "recognizes a neutral emoji note" do + it 'recognizes a neutral emoji note' do note = build(:votable_note, note: "I would :+1: this, but I don't want to") expect(note).not_to be_upvote expect(note).not_to be_downvote end - it "recognizes a +1 note" do - note = create(:votable_note, note: "+1 for this") + it 'recognizes a +1 note' do + note = build(:votable_note, note: '+1 for this') expect(note).to be_upvote end - it "recognizes a +1 emoji as a vote" do - note = build(:votable_note, note: ":+1: for this") + it 'recognizes a +1 emoji as a vote' do + note = build(:votable_note, note: ':+1: for this') expect(note).to be_upvote end - it "recognizes a thumbsup emoji as a vote" do - note = build(:votable_note, note: ":thumbsup: for this") + it 'recognizes a thumbsup emoji as a vote' do + note = build(:votable_note, note: ':thumbsup: for this') expect(note).to be_upvote end - it "recognizes a -1 note" do - note = create(:votable_note, note: "-1 for this") + it 'recognizes a -1 note' do + note = build(:votable_note, note: '-1 for this') expect(note).to be_downvote end - it "recognizes a -1 emoji as a vote" do - note = build(:votable_note, note: ":-1: for this") + it 'recognizes a -1 emoji as a vote' do + note = build(:votable_note, note: ':-1: for this') expect(note).to be_downvote end - it "recognizes a thumbsdown emoji as a vote" do - note = build(:votable_note, note: ":thumbsdown: for this") + it 'recognizes a thumbsdown emoji as a vote' do + note = build(:votable_note, note: ':thumbsdown: for this') expect(note).to be_downvote end end - let(:project) { create(:project) } - describe "Commit notes" do let!(:note) { create(:note_on_commit, note: "+1 from me") } let!(:commit) { note.noteable } @@ -100,10 +120,6 @@ describe Note do it "should be recognized by #for_commit?" do expect(note).to be_for_commit end - - it "should not be votable" do - expect(note).not_to be_votable - end end describe "Commit diff line notes" do @@ -128,461 +144,7 @@ describe Note do end end - describe "Issue notes" do - let!(:note) { create(:note_on_issue, note: "+1 from me") } - - it "should not be votable" do - expect(note).to be_votable - end - end - - describe "Merge request notes" do - let!(:note) { create(:note_on_merge_request, note: "+1 from me") } - - it "should be votable" do - expect(note).to be_votable - end - end - - describe "Merge request diff line notes" do - let!(:note) { create(:note_on_merge_request_diff, note: "+1 from me") } - - it "should not be votable" do - expect(note).not_to be_votable - end - end - - describe '#create_status_change_note' do - let(:project) { create(:project) } - let(:thing) { create(:issue, project: project) } - let(:author) { create(:user) } - let(:status) { 'new_status' } - - subject { Note.create_status_change_note(thing, project, author, status, nil) } - - it 'creates and saves a Note' do - is_expected.to be_a Note - expect(subject.id).not_to be_nil - end - - describe '#noteable' do - subject { super().noteable } - it { is_expected.to eq(thing) } - end - - describe '#project' do - subject { super().project } - it { is_expected.to eq(thing.project) } - end - - describe '#author' do - subject { super().author } - it { is_expected.to eq(author) } - end - - describe '#note' do - subject { super().note } - it { is_expected.to eq("Status changed to #{status}") } - end - - it 'appends a back-reference if a closing mentionable is supplied' do - commit = double('commit', gfm_reference: 'commit 123456') - n = Note.create_status_change_note(thing, project, author, status, commit) - - expect(n.note).to eq("Status changed to #{status} by commit 123456") - end - end - - describe '#create_assignee_change_note' do - let(:project) { create(:project) } - let(:thing) { create(:issue, project: project) } - let(:author) { create(:user) } - let(:assignee) { create(:user, username: "assigned_user") } - - subject { Note.create_assignee_change_note(thing, project, author, assignee) } - - context 'creates and saves a Note' do - it { is_expected.to be_a Note } - - describe '#id' do - subject { super().id } - it { is_expected.not_to be_nil } - end - end - - describe '#noteable' do - subject { super().noteable } - it { is_expected.to eq(thing) } - end - - describe '#project' do - subject { super().project } - it { is_expected.to eq(thing.project) } - end - - describe '#author' do - subject { super().author } - it { is_expected.to eq(author) } - end - - describe '#note' do - subject { super().note } - it { is_expected.to eq('Reassigned to @assigned_user') } - end - - context 'assignee is removed' do - let(:assignee) { nil } - - describe '#note' do - subject { super().note } - it { is_expected.to eq('Assignee removed') } - end - end - end - - describe '#create_labels_change_note' do - let(:project) { create(:project) } - let(:thing) { create(:issue, project: project) } - let(:author) { create(:user) } - let(:label1) { create(:label) } - let(:label2) { create(:label) } - let(:added_labels) { [label1, label2] } - let(:removed_labels) { [] } - - subject { Note.create_labels_change_note(thing, project, author, added_labels, removed_labels) } - - context 'creates and saves a Note' do - it { is_expected.to be_a Note } - - describe '#id' do - subject { super().id } - it { is_expected.not_to be_nil } - end - end - - describe '#noteable' do - subject { super().noteable } - it { is_expected.to eq(thing) } - end - - describe '#project' do - subject { super().project } - it { is_expected.to eq(thing.project) } - end - - describe '#author' do - subject { super().author } - it { is_expected.to eq(author) } - end - - describe '#note' do - subject { super().note } - it { is_expected.to eq("Added ~#{label1.id} ~#{label2.id} labels") } - end - - context 'label is removed' do - let(:added_labels) { [label1] } - let(:removed_labels) { [label2] } - - describe '#note' do - subject { super().note } - it { is_expected.to eq("Added ~#{label1.id} and removed ~#{label2.id} labels") } - end - end - end - - describe '#create_milestone_change_note' do - let(:project) { create(:project) } - let(:thing) { create(:issue, project: project) } - let(:milestone) { create(:milestone, project: project, title: "first_milestone") } - let(:author) { create(:user) } - - subject { Note.create_milestone_change_note(thing, project, author, milestone) } - - context 'creates and saves a Note' do - it { is_expected.to be_a Note } - - describe '#id' do - subject { super().id } - it { is_expected.not_to be_nil } - end - end - - describe '#project' do - subject { super().project } - it { is_expected.to eq(thing.project) } - end - - describe '#author' do - subject { super().author } - it { is_expected.to eq(author) } - end - - describe '#note' do - subject { super().note } - it { is_expected.to eq("Milestone changed to first_milestone") } - end - end - - describe '#create_cross_reference_note' do - let(:project) { create(:project) } - let(:author) { create(:user) } - let(:issue) { create(:issue, project: project) } - let(:mergereq) { create(:merge_request, :simple, target_project: project, source_project: project) } - let(:commit) { project.commit } - - # Test all of {issue, merge request, commit} in both the referenced and referencing - # roles, to ensure that the correct information can be inferred from any argument. - - context 'issue from a merge request' do - subject { Note.create_cross_reference_note(issue, mergereq, author) } - - it { is_expected.to be_valid } - - describe '#noteable' do - subject { super().noteable } - it { is_expected.to eq(issue) } - end - - describe '#project' do - subject { super().project } - it { is_expected.to eq(issue.project) } - end - - describe '#author' do - subject { super().author } - it { is_expected.to eq(author) } - end - - describe '#note' do - subject { super().note } - it { is_expected.to eq("mentioned in merge request !#{mergereq.iid}") } - end - end - - context 'issue from a commit' do - subject { Note.create_cross_reference_note(issue, commit, author) } - - it { is_expected.to be_valid } - - describe '#noteable' do - subject { super().noteable } - it { is_expected.to eq(issue) } - end - - describe '#note' do - subject { super().note } - it { is_expected.to eq("mentioned in commit #{commit.sha}") } - end - end - - context 'merge request from an issue' do - subject { Note.create_cross_reference_note(mergereq, issue, author) } - - it { is_expected.to be_valid } - - describe '#noteable' do - subject { super().noteable } - it { is_expected.to eq(mergereq) } - end - - describe '#project' do - subject { super().project } - it { is_expected.to eq(mergereq.project) } - end - - describe '#note' do - subject { super().note } - it { is_expected.to eq("mentioned in issue ##{issue.iid}") } - end - end - - context 'commit from a merge request' do - subject { Note.create_cross_reference_note(commit, mergereq, author) } - - it { is_expected.to be_valid } - - describe '#noteable' do - subject { super().noteable } - it { is_expected.to eq(commit) } - end - - describe '#project' do - subject { super().project } - it { is_expected.to eq(project) } - end - - describe '#note' do - subject { super().note } - it { is_expected.to eq("mentioned in merge request !#{mergereq.iid}") } - end - end - - context 'commit contained in a merge request' do - subject { Note.create_cross_reference_note(mergereq.commits.first, mergereq, author) } - - it { is_expected.to be_nil } - end - - context 'commit from issue' do - subject { Note.create_cross_reference_note(commit, issue, author) } - - it { is_expected.to be_valid } - - describe '#noteable_type' do - subject { super().noteable_type } - it { is_expected.to eq("Commit") } - end - - describe '#noteable_id' do - subject { super().noteable_id } - it { is_expected.to be_nil } - end - - describe '#commit_id' do - subject { super().commit_id } - it { is_expected.to eq(commit.id) } - end - - describe '#note' do - subject { super().note } - it { is_expected.to eq("mentioned in issue ##{issue.iid}") } - end - end - - context 'commit from commit' do - let(:parent_commit) { commit.parents.first } - subject { Note.create_cross_reference_note(commit, parent_commit, author) } - - it { is_expected.to be_valid } - - describe '#noteable_type' do - subject { super().noteable_type } - it { is_expected.to eq("Commit") } - end - - describe '#noteable_id' do - subject { super().noteable_id } - it { is_expected.to be_nil } - end - - describe '#commit_id' do - subject { super().commit_id } - it { is_expected.to eq(commit.id) } - end - - describe '#note' do - subject { super().note } - it { is_expected.to eq("mentioned in commit #{parent_commit.id}") } - end - end - end - - describe '#cross_reference_exists?' do - let(:project) { create :project } - let(:author) { create :user } - let(:issue) { create :issue } - let(:commit0) { project.commit } - let(:commit1) { project.commit('HEAD~2') } - - before do - Note.create_cross_reference_note(issue, commit0, author) - end - - it 'detects if a mentionable has already been mentioned' do - expect(Note.cross_reference_exists?(issue, commit0)).to be_truthy - end - - it 'detects if a mentionable has not already been mentioned' do - expect(Note.cross_reference_exists?(issue, commit1)).to be_falsey - end - - context 'commit on commit' do - before do - Note.create_cross_reference_note(commit0, commit1, author) - end - - it { expect(Note.cross_reference_exists?(commit0, commit1)).to be_truthy } - it { expect(Note.cross_reference_exists?(commit1, commit0)).to be_falsey } - end - - context 'legacy note with Markdown emphasis' do - let(:issue2) { create :issue, project: project } - let!(:note) do - create :note, system: true, noteable_id: issue2.id, - noteable_type: "Issue", note: "_mentioned in issue " \ - "#{issue.project.path_with_namespace}##{issue.iid}_" - end - - it 'detects if a mentionable with emphasis has been mentioned' do - expect(Note.cross_reference_exists?(issue2, issue)).to be_truthy - end - end - end - - describe '#cross_references_with_underscores?' do - let(:project) { create :project, path: "first_project" } - let(:second_project) { create :project, path: "second_project" } - - let(:author) { create :user } - let(:issue0) { create :issue, project: project } - let(:issue1) { create :issue, project: second_project } - let!(:note) { Note.create_cross_reference_note(issue0, issue1, author) } - - it 'detects if a mentionable has already been mentioned' do - expect(Note.cross_reference_exists?(issue0, issue1)).to be_truthy - end - - it 'detects if a mentionable has not already been mentioned' do - expect(Note.cross_reference_exists?(issue1, issue0)).to be_falsey - end - - it 'detects that text has underscores' do - expect(note.note).to eq("mentioned in issue #{second_project.path_with_namespace}##{issue1.iid}") - end - end - - describe '#system?' do - let(:project) { create(:project) } - let(:issue) { create(:issue, project: project) } - let(:other) { create(:issue, project: project) } - let(:author) { create(:user) } - let(:assignee) { create(:user) } - let(:label) { create(:label) } - let(:milestone) { create(:milestone) } - - it 'should recognize user-supplied notes as non-system' do - @note = create(:note_on_issue) - expect(@note).not_to be_system - end - - it 'should identify status-change notes as system notes' do - @note = Note.create_status_change_note(issue, project, author, 'closed', nil) - expect(@note).to be_system - end - - it 'should identify cross-reference notes as system notes' do - @note = Note.create_cross_reference_note(issue, other, author) - expect(@note).to be_system - end - - it 'should identify assignee-change notes as system notes' do - @note = Note.create_assignee_change_note(issue, project, author, assignee) - expect(@note).to be_system - end - - it 'should identify label-change notes as system notes' do - @note = Note.create_labels_change_note(issue, project, author, [label], []) - expect(@note).to be_system - end - - it 'should identify milestone-change notes as system notes' do - @note = Note.create_milestone_change_note(issue, project, author, milestone) - expect(@note).to be_system - end - end - - describe :authorization do + describe 'authorization' do before do @p1 = create(:project) @p2 = create(:project) @@ -593,7 +155,7 @@ describe Note do @abilities << Ability end - describe :read do + describe 'read' do before do @p1.project_members.create(user: @u2, access_level: ProjectMember::GUEST) @p2.project_members.create(user: @u3, access_level: ProjectMember::GUEST) @@ -604,7 +166,7 @@ describe Note do it { expect(@abilities.allowed?(@u3, :read_note, @p1)).to be_falsey } end - describe :write do + describe 'write' do before do @p1.project_members.create(user: @u2, access_level: ProjectMember::DEVELOPER) @p2.project_members.create(user: @u3, access_level: ProjectMember::DEVELOPER) @@ -615,7 +177,7 @@ describe Note do it { expect(@abilities.allowed?(@u3, :write_note, @p1)).to be_falsey } end - describe :admin do + describe 'admin' do before do @p1.project_members.create(user: @u1, access_level: ProjectMember::REPORTER) @p1.project_members.create(user: @u2, access_level: ProjectMember::MASTER) @@ -631,6 +193,7 @@ describe Note do it_behaves_like 'an editable mentionable' do subject { create :note, noteable: issue, project: project } + let(:project) { create(:project) } let(:issue) { create :issue, project: project } let(:backref_text) { issue.gfm_reference } let(:set_mentionable_text) { ->(txt) { subject.note = txt } } diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 771709c127a..0dddcd5bda2 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -50,6 +50,11 @@ # bitbucket_access_token :string(255) # bitbucket_access_token_secret :string(255) # location :string(255) +# encrypted_otp_secret :string(255) +# encrypted_otp_secret_iv :string(255) +# encrypted_otp_secret_salt :string(255) +# otp_required_for_login :boolean +# otp_backup_codes :text # public_email :string(255) default(""), not null # diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb new file mode 100644 index 00000000000..4e4cb6d19ed --- /dev/null +++ b/spec/services/system_note_service_spec.rb @@ -0,0 +1,346 @@ +require 'spec_helper' + +describe SystemNoteService do + let(:project) { create(:project) } + let(:author) { create(:user) } + let(:noteable) { create(:issue, project: project) } + + shared_examples_for 'a system note' do + it 'is valid' do + expect(subject).to be_valid + end + + it 'sets the noteable model' do + expect(subject.noteable).to eq noteable + end + + it 'sets the project' do + expect(subject.project).to eq project + end + + it 'sets the author' do + expect(subject.author).to eq author + end + + it 'is a system note' do + expect(subject).to be_system + end + end + + describe '.add_commits' do + subject { described_class.add_commits(noteable, project, author, new_commits, old_commits, oldrev) } + + let(:noteable) { create(:merge_request, source_project: project) } + let(:new_commits) { noteable.commits } + let(:old_commits) { [] } + let(:oldrev) { nil } + + it_behaves_like 'a system note' + + describe 'note body' do + let(:note_lines) { subject.note.split("\n").reject(&:blank?) } + + context 'without existing commits' do + it 'adds a message header' do + expect(note_lines[0]).to eq "Added #{new_commits.size} commits:" + end + + it 'adds a message line for each commit' do + new_commits.each_with_index do |commit, i| + # Skip the header + expect(note_lines[i + 1]).to eq "* #{commit.short_id} - #{commit.title}" + end + end + end + + describe 'summary line for existing commits' do + let(:summary_line) { note_lines[1] } + + context 'with one existing commit' do + let(:old_commits) { [noteable.commits.last] } + + it 'includes the existing commit' do + expect(summary_line).to eq "* #{old_commits.first.short_id} - 1 commit from branch `feature`" + end + end + + context 'with multiple existing commits' do + let(:old_commits) { noteable.commits[3..-1] } + + context 'with oldrev' do + let(:oldrev) { noteable.commits[2].id } + + it 'includes a commit range' do + expect(summary_line).to start_with "* #{Commit.truncate_sha(oldrev)}...#{old_commits.last.short_id}" + end + + it 'includes a commit count' do + expect(summary_line).to end_with " - 2 commits from branch `feature`" + end + end + + context 'without oldrev' do + it 'includes a commit range' do + expect(summary_line).to start_with "* #{old_commits[0].short_id}..#{old_commits[-1].short_id}" + end + + it 'includes a commit count' do + expect(summary_line).to end_with " - 2 commits from branch `feature`" + end + end + + context 'on a fork' do + before do + expect(noteable).to receive(:for_fork?).and_return(true) + end + + it 'includes the project namespace' do + expect(summary_line).to end_with "`#{noteable.target_project_namespace}:feature`" + end + end + end + end + end + end + + describe '.change_assignee' do + subject { described_class.change_assignee(noteable, project, author, assignee) } + + let(:assignee) { create(:user) } + + it_behaves_like 'a system note' + + context 'when assignee added' do + it 'sets the note text' do + expect(subject.note).to eq "Reassigned to @#{assignee.username}" + end + end + + context 'when assignee removed' do + let(:assignee) { nil } + + it 'sets the note text' do + expect(subject.note).to eq 'Assignee removed' + end + end + end + + describe '.change_label' do + subject { described_class.change_label(noteable, project, author, added, removed) } + + let(:labels) { create_list(:label, 2) } + let(:added) { [] } + let(:removed) { [] } + + it_behaves_like 'a system note' + + context 'with added labels' do + let(:added) { labels } + let(:removed) { [] } + + it 'sets the note text' do + expect(subject.note).to eq "Added ~#{labels[0].id} ~#{labels[1].id} labels" + end + end + + context 'with removed labels' do + let(:added) { [] } + let(:removed) { labels } + + it 'sets the note text' do + expect(subject.note).to eq "Removed ~#{labels[0].id} ~#{labels[1].id} labels" + end + end + + context 'with added and removed labels' do + let(:added) { [labels[0]] } + let(:removed) { [labels[1]] } + + it 'sets the note text' do + expect(subject.note).to eq "Added ~#{labels[0].id} and removed ~#{labels[1].id} labels" + end + end + end + + describe '.change_milestone' do + subject { described_class.change_milestone(noteable, project, author, milestone) } + + let(:milestone) { create(:milestone, project: project) } + + it_behaves_like 'a system note' + + context 'when milestone added' do + it 'sets the note text' do + expect(subject.note).to eq "Milestone changed to #{milestone.title}" + end + end + + context 'when milestone removed' do + let(:milestone) { nil } + + it 'sets the note text' do + expect(subject.note).to eq 'Milestone removed' + end + end + end + + describe '.change_status' do + subject { described_class.change_status(noteable, project, author, status, source) } + + let(:status) { 'new_status' } + let(:source) { nil } + + it_behaves_like 'a system note' + + context 'with a source' do + let(:source) { double('commit', gfm_reference: 'commit 123456') } + + it 'sets the note text' do + expect(subject.note).to eq "Status changed to #{status} by commit 123456" + end + end + + context 'without a source' do + it 'sets the note text' do + expect(subject.note).to eq "Status changed to #{status}" + end + end + end + + describe '.cross_reference' do + subject { described_class.cross_reference(noteable, mentioner, author) } + + let(:mentioner) { create(:issue, project: project) } + + it_behaves_like 'a system note' + + context 'when cross-reference disallowed' do + before do + expect(described_class).to receive(:cross_reference_disallowed?).and_return(true) + end + + it 'returns nil' do + expect(subject).to be_nil + end + end + + context 'when cross-reference allowed' do + before do + expect(described_class).to receive(:cross_reference_disallowed?).and_return(false) + end + + describe 'note_body' do + context 'cross-project' do + let(:project2) { create(:project) } + let(:mentioner) { create(:issue, project: project2) } + + context 'from Commit' do + let(:mentioner) { project2.repository.commit } + + it 'references the mentioning commit' do + expect(subject.note).to eq "mentioned in commit #{project2.path_with_namespace}@#{mentioner.id}" + end + end + + context 'from non-Commit' do + it 'references the mentioning object' do + expect(subject.note).to eq "mentioned in issue #{project2.path_with_namespace}##{mentioner.iid}" + end + end + end + + context 'within the same project' do + context 'from Commit' do + let(:mentioner) { project.repository.commit } + + it 'references the mentioning commit' do + expect(subject.note).to eq "mentioned in commit #{mentioner.id}" + end + end + + context 'from non-Commit' do + it 'references the mentioning object' do + expect(subject.note).to eq "mentioned in issue ##{mentioner.iid}" + end + end + end + end + end + end + + describe '.cross_reference?' do + it 'is truthy when text begins with expected text' do + expect(described_class.cross_reference?('mentioned in issue #1')).to be_truthy + end + + it 'is falsey when text does not begin with expected text' do + expect(described_class.cross_reference?('this is a note')).to be_falsey + end + end + + describe '.cross_reference_disallowed?' do + context 'when mentioner is not a MergeRequest' do + it 'is falsey' do + mentioner = noteable.dup + expect(described_class.cross_reference_disallowed?(noteable, mentioner)). + to be_falsey + end + end + + context 'when mentioner is a MergeRequest' do + let(:mentioner) { create(:merge_request, :simple, source_project: project) } + let(:noteable) { project.commit } + + it 'is truthy when noteable is in commits' do + expect(mentioner).to receive(:commits).and_return([noteable]) + expect(described_class.cross_reference_disallowed?(noteable, mentioner)). + to be_truthy + end + + it 'is falsey when noteable is not in commits' do + expect(mentioner).to receive(:commits).and_return([]) + expect(described_class.cross_reference_disallowed?(noteable, mentioner)). + to be_falsey + end + end + end + + describe '.cross_reference_exists?' do + let(:commit0) { project.commit } + let(:commit1) { project.commit('HEAD~2') } + + context 'issue from commit' do + before do + # Mention issue (noteable) from commit0 + described_class.cross_reference(noteable, commit0, author) + end + + it 'is truthy when already mentioned' do + expect(described_class.cross_reference_exists?(noteable, commit0)). + to be_truthy + end + + it 'is falsey when not already mentioned' do + expect(described_class.cross_reference_exists?(noteable, commit1)). + to be_falsey + end + end + + context 'commit from commit' do + before do + # Mention commit1 from commit0 + described_class.cross_reference(commit0, commit1, author) + end + + it 'is truthy when already mentioned' do + expect(described_class.cross_reference_exists?(commit0, commit1)). + to be_truthy + end + + it 'is falsey when not already mentioned' do + expect(described_class.cross_reference_exists?(commit1, commit0)). + to be_falsey + end + end + end +end |