diff options
author | Douwe Maan <douwe@gitlab.com> | 2016-11-08 10:47:45 +0000 |
---|---|---|
committer | Douwe Maan <douwe@gitlab.com> | 2016-11-08 10:47:45 +0000 |
commit | 9c3f3e9e359740b10edafaa1b06b0ff3e2070820 (patch) | |
tree | 01846d1a63bd1177b68377822fcb23b86e176cf2 | |
parent | 8f85bd5789d50c080a7214981cc0a45fcb382bef (diff) | |
parent | 011e561bfa227f3ecbafe5b1ffd51700c680a15f (diff) | |
download | gitlab-ce-9c3f3e9e359740b10edafaa1b06b0ff3e2070820.tar.gz |
Merge branch 'use-separate-token-for-incoming-email' into 'master'
Use separate email-friendly token for incoming email
See merge request !5914
26 files changed, 230 insertions, 60 deletions
diff --git a/app/assets/javascripts/issuable.js.es6 b/app/assets/javascripts/issuable.js.es6 index 8fc498be27d..46503c290ae 100644 --- a/app/assets/javascripts/issuable.js.es6 +++ b/app/assets/javascripts/issuable.js.es6 @@ -10,6 +10,7 @@ Issuable.initSearch(); Issuable.initChecks(); Issuable.initResetFilters(); + Issuable.resetIncomingEmailToken(); return Issuable.initLabelFilterRemove(); }, initTemplates: function() { @@ -154,6 +155,27 @@ this.issuableBulkActions.willUpdateLabels = false; } return true; + }, + + resetIncomingEmailToken: function() { + $('.incoming-email-token-reset').on('click', function(e) { + e.preventDefault(); + + $.ajax({ + type: 'PUT', + url: $('.incoming-email-token-reset').attr('href'), + dataType: 'json', + success: function(response) { + $('#issue_email').val(response.new_issue_address).focus(); + }, + beforeSend: function() { + $('.incoming-email-token-reset').text('resetting...'); + }, + complete: function() { + $('.incoming-email-token-reset').text('reset it'); + } + }); + }); } }; diff --git a/app/assets/stylesheets/pages/profile.scss b/app/assets/stylesheets/pages/profile.scss index ede29db1979..6fab97a71aa 100644 --- a/app/assets/stylesheets/pages/profile.scss +++ b/app/assets/stylesheets/pages/profile.scss @@ -23,6 +23,10 @@ color: $md-link-color; } +.private-tokens-reset div.reset-action:not(:first-child) { + padding-top: 15px; +} + .oauth-buttons { .btn-group { margin-right: 10px; diff --git a/app/controllers/profiles_controller.rb b/app/controllers/profiles_controller.rb index f71e0a1302b..f0c71725ea8 100644 --- a/app/controllers/profiles_controller.rb +++ b/app/controllers/profiles_controller.rb @@ -26,7 +26,15 @@ class ProfilesController < Profiles::ApplicationController def reset_private_token if current_user.reset_authentication_token! - flash[:notice] = "Token was successfully updated" + flash[:notice] = "Private token was successfully reset" + end + + redirect_to profile_account_path + end + + def reset_incoming_email_token + if current_user.reset_incoming_email_token! + flash[:notice] = "Incoming email token was successfully reset" end redirect_to profile_account_path diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 8cd50480ec1..28820adcc46 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -160,6 +160,13 @@ class ProjectsController < Projects::ApplicationController end end + def new_issue_address + return render_404 unless Gitlab::IncomingEmail.supports_issue_creation? + + current_user.reset_incoming_email_token! + render json: { new_issue_address: @project.new_issue_address(current_user) } + end + def archive return access_denied! unless can?(current_user, :archive_project, @project) diff --git a/app/helpers/accounts_helper.rb b/app/helpers/accounts_helper.rb new file mode 100644 index 00000000000..5d27d30eaa3 --- /dev/null +++ b/app/helpers/accounts_helper.rb @@ -0,0 +1,5 @@ +module AccountsHelper + def incoming_email_token_enabled? + current_user.incoming_email_token && Gitlab::IncomingEmail.supports_issue_creation? + end +end diff --git a/app/models/concerns/token_authenticatable.rb b/app/models/concerns/token_authenticatable.rb index 24c7b26d223..04d30f46210 100644 --- a/app/models/concerns/token_authenticatable.rb +++ b/app/models/concerns/token_authenticatable.rb @@ -4,17 +4,21 @@ module TokenAuthenticatable private def write_new_token(token_field) - new_token = generate_token(token_field) + new_token = generate_available_token(token_field) write_attribute(token_field, new_token) end - def generate_token(token_field) + def generate_available_token(token_field) loop do - token = Devise.friendly_token + token = generate_token(token_field) break token unless self.class.unscoped.find_by(token_field => token) end end + def generate_token(token_field) + Devise.friendly_token + end + class_methods do def authentication_token_fields @token_fields || [] diff --git a/app/models/project.rb b/app/models/project.rb index 686d285410b..4c9c7c001dd 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -624,13 +624,12 @@ class Project < ActiveRecord::Base end def new_issue_address(author) - # This feature is disabled for the time being. - return nil + return unless Gitlab::IncomingEmail.supports_issue_creation? && author - if Gitlab::IncomingEmail.enabled? && author # rubocop:disable Lint/UnreachableCode - Gitlab::IncomingEmail.reply_address( - "#{path_with_namespace}+#{author.authentication_token}") - end + author.ensure_incoming_email_token! + + Gitlab::IncomingEmail.reply_address( + "#{path_with_namespace}+#{author.incoming_email_token}") end def build_commit_note(commit) diff --git a/app/models/user.rb b/app/models/user.rb index c0dffa7b6ea..3813df6684e 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -13,6 +13,7 @@ class User < ActiveRecord::Base DEFAULT_NOTIFICATION_LEVEL = :participating add_authentication_token_field :authentication_token + add_authentication_token_field :incoming_email_token default_value_for :admin, false default_value_for(:external) { current_application_settings.user_default_external } @@ -119,7 +120,7 @@ class User < ActiveRecord::Base before_validation :set_public_email, if: ->(user) { user.public_email_changed? } after_update :update_emails_with_primary_email, if: ->(user) { user.email_changed? } - before_save :ensure_authentication_token + before_save :ensure_authentication_token, :ensure_incoming_email_token before_save :ensure_external_user_rights after_save :ensure_namespace_correct after_initialize :set_projects_limit @@ -956,4 +957,13 @@ class User < ActiveRecord::Base signup_domain =~ regexp end end + + def generate_token(token_field) + if token_field == :incoming_email_token + # Needs to be all lowercase and alphanumeric because it's gonna be used in an email address. + SecureRandom.hex.to_i(16).to_s(36) + else + super + end + end end diff --git a/app/views/profiles/accounts/show.html.haml b/app/views/profiles/accounts/show.html.haml index e2e974ba072..72f658d1b68 100644 --- a/app/views/profiles/accounts/show.html.haml +++ b/app/views/profiles/accounts/show.html.haml @@ -8,24 +8,36 @@ .row.prepend-top-default .col-lg-3.profile-settings-sidebar %h4.prepend-top-0 - Private Token + = incoming_email_token_enabled? ? "Private Tokens" : "Private Token" %p - Your private token is used to access application resources without authentication. - .col-lg-9 - = form_for @user, url: reset_private_token_profile_path, method: :put, html: { class: "private-token" } do |f| + Keep + = incoming_email_token_enabled? ? "these tokens" : "this token" + secret, anyone with access to them can interact with GitLab as if they were you. + .col-lg-9.private-tokens-reset + .reset-action %p.cgray - if current_user.private_token - = label_tag "token", "Private token", class: "label-light" - = text_field_tag "token", current_user.private_token, class: "form-control" + = label_tag "private-token", "Private token", class: "label-light" + = text_field_tag "private-token", current_user.private_token, class: "form-control", readonly: true, onclick: "this.select()" - else - %span You don`t have one yet. Click generate to fix it. - %p.help-block - It can be used for atom feeds or the API. Keep it secret! + %span You don't have one yet. Click generate to fix it. + %p.help-block + Your private token is used to access the API and Atom feeds without username/password authentication. .prepend-top-default - if current_user.private_token - = f.submit 'Reset private token', data: { confirm: "Are you sure?" }, class: "btn btn-default" + = link_to 'Reset private token', reset_private_token_profile_path, method: :put, data: { confirm: "Are you sure?" }, class: "btn btn-default private-token" - else = f.submit 'Generate', class: "btn btn-default" + - if incoming_email_token_enabled? + .reset-action + %p.cgray + = label_tag "incoming-email-token", "Incoming Email Token", class: 'label-light' + = text_field_tag "incoming-email-token", current_user.incoming_email_token, class: "form-control", readonly: true, onclick: "this.select()" + %p.help-block + Your incoming email token is used to create new issues by email, and is included in your project-specific email addresses. + .prepend-top-default + = link_to 'Reset incoming email token', reset_incoming_email_token_profile_path, method: :put, data: { confirm: "Are you sure?" }, class: "btn btn-default incoming-email-token" + %hr .row.prepend-top-default .col-lg-3.profile-settings-sidebar diff --git a/app/views/projects/issues/_issue_by_email.html.haml b/app/views/projects/issues/_issue_by_email.html.haml index 72669372497..d2038a2be68 100644 --- a/app/views/projects/issues/_issue_by_email.html.haml +++ b/app/views/projects/issues/_issue_by_email.html.haml @@ -12,16 +12,23 @@ Create new issue by email .modal-body %p - Write an email to the below email address. (This is a private email address, so keep it secret.) + You can create a new issue inside this project by sending an email to the following email address: .email-modal-input-group.input-group = text_field_tag :issue_email, email, class: "monospace js-select-on-focus form-control", readonly: true .input-group-btn = clipboard_button(clipboard_target: '#issue_email') %p - Send an email to this address to create an issue. - %p - Use the subject line as the title of your issue. + The subject will be used as the title of the new issue, and the message will be the description. + + = link_to 'Slash commands', help_page_path('user/project/slash_commands'), target: '_blank', tabindex: -1 + and styling with + = link_to 'Markdown', help_page_path('user/markdown'), target: '_blank', tabindex: -1 + are supported. + %p - Use the message as the body of your issue (feel free to include some nice - = succeed ")." do - = link_to "Markdown", help_page_path('markdown', 'markdown') + This is a private email address, generated just for you. + + Anyone who gets ahold of it can create issues as if they were you. + You should + = link_to 'reset it', new_issue_address_namespace_project_path(@project.namespace, @project), class: 'incoming-email-token-reset' + if that ever happens. diff --git a/changelogs/unreleased/use-separate-token-for-incoming-email.yml b/changelogs/unreleased/use-separate-token-for-incoming-email.yml new file mode 100644 index 00000000000..e498f8dd0a6 --- /dev/null +++ b/changelogs/unreleased/use-separate-token-for-incoming-email.yml @@ -0,0 +1,4 @@ +--- +title: Use separate email-token for incoming email and revert back the inactive feature +merge_request: 5914 +author: diff --git a/config/routes/profile.rb b/config/routes/profile.rb index 4cb68c9b34a..52b9a565db8 100644 --- a/config/routes/profile.rb +++ b/config/routes/profile.rb @@ -4,6 +4,7 @@ resource :profile, only: [:show, :update] do get :applications, to: 'oauth/applications#index' put :reset_private_token + put :reset_incoming_email_token put :update_username end diff --git a/config/routes/project.rb b/config/routes/project.rb index 8142e231621..7a1bfa6a9f0 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -18,6 +18,7 @@ resources :namespaces, path: '/', constraints: { id: /[a-zA-Z.0-9_\-]+/ }, only: get :autocomplete_sources get :activity get :refs + put :new_issue_address end scope module: :projects do diff --git a/db/migrate/20160819232256_add_incoming_email_token_to_users.rb b/db/migrate/20160819232256_add_incoming_email_token_to_users.rb new file mode 100644 index 00000000000..f2cf956adc9 --- /dev/null +++ b/db/migrate/20160819232256_add_incoming_email_token_to_users.rb @@ -0,0 +1,16 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddIncomingEmailTokenToUsers < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + disable_ddl_transaction! + + def change + add_column :users, :incoming_email_token, :string + add_concurrent_index :users, :incoming_email_token + end +end diff --git a/db/schema.rb b/db/schema.rb index 60e7b26e29e..62c325a52d7 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1183,6 +1183,7 @@ ActiveRecord::Schema.define(version: 20161106185620) do t.boolean "ldap_email", default: false, null: false t.boolean "external", default: false t.string "organization" + t.string "incoming_email_token" end add_index "users", ["admin"], name: "index_users_on_admin", using: :btree @@ -1192,6 +1193,7 @@ ActiveRecord::Schema.define(version: 20161106185620) do add_index "users", ["current_sign_in_at"], name: "index_users_on_current_sign_in_at", using: :btree add_index "users", ["email"], name: "index_users_on_email", unique: true, using: :btree add_index "users", ["email"], name: "index_users_on_email_trigram", using: :gin, opclasses: {"email"=>"gin_trgm_ops"} + add_index "users", ["incoming_email_token"], name: "index_users_on_incoming_email_token", using: :btree add_index "users", ["name"], name: "index_users_on_name", using: :btree add_index "users", ["name"], name: "index_users_on_name_trigram", using: :gin, opclasses: {"name"=>"gin_trgm_ops"} add_index "users", ["reset_password_token"], name: "index_users_on_reset_password_token", unique: true, using: :btree diff --git a/features/profile/profile.feature b/features/profile/profile.feature index 447dd92a458..dc1339deb4c 100644 --- a/features/profile/profile.feature +++ b/features/profile/profile.feature @@ -59,11 +59,6 @@ Feature: Profile When I unsuccessfully change my password Then I should see a password error message - Scenario: I reset my token - Given I visit profile account page - Then I reset my token - And I should see new token - Scenario: I visit history tab Given I have activity When I visit Audit Log page diff --git a/features/steps/profile/profile.rb b/features/steps/profile/profile.rb index 05ab2a7dc73..ea480d2ad68 100644 --- a/features/steps/profile/profile.rb +++ b/features/steps/profile/profile.rb @@ -104,18 +104,6 @@ class Spinach::Features::Profile < Spinach::FeatureSteps end end - step 'I reset my token' do - page.within '.private-token' do - @old_token = @user.private_token - click_button "Reset private token" - end - end - - step 'I should see new token' do - expect(find("#token").value).not_to eq @old_token - expect(find("#token").value).to eq @user.reload.private_token - end - step 'I have activity' do create(:closed_issue_event, author: current_user) end diff --git a/lib/gitlab/email/handler.rb b/lib/gitlab/email/handler.rb index 5cf9d5ebe28..bd3267e2a80 100644 --- a/lib/gitlab/email/handler.rb +++ b/lib/gitlab/email/handler.rb @@ -4,8 +4,7 @@ require 'gitlab/email/handler/create_issue_handler' module Gitlab module Email module Handler - # The `CreateIssueHandler` feature is disabled for the time being. - HANDLERS = [CreateNoteHandler] + HANDLERS = [CreateNoteHandler, CreateIssueHandler] def self.for(mail, mail_key) HANDLERS.find do |klass| diff --git a/lib/gitlab/email/handler/create_issue_handler.rb b/lib/gitlab/email/handler/create_issue_handler.rb index 4e6566af8ab..9f90a3ec2b2 100644 --- a/lib/gitlab/email/handler/create_issue_handler.rb +++ b/lib/gitlab/email/handler/create_issue_handler.rb @@ -5,16 +5,16 @@ module Gitlab module Email module Handler class CreateIssueHandler < BaseHandler - attr_reader :project_path, :authentication_token + attr_reader :project_path, :incoming_email_token def initialize(mail, mail_key) super(mail, mail_key) - @project_path, @authentication_token = + @project_path, @incoming_email_token = mail_key && mail_key.split('+', 2) end def can_handle? - !authentication_token.nil? + !incoming_email_token.nil? end def execute @@ -29,7 +29,7 @@ module Gitlab end def author - @author ||= User.find_by(authentication_token: authentication_token) + @author ||= User.find_by(incoming_email_token: incoming_email_token) end def project diff --git a/lib/gitlab/incoming_email.rb b/lib/gitlab/incoming_email.rb index d7be50bd437..801dfde9a36 100644 --- a/lib/gitlab/incoming_email.rb +++ b/lib/gitlab/incoming_email.rb @@ -1,5 +1,7 @@ module Gitlab module IncomingEmail + WILDCARD_PLACEHOLDER = '%{key}'.freeze + class << self FALLBACK_MESSAGE_ID_REGEX = /\Areply\-(.+)@#{Gitlab.config.gitlab.host}\Z/.freeze @@ -7,8 +9,16 @@ module Gitlab config.enabled && config.address end + def supports_wildcard? + config.address && config.address.include?(WILDCARD_PLACEHOLDER) + end + + def supports_issue_creation? + enabled? && supports_wildcard? + end + def reply_address(key) - config.address.gsub('%{key}', key) + config.address.gsub(WILDCARD_PLACEHOLDER, key) end def key_from_address(address) diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb index a302d1c57f4..5ddcaa60dc6 100644 --- a/spec/controllers/projects_controller_spec.rb +++ b/spec/controllers/projects_controller_spec.rb @@ -284,6 +284,33 @@ describe ProjectsController do end end + describe 'PUT #new_issue_address' do + subject do + put :new_issue_address, + namespace_id: project.namespace.to_param, + id: project.to_param + user.reload + end + + before do + sign_in(user) + project.team << [user, :developer] + allow(Gitlab.config.incoming_email).to receive(:enabled).and_return(true) + end + + it 'has http status 200' do + expect(response).to have_http_status(200) + end + + it 'changes the user incoming email token' do + expect { subject }.to change { user.incoming_email_token } + end + + it 'changes projects new issue address' do + expect { subject }.to change { project.new_issue_address(user) } + end + end + describe "POST #toggle_star" do it "toggles star if user is signed in" do sign_in(user) diff --git a/spec/features/issues_spec.rb b/spec/features/issues_spec.rb index b504329656f..cdd02a8c8e3 100644 --- a/spec/features/issues_spec.rb +++ b/spec/features/issues_spec.rb @@ -3,6 +3,7 @@ require 'spec_helper' describe 'Issues', feature: true do include IssueHelpers include SortingHelper + include WaitForAjax let(:project) { create(:project) } @@ -368,6 +369,26 @@ describe 'Issues', feature: true do end end + describe 'when I want to reset my incoming email token' do + let(:project1) { create(:project, namespace: @user.namespace) } + + before do + allow(Gitlab.config.incoming_email).to receive(:enabled).and_return(true) + project1.team << [@user, :master] + visit namespace_project_issues_path(@user.namespace, project1) + end + + it 'changes incoming email address token', js: true do + find('.issue-email-modal-btn').click + previous_token = find('input#issue_email').value + + find('.incoming-email-token-reset').click + wait_for_ajax + + expect(find('input#issue_email').value).not_to eq(previous_token) + end + end + describe 'update labels from issue#show', js: true do let(:issue) { create(:issue, project: project, author: @user, assignee: @user) } let!(:label) { create(:label, project: project) } @@ -553,7 +574,7 @@ describe 'Issues', feature: true do end end - xdescribe 'new issue by email' do + describe 'new issue by email' do shared_examples 'show the email in the modal' do before do stub_incoming_email_setting(enabled: true, address: "p+%{key}@gl.ab") diff --git a/spec/features/profile_spec.rb b/spec/features/profile_spec.rb index c3d8c349ca4..7a562b5e03d 100644 --- a/spec/features/profile_spec.rb +++ b/spec/features/profile_spec.rb @@ -32,4 +32,33 @@ describe 'Profile account page', feature: true do expect(current_path).to eq(profile_account_path) end end + + describe 'when I reset private token' do + before do + visit profile_account_path + end + + it 'resets private token' do + previous_token = find("#private-token").value + + click_link('Reset private token') + + expect(find('#private-token').value).not_to eq(previous_token) + end + end + + describe 'when I reset incoming email token' do + before do + allow(Gitlab.config.incoming_email).to receive(:enabled).and_return(true) + visit profile_account_path + end + + it 'resets incoming email token' do + previous_token = find('#incoming-email-token').value + + click_link('Reset incoming email token') + + expect(find('#incoming-email-token').value).not_to eq(previous_token) + end + end end diff --git a/spec/fixtures/emails/wrong_authentication_token.eml b/spec/fixtures/emails/wrong_incoming_email_token.eml index 0994c2f7775..0994c2f7775 100644 --- a/spec/fixtures/emails/wrong_authentication_token.eml +++ b/spec/fixtures/emails/wrong_incoming_email_token.eml diff --git a/spec/lib/gitlab/email/handler/create_issue_handler_spec.rb b/spec/lib/gitlab/email/handler/create_issue_handler_spec.rb index a5cc7b02936..cb3651e3845 100644 --- a/spec/lib/gitlab/email/handler/create_issue_handler_spec.rb +++ b/spec/lib/gitlab/email/handler/create_issue_handler_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' require_relative '../email_shared_blocks' -xdescribe Gitlab::Email::Handler::CreateIssueHandler, lib: true do +describe Gitlab::Email::Handler::CreateIssueHandler, lib: true do include_context :email_shared_context it_behaves_like :email_shared_examples @@ -18,7 +18,7 @@ xdescribe Gitlab::Email::Handler::CreateIssueHandler, lib: true do create( :user, email: 'jake@adventuretime.ooo', - authentication_token: 'auth_token' + incoming_email_token: 'auth_token' ) end @@ -60,8 +60,8 @@ xdescribe Gitlab::Email::Handler::CreateIssueHandler, lib: true do end end - context "when we can't find the authentication_token" do - let(:email_raw) { fixture_file("emails/wrong_authentication_token.eml") } + context "when we can't find the incoming_email_token" do + let(:email_raw) { fixture_file("emails/wrong_incoming_email_token.eml") } it "raises an UserNotFoundError" do expect { receiver.execute }.to raise_error(Gitlab::Email::UserNotFoundError) diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 0245897938c..0810d06b50f 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -295,7 +295,7 @@ describe Project, models: true do end end - xdescribe "#new_issue_address" do + describe "#new_issue_address" do let(:project) { create(:empty_project, path: "somewhere") } let(:user) { create(:user) } @@ -305,8 +305,7 @@ describe Project, models: true do end it 'returns the address to create a new issue' do - token = user.authentication_token - address = "p+#{project.namespace.path}/#{project.path}+#{token}@gl.ab" + address = "p+#{project.path_with_namespace}+#{user.incoming_email_token}@gl.ab" expect(project.new_issue_address(user)).to eq(address) end |