diff options
author | Douwe Maan <douwe@gitlab.com> | 2016-09-20 11:25:55 +0000 |
---|---|---|
committer | Ruben Davila <rdavila84@gmail.com> | 2016-09-20 11:06:35 -0500 |
commit | 57fef8b2a851597ed2a23cf3586aa81ee737d58f (patch) | |
tree | 870c5deb91da44502da55a6ed515f4811a028a1a | |
parent | 84bc26277eda7c22c4c4084fe2a9a986054985f7 (diff) | |
download | gitlab-ce-57fef8b2a851597ed2a23cf3586aa81ee737d58f.tar.gz |
Merge branch 'maxiperezc/gitlab-ce-issues_17198' into 'master'
Fix "Unsubscribe" link in notification emails that is triggered by anti-virus
## What does this MR do?
* The unsubscribe link in an email body only unsubscribes automatically when logged in, otherwise the user is asked for a confirmation.
* The unsubscribe link in an email header unsubscribes automatically whether logged in or not.
## Are there points in the code the reviewer needs to double check?
This addresses all the comments from https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5241, I think!
## Why was this MR needed?
People were getting unsubscribed automatically by AV software.
## Screenshot
![Screen_Shot_2016-09-20_at_09.51.30](/uploads/083ee2865f1ad6c08e2ed97f1c4e7d0d/Screen_Shot_2016-09-20_at_09.51.30.png)
## Does this MR meet the acceptance criteria?
- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- Tests
- [x] Added for this feature/bug
- [ ] All builds are passing
- [x] Conform by the [merge request performance guides](http://docs.gitlab.com/ce/development/merge_request_performance_guidelines.html)
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)
## What are the relevant issue numbers?
Fixes #17198.
See merge request !6223
-rw-r--r-- | CHANGELOG | 1 | ||||
-rw-r--r-- | app/controllers/sent_notifications_controller.rb | 7 | ||||
-rw-r--r-- | app/mailers/notify.rb | 6 | ||||
-rw-r--r-- | app/views/layouts/notify.html.haml | 4 | ||||
-rw-r--r-- | app/views/sent_notifications/unsubscribe.html.haml | 19 | ||||
-rw-r--r-- | spec/controllers/sent_notifications_controller_spec.rb | 109 | ||||
-rw-r--r-- | spec/features/unsubscribe_links_spec.rb | 75 | ||||
-rw-r--r-- | spec/mailers/notify_spec.rb | 10 | ||||
-rw-r--r-- | spec/mailers/shared/notify.rb | 8 |
9 files changed, 219 insertions, 20 deletions
diff --git a/CHANGELOG b/CHANGELOG index dd6920e7abc..aae10cb0d10 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -66,6 +66,7 @@ v 8.12.0 (unreleased) - Test migration paths from 8.5 until current release !4874 - Replace animateEmoji timeout with eventListener (ClemMakesApps) - Optimistic locking for Issues and Merge Requests (title and description overriding prevention) + - Require confirmation when not logged in for unsubscribe links !6223 (Maximiliano Perez Coto) - Add `wiki_page_events` to project hook APIs (Ben Boeckel) - Remove Gitorious import - Fix inconsistent background color for filter input field (ClemMakesApps) diff --git a/app/controllers/sent_notifications_controller.rb b/app/controllers/sent_notifications_controller.rb index 7271c933b9b..3085ff33aba 100644 --- a/app/controllers/sent_notifications_controller.rb +++ b/app/controllers/sent_notifications_controller.rb @@ -3,12 +3,19 @@ class SentNotificationsController < ApplicationController def unsubscribe @sent_notification = SentNotification.for(params[:id]) + return render_404 unless @sent_notification && @sent_notification.unsubscribable? + return unsubscribe_and_redirect if current_user || params[:force] + end + private + + def unsubscribe_and_redirect noteable = @sent_notification.noteable noteable.unsubscribe(@sent_notification.recipient) flash[:notice] = "You have been unsubscribed from this thread." + if current_user case noteable when Issue diff --git a/app/mailers/notify.rb b/app/mailers/notify.rb index 0cc709f68e4..9799f1dc886 100644 --- a/app/mailers/notify.rb +++ b/app/mailers/notify.rb @@ -108,6 +108,12 @@ class Notify < BaseMailer headers["X-GitLab-#{model.class.name}-ID"] = model.id headers['X-GitLab-Reply-Key'] = reply_key + if !@labels_url && @sent_notification && @sent_notification.unsubscribable? + headers['List-Unsubscribe'] = unsubscribe_sent_notification_url(@sent_notification, force: true) + + @sent_notification_url = unsubscribe_sent_notification_url(@sent_notification) + end + if Gitlab::IncomingEmail.enabled? address = Mail::Address.new(Gitlab::IncomingEmail.reply_address(reply_key)) address.display_name = @project.name_with_namespace diff --git a/app/views/layouts/notify.html.haml b/app/views/layouts/notify.html.haml index dde2e2889dc..1ec4c3f0c67 100644 --- a/app/views/layouts/notify.html.haml +++ b/app/views/layouts/notify.html.haml @@ -25,8 +25,8 @@ - if @labels_url adjust your #{link_to 'label subscriptions', @labels_url}. - else - - if @sent_notification && @sent_notification.unsubscribable? - = link_to "unsubscribe", unsubscribe_sent_notification_url(@sent_notification) + - if @sent_notification_url + = link_to "unsubscribe", @sent_notification_url from this thread or adjust your notification settings. diff --git a/app/views/sent_notifications/unsubscribe.html.haml b/app/views/sent_notifications/unsubscribe.html.haml new file mode 100644 index 00000000000..9ce6a1aeef5 --- /dev/null +++ b/app/views/sent_notifications/unsubscribe.html.haml @@ -0,0 +1,19 @@ +- noteable = @sent_notification.noteable +- noteable_type = @sent_notification.noteable_type.humanize(capitalize: false) +- noteable_text = %(#{noteable.title} (#{noteable.to_reference})) + +- page_title "Unsubscribe", noteable_text, @sent_notification.noteable_type.humanize.pluralize, @sent_notification.project.name_with_namespace + + +%h3.page-title + Unsubscribe from #{noteable_type} #{noteable_text} + +%p + = succeed '?' do + Are you sure you want to unsubscribe from #{noteable_type} + = link_to noteable_text, url_for([@sent_notification.project.namespace.becomes(Namespace), @sent_notification.project, noteable]) + +%p + = link_to 'Unsubscribe', unsubscribe_sent_notification_path(@sent_notification, force: true), + class: 'btn btn-primary append-right-10' + = link_to 'Cancel', new_user_session_path, class: 'btn append-right-10' diff --git a/spec/controllers/sent_notifications_controller_spec.rb b/spec/controllers/sent_notifications_controller_spec.rb index 9ced397bd4a..191e290a118 100644 --- a/spec/controllers/sent_notifications_controller_spec.rb +++ b/spec/controllers/sent_notifications_controller_spec.rb @@ -1,25 +1,108 @@ require 'rails_helper' describe SentNotificationsController, type: :controller do - let(:user) { create(:user) } - let(:issue) { create(:issue, author: user) } - let(:sent_notification) { create(:sent_notification, noteable: issue) } + let(:user) { create(:user) } + let(:project) { create(:empty_project) } + let(:sent_notification) { create(:sent_notification, noteable: issue, recipient: user) } - describe 'GET #unsubscribe' do - it 'returns a 404 when calling without existing id' do - get(:unsubscribe, id: '0' * 32) + let(:issue) do + create(:issue, project: project, author: user) do |issue| + issue.subscriptions.create(user: user, subscribed: true) + end + end + + describe 'GET unsubscribe' do + context 'when the user is not logged in' do + context 'when the force param is passed' do + before { get(:unsubscribe, id: sent_notification.reply_key, force: true) } + + it 'unsubscribes the user' do + expect(issue.subscribed?(user)).to be_falsey + end + + it 'sets the flash message' do + expect(controller).to set_flash[:notice].to(/unsubscribed/).now + end + + it 'redirects to the login page' do + expect(response).to redirect_to(new_user_session_path) + end + end + + context 'when the force param is not passed' do + before { get(:unsubscribe, id: sent_notification.reply_key) } + + it 'does not unsubscribe the user' do + expect(issue.subscribed?(user)).to be_truthy + end - expect(response.status).to be 404 + it 'does not set the flash message' do + expect(controller).not_to set_flash[:notice] + end + + it 'redirects to the login page' do + expect(response).to render_template :unsubscribe + end + end end - context 'calling with id' do - it 'shows a flash message to the user' do - get(:unsubscribe, id: sent_notification.reply_key) + context 'when the user is logged in' do + before { sign_in(user) } + + context 'when the ID passed does not exist' do + before { get(:unsubscribe, id: sent_notification.reply_key.reverse) } + + it 'does not unsubscribe the user' do + expect(issue.subscribed?(user)).to be_truthy + end + + it 'does not set the flash message' do + expect(controller).not_to set_flash[:notice] + end + + it 'returns a 404' do + expect(response).to have_http_status(:not_found) + end + end + + context 'when the force param is passed' do + before { get(:unsubscribe, id: sent_notification.reply_key, force: true) } + + it 'unsubscribes the user' do + expect(issue.subscribed?(user)).to be_falsey + end + + it 'sets the flash message' do + expect(controller).to set_flash[:notice].to(/unsubscribed/).now + end + + it 'redirects to the issue page' do + expect(response). + to redirect_to(namespace_project_issue_path(project.namespace, project, issue)) + end + end + + context 'when the force param is not passed' do + let(:merge_request) do + create(:merge_request, source_project: project, author: user) do |merge_request| + merge_request.subscriptions.create(user: user, subscribed: true) + end + end + let(:sent_notification) { create(:sent_notification, noteable: merge_request, recipient: user) } + before { get(:unsubscribe, id: sent_notification.reply_key) } + + it 'unsubscribes the user' do + expect(merge_request.subscribed?(user)).to be_falsey + end - expect(response.status).to be 302 + it 'sets the flash message' do + expect(controller).to set_flash[:notice].to(/unsubscribed/).now + end - expect(response).to redirect_to new_user_session_path - expect(controller).to set_flash[:notice].to(/unsubscribed/).now + it 'redirects to the merge request page' do + expect(response). + to redirect_to(namespace_project_merge_request_path(project.namespace, project, merge_request)) + end end end end diff --git a/spec/features/unsubscribe_links_spec.rb b/spec/features/unsubscribe_links_spec.rb new file mode 100644 index 00000000000..cc40671787c --- /dev/null +++ b/spec/features/unsubscribe_links_spec.rb @@ -0,0 +1,75 @@ +require 'spec_helper' + +describe 'Unsubscribe links', feature: true do + include Warden::Test::Helpers + + let(:recipient) { create(:user) } + let(:author) { create(:user) } + let(:project) { create(:empty_project, :public) } + let(:params) { { title: 'A bug!', description: 'Fix it!', assignee: recipient } } + let(:issue) { Issues::CreateService.new(project, author, params).execute } + + let(:mail) { ActionMailer::Base.deliveries.last } + let(:body) { Capybara::Node::Simple.new(mail.default_part_body.to_s) } + let(:header_link) { mail.header['List-Unsubscribe'] } + let(:body_link) { body.find_link('unsubscribe')['href'] } + + before do + perform_enqueued_jobs { issue } + end + + context 'when logged out' do + context 'when visiting the link from the body' do + it 'shows the unsubscribe confirmation page and redirects to root path when confirming' do + visit body_link + + expect(current_path).to eq unsubscribe_sent_notification_path(SentNotification.last) + expect(page).to have_text(%(Unsubscribe from issue #{issue.title} (#{issue.to_reference}))) + expect(page).to have_text(%(Are you sure you want to unsubscribe from issue #{issue.title} (#{issue.to_reference})?)) + expect(issue.subscribed?(recipient)).to be_truthy + + click_link 'Unsubscribe' + + expect(issue.subscribed?(recipient)).to be_falsey + expect(current_path).to eq new_user_session_path + end + + it 'shows the unsubscribe confirmation page and redirects to root path when canceling' do + visit body_link + + expect(current_path).to eq unsubscribe_sent_notification_path(SentNotification.last) + expect(issue.subscribed?(recipient)).to be_truthy + + click_link 'Cancel' + + expect(issue.subscribed?(recipient)).to be_truthy + expect(current_path).to eq new_user_session_path + end + end + + it 'unsubscribes from the issue when visiting the link from the header' do + visit header_link + + expect(page).to have_text('unsubscribed') + expect(issue.subscribed?(recipient)).to be_falsey + end + end + + context 'when logged in' do + before { login_as(recipient) } + + it 'unsubscribes from the issue when visiting the link from the email body' do + visit body_link + + expect(page).to have_text('unsubscribed') + expect(issue.subscribed?(recipient)).to be_falsey + end + + it 'unsubscribes from the issue when visiting the link from the header' do + visit header_link + + expect(page).to have_text('unsubscribed') + expect(issue.subscribed?(recipient)).to be_falsey + end + end +end diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index eae9c060c38..0363bc74939 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -861,7 +861,7 @@ describe Notify do subject { Notify.repository_push_email(project.id, author_id: user.id, ref: 'refs/heads/master', action: :create) } it_behaves_like 'it should not have Gmail Actions links' - it_behaves_like "a user cannot unsubscribe through footer link" + it_behaves_like 'a user cannot unsubscribe through footer link' it_behaves_like 'an email with X-GitLab headers containing project details' it_behaves_like 'an email that contains a header with author username' @@ -914,7 +914,7 @@ describe Notify do subject { Notify.repository_push_email(project.id, author_id: user.id, ref: 'refs/heads/master', action: :delete) } it_behaves_like 'it should not have Gmail Actions links' - it_behaves_like "a user cannot unsubscribe through footer link" + it_behaves_like 'a user cannot unsubscribe through footer link' it_behaves_like 'an email with X-GitLab headers containing project details' it_behaves_like 'an email that contains a header with author username' @@ -936,7 +936,7 @@ describe Notify do subject { Notify.repository_push_email(project.id, author_id: user.id, ref: 'refs/tags/v1.0', action: :delete) } it_behaves_like 'it should not have Gmail Actions links' - it_behaves_like "a user cannot unsubscribe through footer link" + it_behaves_like 'a user cannot unsubscribe through footer link' it_behaves_like 'an email with X-GitLab headers containing project details' it_behaves_like 'an email that contains a header with author username' @@ -964,7 +964,7 @@ describe Notify do subject { Notify.repository_push_email(project.id, author_id: user.id, ref: 'refs/heads/master', action: :push, compare: compare, reverse_compare: false, diff_refs: diff_refs, send_from_committer_email: send_from_committer_email) } it_behaves_like 'it should not have Gmail Actions links' - it_behaves_like "a user cannot unsubscribe through footer link" + it_behaves_like 'a user cannot unsubscribe through footer link' it_behaves_like 'an email with X-GitLab headers containing project details' it_behaves_like 'an email that contains a header with author username' @@ -1066,7 +1066,7 @@ describe Notify do subject { Notify.repository_push_email(project.id, author_id: user.id, ref: 'refs/heads/master', action: :push, compare: compare, diff_refs: diff_refs) } it_behaves_like 'it should show Gmail Actions View Commit link' - it_behaves_like "a user cannot unsubscribe through footer link" + it_behaves_like 'a user cannot unsubscribe through footer link' it_behaves_like 'an email with X-GitLab headers containing project details' it_behaves_like 'an email that contains a header with author username' diff --git a/spec/mailers/shared/notify.rb b/spec/mailers/shared/notify.rb index 93de5850ba2..56872da9a8f 100644 --- a/spec/mailers/shared/notify.rb +++ b/spec/mailers/shared/notify.rb @@ -169,10 +169,18 @@ shared_examples 'it should show Gmail Actions View Commit link' do end shared_examples 'an unsubscribeable thread' do + it 'has a List-Unsubscribe header' do + is_expected.to have_header 'List-Unsubscribe', /unsubscribe/ + end + it { is_expected.to have_body_text /unsubscribe/ } end shared_examples 'a user cannot unsubscribe through footer link' do + it 'does not have a List-Unsubscribe header' do + is_expected.not_to have_header 'List-Unsubscribe', /unsubscribe/ + end + it { is_expected.not_to have_body_text /unsubscribe/ } end |