Commit b3357308 authored by Maximiliano Perez Coto's avatar Maximiliano Perez Coto Committed by Rémy Coutable

Fix "Unsubscribe" link in notification emails that is triggered by anti-virus

* Created a force=true param that will continue with the previous
  behaviour of the unsubscribe method
* Created a filter for not-logged users so they see a unsubsribe
  confirmation page
* Added the List-Unsubscribe header on emails so the email client can
  display it on top
parent 95b9421a
......@@ -69,6 +69,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)
......
......@@ -2,13 +2,17 @@ class SentNotificationsController < ApplicationController
skip_before_action :authenticate_user!
def unsubscribe
return redirect_to new_user_session_path unless current_user || params[:force]
@sent_notification = SentNotification.for(params[:id])
return render_404 unless @sent_notification && @sent_notification.unsubscribable?
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.
......
%h3.page-title
Are you sure you want to unsubscribe from this thread?
= link_to "Unsubscribe", unsubscribe_sent_notification_path(@sent_notification, force: true), class: 'btn btn-primary wide'
......@@ -2,25 +2,102 @@ 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 redirect_to(new_user_session_path)
end
end
end
expect(response.status).to be 404
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
context 'calling with id' do
it 'shows a flash message to the user' do
get(:unsubscribe, id: sent_notification.reply_key)
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
expect(response.status).to be 302
context 'when the force param is not passed' do
before { get(:unsubscribe, id: sent_notification.reply_key) }
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
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
it 'redirects to the login page when visiting the link from the body' do
visit body_link
expect(current_path).to eq new_user_session_path
expect(issue.subscribed?(recipient)).to be_truthy
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