Commit 57fef8b2 authored by Douwe Maan's avatar Douwe Maan Committed by Ruben Davila

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
parent 84bc2627
...@@ -66,6 +66,7 @@ v 8.12.0 (unreleased) ...@@ -66,6 +66,7 @@ v 8.12.0 (unreleased)
- Test migration paths from 8.5 until current release !4874 - Test migration paths from 8.5 until current release !4874
- Replace animateEmoji timeout with eventListener (ClemMakesApps) - Replace animateEmoji timeout with eventListener (ClemMakesApps)
- Optimistic locking for Issues and Merge Requests (title and description overriding prevention) - 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) - Add `wiki_page_events` to project hook APIs (Ben Boeckel)
- Remove Gitorious import - Remove Gitorious import
- Fix inconsistent background color for filter input field (ClemMakesApps) - Fix inconsistent background color for filter input field (ClemMakesApps)
......
...@@ -3,12 +3,19 @@ class SentNotificationsController < ApplicationController ...@@ -3,12 +3,19 @@ class SentNotificationsController < ApplicationController
def unsubscribe def unsubscribe
@sent_notification = SentNotification.for(params[:id]) @sent_notification = SentNotification.for(params[:id])
return render_404 unless @sent_notification && @sent_notification.unsubscribable? 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 = @sent_notification.noteable
noteable.unsubscribe(@sent_notification.recipient) noteable.unsubscribe(@sent_notification.recipient)
flash[:notice] = "You have been unsubscribed from this thread." flash[:notice] = "You have been unsubscribed from this thread."
if current_user if current_user
case noteable case noteable
when Issue when Issue
......
...@@ -108,6 +108,12 @@ class Notify < BaseMailer ...@@ -108,6 +108,12 @@ class Notify < BaseMailer
headers["X-GitLab-#{model.class.name}-ID"] = model.id headers["X-GitLab-#{model.class.name}-ID"] = model.id
headers['X-GitLab-Reply-Key'] = reply_key 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? if Gitlab::IncomingEmail.enabled?
address = Mail::Address.new(Gitlab::IncomingEmail.reply_address(reply_key)) address = Mail::Address.new(Gitlab::IncomingEmail.reply_address(reply_key))
address.display_name = @project.name_with_namespace address.display_name = @project.name_with_namespace
......
...@@ -25,8 +25,8 @@ ...@@ -25,8 +25,8 @@
- if @labels_url - if @labels_url
adjust your #{link_to 'label subscriptions', @labels_url}. adjust your #{link_to 'label subscriptions', @labels_url}.
- else - else
- if @sent_notification && @sent_notification.unsubscribable? - if @sent_notification_url
= link_to "unsubscribe", unsubscribe_sent_notification_url(@sent_notification) = link_to "unsubscribe", @sent_notification_url
from this thread or from this thread or
adjust your notification settings. 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' ...@@ -2,25 +2,108 @@ require 'rails_helper'
describe SentNotificationsController, type: :controller do describe SentNotificationsController, type: :controller do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:issue) { create(:issue, author: user) } let(:project) { create(:empty_project) }
let(:sent_notification) { create(:sent_notification, noteable: issue) } let(:sent_notification) { create(:sent_notification, noteable: issue, recipient: user) }
describe 'GET #unsubscribe' do let(:issue) do
it 'returns a 404 when calling without existing id' do create(:issue, project: project, author: user) do |issue|
get(:unsubscribe, id: '0' * 32) 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 end
context 'calling with id' do it 'returns a 404' do
it 'shows a flash message to the user' do expect(response).to have_http_status(:not_found)
get(:unsubscribe, id: sent_notification.reply_key) 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 expect(controller).to set_flash[:notice].to(/unsubscribed/).now
end 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 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 ...@@ -861,7 +861,7 @@ describe Notify do
subject { Notify.repository_push_email(project.id, author_id: user.id, ref: 'refs/heads/master', action: :create) } 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 '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 with X-GitLab headers containing project details'
it_behaves_like 'an email that contains a header with author username' it_behaves_like 'an email that contains a header with author username'
...@@ -914,7 +914,7 @@ describe Notify do ...@@ -914,7 +914,7 @@ describe Notify do
subject { Notify.repository_push_email(project.id, author_id: user.id, ref: 'refs/heads/master', action: :delete) } 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 '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 with X-GitLab headers containing project details'
it_behaves_like 'an email that contains a header with author username' it_behaves_like 'an email that contains a header with author username'
...@@ -936,7 +936,7 @@ describe Notify do ...@@ -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) } 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 '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 with X-GitLab headers containing project details'
it_behaves_like 'an email that contains a header with author username' it_behaves_like 'an email that contains a header with author username'
...@@ -964,7 +964,7 @@ describe Notify do ...@@ -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) } 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 '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 with X-GitLab headers containing project details'
it_behaves_like 'an email that contains a header with author username' it_behaves_like 'an email that contains a header with author username'
...@@ -1066,7 +1066,7 @@ describe Notify do ...@@ -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) } 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 '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 with X-GitLab headers containing project details'
it_behaves_like 'an email that contains a header with author username' 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 ...@@ -169,10 +169,18 @@ shared_examples 'it should show Gmail Actions View Commit link' do
end end
shared_examples 'an unsubscribeable thread' do 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/ } it { is_expected.to have_body_text /unsubscribe/ }
end end
shared_examples 'a user cannot unsubscribe through footer link' do 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/ } it { is_expected.not_to have_body_text /unsubscribe/ }
end 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