Commit 74725da1 authored by Douwe Maan's avatar Douwe Maan

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
parents 9346a8d9 ce12749a
......@@ -70,6 +70,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)
......
......@@ -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
......
......@@ -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
......
......@@ -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.
......
- 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'
......@@ -2,25 +2,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(: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
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 'when the user is logged in' do
before { sign_in(user) }
expect(response.status).to be 404
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
context 'calling with id' do
it 'shows a flash message to the user' do
get(:unsubscribe, id: sent_notification.reply_key)
it 'returns a 404' do
expect(response).to have_http_status(:not_found)
end
end
expect(response.status).to be 302
context 'when the force param is passed' do
before { get(:unsubscribe, id: sent_notification.reply_key, force: true) }
expect(response).to redirect_to new_user_session_path
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
it 'sets the flash message' do
expect(controller).to set_flash[:notice].to(/unsubscribed/).now
end
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
end
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
......@@ -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'
......
......@@ -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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment